From d569093624a819568272e2a3859e88581d6cc138 Mon Sep 17 00:00:00 2001 From: Nick Carboni Date: Wed, 25 Mar 2020 14:39:36 -0400 Subject: [PATCH] Make imagebuilder's argument handling mirror "docker build" There were a few inconsistencies in the way imagebuilder handled arguments vs the way that docker handles them. This commit addresses those gaps. Specifically: - Subsequent ARG commands with default values override previous ones, but they don't override the values passed on the command line. - Heading args (ARG commands before the first FROM) are applied only to FROM commands unless a matching ARG command exists in a particular stage in which case the value from the heading arg carries forward into that stage. This was accomplished by creating a dedicated Builder struct field for user args and heading args so that they can be tracked separately from args declared with ARG commands in individual stages. Tracking these all separately allows us to apply these priority rules as the build progresses. Signed-off-by: Nick Carboni --- builder.go | 55 +++-- builder_test.go | 226 ++++++++++++++++++ dispatchers.go | 14 +- .../testdata/multistage/Dockerfile.arg-scope | 9 + .../multistage/Dockerfile.heading-arg | 5 +- .../multistage/Dockerfile.heading-redefine | 7 + 6 files changed, 294 insertions(+), 22 deletions(-) create mode 100644 dockerclient/testdata/multistage/Dockerfile.arg-scope create mode 100644 dockerclient/testdata/multistage/Dockerfile.heading-redefine diff --git a/builder.go b/builder.go index 81d7b842..7f2f6e48 100644 --- a/builder.go +++ b/builder.go @@ -209,12 +209,8 @@ func NewStages(node *parser.Node, b *Builder) (Stages, error) { stages = append(stages, Stage{ Position: i, Name: name, - Builder: &Builder{ - Args: b.Args, - AllowedArgs: b.AllowedArgs, - Env: b.Env, - }, - Node: root, + Builder: b.builderForStage(), + Node: root, }) } return stages, nil @@ -235,17 +231,30 @@ func (b *Builder) extractHeadingArgsFromNode(node *parser.Node) error { } } + // Set children equal to everything except the leading ARG nodes + node.Children = children + + // Use a separate builder to evaluate the heading args + tempBuilder := NewBuilder(b.UserArgs) + + // Evaluate all the heading arg commands for _, c := range args { - step := b.Step() + step := tempBuilder.Step() if err := step.Resolve(c); err != nil { return err } - if err := b.Run(step, NoopExecutor, false); err != nil { + if err := tempBuilder.Run(step, NoopExecutor, false); err != nil { return err } } - node.Children = children + // Add all of the defined heading args to the original builder's HeadingArgs map + for k, v := range tempBuilder.Args { + if _, ok := tempBuilder.AllowedArgs[k]; ok { + b.HeadingArgs[k] = v + } + } + return nil } @@ -264,13 +273,23 @@ func extractNameFromNode(node *parser.Node) (string, bool) { return n.Next.Value, true } +func (b *Builder) builderForStage() *Builder { + stageBuilder := NewBuilder(b.UserArgs) + for k, v := range b.HeadingArgs { + stageBuilder.HeadingArgs[k] = v + } + return stageBuilder +} + type Builder struct { RunConfig docker.Config - Env []string - Args map[string]string - CmdSet bool - Author string + Env []string + Args map[string]string + HeadingArgs map[string]string + UserArgs map[string]string + CmdSet bool + Author string AllowedArgs map[string]bool Volumes VolumeSet @@ -288,12 +307,16 @@ func NewBuilder(args map[string]string) *Builder { for k, v := range builtinAllowedBuildArgs { allowed[k] = v } - provided := make(map[string]string) + userArgs := make(map[string]string) + initialArgs := make(map[string]string) for k, v := range args { - provided[k] = v + userArgs[k] = v + initialArgs[k] = v } return &Builder{ - Args: provided, + Args: initialArgs, + UserArgs: userArgs, + HeadingArgs: make(map[string]string), AllowedArgs: allowed, } } diff --git a/builder_test.go b/builder_test.go index 8f5369e5..5b451768 100644 --- a/builder_test.go +++ b/builder_test.go @@ -7,11 +7,14 @@ import ( "io/ioutil" "os" "reflect" + "regexp" "strings" "testing" "time" docker "github.com/fsouza/go-dockerclient" + + "github.com/openshift/imagebuilder/dockerfile/parser" ) func TestVolumeSet(t *testing.T) { @@ -159,6 +162,19 @@ func TestMultiStageParseHeadingArg(t *testing.T) { if len(stages) != 3 { t.Fatalf("expected 3 stages, got %d", len(stages)) } + + fromImages := []string{"golang:1.9", "busybox:latest", "golang:1.9"} + for i, stage := range stages { + from, err := stage.Builder.From(stage.Node) + if err != nil { + t.Fatal(err) + } + + if expected := fromImages[i]; from != expected { + t.Fatalf("expected %s, got %s", expected, from) + } + } + t.Logf("stages: %#v", stages) } @@ -192,6 +208,216 @@ RUN echo $FOO $BAR`)) } } +func resolveNodeArgs(b *Builder, node *parser.Node) error { + for _, c := range node.Children { + if c.Value != "arg" { + continue + } + step := b.Step() + if err := step.Resolve(c); err != nil { + return err + } + if err := b.Run(step, NoopExecutor, false); err != nil { + return err + } + } + return nil +} + +func builderHasArgument(b *Builder, argString string) bool { + for _, arg := range b.Arguments() { + if arg == argString { + return true + } + } + return false +} + +func TestMultiStageHeadingArgRedefine(t *testing.T) { + n, err := ParseFile("dockerclient/testdata/multistage/Dockerfile.heading-redefine") + if err != nil { + t.Fatal(err) + } + stages, err := NewStages(n, NewBuilder(map[string]string{})) + if err != nil { + t.Fatal(err) + } + if len(stages) != 2 { + t.Fatalf("expected 2 stages, got %d", len(stages)) + } + + for _, stage := range stages { + if err := resolveNodeArgs(stage.Builder, stage.Node); err != nil { + t.Fatal(err) + } + } + + firstStageHasArg := false + for _, arg := range stages[0].Builder.Arguments() { + if match, err := regexp.MatchString(`FOO=.*`, arg); err == nil && match { + firstStageHasArg = true + break + } else if err != nil { + t.Fatal(err) + } + } + if firstStageHasArg { + t.Fatalf("expected FOO to not be present in first stage") + } + + if !builderHasArgument(stages[1].Builder, "FOO=latest") { + t.Fatalf("expected FOO=latest in second stage arguments list, got %v", stages[1].Builder.Arguments()) + } +} + +func TestMultiStageHeadingArgRedefineOverride(t *testing.T) { + n, err := ParseFile("dockerclient/testdata/multistage/Dockerfile.heading-redefine") + if err != nil { + t.Fatal(err) + } + stages, err := NewStages(n, NewBuilder(map[string]string{"FOO": "7"})) + if err != nil { + t.Fatal(err) + } + if len(stages) != 2 { + t.Fatalf("expected 2 stages, got %d", len(stages)) + } + + for _, stage := range stages { + if err := resolveNodeArgs(stage.Builder, stage.Node); err != nil { + t.Fatal(err) + } + } + + firstStageHasArg := false + for _, arg := range stages[0].Builder.Arguments() { + if match, err := regexp.MatchString(`FOO=.*`, arg); err == nil && match { + firstStageHasArg = true + break + } else if err != nil { + t.Fatal(err) + } + } + if firstStageHasArg { + t.Fatalf("expected FOO to not be present in first stage") + } + + if !builderHasArgument(stages[1].Builder, "FOO=7") { + t.Fatalf("expected FOO=7 in second stage arguments list, got %v", stages[1].Builder.Arguments()) + } +} + +func TestArgs(t *testing.T) { + for _, tc := range []struct { + name string + dockerfile string + args map[string]string + expectedValue string + }{ + { + name: "argOverride", + dockerfile: "FROM centos\nARG FOO=stuff\nARG FOO=things\n", + args: map[string]string{}, + expectedValue: "FOO=things", + }, + { + name: "argOverrideWithBuildArgs", + dockerfile: "FROM centos\nARG FOO=stuff\nARG FOO=things\n", + args: map[string]string{"FOO": "bar"}, + expectedValue: "FOO=bar", + }, + { + name: "headingArgRedefine", + dockerfile: "ARG FOO=stuff\nFROM centos\nARG FOO\n", + args: map[string]string{}, + expectedValue: "FOO=stuff", + }, + { + name: "headingArgRedefineWithBuildArgs", + dockerfile: "ARG FOO=stuff\nFROM centos\nARG FOO\n", + args: map[string]string{"FOO": "bar"}, + expectedValue: "FOO=bar", + }, + { + name: "headingArgRedefineDefault", + dockerfile: "ARG FOO=stuff\nFROM centos\nARG FOO=defaultfoovalue\n", + args: map[string]string{}, + expectedValue: "FOO=defaultfoovalue", + }, + { + name: "headingArgRedefineDefaultWithBuildArgs", + dockerfile: "ARG FOO=stuff\nFROM centos\nARG FOO=defaultfoovalue\n", + args: map[string]string{"FOO": "bar"}, + expectedValue: "FOO=bar", + }, + } { + t.Run(tc.name, func(t *testing.T) { + node, err := ParseDockerfile(strings.NewReader(tc.dockerfile)) + if err != nil { + t.Fatal(err) + } + + b := NewBuilder(tc.args) + if err := resolveNodeArgs(b, node); err != nil { + t.Fatal(err) + } + + if !builderHasArgument(b, tc.expectedValue) { + t.Fatalf("expected %s to be contained in arguments list: %v", tc.expectedValue, b.Arguments()) + } + }) + } +} + +func TestMultiStageArgScope(t *testing.T) { + n, err := ParseFile("dockerclient/testdata/multistage/Dockerfile.arg-scope") + if err != nil { + t.Fatal(err) + } + args := map[string]string{ + "SECRET": "secretthings", + "BAR": "notsecretthings", + } + stages, err := NewStages(n, NewBuilder(args)) + if err != nil { + t.Fatal(err) + } + if len(stages) != 2 { + t.Fatalf("expected 2 stages, got %d", len(stages)) + } + + for _, stage := range stages { + if err := resolveNodeArgs(stage.Builder, stage.Node); err != nil { + t.Fatal(err) + } + } + + if !builderHasArgument(stages[0].Builder, "SECRET=secretthings") { + t.Fatalf("expected SECRET=secretthings to be contained in first stage arguments list: %v", stages[0].Builder.Arguments()) + } + + secondStageArguments := stages[1].Builder.Arguments() + secretInSecondStage := false + for _, arg := range secondStageArguments { + if match, err := regexp.MatchString(`SECRET=.*`, arg); err == nil && match { + secretInSecondStage = true + break + } else if err != nil { + t.Fatal(err) + } + } + if secretInSecondStage { + t.Fatalf("expected SECRET to not be present in second stage") + } + + if !builderHasArgument(stages[1].Builder, "FOO=test") { + t.Fatalf("expected FOO=test to be present in second stage arguments list: %v", secondStageArguments) + } + if !builderHasArgument(stages[1].Builder, "BAR=notsecretthings") { + t.Fatalf("expected BAR=notsecretthings to be present in second stage arguments list: %v", secondStageArguments) + } +} + func TestRun(t *testing.T) { f, err := os.Open("dockerclient/testdata/Dockerfile.add") if err != nil { diff --git a/dispatchers.go b/dispatchers.go index e7f2f97b..1d77a193 100644 --- a/dispatchers.go +++ b/dispatchers.go @@ -216,7 +216,7 @@ func from(b *Builder, args []string, attributes map[string]bool, flagArgs []stri // Support ARG before from argStrs := []string{} - for n, v := range b.Args { + for n, v := range b.HeadingArgs { argStrs = append(argStrs, n+"="+v) } var err error @@ -598,10 +598,16 @@ func arg(b *Builder, args []string, attributes map[string]bool, flagArgs []strin // add the arg to allowed list of build-time args from this step on. b.AllowedArgs[name] = true + // If there is still no default value, a value can be assigned from the heading args + if val, ok := b.HeadingArgs[name]; ok && !hasDefault { + b.Args[name] = val + } + // 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.Args[name]; !ok && hasDefault { + // b.buildArgs, later default values for the same arg override earlier ones. + // The args passed to builder (UserArgs) override the default value of 'arg' + // Don't add them here as they were already set in NewBuilder. + if _, ok := b.UserArgs[name]; !ok && hasDefault { b.Args[name] = value } diff --git a/dockerclient/testdata/multistage/Dockerfile.arg-scope b/dockerclient/testdata/multistage/Dockerfile.arg-scope new file mode 100644 index 00000000..1c3e088d --- /dev/null +++ b/dockerclient/testdata/multistage/Dockerfile.arg-scope @@ -0,0 +1,9 @@ +FROM alpine +ARG SECRET +RUN echo "$SECRET" + +FROM alpine +ARG FOO=test +ARG BAR=bartest +RUN echo "$FOO:$BAR" +RUN echo "$SECRET" diff --git a/dockerclient/testdata/multistage/Dockerfile.heading-arg b/dockerclient/testdata/multistage/Dockerfile.heading-arg index 54ee396e..76ccffe1 100644 --- a/dockerclient/testdata/multistage/Dockerfile.heading-arg +++ b/dockerclient/testdata/multistage/Dockerfile.heading-arg @@ -1,5 +1,6 @@ ARG GO_VERSION=1.9 -FROM golang:$GO_VERSION as builder +ARG GO_IMAGE=golang +FROM $GO_IMAGE:$GO_VERSION as builder ARG FOO WORKDIR /tmp COPY . . @@ -10,7 +11,7 @@ WORKDIR /tmp COPY --from=builder /tmp/bar /tmp/bar RUN echo foo2 >> /tmp/bar -FROM busybox:latest +FROM $GO_IMAGE:$GO_VERSION WORKDIR / COPY --from=modifier /tmp/bar /bin/baz diff --git a/dockerclient/testdata/multistage/Dockerfile.heading-redefine b/dockerclient/testdata/multistage/Dockerfile.heading-redefine new file mode 100644 index 00000000..4d476f36 --- /dev/null +++ b/dockerclient/testdata/multistage/Dockerfile.heading-redefine @@ -0,0 +1,7 @@ +ARG FOO=latest +FROM alpine +RUN echo "$FOO" + +FROM centos:$FOO +ARG FOO +RUN echo "$FOO"