From b1e28ddb4f56f67a8ab7523169eb407644a878c4 Mon Sep 17 00:00:00 2001 From: peter-evans Date: Wed, 26 Sep 2018 13:59:11 +0900 Subject: [PATCH] Fix handling of volume directive --- .../dockerfiles/Dockerfile_test_volume | 2 +- .../dockerfiles/Dockerfile_test_volume_2 | 13 +++ integration/integration_test.go | 3 - pkg/commands/volume.go | 4 +- pkg/constants/constants.go | 3 + pkg/executor/build.go | 11 ++- pkg/snapshot/snapshot.go | 41 +--------- pkg/util/fs_util.go | 82 +++++++++++-------- pkg/util/fs_util_test.go | 69 +++++++++++----- 9 files changed, 128 insertions(+), 100 deletions(-) create mode 100644 integration/dockerfiles/Dockerfile_test_volume_2 diff --git a/integration/dockerfiles/Dockerfile_test_volume b/integration/dockerfiles/Dockerfile_test_volume index 1ffb7a5142..a4db420acb 100644 --- a/integration/dockerfiles/Dockerfile_test_volume +++ b/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 diff --git a/integration/dockerfiles/Dockerfile_test_volume_2 b/integration/dockerfiles/Dockerfile_test_volume_2 new file mode 100644 index 0000000000..77a116a082 --- /dev/null +++ b/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 diff --git a/integration/integration_test.go b/integration/integration_test.go index de2409ed75..c86c62ffd2 100644 --- a/integration/integration_test.go +++ b/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) { diff --git a/pkg/commands/volume.go b/pkg/commands/volume.go index 5c9ecd99c2..01c9fdab28 100644 --- a/pkg/commands/volume.go +++ b/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) } diff --git a/pkg/constants/constants.go b/pkg/constants/constants.go index d8fcc722e9..1c8d774b80 100644 --- a/pkg/constants/constants.go +++ b/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 diff --git a/pkg/executor/build.go b/pkg/executor/build.go index 29f52d6ff4..7cd311b348 100644 --- a/pkg/executor/build.go +++ b/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 diff --git a/pkg/snapshot/snapshot.go b/pkg/snapshot/snapshot.go index 55da45d652..4175d47610 100644 --- a/pkg/snapshot/snapshot.go +++ b/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,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 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 @@ -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) { diff --git a/pkg/util/fs_util.go b/pkg/util/fs_util.go index b7c2baddfa..9ce2cb45a8 100644 --- a/pkg/util/fs_util.go +++ b/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 @@ -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) - if HasFilepathPrefix(dirPath, path) { + dirPath := filepath.Join(directory, d.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 -// 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 } @@ -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 diff --git a/pkg/util/fs_util_test.go b/pkg/util/fs_util_test.go index 99202e1bb8..968cacf992 100644 --- a/pkg/util/fs_util_test.go +++ b/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"} - sort.Strings(actualWhitelist) - sort.Strings(expectedWhitelist) + expectedWhitelist := []WhitelistEntry{ + {"/kaniko", false}, + {"/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, }, @@ -227,8 +239,9 @@ func Test_CheckWhitelist(t *testing.T) { func TestHasFilepathPrefix(t *testing.T) { type args struct { - path string - prefix string + path string + prefix string + prefixMatchOnly bool } tests := []struct { name string @@ -238,47 +251,61 @@ func TestHasFilepathPrefix(t *testing.T) { { name: "parent", args: args{ - path: "/foo/bar", - prefix: "/foo", + path: "/foo/bar", + prefix: "/foo", + prefixMatchOnly: false, }, want: true, }, { name: "nested parent", args: args{ - path: "/foo/bar/baz", - prefix: "/foo/bar", + path: "/foo/bar/baz", + prefix: "/foo/bar", + prefixMatchOnly: false, }, want: true, }, { name: "sibling", args: args{ - path: "/foo/bar", - prefix: "/bar", + path: "/foo/bar", + prefix: "/bar", + prefixMatchOnly: false, }, want: false, }, { name: "nested sibling", args: args{ - path: "/foo/bar/baz", - prefix: "/foo/bar", + path: "/foo/bar/baz", + prefix: "/foo/bar", + prefixMatchOnly: false, }, want: true, }, { name: "name prefix", args: args{ - path: "/foo2/bar", - prefix: "/foo", + 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) } })