-
Notifications
You must be signed in to change notification settings - Fork 1.6k
KEP-4006: Synthetic RBAC create check for WebSocket request upgrade -- 1.35 #5524
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
Conversation
| --> | ||
|
|
||
| 1. We will not make *any* changes to current WebSocket based browser/javascript clients. | ||
| 2. We will not extend the WebSockets communication leg from the API Server to Kubelet (in |
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 think is reasonable in order to avoid keeping this KEP and feature gate for longer, this has already an immediate positive impact by allowing the entire ecosystem to use websockets against the apiserver instead of a deprecated protocol like SPDY, with almost zero support for most of the load balancers causing a lot of friction.
|
/lgtm |
|
I do think we should resolve the user-visible permissions change related to kubectl exec / attach / portforward - see kubernetes/kubernetes#133515 (comment) The permissions required by kubectl changed from |
|
|
||
| - `kubectl` environment variables and API Server feature gates are locked to on by default. | ||
| - Deprecate `kubectl` environment variables and API Server feature gates for future removal. | ||
| - Address RBAC authorization for WebSocket upgrades. The mechanism must be compatible with existing authorization rules for subresources (e.g., `pods/exec`) that are typically enforced on POST requests. |
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.
@liggitt for your comment in kubernetes/kubernetes#133515 (comment) and your code there is unclear to me if you want to rollout this authoritzation compatibility as a separate gate or making this GA but not locked by default during one cycle.
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 think we need a separate gate, but we can add a few sentences in this KEP about that gate and it's purpose, since it's sort of prompted by the kubectl switch to websockets.
I'd like to get that merged as a prereq for indicating work on this KEP is complete / GA
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 have updated the KEP to indicate GA in 1.36.
- I have added a separate feature gate
ForceRBACCreateCheckfor the additionalCREATEauthz check. This functionality is a prerequisite for GA.
Please let me know what you think.
| - Deprecate `kubectl` environment variables and API Server feature gates for future removal. | ||
| - Force synthetic RBAC `CREATE` authorization check for WebSocket upgrades on the following | ||
| subresources: `pods/exec`, `pods/attach`, and `pods/portforward`. This additional check | ||
| will be gated by the API Server `ForceRBACCreateCheck` feature flag, which defaults to |
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'd name this as specifically as possible, to indicate what it does... maybe something like AuthorizePodWebsocketUpgradeCreatePermission
Maybe describe this in a paragraph in the design details section. We'll start defaulting to true but not lock to true for a release so people can disable as a mitigation if needed short-term.
1.35: default=true, state=beta
1.36: default=true, state=ga, locked=true
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 believe I have addressed this. Please let me know what you think.
| The upstream connection is the same SPDY connection to the container (through the | ||
| Kubelet and CRI). | ||
|
|
||
| ### Pre-GA: Kubelet `StreamTranslatorProxy` |
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.
restore this paragraph, keep the kubelet StreamTranslatorProxy bits, drop out the container runtime websocket bits
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.
then we need also to change https://github.com/kubernetes/enhancements/pull/5524/files#r2334624987
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 believe I have addressed this. Please let me know what you think.
|
/milestone v1.35 |
| or pods/portforward subresources, the handler will perform an additional check to ensure | ||
| the user also has the create verb permission for that specific subresource. This new | ||
| authorization check will be controlled by a feature gate, | ||
| `AuthorizePodWebsocketUpgradeCreatePermission`, which will be enabled by default to |
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.
Will this feature gate be removed at some point in the future?. Or we'll keep it for a long time?.
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 a couple releases in enabled-by-default state, we'll transition it to locked on, then remove it ~3 releases after that
|
/lgtm |
| communication path from the client to the Kubelet. After the initial | ||
| kubectl-to-API-Server leg is stable, this proposal outlines the next phase: | ||
| transitioning the communication between the API Server and the Kubelet. This is | ||
| achieved by moving the translation logic currently in the API Server—specifically |
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.
| achieved by moving the translation logic currently in the API Server—specifically | |
| achieved by replicating the translation logic currently in the API Server—specifically |
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 have addressed this issue. Please have a look.
| variable associated with the feature to **OFF**. Or the features can be turned off | ||
| for all `kubectl` users communicating with a cluster by turning off the feature flags | ||
| for the API Server. | ||
| for the API Server. A cluster operator can disable the more stringent permissions for |
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.
| for the API Server. A cluster operator can disable the more stringent permissions for | |
| for the API Server. A cluster operator can temporarily disable the more stringent permissions for |
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 have addressed this language issue. Please have a look.
|
a couple language nits, but lgtm overall cc @enj @pmengelbert for kubernetes/kubernetes#133515 (comment) for the incorporation of the kubectl permission check into this KEP |
|
/lgtm |
| alpha: "v1.29" | ||
| beta: "v1.31" | ||
| stable: "v1.32" | ||
| stable: "v1.36" |
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.
35? Or are we waiting till spring to go GA?
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.
We'll wait until v1.36; we're implementing a new RBAC permissions check for WebSocket upgrades in this v1.35 release, and we'll wait one release before attempting GA.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpbetz, liggitt, seans3 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 |
AuthorizePodWebsocketUpgradeCreatePermission; enabled by default