From c8dc2b156a079ce03db8f579094b9643632661a8 Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Mon, 12 Sep 2016 21:06:04 -0700 Subject: [PATCH] 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 --- builder/dockerfile/builder.go | 13 ++++++++ builder/dockerfile/evaluator.go | 41 ++++++++++++++++++++++++ builder/dockerfile/internals.go | 12 +++---- cli/command/image/build.go | 3 ++ integration-cli/docker_cli_build_test.go | 18 +++++++++++ 5 files changed, 80 insertions(+), 7 deletions(-) diff --git a/builder/dockerfile/builder.go b/builder/dockerfile/builder.go index c206aa90c2..ec067eaa2e 100644 --- a/builder/dockerfile/builder.go +++ b/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/builder/dockerfile/evaluator.go b/builder/dockerfile/evaluator.go index cdecf05a3b..304739aa82 100644 --- a/builder/dockerfile/evaluator.go +++ b/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/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index 54d3301f9a..5c137918c1 100644 --- a/builder/dockerfile/internals.go +++ b/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/cli/command/image/build.go b/cli/command/image/build.go index 85f51f14c0..17be405bd5 100644 --- a/cli/command/image/build.go +++ b/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/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index 28c9dc09e2..c96531e427 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/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") +}