From 3e1b539e8d0ed4abf695b0a8c42346fba6d5a6b0 Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Sat, 16 Jul 2016 12:37:16 -0700 Subject: [PATCH] Fix dockerfile parser with empty line after escape This fix tries to fix the bug reported by #24693 where an empty line after escape will not be stopped by the parser. This fix addresses this issue by stop the parser from continue with an empty line after escape. An additional integration test has been added. This fix fixes #24693. Signed-off-by: Yong Tang --- builder/dockerfile/parser/parser.go | 11 +++++- builder/dockerfile/parser/parser_test.go | 6 +-- .../parser/testfile-line/Dockerfile | 4 -- .../testfiles/brimstone-consuldock/Dockerfile | 2 - .../brimstone-docker-consul/Dockerfile | 4 -- .../testfiles/continueIndent/Dockerfile | 7 +--- .../parser/testfiles/continueIndent/result | 3 +- .../empty-line-after-escape/Dockerfile | 15 ++++++++ .../testfiles/empty-line-after-escape/result | 3 ++ .../parser/testfiles/escapes/Dockerfile | 2 - integration-cli/docker_cli_build_test.go | 37 ++++++++++++++++++- 11 files changed, 70 insertions(+), 24 deletions(-) create mode 100644 builder/dockerfile/parser/testfiles/empty-line-after-escape/Dockerfile create mode 100644 builder/dockerfile/parser/testfiles/empty-line-after-escape/result diff --git a/builder/dockerfile/parser/parser.go b/builder/dockerfile/parser/parser.go index d89bdfbaf4..71c2c0d811 100644 --- a/builder/dockerfile/parser/parser.go +++ b/builder/dockerfile/parser/parser.go @@ -176,10 +176,17 @@ func Parse(rwc io.Reader, d *Directive) (*Node, error) { newline := scanner.Text() currentLine++ - if stripComments(strings.TrimSpace(newline)) == "" { - continue + // If escape followed by a comment line then stop + // Note here that comment line starts with `#` at + // the first pos of the line + if stripComments(newline) == "" { + break } + // If escape followed by an empty line then stop + if strings.TrimSpace(newline) == "" { + break + } line, child, err = ParseLine(line+newline, d) if err != nil { return nil, err diff --git a/builder/dockerfile/parser/parser_test.go b/builder/dockerfile/parser/parser_test.go index e8e26961de..7b8f0b2332 100644 --- a/builder/dockerfile/parser/parser_test.go +++ b/builder/dockerfile/parser/parser_test.go @@ -150,8 +150,8 @@ func TestLineInformation(t *testing.T) { t.Fatalf("Error parsing dockerfile %s: %v", testFileLineInfo, err) } - if ast.StartLine != 5 || ast.EndLine != 31 { - fmt.Fprintf(os.Stderr, "Wrong root line information: expected(%d-%d), actual(%d-%d)\n", 5, 31, ast.StartLine, ast.EndLine) + if ast.StartLine != 5 || ast.EndLine != 27 { + fmt.Fprintf(os.Stderr, "Wrong root line information: expected(%d-%d), actual(%d-%d)\n", 5, 27, ast.StartLine, ast.EndLine) t.Fatalf("Root line information doesn't match result.") } if len(ast.Children) != 3 { @@ -161,7 +161,7 @@ func TestLineInformation(t *testing.T) { expected := [][]int{ {5, 5}, {11, 12}, - {17, 31}, + {17, 27}, } for i, child := range ast.Children { if child.StartLine != expected[i][0] || child.EndLine != expected[i][1] { diff --git a/builder/dockerfile/parser/testfile-line/Dockerfile b/builder/dockerfile/parser/testfile-line/Dockerfile index c7601c9f69..19a26b237d 100644 --- a/builder/dockerfile/parser/testfile-line/Dockerfile +++ b/builder/dockerfile/parser/testfile-line/Dockerfile @@ -16,15 +16,11 @@ ENV GOPATH \ # Install the packages we need, clean up after them and us 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.* \ diff --git a/builder/dockerfile/parser/testfiles/brimstone-consuldock/Dockerfile b/builder/dockerfile/parser/testfiles/brimstone-consuldock/Dockerfile index 0364ef9d96..7827da41cc 100644 --- a/builder/dockerfile/parser/testfiles/brimstone-consuldock/Dockerfile +++ b/builder/dockerfile/parser/testfiles/brimstone-consuldock/Dockerfile @@ -16,10 +16,8 @@ RUN apt-get update \ && 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.* \ diff --git a/builder/dockerfile/parser/testfiles/brimstone-docker-consul/Dockerfile b/builder/dockerfile/parser/testfiles/brimstone-docker-consul/Dockerfile index 25ae352166..3f7bcca3c4 100644 --- a/builder/dockerfile/parser/testfiles/brimstone-docker-consul/Dockerfile +++ b/builder/dockerfile/parser/testfiles/brimstone-docker-consul/Dockerfile @@ -23,14 +23,12 @@ RUN apt-get update \ && 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.* @@ -42,10 +40,8 @@ RUN apt-get update \ && 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.* \ diff --git a/builder/dockerfile/parser/testfiles/continueIndent/Dockerfile b/builder/dockerfile/parser/testfiles/continueIndent/Dockerfile index 97f915f16b..d6ed075922 100644 --- a/builder/dockerfile/parser/testfiles/continueIndent/Dockerfile +++ b/builder/dockerfile/parser/testfiles/continueIndent/Dockerfile @@ -27,8 +27,5 @@ bye\ frog RUN echo hello \ -# this is a comment - -# this is a comment with a blank line surrounding it - -this is some more useful stuff +# this is a comment that breaks escape continuation +RUN echo this is some more useful stuff diff --git a/builder/dockerfile/parser/testfiles/continueIndent/result b/builder/dockerfile/parser/testfiles/continueIndent/result index 76a1cb6f21..85e31160b0 100644 --- a/builder/dockerfile/parser/testfiles/continueIndent/result +++ b/builder/dockerfile/parser/testfiles/continueIndent/result @@ -6,4 +6,5 @@ (run "echo hi world goodnight") (run "echo goodbyefrog") (run "echo goodbyefrog") -(run "echo hello this is some more useful stuff") +(run "echo hello") +(run "echo this is some more useful stuff") diff --git a/builder/dockerfile/parser/testfiles/empty-line-after-escape/Dockerfile b/builder/dockerfile/parser/testfiles/empty-line-after-escape/Dockerfile new file mode 100644 index 0000000000..2157f51f09 --- /dev/null +++ b/builder/dockerfile/parser/testfiles/empty-line-after-escape/Dockerfile @@ -0,0 +1,15 @@ +FROM busybox + +# The following will create two instructions +# `Run foo` +# `bar` +# because empty line will break the escape. +# The parser will generate the following: +# (from "busybox") +# (run "foo") +# (bar "") +# And `bar` will return an error instruction later +# Note: Parse() will not immediately error out. +RUN foo \ + +bar diff --git a/builder/dockerfile/parser/testfiles/empty-line-after-escape/result b/builder/dockerfile/parser/testfiles/empty-line-after-escape/result new file mode 100644 index 0000000000..29305cc3bd --- /dev/null +++ b/builder/dockerfile/parser/testfiles/empty-line-after-escape/result @@ -0,0 +1,3 @@ +(from "busybox") +(run "foo") +(bar "") diff --git a/builder/dockerfile/parser/testfiles/escapes/Dockerfile b/builder/dockerfile/parser/testfiles/escapes/Dockerfile index 1ffb17ef08..05c5d97901 100644 --- a/builder/dockerfile/parser/testfiles/escapes/Dockerfile +++ b/builder/dockerfile/parser/testfiles/escapes/Dockerfile @@ -6,9 +6,7 @@ RUN apt-get \update && \ ADD \conf\\" /.znc RUN foo \ - bar \ - baz CMD [ "\/usr\\\"/bin/znc", "-f", "-r" ] diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index 81f484f3b2..6f2ea1ce7e 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/integration-cli/docker_cli_build_test.go @@ -3577,8 +3577,8 @@ RUN [ "$(id -u):$(id -g)/$(id -un):$(id -gn)/$(id -G):$(id -Gn)" = '1001:1001/do # Switch back to root and double check that worked exactly as we might expect it to USER root +# Add a "supplementary" group for our dockerio user RUN [ "$(id -u):$(id -g)/$(id -un):$(id -gn)/$(id -G):$(id -Gn)" = '0:0/root:root/0 10:root wheel' ] && \ - # Add a "supplementary" group for our dockerio user \ echo 'supplementary:x:1002:dockerio' >> /etc/group # ... and then go verify that we get it like we expect @@ -7106,3 +7106,38 @@ func (s *DockerSuite) TestBuildNetContainer(c *check.C) { host, _ := dockerCmd(c, "run", "testbuildnetcontainer", "cat", "/otherhost") c.Assert(strings.TrimSpace(host), check.Equals, "foobar") } + +// Test case for #24693 +func (s *DockerSuite) TestBuildRunEmptyLineAfterEscape(c *check.C) { + name := "testbuildemptylineafterescape" + _, out, err := buildImageWithOut(name, + ` +FROM busybox +RUN echo x \ + +RUN echo y +RUN echo z +# Comment requires the '#' to start from position 1 +# RUN echo w +`, true) + c.Assert(err, checker.IsNil) + c.Assert(out, checker.Contains, "Step 1/4 : FROM busybox") + c.Assert(out, checker.Contains, "Step 2/4 : RUN echo x") + c.Assert(out, checker.Contains, "Step 3/4 : RUN echo y") + c.Assert(out, checker.Contains, "Step 4/4 : RUN echo z") + + // With comment, see #24693 + name = "testbuildcommentandemptylineafterescape" + _, out, err = buildImageWithOut(name, + ` +FROM busybox +RUN echo grafana && \ + echo raintank \ +#echo env-load +RUN echo vegeta +`, true) + c.Assert(err, checker.IsNil) + c.Assert(out, checker.Contains, "Step 1/3 : FROM busybox") + c.Assert(out, checker.Contains, "Step 2/3 : RUN echo grafana && echo raintank") + c.Assert(out, checker.Contains, "Step 3/3 : RUN echo vegeta") +}