[Carry 133278] kubelet: Don't ignore idsPerPod config #133278 #133373
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The idsPerPod where completely ignored since they were introduced in PR: kubernetes#130028 The problem was the following: 1. The userns manager (as well as all managers) is created before the config is copied to the kubelet object[1] 2. The userns manager on creation is calling the kubelet_getter GetUserNamespacesIDsPerPod to get the idsPerPod 3. The getter checks the configuration stored in the kubelet object, which hasn't been set at that point. 4. As the config is nil (unset), it returns the default value. Therefore, the value was ignored. To solve this, let's just pass the idsPerPod as a parameter to MakeUserNsManager(). This is the common pattern already used in the kubelet initialization. [1]: https://github.com/kubernetes/kubernetes/blob/461ba83084ab7cb91ab692687bb7aedb05c6eb65/pkg/kubelet/kubelet.go#L1078-L1087 [2]: https://github.com/kubernetes/kubernetes/blob/461ba83084ab7cb91ab692687bb7aedb05c6eb65/pkg/kubelet/kubelet_getters.go#L145 Signed-off-by: Rodrigo Campos <[email protected]>
The first UID used for userns mappings needs to be at least the idsPerPod. When idsPerPod is extended to be 65536*2, for example, then the default UID of 65536 doesn't work. While this can be configured by the user, let's just improve the default first UID to be the same as idsPerPod. This makes the default first UID work out of the box if the user just wants to tune that. This also simplifies testing, as we don't need to create a system user and /etc/subuid and /etc/subgid files to test the idsPerPod setting. Signed-off-by: Rodrigo Campos <[email protected]>
Golang allows to call methods on a nil object, as long as the methods don't dereference the nil object. This is what we do here. This makes all the userns configurations (idsPerPod or mapping configs in /etc/subuid or /etc/subgid) to be ignored if the feature is off. Signed-off-by: Rodrigo Campos <[email protected]>
Signed-off-by: Rodrigo Campos <[email protected]> Signed-off-by: Akihiro Suda <[email protected]>
k8s-ci-robot
added
kind/bug
k8s-ci-robot
added
area/kubelet
area/test
sig/node
github-project-automation
bot
added this to
SIG Node CI/Test Board and SIG Node: code and documentation PRs
k8s-ci-robot
removed
the
do-not-merge/needs-sig
github-project-automation
bot
moved this to Triage
in SIG Node: code and documentation PRs
k8s-ci-robot
added
release-note
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/test pull-kubernetes-node-crio-cgrpv2-userns-e2e-serial |
|
@AkihiroSuda Thanks for improving the tests! |
rata
approved these changes
Aug 5, 2025
There was a problem hiding this comment.
LGTM
@SergeyKanzhelev can you please add this one to the milestone?
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AkihiroSuda, rata 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 |
bart0sh
moved this from Triage
to Needs Reviewer
in SIG Node: code and documentation PRs
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Carry #133278, so as to address my own comments (tests/e2e_node: Add test for userNamespaces.idsPerPod)
(Below is copied from #133278)
The idsPerPod where completely ignored since they were introduced in PR: #130028
The problem was the following:
Therefore, the value was ignored.
To solve this, let's just pass the idsPerPod as a parameter to MakeUserNsManager(). This is the common pattern already used in the kubelet initialization.
cc @AkihiroSuda @giuseppe
What type of PR is this?
/kind bug
What this PR does / why we need it:
We now honor the idsPerPod config from the kubelet.
Which issue(s) this PR is related to:
Fixes: #133144
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: