From 2aa481c15e90b56e6927b0ac6cc9f3ac9b2f0e03 Mon Sep 17 00:00:00 2001 From: Cole Wippern Date: Wed, 27 Nov 2019 09:35:53 -0800 Subject: [PATCH] add unit tests for caching run and copy --- pkg/commands/copy.go | 18 +++- pkg/commands/copy_test.go | 141 +++++++++++++++++++++++++ pkg/commands/fake_commands.go | 88 ++++++++++++++++ pkg/commands/run.go | 16 ++- pkg/commands/run_test.go | 193 ++++++++++++++++++++++++++++++++++ pkg/executor/build.go | 16 ++- pkg/util/fs_util.go | 16 ++- pkg/util/fs_util_test.go | 2 +- 8 files changed, 477 insertions(+), 13 deletions(-) create mode 100644 pkg/commands/fake_commands.go diff --git a/pkg/commands/copy.go b/pkg/commands/copy.go index dab5917fa3..6bbff699a9 100644 --- a/pkg/commands/copy.go +++ b/pkg/commands/copy.go @@ -17,6 +17,7 @@ limitations under the License. package commands import ( + "fmt" "os" "path/filepath" @@ -156,8 +157,9 @@ func (c *CopyCommand) ShouldCacheOutput() bool { func (c *CopyCommand) CacheCommand(img v1.Image) DockerCommand { return &CachingCopyCommand{ - img: img, - cmd: c.cmd, + img: img, + cmd: c.cmd, + extractFn: util.ExtractFile, } } @@ -170,16 +172,23 @@ type CachingCopyCommand struct { img v1.Image extractedFiles []string cmd *instructions.CopyCommand + extractFn util.ExtractFunction } func (cr *CachingCopyCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error { logrus.Infof("Found cached layer, extracting to filesystem") var err error - cr.extractedFiles, err = util.GetFSFromImage(RootDir, cr.img) + + if cr.img == nil { + return errors.New(fmt.Sprintf("cached command image is nil %v", cr.String())) + } + cr.extractedFiles, err = util.GetFSFromImage(RootDir, cr.img, cr.extractFn) + logrus.Infof("extractedFiles: %s", cr.extractedFiles) if err != nil { return errors.Wrap(err, "extracting fs from image") } + return nil } @@ -188,6 +197,9 @@ func (cr *CachingCopyCommand) FilesToSnapshot() []string { } func (cr *CachingCopyCommand) String() string { + if cr.cmd == nil { + return "nil command" + } return cr.cmd.String() } diff --git a/pkg/commands/copy_test.go b/pkg/commands/copy_test.go index 5f8fb7211d..d031680599 100644 --- a/pkg/commands/copy_test.go +++ b/pkg/commands/copy_test.go @@ -16,6 +16,7 @@ limitations under the License. package commands import ( + "archive/tar" "fmt" "io" "io/ioutil" @@ -215,3 +216,143 @@ func Test_resolveIfSymlink(t *testing.T) { }) } } + +func Test_CachingCopyCommand_ExecuteCommand(t *testing.T) { + tarContent, err := prepareTarFixture([]string{"foo.txt"}) + if err != nil { + t.Errorf("couldn't prepare tar fixture %v", err) + } + + config := &v1.Config{} + buildArgs := &dockerfile.BuildArgs{} + + type testCase struct { + desctiption string + expectLayer bool + expectErr bool + count *int + expectedCount int + command *CachingCopyCommand + extractedFiles []string + contextFiles []string + } + testCases := []testCase{ + func() testCase { + c := &CachingCopyCommand{ + img: fakeImage{ + ImageLayers: []v1.Layer{ + fakeLayer{TarContent: tarContent}, + }, + }, + cmd: &instructions.CopyCommand{ + SourcesAndDest: []string{ + "foo.txt", "foo.txt", + }, + }, + } + count := 0 + tc := testCase{ + desctiption: "with valid image and valid layer", + count: &count, + expectedCount: 1, + expectLayer: true, + extractedFiles: []string{"/foo.txt"}, + contextFiles: []string{"foo.txt"}, + } + c.extractFn = func(_ string, _ *tar.Header, _ io.Reader) error { + *tc.count++ + return nil + } + tc.command = c + return tc + }(), + func() testCase { + c := &CachingCopyCommand{} + tc := testCase{ + desctiption: "with no image", + expectErr: true, + } + c.extractFn = func(_ string, _ *tar.Header, _ io.Reader) error { + return nil + } + tc.command = c + return tc + }(), + func() testCase { + c := &CachingCopyCommand{ + img: fakeImage{}, + } + tc := testCase{ + desctiption: "with image containing no layers", + } + c.extractFn = func(_ string, _ *tar.Header, _ io.Reader) error { + return nil + } + tc.command = c + return tc + }(), + func() testCase { + c := &CachingCopyCommand{ + img: fakeImage{ + ImageLayers: []v1.Layer{ + fakeLayer{}, + }, + }, + } + c.extractFn = func(_ string, _ *tar.Header, _ io.Reader) error { + return nil + } + tc := testCase{ + desctiption: "with image one layer which has no tar content", + expectErr: false, // this one probably should fail but doesn't because of how ExecuteCommand and util.GetFSFromLayers are implemented - cvgw- 2019-11-25 + expectLayer: true, + } + tc.command = c + return tc + }(), + } + + for _, tc := range testCases { + t.Run(tc.desctiption, func(t *testing.T) { + c := tc.command + err := c.ExecuteCommand(config, buildArgs) + if !tc.expectErr && err != nil { + t.Errorf("Expected err to be nil but was %v", err) + } else if tc.expectErr && err == nil { + t.Error("Expected err but was nil") + } + + if tc.count != nil { + if *tc.count != tc.expectedCount { + t.Errorf("Expected extractFn to be called %v times but was called %v times", tc.expectedCount, *tc.count) + } + for _, file := range tc.extractedFiles { + match := false + cFiles := c.FilesToSnapshot() + for _, cFile := range cFiles { + if file == cFile { + match = true + break + } + } + if !match { + t.Errorf("Expected extracted files to include %v but did not %v", file, cFiles) + } + } + // CachingCopyCommand does not override BaseCommand + // FilesUseFromContext so this will always return an empty slice and no error + // This seems like it might be a bug as it results in CopyCommands and CachingCopyCommands generating different cache keys - cvgw - 2019-11-27 + cmdFiles, err := c.FilesUsedFromContext( + config, buildArgs, + ) + if err != nil { + t.Errorf("failed to get files used from context from command") + } + if len(cmdFiles) != 0 { + t.Errorf("expected files used from context to be empty but was not") + } + } + + }) + } +} diff --git a/pkg/commands/fake_commands.go b/pkg/commands/fake_commands.go new file mode 100644 index 0000000000..8efee7c4de --- /dev/null +++ b/pkg/commands/fake_commands.go @@ -0,0 +1,88 @@ +/* +Copyright 2018 Google LLC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// used for testing in the commands package +package commands + +import ( + "bytes" + "io" + "io/ioutil" + + v1 "github.com/google/go-containerregistry/pkg/v1" + "github.com/google/go-containerregistry/pkg/v1/types" +) + +type fakeLayer struct { + TarContent []byte +} + +func (f fakeLayer) Digest() (v1.Hash, error) { + return v1.Hash{}, nil +} +func (f fakeLayer) DiffID() (v1.Hash, error) { + return v1.Hash{}, nil +} +func (f fakeLayer) Compressed() (io.ReadCloser, error) { + return nil, nil +} +func (f fakeLayer) Uncompressed() (io.ReadCloser, error) { + return ioutil.NopCloser(bytes.NewReader(f.TarContent)), nil +} +func (f fakeLayer) Size() (int64, error) { + return 0, nil +} +func (f fakeLayer) MediaType() (types.MediaType, error) { + return "", nil +} + +type fakeImage struct { + ImageLayers []v1.Layer +} + +func (f fakeImage) Layers() ([]v1.Layer, error) { + return f.ImageLayers, nil +} +func (f fakeImage) MediaType() (types.MediaType, error) { + return "", nil +} +func (f fakeImage) Size() (int64, error) { + return 0, nil +} +func (f fakeImage) ConfigName() (v1.Hash, error) { + return v1.Hash{}, nil +} +func (f fakeImage) ConfigFile() (*v1.ConfigFile, error) { + return &v1.ConfigFile{}, nil +} +func (f fakeImage) RawConfigFile() ([]byte, error) { + return []byte{}, nil +} +func (f fakeImage) Digest() (v1.Hash, error) { + return v1.Hash{}, nil +} +func (f fakeImage) Manifest() (*v1.Manifest, error) { + return &v1.Manifest{}, nil +} +func (f fakeImage) RawManifest() ([]byte, error) { + return []byte{}, nil +} +func (f fakeImage) LayerByDigest(v1.Hash) (v1.Layer, error) { + return fakeLayer{}, nil +} +func (f fakeImage) LayerByDiffID(v1.Hash) (v1.Layer, error) { + return fakeLayer{}, nil +} diff --git a/pkg/commands/run.go b/pkg/commands/run.go index 4867fd5468..0dcd4ed520 100644 --- a/pkg/commands/run.go +++ b/pkg/commands/run.go @@ -164,8 +164,9 @@ func (r *RunCommand) FilesToSnapshot() []string { func (r *RunCommand) CacheCommand(img v1.Image) DockerCommand { return &CachingRunCommand{ - img: img, - cmd: r.cmd, + img: img, + cmd: r.cmd, + extractFn: util.ExtractFile, } } @@ -186,15 +187,21 @@ type CachingRunCommand struct { img v1.Image extractedFiles []string cmd *instructions.RunCommand + extractFn util.ExtractFunction } func (cr *CachingRunCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error { logrus.Infof("Found cached layer, extracting to filesystem") var err error - cr.extractedFiles, err = util.GetFSFromImage(constants.RootDir, cr.img) + + if cr.img == nil { + return errors.New(fmt.Sprintf("command image is nil %v", cr.String())) + } + cr.extractedFiles, err = util.GetFSFromImage(constants.RootDir, cr.img, cr.extractFn) if err != nil { return errors.Wrap(err, "extracting fs from image") } + return nil } @@ -203,5 +210,8 @@ func (cr *CachingRunCommand) FilesToSnapshot() []string { } func (cr *CachingRunCommand) String() string { + if cr.cmd == nil { + return "nil command" + } return cr.cmd.String() } diff --git a/pkg/commands/run_test.go b/pkg/commands/run_test.go index 0e7c51e8c0..36ae98bad4 100644 --- a/pkg/commands/run_test.go +++ b/pkg/commands/run_test.go @@ -16,10 +16,19 @@ limitations under the License. package commands import ( + "archive/tar" + "bytes" + "io" + "io/ioutil" + "log" + "os" "os/user" + "path/filepath" "testing" + "github.com/GoogleContainerTools/kaniko/pkg/dockerfile" "github.com/GoogleContainerTools/kaniko/testutil" + v1 "github.com/google/go-containerregistry/pkg/v1" ) func Test_addDefaultHOME(t *testing.T) { @@ -120,3 +129,187 @@ func Test_addDefaultHOME(t *testing.T) { }) } } + +func prepareTarFixture(fileNames []string) ([]byte, error) { + dir, err := ioutil.TempDir("/tmp", "tar-fixture") + if err != nil { + return nil, err + } + + content := ` +Meow meow meow meow +meow meow meow meow +` + for _, name := range fileNames { + if err := ioutil.WriteFile(filepath.Join(dir, name), []byte(content), 0777); err != nil { + return nil, err + } + } + writer := bytes.NewBuffer([]byte{}) + tw := tar.NewWriter(writer) + defer tw.Close() + filepath.Walk(dir, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + + if info.IsDir() { + return nil + } + + hdr, err := tar.FileInfoHeader(info, "") + if err != nil { + return err + } + if err := tw.WriteHeader(hdr); err != nil { + log.Fatal(err) + } + body, err := ioutil.ReadFile(path) + if err != nil { + return err + } + if _, err := tw.Write(body); err != nil { + log.Fatal(err) + } + + return nil + }) + + return writer.Bytes(), nil +} + +func Test_CachingRunCommand_ExecuteCommand(t *testing.T) { + tarContent, err := prepareTarFixture([]string{"foo.txt"}) + if err != nil { + t.Errorf("couldn't prepare tar fixture %v", err) + } + + config := &v1.Config{} + buildArgs := &dockerfile.BuildArgs{} + + type testCase struct { + desctiption string + expectLayer bool + expectErr bool + count *int + expectedCount int + command *CachingRunCommand + extractedFiles []string + contextFiles []string + } + testCases := []testCase{ + func() testCase { + c := &CachingRunCommand{ + img: fakeImage{ + ImageLayers: []v1.Layer{ + fakeLayer{TarContent: tarContent}, + }, + }, + } + count := 0 + tc := testCase{ + desctiption: "with valid image and valid layer", + count: &count, + expectedCount: 1, + expectLayer: true, + extractedFiles: []string{"/foo.txt"}, + contextFiles: []string{"foo.txt"}, + } + c.extractFn = func(_ string, _ *tar.Header, _ io.Reader) error { + *tc.count++ + return nil + } + tc.command = c + return tc + }(), + func() testCase { + c := &CachingRunCommand{} + tc := testCase{ + desctiption: "with no image", + expectErr: true, + } + c.extractFn = func(_ string, _ *tar.Header, _ io.Reader) error { + return nil + } + tc.command = c + return tc + }(), + func() testCase { + c := &CachingRunCommand{ + img: fakeImage{}, + } + tc := testCase{ + desctiption: "with image containing no layers", + } + c.extractFn = func(_ string, _ *tar.Header, _ io.Reader) error { + return nil + } + tc.command = c + return tc + }(), + func() testCase { + c := &CachingRunCommand{ + img: fakeImage{ + ImageLayers: []v1.Layer{ + fakeLayer{}, + }, + }, + } + c.extractFn = func(_ string, _ *tar.Header, _ io.Reader) error { + return nil + } + tc := testCase{ + desctiption: "with image one layer which has no tar content", + expectErr: false, // this one probably should fail but doesn't because of how ExecuteCommand and util.GetFSFromLayers are implemented - cvgw- 2019-11-25 + expectLayer: true, + } + tc.command = c + return tc + }(), + } + + for _, tc := range testCases { + t.Run(tc.desctiption, func(t *testing.T) { + c := tc.command + err := c.ExecuteCommand(config, buildArgs) + if !tc.expectErr && err != nil { + t.Errorf("Expected err to be nil but was %v", err) + } else if tc.expectErr && err == nil { + t.Error("Expected err but was nil") + } + + if tc.count != nil { + if *tc.count != tc.expectedCount { + t.Errorf("Expected extractFn to be called %v times but was called %v times", 1, *tc.count) + } + for _, file := range tc.extractedFiles { + match := false + cmdFiles := c.extractedFiles + for _, f := range cmdFiles { + if file == f { + match = true + break + } + } + if !match { + t.Errorf("Expected extracted files to include %v but did not %v", file, cmdFiles) + } + } + + // CachingRunCommand does not override BaseCommand + // FilesUseFromContext so this will always return an empty slice and no error + // This seems like it might be a bug as it results in RunCommands and CachingRunCommands generating different cache keys - cvgw - 2019-11-27 + cmdFiles, err := c.FilesUsedFromContext( + config, buildArgs, + ) + if err != nil { + t.Errorf("failed to get files used from context from command") + } + + if len(cmdFiles) != 0 { + t.Errorf("expected files used from context to be empty but was not") + } + } + }) + } +} diff --git a/pkg/executor/build.go b/pkg/executor/build.go index caa1cf0638..563a8f379f 100644 --- a/pkg/executor/build.go +++ b/pkg/executor/build.go @@ -211,10 +211,13 @@ func (s *stageBuilder) optimize(compositeKey CompositeCache, cfg v1.Config) erro if err != nil { return errors.Wrap(err, "failed to hash composite key") } + logrus.Debugf("optimize: cache key for command %v %v", command.String(), ck) s.finalCacheKey = ck + if command.ShouldCacheOutput() && !stopCache { img, err := s.layerCache.RetrieveLayer(ck) + if err != nil { logrus.Debugf("Failed to retrieve layer: %s", err) logrus.Infof("No cached layer found for cmd %s", command.String()) @@ -247,6 +250,7 @@ func (s *stageBuilder) build() error { } else { compositeKey = NewCompositeCache(s.baseImageDigest) } + compositeKey.AddKey(s.opts.BuildArgs...) // Apply optimizations to the instructions. @@ -269,21 +273,26 @@ func (s *stageBuilder) build() error { if shouldUnpack { t := timing.Start("FS Unpacking") - if _, err := util.GetFSFromImage(constants.RootDir, s.image); err != nil { + + if _, err := util.GetFSFromImage(constants.RootDir, s.image, util.ExtractFile); err != nil { return errors.Wrap(err, "failed to get filesystem from image") } + timing.DefaultRun.Stop(t) } else { logrus.Info("Skipping unpacking as no commands require it.") } + if err := util.DetectFilesystemWhitelist(constants.WhitelistPath); err != nil { return errors.Wrap(err, "failed to check filesystem whitelist") } + // Take initial snapshot t := timing.Start("Initial FS snapshot") if err := s.snapshotter.Init(); err != nil { return err } + timing.DefaultRun.Stop(t) cacheGroup := errgroup.Group{} @@ -327,6 +336,9 @@ func (s *stageBuilder) build() error { if err != nil { return errors.Wrap(err, "failed to hash composite key") } + + logrus.Debugf("build: cache key for command %v %v", command.String(), ck) + // Push layer to cache (in parallel) now along with new config file if s.opts.Cache && command.ShouldCacheOutput() { cacheGroup.Go(func() error { @@ -641,7 +653,7 @@ func extractImageToDependencyDir(name string, image v1.Image) error { return err } logrus.Debugf("trying to extract to %s", dependencyDir) - _, err := util.GetFSFromImage(dependencyDir, image) + _, err := util.GetFSFromImage(dependencyDir, image, util.ExtractFile) return err } diff --git a/pkg/util/fs_util.go b/pkg/util/fs_util.go index 77376228e4..f7022e8ece 100644 --- a/pkg/util/fs_util.go +++ b/pkg/util/fs_util.go @@ -69,9 +69,17 @@ var volumes = []string{} var excluded []string +type ExtractFunction func(string, *tar.Header, io.Reader) error + // GetFSFromImage extracts the layers of img to root // It returns a list of all files extracted -func GetFSFromImage(root string, img v1.Image) ([]string, error) { +func GetFSFromImage(root string, img v1.Image, extract ExtractFunction) ([]string, error) { + if extract == nil { + return nil, errors.New("must supply an extract function") + } + if img == nil { + return nil, errors.New("image cannot be nil") + } if err := DetectFilesystemWhitelist(constants.WhitelistPath); err != nil { return nil, err } @@ -114,7 +122,7 @@ func GetFSFromImage(root string, img v1.Image) ([]string, error) { } continue } - if err := extractFile(root, hdr, tr); err != nil { + if err := extract(root, hdr, tr); err != nil { return nil, err } extractedFiles = append(extractedFiles, filepath.Join(root, filepath.Clean(hdr.Name))) @@ -179,7 +187,7 @@ func unTar(r io.Reader, dest string) ([]string, error) { if err != nil { return nil, err } - if err := extractFile(dest, hdr, tr); err != nil { + if err := ExtractFile(dest, hdr, tr); err != nil { return nil, err } extractedFiles = append(extractedFiles, dest) @@ -187,7 +195,7 @@ func unTar(r io.Reader, dest string) ([]string, error) { return extractedFiles, nil } -func extractFile(dest string, hdr *tar.Header, tr io.Reader) error { +func ExtractFile(dest string, hdr *tar.Header, tr io.Reader) error { path := filepath.Join(dest, filepath.Clean(hdr.Name)) base := filepath.Base(path) dir := filepath.Dir(path) diff --git a/pkg/util/fs_util_test.go b/pkg/util/fs_util_test.go index 2a4b31c6fd..1ea074f582 100644 --- a/pkg/util/fs_util_test.go +++ b/pkg/util/fs_util_test.go @@ -659,7 +659,7 @@ func TestExtractFile(t *testing.T) { defer os.RemoveAll(r) for _, hdr := range tc.hdrs { - if err := extractFile(r, hdr, bytes.NewReader(tc.contents)); err != nil { + if err := ExtractFile(r, hdr, bytes.NewReader(tc.contents)); err != nil { t.Fatal(err) } }