Copying a symlinked wildcard directory leads to incorrect caching

[aaronlehmann]

In this scenario, there is a symlink from /data -> /mnt/data. On a subsequent layer, files are created under /data. A copy operation with a source path under /mnt/data ends up copying these files, but the cache key is set as if no files would be copied, because the contenthash logic only considers files with a /mnt/data prefix. This can lead to a very bad situation where the results of the copy are returned from cache for any similar copy that does not match any files.

This gist contains a repro case which demonstrates the problem: https://gist.github.com/aaronlehmann/77bd093d5f904719e0a329450d7c8453

In this repro case, we first copy /mnt/data/d1 from st1. The copy is done correctly, but the contenthash logic does not realize that any files would be copied, so it returns digest.FromBytes([]byte{}) for the cache key. Then when we run the same copy with a stock alpine image as input, the copy is cached, which is completely wrong because this directory doesn't even exist in the image.

This is the output from running the repro case:

Copying from st1 with a single file at /data/d1/foo:
#1 docker-image://docker.io/library/alpine:latest
#1 resolve docker.io/library/alpine:latest
#1 resolve docker.io/library/alpine:latest 1.0s done
#1 DONE 1.1s

#3 sh -c mkdir -p /mnt/data/d1 && ln -s /mnt/data /data
#3 CACHED

#4 sh -c echo abc > /data/d1/foo
#4 CACHED

#5 copy /mnt/data/d1 /
#5 CACHED

#2 sh -c apk add -U findutils
#2 CACHED

#6 sh -c find . -ls
#6 0.105   3343154      4 drwxr-xr-x   1 root     root         4096 Aug 10 18:28 .
#6 0.106   3343062      4 drwxr-xr-x   2 root     root         4096 Aug 10 18:17 ./d1
#6 0.106   3340803      4 -rw-r--r--   1 root     root            4 Aug 10 18:17 ./d1/foo
#6 DONE 0.1s

#7 exporting to client
#7 copying files 1.53MB 0.1s
#7 copying files 8.09MB 0.5s done
#7 DONE 0.5s
Copying from stock alpine image - copy should not be cached!:
#4 docker-image://docker.io/library/alpine:latest
#4 resolve docker.io/library/alpine:latest
#4 resolve docker.io/library/alpine:latest 0.2s done
#4 DONE 0.2s

#2 copy /mnt/data/d1 /
#2 CACHED

#1 sh -c apk add -U findutils
#1 CACHED

#3 sh -c find . -ls
#3 0.177   3343164      4 drwxr-xr-x   1 root     root         4096 Aug 10 18:28 .
#3 0.178   3343062      4 drwxr-xr-x   2 root     root         4096 Aug 10 18:17 ./d1
#3 0.178   3340803      4 -rw-r--r--   1 root     root            4 Aug 10 18:17 ./d1/foo
#3 DONE 0.2s

#5 exporting to client
#5 copying files
#5 copying files 8.09MB 0.4s done
#5 DONE 0.4s

cc @coryb @sipsma

[tonistiigi]

I don't understand why the symlink matters if /mnt/data is real dir and this is where you are copying from. Creating a file though symlink in previous layer shouldn't matter at all as no metadata is saved and file actually gets stored in /mnt/data. Is this specific to IncludePatterns?

[aaronlehmann]

I don't understand why the symlink matters if /mnt/data is real dir and this is where you are copying from. Creating a file though symlink in previous layer shouldn't matter at all as no metadata is saved and file actually gets stored in /mnt/data.

In the overlayfs, the file gets created in /data/d1 rather than /mnt/data/d1. When the fsutil code walks /mnt/data/d1 to perform the actual copy, it does the right thing (presumably overlayfs hides the fact that the file was created in /data). But I think the in-memory prefix tree in cache/contenthash ends up tracking this file under /data/d1 and doesn't realize it should actually match the /mnt/data/d1/ prefix.

Is this specific to IncludePatterns?

I think it's specific to the code path where we have IncludePatterns, ExcludePatterns, or Wildcard:

buildkit/cache/contenthash/checksum.go

Lines 400 to 418 in 6034f46

	includedPaths, err := cc.includedPaths(ctx, m, p, opts)
	if err != nil {
	return "", err
	}
	if opts.FollowLinks {
	for i, w := range includedPaths {
	if w.Record.Type == CacheRecordTypeSymlink {
	dgst, err := cc.checksumFollow(ctx, m, w.Path, opts.FollowLinks)
	if err != nil {
	return "", err
	}
	includedPaths[i].Record = &CacheRecord{Digest: dgst}
	}
	}
	}
	if len(includedPaths) == 0 {
	return digest.FromBytes([]byte{}), nil
	}

The really bad part here is that since the code doesn't think any files match, it hits:

	if len(includedPaths) == 0 {
		return digest.FromBytes([]byte{}), nil
	}

and ends up caching the result of this copy as if it was an empty result.

I haven't found a way to reproduce it without IncludePatterns, so it may be IncludePatterns-specific.

[aaronlehmann]

So it turns out that the issue with symlinks was actually a bit more complicated. The original gist was exposing a separate, similar bug with IncludePatterns. I will open a PR to fix that one.

For the actual symlink issue, here's an updated repro case which doesn't use IncludePatterns: https://gist.github.com/aaronlehmann/64054c9a2cff0d27e200cc107bba3d69

The scenario is a little weird so I'll let the code speak for itself. One layer has a symlink /mnt/data -> /data, and another layer has the opposite symlink /data -> /mnt/data. Once again, the copy and caching have different logic, and the copy actually copies files, but Checksum thinks no files would be copied.

