Fix handling of volume directive

integration/dockerfiles/Dockerfile_test_volume

@@ -1,7 +1,7 @@
 FROM gcr.io/google-appengine/debian9@sha256:1d6a9a6d106bd795098f60f4abb7083626354fa6735e81743c7f8cfca11259f0
 RUN mkdir /foo
 RUN echo "hello" > /foo/hey
-VOLUME /foo/bar /tmp
+VOLUME /foo/bar /tmp /qux/quux
 ENV VOL /baz/bat
 VOLUME ["${VOL}"]
 RUN echo "hello again" > /tmp/hey

integration/dockerfiles/Dockerfile_test_volume_2

@@ -0,0 +1,13 @@
+FROM gcr.io/google-appengine/debian9@sha256:1d6a9a6d106bd795098f60f4abb7083626354fa6735e81743c7f8cfca11259f0
+VOLUME /foo1
+RUN echo "hello" > /foo1/hello
+WORKDIR /foo1/bar
+ADD context/foo /foo1/foo
+COPY context/foo /foo1/foo2
+RUN mkdir /bar1
+VOLUME /foo2
+VOLUME /foo3
+RUN echo "bar2"
+VOLUME /foo4
+RUN mkdir /bar3
+VOLUME /foo5

integration/integration_test.go

@@ -213,9 +213,6 @@ func TestLayers(t *testing.T) {
 	offset := map[string]int{
 		"Dockerfile_test_add":     10,
 		"Dockerfile_test_scratch": 3,
-		// the Docker built image combined some of the dirs defined by separate VOLUME commands into one layer
-		// which is why this offset exists
-		"Dockerfile_test_volume": 1,
 	}
 	for dockerfile, built := range imageBuilder.FilesBuilt {
 		t.Run("test_layer_"+dockerfile, func(t *testing.T) {

pkg/commands/volume.go

@@ -48,7 +48,7 @@ func (v *VolumeCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.
 	for _, volume := range resolvedVolumes {
 		var x struct{}
 		existingVolumes[volume] = x
-		err := util.AddPathToVolumeWhitelist(volume)
+		err := util.AddVolumePathToWhitelist(volume)
 		if err != nil {
 			return err
 		}
@@ -56,7 +56,7 @@ func (v *VolumeCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.
 		// Only create and snapshot the dir if it didn't exist already
 		if _, err := os.Stat(volume); os.IsNotExist(err) {
 			logrus.Infof("Creating directory %s", volume)
-			v.snapshotFiles = []string{volume}
+			v.snapshotFiles = append(v.snapshotFiles, volume)
 			if err := os.MkdirAll(volume, 0755); err != nil {
 				return fmt.Errorf("Could not create directory for volume %s: %s", volume, err)
 			}

pkg/constants/constants.go

@@ -62,6 +62,9 @@ const (
 	// Docker command names
 	Cmd        = "cmd"
 	Entrypoint = "entrypoint"
+
+	// VolumeCmdName is the name of the volume command
+	VolumeCmdName = "volume"
 )
 
 // KanikoBuildFiles is the list of files required to build kaniko

pkg/executor/build.go

@@ -138,6 +138,7 @@ func (s *stageBuilder) build(opts *config.KanikoOptions) error {
 	if err := s.snapshotter.Init(); err != nil {
 		return err
 	}
+	var volumes []string
 	args := dockerfile.NewBuildArgs(opts.BuildArgs)
 	for index, cmd := range s.stage.Commands {
 		finalCmd := index == len(s.stage.Commands)-1
@@ -167,6 +168,10 @@ func (s *stageBuilder) build(opts *config.KanikoOptions) error {
 			return err
 		}
 		files := command.FilesToSnapshot()
+		if cmd.Name() == constants.VolumeCmdName {
+			volumes = append(volumes, files...)
+			continue
+		}
 		var contents []byte
 
 		// If this is an intermediate stage, we only snapshot for the last command and we
@@ -188,9 +193,14 @@ func (s *stageBuilder) build(opts *config.KanikoOptions) error {
 				// the files that were changed, we'll snapshot those explicitly, otherwise we'll
 				// check if anything in the filesystem changed.
 				if files != nil {
+					if len(files) > 0 {
+						files = append(files, volumes...)
+						volumes = []string{}
+					}
 					contents, err = s.snapshotter.TakeSnapshot(files)
 				} else {
 					contents, err = s.snapshotter.TakeSnapshotFS()
+					volumes = []string{}
 				}
 			}
 		}
@@ -198,7 +208,6 @@ func (s *stageBuilder) build(opts *config.KanikoOptions) error {
 			return fmt.Errorf("Error taking snapshot of files for command %s: %s", command, err)
 		}
 
-		util.MoveVolumeWhitelistToWhitelist()
 		if contents == nil {
 			logrus.Info("No files were changed, appending empty layer to config. No layer added to image.")
 			continue

pkg/snapshot/snapshot.go

@@ -25,7 +25,6 @@ import (
 	"path/filepath"
 	"syscall"
 
-	"github.com/GoogleContainerTools/kaniko/pkg/constants"
 	"github.com/GoogleContainerTools/kaniko/pkg/util"
 	"github.com/sirupsen/logrus"
 )
@@ -84,21 +83,6 @@ func (s *Snapshotter) TakeSnapshotFS() ([]byte, error) {
 	return contents, err
 }
 
-func shouldSnapshot(file string, snapshottedFiles map[string]bool) (bool, error) {
-	if val, ok := snapshottedFiles[file]; ok && val {
-		return false, nil
-	}
-	whitelisted, err := util.CheckWhitelist(file)
-	if err != nil {
-		return false, fmt.Errorf("Error checking for %s in whitelist: %s", file, err)
-	}
-	if whitelisted && !isBuildFile(file) {
-		logrus.Infof("Not adding %s to layer, as it's whitelisted", file)
-		return false, nil
-	}
-	return true, nil
-}
-
 // snapshotFiles creates a snapshot (tar) and adds the specified files.
 // It will not add files which are whitelisted.
 func (s *Snapshotter) snapshotFiles(f io.Writer, files []string) (bool, error) {
@@ -123,11 +107,7 @@ func (s *Snapshotter) snapshotFiles(f io.Writer, files []string) (bool, error) {
 	}
 	for _, file := range parentDirs {
 		file = filepath.Clean(file)
-		shouldSnapshot, err := shouldSnapshot(file, snapshottedFiles)
+		if val, ok := snapshottedFiles[file]; ok && val {
-		if err != nil {
-			return false, fmt.Errorf("Error checking if parent dir %s can be snapshotted: %s", file, err)
-		}
-		if !shouldSnapshot {
 			continue
 		}
 		snapshottedFiles[file] = true
@@ -148,19 +128,15 @@ func (s *Snapshotter) snapshotFiles(f io.Writer, files []string) (bool, error) {
 	// Next add the files themselves to the tar
 	for _, file := range files {
 		file = filepath.Clean(file)
-		shouldSnapshot, err := shouldSnapshot(file, snapshottedFiles)
+		if val, ok := snapshottedFiles[file]; ok && val {
-		if err != nil {
-			return false, fmt.Errorf("Error checking if file %s can be snapshotted: %s", file, err)
-		}
-		if !shouldSnapshot {
 			continue
 		}
 		snapshottedFiles[file] = true
 
-		if err = s.l.Add(file); err != nil {
+		if err := s.l.Add(file); err != nil {
 			return false, fmt.Errorf("Unable to add file %s to layered map: %s", file, err)
 		}
-		if err = t.AddFileToTar(file); err != nil {
+		if err := t.AddFileToTar(file); err != nil {
 			return false, fmt.Errorf("Error adding file %s to tar: %s", file, err)
 		}
 		filesAdded = true
@@ -168,15 +144,6 @@ func (s *Snapshotter) snapshotFiles(f io.Writer, files []string) (bool, error) {
 	return filesAdded, nil
 }
 
-func isBuildFile(file string) bool {
-	for _, buildFile := range constants.KanikoBuildFiles {
-		if file == buildFile {
-			return true
-		}
-	}
-	return false
-}
-
 // shapShotFS creates a snapshot (tar) of all files in the system which are not
 // whitelisted and which have changed.
 func (s *Snapshotter) snapShotFS(f io.Writer) (bool, error) {

pkg/util/fs_util.go

@@ -34,17 +34,30 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-var whitelist = []string{
+type WhitelistEntry struct {
-	"/kaniko",
+	Path            string
+	PrefixMatchOnly bool
+}
+
+var whitelist = []WhitelistEntry{
+	{
+		Path:            "/kaniko",
+		PrefixMatchOnly: false,
+	},
+	{
 		// /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.
-	"/var/run",
+		Path:            "/var/run",
+		PrefixMatchOnly: false,
+	},
+	{
 		// similarly, we whitelist /etc/mtab, since there is no way to know if the file was mounted or came
 		// from the base image
-	"/etc/mtab",
+		Path:            "/etc/mtab",
+		PrefixMatchOnly: false,
+	},
 }
-var volumeWhitelist = []string{}
 
 // GetFSFromImage extracts the layers of img to root
 // It returns a list of all files extracted
@@ -136,13 +149,13 @@ func DeleteFilesystem() error {
 func ChildDirInWhitelist(path, directory string) bool {
 	for _, d := range constants.KanikoBuildFiles {
 		dirPath := filepath.Join(directory, d)
-		if HasFilepathPrefix(dirPath, path) {
+		if HasFilepathPrefix(dirPath, path, false) {
 			return true
 		}
 	}
 	for _, d := range whitelist {
-		dirPath := filepath.Join(directory, d)
+		dirPath := filepath.Join(directory, d.Path)
-		if HasFilepathPrefix(dirPath, path) {
+		if HasFilepathPrefix(dirPath, path, d.PrefixMatchOnly) {
 			return true
 		}
 	}
@@ -266,7 +279,7 @@ func CheckWhitelist(path string) (bool, error) {
 		return false, err
 	}
 	for _, wl := range whitelist {
-		if HasFilepathPrefix(abs, wl) {
+		if HasFilepathPrefix(abs, wl.Path, wl.PrefixMatchOnly) {
 			return true, nil
 		}
 	}
@@ -278,7 +291,7 @@ func checkWhitelistRoot(root string) bool {
 		return false
 	}
 	for _, wl := range whitelist {
-		if HasFilepathPrefix(root, wl) {
+		if HasFilepathPrefix(root, wl.Path, wl.PrefixMatchOnly) {
 			return true
 		}
 	}
@@ -291,7 +304,7 @@ func checkWhitelistRoot(root string) bool {
 // (1)(2)(3)   (4)   (5)      (6)      (7)   (8) (9)   (10)         (11)
 // Where (5) is the mount point relative to the process's root
 // From: https://www.kernel.org/doc/Documentation/filesystems/proc.txt
-func fileSystemWhitelist(path string) ([]string, error) {
+func fileSystemWhitelist(path string) ([]WhitelistEntry, error) {
 	f, err := os.Open(path)
 	if err != nil {
 		return nil, err
@@ -314,7 +327,10 @@ func fileSystemWhitelist(path string) ([]string, error) {
 		}
 		if lineArr[4] != constants.RootDir {
 			logrus.Debugf("Appending %s from line: %s", lineArr[4], line)
-			whitelist = append(whitelist, lineArr[4])
+			whitelist = append(whitelist, WhitelistEntry{
+				Path:            lineArr[4],
+				PrefixMatchOnly: false,
+			})
 		}
 		if err == io.EOF {
 			logrus.Debugf("Reached end of file %s", path)
@@ -337,7 +353,7 @@ func RelativeFiles(fp string, root string) ([]string, error) {
 		if err != nil {
 			return err
 		}
-		if whitelisted && !HasFilepathPrefix(path, root) {
+		if whitelisted && !HasFilepathPrefix(path, root, false) {
 			return nil
 		}
 		if err != nil {
@@ -400,22 +416,15 @@ func CreateFile(path string, reader io.Reader, perm os.FileMode, uid uint32, gid
 	return dest.Chown(int(uid), int(gid))
 }
 
-// AddPathToVolumeWhitelist adds the given path to the volume whitelist
+// AddVolumePathToWhitelist adds the given path to the whitelist with
-// It will get snapshotted when the VOLUME command is run then ignored
+// PrefixMatchOnly set to true. Snapshotting will ignore paths prefixed
-// for subsequent commands.
+// with the volume, but the volume itself will not be ignored.
-func AddPathToVolumeWhitelist(path string) error {
+func AddVolumePathToWhitelist(path string) error {
-	logrus.Infof("adding %s to volume whitelist", path)
+	logrus.Infof("adding volume %s to whitelist", path)
-	volumeWhitelist = append(volumeWhitelist, path)
+	whitelist = append(whitelist, WhitelistEntry{
-	return nil
+		Path:            path,
-}
+		PrefixMatchOnly: true,
-
+	})
-// MoveVolumeWhitelistToWhitelist copies over all directories that were volume mounted
-// in this step to be whitelisted for all subsequent docker commands.
-func MoveVolumeWhitelistToWhitelist() error {
-	if len(volumeWhitelist) > 0 {
-		whitelist = append(whitelist, volumeWhitelist...)
-		volumeWhitelist = []string{}
-	}
 	return nil
 }
 
@@ -515,7 +524,7 @@ func CopyFile(src, dest string) error {
 }
 
 // HasFilepathPrefix checks if the given file path begins with prefix
-func HasFilepathPrefix(path, prefix string) bool {
+func HasFilepathPrefix(path, prefix string, prefixMatchOnly bool) bool {
 	path = filepath.Clean(path)
 	prefix = filepath.Clean(prefix)
 	pathArray := strings.Split(path, "/")
@@ -524,6 +533,9 @@ func HasFilepathPrefix(path, prefix string) bool {
 	if len(pathArray) < len(prefixArray) {
 		return false
 	}
+	if prefixMatchOnly && len(pathArray) == len(prefixArray) {
+		return false
+	}
 	for index := range prefixArray {
 		if prefixArray[index] == pathArray[index] {
 			continue

pkg/util/fs_util_test.go

@@ -50,9 +50,21 @@ func Test_fileSystemWhitelist(t *testing.T) {
 	}
 
 	actualWhitelist, err := fileSystemWhitelist(path)
-	expectedWhitelist := []string{"/kaniko", "/proc", "/dev", "/dev/pts", "/sys", "/var/run", "/etc/mtab"}
+	expectedWhitelist := []WhitelistEntry{
-	sort.Strings(actualWhitelist)
+		{"/kaniko", false},
-	sort.Strings(expectedWhitelist)
+		{"/proc", false},
+		{"/dev", false},
+		{"/dev/pts", false},
+		{"/sys", false},
+		{"/var/run", false},
+		{"/etc/mtab", false},
+	}
+	sort.Slice(actualWhitelist, func(i, j int) bool {
+		return actualWhitelist[i].Path < actualWhitelist[j].Path
+	})
+	sort.Slice(expectedWhitelist, func(i, j int) bool {
+		return expectedWhitelist[i].Path < expectedWhitelist[j].Path
+	})
 	testutil.CheckErrorAndDeepEqual(t, false, err, expectedWhitelist, actualWhitelist)
 }
 
@@ -167,7 +179,7 @@ func Test_ParentDirectories(t *testing.T) {
 func Test_CheckWhitelist(t *testing.T) {
 	type args struct {
 		path      string
-		whitelist []string
+		whitelist []WhitelistEntry
 	}
 	tests := []struct {
 		name string
@@ -178,7 +190,7 @@ func Test_CheckWhitelist(t *testing.T) {
 			name: "file whitelisted",
 			args: args{
 				path:      "/foo",
-				whitelist: []string{"/foo"},
+				whitelist: []WhitelistEntry{{"/foo", false}},
 			},
 			want: true,
 		},
@@ -186,7 +198,7 @@ func Test_CheckWhitelist(t *testing.T) {
 			name: "directory whitelisted",
 			args: args{
 				path:      "/foo/bar",
-				whitelist: []string{"/foo"},
+				whitelist: []WhitelistEntry{{"/foo", false}},
 			},
 			want: true,
 		},
@@ -194,7 +206,7 @@ func Test_CheckWhitelist(t *testing.T) {
 			name: "grandparent whitelisted",
 			args: args{
 				path:      "/foo/bar/baz",
-				whitelist: []string{"/foo"},
+				whitelist: []WhitelistEntry{{"/foo", false}},
 			},
 			want: true,
 		},
@@ -202,7 +214,7 @@ func Test_CheckWhitelist(t *testing.T) {
 			name: "sibling whitelisted",
 			args: args{
 				path:      "/foo/bar/baz",
-				whitelist: []string{"/foo/bat"},
+				whitelist: []WhitelistEntry{{"/foo/bat", false}},
 			},
 			want: false,
 		},
@@ -229,6 +241,7 @@ func TestHasFilepathPrefix(t *testing.T) {
 	type args struct {
 		path            string
 		prefix          string
+		prefixMatchOnly bool
 	}
 	tests := []struct {
 		name string
@@ -240,6 +253,7 @@ func TestHasFilepathPrefix(t *testing.T) {
 			args: args{
 				path:            "/foo/bar",
 				prefix:          "/foo",
+				prefixMatchOnly: false,
 			},
 			want: true,
 		},
@@ -248,6 +262,7 @@ func TestHasFilepathPrefix(t *testing.T) {
 			args: args{
 				path:            "/foo/bar/baz",
 				prefix:          "/foo/bar",
+				prefixMatchOnly: false,
 			},
 			want: true,
 		},
@@ -256,6 +271,7 @@ func TestHasFilepathPrefix(t *testing.T) {
 			args: args{
 				path:            "/foo/bar",
 				prefix:          "/bar",
+				prefixMatchOnly: false,
 			},
 			want: false,
 		},
@@ -264,6 +280,7 @@ func TestHasFilepathPrefix(t *testing.T) {
 			args: args{
 				path:            "/foo/bar/baz",
 				prefix:          "/foo/bar",
+				prefixMatchOnly: false,
 			},
 			want: true,
 		},
@@ -272,13 +289,23 @@ func TestHasFilepathPrefix(t *testing.T) {
 			args: args{
 				path:            "/foo2/bar",
 				prefix:          "/foo",
+				prefixMatchOnly: false,
+			},
+			want: false,
+		},
+		{
+			name: "prefix match only (volume)",
+			args: args{
+				path:            "/foo",
+				prefix:          "/foo",
+				prefixMatchOnly: true,
 			},
 			want: false,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := HasFilepathPrefix(tt.args.path, tt.args.prefix); got != tt.want {
+			if got := HasFilepathPrefix(tt.args.path, tt.args.prefix, tt.args.prefixMatchOnly); got != tt.want {
 				t.Errorf("HasFilepathPrefix() = %v, want %v", got, tt.want)
 			}
 		})