Merge pull request #6306 from openshift-cherrypick-robot/cherry-pick-6296-to-release-1.41

[release-1.41] Exclude pulled up parent directories at commit-time

Author: openshift-merge-bot[bot]

copier/copier.go

@@ -305,7 +305,8 @@ type removeResponse struct{}
 
 // ensureResponse encodes a response to an Ensure request.
 type ensureResponse struct {
-	Created []string // paths that were created because they weren't already present
+	Created []string           // paths that were created because they weren't already present
+	Noted   []EnsureParentPath // preexisting paths that are parents of created items
 }
 
 // conditionalRemoveResponse encodes a response to a conditionalRemove request.
@@ -479,6 +480,7 @@ func Put(root string, directory string, options PutOptions, bulkReader io.Reader
 // MkdirOptions controls parts of Mkdir()'s behavior.
 type MkdirOptions struct {
 	UIDMap, GIDMap []idtools.IDMap // map from containerIDs to hostIDs when creating directories
+	ModTimeNew     *time.Time      // set mtime and atime of newly-created directories
 	ChownNew       *idtools.IDPair // set ownership of newly-created directories
 	ChmodNew       *os.FileMode    // set permissions on newly-created directories
 }
@@ -2199,6 +2201,7 @@ func copierHandlerMkdir(req request, idMappings *idtools.IDMappings) (*response,
 	}
 
 	subdir := ""
+	var created []string
 	for _, component := range strings.Split(rel, string(os.PathSeparator)) {
 		subdir = filepath.Join(subdir, component)
 		path := filepath.Join(req.Root, subdir)
@@ -2209,6 +2212,7 @@ func copierHandlerMkdir(req request, idMappings *idtools.IDMappings) (*response,
 			if err = chmod(path, dirMode); err != nil {
 				return errorResponse("copier: mkdir: error setting permissions on %q to 0%o: %v", path, dirMode)
 			}
+			created = append(created, path)
 		} else {
 			// FreeBSD can return EISDIR for "mkdir /":
 			// https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=59739.
@@ -2217,6 +2221,17 @@ func copierHandlerMkdir(req request, idMappings *idtools.IDMappings) (*response,
 			}
 		}
 	}
+	// set timestamps last, in case we needed to create some nested directories, which would
+	// update the timestamps on directories that we'd just set timestamps on, if we had done
+	// that immediately
+	if req.MkdirOptions.ModTimeNew != nil {
+		when := *req.MkdirOptions.ModTimeNew
+		for _, newDirectory := range created {
+			if err = lutimes(false, newDirectory, when, when); err != nil {
+				return errorResponse("copier: mkdir: error setting datestamp on %q: %v", newDirectory, err)
+			}
+		}
+	}
 
 	return &response{Error: "", Mkdir: mkdirResponse{}}, nil, nil
 }
@@ -2255,12 +2270,22 @@ type EnsureOptions struct {
 	Paths          []EnsurePath
 }
 
+// EnsureParentPath is a parent (or grandparent, or...) directory of an item
+// created by Ensure(), along with information about it, from before the item
+// in question was created.  If the information about this directory hasn't
+// changed when commit-time rolls around, it's most likely that this directory
+// is only being considered for inclusion in the layer because it was pulled
+// up, and it was not actually changed.
+type EnsureParentPath = ConditionalRemovePath
+
 // Ensure ensures that the specified mount point targets exist under the root.
 // If the root directory is not specified, the current root directory is used.
 // If root is specified and the current OS supports it, and the calling process
 // has the necessary privileges, the operation is performed in a chrooted
 // context.
