diff --git a/builder/dockerfile/builder.go b/builder/dockerfile/builder.go index 771dbe837c..219ba2d3c3 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/evaluator.go b/builder/dockerfile/evaluator.go index d0f9a7ca55..08c05daf8f 100644 --- a/builder/dockerfile/evaluator.go +++ b/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 { diff --git a/builder/dockerfile/parser/line_parsers.go b/builder/dockerfile/parser/line_parsers.go index 9b4a4c59f9..0334834e62 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,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) - - 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)) + // 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 diff --git a/builder/dockerfile/parser/line_parsers_test.go b/builder/dockerfile/parser/line_parsers_test.go new file mode 100644 index 0000000000..30b6bdd824 --- /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) + +} diff --git a/builder/dockerfile/parser/parser.go b/builder/dockerfile/parser/parser.go index 1b2dc8f3a0..2a884ceff6 100644 --- a/builder/dockerfile/parser/parser.go +++ b/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=. 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=. 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 +} diff --git a/builder/dockerfile/parser/utils.go b/builder/dockerfile/parser/split_command.go similarity index 66% rename from builder/dockerfile/parser/utils.go rename to builder/dockerfile/parser/split_command.go index 1f77f057ce..171f454f6d 100644 --- a/builder/dockerfile/parser/utils.go +++ b/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 diff --git a/builder/dockerfile/shell_parser.go b/builder/dockerfile/shell_parser.go index 189afd1fdb..2a8b441134 100644 --- a/builder/dockerfile/shell_parser.go +++ b/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) { diff --git a/builder/dockerfile/shell_parser_test.go b/builder/dockerfile/shell_parser_test.go index ca4728d798..0712fde675 100644 --- a/builder/dockerfile/shell_parser_test.go +++ b/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}) } } }