-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add RFD 183: Access Request Kube Resource Allow List #46691
Add RFD 183: Access Request Kube Resource Allow List #46691
Conversation
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
1 similar comment
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
cbe54dd
to
f697513
Compare
@klizhentas @xinding33 Can you review this when you get a chance? |
a750979
to
26d187f
Compare
460cff6
to
4e7ad75
Compare
```yaml | ||
kind: role | ||
metadata: | ||
name: requester | ||
spec: | ||
options: | ||
request_mode: | ||
# Can request only pods or namespaces | ||
kubernetes_resources: | ||
- kind: namespace | ||
- kind: pod | ||
``` |
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.
I'm curious why we have this under spec.options.request_mode.kubernetes_resources
instead of, say, spec.allow.request.kubernetes_resources
. Basically why is it a role option and not under spec.allow.request
where most other access request configuration is? And what is the effect if the option is set differently in multiple roles the user has?
I haven't really thought about it hard enough to say one of those options is definitively better, I'm just wondering if there's a reason or it just seemed like the default way to do it
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.
After slightly more thought, I think this doesn't really make sense as a role option, and an allow/deny rule would be better
Here's my mental model of role options (I'm sure there are exceptions to this but i think it's mostly accurate). Usually, role options are coalesced across all roles a user has to result in a single value, and usually that value ends up encoded in the user's certificate. For example, the forward_agent
option is combined from all roles the user has, and it's encoded in the SSH cert when the user logs in. The same for many other options. Even when not encoded in the cert, usually you compute a single value from the user's entire role set.
If instead you want one role to allow the user to request certain kubernetes namespaces with certain target roles, and another role to allow the user to request kubernetes clusters with a different target role, and another role can deny the user requesting a specific namespace, I think this would be better as an allow/deny rule under spec.allow.request
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.
@nklaassen updated RFD to reflect field change
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.
@kimlisa lgtm with one syntactic comment
spec: | ||
allow: | ||
request: | ||
mode: |
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.
Do we need "mode" sub-field at all here then? Sounds like something like this would be slightly cleaner:
spec:
allow:
request:
kubernetes_resources:
- kind: "*"
dc9c7b5
to
93f98d5
Compare
495c630
to
b64659f
Compare
rendered
part of: #46742