Merge pull request #24725 from yongtang/24693-dockerfile-empty-line-after-escape

Fix dockerfile parser with empty line after escape

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

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] {

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.* \

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.* \

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.* \

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

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")

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

builder/dockerfile/parser/testfiles/empty-line-after-escape/result

@@ -0,0 +1,3 @@
+(from "busybox")
+(run "foo")
+(bar "")

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" ]

integration-cli/docker_cli_build_test.go

@@ -3610,8 +3610,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
@@ -7139,3 +7139,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")
+}