From 9a730a715e44550536fc823fe87bd63a0264bbc0 Mon Sep 17 00:00:00 2001
From: Yong Tang <yong.tang.github@outlook.com>
Date: Mon, 12 Sep 2016 21:06:04 -0700
Subject: [PATCH 1/2] Check bad syntax on dockerfile before building.

This fix tries to address the issue raised in 26453 where bad syntax
on dockerfile is not checked before building, thus user has to wait
before seeing error in dockerfile.

This fix fixes the issue by evaluating all the instructions and check
syntax before dockerfile is invoked actually.

All existing tests pass.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Upstream-commit: c8dc2b156a079ce03db8f579094b9643632661a8
Component: engine
---
 .../engine/builder/dockerfile/builder.go      | 13 ++++++
 .../engine/builder/dockerfile/evaluator.go    | 41 +++++++++++++++++++
 .../engine/builder/dockerfile/internals.go    | 12 +++---
 components/engine/cli/command/image/build.go  |  3 ++
 .../integration-cli/docker_cli_build_test.go  | 18 ++++++++
 5 files changed, 80 insertions(+), 7 deletions(-)

diff --git a/components/engine/builder/dockerfile/builder.go b/components/engine/builder/dockerfile/builder.go
index c206aa90c2..ec067eaa2e 100644
--- a/components/engine/builder/dockerfile/builder.go
+++ b/components/engine/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
diff --git a/components/engine/builder/dockerfile/evaluator.go b/components/engine/builder/dockerfile/evaluator.go
index cdecf05a3b..304739aa82 100644
--- a/components/engine/builder/dockerfile/evaluator.go
+++ b/components/engine/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)
+}
diff --git a/components/engine/builder/dockerfile/internals.go b/components/engine/builder/dockerfile/internals.go
index 54d3301f9a..5c137918c1 100644
--- a/components/engine/builder/dockerfile/internals.go
+++ b/components/engine/builder/dockerfile/internals.go
@@ -435,14 +435,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
 			}
diff --git a/components/engine/cli/command/image/build.go b/components/engine/cli/command/image/build.go
index 85f51f14c0..17be405bd5 100644
--- a/components/engine/cli/command/image/build.go
+++ b/components/engine/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()
diff --git a/components/engine/integration-cli/docker_cli_build_test.go b/components/engine/integration-cli/docker_cli_build_test.go
index 28c9dc09e2..c96531e427 100644
--- a/components/engine/integration-cli/docker_cli_build_test.go
+++ b/components/engine/integration-cli/docker_cli_build_test.go
@@ -6901,3 +6901,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")
+}

From 9e4c8c252aac971802b88da0ab6ee3b31ce87974 Mon Sep 17 00:00:00 2001
From: Yong Tang <yong.tang.github@outlook.com>
Date: Wed, 21 Sep 2016 17:42:53 -0700
Subject: [PATCH 2/2] Update documentation and change log to include the
 preliminary validation of dockerfile.

This commit updates documentation and change log to include
the preliminary validation of the dockerfile before instructions
in dockerfile is run one-by-one.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Upstream-commit: e33dea5b40a52c5dcda682a1a292584ae6bff00d
Component: engine
---
 components/engine/docs/reference/api/docker_remote_api.md  | 1 +
 .../engine/docs/reference/api/docker_remote_api_v1.18.md   | 4 ++++
 .../engine/docs/reference/api/docker_remote_api_v1.19.md   | 4 ++++
 .../engine/docs/reference/api/docker_remote_api_v1.20.md   | 4 ++++
 .../engine/docs/reference/api/docker_remote_api_v1.21.md   | 4 ++++
 .../engine/docs/reference/api/docker_remote_api_v1.22.md   | 4 ++++
 .../engine/docs/reference/api/docker_remote_api_v1.23.md   | 4 ++++
 .../engine/docs/reference/api/docker_remote_api_v1.24.md   | 4 ++++
 .../engine/docs/reference/api/docker_remote_api_v1.25.md   | 4 ++++
 components/engine/docs/reference/builder.md                | 7 +++++++
 10 files changed, 40 insertions(+)

