Skip to content
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

Merged

Conversation

saschagrunert
Copy link
Member

@saschagrunert saschagrunert commented Feb 12, 2024

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?

Added user namespaces tests to `critest`. The tests try to find a working runtime handler and automatically skip if not existing.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 12, 2024
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 12, 2024
@saschagrunert saschagrunert force-pushed the user-namespaces-tests branch 2 times, most recently from 2a84587 to 3527be0 Compare February 12, 2024 13:25
@saschagrunert
Copy link
Member Author

Hm, I'm wondering why the containerd+runc tests fail, because they seem to work on my local env:

[FAILED] failed to create PodSandbox: rpc error: code = Unknown desc = failed to start sandbox "903e58f22a4f243faf832c1994a7e2d34ebb92159dec4723296ce28341a579e2": failed to create containerd task: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: error during container init: error preparing rootfs: mount /home/runner/work/cri-tools/cri-tools/tmp.1vM5EVKL2b/state/io.containerd.runtime.v2.task/k8s.io/903e58f22a4f243faf832c1994a7e2d34ebb92159dec4723296ce28341a579e2/rootfs:/home/runner/work/cri-tools/cri-tools/tmp.1vM5EVKL2b/state/io.containerd.runtime.v2.task/k8s.io/903e58f22a4f243faf832c1994a7e2d34ebb92159dec4723296ce28341a579e2/rootfs, flags: 0x5000: permission denied: unknown

I also assume that containerd v1.6 is not supported, so we may want to skip the tests for them.

@saschagrunert saschagrunert force-pushed the user-namespaces-tests branch 2 times, most recently from f3747eb to f0cbb20 Compare February 13, 2024 08:39
@saschagrunert
Copy link
Member Author

@mikebrow do we have any requirements for containerd to run the user namespaces tests? Version constraints for example?

cc @rata

@rata
Copy link
Contributor

rata commented Feb 14, 2024

@saschagrunert the most likely reason is a permission issue.

This smells like the rootfs is created inside a t.TmpDir() and golang creates that with permissions only to the owner, so no-one else can traverse the path or something like that? Is containerd running rootless?

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?

@mikebrow
Copy link
Contributor

@mikebrow do we have any requirements for containerd to run the user namespaces tests? Version constraints for example?

cc @rata

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..

@rata
Copy link
Contributor

rata commented Feb 14, 2024

Also, in the linked file there is a traversePath() function that might be handy to test if that is the issue.

@saschagrunert saschagrunert force-pushed the user-namespaces-tests branch 7 times, most recently from acc4855 to daf0749 Compare February 15, 2024 10:34
@saschagrunert
Copy link
Member Author

saschagrunert commented Feb 15, 2024

@saschagrunert the most likely reason is a permission issue.

This smells like the rootfs is created inside a t.TmpDir() and golang creates that with permissions only to the owner, so no-one else can traverse the path or something like that? Is containerd running rootless?

No, it's running as root:

sudo PATH=$PATH /usr/local/bin/containerd -a ${BDIR}/c.sock -root ${BDIR}/root -state ${BDIR}/state -log-level debug &> ${BDIR}/containerd-cri.log &

Which runc version is this using?

runc version 1.1.12
commit: v1.1.12-0-g51d5e946
spec: 1.0.2-dev
go: go1.21.7
libseccomp: 2.5.3

Do you face a similar error if you remove exec permissions for some dir in the path to the rootfs locally?

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:

time="2024-02-15T10:37:37.169555571Z" level=debug msg="serving api on socket" socket="[inherited from parent]"
time="2024-02-15T10:37:37.169590115Z" level=debug msg="starting signal loop" namespace=k8s.io path=/home/runner/work/cri-tools/cri-tools/tmp.xBdNncRfyZ/state/io.containerd.runtime.v2.task/k8s.io/4fd2091a07c23a02b8140021484ac7bd42d49bd390fb0bfe3a8d0322e6c8978f pid=24545 runtime=io.containerd.runc.v2
time="2024-02-15T10:37:38.253992446Z" level=debug msg="failed to delete task" error="rpc error: code = NotFound desc = container not created: not found" id=4fd2091a07c23a02b8140021484ac7bd42d49bd390fb0bfe3a8d0322e6c8978f
time="2024-02-15T10:37:38.254300658Z" level=info msg="shim disconnected" id=4fd2091a07c23a02b8140021484ac7bd42d49bd390fb0bfe3a8d0322e6c8978f namespace=k8s.io
time="2024-02-15T10:37:38.254328579Z" level=warning msg="cleaning up after shim disconnected" id=4fd2091a07c23a02b8140021484ac7bd42d49bd390fb0bfe3a8d0322e6c8978f namespace=k8s.io
time="2024-02-15T10:37:38.254345691Z" level=info msg="cleaning up dead shim" namespace=k8s.io
time="2024-02-15T10:37:38.264026692Z" level=warning msg="cleanup warnings time=\"2024-02-15T10:37:38Z\" level=debug msg=\"starting signal loop\" namespace=k8s.io pid=[245](https://github.com/kubernetes-sigs/cri-tools/actions/runs/7914561901/job/21604538082?pr=1354#step:21:246)75 runtime=io.containerd.runc.v2\ntime=\"2024-02-15T10:37:38Z\" level=warning msg=\"failed to read init pid file\" error=\"open /home/runner/work/cri-tools/cri-tools/tmp.xBdNncRfyZ/state/io.containerd.runtime.v2.task/k8s.io/4fd2091a07c23a02b8140021484ac7bd42d49bd390fb0bfe3a8d0322e6c8978f/init.pid: no such file or directory\" runtime=io.containerd.runc.v2\n" namespace=k8s.io
time="2024-02-15T10:37:38.264675518Z" level=error msg="copy shim log" error="read /proc/self/fd/21: file already closed" namespace=k8s.io
time="2024-02-15T10:37:38.266362131Z" level=error msg="RunPodSandbox for &PodSandboxMetadata{Name:user-namespaces-pod-0a5933a7-6ac4-4132-a00d-a2fea1b39e06,Uid:cri-test-uid62fdfb09-009c-445a-87ea-1969fed1eb12,Namespace:cri-test-namespace13a4f136-1533-47cc-822f-18f9a12e5267,Attempt:2,} failed, error" error="failed to start sandbox \"4fd2091a07c23a02b8140021484ac7bd42d49bd390fb0bfe3a8d0322e6c8978f\": failed to create containerd task: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: error during container init: error preparing rootfs: mount /home/runner/work/cri-tools/cri-tools/tmp.xBdNncRfyZ/state/io.containerd.runtime.v2.task/k8s.io/4fd2091a07c23a02b8140021484ac7bd42d49bd390fb0bfe3a8d0322e6c8978f/rootfs:/home/runner/work/cri-tools/cri-tools/tmp.xBdNncRfyZ/state/io.containerd.runtime.v2.task/k8s.io/4fd2091a07c23a02b8140021484ac7bd42d49bd390fb0bfe3a8d0322e6c8978f/rootfs, flags: 0x5000: permission denied: unknown"

@rata
Copy link
Contributor

rata commented Feb 15, 2024

@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.

@saschagrunert saschagrunert force-pushed the user-namespaces-tests branch 4 times, most recently from 432d3f7 to a662a96 Compare February 16, 2024 08:44
@utam0k
Copy link
Member

utam0k commented Feb 17, 2024

FYI: The blockers for idmap mount in runc
opencontainers/runc#4114
opencontainers/runc#3963

The error you hit now is that we require a runc version that supports idmap mounts (from the error message: OCI runtime doesn't support idmap mounts). There is currently no runc release that has this functionality (the upcoming 1.2 release will have it, but unsure when that will be released). But crun supports it for some time already.

@saschagrunert saschagrunert force-pushed the user-namespaces-tests branch 2 times, most recently from 886e5c8 to 42e3896 Compare February 19, 2024 10:28
@saschagrunert
Copy link
Member Author

I created a pr to bump k8s to the latest alpha: #1358

Copy link
Contributor

@rata rata left a 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

.github/workflows/containerd.yml Outdated Show resolved Hide resolved
@saschagrunert
Copy link
Member Author

saschagrunert commented Feb 21, 2024

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.

@saschagrunert saschagrunert force-pushed the user-namespaces-tests branch 6 times, most recently from ea2f4b5 to 28c22c4 Compare February 21, 2024 08:59
@rata
Copy link
Contributor

rata commented Feb 22, 2024

FYI: the CRI api landed via kubernetes/kubernetes#123356

@saschagrunert
Copy link
Member Author

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.

@saschagrunert saschagrunert force-pushed the user-namespaces-tests branch 3 times, most recently from 0e07ac4 to efb5f92 Compare February 29, 2024 09:32
Adding user namespaces tests for covering the `UsernamespaceMode`
supported by the CRI.

Fixes kubernetes-sigs#1348

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
@saschagrunert saschagrunert changed the title WIP: Add user namespaces tests Add user namespaces tests Feb 29, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 29, 2024
@saschagrunert
Copy link
Member Author

Ready for review. PTAL @rata @giuseppe @kubernetes-sigs/cri-tools-maintainers

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 29, 2024
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 3a1f73d into kubernetes-sigs:master Feb 29, 2024
23 checks passed
@saschagrunert saschagrunert deleted the user-namespaces-tests branch February 29, 2024 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

critest should cover UserNamespace
6 participants