Add --parents option for COPY in Containerfiles #6008

Honny1 · 2025-02-25T15:54:48Z

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake
/kind other

What this PR does / why we need it:

This PR adds the --parents option for COPY in Containerfiles. It also includes an implementation of the --parents option for the buildah copy command.

How to verify it

Try using --parents as described in the docker documentation.

Which issue(s) this PR fixes:

Fixes: https://issues.redhat.com/browse/RUN-2193
Fixes: #5557

Special notes for your reviewer:

Does this PR introduce a user-facing change?

COPY option and buildah copy now support --parents options.

rhatdan · 2025-02-26T15:06:10Z

@nalind PTAL

nalind

Not convinced we need two fields in the AddAndCopyOptions, and I'm not a fan of having fields which can be nil which, when left unset, will trigger a crash.

nalind · 2025-02-26T19:25:14Z

add.go

@@ -94,6 +94,9 @@ type AddAndCopyOptions struct {
 	// RetryDelay is how long to wait before retrying attempts to retrieve
 	// remote contents.
 	RetryDelay time.Duration
+	// Parents preserve parent directories of source content
+	Parents         bool
+	ParentsPatterns map[string]string


When would a caller set ParentsPatterns, and what should they put in it if they do? What happens if they leave the field set to its zero value of nil? They're already passing in a list of sources, why are they required to supply more than that to get the desired effect?

I tried to explain what the content should be in the comment. If ParentsPatterns is a nil value, it will be ignored. The sources list is cleaned up with filepath.Join, which removes /./ from the path so it can't be used. Therefore, I decided to use a map to connect the source path and the path (pattern) with /./.

add.go

cmd/buildah/addcopy.go

copier/copier.go

nalind · 2025-02-26T19:49:46Z

tests/conformance/conformance_test.go

+			*/
+		}, "\n"),
+		contextDir:          "copy-parents",
+		fsSkip:              []string{"(dir):parents:mtime", "(dir):parents:(dir):y:mtime"},


Does docker build preserve the timestamp on y, or its permissions if they're something other than 0o755?

I have made a rework and the current version preserves the timestamp of the copied directory.

I don't think Docker preserves the y timestamp.

nalind · 2025-02-26T19:50:50Z

copier/copier.go

+	ChmodDirs             *os.FileMode      // set permissions on directories. no effect on archives being extracted
+	ChownFiles            *idtools.IDPair   // set ownership of files. no effect on archives being extracted
+	ChmodFiles            *os.FileMode      // set permissions on files. no effect on archives being extracted
+	Parents               bool              // maintain the sources parent directory in the destination


Are the parent directories of the items being read supposed to be emitted in the tarstream when this flag is set?

I have made a rework and the current version copies the parent directories to the tarstream.

nalind

I think this really needs some unit tests in the copier package to ensure that it produces exactly the right set of outputs for a given set of inputs, with no parent directories being implicit or being output multiple times.

cmd/buildah/addcopy.go

nalind · 2025-03-03T22:24:30Z

copier/copier.go

+
+				name := filepath.Base(queue[i])
+				if len(req.GetOptions.ParentsPrefixToRemove) > 0 {
+					name = filepath.Clean(strings.TrimPrefix(item, filepath.Join(req.Directory, req.GetOptions.ParentsPrefixToRemove)))


Between this and https://github.com/containers/buildah/pull/6008/files#diff-cc55d39a0428ec028a6aefb86560d0e304d9618cfcfd5ead0130af486db41f24R1363, which are undoing what https://github.com/containers/buildah/pull/6008/files#diff-f2e4566c6b7e38384283187aba6d7fd91e5ba8da2ffd0f849277bb76bff27fb3R561 does, I'm wondering if we even need the field in AddAndCopyOptions to be a map instead of a Parents boolean.

nalind · 2025-03-03T22:43:34Z

copier/copier.go

+		alreadyCopied[parentPath] = struct{}{}
+		parentName = filepath.Dir(parentName)
+		parentPath = filepath.Dir(parentPath)
+	}


Is the order in which these are being emitted such that directories are being output before their parents?

Oh, Yes, they are. That's probably wrong.

nalind · 2025-03-03T23:04:11Z

cmd/buildah/addcopy.go

