From d0cdce5ce4cd2fbfdb5668017d71539aafea4558 Mon Sep 17 00:00:00 2001 From: Tianon Gravi Date: Fri, 2 Jan 2015 22:36:58 -0700 Subject: [PATCH 1/3] Simplify builder ast.Dump by using strconv.Quote instead of a custom QuoteString function (the only change to existing files is literal tabs becoming \t, but future files may use nonprintable characters and the like now) Signed-off-by: Andrew "Tianon" Page Upstream-commit: 1edf16e977a3dd18f9ff15b4acf40b0c1ac64788 Component: engine --- .../testfiles/brimstone-consuldock/result | 2 +- .../testfiles/brimstone-docker-consul/result | 6 ++--- .../builder/parser/testfiles/docker/result | 4 ++-- components/engine/builder/parser/utils.go | 24 ++----------------- 4 files changed, 8 insertions(+), 28 deletions(-) diff --git a/components/engine/builder/parser/testfiles/brimstone-consuldock/result b/components/engine/builder/parser/testfiles/brimstone-consuldock/result index cc8fab2131..227f748cda 100644 --- a/components/engine/builder/parser/testfiles/brimstone-consuldock/result +++ b/components/engine/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") diff --git a/components/engine/builder/parser/testfiles/brimstone-docker-consul/result b/components/engine/builder/parser/testfiles/brimstone-docker-consul/result index 8c989e6211..16492e516a 100644 --- a/components/engine/builder/parser/testfiles/brimstone-docker-consul/result +++ b/components/engine/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") diff --git a/components/engine/builder/parser/testfiles/docker/result b/components/engine/builder/parser/testfiles/docker/result index 80f219ecb4..773b640a9d 100644 --- a/components/engine/builder/parser/testfiles/docker/result +++ b/components/engine/builder/parser/testfiles/docker/result @@ -1,13 +1,13 @@ (from "ubuntu:14.04") (maintainer "Tianon Gravi (@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") diff --git a/components/engine/builder/parser/utils.go b/components/engine/builder/parser/utils.go index 096c4e31e9..3a8cd24e8c 100644 --- a/components/engine/builder/parser/utils.go +++ b/components/engine/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) } } } From 74fe90f08949f598e933e4153a2e564a3d44e759 Mon Sep 17 00:00:00 2001 From: Tianon Gravi Date: Fri, 2 Jan 2015 22:38:52 -0700 Subject: [PATCH 2/3] Simplify builder TestTestData slightly by using ioutil.ReadFile instead of os.Open+ioutil.ReadAll Signed-off-by: Andrew "Tianon" Page Upstream-commit: f6cb1ea85a7a75193644b8ba4516cb586c5dc52f Component: engine --- components/engine/builder/parser/parser_test.go | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/components/engine/builder/parser/parser_test.go b/components/engine/builder/parser/parser_test.go index 1b517fcc14..daceb9839c 100644 --- a/components/engine/builder/parser/parser_test.go +++ b/components/engine/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() } } From 3bf489de75c89685c59b6c0e33a6aae1c8cc6e9c Mon Sep 17 00:00:00 2001 From: Tianon Gravi Date: Fri, 2 Jan 2015 22:40:43 -0700 Subject: [PATCH 3/3] 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) This also adds more tests to help verify this, including unicode and nonprintable characters (hence the earlier commit switching to strconv.Quote). As a bonus, this fixes a subtle bug where [] was turned into [""] and then turned back into [] (and thus [""] was impossible to actually round-trip correctly in a Dockerfile). Signed-off-by: Andrew "Tianon" Page Upstream-commit: 05c2d2db9a95266217a639e2109be6fc6482a716 Component: engine --- components/engine/builder/parser/json_test.go | 55 +++++++++++++++++++ .../engine/builder/parser/line_parsers.go | 40 ++++++-------- components/engine/builder/parser/parser.go | 5 +- .../builder/parser/testfiles/json/Dockerfile | 8 +++ .../builder/parser/testfiles/json/result | 8 +++ 5 files changed, 88 insertions(+), 28 deletions(-) create mode 100644 components/engine/builder/parser/json_test.go create mode 100644 components/engine/builder/parser/testfiles/json/Dockerfile create mode 100644 components/engine/builder/parser/testfiles/json/result diff --git a/components/engine/builder/parser/json_test.go b/components/engine/builder/parser/json_test.go new file mode 100644 index 0000000000..a256f845d3 --- /dev/null +++ b/components/engine/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) + } + } +} diff --git a/components/engine/builder/parser/line_parsers.go b/components/engine/builder/parser/line_parsers.go index abde85d292..8a94e1e5db 100644 --- a/components/engine/builder/parser/line_parsers.go +++ b/components/engine/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 } diff --git a/components/engine/builder/parser/parser.go b/components/engine/builder/parser/parser.go index ad42a1586e..4360623e6f 100644 --- a/components/engine/builder/parser/parser.go +++ b/components/engine/builder/parser/parser.go @@ -85,10 +85,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 diff --git a/components/engine/builder/parser/testfiles/json/Dockerfile b/components/engine/builder/parser/testfiles/json/Dockerfile new file mode 100644 index 0000000000..a586917110 --- /dev/null +++ b/components/engine/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"] diff --git a/components/engine/builder/parser/testfiles/json/result b/components/engine/builder/parser/testfiles/json/result new file mode 100644 index 0000000000..c6553e6e1a --- /dev/null +++ b/components/engine/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")