From 239c53bf836174108dbae445a394a290f5fe2898 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 6 Apr 2017 17:38:02 -0400 Subject: [PATCH] Refactor BuildArgs Add MetaArgs for ARG that occur before the first FROM Integration test for these cases. Signed-off-by: Daniel Nephin --- builder/dockerfile/buildargs.go | 124 +++++++++++++++++++++++ builder/dockerfile/buildargs_test.go | 63 ++++++++++++ builder/dockerfile/builder.go | 72 +++++-------- builder/dockerfile/dispatchers.go | 23 ++--- builder/dockerfile/dispatchers_test.go | 38 +++---- builder/dockerfile/evaluator.go | 2 +- builder/dockerfile/evaluator_test.go | 12 ++- builder/dockerfile/internals.go | 33 ------ integration-cli/docker_cli_build_test.go | 54 ++++++---- 9 files changed, 281 insertions(+), 140 deletions(-) create mode 100644 builder/dockerfile/buildargs.go create mode 100644 builder/dockerfile/buildargs_test.go diff --git a/builder/dockerfile/buildargs.go b/builder/dockerfile/buildargs.go new file mode 100644 index 0000000000..4fde2e911e --- /dev/null +++ b/builder/dockerfile/buildargs.go @@ -0,0 +1,124 @@ +package dockerfile + +// builtinAllowedBuildArgs is list of built-in allowed build args +// these args are considered transparent and are excluded from the image history. +// Filtering from history is implemented in dispatchers.go +var builtinAllowedBuildArgs = map[string]bool{ + "HTTP_PROXY": true, + "http_proxy": true, + "HTTPS_PROXY": true, + "https_proxy": true, + "FTP_PROXY": true, + "ftp_proxy": true, + "NO_PROXY": true, + "no_proxy": true, +} + +// buildArgs manages arguments used by the builder +type buildArgs struct { + // args that are allowed for expansion/substitution and passing to commands in 'run'. + allowedBuildArgs map[string]*string + // args defined before the first `FROM` in a Dockerfile + allowedMetaArgs map[string]*string + // args referenced by the Dockerfile + referencedArgs map[string]struct{} + // args provided by the user on the command line + argsFromOptions map[string]*string +} + +func newBuildArgs(argsFromOptions map[string]*string) *buildArgs { + return &buildArgs{ + allowedBuildArgs: make(map[string]*string), + allowedMetaArgs: make(map[string]*string), + referencedArgs: make(map[string]struct{}), + argsFromOptions: argsFromOptions, + } +} + +// UnreferencedOptionArgs returns the list of args that were set from options but +// were never referenced from the Dockerfile +func (b *buildArgs) UnreferencedOptionArgs() []string { + leftoverArgs := []string{} + for arg := range b.argsFromOptions { + if _, ok := b.referencedArgs[arg]; !ok { + leftoverArgs = append(leftoverArgs, arg) + } + } + return leftoverArgs +} + +// ResetAllowed clears the list of args that are allowed to be used by a +// directive +func (b *buildArgs) ResetAllowed() { + b.allowedBuildArgs = make(map[string]*string) +} + +// AddMetaArg adds a new meta arg that can be used by FROM directives +func (b *buildArgs) AddMetaArg(key string, value *string) { + b.allowedMetaArgs[key] = value +} + +// AddArg adds a new arg that can be used by directives +func (b *buildArgs) AddArg(key string, value *string) { + b.allowedBuildArgs[key] = value + b.referencedArgs[key] = struct{}{} +} + +// IsUnreferencedBuiltin checks if the key is a built-in arg, or if it has been +// referenced by the Dockerfile. Returns true if the arg is a builtin that has +// not been referenced in the Dockerfile. +func (b *buildArgs) IsUnreferencedBuiltin(key string) bool { + _, isBuiltin := builtinAllowedBuildArgs[key] + _, isAllowed := b.allowedBuildArgs[key] + return isBuiltin && !isAllowed +} + +// GetAllAllowed returns a mapping with all the allowed args +func (b *buildArgs) GetAllAllowed() map[string]string { + return b.getAllFromMapping(b.allowedBuildArgs) +} + +// GetAllMeta returns a mapping with all the meta meta args +func (b *buildArgs) GetAllMeta() map[string]string { + return b.getAllFromMapping(b.allowedMetaArgs) +} + +func (b *buildArgs) getAllFromMapping(source map[string]*string) map[string]string { + m := make(map[string]string) + + keys := keysFromMaps(source, builtinAllowedBuildArgs) + for _, key := range keys { + v, ok := b.getBuildArg(key, source) + if ok { + m[key] = v + } + } + return m +} + +func (b *buildArgs) getBuildArg(key string, mapping map[string]*string) (string, bool) { + defaultValue, exists := mapping[key] + // Return override from options if one is defined + if v, ok := b.argsFromOptions[key]; ok && v != nil { + return *v, ok + } + + if defaultValue == nil { + if v, ok := b.allowedMetaArgs[key]; ok && v != nil { + return *v, ok + } + return "", false + } + return *defaultValue, exists +} + +func keysFromMaps(source map[string]*string, builtin map[string]bool) []string { + keys := []string{} + for key := range source { + keys = append(keys, key) + } + for key := range builtin { + keys = append(keys, key) + } + return keys +} diff --git a/builder/dockerfile/buildargs_test.go b/builder/dockerfile/buildargs_test.go new file mode 100644 index 0000000000..03df19b2a1 --- /dev/null +++ b/builder/dockerfile/buildargs_test.go @@ -0,0 +1,63 @@ +package dockerfile + +import ( + "github.com/docker/docker/pkg/testutil/assert" + "testing" +) + +func strPtr(source string) *string { + return &source +} + +func TestGetAllAllowed(t *testing.T) { + buildArgs := newBuildArgs(map[string]*string{ + "ArgNotUsedInDockerfile": strPtr("fromopt1"), + "ArgOverriddenByOptions": strPtr("fromopt2"), + "ArgNoDefaultInDockerfileFromOptions": strPtr("fromopt3"), + "HTTP_PROXY": strPtr("theproxy"), + }) + + buildArgs.AddMetaArg("ArgFromMeta", strPtr("frommeta1")) + buildArgs.AddMetaArg("ArgFromMetaOverriden", strPtr("frommeta2")) + buildArgs.AddMetaArg("ArgFromMetaNotUsed", strPtr("frommeta3")) + + buildArgs.AddArg("ArgOverriddenByOptions", strPtr("fromdockerfile2")) + buildArgs.AddArg("ArgWithDefaultInDockerfile", strPtr("fromdockerfile1")) + buildArgs.AddArg("ArgNoDefaultInDockerfile", nil) + buildArgs.AddArg("ArgNoDefaultInDockerfileFromOptions", nil) + buildArgs.AddArg("ArgFromMeta", nil) + buildArgs.AddArg("ArgFromMetaOverriden", strPtr("fromdockerfile3")) + + all := buildArgs.GetAllAllowed() + expected := map[string]string{ + "HTTP_PROXY": "theproxy", + "ArgOverriddenByOptions": "fromopt2", + "ArgWithDefaultInDockerfile": "fromdockerfile1", + "ArgNoDefaultInDockerfileFromOptions": "fromopt3", + "ArgFromMeta": "frommeta1", + "ArgFromMetaOverriden": "fromdockerfile3", + } + assert.DeepEqual(t, all, expected) +} + +func TestGetAllMeta(t *testing.T) { + buildArgs := newBuildArgs(map[string]*string{ + "ArgNotUsedInDockerfile": strPtr("fromopt1"), + "ArgOverriddenByOptions": strPtr("fromopt2"), + "ArgNoDefaultInMetaFromOptions": strPtr("fromopt3"), + "HTTP_PROXY": strPtr("theproxy"), + }) + + buildArgs.AddMetaArg("ArgFromMeta", strPtr("frommeta1")) + buildArgs.AddMetaArg("ArgOverriddenByOptions", strPtr("frommeta2")) + buildArgs.AddMetaArg("ArgNoDefaultInMetaFromOptions", nil) + + all := buildArgs.GetAllMeta() + expected := map[string]string{ + "HTTP_PROXY": "theproxy", + "ArgFromMeta": "frommeta1", + "ArgOverriddenByOptions": "fromopt2", + "ArgNoDefaultInMetaFromOptions": "fromopt3", + } + assert.DeepEqual(t, all, expected) +} diff --git a/builder/dockerfile/builder.go b/builder/dockerfile/builder.go index c1d1e1b128..c4f7d20440 100644 --- a/builder/dockerfile/builder.go +++ b/builder/dockerfile/builder.go @@ -36,20 +36,6 @@ var validCommitCommands = map[string]bool{ "workdir": true, } -// BuiltinAllowedBuildArgs is list of built-in allowed build args -// these args are considered transparent and are excluded from the image history. -// Filtering from history is implemented in dispatchers.go -var BuiltinAllowedBuildArgs = map[string]bool{ - "HTTP_PROXY": true, - "http_proxy": true, - "HTTPS_PROXY": true, - "https_proxy": true, - "FTP_PROXY": true, - "ftp_proxy": true, - "NO_PROXY": true, - "no_proxy": true, -} - var defaultLogConfig = container.LogConfig{Type: "none"} // Builder is a Dockerfile builder @@ -66,20 +52,19 @@ type Builder struct { clientCtx context.Context cancel context.CancelFunc - dockerfile *parser.Node - runConfig *container.Config // runconfig for cmd, run, entrypoint etc. - flags *BFlags - tmpContainers map[string]struct{} - image string // imageID - imageContexts *imageContexts // helper for storing contexts from builds - noBaseImage bool // A flag to track the use of `scratch` as the base image - maintainer string - cmdSet bool - disableCommit bool - cacheBusted bool - 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 + dockerfile *parser.Node + runConfig *container.Config // runconfig for cmd, run, entrypoint etc. + flags *BFlags + tmpContainers map[string]struct{} + image string // imageID + imageContexts *imageContexts // helper for storing contexts from builds + noBaseImage bool // A flag to track the use of `scratch` as the base image + maintainer string + cmdSet bool + disableCommit bool + cacheBusted bool + buildArgs *buildArgs + directive parser.Directive // TODO: remove once docker.Commit can receive a tag id string @@ -134,18 +119,17 @@ func NewBuilder(clientCtx context.Context, config *types.ImageBuildOptions, back } ctx, cancel := context.WithCancel(clientCtx) b = &Builder{ - clientCtx: ctx, - cancel: cancel, - options: config, - Stdout: os.Stdout, - Stderr: os.Stderr, - docker: backend, - context: buildContext, - runConfig: new(container.Config), - tmpContainers: map[string]struct{}{}, - id: stringid.GenerateNonCryptoID(), - allowedBuildArgs: make(map[string]*string), - allBuildArgs: make(map[string]struct{}), + clientCtx: ctx, + cancel: cancel, + options: config, + Stdout: os.Stdout, + Stderr: os.Stderr, + docker: backend, + context: buildContext, + runConfig: new(container.Config), + tmpContainers: map[string]struct{}{}, + id: stringid.GenerateNonCryptoID(), + buildArgs: newBuildArgs(config.BuildArgs), directive: parser.Directive{ EscapeSeen: false, LookingForDirectives: true, @@ -316,13 +300,7 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri // check if there are any leftover build-args that were passed but not // consumed during build. Print a warning, if there are any. func (b *Builder) warnOnUnusedBuildArgs() { - leftoverArgs := []string{} - for arg := range b.options.BuildArgs { - if _, ok := b.allBuildArgs[arg]; !ok { - leftoverArgs = append(leftoverArgs, arg) - } - } - + leftoverArgs := b.buildArgs.UnreferencedOptionArgs() if len(leftoverArgs) > 0 { fmt.Fprintf(b.Stderr, "[Warning] One or more build-args %v were not consumed\n", leftoverArgs) } diff --git a/builder/dockerfile/dispatchers.go b/builder/dockerfile/dispatchers.go index ba98c636e9..fb064e5971 100644 --- a/builder/dockerfile/dispatchers.go +++ b/builder/dockerfile/dispatchers.go @@ -218,7 +218,11 @@ func from(b *Builder, args []string, attributes map[string]bool, original string return err } - substituionArgs := b.buildArgsWithoutConfigEnv() + substituionArgs := []string{} + for key, value := range b.buildArgs.GetAllMeta() { + substituionArgs = append(substituionArgs, key+"="+value) + } + name, err := ProcessWord(args[0], substituionArgs, b.directive.EscapeToken) if err != nil { return err @@ -256,8 +260,7 @@ func from(b *Builder, args []string, attributes map[string]bool, original string } b.from = image - b.allowedBuildArgs = make(map[string]*string) - + b.buildArgs.ResetAllowed() return b.processImageFrom(image) } @@ -442,7 +445,7 @@ func run(b *Builder, args []string, attributes map[string]bool, original string) // properly match it. b.runConfig.Env = env - // remove BuiltinAllowedBuildArgs (see: builder.go) from the saveCmd + // remove builtinAllowedBuildArgs (see: builder.go) from the saveCmd // these args are transparent so resulting image should be the same regardless of the value if len(cmdBuildEnv) > 0 { saveCmd = config.Cmd @@ -450,11 +453,8 @@ func run(b *Builder, args []string, attributes map[string]bool, original string) copy(tmpBuildEnv, cmdBuildEnv) for i, env := range tmpBuildEnv { key := strings.SplitN(env, "=", 2)[0] - if _, ok := BuiltinAllowedBuildArgs[key]; ok { - // If an built-in arg is explicitly added in the Dockerfile, don't prune it - if _, ok := b.allowedBuildArgs[key]; !ok { - tmpBuildEnv = append(tmpBuildEnv[:i], tmpBuildEnv[i+1:]...) - } + if b.buildArgs.IsUnreferencedBuiltin(key) { + tmpBuildEnv = append(tmpBuildEnv[:i], tmpBuildEnv[i+1:]...) } } sort.Strings(tmpBuildEnv) @@ -785,17 +785,16 @@ func arg(b *Builder, args []string, attributes map[string]bool, original string) name = arg hasDefault = false } - // add the arg to allowed list of build-time args from this step on. - b.allBuildArgs[name] = struct{}{} var value *string if hasDefault { value = &newValue } - b.allowedBuildArgs[name] = value + b.buildArgs.AddArg(name, value) // Arg before FROM doesn't add a layer if !b.hasFromImage() { + b.buildArgs.AddMetaArg(name, value) return nil } 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 119d5e904b..77506d712a 100644 --- a/builder/dockerfile/dispatchers_test.go +++ b/builder/dockerfile/dispatchers_test.go @@ -193,12 +193,11 @@ func TestLabel(t *testing.T) { func newBuilderWithMockBackend() *Builder { b := &Builder{ - flags: &BFlags{}, - runConfig: &container.Config{}, - options: &types.ImageBuildOptions{}, - docker: &MockBackend{}, - allowedBuildArgs: make(map[string]*string), - allBuildArgs: make(map[string]struct{}), + flags: &BFlags{}, + runConfig: &container.Config{}, + options: &types.ImageBuildOptions{}, + docker: &MockBackend{}, + buildArgs: newBuildArgs(make(map[string]*string)), } b.imageContexts = &imageContexts{b: b} return b @@ -235,8 +234,8 @@ func TestFromWithArg(t *testing.T) { assert.NilError(t, err) assert.Equal(t, b.image, expected) assert.Equal(t, b.from.ImageID(), expected) - assert.NotNil(t, b.allowedBuildArgs) - assert.Equal(t, len(b.allowedBuildArgs), 0) + assert.Equal(t, len(b.buildArgs.GetAllAllowed()), 0) + assert.Equal(t, len(b.buildArgs.GetAllMeta()), 1) } func TestFromWithUndefinedArg(t *testing.T) { @@ -496,29 +495,18 @@ 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]*string), allBuildArgs: make(map[string]struct{}), options: buildOptions} + b := newBuilderWithMockBackend() argName := "foo" argVal := "bar" argDef := fmt.Sprintf("%s=%s", argName, argVal) - if err := arg(b, []string{argDef}, nil, ""); err != nil { - t.Fatalf("Error should be empty, got: %s", err.Error()) - } + err := arg(b, []string{argDef}, nil, "") + assert.NilError(t, err) - value, ok := b.getBuildArg(argName) - - if !ok { - t.Fatalf("%s argument should be a build arg", argName) - } - - if value != "bar" { - t.Fatalf("%s argument should have default value 'bar', got %s", argName, value) - } + expected := map[string]string{argName: argVal} + allowed := b.buildArgs.GetAllAllowed() + assert.DeepEqual(t, allowed, expected) } func TestShell(t *testing.T) { diff --git a/builder/dockerfile/evaluator.go b/builder/dockerfile/evaluator.go index 78cda4bb92..d29a18c7b1 100644 --- a/builder/dockerfile/evaluator.go +++ b/builder/dockerfile/evaluator.go @@ -189,7 +189,7 @@ func (b *Builder) buildArgsWithoutConfigEnv() []string { envs := []string{} configEnv := runconfigopts.ConvertKVStringsToMap(b.runConfig.Env) - for key, val := range b.getBuildArgs() { + for key, val := range b.buildArgs.GetAllAllowed() { if _, ok := configEnv[key]; !ok { envs = append(envs, fmt.Sprintf("%s=%s", key, val)) } diff --git a/builder/dockerfile/evaluator_test.go b/builder/dockerfile/evaluator_test.go index 4340a2f8ac..0d4952837f 100644 --- a/builder/dockerfile/evaluator_test.go +++ b/builder/dockerfile/evaluator_test.go @@ -180,9 +180,17 @@ func executeTestCase(t *testing.T, testCase dispatchTestCase) { } config := &container.Config{} - options := &types.ImageBuildOptions{} + options := &types.ImageBuildOptions{ + BuildArgs: make(map[string]*string), + } - b := &Builder{runConfig: config, options: options, Stdout: ioutil.Discard, context: context} + b := &Builder{ + runConfig: config, + options: options, + Stdout: ioutil.Discard, + context: context, + buildArgs: newBuildArgs(options.BuildArgs), + } err = b.dispatch(0, len(n.Children), n.Children[0]) diff --git a/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index 5ad4db9d93..209fc2fc46 100644 --- a/builder/dockerfile/internals.go +++ b/builder/dockerfile/internals.go @@ -697,36 +697,3 @@ func (b *Builder) parseDockerfile() error { return nil } - -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 defaultValue == nil { - 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 81d08e5419..8cbacf863e 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/integration-cli/docker_cli_build_test.go @@ -4728,7 +4728,7 @@ func (s *DockerSuite) TestBuildBuildTimeArgDefintionWithNoEnvInjection(c *check. ARG %s RUN env`, envKey) - result := buildImage(imgName, build.WithDockerfile(dockerfile)) + result := cli.BuildCmd(c, imgName, build.WithDockerfile(dockerfile)) result.Assert(c, icmd.Success) if strings.Count(result.Combined(), envKey) != 1 { c.Fatalf("unexpected number of occurrences of the arg in output: %q expected: 1", result.Combined()) @@ -4745,29 +4745,45 @@ func (s *DockerSuite) TestBuildBuildTimeArgMultipleFrom(c *check.C) { ARG bar=def RUN env > /out` - result := buildImage(imgName, build.WithDockerfile(dockerfile)) + result := cli.BuildCmd(c, imgName, build.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) + result = cli.DockerCmd(c, "images", "-q", "-f", "label=multifromtest=1") parentID := strings.TrimSpace(result.Stdout()) - result = icmd.RunCmd(icmd.Cmd{ - Command: []string{dockerBinary, "run", "--rm", parentID, "cat", "/out"}, - }) - result.Assert(c, icmd.Success) + result = cli.DockerCmd(c, "run", "--rm", parentID, "cat", "/out") 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) + result = cli.DockerCmd(c, "run", "--rm", imgName, "cat", "/out") c.Assert(result.Stdout(), checker.Not(checker.Contains), "foo") c.Assert(result.Stdout(), checker.Contains, "bar=def") } +func (s *DockerSuite) TestBuildBuildTimeFromArgMultipleFrom(c *check.C) { + imgName := "multifrombldargtest" + dockerfile := `ARG tag=nosuchtag + FROM busybox:${tag} + LABEL multifromtest=1 + RUN env > /out + FROM busybox:${tag} + ARG tag + RUN env > /out` + + result := cli.BuildCmd(c, imgName, + build.WithDockerfile(dockerfile), + cli.WithFlags("--build-arg", fmt.Sprintf("tag=latest"))) + result.Assert(c, icmd.Success) + + result = cli.DockerCmd(c, "images", "-q", "-f", "label=multifromtest=1") + parentID := strings.TrimSpace(result.Stdout()) + + result = cli.DockerCmd(c, "run", "--rm", parentID, "cat", "/out") + c.Assert(result.Stdout(), checker.Not(checker.Contains), "tag") + + result = cli.DockerCmd(c, "run", "--rm", imgName, "cat", "/out") + c.Assert(result.Stdout(), checker.Contains, "tag=latest") +} + func (s *DockerSuite) TestBuildBuildTimeUnusedArgMultipleFrom(c *check.C) { imgName := "multifromunusedarg" dockerfile := `FROM busybox @@ -4776,16 +4792,14 @@ func (s *DockerSuite) TestBuildBuildTimeUnusedArgMultipleFrom(c *check.C) { ARG bar RUN env > /out` - result := buildImage(imgName, build.WithDockerfile(dockerfile), cli.WithFlags( - "--build-arg", fmt.Sprintf("baz=abc"))) + result := cli.BuildCmd(c, imgName, + build.WithDockerfile(dockerfile), + cli.WithFlags("--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) + result = cli.DockerCmd(c, "run", "--rm", imgName, "cat", "/out") c.Assert(result.Stdout(), checker.Not(checker.Contains), "bar") c.Assert(result.Stdout(), checker.Not(checker.Contains), "baz") }