From cc2c9a0663a64e3a2c2e22eac9d987b998daa48a Mon Sep 17 00:00:00 2001 From: tinkerborg <15373049+tinkerborg@users.noreply.github.com> Date: Tue, 14 Jan 2020 14:55:06 -0500 Subject: [PATCH 1/5] sort filesToAdd in TakeSnapshot filesToAdd is sorted in TakeSnapshotFS, but not here. This makes ordering unpredictable within the layer's tarball, causing the SHA to differ even if layer contents haven't changed --- pkg/snapshot/snapshot.go | 2 + pkg/snapshot/snapshot_test.go | 69 +++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/pkg/snapshot/snapshot.go b/pkg/snapshot/snapshot.go index 80b5c5ce04..59be2f0e0c 100644 --- a/pkg/snapshot/snapshot.go +++ b/pkg/snapshot/snapshot.go @@ -79,6 +79,8 @@ func (s *Snapshotter) TakeSnapshot(files []string) (string, error) { // Also add parent directories to keep the permission of them correctly. filesToAdd := filesWithParentDirs(files) + sort.Strings(filesToAdd) + // Add files to the layered map for _, file := range filesToAdd { if err := s.l.Add(file); err != nil { diff --git a/pkg/snapshot/snapshot_test.go b/pkg/snapshot/snapshot_test.go index 8e3f16598e..3a1ec73539 100644 --- a/pkg/snapshot/snapshot_test.go +++ b/pkg/snapshot/snapshot_test.go @@ -302,6 +302,75 @@ func TestFileWithLinks(t *testing.T) { } } +func TestSnasphotPreservesFileOrder(t *testing.T) { + newFiles := map[string]string{ + "foo": "newbaz1", + "bar/bat": "baz", + "bar/qux": "quuz", + "qux": "quuz", + "corge": "grault", + "garply": "waldo", + "fred": "plugh", + "xyzzy": "thud", + } + + newFileNames := []string{} + + for fileName := range newFiles { + newFileNames = append(newFileNames, fileName) + } + + filesInTars := [][]string{} + + for i := 0; i<= 2; i++ { + testDir, snapshotter, cleanup, err := setUpTestDir() + testDirWithoutLeadingSlash := strings.TrimLeft(testDir, "/") + defer cleanup() + + if err != nil { + t.Fatal(err) + } + // Make some changes to the filesystem + if err := testutil.SetupFiles(testDir, newFiles); err != nil { + t.Fatalf("Error setting up fs: %s", err) + } + + filesToSnapshot := []string{} + for _, file := range newFileNames { + filesToSnapshot = append(filesToSnapshot, filepath.Join(testDir, file)) + } + + // Take a snapshot + tarPath, err := snapshotter.TakeSnapshot(filesToSnapshot) + + if err != nil { + t.Fatalf("Error taking snapshot of fs: %s", err) + } + + f, err := os.Open(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)) + } + } + + // Check contents of all snapshots, make sure files appear in consistent order + for i := 1; i Date: Thu, 6 Feb 2020 12:40:19 -0500 Subject: [PATCH 2/5] omit uname/gname in tar headers When using cache, the rootfs may not have been extracted. This prevents uname/gname from resolving as there is no /etc/password or /etc/group. This makes this layer unnecessarily differ from a cached layer which does contain this information. Omitting these should be consistent with Docker's behavior. --- pkg/snapshot/snapshot_test.go | 30 ++++++++++++++++++++++++++++++ pkg/util/tar_util.go | 5 +++++ 2 files changed, 35 insertions(+) diff --git a/pkg/snapshot/snapshot_test.go b/pkg/snapshot/snapshot_test.go index 3a1ec73539..2fa6bb104a 100644 --- a/pkg/snapshot/snapshot_test.go +++ b/pkg/snapshot/snapshot_test.go @@ -371,6 +371,36 @@ func TestSnasphotPreservesFileOrder(t *testing.T) { } } +func TestSnapshotOmitsUnameGname(t *testing.T) { + testDir, snapshotter, cleanup, err := setUpTestDir() + testDirWithoutLeadingSlash := strings.TrimLeft(testDir, "/") + defer cleanup() + if err != nil { + t.Fatal(err) + } + + tarPath, err := snapshotter.TakeSnapshotFS() + + f, err := os.Open(tarPath) + if err != nil { + t.Fatal(err) + } + tr := tar.NewReader(f) + for { + hdr, err := tr.Next() + if err == io.EOF { + break + } + if err != nil { + t.Fatal(err) + } + if hdr.Uname != "" || hdr.Gname != "" { + t.Fatalf("Expected Uname/Gname for %s to be empty: Uname = '%s', Gname = '%s'", hdr.Name, hdr.Uname, hdr.Gname) + } + } + +} + func setupSymlink(dir string, link string, target string) error { return os.Symlink(target, filepath.Join(dir, link)) } diff --git a/pkg/util/tar_util.go b/pkg/util/tar_util.go index 213ef68e38..15c4e933d8 100644 --- a/pkg/util/tar_util.go +++ b/pkg/util/tar_util.go @@ -84,6 +84,11 @@ func (t *Tar) AddFileToTar(p string) error { hdr.Name = p } + // 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 = "" + hdr.Gname = "" + hardlink, linkDst := t.checkHardlink(p, i) if hardlink { hdr.Linkname = linkDst From 674f5bda5dd4b1f61b1a5ee5e9fa25b2a19b10f5 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Thu, 6 Feb 2020 15:17:09 -0800 Subject: [PATCH 3/5] fix tests --- pkg/snapshot/snapshot_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/snapshot/snapshot_test.go b/pkg/snapshot/snapshot_test.go index 2fa6bb104a..d99e1e19aa 100644 --- a/pkg/snapshot/snapshot_test.go +++ b/pkg/snapshot/snapshot_test.go @@ -323,7 +323,7 @@ func TestSnasphotPreservesFileOrder(t *testing.T) { filesInTars := [][]string{} for i := 0; i<= 2; i++ { - testDir, snapshotter, cleanup, err := setUpTestDir() + testDir, snapshotter, cleanup, err := setUpTest() testDirWithoutLeadingSlash := strings.TrimLeft(testDir, "/") defer cleanup() @@ -372,8 +372,8 @@ func TestSnasphotPreservesFileOrder(t *testing.T) { } func TestSnapshotOmitsUnameGname(t *testing.T) { - testDir, snapshotter, cleanup, err := setUpTestDir() - testDirWithoutLeadingSlash := strings.TrimLeft(testDir, "/") + _, snapshotter, cleanup, err := setUpTest() + defer cleanup() if err != nil { t.Fatal(err) From 8b69a13641e2ed1ba7cdf9f71688d51c88ff4e5f Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Thu, 6 Feb 2020 15:37:47 -0800 Subject: [PATCH 4/5] fix commut --- pkg/snapshot/snapshot_test.go | 4 ++-- pkg/util/tar_util.go | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/snapshot/snapshot_test.go b/pkg/snapshot/snapshot_test.go index d99e1e19aa..4cf698c52d 100644 --- a/pkg/snapshot/snapshot_test.go +++ b/pkg/snapshot/snapshot_test.go @@ -322,7 +322,7 @@ func TestSnasphotPreservesFileOrder(t *testing.T) { filesInTars := [][]string{} - for i := 0; i<= 2; i++ { + for i := 0; i <= 2; i++ { testDir, snapshotter, cleanup, err := setUpTest() testDirWithoutLeadingSlash := strings.TrimLeft(testDir, "/") defer cleanup() @@ -366,7 +366,7 @@ func TestSnasphotPreservesFileOrder(t *testing.T) { } // Check contents of all snapshots, make sure files appear in consistent order - for i := 1; i Date: Fri, 7 Feb 2020 13:47:17 -0800 Subject: [PATCH 5/5] fix linter --- pkg/snapshot/snapshot_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/snapshot/snapshot_test.go b/pkg/snapshot/snapshot_test.go index 4cf698c52d..67b70b9c51 100644 --- a/pkg/snapshot/snapshot_test.go +++ b/pkg/snapshot/snapshot_test.go @@ -380,6 +380,9 @@ func TestSnapshotOmitsUnameGname(t *testing.T) { } tarPath, err := snapshotter.TakeSnapshotFS() + if err != nil { + t.Fatal(err) + } f, err := os.Open(tarPath) if err != nil {