From 56c0981f1ecabc129e78a970a06e487b3a9b7a28 Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Wed, 18 Aug 2021 10:26:08 -0700 Subject: [PATCH] Follow links in includedPaths to resolve incorrect caching when source path is behind symlink As discussed in #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 #2300 Signed-off-by: Aaron Lehmann --- cache/contenthash/checksum.go | 119 ++++++++++++++++++++++++++--- cache/contenthash/checksum_test.go | 39 +++++++--- 2 files changed, 140 insertions(+), 18 deletions(-) diff --git a/cache/contenthash/checksum.go b/cache/contenthash/checksum.go index dbc2d9187cb0..78d0bcec7e9c 100644 --- 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 index c83b3c02c61a..13046799a201 100644 --- 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,9 +652,10 @@ 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) @@ -664,20 +663,42 @@ func TestChecksumIncludeSymlink(t *testing.T) { 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) {