Don't write whiteout files to directories that were replaced with files or links (#2590)

If a non-empty directory gets replaced with something other than a
directory (e.g. file or symlink), the files in that directory also get
deleted. However, there should not be any whiteout files for them in the
layer. If there were, the resulting tar file would not be extractable.

Fixes #2308

integration/dockerfiles/TestReplaceFolderWithFile

@@ -0,0 +1,5 @@
+# Not prefixed Dockerfile_test to exclude it from TestRun()
+FROM busybox
+
+RUN mkdir /a /b /c && echo a > /a/d
+RUN rm -r /a && echo "foo" > /a

integration/dockerfiles/TestReplaceFolderWithLink

@@ -0,0 +1,5 @@
+# Not prefixed Dockerfile_test to exclude it from TestRun()
+FROM busybox
+
+RUN mkdir /a /b /c && echo a > /a/d
+RUN rm -r /a && ln -sf /b /a

integration/integration_test.go

@@ -17,10 +17,12 @@ limitations under the License.
 package integration
 
 import (
+	"archive/tar"
 	"context"
 	"encoding/json"
 	"flag"
 	"fmt"
+	"io"
 	"log"
 	"math"
 	"os"
@@ -33,6 +35,7 @@ import (
 
 	"github.com/google/go-containerregistry/pkg/name"
 	"github.com/google/go-containerregistry/pkg/v1/daemon"
+	"github.com/google/go-containerregistry/pkg/v1/remote"
 	"github.com/pkg/errors"
 	"google.golang.org/api/option"
 
@@ -549,6 +552,28 @@ func TestLayers(t *testing.T) {
 	}
 }
 
+func TestReplaceFolderWithFileOrLink(t *testing.T) {
+	dockerfiles := []string{"TestReplaceFolderWithFile", "TestReplaceFolderWithLink"}
+	for _, dockerfile := range dockerfiles {
+		t.Run(dockerfile, func(t *testing.T) {
+			buildImage(t, dockerfile, imageBuilder)
+			kanikoImage := GetKanikoImage(config.imageRepo, dockerfile)
+
+			kanikoFiles, err := getLastLayerFiles(kanikoImage)
+			if err != nil {
+				t.Fatal(err)
+			}
+			fmt.Println(kanikoFiles)
+
+			for _, file := range kanikoFiles {
+				if strings.HasPrefix(file, "a/.wh.") {
+					t.Errorf("Last layer should not add whiteout files to deleted directory but found %s", file)
+				}
+			}
+		})
+	}
+}
+
 func buildImage(t *testing.T, dockerfile string, imageBuilder *DockerFileBuilder) {
 	t.Logf("Building image '%v'...", dockerfile)
 
@@ -868,6 +893,40 @@ func getImageDetails(image string) (*imageDetails, error) {
 	}, nil
 }
 
+func getLastLayerFiles(image string) ([]string, error) {
+	ref, err := name.ParseReference(image, name.WeakValidation)
+	if err != nil {
+		return nil, fmt.Errorf("Couldn't parse referance to image %s: %w", image, err)
+	}
+
+	imgRef, err := remote.Image(ref)
+	if err != nil {
+		return nil, fmt.Errorf("Couldn't get reference to image %s from daemon: %w", image, err)
+	}
+	layers, err := imgRef.Layers()
+	if err != nil {
+		return nil, fmt.Errorf("Error getting layers for image %s: %w", image, err)
+	}
+	readCloser, err := layers[len(layers)-1].Uncompressed()
+	if err != nil {
+		return nil, err
+	}
+
+	tr := tar.NewReader(readCloser)
+	var files []string
+	for {
+		hdr, err := tr.Next()
+		if errors.Is(err, io.EOF) {
+			break
+		}
+		if err != nil {
+			return nil, err
+		}
+		files = append(files, hdr.Name)
+	}
+	return files, nil
+}
+
 func logBenchmarks(benchmark string) error {
 	if b, err := strconv.ParseBool(os.Getenv("BENCHMARK")); err == nil && b {
 		f, err := os.Create(benchmark)

pkg/snapshot/snapshot.go

@@ -224,6 +224,14 @@ func writeToTar(t util.Tar, files, whiteouts []string) error {
 	addedPaths := make(map[string]bool)
 
 	for _, path := range whiteouts {
+		skipWhiteout, err := parentPathIncludesNonDirectory(path)
+		if err != nil {
+			return err
+		}
+		if skipWhiteout {
+			continue
+		}
+
 		if err := addParentDirectories(t, addedPaths, path); err != nil {
 			return err
 		}
@@ -247,6 +255,21 @@ func writeToTar(t util.Tar, files, whiteouts []string) error {
 	return nil
 }
 
+// Returns true if a parent of the given path has been replaced with anything other than a directory
+func parentPathIncludesNonDirectory(path string) (bool, error) {
+	for _, parentPath := range util.ParentDirectories(path) {
+		lstat, err := os.Lstat(parentPath)
+		if err != nil {
+			return false, err
+		}
+
+		if !lstat.IsDir() {
+			return true, nil
+		}
+	}
+	return false, nil
+}
+
 func addParentDirectories(t util.Tar, addedPaths map[string]bool, path string) error {
 	for _, parentPath := range util.ParentDirectories(path) {
 		if _, pathAdded := addedPaths[parentPath]; pathAdded {

pkg/snapshot/snapshot_test.go

@@ -189,6 +189,51 @@ func TestSnapshotFSChangePermissions(t *testing.T) {
 	}
 }
 
+func TestSnapshotFSReplaceDirWithLink(t *testing.T) {
+	testDir, snapshotter, cleanup, err := setUpTest(t)
+	defer cleanup()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// replace non-empty directory "bar" with link to file "foo"
+	bar := filepath.Join(testDir, "bar")
+	err = os.RemoveAll(bar)
+	if err != nil {
+		t.Fatal(err)
+	}
+	foo := filepath.Join(testDir, "foo")
+	err = os.Symlink(foo, bar)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	tarPath, err := snapshotter.TakeSnapshotFS()
+	if err != nil {
+		t.Fatalf("Error taking snapshot of fs: %s", err)
+	}
+
+	actualFiles, err := listFilesInTar(tarPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Expect "bar", which used to be a non-empty directory but now is a symlink. We don't want whiteout files for
+	// the deleted files in bar, because without a parent directory for them the tar cannot be extracted.
+	testDirWithoutLeadingSlash := strings.TrimLeft(testDir, "/")
+	expectedFiles := []string{
+		filepath.Join(testDirWithoutLeadingSlash, "bar"),
+		filepath.Join(testDirWithoutLeadingSlash, "foo"),
+	}
+	for _, path := range util.ParentDirectoriesWithoutLeadingSlash(filepath.Join(testDir, "foo")) {
+		expectedFiles = append(expectedFiles, strings.TrimRight(path, "/")+"/")
+	}
+
+	sort.Strings(expectedFiles)
+	sort.Strings(actualFiles)
+	testutil.CheckErrorAndDeepEqual(t, false, nil, expectedFiles, actualFiles)
+}
+
 func TestSnapshotFiles(t *testing.T) {
 	testDir, snapshotter, cleanup, err := setUpTest(t)
 	testDirWithoutLeadingSlash := strings.TrimLeft(testDir, "/")