Skip to content

Commit cdb8ea9

Browse files
author
Doug Davis
committed
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 moby#29084 Signed-off-by: Doug Davis <dug@us.ibm.com>
1 parent 3cb310c commit cdb8ea9

File tree

12 files changed

+129
-73
lines changed

12 files changed

+129
-73
lines changed

‎api/server/router/build/build_routes.go‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,14 +84,28 @@ func newImageBuildOptions(ctx context.Context, r *http.Request) (*types.ImageBui
8484
options.Ulimits = buildUlimits
8585
}
8686

87-
var buildArgs = map[string]string{}
87+
var buildArgs = map[string]*string{}
8888
buildArgsJSON := r.FormValue("buildargs")
89+
90+
// Note that there are two ways a --build-arg might appear in the
91+
// json of the query param:
92+
// "foo":"bar"
93+
// and "foo":nil
94+
// The first is the normal case, ie. --build-arg foo=bar
95+
// or --build-arg foo
96+
// where foo's value was picked up from an env var.
97+
// The second ("foo":nil) is where they put --build-arg foo
98+
// but "foo" isn't set as an env var. In that case we can't just drop
99+
// the fact they mentioned it, we need to pass that along to the builder
100+
// so that it can print a warning about "foo" being unused if there is
101+
// no "ARG foo" in the Dockerfile.
89102
if buildArgsJSON != "" {
90103
if err := json.Unmarshal([]byte(buildArgsJSON), &buildArgs); err != nil {
91104
return nil, err
92105
}
93106
options.BuildArgs = buildArgs
94107
}
108+
95109
var labels = map[string]string{}
96110
labelsJSON := r.FormValue("labels")
97111
if labelsJSON != "" {

‎api/types/client.go‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -160,10 +160,13 @@ type ImageBuildOptions struct {
160160
ShmSize int64
161161
Dockerfile string
162162
Ulimits []*units.Ulimit
163-
BuildArgs map[string]string
164-
AuthConfigs map[string]AuthConfig
165-
Context io.Reader
166-
Labels map[string]string
163+
// See the parsing of buildArgs in api/server/router/build/build_routes.go
164+
// for an explaination of why BuildArgs needs to use *string instead of
165+
// just a string
166+
BuildArgs map[string]*string
167+
AuthConfigs map[string]AuthConfig
168+
Context io.Reader
169+
Labels map[string]string
167170
// squash the resulting image's layers to the parent
168171
// preserves the original image and creates a new one from the parent with all
169172
// the changes applied to a single layer

‎builder/dockerfile/builder.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ func NewBuilder(clientCtx context.Context, config *types.ImageBuildOptions, back
125125
config = new(types.ImageBuildOptions)
126126
}
127127
if config.BuildArgs == nil {
128-
config.BuildArgs = make(map[string]string)
128+
config.BuildArgs = make(map[string]*string)
129129
}
130130
ctx, cancel := context.WithCancel(clientCtx)
131131
b = &Builder{

‎builder/dockerfile/dispatchers.go‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -384,8 +384,8 @@ func run(b *Builder, args []string, attributes map[string]bool, original string)
384384
// the entire file (see 'leftoverArgs' processing in evaluator.go )
385385
continue
386386
}
387-
if _, ok := configEnv[key]; !ok {
388-
cmdBuildEnv = append(cmdBuildEnv, fmt.Sprintf("%s=%s", key, val))
387+
if _, ok := configEnv[key]; !ok && val != nil {
388+
cmdBuildEnv = append(cmdBuildEnv, fmt.Sprintf("%s=%s", key, *val))
389389
}
390390
}
391391

@@ -728,7 +728,7 @@ func arg(b *Builder, args []string, attributes map[string]bool, original string)
728728

729729
var (
730730
name string
731-
value string
731+
newValue string
732732
hasDefault bool
733733
)
734734

@@ -745,7 +745,7 @@ func arg(b *Builder, args []string, attributes map[string]bool, original string)
745745
}
746746

747747
name = parts[0]
748-
value = parts[1]
748+
newValue = parts[1]
749749
hasDefault = true
750750
} else {
751751
name = arg
@@ -756,9 +756,12 @@ func arg(b *Builder, args []string, attributes map[string]bool, original string)
756756

757757
// If there is a default value associated with this arg then add it to the
758758
// b.buildArgs if one is not already passed to the builder. The args passed
759-
// to builder override the default value of 'arg'.
760-
if _, ok := b.options.BuildArgs[name]; !ok && hasDefault {
761-
b.options.BuildArgs[name] = value
759+
// to builder override the default value of 'arg'. Note that a 'nil' for
760+
// a value means that the user specified "--build-arg FOO" and "FOO" wasn't
761+
// defined as an env var - and in that case we DO want to use the default
762+
// value specified in the ARG cmd.
763+
if baValue, ok := b.options.BuildArgs[name]; (!ok || baValue == nil) && hasDefault {
764+
b.options.BuildArgs[name] = &newValue
762765
}
763766

764767
return b.commit("", b.runConfig.Cmd, fmt.Sprintf("ARG %s", arg))

‎builder/dockerfile/dispatchers_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -460,7 +460,7 @@ func TestStopSignal(t *testing.T) {
460460
}
461461

462462
func TestArg(t *testing.T) {
463-
buildOptions := &types.ImageBuildOptions{BuildArgs: make(map[string]string)}
463+
buildOptions := &types.ImageBuildOptions{BuildArgs: make(map[string]*string)}
464464

465465
b := &Builder{flags: &BFlags{}, runConfig: &container.Config{}, disableCommit: true, allowedBuildArgs: make(map[string]bool), options: buildOptions}
466466

@@ -488,7 +488,7 @@ func TestArg(t *testing.T) {
488488
t.Fatalf("%s argument should be a build arg", argName)
489489
}
490490

491-
if val != "bar" {
491+
if *val != "bar" {
492492
t.Fatalf("%s argument should have default value 'bar', got %s", argName, val)
493493
}
494494
}

‎builder/dockerfile/evaluator.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ func (b *Builder) dispatch(stepN int, stepTotal int, ast *parser.Node) error {
158158
// the entire file (see 'leftoverArgs' processing in evaluator.go )
159159
continue
160160
}
161-
envs = append(envs, fmt.Sprintf("%s=%s", key, val))
161+
envs = append(envs, fmt.Sprintf("%s=%s", key, *val))
162162
}
163163
for ast.Next != nil {
164164
ast = ast.Next

‎cli/command/image/build.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ func NewBuildCommand(dockerCli *command.DockerCli) *cobra.Command {
6767
ulimits := make(map[string]*units.Ulimit)
6868
options := buildOptions{
6969
tags: opts.NewListOpts(validateTag),
70-
buildArgs: opts.NewListOpts(runconfigopts.ValidateArg),
70+
buildArgs: opts.NewListOpts(runconfigopts.ValidateEnv),
7171
ulimits: runconfigopts.NewUlimitOpt(&ulimits),
7272
labels: opts.NewListOpts(runconfigopts.ValidateEnv),
7373
}
@@ -300,7 +300,7 @@ func runBuild(dockerCli *command.DockerCli, options buildOptions) error {
300300
Dockerfile: relDockerfile,
301301
ShmSize: shmSize,
302302
Ulimits: options.ulimits.GetList(),
303-
BuildArgs: runconfigopts.ConvertKVStringsToMap(options.buildArgs.GetAll()),
303+
BuildArgs: runconfigopts.ConvertKVStringsToMapWithNil(options.buildArgs.GetAll()),
304304
AuthConfigs: authConfigs,
305305
Labels: runconfigopts.ConvertKVStringsToMap(options.labels.GetAll()),
306306
CacheFrom: options.cacheFrom,

‎client/image_build_test.go‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ func TestImageBuildError(t *testing.T) {
2727
}
2828

2929
func TestImageBuild(t *testing.T) {
30+
v1 := "value1"
31+
v2 := "value2"
3032
emptyRegistryConfig := "bnVsbA=="
3133
buildCases := []struct {
3234
buildOptions types.ImageBuildOptions
@@ -105,13 +107,14 @@ func TestImageBuild(t *testing.T) {
105107
},
106108
{
107109
buildOptions: types.ImageBuildOptions{
108-
BuildArgs: map[string]string{
109-
"ARG1": "value1",
110-
"ARG2": "value2",
110+
BuildArgs: map[string]*string{
111+
"ARG1": &v1,
112+
"ARG2": &v2,
113+
"ARG3": nil,
111114
},
112115
},
113116
expectedQueryParams: map[string]string{
114-
"buildargs": `{"ARG1":"value1","ARG2":"value2"}`,
117+
"buildargs": `{"ARG1":"value1","ARG2":"value2","ARG3":null}`,
115118
"rm": "0",
116119
},
117120
expectedTags: []string{},

‎integration-cli/docker_cli_build_test.go‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6070,6 +6070,71 @@ func (s *DockerSuite) TestBuildBuildTimeArgUnconsumedArg(c *check.C) {
60706070

60716071
}
60726072

6073+
func (s *DockerSuite) TestBuildBuildTimeArgEnv(c *check.C) {
6074+
testRequires(c, DaemonIsLinux) // Windows does not support ARG
6075+
args := []string{
6076+
"build",
6077+
"--build-arg", fmt.Sprintf("FOO1=fromcmd"),
6078+
"--build-arg", fmt.Sprintf("FOO2="),
6079+
"--build-arg", fmt.Sprintf("FOO3"), // set in env
6080+
"--build-arg", fmt.Sprintf("FOO4"), // not set in env
6081+
"--build-arg", fmt.Sprintf("FOO5=fromcmd"),
6082+
// FOO6 is not set at all
6083+
"--build-arg", fmt.Sprintf("FOO7=fromcmd"), // should produce a warning
6084+
"--build-arg", fmt.Sprintf("FOO8="), // should produce a warning
6085+
"--build-arg", fmt.Sprintf("FOO9"), // should produce a warning
6086+
".",
6087+
}
6088+
6089+
dockerfile := fmt.Sprintf(`FROM busybox
6090+
ARG FOO1=fromfile
6091+
ARG FOO2=fromfile
6092+
ARG FOO3=fromfile
6093+
ARG FOO4=fromfile
6094+
ARG FOO5
6095+
ARG FOO6
6096+
RUN env
6097+
RUN [ "$FOO1" == "fromcmd" ]
6098+
RUN [ "$FOO2" == "" ]
6099+
RUN [ "$FOO3" == "fromenv" ]
6100+
RUN [ "$FOO4" == "fromfile" ]
6101+
RUN [ "$FOO5" == "fromcmd" ]
6102+
# The following should not exist at all in the env
6103+
RUN [ "$(env | grep FOO6)" == "" ]
6104+
RUN [ "$(env | grep FOO7)" == "" ]
6105+
RUN [ "$(env | grep FOO8)" == "" ]
6106+
RUN [ "$(env | grep FOO9)" == "" ]
6107+
`)
6108+
6109+
ctx, err := fakeContext(dockerfile, nil)
6110+
c.Assert(err, check.IsNil)
6111+
defer ctx.Close()
6112+
6113+
cmd := exec.Command(dockerBinary, args...)
6114+
cmd.Dir = ctx.Dir
6115+
cmd.Env = append(os.Environ(),
6116+
"FOO1=fromenv",
6117+
"FOO2=fromenv",
6118+
"FOO3=fromenv")
6119+
out, _, err := runCommandWithOutput(cmd)
6120+
if err != nil {
6121+
c.Fatal(err, out)
6122+
}
6123+
6124+
// Now check to make sure we got a warning msg about unused build-args
6125+
i := strings.Index(out, "[Warning]")
6126+
if i < 0 {
6127+
c.Fatalf("Missing the build-arg warning in %q", out)
6128+
}
6129+
6130+
out = out[i:] // "out" should contain just the warning message now
6131+
6132+
// These were specified on a --build-arg but no ARG was in the Dockerfile
6133+
c.Assert(out, checker.Contains, "FOO7")
6134+
c.Assert(out, checker.Contains, "FOO8")
6135+
c.Assert(out, checker.Contains, "FOO9")
6136+
}
6137+
60736138
func (s *DockerSuite) TestBuildBuildTimeArgQuotedValVariants(c *check.C) {
60746139
imgName := "bldargtest"
60756140
envKey := "foo"

‎runconfig/opts/opts.go‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -59,21 +59,6 @@ func doesEnvExist(name string) bool {
5959
return false
6060
}
6161

62-
// ValidateArg validates a build-arg variable and returns it.
63-
// Build-arg is in the form of <varname>=<value> where <varname> is required.
64-
func ValidateArg(val string) (string, error) {
65-
arr := strings.Split(val, "=")
66-
if len(arr) > 1 && isNotEmpty(arr[0]) {
67-
return val, nil
68-
}
69-
70-
return "", fmt.Errorf("bad format for build-arg: %s", val)
71-
}
72-
73-
func isNotEmpty(val string) bool {
74-
return len(val) > 0
75-
}
76-
7762
// ValidateExtraHost validates that the specified string is a valid extrahost and returns it.
7863
// ExtraHost is in the form of name:ip where the ip has to be a valid ip (IPv4 or IPv6).
7964
func ValidateExtraHost(val string) (string, error) {

0 commit comments

Comments
 (0)