From 9c238ebd55e4105ad7f7edc04231ea61bb278ae8 Mon Sep 17 00:00:00 2001 From: Dennis Chen Date: Mon, 7 May 2018 13:28:55 +0800 Subject: [PATCH 1/3] Add 'LABEL' command from '--label' to the last stage This PR is tring to fix issue #36996. Currently for multi-stage build, if `--target` specified, the `--label` option will be ignored. The root cause is the last stage build will remove the `LABEL` command(s) node created from the `--label` option. In order to address this issue, we can create `LABEL` command(s) and add it/tem to the last stage. Signed-off-by: Dennis Chen --- builder/dockerfile/builder.go | 21 ++++++++++++++++++-- builder/dockerfile/instructions/commands.go | 22 ++++++++++++++++++++- 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/builder/dockerfile/builder.go b/builder/dockerfile/builder.go index 21d84cb513..c63661e7f8 100644 --- a/builder/dockerfile/builder.go +++ b/builder/dockerfile/builder.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "io/ioutil" + "sort" "strings" "time" @@ -208,13 +209,26 @@ func newBuilder(clientCtx context.Context, options builderOptions) *Builder { return b } +// Build 'LABEL' command(s) from '--label' options and add to the last stage +func buildLabelOptions(labels map[string]string, stages []instructions.Stage) { + keys := []string{} + for key := range labels { + keys = append(keys, key) + } + + // Sort the label to have a repeatable order + sort.Strings(keys) + for _, key := range keys { + value := labels[key] + stages[len(stages)-1].AddCommand(instructions.NewLabelCommand(key, value, true)) + } +} + // Build runs the Dockerfile builder by parsing the Dockerfile and executing // the instructions from the file. func (b *Builder) build(source builder.Source, dockerfile *parser.Result) (*builder.Result, error) { defer b.imageSources.Unmount() - addNodesForLabelOption(dockerfile.AST, b.options.Labels) - stages, metaArgs, err := instructions.Parse(dockerfile.AST) if err != nil { if instructions.IsUnknownInstruction(err) { @@ -231,6 +245,9 @@ func (b *Builder) build(source builder.Source, dockerfile *parser.Result) (*buil stages = stages[:targetIx+1] } + // Add 'LABEL' command specified by '--label' option to the last stage + buildLabelOptions(b.options.Labels, stages) + dockerfile.PrintWarnings(b.Stderr) dispatchState, err := b.dispatchDockerfileWithCancellation(stages, metaArgs, dockerfile.EscapeToken, source) if err != nil { diff --git a/builder/dockerfile/instructions/commands.go b/builder/dockerfile/instructions/commands.go index 9d864e5325..633a2b3fc7 100644 --- a/builder/dockerfile/instructions/commands.go +++ b/builder/dockerfile/instructions/commands.go @@ -110,17 +110,37 @@ type MaintainerCommand struct { Maintainer string } +// NewLabelCommand creates a new 'LABEL' command +func NewLabelCommand(k string, v string, NoExp bool) *LabelCommand { + kvp := KeyValuePair{Key: k, Value: v} + c := "LABEL " + c += kvp.String() + nc := withNameAndCode{code: c, name: "label"} + cmd := &LabelCommand{ + withNameAndCode: nc, + Labels: KeyValuePairs{ + kvp, + }, + noExpand: NoExp, + } + return cmd +} + // LabelCommand : LABEL some json data describing the image // // Sets the Label variable foo to bar, // type LabelCommand struct { withNameAndCode - Labels KeyValuePairs // kvp slice instead of map to preserve ordering + Labels KeyValuePairs // kvp slice instead of map to preserve ordering + noExpand bool } // Expand variables func (c *LabelCommand) Expand(expander SingleWordExpander) error { + if c.noExpand { + return nil + } return expandKvpsInPlace(c.Labels, expander) } From f7add4262b69294aa8035c4794236193ce4bc68e Mon Sep 17 00:00:00 2001 From: Dennis Chen Date: Mon, 7 May 2018 15:06:07 +0800 Subject: [PATCH 2/3] Add test case for `--label` with `--target` Add a new test case `TestBuildLabelWithTargets` to cover the Docker builder with both `--label` and `--target` options. Signed-off-by: Dennis Chen --- integration/build/build_test.go | 75 +++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/integration/build/build_test.go b/integration/build/build_test.go index 52c03b4734..c0aea0fe6b 100644 --- a/integration/build/build_test.go +++ b/integration/build/build_test.go @@ -173,6 +173,81 @@ func TestBuildMultiStageParentConfig(t *testing.T) { assert.Check(t, is.Contains(image.Config.Env, "WHO=parent")) } +// Test cases in #36996 +func TestBuildLabelWithTargets(t *testing.T) { + bldName := "build-a" + testLabels := map[string]string{ + "foo": "bar", + "dead": "beef", + } + + dockerfile := ` + FROM busybox AS target-a + CMD ["/dev"] + LABEL label-a=inline-a + FROM busybox AS target-b + CMD ["/dist"] + LABEL label-b=inline-b + ` + + ctx := context.Background() + source := fakecontext.New(t, "", fakecontext.WithDockerfile(dockerfile)) + defer source.Close() + + apiclient := testEnv.APIClient() + // For `target-a` build + resp, err := apiclient.ImageBuild(ctx, + source.AsTarReader(t), + types.ImageBuildOptions{ + Remove: true, + ForceRemove: true, + Tags: []string{bldName}, + Labels: testLabels, + Target: "target-a", + }) + assert.NilError(t, err) + _, err = io.Copy(ioutil.Discard, resp.Body) + resp.Body.Close() + assert.NilError(t, err) + + image, _, err := apiclient.ImageInspectWithRaw(ctx, bldName) + assert.NilError(t, err) + + testLabels["label-a"] = "inline-a" + for k, v := range testLabels { + x, ok := image.Config.Labels[k] + assert.Assert(t, ok) + assert.Assert(t, x == v) + } + + // For `target-b` build + bldName = "build-b" + delete(testLabels, "label-a") + resp, err = apiclient.ImageBuild(ctx, + source.AsTarReader(t), + types.ImageBuildOptions{ + Remove: true, + ForceRemove: true, + Tags: []string{bldName}, + Labels: testLabels, + Target: "target-b", + }) + assert.NilError(t, err) + _, err = io.Copy(ioutil.Discard, resp.Body) + resp.Body.Close() + assert.NilError(t, err) + + image, _, err = apiclient.ImageInspectWithRaw(ctx, bldName) + assert.NilError(t, err) + + testLabels["label-b"] = "inline-b" + for k, v := range testLabels { + x, ok := image.Config.Labels[k] + assert.Assert(t, ok) + assert.Assert(t, x == v) + } +} + func TestBuildWithEmptyLayers(t *testing.T) { dockerfile := ` FROM busybox From c7b543164daed58fbea36471592438b4e53ab748 Mon Sep 17 00:00:00 2001 From: Dennis Chen Date: Tue, 8 May 2018 17:15:57 +0800 Subject: [PATCH 3/3] Remove unused 'label' related functions Since we use `NewLabelCommand()` instead of `addNodesForLabelOption()` to create the 'LABEL' commands from '--label' options, so all the related functions should be removed. Signed-off-by: Dennis Chen --- builder/dockerfile/builder.go | 9 ----- builder/dockerfile/builder_test.go | 35 ------------------- builder/dockerfile/parser/line_parsers.go | 31 ---------------- .../dockerfile/parser/line_parsers_test.go | 26 -------------- 4 files changed, 101 deletions(-) delete mode 100644 builder/dockerfile/builder_test.go diff --git a/builder/dockerfile/builder.go b/builder/dockerfile/builder.go index c63661e7f8..1455fd966e 100644 --- a/builder/dockerfile/builder.go +++ b/builder/dockerfile/builder.go @@ -350,15 +350,6 @@ func (b *Builder) dispatchDockerfileWithCancellation(parseResult []instructions. return dispatchRequest.state, nil } -func addNodesForLabelOption(dockerfile *parser.Node, labels map[string]string) { - if len(labels) == 0 { - return - } - - node := parser.NodeFromLabels(labels) - dockerfile.Children = append(dockerfile.Children, node) -} - // BuildFromConfig builds directly from `changes`, treating it as if it were the contents of a Dockerfile // It will: // - Call parse.Parse() to get an AST root for the concatenated Dockerfile entries. diff --git a/builder/dockerfile/builder_test.go b/builder/dockerfile/builder_test.go deleted file mode 100644 index 6c73b6cced..0000000000 --- a/builder/dockerfile/builder_test.go +++ /dev/null @@ -1,35 +0,0 @@ -package dockerfile // import "github.com/docker/docker/builder/dockerfile" - -import ( - "strings" - "testing" - - "github.com/docker/docker/builder/dockerfile/parser" - "github.com/gotestyourself/gotestyourself/assert" - is "github.com/gotestyourself/gotestyourself/assert/cmp" -) - -func TestAddNodesForLabelOption(t *testing.T) { - dockerfile := "FROM scratch" - result, err := parser.Parse(strings.NewReader(dockerfile)) - assert.Check(t, err) - - labels := map[string]string{ - "org.e": "cli-e", - "org.d": "cli-d", - "org.c": "cli-c", - "org.b": "cli-b", - "org.a": "cli-a", - } - nodes := result.AST - addNodesForLabelOption(nodes, labels) - - expected := []string{ - "FROM scratch", - `LABEL "org.a"='cli-a' "org.b"='cli-b' "org.c"='cli-c' "org.d"='cli-d' "org.e"='cli-e'`, - } - assert.Check(t, is.Len(nodes.Children, 2)) - for i, v := range nodes.Children { - assert.Check(t, is.Equal(expected[i], v.Original)) - } -} diff --git a/builder/dockerfile/parser/line_parsers.go b/builder/dockerfile/parser/line_parsers.go index 94091f5f6b..c454835373 100644 --- a/builder/dockerfile/parser/line_parsers.go +++ b/builder/dockerfile/parser/line_parsers.go @@ -10,12 +10,9 @@ import ( "encoding/json" "errors" "fmt" - "sort" "strings" "unicode" "unicode/utf8" - - "github.com/docker/docker/builder/dockerfile/command" ) var ( @@ -205,34 +202,6 @@ func parseLabel(rest string, d *Directive) (*Node, map[string]bool, error) { return node, nil, err } -// NodeFromLabels returns a Node for the injected labels -func NodeFromLabels(labels map[string]string) *Node { - keys := []string{} - for key := range labels { - keys = append(keys, key) - } - // Sort the label to have a repeatable order - sort.Strings(keys) - - labelPairs := []string{} - var rootNode *Node - var prevNode *Node - for _, key := range keys { - value := labels[key] - labelPairs = append(labelPairs, fmt.Sprintf("%q='%s'", key, value)) - // Value must be single quoted to prevent env variable expansion - // See https://github.com/docker/docker/issues/26027 - node := newKeyValueNode(key, "'"+value+"'") - rootNode, prevNode = appendKeyValueNode(node, rootNode, prevNode) - } - - return &Node{ - Value: command.Label, - Original: commandLabel + " " + strings.Join(labelPairs, " "), - Next: rootNode, - } -} - // parses a statement containing one or more keyword definition(s) and/or // value assignments, like `name1 name2= name3="" name4=value`. // Note that this is a stricter format than the old format of assignment, diff --git a/builder/dockerfile/parser/line_parsers_test.go b/builder/dockerfile/parser/line_parsers_test.go index 50b8d03c23..f7580efa3e 100644 --- a/builder/dockerfile/parser/line_parsers_test.go +++ b/builder/dockerfile/parser/line_parsers_test.go @@ -42,32 +42,6 @@ func TestParseNameValNewFormat(t *testing.T) { assert.DeepEqual(t, expected, node, cmpNodeOpt) } -func TestNodeFromLabels(t *testing.T) { - labels := map[string]string{ - "foo": "bar", - "weird": "first' second", - } - expected := &Node{ - Value: "label", - Original: `LABEL "foo"='bar' "weird"='first' second'`, - Next: &Node{ - Value: "foo", - Next: &Node{ - Value: "'bar'", - Next: &Node{ - Value: "weird", - Next: &Node{ - Value: "'first' second'", - }, - }, - }, - }, - } - - node := NodeFromLabels(labels) - assert.DeepEqual(t, expected, node, cmpNodeOpt) -} - func TestParseNameValWithoutVal(t *testing.T) { directive := Directive{} // In Config.Env, a variable without `=` is removed from the environment. (#31634)