builder: ONBUILD triggers were not accurately being executed in JSON cases.

Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <github@hollensbe.org> (github: erikh)

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 <maybe@an.email.address>
 //
 // 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")
 }

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

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

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

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")