builder: produce error when using unsupported Dockerfile option by thaJeztah · Pull Request #41912 · moby/moby

thaJeztah · 2021-01-21T12:30:09Z

fixes #41911

depends on

vendor: github.com/moby/buildkit v0.8.2 #42056 vendor: github.com/moby/buildkit v0.8.2
- depends on frontend/dockerfile: add RunCommand.FlagsUsed field buildkit#1954 / [v0.8 backport] frontend/dockerfile: add RunCommand.FlagsUsed field buildkit#1971

builder: produce error when using unsupported Dockerfile option

With the promotion of the experimental Dockerfile syntax to "stable", the Dockerfile
syntax now includes some options that are supported by BuildKit, but not (yet)
supported in the classic builder.

As a result, parsing a Dockerfile may succeed, but any flag that's known to BuildKit,
but not supported by the classic builder is silently ignored;

$ mkdir buildkit_flags && cd buildkit_flags
$ touch foo.txt

For example, RUN --mount:

DOCKER_BUILDKIT=0 docker build --no-cache -f- . <<EOF
FROM busybox
RUN --mount=type=cache,target=/foo echo hello
EOF

Sending build context to Docker daemon  2.095kB
Step 1/2 : FROM busybox
 ---> 219ee5171f80
Step 2/2 : RUN --mount=type=cache,target=/foo echo hello
 ---> Running in 022fdb856bc8
hello
Removing intermediate container 022fdb856bc8
 ---> e9f0988844d1
Successfully built e9f0988844d1

Or COPY --chmod (same for ADD --chmod):

DOCKER_BUILDKIT=0 docker build --no-cache -f- . <<EOF
FROM busybox
COPY --chmod=0777 /foo.txt /foo.txt
EOF

Sending build context to Docker daemon  2.095kB
Step 1/2 : FROM busybox
 ---> 219ee5171f80
Step 2/2 : COPY --chmod=0777 /foo.txt /foo.txt
 ---> 8b7117932a2a
Successfully built 8b7117932a2a

Note that unknown flags still produce and error, for example, the below fails because --hello is an unknown flag;

DOCKER_BUILDKIT=0 docker build -<<EOF
FROM busybox
RUN --hello echo hello
EOF

Sending build context to Docker daemon  2.048kB
Error response from daemon: dockerfile parse error line 2: Unknown flag: hello

With this patch applied

With this patch applied, flags that are known in the Dockerfile spec, but are not
supported by the classic builder, produce an error, which includes a link to the
documentation how to enable BuildKit:

DOCKER_BUILDKIT=0 docker build --no-cache -f- . <<EOF
FROM busybox
RUN --mount=type=cache,target=/foo echo hello
EOF

Sending build context to Docker daemon  2.048kB
Step 1/2 : FROM busybox
 ---> b97242f89c8a
Step 2/2 : RUN --mount=type=cache,target=/foo echo hello
the --mount option BuildKit. Refer to https://docs.docker.com/go/buildkit/ to learn how to build images with BuildKit enabled.


DOCKER_BUILDKIT=0 docker build --no-cache -f- . <<EOF
FROM busybox
COPY --chmod=0777 /foo.txt /foo.txt
EOF

Sending build context to Docker daemon  2.095kB
Step 1/2 : FROM busybox
 ---> b97242f89c8a
Step 2/2 : COPY --chmod=0777 /foo.txt /foo.txt
the --chmod option requires BuildKit. Refer to https://docs.docker.com/go/buildkit/ to learn how to build images with BuildKit enabled.

- Description for the changelog

Fix classic builder silently ignoring unsupported Dockerfile options

thaJeztah · 2021-01-21T12:31:21Z

Temporary patch; perhaps there's a cleaner approach for this, but thought I'd open a PR to discuss the best approach.

thaJeztah · 2021-01-21T12:32:59Z

