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

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

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)
+}

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
 			}