Skip to content

Commit

Permalink
Don't write whiteout files if parent path got replaced with link
Browse files Browse the repository at this point in the history
If a non-empty directory gets replaced with a link, 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 GoogleContainerTools#2308
  • Loading branch information
andreasf committed Jun 19, 2023
1 parent bb712b6 commit ac66f1e
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 0 deletions.
23 changes: 23 additions & 0 deletions pkg/snapshot/snapshot.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,14 @@ func writeToTar(t util.Tar, files, whiteouts []string) error {
addedPaths := make(map[string]bool)

for _, path := range whiteouts {
skipWhiteout, err := parentPathIsLink(path)

This comment has been minimized.

Copy link
@gabyx

gabyx Jun 19, 2023

No this is the wrong way arround IMO:

If the parent path chain contains ANY link, no parent paths should be added before the whiteout.
The whiteout must be added always.

This comment has been minimized.

Copy link
@andreasf

andreasf Jun 19, 2023

Author Owner

Kaniko 1.8.1 wrote files in this order, but it caused issues in Cloud Foundry, and tar would also throw an error extracting the file: GoogleContainerTools#2308 (comment)

The image spec is not very explicit about this situation, but I think changesets over existing files covers it from the point of view of applying changes:

In all other cases, the implementation MUST do the semantic equivalent of the following:
removing the file path (e.g. unlink(2) on Linux systems)
recreating the file path, based on the contents and attributes of the changeset entry

I read that as: when a directory is replaced by a symlink in a layer, apply the layer by removing the entire directory and adding the symlink. So just placing the symlink in the layer should be enough, and that's also what Docker does.

This comment has been minimized.

Copy link
@gabyx

gabyx Jun 19, 2023

Ah ok, thanks.

Ok then your PR is correct! Cool.
Can you make a PR from it?

This comment has been minimized.

Copy link
@andreasf

andreasf Jun 20, 2023

Author Owner

Done, I made a PR for the other branch in which I generalized the parent path check to match all non-directories.

if err != nil {
return err
}
if skipWhiteout {
continue
}

if err := addParentDirectories(t, addedPaths, path); err != nil {
return err
}
Expand All @@ -247,6 +255,21 @@ func writeToTar(t util.Tar, files, whiteouts []string) error {
return nil
}

func parentPathIsLink(path string) (bool, error) {
for _, parentPath := range util.ParentDirectories(path) {
lstat, err := os.Lstat(parentPath)
if err != nil {
return false, err
}

isLink := lstat.Mode()&os.ModeSymlink != 0
if isLink {
return true, err
}
}
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 {
Expand Down
45 changes: 45 additions & 0 deletions pkg/snapshot/snapshot_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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-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, "/")
Expand Down

0 comments on commit ac66f1e

Please sign in to comment.