feat: Allow containers to use both host network and user namespace#12518
feat: Allow containers to use both host network and user namespace#12518AkihiroSuda merged 3 commits intocontainerd:mainfrom
Conversation
23b8502 to
da87ae8
Compare
da87ae8 to
4043d30
Compare
|
Hello! see: " FAIL - does not have a valid DCO" All commits must be signed. Suggest setting your git config to have user name and email.. Then git commit -s --amend to sign your commit." Detail: https://github.com/containerd/project/blob/main/CONTRIBUTING.md#sign-your-work |
|
note to self.. needs feature issue for tracking support of the case. |
4043d30 to
97c1095
Compare
Signed |
rata
left a comment
There was a problem hiding this comment.
Oh, I missed this PR was open, sorry! Thanks a lot for the PR.
The code looks good, simple and effective. I've tested it locally and works as expected.
I've revisited #10607 to make sure I wasn't forgetting any reason to pin the userns. But no, the pinning is just needed because the netns is created by containerd and, therefore, the userns needs to be created by containerd (since that PR, before we were letting runc create it for us).
So, when no netns is created (using the host netns), the pinning is not needed. So, just skping it and adding the /sys bind mount works just fine, with both runc and crun. This is a quite an elegant solution :)
IMHO now is safe to add tests, as everything works and IMHO looks good. For that, please check TestLinuxSandboxContainerSpec() and TestLinuxSandboxContainerSpec() to test the changes in these two functions. There are quite a bunch of userns tests you can check. For the not pinned userns, see the specCheck func in the user namespace test case. You can use something similar but with assert.NotContains, for example.
Please ping me when tests are ready and thanks again for the PR! :)
97c1095 to
5c45615
Compare
|
Also, can you update the PR description? The runc pr is not needed. |
5c45615 to
9f843b3
Compare
updated.
Unit tests have been added. |
9f843b3 to
89b5612
Compare
rata
left a comment
There was a problem hiding this comment.
Thanks!
Sorry, my previous suggestion was wrong. Let's change back that code :-/
89b5612 to
114fd58
Compare
rata
left a comment
There was a problem hiding this comment.
This mostly LGTM, left some comments on the tests (on the existing threads we had).
114fd58 to
200b05f
Compare
I have recommited it |
|
@HirazawaUi CI is still broken :( |
8317fba to
76bbb4b
Compare
|
@mikebrow all tests have passed. Could you please approve this PR? |
76bbb4b to
50e372b
Compare
|
CI is broken (windows, so unrelated). Can you push again? |
|
/retest (you can do this too, it should rerun the failing GitHub Actions workflows) |
|
Oh, I missed containerd suppoted this! Cool, CI is green now, PTAL :) |
Signed-off-by: HirazawaUi <695097494plus@gmail.com>
Signed-off-by: HirazawaUi <695097494plus@gmail.com>
Signed-off-by: HirazawaUi <695097494plus@gmail.com>
50e372b to
339b0cc
Compare
This PR implements the feature proposed in KEP: kubernetes/enhancements#5607 for containerd.
This PR modifies the behavior to use bind mounts for /sys when a pod employs both hostNetwork and user namespace.
relate: #12489