Merge pull request #29104 from duglin/Issue29084

Fix processing of unset build-args during build

api/server/router/build/build_routes.go

@@ -84,14 +84,28 @@ func newImageBuildOptions(ctx context.Context, r *http.Request) (*types.ImageBui
 		options.Ulimits = buildUlimits
 	}
 
-	var buildArgs = map[string]string{}
+	var buildArgs = map[string]*string{}
 	buildArgsJSON := r.FormValue("buildargs")
+
+	// Note that there are two ways a --build-arg might appear in the
+	// json of the query param:
+	//     "foo":"bar"
+	// and "foo":nil
+	// The first is the normal case, ie. --build-arg foo=bar
+	// or  --build-arg foo
+	// where foo's value was picked up from an env var.
+	// The second ("foo":nil) is where they put --build-arg foo
+	// but "foo" isn't set as an env var. In that case we can't just drop
+	// the fact they mentioned it, we need to pass that along to the builder
+	// so that it can print a warning about "foo" being unused if there is
+	// no "ARG foo" in the Dockerfile.
 	if buildArgsJSON != "" {
 		if err := json.Unmarshal([]byte(buildArgsJSON), &buildArgs); err != nil {
 			return nil, err
 		}
 		options.BuildArgs = buildArgs
 	}
+
 	var labels = map[string]string{}
 	labelsJSON := r.FormValue("labels")
 	if labelsJSON != "" {

api/types/client.go

@@ -160,10 +160,13 @@ type ImageBuildOptions struct {
 	ShmSize        int64
 	Dockerfile     string
 	Ulimits        []*units.Ulimit
-	BuildArgs      map[string]string
-	AuthConfigs    map[string]AuthConfig
-	Context        io.Reader
-	Labels         map[string]string
+	// See the parsing of buildArgs in api/server/router/build/build_routes.go
+	// for an explaination of why BuildArgs needs to use *string instead of
+	// just a string
+	BuildArgs   map[string]*string
+	AuthConfigs map[string]AuthConfig
+	Context     io.Reader
+	Labels      map[string]string
 	// squash the resulting image's layers to the parent
 	// preserves the original image and creates a new one from the parent with all
 	// the changes applied to a single layer

builder/dockerfile/builder.go

@@ -125,7 +125,7 @@ func NewBuilder(clientCtx context.Context, config *types.ImageBuildOptions, back
 		config = new(types.ImageBuildOptions)
 	}
 	if config.BuildArgs == nil {
-		config.BuildArgs = make(map[string]string)
+		config.BuildArgs = make(map[string]*string)
 	}
 	ctx, cancel := context.WithCancel(clientCtx)
 	b = &Builder{

builder/dockerfile/dispatchers.go

@@ -384,8 +384,8 @@ func run(b *Builder, args []string, attributes map[string]bool, original string)
 			// the entire file (see 'leftoverArgs' processing in evaluator.go )
 			continue
 		}
-		if _, ok := configEnv[key]; !ok {
-			cmdBuildEnv = append(cmdBuildEnv, fmt.Sprintf("%s=%s", key, val))
+		if _, ok := configEnv[key]; !ok && val != nil {
+			cmdBuildEnv = append(cmdBuildEnv, fmt.Sprintf("%s=%s", key, *val))
 		}
 	}
 
@@ -728,7 +728,7 @@ func arg(b *Builder, args []string, attributes map[string]bool, original string)
 
 	var (
 		name       string
-		value      string
+		newValue   string
 		hasDefault bool
 	)
 
@@ -745,7 +745,7 @@ func arg(b *Builder, args []string, attributes map[string]bool, original string)
 		}
 
 		name = parts[0]
-		value = parts[1]
+		newValue = parts[1]
 		hasDefault = true
 	} else {
 		name = arg
@@ -756,9 +756,12 @@ func arg(b *Builder, args []string, attributes map[string]bool, original string)
 
 	// 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.options.BuildArgs[name]; !ok && hasDefault {
-		b.options.BuildArgs[name] = value
+	// to builder override the default value of 'arg'. Note that a 'nil' for
+	// a value means that the user specified "--build-arg FOO" and "FOO" wasn't
+	// defined as an env var - and in that case we DO want to use the default
+	// value specified in the ARG cmd.
+	if baValue, ok := b.options.BuildArgs[name]; (!ok || baValue == nil) && hasDefault {
+		b.options.BuildArgs[name] = &newValue
 	}
 
 	return b.commit("", b.runConfig.Cmd, fmt.Sprintf("ARG %s", arg))

builder/dockerfile/dispatchers_test.go

@@ -460,7 +460,7 @@ func TestStopSignal(t *testing.T) {
 }
 
 func TestArg(t *testing.T) {
-	buildOptions := &types.ImageBuildOptions{BuildArgs: make(map[string]string)}
+	buildOptions := &types.ImageBuildOptions{BuildArgs: make(map[string]*string)}
 
 	b := &Builder{flags: &BFlags{}, runConfig: &container.Config{}, disableCommit: true, allowedBuildArgs: make(map[string]bool), options: buildOptions}
 
@@ -488,7 +488,7 @@ func TestArg(t *testing.T) {
 		t.Fatalf("%s argument should be a build arg", argName)
 	}
 
-	if val != "bar" {
+	if *val != "bar" {
 		t.Fatalf("%s argument should have default value 'bar', got %s", argName, val)
 	}
 }

builder/dockerfile/evaluator.go

@@ -158,7 +158,7 @@ func (b *Builder) dispatch(stepN int, stepTotal int, ast *parser.Node) error {
 			// the entire file (see 'leftoverArgs' processing in evaluator.go )
 			continue
 		}
-		envs = append(envs, fmt.Sprintf("%s=%s", key, val))
+		envs = append(envs, fmt.Sprintf("%s=%s", key, *val))
 	}
 	for ast.Next != nil {
 		ast = ast.Next

cli/command/image/build.go

@@ -67,7 +67,7 @@ func NewBuildCommand(dockerCli *command.DockerCli) *cobra.Command {
 	ulimits := make(map[string]*units.Ulimit)
 	options := buildOptions{
 		tags:      opts.NewListOpts(validateTag),
-		buildArgs: opts.NewListOpts(runconfigopts.ValidateArg),
+		buildArgs: opts.NewListOpts(runconfigopts.ValidateEnv),
 		ulimits:   runconfigopts.NewUlimitOpt(&ulimits),
 		labels:    opts.NewListOpts(runconfigopts.ValidateEnv),
 	}
@@ -300,7 +300,7 @@ func runBuild(dockerCli *command.DockerCli, options buildOptions) error {
 		Dockerfile:     relDockerfile,
 		ShmSize:        shmSize,
 		Ulimits:        options.ulimits.GetList(),
-		BuildArgs:      runconfigopts.ConvertKVStringsToMap(options.buildArgs.GetAll()),
+		BuildArgs:      runconfigopts.ConvertKVStringsToMapWithNil(options.buildArgs.GetAll()),
 		AuthConfigs:    authConfigs,
 		Labels:         runconfigopts.ConvertKVStringsToMap(options.labels.GetAll()),
 		CacheFrom:      options.cacheFrom,

client/image_build_test.go

@@ -27,6 +27,8 @@ func TestImageBuildError(t *testing.T) {
 }
 
 func TestImageBuild(t *testing.T) {
+	v1 := "value1"
+	v2 := "value2"
 	emptyRegistryConfig := "bnVsbA=="
 	buildCases := []struct {
 		buildOptions           types.ImageBuildOptions
@@ -105,13 +107,14 @@ func TestImageBuild(t *testing.T) {
 		},
 		{
 			buildOptions: types.ImageBuildOptions{
-				BuildArgs: map[string]string{
-					"ARG1": "value1",
-					"ARG2": "value2",
+				BuildArgs: map[string]*string{
+					"ARG1": &v1,
+					"ARG2": &v2,
+					"ARG3": nil,
 				},
 			},
 			expectedQueryParams: map[string]string{
-				"buildargs": `{"ARG1":"value1","ARG2":"value2"}`,
+				"buildargs": `{"ARG1":"value1","ARG2":"value2","ARG3":null}`,
 				"rm":        "0",
 			},
 			expectedTags:           []string{},

integration-cli/docker_cli_build_test.go

@@ -6070,6 +6070,71 @@ func (s *DockerSuite) TestBuildBuildTimeArgUnconsumedArg(c *check.C) {
 
 }
 
+func (s *DockerSuite) TestBuildBuildTimeArgEnv(c *check.C) {
+	testRequires(c, DaemonIsLinux) // Windows does not support ARG
+	args := []string{
+		"build",
+		"--build-arg", fmt.Sprintf("FOO1=fromcmd"),
+		"--build-arg", fmt.Sprintf("FOO2="),
+		"--build-arg", fmt.Sprintf("FOO3"), // set in env
+		"--build-arg", fmt.Sprintf("FOO4"), // not set in env
+		"--build-arg", fmt.Sprintf("FOO5=fromcmd"),
+		// FOO6 is not set at all
+		"--build-arg", fmt.Sprintf("FOO7=fromcmd"), // should produce a warning
+		"--build-arg", fmt.Sprintf("FOO8="), // should produce a warning
+		"--build-arg", fmt.Sprintf("FOO9"), // should produce a warning
+		".",
+	}
+
+	dockerfile := fmt.Sprintf(`FROM busybox
+		ARG FOO1=fromfile
+		ARG FOO2=fromfile
+		ARG FOO3=fromfile
+		ARG FOO4=fromfile
+		ARG FOO5
+		ARG FOO6
+		RUN env
+		RUN [ "$FOO1" == "fromcmd" ]
+		RUN [ "$FOO2" == "" ]
+		RUN [ "$FOO3" == "fromenv" ]
+		RUN [ "$FOO4" == "fromfile" ]
+		RUN [ "$FOO5" == "fromcmd" ]
+		# The following should not exist at all in the env
+		RUN [ "$(env | grep FOO6)" == "" ]
+		RUN [ "$(env | grep FOO7)" == "" ]
+		RUN [ "$(env | grep FOO8)" == "" ]
+		RUN [ "$(env | grep FOO9)" == "" ]
+	    `)
+
+	ctx, err := fakeContext(dockerfile, nil)
+	c.Assert(err, check.IsNil)
+	defer ctx.Close()
+
+	cmd := exec.Command(dockerBinary, args...)
+	cmd.Dir = ctx.Dir
+	cmd.Env = append(os.Environ(),
+		"FOO1=fromenv",
+		"FOO2=fromenv",
+		"FOO3=fromenv")
+	out, _, err := runCommandWithOutput(cmd)
+	if err != nil {
+		c.Fatal(err, out)
+	}
+
+	// Now check to make sure we got a warning msg about unused build-args
+	i := strings.Index(out, "[Warning]")
+	if i < 0 {
+		c.Fatalf("Missing the build-arg warning in %q", out)
+	}
+
+	out = out[i:] // "out" should contain just the warning message now
+
+	// These were specified on a --build-arg but no ARG was in the Dockerfile
+	c.Assert(out, checker.Contains, "FOO7")
+	c.Assert(out, checker.Contains, "FOO8")
+	c.Assert(out, checker.Contains, "FOO9")
+}
+
 func (s *DockerSuite) TestBuildBuildTimeArgQuotedValVariants(c *check.C) {
 	imgName := "bldargtest"
 	envKey := "foo"

runconfig/opts/opts.go

@@ -59,21 +59,6 @@ func doesEnvExist(name string) bool {
 	return false
 }
 
-// ValidateArg validates a build-arg variable and returns it.
-// Build-arg is in the form of <varname>=<value> where <varname> is required.
-func ValidateArg(val string) (string, error) {
-	arr := strings.Split(val, "=")
-	if len(arr) > 1 && isNotEmpty(arr[0]) {
-		return val, nil
-	}
-
-	return "", fmt.Errorf("bad format for build-arg: %s", val)
-}
-
-func isNotEmpty(val string) bool {
-	return len(val) > 0
-}
-
 // ValidateExtraHost validates that the specified string is a valid extrahost and returns it.
 // ExtraHost is in the form of name:ip where the ip has to be a valid ip (IPv4 or IPv6).
 func ValidateExtraHost(val string) (string, error) {

runconfig/opts/opts_test.go

@@ -66,42 +66,6 @@ func TestValidateEnv(t *testing.T) {
 	}
 }
 
-func TestValidateArg(t *testing.T) {
-	valids := map[string]string{
-		"_=a":                "_=a",
-		"var1=value1":        "var1=value1",
-		"_var1=value1":       "_var1=value1",
-		"var2=value2=value3": "var2=value2=value3",
-		"var3=abc!qwe":       "var3=abc!qwe",
-		"var_4=value 4":      "var_4=value 4",
-		"var_5=":             "var_5=",
-	}
-	for value, expected := range valids {
-		actual, err := ValidateArg(value)
-		if err != nil {
-			t.Fatal(err)
-		}
-		if actual != expected {
-			t.Fatalf("Expected [%v], got [%v]", expected, actual)
-		}
-	}
-
-	invalid := map[string]string{
-		"foo":  "bad format",
-		"=foo": "bad format",
-		"cc c": "bad format",
-	}
-	for value, expectedError := range invalid {
-		if _, err := ValidateArg(value); err == nil {
-			t.Fatalf("ValidateArg(`%s`) should have failed validation", value)
-		} else {
-			if !strings.Contains(err.Error(), expectedError) {
-				t.Fatalf("ValidateArg(`%s`) error should contain %q", value, expectedError)
-			}
-		}
-	}
-}
-
 func TestValidateExtraHosts(t *testing.T) {
 	valid := []string{
 		`myhost:192.168.0.1`,

runconfig/opts/parse.go

@@ -705,6 +705,25 @@ func ConvertKVStringsToMap(values []string) map[string]string {
 	return result
 }
 
+// ConvertKVStringsToMapWithNil converts ["key=value"] to {"key":"value"}
+// but set unset keys to nil - meaning the ones with no "=" in them.
+// We use this in cases where we need to distinguish between
+//   FOO=  and FOO
+// where the latter case just means FOO was mentioned but not given a value
+func ConvertKVStringsToMapWithNil(values []string) map[string]*string {
+	result := make(map[string]*string, len(values))
+	for _, value := range values {
+		kv := strings.SplitN(value, "=", 2)
+		if len(kv) == 1 {
+			result[kv[0]] = nil
+		} else {
+			result[kv[0]] = &kv[1]
+		}
+	}
+
+	return result
+}
+
 func parseLoggingOpts(loggingDriver string, loggingOpts []string) (map[string]string, error) {
 	loggingOptsMap := ConvertKVStringsToMap(loggingOpts)
 	if loggingDriver == "none" && len(loggingOpts) > 0 {