Follow links in includedPaths to resolve incorrect caching when source path is behind symlink #2318
Conversation
…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]>
|
Any comments on this PR? |
cache/contenthash/checksum.go
Outdated
| // components before the last component, so | ||
| // handle last component in d1 specially. | ||
| for { | ||
| v, ok := root.Get(k) |
There was a problem hiding this comment.
Not sure I get this. Isn't this supposed to walk per component? see getFollowLinksWalk()
There was a problem hiding this comment.
There is a call to getFollowLinks below (line 568) which does the walk by component. This loop is just to handle the case where the final component is a symlink, which getFollowLinks does not handle.
I've added a test case for this specific scenario.
…ldcard Signed-off-by: Aaron Lehmann <[email protected]>
…omponent) in wildcard prefix Signed-off-by: Aaron Lehmann <[email protected]>
cache/contenthash/checksum.go
Outdated
| return "", nil, false, nil | ||
| } | ||
|
|
||
| k, cr, err := getFollowLinks(root, convertPathToKey([]byte(d1)), true) |
There was a problem hiding this comment.
this could use getFollowLinksWalk so you can reuse linkWalked without double counting.
There was a problem hiding this comment.
Updated to use getFollowLinksWalk.
| @@ -570,8 +607,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 | |||
There was a problem hiding this comment.
At the beginning, we seek to the first path with this prefix (line 541). If we reach a path that don't match the prefix, we can stop iterating, because we're now outside the directory of interest. The old code would keep iterating, but there was no reason to - I think this was left over from before we had that initial seek. So breaking here is an optimization.
Signed-off-by: Aaron Lehmann <[email protected]>
|
Thanks for the thorough review! |
As discussed in #2300,
includedPathsdoes not resolve symlinks whenlooking 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
getFollowLinksto 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 skipped test, and also the repro case here:
https://gist.github.com/aaronlehmann/64054c9a2cff0d27e200cc107bba3d69
Fixes #2300