+	if iopts.parents {
+		options.ParentsPatterns = map[string]string{}
+		for _, pattern := range args {
+			options.ParentsPatterns[pattern] = pattern


This doesn't appear to be doing anything different when contextdir is set, while https://github.com/containers/buildah/pull/6008/files#diff-f2e4566c6b7e38384283187aba6d7fd91e5ba8da2ffd0f849277bb76bff27fb3R561-R562 does. If it's required, I'd expect it to be required in both places, and it's something that the integration tests should be verifying works correctly.

nalind · 2025-03-03T23:16:35Z

copier/copier.go

+		parentName = filepath.Dir(parentName)
+		parentPath = filepath.Dir(parentPath)
+	}
+	return alreadyCopied, nil


This kind of implies that alreadyCopied wasn't already modified in the caller's scope, but this function was passed the map itself rather than a copy, so returning it here isn't really necessary.

nalind · 2025-03-03T23:18:02Z

copier/copier.go

@@ -1400,6 +1428,31 @@ func copierHandlerGet(bulkWriter io.Writer, req request, pm *fileutils.PatternMa
 	return &response{Stat: statResponse.Stat, Get: getResponse{}}, cb, nil
 }

+func copyParentsDirs(name string, path string, alreadyCopied map[string]struct{}, copy func(string, string, os.FileInfo) error) (map[string]struct{}, error) {


Does this avoid outputting a directory that has already been output while walking the tree?

Honny1 · 2025-03-05T16:20:25Z

@nalind I removed the use of ParentsPrefix/Patterns and only use bool flags Parents. And I added copier tests.

nalind

Mostly nits, except that Get() really shouldn't be outputting a given item more than once.

imagebuildah/stage_executor.go

add.go

copier/copier.go

copier/copier_test.go

Honny1 · 2025-03-14T17:14:24Z

/packit rebuild-failed

nalind · 2025-03-17T18:26:43Z

One thing I often forget when manipulating the Name field of headers in an archive is that we also have to pay attention to the Linkname for hard link entries, and possibly also symbolic links. We should have a test case where at least two of the items being copied are hard linked together, and expect them to remain hard linked in the finished image.

When an entry with Typeflag=TypeSymlink is extracted, the value is used directly, so symbolic links in archives can point anywhere on the filesystem, regardless of whether they're relative or absolute. Dangling links are okay. We probably don't need to do anything special for them.

When an entry with Typeflag=TypeLink is extracted, a hard link is made to the path computed using the directory the archive is being extracted to and the Linkname value in the entry (this is the "oldname" parameter for os.Link()) -- the path in the Name value is only used in constructing the newname parameter for os.Link(). Hard links to contents outside of the directory where the archive is being extracted, or to items not included in the archive, are not intended to be possible.

I think that's the last part of this we need to be sure we've considered and have tests for.

Honny1 · 2025-03-18T16:54:18Z

@nalind I added tests for hardlinks and symlinks.

nalind

We have special logic for handling pivot points ("/./" in source paths), and for handling hard links, so both the conformance test and integration test should also include cases where they both come into play.

The tests don't automatically enforce that items that are hard linked to each other remain hard links when they're copied together, so the tests will need to check that directly by comparing the results of running something like stat -c %i against the items that are expected to be hard links. We already have tests that ensure links show up in a committed image if they're in the working container's rootfs, so if it's easier, it would be sufficient to check on that during a RUN instruction right after the COPY instruction.

It also includes an implementation of the --parents flag for the buildah copy command. Fixes: https://issues.redhat.com/browse/RUN-2193 Fixes: containers#5557 Signed-off-by: Jan Rodák <[email protected]>

Honny1 · 2025-03-18T21:05:24Z

@nalind Oh, I forgot about that. Thank you. I found the mistake. It should be fine now.

Honny1 · 2025-03-19T14:58:04Z

/packit rebuild-failed

nalind · 2025-03-19T18:46:06Z

LGTM

rhatdan · 2025-03-20T10:25:13Z

copier/copier.go

+				if err := copierHandlerGetOne(parentInfo, "", parentName, parent, req.GetOptions, tw, hardlinkChecker, idMappings); err != nil {
+					if req.GetOptions.IgnoreUnreadable && errorIsPermission(err) {
+						continue
+					} else if errors.Is(err, os.ErrNotExist) {


Nit: No need for else here.

rhatdan · 2025-03-20T10:26:23Z

copier/copier.go

 				if err != nil {
-					continue
+					return fmt.Errorf("copier: get: lstat %q: %w", parent, err)


Nit: this is kind of a stutter. Since parent path will be mentioned twice in error message.

rhatdan · 2025-03-20T10:27:08Z

copier/copier.go

+						logrus.Warningf("copier: file disappeared while reading: %q", parent)
+						return nil
+					}
+					return fmt.Errorf("copier: get: %q: %w", queue[i].glob, err)


rhatdan · 2025-03-20T10:28:47Z

Great job @Honny1. A couple of nits, and potential stutters but I will merge
/lgtm

rhatdan · 2025-03-20T10:44:41Z

/approve

openshift-ci · 2025-03-20T10:44:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Honny1, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Honny1 · 2025-03-20T10:47:45Z

@rhatdan Thanks you. ~~Should I fix nits?~~

openshift-ci bot added do-not-merge/work-in-progress kind/feature Categorizes issue or PR as related to a new feature. labels Feb 25, 2025

Honny1 force-pushed the copy-parents branch from 3010aed to a3a6dcb Compare February 25, 2025 16:01

Honny1 marked this pull request as ready for review February 25, 2025 16:01

openshift-ci bot removed the do-not-merge/work-in-progress label Feb 25, 2025

Honny1 force-pushed the copy-parents branch from a3a6dcb to 3b19e84 Compare February 25, 2025 16:34

nalind requested changes Feb 26, 2025

View reviewed changes

nalind reviewed Feb 26, 2025

View reviewed changes

Honny1 force-pushed the copy-parents branch 3 times, most recently from 8c6ca1f to 638f410 Compare February 27, 2025 19:25

Honny1 requested a review from nalind February 27, 2025 19:33

Honny1 force-pushed the copy-parents branch 2 times, most recently from a37d19e to c3349de Compare March 3, 2025 08:21

nalind requested changes Mar 3, 2025

View reviewed changes

Honny1 force-pushed the copy-parents branch 2 times, most recently from 164e737 to d467554 Compare March 5, 2025 16:16

Honny1 force-pushed the copy-parents branch 6 times, most recently from e1d2e20 to da2d4c7 Compare March 6, 2025 23:02

Honny1 requested a review from nalind March 10, 2025 10:25

nalind requested changes Mar 13, 2025

View reviewed changes

Honny1 force-pushed the copy-parents branch from da2d4c7 to 8161c44 Compare March 14, 2025 14:19

Honny1 requested a review from nalind March 14, 2025 14:23

Honny1 force-pushed the copy-parents branch 2 times, most recently from 2f69989 to 23a49a8 Compare March 18, 2025 16:14

nalind requested changes Mar 18, 2025

View reviewed changes

Honny1 force-pushed the copy-parents branch from 23a49a8 to b0e5b1f Compare March 18, 2025 20:49

Add --parents option for COPY in Dockerfiles

7ca9f34

It also includes an implementation of the --parents flag for the buildah copy command. Fixes: https://issues.redhat.com/browse/RUN-2193 Fixes: containers#5557 Signed-off-by: Jan Rodák <[email protected]>

Honny1 force-pushed the copy-parents branch from b0e5b1f to 7ca9f34 Compare March 18, 2025 20:58

Honny1 requested a review from nalind March 18, 2025 21:37

rhatdan reviewed Mar 20, 2025

View reviewed changes

openshift-ci bot assigned rhatdan Mar 20, 2025

openshift-ci bot added the lgtm label Mar 20, 2025

openshift-ci bot added the approved label Mar 20, 2025

openshift-merge-bot bot merged commit 3e3baee into containers:main Mar 20, 2025
37 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --parents option for COPY in Containerfiles #6008

Add --parents option for COPY in Containerfiles #6008

Honny1 commented Feb 25, 2025

rhatdan commented Feb 26, 2025

nalind left a comment

nalind Feb 26, 2025

Honny1 Feb 27, 2025

nalind Feb 26, 2025

Honny1 Feb 27, 2025

Honny1 Mar 3, 2025

nalind Feb 26, 2025

Honny1 Feb 27, 2025

nalind left a comment

nalind Mar 3, 2025

nalind Mar 3, 2025

Honny1 Mar 4, 2025

nalind Mar 3, 2025

nalind Mar 3, 2025

nalind Mar 3, 2025

Honny1 commented Mar 5, 2025

nalind left a comment

Honny1 commented Mar 14, 2025

nalind commented Mar 17, 2025

Honny1 commented Mar 18, 2025

nalind left a comment

Honny1 commented Mar 18, 2025

Honny1 commented Mar 19, 2025

nalind commented Mar 19, 2025

rhatdan Mar 20, 2025

rhatdan Mar 20, 2025

rhatdan Mar 20, 2025

rhatdan commented Mar 20, 2025

rhatdan commented Mar 20, 2025

openshift-ci bot commented Mar 20, 2025

Honny1 commented Mar 20, 2025 •

edited

Loading

Add --parents option for COPY in Containerfiles #6008

Add --parents option for COPY in Containerfiles #6008

Conversation

Honny1 commented Feb 25, 2025

What type of PR is this?

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

rhatdan commented Feb 26, 2025

nalind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Honny1 commented Mar 5, 2025

nalind left a comment

Choose a reason for hiding this comment

Honny1 commented Mar 14, 2025

nalind commented Mar 17, 2025

Honny1 commented Mar 18, 2025

nalind left a comment

Choose a reason for hiding this comment

Honny1 commented Mar 18, 2025

Honny1 commented Mar 19, 2025

nalind commented Mar 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Mar 20, 2025

rhatdan commented Mar 20, 2025

openshift-ci bot commented Mar 20, 2025

Honny1 commented Mar 20, 2025 • edited Loading

Honny1 commented Mar 20, 2025 •

edited

Loading