*: should align pipe's owner with init process #10906
Conversation
9ea1b2f to
8eebe2f
Compare
|
hmm. failed when using crun. checking |
2f01bba to
ef09f27
Compare
|
kindly ping @AkihiroSuda I can pass that new test in my local and in ubuntu github action env. However, it failed in vagrant box. I don't have env to support nested vm yet. Any thought about this? Is it related to selinux profile to block read on proc? Thanks |
ca18a1b to
c3fc548
Compare
|
kindly ping @AkihiroSuda @dmcgowan @samuelkarp it's ready for review. |
|
/retest |
| } | ||
| } | ||
|
|
||
| func TestIssue10598(t *testing.T) { |
There was a problem hiding this comment.
I guess you have verified this test fails before the patch, right? As there are several subtle things, I'd like to confirm this.
|
/retest |
|
But for some reason for other PRs doesn't seem to be failing. I can't see anything weird in the logs, just it didn't find a container in the kube-system namespace, the readiness is false. But the runtime is not using userns and the container seems running just fine. Anyone has any clues? |
|
/retest |
|
|
|
/retest |
261ad4e to
7b69b48
Compare
|
kindly ping @dmcgowan @AkihiroSuda @samuelkarp @rata |
rata
left a comment
There was a problem hiding this comment.
LGTM, thanks!
I've tested manually and this indeed solves the issue
| } | ||
| } | ||
|
|
||
| func TestIssue10598(t *testing.T) { |
There was a problem hiding this comment.
Could you add a comment and the issue link to explain what is being tested here?
There was a problem hiding this comment.
Updated. Please take a look.
7b69b48 to
4d35076
Compare
The containerd-shim creates pipes and passes them to the init container as stdin, stdout, and stderr for logging purposes. By default, these pipes are owned by the root user (UID/GID: 0/0). The init container can access them directly through inheritance. However, if the init container attempts to open any files pointing to these pipes (e.g., /proc/1/fd/2, /dev/stderr), it will encounter a permission issue since it is not the owner. To avoid this, we need to align the ownership of the pipes with the init process. Fixes: containerd#10598 Signed-off-by: Wei Fu <fuweid89@gmail.com>
4d35076 to
a21b178
Compare
|
/retest |
|
/cherrypick release/2.0 |
|
@fuweid: new pull request created: #11035 DetailsIn response to this:
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-sigs/prow repository. |
|
This change breaks runtimes which don't use runc's option format (like gVisor). Using WithUIDOwner/WithGIDOwner is only possible with a runc-style runtime, otherwise either the runtime returns an error (if that's the first option) or the function returns an error on task creation as it can only work on runc's option format. |
|
Hi @lorenz containerd uses runc/options as task option. IMO, it's API proto and the containerd client SDK is using this proto. Checked https://github.com/google/gvisor/blob/4971756d8d9b7b9f1954a1357348f3dd2b9d4a68/pkg/shim/runsc/service.go#L245. You could file an issue as well if you have any question. Thanks |
|
a) This is a breaking change inside the 2.0 release, this is IMO unacceptable to be released as is. It also affects pods not using user namespaces, so it doesn't even just break use cases enabled by this change. gVisor has a fully incompatible option concept, it's not just some minor changes/renames, they store fundamentally different things in it. |
For the spec, I don't think we make breaking change here. If you can take a look on release 1.6 (check the below url), we, containerd, move the proto to another folder. https://github.com/containerd/containerd/blob/release/1.6/runtime/v2/runc/options/oci.proto Checked that gvisor again https://github.com/google/gvisor/blob/master/g3doc/user_guide/containerd/configuration.md If you pass the Again, suggest file new issue to describe how to reproduce your issue. Thanks Edited. ping @lorenz just in case that miss the notification since this is closed pull request. |
This works with the 2.0 release, but stopped working after this CL.
The config I'm testing with already sets these options today as I need to set some gVisor-specific options. |
Could you please create issue for this? I will take a look with reproduce steps. thanks Edited. Ping @lorenz |
The containerd-shim creates pipes and passes them to the init container as stdin, stdout, and stderr for logging purposes. By default, these pipes are owned by the root user (UID/GID: 0/0). The init container can access them directly through inheritance.
However, if the init container attempts to open any files pointing to these pipes (e.g., /proc/1/fd/2, /dev/stderr), it will encounter a permission issue since it is not the owner. To avoid this, we need to align the ownership of the pipes with the init process.
Fixes: #10598