Perhaps all command should take the same approach; COPY and ADD have some additional properties to store the Chmod, Chown values, but other commands may not; perhaps using the FlagsUsed approach could make it more generic (we could make a supportedOptions variable, containing the supported options per command.

thaJeztah · 2021-01-21T12:33:28Z

@tonistiigi @tiborvass PTAL; let me know what approach you think is best for this

AkihiroSuda · 2021-01-21T12:49:28Z

Can we continue ignoring cache mounts? (With warning)

thaJeztah · 2021-01-21T13:01:00Z

Can we continue ignoring cache mounts? (With warning)

I initially proposed doing so in moby/buildkit#799, but I know @tonistiigi didn't agree on silently ignoring them; moby/buildkit#799 (comment)

Also wondering if we have a mechanism to return warnings back to the CLI; we can of course log them, but daemon logs won't be visible to the user (unless explicitly looking for them), or do you know a way to return warnings?

Alternative would be to parse the Dockerfile on the client-side before starting the build, but that would complicate things (especially if client and daemon are different versions).

thaJeztah · 2021-01-21T13:02:25Z

or do you know a way to return warnings?

Would writing to builder.Stdout / builder.Stderr work for that?

moby/builder/dockerfile/dispatchers.go

Line 378 in cf0ce96

    
           if err := d.builder.containerManager.Run(d.builder.clientCtx, cID, d.builder.Stdout, d.builder.Stderr); err != nil {

thaJeztah · 2021-01-21T13:06:42Z

Yes, that looks to work to print warnings if we want to;

Sending build context to Docker daemon  2.095kB
Step 1/2 : FROM busybox
 ---> b97242f89c8a
Step 2/2 : RUN --mount=type=cache,target=/foo echo hello
 ---> [Warning] RUN --mount requires BuildKit. Refer to https://docs.docker.com/go/buildkit/ to learn how to build images with BuildKit enabled.
 ---> Running in af2590e5f0d0
hello
Removing intermediate container af2590e5f0d0
 ---> e86a5e0157b2
Successfully built e86a5e0157b2

thaJeztah · 2021-02-22T18:00:03Z

Perhaps needs a nolint;

 builder/dockerfile/dispatchers.go:96:21: error strings should not be capitalized or end with punctuation or a newline (golint)
 		return errors.New("ADD --chmod requires BuildKit. Refer to https://docs.docker.com/go/buildkit/ to learn how to build images with BuildKit enabled.")
 		                  ^
 builder/dockerfile/dispatchers.go:118:21: error strings should not be capitalized or end with punctuation or a newline (golint)
 		return errors.New("COPY --chmod requires BuildKit. Refer to https://docs.docker.com/go/buildkit/ to learn how to build images with BuildKit enabled.")

thaJeztah · 2021-03-04T18:06:35Z

This should be ready for review now; @tonistiigi @AkihiroSuda @cpuguy83 ptal

thaJeztah · 2021-03-09T09:23:55Z

@tonistiigi @AkihiroSuda @cpuguy83 ptal

tiborvass · 2021-03-12T17:25:44Z

bit weird to have a return in a for loop. Would be slightly better with an if len(c.FlagsUsed) > 0 { return errors.Errorf(..., c.FlagsUsed[0]) }

Ah, yes, I think I started with creating an error-message for all flags or to have a "whitelist", but then decided "meh, just return an error for the first one"; I'll update

tiborvass · 2021-03-12T17:26:07Z

Overall looks good to me

With the promotion of the experimental Dockerfile syntax to "stable", the Dockerfile syntax now includes some options that are supported by BuildKit, but not (yet) supported in the classic builder. As a result, parsing a Dockerfile may succeed, but any flag that's known to BuildKit, but not supported by the classic builder is silently ignored; $ mkdir buildkit_flags && cd buildkit_flags $ touch foo.txt For example, `RUN --mount`: DOCKER_BUILDKIT=0 docker build --no-cache -f- . <<EOF FROM busybox RUN --mount=type=cache,target=/foo echo hello EOF Sending build context to Docker daemon 2.095kB Step 1/2 : FROM busybox ---> 219ee5171f80 Step 2/2 : RUN --mount=type=cache,target=/foo echo hello ---> Running in 022fdb856bc8 hello Removing intermediate container 022fdb856bc8 ---> e9f0988844d1 Successfully built e9f0988844d1 Or `COPY --chmod` (same for `ADD --chmod`): DOCKER_BUILDKIT=0 docker build --no-cache -f- . <<EOF FROM busybox COPY --chmod=0777 /foo.txt /foo.txt EOF Sending build context to Docker daemon 2.095kB Step 1/2 : FROM busybox ---> 219ee5171f80 Step 2/2 : COPY --chmod=0777 /foo.txt /foo.txt ---> 8b7117932a2a Successfully built 8b7117932a2a Note that unknown flags still produce and error, for example, the below fails because `--hello` is an unknown flag; DOCKER_BUILDKIT=0 docker build -<<EOF FROM busybox RUN --hello echo hello EOF Sending build context to Docker daemon 2.048kB Error response from daemon: dockerfile parse error line 2: Unknown flag: hello With this patch applied ---------------------------- With this patch applied, flags that are known in the Dockerfile spec, but are not supported by the classic builder, produce an error, which includes a link to the documentation how to enable BuildKit: DOCKER_BUILDKIT=0 docker build --no-cache -f- . <<EOF FROM busybox RUN --mount=type=cache,target=/foo echo hello EOF Sending build context to Docker daemon 2.048kB Step 1/2 : FROM busybox ---> b97242f89c8a Step 2/2 : RUN --mount=type=cache,target=/foo echo hello the --mount option requires BuildKit. Refer to https://docs.docker.com/go/buildkit/ to learn how to build images with BuildKit enabled DOCKER_BUILDKIT=0 docker build --no-cache -f- . <<EOF FROM busybox COPY --chmod=0777 /foo.txt /foo.txt EOF Sending build context to Docker daemon 2.095kB Step 1/2 : FROM busybox ---> b97242f89c8a Step 2/2 : COPY --chmod=0777 /foo.txt /foo.txt the --chmod option requires BuildKit. Refer to https://docs.docker.com/go/buildkit/ to learn how to build images with BuildKit enabled Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah · 2021-03-14T13:11:56Z

@tiborvass @AkihiroSuda updated; PTAL

AkihiroSuda

LGTM but a nit

AkihiroSuda · 2021-03-14T13:34:34Z

+		for _, f := range []string{"mount", "network", "security", "any-flag"} {
+			cmd.FlagsUsed = []string{f}
+			err := dispatch(sb, cmd)
+			assert.Error(t, err, fmt.Sprintf("the --%s option requires BuildKit. Refer to https://docs.docker.com/go/buildkit/ to learn how to build images with BuildKit enabled", f))


"the --any-flag option requires BuildKit" is slightly inaccurate. What about "the --%s option is not supported on legacy mode"

Hmmm... yes, that's a bit tricky; it would indeed suggest that in case someone made an actual RUN --i-made-a-typo, it would print the standard message; do you think we should keep an actual list of known options?

oh, actually, that should already be handled, because the frontend will produce an error;

DOCKER_BUILDKIT=0 docker build -<<EOF FROM busybox RUN --no-such-flag echo hello EOF Sending build context to Docker daemon 2.048kB Error response from daemon: dockerfile parse error line 2: Unknown flag: no-such-flag

thaJeztah · 2021-03-22T21:23:18Z

@tiborvass PTAL

thaJeztah added area/builder Build status/2-code-review kind/bugfix PR's that fix bugs labels Jan 21, 2021

thaJeztah added this to the 20.10.3 milestone Jan 21, 2021

thaJeztah commented Jan 21, 2021

View reviewed changes

thaJeztah mentioned this pull request Jan 26, 2021

frontend/dockerfile: add RunCommand.FlagsUsed field moby/buildkit#1954

Merged

thaJeztah force-pushed the improve_build_errors branch from 392c4ec to f502aa2 Compare January 26, 2021 15:59

thaJeztah modified the milestones: 20.10.3, 20.10.4 Feb 2, 2021

thaJeztah added the process/cherry-pick label Feb 2, 2021

thaJeztah modified the milestones: 20.10.4, 21.xx Feb 2, 2021

thaJeztah mentioned this pull request Feb 4, 2021

[v0.8 backport] frontend/dockerfile: add RunCommand.FlagsUsed field moby/buildkit#1971

Merged

thaJeztah force-pushed the improve_build_errors branch 2 times, most recently from 215fd10 to f6ae6d7 Compare February 15, 2021 16:31

thaJeztah mentioned this pull request Feb 22, 2021

vendor: github.com/moby/buildkit v0.8.2 #42056

Merged

thaJeztah force-pushed the improve_build_errors branch from f6ae6d7 to 9d635e1 Compare February 22, 2021 09:47

thaJeztah mentioned this pull request Feb 23, 2021

[20.10 backport] vendor: github.com/moby/buildkit v0.8.2 #42061

Merged

thaJeztah force-pushed the improve_build_errors branch from 9d635e1 to 848c076 Compare March 4, 2021 18:01

thaJeztah marked this pull request as ready for review March 4, 2021 18:02

thaJeztah requested a review from tonistiigi as a code owner March 4, 2021 18:02

thaJeztah force-pushed the improve_build_errors branch from 848c076 to b7ce94d Compare March 4, 2021 18:05

tiborvass reviewed Mar 12, 2021

View reviewed changes

thaJeztah force-pushed the improve_build_errors branch from b7ce94d to a09c027 Compare March 14, 2021 13:11

AkihiroSuda approved these changes Mar 14, 2021

View reviewed changes

tiborvass approved these changes Mar 24, 2021

View reviewed changes

tiborvass merged commit 797b974 into moby:master Mar 24, 2021

thaJeztah deleted the improve_build_errors branch March 24, 2021 22:56

thaJeztah mentioned this pull request Mar 24, 2021

[20.10 backport] builder: produce error when using unsupported Dockerfile option #42197

Merged

2 tasks

thaJeztah added process/cherry-picked area/builder/classic-builder Build and removed process/cherry-pick labels Mar 24, 2021

thaJeztah mentioned this pull request May 27, 2021

Dockerfile heredocs moby/buildkit#2132

Merged

4 tasks

Conversation

thaJeztah commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

builder: produce error when using unsupported Dockerfile option

With this patch applied

Uh oh!

thaJeztah Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jan 21, 2021

Uh oh!

AkihiroSuda commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Jan 21, 2021

Uh oh!

thaJeztah commented Jan 21, 2021

Uh oh!

thaJeztah commented Jan 21, 2021

Uh oh!

thaJeztah commented Feb 22, 2021

Uh oh!

thaJeztah commented Mar 4, 2021

Uh oh!

thaJeztah commented Mar 9, 2021

Uh oh!

tiborvass Mar 12, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 14, 2021

Choose a reason for hiding this comment

Uh oh!

tiborvass commented Mar 12, 2021

Uh oh!

thaJeztah commented Mar 14, 2021

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Mar 14, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 14, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 14, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thaJeztah commented Jan 21, 2021 •

edited

Loading

AkihiroSuda commented Jan 21, 2021 •

edited

Loading