Skip to content

Commit

Permalink
diffapply: make overwritten dirs opaque.
Browse files Browse the repository at this point in the history
A previous commit fixed an issue where directories that overwrite
whiteout devices needed to be set to be opaque. While that was needed,
it wasn't enough because directories also need to be set opaque if they
are overwriting anything that isn't a directory, such as a file or a
symlink. This commit updates the code to handle that situation in
general.

Signed-off-by: Erik Sipsma <[email protected]>
  • Loading branch information
sipsma committed Mar 8, 2022
1 parent fdecd0a commit 53722cc
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 12 deletions.
15 changes: 14 additions & 1 deletion client/mergediff_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1077,7 +1077,7 @@ func diffOpTestCases() (tests []integration.Test) {
}
return []integration.Test{
verifyContents{
// Verifies that when a directory with contents is used a a base layer
// Verifies that when a directory with contents is used as a base layer
// in a merge, subsequent merges that first delete the dir (resulting in
// a whiteout device w/ overlay snapshotters) and then recreate the dir
// correctly set it as opaque.
Expand All @@ -1092,6 +1092,19 @@ func diffOpTestCases() (tests []integration.Test) {
fstest.CreateFile("/dir/b", nil, 0644),
),
},
verifyContents{
// Same as above, but with a file overwrite instead of an rm
name: "TestDiffMergeOpaqueRegressionWithFileOverwrite",
state: llb.Merge([]llb.State{
base().File(llb.Mkfile("/dir/a", 0644, nil)),
llb.Scratch().File(llb.Mkfile("/dir", 0644, nil)),
base().File(llb.Mkfile("/dir/b", 0644, nil)),
}),
contents: apply(
fstest.CreateDir("/dir", 0755),
fstest.CreateFile("/dir/b", nil, 0644),
),
},
}
}()...)

Expand Down
18 changes: 7 additions & 11 deletions snapshot/diffapply_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -266,17 +266,18 @@ func (a *applier) applyDelete(ctx context.Context, ca *changeApply) (bool, error
return false, nil
}

if overwrite && a.createWhiteoutDelete && isWhiteoutDevice(ca.dstStat) && ca.srcStat.Mode&unix.S_IFMT == unix.S_IFDIR {
// If we are overwriting a whiteout device with a directory, we need this new dir to be opaque
// so that any files from lowerdirs under it are not visible.
ca.setOpaque = true
}

if err := os.RemoveAll(ca.dstPath); err != nil {
return false, errors.Wrap(err, "failed to remove during apply")
}
ca.dstStat = nil

if overwrite && a.createWhiteoutDelete && ca.srcStat.Mode&unix.S_IFMT == unix.S_IFDIR {
// If we are using an overlay snapshotter and overwriting an existing non-directory
// with a directory, we need this new dir to be opaque so that any files from lowerdirs
// under it are not visible.
ca.setOpaque = true
}

if deleteOnly && a.createWhiteoutDelete {
// only create a whiteout device if there is something to delete
var foundLower bool
Expand Down Expand Up @@ -849,8 +850,3 @@ func needsUserXAttr(ctx context.Context, sn Snapshotter, lm leases.Manager) (boo
}
return userxattr, nil
}

func isWhiteoutDevice(st *syscall.Stat_t) bool {
// it's a whiteout if it's a char device and has a major/minor of 0/0
return st != nil && st.Mode&unix.S_IFMT == unix.S_IFCHR && st.Rdev == unix.Mkdev(0, 0)
}

0 comments on commit 53722cc

Please sign in to comment.