-
Notifications
You must be signed in to change notification settings - Fork 177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate & Swap for Auth over DashboardConfig for group configs #3577
Deprecate & Swap for Auth over DashboardConfig for group configs #3577
Conversation
70dcf3c
to
2a290bf
Compare
cf9f9e4
to
1176e18
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3577 +/- ##
==========================================
- Coverage 85.18% 85.14% -0.04%
==========================================
Files 1393 1395 +2
Lines 31908 32034 +126
Branches 8940 8973 +33
==========================================
+ Hits 27181 27276 +95
- Misses 4727 4758 +31
... and 23 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
1176e18
to
f015f87
Compare
@Gkrumbach07 I had to rebase, conflict. Can you also explain this comment more:
admin -- when role is applied? Or always... because admins have no permissions today and will not succeed on the RBAC SSAR check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Gkrumbach07 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
https://issues.redhat.com/browse/RHOAIENG-16519
Description
A new Auth resource is being added by the Platform team. OdhDashboardConfig's group configurations are no longer needed.
When we go live with this the Auth resource, the OdhDashboardConfig groups will become read-only (see #3585), the Operator will copy over the items, and grant permissions to the admin users. This PR uses this functionality of new permissions to check if the admin can modify the Auth resource directly -- if so, pull & update there; if not fall back on todays logic (aka backwards compatibility). Do note, if the Auth resource cannot be modified by this user AND it exists, we will not be able to update the Groups Config from the UI as it will attempt to update the OdhDashboardConfig while it is read-only; known limitation but should be an untravelled path.
The infrastructure to get
isAdmin
uses the Auth resource if it exists, SA has been granted permissions to get the resource.Known Error States
If the CRD or the CR does not exist...
How it happens and for who, are noted below:
cluster-admin
sproduct
adminsNo matter what for the UI admin page, the resource can still be modified up until the CRD is merged with a no change allowed setting (see #3585)
How Has This Been Tested?
Today, the work for the Operator is a WIP. I used a CRD from their code & setup a rolebinding for my user:
CRD
Testing roles (will be handled by Platform more directly later):
Test Impact
Updated the unit tests to use the new function and make sure the old one is not called.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main