[RHCLOUD-35997] Add admin endpoint to delete/list workspace/tenantmapping #1287
Conversation
rbac/internal/views.py
Outdated
| @@ -55,6 +56,10 @@ | |||
|
|
|||
| logger = logging.getLogger(__name__) | |||
| TENANTS = TenantCache() | |||
| RESOURCE_MODEL_MAPPING = { | |||
| "workspace": Workspace, | |||
| "mapping": TenantMapping, | |||
There was a problem hiding this comment.
Will we want to manage BindingMapping as well?
rbac/internal/views.py
Outdated
| if resource == "workspace": | ||
| # Have to delete the ones without references (default workspaces) | ||
| logger.info("Deleting default workspaces.") | ||
| resource_objs.exclude(parent=None).delete() |
There was a problem hiding this comment.
This would be all default + standard, correct? Not just default? Some of these would still fail if they have FK refs?
There was a problem hiding this comment.
I guess in our case for now, we're just using this for default and root though, right? So we should be able to get away with just deleting defaults first and then root, I guess is what you're saying? Platex may start testing with full hierarchy though, which would mean we'd need to do some recursive deletion.
rbac/api/utils.py
Outdated
| if org_id: | ||
| tenant = get_object_or_404(Tenant, org_id=org_id) | ||
| if resource == "binding": | ||
| resource_objs = resource_objs.filter(role__tenant=tenant) |
There was a problem hiding this comment.
This will miss bindings for system roles because those are in the public tenant. Is this intentional?
I think we probably want removing the binding to be based off of the tenant of the resource, however we don't have a foreign key here, and we might not always know the owning Tenant of the bound resource. For now, we actually do, though, since the resource is always a Workspace. Maybe we could just start there? That is, find all workspaces for a tenant, and then delete all the binding mappings for those workspaces (using resource type namespace, resource type name, and resource id fields to filter).
In the long run, we probably won't be able to support removing bindings by org, unless we make it clear it's limited to only certain narrow filter options (like custom roles, or workspace bindings).
It also may be safer for us to just blow away everything, but it probably doesn't hurt to have something org specific in case it is needed. It just has to be used very carefully of course.
There was a problem hiding this comment.
I had the wrong impression the bindings for system roles have to be kept. I am ignoring the org_id for bindingmapping for now
|
/retest |
astrozzc
force-pushed
the
list/clean
branch
4 times, most recently
from
7c5e747
to
a41d699
Compare
rbac/api/utils.py
Outdated
| if resource != "binding" and org_id: | ||
| tenant = get_object_or_404(Tenant, org_id=org_id) | ||
| queryset = queryset.filter(tenant=tenant) |
There was a problem hiding this comment.
Maybe abort or raise an error if its the binding and org_id is passed? Just so we have to be intentional if we want to delete everything, and someone doesn't think they're only deleting bindings for one org and end up accidentally deleting everything.
astrozzc
force-pushed
the
list/clean
branch
2 times, most recently
from
deada1c
to
ac96c19
Compare
|
@astrozzc Does this need update to open api spec ? |
|
|
||
| def migration_resource_deletion(resource, org_id): | ||
| """Delete migration related resources.""" | ||
| resource_objs = get_resources(resource, org_id) |
There was a problem hiding this comment.
Does resource needs to be checked if exists in RESOURCE_MODEL_MAPPING ? Maybe this check should in get_resources function ?
There was a problem hiding this comment.
It has already been checked at the beginning of the migration_resources function: https://github.com/RedHatInsights/insights-rbac/pull/1287/files#diff-fb45a5441f65035cd9b020f4b00090da2927a3b9c40c9dc56848e7129c9219ceR557-R568
rbac/internal/views.py
Outdated
|
|
||
| DELETE /_private/api/utils/migration_resources/?resource=xxx&org_id=xxx | ||
| GET /_private/api/utils/migration_resources/?resource=xxx&org_id=xxx&limit=1000 | ||
| optinos of resource: workspace, mapping(tenantmapping), binding(bindingmapping) |
There was a problem hiding this comment.
Oops, forgot to update this, good catch!
Oh, good point! Let me update that |
|
/retest |
1 similar comment
|
/retest |
Remove filter of binding. Fix unit test failure
Update test; Support offset for listing.
Link(s) to Jira
Description of Intent of Change(s)
The what, why and how.
Local Testing
How can the feature be exercised?
How can the bug be exploited and fix confirmed?
Is any special local setup required?
Checklist
Secure Coding Practices Checklist Link
Secure Coding Practices Checklist