From e5585fded80f92d90cc1b2dbe3319d53dc8e498b Mon Sep 17 00:00:00 2001 From: Gilbert Gilb's Date: Sun, 29 Mar 2020 18:03:32 +0200 Subject: [PATCH 1/2] Always add parent directories of files to snapshots. During a snapshot, when a file changed and not its parent directories, the parent directories weren't added to the layer. This is inconsistent with Docker's behavior which always add parent directories to the layer. In some edge-cases, it could lead to problems with docker considering that parent directories where owned by root in forthcoming layers although they shouldn't (see #1163). Also, Docker seems to be POSIX compliant regarding the name of directories in the archive, which always have a slash appended. This commit also fixes this. Fixes #1163 --- pkg/snapshot/snapshot.go | 18 ++++++++++++++++++ pkg/snapshot/snapshot_test.go | 22 +++++++++++++++++++++- pkg/util/tar_util.go | 4 ++++ 3 files changed, 43 insertions(+), 1 deletion(-) diff --git a/pkg/snapshot/snapshot.go b/pkg/snapshot/snapshot.go index e64e28dd3..7e027fe8f 100644 --- a/pkg/snapshot/snapshot.go +++ b/pkg/snapshot/snapshot.go @@ -226,10 +226,28 @@ func writeToTar(t util.Tar, files, whiteouts []string) error { return err } } + + addedPaths := make(map[string]bool) for _, path := range files { + if _, fileExists := addedPaths[path]; fileExists { + continue + } + 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 err := t.AddFileToTar(path); err != nil { return err } + addedPaths[path] = true } return nil } diff --git a/pkg/snapshot/snapshot_test.go b/pkg/snapshot/snapshot_test.go index 19fc9c4cb..2e2d9cc21 100644 --- a/pkg/snapshot/snapshot_test.go +++ b/pkg/snapshot/snapshot_test.go @@ -63,6 +63,12 @@ func TestSnapshotFSFileChange(t *testing.T) { fooPath: "newbaz1", batPath: "baz", } + for _, path := range util.ParentDirectoriesWithoutLeadingSlash(batPath) { + if path == "/" { + continue + } + snapshotFiles[path+"/"] = "" + } actualFiles := []string{} for { @@ -76,6 +82,9 @@ func TestSnapshotFSFileChange(t *testing.T) { if _, isFile := snapshotFiles[hdr.Name]; !isFile { t.Fatalf("File %s unexpectedly in tar", hdr.Name) } + if hdr.Typeflag == tar.TypeDir { + continue + } contents, _ := ioutil.ReadAll(tr) if string(contents) != snapshotFiles[hdr.Name] { t.Fatalf("Contents of %s incorrect, expected: %s, actual: %s", hdr.Name, snapshotFiles[hdr.Name], string(contents)) @@ -152,6 +161,12 @@ func TestSnapshotFSChangePermissions(t *testing.T) { snapshotFiles := map[string]string{ batPathWithoutLeadingSlash: "baz2", } + for _, path := range util.ParentDirectoriesWithoutLeadingSlash(batPathWithoutLeadingSlash) { + if path == "/" { + continue + } + snapshotFiles[path+"/"] = "" + } foundFiles := []string{} for { @@ -164,6 +179,9 @@ func TestSnapshotFSChangePermissions(t *testing.T) { if _, isFile := snapshotFiles[hdr.Name]; !isFile { t.Fatalf("File %s unexpectedly in tar", hdr.Name) } + if hdr.Typeflag == tar.TypeDir { + continue + } contents, _ := ioutil.ReadAll(tr) if string(contents) != snapshotFiles[hdr.Name] { t.Fatalf("Contents of %s incorrect, expected: %s, actual: %s", hdr.Name, snapshotFiles[hdr.Name], string(contents)) @@ -203,7 +221,9 @@ func TestSnapshotFiles(t *testing.T) { expectedFiles := []string{ filepath.Join(testDirWithoutLeadingSlash, "foo"), } - expectedFiles = append(expectedFiles, util.ParentDirectoriesWithoutLeadingSlash(filepath.Join(testDir, "foo"))...) + for _, path := range util.ParentDirectoriesWithoutLeadingSlash(filepath.Join(testDir, "foo")) { + expectedFiles = append(expectedFiles, strings.TrimRight(path, "/")+"/") + } f, err := os.Open(tarPath) if err != nil { diff --git a/pkg/util/tar_util.go b/pkg/util/tar_util.go index 823e42f04..76a6b5405 100644 --- a/pkg/util/tar_util.go +++ b/pkg/util/tar_util.go @@ -84,6 +84,10 @@ func (t *Tar) AddFileToTar(p string) error { hdr.Name = p } + if hdr.Typeflag == tar.TypeDir && !strings.HasSuffix(hdr.Name, "/") { + hdr.Name = hdr.Name + "/" + } + // rootfs may not have been extracted when using cache, preventing uname/gname from resolving // this makes this layer unnecessarily differ from a cached layer which does contain this information hdr.Uname = "" From 14170aa455fd16ed606bb3afda2ab1c5847e3d90 Mon Sep 17 00:00:00 2001 From: Gilbert Gilb's Date: Tue, 31 Mar 2020 20:12:00 +0200 Subject: [PATCH 2/2] Fix sorting of parent directories. This refactoring reversed the order of the "ParentDirectories" function: https://github.com/GoogleContainerTools/kaniko/pull/1155/commits/ffc372a63bca75e5b7c680c981d2444fc4017897#diff-d36eb675aa49a7b471e3a2be77005b18R465 As a side-effect, parent directories weren't added in lexicographical order, which broke some tests. We now ensure in unit test that the order of the ParentDirectories function is stable. --- pkg/util/fs_util.go | 4 ++-- pkg/util/fs_util_test.go | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/util/fs_util.go b/pkg/util/fs_util.go index a5ecaab1b..230d87669 100644 --- a/pkg/util/fs_util.go +++ b/pkg/util/fs_util.go @@ -468,10 +468,10 @@ func ParentDirectories(path string) []string { } dir, _ = filepath.Split(dir) dir = filepath.Clean(dir) - paths = append(paths, dir) + paths = append([]string{dir}, paths...) } if len(paths) == 0 { - paths = append(paths, config.RootDir) + paths = []string{config.RootDir} } return paths } diff --git a/pkg/util/fs_util_test.go b/pkg/util/fs_util_test.go index b560330b2..5ba37fef9 100644 --- a/pkg/util/fs_util_test.go +++ b/pkg/util/fs_util_test.go @@ -213,8 +213,6 @@ func Test_ParentDirectories(t *testing.T) { defer func() { config.RootDir = original }() config.RootDir = tt.rootDir actual := ParentDirectories(tt.path) - sort.Strings(actual) - sort.Strings(tt.expected) testutil.CheckErrorAndDeepEqual(t, false, nil, tt.expected, actual) })