Fix snapshotter ignore list; do not attempt to delete whiteouts of ignored paths (#1652)

* avoid deleting whiteouts if they are in ignore list

* fix snapshotter ignore list

* include filesystem mounts in ignorelist of snapshotter
* clean up ignore list logic

* add unit and integration tests for #1652

* fix tests and ignore list updates

cmd/executor/cmd/root.go

@@ -88,9 +88,19 @@ var RootCmd = &cobra.Command{
 				return errors.New("You must provide --destination if setting ImageNameTagDigestFile")
 			}
 			// Update ignored paths
-			util.UpdateInitialIgnoreList(opts.IgnoreVarRun)
+			if opts.IgnoreVarRun {
+				// /var/run is a special case. It's common to mount in /var/run/docker.sock
+				// or something similar which leads to a special mount on the /var/run/docker.sock
+				// file itself, but the directory to exist in the image with no way to tell if it came
+				// from the base image or not.
+				logrus.Trace("Adding /var/run to default ignore list")
+				util.AddToDefaultIgnoreList(util.IgnoreListEntry{
+					Path:            "/var/run",
+					PrefixMatchOnly: false,
+				})
+			}
 			for _, p := range opts.IgnorePaths {
-				util.AddToBaseIgnoreList(util.IgnoreListEntry{
+				util.AddToDefaultIgnoreList(util.IgnoreListEntry{
 					Path:            p,
 					PrefixMatchOnly: false,
 				})

integration/dockerfiles/Dockerfile_test_snapshotter_ignorelist

@@ -0,0 +1,2 @@
+FROM alpine@sha256:def822f9851ca422481ec6fee59a9966f12b351c62ccb9aca841526ffaa9f748
+RUN ln -s /dev/null /hello

integration/images.go

@@ -75,17 +75,38 @@ var additionalDockerFlagsMap = map[string][]string{
 
 // Arguments to build Dockerfiles with when building with kaniko
 var additionalKanikoFlagsMap = map[string][]string{
-	"Dockerfile_test_add":        {"--single-snapshot"},
+	"Dockerfile_test_add":                    {"--single-snapshot"},
-	"Dockerfile_test_run_new":    {"--use-new-run=true"},
+	"Dockerfile_test_run_new":                {"--use-new-run=true"},
-	"Dockerfile_test_run_redo":   {"--snapshotMode=redo"},
+	"Dockerfile_test_run_redo":               {"--snapshotMode=redo"},
-	"Dockerfile_test_scratch":    {"--single-snapshot"},
+	"Dockerfile_test_scratch":                {"--single-snapshot"},
-	"Dockerfile_test_maintainer": {"--single-snapshot"},
+	"Dockerfile_test_maintainer":             {"--single-snapshot"},
-	"Dockerfile_test_target":     {"--target=second"},
+	"Dockerfile_test_target":                 {"--target=second"},
+	"Dockerfile_test_snapshotter_ignorelist": {"--use-new-run=true", "-v=debug"},
 }
 
 // output check to do when building with kaniko
 var outputChecks = map[string]func(string, []byte) error{
 	"Dockerfile_test_arg_secret": checkArgsNotPrinted,
+	"Dockerfile_test_snapshotter_ignorelist": func(_ string, out []byte) error {
+		for _, s := range []string{
+			"Adding whiteout for /dev",
+		} {
+			if strings.Contains(string(out), s) {
+				return fmt.Errorf("output must not contain %s", s)
+			}
+		}
+
+		for _, s := range []string{
+			"resolved symlink /hello to /dev/null",
+			"path /dev/null is ignored, ignoring it",
+		} {
+			if !strings.Contains(string(out), s) {
+				return fmt.Errorf("output must contain %s", s)
+			}
+		}
+
+		return nil
+	},
 }
 
 // Checks if argument are not printed in output.
@@ -450,11 +471,11 @@ func buildKanikoImage(
 
 	out, err := RunCommandWithoutTest(kanikoCmd)
 	if err != nil {
-		return "", fmt.Errorf("Failed to build image %s with kaniko command \"%s\": %s %s", kanikoImage, kanikoCmd.Args, err, string(out))
+		return "", fmt.Errorf("Failed to build image %s with kaniko command \"%s\": %s\n%s", kanikoImage, kanikoCmd.Args, err, string(out))
 	}
 	if outputCheck := outputChecks[dockerfile]; outputCheck != nil {
 		if err := outputCheck(dockerfile, out); err != nil {
-			return "", fmt.Errorf("Output check failed for image %s with kaniko command : %s %s", kanikoImage, err, string(out))
+			return "", fmt.Errorf("Output check failed for image %s with kaniko command : %s\n%s", kanikoImage, err, string(out))
 		}
 	}
 	return benchmarkDir, nil

pkg/executor/build.go

@@ -98,6 +98,11 @@ func newStageBuilder(opts *config.KanikoOptions, stage config.KanikoStage, cross
 		return nil, err
 	}
 
+	err = util.InitIgnoreList(true)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to initialize ignore list")
+	}
+
 	hasher, err := getHasher(opts.SnapshotMode)
 	if err != nil {
 		return nil, err
@@ -311,10 +316,6 @@ func (s *stageBuilder) build() error {
 		logrus.Info("Skipping unpacking as no commands require it.")
 	}
 
-	if err := util.DetectFilesystemIgnoreList(config.IgnoreListPath); err != nil {
-		return errors.Wrap(err, "failed to check filesystem mount paths")
-	}
-
 	initSnapshotTaken := false
 	if s.opts.SingleSnapshot || s.opts.RunV2 {
 		if err := s.initSnapshotWithTimings(); err != nil {

pkg/filesystem/resolve.go

@@ -74,10 +74,13 @@ func ResolvePaths(paths []string, wl []util.IgnoreListEntry) (pathsToAdd []strin
 			logrus.Debugf("symlink path %s, target does not exist", f)
 			continue
 		}
+		if f != evaled {
+			logrus.Debugf("resolved symlink %s to %s", f, evaled)
+		}
 
 		// If the given path is a symlink and the target is part of the ignorelist
 		// ignore the target
-		if util.IsInProvidedIgnoreList(evaled, wl) {
+		if util.CheckProvidedIgnoreList(evaled, wl) {
 			logrus.Debugf("path %s is ignored, ignoring it", evaled)
 			continue
 		}

pkg/util/fs_util.go

@@ -74,8 +74,7 @@ var defaultIgnoreList = []IgnoreListEntry{
 	},
 }
 
-var baseIgnoreList = defaultIgnoreList
+var ignorelist = append([]IgnoreListEntry{}, defaultIgnoreList...)
-var ignorelist = baseIgnoreList
 
 var volumes = []string{}
 
@@ -101,8 +100,8 @@ func AddToIgnoreList(entry IgnoreListEntry) {
 	ignorelist = append(ignorelist, entry)
 }
 
-func AddToBaseIgnoreList(entry IgnoreListEntry) {
+func AddToDefaultIgnoreList(entry IgnoreListEntry) {
-	baseIgnoreList = append(baseIgnoreList, entry)
+	defaultIgnoreList = append(defaultIgnoreList, entry)
 }
 
 func IncludeWhiteout() FSOpt {
@@ -133,7 +132,12 @@ func GetFSFromImage(root string, img v1.Image, extract ExtractFunction) ([]strin
 }
 
 func GetFSFromLayers(root string, layers []v1.Layer, opts ...FSOpt) ([]string, error) {
+	volumes = []string{}
 	cfg := new(FSConfig)
+	if err := InitIgnoreList(true); err != nil {
+		return nil, errors.Wrap(err, "initializing filesystem ignore list")
+	}
+	logrus.Debugf("Ignore list: %v", ignorelist)
 
 	for _, opt := range opts {
 		opt(cfg)
@@ -143,12 +147,6 @@ func GetFSFromLayers(root string, layers []v1.Layer, opts ...FSOpt) ([]string, e
 		return nil, errors.New("must supply an extract function")
 	}
 
-	if err := DetectFilesystemIgnoreList(config.IgnoreListPath); err != nil {
-		return nil, err
-	}
-
-	logrus.Debugf("Mounted directories: %v", ignorelist)
-
 	extractedFiles := []string{}
 	for i, l := range layers {
 		if mediaType, err := l.MediaType(); err == nil {
@@ -182,7 +180,18 @@ func GetFSFromLayers(root string, layers []v1.Layer, opts ...FSOpt) ([]string, e
 				logrus.Debugf("Whiting out %s", path)
 
 				name := strings.TrimPrefix(base, ".wh.")
-				if err := os.RemoveAll(filepath.Join(dir, name)); err != nil {
+				path := filepath.Join(dir, name)
+
+				if CheckIgnoreList(path) {
+					logrus.Debugf("Not deleting %s, as it's ignored", path)
+					continue
+				}
+				if childDirInIgnoreList(path) {
+					logrus.Debugf("Not deleting %s, as it contains a ignored path", path)
+					continue
+				}
+
+				if err := os.RemoveAll(path); err != nil {
 					return nil, errors.Wrapf(err, "removing whiteout %s", hdr.Name)
 				}
 
@@ -382,20 +391,21 @@ func ExtractFile(dest string, hdr *tar.Header, tr io.Reader) error {
 	return nil
 }
 
-func IsInIgnoreList(path string) bool {
-	return IsInProvidedIgnoreList(path, ignorelist)
-}
-
 func IsInProvidedIgnoreList(path string, wl []IgnoreListEntry) bool {
 	for _, entry := range wl {
 		if !entry.PrefixMatchOnly && path == entry.Path {
 			return true
 		}
 	}
+
 	return false
 }
 
-func CheckIgnoreList(path string) bool {
+func IsInIgnoreList(path string) bool {
+	return IsInProvidedIgnoreList(path, ignorelist)
+}
+
+func CheckProvidedIgnoreList(path string, wl []IgnoreListEntry) bool {
 	for _, wl := range ignorelist {
 		if HasFilepathPrefix(path, wl.Path, wl.PrefixMatchOnly) {
 			return true
@@ -405,6 +415,10 @@ func CheckIgnoreList(path string) bool {
 	return false
 }
 
+func CheckIgnoreList(path string) bool {
+	return CheckProvidedIgnoreList(path, ignorelist)
+}
+
 func checkIgnoreListRoot(root string) bool {
 	if root == config.RootDir {
 		return false
@@ -419,8 +433,7 @@ func checkIgnoreListRoot(root string) bool {
 // Where (5) is the mount point relative to the process's root
 // From: https://www.kernel.org/doc/Documentation/filesystems/proc.txt
 func DetectFilesystemIgnoreList(path string) error {
-	ignorelist = baseIgnoreList
+	logrus.Trace("Detecting filesystem ignore list")
-	volumes = []string{}
 	f, err := os.Open(path)
 	if err != nil {
 		return err
@@ -442,7 +455,7 @@ func DetectFilesystemIgnoreList(path string) error {
 			continue
 		}
 		if lineArr[4] != config.RootDir {
-			logrus.Tracef("Appending %s from line: %s", lineArr[4], line)
+			logrus.Tracef("Adding ignore list entry %s from line: %s", lineArr[4], line)
 			ignorelist = append(ignorelist, IgnoreListEntry{
 				Path:            lineArr[4],
 				PrefixMatchOnly: false,
@@ -898,19 +911,20 @@ func createParentDirectory(path string) error {
 	return nil
 }
 
-// UpdateInitialIgnoreList will add /var/run to ignored paths if
+// InitIgnoreList will initialize the ignore list using:
-func UpdateInitialIgnoreList(ignoreVarRun bool) {
+// - defaultIgnoreList
-	if !ignoreVarRun {
+// - mounted paths via DetectFilesystemIgnoreList()
-		return
+func InitIgnoreList(detectFilesystem bool) error {
+	logrus.Trace("Initializing ignore list")
+	ignorelist = append([]IgnoreListEntry{}, defaultIgnoreList...)
+
+	if detectFilesystem {
+		if err := DetectFilesystemIgnoreList(config.IgnoreListPath); err != nil {
+			return errors.Wrap(err, "checking filesystem mount paths for ignore list")
+		}
 	}
-	logrus.Trace("Adding /var/run to initialIgnoreList ")
+
-	baseIgnoreList = append(baseIgnoreList, IgnoreListEntry{
+	return nil
-		// /var/run is a special case. It's common to mount in /var/run/docker.sock or something similar
-		// which leads to a special mount on the /var/run/docker.sock file itself, but the directory to exist
-		// in the image with no way to tell if it came from the base image or not.
-		Path:            "/var/run",
-		PrefixMatchOnly: false,
-	})
 }
 
 type walkFSResult struct {
@@ -985,7 +999,7 @@ func GetFSInfoMap(dir string, existing map[string]os.FileInfo) (map[string]os.Fi
 	timer := timing.Start("Walking filesystem with Stat")
 	godirwalk.Walk(dir, &godirwalk.Options{
 		Callback: func(path string, ent *godirwalk.Dirent) error {
-			if IsInIgnoreList(path) {
+			if CheckIgnoreList(path) {
 				if IsDestDir(path) {
 					logrus.Tracef("Skipping paths under %s, as it is a ignored directory", path)
 					return filepath.SkipDir

pkg/util/fs_util_test.go

@@ -31,6 +31,7 @@ import (
 	"time"
 
 	"github.com/GoogleContainerTools/kaniko/pkg/config"
+	"github.com/GoogleContainerTools/kaniko/pkg/constants"
 	"github.com/GoogleContainerTools/kaniko/pkg/mocks/go-containerregistry/mockv1"
 	"github.com/GoogleContainerTools/kaniko/testutil"
 	"github.com/golang/mock/gomock"
@@ -79,6 +80,10 @@ func Test_DetectFilesystemSkiplist(t *testing.T) {
 }
 
 func Test_AddToIgnoreList(t *testing.T) {
+	t.Cleanup(func() {
+		ignorelist = append([]IgnoreListEntry{}, defaultIgnoreList...)
+	})
+
 	AddToIgnoreList(IgnoreListEntry{
 		Path:            "/tmp",
 		PrefixMatchOnly: false,
@@ -89,21 +94,6 @@ func Test_AddToIgnoreList(t *testing.T) {
 	}
 }
 
-func Test_AddToBaseIgnoreList(t *testing.T) {
-	t.Cleanup(func() {
-		baseIgnoreList = defaultIgnoreList
-	})
-
-	AddToBaseIgnoreList(IgnoreListEntry{
-		Path:            "/tmp",
-		PrefixMatchOnly: false,
-	})
-
-	if !IsInProvidedIgnoreList("/tmp", baseIgnoreList) {
-		t.Errorf("CheckIgnoreList() = %v, want %v", false, true)
-	}
-}
-
 var tests = []struct {
 	files         map[string]string
 	directory     string
@@ -1249,6 +1239,152 @@ func Test_GetFSFromLayers_with_whiteouts_include_whiteout_disabled(t *testing.T)
 	}
 }
 
+func Test_GetFSFromLayers_ignorelist(t *testing.T) {
+	ctrl := gomock.NewController(t)
+
+	root, err := ioutil.TempDir("", "layers-test")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.Remove(root)
+	// Write a whiteout path
+	fileContents := []byte("Hello World\n")
+	if err := os.Mkdir(filepath.Join(root, "testdir"), 0775); err != nil {
+		t.Fatal(err)
+	}
+
+	opts := []FSOpt{
+		// I'd rather use the real func (util.ExtractFile)
+		// but you have to be root to chown
+		ExtractFunc(fakeExtract),
+		IncludeWhiteout(),
+	}
+
+	f := func(expectedFiles []string, tw *tar.Writer) {
+		for _, f := range expectedFiles {
+			f := strings.TrimPrefix(strings.TrimPrefix(f, root), "/")
+
+			hdr := &tar.Header{
+				Name: f,
+				Mode: 0644,
+				Size: int64(len(string(fileContents))),
+			}
+
+			if err := tw.WriteHeader(hdr); err != nil {
+				t.Fatal(err)
+			}
+
+			if _, err := tw.Write(fileContents); err != nil {
+				t.Fatal(err)
+			}
+		}
+
+		if err := tw.Close(); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	// first, testdir is not in ignorelist, so it should be deleted
+	expectedFiles := []string{
+		filepath.Join(root, ".wh.testdir"),
+		filepath.Join(root, "testdir", "file"),
+		filepath.Join(root, "other-file"),
+	}
+
+	buf := new(bytes.Buffer)
+	tw := tar.NewWriter(buf)
+
+	f(expectedFiles, tw)
+
+	mockLayer := mockv1.NewMockLayer(ctrl)
+	mockLayer.EXPECT().MediaType().Return(types.OCILayer, nil)
+	layerFiles := []string{
+		filepath.Join(root, ".wh.testdir"),
+		filepath.Join(root, "testdir", "file"),
+		filepath.Join(root, "other-file"),
+	}
+	buf = new(bytes.Buffer)
+	tw = tar.NewWriter(buf)
+
+	f(layerFiles, tw)
+
+	rc := ioutil.NopCloser(buf)
+	mockLayer.EXPECT().Uncompressed().Return(rc, nil)
+
+	layers := []v1.Layer{
+		mockLayer,
+	}
+
+	actualFiles, err := GetFSFromLayers(root, layers, opts...)
+	assertGetFSFromLayers(
+		t,
+		actualFiles,
+		expectedFiles,
+		err,
+		false,
+	)
+
+	// Make sure whiteout files are removed form the root.
+	_, err = os.Lstat(filepath.Join(root, "testdir"))
+	if err == nil || !os.IsNotExist(err) {
+		t.Errorf("expected testdir to be deleted. However found it.")
+	}
+
+	// second, testdir is in ignorelist, so it should not be deleted
+	original := append([]IgnoreListEntry{}, defaultIgnoreList...)
+	defer func() {
+		defaultIgnoreList = original
+	}()
+	defaultIgnoreList = append(defaultIgnoreList, IgnoreListEntry{
+		Path: filepath.Join(root, "testdir"),
+	})
+	if err := os.Mkdir(filepath.Join(root, "testdir"), 0775); err != nil {
+		t.Fatal(err)
+	}
+
+	expectedFiles = []string{
+		filepath.Join(root, "other-file"),
+	}
+
+	buf = new(bytes.Buffer)
+	tw = tar.NewWriter(buf)
+
+	f(expectedFiles, tw)
+
+	mockLayer = mockv1.NewMockLayer(ctrl)
+	mockLayer.EXPECT().MediaType().Return(types.OCILayer, nil)
+	layerFiles = []string{
+		filepath.Join(root, ".wh.testdir"),
+		filepath.Join(root, "other-file"),
+	}
+	buf = new(bytes.Buffer)
+	tw = tar.NewWriter(buf)
+
+	f(layerFiles, tw)
+
+	rc = ioutil.NopCloser(buf)
+	mockLayer.EXPECT().Uncompressed().Return(rc, nil)
+
+	layers = []v1.Layer{
+		mockLayer,
+	}
+
+	actualFiles, err = GetFSFromLayers(root, layers, opts...)
+	assertGetFSFromLayers(
+		t,
+		actualFiles,
+		expectedFiles,
+		err,
+		false,
+	)
+
+	// Make sure testdir still exists.
+	_, err = os.Lstat(filepath.Join(root, "testdir"))
+	if err != nil {
+		t.Errorf("expected testdir to exist, but could not Lstat it: %v", err)
+	}
+}
+
 func Test_GetFSFromLayers(t *testing.T) {
 	ctrl := gomock.NewController(t)
 
@@ -1320,8 +1456,9 @@ func assertGetFSFromLayers(
 	actualFiles []string,
 	expectedFiles []string,
 	err error,
-	expectErr bool,
+	expectErr bool, //nolint:unparam
 ) {
+	t.Helper()
 	if !expectErr && err != nil {
 		t.Error(err)
 		t.FailNow()
@@ -1342,66 +1479,60 @@ func assertGetFSFromLayers(
 	}
 }
 
-func TestUpdateSkiplist(t *testing.T) {
+func TestInitIgnoreList(t *testing.T) {
-	tests := []struct {
+	mountInfo := `36 35 98:0 /kaniko /test/kaniko rw,noatime master:1 - ext3 /dev/root rw,errors=continue
-		name       string
+36 35 98:0 /proc /test/proc rw,noatime master:1 - ext3 /dev/root rw,errors=continue
-		skipVarRun bool
+`
-		expected   []IgnoreListEntry
+	mFile, err := ioutil.TempFile("", "mountinfo")
-	}{
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer mFile.Close()
+	if _, err := mFile.WriteString(mountInfo); err != nil {
+		t.Fatal(err)
+	}
+	config.IgnoreListPath = mFile.Name()
+	defer func() {
+		config.IgnoreListPath = constants.IgnoreListPath
+	}()
+
+	expected := []IgnoreListEntry{
 		{
-			name:       "var/run ignored",
+			Path:            "/kaniko",
-			skipVarRun: true,
+			PrefixMatchOnly: false,
-			expected: []IgnoreListEntry{
-				{
-					Path:            "/kaniko",
-					PrefixMatchOnly: false,
-				},
-				{
-					Path:            "/etc/mtab",
-					PrefixMatchOnly: false,
-				},
-				{
-					Path:            "/var/run",
-					PrefixMatchOnly: false,
-				},
-				{
-					Path:            "/tmp/apt-key-gpghome",
-					PrefixMatchOnly: true,
-				},
-			},
 		},
 		{
-			name: "var/run not ignored",
+			Path:            "/test/kaniko",
-			expected: []IgnoreListEntry{
+			PrefixMatchOnly: false,
-				{
+		},
-					Path:            "/kaniko",
+		{
-					PrefixMatchOnly: false,
+			Path:            "/test/proc",
-				},
+			PrefixMatchOnly: false,
-				{
+		},
-					Path:            "/etc/mtab",
+		{
-					PrefixMatchOnly: false,
+			Path:            "/etc/mtab",
-				},
+			PrefixMatchOnly: false,
-				{
+		},
-					Path:            "/tmp/apt-key-gpghome",
+		{
-					PrefixMatchOnly: true,
+			Path:            "/tmp/apt-key-gpghome",
-				},
+			PrefixMatchOnly: true,
-			},
 		},
 	}
-	for _, tt := range tests {
+
-		t.Run(tt.name, func(t *testing.T) {
+	original := append([]IgnoreListEntry{}, ignorelist...)
-			original := baseIgnoreList
+	defer func() { ignorelist = original }()
-			defer func() { baseIgnoreList = original }()
+
-			UpdateInitialIgnoreList(tt.skipVarRun)
+	err = InitIgnoreList(true)
-			sort.Slice(tt.expected, func(i, j int) bool {
+	if err != nil {
-				return tt.expected[i].Path < tt.expected[j].Path
+		t.Fatal(err)
-			})
-			sort.Slice(baseIgnoreList, func(i, j int) bool {
-				return baseIgnoreList[i].Path < baseIgnoreList[j].Path
-			})
-			testutil.CheckDeepEqual(t, tt.expected, baseIgnoreList)
-		})
 	}
+	sort.Slice(expected, func(i, j int) bool {
+		return expected[i].Path < expected[j].Path
+	})
+	sort.Slice(ignorelist, func(i, j int) bool {
+		return ignorelist[i].Path < ignorelist[j].Path
+	})
+	testutil.CheckDeepEqual(t, expected, ignorelist)
 }
 
 func Test_setFileTimes(t *testing.T) {