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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+89 −10
Open

add,copy: if multiple sources are used in ADD or COPY then dest must end with a slash #4183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7ffe8b9
add,copy: if multiple sources then dest must end with a slash
flouthoc Jan 26, 2023
86a9ac4
test: retrofit tests for new validation
flouthoc Jan 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions add.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,38 @@ func includeDirectoryAnyway(path string, pm *fileutils.PatternMatcher) bool {
return false
}

// validateSourceAndDest performs a sanity check on the case that if more
// than one source is provided then destination must end with a slash (`/`),
// which signifies that destination is a directory and similarly function
// returns error if destination does not ends with a slash (`/`).
// Following function can contain more validation cases in future.
func validateSourceAndDest(destination string, sources []string) error {
multipleSources := false
if len(sources) > 1 {
multipleSources = true
} else {
// Check length before accessing `0`
// to avoid `index out of range`.
if len(sources) == 1 {
if strings.Contains(sources[0], "*") {
matches, err := filepath.Glob(sources[0])
if err != nil {
return err
}
if len(matches) > 1 {
multipleSources = true
}
}
}
}
if multipleSources {
if !strings.HasSuffix(destination, "/") {
Copy link
Member

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?

Copy link

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

Copy link
Collaborator Author

@flouthoc flouthoc Aug 24, 2022 •
edited
Loading

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 /.

https://docs.docker.com/engine/reference/builder/#add

Copy link
Member

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 /

Copy link
Collaborator Author

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.

Copy link
Member

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.

Copy link
Member

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

Copy link

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

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

return errors.New("adding multiple sources to non-directory destination")
}
}
return nil
}

