From 726fb269cf004814bd7065ca2f29f6955746a9d9 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 10 Mar 2017 17:07:12 -0500 Subject: [PATCH] Fix --label on docker build when a single quote is used in the value Signed-off-by: Daniel Nephin --- builder/dockerfile/builder.go | 24 +--- builder/dockerfile/builder_test.go | 18 +-- builder/dockerfile/parser/line_parsers.go | 114 ++++++++++++------ .../dockerfile/parser/line_parsers_test.go | 65 ++++++++++ 4 files changed, 149 insertions(+), 72 deletions(-) create mode 100644 builder/dockerfile/parser/line_parsers_test.go diff --git a/builder/dockerfile/builder.go b/builder/dockerfile/builder.go index 03903a7ae4..9b3f173a75 100644 --- a/builder/dockerfile/builder.go +++ b/builder/dockerfile/builder.go @@ -7,7 +7,6 @@ import ( "io" "io/ioutil" "os" - "sort" "strings" "github.com/Sirupsen/logrus" @@ -209,26 +208,13 @@ func sanitizeRepoAndTags(names []string) ([]reference.Named, error) { return repoAndTags, nil } -func (b *Builder) processLabels() error { +func (b *Builder) processLabels() { if len(b.options.Labels) == 0 { - return nil + return } - var labels []string - for k, v := range b.options.Labels { - labels = append(labels, fmt.Sprintf("%q='%s'", k, v)) - } - // Sort the label to have a repeatable order - sort.Strings(labels) - - line := "LABEL " + strings.Join(labels, " ") - _, node, err := parser.ParseLine(line, &b.directive, false) - if err != nil { - return err - } + node := parser.NodeFromLabels(b.options.Labels) b.dockerfile.Children = append(b.dockerfile.Children, node) - - return nil } // build runs the Dockerfile builder from a context and a docker object that allows to make calls @@ -263,9 +249,7 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri return "", err } - if err := b.processLabels(); err != nil { - return "", err - } + b.processLabels() var shortImgID string total := len(b.dockerfile.Children) diff --git a/builder/dockerfile/builder_test.go b/builder/dockerfile/builder_test.go index 8f6b9e9883..c9e5453aee 100644 --- a/builder/dockerfile/builder_test.go +++ b/builder/dockerfile/builder_test.go @@ -8,6 +8,7 @@ import ( "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/container" "github.com/docker/docker/builder/dockerfile/parser" + "github.com/docker/docker/pkg/testutil/assert" ) func TestBuildProcessLabels(t *testing.T) { @@ -15,9 +16,7 @@ func TestBuildProcessLabels(t *testing.T) { d := parser.Directive{} parser.SetEscapeToken(parser.DefaultEscapeToken, &d) n, err := parser.Parse(strings.NewReader(dockerfile), &d) - if err != nil { - t.Fatalf("Error when parsing Dockerfile: %s", err) - } + assert.NilError(t, err) options := &types.ImageBuildOptions{ Labels: map[string]string{ @@ -34,21 +33,14 @@ func TestBuildProcessLabels(t *testing.T) { directive: d, dockerfile: n, } - err = b.processLabels() - if err != nil { - t.Fatalf("Error when processing labels: %s", err) - } + b.processLabels() 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'`, } - if len(b.dockerfile.Children) != 2 { - t.Fatalf("Expect 2, got %d", len(b.dockerfile.Children)) - } + assert.Equal(t, len(b.dockerfile.Children), 2) for i, v := range b.dockerfile.Children { - if v.Original != expected[i] { - t.Fatalf("Expect '%s' for %dth children, got, '%s'", expected[i], i, v.Original) - } + assert.Equal(t, v.Original, expected[i]) } } diff --git a/builder/dockerfile/parser/line_parsers.go b/builder/dockerfile/parser/line_parsers.go index 9b4a4c59f9..4efae66823 100644 --- a/builder/dockerfile/parser/line_parsers.go +++ b/builder/dockerfile/parser/line_parsers.go @@ -10,15 +10,22 @@ import ( "encoding/json" "errors" "fmt" + "sort" "strings" "unicode" "unicode/utf8" + + "github.com/docker/docker/builder/dockerfile/command" ) var ( errDockerfileNotStringArray = errors.New("When using JSON array syntax, arrays must be comprised of strings only.") ) +const ( + commandLabel = "LABEL" +) + // ignore the current argument. This will still leave a command parsed, but // will not incorporate the arguments into the ast. func parseIgnore(rest string, d *Directive) (*Node, map[string]bool, error) { @@ -133,7 +140,7 @@ func parseWords(rest string, d *Directive) []string { // parse environment like statements. Note that this does *not* handle // variable interpolation, which will be handled in the evaluator. -func parseNameVal(rest string, key string, d *Directive) (*Node, map[string]bool, error) { +func parseNameVal(rest string, key string, d *Directive) (*Node, error) { // This is kind of tricky because we need to support the old // variant: KEY name value // as well as the new one: KEY name=value ... @@ -142,57 +149,86 @@ func parseNameVal(rest string, key string, d *Directive) (*Node, map[string]bool words := parseWords(rest, d) if len(words) == 0 { - return nil, nil, nil + return nil, nil } - var rootnode *Node - // Old format (KEY name value) if !strings.Contains(words[0], "=") { - node := &Node{} - rootnode = node - strs := tokenWhitespace.Split(rest, 2) - - if len(strs) < 2 { - return nil, nil, fmt.Errorf(key + " must have two arguments") - } - - node.Value = strs[0] - node.Next = &Node{} - node.Next.Value = strs[1] - } else { - var prevNode *Node - for i, word := range words { - if !strings.Contains(word, "=") { - return nil, nil, fmt.Errorf("Syntax error - can't find = in %q. Must be of the form: name=value", word) - } - parts := strings.SplitN(word, "=", 2) - - name := &Node{} - value := &Node{} - - name.Next = value - name.Value = parts[0] - value.Value = parts[1] - - if i == 0 { - rootnode = name - } else { - prevNode.Next = name - } - prevNode = value + parts := tokenWhitespace.Split(rest, 2) + if len(parts) < 2 { + return nil, fmt.Errorf(key + " must have two arguments") } + return newKeyValueNode(parts[0], parts[1]), nil } - return rootnode, nil, nil + var rootNode *Node + var prevNode *Node + for _, word := range words { + if !strings.Contains(word, "=") { + return nil, fmt.Errorf("Syntax error - can't find = in %q. Must be of the form: name=value", word) + } + + parts := strings.SplitN(word, "=", 2) + node := newKeyValueNode(parts[0], parts[1]) + rootNode, prevNode = appendKeyValueNode(node, rootNode, prevNode) + } + + return rootNode, nil +} + +func newKeyValueNode(key, value string) *Node { + return &Node{ + Value: key, + Next: &Node{Value: value}, + } +} + +func appendKeyValueNode(node, rootNode, prevNode *Node) (*Node, *Node) { + if rootNode == nil { + rootNode = node + } + if prevNode != nil { + prevNode.Next = node + } + + prevNode = node.Next + return rootNode, prevNode } func parseEnv(rest string, d *Directive) (*Node, map[string]bool, error) { - return parseNameVal(rest, "ENV", d) + node, err := parseNameVal(rest, "ENV", d) + return node, nil, err } func parseLabel(rest string, d *Directive) (*Node, map[string]bool, error) { - return parseNameVal(rest, "LABEL", d) + node, err := parseNameVal(rest, commandLabel, d) + 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)) + 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 diff --git a/builder/dockerfile/parser/line_parsers_test.go b/builder/dockerfile/parser/line_parsers_test.go new file mode 100644 index 0000000000..5f762de422 --- /dev/null +++ b/builder/dockerfile/parser/line_parsers_test.go @@ -0,0 +1,65 @@ +package parser + +import ( + "github.com/docker/docker/pkg/testutil/assert" + "testing" +) + +func TestParseNameValOldFormat(t *testing.T) { + directive := Directive{} + node, err := parseNameVal("foo bar", "LABEL", &directive) + assert.NilError(t, err) + + expected := &Node{ + Value: "foo", + Next: &Node{Value: "bar"}, + } + assert.DeepEqual(t, node, expected) +} + +func TestParseNameValNewFormat(t *testing.T) { + directive := Directive{} + node, err := parseNameVal("foo=bar thing=star", "LABEL", &directive) + assert.NilError(t, err) + + expected := &Node{ + Value: "foo", + Next: &Node{ + Value: "bar", + Next: &Node{ + Value: "thing", + Next: &Node{ + Value: "star", + }, + }, + }, + } + assert.DeepEqual(t, node, expected) +} + +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, node, expected) + +}