core/runtime: Check shim PluginInfo to enforce idmap support #9781
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. |
rata
force-pushed
the
rata/userns-use-pluginInfo
branch
2 times, most recently
from
d21884d to
9243ae1
Compare
rata
force-pushed
the
rata/userns-use-pluginInfo
branch
2 times, most recently
from
51bb4e7 to
39dcd52
Compare
|
The unrelated flake is gone now. All green :) |
|
@AkihiroSuda can you add the ok-to-test ? I think that kicks the k8s tests |
|
/ok-to-test |
rata
force-pushed
the
rata/userns-use-pluginInfo
branch
from
39dcd52 to
78396c6
Compare
rata
force-pushed
the
rata/userns-use-pluginInfo
branch
from
78396c6 to
ae7be77
Compare
Head branch was pushed to by a user without write access
rata
force-pushed
the
rata/userns-use-pluginInfo
branch
4 times, most recently
from
c1cbfdf to
f1908cf
Compare
|
Pushing to solve unrelated CI failures in vagrant (I've also removed some blank lines and improve some comments/error msgs, very minor details really). |
rata
force-pushed
the
rata/userns-use-pluginInfo
branch
from
f1908cf to
8fbf14d
Compare
This commit gets rid of the TODO by moving the check to use the pluginInfo() infrastructure. The check is only enforced for shims that return info that can be read as type runtime.Features. For shims that don't provide that, we just ignore it, as those shims might not be affected by this. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
rata
force-pushed
the
rata/userns-use-pluginInfo
branch
from
8fbf14d to
f1e265b
Compare
|
The k8s failure seems a flake too, it was passing just fine until now. Also, I can't see any logs from the failure, maybe something is broken on the k8s CI? |
|
/retest |
1 similar comment
|
/retest |
|
@AkihiroSuda all green now! Please LGTM again or merge :D |
|
@fuweid maybe you can LGTM again/merge too? :) |
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
|
@AkihiroSuda this was removed from the merge queue due to an issue in CI. Can you merge or put it again? AFAIK, the issue is really unrelated to this PR (no behavior is changed here, we change where we do this check). |
This commit gets rid of the TODO by moving the check to use the pluginInfo() infrastructure (#9442).
The check is only enforced for shims that return info that can be read as type runtime.Features. For shims that don't provide that, we just ignore it, as those shims might not be affected by this.
NOTE: Since Kubernetes 1.30, the kubelet checks that the container runtime supports all the features for user namespaces (iow, it checks idmap mounts support too) and, if the runtime doesn't support them, it just rejects the pod creation without sending it to the runtime. That makes it less of an issue, as this PR is not really needed now for kubernetes >= 1.30 workloads. However, it is the right thing to do (we shouldn't rely on k8s doing it, we should check it here too), and it is still relevant for kubernetes < 1.30 and workloads not using Kubernetes.
cc @AkihiroSuda