Refactor BuildArgs

Add MetaArgs for ARG that occur before the first FROM
Integration test for these cases.

Signed-off-by: Daniel Nephin <dnephin@docker.com>

builder/dockerfile/buildargs.go

@@ -0,0 +1,124 @@
+package dockerfile
+
+// builtinAllowedBuildArgs is list of built-in allowed build args
+// these args are considered transparent and are excluded from the image history.
+// Filtering from history is implemented in dispatchers.go
+var builtinAllowedBuildArgs = map[string]bool{
+	"HTTP_PROXY":  true,
+	"http_proxy":  true,
+	"HTTPS_PROXY": true,
+	"https_proxy": true,
+	"FTP_PROXY":   true,
+	"ftp_proxy":   true,
+	"NO_PROXY":    true,
+	"no_proxy":    true,
+}
+
+// buildArgs manages arguments used by the builder
+type buildArgs struct {
+	// args that are allowed for expansion/substitution and passing to commands in 'run'.
+	allowedBuildArgs map[string]*string
+	// args defined before the first `FROM` in a Dockerfile
+	allowedMetaArgs map[string]*string
+	// args referenced by the Dockerfile
+	referencedArgs map[string]struct{}
+	// args provided by the user on the command line
+	argsFromOptions map[string]*string
+}
+
+func newBuildArgs(argsFromOptions map[string]*string) *buildArgs {
+	return &buildArgs{
+		allowedBuildArgs: make(map[string]*string),
+		allowedMetaArgs:  make(map[string]*string),
+		referencedArgs:   make(map[string]struct{}),
+		argsFromOptions:  argsFromOptions,
+	}
+}
+
+// UnreferencedOptionArgs returns the list of args that were set from options but
+// were never referenced from the Dockerfile
+func (b *buildArgs) UnreferencedOptionArgs() []string {
+	leftoverArgs := []string{}
+	for arg := range b.argsFromOptions {
+		if _, ok := b.referencedArgs[arg]; !ok {
+			leftoverArgs = append(leftoverArgs, arg)
+		}
+	}
+	return leftoverArgs
+}
+
+// ResetAllowed clears the list of args that are allowed to be used by a
+// directive
+func (b *buildArgs) ResetAllowed() {
+	b.allowedBuildArgs = make(map[string]*string)
+}
+
+// AddMetaArg adds a new meta arg that can be used by FROM directives
+func (b *buildArgs) AddMetaArg(key string, value *string) {
+	b.allowedMetaArgs[key] = value
+}
+
+// AddArg adds a new arg that can be used by directives
+func (b *buildArgs) AddArg(key string, value *string) {
+	b.allowedBuildArgs[key] = value
+	b.referencedArgs[key] = struct{}{}
+}
+
+// IsUnreferencedBuiltin checks if the key is a built-in arg, or if it has been
+// referenced by the Dockerfile. Returns true if the arg is a builtin that has
+// not been referenced in the Dockerfile.
+func (b *buildArgs) IsUnreferencedBuiltin(key string) bool {
+	_, isBuiltin := builtinAllowedBuildArgs[key]
+	_, isAllowed := b.allowedBuildArgs[key]
+	return isBuiltin && !isAllowed
+}
+
+// GetAllAllowed returns a mapping with all the allowed args
+func (b *buildArgs) GetAllAllowed() map[string]string {
+	return b.getAllFromMapping(b.allowedBuildArgs)
+}
+
+// GetAllMeta returns a mapping with all the meta meta args
+func (b *buildArgs) GetAllMeta() map[string]string {
+	return b.getAllFromMapping(b.allowedMetaArgs)
+}
+
+func (b *buildArgs) getAllFromMapping(source map[string]*string) map[string]string {
+	m := make(map[string]string)
+
+	keys := keysFromMaps(source, builtinAllowedBuildArgs)
+	for _, key := range keys {
+		v, ok := b.getBuildArg(key, source)
+		if ok {
+			m[key] = v
+		}
+	}
+	return m
+}
+
+func (b *buildArgs) getBuildArg(key string, mapping map[string]*string) (string, bool) {
+	defaultValue, exists := mapping[key]
+	// Return override from options if one is defined
+	if v, ok := b.argsFromOptions[key]; ok && v != nil {
+		return *v, ok
+	}
+
+	if defaultValue == nil {
+		if v, ok := b.allowedMetaArgs[key]; ok && v != nil {
+			return *v, ok
+		}
+		return "", false
+	}
+	return *defaultValue, exists
+}
+
+func keysFromMaps(source map[string]*string, builtin map[string]bool) []string {
+	keys := []string{}
+	for key := range source {
+		keys = append(keys, key)
+	}
+	for key := range builtin {
+		keys = append(keys, key)
+	}
+	return keys
+}