[tonistiigi]

In the overlayfs, the file gets created in /data/d1 rather than /mnt/data/d1.

That does not seem right. What's the difference?

/tmp # mkdir -p lower upper work merged
/tmp # mkdir -p lower/mnt/data/d1
/tmp # ln -s mnt/data lower/data
/tmp # mount -t overlay overlay -o lowerdir=lower,upperdir=upper,workdir=work merged
/tmp # echo abc > merged/data/d1/foo
/tmp # find upper
upper
upper/mnt
upper/mnt/data
upper/mnt/data/d1
upper/mnt/data/d1/foo

[aaronlehmann]

Please refer to the updated scenario in https://gist.github.com/aaronlehmann/64054c9a2cff0d27e200cc107bba3d69:

(root) /tmp # mkdir -p lower upper work merged
(root) /tmp # mkdir -p lower/mnt/data/d1 &&  ln -s mnt/data lower/data
(root) /tmp # mkdir -p upper/data upper/mnt && ln -s ../data upper/mnt/data
(root) /tmp # mount -t overlay overlay -o lowerdir=lower,upperdir=upper,workdir=work merged
(root) /tmp # mkdir merged/data/d1
(root) /tmp # echo abc > merged/data/d1/foo
(root) /tmp # find upper
upper
upper/data
upper/data/d1
upper/data/d1/foo
upper/mnt
upper/mnt/data
(root) /tmp # ls upper/mnt/data/d1
foo

[tonistiigi]

Ok, in the latest gist you are copying through symlink so I guess something can get wrong in scanning there. But I don't think it is any weird overlay logic. In your overlay commands, you create a dir manually in upper that overrides the symlink in lower. Afaik this can't happen when just creating files through the overlay mountpoint.

[aaronlehmann]

Yes, I think something does go wrong. The in-memory radix tree does not have files matching the prefix we are copying from, so it thinks nothing will be copied.

I don't think there's any weird overlay logic. It's just an inconsistency between the actual copy and contenthash.

[tonistiigi]

I guess for the symlink and wildcard to work together, only way is for matcher to match per path component, and then follow symlink per component if needed. Does the same thing work for diffcopy?

[aaronlehmann]

I guess for the symlink and wildcard to work together, only way is for matcher to match per path component, and then follow symlink per component if needed.

I think this makes sense. There might be some extra complexity from the FollowLinks option. I think if FollowLinks is false, we need to follow links that might lead to something ending up inside the source path (i.e. data -> mnt/data in this case), but not follow links inside the actual source path. I'm not sure exactly how this would look.

Does the same thing work for diffcopy?

I'm not sure - I'm not familiar with the diffcopy code.

[tonistiigi]

I think if FollowLinks is false, we need to follow links that might lead to something ending up inside the source path

Yes,

buildkit/cache/contenthash/checksum.go

Line 905 in 6034f46

func getFollowLinks(root *iradix.Node, k []byte, follow bool) ([]byte, *CacheRecord, error) {

gets called even on !FollowLinks, same for scanning I believe. But wildcards are trickier as you can't easily split the pattern.

[aaronlehmann]

Started looking at this, and I think the following should be done to make it consistent with the actual copy implementation:

• Non-wildcard case: Split the base path into components, and resolve the path one component at a time, following symlinks along the way (this will match the behavior of the fsutil copier calling os.Lstat and ioutil.ReadDir on a dir that has a symlink somewhere inside the path prefix). Keep track of the prefix tree key (which has path prefix symlinks resolved) separately from the path used as input to the matcher (path prefix symlinks not resolved).
• Wildcard case: Split the wildcard pattern into a prefix without wildcards, and the rest of the pattern. Then apply the same logic as above on the non-wildcard part of the prefix. See https://github.com/tonistiigi/fsutil/blob/d72af97c0eaf93c1d20360e3cb9c63c223675b83/copy/copy.go#L41 for the equivalent fsutil code.

The complexity here is that several places where we call convertPathToKey in order to do a prefix tree lookup need to be changed to do the lookup one component at a time, so this will touch a lot of code. Right now we have a fast path that uses checksumFollow for no wildcards and no include/exclude patterns, and a slow path using includedPaths. They each have separate logic for walking the tree, and both need to be fixed.

I might work on this but I'm not sure if or when I'll have time.

I am wondering if a narrower fix might make sense, at least as a short-term work around. It seems like it's been very complex to get the caching logic to exactly match what the copier does, and small differences can be catastrophic (for example, returning the wrong cached values for very broad sets of copy ops). Is there a way we could fall back to a slow path without caching for cases where the caching logic isn't known to match the copier 100%? Maybe we could detect a digest(null) cache key being returned for a copy operation that actually copied things, and avoid caching that data.

[tonistiigi]

Non-wildcard case:

What's the reproducer for the non-wildcard case?

[aaronlehmann]

What's the reproducer for the non-wildcard case?

Anything using IncludePatterns or ExcludePatterns would reproduce it, even with opts.AllowWildcard = false, because includedPaths does not call getFollowLinks. But I suppose your point is that getFollowLinks should take care of this for the fast path (non-wildcard, no include or exclude patterns), and the slow path without wildcard should be relatively easily fixable since it could also call getFollowLinks to resolve the base path in the tree. Then the only remaining hurdle is the wildcard case in includedPaths, and I think splitting out the non-wildcard prefix from the pattern and using getFollowLinks on it would address that. So maybe this isn't a big change, after all.