-
Notifications
You must be signed in to change notification settings - Fork 39.3k
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 missing checkpoint permissions to system:kubelet-api-admin #126724
base: master
Are you sure you want to change the base?
Conversation
Skipping CI for Draft Pull Request. |
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vinayakankugoyal The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/sig-node |
/sig node |
354c3e2
to
aa0825e
Compare
aa0825e
to
a7b5d76
Compare
Signed-off-by: Vinayak Goyal <vinaygo@google.com>
a7b5d76
to
5b31795
Compare
LGTM to me. I would prefer a review from sig-auth though. |
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.
mechanically, this looks correct
did we verify it was unintentional that kube-apiserver not be able to trigger this endpoint, or was the separate permission intentional so it couldn't be driven via the kube apiserver?
} | ||
|
||
if utilfeature.DefaultFeatureGate.Enabled(features.ContainerCheckpoint) { | ||
kubeletAPIAdminRules = append(kubeletAPIAdminRules, rbacv1helpers.NewRule("*").Groups(legacyGroup).Resources("nodes/checkpoint").RuleOrDie()) |
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.
nit: prefer a specific verb if the kubelet only supports a single verb on the checkpoint subresource
AFAIK forensic checkpointing KEP was about having a kubelet admin be able to run checkpoints against the kubelet API. I think we'd like to revisit checkpoint/restore in a new KEP to focus on RBAC for a more friendly UX. So I think it is intential that you can't drive checkpointing via kube apiserver. |
ok... I wondered about that. If that is the case then we should not add this permission until that is resolved. |
So I thought that this role would be for a kubelet admin (system:kubelet-api-admin)? |
My motivation of adding it was that it seemed to me that this role it meant to have all the permissions for the kubelet API. @SergeyKanzhelev WDYT? |
Exposing the forensic checkpoint from control plane was not in scope: https://github.com/kubernetes/kubernetes/pull/104907/files#r920403597. I would suggest we take this to the KEP discussion on what kind of API we want to expose |
By default no binding is created referencing that ClusterRole:
|
Sure, but practically, this is the role everyone binds to the identity given to the kube-apiserver to talk to the kubelet |
What type of PR is this?
/kind bug
What this PR does / why we need it:
kubelet has a new endpoint called checkpoint which can be used to take snapshots of containers running on the node.
If the kubelet wehbook authn and authz is enabled then to call it you need verb: "post" on resource: "nodes/checkpoint".
"system:kubelet-api-admin" is a ClusterRole that should have permission to call all webhook endpoints. It does not have permissions to call this endpoint. This makes it so that a user with system:kubelet-api-admin ClusterRole can call this endpoint.
Which issue(s) this PR fixes:
Fixes #126232
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: