Fix processing of unset build-args during build

This reverts 26103.  26103 was trying to make it so that if someone did:
  docker build --build-arg FOO .
and FOO wasn't set as an env var then it would pick-up FOO from the
Dockerfile's ARG cmd.  However, it went too far and removed the ability
to specify a build arg w/o any value. Meaning it required the --build-arg
param to always be in the form "name=value", and not just "name".

This PR does the right fix - it allows just "name" and it'll grab the value
from the env vars if set. If "name" isn't set in the env then it still needs
to send "name" to the server so that a warning can be printed about an
unused --build-arg. And this is why buildArgs in the options is now a
*string instead of just a string - 'nil' == mentioned but no value.

Closes #29084

Signed-off-by: Doug Davis <dug@us.ibm.com>

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 {