Browse code

Fix ARG scoping for Dockerfiles with multiple FROM

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

Tonis Tiigi authored on 2017/03/17 06:31:02
Showing 6 changed files
... ...
@@ -75,7 +75,8 @@ type Builder struct {
75 75
 	cmdSet           bool
76 76
 	disableCommit    bool
77 77
 	cacheBusted      bool
78
-	allowedBuildArgs map[string]bool // list of build-time args that are allowed for expansion/substitution and passing to commands in 'run'.
78
+	allowedBuildArgs map[string]*string  // list of build-time args that are allowed for expansion/substitution and passing to commands in 'run'.
79
+	allBuildArgs     map[string]struct{} // list of all build-time args found during parsing of the Dockerfile
79 80
 	directive        parser.Directive
80 81
 
81 82
 	// TODO: remove once docker.Commit can receive a tag
... ...
@@ -127,9 +128,6 @@ func NewBuilder(clientCtx context.Context, config *types.ImageBuildOptions, back
127 127
 	if config == nil {
128 128
 		config = new(types.ImageBuildOptions)
129 129
 	}
130
-	if config.BuildArgs == nil {
131
-		config.BuildArgs = make(map[string]*string)
132
-	}
133 130
 	ctx, cancel := context.WithCancel(clientCtx)
134 131
 	b = &Builder{
135 132
 		clientCtx:        ctx,
... ...
@@ -142,7 +140,8 @@ func NewBuilder(clientCtx context.Context, config *types.ImageBuildOptions, back
142 142
 		runConfig:        new(container.Config),
143 143
 		tmpContainers:    map[string]struct{}{},
144 144
 		id:               stringid.GenerateNonCryptoID(),
145
-		allowedBuildArgs: make(map[string]bool),
145
+		allowedBuildArgs: make(map[string]*string),
146
+		allBuildArgs:     make(map[string]struct{}),
146 147
 		directive: parser.Directive{
147 148
 			EscapeSeen:           false,
148 149
 			LookingForDirectives: true,
... ...
@@ -320,7 +319,7 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
320 320
 func (b *Builder) warnOnUnusedBuildArgs() {
321 321
 	leftoverArgs := []string{}
322 322
 	for arg := range b.options.BuildArgs {
323
-		if !b.isBuildArgAllowed(arg) {
323
+		if _, ok := b.allBuildArgs[arg]; !ok {
324 324
 			leftoverArgs = append(leftoverArgs, arg)
325 325
 		}
326 326
 	}
... ...
@@ -205,6 +205,8 @@ func from(b *Builder, args []string, attributes map[string]bool, original string
205 205
 
206 206
 	var image builder.Image
207 207
 
208
+	b.noBaseImage = false
209
+
208 210
 	// Windows cannot support a container with no base image.
209 211
 	if name == api.NoBaseImageSpecifier {
210 212
 		if runtime.GOOS == "windows" {
... ...
@@ -228,6 +230,8 @@ func from(b *Builder, args []string, attributes map[string]bool, original string
228 228
 	}
229 229
 	b.from = image
230 230
 
231
+	b.allowedBuildArgs = make(map[string]*string)
232
+
231 233
 	return b.processImageFrom(image)
232 234
 }
233 235
 
... ...
@@ -729,17 +733,13 @@ func arg(b *Builder, args []string, attributes map[string]bool, original string)
729 729
 		hasDefault = false
730 730
 	}
731 731
 	// add the arg to allowed list of build-time args from this step on.
732
-	b.allowedBuildArgs[name] = true
732
+	b.allBuildArgs[name] = struct{}{}
733 733
 
734
-	// If there is a default value associated with this arg then add it to the
735
-	// b.buildArgs if one is not already passed to the builder. The args passed
736
-	// to builder override the default value of 'arg'. Note that a 'nil' for
737
-	// a value means that the user specified "--build-arg FOO" and "FOO" wasn't
738
-	// defined as an env var - and in that case we DO want to use the default
739
-	// value specified in the ARG cmd.
740
-	if baValue, ok := b.options.BuildArgs[name]; (!ok || baValue == nil) && hasDefault {
741
-		b.options.BuildArgs[name] = &newValue
734
+	var value *string
735
+	if hasDefault {
736
+		value = &newValue
742 737
 	}
738
+	b.allowedBuildArgs[name] = value
743 739
 
744 740
 	return b.commit("", b.runConfig.Cmd, fmt.Sprintf("ARG %s", arg))
745 741
 }
... ...
@@ -460,9 +460,11 @@ func TestStopSignal(t *testing.T) {
460 460
 }
461 461
 
462 462
 func TestArg(t *testing.T) {
463
+	// This is a bad test that tests implementation details and not at
464
+	// any features of the builder. Replace or remove.
463 465
 	buildOptions := &types.ImageBuildOptions{BuildArgs: make(map[string]*string)}
464 466
 
465
-	b := &Builder{flags: &BFlags{}, runConfig: &container.Config{}, disableCommit: true, allowedBuildArgs: make(map[string]bool), options: buildOptions}
467
+	b := &Builder{flags: &BFlags{}, runConfig: &container.Config{}, disableCommit: true, allowedBuildArgs: make(map[string]*string), allBuildArgs: make(map[string]struct{}), options: buildOptions}
466 468
 
467 469
 	argName := "foo"
468 470
 	argVal := "bar"
... ...
@@ -472,24 +474,14 @@ func TestArg(t *testing.T) {
472 472
 		t.Fatalf("Error should be empty, got: %s", err.Error())
473 473
 	}
474 474
 
475
-	allowed, ok := b.allowedBuildArgs[argName]
476
-
477
-	if !ok {
478
-		t.Fatalf("%s argument should be allowed as a build arg", argName)
479
-	}
480
-
481
-	if !allowed {
482
-		t.Fatalf("%s argument was present in map but disallowed as a build arg", argName)
483
-	}
484
-
485
-	val, ok := b.options.BuildArgs[argName]
475
+	value, ok := b.getBuildArg(argName)
486 476
 
487 477
 	if !ok {
488 478
 		t.Fatalf("%s argument should be a build arg", argName)
489 479
 	}
490 480
 
491
-	if *val != "bar" {
492
-		t.Fatalf("%s argument should have default value 'bar', got %s", argName, *val)
481
+	if value != "bar" {
482
+		t.Fatalf("%s argument should have default value 'bar', got %s", argName, value)
493 483
 	}
494 484
 }
495 485
 
... ...
@@ -192,17 +192,9 @@ func (b *Builder) buildArgsWithoutConfigEnv() []string {
192 192
 	envs := []string{}
193 193
 	configEnv := runconfigopts.ConvertKVStringsToMap(b.runConfig.Env)
194 194
 
195
-	for key, val := range b.options.BuildArgs {
196
-		if !b.isBuildArgAllowed(key) {
197
-			// skip build-args that are not in allowed list, meaning they have
198
-			// not been defined by an "ARG" Dockerfile command yet.
199
-			// This is an error condition but only if there is no "ARG" in the entire
200
-			// Dockerfile, so we'll generate any necessary errors after we parsed
201
-			// the entire file (see 'leftoverArgs' processing in evaluator.go )
202
-			continue
203
-		}
204
-		if _, ok := configEnv[key]; !ok && val != nil {
205
-			envs = append(envs, fmt.Sprintf("%s=%s", key, *val))
195
+	for key, val := range b.getBuildArgs() {
196
+		if _, ok := configEnv[key]; !ok {
197
+			envs = append(envs, fmt.Sprintf("%s=%s", key, val))
206 198
 		}
207 199
 	}
208 200
 	return envs
... ...
@@ -668,14 +668,35 @@ func (b *Builder) parseDockerfile() error {
668 668
 	return nil
669 669
 }
670 670
 
671
-// determine if build arg is part of built-in args or user
672
-// defined args in Dockerfile at any point in time.
673
-func (b *Builder) isBuildArgAllowed(arg string) bool {
674
-	if _, ok := BuiltinAllowedBuildArgs[arg]; ok {
675
-		return true
671
+func (b *Builder) getBuildArg(arg string) (string, bool) {
672
+	defaultValue, defined := b.allowedBuildArgs[arg]
673
+	_, builtin := BuiltinAllowedBuildArgs[arg]
674
+	if defined || builtin {
675
+		if v, ok := b.options.BuildArgs[arg]; ok && v != nil {
676
+			return *v, ok
677
+		}
678
+	}
679
+	if defaultValue == nil {
680
+		return "", false
681
+	}
682
+	return *defaultValue, defined
683
+}
684
+
685
+func (b *Builder) getBuildArgs() map[string]string {
686
+	m := make(map[string]string)
687
+	for arg := range b.options.BuildArgs {
688
+		v, ok := b.getBuildArg(arg)
689
+		if ok {
690
+			m[arg] = v
691
+		}
676 692
 	}
677
-	if _, ok := b.allowedBuildArgs[arg]; ok {
678
-		return true
693
+	for arg := range b.allowedBuildArgs {
694
+		if _, ok := m[arg]; !ok {
695
+			v, ok := b.getBuildArg(arg)
696
+			if ok {
697
+				m[arg] = v
698
+			}
699
+		}
679 700
 	}
680
-	return false
701
+	return m
681 702
 }
... ...
@@ -4740,6 +4740,61 @@ func (s *DockerSuite) TestBuildBuildTimeArgDefintionWithNoEnvInjection(c *check.
4740 4740
 	}
4741 4741
 }
4742 4742
 
4743
+func (s *DockerSuite) TestBuildBuildTimeArgMultipleFrom(c *check.C) {
4744
+	imgName := "multifrombldargtest"
4745
+	dockerfile := `FROM busybox
4746
+    ARG foo=abc
4747
+    LABEL multifromtest=1
4748
+    RUN env > /out
4749
+    FROM busybox
4750
+    ARG bar=def
4751
+    RUN env > /out`
4752
+
4753
+	result := buildImage(imgName, withDockerfile(dockerfile))
4754
+	result.Assert(c, icmd.Success)
4755
+
4756
+	result = icmd.RunCmd(icmd.Cmd{
4757
+		Command: []string{dockerBinary, "images", "-q", "-f", "label=multifromtest=1"},
4758
+	})
4759
+	result.Assert(c, icmd.Success)
4760
+	parentID := strings.TrimSpace(result.Stdout())
4761
+
4762
+	result = icmd.RunCmd(icmd.Cmd{
4763
+		Command: []string{dockerBinary, "run", "--rm", parentID, "cat", "/out"},
4764
+	})
4765
+	result.Assert(c, icmd.Success)
4766
+	c.Assert(result.Stdout(), checker.Contains, "foo=abc")
4767
+
4768
+	result = icmd.RunCmd(icmd.Cmd{
4769
+		Command: []string{dockerBinary, "run", "--rm", imgName, "cat", "/out"},
4770
+	})
4771
+	result.Assert(c, icmd.Success)
4772
+	c.Assert(result.Stdout(), checker.Not(checker.Contains), "foo")
4773
+	c.Assert(result.Stdout(), checker.Contains, "bar=def")
4774
+}
4775
+
4776
+func (s *DockerSuite) TestBuildBuildTimeUnusedArgMultipleFrom(c *check.C) {
4777
+	imgName := "multifromunusedarg"
4778
+	dockerfile := `FROM busybox
4779
+    ARG foo
4780
+    FROM busybox
4781
+    ARG bar
4782
+    RUN env > /out`
4783
+
4784
+	result := buildImage(imgName, withDockerfile(dockerfile), withBuildFlags(
4785
+		"--build-arg", fmt.Sprintf("baz=abc")))
4786
+	result.Assert(c, icmd.Success)
4787
+	c.Assert(result.Combined(), checker.Contains, "[Warning]")
4788
+	c.Assert(result.Combined(), checker.Contains, "[baz] were not consumed")
4789
+
4790
+	result = icmd.RunCmd(icmd.Cmd{
4791
+		Command: []string{dockerBinary, "run", "--rm", imgName, "cat", "/out"},
4792
+	})
4793
+	result.Assert(c, icmd.Success)
4794
+	c.Assert(result.Stdout(), checker.Not(checker.Contains), "bar")
4795
+	c.Assert(result.Stdout(), checker.Not(checker.Contains), "baz")
4796
+}
4797
+
4743 4798
 func (s *DockerSuite) TestBuildNoNamedVolume(c *check.C) {
4744 4799
 	volName := "testname:/foo"
4745 4800