From 9e9b8a6e710a64f4d609e6b752b58bc3c17f7f87 Mon Sep 17 00:00:00 2001 From: Cole Wippern Date: Sun, 15 Dec 2019 10:23:31 -0800 Subject: [PATCH] Fix #899 cached copy results in inconsistent key * Update cached copy command to return the same result for files used from context so that cached and uncached copy commands produce the same cache key * Update tests for fix * Add test for cached run command key consistency --- pkg/commands/copy.go | 60 +++++++++++++-------- pkg/commands/copy_test.go | 20 ++++--- pkg/executor/build_test.go | 103 +++++++++++++++++++++++++++++++------ 3 files changed, 141 insertions(+), 42 deletions(-) diff --git a/pkg/commands/copy.go b/pkg/commands/copy.go index 6bbff699a9..fd4b18a04f 100644 --- a/pkg/commands/copy.go +++ b/pkg/commands/copy.go @@ -121,24 +121,7 @@ func (c *CopyCommand) String() string { } func (c *CopyCommand) FilesUsedFromContext(config *v1.Config, buildArgs *dockerfile.BuildArgs) ([]string, error) { - // We don't use the context if we're performing a copy --from. - if c.cmd.From != "" { - return nil, nil - } - - replacementEnvs := buildArgs.ReplacementEnvs(config.Env) - srcs, _, err := util.ResolveEnvAndWildcards(c.cmd.SourcesAndDest, c.buildcontext, replacementEnvs) - if err != nil { - return nil, err - } - - files := []string{} - for _, src := range srcs { - fullPath := filepath.Join(c.buildcontext, src) - files = append(files, fullPath) - } - logrus.Infof("Using files from context: %v", files) - return files, nil + return copyCmdFilesUsedFromContext(config, buildArgs, c.cmd, c.buildcontext) } func (c *CopyCommand) MetadataOnly() bool { @@ -157,9 +140,10 @@ func (c *CopyCommand) ShouldCacheOutput() bool { func (c *CopyCommand) CacheCommand(img v1.Image) DockerCommand { return &CachingCopyCommand{ - img: img, - cmd: c.cmd, - extractFn: util.ExtractFile, + img: img, + cmd: c.cmd, + buildcontext: c.buildcontext, + extractFn: util.ExtractFile, } } @@ -172,6 +156,7 @@ type CachingCopyCommand struct { img v1.Image extractedFiles []string cmd *instructions.CopyCommand + buildcontext string extractFn util.ExtractFunction } @@ -192,6 +177,10 @@ func (cr *CachingCopyCommand) ExecuteCommand(config *v1.Config, buildArgs *docke return nil } +func (cr *CachingCopyCommand) FilesUsedFromContext(config *v1.Config, buildArgs *dockerfile.BuildArgs) ([]string, error) { + return copyCmdFilesUsedFromContext(config, buildArgs, cr.cmd, cr.buildcontext) +} + func (cr *CachingCopyCommand) FilesToSnapshot() []string { return cr.extractedFiles } @@ -224,3 +213,32 @@ func resolveIfSymlink(destPath string) (string, error) { } return destPath, nil } + +func copyCmdFilesUsedFromContext( + config *v1.Config, buildArgs *dockerfile.BuildArgs, cmd *instructions.CopyCommand, + buildcontext string, +) ([]string, error) { + // We don't use the context if we're performing a copy --from. + if cmd.From != "" { + return nil, nil + } + + replacementEnvs := buildArgs.ReplacementEnvs(config.Env) + + srcs, _, err := util.ResolveEnvAndWildcards( + cmd.SourcesAndDest, buildcontext, replacementEnvs, + ) + if err != nil { + return nil, err + } + + files := []string{} + for _, src := range srcs { + fullPath := filepath.Join(buildcontext, src) + files = append(files, fullPath) + } + + logrus.Debugf("Using files from context: %v", files) + + return files, nil +} diff --git a/pkg/commands/copy_test.go b/pkg/commands/copy_test.go index d031680599..492211951c 100644 --- a/pkg/commands/copy_test.go +++ b/pkg/commands/copy_test.go @@ -218,6 +218,8 @@ func Test_resolveIfSymlink(t *testing.T) { } func Test_CachingCopyCommand_ExecuteCommand(t *testing.T) { + tempDir := setupTestTemp() + tarContent, err := prepareTarFixture([]string{"foo.txt"}) if err != nil { t.Errorf("couldn't prepare tar fixture %v", err) @@ -238,12 +240,19 @@ func Test_CachingCopyCommand_ExecuteCommand(t *testing.T) { } testCases := []testCase{ func() testCase { + err = ioutil.WriteFile(filepath.Join(tempDir, "foo.txt"), []byte("meow"), 0644) + if err != nil { + t.Errorf("couldn't write tempfile %v", err) + t.FailNow() + } + c := &CachingCopyCommand{ img: fakeImage{ ImageLayers: []v1.Layer{ fakeLayer{TarContent: tarContent}, }, }, + buildcontext: tempDir, cmd: &instructions.CopyCommand{ SourcesAndDest: []string{ "foo.txt", "foo.txt", @@ -339,17 +348,16 @@ func Test_CachingCopyCommand_ExecuteCommand(t *testing.T) { 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") + t.Errorf("failed to get files used from context from command %v", err) } - if len(cmdFiles) != 0 { - t.Errorf("expected files used from context to be empty but was not") + + if len(cmdFiles) != len(tc.contextFiles) { + t.Errorf("expected files used from context to equal %v but was %v", tc.contextFiles, cmdFiles) } } diff --git a/pkg/executor/build_test.go b/pkg/executor/build_test.go index d5c07acfc0..db3f551548 100644 --- a/pkg/executor/build_test.go +++ b/pkg/executor/build_test.go @@ -36,7 +36,6 @@ 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) { @@ -620,7 +619,7 @@ func Test_stageBuilder_build(t *testing.T) { ch := NewCompositeCache("", "") ch.AddPath(filepath) - logrus.SetLevel(logrus.DebugLevel) + hash, err := ch.Hash() if err != nil { t.Errorf("couldn't create hash %v", err) @@ -664,7 +663,7 @@ func Test_stageBuilder_build(t *testing.T) { 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) @@ -698,22 +697,24 @@ func Test_stageBuilder_build(t *testing.T) { dir, filenames := tempDirAndFile(t) filename := filenames[0] tarContent := generateTar(t, 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) + + ch := NewCompositeCache("", fmt.Sprintf("RUN foobar")) + 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) @@ -721,11 +722,78 @@ func Test_stageBuilder_build(t *testing.T) { 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() + + image := fakeImage{ + ImageLayers: []v1.Layer{ + fakeLayer{ + TarContent: tarContent, + }, + }, + } + + dockerFile := fmt.Sprintf(` +FROM ubuntu:16.04 +RUN foobar +COPY %s bar.txt +`, 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 run command followed by uncached copy command result in consistent 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}, + pushedCacheKeys: []string{hash2}, + commands: getCommands(dir, cmds), + } + }(), + func() testcase { + dir, filenames := tempDirAndFile(t) + filename := filenames[0] + tarContent := generateTar(t, 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) + + 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) + + 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) + image := fakeImage{ ImageLayers: []v1.Layer{ fakeLayer{ @@ -749,10 +817,12 @@ COPY %s bar.txt 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", + description: "cached copy command followed by uncached copy command result in consistent read and write hashes", opts: &config.KanikoOptions{Cache: true}, rootDir: dir, config: &v1.ConfigFile{Config: v1.Config{WorkingDir: destDir}}, @@ -764,10 +834,8 @@ COPY %s bar.txt // 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: getCommands(dir, cmds), + pushedCacheKeys: []string{hash2}, + commands: getCommands(dir, cmds), } }(), } @@ -848,6 +916,11 @@ func assertCacheKeys(t *testing.T, expectedCacheKeys, actualCacheKeys []string, sort.Slice(actualCacheKeys, func(x, y int) bool { return actualCacheKeys[x] > actualCacheKeys[y] }) + + if len(expectedCacheKeys) != len(actualCacheKeys) { + t.Errorf("expected %v to equal %v", actualCacheKeys, expectedCacheKeys) + } + 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)