Merge pull request #31750 from dnephin/some-builder-cleanup

Fix `docker build --label` when the label includes single quotes and a space

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)

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])
 	}
 }

builder/dockerfile/evaluator.go

@@ -129,47 +129,18 @@ func (b *Builder) dispatch(stepN int, stepTotal int, ast *parser.Node) error {
 
 	}
 
-	// count the number of nodes that we are going to traverse first
-	// so we can pre-create the argument and message array. This speeds up the
-	// allocation of those list a lot when they have a lot of arguments
-	cursor := ast
-	var n int
-	for cursor.Next != nil {
-		cursor = cursor.Next
-		n++
-	}
-	msgList := make([]string, n)
-
-	var i int
+	msgList := initMsgList(ast)
 	// Append build args to runConfig environment variables
 	envs := append(b.runConfig.Env, b.buildArgsWithoutConfigEnv()...)
 
-	for ast.Next != nil {
+	for i := 0; ast.Next != nil; i++ {
 		ast = ast.Next
-		var str string
-		str = ast.Value
-		if replaceEnvAllowed[cmd] {
-			var err error
-			var words []string
-
-			if allowWordExpansion[cmd] {
-				words, err = ProcessWords(str, envs, b.directive.EscapeToken)
-				if err != nil {
-					return err
-				}
-				strList = append(strList, words...)
-			} else {
-				str, err = ProcessWord(str, envs, b.directive.EscapeToken)
-				if err != nil {
-					return err
-				}
-				strList = append(strList, str)
-			}
-		} else {
-			strList = append(strList, str)
+		words, err := b.evaluateEnv(cmd, ast.Value, envs)
+		if err != nil {
+			return err
 		}
+		strList = append(strList, words...)
 		msgList[i] = ast.Value
-		i++
 	}
 
 	msg += " " + strings.Join(msgList, " ")
@@ -186,6 +157,29 @@ func (b *Builder) dispatch(stepN int, stepTotal int, ast *parser.Node) error {
 	return fmt.Errorf("Unknown instruction: %s", upperCasedCmd)
 }
 
+// count the number of nodes that we are going to traverse first
+// allocation of those list a lot when they have a lot of arguments
+func initMsgList(cursor *parser.Node) []string {
+	var n int
+	for ; cursor.Next != nil; n++ {
+		cursor = cursor.Next
+	}
+	return make([]string, n)
+}
+
+func (b *Builder) evaluateEnv(cmd string, str string, envs []string) ([]string, error) {
+	if !replaceEnvAllowed[cmd] {
+		return []string{str}, nil
+	}
+	var processFunc func(string, []string, rune) ([]string, error)
+	if allowWordExpansion[cmd] {
+		processFunc = ProcessWords
+	} else {
+		processFunc = ProcessWord
+	}
+	return processFunc(str, envs, b.directive.EscapeToken)
+}
+
 // buildArgsWithoutConfigEnv returns a list of key=value pairs for all the build
 // args that are not overriden by runConfig environment variables.
 func (b *Builder) buildArgsWithoutConfigEnv() []string {

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,88 @@ 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)
+		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
+	}
 
-		if len(strs) < 2 {
-			return nil, nil, fmt.Errorf(key + " must have two arguments")
+	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)
 		}
 
-		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)
+		parts := strings.SplitN(word, "=", 2)
+		node := newKeyValueNode(parts[0], parts[1])
+		rootNode, prevNode = appendKeyValueNode(node, rootNode, prevNode)
+	}
 
-			name := &Node{}
-			value := &Node{}
+	return rootNode, nil
+}
 
-			name.Next = value
-			name.Value = parts[0]
-			value.Value = parts[1]
+func newKeyValueNode(key, value string) *Node {
+	return &Node{
+		Value: key,
+		Next:  &Node{Value: value},
+	}
+}
 