builder/dockerfile/buildargs_test.go

@@ -0,0 +1,63 @@
+package dockerfile
+
+import (
+	"github.com/docker/docker/pkg/testutil/assert"
+	"testing"
+)
+
+func strPtr(source string) *string {
+	return &source
+}
+
+func TestGetAllAllowed(t *testing.T) {
+	buildArgs := newBuildArgs(map[string]*string{
+		"ArgNotUsedInDockerfile":              strPtr("fromopt1"),
+		"ArgOverriddenByOptions":              strPtr("fromopt2"),
+		"ArgNoDefaultInDockerfileFromOptions": strPtr("fromopt3"),
+		"HTTP_PROXY":                          strPtr("theproxy"),
+	})
+
+	buildArgs.AddMetaArg("ArgFromMeta", strPtr("frommeta1"))
+	buildArgs.AddMetaArg("ArgFromMetaOverriden", strPtr("frommeta2"))
+	buildArgs.AddMetaArg("ArgFromMetaNotUsed", strPtr("frommeta3"))
+
+	buildArgs.AddArg("ArgOverriddenByOptions", strPtr("fromdockerfile2"))
+	buildArgs.AddArg("ArgWithDefaultInDockerfile", strPtr("fromdockerfile1"))
+	buildArgs.AddArg("ArgNoDefaultInDockerfile", nil)
+	buildArgs.AddArg("ArgNoDefaultInDockerfileFromOptions", nil)
+	buildArgs.AddArg("ArgFromMeta", nil)
+	buildArgs.AddArg("ArgFromMetaOverriden", strPtr("fromdockerfile3"))
+
+	all := buildArgs.GetAllAllowed()
+	expected := map[string]string{
+		"HTTP_PROXY":                          "theproxy",
+		"ArgOverriddenByOptions":              "fromopt2",
+		"ArgWithDefaultInDockerfile":          "fromdockerfile1",
+		"ArgNoDefaultInDockerfileFromOptions": "fromopt3",
+		"ArgFromMeta":                         "frommeta1",
+		"ArgFromMetaOverriden":                "fromdockerfile3",
+	}
+	assert.DeepEqual(t, all, expected)
+}
+
+func TestGetAllMeta(t *testing.T) {
+	buildArgs := newBuildArgs(map[string]*string{
+		"ArgNotUsedInDockerfile":        strPtr("fromopt1"),
+		"ArgOverriddenByOptions":        strPtr("fromopt2"),
+		"ArgNoDefaultInMetaFromOptions": strPtr("fromopt3"),
+		"HTTP_PROXY":                    strPtr("theproxy"),
+	})
+
+	buildArgs.AddMetaArg("ArgFromMeta", strPtr("frommeta1"))
+	buildArgs.AddMetaArg("ArgOverriddenByOptions", strPtr("frommeta2"))
+	buildArgs.AddMetaArg("ArgNoDefaultInMetaFromOptions", nil)
+
+	all := buildArgs.GetAllMeta()
+	expected := map[string]string{
+		"HTTP_PROXY":                    "theproxy",
+		"ArgFromMeta":                   "frommeta1",
+		"ArgOverriddenByOptions":        "fromopt2",
+		"ArgNoDefaultInMetaFromOptions": "fromopt3",
+	}
+	assert.DeepEqual(t, all, expected)
+}

builder/dockerfile/builder.go

@@ -36,20 +36,6 @@ var validCommitCommands = map[string]bool{
 	"workdir":     true,
 }
 
