Dockerfile COPY []-Syntax no comma-validation for 2 arguments

CTaeumel commented 2 years ago

[X] I have tried with the latest version of Docker Desktop
[ ] I have tried disabling enabled experimental features
[ ] I have uploaded Diagnostics
Diagnostics ID:

Problem: When building an image with the current version of Docker for Windows (4.7.1 (77678)) using docker builder, We've encountered the following problem:

COPY ["testfile" "/folder/"] --> no build errors, no file in image COPY ["testfile","testfile2" "/folder/"] --> no build errors, no file in image COPY [ "testfile" "/folder/"] --> build fails (expected) COPY [ "testfile","/folder/"] --> no build errors (expected)

Actual behavior

No errors while building => [2/2] COPY [testfile /folder/] => exporting to image => => exporting layers => => writing image sha256:3bfe5e2887105bf2f967b21ebcf5f5f28bf3b30e2e94059f8f554acc7b70ae28 docker container run testimage:latest ls -l /folder => ls: cannot access '/folder': No such file or directory

Expected behavior

Build fails with an error message, because there is no comma in the COPY statement. The expected and actual behavior is equal when running in linux.

Information

Windows Version: 10 Pro 20H2
Docker Desktop Version: 4.7.1 (77678)
WSL2 or Hyper-V backend? WSL2
Are you running inside a virtualized Windows e.g. on a cloud server or a VM: No

This bug doesn't occur without buildkit.

Output of `& "C:\Program Files\Docker\Docker\resources\com.docker.diagnose.exe" check`

Steps to reproduce the behavior

touch testfile echo -e 'FROM ubuntu:20.04\nCOPY ["testfile" "/folder/"]' > Dockerfile docker image build -t testimage:latest .

thaJeztah commented 2 years ago

Thanks for reporting; this looks like an issue related to build, and possibly best reported in the BuildKit repository. Let me transfer this ticket.

So, the JSON vs non-JSON format can be slightly complicated for some cases, as ["testfile" technically (although unlikely) is a valid filename, and "/folder/"] a valid name for a directory (which can make changing this to an error not always possible; see https://github.com/moby/buildkit/issues/1952, so it won't be possible to produce an error for the syntax (as it's technically correct).

However, in this case there's indeed something odd happening; for some reason BuildKit seems to ignore that the source file or directory (["testfile") doesn't exist, and continues without copying a file (or directory), whereas it likely should've failed with an error that ["testfile" was not found in the build-context.

using this dockerfile;

# syntax=docker/dockerfile:1

FROM busybox
COPY ["testfile" "/folder/"]

The build fails on the classic (non-buildkit builder);

docker build -t foo .
Sending build context to Docker daemon  2.048kB
Step 1/2 : FROM busybox
 ---> db8ee88ad75f
Step 2/2 : COPY ["testfile" "/folder/"]
COPY failed: no source files were specified

But passes on BuildKit;

DOCKER_BUILDKIT=1 docker build -t foo .
[+] Building 0.2s (7/7) FINISHED
 => [internal] load build definition from Dockerfile                                          0.1s
 => => transferring dockerfile: 80B                                                           0.0s
 => [internal] load .dockerignore                                                             0.0s
 => => transferring context: 2B                                                               0.0s
 => [internal] load metadata for docker.io/library/busybox:latest                             0.0s
 => [internal] load build context                                                             0.0s
 => => transferring context: 2B                                                               0.0s
 => [1/2] FROM docker.io/library/busybox                                                      0.0s
 => [2/2] COPY [testfile /folder/]                                                            0.1s
 => exporting to image                                                                        0.0s
 => => exporting layers                                                                       0.0s
 => => writing image sha256:429bfd4e7f4f691fa351c6a050f6d8fae5af2d11f858513d3ed53b327315be01  0.0s
 => => naming to docker.io/library/foo                                                        0.0s

thaJeztah commented 2 years ago

Oh! Can't move it to the buildkit issue tracker, because that's in a different org on GitHub (I could move it to the buildx issue tracker though if that is more suitable).

ping @crazy-max @tonistiigi PTAL

CTaeumel commented 2 years ago

I agree that the it looks like a bug for builder to accept a file that doesn't exist. But I still think the Dockerfile reference clearly states that the [] syntax must contain commata: COPY [--chown=:] ["",... ""] At leasts thats the way I would read it.

If I understand your comment correctly, you state that ["testfile" "/folder/"] would be valid if the testfile would exist. I think this statement contradicts the Dockerfile reference for COPY

thaJeztah commented 2 years ago

The command checks if it's a valid JSON array (["<src>", "<dest>"] or ["<src1>", "<src2>", "<srcX>", "<dest>"]), if that's not the case it uses the non-JSON (<src> <dest>) syntax.

In this case that's ["testfile" "/folder/"] (["testfile" is <src> and "/folder/"] is <dest>), and while it's not a very "usual" name, yes, you can have a filename named like that;

mkdir test && cd test
touch \[\"testfile\"
ls -l

ls -l
total 0
-rw-r--r--    1 root     root             0 Jun 24 16:00 ["testfile"

tonistiigi commented 2 years ago

I think this is because the first argument is detected as a wildcard (that eventually matches to nothing). https://github.com/moby/buildkit/blob/master/cache/contenthash/checksum.go#L805 If we change that detection to always check for the closing ] as well then it should fix this specific case. But you could come up with other weird examples because this field allows so many different formats with different definitions.

docker-robott commented 2 years ago

Issues go stale after 90 days of inactivity. Mark the issue as fresh with /remove-lifecycle stale comment. Stale issues will be closed after an additional 30 days of inactivity.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so.

Send feedback to Docker Community Slack channels #docker-for-mac or #docker-for-windows. /lifecycle stale

tonistiigi commented 2 years ago

@thaJeztah Do you see anything actionable here?

thaJeztah commented 2 years ago

@tonistiigi not sure how much we can do; perhaps we can make these cases more visible in the build progress output? ("not a JSON array; falling back to ...")

docker / buildx