Include parent directories of include patterns in cache checksum

Signed-off-by: Aaron Lehmann <[email protected]>
moby · Apr 27, 2021 · 73c0496 · 73c0496
1 parent bb9c8eb
commit 73c0496
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 20 deletions.
diff --git a/cache/contenthash/checksum.go b/cache/contenthash/checksum.go
@@ -510,15 +510,20 @@ treeWalk:
 		if !ok {
 			break
 		}
+		fn := string(convertKeyToPath(k))
+		dirHeader := false
 		if len(k) > 0 && k[len(k)-1] == byte(0) {
-			continue
+			dirHeader = true
+			fn = fn[:len(fn)-1]
 		}
-		fn := string(convertKeyToPath(k))
-		b, err := shouldIncludePath(p, fn, opts.Wildcard, rootedIncludePatterns, excludePatternMatcher, lastIncludedDir)
+		b, partialMatch, err := shouldIncludePath(p, fn, opts.Wildcard, rootedIncludePatterns, excludePatternMatcher, lastIncludedDir)
 		if err != nil {
 			return nil, err
 		}
-		if !b {
+		// Dir headers for parent dirs of an include pattern should be included
+		// in the digest because their metadata may be copied by a copy that
+		// includes files or subdirs underneath them.
+		if !b || dirHeader != partialMatch {
 			continue
 		}
 
@@ -567,42 +572,47 @@ func shouldIncludePath(
 	rootedIncludePatterns []string,
 	excludePatternMatcher *fileutils.PatternMatcher,
 	lastIncludedDir string,
-) (bool, error) {
+) (bool, bool, error) {
 	if wildcard {
 		include, err := path.Match(p, candidate)
 		if err != nil {
-			return include, err
+			return include, false, err
 		}
 		if !include {
-			return false, nil
+			return false, false, nil
 		}
 	} else if !strings.HasPrefix(candidate+"/", p+"/") {
-		return false, nil
+		return false, false, nil
 	}
 
+	partial := false
 	if len(rootedIncludePatterns) != 0 &&
 		(lastIncludedDir == "" ||
 			!strings.HasPrefix(candidate, lastIncludedDir+"/")) {
+		partial = true
 		matched := false
 		for _, pattern := range rootedIncludePatterns {
-			if ok, partial := prefix.Match(pattern, candidate); ok && !partial {
+			if ok, partialMatch := prefix.Match(pattern, candidate); ok {
 				matched = true
-				break
+				if !partialMatch {
+					partial = false
+					break
+				}
 			}
 		}
 		if !matched {
-			return false, nil
+			return false, partial, nil
 		}
 	}
 
 	if excludePatternMatcher == nil {
-		return true, nil
+		return true, partial, nil
 	}
 	m, err := excludePatternMatcher.Matches(candidate)
 	if err != nil {
-		return false, errors.Wrap(err, "failed to match excludepatterns")
+		return false, partial, errors.Wrap(err, "failed to match excludepatterns")
 	}
-	return !m, nil
+	return !m, partial, nil
 }
 
 func (cc *cacheContext) checksumNoFollow(ctx context.Context, m *mount, p string) (*CacheRecord, error) {

diff --git a/cache/contenthash/checksum_test.go b/cache/contenthash/checksum_test.go
@@ -471,9 +471,18 @@ func TestChecksumIncludeExclude(t *testing.T) {
 
 	require.NotEqual(t, dgstFoo, dgstFooBar)
 
-	dgstD0, err := cc.Checksum(context.TODO(), ref, "", ChecksumOpts{IncludePatterns: []string{"d0/*"}}, nil)
+	dgstD0, err := cc.Checksum(context.TODO(), ref, "", ChecksumOpts{IncludePatterns: []string{"d0"}}, nil)
 	require.NoError(t, err)
-	dgstD1, err := cc.Checksum(context.TODO(), ref, "", ChecksumOpts{IncludePatterns: []string{"d1/*"}}, nil)
+	dgstD1, err := cc.Checksum(context.TODO(), ref, "", ChecksumOpts{IncludePatterns: []string{"d1"}}, nil)
+	require.NoError(t, err)
+
+	dgstD0Star, err := cc.Checksum(context.TODO(), ref, "", ChecksumOpts{IncludePatterns: []string{"d0/*"}}, nil)
+	require.NoError(t, err)
+	dgstD1Star, err := cc.Checksum(context.TODO(), ref, "", ChecksumOpts{IncludePatterns: []string{"d1/*"}}, nil)
+	require.NoError(t, err)
+
+	// Nothing matches pattern, but d2's metadata should be captured in the checksum
+	dgstD2Foo, err := cc.Checksum(context.TODO(), ref, "", ChecksumOpts{IncludePatterns: []string{"d2/foo"}}, nil)
 	require.NoError(t, err)
 
 	err = ref.Release(context.TODO())
@@ -489,6 +498,7 @@ func TestChecksumIncludeExclude(t *testing.T) {
 		"ADD d0/xyz file xyz",
 		"ADD d1 dir",
 		"ADD d1/def file def",
+		"ADD d2 dir",
 	}
 
 	ref = createRef(t, cm, ch)
@@ -504,17 +514,29 @@ func TestChecksumIncludeExclude(t *testing.T) {
 	require.Equal(t, dgstFoo, dgstFoo2)
 	require.Equal(t, dgstFooBar, dgstFooBar2)
 
-	dgstD02, err := cc.Checksum(context.TODO(), ref, "", ChecksumOpts{IncludePatterns: []string{"d0/*"}}, nil)
+	dgstD02, err := cc.Checksum(context.TODO(), ref, "", ChecksumOpts{IncludePatterns: []string{"d0"}}, nil)
 	require.NoError(t, err)
 	require.NotEqual(t, dgstD0, dgstD02)
 
-	dgstD12, err := cc.Checksum(context.TODO(), ref, "", ChecksumOpts{IncludePatterns: []string{"d1/*"}}, nil)
+	dgstD12, err := cc.Checksum(context.TODO(), ref, "", ChecksumOpts{IncludePatterns: []string{"d1"}}, nil)
 	require.NoError(t, err)
 	require.Equal(t, dgstD1, dgstD12)
 
-	dgstD0Exclude, err := cc.Checksum(context.TODO(), ref, "", ChecksumOpts{IncludePatterns: []string{"d0/*"}, ExcludePatterns: []string{"d0/xyz"}}, nil)
+	dgstD0Star2, err := cc.Checksum(context.TODO(), ref, "", ChecksumOpts{IncludePatterns: []string{"d0/*"}}, nil)
+	require.NoError(t, err)
+	require.NotEqual(t, dgstD0Star, dgstD0Star2)
+
+	dgstD1Star2, err := cc.Checksum(context.TODO(), ref, "", ChecksumOpts{IncludePatterns: []string{"d1/*"}}, nil)
+	require.NoError(t, err)
+	require.Equal(t, dgstD1Star, dgstD1Star2)
+
+	dgstD0StarExclude, err := cc.Checksum(context.TODO(), ref, "", ChecksumOpts{IncludePatterns: []string{"d0/*"}, ExcludePatterns: []string{"d0/xyz"}}, nil)
+	require.NoError(t, err)
+	require.Equal(t, dgstD0Star, dgstD0StarExclude)
+
+	dgstD2Foo2, err := cc.Checksum(context.TODO(), ref, "", ChecksumOpts{IncludePatterns: []string{"d2/foo"}}, nil)
 	require.NoError(t, err)
-	require.Equal(t, dgstD0, dgstD0Exclude)
+	require.NotEqual(t, dgstD2Foo, dgstD2Foo2)
 
 	err = ref.Release(context.TODO())
 	require.NoError(t, err)