diff --git a/builder/dispatchers.go b/builder/dispatchers.go index ec79f2c154..82bb6ce5fd 100644 --- a/builder/dispatchers.go +++ b/builder/dispatchers.go @@ -20,7 +20,7 @@ import ( ) // dispatch with no layer / parsing. This is effectively not a command. -func nullDispatch(b *Builder, args []string, attributes map[string]bool) error { +func nullDispatch(b *Builder, args []string, attributes map[string]bool, original string) error { return nil } @@ -29,7 +29,7 @@ func nullDispatch(b *Builder, args []string, attributes map[string]bool) error { // Sets the environment variable foo to bar, also makes interpolation // in the dockerfile available from the next statement on via ${foo}. // -func env(b *Builder, args []string, attributes map[string]bool) error { +func env(b *Builder, args []string, attributes map[string]bool, original string) error { if len(args) != 2 { return fmt.Errorf("ENV accepts two arguments") } @@ -50,7 +50,7 @@ func env(b *Builder, args []string, attributes map[string]bool) error { // MAINTAINER some text // // Sets the maintainer metadata. -func maintainer(b *Builder, args []string, attributes map[string]bool) error { +func maintainer(b *Builder, args []string, attributes map[string]bool, original string) error { if len(args) != 1 { return fmt.Errorf("MAINTAINER requires only one argument") } @@ -64,7 +64,7 @@ func maintainer(b *Builder, args []string, attributes map[string]bool) error { // Add the file 'foo' to '/path'. Tarball and Remote URL (git, http) handling // exist here. If you do not wish to have this automatic handling, use COPY. // -func add(b *Builder, args []string, attributes map[string]bool) error { +func add(b *Builder, args []string, attributes map[string]bool, original string) error { if len(args) < 2 { return fmt.Errorf("ADD requires at least two arguments") } @@ -76,7 +76,7 @@ func add(b *Builder, args []string, attributes map[string]bool) error { // // Same as 'ADD' but without the tar and remote url handling. // -func dispatchCopy(b *Builder, args []string, attributes map[string]bool) error { +func dispatchCopy(b *Builder, args []string, attributes map[string]bool, original string) error { if len(args) < 2 { return fmt.Errorf("COPY requires at least two arguments") } @@ -88,7 +88,7 @@ func dispatchCopy(b *Builder, args []string, attributes map[string]bool) error { // // This sets the image the dockerfile will build on top of. // -func from(b *Builder, args []string, attributes map[string]bool) error { +func from(b *Builder, args []string, attributes map[string]bool, original string) error { if len(args) != 1 { return fmt.Errorf("FROM requires one argument") } @@ -120,7 +120,7 @@ func from(b *Builder, args []string, attributes map[string]bool) error { // special cases. search for 'OnBuild' in internals.go for additional special // cases. // -func onbuild(b *Builder, args []string, attributes map[string]bool) error { +func onbuild(b *Builder, args []string, attributes map[string]bool, original string) error { triggerInstruction := strings.ToUpper(strings.TrimSpace(args[0])) switch triggerInstruction { case "ONBUILD": @@ -129,17 +129,17 @@ func onbuild(b *Builder, args []string, attributes map[string]bool) error { return fmt.Errorf("%s isn't allowed as an ONBUILD trigger", triggerInstruction) } - trigger := strings.Join(args, " ") + original = strings.TrimSpace(strings.TrimLeft(original, "ONBUILD")) - b.Config.OnBuild = append(b.Config.OnBuild, trigger) - return b.commit("", b.Config.Cmd, fmt.Sprintf("ONBUILD %s", trigger)) + b.Config.OnBuild = append(b.Config.OnBuild, original) + return b.commit("", b.Config.Cmd, fmt.Sprintf("ONBUILD %s", original)) } // WORKDIR /tmp // // Set the working directory for future RUN/CMD/etc statements. // -func workdir(b *Builder, args []string, attributes map[string]bool) error { +func workdir(b *Builder, args []string, attributes map[string]bool, original string) error { if len(args) != 1 { return fmt.Errorf("WORKDIR requires exactly one argument") } @@ -167,7 +167,7 @@ func workdir(b *Builder, args []string, attributes map[string]bool) error { // RUN echo hi # sh -c echo hi // RUN [ "echo", "hi" ] # echo hi // -func run(b *Builder, args []string, attributes map[string]bool) error { +func run(b *Builder, args []string, attributes map[string]bool, original string) error { if b.image == "" { return fmt.Errorf("Please provide a source image with `from` prior to run") } @@ -230,7 +230,7 @@ func run(b *Builder, args []string, attributes map[string]bool) error { // Set the default command to run in the container (which may be empty). // Argument handling is the same as RUN. // -func cmd(b *Builder, args []string, attributes map[string]bool) error { +func cmd(b *Builder, args []string, attributes map[string]bool, original string) error { b.Config.Cmd = handleJsonArgs(args, attributes) if !attributes["json"] && len(b.Config.Entrypoint) == 0 { @@ -256,7 +256,7 @@ func cmd(b *Builder, args []string, attributes map[string]bool) error { // Handles command processing similar to CMD and RUN, only b.Config.Entrypoint // is initialized at NewBuilder time instead of through argument parsing. // -func entrypoint(b *Builder, args []string, attributes map[string]bool) error { +func entrypoint(b *Builder, args []string, attributes map[string]bool, original string) error { parsed := handleJsonArgs(args, attributes) switch { @@ -289,7 +289,7 @@ func entrypoint(b *Builder, args []string, attributes map[string]bool) error { // Expose ports for links and port mappings. This all ends up in // b.Config.ExposedPorts for runconfig. // -func expose(b *Builder, args []string, attributes map[string]bool) error { +func expose(b *Builder, args []string, attributes map[string]bool, original string) error { portsTab := args if b.Config.ExposedPorts == nil { @@ -316,7 +316,7 @@ func expose(b *Builder, args []string, attributes map[string]bool) error { // Set the user to 'foo' for future commands and when running the // ENTRYPOINT/CMD at container run time. // -func user(b *Builder, args []string, attributes map[string]bool) error { +func user(b *Builder, args []string, attributes map[string]bool, original string) error { if len(args) != 1 { return fmt.Errorf("USER requires exactly one argument") } @@ -329,7 +329,7 @@ func user(b *Builder, args []string, attributes map[string]bool) error { // // Expose the volume /foo for use. Will also accept the JSON array form. // -func volume(b *Builder, args []string, attributes map[string]bool) error { +func volume(b *Builder, args []string, attributes map[string]bool, original string) error { if len(args) == 0 { return fmt.Errorf("Volume cannot be empty") } @@ -347,6 +347,6 @@ func volume(b *Builder, args []string, attributes map[string]bool) error { } // INSERT is no longer accepted, but we still parse it. -func insert(b *Builder, args []string, attributes map[string]bool) error { +func insert(b *Builder, args []string, attributes map[string]bool, original string) error { return fmt.Errorf("INSERT has been deprecated. Please use ADD instead") } diff --git a/builder/evaluator.go b/builder/evaluator.go index 4ce9b76e2b..4122616350 100644 --- a/builder/evaluator.go +++ b/builder/evaluator.go @@ -41,10 +41,10 @@ var ( ErrDockerfileEmpty = errors.New("Dockerfile cannot be empty") ) -var evaluateTable map[string]func(*Builder, []string, map[string]bool) error +var evaluateTable map[string]func(*Builder, []string, map[string]bool, string) error func init() { - evaluateTable = map[string]func(*Builder, []string, map[string]bool) error{ + evaluateTable = map[string]func(*Builder, []string, map[string]bool, string) error{ "env": env, "maintainer": maintainer, "add": add, @@ -190,6 +190,7 @@ func (b *Builder) Run(context io.Reader) (string, error) { func (b *Builder) dispatch(stepN int, ast *parser.Node) error { cmd := ast.Value attrs := ast.Attributes + original := ast.Original strs := []string{} msg := fmt.Sprintf("Step %d : %s", stepN, strings.ToUpper(cmd)) @@ -210,7 +211,7 @@ func (b *Builder) dispatch(stepN int, ast *parser.Node) error { // XXX yes, we skip any cmds that are not valid; the parser should have // picked these out already. if f, ok := evaluateTable[cmd]; ok { - return f(b, strs, attrs) + return f(b, strs, attrs, original) } fmt.Fprintf(b.ErrStream, "# Skipping unknown instruction %s\n", strings.ToUpper(cmd)) diff --git a/builder/internals.go b/builder/internals.go index 9f49ccf9ee..5fd03f7745 100644 --- a/builder/internals.go +++ b/builder/internals.go @@ -18,6 +18,7 @@ import ( "syscall" "time" + "github.com/docker/docker/builder/parser" "github.com/docker/docker/daemon" imagepkg "github.com/docker/docker/image" "github.com/docker/docker/pkg/archive" @@ -436,30 +437,26 @@ func (b *Builder) processImageFrom(img *imagepkg.Image) error { onBuildTriggers := b.Config.OnBuild b.Config.OnBuild = []string{} - // FIXME rewrite this so that builder/parser is used; right now steps in - // onbuild are muted because we have no good way to represent the step - // number + // parse the ONBUILD triggers by invoking the parser for stepN, step := range onBuildTriggers { - splitStep := strings.Split(step, " ") - stepInstruction := strings.ToUpper(strings.Trim(splitStep[0], " ")) - switch stepInstruction { - case "ONBUILD": - return fmt.Errorf("Source image contains forbidden chained `ONBUILD ONBUILD` trigger: %s", step) - case "MAINTAINER", "FROM": - return fmt.Errorf("Source image contains forbidden %s trigger: %s", stepInstruction, step) + ast, err := parser.Parse(strings.NewReader(step)) + if err != nil { + return err } - // FIXME we have to run the evaluator manually here. This does not belong - // in this function. Once removed, the init() in evaluator.go should no - // longer be necessary. + 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) + } - if f, ok := evaluateTable[strings.ToLower(stepInstruction)]; ok { fmt.Fprintf(b.OutStream, "Trigger %d, %s\n", stepN, step) - if err := f(b, splitStep[1:], nil); err != nil { + + if err := b.dispatch(i, n); err != nil { return err } - } else { - return fmt.Errorf("%s doesn't appear to be a valid Dockerfile instruction", splitStep[0]) } } diff --git a/builder/parser/parser.go b/builder/parser/parser.go index 70e916e70a..c24a925e43 100644 --- a/builder/parser/parser.go +++ b/builder/parser/parser.go @@ -26,6 +26,7 @@ type Node struct { Next *Node // the next item in the current sexp Children []*Node // the children of this sexp Attributes map[string]bool // special attributes for this node + Original string // original line used before parsing } var ( @@ -84,6 +85,7 @@ func parseLine(line string) (string, *Node, error) { if sexp.Value != "" || sexp.Next != nil || sexp.Children != nil { node.Next = sexp node.Attributes = attrs + node.Original = line } return "", node, nil diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index 1bd9fe4d1d..848dbfe4bd 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/integration-cli/docker_cli_build_test.go @@ -7,6 +7,7 @@ import ( "os" "os/exec" "path/filepath" + "regexp" "strings" "testing" "time" @@ -14,6 +15,165 @@ import ( "github.com/docker/docker/pkg/archive" ) +func TestBuildOnBuildForbiddenMaintainerInSourceImage(t *testing.T) { + name := "testbuildonbuildforbiddenmaintainerinsourceimage" + defer deleteImages(name) + createCmd := exec.Command(dockerBinary, "create", "busybox", "true") + out, _, _, err := runCommandWithStdoutStderr(createCmd) + errorOut(err, t, out) + + cleanedContainerID := stripTrailingCharacters(out) + + commitCmd := exec.Command(dockerBinary, "commit", "--run", "{\"OnBuild\":[\"MAINTAINER docker.io\"]}", cleanedContainerID, "onbuild") + + if _, err := runCommand(commitCmd); err != nil { + t.Fatal(err) + } + + _, err = buildImage(name, + `FROM onbuild`, + true) + if err != nil { + if !strings.Contains(err.Error(), "maintainer isn't allowed as an ONBUILD trigger") { + t.Fatalf("Wrong error %v, must be about MAINTAINER and ONBUILD in source image", err) + } + } else { + t.Fatal("Error must not be nil") + } + logDone("build - onbuild forbidden maintainer in source image") + +} + +func TestBuildOnBuildForbiddenFromInSourceImage(t *testing.T) { + name := "testbuildonbuildforbiddenfrominsourceimage" + defer deleteImages(name) + createCmd := exec.Command(dockerBinary, "create", "busybox", "true") + out, _, _, err := runCommandWithStdoutStderr(createCmd) + errorOut(err, t, out) + + cleanedContainerID := stripTrailingCharacters(out) + + commitCmd := exec.Command(dockerBinary, "commit", "--run", "{\"OnBuild\":[\"FROM busybox\"]}", cleanedContainerID, "onbuild") + + if _, err := runCommand(commitCmd); err != nil { + t.Fatal(err) + } + + _, err = buildImage(name, + `FROM onbuild`, + true) + if err != nil { + if !strings.Contains(err.Error(), "from isn't allowed as an ONBUILD trigger") { + t.Fatalf("Wrong error %v, must be about FROM and ONBUILD in source image", err) + } + } else { + t.Fatal("Error must not be nil") + } + logDone("build - onbuild forbidden from in source image") + +} + +func TestBuildOnBuildForbiddenChainedInSourceImage(t *testing.T) { + name := "testbuildonbuildforbiddenchainedinsourceimage" + defer deleteImages(name) + createCmd := exec.Command(dockerBinary, "create", "busybox", "true") + out, _, _, err := runCommandWithStdoutStderr(createCmd) + errorOut(err, t, out) + + cleanedContainerID := stripTrailingCharacters(out) + + commitCmd := exec.Command(dockerBinary, "commit", "--run", "{\"OnBuild\":[\"ONBUILD RUN ls\"]}", cleanedContainerID, "onbuild") + + if _, err := runCommand(commitCmd); err != nil { + t.Fatal(err) + } + + _, err = buildImage(name, + `FROM onbuild`, + true) + if err != nil { + if !strings.Contains(err.Error(), "Chaining ONBUILD via `ONBUILD ONBUILD` isn't allowed") { + t.Fatalf("Wrong error %v, must be about chaining ONBUILD in source image", err) + } + } else { + t.Fatal("Error must not be nil") + } + logDone("build - onbuild forbidden chained in source image") + +} + +func TestBuildOnBuildCmdEntrypointJSON(t *testing.T) { + name1 := "onbuildcmd" + name2 := "onbuildgenerated" + + defer deleteAllContainers() + defer deleteImages(name2) + defer deleteImages(name1) + + _, err := buildImage(name1, ` +FROM busybox +ONBUILD CMD ["hello world"] +ONBUILD ENTRYPOINT ["echo"] +ONBUILD RUN ["true"]`, + false) + + if err != nil { + t.Fatal(err) + } + + _, err = buildImage(name2, fmt.Sprintf(`FROM %s`, name1), false) + + if err != nil { + t.Fatal(err) + } + + out, _, err := runCommandWithOutput(exec.Command(dockerBinary, "run", "-t", name2)) + if err != nil { + t.Fatal(err) + } + + if !regexp.MustCompile(`(?m)^hello world`).MatchString(out) { + t.Fatal("did not get echo output from onbuild", out) + } + + logDone("build - onbuild with json entrypoint/cmd") +} + +func TestBuildOnBuildEntrypointJSON(t *testing.T) { + name1 := "onbuildcmd" + name2 := "onbuildgenerated" + + defer deleteAllContainers() + defer deleteImages(name2) + defer deleteImages(name1) + + _, err := buildImage(name1, ` +FROM busybox +ONBUILD ENTRYPOINT ["echo"]`, + false) + + if err != nil { + t.Fatal(err) + } + + _, err = buildImage(name2, fmt.Sprintf("FROM %s\nCMD [\"hello world\"]\n", name1), false) + + if err != nil { + t.Fatal(err) + } + + out, _, err := runCommandWithOutput(exec.Command(dockerBinary, "run", "-t", name2)) + if err != nil { + t.Fatal(err) + } + + if !regexp.MustCompile(`(?m)^hello world`).MatchString(out) { + t.Fatal("got malformed output from onbuild", out) + } + + logDone("build - onbuild with json entrypoint") +} + func TestBuildCacheADD(t *testing.T) { name := "testbuildtwoimageswithadd" defer deleteImages(name) @@ -2386,8 +2546,8 @@ func TestBuildOnBuildOutput(t *testing.T) { t.Fatal(err) } - if !strings.Contains(out, "Trigger 0, run echo foo") { - t.Fatal("failed to find the ONBUILD output") + if !strings.Contains(out, "Trigger 0, RUN echo foo") { + t.Fatal("failed to find the ONBUILD output", out) } logDone("build - onbuild output")