Skip to content

Commit

Permalink
Fix GoogleContainerTools#899 cached copy results in inconsistent key
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@cvgw
cvgw committed Dec 15, 2019
1 parent d72a960 commit 9e9b8a6
Show file tree
Hide file tree
Showing 3 changed files with 141 additions and 42 deletions.
60 changes: 39 additions & 21 deletions pkg/commands/copy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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,
}
}

Expand All @@ -172,6 +156,7 @@ type CachingCopyCommand struct {
img v1.Image
extractedFiles []string
cmd *instructions.CopyCommand
buildcontext string
extractFn util.ExtractFunction
}

Expand All @@ -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
}
Expand Down Expand Up @@ -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
}
20 changes: 14 additions & 6 deletions pkg/commands/copy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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",
Expand Down Expand Up @@ -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)
}
}

Expand Down
103 changes: 88 additions & 15 deletions pkg/executor/build_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -698,34 +697,103 @@ 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)
}
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{
Expand All @@ -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}},
Expand All @@ -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),
}
}(),
}
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit 9e9b8a6

Please sign in to comment.