-
Notifications
You must be signed in to change notification settings - Fork 1.6k
KEP-5474: Introduce WritableCgroups option #5475
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
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Divya063 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 |
Welcome @Divya063! |
Hi @Divya063. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
018f069
to
bb09551
Compare
bb09551
to
dcd59fa
Compare
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.
Hi! Thank you for the work on this KEP. I'm requesting changes before this can be approved, primarily to address a security and resource isolation concern.
I strongly support the proposed direction of adding a field to the Pod SecurityContext
and CRI API. This is the right approach for providing fine-grained control and is much more secure than relying on a broad, runtime-level configuration.
My primary concern, which I believe is a blocker for approval, is the risk of intra-pod resource starvation. Please see my comments under "Risks and Mitigations" for details.
I also left a couple of comments with suggestions to improve clarify, primarily around removing any potentially confusing references to the Runtime
config field to make it unambiguous that the CRI-based API field is the sole proposed approach.
I'm happy to discuss these points further. Thank you!
/assign @chrishenzie |
Thank you @chrishenzie for providing the initial feedback! I'm changing the PR to draft state while I work on the doc. Once it's ready, I'll let you know. |
Thanks for the update and taking my feedback into consideration. I look forward to the next version of the document. As you might have gathered from my review, this is a feature I'm interested in and have spent a good deal of time researching independently. It seems we're aligned on the importance of this capability for the community and the need to get the API and security model right. Given our mutual interest, I wanted to offer to partner with you on this KEP if you'd be open to it. I'd be happy to contribute my research and help however I can to move this forward. If you're interested, please feel free to DM me on the Kubernetes Slack (@chrishenzie) or reach out via email so we can connect. Regardless, I appreciate you driving this important work. Let me know if there is any way I can help. |
60f61fc
to
bd0bdae
Compare
Thanks @chrishenzie for offering to partner on this KEP! I will be happy to collaborate. I have addressed your comments and would like to discuss a few things.
Let me know what you think. |
bd0bdae
to
ca08278
Compare
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.
Hi @Divya063,
Thanks again for the excellent work on this KEP and for your collaboration. I've left some detailed comments, but my main feedback is summarized here.
To help coordinate the implementation work, I have opened a tracking issue in the containerd/containerd repository. This follows the new SIG-Node collaboration process and will be the best place to discuss the runtime-specific details:
- containerd Tracking Issue: containerd/containerd#12252
My primary feedback is that the KEP would be stronger if it remained focused on the CRI API as a runtime-agnostic contract, rather than referencing specific runtimes. This approach ensures the feature is applicable to the entire ecosystem (including CRI-O) and aligns with the SIG-Node policy requiring multiple implementations for graduation.
I believe the proposed changes, especially the validation requiring the Guaranteed QoS class, effectively address the key security and resource isolation concerns.
I'm looking forward to collaborating with you to move this forward.
|
||
**kubelet vs Container Runtime**: | ||
- On unsupported runtimes, the feature is silently ignored because the WritableCgroups field is not used. | ||
- Runtime version check not enforced to avoid breaking existing setups |
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.
Can you expand on this? What would it mean to be enforced? Is this new behavior or leveraging existing kubelet behavior? Any links to relevant files are appreciated
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 changed this section, on unsupported runtimes Kubelet will now return an error.
bool writable_cgroups = 18; | ||
} | ||
``` | ||
|
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.
RuntimeHandlerFeatures
should be updated too
https://github.com/kubernetes/cri-api/blob/v0.34.0/pkg/apis/runtime/v1/api.proto#L1708
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.
Thanks @AkihiroSuda! I didn't know about this field.
|
||
### Notes/Constraints/Caveats (Optional) | ||
|
||
- **cgroup v2 Only**: This feature requires cgroup v2 and will be ignored on cgroup v1 systems |
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.
Why not return an error on cgroup v1 ?
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.
Updated the KEP, we will do a validation and return an error.
ca08278
to
ab340e6
Compare
This commit introduces a KEP that proposes adding a `CgroupWritable` field to the container SecurityContext in Kubernetes to allow unprivileged containers to have writable access to cgroup interfaces on cgroup v2 systems
ab340e6
to
634ee7b
Compare
Thanks @chrishenzie and @AkihiroSuda! I have addressed the comments and have removed specific runtime references. The PR is ready for another round of review. |
Uh oh!
There was an error while loading. Please reload this page.