Skip to content

Commit

Permalink
add,copy: if multiple sources then dest must end with a slash
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@flouthoc
flouthoc committed Aug 23, 2022
1 parent ffbe0e4 commit 780affb
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 9 deletions.
35 changes: 35 additions & 0 deletions add.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,10 +188,45 @@ 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, "/") {
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.
func (b *Builder) Add(destination string, extract bool, options AddAndCopyOptions, sources ...string) error {
if err := validateSourceAndDest(destination, sources); err != nil {
return err
}
mountPoint, err := b.Mount(b.MountLabel)
if err != nil {
return err
Expand Down
34 changes: 31 additions & 3 deletions tests/add.bats
View file
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.missing
View file
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
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

0 comments on commit 780affb

Please sign in to comment.