Fix dryRun issue with permissionless user #3530
Conversation
|
Skipping CI for Draft Pull Request. |
| return createRoleBinding(rolebinding, opts).catch((error) => { | ||
| if (error.statusObject?.code === 404 && opts?.dryRun) { | ||
| // If dryRun is enabled and the user is not Cluster Admin it seems that there's a k8s error | ||
| // that raises a 404 trying to find the role, which is missing from the dryRun |
There was a problem hiding this comment.
Missing from the Dry Run -- because we are dry running everything and the RoleBinding validates the existence of the Role on the cluster?
There was a problem hiding this comment.
Yup, that's right, I assume this is a k8s error, cause it only happens with permisionless users, when dryRunning, creating the rolebinding i think internally tries to get the role it's trying to assign, since it's not yet created it fails. My guess is that with cluster admins k8s does the correct check and it assumes it's dryRunned too.
There was a problem hiding this comment.
It's funny, cause if you change the rolebinding to not use an specific role it works just fine, or if you run this with cluster admins, it's just with this specific scenario.
There was a problem hiding this comment.
This is definitely strange. It doesn't make sense that the createRoleBinding util would return a 404 since it's a direct call to k8sCreateResource with the RoleBindingModel, the client never does a GET for the role - you mean that is happening internally in the k8s API? Seems bizarre that it would respond with 404 then and not 400.
It also doesn't make sense that this ever worked for admins tbh - I don't see why the API would be aware of the previous dryRun requests, they're all separate HTTP requests. So for some reason that internal check for the role existing isn't happening for admins? I wonder if admin permissions mean overriding checks like that or something? Confusing all around...
This solution seems fine to me based on the behavior we're seeing, I would love to understand it more but I suppose that doesn't block this PR.
lucferbux
force-pushed
the
rhoaieng-15479
branch
from
0ab5952 to
1d62136
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
lucferbux
force-pushed
the
rhoaieng-15479
branch
from
1d62136 to
18f3362
Compare
lucferbux
force-pushed
the
rhoaieng-15479
branch
from
18f3362 to
90e3338
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3530 +/- ##
==========================================
+ Coverage 85.42% 85.57% +0.15%
==========================================
Files 1353 1342 -11
Lines 31129 31012 -117
Branches 8692 8667 -25
==========================================
- Hits 26591 26538 -53
+ Misses 4538 4474 -64
... and 19 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
i tested the PR image on my cluster with a non admin user and the issue was fixed. the extra isModelMesh is slightly confusing but i suppose it makes sense to avoid unnecessary calls |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mturley 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 |
Yes thanks Emily, this is just because debugging i realized in kserve we are calling it twice, one for the inferenceservice creation and the other for servingruntime, it's just to avoid duplicated calls. |
Description
Closes https://issues.redhat.com/browse/RHOAIENG-15479
Fix an issue in which users without cluster admin roles would dry run rolebindings referencing a role that's not there.
Also, avoiding calling setUpTokenAuth twice in kserve.
How Has This Been Tested?
These changes will fix that scenario
Test Impact
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main