Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: WithFile and WithDirectory unexpectedly overwrite symlinks #6073

Open
sipsma opened this issue Nov 6, 2023 · 1 comment
Open

Bug: WithFile and WithDirectory unexpectedly overwrite symlinks #6073

sipsma opened this issue Nov 6, 2023 · 1 comment
Assignees

Comments

@sipsma
Copy link
Contributor

sipsma commented Nov 6, 2023

There's a bug in our use of merge-op to implement WithFile/WithDirectory which results in any parts of a parent dir that are a symlink to be replaced with an actual dir if With* is called to put a file/dir under them.

E.g. on ubuntu /bin is a symlink to usr/bin, but if you do 'ubuntu.WithFile("/bin/foo", someFile)' then the bin symlink is replaced with a dir named bin.

This is a situation in which we can't use merge op to optimize.


The approach to fixing that I’m honing in on will keep the opportunistic merge-op optimizations by default and only fall back to llb copy when needed.

The only tricky part is that we want it all to be hidden from users, which means the end result has to be identical for both backend implementations, whether we can optimize to merge op or not.

It almost works today but we need one upstream change to the copy implementation to support a flag that enables it to work in the same situations merge op works, sent out a pr for that: tonistiigi/fsutil#169

Without that, there’s situations where the mergeop implementation will work but copy returns an error, which would leak the details to the user. But once that flag is available, then it can be totally seamless.

At most, we could consider giving our users a flag that results in an error being returned if an existing path is going to be overwritten (in which case our backend would just have to skip merge op optimizations), but that would be an option totally agnostic to merge op vs copy and would thus still avoid leaking too much to users. And we don’t need to add that option immediately, just if a need comes up.

@sipsma sipsma self-assigned this Dec 4, 2023
@sipsma
Copy link
Contributor Author

sipsma commented Dec 4, 2023

Just to update, slowly working through the upstream changes needed to do this:

  1. change in fsutil is merged: Add support for always overwriting existing paths tonistiigi/fsutil#169
  2. change in buildkit is open, but blocked for obscure reasons by some other PRs atm (details in description) add support for AlwaysReplaceExistingDestPaths in llb copy moby/buildkit#4455

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant