Merge pull request #9881 from tianon/json-array-of-strings

Adjust builder to validate that JSON in Dockerfiles are arrays of strings and nothing else to match how we describe them to people (and what all our existing tests already assumed)

builder/parser/json_test.go

@@ -0,0 +1,55 @@
+package parser
+
+import (
+	"testing"
+)
+
+var invalidJSONArraysOfStrings = []string{
+	`["a",42,"b"]`,
+	`["a",123.456,"b"]`,
+	`["a",{},"b"]`,
+	`["a",{"c": "d"},"b"]`,
+	`["a",["c"],"b"]`,
+	`["a",true,"b"]`,
+	`["a",false,"b"]`,
+	`["a",null,"b"]`,
+}
+
+var validJSONArraysOfStrings = map[string][]string{
+	`[]`:           {},
+	`[""]`:         {""},
+	`["a"]`:        {"a"},
+	`["a","b"]`:    {"a", "b"},
+	`[ "a", "b" ]`: {"a", "b"},
+	`[	"a",	"b"	]`: {"a", "b"},
+	`	[	"a",	"b"	]	`: {"a", "b"},
+	`["abc 123", "♥", "☃", "\" \\ \/ \b \f \n \r \t \u0000"]`: {"abc 123", "♥", "☃", "\" \\ / \b \f \n \r \t \u0000"},
+}
+
+func TestJSONArraysOfStrings(t *testing.T) {
+	for json, expected := range validJSONArraysOfStrings {
+		if node, _, err := parseJSON(json); err != nil {
+			t.Fatalf("%q should be a valid JSON array of strings, but wasn't! (err: %q)", json, err)
+		} else {
+			i := 0
+			for node != nil {
+				if i >= len(expected) {
+					t.Fatalf("expected result is shorter than parsed result (%d vs %d+) in %q", len(expected), i+1, json)
+				}
+				if node.Value != expected[i] {
+					t.Fatalf("expected %q (not %q) in %q at pos %d", expected[i], node.Value, json, i)
+				}
+				node = node.Next
+				i++
+			}
+			if i != len(expected) {
+				t.Fatalf("expected result is longer than parsed result (%d vs %d) in %q", len(expected), i+1, json)
+			}
+		}
+	}
+	for _, json := range invalidJSONArraysOfStrings {
+		if _, _, err := parseJSON(json); err != errDockerfileNotStringArray {
+			t.Fatalf("%q should be an invalid JSON array of strings, but wasn't!", json)
+		}
+	}
+}

builder/parser/line_parsers.go

@@ -10,13 +10,12 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"strconv"
 	"strings"
 	"unicode"
 )
 
 var (
-	errDockerfileJSONNesting = errors.New("You may not nest arrays in Dockerfile statements.")
+	errDockerfileNotStringArray = errors.New("When using JSON array syntax, arrays must be comprised of strings only.")
 )
 
 // ignore the current argument. This will still leave a command parsed, but
