Follow links in includedPaths to resolve incorrect caching when sourc…

…e path is behind symlink As discussed in moby#2300, includedPaths does not resolve symlinks when looking up the source path in the prefix tree. If the user requests a path that involves symlinks (for example, /a/foo when a symlink /a -> /b exists), includedPaths will not find it, and will expect nothing to be copied. This does not match the actual copy behavior implemented in fsutil, which will follow symlinks in prefix components of a given path, so it can end up caching an empty result even though the copy will produce a non-empty result, which is quite bad. To fix this, use getFollowLinks to resolve the path before walking it. In the wildcard case, this is done to the non-wildcard prefix of the path (if any), which matches the behavior in fsutil. Fixes the repro case here: https://gist.github.com/aaronlehmann/64054c9a2cff0d27e200cc107bba3d69 Fixes moby#2300 Signed-off-by: Aaron Lehmann <[email protected]>
aaronlehmann · Aug 18, 2021 · 56c0981 · 56c0981
1 parent 7fa9e57
commit 56c0981
Show file tree

Hide file tree

Showing 2 changed files with 140 additions and 18 deletions.
diff --git a/cache/contenthash/checksum.go b/cache/contenthash/checksum.go
@@ -506,21 +506,77 @@ func (cc *cacheContext) includedPaths(ctx context.Context, m *mount, p string, o
 	txn := cc.tree.Txn()
 	root = txn.Root()
 	var (
-		updated bool
-		iter    *iradix.Iterator
-		k       []byte
-		kOk     bool
+		updated        bool
+		iter           *iradix.Iterator
+		k              []byte
+		kOk            bool
+		origPrefix     string
+		resolvedPrefix string
 	)
 
 	iter = root.Iterator()
 
 	if opts.Wildcard {
-		k, _, kOk = iter.Next()
+		// For consistency with what the copy implementation in fsutil
+		// does: split pattern into non-wildcard prefix and rest of
+		// pattern, then follow symlinks when resolving the non-wildcard
+		// prefix.
+
+		d1, d2 := splitWildcards(p)
+		if d1 != "/" {
+			origPrefix = d1
+			k = convertPathToKey([]byte(d1))
+			linksWalked := 0
+			if d2 != "" {
+				// getFollowLinks only handles symlinks in path
+				// components before the last component, so
+				// handle last component in d1 specially.
+				for {
+					v, ok := root.Get(k)
+
+					if !ok || v.(*CacheRecord).Type != CacheRecordTypeSymlink {
+						break
+					}
+
+					linksWalked++
+					if linksWalked > 255 {
+						return nil, errors.Errorf("too many links")
+					}
+
+					dirPath := path.Clean(d1)
+					if dirPath == "." || dirPath == "/" {
+						dirPath = ""
+					}
+					d1 = path.Clean(v.(*CacheRecord).Linkname)
+					if !path.IsAbs(d1) {
+						d1 = path.Clean(path.Join("/", path.Join(path.Dir(dirPath), d1)))
+					}
+					k = convertPathToKey([]byte(d1))
+				}
+			}
+		}
 	} else {
-		k = convertPathToKey([]byte(p))
-		if _, kOk = root.Get(k); kOk {
+		origPrefix = p
+		k = convertPathToKey([]byte(origPrefix))
+	}
+
+	if origPrefix != "" {
+		// We need to resolve symlinks here, in case the base path
+		// involves a symlink. That will match fsutil behavior of
+		// calling functions such as stat and walk.
+		var cr *CacheRecord
+		k, cr, err = getFollowLinks(root, k, true)
+		if err != nil {
+			return nil, err
+		}
+
+		if kOk = (cr != nil); kOk {
 			iter.SeekLowerBound(append(append([]byte{}, k...), 0))
 		}
+
+		resolvedPrefix = string(convertKeyToPath(k))
+	} else {
+		k, _, kOk = iter.Next()
 	}
 
 	var (
@@ -531,6 +587,20 @@ func (cc *cacheContext) includedPaths(ctx context.Context, m *mount, p string, o
 	for kOk {
 		fn := string(convertKeyToPath(k))
 
+		// Convert the path prefix from what we found in the prefix
+		// tree to what the argument specified.
+		//
+		// For example, if the original 'p' argument was /a/b and there
+		// is a symlink a->c, we want fn to be /a/b/foo rather than
+		// /c/b/foo. This is necessary to ensure correct pattern
+		// matching.
+		//
+		// When wildcards are enabled, this translation applies to the
+		// portion of 'p' before any wildcards.
+		if strings.HasPrefix(fn, resolvedPrefix) {
+			fn = origPrefix + strings.TrimPrefix(fn, resolvedPrefix)
+		}
+
 		for len(parentDirHeaders) != 0 {
 			lastParentDir := parentDirHeaders[len(parentDirHeaders)-1]
 			if strings.HasPrefix(fn, lastParentDir.Path+"/") {
@@ -570,8 +640,7 @@ func (cc *cacheContext) includedPaths(ctx context.Context, m *mount, p string, o
 			}
 		} else {
 			if !strings.HasPrefix(fn+"/", p+"/") {
-				k, _, kOk = iter.Next()
-				continue
+				break
 			}
 
 			shouldInclude, err = shouldIncludePath(strings.TrimSuffix(strings.TrimPrefix(fn+"/", p+"/"), "/"), includePatternMatcher, excludePatternMatcher)
@@ -648,6 +717,38 @@ func shouldIncludePath(
 	return true, nil
 }
 
+func splitWildcards(p string) (d1, d2 string) {
+	parts := strings.Split(path.Join(p), "/")
+	var p1, p2 []string
+	var found bool
+	for _, p := range parts {
+		if !found && containsWildcards(p) {
+			found = true
+		}
+		if p == "" {
+			p = "/"
+		}
+		if !found {
+			p1 = append(p1, p)
+		} else {
+			p2 = append(p2, p)
+		}
+	}
+	return filepath.Join(p1...), filepath.Join(p2...)
+}
+
+func containsWildcards(name string) bool {
+	for i := 0; i < len(name); i++ {
+		ch := name[i]
+		if ch == '\\' {
+			i++
+		} else if ch == '*' || ch == '?' || ch == '[' {
+			return true
+		}
+	}
+	return false
+}
+
 func (cc *cacheContext) checksumNoFollow(ctx context.Context, m *mount, p string) (*CacheRecord, error) {
 	p = keyPath(p)
 

diff --git a/cache/contenthash/checksum_test.go b/cache/contenthash/checksum_test.go
@@ -639,8 +639,6 @@ func TestChecksumIncludeDoubleStar(t *testing.T) {
 }
 
 func TestChecksumIncludeSymlink(t *testing.T) {
-	t.Skip("Test will fail until https://github.com/moby/buildkit/issues/2300 is fixed")
-
 	t.Parallel()
 	tmpdir, err := ioutil.TempDir("", "buildkit-state")
 	require.NoError(t, err)
@@ -654,30 +652,53 @@ func TestChecksumIncludeSymlink(t *testing.T) {
 	ch := []string{
 		"ADD data dir",
 		"ADD data/d1 dir",
+		"ADD data/d1/d2 dir",
 		"ADD mnt dir",
 		"ADD mnt/data symlink ../data",
-		"ADD data/d1/foo file abc",
+		"ADD data/d1/d2/foo file abc",
 	}
 
 	ref := createRef(t, cm, ch)
 
 	cc, err := newCacheContext(ref.Metadata(), nil)
 	require.NoError(t, err)
 
-	dgst, err := cc.Checksum(context.TODO(), ref, "data/d1", ChecksumOpts{IncludePatterns: []string{"**/foo"}}, nil)
+	dgstD1, err := cc.Checksum(context.TODO(), ref, "data/d1", ChecksumOpts{IncludePatterns: []string{"**/foo"}}, nil)
+	require.NoError(t, err)
+	// File should be included
+	require.NotEqual(t, digest.FromBytes([]byte{}), dgstD1)
+
+	dgstMntD1, err := cc.Checksum(context.TODO(), ref, "mnt/data/d1", ChecksumOpts{IncludePatterns: []string{"**/foo"}}, nil)
+	require.NoError(t, err)
+	// File should be included despite symlink
+	require.Equal(t, dgstD1, dgstMntD1)
+
+	dgstD2, err := cc.Checksum(context.TODO(), ref, "data/d1/d2", ChecksumOpts{IncludePatterns: []string{"**/foo"}}, nil)
 	require.NoError(t, err)
 	// File should be included
-	require.NotEqual(t, digest.FromBytes([]byte{}), dgst)
+	require.NotEqual(t, digest.FromBytes([]byte{}), dgstD2)
 
-	dgst, err = cc.Checksum(context.TODO(), ref, "mnt/data/d1", ChecksumOpts{IncludePatterns: []string{"**/foo"}}, nil)
+	dgstMntD2, err := cc.Checksum(context.TODO(), ref, "mnt/data/d1/d2", ChecksumOpts{IncludePatterns: []string{"**/foo"}}, nil)
 	require.NoError(t, err)
 	// File should be included despite symlink
-	require.NotEqual(t, digest.FromBytes([]byte{}), dgst)
+	require.Equal(t, dgstD2, dgstMntD2)
 
 	// Same, with Wildcard = true
-	dgst, err = cc.Checksum(context.TODO(), ref, "mnt/data/d1", ChecksumOpts{IncludePatterns: []string{"**/foo"}, Wildcard: true}, nil)
+	dgstMntD1Wildcard, err := cc.Checksum(context.TODO(), ref, "mnt/data/d1", ChecksumOpts{IncludePatterns: []string{"**/foo"}, Wildcard: true}, nil)
+	require.NoError(t, err)
+	require.Equal(t, dgstD1, dgstMntD1Wildcard)
+
+	dgstMntD1Wildcard2, err := cc.Checksum(context.TODO(), ref, "mnt/data/d*", ChecksumOpts{IncludePatterns: []string{"**/foo"}, Wildcard: true}, nil)
+	require.NoError(t, err)
+	require.Equal(t, dgstD1, dgstMntD1Wildcard2)
+
+	dgstMntD2Wildcard, err := cc.Checksum(context.TODO(), ref, "mnt/data/d1/d2", ChecksumOpts{IncludePatterns: []string{"**/foo"}, Wildcard: true}, nil)
+	require.NoError(t, err)
+	require.Equal(t, dgstD2, dgstMntD2Wildcard)
+
+	dgstMntD2Wildcard2, err := cc.Checksum(context.TODO(), ref, "mnt/data/d1/d*", ChecksumOpts{IncludePatterns: []string{"**/foo"}, Wildcard: true}, nil)
 	require.NoError(t, err)
-	require.NotEqual(t, digest.FromBytes([]byte{}), dgst)
+	require.Equal(t, dgstD2, dgstMntD2Wildcard2)
 }
 
 func TestHandleChange(t *testing.T) {