-
Notifications
You must be signed in to change notification settings - Fork 766
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
add,copy: if multiple sources are used in ADD
or COPY
then dest must end with a slash
#4183
base: main
Are you sure you want to change the base?
add,copy: if multiple sources are used in ADD
or COPY
then dest must end with a slash
#4183
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc 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:
Approvers can indicate their approval by writing |
7274705
to
7ab4388
Compare
add.go
Outdated
// Check length before accessing `0` | ||
// to avoid `index out of range`. | ||
if len(sources) == 1 { | ||
if strings.HasSuffix(sources[0], "*") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you check for len(sources)==0 and return an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is a suffix of "*" the special case here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Special case is here to match with the current behavior of docker #4167 (comment)
} | ||
} | ||
if multipleSources { | ||
if !strings.HasSuffix(destination, "/") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does docker enforce this? Or does it just check if destination is not a directory then throw an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just chiming in that if the directory already exists, podman/buildah does the expected (?) thing and copies all the matches of the wildcard into the existing directory
Re-using the reproducer from #4167, if you modify the Dockerfile to this
FROM alpine:latest as builder
RUN mkdir /spam
COPY spam/*.txt /spam
Then it copies all the files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rhatdan I think docker checks this cause rule is
* If multiple <src> resources are specified, either directly or due to the use of a wildcard, then <dest> must be a directory, and it must end with a slash /.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still believe this should be done on in the copier.
Basically if the SRC contains more then one file and the DEST is not a directory, then it should return an error.
The directory should not be checked for ending with /
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rhatdan Yes the function is validateSourceAndDest
to validates this using filepath.Glob(
there is also a test which verifies the same, i can add few more test cases if this is a concern and move it to copier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're already globbing inside a chroot further down, when we call copier.Stat()
, and we're probably better off doing this sort of check just after that, when we have all of the facts. Doing this before that means that the current iteration of this patch passes URLs for remote locations to filepath.Glob()
, which is a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess they do document the following:
If <dest> does not end with a trailing slash, it will be considered a regular file and the contents of <src> will be written at <dest>.
But @nalind question above has not been addressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically if the SRC contains more then one file and the DEST is not a directory, then it should return an error.
The directory should not be checked for ending with /
+1 to this, it sounds reasonable to me that if the destination already exists then the trailing slash doesn't need to be enforced. It wouldn't be 100% consistent with docker, but isn't it likely that some users are already doing this? E.g. copying things to /etc
or other standard dirs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chmeliik Yes that is already addressed I just need to move code around as per @nalind 's suggestion here #4183 (comment) , I'll amend this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done moved to just after we split local and remote sources and only validating local sources now.
e60a9ef
to
f1d47cc
Compare
I know I lost my mind a long time ago, but didn't we enforce an ending slash at one point and then removed the enforcement as it was counter to Docker? |
A friendly reminder that this PR had no activity for 30 days. |
@flouthoc any chance you get get back to this one? |
Sure yes, I'll amend this again in this week. |
A friendly reminder that this PR had no activity for 30 days. |
@flouthoc Any movement on this? |
A friendly reminder that this PR had no activity for 30 days. |
@flouthoc Reminder |
A friendly reminder that this PR had no activity for 30 days. |
@flouthoc after the release lets get back to this one. |
@rhatdan Yes getting back on this and closing this one. |
f1d47cc
to
33bcb72
Compare
When using `ADD` or `COPY` enforce the condition that destination must look like a directory by making sure that destination ends with a slash `/`. This ensures that we don't hit undefined behaviour for example with the use case `COPY some/* /oncontainer` it is not clear that `oncontainer` will be a directory or a file, users expect it to be a directory if `*` wildcard resolves to multiple files and logically if wildcard resolves to single file then it should be a directory, since this condition is undefined and not in parity with docker so lets drop support for it. Docker's defined rule: * If multiple <src> resources are specified, either directly or due to the use of a wildcard, then <dest> must be a directory, and it must end with a slash /. Reference: https://docs.docker.com/engine/reference/builder/#add Closes: containers#4167 Signed-off-by: Aditya R <arajan@redhat.com>
Accoring to new validation in ADD and COPY if there are multiple sources then destination must specify that it is a directory Signed-off-by: Aditya R <arajan@redhat.com>
33bcb72
to
86a9ac4
Compare
A friendly reminder that this PR had no activity for 30 days. |
LGTM |
LGTM, but would like a head nod from @vrothberg or @nalind to make sure comments were addressed. |
A friendly reminder that this PR had no activity for 30 days. |
This still needs to be reworked. |
When using
ADD
orCOPY
enforce the condition that destination mustlook like a directory by making sure that destination ends with a slash
/
.This ensures that we don't hit undefined behaviour for example with
the use case
COPY some/* /oncontainer
it is not clear thatoncontainer
will be a directory or a file, users expect it to be adirectory if
*
wildcard resolves to multiple files and logically ifwildcard resolves to single file then it should be a directory, since
this condition is undefined and not in parity with docker so lets drop
support for it.
Docker's defined rule:
<src>
resources are specified, either directly or due to theuse of a wildcard, then
<dest>
must be a directory, and it must end with a slash/
.Reference: https://docs.docker.com/engine/reference/builder/#add
Closes: #4167