Skip to content

run: handle relabeling bind mounts ourselves #6132

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

run: handle relabeling bind mounts ourselves #6132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions .cirrus.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ env:
IN_PODMAN: 'false'
# root or rootless
PRIV_NAME: root
# default "mention the $BUILDAH_RUNTIME in the task alias" value
RUNTIME_N: ""

####
#### Cache-image names to test with
Expand Down Expand Up @@ -196,7 +198,7 @@ conformance_task:


integration_task:
name: "Integration $DISTRO_NV w/ $STORAGE_DRIVER"
name: "Integration $DISTRO_NV$RUNTIME_N w/ $STORAGE_DRIVER"
alias: integration
skip: *not_build_docs
depends_on: *smoke_vendor
Expand All @@ -207,6 +209,14 @@ integration_task:
DISTRO_NV: "${FEDORA_NAME}"
IMAGE_NAME: "${FEDORA_CACHE_IMAGE_NAME}"
STORAGE_DRIVER: 'vfs'
BUILDAH_RUNTIME: crun
RUNTIME_N: " using crun"
- env:
DISTRO_NV: "${FEDORA_NAME}"
IMAGE_NAME: "${FEDORA_CACHE_IMAGE_NAME}"
STORAGE_DRIVER: 'vfs'
BUILDAH_RUNTIME: runc
RUNTIME_N: " using runc"
Comment on lines +215 to +219
Copy link
Member

Choose a reason for hiding this comment

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

We have dropped all runc testing not to long ago in podman. It was a huge maintenance headache.

Every image update now has yet another big failure point which causes regressions. As the person who currently manages all the updates I am strongly against making my life harder with these things.

Copy link
Member Author

Choose a reason for hiding this comment

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

What failure point do you mean? I wouldn't expect that installing the package, which should already be available, would be more than a one-time change. Beyond that, I'm not clear on what it is that you think I'm asking you to take on here.

Copy link
Member

Choose a reason for hiding this comment

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

