feat: convert DomainOrg entries to RBAC role assignments via data migration - #1413
Open
CryptoRodeo wants to merge 1 commit into
Open
feat: convert DomainOrg entries to RBAC role assignments via data migration#1413CryptoRodeo wants to merge 1 commit into
CryptoRodeo wants to merge 1 commit into
Conversation
Contributor
Reviewer's GuideAdds a data migration that backfills pulpcore RBAC user/group role assignments from existing DomainOrg entries and configured domain access policies, ensuring object-level domain owner/viewer and domain-scoped service admin/viewer roles are created and aligned with previous access semantics. Entity relationship diagram for DomainOrg to RBAC role assignmentserDiagram
DomainOrg {
int org_id
}
User {
int id
}
Group {
int id
string name
}
Domain {
int id
string name
}
Role {
int id
string name
}
UserRole {
int id
}
GroupRole {
int id
}
DomainOrg }o--|| User : user
DomainOrg }o--|| Group : group
DomainOrg ||--o{ Domain : domains
User ||--o{ UserRole : has_user_roles
Group ||--o{ GroupRole : has_group_roles
Role ||--o{ UserRole : user_role_type
Role ||--o{ GroupRole : group_role_type
Domain ||--o{ UserRole : object_or_scoped
Domain ||--o{ GroupRole : object_or_scoped
Group ||--o{ DomainOrg : org_id_group
Group ||--o{ GroupRole : readonly_group_role
Flow diagram for DomainOrg to RBAC role migrationflowchart TD
start[convert_domainorgs_to_roles] --> ensure[_ensure_service_roles]
ensure --> svc_admin[Role service.domain_admin]
ensure --> svc_viewer[Role service.domain_viewer]
start --> core_roles[Role core.domain_owner / core.domain_viewer]
core_roles --> domain_ct[ContentType core.domain]
start --> qset[Query DomainOrg with domains, user, group]
qset --> loop_domainorg[for each DomainOrg]
loop_domainorg --> has_domains{has domains?}
has_domains -->|no| next_domainorg[skip entry]
has_domains -->|yes| user_group_check{user/group present?}
user_group_check -->|user or group| assign_existing[assign roles for existing user/group]
user_group_check -->|org_id only| create_org_group[Group get_or_create rh-org-<org_id>]
assign_existing --> loop_domains[for each domain]
create_org_group --> loop_domains
loop_domains --> user_role[_assign_pair to UserRole]
loop_domains --> group_role[_assign_pair to GroupRole]
start --> policies[iterate DOMAIN_ACCESS_POLICIES]
policies --> readonly_check{readonly_group set?}
readonly_check -->|no| next_policy[skip policy]
readonly_check -->|yes| find_domain[Domain filter by name]
find_domain --> domain_exists{domain found?}
domain_exists -->|no| next_policy
domain_exists -->|yes| ro_group[Group get_or_create readonly_group]
ro_group --> ro_assign[_assign_pair to GroupRole with core.domain_viewer / service.domain_viewer]
ro_assign --> migration_end[migration completes]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
DomainOrg.objects.prefetch_related("domains").all()loop may load a large number of rows into memory during migration; consider using.iterator()to reduce memory usage for large deployments. - The migration logic that derives readonly groups from
settings.DOMAIN_ACCESS_POLICIESbakes the current configuration into the database; if policies are expected to change over time, consider whether this behavior should instead be driven by runtime configuration rather than a one-time migration.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `DomainOrg.objects.prefetch_related("domains").all()` loop may load a large number of rows into memory during migration; consider using `.iterator()` to reduce memory usage for large deployments.
- The migration logic that derives readonly groups from `settings.DOMAIN_ACCESS_POLICIES` bakes the current configuration into the database; if policies are expected to change over time, consider whether this behavior should instead be driven by runtime configuration rather than a one-time migration.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
CryptoRodeo
force-pushed
the
feat/pulp-1893
branch
from
August 20, 2026 18:42
2106e44 to
8d77033
Compare
Contributor
Author
|
@sourcery-ai Thanks for the review. Addressed in the amended commit (8d77033):
|
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider adding
select_related("user", "group")to theDomainOrgqueryset to avoid extra queries for those FK lookups while iterating over many rows. - The migration currently imports
django.appsandPulpPluginAppConfigand inspects live app configs; if plugin labels or app config types change in future this could break the migration, so it may be safer to derive the relevant app labels from a more stable source (e.g. settings or a fixed list used elsewhere).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding `select_related("user", "group")` to the `DomainOrg` queryset to avoid extra queries for those FK lookups while iterating over many rows.
- The migration currently imports `django.apps` and `PulpPluginAppConfig` and inspects live app configs; if plugin labels or app config types change in future this could break the migration, so it may be safer to derive the relevant app labels from a more stable source (e.g. settings or a fixed list used elsewhere).
## Individual Comments
### Comment 1
<location path="pulp_service/pulp_service/app/migrations/0019_convert_domainorg_to_roles.py" line_range="80-82" />
<code_context>
+ # may be absent on a fresh DB. Creating it early is harmless, a no-op for rows that already exist.
+ domain_ct, _ = ContentType.objects.get_or_create(app_label="core", model="domain")
+
+ # Convert every DomainOrg entry. iterator(chunk_size=...) streams rows instead of
+ # loading them all at once; chunk_size is required for prefetch_related to be observed.
+ for domain_org in DomainOrg.objects.prefetch_related("domains").iterator(chunk_size=500):
+ domains = list(domain_org.domains.all())
+ if not domains:
</code_context>
<issue_to_address>
**issue (performance):** `iterator()` causes `prefetch_related()` to be ignored, leading to N+1 queries.
Because `QuerySet.iterator()` disables `prefetch_related()`, this loop will issue a separate query for each `domain_org.domains.all()` call instead of using the prefetched data, creating an N+1 pattern on large datasets. If you need streaming, consider removing `iterator()` so prefetch works, or implement custom batching that preserves `prefetch_related()`.
</issue_to_address>
### Comment 2
<location path="pulp_service/pulp_service/app/migrations/0019_convert_domainorg_to_roles.py" line_range="103-111" />
<code_context>
+
+ # readonly groups (Lightwell-ReadOnly) for DOMAIN_ACCESS_POLICIES. Runs once total,
+ # independent of DomainOrg rows (must still fire on a DB with no DomainOrg entries).
+ for domain_name, policy in getattr(settings, "DOMAIN_ACCESS_POLICIES", {}).items():
+ readonly_group_name = policy.get("readonly_group")
+ if not readonly_group_name:
+ continue
+ domain = Domain.objects.filter(name=domain_name).first()
+ if domain is None:
+ continue
+ group, _ = Group.objects.get_or_create(name=readonly_group_name)
+ _assign_pair(GroupRole, {"group": group}, domain_viewer_role, viewer_role, domain, domain_ct)
+
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Relying on `settings.DOMAIN_ACCESS_POLICIES` inside a migration can make it non-deterministic over time.
Because migrations must be reproducible, tying behavior to `settings.DOMAIN_ACCESS_POLICIES` (which can differ by environment or change over time) risks inconsistent DB state across deployments. If these policies need to influence data at migration time, either embed a fixed snapshot of the relevant data into the migration, or move this logic into a post-migrate/bootstrap step where runtime configuration is expected.
Suggested implementation:
```python
for domain in domains:
# user-set
if user is not None:
_assign_pair(UserRole, {"user": user}, domain_owner_role, admin_role, domain, domain_ct)
# group-set
if group is not None:
_assign_pair(GroupRole, {"group": group}, domain_owner_role, admin_role, domain, domain_ct)
# NOTE: Logic that depends on runtime configuration such as settings.DOMAIN_ACCESS_POLICIES
# has been intentionally excluded from this migration to keep it deterministic. Any behavior
# that uses DOMAIN_ACCESS_POLICIES to create readonly groups or assign roles should be
# implemented in a post-migrate/bootstrap step instead of here.
def _ensure_service_roles(apps):
```
To fully implement the suggestion, you should:
1. Add a post-migrate or bootstrap hook (e.g., in `pulp_service/pulp_service/app/apps.py` or a dedicated `bootstrap.py`) that:
- Reads `settings.DOMAIN_ACCESS_POLICIES`.
- For each domain/policy, creates the readonly group and assigns the appropriate roles using the same `_assign_pair` semantics, but via the runtime ORM (not migration `apps`).
2. Ensure that hook is idempotent (safe to run multiple times) and documented so operators know that `DOMAIN_ACCESS_POLICIES` affect runtime bootstrap, not historical migrations.
3. If you need historical behavior to be frozen, capture a fixed snapshot of policies and implement that as a separate data migration that does **not** read `settings`, but uses hard-coded values.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
CryptoRodeo
force-pushed
the
feat/pulp-1893
branch
from
August 20, 2026 20:10
8d77033 to
a4b5e58
Compare
Contributor
Author
|
Thanks for the review. Addressed in a4b5e58 (amended): N+1 on FK lookups - added
|
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider avoiding direct use of
settings.DOMAIN_ACCESS_POLICIESinside the migration, or at least documenting that the migration’s behavior depends on the runtime configuration at migration time and will not automatically reflect future policy changes. - In
_ensure_service_roles, you reach intodjango.appsandPulpPluginAppConfiginstead of using only the historical models provided byapps; if plugin registration or app configs change, this migration may behave differently than expected, so it may be worth constraining or snapshotting the plugin set used here.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider avoiding direct use of `settings.DOMAIN_ACCESS_POLICIES` inside the migration, or at least documenting that the migration’s behavior depends on the runtime configuration at migration time and will not automatically reflect future policy changes.
- In `_ensure_service_roles`, you reach into `django.apps` and `PulpPluginAppConfig` instead of using only the historical models provided by `apps`; if plugin registration or app configs change, this migration may behave differently than expected, so it may be worth constraining or snapshotting the plugin set used here.
## Individual Comments
### Comment 1
<location path="pulp_service/pulp_service/app/migrations/0019_convert_domainorg_to_roles.py" line_range="105-109" />
<code_context>
+
+ # readonly groups (Lightwell-ReadOnly) for DOMAIN_ACCESS_POLICIES. Runs once total,
+ # independent of DomainOrg rows (must still fire on a DB with no DomainOrg entries).
+ for domain_name, policy in getattr(settings, "DOMAIN_ACCESS_POLICIES", {}).items():
+ readonly_group_name = policy.get("readonly_group")
+ if not readonly_group_name:
+ continue
+ domain = Domain.objects.filter(name=domain_name).first()
+ if domain is None:
+ continue
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Silently taking the first domain by name can hide configuration issues when multiple domains share a name or a name is misconfigured.
Using `Domain.objects.filter(name=domain_name).first()` means roles may be attached to an arbitrary domain when multiple share the same name, and misconfigured `DOMAIN_ACCESS_POLICIES` entries (typos/non-existent names) are silently ignored. Consider using `get()` to fail fast on bad config, or at least logging when multiple domains match or none are found, so these issues are visible instead of silently skipped.
Suggested implementation:
```python
# readonly groups (Lightwell-ReadOnly) for DOMAIN_ACCESS_POLICIES. Runs once total,
# independent of DomainOrg rows (must still fire on a DB with no DomainOrg entries).
for domain_name, policy in getattr(settings, "DOMAIN_ACCESS_POLICIES", {}).items():
readonly_group_name = policy.get("readonly_group")
if not readonly_group_name:
continue
try:
domain = Domain.objects.get(name=domain_name)
except Domain.DoesNotExist:
raise RuntimeError(
f"DOMAIN_ACCESS_POLICIES readonly_group entry refers to non-existent domain "
f'"{domain_name}". Please fix your DOMAIN_ACCESS_POLICIES configuration.'
)
except Domain.MultipleObjectsReturned:
raise RuntimeError(
f"DOMAIN_ACCESS_POLICIES readonly_group entry refers to domain name "
f'"{domain_name}" which matches multiple Domain rows. Domain names must be '
f"unique or DOMAIN_ACCESS_POLICIES must be updated to disambiguate."
)
user = domain_org.user
```
If this migration later uses `readonly_group_name` and `domain` to create roles or groups, ensure that logic appears after this loop and uses the now-guaranteed single `domain` instance. No further structural changes are required; this edit will cause the migration to fail fast with clear errors when `DOMAIN_ACCESS_POLICIES` is misconfigured or when multiple domains share the same name.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ration Add data migration 0019_convert_domainorg_to_roles that backfills pulpcore RBAC role assignments from existing DomainOrg rows, so the ObjectRolePermission backend grants the same access the DomainOrg checks used to. Runs in the migrate-db pod at deploy time. For each DomainOrg, per domain, it creates the two-role pair: - object-level core.domain_owner ON the Domain (view/manage the domain, list it) - domain-scoped service.domain_admin (manage repos/remotes/distributions within) Rules honored: - user-only -> UserRole pair; group-only -> GroupRole pair - org_id-only -> create group rh-org-<org_id>, then GroupRole pair - both user and group -> both the UserRole pair and the GroupRole pair - Lightwell-ReadOnly -> object-level core.domain_viewer + domain-scoped service.domain_viewer on the lightwell domain (driven by DOMAIN_ACCESS_POLICIES) The service.domain_admin/service.domain_viewer roles are created inline via Role.objects.get_or_create() + permissions.set(), because post_migrate (which normally creates them) only fires after all migrations complete. The post_migrate handler remains the maintenance path for keeping those roles in sync as plugins change. Idempotent (get_or_create on the full unique_together tuple), reverse is a no-op, and DomainOrg data is preserved for rollback. Signed-off-by: Bryan ramos <bramos@redhat.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CryptoRodeo
force-pushed
the
feat/pulp-1893
branch
from
August 20, 2026 20:59
a4b5e58 to
95a35b5
Compare
Contributor
Author
|
Re: This is intentional and required. The role permission set must match exactly what the runtime |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add data migration
0019_convert_domainorg_to_rolesthat backfills pulpcore RBAC role assignments from existingDomainOrgrows, so theObjectRolePermissionbackend grants the same access theDomainOrgchecks used to. Runs in themigrate-dbpod at deploy time.For each
DomainOrg, per domain, it creates the two-role pair:core.domain_ownerON theDomain(view/manage the domain, list it)service.domain_admin(manage repos/remotes/distributions within)Rules honored:
DOMAIN_ACCESS_POLICIES)The
service.domain_admin/service.domain_viewerroles are created inline viaRole.objects.get_or_create()+permissions.set(), because post_migrate (which normally creates them) only fires after all migrations complete.The
post_migratehandler remains the maintenance path for keeping those roles in sync as plugins change.Idempotent (
get_or_createon the fullunique_togethertuple), reverse is a no-op, andDomainOrgdata is preserved for rollback.Summary by Sourcery
Convert legacy DomainOrg access controls to equivalent RBAC role assignments during database migration.
New Features:
Enhancements:
Deployment: