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

diffapply: do chown before xattrs #3671

Merged
merged 2 commits into from
Feb 24, 2023
Merged

Conversation

sipsma
Copy link
Collaborator

@sipsma sipsma commented Feb 24, 2023

Chown will result file capabilities getting reset, so it should be done
before setting xattrs to ensure they are retained.

Signed-off-by: Erik Sipsma erik@sipsma.dev

The included test failed before only on the snapshotter w/ disable hardlinking. Passes after the new fix.

Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Chown will result file capabilities getting reset, so it should be done
before setting xattrs to ensure they are retained.

Signed-off-by: Erik Sipsma <erik@sipsma.dev>
@sipsma sipsma merged commit db6342f into moby:master Feb 24, 2023
@thaJeztah
Copy link
Member

Does this fix the issue reported in moby 23.0? or are changes needed on the moby side for that? (does this one need cherry-pick?)

@sipsma
Copy link
Collaborator Author

sipsma commented Feb 28, 2023

@thaJeztah The moby issue was a combination of two orthogonal problems:

  1. Merged snapshots on disk didn't preserve file capabilities (this was true in both vanilla buildkit and moby-flavored buildkit)
  2. Images exported from merged snapshots were incorrectly flattened into a single layer derived directly from the merged snapshot (this was only a problem in moby, not vanilla buildkit)

The PR here fixes 1 but not 2. 2 needs to be fixed in moby.

I think 1 on it's own is pretty obscure since it only affected local snapshots, not exported container images (it's only in combination w/ 2 that it's much worse). So I'll defer to @tonistiigi on whether we should backport this or not. It shouldn't be hard if desired.

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

Successfully merging this pull request may close these issues.

None yet

4 participants