🐛 Skip maximal permission policy authorizer for deep SAR requests #2385
Conversation
astefanutti
force-pushed
the
pr-23
branch
2 times, most recently
from
fcd1dae
to
5f43d1b
Compare
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
Hitting kcp-dev/contrib-tmc#78 and kcp-dev/contrib-tmc#77 flakes. |
|
/retest |
|
/assign @s-urbaniak |
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
|
/milestone v0.10 |
|
As an after-though this looks correct. Initially I assumed this has to go via the maximum permission policy authorizer but in the deep SAR case it indeed has to be delegated up to the actual local authorizer. |
|
/lgtm |
|
/approve |
1 similar comment
|
/approve |
openshift-ci
bot
added
the
approved
|
/retest |
|
/lgtm |
|
/lgtm cancel |
| }, | ||
| }, | ||
| } | ||
| err = retry.OnError(retry.DefaultBackoff, errors.IsForbidden, func() error { |
There was a problem hiding this comment.
Out of curiosity, why is it being forbidden?
There was a problem hiding this comment.
It happens the ClusterRole and ClusterRoleBinding created few lines above do not apply synchronously, so the creation of the APIBinding can fail until they are effectively applied. That is actually the reason why I favoured retry.OnError over framework.Eventually, because it's only useful to retry on 403, and it can fail-fast otherwise.
There was a problem hiding this comment.
Next commit changes to use framework.Eventually - but to the point I'd say it's highly unlikely we get an error we can fail fast on and the benefit from the eventually bit is good. We can try to improve framework.Eventually for a short-circuit exit if we want
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ncdc, s-urbaniak, stevekuznetsov 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 |
kcp-ci-bot
added
the
size/L
Summary
This PR fixes authorization for deep SAR requests on attributes that resolve to workspace with local maximum permission policy, where the APIExport is also bound, such as the APIExport in the
rootworkspace.Related issue(s)
Fixes #2384.