diff --git a/components/engine/docs/reference/api/docker_remote_api.md b/components/engine/docs/reference/api/docker_remote_api.md
index 61adcad43c..30a983becb 100644
--- a/components/engine/docs/reference/api/docker_remote_api.md
+++ b/components/engine/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
 
diff --git a/components/engine/docs/reference/api/docker_remote_api_v1.18.md b/components/engine/docs/reference/api/docker_remote_api_v1.18.md
index 3daf78850e..344ce44f49 100644
--- a/components/engine/docs/reference/api/docker_remote_api_v1.18.md
+++ b/components/engine/docs/reference/api/docker_remote_api_v1.18.md
@@ -1201,6 +1201,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.
 
diff --git a/components/engine/docs/reference/api/docker_remote_api_v1.19.md b/components/engine/docs/reference/api/docker_remote_api_v1.19.md
index 94eea6eb97..bf9589e170 100644
--- a/components/engine/docs/reference/api/docker_remote_api_v1.19.md
+++ b/components/engine/docs/reference/api/docker_remote_api_v1.19.md
@@ -1243,6 +1243,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.
 
diff --git a/components/engine/docs/reference/api/docker_remote_api_v1.20.md b/components/engine/docs/reference/api/docker_remote_api_v1.20.md
index c614d0647e..58a44e8da0 100644
--- a/components/engine/docs/reference/api/docker_remote_api_v1.20.md
+++ b/components/engine/docs/reference/api/docker_remote_api_v1.20.md
@@ -1370,6 +1370,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.
 
diff --git a/components/engine/docs/reference/api/docker_remote_api_v1.21.md b/components/engine/docs/reference/api/docker_remote_api_v1.21.md
index a05cb5a964..bcc2262bcc 100644
--- a/components/engine/docs/reference/api/docker_remote_api_v1.21.md
+++ b/components/engine/docs/reference/api/docker_remote_api_v1.21.md
@@ -1448,6 +1448,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.
 
diff --git a/components/engine/docs/reference/api/docker_remote_api_v1.22.md b/components/engine/docs/reference/api/docker_remote_api_v1.22.md
index 8f03a7b674..7fdb27c2fd 100644
--- a/components/engine/docs/reference/api/docker_remote_api_v1.22.md
+++ b/components/engine/docs/reference/api/docker_remote_api_v1.22.md
@@ -1625,6 +1625,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.
 
diff --git a/components/engine/docs/reference/api/docker_remote_api_v1.23.md b/components/engine/docs/reference/api/docker_remote_api_v1.23.md
index 9817eb5913..a24d84219a 100644
--- a/components/engine/docs/reference/api/docker_remote_api_v1.23.md
+++ b/components/engine/docs/reference/api/docker_remote_api_v1.23.md
@@ -1658,6 +1658,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.
 
diff --git a/components/engine/docs/reference/api/docker_remote_api_v1.24.md b/components/engine/docs/reference/api/docker_remote_api_v1.24.md
index f5e6c6580c..0792646a20 100644
--- a/components/engine/docs/reference/api/docker_remote_api_v1.24.md
+++ b/components/engine/docs/reference/api/docker_remote_api_v1.24.md
@@ -1661,6 +1661,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.
 
diff --git a/components/engine/docs/reference/api/docker_remote_api_v1.25.md b/components/engine/docs/reference/api/docker_remote_api_v1.25.md
index 11e5017fc5..f13a662ebb 100644
--- a/components/engine/docs/reference/api/docker_remote_api_v1.25.md
+++ b/components/engine/docs/reference/api/docker_remote_api_v1.25.md
@@ -1682,6 +1682,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.
 
diff --git a/components/engine/docs/reference/builder.md b/components/engine/docs/reference/builder.md
index cd94170807..53b0532c1a 100644
--- a/components/engine/docs/reference/builder.md
+++ b/components/engine/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