[release/1.5 backport] cri: filter selinux xattr for image volumes by dweomer · Pull Request #5104 · containerd/containerd

dweomer · 2021-03-01T21:32:13Z

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.

Backport of cri: filter selinux xattr for image volumes #5902
Fixes image volumes have incorrect selinux labels #5090 (for 1.5.x)
See [Backport] With rke2 as local management cluster, rancher pod is in CrashLoop state when selinux is enabled rancher/rke2#690

k8s-ci-robot · 2021-03-01T21:32:23Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

dweomer · 2021-03-01T21:34:29Z

@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

theopenlab-ci · 2021-03-01T21:39:51Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 6m 21s (non-voting)

dweomer · 2021-03-01T23:18:16Z

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.

dweomer · 2021-03-01T23:59:56Z

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, SELINUX=Enforcing vagrant up --provision-with=shell,selinux,test-integration runs successfully for me with:

vagrant 2.2.9
libvirt 6.0.0
ubuntu/focal 20.04.2

samuelkarp · 2021-03-02T02:04:01Z

/ok-to-test

samuelkarp

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

kzys · 2021-06-01T20:44:55Z

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 <[email protected]>

bcressey · 2021-08-12T23:32:26Z

@AkihiroSuda @fuweid Are we going to release the continuity package to have the change in a released version?

The xattr fix to continuity was in the v0.1.0 release, which came in before 1.5.0 from 3ef337a.

The change to pkg/cri/opts/container.go should be all that's still needed.

bcressey · 2021-08-13T15:49:07Z

The change to pkg/cri/opts/container.go should be all that's still needed.

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 <[email protected]>

The logging reduction patch is superseded by an upstream fix: containerd/containerd@1f5b84f27 The relabeling fix is not needed after excluding SELinux attributes when copying files for the volume: containerd/containerd#5104 Signed-off-by: Ben Cressey <[email protected]>

AkihiroSuda

Please open a PR for the main branch first and then cherry-pick to 1.5

AkihiroSuda · 2021-08-21T07:04:41Z

Let me mark as a draft until #5902 gets merged

dweomer · 2021-08-21T07:06:52Z

Please open a PR for the main branch first and then cherry-pick to 1.5

So, I did open #5902 because of course it belongs there. It replicates this PR. If a cherry-pick commit is necessary for process and/or tracing change (as per policy I assume) then I can surely do that here and force push one more time. I see some events alerting me that you are poking around. Please let me know if the cherry-pick commit here is required.

theopenlab-ci · 2021-08-21T07:52:20Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 3m 50s (non-voting)
containerd-test-arm64 : SUCCESS in 54m 25s (non-voting)
containerd-integration-test-arm64 : SUCCESS in 24m 18s (non-voting)

theopenlab-ci · 2021-08-25T06:11:43Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 17s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 22s (non-voting)
containerd-integration-test-arm64 : SUCCESS in 24m 14s (non-voting)

dweomer · 2021-08-25T08:45:51Z

/test pull-containerd-node-e2e

AkihiroSuda · 2021-08-25T08:47:37Z

Could you use git cherry-pick -xs so that the original commit hash appears in the cherry-pick commit message?

dweomer · 2021-08-25T09:16:13Z

Could you use git cherry-pick -xs so that the original commit hash appears in the cherry-pick commit message?

@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 <[email protected]> (cherry picked from commit c3609ff)

theopenlab-ci · 2021-08-25T09:54:37Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 3m 43s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 01s (non-voting)
containerd-integration-test-arm64 : FAILURE in 29m 22s (non-voting)

samuelkarp

LGTM

dweomer · 2021-09-29T16:17:59Z

While we do carry this for the k3s fork of containerd, we would like to get this merged. It is on main via #5902. Can we get this into a 1.5.x release?

theopenlab-ci · 2021-09-29T18:27:16Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 3m 55s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 25s (non-voting)
containerd-integration-test-arm64 : SUCCESS in 24m 47s (non-voting)

estesp

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 <[email protected]>

k8s-ci-robot added the needs-ok-to-test label Mar 1, 2021

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Mar 2, 2021

samuelkarp reviewed Mar 2, 2021

View reviewed changes

dweomer mentioned this pull request Mar 9, 2021

fix for image volumes under selinux k3s-io/containerd#10

Merged

dweomer mentioned this pull request Mar 9, 2021

fix when running rancher with selinux enforcing rancher/rke2#758

Merged

dweomer mentioned this pull request Mar 9, 2021

[release-1.19] fix when running rancher with selinux enforcing rancher/rke2#759

Merged

kzys added the status/needs-update Awaiting contributor update label Jun 1, 2021

AkihiroSuda requested changes Aug 21, 2021

View reviewed changes

AkihiroSuda marked this pull request as draft August 21, 2021 07:04

dweomer force-pushed the 1.5.0-rke2-690 branch from 5b3519c to 1ef4cb5 Compare August 25, 2021 05:45

dweomer changed the title ~~[release/1.5] cri: filter selinux xattr for image volumes~~ [release/1.5 backport] cri: filter selinux xattr for image volumes Aug 25, 2021

dweomer marked this pull request as ready for review August 25, 2021 05:47

dweomer requested a review from AkihiroSuda August 25, 2021 05:47

dweomer force-pushed the 1.5.0-rke2-690 branch from 1ef4cb5 to c0534c1 Compare August 25, 2021 09:18

AkihiroSuda approved these changes Aug 25, 2021

View reviewed changes

samuelkarp approved these changes Sep 1, 2021

View reviewed changes

samuelkarp removed the status/needs-update Awaiting contributor update label Sep 1, 2021

dcantah mentioned this pull request Sep 17, 2021

[WIP] Prepare release notes for v1.5.6 #6026

Closed

estesp closed this Sep 29, 2021

estesp reopened this Sep 29, 2021

estesp approved these changes Sep 29, 2021

View reviewed changes

dmcgowan approved these changes Oct 8, 2021

View reviewed changes

dmcgowan merged commit c41d458 into containerd:release/1.5 Oct 9, 2021

dweomer deleted the 1.5.0-rke2-690 branch July 27, 2024 06:52

Conversation

dweomer commented Mar 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 1, 2021

Uh oh!

dweomer commented Mar 1, 2021

Uh oh!

theopenlab-ci Bot commented Mar 1, 2021

Uh oh!

dweomer commented Mar 1, 2021

Uh oh!

dweomer commented Mar 1, 2021

Uh oh!

samuelkarp commented Mar 2, 2021

Uh oh!

samuelkarp left a comment

Choose a reason for hiding this comment

Uh oh!

kzys commented Jun 1, 2021

Uh oh!

bcressey commented Aug 12, 2021

Uh oh!

bcressey commented Aug 13, 2021

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Aug 21, 2021

Uh oh!

dweomer commented Aug 21, 2021

Uh oh!

theopenlab-ci Bot commented Aug 21, 2021

Uh oh!

theopenlab-ci Bot commented Aug 25, 2021

Uh oh!

dweomer commented Aug 25, 2021

Uh oh!

AkihiroSuda commented Aug 25, 2021

Uh oh!

dweomer commented Aug 25, 2021

Uh oh!

theopenlab-ci Bot commented Aug 25, 2021

Uh oh!

samuelkarp left a comment

Choose a reason for hiding this comment

Uh oh!

dweomer commented Sep 29, 2021

Uh oh!

theopenlab-ci Bot commented Sep 29, 2021

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dweomer commented Mar 1, 2021 •

edited

Loading