[RHCLOUD-35855] System roles assigments in migrator #1288

lpichler · 2024-11-05T16:06:14Z

This generates and replicates relations for system role assignments of custom default groups and custom groups.
Also:

guard condition to allow migration of when read only mode is enabled
logic for migration user-group assignments was replaced by logic in dual write handler.

Links

alechenninger · 2024-11-05T19:32:17Z

rbac/management/relation_replicator/relation_replicator.py

@@ -52,6 +52,7 @@ class ReplicationEventType(str, Enum):
    MIGRATE_CUSTOM_ROLE = "migrate_custom_role"
    MIGRATE_TENANT_GROUPS = "migrate_tenant_groups"
    CUSTOMIZE_DEFAULT_GROUP = "customize_default_group"
+    ASSIGN_SYSTEM_ROLE_IN_MIGRATOR = "assign_system_role_in_migrator"


maybe migrate_system_role_assignment (so all migrate_ events have a common prefix / convention?)

alechenninger · 2024-11-05T19:53:01Z

rbac/migration_tool/migrate.py

+                group, ReplicationEventType.ASSIGN_SYSTEM_ROLE_IN_MIGRATOR
+            )
+            if group.platform_default is True:
+                dual_write_handler.generate_relations_to_add_roles(public_default_roles)


I don't think we can add all default roles, because they may have removed some of these.

But, if you just go by the policy like you do below, I think you should be okay. So I think you can just take this part out.

alechenninger · 2024-11-05T19:54:27Z

rbac/migration_tool/migrate.py

+    public_default_roles = Role.objects.filter(platform_default=True, tenant=Tenant.objects.get(tenant_name="public"))
+
+    with transaction.atomic():
+        for group in tenant.group_set.all():


I wonder if we can combine this with migrate_users_for_groups in some way, which already iterates through (almost) all tenant groups.

alechenninger · 2024-11-05T19:55:57Z

rbac/migration_tool/migrate.py

