From 0d925dd651104601b7ef64347c7c5b8d266017cb Mon Sep 17 00:00:00 2001 From: Andreas Fleig Date: Thu, 22 Jun 2023 19:45:47 +0200 Subject: [PATCH] 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 --- .../dockerfiles/TestReplaceFolderWithFile | 5 ++ .../dockerfiles/TestReplaceFolderWithLink | 5 ++ integration/integration_test.go | 59 +++++++++++++++++++ pkg/snapshot/snapshot.go | 23 ++++++++ pkg/snapshot/snapshot_test.go | 45 ++++++++++++++ 5 files changed, 137 insertions(+) create mode 100644 integration/dockerfiles/TestReplaceFolderWithFile create mode 100644 integration/dockerfiles/TestReplaceFolderWithLink diff --git a/integration/dockerfiles/TestReplaceFolderWithFile b/integration/dockerfiles/TestReplaceFolderWithFile new file mode 100644 index 000000000..ca3e6f2a7 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/integration/dockerfiles/TestReplaceFolderWithLink b/integration/dockerfiles/TestReplaceFolderWithLink new file mode 100644 index 000000000..d22a550df --- /dev/null +++ b/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 \ No newline at end of file diff --git a/integration/integration_test.go b/integration/integration_test.go index 323901d00..b4785ea30 100644 --- a/integration/integration_test.go +++ b/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) diff --git a/pkg/snapshot/snapshot.go b/pkg/snapshot/snapshot.go index 7aab450fe..018221ffc 100644 --- a/pkg/snapshot/snapshot.go +++ b/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 { diff --git a/pkg/snapshot/snapshot_test.go b/pkg/snapshot/snapshot_test.go index d5d19c4e9..e343fd3bd 100644 --- a/pkg/snapshot/snapshot_test.go +++ b/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, "/")