Write parent directories to tar before whiteout files (#2113)

* Write parent directories to tar before whiteout files

Fixes #1149

The OCI image spec does not specify this order but it's a good idea and Docker
does the same.

When manually comparing layers created by Docker and Kaniko there are still
some differences (that container-diff does not show):

* Kaniko adds / to layers
* For `mkdir /test`, docker adds `/test` and an opaque whiteout file
  `/test/.wh..wh..opq`. Kaniko only adds `/test/` (and /).

* snapshot_test: cleanup

Fix typos and use listFilesInTar() where possible

pkg/snapshot/snapshot.go

@@ -220,28 +220,23 @@ func writeToTar(t util.Tar, files, whiteouts []string) error {
 	defer timing.DefaultRun.Stop(timer)
 
 	// Now create the tar.
+	addedPaths := make(map[string]bool)
+
 	for _, path := range whiteouts {
+		if err := addParentDirectories(t, addedPaths, path); err != nil {
+			return err
+		}
 		if err := t.Whiteout(path); err != nil {
 			return err
 		}
 	}
 
-	addedPaths := make(map[string]bool)
 	for _, path := range files {
-		if _, fileExists := addedPaths[path]; fileExists {
+		if err := addParentDirectories(t, addedPaths, path); err != nil {
-			continue
+			return err
 		}
-		for _, parentPath := range util.ParentDirectories(path) {
+		if _, pathAdded := addedPaths[path]; pathAdded {
-			if parentPath == "/" {
+			continue
-				continue
-			}
-			if _, dirExists := addedPaths[parentPath]; dirExists {
-				continue
-			}
-			if err := t.AddFileToTar(parentPath); err != nil {
-				return err
-			}
-			addedPaths[parentPath] = true
 		}
 		if err := t.AddFileToTar(path); err != nil {
 			return err
@@ -251,6 +246,19 @@ func writeToTar(t util.Tar, files, whiteouts []string) error {
 	return nil
 }
 
+func addParentDirectories(t util.Tar, addedPaths map[string]bool, path string) error {
+	for _, parentPath := range util.ParentDirectories(path) {
+		if _, pathAdded := addedPaths[parentPath]; pathAdded {
+			continue
+		}
+		if err := t.AddFileToTar(parentPath); err != nil {
+			return err
+		}
+		addedPaths[parentPath] = true
+	}
+	return nil
+}
+
 // filesWithLinks returns the symlink and the target path if its exists.
 func filesWithLinks(path string) ([]string, error) {
 	link, err := util.GetSymLink(path)

pkg/snapshot/snapshot_test.go

@@ -117,20 +117,11 @@ func TestSnapshotFSIsReproducible(t *testing.T) {
 		t.Fatalf("Error taking snapshot of fs: %s", err)
 	}
 
-	f, err := os.Open(tarPath)
+	// Check contents of the snapshot, make sure contents are sorted by name
+	filesInTar, err := listFilesInTar(tarPath)
 	if err != nil {
 		t.Fatal(err)
 	}
-	// Check contents of the snapshot, make sure contents are sorted by name
-	tr := tar.NewReader(f)
-	var filesInTar []string
-	for {
-		hdr, err := tr.Next()
-		if err == io.EOF {
-			break
-		}
-		filesInTar = append(filesInTar, hdr.Name)
-	}
 	if !sort.StringsAreSorted(filesInTar) {
 		t.Fatalf("Expected the file in the tar archive were sorted, actual list was not sorted: %v", filesInTar)
 	}
@@ -227,23 +218,12 @@ func TestSnapshotFiles(t *testing.T) {
 		expectedFiles = append(expectedFiles, strings.TrimRight(path, "/")+"/")
 	}
 
-	f, err := os.Open(tarPath)
+	// Check contents of the snapshot, make sure contents is equivalent to snapshotFiles
+	actualFiles, err := listFilesInTar(tarPath)
 	if err != nil {
 		t.Fatal(err)
 	}
-	// Check contents of the snapshot, make sure contents is equivalent to snapshotFiles
+
-	tr := tar.NewReader(f)
-	var actualFiles []string
-	for {
-		hdr, err := tr.Next()
-		if err == io.EOF {
-			break
-		}
-		if err != nil {
-			t.Fatal(err)
-		}
-		actualFiles = append(actualFiles, hdr.Name)
-	}
 	sort.Strings(expectedFiles)
 	sort.Strings(actualFiles)
 	testutil.CheckErrorAndDeepEqual(t, false, nil, expectedFiles, actualFiles)
@@ -321,7 +301,7 @@ func TestFileWithLinks(t *testing.T) {
 	}
 }
 
-func TestSnasphotPreservesFileOrder(t *testing.T) {
+func TestSnapshotPreservesFileOrder(t *testing.T) {
 	newFiles := map[string]string{
 		"foo":     "newbaz1",
 		"bar/bat": "baz",
@@ -366,21 +346,14 @@ func TestSnasphotPreservesFileOrder(t *testing.T) {
 			t.Fatalf("Error taking snapshot of fs: %s", err)
 		}
 
-		f, err := os.Open(tarPath)
+		filesInTar, err := listFilesInTar(tarPath)
 		if err != nil {
 			t.Fatal(err)
 		}
-		tr := tar.NewReader(f)
+
 		filesInTars = append(filesInTars, []string{})
-		for {
+		for _, fn := range filesInTar {
-			hdr, err := tr.Next()
+			filesInTars[i] = append(filesInTars[i], strings.TrimPrefix(fn, testDirWithoutLeadingSlash))
-			if err == io.EOF {
-				break
-			}
-			if err != nil {
-				t.Fatal(err)
-			}
-			filesInTars[i] = append(filesInTars[i], strings.TrimPrefix(hdr.Name, testDirWithoutLeadingSlash))
 		}
 	}
 
@@ -430,7 +403,68 @@ func TestSnapshotWithForceBuildMetadataIsNotSet(t *testing.T) {
 	}
 }
 
-func TestSnasphotPreservesWhiteoutOrder(t *testing.T) {
+func TestSnapshotIncludesParentDirBeforeWhiteoutFile(t *testing.T) {
+	testDir, snapshotter, cleanup, err := setUpTest(t)
+	defer cleanup()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Take a snapshot
+	filesToSnapshot := []string{filepath.Join(testDir, "kaniko/file", "bar/bat")}
+	_, err = snapshotter.TakeSnapshot(filesToSnapshot, false, false)
+	if err != nil {
+		t.Fatalf("Error taking snapshot of fs: %s", err)
+	}
+
+	// Add a file
+	newFiles := map[string]string{
+		"kaniko/new-file": "baz",
+	}
+	if err := testutil.SetupFiles(testDir, newFiles); err != nil {
+		t.Fatalf("Error setting up fs: %s", err)
+	}
+	filesToSnapshot = append(filesToSnapshot, filepath.Join(testDir, "kaniko/new-file"))
+
+	// Delete files
+	filesToDelete := []string{"kaniko/file", "bar"}
+	for _, fn := range filesToDelete {
+		err = os.RemoveAll(filepath.Join(testDir, fn))
+		if err != nil {
+			t.Fatalf("Error deleting file: %s", err)
+		}
+	}
+
+	// Take a snapshot again
+	tarPath, err := snapshotter.TakeSnapshot(filesToSnapshot, true, false)
+	if err != nil {
+		t.Fatalf("Error taking snapshot of fs: %s", err)
+	}
+
+	actualFiles, err := listFilesInTar(tarPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	testDirWithoutLeadingSlash := strings.TrimLeft(testDir, "/")
+	expectedFiles := []string{
+		filepath.Join(testDirWithoutLeadingSlash, "kaniko/.wh.file"),
+		filepath.Join(testDirWithoutLeadingSlash, "kaniko/new-file"),
+		filepath.Join(testDirWithoutLeadingSlash, ".wh.bar"),
+		"/",
+	}
+	for parentDir := filepath.Dir(expectedFiles[0]); parentDir != "."; parentDir = filepath.Dir(parentDir) {
+		expectedFiles = append(expectedFiles, parentDir+"/")
+	}
+
+	// Sorting does the right thing in this case. The expected order for a directory is:
+	// Parent dirs first, then whiteout files in the directory, then other files in that directory
+	sort.Strings(expectedFiles)
+
+	testutil.CheckErrorAndDeepEqual(t, false, nil, expectedFiles, actualFiles)
+}
+
+func TestSnapshotPreservesWhiteoutOrder(t *testing.T) {
 	newFiles := map[string]string{
 		"foo":     "newbaz1",
 		"bar/bat": "baz",
@@ -488,21 +522,14 @@ func TestSnasphotPreservesWhiteoutOrder(t *testing.T) {
 			t.Fatalf("Error taking snapshot of fs: %s", err)
 		}
 
-		f, err := os.Open(tarPath)
+		filesInTar, err := listFilesInTar(tarPath)
 		if err != nil {
 			t.Fatal(err)
 		}
-		tr := tar.NewReader(f)
+
 		filesInTars = append(filesInTars, []string{})
-		for {
+		for _, fn := range filesInTar {
-			hdr, err := tr.Next()
+			filesInTars[i] = append(filesInTars[i], strings.TrimPrefix(fn, testDirWithoutLeadingSlash))
-			if err == io.EOF {
-				break
-			}
-			if err != nil {
-				t.Fatal(err)
-			}
-			filesInTars[i] = append(filesInTars[i], strings.TrimPrefix(hdr.Name, testDirWithoutLeadingSlash))
 		}
 	}
 
@@ -599,3 +626,23 @@ func setUpTest(t *testing.T) (string, *Snapshotter, func(), error) {
 
 	return testDir, snapshotter, cleanup, nil
 }
+
+func listFilesInTar(path string) ([]string, error) {
+	f, err := os.Open(path)
+	if err != nil {
+		return nil, err
+	}
+	tr := tar.NewReader(f)
+	var files []string
+	for {
+		hdr, err := tr.Next()
+		if err == io.EOF {
+			break
+		}
+		if err != nil {
+			return nil, err
+		}
+		files = append(files, hdr.Name)
+	}
+	return files, nil
+}