@@ -152,6 +154,19 @@ def migrate_data_for_tenant(tenant: Tenant, exclude_apps: list, replicator: Rela
        logger.info(f"Migration completed for role: {role.name} with UUID {role.uuid}.")
    logger.info(f"Migrated {roles.count()} roles for tenant: {tenant.org_id}")

+    public_default_roles = Role.objects.filter(platform_default=True, tenant=Tenant.objects.get(tenant_name="public"))
+
+    with transaction.atomic():


It doesn't hurt to be all in one tx, but nothing else in the migrator is currently dealing with concurrency. It is assumed that it runs without other concurrent writes.

You mentioned the issue about the middleware possibly doing concurrent writes which I'm not sure if you've looked into more yet but deserves some thought.

alechenninger · 2024-11-07T15:48:47Z

rbac/migration_tool/migrate.py

-    """Write users relationship to groups."""
+def migrate_system_role_assignments_for_groups(groups: list[Group]) -> list[common_pb2.Relationship]:
+    """Generate system role assignments for groups."""
+    with transaction.atomic():


Is this required because of the select for update?

If not, I would probably take it out because it might imply it's needed or effective in some way that it isn't. Migration does NOT account for concurrent updates, so I am worried about implying that it does.

If it's required, maybe just add a comment to that effect (required due to select for update even though migrator does not generally account for concurrent updates and so should NOT be run outside of read-only mode.)

yes it is and I wanted to ask what scope for transaction block should be in this case and I will add comment.

It shouldn't really matter I guess since we shouldn't do this with concurrent writes. Not too big though, so a TX per group or per tenant is probably okay. Per group might be better but probably would require testing to confirm. (Normally there is a TX per every single SQL operation so per group is not too many.)

We also may want to add a check in the beginning of the migrator that requires read only mode is set otherwise it doesn't run as a safety mechanism.

alechenninger · 2024-11-07T15:52:28Z

rbac/migration_tool/migrate.py

+            if dual_write_handler is None:
+                dual_write_handler = RelationApiDualWriteGroupHandler(group, None)
+            else:
+                dual_write_handler.group = group
+            system_roles = group.roles().filter(system=True)
+            if system_roles.exists() > 0:
+                dual_write_handler.generate_relations_to_add_roles(system_roles)
+                relationships.extend(dual_write_handler.group_relations_to_add)
+    return relationships


IMO I think it's cleaner & less risky to let the dual write handler replicate these each on its own, and reinitialize a new replicator for each loop. There's no requirement that these need to all be in their own message. In fact, being in their own message can be problematic because of message and SpiceDB request size limits.

If the class is not carefully designed for mutation, this could result in bugs. So it's kind of maybe optimizing something that doesn't need to be, and adding some risks. What do you think?

yes I know that dual write handler is not designed for multiple groups and I wanted to avoid to do same query for default workspace for each group. Do you think it is fine with this fact ?

I think this is fine, we can cache default workspaces in follow up PR.

alechenninger · 2024-11-07T16:03:08Z

rbac/migration_tool/migrate.py

+    user_relationships = migrate_user_relationships_for_groups(groups)
    replicator.replicate(
        ReplicationEvent(
            event_type=ReplicationEventType.MIGRATE_TENANT_GROUPS,
            info={"tenant": tenant.org_id},
            partition_key=PartitionKey.byEnvironment(),
-            add=tuples,
+            add=user_relationships,
+        )
+    )
+
+    system_role_relationships = migrate_system_role_assignments_for_groups(groups)


What if we combined principals and roles together by using the group dual write handler?

Something like (psuedo-code):

for group in groups: handler = RelationApiDualWriteGropuHandler(group, MIGRATE_GROUP) handler.generate_relations_to_add_principals(group.principals) handler.generate_relations_to_add_roles(group.system_roles()) handler.replicate()

It might require a small change to dealing with principals but it would consolidate the logic for group membership replication and clean up the code a bit.

Not required by any means I think as long as testing is good either way. What do you think?

sure, it is simpler, done 👍

lpichler · 2024-11-07T18:21:56Z

@alechenninger I addressed you suggestions, can you re-review ? 🙏

rbac/migration_tool/migrate.py

alechenninger · 2024-11-07T20:03:23Z

tests/migration_tool/tests_migrate.py

@@ -31,7 +36,7 @@ def setUp(self):
        """Set up the utils tests."""
        super().setUp()
        public_tenant = Tenant.objects.get(tenant_name="public")
-        Group.objects.create(name="default", tenant=public_tenant, platform_default=True)
+        default_group = Group.objects.create(name="default", tenant=public_tenant, platform_default=True, system=True)


I wonder if we can use seed_group() instead in the setup (probably after the roles are created then)? This would ensure test setup stays consistent with the app and maybe reduce some of this code.

I added seed_group and clone_default_group_in_public_schema

it is possible use seed_group(), function creates also admin default group

alechenninger · 2024-11-07T21:49:37Z

rbac/migration_tool/migrate.py

+                system_roles = group.roles().filter(system=True)
+                if system_roles.exists() > 0:
+                    dual_write_handler.generate_relations_to_add_roles(system_roles)
+            dual_write_handler.replicate()


We can address this in a follow-up PR if you would like, but along the lines of RHCLOUD-36014, I wonder if we should avoid calling this if we know there were no system_roles added and no principals added?

We could add that logic inside replicate(), but then it makes you wonder if we should really warn there? And in that case it's redundant because the outbox replicator already warns. So it feels like we should avoid calling replicate if we know there's nothing to replicate, otherwise the replicate method will be "suspicious" in some way :)

yes, .replicate() should not be called when it is possible to determine that it is not needed otherwise we cannot capture unintended "empty" replication.

I made changes here. Existence checks are moved outside the transaction block to avoid initializing the dual write handler when it is not needed. These checks are also not needed before the generate_relations_XXX method, as it they don't perform any work on empty arrays (system_role, principals)."

alechenninger

LGTM!

I have another branch where I've added another migrator test which is a little more exacting to be sure, but I'll open a PR for that separately. Thank you and nice work!

Co-authored-by: Alec Henninger <[email protected]>

… seed_group methods

lpichler force-pushed the system_roles_assigments_in_migrator branch from 0698d46 to 8171f76 Compare November 5, 2024 16:06

lpichler changed the base branch from master to update_custom_default_group_logic November 5, 2024 16:08

lpichler changed the title ~~[WIP] System roles assigments in migratorr~~ [WIP] System roles assigments in migrator Nov 5, 2024

lpichler force-pushed the system_roles_assigments_in_migrator branch 2 times, most recently from 188e976 to df77675 Compare November 5, 2024 16:16

alechenninger reviewed Nov 5, 2024

View reviewed changes

