From f2ef2b385ce2f2938e259209349eb2fc9e47c0a1 Mon Sep 17 00:00:00 2001 From: Steve Ramage Date: Sun, 16 Jun 2024 12:51:30 -0700 Subject: [PATCH] Use filename in cache key to prevent collisions under rename Issues #2241 #1678 both point to cases where renames can point to incorrect images being used with caching. This commit adds the path of the file (relative to the build context to the hash). A different approach would be to change the underlying function in CacheHasher to include the name (and maybe file size), this was avoided for two reasons: 1. It was unclear whether this would change or break the computed digests outside the context of caching. 2. The CacheHasher does not know the prefix to strip in the filename to compute the hash. --- pkg/executor/composite_cache.go | 15 +++++ pkg/executor/composite_cache_test.go | 87 ++++++++++++++++++++++++++-- 2 files changed, 98 insertions(+), 4 deletions(-) diff --git a/pkg/executor/composite_cache.go b/pkg/executor/composite_cache.go index 2c1e00d25e..70333cf8eb 100644 --- a/pkg/executor/composite_cache.go +++ b/pkg/executor/composite_cache.go @@ -108,6 +108,21 @@ func hashDir(p string, context util.FileContext) (bool, string, error) { if err != nil { return err } + + absPath, err := filepath.Abs(path) + if err != nil { + return err + } + + absRoot, err := filepath.Abs(context.Root) + if err != nil { + return err + } + + if _, err := sha.Write([]byte(strings.TrimPrefix(absPath, absRoot))); err != nil { + return err + } + if _, err := sha.Write([]byte(fileHash)); err != nil { return err } diff --git a/pkg/executor/composite_cache_test.go b/pkg/executor/composite_cache_test.go index 86dc73f14c..1212fac906 100644 --- a/pkg/executor/composite_cache_test.go +++ b/pkg/executor/composite_cache_test.go @@ -96,6 +96,7 @@ func Test_CompositeCache_AddPath_dir(t *testing.T) { t.Errorf("expected hash %v to equal hash %v", hash1, hash2) } } + func Test_CompositeCache_AddPath_file(t *testing.T) { tmpfile, err := os.CreateTemp("/tmp", "foo.txt") if err != nil { @@ -206,8 +207,6 @@ func Test_CompositeKey_AddPath_Works(t *testing.T) { }, } - fileContext := util.FileContext{} - for _, test := range tests { t.Run(test.name, func(t *testing.T) { testDir1 := t.TempDir() @@ -222,11 +221,11 @@ func Test_CompositeKey_AddPath_Works(t *testing.T) { t.Fatalf("Error creating filesytem structure: %s", err) } - hash1, err := hashDirectory(testDir1, fileContext) + hash1, err := hashDirectory(testDir1, util.FileContext{Root: testDir1}) if err != nil { t.Fatalf("Failed to calculate hash: %s", err) } - hash2, err := hashDirectory(testDir2, fileContext) + hash2, err := hashDirectory(testDir2, util.FileContext{Root: testDir2}) if err != nil { t.Fatalf("Failed to calculate hash: %s", err) } @@ -435,10 +434,13 @@ func Test_CompositeKey_AddPath_WithExtraFilIgnored_Works(t *testing.T) { t.Fatalf("Error creating filesytem structure: %s", err) } + fileContext.Root = testDir1 hash1, err := hashDirectory(testDir1, fileContext) if err != nil { t.Fatalf("Failed to calculate hash: %s", err) } + + fileContext.Root = testDir2 hash2, err := hashDirectory(testDir2, fileContext) if err != nil { t.Fatalf("Failed to calculate hash: %s", err) @@ -508,10 +510,13 @@ func Test_CompositeKey_AddPath_WithExtraDirIgnored_Works(t *testing.T) { t.Fatalf("Error creating filesytem structure: %s", err) } + fileContext.Root = testDir1 hash1, err := hashDirectory(testDir1, fileContext) if err != nil { t.Fatalf("Failed to calculate hash: %s", err) } + + fileContext.Root = testDir2 hash2, err := hashDirectory(testDir2, fileContext) if err != nil { t.Fatalf("Failed to calculate hash: %s", err) @@ -523,3 +528,77 @@ func Test_CompositeKey_AddPath_WithExtraDirIgnored_Works(t *testing.T) { }) } } + +func Test_CompositeCache_AddPath_DiffFileNames_Cache_Differently_Works(t *testing.T) { + tmpDir1 := t.TempDir() + tmpDir2 := t.TempDir() + + content := `meow meow meow` + if err := os.WriteFile(filepath.Join(tmpDir1, "foo.txt"), []byte(content), 0777); err != nil { + t.Errorf("got error writing temp file %v", err) + } + + if err := os.WriteFile(filepath.Join(tmpDir2, "bar.txt"), []byte(content), 0777); err != nil { + t.Errorf("got error writing temp file %v", err) + } + + fn := func(p string) string { + r := NewCompositeCache() + if err := r.AddPath(p, util.FileContext{Root: 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(tmpDir1) + hash2 := fn(tmpDir2) + if hash1 == hash2 { + t.Errorf("expected hash %v to be different than hash %v", hash1, hash2) + } +} + +func Test_CompositeCache_AddPath_SameFileNames_In_Diff_Contexts_Works(t *testing.T) { + tmpDir1 := t.TempDir() + tmpDir2 := t.TempDir() + + content := `meow meow meow` + if err := os.WriteFile(filepath.Join(tmpDir1, "foo.txt"), []byte(content), 0777); err != nil { + t.Errorf("got error writing temp file %v", err) + } + + if err := os.WriteFile(filepath.Join(tmpDir2, "foo.txt"), []byte(content), 0777); err != nil { + t.Errorf("got error writing temp file %v", err) + } + + fn := func(p string) string { + r := NewCompositeCache() + if err := r.AddPath(p, util.FileContext{Root: 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(tmpDir1) + hash2 := fn(tmpDir2) + if hash1 != hash2 { + t.Errorf("expected hash %v to be the same as hash %v", hash1, hash2) + } +}