Skip to content

Commit

Permalink
Use filename in cache key to prevent collisions under rename
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Steve Ramage committed Jun 16, 2024
1 parent ad1896a commit f2ef2b3
Show file tree
Hide file tree
Showing 2 changed files with 98 additions and 4 deletions.
15 changes: 15 additions & 0 deletions pkg/executor/composite_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
87 changes: 83 additions & 4 deletions pkg/executor/composite_cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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()
Expand All @@ -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)
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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)
}
}

0 comments on commit f2ef2b3

Please sign in to comment.