-
Notifications
You must be signed in to change notification settings - Fork 453
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 user namespaces tests #1354
Add user namespaces tests #1354
Conversation
2a84587
to
3527be0
Compare
Hm, I'm wondering why the containerd+runc tests fail, because they seem to work on my local env:
I also assume that containerd v1.6 is not supported, so we may want to skip the tests for them. |
f3747eb
to
f0cbb20
Compare
@saschagrunert the most likely reason is a permission issue. This smells like the rootfs is created inside a Which runc version is this using? Do you face a similar error if you remove exec permissions for some dir in the path to the rootfs locally? |
works on 1.7 and 2.0/main thanks Rata... not implemented on 1.6 run this test if you like on containerd and grab the log of the run pod request.. or I can get it for ya, if you are looking to match up your test parameters.. and checkout how rata did the integration bucket https://github.com/containerd/containerd/blob/main/integration/pod_userns_linux_test.go#L129 nod to rata on /home/runner/work/cri-tools/cri-tools/tmp* is probably created with the wrong permissions.. |
Also, in the linked file there is a traversePath() function that might be handy to test if that is the issue. |
acc4855
to
daf0749
Compare
No, it's running as root: cri-tools/.github/workflows/containerd.yml Line 315 in 8463eae
I'm not able to reproduce the issue locally with containerd. It also works with crun, so I'm wondering if it's related to runc. I focused the test and fixed the log collection in the CI:
|
@saschagrunert so the read+exec permission on all components of the path is the next thing we can try. Can you add some debugging info here, so we can see that? You can also check the go function I mentioned earlier, you can use it here maybe and see if it fixes the issue. |
432d3f7
to
a662a96
Compare
FYI: The blockers for idmap mount in runc
|
886e5c8
to
42e3896
Compare
I created a pr to bump k8s to the latest alpha: #1358 |
42e3896
to
457cebe
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.
Very nice work. Please ping when you do the check via the cri-api to see that the runtime supports userns or skip it otherwise
Thanks, I think we have to wait for kubernetes/kubernetes#123216 as you already mentioned. |
ea2f4b5
to
28c22c4
Compare
FYI: the CRI api landed via kubernetes/kubernetes#123356 |
We can wait for 1.30.0-alpha.3 (scheduled for Feb 27), vendor that in and then I'll rebase. |
0e07ac4
to
efb5f92
Compare
Adding user namespaces tests for covering the `UsernamespaceMode` supported by the CRI. Fixes kubernetes-sigs#1348 Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
efb5f92
to
4d1e304
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.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, saschagrunert The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adding user namespaces tests for covering the
UsernamespaceMode
supported by the CRI.Which issue(s) this PR fixes:
Fixes #1348
Special notes for your reviewer:
cc @giuseppe @rata
Does this PR introduce a user-facing change?