-// BuiltinAllowedBuildArgs is list of built-in allowed build args
-// these args are considered transparent and are excluded from the image history.
-// Filtering from history is implemented in dispatchers.go
-var BuiltinAllowedBuildArgs = map[string]bool{
-	"HTTP_PROXY":  true,
-	"http_proxy":  true,
-	"HTTPS_PROXY": true,
-	"https_proxy": true,
-	"FTP_PROXY":   true,
-	"ftp_proxy":   true,
-	"NO_PROXY":    true,
-	"no_proxy":    true,
-}
-
 var defaultLogConfig = container.LogConfig{Type: "none"}
 
 // Builder is a Dockerfile builder
@@ -66,20 +52,19 @@ type Builder struct {
 	clientCtx context.Context
 	cancel    context.CancelFunc
 
-	dockerfile       *parser.Node
-	runConfig        *container.Config // runconfig for cmd, run, entrypoint etc.
-	flags            *BFlags
-	tmpContainers    map[string]struct{}
-	image            string         // imageID
-	imageContexts    *imageContexts // helper for storing contexts from builds
-	noBaseImage      bool           // A flag to track the use of `scratch` as the base image
-	maintainer       string
-	cmdSet           bool
-	disableCommit    bool
-	cacheBusted      bool
-	allowedBuildArgs map[string]*string  // list of build-time args that are allowed for expansion/substitution and passing to commands in 'run'.
-	allBuildArgs     map[string]struct{} // list of all build-time args found during parsing of the Dockerfile
-	directive        parser.Directive
+	dockerfile    *parser.Node
+	runConfig     *container.Config // runconfig for cmd, run, entrypoint etc.
+	flags         *BFlags
+	tmpContainers map[string]struct{}
+	image         string         // imageID
+	imageContexts *imageContexts // helper for storing contexts from builds
+	noBaseImage   bool           // A flag to track the use of `scratch` as the base image
+	maintainer    string
+	cmdSet        bool
+	disableCommit bool
+	cacheBusted   bool
+	buildArgs     *buildArgs
+	directive     parser.Directive
 
 	// TODO: remove once docker.Commit can receive a tag
 	id string
@@ -134,18 +119,17 @@ func NewBuilder(clientCtx context.Context, config *types.ImageBuildOptions, back
 	}
 	ctx, cancel := context.WithCancel(clientCtx)
 	b = &Builder{
-		clientCtx:        ctx,
-		cancel:           cancel,
-		options:          config,
-		Stdout:           os.Stdout,
-		Stderr:           os.Stderr,
-		docker:           backend,
-		context:          buildContext,
-		runConfig:        new(container.Config),
-		tmpContainers:    map[string]struct{}{},
-		id:               stringid.GenerateNonCryptoID(),
-		allowedBuildArgs: make(map[string]*string),
-		allBuildArgs:     make(map[string]struct{}),
+		clientCtx:     ctx,
+		cancel:        cancel,
+		options:       config,
+		Stdout:        os.Stdout,
+		Stderr:        os.Stderr,
+		docker:        backend,
+		context:       buildContext,
+		runConfig:     new(container.Config),
+		tmpContainers: map[string]struct{}{},
+		id:            stringid.GenerateNonCryptoID(),
+		buildArgs:     newBuildArgs(config.BuildArgs),
 		directive: parser.Directive{
 			EscapeSeen:           false,
 			LookingForDirectives: true,
@@ -316,13 +300,7 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
 // check if there are any leftover build-args that were passed but not
 // consumed during build. Print a warning, if there are any.
 func (b *Builder) warnOnUnusedBuildArgs() {
-	leftoverArgs := []string{}
-	for arg := range b.options.BuildArgs {
-		if _, ok := b.allBuildArgs[arg]; !ok {
-			leftoverArgs = append(leftoverArgs, arg)
-		}
-	}
-
+	leftoverArgs := b.buildArgs.UnreferencedOptionArgs()
 	if len(leftoverArgs) > 0 {
 		fmt.Fprintf(b.Stderr, "[Warning] One or more build-args %v were not consumed\n", leftoverArgs)
 	}

builder/dockerfile/dispatchers.go

@@ -218,7 +218,11 @@ func from(b *Builder, args []string, attributes map[string]bool, original string
 		return err
 	}
 
-	substituionArgs := b.buildArgsWithoutConfigEnv()
+	substituionArgs := []string{}
+	for key, value := range b.buildArgs.GetAllMeta() {
+		substituionArgs = append(substituionArgs, key+"="+value)
+	}
+
 	name, err := ProcessWord(args[0], substituionArgs, b.directive.EscapeToken)
 	if err != nil {
 		return err
@@ -256,8 +260,7 @@ func from(b *Builder, args []string, attributes map[string]bool, original string
 	}
 	b.from = image
 
-	b.allowedBuildArgs = make(map[string]*string)
-
+	b.buildArgs.ResetAllowed()
 	return b.processImageFrom(image)
 }
 
@@ -442,7 +445,7 @@ func run(b *Builder, args []string, attributes map[string]bool, original string)
 	// properly match it.
 	b.runConfig.Env = env
 
-	// remove BuiltinAllowedBuildArgs (see: builder.go)  from the saveCmd
+	// remove builtinAllowedBuildArgs (see: builder.go)  from the saveCmd
 	// these args are transparent so resulting image should be the same regardless of the value
 	if len(cmdBuildEnv) > 0 {
 		saveCmd = config.Cmd
@@ -450,11 +453,8 @@ func run(b *Builder, args []string, attributes map[string]bool, original string)
 		copy(tmpBuildEnv, cmdBuildEnv)
 		for i, env := range tmpBuildEnv {
 			key := strings.SplitN(env, "=", 2)[0]
-			if _, ok := BuiltinAllowedBuildArgs[key]; ok {
-				// If an built-in arg is explicitly added in the Dockerfile, don't prune it
-				if _, ok := b.allowedBuildArgs[key]; !ok {
-					tmpBuildEnv = append(tmpBuildEnv[:i], tmpBuildEnv[i+1:]...)
-				}
+			if b.buildArgs.IsUnreferencedBuiltin(key) {
+				tmpBuildEnv = append(tmpBuildEnv[:i], tmpBuildEnv[i+1:]...)
 			}
 		}
 		sort.Strings(tmpBuildEnv)
@@ -785,17 +785,16 @@ func arg(b *Builder, args []string, attributes map[string]bool, original string)
 		name = arg
 		hasDefault = false
 	}
-	// add the arg to allowed list of build-time args from this step on.
-	b.allBuildArgs[name] = struct{}{}
 
 	var value *string
 	if hasDefault {
 		value = &newValue
 	}
-	b.allowedBuildArgs[name] = value
+	b.buildArgs.AddArg(name, value)
 
 	// Arg before FROM doesn't add a layer
 	if !b.hasFromImage() {
+		b.buildArgs.AddMetaArg(name, value)
 		return nil
 	}
 	return b.commit("", b.runConfig.Cmd, fmt.Sprintf("ARG %s", arg))

builder/dockerfile/dispatchers_test.go

@@ -193,12 +193,11 @@ func TestLabel(t *testing.T) {
 
 func newBuilderWithMockBackend() *Builder {
 	b := &Builder{
-		flags:            &BFlags{},
-		runConfig:        &container.Config{},
-		options:          &types.ImageBuildOptions{},
-		docker:           &MockBackend{},
-		allowedBuildArgs: make(map[string]*string),
-		allBuildArgs:     make(map[string]struct{}),
+		flags:     &BFlags{},
+		runConfig: &container.Config{},
+		options:   &types.ImageBuildOptions{},
+		docker:    &MockBackend{},
+		buildArgs: newBuildArgs(make(map[string]*string)),
 	}
 	b.imageContexts = &imageContexts{b: b}
 	return b
@@ -235,8 +234,8 @@ func TestFromWithArg(t *testing.T) {
 	assert.NilError(t, err)
 	assert.Equal(t, b.image, expected)
 	assert.Equal(t, b.from.ImageID(), expected)
-	assert.NotNil(t, b.allowedBuildArgs)
-	assert.Equal(t, len(b.allowedBuildArgs), 0)
+	assert.Equal(t, len(b.buildArgs.GetAllAllowed()), 0)
+	assert.Equal(t, len(b.buildArgs.GetAllMeta()), 1)
 }
 
 func TestFromWithUndefinedArg(t *testing.T) {
@@ -496,29 +495,18 @@ func TestStopSignal(t *testing.T) {
 }
 
 func TestArg(t *testing.T) {
-	// This is a bad test that tests implementation details and not at
-	// any features of the builder. Replace or remove.
-	buildOptions := &types.ImageBuildOptions{BuildArgs: make(map[string]*string)}
-
-	b := &Builder{flags: &BFlags{}, runConfig: &container.Config{}, disableCommit: true, allowedBuildArgs: make(map[string]*string), allBuildArgs: make(map[string]struct{}), options: buildOptions}
+	b := newBuilderWithMockBackend()
 
 	argName := "foo"
 	argVal := "bar"
 	argDef := fmt.Sprintf("%s=%s", argName, argVal)
 
-	if err := arg(b, []string{argDef}, nil, ""); err != nil {
-		t.Fatalf("Error should be empty, got: %s", err.Error())
-	}
+	err := arg(b, []string{argDef}, nil, "")
+	assert.NilError(t, err)
 
-	value, ok := b.getBuildArg(argName)
-
-	if !ok {
-		t.Fatalf("%s argument should be a build arg", argName)
-	}
-
-	if value != "bar" {
-		t.Fatalf("%s argument should have default value 'bar', got %s", argName, value)
-	}
+	expected := map[string]string{argName: argVal}
+	allowed := b.buildArgs.GetAllAllowed()
+	assert.DeepEqual(t, allowed, expected)
 }
 
 func TestShell(t *testing.T) {

builder/dockerfile/evaluator.go

@@ -189,7 +189,7 @@ func (b *Builder) buildArgsWithoutConfigEnv() []string {
 	envs := []string{}
 	configEnv := runconfigopts.ConvertKVStringsToMap(b.runConfig.Env)
 
-	for key, val := range b.getBuildArgs() {
+	for key, val := range b.buildArgs.GetAllAllowed() {
 		if _, ok := configEnv[key]; !ok {
 			envs = append(envs, fmt.Sprintf("%s=%s", key, val))
 		}

builder/dockerfile/evaluator_test.go

@@ -180,9 +180,17 @@ func executeTestCase(t *testing.T, testCase dispatchTestCase) {
 	}
 
 	config := &container.Config{}
-	options := &types.ImageBuildOptions{}
+	options := &types.ImageBuildOptions{
+		BuildArgs: make(map[string]*string),
+	}
 
-	b := &Builder{runConfig: config, options: options, Stdout: ioutil.Discard, context: context}
+	b := &Builder{
+		runConfig: config,
+		options:   options,
+		Stdout:    ioutil.Discard,
+		context:   context,
+		buildArgs: newBuildArgs(options.BuildArgs),
+	}
 
 	err = b.dispatch(0, len(n.Children), n.Children[0])
 

builder/dockerfile/internals.go

@@ -697,36 +697,3 @@ func (b *Builder) parseDockerfile() error {
 
 	return nil
 }
-
-func (b *Builder) getBuildArg(arg string) (string, bool) {
-	defaultValue, defined := b.allowedBuildArgs[arg]
-	_, builtin := BuiltinAllowedBuildArgs[arg]
-	if defined || builtin {
-		if v, ok := b.options.BuildArgs[arg]; ok && v != nil {
-			return *v, ok
-		}
-	}
-	if defaultValue == nil {
-		return "", false
-	}
-	return *defaultValue, defined
-}
-
-func (b *Builder) getBuildArgs() map[string]string {
-	m := make(map[string]string)
-	for arg := range b.options.BuildArgs {
-		v, ok := b.getBuildArg(arg)
-		if ok {
-			m[arg] = v
-		}
-	}
-	for arg := range b.allowedBuildArgs {
-		if _, ok := m[arg]; !ok {
-			v, ok := b.getBuildArg(arg)
-			if ok {
-				m[arg] = v
-			}
-		}
-	}
-	return m
-}

integration-cli/docker_cli_build_test.go

@@ -4728,7 +4728,7 @@ func (s *DockerSuite) TestBuildBuildTimeArgDefintionWithNoEnvInjection(c *check.
 		ARG %s
 		RUN env`, envKey)
 
-	result := buildImage(imgName, build.WithDockerfile(dockerfile))
+	result := cli.BuildCmd(c, imgName, build.WithDockerfile(dockerfile))
 	result.Assert(c, icmd.Success)
 	if strings.Count(result.Combined(), envKey) != 1 {
 		c.Fatalf("unexpected number of occurrences of the arg in output: %q expected: 1", result.Combined())
@@ -4745,29 +4745,45 @@ func (s *DockerSuite) TestBuildBuildTimeArgMultipleFrom(c *check.C) {
     ARG bar=def
     RUN env > /out`
 
-	result := buildImage(imgName, build.WithDockerfile(dockerfile))
+	result := cli.BuildCmd(c, imgName, build.WithDockerfile(dockerfile))
 	result.Assert(c, icmd.Success)
 
-	result = icmd.RunCmd(icmd.Cmd{
-		Command: []string{dockerBinary, "images", "-q", "-f", "label=multifromtest=1"},
-	})
-	result.Assert(c, icmd.Success)
+	result = cli.DockerCmd(c, "images", "-q", "-f", "label=multifromtest=1")
 	parentID := strings.TrimSpace(result.Stdout())
 
-	result = icmd.RunCmd(icmd.Cmd{
-		Command: []string{dockerBinary, "run", "--rm", parentID, "cat", "/out"},
-	})
-	result.Assert(c, icmd.Success)
+	result = cli.DockerCmd(c, "run", "--rm", parentID, "cat", "/out")
 	c.Assert(result.Stdout(), checker.Contains, "foo=abc")
 
-	result = icmd.RunCmd(icmd.Cmd{
-		Command: []string{dockerBinary, "run", "--rm", imgName, "cat", "/out"},
-	})
-	result.Assert(c, icmd.Success)
+	result = cli.DockerCmd(c, "run", "--rm", imgName, "cat", "/out")
 	c.Assert(result.Stdout(), checker.Not(checker.Contains), "foo")
 	c.Assert(result.Stdout(), checker.Contains, "bar=def")
 }
 
+func (s *DockerSuite) TestBuildBuildTimeFromArgMultipleFrom(c *check.C) {
+	imgName := "multifrombldargtest"
+	dockerfile := `ARG tag=nosuchtag
+     FROM busybox:${tag}
+     LABEL multifromtest=1
+     RUN env > /out
+     FROM busybox:${tag}
+     ARG tag
+     RUN env > /out`
+
+	result := cli.BuildCmd(c, imgName,
+		build.WithDockerfile(dockerfile),
+		cli.WithFlags("--build-arg", fmt.Sprintf("tag=latest")))
+	result.Assert(c, icmd.Success)
+
+	result = cli.DockerCmd(c, "images", "-q", "-f", "label=multifromtest=1")
+	parentID := strings.TrimSpace(result.Stdout())
+
+	result = cli.DockerCmd(c, "run", "--rm", parentID, "cat", "/out")
+	c.Assert(result.Stdout(), checker.Not(checker.Contains), "tag")
+
+	result = cli.DockerCmd(c, "run", "--rm", imgName, "cat", "/out")
+	c.Assert(result.Stdout(), checker.Contains, "tag=latest")
+}
+
 func (s *DockerSuite) TestBuildBuildTimeUnusedArgMultipleFrom(c *check.C) {
 	imgName := "multifromunusedarg"
 	dockerfile := `FROM busybox
@@ -4776,16 +4792,14 @@ func (s *DockerSuite) TestBuildBuildTimeUnusedArgMultipleFrom(c *check.C) {
     ARG bar
     RUN env > /out`
 
-	result := buildImage(imgName, build.WithDockerfile(dockerfile), cli.WithFlags(
-		"--build-arg", fmt.Sprintf("baz=abc")))
+	result := cli.BuildCmd(c, imgName,
+		build.WithDockerfile(dockerfile),
+		cli.WithFlags("--build-arg", fmt.Sprintf("baz=abc")))
 	result.Assert(c, icmd.Success)
 	c.Assert(result.Combined(), checker.Contains, "[Warning]")
 	c.Assert(result.Combined(), checker.Contains, "[baz] were not consumed")
 
-	result = icmd.RunCmd(icmd.Cmd{
-		Command: []string{dockerBinary, "run", "--rm", imgName, "cat", "/out"},
-	})
-	result.Assert(c, icmd.Success)
+	result = cli.DockerCmd(c, "run", "--rm", imgName, "cat", "/out")
 	c.Assert(result.Stdout(), checker.Not(checker.Contains), "bar")
 	c.Assert(result.Stdout(), checker.Not(checker.Contains), "baz")
 }