Make imagebuilder's argument handling mirror "docker build"

There were a few inconsistencies in the way imagebuilder handled arguments vs the way that docker handles them. This commit addresses those gaps. Specifically: - Subsequent ARG commands with default values override previous ones, but they don't override the values passed on the command line. - Heading args (ARG commands before the first FROM) are applied only to FROM commands unless a matching ARG command exists in a particular stage in which case the value from the heading arg carries forward into that stage. This was accomplished by creating a dedicated Builder struct field for user args and heading args so that they can be tracked separately from args declared with ARG commands in individual stages. Tracking these all separately allows us to apply these priority rules as the build progresses. Signed-off-by: Nick Carboni <[email protected]>
openshift · Mar 25, 2020 · d569093 · d569093
1 parent 5c94679
commit d569093
Show file tree

Hide file tree

Showing 6 changed files with 294 additions and 22 deletions.
diff --git a/builder.go b/builder.go
@@ -209,12 +209,8 @@ func NewStages(node *parser.Node, b *Builder) (Stages, error) {
 		stages = append(stages, Stage{
 			Position: i,
 			Name:     name,
-			Builder: &Builder{
-				Args:        b.Args,
-				AllowedArgs: b.AllowedArgs,
-				Env:         b.Env,
-			},
-			Node: root,
+			Builder:  b.builderForStage(),
+			Node:     root,
 		})
 	}
 	return stages, nil
@@ -235,17 +231,30 @@ func (b *Builder) extractHeadingArgsFromNode(node *parser.Node) error {
 		}
 	}
 
+	// Set children equal to everything except the leading ARG nodes
+	node.Children = children
+
+	// Use a separate builder to evaluate the heading args
+	tempBuilder := NewBuilder(b.UserArgs)
+
+	// Evaluate all the heading arg commands
 	for _, c := range args {
-		step := b.Step()
+		step := tempBuilder.Step()
 		if err := step.Resolve(c); err != nil {
 			return err
 		}
-		if err := b.Run(step, NoopExecutor, false); err != nil {
+		if err := tempBuilder.Run(step, NoopExecutor, false); err != nil {
 			return err
 		}
 	}
 
-	node.Children = children
+	// Add all of the defined heading args to the original builder's HeadingArgs map
+	for k, v := range tempBuilder.Args {
+		if _, ok := tempBuilder.AllowedArgs[k]; ok {
+			b.HeadingArgs[k] = v
+		}
+	}
+
 	return nil
 }
 
@@ -264,13 +273,23 @@ func extractNameFromNode(node *parser.Node) (string, bool) {
 	return n.Next.Value, true
 }
 
