Merge pull request moby#2318 from aaronlehmann/follow-links-includedpaths

Follow links in includedPaths to resolve incorrect caching when source path is behind symlink

Author: tonistiigi

cache/contenthash/checksum.go

@@ -516,21 +516,44 @@ 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()
+		origPrefix, k, kOk, err = wildcardPrefix(root, p)
+		if err != nil {
+			return nil, err
+		}
 	} else {
-		k = convertPathToKey([]byte(p))
-		if _, kOk = root.Get(k); kOk {
+		origPrefix = p
+		k = convertPathToKey([]byte(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
+		}
+		kOk = (cr != nil)
+	}
+
+	if origPrefix != "" {
+		if kOk {
 			iter.SeekLowerBound(append(append([]byte{}, k...), 0))
 		}
+
+		resolvedPrefix = string(convertKeyToPath(k))
+	} else {
+		k, _, kOk = iter.Next()
 	}
 
 	var (
@@ -541,6 +564,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+"/") {
@@ -591,8 +628,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(
@@ -692,6 +728,82 @@ func shouldIncludePath(
 	return true, nil
 }
 
+func wildcardPrefix(root *iradix.Node, p string) (string, []byte, bool, error) {
+	// 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 == "/" {
+		return "", nil, false, nil
+	}
+
+	linksWalked := 0
+	k, cr, err := getFollowLinksWalk(root, convertPathToKey([]byte(d1)), true, &linksWalked)
+	if err != nil {
+		return "", k, false, err
+	}
+
+	if d2 != "" && cr != nil && cr.Type == CacheRecordTypeSymlink {
+		// getFollowLinks only handles symlinks in path
+		// components before the last component, so
+		// handle last component in d1 specially.
+		resolved := string(convertKeyToPath(k))
+		for {
+			v, ok := root.Get(k)
+
+			if !ok {
+				return d1, k, false, nil
+			}
+			if v.(*CacheRecord).Type != CacheRecordTypeSymlink {
+				break
+			}
+
+			linksWalked++
+			if linksWalked > 255 {
+				return "", k, false, errors.Errorf("too many links")
+			}
+
+			resolved := cleanLink(resolved, v.(*CacheRecord).Linkname)
+			k = convertPathToKey([]byte(resolved))
+		}
+	}
+	return d1, k, cr != nil, 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)
 
@@ -973,14 +1085,8 @@ func getFollowLinksWalk(root *iradix.Node, k []byte, follow bool, linksWalked *i
 			if *linksWalked > 255 {
 				return nil, nil, errors.Errorf("too many links")
 			}
-			dirPath := path.Clean(string(convertKeyToPath(dir)))
-			if dirPath == "." || dirPath == "/" {
-				dirPath = ""
-			}
-			link := path.Clean(parent.Linkname)
-			if !path.IsAbs(link) {
-				link = path.Join("/", path.Join(path.Dir(dirPath), link))
-			}
+
+			link := cleanLink(string(convertKeyToPath(dir)), parent.Linkname)
 			return getFollowLinksWalk(root, append(convertPathToKey([]byte(link)), file...), follow, linksWalked)
 		}
 	}
@@ -992,6 +1098,18 @@ func getFollowLinksWalk(root *iradix.Node, k []byte, follow bool, linksWalked *i
 	return k, nil, nil
 }
 
+func cleanLink(dir, linkname string) string {
+	dirPath := path.Clean(dir)
+	if dirPath == "." || dirPath == "/" {
+		dirPath = ""
+	}
+	link := path.Clean(linkname)
+	if !path.IsAbs(link) {
+		return path.Join("/", path.Join(path.Dir(dirPath), link))
+	}
+	return link
+}
+
 func prepareDigest(fp, p string, fi os.FileInfo) (digest.Digest, error) {
 	h, err := NewFileHash(fp, fi)
 	if err != nil {

cache/contenthash/checksum_test.go

@@ -649,8 +649,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)
@@ -663,31 +661,64 @@ func TestChecksumIncludeSymlink(t *testing.T) {
 
 	ch := []string{
 		"ADD data dir",
-		"ADD data/d1 dir",
+		"ADD data/d0 dir",
+		"ADD data/d0/d1 dir",
+		"ADD data/d0/d1/d2 dir",
 		"ADD mnt dir",
 		"ADD mnt/data symlink ../data",
-		"ADD data/d1/foo file abc",
+		"ADD data/d0/d1/d2/foo file abc",
+		"ADD data/symlink-to-d0 symlink d0",
 	}
 
 	ref := createRef(t, cm, ch)
 
 	cc, err := newCacheContext(ref)
 	require.NoError(t, err)
 
-	dgst, err := cc.Checksum(context.TODO(), ref, "data/d1", ChecksumOpts{IncludePatterns: []string{"**/foo"}}, nil)
+	dgstD0, err := cc.Checksum(context.TODO(), ref, "data/d0", 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{}), dgstD0)
 
-	dgst, err = cc.Checksum(context.TODO(), ref, "mnt/data/d1", ChecksumOpts{IncludePatterns: []string{"**/foo"}}, nil)
+	dgstMntD0, err := cc.Checksum(context.TODO(), ref, "mnt/data/d0", 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, dgstD0, dgstMntD0)
+
+	dgstD2, err := cc.Checksum(context.TODO(), ref, "data/d0/d1/d2", ChecksumOpts{IncludePatterns: []string{"**/foo"}}, nil)
+	require.NoError(t, err)
+	// File should be included
+	require.NotEqual(t, digest.FromBytes([]byte{}), dgstD2)
+
+	dgstMntD2, err := cc.Checksum(context.TODO(), ref, "mnt/data/d0/d1/d2", ChecksumOpts{IncludePatterns: []string{"**/foo"}}, nil)
+	require.NoError(t, err)
+	// File should be included despite symlink
+	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)
+	dgstMntD0Wildcard, err := cc.Checksum(context.TODO(), ref, "mnt/data/d0", ChecksumOpts{IncludePatterns: []string{"**/foo"}, Wildcard: true}, nil)
+	require.NoError(t, err)
+	require.Equal(t, dgstD0, dgstMntD0Wildcard)
+
+	dgstMntD0Wildcard2, err := cc.Checksum(context.TODO(), ref, "mnt/data/d*", ChecksumOpts{IncludePatterns: []string{"**/foo"}, Wildcard: true}, nil)
+	require.NoError(t, err)
+	require.Equal(t, dgstD0, dgstMntD0Wildcard2)
+
+	dgstMntD2Wildcard, err := cc.Checksum(context.TODO(), ref, "mnt/data/d0/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/d0/d1/d*", ChecksumOpts{IncludePatterns: []string{"**/foo"}, Wildcard: true}, nil)
+	require.NoError(t, err)
+	require.Equal(t, dgstD2, dgstMntD2Wildcard2)
+
+	dgstMntInnerWildcard, err := cc.Checksum(context.TODO(), ref, "mnt/data/d0/d*/d2", ChecksumOpts{IncludePatterns: []string{"**/foo"}, Wildcard: true}, nil)
+	require.NoError(t, err)
+	require.Equal(t, dgstD2, dgstMntInnerWildcard)
+
+	dgstMntInnerWildcard2, err := cc.Checksum(context.TODO(), ref, "mnt/data/symlink-to-d0/d*/d2", ChecksumOpts{IncludePatterns: []string{"**/foo"}, Wildcard: true}, nil)
 	require.NoError(t, err)
-	require.NotEqual(t, digest.FromBytes([]byte{}), dgst)
+	require.Equal(t, dgstD2, dgstMntInnerWildcard2)
 }
 
 func TestHandleChange(t *testing.T) {