From a211c1ec71a1b48598b4e2439f71cee34b47f219 Mon Sep 17 00:00:00 2001 From: Priya Wadhwa Date: Tue, 24 Apr 2018 14:56:30 -0700 Subject: [PATCH] Make sure to snapshot parent directories of specific files for add/copy --- pkg/commands/copy.go | 11 ++++++- pkg/snapshot/snapshot.go | 62 +++++++++++++++++++++-------------- pkg/snapshot/snapshot_test.go | 21 ++++-------- pkg/util/fs_util.go | 17 ++++++++++ pkg/util/fs_util_test.go | 32 ++++++++++++++++++ 5 files changed, 103 insertions(+), 40 deletions(-) diff --git a/pkg/commands/copy.go b/pkg/commands/copy.go index bded4bf99..1ad9d77ec 100644 --- a/pkg/commands/copy.go +++ b/pkg/commands/copy.go @@ -17,6 +17,7 @@ limitations under the License. package commands import ( + "github.com/GoogleContainerTools/kaniko/pkg/constants" "github.com/GoogleContainerTools/kaniko/pkg/util" "github.com/containers/image/manifest" "github.com/docker/docker/builder/dockerfile/instructions" @@ -57,11 +58,19 @@ func (c *CopyCommand) ExecuteCommand(config *manifest.Schema2Config) error { if err != nil { return err } - destPath, err := util.DestinationFilepath(src, dest, config.WorkingDir) + cwd := config.WorkingDir + if cwd == "" { + cwd = constants.RootDir + } + destPath, err := util.DestinationFilepath(src, dest, cwd) if err != nil { return err } if fi.IsDir() { + if !filepath.IsAbs(dest) { + // we need to add '/' to the end to indicate the destination is a directory + dest = filepath.Join(cwd, dest) + "/" + } if err := util.CopyDir(fullPath, dest); err != nil { return err } diff --git a/pkg/snapshot/snapshot.go b/pkg/snapshot/snapshot.go index 70cfcd117..5b0a4cc1c 100644 --- a/pkg/snapshot/snapshot.go +++ b/pkg/snapshot/snapshot.go @@ -19,10 +19,8 @@ package snapshot import ( "archive/tar" "bytes" - "github.com/GoogleContainerTools/kaniko/pkg/util" "github.com/sirupsen/logrus" - "io" "io/ioutil" "os" @@ -52,12 +50,14 @@ func (s *Snapshotter) Init() error { // TakeSnapshot takes a snapshot of the filesystem, avoiding directories in the whitelist, and creates // a tarball of the changed files. Return contents of the tarball, and whether or not any files were changed func (s *Snapshotter) TakeSnapshot(files []string) ([]byte, error) { - if files != nil { - return s.TakeSnapshotOfFiles(files) - } - logrus.Info("Taking snapshot of full filesystem...") buf := bytes.NewBuffer([]byte{}) - filesAdded, err := s.snapShotFS(buf) + var filesAdded bool + var err error + if files == nil { + filesAdded, err = s.snapShotFS(buf) + } else { + filesAdded, err = s.snapshotFiles(buf, files) + } if err != nil { return nil, err } @@ -68,45 +68,57 @@ func (s *Snapshotter) TakeSnapshot(files []string) ([]byte, error) { return contents, err } -// TakeSnapshotOfFiles takes a snapshot of specific files +// snapshotFiles takes a snapshot of specific files // Used for ADD/COPY commands, when we know which files have changed -func (s *Snapshotter) TakeSnapshotOfFiles(files []string) ([]byte, error) { - logrus.Infof("Taking snapshot of files %v...", files) +func (s *Snapshotter) snapshotFiles(f io.Writer, files []string) (bool, error) { + s.hardlinks = map[uint64]string{} s.l.Snapshot() if len(files) == 0 { logrus.Info("No files changed in this command, skipping snapshotting.") - return nil, nil + return false, nil } - buf := bytes.NewBuffer([]byte{}) - w := tar.NewWriter(buf) - defer w.Close() - filesAdded := false + logrus.Infof("Taking snapshot of files %v...", files) + snapshottedFiles := make(map[string]bool) for _, file := range files { - info, err := os.Lstat(file) - if err != nil { - return nil, err + parentDirs := util.ParentDirectories(file) + files = append(parentDirs, files...) + } + filesAdded := false + w := tar.NewWriter(f) + defer w.Close() + + // Now create the tar. + for _, file := range files { + file = filepath.Clean(file) + if val, ok := snapshottedFiles[file]; ok && val { + continue } if util.PathInWhitelist(file, s.directory) { - logrus.Debugf("Not adding %s to layer, as it is whitelisted", file) + logrus.Debugf("Not adding %s to layer, as it's whitelisted", file) continue } + snapshottedFiles[file] = true + info, err := os.Lstat(file) + if err != nil { + return false, err + } // Only add to the tar if we add it to the layeredmap. maybeAdd, err := s.l.MaybeAdd(file) if err != nil { - return nil, err + return false, err } if maybeAdd { filesAdded = true - util.AddToTar(file, info, s.hardlinks, w) + if err := util.AddToTar(file, info, s.hardlinks, w); err != nil { + return false, err + } } } - if !filesAdded { - return nil, nil - } - return ioutil.ReadAll(buf) + return filesAdded, nil } func (s *Snapshotter) snapShotFS(f io.Writer) (bool, error) { + logrus.Info("Taking snapshot of full filesystem...") s.hardlinks = map[uint64]string{} s.l.Snapshot() existingPaths := s.l.GetFlattenedPathsForWhiteOut() diff --git a/pkg/snapshot/snapshot_test.go b/pkg/snapshot/snapshot_test.go index 2def85db0..7cee2ad11 100644 --- a/pkg/snapshot/snapshot_test.go +++ b/pkg/snapshot/snapshot_test.go @@ -149,30 +149,23 @@ func TestSnapshotFiles(t *testing.T) { if err != nil { t.Fatal(err) } - expectedContents := map[string]string{ - filepath.Join(testDir, "foo"): "newbaz1", - } + expectedFiles := []string{"/tmp", filepath.Join(testDir, "foo")} + // Check contents of the snapshot, make sure contents is equivalent to snapshotFiles reader := bytes.NewReader(contents) tr := tar.NewReader(reader) - numFiles := 0 + var actualFiles []string for { hdr, err := tr.Next() if err == io.EOF { break } - numFiles = numFiles + 1 - if _, isFile := expectedContents[hdr.Name]; !isFile { - t.Fatalf("File %s unexpectedly in tar", hdr.Name) - } - contents, _ := ioutil.ReadAll(tr) - if string(contents) != expectedContents[hdr.Name] { - t.Fatalf("Contents of %s incorrect, expected: %s, actual: %s", hdr.Name, expectedContents[hdr.Name], string(contents)) + if err != nil { + t.Fatal(err) } + actualFiles = append(actualFiles, hdr.Name) } - if numFiles != 1 { - t.Fatalf("%s was not added.", filepath.Join(testDir, "foo")) - } + testutil.CheckErrorAndDeepEqual(t, false, nil, expectedFiles, actualFiles) } func TestEmptySnapshot(t *testing.T) { diff --git a/pkg/util/fs_util.go b/pkg/util/fs_util.go index f5c96b80b..e2b393d5b 100644 --- a/pkg/util/fs_util.go +++ b/pkg/util/fs_util.go @@ -141,6 +141,23 @@ func Files(root string) ([]string, error) { return files, err } +// ParentDirectories returns a list of paths to all parent directories +// Ex. /some/temp/dir -> [/some, /some/temp, /some/temp/dir] +func ParentDirectories(path string) []string { + path = filepath.Clean(path) + dirs := strings.Split(path, "/") + dirPath := constants.RootDir + var paths []string + for _, dir := range dirs { + if dir == "" { + continue + } + dirPath = filepath.Join(dirPath, dir) + paths = append(paths, dirPath) + } + return paths +} + // FilepathExists returns true if the path exists func FilepathExists(path string) bool { _, err := os.Lstat(path) diff --git a/pkg/util/fs_util_test.go b/pkg/util/fs_util_test.go index 36583564c..7b41a4464 100644 --- a/pkg/util/fs_util_test.go +++ b/pkg/util/fs_util_test.go @@ -131,3 +131,35 @@ func Test_RelativeFiles(t *testing.T) { testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedFiles, actualFiles) } } + +func Test_ParentDirectories(t *testing.T) { + tests := []struct { + name string + path string + expected []string + }{ + { + name: "regular path", + path: "/path/to/dir", + expected: []string{ + "/path", + "/path/to", + "/path/to/dir", + }, + }, + { + name: "current directory", + path: ".", + expected: []string{ + "/", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + actual := ParentDirectories(tt.path) + testutil.CheckErrorAndDeepEqual(t, false, nil, tt.expected, actual) + }) + } +}