Dockerfile: bug: copying multiple sources to a destination overwrites destination

[thaJeztah]

This is a combination of two issues;

• Inconsistent validation of Dockerfile syntax when copying multiple source to a destination using COPY (and ADD)
• A bug related to the above, causing destination to be overwritten instead of creating destination path

reproduction steps for inconsistent validation

Create a directory with two files:

mkdir foo && cd foo
touch one two

Perform a build that copies both files to a destination path:

docker build --no-cache -f- . <<'EOF'
FROM busybox
COPY one two /dest
EOF

Repeat the same when using a wildcard as source;

docker build --no-cache -f- . <<'EOF'
FROM busybox
COPY * /dest
EOF

Without BuildKit, both cases produce an error (note that an error is only produced if the wildcard matches multiple files):

Sending build context to Docker daemon  2.607kB
Step 1/2 : FROM busybox
 ---> f0b02e9d092d
Step 2/2 : COPY one two /dest
When using COPY with more than one source file, the destination must be a directory and end with a /

With BuildKit, no error is produced:

[+] Building 0.5s (7/7) FINISHED
 => [internal] load .dockerignore                                                              0.1s
 => => transferring context: 2B                                                                0.0s
 => [internal] load build definition from Dockerfile                                           0.2s
 => => transferring dockerfile: 74B                                                            0.0s
 => [internal] load metadata for docker.io/library/busybox:latest                              0.0s
 => [internal] load build context                                                              0.1s
 => => transferring context: 42B                                                               0.0s
 => CACHED [1/2] FROM docker.io/library/busybox                                                0.0s
 => [2/2] COPY one two /dest                                                                   0.1s
 => exporting to image                                                                         0.0s
 => => exporting layers                                                                        0.0s
 => => writing image sha256:ca15ac822b41facebb19415c225c4ab1b1c7922911b354405d42e6b31dcb178d   0.0s

I would expect both builders to produce the same result. This error was added in moby/moby#8076 (original implementation to support multiple source in COPY/ADD). Although that PR does not mention why the error was added, I expect this was done to prevent ambiguity, and to account for the follow-up PR to implement wildcard copy.

Bug: copying multiple sources does not create directory, and overwrites destination

mkdir foo && cd foo
echo "I am one" > one

DOCKER_BUILDKIT=1 docker build --no-cache --progress=plain -f- . <<'EOF'
FROM busybox
COPY * /dest
RUN ls -la /dest
RUN cat /dest
EOF

#7 [3/4] RUN ls -la /dest
#7 sha256:b50a5cfb31616d8591990732aba8ccf693668d610b2a4e0d371306c11d640d47
#7 0.257 -rw-r--r--    1 root     root             9 Nov 26 10:52 /dest
#7 DONE 0.3s

#8 [4/4] RUN cat /dest
#8 sha256:96703a4cf33b9584ca11a545c8a1e6dd2b4d7b0d3ea5dc94a109366d4cc20e2e
#8 0.256 I am one
#8 DONE 0.3s

Now create a second file in the current directory, and repeat the build;

echo "I am two" > two

DOCKER_BUILDKIT=1 docker build --no-cache --progress=plain -f- . <<'EOF'
FROM busybox
COPY * /dest
RUN ls -la /dest
RUN cat /dest
EOF

#7 [3/4] RUN ls -la /dest
#7 sha256:e33a1a1c73bb96096bb5101a3d4391ee9894cc9a84faa7cfa8f016d8f9df7394
#7 0.280 -rw-r--r--    1 root     root             9 Nov 26 10:53 /dest
#7 DONE 0.4s

#8 [4/4] RUN cat /dest
#8 sha256:4f1c74800842289bd292c2c237042f2bd7d30bbd1e23728e407accc1897fac95
#8 0.287 I am two
#8 DONE 0.4s

As can be seen in the above, BuildKit creates a file named dest, copies one to that file, and then overwrites the file with file two

Note that adding a trailing / to the destination fixes this situation;

DOCKER_BUILDKIT=1 docker build --no-cache --progress=plain -f- . <<'EOF'
FROM busybox
COPY * /dest/
RUN ls -la /dest
RUN cat /dest
EOF


#7 [3/4] RUN ls -la /dest
#7 sha256:023e34d9abccf53df55a31b2a935227b5b9d847931466a98d65f975b9aa944a4
#7 0.265 total 16
#7 0.265 drwxr-xr-x    2 root     root          4096 Nov 26 11:11 .
#7 0.265 drwxr-xr-x    1 root     root          4096 Nov 26 11:11 ..
#7 0.265 -rw-r--r--    1 root     root             9 Nov 26 10:59 one
#7 0.265 -rw-r--r--    1 root     root             9 Nov 26 10:59 two
#7 DONE 0.4s

#8 [4/4] RUN cat /dest
#8 sha256:fde1c24f2aca4ddf7192ad912ec51afdae2879dafb6100b2d23b59b60aadef73
#8 0.262 cat: read error: Is a directory
#8 ERROR: executor failed running [/bin/sh -c cat /dest]: exit code: 1
------
 > [4/4] RUN cat /dest:
------
executor failed running [/bin/sh -c cat /dest]: exit code: 1

What I expected

I expected BuildKit to;

1. produce an error that dest should have a trailing / if multiple sources are specified
2. produce an error that dest should have a trailing / if SRC uses a wildcard / globbing, and matches multiple files
3. create a directory named dest, and to copy files one and two into that directory

While 1. and 2. could be "optional", I think we should produce an error because;

• without trailing slash, the instruction can be ambiguous (in the case of explicitly specifying multiple sources).
• forcing a trailing / also (see earlier example) prevents the problem of creating a file (and overwriting), which makes the error useful to have
• finally, the syntax would be be (somewhat) consistent with the syntax described in docker cp https://docs.docker.com/engine/reference/commandline/cp/#extended-description;

Assuming a path separator of /, a first argument of SRC_PATH and second
argument of DEST_PATH, the behavior is as follows:

• SRC_PATH specifies a file
  • DEST_PATH does not exist
    • the file is saved to a file created at DEST_PATH
  • DEST_PATH does not exist and ends with /
    • Error condition: the destination directory must exist.
  • DEST_PATH exists and is a file
    • the destination is overwritten with the source file's contents
  • DEST_PATH exists and is a directory
    • the file is copied into this directory using the basename from
      SRC_PATH

If we enforce the trailing / for multiple files, this case would be similar to;

DEST_PATH does not exist and ends with /
- Error condition: the destination directory must exist.

With the exception of the "error" condition, because Docker defaults to creating the destination directory

[thaJeztah]

@tonistiigi @tiborvass ptal

[thaJeztah]

I just found #1667, which also describes some of this