-			if i == 0 {
-				rootnode = name
-			} else {
-				prevNode.Next = name
-			}
-			prevNode = value
-		}
+func appendKeyValueNode(node, rootNode, prevNode *Node) (*Node, *Node) {
+	if rootNode == nil {
+		rootNode = node
+	}
+	if prevNode != nil {
+		prevNode.Next = node
 	}
 
-	return rootnode, nil, nil
+	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))
+		// 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

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)
+
+}

builder/dockerfile/parser/parser.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"io"
 	"regexp"
+	"strconv"
 	"strings"
 	"unicode"
 
@@ -36,6 +37,31 @@ type Node struct {
 	EndLine    int             // the line in the original dockerfile where the node ends
 }
 
+// Dump dumps the AST defined by `node` as a list of sexps.
+// Returns a string suitable for printing.
+func (node *Node) Dump() string {
+	str := ""
+	str += node.Value
+
+	if len(node.Flags) > 0 {
+		str += fmt.Sprintf(" %q", node.Flags)
+	}
+
+	for _, n := range node.Children {
+		str += "(" + n.Dump() + ")\n"
+	}
+
+	for n := node.Next; n != nil; n = n.Next {
+		if len(n.Children) > 0 {
+			str += " " + n.Dump()
+		} else {
+			str += " " + strconv.Quote(n.Value)
+		}
+	}
+
+	return strings.TrimSpace(str)
+}
+
 // Directive is the structure used during a build run to hold the state of
 // parsing directives.
 type Directive struct {
@@ -96,24 +122,9 @@ func init() {
 
 // ParseLine parses a line and returns the remainder.
 func ParseLine(line string, d *Directive, ignoreCont bool) (string, *Node, error) {
-	// Handle the parser directive '# escape=<char>. Parser directives must precede
-	// any builder instruction or other comments, and cannot be repeated.
-	if d.LookingForDirectives {
-		tecMatch := tokenEscapeCommand.FindStringSubmatch(strings.ToLower(line))
-		if len(tecMatch) > 0 {
-			if d.EscapeSeen == true {
-				return "", nil, fmt.Errorf("only one escape parser directive can be used")
-			}
-			for i, n := range tokenEscapeCommand.SubexpNames() {
-				if n == "escapechar" {
-					if err := SetEscapeToken(tecMatch[i], d); err != nil {
-						return "", nil, err
-					}
-					d.EscapeSeen = true
-					return "", nil, nil
-				}
-			}
-		}
+	if escapeFound, err := handleParserDirective(line, d); err != nil || escapeFound {
+		d.EscapeSeen = escapeFound
+		return "", nil, err
 	}
 
 	d.LookingForDirectives = false
@@ -127,25 +138,60 @@ func ParseLine(line string, d *Directive, ignoreCont bool) (string, *Node, error
 		return line, nil, nil
 	}
 
+	node, err := newNodeFromLine(line, d)
+	return "", node, err
+}
+
+// newNodeFromLine splits the line into parts, and dispatches to a function
+// based on the command and command arguments. A Node is created from the
+// result of the dispatch.
+func newNodeFromLine(line string, directive *Directive) (*Node, error) {
 	cmd, flags, args, err := splitCommand(line)
 	if err != nil {
-		return "", nil, err
+		return nil, err
 	}
 
-	node := &Node{}
-	node.Value = cmd
-
-	sexp, attrs, err := fullDispatch(cmd, args, d)
+	fn := dispatch[cmd]
+	// Ignore invalid Dockerfile instructions
+	if fn == nil {
+		fn = parseIgnore
+	}
+	next, attrs, err := fn(args, directive)
 	if err != nil {
-		return "", nil, err
+		return nil, err
 	}
 
-	node.Next = sexp
-	node.Attributes = attrs
-	node.Original = line
-	node.Flags = flags
+	return &Node{
+		Value:      cmd,
+		Original:   line,
+		Flags:      flags,
+		Next:       next,
+		Attributes: attrs,
+	}, nil
+}
 
-	return "", node, nil
+// Handle the parser directive '# escape=<char>. Parser directives must precede
+// any builder instruction or other comments, and cannot be repeated.
+func handleParserDirective(line string, d *Directive) (bool, error) {
+	if !d.LookingForDirectives {
+		return false, nil
+	}
+	tecMatch := tokenEscapeCommand.FindStringSubmatch(strings.ToLower(line))
+	if len(tecMatch) == 0 {
+		return false, nil
+	}
+	if d.EscapeSeen == true {
+		return false, fmt.Errorf("only one escape parser directive can be used")
+	}
+	for i, n := range tokenEscapeCommand.SubexpNames() {
+		if n == "escapechar" {
+			if err := SetEscapeToken(tecMatch[i], d); err != nil {
+				return false, err
+			}
+			return true, nil
+		}
+	}
+	return false, nil
 }
 
 // Parse is the main parse routine.
@@ -219,3 +265,14 @@ func Parse(rwc io.Reader, d *Directive) (*Node, error) {
 
 	return root, nil
 }
+
+// covers comments and empty lines. Lines should be trimmed before passing to
+// this function.
+func stripComments(line string) string {
+	// string is already trimmed at this point
+	if tokenComment.MatchString(line) {
+		return tokenComment.ReplaceAllString(line, "")
+	}
+
+	return line
+}

builder/dockerfile/parser/utils.go → builder/dockerfile/parser/split_command.go

@@ -1,55 +1,10 @@
 package parser
 
 import (
-	"fmt"
-	"strconv"
 	"strings"
 	"unicode"
 )
 
-// Dump dumps the AST defined by `node` as a list of sexps.
-// Returns a string suitable for printing.
-func (node *Node) Dump() string {
-	str := ""
-	str += node.Value
-
-	if len(node.Flags) > 0 {
-		str += fmt.Sprintf(" %q", node.Flags)
-	}
-
-	for _, n := range node.Children {
-		str += "(" + n.Dump() + ")\n"
-	}
-
-	for n := node.Next; n != nil; n = n.Next {
-		if len(n.Children) > 0 {
-			str += " " + n.Dump()
-		} else {
-			str += " " + strconv.Quote(n.Value)
-		}
-	}
-
-	return strings.TrimSpace(str)
-}
-
-// performs the dispatch based on the two primal strings, cmd and args. Please
-// look at the dispatch table in parser.go to see how these dispatchers work.
-func fullDispatch(cmd, args string, d *Directive) (*Node, map[string]bool, error) {
-	fn := dispatch[cmd]
-
-	// Ignore invalid Dockerfile instructions
-	if fn == nil {
-		fn = parseIgnore
-	}
-
-	sexp, attrs, err := fn(args, d)
-	if err != nil {
-		return nil, nil, err
-	}
-
-	return sexp, attrs, nil
-}
-
 // splitCommand takes a single line of text and parses out the cmd and args,
 // which are used for dispatching to more exact parsing functions.
 func splitCommand(line string) (string, []string, string, error) {
@@ -71,17 +26,6 @@ func splitCommand(line string) (string, []string, string, error) {
 	return cmd, flags, strings.TrimSpace(args), nil
 }
 
-// covers comments and empty lines. Lines should be trimmed before passing to
-// this function.
-func stripComments(line string) string {
-	// string is already trimmed at this point
-	if tokenComment.MatchString(line) {
-		return tokenComment.ReplaceAllString(line, "")
-	}
-
-	return line
-}
-
 func extractBuilderFlags(line string) (string, []string, error) {
 	// Parses the BuilderFlags and returns the remaining part of the line
 

builder/dockerfile/shell_parser.go

@@ -24,16 +24,9 @@ type shellWord struct {
 
 // ProcessWord will use the 'env' list of environment variables,
 // and replace any env var references in 'word'.
-func ProcessWord(word string, env []string, escapeToken rune) (string, error) {
-	sw := &shellWord{
-		word:        word,
-		envs:        env,
-		pos:         0,
-		escapeToken: escapeToken,
-	}
-	sw.scanner.Init(strings.NewReader(word))
-	word, _, err := sw.process()
-	return word, err
+func ProcessWord(word string, env []string, escapeToken rune) ([]string, error) {
+	word, _, err := process(word, env, escapeToken)
+	return []string{word}, err
 }
 
 // ProcessWords will use the 'env' list of environment variables,
@@ -44,6 +37,11 @@ func ProcessWord(word string, env []string, escapeToken rune) (string, error) {
 // Note, each one is trimmed to remove leading and trailing spaces (unless
 // they are quoted", but ProcessWord retains spaces between words.
 func ProcessWords(word string, env []string, escapeToken rune) ([]string, error) {
+	_, words, err := process(word, env, escapeToken)
+	return words, err
+}
+
+func process(word string, env []string, escapeToken rune) (string, []string, error) {
 	sw := &shellWord{
 		word:        word,
 		envs:        env,
@@ -51,8 +49,7 @@ func ProcessWords(word string, env []string, escapeToken rune) ([]string, error)
 		escapeToken: escapeToken,
 	}
 	sw.scanner.Init(strings.NewReader(word))
-	_, words, err := sw.process()
-	return words, err
+	return sw.process()
 }
 
 func (sw *shellWord) process() (string, []string, error) {

builder/dockerfile/shell_parser_test.go

@@ -6,6 +6,8 @@ import (
 	"runtime"
 	"strings"
 	"testing"
+
+	"github.com/docker/docker/pkg/testutil/assert"
 )
 
 func TestShellParser4EnvVars(t *testing.T) {
@@ -13,9 +15,7 @@ func TestShellParser4EnvVars(t *testing.T) {
 	lineCount := 0
 
 	file, err := os.Open(fn)
-	if err != nil {
-		t.Fatalf("Can't open '%s': %s", err, fn)
-	}
+	assert.NilError(t, err)
 	defer file.Close()
 
 	scanner := bufio.NewScanner(file)
@@ -36,29 +36,25 @@ func TestShellParser4EnvVars(t *testing.T) {
 		}
 
 		words := strings.Split(line, "|")
-		if len(words) != 3 {
-			t.Fatalf("Error in '%s' - should be exactly one | in:%q", fn, line)
-		}
+		assert.Equal(t, len(words), 3)
 
-		words[0] = strings.TrimSpace(words[0])
-		words[1] = strings.TrimSpace(words[1])
-		words[2] = strings.TrimSpace(words[2])
+		platform := strings.TrimSpace(words[0])
+		source := strings.TrimSpace(words[1])
+		expected := strings.TrimSpace(words[2])
 
 		// Key W=Windows; A=All; U=Unix
-		if (words[0] != "W") && (words[0] != "A") && (words[0] != "U") {
-			t.Fatalf("Invalid tag %s at line %d of %s. Must be W, A or U", words[0], lineCount, fn)
+		if platform != "W" && platform != "A" && platform != "U" {
+			t.Fatalf("Invalid tag %s at line %d of %s. Must be W, A or U", platform, lineCount, fn)
 		}
 
-		if ((words[0] == "W" || words[0] == "A") && runtime.GOOS == "windows") ||
-			((words[0] == "U" || words[0] == "A") && runtime.GOOS != "windows") {
-			newWord, err := ProcessWord(words[1], envs, '\\')
-
-			if err != nil {
-				newWord = "error"
-			}
-
-			if newWord != words[2] {
-				t.Fatalf("Error. Src: %s  Calc: %s  Expected: %s at line %d", words[1], newWord, words[2], lineCount)
+		if ((platform == "W" || platform == "A") && runtime.GOOS == "windows") ||
+			((platform == "U" || platform == "A") && runtime.GOOS != "windows") {
+			newWord, err := ProcessWord(source, envs, '\\')
+			if expected == "error" {
+				assert.Error(t, err, "")
+			} else {
+				assert.NilError(t, err)
+				assert.DeepEqual(t, newWord, []string{expected})
 			}
 		}
 	}