diff --git a/api/server/router/build/build_routes.go b/api/server/router/build/build_routes.go index e41c0a81ef..75425b19fb 100644 --- a/api/server/router/build/build_routes.go +++ b/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 != "" { diff --git a/api/types/client.go b/api/types/client.go index 850e19e6e3..d58a1a10ed 100644 --- a/api/types/client.go +++ b/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 diff --git a/builder/dockerfile/builder.go b/builder/dockerfile/builder.go index d13fe184f5..da43513fff 100644 --- a/builder/dockerfile/builder.go +++ b/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{ diff --git a/builder/dockerfile/dispatchers.go b/builder/dockerfile/dispatchers.go index cf69fa496a..4f3dcd8cd3 100644 --- a/builder/dockerfile/dispatchers.go +++ b/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)) diff --git a/builder/dockerfile/dispatchers_test.go b/builder/dockerfile/dispatchers_test.go index a57f78a0e1..f7c57f7e3b 100644 --- a/builder/dockerfile/dispatchers_test.go +++ b/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) } } diff --git a/builder/dockerfile/evaluator.go b/builder/dockerfile/evaluator.go index c3843936f2..f5997c91a6 100644 --- a/builder/dockerfile/evaluator.go +++ b/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 diff --git a/cli/command/image/build.go b/cli/command/image/build.go index 78cc41494f..1699b2c45c 100644 --- a/cli/command/image/build.go +++ b/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, diff --git a/client/image_build_test.go b/client/image_build_test.go index ec0cbe2ee4..b9d04f817a 100644 --- a/client/image_build_test.go +++ b/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{}, diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index e3524c7bd9..c05e545b01 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/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" diff --git a/runconfig/opts/opts.go b/runconfig/opts/opts.go index 2abce5744b..cae0432f05 100644 --- a/runconfig/opts/opts.go +++ b/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 = where 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) { diff --git a/runconfig/opts/opts_test.go b/runconfig/opts/opts_test.go index 7bf9d2672a..43f8730fc4 100644 --- a/runconfig/opts/opts_test.go +++ b/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`, diff --git a/runconfig/opts/parse.go b/runconfig/opts/parse.go index c3aa6e98b1..71a89277ec 100644 --- a/runconfig/opts/parse.go +++ b/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 {