From 9d363ded0209dcf7c256938b0d1512ed11f0db21 Mon Sep 17 00:00:00 2001 From: priyawadhwa Date: Mon, 1 Oct 2018 14:54:56 -0700 Subject: [PATCH] Revert "Fix handling of the 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, 100 insertions(+), 128 deletions(-) delete mode 100644 integration/dockerfiles/Dockerfile_test_volume_2 diff --git a/integration/dockerfiles/Dockerfile_test_volume b/integration/dockerfiles/Dockerfile_test_volume index a4db420ac..1ffb7a514 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 /qux/quux +VOLUME /foo/bar /tmp 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 deleted file mode 100644 index 77a116a08..000000000 --- a/integration/dockerfiles/Dockerfile_test_volume_2 +++ /dev/null @@ -1,13 +0,0 @@ -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 67cb7ebbe..008927bde 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -214,6 +214,9 @@ 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) { diff --git a/pkg/commands/volume.go b/pkg/commands/volume.go index 01c9fdab2..5c9ecd99c 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.AddVolumePathToWhitelist(volume) + err := util.AddPathToVolumeWhitelist(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 = append(v.snapshotFiles, volume) + v.snapshotFiles = []string{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 1c8d774b8..d8fcc722e 100644 --- a/pkg/constants/constants.go +++ b/pkg/constants/constants.go @@ -62,9 +62,6 @@ 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 50baad886..080d24b8b 100644 --- a/pkg/executor/build.go +++ b/pkg/executor/build.go @@ -138,7 +138,6 @@ 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 @@ -168,10 +167,6 @@ 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 @@ -193,14 +188,9 @@ 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{} } } } @@ -208,6 +198,7 @@ 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 4175d4761..55da45d65 100644 --- a/pkg/snapshot/snapshot.go +++ b/pkg/snapshot/snapshot.go @@ -25,6 +25,7 @@ import ( "path/filepath" "syscall" + "github.com/GoogleContainerTools/kaniko/pkg/constants" "github.com/GoogleContainerTools/kaniko/pkg/util" "github.com/sirupsen/logrus" ) @@ -83,6 +84,21 @@ 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) { @@ -107,7 +123,11 @@ func (s *Snapshotter) snapshotFiles(f io.Writer, files []string) (bool, error) { } for _, file := range parentDirs { file = filepath.Clean(file) - if val, ok := snapshottedFiles[file]; ok && val { + 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 { continue } snapshottedFiles[file] = true @@ -128,15 +148,19 @@ 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) - if val, ok := snapshottedFiles[file]; ok && val { + 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 { 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 @@ -144,6 +168,15 @@ 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 07e3fbc59..f3c5eb1a6 100644 --- a/pkg/util/fs_util.go +++ b/pkg/util/fs_util.go @@ -34,30 +34,17 @@ import ( "github.com/sirupsen/logrus" ) -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 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", } +var volumeWhitelist = []string{} // GetFSFromImage extracts the layers of img to root // It returns a list of all files extracted @@ -131,13 +118,13 @@ func DeleteFilesystem() error { func ChildDirInWhitelist(path, directory string) bool { for _, d := range constants.KanikoBuildFiles { dirPath := filepath.Join(directory, d) - if HasFilepathPrefix(dirPath, path, false) { + if HasFilepathPrefix(dirPath, path) { return true } } for _, d := range whitelist { - dirPath := filepath.Join(directory, d.Path) - if HasFilepathPrefix(dirPath, path, d.PrefixMatchOnly) { + dirPath := filepath.Join(directory, d) + if HasFilepathPrefix(dirPath, path) { return true } } @@ -278,7 +265,7 @@ func CheckWhitelist(path string) (bool, error) { return false, err } for _, wl := range whitelist { - if HasFilepathPrefix(abs, wl.Path, wl.PrefixMatchOnly) { + if HasFilepathPrefix(abs, wl) { return true, nil } } @@ -290,7 +277,7 @@ func checkWhitelistRoot(root string) bool { return false } for _, wl := range whitelist { - if HasFilepathPrefix(root, wl.Path, wl.PrefixMatchOnly) { + if HasFilepathPrefix(root, wl) { return true } } @@ -303,7 +290,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) ([]WhitelistEntry, error) { +func fileSystemWhitelist(path string) ([]string, error) { f, err := os.Open(path) if err != nil { return nil, err @@ -326,10 +313,7 @@ func fileSystemWhitelist(path string) ([]WhitelistEntry, error) { } if lineArr[4] != constants.RootDir { logrus.Debugf("Appending %s from line: %s", lineArr[4], line) - whitelist = append(whitelist, WhitelistEntry{ - Path: lineArr[4], - PrefixMatchOnly: false, - }) + whitelist = append(whitelist, lineArr[4]) } if err == io.EOF { logrus.Debugf("Reached end of file %s", path) @@ -352,7 +336,7 @@ func RelativeFiles(fp string, root string) ([]string, error) { if err != nil { return err } - if whitelisted && !HasFilepathPrefix(path, root, false) { + if whitelisted && !HasFilepathPrefix(path, root) { return nil } if err != nil { @@ -415,15 +399,22 @@ func CreateFile(path string, reader io.Reader, perm os.FileMode, uid uint32, gid return dest.Chown(int(uid), int(gid)) } -// 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, - }) +// 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{} + } return nil } @@ -523,7 +514,7 @@ func CopyFile(src, dest string) error { } // HasFilepathPrefix checks if the given file path begins with prefix -func HasFilepathPrefix(path, prefix string, prefixMatchOnly bool) bool { +func HasFilepathPrefix(path, prefix string) bool { path = filepath.Clean(path) prefix = filepath.Clean(prefix) pathArray := strings.Split(path, "/") @@ -532,9 +523,6 @@ func HasFilepathPrefix(path, prefix string, prefixMatchOnly bool) 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 968cacf99..99202e1bb 100644 --- a/pkg/util/fs_util_test.go +++ b/pkg/util/fs_util_test.go @@ -50,21 +50,9 @@ func Test_fileSystemWhitelist(t *testing.T) { } actualWhitelist, err := fileSystemWhitelist(path) - 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 - }) + expectedWhitelist := []string{"/kaniko", "/proc", "/dev", "/dev/pts", "/sys", "/var/run", "/etc/mtab"} + sort.Strings(actualWhitelist) + sort.Strings(expectedWhitelist) testutil.CheckErrorAndDeepEqual(t, false, err, expectedWhitelist, actualWhitelist) } @@ -179,7 +167,7 @@ func Test_ParentDirectories(t *testing.T) { func Test_CheckWhitelist(t *testing.T) { type args struct { path string - whitelist []WhitelistEntry + whitelist []string } tests := []struct { name string @@ -190,7 +178,7 @@ func Test_CheckWhitelist(t *testing.T) { name: "file whitelisted", args: args{ path: "/foo", - whitelist: []WhitelistEntry{{"/foo", false}}, + whitelist: []string{"/foo"}, }, want: true, }, @@ -198,7 +186,7 @@ func Test_CheckWhitelist(t *testing.T) { name: "directory whitelisted", args: args{ path: "/foo/bar", - whitelist: []WhitelistEntry{{"/foo", false}}, + whitelist: []string{"/foo"}, }, want: true, }, @@ -206,7 +194,7 @@ func Test_CheckWhitelist(t *testing.T) { name: "grandparent whitelisted", args: args{ path: "/foo/bar/baz", - whitelist: []WhitelistEntry{{"/foo", false}}, + whitelist: []string{"/foo"}, }, want: true, }, @@ -214,7 +202,7 @@ func Test_CheckWhitelist(t *testing.T) { name: "sibling whitelisted", args: args{ path: "/foo/bar/baz", - whitelist: []WhitelistEntry{{"/foo/bat", false}}, + whitelist: []string{"/foo/bat"}, }, want: false, }, @@ -239,9 +227,8 @@ func Test_CheckWhitelist(t *testing.T) { func TestHasFilepathPrefix(t *testing.T) { type args struct { - path string - prefix string - prefixMatchOnly bool + path string + prefix string } tests := []struct { name string @@ -251,61 +238,47 @@ func TestHasFilepathPrefix(t *testing.T) { { name: "parent", args: args{ - path: "/foo/bar", - prefix: "/foo", - prefixMatchOnly: false, + path: "/foo/bar", + prefix: "/foo", }, want: true, }, { name: "nested parent", args: args{ - path: "/foo/bar/baz", - prefix: "/foo/bar", - prefixMatchOnly: false, + path: "/foo/bar/baz", + prefix: "/foo/bar", }, want: true, }, { name: "sibling", args: args{ - path: "/foo/bar", - prefix: "/bar", - prefixMatchOnly: false, + path: "/foo/bar", + prefix: "/bar", }, want: false, }, { name: "nested sibling", args: args{ - path: "/foo/bar/baz", - prefix: "/foo/bar", - prefixMatchOnly: false, + path: "/foo/bar/baz", + prefix: "/foo/bar", }, want: true, }, { name: "name prefix", args: args{ - path: "/foo2/bar", - prefix: "/foo", - prefixMatchOnly: false, - }, - want: false, - }, - { - name: "prefix match only (volume)", - args: args{ - path: "/foo", - prefix: "/foo", - prefixMatchOnly: true, + path: "/foo2/bar", + prefix: "/foo", }, want: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := HasFilepathPrefix(tt.args.path, tt.args.prefix, tt.args.prefixMatchOnly); got != tt.want { + if got := HasFilepathPrefix(tt.args.path, tt.args.prefix); got != tt.want { t.Errorf("HasFilepathPrefix() = %v, want %v", got, tt.want) } })