-
Notifications
You must be signed in to change notification settings - Fork 178
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
Fetch correct cluster admin list for the notebook controller admin panel #3538
Fetch correct cluster admin list for the notebook controller admin panel #3538
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3538 +/- ##
==========================================
- Coverage 85.55% 85.55% -0.01%
==========================================
Files 1372 1372
Lines 31211 31211
Branches 8741 8741
==========================================
- Hits 26704 26703 -1
- Misses 4507 4508 +1 see 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
I am not sure if this is a concern but before a admin user would see this from [
{
"username": "adminuser1",
"privilege": "Admin",
"lastActivity": null
},
] but now they can see every user that has cluster admin role on the cluster. for example clusteradminuser1 is a cluster admin but is not part of the groups for cluster-admin or odh-admin [
{
"username": "clusteradminuser1",
"privilege": "Admin",
"lastActivity": null
},
{
"username": "adminuser1",
"privilege": "Admin",
"lastActivity": null
}
] @andrewballantyne I remember you saying this is something we want to avoid, or maybe I am misremembering it. |
IMO any users or groups that are bound with the |
@Gkrumbach07 @DaoDaoNoCode it's interesting -- without a notebook, it's hard to say who is and who isn't going to be using the Dashboard's Jupyter Tile. The whole feature is kinda problematic... I think we want to make sure anyone who has a notebook is reflected clearly as either an admin or a user -- after that it's a bit uncharted. Cluster-admins can be IT or SRE and not at all users of the app. It seems that would be undesirable. I feel since the Jira didn't specifically say I want to see cluster-admins, and is just more concerned with admins being called users... is there anyway we can not show all cluster admins, and just the cluster admins that have had a notebook (aka last activity)? |
@andrewballantyne I believe so. I will double check and update the PR. |
ccc9858
to
630a2fb
Compare
630a2fb
to
2386efb
Compare
@Gkrumbach07 @andrewballantyne Hi, I updated the PR (see my second commit), now it only shows the cluster admins that have a Notebook resource in the notebook namespace (which means they ever try to use the KFNBC) in the admin panel and make sure the role column is |
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
allowed users is not showing other cluster admins now
[
{
"username": "adminuser1",
"privilege": "Admin",
"lastActivity": null
},
{
"username": "kubeadmin",
"privilege": "Admin",
"lastActivity": null
}
]
[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 |
…nel (opendatahub-io#3538) * Fetch correct cluster admin list for the notebook controller admin panel * Only show cluster admins with notebooks
…nel (opendatahub-io#3538) * Fetch correct cluster admin list for the notebook controller admin panel * Only show cluster admins with notebooks
JIRA: https://issues.redhat.com/browse/RHOAIENG-16434
Description
Use
ResourceAccessReview
API as described here to fetch the cluster admin users and groups instead of arbitrarily pushcluster-admins
group to the list and fetch it. This could also avoid the error mentioned in the JIRA where we are fetching a non-exist user group.How Has This Been Tested?
cluster-admin
roleAdmin
Test Impact
N/A
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main