Almost every CI image update contains one or more regressions in our dependencies, kernel, systemd, golang, crun, glibc, well you get the point. By testing with runc every runc update is now a new potential failure point that then blocks image updates and requires me (or whoever does the image update) to do the work of investigating the test failure, reporting the bug and/or adding a work around or skip to test. Can that be done? Sure, but we decided over a year ago that we no longer want to test runc (containers/podman#22706) and I at least am not interested in doing a ton of extra work for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then those can be marked as not required for the "Total Success" check. But the idea of fixing this bug that runc exposes, and then not taking available steps to help ensure that we won't encounter something similar again, doesn't sit well with me.

Copy link
Member

Choose a reason for hiding this comment

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

Sure that would not block the image update but then you have a permanent red check until someone takes the time to fix it. My general experiences around such things have no been good at all as people just quickly ignore that then. And overall it does not remove the time someone has to invest to fix that.

So to me the question is do we want to spend time dealing with runc issues or not, as I remember the discussion a year ago the answer was no. If you think we should then we likely should discus that as a team again.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could have caught this bug earlier. If we need to have a conversation about that, then I think we should have the conversation.

# Disabled until we update to f41/42 as f40 does not have go 1.22
# - env:
# DISTRO_NV: "${PRIOR_FEDORA_NAME}"
Expand All @@ -221,6 +231,14 @@ integration_task:
DISTRO_NV: "${FEDORA_NAME}"
IMAGE_NAME: "${FEDORA_CACHE_IMAGE_NAME}"
STORAGE_DRIVER: 'overlay'
BUILDAH_RUNTIME: crun
RUNTIME_N: " using crun"
- env:
DISTRO_NV: "${FEDORA_NAME}"
IMAGE_NAME: "${FEDORA_CACHE_IMAGE_NAME}"
STORAGE_DRIVER: 'overlay'
BUILDAH_RUNTIME: runc
RUNTIME_N: " using runc"
# Disabled until we update to f41/42 as f40 does not have go 1.22
# - env:
# DISTRO_NV: "${PRIOR_FEDORA_NAME}"
Expand Down Expand Up @@ -255,7 +273,7 @@ integration_task:
golang_version_script: '$GOSRC/$SCRIPT_BASE/logcollector.sh golang'

integration_rootless_task:
name: "Integration rootless $DISTRO_NV w/ $STORAGE_DRIVER"
name: "Integration rootless $DISTRO_NV$RUNTIME_N w/ $STORAGE_DRIVER"
alias: integration_rootless
skip: *not_build_docs
depends_on: *smoke_vendor
Expand All @@ -268,6 +286,15 @@ integration_rootless_task:
IMAGE_NAME: "${FEDORA_CACHE_IMAGE_NAME}"
STORAGE_DRIVER: 'overlay'
PRIV_NAME: rootless
BUILDAH_RUNTIME: runc
RUNTIME_N: " using runc"
- env:
DISTRO_NV: "${FEDORA_NAME}"
IMAGE_NAME: "${FEDORA_CACHE_IMAGE_NAME}"
STORAGE_DRIVER: 'overlay'
PRIV_NAME: rootless
BUILDAH_RUNTIME: crun
RUNTIME_N: " using crun"
# Disabled until we update to f40/41 as f39 does not have go 1.22
# - env:
# DISTRO_NV: "${PRIOR_FEDORA_NAME}"
Expand Down
30 changes: 30 additions & 0 deletions run_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,33 @@ rootless=%d

defer b.cleanupTempVolumes()

// Handle mount flags that request that the source locations for "bind" mountpoints be
// relabeled, and filter those flags out of the list of mount options we pass to the
// runtime.
for i := range spec.Mounts {
switch spec.Mounts[i].Type {
default:
continue
case "bind", "rbind":
// all good, keep going
}
Comment on lines +550 to +555
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
switch spec.Mounts[i].Type {
default:
continue
case "bind", "rbind":
// all good, keep going
}
if switch spec.Mounts[i].Type == "bind" || switch spec.Mounts[i].Type == "rbind" {

Do you think if is easier for readability ? We are just handling one case.

zflag := ""
for _, opt := range spec.Mounts[i].Options {
if opt == "z" || opt == "Z" {
zflag = opt
}
}
if zflag == "" {
continue
}
spec.Mounts[i].Options = slices.DeleteFunc(spec.Mounts[i].Options, func(opt string) bool {
return opt == "z" || opt == "Z"
})
if err := relabel(spec.Mounts[i].Source, b.MountLabel, zflag == "z"); err != nil {
return fmt.Errorf("setting file label %q on %q: %w", b.MountLabel, spec.Mounts[i].Source, err)
}
}

switch isolation {
case define.IsolationOCI:
var moreCreateArgs []string
Expand Down Expand Up @@ -1139,16 +1166,19 @@ func (b *Builder) runSetupVolumeMounts(mountLabel string, volumeMounts []string,
if err := relabel(host, mountLabel, true); err != nil {
return specs.Mount{}, err
}
options = slices.DeleteFunc(options, func(o string) bool { return o == "z" })
}
if foundZ {
if err := relabel(host, mountLabel, false); err != nil {
return specs.Mount{}, err
}
options = slices.DeleteFunc(options, func(o string) bool { return o == "Z" })
}
if foundU {
if err := chown.ChangeHostPathOwnership(host, true, idMaps.processUID, idMaps.processGID); err != nil {
return specs.Mount{}, err
}
options = slices.DeleteFunc(options, func(o string) bool { return o == "U" })
}
if foundO {
if (upperDir != "" && workDir == "") || (workDir != "" && upperDir == "") {
Expand Down
12 changes: 6 additions & 6 deletions tests/run.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ function configure_and_check_user() {
zflag=
if which selinuxenabled > /dev/null 2> /dev/null ; then
if selinuxenabled ; then
zflag=z
zflag=,z
fi
fi
${OCI} --version
Expand All @@ -426,23 +426,23 @@ function configure_and_check_user() {
cid=$output
mkdir -p ${TEST_SCRATCH_DIR}/was:empty
# As a baseline, this should succeed.
run_buildah run --mount type=tmpfs,dst=/var/tmpfs-not-empty $cid touch /var/tmpfs-not-empty/testfile
run_buildah run --mount type=tmpfs,dst=/var/tmpfs-not-empty $cid touch /var/tmpfs-not-empty/testfile
# This should succeed, but the writes should effectively be discarded
run_buildah run --mount type=bind,src=${TEST_SCRATCH_DIR}/was:empty,dst=/var/not-empty,rw${zflag:+,${zflag}} $cid touch /var/not-empty/testfile
run_buildah run --mount type=bind,src=${TEST_SCRATCH_DIR}/was:empty,dst=/var/not-empty,rw${zflag} $cid touch /var/not-empty/testfile
if test -r ${TEST_SCRATCH_DIR}/was:empty/testfile ; then
die write to mounted type=bind was not discarded, ${TEST_SCRATCH_DIR}/was:empty/testfile exists
fi
# If we're parsing the options at all, this should be read-only, so it should fail.
run_buildah 1 run --mount type=bind,src=${TEST_SCRATCH_DIR}/was:empty,dst=/var/not-empty,ro${zflag:+,${zflag}} $cid touch /var/not-empty/testfile
run_buildah 1 run --mount type=bind,src=${TEST_SCRATCH_DIR}/was:empty,dst=/var/not-empty,ro${zflag} $cid touch /var/not-empty/testfile
# Even if the parent directory doesn't exist yet, this should succeed, but again the write should be discarded.
run_buildah run --mount type=bind,src=${TEST_SCRATCH_DIR}/was:empty,dst=/var/multi-level/subdirectory,rw${zflag:+,${zflag}} $cid touch /var/multi-level/subdirectory/testfile
run_buildah run --mount type=bind,src=${TEST_SCRATCH_DIR}/was:empty,dst=/var/multi-level/subdirectory,rw${zflag} $cid touch /var/multi-level/subdirectory/testfile
if test -r ${TEST_SCRATCH_DIR}/was:empty/testfile ; then
die write to mounted type=bind was not discarded, ${TEST_SCRATCH_DIR}/was:empty/testfile exists
fi
# And check the same for file volumes, which make life harder because the kernel's overlay
# filesystem really only wants to be dealing with directories.
: > ${TEST_SCRATCH_DIR}/was:empty/testfile
run_buildah run --mount type=bind,src=${TEST_SCRATCH_DIR}/was:empty/testfile,dst=/var/different-multi-level/subdirectory/testfile,rw${zflag:+,${zflag}} $cid sh -c 'echo wrote > /var/different-multi-level/subdirectory/testfile'
run_buildah run --mount type=bind,src=${TEST_SCRATCH_DIR}/was:empty/testfile,dst=/var/different-multi-level/subdirectory/testfile,rw${zflag} $cid sh -c 'echo wrote > /var/different-multi-level/subdirectory/testfile'
if test -s ${TEST_SCRATCH_DIR}/was:empty/testfile ; then
die write to mounted type=bind was not discarded, ${TEST_SCRATCH_DIR}/was:empty/testfile was written to
fi
Expand Down