Merge pull request #334 from peter-evans/fix-volume-cmd

Fix handling of the 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

@@ -214,9 +214,6 @@ func TestLayers(t *testing.T) {
 	offset := map[string]int{
 		"Dockerfile_test_add":     11,
 		"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,15 +48,15 @@ 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
 		}
 
 		// 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,17 +193,21 @@ 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{}
 				}
 			}
 		}
 		if err != nil {
 			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 err != nil {
-			return false, fmt.Errorf("Error checking if parent dir %s can be snapshotted: %s", file, err)
-		}
-		if !shouldSnapshot {
+		if val, ok := snapshottedFiles[file]; ok && val {
 			continue
 		}
 		snapshottedFiles[file] = true
@@ -148,35 +128,22 @@ 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 err != nil {
-			return false, fmt.Errorf("Error checking if file %s can be snapshotted: %s", file, err)
-		}
-		if !shouldSnapshot {
+		if val, ok := snapshottedFiles[file]; ok && val {
 			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
 	}
 	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{
-	"/kaniko",
-	// /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",
-	// 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",
+type WhitelistEntry struct {
+	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.
+		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
+		Path:            "/etc/mtab",
+		PrefixMatchOnly: false,
+	},
 }
-var volumeWhitelist = []string{}
 
 // GetFSFromImage extracts the layers of img to root
 // It returns a list of all files extracted
@@ -118,13 +131,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)
-		if HasFilepathPrefix(dirPath, path) {
+		dirPath := filepath.Join(directory, d.Path)
+		if HasFilepathPrefix(dirPath, path, d.PrefixMatchOnly) {
 			return true
 		}
 	}
@@ -265,7 +278,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
 		}
 	}
@@ -277,7 +290,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
 		}
 	}
@@ -290,7 +303,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
@@ -313,7 +326,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)
@@ -336,7 +352,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 {
@@ -399,22 +415,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
-// It will get snapshotted when the VOLUME command is run then ignored
-// for subsequent commands.
-func AddPathToVolumeWhitelist(path string) error {
-	logrus.Infof("adding %s to volume whitelist", path)
-	volumeWhitelist = append(volumeWhitelist, path)
-	return nil
-}
-
-// 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{}
-	}
+// AddVolumePathToWhitelist adds the given path to the whitelist with
+// PrefixMatchOnly set to true. Snapshotting will ignore paths prefixed
+// with the volume, but the volume itself will not be ignored.
+func AddVolumePathToWhitelist(path string) error {
+	logrus.Infof("adding volume %s to whitelist", path)
+	whitelist = append(whitelist, WhitelistEntry{
+		Path:            path,
+		PrefixMatchOnly: true,
+	})
 	return nil
 }
 
@@ -514,7 +523,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, "/")
@@ -523,6 +532,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