Base automatically changed from update_custom_default_group_logic to master November 6, 2024 09:06

lpichler force-pushed the system_roles_assigments_in_migrator branch 5 times, most recently from 4060072 to b018976 Compare November 7, 2024 12:58

alechenninger reviewed Nov 7, 2024

View reviewed changes

lpichler force-pushed the system_roles_assigments_in_migrator branch from b018976 to 87e2851 Compare November 7, 2024 15:55

alechenninger reviewed Nov 7, 2024

View reviewed changes

lpichler force-pushed the system_roles_assigments_in_migrator branch from 87e2851 to 672b8db Compare November 7, 2024 16:18

lpichler changed the title ~~[WIP] System roles assigments in migrator~~ System roles assigments in migrator Nov 7, 2024

lpichler changed the title ~~System roles assigments in migrator~~ [RHCLOUD-35855] System roles assigments in migrator Nov 7, 2024

lpichler force-pushed the system_roles_assigments_in_migrator branch from bc7f434 to 49d4bf9 Compare November 7, 2024 18:20

lpichler requested a review from alechenninger November 7, 2024 18:20

lpichler requested review from coderbydesign and astrozzc November 7, 2024 18:22

alechenninger reviewed Nov 7, 2024

View reviewed changes

rbac/migration_tool/migrate.py Outdated Show resolved Hide resolved

alechenninger reviewed Nov 7, 2024

View reviewed changes

rbac/migration_tool/migrate.py Outdated Show resolved Hide resolved

alechenninger reviewed Nov 7, 2024

View reviewed changes

rbac/migration_tool/migrate.py Outdated Show resolved Hide resolved

alechenninger reviewed Nov 7, 2024

View reviewed changes

lpichler force-pushed the system_roles_assigments_in_migrator branch 2 times, most recently from 77e5167 to 0cd909b Compare November 8, 2024 09:57

alechenninger approved these changes Nov 9, 2024

View reviewed changes

lpichler and others added 8 commits November 9, 2024 21:51

Replicate system role assignments for custom roles in migrator

71ef2dd

Leverage replication for user-groups membership from dual write handler

cffae10

Allow migration only on read only mode

4618398

Update rbac/migration_tool/migrate.py

33771a7

Co-authored-by: Alec Henninger <[email protected]>

Update rbac/migration_tool/migrate.py

5e35fbe

Co-authored-by: Alec Henninger <[email protected]>

Use check in memory for existence of system relations

663b08b

Co-authored-by: Alec Henninger <[email protected]>

Don't call replicate method for group assigment if it is not needed

2346391

Reorder migration tests; Use clone_default_group_in_public_schema and…

e22761e

… seed_group methods

lpichler force-pushed the system_roles_assigments_in_migrator branch from 794b916 to e22761e Compare November 9, 2024 20:51

lpichler merged commit e1af880 into master Nov 11, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RHCLOUD-35855] System roles assigments in migrator #1288

[RHCLOUD-35855] System roles assigments in migrator #1288

lpichler commented Nov 5, 2024 •

edited

Loading

alechenninger Nov 5, 2024

alechenninger Nov 5, 2024

alechenninger Nov 5, 2024

alechenninger Nov 5, 2024

alechenninger Nov 7, 2024

lpichler Nov 7, 2024 •

edited

Loading

alechenninger Nov 7, 2024

alechenninger Nov 7, 2024

alechenninger Nov 7, 2024

lpichler Nov 7, 2024

lpichler Nov 7, 2024 •

edited

Loading

alechenninger Nov 7, 2024

lpichler Nov 7, 2024

lpichler commented Nov 7, 2024 •

edited

Loading

alechenninger Nov 7, 2024

lpichler Nov 8, 2024

alechenninger Nov 7, 2024

lpichler Nov 8, 2024

alechenninger left a comment

[RHCLOUD-35855] System roles assigments in migrator #1288

[RHCLOUD-35855] System roles assigments in migrator #1288

Conversation

lpichler commented Nov 5, 2024 • edited Loading

Links

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpichler Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpichler Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpichler commented Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alechenninger left a comment

Choose a reason for hiding this comment

lpichler commented Nov 5, 2024 •

edited

Loading

lpichler Nov 7, 2024 •

edited

Loading

lpichler Nov 7, 2024 •

edited

Loading

lpichler commented Nov 7, 2024 •

edited

Loading