+func (b *Builder) builderForStage() *Builder {
+	stageBuilder := NewBuilder(b.UserArgs)
+	for k, v := range b.HeadingArgs {
+		stageBuilder.HeadingArgs[k] = v
+	}
+	return stageBuilder
+}
+
 type Builder struct {
 	RunConfig docker.Config
 
-	Env    []string
-	Args   map[string]string
-	CmdSet bool
-	Author string
+	Env         []string
+	Args        map[string]string
+	HeadingArgs map[string]string
+	UserArgs    map[string]string
+	CmdSet      bool
+	Author      string
 
 	AllowedArgs map[string]bool
 	Volumes     VolumeSet
@@ -288,12 +307,16 @@ func NewBuilder(args map[string]string) *Builder {
 	for k, v := range builtinAllowedBuildArgs {
 		allowed[k] = v
 	}
-	provided := make(map[string]string)
+	userArgs := make(map[string]string)
+	initialArgs := make(map[string]string)
 	for k, v := range args {
-		provided[k] = v
+		userArgs[k] = v
+		initialArgs[k] = v
 	}
 	return &Builder{
-		Args:        provided,
+		Args:        initialArgs,
+		UserArgs:    userArgs,
+		HeadingArgs: make(map[string]string),
 		AllowedArgs: allowed,
 	}
 }

diff --git a/builder_test.go b/builder_test.go
@@ -7,11 +7,14 @@ import (
 	"io/ioutil"
 	"os"
 	"reflect"
+	"regexp"
 	"strings"
 	"testing"
 	"time"
 
 	docker "github.com/fsouza/go-dockerclient"
+
+	"github.com/openshift/imagebuilder/dockerfile/parser"
 )
 
 func TestVolumeSet(t *testing.T) {
@@ -159,6 +162,19 @@ func TestMultiStageParseHeadingArg(t *testing.T) {
 	if len(stages) != 3 {
 		t.Fatalf("expected 3 stages, got %d", len(stages))
 	}
+
+	fromImages := []string{"golang:1.9", "busybox:latest", "golang:1.9"}
+	for i, stage := range stages {
+		from, err := stage.Builder.From(stage.Node)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if expected := fromImages[i]; from != expected {
+			t.Fatalf("expected %s, got %s", expected, from)
+		}
+	}
+
 	t.Logf("stages: %#v", stages)
 }
 
@@ -192,6 +208,216 @@ RUN echo $FOO $BAR`))
 	}
 }
 
+func resolveNodeArgs(b *Builder, node *parser.Node) error {
+	for _, c := range node.Children {
+		if c.Value != "arg" {
+			continue
+		}
+		step := b.Step()
+		if err := step.Resolve(c); err != nil {
+			return err
+		}
+		if err := b.Run(step, NoopExecutor, false); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func builderHasArgument(b *Builder, argString string) bool {
+	for _, arg := range b.Arguments() {
+		if arg == argString {
+			return true
+		}
+	}
+	return false
+}
+
+func TestMultiStageHeadingArgRedefine(t *testing.T) {
+	n, err := ParseFile("dockerclient/testdata/multistage/Dockerfile.heading-redefine")
+	if err != nil {
+		t.Fatal(err)
+	}
+	stages, err := NewStages(n, NewBuilder(map[string]string{}))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(stages) != 2 {
+		t.Fatalf("expected 2 stages, got %d", len(stages))
+	}
+
+	for _, stage := range stages {
+		if err := resolveNodeArgs(stage.Builder, stage.Node); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	firstStageHasArg := false
+	for _, arg := range stages[0].Builder.Arguments() {
+		if match, err := regexp.MatchString(`FOO=.*`, arg); err == nil && match {
+			firstStageHasArg = true
+			break
+		} else if err != nil {
+			t.Fatal(err)
+		}
+	}
+	if firstStageHasArg {
+		t.Fatalf("expected FOO to not be present in first stage")
+	}
+
+	if !builderHasArgument(stages[1].Builder, "FOO=latest") {
+		t.Fatalf("expected FOO=latest in second stage arguments list, got %v", stages[1].Builder.Arguments())
+	}
+}
+
+func TestMultiStageHeadingArgRedefineOverride(t *testing.T) {
+	n, err := ParseFile("dockerclient/testdata/multistage/Dockerfile.heading-redefine")
+	if err != nil {
+		t.Fatal(err)
+	}
+	stages, err := NewStages(n, NewBuilder(map[string]string{"FOO": "7"}))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(stages) != 2 {
+		t.Fatalf("expected 2 stages, got %d", len(stages))
+	}
+
+	for _, stage := range stages {
+		if err := resolveNodeArgs(stage.Builder, stage.Node); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	firstStageHasArg := false
+	for _, arg := range stages[0].Builder.Arguments() {
+		if match, err := regexp.MatchString(`FOO=.*`, arg); err == nil && match {
+			firstStageHasArg = true
+			break
+		} else if err != nil {
+			t.Fatal(err)
+		}
+	}
+	if firstStageHasArg {
+		t.Fatalf("expected FOO to not be present in first stage")
+	}
+
+	if !builderHasArgument(stages[1].Builder, "FOO=7") {
+		t.Fatalf("expected FOO=7 in second stage arguments list, got %v", stages[1].Builder.Arguments())
+	}
+}
+
+func TestArgs(t *testing.T) {
+	for _, tc := range []struct {
+		name          string
+		dockerfile    string
+		args          map[string]string
+		expectedValue string
+	}{
+		{
+			name:          "argOverride",
+			dockerfile:    "FROM centos\nARG FOO=stuff\nARG FOO=things\n",
+			args:          map[string]string{},
+			expectedValue: "FOO=things",
+		},
+		{
+			name:          "argOverrideWithBuildArgs",
+			dockerfile:    "FROM centos\nARG FOO=stuff\nARG FOO=things\n",
+			args:          map[string]string{"FOO": "bar"},
+			expectedValue: "FOO=bar",
+		},
+		{
+			name:          "headingArgRedefine",
+			dockerfile:    "ARG FOO=stuff\nFROM centos\nARG FOO\n",
+			args:          map[string]string{},
+			expectedValue: "FOO=stuff",
+		},
+		{
+			name:          "headingArgRedefineWithBuildArgs",
+			dockerfile:    "ARG FOO=stuff\nFROM centos\nARG FOO\n",
+			args:          map[string]string{"FOO": "bar"},
+			expectedValue: "FOO=bar",
+		},
+		{
+			name:          "headingArgRedefineDefault",
+			dockerfile:    "ARG FOO=stuff\nFROM centos\nARG FOO=defaultfoovalue\n",
+			args:          map[string]string{},
+			expectedValue: "FOO=defaultfoovalue",
+		},
+		{
+			name:          "headingArgRedefineDefaultWithBuildArgs",
+			dockerfile:    "ARG FOO=stuff\nFROM centos\nARG FOO=defaultfoovalue\n",
+			args:          map[string]string{"FOO": "bar"},
+			expectedValue: "FOO=bar",
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			node, err := ParseDockerfile(strings.NewReader(tc.dockerfile))
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			b := NewBuilder(tc.args)
+			if err := resolveNodeArgs(b, node); err != nil {
+				t.Fatal(err)
+			}
+
+			if !builderHasArgument(b, tc.expectedValue) {
+				t.Fatalf("expected %s to be contained in arguments list: %v", tc.expectedValue, b.Arguments())
+			}
+		})
+	}
+}
+
+func TestMultiStageArgScope(t *testing.T) {
+	n, err := ParseFile("dockerclient/testdata/multistage/Dockerfile.arg-scope")
+	if err != nil {
+		t.Fatal(err)
+	}
+	args := map[string]string{
+		"SECRET": "secretthings",
+		"BAR":    "notsecretthings",
+	}
+	stages, err := NewStages(n, NewBuilder(args))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(stages) != 2 {
+		t.Fatalf("expected 2 stages, got %d", len(stages))
+	}
+
+	for _, stage := range stages {
+		if err := resolveNodeArgs(stage.Builder, stage.Node); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	if !builderHasArgument(stages[0].Builder, "SECRET=secretthings") {
+		t.Fatalf("expected SECRET=secretthings to be contained in first stage arguments list: %v", stages[0].Builder.Arguments())
+	}
+
+	secondStageArguments := stages[1].Builder.Arguments()
+	secretInSecondStage := false
+	for _, arg := range secondStageArguments {
+		if match, err := regexp.MatchString(`SECRET=.*`, arg); err == nil && match {
+			secretInSecondStage = true
+			break
+		} else if err != nil {
+			t.Fatal(err)
+		}
+	}
+	if secretInSecondStage {
+		t.Fatalf("expected SECRET to not be present in second stage")
+	}
+
+	if !builderHasArgument(stages[1].Builder, "FOO=test") {
+		t.Fatalf("expected FOO=test to be present in second stage arguments list: %v", secondStageArguments)
+	}
+	if !builderHasArgument(stages[1].Builder, "BAR=notsecretthings") {
+		t.Fatalf("expected BAR=notsecretthings to be present in second stage arguments list: %v", secondStageArguments)
+	}
+}
+
 func TestRun(t *testing.T) {
 	f, err := os.Open("dockerclient/testdata/Dockerfile.add")
 	if err != nil {

diff --git a/dispatchers.go b/dispatchers.go
@@ -216,7 +216,7 @@ func from(b *Builder, args []string, attributes map[string]bool, flagArgs []stri
 
 	// Support ARG before from
 	argStrs := []string{}
-	for n, v := range b.Args {
+	for n, v := range b.HeadingArgs {
 		argStrs = append(argStrs, n+"="+v)
 	}
 	var err error
@@ -598,10 +598,16 @@ func arg(b *Builder, args []string, attributes map[string]bool, flagArgs []strin
 	// add the arg to allowed list of build-time args from this step on.
 	b.AllowedArgs[name] = true
 
+	// If there is still no default value, a value can be assigned from the heading args
+	if val, ok := b.HeadingArgs[name]; ok && !hasDefault {
+		b.Args[name] = val
+	}
+
 	// If there is a default value associated with this arg then add it to the
-	// b.buildArgs if one is not already passed to the builder. The args passed
-	// to builder override the default value of 'arg'.
-	if _, ok := b.Args[name]; !ok && hasDefault {
+	// b.buildArgs, later default values for the same arg override earlier ones.
+	// The args passed to builder (UserArgs) override the default value of 'arg'
+	// Don't add them here as they were already set in NewBuilder.
+	if _, ok := b.UserArgs[name]; !ok && hasDefault {
 		b.Args[name] = value
 	}
 

diff --git a/dockerclient/testdata/multistage/Dockerfile.arg-scope b/dockerclient/testdata/multistage/Dockerfile.arg-scope
@@ -0,0 +1,9 @@
+FROM alpine
+ARG SECRET
+RUN echo "$SECRET"
+
+FROM alpine
+ARG FOO=test
+ARG BAR=bartest
+RUN echo "$FOO:$BAR"
+RUN echo "$SECRET"