-func Ensure(root, directory string, options EnsureOptions) ([]string, error) {
+// Returns a slice with the pathnames of items that needed to be created and a
+// slice of affected parent directories and information about them.
+func Ensure(root, directory string, options EnsureOptions) ([]string, []EnsureParentPath, error) {
 	req := request{
 		Request:       requestEnsure,
 		Root:          root,
@@ -2269,12 +2294,12 @@ func Ensure(root, directory string, options EnsureOptions) ([]string, error) {
 	}
 	resp, err := copier(nil, nil, req)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	if resp.Error != "" {
-		return nil, errors.New(resp.Error)
+		return nil, nil, errors.New(resp.Error)
 	}
-	return resp.Ensure.Created, nil
+	return resp.Ensure.Created, resp.Ensure.Noted, nil
 }
 
 func copierHandlerEnsure(req request, idMappings *idtools.IDMappings) *response {
@@ -2283,6 +2308,7 @@ func copierHandlerEnsure(req request, idMappings *idtools.IDMappings) *response
 	}
 	slices.SortFunc(req.EnsureOptions.Paths, func(a, b EnsurePath) int { return strings.Compare(a.Path, b.Path) })
 	var created []string
+	notedByName := map[string]EnsureParentPath{}
 	for _, item := range req.EnsureOptions.Paths {
 		uid, gid := 0, 0
 		if item.Chown != nil {
@@ -2326,11 +2352,25 @@ func copierHandlerEnsure(req request, idMappings *idtools.IDMappings) *response
 			if parentPath == "" {
 				parentPath = "."
 			}
-			leaf := filepath.Join(subdir, component)
+			leaf := filepath.Join(parentPath, component)
 			parentInfo, err := os.Stat(filepath.Join(req.Root, parentPath))
 			if err != nil {
 				return errorResponse("copier: ensure: checking datestamps on %q (%d: %v): %v", parentPath, i, components, err)
 			}
+			if parentPath != "." {
+				parentModTime := parentInfo.ModTime().UTC()
+				parentMode := parentInfo.Mode()
+				uid, gid, err := owner(parentInfo)
+				if err != nil {
+					return errorResponse("copier: ensure: error reading owner of %q: %v", parentPath, err)
+				}
+				notedByName[parentPath] = EnsureParentPath{
+					Path:    parentPath,
+					ModTime: &parentModTime,
+					Mode:    &parentMode,
+					Owner:   &idtools.IDPair{UID: uid, GID: gid},
+				}
+			}
 			if i < len(components)-1 || item.Typeflag == tar.TypeDir {
 				err = os.Mkdir(filepath.Join(req.Root, leaf), mode)
 				subdir = leaf
@@ -2372,7 +2412,15 @@ func copierHandlerEnsure(req request, idMappings *idtools.IDMappings) *response
 		}
 	}
 	slices.Sort(created)
-	return &response{Error: "", Ensure: ensureResponse{Created: created}}
+	noted := make([]EnsureParentPath, 0, len(notedByName))
+	for _, n := range notedByName {
+		if slices.Contains(created, n.Path) {
+			continue
+		}
+		noted = append(noted, n)
+	}
+	slices.SortFunc(noted, func(a, b EnsureParentPath) int { return strings.Compare(a.Path, b.Path) })
+	return &response{Error: "", Ensure: ensureResponse{Created: created, Noted: noted}}
 }
 
 // ConditionalRemovePath is a single item being passed to an ConditionalRemove() call.

copier/copier_test.go

@@ -2045,12 +2045,15 @@ func TestExtendedGlob(t *testing.T) {
 func testEnsure(t *testing.T) {
 	zero := time.Unix(0, 0)
 	worldReadable := os.FileMode(0o644)
+	ugReadable := os.FileMode(0o750)
 
 	testCases := []struct {
-		description string
-		subdir      string
-		options     EnsureOptions
-		expected    []string
+		description   string
+		subdir        string
+		mkdirs        []string
+		options       EnsureOptions
+		expectCreated []string
+		expectNoted   []EnsureParentPath
 	}{
 		{
 			description: "base",
@@ -2078,7 +2081,7 @@ func testEnsure(t *testing.T) {
 					},
 				},
 			},
-			expected: []string{
+			expectCreated: []string{
 				"subdir",
 				"subdir/a",
 				"subdir/a/b",
@@ -2087,6 +2090,7 @@ func testEnsure(t *testing.T) {
 				"subdir/a/b/c",
 				"subdir/a/b/d",
 			},
+			expectNoted: []EnsureParentPath{},
 		},
 		{
 			description: "nosubdir",
@@ -2103,21 +2107,93 @@ func testEnsure(t *testing.T) {
 					},
 				},
 			},
-			expected: []string{
+			expectCreated: []string{
 				"a",
 				"a/b",
 				"a/b/c",
 				"a/b/d",
 			},
+			expectNoted: []EnsureParentPath{},
+		},
+		{
+			description: "mkdir-first",
+			subdir:      "dir/subdir",
+			mkdirs:      []string{"dir", "dir/subdir"},
+			options: EnsureOptions{
+				Paths: []EnsurePath{
+					{
+						Path:     filepath.Join(string(os.PathSeparator), "a", "b", "a"),
+						Typeflag: tar.TypeReg,
+						Chmod:    &worldReadable,
+					},
+					{
+						Path:     filepath.Join("a", "b", "b"),
+						Typeflag: tar.TypeReg,
+						ModTime:  &zero,
+					},
+					{
+						Path:     filepath.Join(string(os.PathSeparator), "a", "b", "c"),
+						Typeflag: tar.TypeDir,
+						ModTime:  &zero,
+					},
+					{
+						Path:     filepath.Join("a", "b", "d"),
+						Typeflag: tar.TypeDir,
+					},
+				},
+			},
+			expectCreated: []string{
+				"dir/subdir/a",
+				"dir/subdir/a/b",
+				"dir/subdir/a/b/a",
+				"dir/subdir/a/b/b",
+				"dir/subdir/a/b/c",
+				"dir/subdir/a/b/d",
+			},
+			expectNoted: []EnsureParentPath{
+				{
+					Path:  "dir",
+					Mode:  &ugReadable,
+					Owner: &idtools.IDPair{UID: 1, GID: 1},
+					// ModTime gets updated when we create dir/subdir, can't check it
+				},
+				{
+					Path:    "dir/subdir",
+					Mode:    &ugReadable,
+					Owner:   &idtools.IDPair{UID: 1, GID: 1},
+					ModTime: &zero,
+				},
+			},
 		},
 	}
 	for i := range testCases {
 		t.Run(testCases[i].description, func(t *testing.T) {
 			testStarted := time.Now()
 			tmpdir := t.TempDir()
-			created, err := Ensure(tmpdir, testCases[i].subdir, testCases[i].options)
+			for _, mkdir := range testCases[i].mkdirs {
+				err := Mkdir(tmpdir, mkdir, MkdirOptions{
+					ModTimeNew: &zero,
+					ChmodNew:   &ugReadable,
+					ChownNew:   &idtools.IDPair{UID: 1, GID: 1},
+				})
+				require.NoError(t, err, "unexpected error ensuring")
+			}
+			created, noted, err := Ensure(tmpdir, testCases[i].subdir, testCases[i].options)
 			require.NoError(t, err, "unexpected error ensuring")
-			require.EqualValues(t, testCases[i].expected, created, "did not expect these")
+			require.EqualValues(t, testCases[i].expectCreated, created, "did not expect to create these")
+			require.Equal(t, len(testCases[i].expectNoted), len(noted), "noticed the wrong number of things")
+			for n := range noted {
+				require.Equalf(t, testCases[i].expectNoted[n].Path, noted[n].Path, "noticed item %d path", n)
+				if testCases[i].expectNoted[n].Mode != nil {
+					require.Equalf(t, testCases[i].expectNoted[n].Mode.Perm(), noted[n].Mode.Perm(), "noticed item %q mode", noted[n].Path)
+				}
+				if testCases[i].expectNoted[n].Owner != nil {
+					require.Equalf(t, *testCases[i].expectNoted[n].Owner, *noted[n].Owner, "noticed item %q owner", noted[n].Path)
+				}
+				if testCases[i].expectNoted[n].ModTime != nil {
+					require.Equalf(t, testCases[i].expectNoted[n].ModTime.UnixNano(), noted[n].ModTime.UnixNano(), "noticed item %q mtime", noted[n].Path)
+				}
+			}
 			for _, item := range testCases[i].options.Paths {
 				target := filepath.Join(tmpdir, testCases[i].subdir, item.Path)
 				st, err := os.Stat(target)
@@ -2298,7 +2374,7 @@ func testConditionalRemove(t *testing.T) {
 					Chmod:    what.mode,
 				})
 			}
-			created, err := Ensure(tmpdir, testCases[i].subdir, create)
+			created, _, err := Ensure(tmpdir, testCases[i].subdir, create)
 			require.NoErrorf(t, err, "unexpected error creating %#v", create)
 			remove := testCases[i].remove
 			for _, what := range created {

image.go

@@ -1462,6 +1462,7 @@ func (b *Builder) makeContainerImageRef(options CommitOptions) (*containerImageR
 	if options.CompatLayerOmissions == types.OptionalBoolTrue {
 		layerExclusions = append(layerExclusions, compatLayerExclusions...)
 	}
+	logrus.Debugf("excluding these items from committed layer: %#v", layerExclusions)
 
 	manifestType := options.PreferredManifestType
 	if manifestType == "" {

run_common.go

@@ -2119,11 +2119,12 @@ func (b *Builder) createMountTargets(spec *specs.Spec) ([]copier.ConditionalRemo
 	if len(targets.Paths) == 0 {
 		return nil, nil
 	}
-	created, err := copier.Ensure(rootfsPath, rootfsPath, targets)
+	created, noted, err := copier.Ensure(rootfsPath, rootfsPath, targets)
 	if err != nil {
 		return nil, err
 	}
 	logrus.Debugf("created mount targets at %v", created)
+	logrus.Debugf("parents of mount targets at %+v", noted)
 	var remove []copier.ConditionalRemovePath
 	for _, target := range created {
 		cleanedTarget := strings.Trim(path.Clean(filepath.ToSlash(target)), "/")
@@ -2154,7 +2155,7 @@ func (b *Builder) createMountTargets(spec *specs.Spec) ([]copier.ConditionalRemo
 	if err := os.Mkdir(filepath.Join(cdir, containerExcludesDir), 0o700); err != nil && !errors.Is(err, os.ErrExist) {
 		return nil, fmt.Errorf("creating exclusions directory: %w", err)
 	}
-	encoded, err := json.Marshal(remove)
+	encoded, err := json.Marshal(append(slices.Clone(noted), remove...))
 	if err != nil {
 		return nil, fmt.Errorf("encoding list of items to exclude at commit-time: %w", err)
 	}

tests/bud.bats

@@ -8479,3 +8479,26 @@ _EOF
   run_buildah build --layers -f $BUDFILES/from-scratch/Containerfile2 $BUDFILES/from-scratch/
   assert "$output" !~ "Using cache"
 }
+
+@test "bud --layers should not include pulled up parent directories of mount points" {
+  _prefetch busybox
+  local contextdir=${TEST_SCRATCH_DIR}/context
+  mkdir $contextdir
+  mkdir $contextdir/dev
+  mkdir $contextdir/proc
+  mkdir $contextdir/sys
+
+  cat > $contextdir/Dockerfile << _EOF
+FROM scratch
+COPY / /
+COPY --from=quay.io/libpod/busybox / /
+RUN rm -f /Dockerfile
+RUN --mount=type=bind,ro,src=/Dockerfile,target=/var/spool/mail/tmpfile touch /newfile
+_EOF
+  run_buildah build --layers -t oci:${TEST_SCRATCH_DIR}/oci-image ${contextdir}
+  lastlayer=$(oci_image_last_diff ${TEST_SCRATCH_DIR}/oci-image)
+  run tar tf ${TEST_SCRATCH_DIR}/oci-image/"${lastlayer}"
+  echo "$output"
+  assert "$status" = "0"
+  assert "${#lines[*]}" = "1"
+}