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 <yong.tang.github@outlook.com>

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)) == "" {
+				// If escape followed by a comment line then stop
-					continue
+				// 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 {
+	if ast.StartLine != 5 || ast.EndLine != 27 {
-		fmt.Fprintf(os.Stderr, "Wrong root line information: expected(%d-%d), actual(%d-%d)\n", 5, 31, ast.StartLine, ast.EndLine)
+		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 that breaks escape continuation
-
+RUN echo this is some more useful stuff
-# this is a comment with a blank line surrounding it
-
-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

@@ -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")
+}