diff --git a/builder/dockerfile/builder.go b/builder/dockerfile/builder.go index 6cc4b139a7..aa93129c91 100644 --- a/builder/dockerfile/builder.go +++ b/builder/dockerfile/builder.go @@ -75,7 +75,8 @@ type Builder struct { cmdSet bool disableCommit bool cacheBusted bool - allowedBuildArgs map[string]bool // list of build-time args that are allowed for expansion/substitution and passing to commands in 'run'. + 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 // TODO: remove once docker.Commit can receive a tag @@ -127,9 +128,6 @@ func NewBuilder(clientCtx context.Context, config *types.ImageBuildOptions, back if config == nil { config = new(types.ImageBuildOptions) } - if config.BuildArgs == nil { - config.BuildArgs = make(map[string]*string) - } ctx, cancel := context.WithCancel(clientCtx) b = &Builder{ clientCtx: ctx, @@ -142,7 +140,8 @@ func NewBuilder(clientCtx context.Context, config *types.ImageBuildOptions, back runConfig: new(container.Config), tmpContainers: map[string]struct{}{}, id: stringid.GenerateNonCryptoID(), - allowedBuildArgs: make(map[string]bool), + allowedBuildArgs: make(map[string]*string), + allBuildArgs: make(map[string]struct{}), directive: parser.Directive{ EscapeSeen: false, LookingForDirectives: true, @@ -320,7 +319,7 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri func (b *Builder) warnOnUnusedBuildArgs() { leftoverArgs := []string{} for arg := range b.options.BuildArgs { - if !b.isBuildArgAllowed(arg) { + if _, ok := b.allBuildArgs[arg]; !ok { leftoverArgs = append(leftoverArgs, arg) } } diff --git a/builder/dockerfile/dispatchers.go b/builder/dockerfile/dispatchers.go index e10bb32594..09612e6e8d 100644 --- a/builder/dockerfile/dispatchers.go +++ b/builder/dockerfile/dispatchers.go @@ -205,6 +205,8 @@ func from(b *Builder, args []string, attributes map[string]bool, original string var image builder.Image + b.noBaseImage = false + // Windows cannot support a container with no base image. if name == api.NoBaseImageSpecifier { if runtime.GOOS == "windows" { @@ -228,6 +230,8 @@ func from(b *Builder, args []string, attributes map[string]bool, original string } b.from = image + b.allowedBuildArgs = make(map[string]*string) + return b.processImageFrom(image) } @@ -729,17 +733,13 @@ func arg(b *Builder, args []string, attributes map[string]bool, original string) hasDefault = false } // add the arg to allowed list of build-time args from this step on. - b.allowedBuildArgs[name] = true + b.allBuildArgs[name] = struct{}{} - // 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'. 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 + var value *string + if hasDefault { + value = &newValue } + b.allowedBuildArgs[name] = value 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 d81950c608..61ef3a753d 100644 --- a/builder/dockerfile/dispatchers_test.go +++ b/builder/dockerfile/dispatchers_test.go @@ -460,9 +460,11 @@ 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]bool), options: buildOptions} + b := &Builder{flags: &BFlags{}, runConfig: &container.Config{}, disableCommit: true, allowedBuildArgs: make(map[string]*string), allBuildArgs: make(map[string]struct{}), options: buildOptions} argName := "foo" argVal := "bar" @@ -472,24 +474,14 @@ func TestArg(t *testing.T) { t.Fatalf("Error should be empty, got: %s", err.Error()) } - allowed, ok := b.allowedBuildArgs[argName] - - if !ok { - t.Fatalf("%s argument should be allowed as a build arg", argName) - } - - if !allowed { - t.Fatalf("%s argument was present in map but disallowed as a build arg", argName) - } - - val, ok := b.options.BuildArgs[argName] + value, ok := b.getBuildArg(argName) if !ok { t.Fatalf("%s argument should be a build arg", argName) } - if *val != "bar" { - t.Fatalf("%s argument should have default value 'bar', got %s", argName, *val) + if value != "bar" { + t.Fatalf("%s argument should have default value 'bar', got %s", argName, value) } } diff --git a/builder/dockerfile/evaluator.go b/builder/dockerfile/evaluator.go index 2fbdbb3c44..d0f9a7ca55 100644 --- a/builder/dockerfile/evaluator.go +++ b/builder/dockerfile/evaluator.go @@ -192,17 +192,9 @@ func (b *Builder) buildArgsWithoutConfigEnv() []string { envs := []string{} configEnv := runconfigopts.ConvertKVStringsToMap(b.runConfig.Env) - for key, val := range b.options.BuildArgs { - if !b.isBuildArgAllowed(key) { - // skip build-args that are not in allowed list, meaning they have - // not been defined by an "ARG" Dockerfile command yet. - // This is an error condition but only if there is no "ARG" in the entire - // Dockerfile, so we'll generate any necessary errors after we parsed - // the entire file (see 'leftoverArgs' processing in evaluator.go ) - continue - } - if _, ok := configEnv[key]; !ok && val != nil { - envs = append(envs, fmt.Sprintf("%s=%s", key, *val)) + for key, val := range b.getBuildArgs() { + if _, ok := configEnv[key]; !ok { + envs = append(envs, fmt.Sprintf("%s=%s", key, val)) } } return envs diff --git a/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index a3ec20b438..6eb6b1dfeb 100644 --- a/builder/dockerfile/internals.go +++ b/builder/dockerfile/internals.go @@ -668,14 +668,35 @@ func (b *Builder) parseDockerfile() error { return nil } -// determine if build arg is part of built-in args or user -// defined args in Dockerfile at any point in time. -func (b *Builder) isBuildArgAllowed(arg string) bool { - if _, ok := BuiltinAllowedBuildArgs[arg]; ok { - return true +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 _, ok := b.allowedBuildArgs[arg]; ok { - return true + if defaultValue == nil { + return "", false } - 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 } diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index c4396d82c4..500727a481 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/integration-cli/docker_cli_build_test.go @@ -4740,6 +4740,61 @@ func (s *DockerSuite) TestBuildBuildTimeArgDefintionWithNoEnvInjection(c *check. } } +func (s *DockerSuite) TestBuildBuildTimeArgMultipleFrom(c *check.C) { + imgName := "multifrombldargtest" + dockerfile := `FROM busybox + ARG foo=abc + LABEL multifromtest=1 + RUN env > /out + FROM busybox + ARG bar=def + RUN env > /out` + + result := buildImage(imgName, 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) + parentID := strings.TrimSpace(result.Stdout()) + + result = icmd.RunCmd(icmd.Cmd{ + Command: []string{dockerBinary, "run", "--rm", parentID, "cat", "/out"}, + }) + result.Assert(c, icmd.Success) + 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) + c.Assert(result.Stdout(), checker.Not(checker.Contains), "foo") + c.Assert(result.Stdout(), checker.Contains, "bar=def") +} + +func (s *DockerSuite) TestBuildBuildTimeUnusedArgMultipleFrom(c *check.C) { + imgName := "multifromunusedarg" + dockerfile := `FROM busybox + ARG foo + FROM busybox + ARG bar + RUN env > /out` + + result := buildImage(imgName, withDockerfile(dockerfile), withBuildFlags( + "--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) + c.Assert(result.Stdout(), checker.Not(checker.Contains), "bar") + c.Assert(result.Stdout(), checker.Not(checker.Contains), "baz") +} + func (s *DockerSuite) TestBuildNoNamedVolume(c *check.C) { volName := "testname:/foo"