From cdb8ea90b04683adb25c8ccd71b6eaedc44b51e2 Mon Sep 17 00:00:00 2001 From: Doug Davis Date: Sat, 3 Dec 2016 05:46:04 -0800 Subject: [PATCH] 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 --- api/server/router/build/build_routes.go | 16 +++++- api/types/client.go | 11 ++-- builder/dockerfile/builder.go | 2 +- builder/dockerfile/dispatchers.go | 17 ++++--- builder/dockerfile/dispatchers_test.go | 4 +- builder/dockerfile/evaluator.go | 2 +- cli/command/image/build.go | 4 +- client/image_build_test.go | 11 ++-- integration-cli/docker_cli_build_test.go | 65 ++++++++++++++++++++++++ runconfig/opts/opts.go | 15 ------ runconfig/opts/opts_test.go | 36 ------------- runconfig/opts/parse.go | 19 +++++++ 12 files changed, 129 insertions(+), 73 deletions(-) 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 {