Merge pull request #26516 from yongtang/26453-build-bad-syntax

Check bad syntax on dockerfile before building.

builder/dockerfile/builder.go

@@ -234,6 +234,12 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
 
 	var shortImgID string
 	total := len(b.dockerfile.Children)
+	for _, n := range b.dockerfile.Children {
+		if err := b.checkDispatch(n, false); err != nil {
+			return "", err
+		}
+	}
+
 	for i, n := range b.dockerfile.Children {
 		select {
 		case <-b.clientCtx.Done():
@@ -243,6 +249,7 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
 		default:
 			// Not cancelled yet, keep going...
 		}
+
 		if err := b.dispatch(i, total, n); err != nil {
 			if b.options.ForceRemove {
 				b.clearTmp()
@@ -322,6 +329,12 @@ func BuildFromConfig(config *container.Config, changes []string) (*container.Con
 	b.disableCommit = true
 
 	total := len(ast.Children)
+	for _, n := range ast.Children {
+		if err := b.checkDispatch(n, false); err != nil {
+			return nil, err
+		}
+	}
+
 	for i, n := range ast.Children {
 		if err := b.dispatch(i, total, n); err != nil {
 			return nil, err

builder/dockerfile/evaluator.go

@@ -201,3 +201,44 @@ func (b *Builder) dispatch(stepN int, stepTotal int, ast *parser.Node) error {
 
 	return fmt.Errorf("Unknown instruction: %s", upperCasedCmd)
 }
+
+// checkDispatch does a simple check for syntax errors of the Dockerfile.
+// Because some of the instructions can only be validated through runtime,
+// arg, env, etc., this syntax check will not be complete and could not replace
+// the runtime check. Instead, this function is only a helper that allows
+// user to find out the obvious error in Dockerfile earlier on.
+// onbuild bool: indicate if instruction XXX is part of `ONBUILD XXX` trigger
+func (b *Builder) checkDispatch(ast *parser.Node, onbuild bool) error {
+	cmd := ast.Value
+	upperCasedCmd := strings.ToUpper(cmd)
+
+	// To ensure the user is given a decent error message if the platform
+	// on which the daemon is running does not support a builder command.
+	if err := platformSupports(strings.ToLower(cmd)); err != nil {
+		return err
+	}
+
+	// The instruction itself is ONBUILD, we will make sure it follows with at
+	// least one argument
+	if upperCasedCmd == "ONBUILD" {
+		if ast.Next == nil {
+			return fmt.Errorf("ONBUILD requires at least one argument")
+		}
+	}
+
+	// The instruction is part of ONBUILD trigger (not the instruction itself)
+	if onbuild {
+		switch upperCasedCmd {
+		case "ONBUILD":
+			return fmt.Errorf("Chaining ONBUILD via `ONBUILD ONBUILD` isn't allowed")
+		case "MAINTAINER", "FROM":
+			return fmt.Errorf("%s isn't allowed as an ONBUILD trigger", upperCasedCmd)
+		}
+	}
+
+	if _, ok := evaluateTable[cmd]; ok {
+		return nil
+	}
+
+	return fmt.Errorf("Unknown instruction: %s", upperCasedCmd)
+}

builder/dockerfile/internals.go

@@ -423,14 +423,12 @@ func (b *Builder) processImageFrom(img builder.Image) error {
 		}
 
 		total := len(ast.Children)
-		for i, n := range ast.Children {
-			switch strings.ToUpper(n.Value) {
-			case "ONBUILD":
-				return fmt.Errorf("Chaining ONBUILD via `ONBUILD ONBUILD` isn't allowed")
-			case "MAINTAINER", "FROM":
-				return fmt.Errorf("%s isn't allowed as an ONBUILD trigger", n.Value)
+		for _, n := range ast.Children {
+			if err := b.checkDispatch(n, true); err != nil {
+				return err
 			}
-
+		}
+		for i, n := range ast.Children {
 			if err := b.dispatch(i, total, n); err != nil {
 				return err
 			}

cli/command/image/build.go

@@ -293,6 +293,9 @@ func runBuild(dockerCli *command.DockerCli, options buildOptions) error {
 
 	response, err := dockerCli.Client().ImageBuild(ctx, body, buildOptions)
 	if err != nil {
+		if options.quiet {
+			fmt.Fprintf(dockerCli.Err(), "%s", progBuff)
+		}
 		return err
 	}
 	defer response.Body.Close()

docs/reference/api/docker_remote_api.md

@@ -123,6 +123,7 @@ This section lists each version from latest to oldest.  Each listing includes a
 * `POST /containers/create/` and `POST /containers/(name)/update` now validates restart policies.
 * `POST /containers/create` now validates IPAMConfig in NetworkingConfig, and returns error for invalid IPv4 and IPv6 addresses (`--ip` and `--ip6` in `docker create/run`).
 * `POST /containers/create` now takes a `Mounts` field in `HostConfig` which replaces `Binds` and `Volumes`. *note*: `Binds` and `Volumes` are still available but are exclusive with `Mounts`
+* `POST /build` now performs a preliminary validation of the `Dockerfile` before starting the build, and returns an error if the syntax is incorrect. Note that this change is _unversioned_ and applied to all API versions.
 
 ### v1.24 API changes
 

docs/reference/api/docker_remote_api_v1.18.md

@@ -1204,6 +1204,10 @@ The archive may include any number of other files,
 which are accessible in the build context (See the [*ADD build
 command*](../../reference/builder.md#add)).
 
+The Docker daemon performs a preliminary validation of the `Dockerfile` before
+starting the build, and returns an error if the syntax is incorrect. After that,
+each instruction is run one-by-one until the ID of the new image is output.
+
 The build is canceled if the client drops the connection by quitting
 or being killed.
 

docs/reference/api/docker_remote_api_v1.19.md

@@ -1246,6 +1246,10 @@ The archive may include any number of other files,
 which are accessible in the build context (See the [*ADD build
 command*](../../reference/builder.md#add)).
 
+The Docker daemon performs a preliminary validation of the `Dockerfile` before
+starting the build, and returns an error if the syntax is incorrect. After that,
+each instruction is run one-by-one until the ID of the new image is output.
+
 The build is canceled if the client drops the connection by quitting
 or being killed.
 

docs/reference/api/docker_remote_api_v1.20.md

@@ -1373,6 +1373,10 @@ The archive may include any number of other files,
 which are accessible in the build context (See the [*ADD build
 command*](../../reference/builder.md#add)).
 
+The Docker daemon performs a preliminary validation of the `Dockerfile` before
+starting the build, and returns an error if the syntax is incorrect. After that,
+each instruction is run one-by-one until the ID of the new image is output.
+
 The build is canceled if the client drops the connection by quitting
 or being killed.
 

docs/reference/api/docker_remote_api_v1.21.md

@@ -1454,6 +1454,10 @@ The archive may include any number of other files,
 which are accessible in the build context (See the [*ADD build
 command*](../../reference/builder.md#add)).
 
+The Docker daemon performs a preliminary validation of the `Dockerfile` before
+starting the build, and returns an error if the syntax is incorrect. After that,
+each instruction is run one-by-one until the ID of the new image is output.
+
 The build is canceled if the client drops the connection by quitting
 or being killed.
 

docs/reference/api/docker_remote_api_v1.22.md

@@ -1632,6 +1632,10 @@ The archive may include any number of other files,
 which are accessible in the build context (See the [*ADD build
 command*](../../reference/builder.md#add)).
 
+The Docker daemon performs a preliminary validation of the `Dockerfile` before
+starting the build, and returns an error if the syntax is incorrect. After that,
+each instruction is run one-by-one until the ID of the new image is output.
+
 The build is canceled if the client drops the connection by quitting
 or being killed.
 

docs/reference/api/docker_remote_api_v1.23.md

@@ -1665,6 +1665,10 @@ The archive may include any number of other files,
 which are accessible in the build context (See the [*ADD build
 command*](../../reference/builder.md#add)).
 
+The Docker daemon performs a preliminary validation of the `Dockerfile` before
+starting the build, and returns an error if the syntax is incorrect. After that,
+each instruction is run one-by-one until the ID of the new image is output.
+
 The build is canceled if the client drops the connection by quitting
 or being killed.
 

docs/reference/api/docker_remote_api_v1.24.md

@@ -1666,6 +1666,10 @@ The archive may include any number of other files,
 which are accessible in the build context (See the [*ADD build
 command*](../../reference/builder.md#add)).
 
+The Docker daemon performs a preliminary validation of the `Dockerfile` before
+starting the build, and returns an error if the syntax is incorrect. After that,
+each instruction is run one-by-one until the ID of the new image is output.
+
 The build is canceled if the client drops the connection by quitting
 or being killed.
 

docs/reference/api/docker_remote_api_v1.25.md

@@ -1692,6 +1692,10 @@ The archive may include any number of other files,
 which are accessible in the build context (See the [*ADD build
 command*](../../reference/builder.md#add)).
 
+The Docker daemon performs a preliminary validation of the `Dockerfile` before
+starting the build, and returns an error if the syntax is incorrect. After that,
+each instruction is run one-by-one until the ID of the new image is output.
+
 The build is canceled if the client drops the connection by quitting
 or being killed.
 

docs/reference/builder.md

@@ -68,6 +68,13 @@ add multiple `-t` parameters when you run the `build` command:
 
     $ docker build -t shykes/myapp:1.0.2 -t shykes/myapp:latest .
 
+Before the Docker daemon runs the instructions in the `Dockerfile`, it performs
+a preliminary validation of the `Dockerfile` and returns an error if the syntax is incorrect:
+
+    $ docker build -t test/myapp .
+    Sending build context to Docker daemon 2.048 kB
+    Error response from daemon: Unknown instruction: RUNCMD
+
 The Docker daemon runs the instructions in the `Dockerfile` one-by-one,
 committing the result of each instruction
 to a new image if necessary, before finally outputting the ID of your

integration-cli/docker_cli_build_test.go

@@ -6913,3 +6913,21 @@ func (s *DockerSuite) TestBuildStepsWithProgress(c *check.C) {
 		c.Assert(out, checker.Contains, fmt.Sprintf("Step %d/%d : RUN echo foo", i, 1+totalRun))
 	}
 }
+
+func (s *DockerSuite) TestBuildWithFailure(c *check.C) {
+	name := "testbuildwithfailure"
+
+	// First test case can only detect `nobody` in runtime so all steps will show up
+	buildCmd := "FROM busybox\nRUN nobody"
+	_, stdout, _, err := buildImageWithStdoutStderr(name, buildCmd, false, "--force-rm", "--rm")
+	c.Assert(err, checker.NotNil)
+	c.Assert(stdout, checker.Contains, "Step 1/2 : FROM busybox")
+	c.Assert(stdout, checker.Contains, "Step 2/2 : RUN nobody")
+
+	// Second test case `FFOM` should have been detected before build runs so no steps
+	buildCmd = "FFOM nobody\nRUN nobody"
+	_, stdout, _, err = buildImageWithStdoutStderr(name, buildCmd, false, "--force-rm", "--rm")
+	c.Assert(err, checker.NotNil)
+	c.Assert(stdout, checker.Not(checker.Contains), "Step 1/2 : FROM busybox")
+	c.Assert(stdout, checker.Not(checker.Contains), "Step 2/2 : RUN nobody")
+}