@@ -209,34 +208,27 @@ func parseString(rest string) (*Node, map[string]bool, error) {
 
 // parseJSON converts JSON arrays to an AST.
 func parseJSON(rest string) (*Node, map[string]bool, error) {
-	var (
-		myJson   []interface{}
-		next     = &Node{}
-		orignext = next
-		prevnode = next
-	)
-
+	var myJson []interface{}
 	if err := json.Unmarshal([]byte(rest), &myJson); err != nil {
 		return nil, nil, err
 	}
 
+	var top, prev *Node
 	for _, str := range myJson {
-		switch str.(type) {
-		case string:
-		case float64:
-			str = strconv.FormatFloat(str.(float64), 'G', -1, 64)
-		default:
-			return nil, nil, errDockerfileJSONNesting
+		if s, ok := str.(string); !ok {
+			return nil, nil, errDockerfileNotStringArray
+		} else {
+			node := &Node{Value: s}
+			if prev == nil {
+				top = node
+			} else {
+				prev.Next = node
+			}
+			prev = node
 		}
-		next.Value = str.(string)
-		next.Next = &Node{}
-		prevnode = next
-		next = next.Next
 	}
 
-	prevnode.Next = nil
-
-	return orignext, map[string]bool{"json": true}, nil
+	return top, map[string]bool{"json": true}, nil
 }
 
 // parseMaybeJSON determines if the argument appears to be a JSON array. If
@@ -250,7 +242,7 @@ func parseMaybeJSON(rest string) (*Node, map[string]bool, error) {
 	if err == nil {
 		return node, attrs, nil
 	}
-	if err == errDockerfileJSONNesting {
+	if err == errDockerfileNotStringArray {
 		return nil, nil, err
 	}
 
@@ -270,7 +262,7 @@ func parseMaybeJSONToList(rest string) (*Node, map[string]bool, error) {
 	if err == nil {
 		return node, attrs, nil
 	}
-	if err == errDockerfileJSONNesting {
+	if err == errDockerfileNotStringArray {
 		return nil, nil, err
 	}
 

builder/parser/parser.go

@@ -90,10 +90,7 @@ func parseLine(line string) (string, *Node, error) {
 		return "", nil, err
 	}
 
-	if sexp.Value != "" || sexp.Next != nil || sexp.Children != nil {
-		node.Next = sexp
-	}
-
+	node.Next = sexp
 	node.Attributes = attrs
 	node.Original = line
 

builder/parser/parser_test.go

@@ -54,18 +54,14 @@ func TestTestData(t *testing.T) {
 		if err != nil {
 			t.Fatalf("Dockerfile missing for %s: %s", dir.Name(), err.Error())
 		}
-
-		rf, err := os.Open(resultfile)
-		if err != nil {
-			t.Fatalf("Result file missing for %s: %s", dir.Name(), err.Error())
-		}
+		defer df.Close()
 
 		ast, err := Parse(df)
 		if err != nil {
 			t.Fatalf("Error parsing %s's dockerfile: %s", dir.Name(), err.Error())
 		}
 
-		content, err := ioutil.ReadAll(rf)
+		content, err := ioutil.ReadFile(resultfile)
 		if err != nil {
 			t.Fatalf("Error reading %s's result file: %s", dir.Name(), err.Error())
 		}
@@ -75,8 +71,5 @@ func TestTestData(t *testing.T) {
 			fmt.Fprintln(os.Stderr, "Expected:\n"+string(content))
 			t.Fatalf("%s: AST dump of dockerfile does not match result", dir.Name())
 		}
-
-		df.Close()
-		rf.Close()
 	}
 }

builder/parser/testfiles/brimstone-consuldock/result

@@ -2,4 +2,4 @@
 (maintainer "brimstone@the.narro.ws")
 (env "GOPATH" "/go")
 (entrypoint "/usr/local/bin/consuldock")
-(run "apt-get update 	&& dpkg -l | awk '/^ii/ {print $2}' > /tmp/dpkg.clean     && apt-get install -y --no-install-recommends git golang ca-certificates     && apt-get clean     && rm -rf /var/lib/apt/lists 	&& go get -v github.com/brimstone/consuldock     && mv $GOPATH/bin/consuldock /usr/local/bin/consuldock 	&& dpkg -l | awk '/^ii/ {print $2}' > /tmp/dpkg.dirty 	&& apt-get remove --purge -y $(diff /tmp/dpkg.clean /tmp/dpkg.dirty | awk '/^>/ {print $2}') 	&& rm /tmp/dpkg.* 	&& rm -rf $GOPATH")
+(run "apt-get update \t&& dpkg -l | awk '/^ii/ {print $2}' > /tmp/dpkg.clean     && apt-get install -y --no-install-recommends git golang ca-certificates     && apt-get clean     && rm -rf /var/lib/apt/lists \t&& go get -v github.com/brimstone/consuldock     && mv $GOPATH/bin/consuldock /usr/local/bin/consuldock \t&& dpkg -l | awk '/^ii/ {print $2}' > /tmp/dpkg.dirty \t&& apt-get remove --purge -y $(diff /tmp/dpkg.clean /tmp/dpkg.dirty | awk '/^>/ {print $2}') \t&& rm /tmp/dpkg.* \t&& rm -rf $GOPATH")

builder/parser/testfiles/brimstone-docker-consul/result

@@ -2,8 +2,8 @@
 (cmd)
 (entrypoint "/usr/bin/consul" "agent" "-server" "-data-dir=/consul" "-client=0.0.0.0" "-ui-dir=/webui")
 (expose "8500" "8600" "8400" "8301" "8302")
-(run "apt-get update     && apt-get install -y unzip wget 	&& apt-get clean 	&& rm -rf /var/lib/apt/lists")
+(run "apt-get update     && apt-get install -y unzip wget \t&& apt-get clean \t&& rm -rf /var/lib/apt/lists")
 (run "cd /tmp     && wget https://dl.bintray.com/mitchellh/consul/0.3.1_web_ui.zip        -O web_ui.zip     && unzip web_ui.zip     && mv dist /webui     && rm web_ui.zip")
-(run "apt-get update 	&& dpkg -l | awk '/^ii/ {print $2}' > /tmp/dpkg.clean     && apt-get install -y --no-install-recommends unzip wget     && apt-get clean     && rm -rf /var/lib/apt/lists     && cd /tmp     && wget https://dl.bintray.com/mitchellh/consul/0.3.1_web_ui.zip        -O web_ui.zip     && unzip web_ui.zip     && mv dist /webui     && rm web_ui.zip 	&& dpkg -l | awk '/^ii/ {print $2}' > /tmp/dpkg.dirty 	&& apt-get remove --purge -y $(diff /tmp/dpkg.clean /tmp/dpkg.dirty | awk '/^>/ {print $2}') 	&& rm /tmp/dpkg.*")
+(run "apt-get update \t&& dpkg -l | awk '/^ii/ {print $2}' > /tmp/dpkg.clean     && apt-get install -y --no-install-recommends unzip wget     && apt-get clean     && rm -rf /var/lib/apt/lists     && cd /tmp     && wget https://dl.bintray.com/mitchellh/consul/0.3.1_web_ui.zip        -O web_ui.zip     && unzip web_ui.zip     && mv dist /webui     && rm web_ui.zip \t&& dpkg -l | awk '/^ii/ {print $2}' > /tmp/dpkg.dirty \t&& apt-get remove --purge -y $(diff /tmp/dpkg.clean /tmp/dpkg.dirty | awk '/^>/ {print $2}') \t&& rm /tmp/dpkg.*")
 (env "GOPATH" "/go")
-(run "apt-get update 	&& dpkg -l | awk '/^ii/ {print $2}' > /tmp/dpkg.clean     && apt-get install -y --no-install-recommends git golang ca-certificates build-essential     && apt-get clean     && rm -rf /var/lib/apt/lists 	&& go get -v github.com/hashicorp/consul 	&& mv $GOPATH/bin/consul /usr/bin/consul 	&& dpkg -l | awk '/^ii/ {print $2}' > /tmp/dpkg.dirty 	&& apt-get remove --purge -y $(diff /tmp/dpkg.clean /tmp/dpkg.dirty | awk '/^>/ {print $2}') 	&& rm /tmp/dpkg.* 	&& rm -rf $GOPATH")
+(run "apt-get update \t&& dpkg -l | awk '/^ii/ {print $2}' > /tmp/dpkg.clean     && apt-get install -y --no-install-recommends git golang ca-certificates build-essential     && apt-get clean     && rm -rf /var/lib/apt/lists \t&& go get -v github.com/hashicorp/consul \t&& mv $GOPATH/bin/consul /usr/bin/consul \t&& dpkg -l | awk '/^ii/ {print $2}' > /tmp/dpkg.dirty \t&& apt-get remove --purge -y $(diff /tmp/dpkg.clean /tmp/dpkg.dirty | awk '/^>/ {print $2}') \t&& rm /tmp/dpkg.* \t&& rm -rf $GOPATH")

builder/parser/testfiles/docker/result

@@ -1,13 +1,13 @@
 (from "ubuntu:14.04")
 (maintainer "Tianon Gravi <admwiggin@gmail.com> (@tianon)")
-(run "apt-get update && DEBIAN_FRONTEND=noninteractive apt-get install -yq 	apt-utils 	aufs-tools 	automake 	btrfs-tools 	build-essential 	curl 	dpkg-sig 	git 	iptables 	libapparmor-dev 	libcap-dev 	libsqlite3-dev 	lxc=1.0* 	mercurial 	pandoc 	parallel 	reprepro 	ruby1.9.1 	ruby1.9.1-dev 	s3cmd=1.1.0* 	--no-install-recommends")
+(run "apt-get update && DEBIAN_FRONTEND=noninteractive apt-get install -yq \tapt-utils \taufs-tools \tautomake \tbtrfs-tools \tbuild-essential \tcurl \tdpkg-sig \tgit \tiptables \tlibapparmor-dev \tlibcap-dev \tlibsqlite3-dev \tlxc=1.0* \tmercurial \tpandoc \tparallel \treprepro \truby1.9.1 \truby1.9.1-dev \ts3cmd=1.1.0* \t--no-install-recommends")
 (run "git clone --no-checkout https://git.fedorahosted.org/git/lvm2.git /usr/local/lvm2 && cd /usr/local/lvm2 && git checkout -q v2_02_103")
 (run "cd /usr/local/lvm2 && ./configure --enable-static_link && make device-mapper && make install_device-mapper")
 (run "curl -sSL https://golang.org/dl/go1.3.src.tar.gz | tar -v -C /usr/local -xz")
 (env "PATH" "/usr/local/go/bin:$PATH")
 (env "GOPATH" "/go:/go/src/github.com/docker/docker/vendor")
 (run "cd /usr/local/go/src && ./make.bash --no-clean 2>&1")
-(env "DOCKER_CROSSPLATFORMS" "linux/386 linux/arm 	darwin/amd64 darwin/386 	freebsd/amd64 freebsd/386 freebsd/arm")
+(env "DOCKER_CROSSPLATFORMS" "linux/386 linux/arm \tdarwin/amd64 darwin/386 \tfreebsd/amd64 freebsd/386 freebsd/arm")
 (env "GOARM" "5")
 (run "cd /usr/local/go/src && bash -xc 'for platform in $DOCKER_CROSSPLATFORMS; do GOOS=${platform%/*} GOARCH=${platform##*/} ./make.bash --no-clean 2>&1; done'")
 (run "go get golang.org/x/tools/cmd/cover")

builder/parser/testfiles/json/Dockerfile

@@ -0,0 +1,8 @@
+CMD []
+CMD [""]
+CMD ["a"]
+CMD ["a","b"]
+CMD [ "a", "b" ]
+CMD [	"a",	"b"	]
+CMD	[	"a",	"b"	]	
+CMD ["abc 123", "♥", "☃", "\" \\ \/ \b \f \n \r \t \u0000"]

builder/parser/testfiles/json/result

@@ -0,0 +1,8 @@
+(cmd)
+(cmd "")
+(cmd "a")
+(cmd "a" "b")
+(cmd "a" "b")
+(cmd "a" "b")
+(cmd "a" "b")
+(cmd "abc 123" "♥" "☃" "\" \\ / \b \f \n \r \t \x00")

builder/parser/utils.go

@@ -2,30 +2,10 @@ package parser
 
 import (
 	"fmt"
+	"strconv"
 	"strings"
 )
 
-// QuoteString walks characters (after trimming), escapes any quotes and
-// escapes, then wraps the whole thing in quotes. Very useful for generating
-// argument output in nodes.
-func QuoteString(str string) string {
-	result := ""
-	chars := strings.Split(strings.TrimSpace(str), "")
-
-	for _, char := range chars {
-		switch char {
-		case `"`:
-			result += `\"`
-		case `\`:
-			result += `\\`
-		default:
-			result += char
-		}
-	}
-
-	return `"` + result + `"`
-}
-
 // dumps the AST defined by `node` as a list of sexps. Returns a string
 // suitable for printing.
 func (node *Node) Dump() string {
@@ -41,7 +21,7 @@ func (node *Node) Dump() string {
 			if len(n.Children) > 0 {
 				str += " " + n.Dump()
 			} else {
-				str += " " + QuoteString(n.Value)
+				str += " " + strconv.Quote(n.Value)
 			}
 		}
 	}