From 2fed5fc912e707823d0582a54fdb41c28f2f9c34 Mon Sep 17 00:00:00 2001 From: Aditya R Date: Thu, 4 May 2023 11:25:16 +0530 Subject: [PATCH] run,mount: remove path only if they didnt pre-exist It seems buildah was cleaning path after unmounting the content which was added by `--mount`, however buildah should `remove` only if the path it did not pre-exist otherwise it should just simply `unmount`. Following behaviour aligns buildah to buildkit Closes: https://github.com/containers/buildah/issues/4739 Signed-off-by: Aditya R --- run_common.go | 72 ++++++++++++++++++++-------- tests/bud.bats | 14 ++++++ tests/bud/verify-cleanup/Dockerfile | 24 ++++++++++ tests/bud/verify-cleanup/hey | 1 + tests/bud/verify-cleanup/secret1.txt | 1 + 5 files changed, 91 insertions(+), 21 deletions(-) create mode 100644 tests/bud/verify-cleanup/Dockerfile create mode 100644 tests/bud/verify-cleanup/hey create mode 100644 tests/bud/verify-cleanup/secret1.txt diff --git a/run_common.go b/run_common.go index bcfd70e1a77..dfab80855de 100644 --- a/run_common.go +++ b/run_common.go @@ -1328,7 +1328,7 @@ func (b *Builder) setupMounts(mountPoint string, spec *specs.Spec, bundlePath st processGID: int(processGID), } // Get the list of mounts that are just for this Run() call. - runMounts, mountArtifacts, err := b.runSetupRunMounts(runFileMounts, runMountInfo, idMaps) + runMounts, mountArtifacts, err := b.runSetupRunMounts(mountPoint, runFileMounts, runMountInfo, idMaps) if err != nil { return nil, err } @@ -1464,10 +1464,28 @@ func cleanableDestinationListFromMounts(mounts []spec.Mount) []string { return mountDest } +func checkIfMountDestinationPreExists(root string, dest string) (bool, error) { + statResults, err := copier.Stat(root, "", copier.StatOptions{}, []string{dest}) + if err != nil { + return false, err + } + if len(statResults) > 0 { + // We created exact path for globbing so it will + // return only one result. + if statResults[0].Error != "" && len(statResults[0].Globbed) == 0 { + // Path do not exsits. + return false, nil + } + // Path exists. + return true, nil + } + return false, nil +} + // runSetupRunMounts sets up mounts that exist only in this RUN, not in subsequent runs // // If this function succeeds, the caller must unlock runMountArtifacts.TargetLocks (when??) -func (b *Builder) runSetupRunMounts(mounts []string, sources runMountInfo, idMaps IDMaps) ([]spec.Mount, *runMountArtifacts, error) { +func (b *Builder) runSetupRunMounts(mountPoint string, mounts []string, sources runMountInfo, idMaps IDMaps) ([]spec.Mount, *runMountArtifacts, error) { // If `type` is not set default to TypeBind mountType := define.TypeBind mountTargets := make([]string, 0, 10) @@ -1485,6 +1503,11 @@ func (b *Builder) runSetupRunMounts(mounts []string, sources runMountInfo, idMap } }() for _, mount := range mounts { + var mountSpec *spec.Mount + var err error + var envFile, image string + var agent *sshagent.AgentServer + var tl *lockfile.LockFile tokens := strings.Split(mount, ",") for _, field := range tokens { if strings.HasPrefix(field, "type=") { @@ -1497,63 +1520,71 @@ func (b *Builder) runSetupRunMounts(mounts []string, sources runMountInfo, idMap } switch mountType { case "secret": - mount, envFile, err := b.getSecretMount(tokens, sources.Secrets, idMaps, sources.WorkDir) + mountSpec, envFile, err = b.getSecretMount(tokens, sources.Secrets, idMaps, sources.WorkDir) if err != nil { return nil, nil, err } - if mount != nil { - finalMounts = append(finalMounts, *mount) - mountTargets = append(mountTargets, mount.Destination) + if mountSpec != nil { + finalMounts = append(finalMounts, *mountSpec) if envFile != "" { tmpFiles = append(tmpFiles, envFile) } } case "ssh": - mount, agent, err := b.getSSHMount(tokens, sshCount, sources.SSHSources, idMaps) + mountSpec, agent, err = b.getSSHMount(tokens, sshCount, sources.SSHSources, idMaps) if err != nil { return nil, nil, err } - if mount != nil { - finalMounts = append(finalMounts, *mount) - mountTargets = append(mountTargets, mount.Destination) + if mountSpec != nil { + finalMounts = append(finalMounts, *mountSpec) agents = append(agents, agent) if sshCount == 0 { - defaultSSHSock = mount.Destination + defaultSSHSock = mountSpec.Destination } // Count is needed as the default destination of the ssh sock inside the container is /run/buildkit/ssh_agent.{i} sshCount++ } case define.TypeBind: - mount, image, err := b.getBindMount(tokens, sources.SystemContext, sources.ContextDir, sources.StageMountPoints, idMaps, sources.WorkDir) + mountSpec, image, err = b.getBindMount(tokens, sources.SystemContext, sources.ContextDir, sources.StageMountPoints, idMaps, sources.WorkDir) if err != nil { return nil, nil, err } - finalMounts = append(finalMounts, *mount) - mountTargets = append(mountTargets, mount.Destination) + finalMounts = append(finalMounts, *mountSpec) // only perform cleanup if image was mounted ignore everything else if image != "" { mountImages = append(mountImages, image) } case "tmpfs": - mount, err := b.getTmpfsMount(tokens, idMaps) + mountSpec, err = b.getTmpfsMount(tokens, idMaps) if err != nil { return nil, nil, err } - finalMounts = append(finalMounts, *mount) - mountTargets = append(mountTargets, mount.Destination) + finalMounts = append(finalMounts, *mountSpec) case "cache": - mount, tl, err := b.getCacheMount(tokens, sources.StageMountPoints, idMaps, sources.WorkDir) + mountSpec, tl, err = b.getCacheMount(tokens, sources.StageMountPoints, idMaps, sources.WorkDir) if err != nil { return nil, nil, err } - finalMounts = append(finalMounts, *mount) - mountTargets = append(mountTargets, mount.Destination) + finalMounts = append(finalMounts, *mountSpec) if tl != nil { targetLocks = append(targetLocks, tl) } default: return nil, nil, fmt.Errorf("invalid mount type %q", mountType) } + + if mountSpec != nil { + pathPreExists, err := checkIfMountDestinationPreExists(mountPoint, mountSpec.Destination) + if err != nil { + return nil, nil, err + } + if !pathPreExists { + // In such case it means that the path did not exists before + // creating any new mounts therefore we must clean the newly + // created directory after this step. + mountTargets = append(mountTargets, mountSpec.Destination) + } + } } succeeded = true artifacts := &runMountArtifacts{ @@ -1876,7 +1907,6 @@ func (b *Builder) cleanupRunMounts(context *imageTypes.SystemContext, mountpoint return err } } - opts := copier.RemoveOptions{ All: true, } diff --git a/tests/bud.bats b/tests/bud.bats index 74826dde2f5..f50bb516579 100644 --- a/tests/bud.bats +++ b/tests/bud.bats @@ -5705,6 +5705,20 @@ _EOF run_buildah rmi -f testbud } +@test "bud-verify-if-we-dont-clean-prexisting-path" { + skip_if_no_runtime + skip_if_in_container + run_buildah 1 build -t testbud $WITH_POLICY_JSON --secret id=secret-foo,src=$BUDFILES/verify-cleanup/secret1.txt -f $BUDFILES/verify-cleanup/Dockerfile $BUDFILES/verify-cleanup/ + expect_output --substring "hello" + expect_output --substring "secrettext" + expect_output --substring "Directory /tmp exists." + expect_output --substring "Directory /var/tmp exists." + expect_output --substring "Directory /testdir DOES NOT exists." + expect_output --substring "Cache Directory /cachedir DOES NOT exists." + expect_output --substring "Secret File secret1.txt DOES NOT exists." + expect_output --substring "/tmp/hey: No such file or directory" +} + @test "bud-with-mount-with-tmpfs-like-buildkit" { skip_if_no_runtime skip_if_in_container diff --git a/tests/bud/verify-cleanup/Dockerfile b/tests/bud/verify-cleanup/Dockerfile new file mode 100644 index 00000000000..f2d20cd9e91 --- /dev/null +++ b/tests/bud/verify-cleanup/Dockerfile @@ -0,0 +1,24 @@ +FROM alpine as builder +RUN mkdir subdir +COPY hey . + +FROM docker.io/library/debian:testing-slim +RUN --mount=type=bind,source=.,dst=/tmp,z \ + --mount=type=tmpfs,dst=/var/tmp \ + cat /tmp/hey +RUN --mount=type=cache,from=builder,target=/cachedir cat /cachedir/hey +RUN --mount=type=secret,id=secret-foo,dst=secret1.txt cat secret1.txt +ARG TMP="/tmp" +ARG VARTMP="/var/tmp" +ARG CACHEDIR="/cachedir" +ARG TESTDIR="/testdir" +ARG SECRETFILE="secret1.txt" +RUN [ -d "/tmp" ] && echo "Directory $TMP exists." +RUN [ -d "/var/tmp" ] && echo "Directory $VARTMP exists." + +#Following path should not exists after the --mount step +RUN [ ! -d "/testdir" ] && echo "Directory $TESTDIR DOES NOT exists." +RUN [ ! -d "/cachedir" ] && echo "Cache Directory $CACHEDIR DOES NOT exists." +RUN [ ! -f "secret1.txt" ] && echo "Secret File $SECRETFILE DOES NOT exists." +# This should fail +RUN cat /tmp/hey diff --git a/tests/bud/verify-cleanup/hey b/tests/bud/verify-cleanup/hey new file mode 100644 index 00000000000..ce013625030 --- /dev/null +++ b/tests/bud/verify-cleanup/hey @@ -0,0 +1 @@ +hello diff --git a/tests/bud/verify-cleanup/secret1.txt b/tests/bud/verify-cleanup/secret1.txt new file mode 100644 index 00000000000..e05f3661002 --- /dev/null +++ b/tests/bud/verify-cleanup/secret1.txt @@ -0,0 +1 @@ +secrettext