From 697037cbcffb178cb1c3327b9be8425243cdda6d Mon Sep 17 00:00:00 2001 From: Cole Wippern Date: Wed, 27 Nov 2019 10:17:03 -0800 Subject: [PATCH 1/5] Add unit tests for compositecache and stagebuilder * add mock types for testing * enhance error messaging * add tests --- pkg/commands/commands.go | 7 + pkg/commands/copy.go | 2 +- pkg/commands/fake_commands.go | 88 ++++++++ pkg/executor/build.go | 52 +++-- pkg/executor/build_test.go | 316 +++++++++++++++++++++++++++ pkg/executor/composite_cache_test.go | 121 ++++++++++ pkg/executor/fakes.go | 180 +++++++++++++++ pkg/snapshot/snapshot.go | 2 +- pkg/util/command_util.go | 17 +- 9 files changed, 756 insertions(+), 29 deletions(-) create mode 100644 pkg/commands/fake_commands.go create mode 100644 pkg/executor/composite_cache_test.go create mode 100644 pkg/executor/fakes.go diff --git a/pkg/commands/commands.go b/pkg/commands/commands.go index a2ea7788d..cc58d51c7 100644 --- a/pkg/commands/commands.go +++ b/pkg/commands/commands.go @@ -17,6 +17,7 @@ limitations under the License. package commands import ( + "github.com/GoogleContainerTools/kaniko/pkg/constants" "github.com/GoogleContainerTools/kaniko/pkg/dockerfile" v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/moby/buildkit/frontend/dockerfile/instructions" @@ -24,6 +25,12 @@ import ( "github.com/sirupsen/logrus" ) +var RootDir string + +func init() { + RootDir = constants.RootDir +} + type CurrentCacheKey func() (string, error) type DockerCommand interface { diff --git a/pkg/commands/copy.go b/pkg/commands/copy.go index ad75f6f3f..4071f957b 100644 --- a/pkg/commands/copy.go +++ b/pkg/commands/copy.go @@ -171,7 +171,7 @@ type CachingCopyCommand struct { 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(constants.RootDir, cr.img) + cr.extractedFiles, err = util.GetFSFromImage(RootDir, cr.img) logrus.Infof("extractedFiles: %s", cr.extractedFiles) if err != nil { return errors.Wrap(err, "extracting fs from image") diff --git a/pkg/commands/fake_commands.go b/pkg/commands/fake_commands.go new file mode 100644 index 000000000..8efee7c4d --- /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/executor/build.go b/pkg/executor/build.go index c1d987120..e3c48b5e1 100644 --- a/pkg/executor/build.go +++ b/pkg/executor/build.go @@ -23,7 +23,7 @@ import ( "strconv" "time" - "github.com/otiai10/copy" + otiai10Cpy "github.com/otiai10/copy" "github.com/google/go-containerregistry/pkg/v1/partial" @@ -52,12 +52,21 @@ import ( // This is the size of an empty tar in Go const emptyTarSize = 1024 +type cachePusher func(*config.KanikoOptions, string, string, string) error +type snapShotter interface { + Init() error + TakeSnapshotFS() (string, error) + TakeSnapshot([]string) (string, error) +} + // stageBuilder contains all fields necessary to build one stage of a Dockerfile type stageBuilder struct { stage config.KanikoStage image v1.Image cf *v1.ConfigFile - snapshotter *snapshot.Snapshotter + snapshotter snapShotter + layerCache cache.LayerCache + pushCache cachePusher baseImageDigest string finalCacheKey string opts *config.KanikoOptions @@ -103,6 +112,10 @@ func newStageBuilder(opts *config.KanikoOptions, stage config.KanikoStage, cross opts: opts, crossStageDeps: crossStageDeps, digestToCacheKeyMap: dcm, + layerCache: &cache.RegistryCache{ + Opts: opts, + }, + pushCache: pushLayerToCache, } for _, cmd := range s.stage.Commands { @@ -138,9 +151,6 @@ func (s *stageBuilder) optimize(compositeKey CompositeCache, cfg v1.Config) erro return nil } - layerCache := &cache.RegistryCache{ - Opts: s.opts, - } stopCache := false // Possibly replace commands with their cached implementations. // We walk through all the commands, running any commands that only operate on metadata. @@ -154,21 +164,21 @@ func (s *stageBuilder) optimize(compositeKey CompositeCache, cfg v1.Config) erro // If the command uses files from the context, add them. files, err := command.FilesUsedFromContext(&cfg, s.args) if err != nil { - return err + return errors.Wrap(err, "failed to get files used from context") } for _, f := range files { if err := compositeKey.AddPath(f); err != nil { - return err + return errors.Wrap(err, "failed to add path to composite key") } } ck, err := compositeKey.Hash() if err != nil { - return err + return errors.Wrap(err, "failed to hash composite key") } s.finalCacheKey = ck if command.ShouldCacheOutput() && !stopCache { - img, err := layerCache.RetrieveLayer(ck) + 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()) @@ -205,7 +215,7 @@ func (s *stageBuilder) build() error { // Apply optimizations to the instructions. if err := s.optimize(*compositeKey, s.cf.Config); err != nil { - return err + return errors.Wrap(err, "failed to optimize instructions") } // Unpack file system to root if we need to. @@ -224,14 +234,14 @@ func (s *stageBuilder) build() error { if shouldUnpack { t := timing.Start("FS Unpacking") if _, err := util.GetFSFromImage(constants.RootDir, s.image); err != nil { - return err + 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 err + return errors.Wrap(err, "failed to check filesystem whitelist") } // Take initial snapshot t := timing.Start("Initial FS snapshot") @@ -252,17 +262,17 @@ func (s *stageBuilder) build() error { // If the command uses files from the context, add them. files, err := command.FilesUsedFromContext(&s.cf.Config, s.args) if err != nil { - return err + return errors.Wrap(err, "failed to get files used from context") } for _, f := range files { if err := compositeKey.AddPath(f); err != nil { - return err + return errors.Wrap(err, fmt.Sprintf("failed to add path to composite key %v", f)) } } logrus.Info(command.String()) if err := command.ExecuteCommand(&s.cf.Config, s.args); err != nil { - return err + return errors.Wrap(err, "failed to execute command") } files = command.FilesToSnapshot() timing.DefaultRun.Stop(t) @@ -273,21 +283,21 @@ func (s *stageBuilder) build() error { tarPath, err := s.takeSnapshot(files) if err != nil { - return err + return errors.Wrap(err, "failed to take snapshot") } ck, err := compositeKey.Hash() if err != nil { - return err + return errors.Wrap(err, "failed to hash composite key") } // Push layer to cache (in parallel) now along with new config file if s.opts.Cache && command.ShouldCacheOutput() { cacheGroup.Go(func() error { - return pushLayerToCache(s.opts, ck, tarPath, command.String()) + return s.pushCache(s.opts, ck, tarPath, command.String()) }) } if err := s.saveSnapshotToImage(command.String(), tarPath); err != nil { - return err + return errors.Wrap(err, "failed to save snapshot to image") } } if err := cacheGroup.Wait(); err != nil { @@ -343,7 +353,7 @@ func (s *stageBuilder) saveSnapshotToImage(createdBy string, tarPath string) err } fi, err := os.Stat(tarPath) if err != nil { - return err + return errors.Wrap(err, "tar file path does not exist") } if fi.Size() <= emptyTarSize { logrus.Info("No files were changed, appending empty layer to config. No layer added to image.") @@ -505,7 +515,7 @@ func DoBuild(opts *config.KanikoOptions) (v1.Image, error) { } for _, p := range filesToSave { logrus.Infof("Saving file %s for later use.", p) - copy.Copy(p, filepath.Join(dstDir, p)) + otiai10Cpy.Copy(p, filepath.Join(dstDir, p)) } // Delete the filesystem diff --git a/pkg/executor/build_test.go b/pkg/executor/build_test.go index 44f6a1211..47a7fdacd 100644 --- a/pkg/executor/build_test.go +++ b/pkg/executor/build_test.go @@ -17,6 +17,8 @@ limitations under the License. package executor import ( + "archive/tar" + "bytes" "io/ioutil" "os" "path/filepath" @@ -24,6 +26,7 @@ import ( "sort" "testing" + "github.com/GoogleContainerTools/kaniko/pkg/commands" "github.com/GoogleContainerTools/kaniko/pkg/config" "github.com/GoogleContainerTools/kaniko/pkg/dockerfile" "github.com/GoogleContainerTools/kaniko/testutil" @@ -462,3 +465,316 @@ func TestInitializeConfig(t *testing.T) { testutil.CheckDeepEqual(t, tt.expected, actual.Config) } } + +func Test_stageBuilder_optimize(t *testing.T) { + testCases := []struct { + opts *config.KanikoOptions + retrieve bool + name string + }{ + { + name: "cache enabled and layer not present in cache", + opts: &config.KanikoOptions{Cache: true}, + }, + { + name: "cache enabled and layer present in cache", + opts: &config.KanikoOptions{Cache: true}, + retrieve: true, + }, + { + name: "cache disabled and layer not present in cache", + opts: &config.KanikoOptions{Cache: false}, + }, + { + name: "cache disabled and layer present in cache", + opts: &config.KanikoOptions{Cache: false}, + retrieve: true, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + cf := &v1.ConfigFile{} + snap := fakeSnapShotter{} + lc := &fakeLayerCache{retrieve: tc.retrieve} + sb := &stageBuilder{opts: tc.opts, cf: cf, snapshotter: snap, layerCache: lc} + ck := CompositeCache{} + file, err := ioutil.TempFile("", "foo") + if err != nil { + t.Error(err) + } + command := MockDockerCommand{ + contextFiles: []string{file.Name()}, + cacheCommand: MockCachedDockerCommand{}, + } + sb.cmds = []commands.DockerCommand{command} + err = sb.optimize(ck, cf.Config) + if err != nil { + t.Errorf("Expected error to be nil but was %v", err) + } + + }) + } +} + +func Test_stageBuilder_build(t *testing.T) { + type testcase struct { + description string + opts *config.KanikoOptions + layerCache *fakeLayerCache + expectedCacheKeys []string + pushedCacheKeys []string + commands []commands.DockerCommand + fileName string + rootDir string + image v1.Image + config *v1.ConfigFile + } + // The two copy command test cases use the same filesystem content and should generate the same cache keys. If they don't then something is wrong. They share this variable to help ensure that. + copyCommandCacheKey := "7263908b66952551d89fd895ffb067e2e30f474be9f38a8f1792af2b6df7c6e3" + tempDirAndFile := func() (string, string) { + dir, err := ioutil.TempDir("", "foo") + if err != nil { + t.Errorf("could not create temp dir %v", err) + } + filename := "bar.txt" + filepath := filepath.Join(dir, filename) + err = ioutil.WriteFile(filepath, []byte(`meow`), 0777) + if err != nil { + t.Errorf("could not create temp file %v", err) + } + buf := bytes.NewBuffer([]byte{}) + writer := tar.NewWriter(buf) + defer writer.Close() + + return dir, filename + } + testCases := []testcase{ + { + description: "fake command cache enabled but key not in cache", + opts: &config.KanikoOptions{Cache: true}, + expectedCacheKeys: []string{"2cd95a0195a42f2873273b7e8c970e3a87970bd0e5d330b3c7d068e7419e5017"}, + pushedCacheKeys: []string{"2cd95a0195a42f2873273b7e8c970e3a87970bd0e5d330b3c7d068e7419e5017"}, + }, + { + description: "fake command cache enabled and key in cache", + opts: &config.KanikoOptions{Cache: true}, + layerCache: &fakeLayerCache{ + retrieve: true, + }, + expectedCacheKeys: []string{"2cd95a0195a42f2873273b7e8c970e3a87970bd0e5d330b3c7d068e7419e5017"}, + pushedCacheKeys: []string{"2cd95a0195a42f2873273b7e8c970e3a87970bd0e5d330b3c7d068e7419e5017"}, + }, + { + description: "fake command cache disabled and key not in cache", + opts: &config.KanikoOptions{Cache: false}, + }, + { + description: "fake command cache disabled and key in cache", + opts: &config.KanikoOptions{Cache: false}, + layerCache: &fakeLayerCache{ + retrieve: true, + }, + }, + func() testcase { + dir, filename := tempDirAndFile() + filepath := filepath.Join(dir, filename) + + buf := bytes.NewBuffer([]byte{}) + writer := tar.NewWriter(buf) + defer writer.Close() + + info, err := os.Stat(filepath) + if err != nil { + t.Errorf("could not get file info for temp file %v", err) + } + hdr, err := tar.FileInfoHeader(info, filename) + if err != nil { + t.Errorf("could not get tar header for temp file %v", err) + } + + if err := writer.WriteHeader(hdr); err != nil { + t.Errorf("could not write tar header %v", err) + } + + content, err := ioutil.ReadFile(filepath) + if err != nil { + t.Errorf("could not read tempfile %v", err) + } + + if _, err := writer.Write(content); err != nil { + t.Errorf("could not write file contents to tar") + } + tarContent := buf.Bytes() + return testcase{ + description: "copy command cache enabled and key in cache", + opts: &config.KanikoOptions{Cache: true}, + layerCache: &fakeLayerCache{ + retrieve: true, + img: fakeImage{ + ImageLayers: []v1.Layer{ + fakeLayer{ + TarContent: tarContent, + }, + }, + }, + }, + rootDir: dir, + expectedCacheKeys: []string{copyCommandCacheKey}, + // CachingCopyCommand is not pushed to the cache + pushedCacheKeys: []string{}, + commands: func() []commands.DockerCommand { + cmd, err := commands.GetCommand( + &instructions.CopyCommand{ + SourcesAndDest: []string{ + filename, "foo.txt", + }, + }, + dir, + ) + if err != nil { + panic(err) + } + return []commands.DockerCommand{ + cmd, + } + }(), + fileName: filename, + } + }(), + func() testcase { + dir, filename := tempDirAndFile() + + tarContent := []byte{} + destDir, err := ioutil.TempDir("", "baz") + if err != nil { + t.Errorf("could not create temp dir %v", err) + } + return testcase{ + description: "copy command cache enabled and key is not in cache", + opts: &config.KanikoOptions{Cache: true}, + config: &v1.ConfigFile{Config: v1.Config{WorkingDir: destDir}}, + layerCache: &fakeLayerCache{ + retrieve: false, + }, + image: fakeImage{ + ImageLayers: []v1.Layer{ + fakeLayer{ + TarContent: tarContent, + }, + }, + }, + rootDir: dir, + expectedCacheKeys: []string{copyCommandCacheKey}, + pushedCacheKeys: []string{copyCommandCacheKey}, + commands: func() []commands.DockerCommand { + cmd, err := commands.GetCommand( + &instructions.CopyCommand{ + SourcesAndDest: []string{ + filename, "foo.txt", + }, + }, + dir, + ) + if err != nil { + panic(err) + } + return []commands.DockerCommand{ + cmd, + } + }(), + fileName: filename, + } + }(), + } + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + var fileName string + if tc.commands == nil { + file, err := ioutil.TempFile("", "foo") + if err != nil { + t.Error(err) + } + command := MockDockerCommand{ + contextFiles: []string{file.Name()}, + cacheCommand: MockCachedDockerCommand{ + contextFiles: []string{file.Name()}, + }, + } + tc.commands = []commands.DockerCommand{command} + fileName = file.Name() + } else { + fileName = tc.fileName + } + + cf := tc.config + if cf == nil { + cf = &v1.ConfigFile{ + Config: v1.Config{ + Env: make([]string, 0), + }, + } + } + + snap := fakeSnapShotter{file: fileName} + lc := tc.layerCache + if lc == nil { + lc = &fakeLayerCache{} + } + keys := []string{} + sb := &stageBuilder{ + args: &dockerfile.BuildArgs{}, //required or code will panic + image: tc.image, + opts: tc.opts, + cf: cf, + snapshotter: snap, + layerCache: lc, + pushCache: func(_ *config.KanikoOptions, cacheKey, _, _ string) error { + keys = append(keys, cacheKey) + return nil + }, + } + sb.cmds = tc.commands + tmp := commands.RootDir + if tc.rootDir != "" { + commands.RootDir = tc.rootDir + } + err := sb.build() + if err != nil { + t.Errorf("Expected error to be nil but was %v", err) + } + + if len(tc.expectedCacheKeys) != len(lc.receivedKeys) { + t.Errorf("expected to receive %v keys but was %v", len(tc.expectedCacheKeys), len(lc.receivedKeys)) + } + for _, key := range tc.expectedCacheKeys { + match := false + for _, receivedKey := range lc.receivedKeys { + if key == receivedKey { + match = true + break + } + } + if !match { + t.Errorf("expected received keys to include %v but did not %v", key, lc.receivedKeys) + } + } + if len(tc.pushedCacheKeys) != len(keys) { + t.Errorf("expected to push %v keys but was %v", len(tc.pushedCacheKeys), len(keys)) + } + for _, key := range tc.pushedCacheKeys { + match := false + for _, pushedKey := range keys { + if key == pushedKey { + match = true + break + } + } + if !match { + t.Errorf("expected pushed keys to include %v but did not %v", key, keys) + } + } + commands.RootDir = tmp + + }) + } +} diff --git a/pkg/executor/composite_cache_test.go b/pkg/executor/composite_cache_test.go new file mode 100644 index 000000000..edd75c917 --- /dev/null +++ b/pkg/executor/composite_cache_test.go @@ -0,0 +1,121 @@ +package executor + +import ( + "io/ioutil" + "os" + "path/filepath" + "reflect" + "testing" +) + +func Test_NewCompositeCache(t *testing.T) { + r := NewCompositeCache() + if reflect.TypeOf(r).String() != "*executor.CompositeCache" { + t.Errorf("expected return to be *executor.CompositeCache but was %v", reflect.TypeOf(r).String()) + } +} + +func Test_CompositeCache_AddKey(t *testing.T) { + keys := []string{ + "meow", + "purr", + } + r := NewCompositeCache() + r.AddKey(keys...) + if len(r.keys) != 2 { + t.Errorf("expected keys to have length 2 but was %v", len(r.keys)) + } +} + +func Test_CompositeCache_Key(t *testing.T) { + r := NewCompositeCache("meow", "purr") + k := r.Key() + if k != "meow-purr" { + t.Errorf("expected result to equal meow-purr but was %v", k) + } +} + +func Test_CompositeCache_Hash(t *testing.T) { + r := NewCompositeCache("meow", "purr") + h, err := r.Hash() + if err != nil { + t.Errorf("expected error to be nil but was %v", err) + } + + expectedHash := "b4fd5a11af812a11a79d794007c842794cc668c8e7ebaba6d1e6d021b8e06c71" + if h != expectedHash { + t.Errorf("expected result to equal %v but was %v", expectedHash, h) + } +} + +func Test_CompositeCache_AddPath_dir(t *testing.T) { + tmpDir, err := ioutil.TempDir("/tmp", "foo") + if err != nil { + t.Errorf("got error setting up test %v", err) + } + + content := `meow meow meow` + if err := ioutil.WriteFile(filepath.Join(tmpDir, "foo.txt"), []byte(content), 0777); err != nil { + t.Errorf("got error writing temp file %v", err) + } + + fn := func() string { + r := NewCompositeCache() + if err := r.AddPath(tmpDir); err != nil { + t.Errorf("expected error to be nil but was %v", err) + } + + if len(r.keys) != 1 { + t.Errorf("expected len of keys to be 1 but was %v", len(r.keys)) + } + hash, err := r.Hash() + if err != nil { + t.Errorf("couldnt generate hash from test cache") + } + return hash + } + + hash1 := fn() + hash2 := fn() + if hash1 != hash2 { + t.Errorf("expected hash %v to equal hash %v", hash1, hash2) + } +} +func Test_CompositeCache_AddPath_file(t *testing.T) { + tmpfile, err := ioutil.TempFile("/tmp", "foo.txt") + if err != nil { + t.Errorf("got error setting up test %v", err) + } + defer os.Remove(tmpfile.Name()) // clean up + + content := `meow meow meow` + if _, err := tmpfile.Write([]byte(content)); err != nil { + t.Errorf("got error writing temp file %v", err) + } + if err := tmpfile.Close(); err != nil { + t.Errorf("got error closing temp file %v", err) + } + + p := tmpfile.Name() + fn := func() string { + r := NewCompositeCache() + if err := r.AddPath(p); err != nil { + t.Errorf("expected error to be nil but was %v", err) + } + + if len(r.keys) != 1 { + t.Errorf("expected len of keys to be 1 but was %v", len(r.keys)) + } + hash, err := r.Hash() + if err != nil { + t.Errorf("couldnt generate hash from test cache") + } + return hash + } + + hash1 := fn() + hash2 := fn() + if hash1 != hash2 { + t.Errorf("expected hash %v to equal hash %v", hash1, hash2) + } +} diff --git a/pkg/executor/fakes.go b/pkg/executor/fakes.go new file mode 100644 index 000000000..86ee673ed --- /dev/null +++ b/pkg/executor/fakes.go @@ -0,0 +1,180 @@ +/* +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. +*/ + +// for use in tests +package executor + +import ( + "bytes" + "errors" + "io" + "io/ioutil" + + "github.com/GoogleContainerTools/kaniko/pkg/commands" + "github.com/GoogleContainerTools/kaniko/pkg/config" + "github.com/GoogleContainerTools/kaniko/pkg/dockerfile" + v1 "github.com/google/go-containerregistry/pkg/v1" + "github.com/google/go-containerregistry/pkg/v1/types" +) + +func fakeCachePush(_ *config.KanikoOptions, _, _, _ string) error { + return nil +} + +type fakeSnapShotter struct { + file string + tarPath string +} + +func (f fakeSnapShotter) Init() error { return nil } +func (f fakeSnapShotter) TakeSnapshotFS() (string, error) { + return f.tarPath, nil +} +func (f fakeSnapShotter) TakeSnapshot(_ []string) (string, error) { + return f.tarPath, nil +} + +type MockDockerCommand struct { + contextFiles []string + cacheCommand commands.DockerCommand +} + +func (m MockDockerCommand) ExecuteCommand(c *v1.Config, args *dockerfile.BuildArgs) error { return nil } +func (m MockDockerCommand) String() string { + return "meow" +} +func (m MockDockerCommand) FilesToSnapshot() []string { + return []string{"meow-snapshot-no-cache"} +} +func (m MockDockerCommand) CacheCommand(image v1.Image) commands.DockerCommand { + return m.cacheCommand +} +func (m MockDockerCommand) FilesUsedFromContext(c *v1.Config, args *dockerfile.BuildArgs) ([]string, error) { + return m.contextFiles, nil +} +func (m MockDockerCommand) MetadataOnly() bool { + return false +} +func (m MockDockerCommand) RequiresUnpackedFS() bool { + return false +} +func (m MockDockerCommand) ShouldCacheOutput() bool { + return true +} + +type MockCachedDockerCommand struct { + contextFiles []string +} + +func (m MockCachedDockerCommand) ExecuteCommand(c *v1.Config, args *dockerfile.BuildArgs) error { + return nil +} +func (m MockCachedDockerCommand) String() string { + return "meow" +} +func (m MockCachedDockerCommand) FilesToSnapshot() []string { + return []string{"meow-snapshot"} +} +func (m MockCachedDockerCommand) CacheCommand(image v1.Image) commands.DockerCommand { + return nil +} +func (m MockCachedDockerCommand) FilesUsedFromContext(c *v1.Config, args *dockerfile.BuildArgs) ([]string, error) { + return m.contextFiles, nil +} +func (m MockCachedDockerCommand) MetadataOnly() bool { + return false +} +func (m MockCachedDockerCommand) RequiresUnpackedFS() bool { + return false +} +func (m MockCachedDockerCommand) ShouldCacheOutput() bool { + return true +} + +type fakeLayerCache struct { + retrieve bool + receivedKeys []string + img v1.Image +} + +func (f *fakeLayerCache) RetrieveLayer(key string) (v1.Image, error) { + f.receivedKeys = append(f.receivedKeys, key) + if !f.retrieve { + return nil, errors.New("could not find layer") + } + return f.img, nil +} + +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/snapshot/snapshot.go b/pkg/snapshot/snapshot.go index 5577da087..b54897d59 100644 --- a/pkg/snapshot/snapshot.go +++ b/pkg/snapshot/snapshot.go @@ -176,7 +176,7 @@ func (s *Snapshotter) scanFullFilesystem() ([]string, []string, error) { // Only add changed files. fileChanged, err := s.l.CheckFileChange(path) if err != nil { - return nil, nil, err + return nil, nil, fmt.Errorf("could not check if file has changed %s %s", path, err) } if fileChanged { logrus.Debugf("Adding %s to layer, because it was changed.", path) diff --git a/pkg/util/command_util.go b/pkg/util/command_util.go index c4b3437ea..103d22e0b 100644 --- a/pkg/util/command_util.go +++ b/pkg/util/command_util.go @@ -17,6 +17,7 @@ limitations under the License. package util import ( + "fmt" "net/http" "net/url" "os" @@ -25,7 +26,7 @@ import ( "strings" "github.com/GoogleContainerTools/kaniko/pkg/constants" - "github.com/google/go-containerregistry/pkg/v1" + v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/moby/buildkit/frontend/dockerfile/instructions" "github.com/moby/buildkit/frontend/dockerfile/parser" "github.com/moby/buildkit/frontend/dockerfile/shell" @@ -77,13 +78,16 @@ func ResolveEnvAndWildcards(sd instructions.SourcesAndDest, buildcontext string, // First, resolve any environment replacement resolvedEnvs, err := ResolveEnvironmentReplacementList(sd, envs, true) if err != nil { - return nil, "", err + return nil, "", errors.Wrap(err, "failed to resolve environment") + } + if len(resolvedEnvs) == 0 { + return nil, "", errors.New("resolved envs is empty") } dest := resolvedEnvs[len(resolvedEnvs)-1] // Resolve wildcards and get a list of resolved sources srcs, err := ResolveSources(resolvedEnvs[0:len(resolvedEnvs)-1], buildcontext) if err != nil { - return nil, "", err + return nil, "", errors.Wrap(err, "failed to resolve sources") } err = IsSrcsValid(sd, srcs, buildcontext) return srcs, dest, err @@ -219,9 +223,10 @@ func IsSrcsValid(srcsAndDest instructions.SourcesAndDest, resolvedSources []stri if IsSrcRemoteFileURL(resolvedSources[0]) { return nil } - fi, err := os.Lstat(filepath.Join(root, resolvedSources[0])) + path := filepath.Join(root, resolvedSources[0]) + fi, err := os.Lstat(path) if err != nil { - return err + return errors.Wrap(err, fmt.Sprintf("failed to get fileinfo for %v", path)) } if fi.IsDir() { return nil @@ -237,7 +242,7 @@ func IsSrcsValid(srcsAndDest instructions.SourcesAndDest, resolvedSources []stri src = filepath.Clean(src) files, err := RelativeFiles(src, root) if err != nil { - return err + return errors.Wrap(err, "failed to get relative files") } for _, file := range files { if excludeFile(file, root) { From 33f3191b1730a5b70a8abd580247b260d0f21ab8 Mon Sep 17 00:00:00 2001 From: Cole Wippern Date: Wed, 27 Nov 2019 15:03:46 -0800 Subject: [PATCH 2/5] Don't hardcode hashes for stagebuilder tests --- pkg/executor/build_test.go | 104 ++++++++++++++++++++++++++++++------- pkg/executor/fakes.go | 2 +- 2 files changed, 86 insertions(+), 20 deletions(-) diff --git a/pkg/executor/build_test.go b/pkg/executor/build_test.go index 47a7fdacd..fac9bed40 100644 --- a/pkg/executor/build_test.go +++ b/pkg/executor/build_test.go @@ -35,6 +35,7 @@ import ( "github.com/google/go-containerregistry/pkg/v1/empty" "github.com/google/go-containerregistry/pkg/v1/mutate" "github.com/moby/buildkit/frontend/dockerfile/instructions" + "github.com/sirupsen/logrus" ) func Test_reviewConfig(t *testing.T) { @@ -529,8 +530,6 @@ func Test_stageBuilder_build(t *testing.T) { image v1.Image config *v1.ConfigFile } - // The two copy command test cases use the same filesystem content and should generate the same cache keys. If they don't then something is wrong. They share this variable to help ensure that. - copyCommandCacheKey := "7263908b66952551d89fd895ffb067e2e30f474be9f38a8f1792af2b6df7c6e3" tempDirAndFile := func() (string, string) { dir, err := ioutil.TempDir("", "foo") if err != nil { @@ -549,21 +548,71 @@ func Test_stageBuilder_build(t *testing.T) { return dir, filename } testCases := []testcase{ - { - description: "fake command cache enabled but key not in cache", - opts: &config.KanikoOptions{Cache: true}, - expectedCacheKeys: []string{"2cd95a0195a42f2873273b7e8c970e3a87970bd0e5d330b3c7d068e7419e5017"}, - pushedCacheKeys: []string{"2cd95a0195a42f2873273b7e8c970e3a87970bd0e5d330b3c7d068e7419e5017"}, - }, - { - description: "fake command cache enabled and key in cache", - opts: &config.KanikoOptions{Cache: true}, - layerCache: &fakeLayerCache{ - retrieve: true, - }, - expectedCacheKeys: []string{"2cd95a0195a42f2873273b7e8c970e3a87970bd0e5d330b3c7d068e7419e5017"}, - pushedCacheKeys: []string{"2cd95a0195a42f2873273b7e8c970e3a87970bd0e5d330b3c7d068e7419e5017"}, - }, + func() testcase { + dir, file := tempDirAndFile() + filePath := filepath.Join(dir, file) + ch := NewCompositeCache("", "meow") + + ch.AddPath(filePath) + hash, err := ch.Hash() + if err != nil { + t.Errorf("couldn't create hash %v", err) + } + command := MockDockerCommand{ + contextFiles: []string{filePath}, + cacheCommand: MockCachedDockerCommand{ + contextFiles: []string{filePath}, + }, + } + + destDir, err := ioutil.TempDir("", "baz") + if err != nil { + t.Errorf("could not create temp dir %v", err) + } + return testcase{ + description: "fake command cache enabled but key not in cache", + config: &v1.ConfigFile{Config: v1.Config{WorkingDir: destDir}}, + opts: &config.KanikoOptions{Cache: true}, + expectedCacheKeys: []string{hash}, + pushedCacheKeys: []string{hash}, + commands: []commands.DockerCommand{command}, + rootDir: dir, + } + }(), + func() testcase { + dir, file := tempDirAndFile() + filePath := filepath.Join(dir, file) + ch := NewCompositeCache("", "meow") + + ch.AddPath(filePath) + hash, err := ch.Hash() + if err != nil { + t.Errorf("couldn't create hash %v", err) + } + command := MockDockerCommand{ + contextFiles: []string{filePath}, + cacheCommand: MockCachedDockerCommand{ + contextFiles: []string{filePath}, + }, + } + + destDir, err := ioutil.TempDir("", "baz") + if err != nil { + t.Errorf("could not create temp dir %v", err) + } + return testcase{ + description: "fake command cache enabled and key in cache", + opts: &config.KanikoOptions{Cache: true}, + config: &v1.ConfigFile{Config: v1.Config{WorkingDir: destDir}}, + layerCache: &fakeLayerCache{ + retrieve: true, + }, + expectedCacheKeys: []string{hash}, + pushedCacheKeys: []string{}, + commands: []commands.DockerCommand{command}, + rootDir: dir, + } + }(), { description: "fake command cache disabled and key not in cache", opts: &config.KanikoOptions{Cache: false}, @@ -605,6 +654,15 @@ func Test_stageBuilder_build(t *testing.T) { t.Errorf("could not write file contents to tar") } tarContent := buf.Bytes() + + ch := NewCompositeCache("", "") + ch.AddPath(filepath) + logrus.SetLevel(logrus.DebugLevel) + hash, err := ch.Hash() + if err != nil { + t.Errorf("couldn't create hash %v", err) + } + copyCommandCacheKey := hash return testcase{ description: "copy command cache enabled and key in cache", opts: &config.KanikoOptions{Cache: true}, @@ -649,6 +707,14 @@ func Test_stageBuilder_build(t *testing.T) { if err != nil { t.Errorf("could not create temp dir %v", err) } + filePath := filepath.Join(dir, filename) + ch := NewCompositeCache("", "") + ch.AddPath(filePath) + logrus.SetLevel(logrus.DebugLevel) + hash, err := ch.Hash() + if err != nil { + t.Errorf("couldn't create hash %v", err) + } return testcase{ description: "copy command cache enabled and key is not in cache", opts: &config.KanikoOptions{Cache: true}, @@ -664,8 +730,8 @@ func Test_stageBuilder_build(t *testing.T) { }, }, rootDir: dir, - expectedCacheKeys: []string{copyCommandCacheKey}, - pushedCacheKeys: []string{copyCommandCacheKey}, + expectedCacheKeys: []string{hash}, + pushedCacheKeys: []string{hash}, commands: func() []commands.DockerCommand { cmd, err := commands.GetCommand( &instructions.CopyCommand{ diff --git a/pkg/executor/fakes.go b/pkg/executor/fakes.go index 86ee673ed..e34a5ec23 100644 --- a/pkg/executor/fakes.go +++ b/pkg/executor/fakes.go @@ -101,7 +101,7 @@ func (m MockCachedDockerCommand) RequiresUnpackedFS() bool { return false } func (m MockCachedDockerCommand) ShouldCacheOutput() bool { - return true + return false } type fakeLayerCache struct { From 6d0c8da90eab42860dda748dc2bf26a239436477 Mon Sep 17 00:00:00 2001 From: Cole Wippern Date: Wed, 27 Nov 2019 23:02:33 -0800 Subject: [PATCH 3/5] more stagebuilder caching tests --- pkg/commands/fake_commands.go | 88 ------------- pkg/executor/build_test.go | 234 +++++++++++++++++++++++++--------- pkg/executor/fakes.go | 14 +- 3 files changed, 185 insertions(+), 151 deletions(-) delete mode 100644 pkg/commands/fake_commands.go diff --git a/pkg/commands/fake_commands.go b/pkg/commands/fake_commands.go deleted file mode 100644 index 8efee7c4d..000000000 --- a/pkg/commands/fake_commands.go +++ /dev/null @@ -1,88 +0,0 @@ -/* -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/executor/build_test.go b/pkg/executor/build_test.go index fac9bed40..02d73a9d6 100644 --- a/pkg/executor/build_test.go +++ b/pkg/executor/build_test.go @@ -19,6 +19,7 @@ package executor import ( "archive/tar" "bytes" + "fmt" "io/ioutil" "os" "path/filepath" @@ -530,26 +531,60 @@ func Test_stageBuilder_build(t *testing.T) { image v1.Image config *v1.ConfigFile } - tempDirAndFile := func() (string, string) { + tempDirAndFile := func(filenames ...string) (string, []string) { + if len(filenames) == 0 { + filenames = []string{"bar.txt"} + } dir, err := ioutil.TempDir("", "foo") if err != nil { t.Errorf("could not create temp dir %v", err) } - filename := "bar.txt" - filepath := filepath.Join(dir, filename) - err = ioutil.WriteFile(filepath, []byte(`meow`), 0777) - if err != nil { - t.Errorf("could not create temp file %v", err) + for _, filename := range filenames { + filepath := filepath.Join(dir, filename) + err = ioutil.WriteFile(filepath, []byte(`meow`), 0777) + if err != nil { + t.Errorf("could not create temp file %v", err) + } } + + return dir, filenames + } + generateTar := func(dir string, fileNames ...string) []byte { buf := bytes.NewBuffer([]byte{}) writer := tar.NewWriter(buf) defer writer.Close() - return dir, filename + for _, filename := range fileNames { + filePath := filepath.Join(dir, filename) + info, err := os.Stat(filePath) + if err != nil { + t.Errorf("could not get file info for temp file %v", err) + } + hdr, err := tar.FileInfoHeader(info, filename) + if err != nil { + t.Errorf("could not get tar header for temp file %v", err) + } + + if err := writer.WriteHeader(hdr); err != nil { + t.Errorf("could not write tar header %v", err) + } + + content, err := ioutil.ReadFile(filePath) + if err != nil { + t.Errorf("could not read tempfile %v", err) + } + + if _, err := writer.Write(content); err != nil { + t.Errorf("could not write file contents to tar") + } + } + return buf.Bytes() } + testCases := []testcase{ func() testcase { - dir, file := tempDirAndFile() + dir, files := tempDirAndFile() + file := files[0] filePath := filepath.Join(dir, file) ch := NewCompositeCache("", "meow") @@ -580,7 +615,8 @@ func Test_stageBuilder_build(t *testing.T) { } }(), func() testcase { - dir, file := tempDirAndFile() + dir, files := tempDirAndFile() + file := files[0] filePath := filepath.Join(dir, file) ch := NewCompositeCache("", "meow") @@ -625,35 +661,11 @@ func Test_stageBuilder_build(t *testing.T) { }, }, func() testcase { - dir, filename := tempDirAndFile() + dir, filenames := tempDirAndFile() + filename := filenames[0] filepath := filepath.Join(dir, filename) - buf := bytes.NewBuffer([]byte{}) - writer := tar.NewWriter(buf) - defer writer.Close() - - info, err := os.Stat(filepath) - if err != nil { - t.Errorf("could not get file info for temp file %v", err) - } - hdr, err := tar.FileInfoHeader(info, filename) - if err != nil { - t.Errorf("could not get tar header for temp file %v", err) - } - - if err := writer.WriteHeader(hdr); err != nil { - t.Errorf("could not write tar header %v", err) - } - - content, err := ioutil.ReadFile(filepath) - if err != nil { - t.Errorf("could not read tempfile %v", err) - } - - if _, err := writer.Write(content); err != nil { - t.Errorf("could not write file contents to tar") - } - tarContent := buf.Bytes() + tarContent := generateTar(dir, filename) ch := NewCompositeCache("", "") ch.AddPath(filepath) @@ -700,8 +712,8 @@ func Test_stageBuilder_build(t *testing.T) { } }(), func() testcase { - dir, filename := tempDirAndFile() - + dir, filenames := tempDirAndFile() + filename := filenames[0] tarContent := []byte{} destDir, err := ioutil.TempDir("", "baz") if err != nil { @@ -751,6 +763,110 @@ func Test_stageBuilder_build(t *testing.T) { fileName: filename, } }(), + func() testcase { + dir, filenames := tempDirAndFile() + filename := filenames[0] + tarContent := generateTar(filename) + destDir, err := ioutil.TempDir("", "baz") + if err != nil { + t.Errorf("could not create temp dir %v", err) + } + filePath := filepath.Join(dir, filename) + ch := NewCompositeCache("", fmt.Sprintf("COPY %s foo.txt", filename)) + ch.AddPath(filePath) + logrus.SetLevel(logrus.DebugLevel) + logrus.Infof("test composite key %v", ch) + hash1, err := ch.Hash() + if err != nil { + t.Errorf("couldn't create hash %v", err) + } + ch.AddKey(fmt.Sprintf("COPY %s bar.txt", filename)) + ch.AddPath(filePath) + logrus.Infof("test composite key %v", ch) + hash2, err := ch.Hash() + if err != nil { + t.Errorf("couldn't create hash %v", err) + } + ch = NewCompositeCache("", fmt.Sprintf("COPY %s foo.txt", filename)) + ch.AddKey(fmt.Sprintf("COPY %s bar.txt", filename)) + ch.AddPath(filePath) + logrus.Infof("test composite key %v", ch) + hash3, err := ch.Hash() + if err != nil { + t.Errorf("couldn't create hash %v", err) + } + image := fakeImage{ + ImageLayers: []v1.Layer{ + fakeLayer{ + TarContent: tarContent, + }, + }, + } + + dockerFile := fmt.Sprintf(` +FROM ubuntu:16.04 +COPY %s foo.txt +COPY %s bar.txt +`, filename, filename) + f, _ := ioutil.TempFile("", "") + ioutil.WriteFile(f.Name(), []byte(dockerFile), 0755) + opts := &config.KanikoOptions{ + DockerfilePath: f.Name(), + } + + stages, err := dockerfile.Stages(opts) + if err != nil { + t.Errorf("could not parse test dockerfile") + } + stage := stages[0] + cmds := stage.Commands + return testcase{ + description: "cached copy command followed by uncached copy command result in different read and write hashes", + opts: &config.KanikoOptions{Cache: true}, + rootDir: dir, + config: &v1.ConfigFile{Config: v1.Config{WorkingDir: destDir}}, + layerCache: &fakeLayerCache{ + keySequence: []string{hash1}, + img: image, + }, + image: image, + // hash1 is the read cachekey for the first layer + // hash2 is the read cachekey for the second layer + expectedCacheKeys: []string{hash1, hash2}, + // Due to CachingCopyCommand and CopyCommand returning different values the write cache key for the second copy command will never match the read cache key + // hash3 is the cachekey used to write to the cache for layer 2 + pushedCacheKeys: []string{hash3}, + commands: func() []commands.DockerCommand { + outCommands := make([]commands.DockerCommand, 0) + for _, c := range cmds { + cmd, err := commands.GetCommand( + c, + dir, + ) + if err != nil { + panic(err) + } + outCommands = append(outCommands, cmd) + } + return outCommands + //fn := func(srcAndDest []string) commands.DockerCommand { + // cmd, err := commands.GetCommand( + // &instructions.CopyCommand{ + // SourcesAndDest: srcAndDest, + // }, + // dir, + // ) + // if err != nil { + // panic(err) + // } + // return cmd + //} + //return []commands.DockerCommand{ + // fn([]string{filename, "foo.txt"}), fn([]string{filename, "bar.txt"}), + //} + }(), + } + }(), } for _, tc := range testCases { t.Run(tc.description, func(t *testing.T) { @@ -812,31 +928,33 @@ func Test_stageBuilder_build(t *testing.T) { if len(tc.expectedCacheKeys) != len(lc.receivedKeys) { t.Errorf("expected to receive %v keys but was %v", len(tc.expectedCacheKeys), len(lc.receivedKeys)) } - for _, key := range tc.expectedCacheKeys { - match := false - for _, receivedKey := range lc.receivedKeys { - if key == receivedKey { - match = true - break - } - } - if !match { - t.Errorf("expected received keys to include %v but did not %v", key, lc.receivedKeys) + expectedCached := tc.expectedCacheKeys + actualCached := lc.receivedKeys + sort.Slice(expectedCached, func(x, y int) bool { + return expectedCached[x] > expectedCached[y] + }) + sort.Slice(actualCached, func(x, y int) bool { + return actualCached[x] > actualCached[y] + }) + for i, key := range expectedCached { + if key != actualCached[i] { + t.Errorf("expected retrieved keys %d to be %v but was %v %v", i, key, actualCached[i], actualCached) } } if len(tc.pushedCacheKeys) != len(keys) { t.Errorf("expected to push %v keys but was %v", len(tc.pushedCacheKeys), len(keys)) } - for _, key := range tc.pushedCacheKeys { - match := false - for _, pushedKey := range keys { - if key == pushedKey { - match = true - break - } - } - if !match { - t.Errorf("expected pushed keys to include %v but did not %v", key, keys) + expectedPushed := tc.pushedCacheKeys + actualPushed := keys + sort.Slice(expectedPushed, func(x, y int) bool { + return expectedPushed[x] > expectedPushed[y] + }) + sort.Slice(actualPushed, func(x, y int) bool { + return actualPushed[x] > actualPushed[y] + }) + for i, key := range expectedPushed { + if key != actualPushed[i] { + t.Errorf("expected pushed keys %d to be %v but was %v %v", i, key, actualPushed[i], actualPushed) } } commands.RootDir = tmp diff --git a/pkg/executor/fakes.go b/pkg/executor/fakes.go index e34a5ec23..e9cfdc694 100644 --- a/pkg/executor/fakes.go +++ b/pkg/executor/fakes.go @@ -24,16 +24,11 @@ import ( "io/ioutil" "github.com/GoogleContainerTools/kaniko/pkg/commands" - "github.com/GoogleContainerTools/kaniko/pkg/config" "github.com/GoogleContainerTools/kaniko/pkg/dockerfile" v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/google/go-containerregistry/pkg/v1/types" ) -func fakeCachePush(_ *config.KanikoOptions, _, _, _ string) error { - return nil -} - type fakeSnapShotter struct { file string tarPath string @@ -108,10 +103,19 @@ type fakeLayerCache struct { retrieve bool receivedKeys []string img v1.Image + keySequence []string } func (f *fakeLayerCache) RetrieveLayer(key string) (v1.Image, error) { f.receivedKeys = append(f.receivedKeys, key) + if len(f.keySequence) > 0 { + if f.keySequence[0] == key { + f.keySequence = f.keySequence[1:] + return f.img, nil + } + return f.img, errors.New("could not find layer") + } + if !f.retrieve { return nil, errors.New("could not find layer") } From 828e764b95456693704518126d334a01a9ce6586 Mon Sep 17 00:00:00 2001 From: Cole Wippern Date: Thu, 28 Nov 2019 09:18:58 -0800 Subject: [PATCH 4/5] add boilerplate for composite_cache_test --- pkg/executor/composite_cache_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/pkg/executor/composite_cache_test.go b/pkg/executor/composite_cache_test.go index edd75c917..a3c7f55af 100644 --- a/pkg/executor/composite_cache_test.go +++ b/pkg/executor/composite_cache_test.go @@ -1,3 +1,19 @@ +/* +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. +*/ + package executor import ( From 7ba65daf7f0d1dfa29507ad5d4a3ded0950b25c3 Mon Sep 17 00:00:00 2001 From: Cole Wippern Date: Thu, 28 Nov 2019 09:35:41 -0800 Subject: [PATCH 5/5] cleanup executor/build_test.go --- pkg/executor/build_test.go | 257 +++++++++++++++---------------------- 1 file changed, 107 insertions(+), 150 deletions(-) diff --git a/pkg/executor/build_test.go b/pkg/executor/build_test.go index 02d73a9d6..66c0ab5b5 100644 --- a/pkg/executor/build_test.go +++ b/pkg/executor/build_test.go @@ -531,59 +531,10 @@ func Test_stageBuilder_build(t *testing.T) { image v1.Image config *v1.ConfigFile } - tempDirAndFile := func(filenames ...string) (string, []string) { - if len(filenames) == 0 { - filenames = []string{"bar.txt"} - } - dir, err := ioutil.TempDir("", "foo") - if err != nil { - t.Errorf("could not create temp dir %v", err) - } - for _, filename := range filenames { - filepath := filepath.Join(dir, filename) - err = ioutil.WriteFile(filepath, []byte(`meow`), 0777) - if err != nil { - t.Errorf("could not create temp file %v", err) - } - } - - return dir, filenames - } - generateTar := func(dir string, fileNames ...string) []byte { - buf := bytes.NewBuffer([]byte{}) - writer := tar.NewWriter(buf) - defer writer.Close() - - for _, filename := range fileNames { - filePath := filepath.Join(dir, filename) - info, err := os.Stat(filePath) - if err != nil { - t.Errorf("could not get file info for temp file %v", err) - } - hdr, err := tar.FileInfoHeader(info, filename) - if err != nil { - t.Errorf("could not get tar header for temp file %v", err) - } - - if err := writer.WriteHeader(hdr); err != nil { - t.Errorf("could not write tar header %v", err) - } - - content, err := ioutil.ReadFile(filePath) - if err != nil { - t.Errorf("could not read tempfile %v", err) - } - - if _, err := writer.Write(content); err != nil { - t.Errorf("could not write file contents to tar") - } - } - return buf.Bytes() - } testCases := []testcase{ func() testcase { - dir, files := tempDirAndFile() + dir, files := tempDirAndFile(t) file := files[0] filePath := filepath.Join(dir, file) ch := NewCompositeCache("", "meow") @@ -615,7 +566,7 @@ func Test_stageBuilder_build(t *testing.T) { } }(), func() testcase { - dir, files := tempDirAndFile() + dir, files := tempDirAndFile(t) file := files[0] filePath := filepath.Join(dir, file) ch := NewCompositeCache("", "meow") @@ -661,11 +612,11 @@ func Test_stageBuilder_build(t *testing.T) { }, }, func() testcase { - dir, filenames := tempDirAndFile() + dir, filenames := tempDirAndFile(t) filename := filenames[0] filepath := filepath.Join(dir, filename) - tarContent := generateTar(dir, filename) + tarContent := generateTar(t, dir, filename) ch := NewCompositeCache("", "") ch.AddPath(filepath) @@ -692,27 +643,18 @@ func Test_stageBuilder_build(t *testing.T) { expectedCacheKeys: []string{copyCommandCacheKey}, // CachingCopyCommand is not pushed to the cache pushedCacheKeys: []string{}, - commands: func() []commands.DockerCommand { - cmd, err := commands.GetCommand( - &instructions.CopyCommand{ - SourcesAndDest: []string{ - filename, "foo.txt", - }, + commands: getCommands(dir, []instructions.Command{ + &instructions.CopyCommand{ + SourcesAndDest: []string{ + filename, "foo.txt", }, - dir, - ) - if err != nil { - panic(err) - } - return []commands.DockerCommand{ - cmd, - } - }(), + }, + }), fileName: filename, } }(), func() testcase { - dir, filenames := tempDirAndFile() + dir, filenames := tempDirAndFile(t) filename := filenames[0] tarContent := []byte{} destDir, err := ioutil.TempDir("", "baz") @@ -731,9 +673,7 @@ func Test_stageBuilder_build(t *testing.T) { description: "copy command cache enabled and key is not in cache", opts: &config.KanikoOptions{Cache: true}, config: &v1.ConfigFile{Config: v1.Config{WorkingDir: destDir}}, - layerCache: &fakeLayerCache{ - retrieve: false, - }, + layerCache: &fakeLayerCache{}, image: fakeImage{ ImageLayers: []v1.Layer{ fakeLayer{ @@ -744,29 +684,20 @@ func Test_stageBuilder_build(t *testing.T) { rootDir: dir, expectedCacheKeys: []string{hash}, pushedCacheKeys: []string{hash}, - commands: func() []commands.DockerCommand { - cmd, err := commands.GetCommand( - &instructions.CopyCommand{ - SourcesAndDest: []string{ - filename, "foo.txt", - }, + commands: getCommands(dir, []instructions.Command{ + &instructions.CopyCommand{ + SourcesAndDest: []string{ + filename, "foo.txt", }, - dir, - ) - if err != nil { - panic(err) - } - return []commands.DockerCommand{ - cmd, - } - }(), + }, + }), fileName: filename, } }(), func() testcase { - dir, filenames := tempDirAndFile() + dir, filenames := tempDirAndFile(t) filename := filenames[0] - tarContent := generateTar(filename) + tarContent := generateTar(t, filename) destDir, err := ioutil.TempDir("", "baz") if err != nil { t.Errorf("could not create temp dir %v", err) @@ -836,35 +767,7 @@ COPY %s bar.txt // Due to CachingCopyCommand and CopyCommand returning different values the write cache key for the second copy command will never match the read cache key // hash3 is the cachekey used to write to the cache for layer 2 pushedCacheKeys: []string{hash3}, - commands: func() []commands.DockerCommand { - outCommands := make([]commands.DockerCommand, 0) - for _, c := range cmds { - cmd, err := commands.GetCommand( - c, - dir, - ) - if err != nil { - panic(err) - } - outCommands = append(outCommands, cmd) - } - return outCommands - //fn := func(srcAndDest []string) commands.DockerCommand { - // cmd, err := commands.GetCommand( - // &instructions.CopyCommand{ - // SourcesAndDest: srcAndDest, - // }, - // dir, - // ) - // if err != nil { - // panic(err) - // } - // return cmd - //} - //return []commands.DockerCommand{ - // fn([]string{filename, "foo.txt"}), fn([]string{filename, "bar.txt"}), - //} - }(), + commands: getCommands(dir, cmds), } }(), } @@ -925,40 +828,94 @@ COPY %s bar.txt t.Errorf("Expected error to be nil but was %v", err) } - if len(tc.expectedCacheKeys) != len(lc.receivedKeys) { - t.Errorf("expected to receive %v keys but was %v", len(tc.expectedCacheKeys), len(lc.receivedKeys)) - } - expectedCached := tc.expectedCacheKeys - actualCached := lc.receivedKeys - sort.Slice(expectedCached, func(x, y int) bool { - return expectedCached[x] > expectedCached[y] - }) - sort.Slice(actualCached, func(x, y int) bool { - return actualCached[x] > actualCached[y] - }) - for i, key := range expectedCached { - if key != actualCached[i] { - t.Errorf("expected retrieved keys %d to be %v but was %v %v", i, key, actualCached[i], actualCached) - } - } - if len(tc.pushedCacheKeys) != len(keys) { - t.Errorf("expected to push %v keys but was %v", len(tc.pushedCacheKeys), len(keys)) - } - expectedPushed := tc.pushedCacheKeys - actualPushed := keys - sort.Slice(expectedPushed, func(x, y int) bool { - return expectedPushed[x] > expectedPushed[y] - }) - sort.Slice(actualPushed, func(x, y int) bool { - return actualPushed[x] > actualPushed[y] - }) - for i, key := range expectedPushed { - if key != actualPushed[i] { - t.Errorf("expected pushed keys %d to be %v but was %v %v", i, key, actualPushed[i], actualPushed) - } - } + assertCacheKeys(t, tc.expectedCacheKeys, lc.receivedKeys, "receive") + assertCacheKeys(t, tc.pushedCacheKeys, keys, "push") + commands.RootDir = tmp }) } } + +func assertCacheKeys(t *testing.T, expectedCacheKeys, actualCacheKeys []string, description string) { + if len(expectedCacheKeys) != len(actualCacheKeys) { + t.Errorf("expected to %v %v keys but was %v", description, len(expectedCacheKeys), len(actualCacheKeys)) + } + + sort.Slice(expectedCacheKeys, func(x, y int) bool { + return expectedCacheKeys[x] > expectedCacheKeys[y] + }) + sort.Slice(actualCacheKeys, func(x, y int) bool { + return actualCacheKeys[x] > actualCacheKeys[y] + }) + for i, key := range expectedCacheKeys { + if key != actualCacheKeys[i] { + t.Errorf("expected to %v keys %d to be %v but was %v %v", description, i, key, actualCacheKeys[i], actualCacheKeys) + } + } +} + +func getCommands(dir string, cmds []instructions.Command) []commands.DockerCommand { + outCommands := make([]commands.DockerCommand, 0) + for _, c := range cmds { + cmd, err := commands.GetCommand( + c, + dir, + ) + if err != nil { + panic(err) + } + outCommands = append(outCommands, cmd) + } + return outCommands + +} + +func tempDirAndFile(t *testing.T) (string, []string) { + filenames := []string{"bar.txt"} + + dir, err := ioutil.TempDir("", "foo") + if err != nil { + t.Errorf("could not create temp dir %v", err) + } + for _, filename := range filenames { + filepath := filepath.Join(dir, filename) + err = ioutil.WriteFile(filepath, []byte(`meow`), 0777) + if err != nil { + t.Errorf("could not create temp file %v", err) + } + } + + return dir, filenames +} +func generateTar(t *testing.T, dir string, fileNames ...string) []byte { + buf := bytes.NewBuffer([]byte{}) + writer := tar.NewWriter(buf) + defer writer.Close() + + for _, filename := range fileNames { + filePath := filepath.Join(dir, filename) + info, err := os.Stat(filePath) + if err != nil { + t.Errorf("could not get file info for temp file %v", err) + } + hdr, err := tar.FileInfoHeader(info, filename) + if err != nil { + t.Errorf("could not get tar header for temp file %v", err) + } + + if err := writer.WriteHeader(hdr); err != nil { + t.Errorf("could not write tar header %v", err) + } + + content, err := ioutil.ReadFile(filePath) + if err != nil { + t.Errorf("could not read tempfile %v", err) + } + + if _, err := writer.Write(content); err != nil { + t.Errorf("could not write file contents to tar") + } + } + return buf.Bytes() +}