From a42adb9bb41755f72f357504f717a9d8693af2fd Mon Sep 17 00:00:00 2001 From: Gilbert Gilb's Date: Tue, 19 Oct 2021 08:31:01 +0200 Subject: [PATCH] Fix composite cache key for multi-stage copy command (#1735) * chore: add workflows for pr tests * fix unit tests * fix formatting * chore: fix gobuild * change minikube script * chore: fix lint install script * chore: ignore and fix tests * fix lint and run gofmt * lint fixes * k8s executor image only * fix Makefile * fix travis env variables * more info on k8s tests * fix travis run * fix * fix * fix * fix log * some more changes * increase timeout * delete travis.yml and fix multiple copy tests * fix registry mirror * fix lint * add concurency * last attemot to fix k8 integrations * diff id for diff workflows * Fix composite cache key for multi-stage copy command (#1706) PR #1518 reintroduced COPY layers caching using the `--cache-copy-layers` flag. Unfortunately, this PR also introduced a bug by not including the stage digest into the caching key of the COPY command when the `--cache-copy-layers` flag was not set. As a result, kaniko would use any previous (possibly stalled) layer from the cache because the digest of the "COPY --from" command would never change. PR author probably expected Go to fallthrough in the switch just like C does. However, this is not the case. Go does not fallthrough in switch-statements by default and requires the fallthrough keyword to be used. Note that this keyword is not available in type-switches though, because it wouldn't work properly with typings. * refactor: add an abstract copy command interface to avoid code duplication * fix typo in error message Co-authored-by: Tejal Desai --- integration/integration_test.go | 2 +- pkg/commands/copy.go | 17 ++++++++ pkg/executor/build.go | 6 +-- pkg/executor/build_test.go | 75 +++++++++++++++++++++++++++++++++ 4 files changed, 95 insertions(+), 5 deletions(-) diff --git a/integration/integration_test.go b/integration/integration_test.go index 278eb0ed7..bfe0c2084 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -483,7 +483,7 @@ func TestCache(t *testing.T) { } // Build the second image which should pull from the cache if err := imageBuilder.buildCachedImages(config, cache, dockerfilesPath, 1, args); err != nil { - t.Fatalf("error building cached image for the first time: %v", err) + t.Fatalf("error building cached image for the second time: %v", err) } // Make sure both images are the same kanikoVersion0 := GetVersionedKanikoImage(config.imageRepo, dockerfile, 0) diff --git a/pkg/commands/copy.go b/pkg/commands/copy.go index 86671b2b4..977af6673 100644 --- a/pkg/commands/copy.go +++ b/pkg/commands/copy.go @@ -293,3 +293,20 @@ func copyCmdFilesUsedFromContext( return files, nil } + +// AbstractCopyCommand can either be a CopyCommand or a CachingCopyCommand. +type AbstractCopyCommand interface { + From() string +} + +// CastAbstractCopyCommand tries to convert a command to an AbstractCopyCommand. +func CastAbstractCopyCommand(cmd interface{}) (AbstractCopyCommand, bool) { + switch v := cmd.(type) { + case *CopyCommand: + return v, true + case *CachingCopyCommand: + return v, true + } + + return nil, false +} diff --git a/pkg/executor/build.go b/pkg/executor/build.go index f29459480..a37a7fc0e 100644 --- a/pkg/executor/build.go +++ b/pkg/executor/build.go @@ -187,10 +187,8 @@ func (s *stageBuilder) populateCompositeKey(command fmt.Stringer, files []string } // Add the next command to the cache key. compositeKey.AddKey(resolvedCmd) - switch v := command.(type) { - case *commands.CopyCommand: - case *commands.CachingCopyCommand: - compositeKey = s.populateCopyCmdCompositeKey(command, v.From(), compositeKey) + if copyCmd, ok := commands.CastAbstractCopyCommand(command); ok == true { + compositeKey = s.populateCopyCmdCompositeKey(command, copyCmd.From(), compositeKey) } for _, f := range files { diff --git a/pkg/executor/build_test.go b/pkg/executor/build_test.go index 4cb872c4d..e2ee3ee83 100644 --- a/pkg/executor/build_test.go +++ b/pkg/executor/build_test.go @@ -1361,6 +1361,81 @@ func hashCompositeKeys(t *testing.T, ck1 CompositeCache, ck2 CompositeCache) (st return key1, key2 } +func Test_stageBuild_populateCompositeKeyForCopyCommand(t *testing.T) { + // See https://github.com/GoogleContainerTools/kaniko/issues/589 + + for _, tc := range []struct { + description string + command string + expectedCacheKey string + }{ + { + description: "multi-stage copy command", + command: "COPY --from=0 foo.txt bar.txt", + expectedCacheKey: "COPY --from=0 foo.txt bar.txt-some-cache-key", + }, + { + description: "copy command", + command: "COPY foo.txt bar.txt", + expectedCacheKey: "COPY foo.txt bar.txt", + }, + } { + t.Run(tc.description, func(t *testing.T) { + instructions, err := dockerfile.ParseCommands([]string{tc.command}) + if err != nil { + t.Fatal(err) + } + + fc := util.FileContext{Root: "workspace"} + copyCommand, err := commands.GetCommand(instructions[0], fc, false, true) + if err != nil { + t.Fatal(err) + } + + for _, useCacheCommand := range []bool{false, true} { + t.Run(fmt.Sprintf("CacheCommand=%t", useCacheCommand), func(t *testing.T) { + var cmd fmt.Stringer = copyCommand + if useCacheCommand { + cmd = copyCommand.(*commands.CopyCommand).CacheCommand(nil) + } + + sb := &stageBuilder{ + fileContext: fc, + stageIdxToDigest: map[string]string{ + "0": "some-digest", + }, + digestToCacheKey: map[string]string{ + "some-digest": "some-cache-key", + }, + } + + ck := CompositeCache{} + ck, err = sb.populateCompositeKey( + cmd, + []string{}, + ck, + dockerfile.NewBuildArgs([]string{}), + []string{}, + ) + if err != nil { + t.Fatal(err) + } + + actualCacheKey := ck.Key() + if tc.expectedCacheKey != actualCacheKey { + t.Errorf( + "Expected cache key to be %s, was %s", + tc.expectedCacheKey, + actualCacheKey, + ) + } + + }) + } + }) + } +} + func Test_ResolveCrossStageInstructions(t *testing.T) { df := ` FROM scratch