Skip to content

Commit

Permalink
Write parent directories to tar before whiteout files (#2113)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@andreasf
andreasf authored May 31, 2022
1 parent 1c0e5a0 commit bc46c24
Show file tree
Hide file tree
Showing 2 changed files with 118 additions and 63 deletions.
36 changes: 22 additions & 14 deletions pkg/snapshot/snapshot.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
continue
if err := addParentDirectories(t, addedPaths, path); err != nil {
return err
}
for _, parentPath := range util.ParentDirectories(path) {
if parentPath == "/" {
continue
}
if _, dirExists := addedPaths[parentPath]; dirExists {
continue
}
if err := t.AddFileToTar(parentPath); err != nil {
return err
}
addedPaths[parentPath] = true
if _, pathAdded := addedPaths[path]; pathAdded {
continue
}
if err := t.AddFileToTar(path); err != nil {
return err
Expand All @@ -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)
Expand Down
145 changes: 96 additions & 49 deletions pkg/snapshot/snapshot_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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 {
hdr, err := tr.Next()
if err == io.EOF {
break
}
if err != nil {
t.Fatal(err)
}
filesInTars[i] = append(filesInTars[i], strings.TrimPrefix(hdr.Name, testDirWithoutLeadingSlash))
for _, fn := range filesInTar {
filesInTars[i] = append(filesInTars[i], strings.TrimPrefix(fn, testDirWithoutLeadingSlash))
}
}

Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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 {
hdr, err := tr.Next()
if err == io.EOF {
break
}
if err != nil {
t.Fatal(err)
}
filesInTars[i] = append(filesInTars[i], strings.TrimPrefix(hdr.Name, testDirWithoutLeadingSlash))
for _, fn := range filesInTar {
filesInTars[i] = append(filesInTars[i], strings.TrimPrefix(fn, testDirWithoutLeadingSlash))
}
}

Expand Down Expand Up @@ -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
}

0 comments on commit bc46c24

Please sign in to comment.