Fix parent directory permissions (#619)

* Add parent directories of adding files

* Add integration Dockerfile to test parent directory permissions

* Remove unnecessary helper method

* Use a file on the internet for integration Dockerfile

integration/dockerfiles/Dockerfile_test_parent_dir_perms

@@ -0,0 +1,8 @@
+FROM busybox
+
+RUN adduser --disabled-password --gecos "" --uid 1000 user
+RUN mkdir -p /home/user/foo
+RUN chown -R user /home/user
+RUN chmod 700 /home/user/foo
+ADD https://raw.githubusercontent.com/GoogleContainerTools/kaniko/master/README.md /home/user/foo/README.md
+RUN chown -R user /home/user

pkg/snapshot/layered_map.go

@@ -109,11 +109,10 @@ func (l *LayeredMap) Add(s string) error {
 	return nil
 }
 
-// MaybeAdd will add the specified file s to the layered map if
-// the layered map's hashing function determines it has changed. If
-// it has not changed, it will not be added. Returns true if the file
-// was added.
-func (l *LayeredMap) MaybeAdd(s string) (bool, error) {
+// CheckFileChange checkes whether a given file changed
+// from the current layered map by its hashing function.
+// Returns true if the file is changed.
+func (l *LayeredMap) CheckFileChange(s string) (bool, error) {
 	oldV, ok := l.Get(s)
 	t := timing.Start("Hashing files")
 	defer timing.DefaultRun.Stop(t)
@@ -124,6 +123,5 @@ func (l *LayeredMap) MaybeAdd(s string) (bool, error) {
 	if ok && newV == oldV {
 		return false, nil
 	}
-	l.layers[len(l.layers)-1][s] = newV
 	return true, nil
 }

pkg/snapshot/snapshot.go

@@ -73,44 +73,15 @@ func (s *Snapshotter) TakeSnapshot(files []string) (string, error) {
 	}
 	logrus.Info("Taking snapshot of files...")
 	logrus.Debugf("Taking snapshot of files %v", files)
-	snapshottedFiles := make(map[string]bool)
 
-	// First add to the tar any parent directories that haven't been added
-	parentDirs := map[string]struct{}{}
-	for _, file := range files {
-		for _, p := range util.ParentDirectories(file) {
-			parentDirs[p] = struct{}{}
-		}
-	}
-	filesToAdd := []string{}
-	for file := range parentDirs {
-		file = filepath.Clean(file)
-		snapshottedFiles[file] = true
-
-		// The parent directory might already be in a previous layer.
-		fileAdded, err := s.l.MaybeAdd(file)
-		if err != nil {
-			return "", fmt.Errorf("Unable to add parent dir %s to layered map: %s", file, err)
-		}
-
-		if fileAdded {
-			filesToAdd = append(filesToAdd, file)
-		}
-	}
-
-	// Next add the files themselves to the tar
-	for _, file := range files {
-		// We might have already added the file above as a parent directory of another file.
-		file = filepath.Clean(file)
-		if _, ok := snapshottedFiles[file]; ok {
-			continue
-		}
-		snapshottedFiles[file] = true
+	// Also add parent directories to keep the permission of them correctly.
+	filesToAdd := filesWithParentDirs(files)
 
+	// Add files to the layered map
+	for _, file := range filesToAdd {
 		if err := s.l.Add(file); err != nil {
 			return "", fmt.Errorf("Unable to add file %s to layered map: %s", file, err)
 		}
-		filesToAdd = append(filesToAdd, file)
 	}
 
 	t := util.NewTar(f)
@@ -201,16 +172,27 @@ func (s *Snapshotter) scanFullFilesystem() ([]string, []string, error) {
 			logrus.Debugf("Not adding %s to layer, as it's whitelisted", path)
 			continue
 		}
-		// Only add to the tar if we add it to the layeredmap.
-		maybeAdd, err := s.l.MaybeAdd(path)
+		// Only add changed files.
+		fileChanged, err := s.l.CheckFileChange(path)
 		if err != nil {
 			return nil, nil, err
 		}
-		if maybeAdd {
-			logrus.Debugf("Adding %s to layer, because it was changed.", path)
+		if fileChanged {
+			logrus.Infof("Adding %s to layer, because it was changed.", path)
 			filesToAdd = append(filesToAdd, path)
 		}
 	}
+
+	// Also add parent directories to keep the permission of them correctly.
+	filesToAdd = filesWithParentDirs(filesToAdd)
+
+	// Add files to the layered map
+	for _, file := range filesToAdd {
+		if err := s.l.Add(file); err != nil {
+			return nil, nil, fmt.Errorf("Unable to add file %s to layered map: %s", file, err)
+		}
+	}
+
 	return filesToAdd, filesToWhiteOut, nil
 }
 
@@ -230,3 +212,24 @@ func writeToTar(t util.Tar, files, whiteouts []string) error {
 	}
 	return nil
 }
+
+func filesWithParentDirs(files []string) []string {
+	filesSet := map[string]bool{}
+
+	for _, file := range files {
+		file = filepath.Clean(file)
+		filesSet[file] = true
+
+		for _, dir := range util.ParentDirectories(file) {
+			dir = filepath.Clean(dir)
+			filesSet[dir] = true
+		}
+	}
+
+	newFiles := []string{}
+	for file := range filesSet {
+		newFiles = append(newFiles, file)
+	}
+
+	return newFiles
+}

pkg/snapshot/snapshot_test.go

@@ -21,6 +21,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"sort"
 	"testing"
 
 	"github.com/GoogleContainerTools/kaniko/pkg/util"
@@ -60,6 +61,12 @@ func TestSnapshotFSFileChange(t *testing.T) {
 		fooPath: "newbaz1",
 		batPath: "baz",
 	}
+	for _, dir := range util.ParentDirectories(fooPath) {
+		snapshotFiles[dir] = ""
+	}
+	for _, dir := range util.ParentDirectories(batPath) {
+		snapshotFiles[dir] = ""
+	}
 	numFiles := 0
 	for {
 		hdr, err := tr.Next()
@@ -75,7 +82,7 @@ func TestSnapshotFSFileChange(t *testing.T) {
 			t.Fatalf("Contents of %s incorrect, expected: %s, actual: %s", hdr.Name, snapshotFiles[hdr.Name], string(contents))
 		}
 	}
-	if numFiles != 2 {
+	if numFiles != len(snapshotFiles) {
 		t.Fatalf("Incorrect number of files were added, expected: 2, actual: %v", numFiles)
 	}
 }
@@ -105,6 +112,9 @@ func TestSnapshotFSChangePermissions(t *testing.T) {
 	snapshotFiles := map[string]string{
 		batPath: "baz2",
 	}
+	for _, dir := range util.ParentDirectories(batPath) {
+		snapshotFiles[dir] = ""
+	}
 	numFiles := 0
 	for {
 		hdr, err := tr.Next()
@@ -120,7 +130,7 @@ func TestSnapshotFSChangePermissions(t *testing.T) {
 			t.Fatalf("Contents of %s incorrect, expected: %s, actual: %s", hdr.Name, snapshotFiles[hdr.Name], string(contents))
 		}
 	}
-	if numFiles != 1 {
+	if numFiles != len(snapshotFiles) {
 		t.Fatalf("Incorrect number of files were added, expected: 1, got: %v", numFiles)
 	}
 }
@@ -147,7 +157,10 @@ func TestSnapshotFiles(t *testing.T) {
 	}
 	defer os.Remove(tarPath)
 
-	expectedFiles := []string{"/", "/tmp", filepath.Join(testDir, "foo")}
+	expectedFiles := []string{
+		filepath.Join(testDir, "foo"),
+	}
+	expectedFiles = append(expectedFiles, util.ParentDirectories(filepath.Join(testDir, "foo"))...)
 
 	f, err := os.Open(tarPath)
 	if err != nil {
@@ -166,6 +179,8 @@ func TestSnapshotFiles(t *testing.T) {
 		}
 		actualFiles = append(actualFiles, hdr.Name)
 	}
+	sort.Strings(expectedFiles)
+	sort.Strings(actualFiles)
 	testutil.CheckErrorAndDeepEqual(t, false, nil, expectedFiles, actualFiles)
 }