Browse code

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>

Yong Tang authored on 2016/09/13 13:06:04
Showing 5 changed files
... ...
@@ -234,6 +234,12 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
234 234
 
235 235
 	var shortImgID string
236 236
 	total := len(b.dockerfile.Children)
237
+	for _, n := range b.dockerfile.Children {
238
+		if err := b.checkDispatch(n, false); err != nil {
239
+			return "", err
240
+		}
241
+	}
242
+
237 243
 	for i, n := range b.dockerfile.Children {
238 244
 		select {
239 245
 		case <-b.clientCtx.Done():
... ...
@@ -243,6 +249,7 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
243 243
 		default:
244 244
 			// Not cancelled yet, keep going...
245 245
 		}
246
+
246 247
 		if err := b.dispatch(i, total, n); err != nil {
247 248
 			if b.options.ForceRemove {
248 249
 				b.clearTmp()
... ...
@@ -322,6 +329,12 @@ func BuildFromConfig(config *container.Config, changes []string) (*container.Con
322 322
 	b.disableCommit = true
323 323
 
324 324
 	total := len(ast.Children)
325
+	for _, n := range ast.Children {
326
+		if err := b.checkDispatch(n, false); err != nil {
327
+			return nil, err
328
+		}
329
+	}
330
+
325 331
 	for i, n := range ast.Children {
326 332
 		if err := b.dispatch(i, total, n); err != nil {
327 333
 			return nil, err
... ...
@@ -201,3 +201,44 @@ func (b *Builder) dispatch(stepN int, stepTotal int, ast *parser.Node) error {
201 201
 
202 202
 	return fmt.Errorf("Unknown instruction: %s", upperCasedCmd)
203 203
 }
204
+
205
+// checkDispatch does a simple check for syntax errors of the Dockerfile.
206
+// Because some of the instructions can only be validated through runtime,
207
+// arg, env, etc., this syntax check will not be complete and could not replace
208
+// the runtime check. Instead, this function is only a helper that allows
209
+// user to find out the obvious error in Dockerfile earlier on.
210
+// onbuild bool: indicate if instruction XXX is part of `ONBUILD XXX` trigger
211
+func (b *Builder) checkDispatch(ast *parser.Node, onbuild bool) error {
212
+	cmd := ast.Value
213
+	upperCasedCmd := strings.ToUpper(cmd)
214
+
215
+	// To ensure the user is given a decent error message if the platform
216
+	// on which the daemon is running does not support a builder command.
217
+	if err := platformSupports(strings.ToLower(cmd)); err != nil {
218
+		return err
219
+	}
220
+
221
+	// The instruction itself is ONBUILD, we will make sure it follows with at
222
+	// least one argument
223
+	if upperCasedCmd == "ONBUILD" {
224
+		if ast.Next == nil {
225
+			return fmt.Errorf("ONBUILD requires at least one argument")
226
+		}
227
+	}
228
+
229
+	// The instruction is part of ONBUILD trigger (not the instruction itself)
230
+	if onbuild {
231
+		switch upperCasedCmd {
232
+		case "ONBUILD":
233
+			return fmt.Errorf("Chaining ONBUILD via `ONBUILD ONBUILD` isn't allowed")
234
+		case "MAINTAINER", "FROM":
235
+			return fmt.Errorf("%s isn't allowed as an ONBUILD trigger", upperCasedCmd)
236
+		}
237
+	}
238
+
239
+	if _, ok := evaluateTable[cmd]; ok {
240
+		return nil
241
+	}
242
+
243
+	return fmt.Errorf("Unknown instruction: %s", upperCasedCmd)
244
+}
... ...
@@ -435,14 +435,12 @@ func (b *Builder) processImageFrom(img builder.Image) error {
435 435
 		}
436 436
 
437 437
 		total := len(ast.Children)
438
-		for i, n := range ast.Children {
439
-			switch strings.ToUpper(n.Value) {
440
-			case "ONBUILD":
441
-				return fmt.Errorf("Chaining ONBUILD via `ONBUILD ONBUILD` isn't allowed")
442
-			case "MAINTAINER", "FROM":
443
-				return fmt.Errorf("%s isn't allowed as an ONBUILD trigger", n.Value)
438
+		for _, n := range ast.Children {
439
+			if err := b.checkDispatch(n, true); err != nil {
440
+				return err
444 441
 			}
445
-
442
+		}
443
+		for i, n := range ast.Children {
446 444
 			if err := b.dispatch(i, total, n); err != nil {
447 445
 				return err
448 446
 			}
... ...
@@ -293,6 +293,9 @@ func runBuild(dockerCli *command.DockerCli, options buildOptions) error {
293 293
 
294 294
 	response, err := dockerCli.Client().ImageBuild(ctx, body, buildOptions)
295 295
 	if err != nil {
296
+		if options.quiet {
297
+			fmt.Fprintf(dockerCli.Err(), "%s", progBuff)
298
+		}
296 299
 		return err
297 300
 	}
298 301
 	defer response.Body.Close()
... ...
@@ -6901,3 +6901,21 @@ func (s *DockerSuite) TestBuildStepsWithProgress(c *check.C) {
6901 6901
 		c.Assert(out, checker.Contains, fmt.Sprintf("Step %d/%d : RUN echo foo", i, 1+totalRun))
6902 6902
 	}
6903 6903
 }
6904
+
6905
+func (s *DockerSuite) TestBuildWithFailure(c *check.C) {
6906
+	name := "testbuildwithfailure"
6907
+
6908
+	// First test case can only detect `nobody` in runtime so all steps will show up
6909
+	buildCmd := "FROM busybox\nRUN nobody"
6910
+	_, stdout, _, err := buildImageWithStdoutStderr(name, buildCmd, false, "--force-rm", "--rm")
6911
+	c.Assert(err, checker.NotNil)
6912
+	c.Assert(stdout, checker.Contains, "Step 1/2 : FROM busybox")
6913
+	c.Assert(stdout, checker.Contains, "Step 2/2 : RUN nobody")
6914
+
6915
+	// Second test case `FFOM` should have been detected before build runs so no steps
6916
+	buildCmd = "FFOM nobody\nRUN nobody"
6917
+	_, stdout, _, err = buildImageWithStdoutStderr(name, buildCmd, false, "--force-rm", "--rm")
6918
+	c.Assert(err, checker.NotNil)
6919
+	c.Assert(stdout, checker.Not(checker.Contains), "Step 1/2 : FROM busybox")
6920
+	c.Assert(stdout, checker.Not(checker.Contains), "Step 2/2 : RUN nobody")
6921
+}