Merge pull request #4755 from flouthoc/proper-cleanup-after-run-step

run,mount: remove path only if they didnt pre-exist

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,
 	}

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

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

tests/bud/verify-cleanup/hey

@@ -0,0 +1 @@
+hello

tests/bud/verify-cleanup/secret1.txt

@@ -0,0 +1 @@
+secrettext