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
[release/1.7] Throw an error if the kubelet requests mounts with uid/gid mappings #8211
[release/1.7] Throw an error if the kubelet requests mounts with uid/gid mappings #8211
Conversation
Hi @rata. Thanks for your PR. I'm waiting for a containerd 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/test-infra repository. |
9d5c3d2
to
1e4e9fe
Compare
1e4e9fe
to
fb937d8
Compare
fb937d8
to
a86c0e3
Compare
The k8s PR has been merged, so I've updated this to do the proper vendoring and mark it ready for review. I've opened it against branch 1.7, as that is where we need this (see #8209 for more info). I'll open a draft PR that relies on runc 1.2 for the main branch, to properly handle idmap mounts. cc @fuweid as you were active in the issue and userns support PRs. |
/ok-to-test |
Friendly ping? |
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.
LGTM
ping? @estesp thanks! Can you maybe get another maintainer to review this too? :) |
sorry for missing the mention. Did we have pr to main branch? @rata |
@rata We usually don't apply patch to release branch directly. Hope you don't mind. I think you can just file a similar pr X to main branch so that we can know that this pr 8211 is cherry-picked from X. So the steps are
Sounds good to you? BTW, this patch looks good to me. |
Perfect, will do that then. Thanks! |
Switching to draft until the one in main is merged, then I'll |
We will use this in future commits to see if the kubelet requested idmap mounts for volumes, that we don't yet support. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
We need support in containerd and the OCI runtime to use idmap mounts. Let's just throw an error for now if the kubelet requests some mounts with mappings. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com> (cherry picked from commit 7e6ab84)
a86c0e3
to
7de8629
Compare
@fuweid thanks! I updated this PR now with the cherry-pick, PTAL |
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.
LGTM
Two LGTM are enough? Or do we want to re-review as technically although the code hasn't changed, now the commit was cherr-picked? |
ping @AkihiroSuda @dmcgowan |
@@ -77,7 +77,7 @@ require ( | |||
k8s.io/apiserver v0.26.2 | |||
k8s.io/client-go v0.26.2 | |||
k8s.io/component-base v0.26.2 | |||
k8s.io/cri-api v0.26.2 | |||
k8s.io/cri-api v0.27.0-beta.0 |
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.
Non-beta might be preferable, but can be bumped up later
As requested by Akihiro Suda here: containerd#8211 (comment) This just bumps the tag name to the k8s final release. There are no changes other than the tag name, though. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
As requested by Akihiro Suda here: containerd#8211 (comment) This just bumps the tag name to the k8s final release. There are no changes other than the tag name, though. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
As requested by Akihiro Suda here: containerd#8211 (comment) This just bumps the tag name to the k8s final release. There are no changes other than the tag name, though. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
As requested by Akihiro Suda here: containerd#8211 (comment) This just bumps the tag name to the k8s final release. There are no changes other than the tag name, though. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com> (cherry picked from commit 92b93e3)
As requested by Akihiro Suda here: containerd#8211 (comment) This just bumps the tag name to the k8s final release. There are no changes other than the tag name, though. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
As requested by Akihiro Suda here: containerd#8211 (comment) This just bumps the tag name to the k8s final release. There are no changes other than the tag name, though. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
This is a backport of #8376 to the 1.7 release branch (the only release with userns support, so the only one affected).
Please note I
cherry-pick -x
commit "cri: Throw an error if idmap mounts is requested", but manually did the vendor again (instead of cherry.picking) because there are other differences in the go modules.