-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
[release/1.5 backport] cri: filter selinux xattr for image volumes #5104
Conversation
Hi @dweomer. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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/test-infra repository. |
@dmcgowan @AkihiroSuda @mikebrow: I've submitted this is a draft/WIP because it depends on the as-of-yet not merged containerd/continuity#178. Once it is merged I will update this PR. /cc @samuelkarp |
Build succeeded.
|
This SELinux/Integration failure, https://github.com/containerd/containerd/pull/5104/checks?check_run_id=2007903504, seems unrelated to this change but I will dig a bit further. |
Seems like a flake,
|
/ok-to-test |
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.
This LGTM pending the merge into continuity and removing the replace
in go.mod
.
Pull in a fix for containerd/cri that pulls in a fix for containerd/continuity: when copying directories for image volumes we should filter out the security.selinux xattr key because the target has been correctly relabeled already (and image volumes are copying from layer storage which has a read-only selinux context). Upstream PR(s) for 1.5.x: - containerd/continuity#178 - containerd#5104 Addresses: - rancher/rke2#690 Signed-off-by: Jacob Blain Christen <jacob@rancher.com>
Originally reported at rancher#690 against a v1.19.7 beta pre-release, there is an issue with containerd versions 1.4+ that prevented the correct selinux labels from being applied for image volumes (volumes declared in the docker image that containerd/cri will set up for you by default ... but they aren't visible to k8s). Patches to fix this have been submitted upstream, see: - containerd/containerd#5090 - containerd/containerd#5104 - containerd/continuity#178 Signed-off-by: Jacob Blain Christen <jacob@rancher.com>
Originally reported at rancher#690 against a v1.19.7 beta pre-release, there is an issue with containerd versions 1.4+ that prevented the correct selinux labels from being applied for image volumes (volumes declared in the docker image that containerd/cri will set up for you by default ... but they aren't visible to k8s). Patches to fix this have been submitted upstream, see: - containerd/containerd#5090 - containerd/containerd#5104 - containerd/continuity#178 Signed-off-by: Jacob Blain Christen <jacob@rancher.com>
Originally reported at rancher#690 against a v1.19.7 beta pre-release, there is an issue with containerd versions 1.4+ that prevented the correct selinux labels from being applied for image volumes (volumes declared in the docker image that containerd/cri will set up for you by default ... but they aren't visible to k8s). Patches to fix this have been submitted upstream, see: - containerd/containerd#5090 - containerd/containerd#5104 - containerd/continuity#178 Signed-off-by: Jacob Blain Christen <jacob@rancher.com>
Originally reported at #690 against a v1.19.7 beta pre-release, there is an issue with containerd versions 1.4+ that prevented the correct selinux labels from being applied for image volumes (volumes declared in the docker image that containerd/cri will set up for you by default ... but they aren't visible to k8s). Patches to fix this have been submitted upstream, see: - containerd/containerd#5090 - containerd/containerd#5104 - containerd/continuity#178 Signed-off-by: Jacob Blain Christen <jacob@rancher.com>
Originally reported at rancher#690 against a v1.19.7 beta pre-release, there is an issue with containerd versions 1.4+ that prevented the correct selinux labels from being applied for image volumes (volumes declared in the docker image that containerd/cri will set up for you by default ... but they aren't visible to k8s). Patches to fix this have been submitted upstream, see: - containerd/containerd#5090 - containerd/containerd#5104 - containerd/continuity#178 Addresses rancher#690 Signed-off-by: Jacob Blain Christen <jacob@rancher.com>
Originally reported at #690 against a v1.19.7 beta pre-release, there is an issue with containerd versions 1.4+ that prevented the correct selinux labels from being applied for image volumes (volumes declared in the docker image that containerd/cri will set up for you by default ... but they aren't visible to k8s). Patches to fix this have been submitted upstream, see: - containerd/containerd#5090 - containerd/containerd#5104 - containerd/continuity#178 Addresses #690 Signed-off-by: Jacob Blain Christen <jacob@rancher.com>
Pull in a fix for containerd/cri that pulls in a fix for containerd/continuity: when copying directories for image volumes we should filter out the security.selinux xattr key because the target has been correctly relabeled already (and image volumes are copying from layer storage which has a read-only selinux context). Upstream PR(s) for 1.5.x: - containerd/continuity#178 - containerd#5104 Addresses: - rancher/rke2#690 Signed-off-by: Jacob Blain Christen <jacob@rancher.com>
containerd/continuity#178 has been merged! @dweomer Can you update the PR to remove the replace directive? @AkihiroSuda @fuweid Are we going to release the continuity package to have the change in a released version? |
Pull in a fix for containerd/cri that pulls in a fix for containerd/continuity: when copying directories for image volumes we should filter out the security.selinux xattr key because the target has been correctly relabeled already (and image volumes are copying from layer storage which has a read-only selinux context). Upstream PR(s) for 1.5.x: - containerd/continuity#178 - containerd#5104 Addresses: - rancher/rke2#690 Signed-off-by: Jacob Blain Christen <jacob@rancher.com>
Pull in a fix for containerd/cri that pulls in a fix for containerd/continuity: when copying directories for image volumes we should filter out the security.selinux xattr key because the target has been correctly relabeled already (and image volumes are copying from layer storage which has a read-only selinux context). Upstream PR(s) for 1.5.x: - containerd/continuity#178 - containerd#5104 Addresses: - rancher/rke2#690 Signed-off-by: Jacob Blain Christen <jacob@rancher.com>
The xattr fix to continuity was in the v0.1.0 release, which came in before 1.5.0 from 3ef337a. The change to |
I've tested a patch with just this change locally and confirmed that it fixes labeling for image volumes. |
Pull in a fix for containerd/cri that pulls in a fix for containerd/continuity: when copying directories for image volumes we should filter out the security.selinux xattr key because the target has been correctly relabeled already (and image volumes are copying from layer storage which has a read-only selinux context). Upstream PR(s) for 1.5.x: - containerd/continuity#178 - containerd#5104 Addresses: - rancher/rke2#690 Signed-off-by: Jacob Blain Christen <jacob@rancher.com>
Build succeeded.
|
5b3519c
to
1ef4cb5
Compare
Build succeeded.
|
/test pull-containerd-node-e2e |
Could you use |
@AkihiroSuda thanks, I was looking for the magic that a cursory search through the contributing docs didn't make obvious for me. |
Exclude the `security.selinux` xattr when copying content from layer storage for image volumes. This allows for the already correct label at the target location to be applied to the copied content, thus enabling containers to write to volumes that they implicitly expect to be able to write to. - Fixes containerd#5090 for 1.5.x - See rancher/rke2#690 Signed-off-by: Jacob Blain Christen <jacob@rancher.com> (cherry picked from commit c3609ff)
1ef4cb5
to
c0534c1
Compare
Build succeeded.
|
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
While we do carry this for the k3s fork of containerd, we would like to get this merged. It is on |
Build succeeded.
|
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
Pull in a fix for containerd/cri that pulls in a fix for containerd/continuity: when copying directories for image volumes we should filter out the security.selinux xattr key because the target has been correctly relabeled already (and image volumes are copying from layer storage which has a read-only selinux context). Upstream PR(s) for 1.5.x: - containerd/continuity#178 - containerd#5104 Addresses: - rancher/rke2#690 Signed-off-by: Jacob Blain Christen <jacob@rancher.com>
Pull in a fix for containerd/cri that pulls in a fix for containerd/continuity: when copying directories for image volumes we should filter out the security.selinux xattr key because the target has been correctly relabeled already (and image volumes are copying from layer storage which has a read-only selinux context). Upstream PR(s) for 1.5.x: - containerd/continuity#178 - containerd#5104 Addresses: - rancher/rke2#690 Signed-off-by: Jacob Blain Christen <jacob@rancher.com>
Exclude the
security.selinux
xattr when copying content from layerstorage for image volumes. This allows for the already correct label at
the target location to be applied to the copied content, thus enabling
containers to write to volumes that they implicitly expect to be able to
write to.