// Add copies the contents of the specified sources into the container's root
// filesystem, optionally extracting contents of local files that look like
// non-empty archives.
Expand Down Expand Up @@ -244,6 +276,9 @@ func (b *Builder) Add(destination string, extract bool, options AddAndCopyOption
return fmt.Errorf("checking on sources under %q: %w", contextDir, err)
}
}
if err := validateSourceAndDest(destination, localSources); err != nil {
return err
}
numLocalSourceItems := 0
for _, localSourceStat := range localSourceStats {
if localSourceStat.Error != "" {
Expand Down
34 changes: 31 additions & 3 deletions tests/add.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,13 @@ load helpers
# Copy a file to a specific subdirectory
run_buildah add $cid ${TEST_SCRATCH_DIR}/randomfile /subdir
# Copy two files to a specific subdirectory
run_buildah add $cid ${TEST_SCRATCH_DIR}/randomfile ${TEST_SCRATCH_DIR}/other-randomfile /other-subdir
run_buildah add $cid ${TEST_SCRATCH_DIR}/randomfile ${TEST_SCRATCH_DIR}/other-randomfile /other-subdir/
# Copy two files to a specific location, which succeeds because we can create it as a directory.
run_buildah add $cid ${TEST_SCRATCH_DIR}/randomfile ${TEST_SCRATCH_DIR}/other-randomfile /notthereyet-subdir
run_buildah add $cid ${TEST_SCRATCH_DIR}/randomfile ${TEST_SCRATCH_DIR}/other-randomfile /notthereyet-subdir/
# Add with a wildcard and destination dir does not exists
run_buildah add $cid "${TEST_SCRATCH_DIR}/*" /notthereyet2-subdir/
# Copy two files to a specific location, which fails because it's not a directory.
run_buildah 125 add $cid ${TEST_SCRATCH_DIR}/randomfile ${TEST_SCRATCH_DIR}/other-randomfile /randomfile
run_buildah 125 add $cid ${TEST_SCRATCH_DIR}/randomfile ${TEST_SCRATCH_DIR}/other-randomfile /randomfile/
# Copy a file to a different working directory
run_buildah config --workingdir=/cwd $cid
run_buildah add $cid ${TEST_SCRATCH_DIR}/randomfile
Expand All @@ -58,6 +60,32 @@ load helpers
run_buildah rm $newcid
}

@test "add-multiple-files to destination not ending with slash" {
createrandom ${TEST_SCRATCH_DIR}/randomfile
createrandom ${TEST_SCRATCH_DIR}/other-randomfile
createrandom ${TEST_SCRATCH_DIR}/third-randomfile
createrandom ${TEST_SCRATCH_DIR}/hello
createrandom ${TEST_SCRATCH_DIR}/world1
createrandom ${TEST_SCRATCH_DIR}/world2

run_buildah from $WITH_POLICY_JSON scratch
cid=$output
run_buildah mount $cid
root=$output
run_buildah config --workingdir / $cid
# Match buildkit parity
# This should be successful since `BASE/hello*` only resolves to one file.
run_buildah add $cid "${TEST_SCRATCH_DIR}/hello*" /test
# This should fail since `BASE/world*` resolves to more than one file.
run_buildah 125 add $cid "${TEST_SCRATCH_DIR}/world*" /test
expect_output --substring "adding multiple sources to non-directory destination"
# This should fail since `BASE/*` resolves to more than one file.
run_buildah 125 add $cid "${TEST_SCRATCH_DIR}/*" /test
expect_output --substring "adding multiple sources to non-directory destination"
run_buildah 125 add $cid ${TEST_SCRATCH_DIR}/randomefile ${TEST_SCRATCH_DIR}/other-randomfile /test
expect_output --substring "adding multiple sources to non-directory destination"
}

@test "add-local-archive" {
createrandom ${TEST_SCRATCH_DIR}/randomfile
createrandom ${TEST_SCRATCH_DIR}/other-randomfile
Expand Down
2 changes: 1 addition & 1 deletion tests/bud/copy-globs/Containerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
FROM scratch
# *txt exists so should succeed
COPY *.txt /testdir
COPY *.txt /testdir/
2 changes: 1 addition & 1 deletion tests/bud/copy-globs/Containerfile.missing
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
FROM scratch
# No match for *foo, but *txt exists so should succeed
COPY *foo *.txt /testdir
COPY *foo *.txt /testdir/
26 changes: 21 additions & 5 deletions tests/copy.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ load helpers
# copy ${TEST_SCRATCH_DIR}/randomfile to a file of the same name in the container's working directory
run_buildah copy --retry 4 --retry-delay 4s $cid ${TEST_SCRATCH_DIR}/randomfile
# copy ${TEST_SCRATCH_DIR}/other-randomfile and ${TEST_SCRATCH_DIR}/third-randomfile to a new directory named ${TEST_SCRATCH_DIR}/randomfile in the container
run_buildah copy $cid ${TEST_SCRATCH_DIR}/other-randomfile ${TEST_SCRATCH_DIR}/third-randomfile ${TEST_SCRATCH_DIR}/randomfile
run_buildah copy $cid ${TEST_SCRATCH_DIR}/other-randomfile ${TEST_SCRATCH_DIR}/third-randomfile ${TEST_SCRATCH_DIR}/randomfile/
# try to copy ${TEST_SCRATCH_DIR}/other-randomfile and ${TEST_SCRATCH_DIR}/third-randomfile to a /randomfile, which already exists and is a file
run_buildah 125 copy $cid ${TEST_SCRATCH_DIR}/other-randomfile ${TEST_SCRATCH_DIR}/third-randomfile /randomfile
run_buildah 125 copy $cid ${TEST_SCRATCH_DIR}/other-randomfile ${TEST_SCRATCH_DIR}/third-randomfile /randomfile/
# copy ${TEST_SCRATCH_DIR}/other-randomfile and ${TEST_SCRATCH_DIR}/third-randomfile to previously-created directory named ${TEST_SCRATCH_DIR}/randomfile in the container
run_buildah copy $cid ${TEST_SCRATCH_DIR}/other-randomfile ${TEST_SCRATCH_DIR}/third-randomfile ${TEST_SCRATCH_DIR}/randomfile
run_buildah copy $cid ${TEST_SCRATCH_DIR}/other-randomfile ${TEST_SCRATCH_DIR}/third-randomfile ${TEST_SCRATCH_DIR}/randomfile/
run_buildah rm $cid

_prefetch alpine
Expand All @@ -40,15 +40,15 @@ load helpers
root=$output
run_buildah config --workingdir / $cid
run_buildah copy $cid ${TEST_SCRATCH_DIR}/randomfile
run_buildah copy $cid ${TEST_SCRATCH_DIR}/other-randomfile ${TEST_SCRATCH_DIR}/third-randomfile ${TEST_SCRATCH_DIR}/randomfile /etc
run_buildah copy $cid ${TEST_SCRATCH_DIR}/other-randomfile ${TEST_SCRATCH_DIR}/third-randomfile ${TEST_SCRATCH_DIR}/randomfile /etc/
run_buildah rm $cid

run_buildah from --quiet --pull=false $WITH_POLICY_JSON alpine
cid=$output
run_buildah mount $cid
root=$output
run_buildah config --workingdir / $cid
run_buildah copy $cid "${TEST_SCRATCH_DIR}/*randomfile" /etc
run_buildah copy $cid "${TEST_SCRATCH_DIR}/*randomfile" /etc/
(cd ${TEST_SCRATCH_DIR}; for i in *randomfile; do cmp $i ${root}/etc/$i; done)
}

Expand Down Expand Up @@ -78,6 +78,22 @@ load helpers
cmp ${TEST_SCRATCH_DIR}/other-randomfile $newroot/other-randomfile
}

@test "copy-multiple-files to destination not ending with slash" {
createrandom ${TEST_SCRATCH_DIR}/randomfile
createrandom ${TEST_SCRATCH_DIR}/other-randomfile
createrandom ${TEST_SCRATCH_DIR}/third-randomfile

run_buildah from $WITH_POLICY_JSON scratch
cid=$output
run_buildah mount $cid
root=$output
run_buildah config --workingdir / $cid
run_buildah 125 copy $cid ${TEST_SCRATCH_DIR}/* /test
expect_output --substring "adding multiple sources to non-directory destination"
run_buildah 125 copy $cid ${TEST_SCRATCH_DIR}/randomefile ${TEST_SCRATCH_DIR}/other-randomfile /test
expect_output --substring "adding multiple sources to non-directory destination"
}

@test "copy-local-subdirectory" {
mkdir -p ${TEST_SCRATCH_DIR}/subdir
createrandom ${TEST_SCRATCH_DIR}/subdir/randomfile
Expand Down