Repository: uber/makisu
Pull request #321 (2020-04-23 14:19:27)

Entrypoint: Handle shell form
I did not run the integration tests so this may break everything...

Rowern: Should fix: https://github.com/uber/makisu/issues/294
yiranwang52: 2 unit tests need update.

Patch:
diff --git a/lib/parser/dockerfile/arg.go b/lib/parser/dockerfile/arg.go
index e488cad..0516131 100644
--- a/lib/parser/dockerfile/arg.go
+++ b/lib/parser/dockerfile/arg.go
@@ -45,7 +45,7 @@ func newArgDirective(base *baseDirective, state *parsingState) (Directive, error
 		return &ArgDirective{base, name, defaultVal, nil}, nil
 	}
 
-	args, err := splitArgs(base.Args)
+	args, err := splitArgs(base.Args, false)
 	if err != nil {
 		return nil, base.err(err)
 	}
diff --git a/lib/parser/dockerfile/base.go b/lib/parser/dockerfile/base.go
index 68682b5..237218a 100644
--- a/lib/parser/dockerfile/base.go
+++ b/lib/parser/dockerfile/base.go
@@ -32,6 +32,31 @@ type baseDirective struct {
 	Commit bool
 }
 
+// uncomment the line
+func uncomment(line string) string {
+	r := regexp.MustCompile(`#`)
+	for _, idx := range r.FindAllStringIndex(line, -1) {
+		char := idx[0]
+		qStatus := make(map[string]bool)
+		// Check for all types of quotes.
+		for _, qRune := range `'"` {
+			qStr := string(qRune)
+			// If there is an even number of quotes of this type to the left of #, mark the iteration as successful.
+			if strings.Count(line[:char], qStr)%2 == 0 {
+				qStatus[qStr] = true
+				// If this is the last # and there are an odd number of quotes of this type on the left, return the part of the line before #.
+			} else if strings.LastIndex(line, `#`) == char && strings.Count(line[char:], qStr)%2 == 0 {
+				return line[:char]
+			}
+		}
+		if qStatus[`'`] && qStatus[`"`] {
+			return line[:char]
+		}
+	}
+	// By default return the original line.
+	return line
+}
+
 // newBaseDirective strips and splits the input line. If the line contains only whitespace
 // or is empty, returns nil, nil. If the line doesn't contain a directive and arguments,
 // returns an error.
@@ -41,7 +66,7 @@ func newBaseDirective(line string) (*baseDirective, error) {
 	var commit bool
 	if commentIndex := strings.Index(line, "#"); commentIndex != -1 {
 		commit = commitRegexp.MatchString(strings.ToLower(line[commentIndex:]))
-		line = line[:commentIndex]
+		line = uncomment(line)
 	}
 
 	trimmed := strings.TrimSpace(line)
diff --git a/lib/parser/dockerfile/cmd.go b/lib/parser/dockerfile/cmd.go
index 9ad5859..b7206a3 100644
--- a/lib/parser/dockerfile/cmd.go
+++ b/lib/parser/dockerfile/cmd.go
@@ -34,7 +34,7 @@ func newCmdDirective(base *baseDirective, state *parsingState) (Directive, error
 		return &CmdDirective{base, cmd}, nil
 	}
 
-	args, err := splitArgs(base.Args)
+	args, err := splitArgs(base.Args, false)
 	if err != nil {
 		return nil, base.err(err)
 	}
diff --git a/lib/parser/dockerfile/entrypoint.go b/lib/parser/dockerfile/entrypoint.go
index 726df64..694d842 100644
--- a/lib/parser/dockerfile/entrypoint.go
+++ b/lib/parser/dockerfile/entrypoint.go
@@ -14,6 +14,8 @@
 
 package dockerfile
 
+import "strings"
+
 // EntrypointDirective represents the "ENTRYPOINT" dockerfile command.
 type EntrypointDirective struct {
 	*baseDirective
@@ -34,11 +36,17 @@ func newEntrypointDirective(base *baseDirective, state *parsingState) (Directive
 		return &EntrypointDirective{base, entrypoint}, nil
 	}
 
-	args, err := splitArgs(base.Args)
+	// This is the Shell form (https://docs.docker.com/engine/reference/builder/#shell-form-entrypoint-example)
+	// It is expected to wrap the whole entrypoint into a sh -c command
+	var shell []string
+	shell = append(shell, "/bin/sh", "-c")
+	args, err := splitArgs(base.Args, true)
 	if err != nil {
 		return nil, base.err(err)
 	}
-	return &EntrypointDirective{base, args}, nil
+
+	cmd := append(shell, strings.Join(args, " "))
+	return &EntrypointDirective{base, cmd}, nil
 }
 
 // Add this command to the build stage.
diff --git a/lib/parser/dockerfile/healthcheck.go b/lib/parser/dockerfile/healthcheck.go
index 19476ca..5757446 100644
--- a/lib/parser/dockerfile/healthcheck.go
+++ b/lib/parser/dockerfile/healthcheck.go
@@ -54,7 +54,7 @@ func newHealthcheckDirective(base *baseDirective, state *parsingState) (Directiv
 		return nil, base.err(fmt.Errorf("CMD not defined"))
 	}
 
-	flags, err := splitArgs(base.Args[:cmdIndices[0]])
+	flags, err := splitArgs(base.Args[:cmdIndices[0]], false)
 	if err != nil {
 		return nil, fmt.Errorf("failed to parse interval")
 	}
@@ -133,7 +133,7 @@ func newHealthcheckDirective(base *baseDirective, state *parsingState) (Directiv
 	}
 
 	// Verify cmd arg is a valid array, but return the whole arg as one string.
-	args, err := splitArgs(remaining)
+	args, err := splitArgs(remaining, false)
 	if err != nil {
 		return nil, base.err(err)
 	}
diff --git a/lib/parser/dockerfile/split_args.go b/lib/parser/dockerfile/split_args.go
index 8514d6b..6ce7ef1 100644
--- a/lib/parser/dockerfile/split_args.go
+++ b/lib/parser/dockerfile/split_args.go
@@ -22,10 +22,10 @@ import (
 
 // splitArgs splits a whitespace-delimited string into an array of arguments,
 // not splitting quoted arguments.
-func splitArgs(input string) ([]string, error) {
+func splitArgs(input string, keepQuotes bool) ([]string, error) {
 	var err error
 	var state splitArgsState = &splitArgsStateSpace{
-		&splitArgsBase{args: make([]string, 0)},
+		&splitArgsBase{args: make([]string, 0), keepQuotes: keepQuotes},
 	}
 	for i := 0; i < len(input); i++ {
 		state, err = state.nextRune(rune(input[i]))
@@ -45,9 +45,10 @@ type splitArgsState interface {
 
 // splitArgsBase defines pieces of data that are used & managed by each state.
 type splitArgsBase struct {
-	args    []string
-	currArg string
-	escaped bool
+	args       []string
+	currArg    string
+	escaped    bool
+	keepQuotes bool
 }
 
 // splitArgsStateSpace is the starting state for the state machine. It should be entered
@@ -60,6 +61,9 @@ func (s *splitArgsStateSpace) nextRune(r rune) (splitArgsState, error) {
 	if unicode.IsSpace(r) {
 		return s, nil
 	} else if r == '"' {
+		if s.keepQuotes {
+			s.currArg += "\""
+		}
 		return &splitArgsStateQuote{s.splitArgsBase}, nil
 	} else if r == '\\' {
 		s.escaped = true
@@ -107,7 +111,7 @@ type splitArgsStateQuote struct{ *splitArgsBase }
 // transitioning to splitArgsStateEndQuote.
 func (s *splitArgsStateQuote) nextRune(r rune) (splitArgsState, error) {
 	if s.escaped {
-		if r != '"' {
+		if r != '"' || s.keepQuotes {
 			s.currArg += "\\"
 		}
 		s.escaped = false
@@ -115,6 +119,9 @@ func (s *splitArgsStateQuote) nextRune(r rune) (splitArgsState, error) {
 		s.escaped = true
 		return s, nil
 	} else if r == '"' {
+		if s.keepQuotes {
+			s.currArg += "\""
+		}
 		s.args = append(s.args, s.currArg)
 		s.currArg = ""
 		return &splitArgsStateEndQuote{s.splitArgsBase}, nil


Test patch:
diff --git a/lib/parser/dockerfile/entrypoint_test.go b/lib/parser/dockerfile/entrypoint_test.go
index 94a1868..c9e8841 100644
--- a/lib/parser/dockerfile/entrypoint_test.go
+++ b/lib/parser/dockerfile/entrypoint_test.go
@@ -33,8 +33,9 @@ func TestNewEntrypointDirective(t *testing.T) {
 		{"good json", true, `entrypoint ["this", "entrypoint"]`, []string{"this", "entrypoint"}},
 		{"substitution", true, `entrypoint ["${prefix}this", "entrypoint${suffix}"]`, []string{"test_this", "entrypoint_test"}},
 		{"substitution2", true, `entrypoint ["this"$comma "entrypoint"]`, []string{"this", "entrypoint"}},
-		{"good entrypoint", true, "entrypoint this entrypoint", []string{"this", "entrypoint"}},
-		{"substitution", true, "entrypoint ${prefix}this entrypoint$suffix", []string{"test_this", "entrypoint_test"}},
+		{"good entrypoint", true, "entrypoint this entrypoint", []string{"/bin/sh", "-c", "this entrypoint"}},
+		{"substitution", true, "entrypoint ${prefix}this entrypoint$suffix", []string{"/bin/sh", "-c", "test_this entrypoint_test"}},
+		{"substitution", true, `entrypoint "${prefix}this" entrypoint$suffix`, []string{"/bin/sh", "-c", `"test_this" entrypoint_test`}},
 		{"bad json", false, `entrypoint ["this, "entrypoint"]`, nil},
 		{"bad substitution", false, `entrypoint ["${prefixthis", "entrypoint${suffix}"]`, nil},
 	}
diff --git a/lib/parser/dockerfile/split_args_test.go b/lib/parser/dockerfile/split_args_test.go
index bc5eb36..ccff8a7 100644
--- a/lib/parser/dockerfile/split_args_test.go
+++ b/lib/parser/dockerfile/split_args_test.go
@@ -22,24 +22,28 @@ import (
 
 func TestSplitArgs(t *testing.T) {
 	tests := []struct {
-		desc    string
-		input   string
-		succeed bool
-		output  []string
+		desc       string
+		input      string
+		keepQuotes bool
+		succeed    bool
+		output     []string
 	}{
-		{"single", "  a	", true, []string{"a"}},
-		{"single quoted", `  "a   "	`, true, []string{"a   "}},
-		{"quoted contains backslash", `  "a \  "	`, true, []string{`a \  `}},
-		{"quoted contains quote", `  "a \"  "	`, true, []string{`a "  `}},
-		{"single quoted incomplete", `"a `, false, nil},
-		{"multiple", `a=b   c d  "e f" \"g`, true, []string{"a=b", "c", "d", "e f", "\"g"}},
-		{"arg chars", "`1'{}$@#! *&@)(*_&", true, []string{"`1'{}$@#!", "*&@)(*_&"}},
-		{"non-escapes", "\\`1'{}$\\@#! *&\\;)\\(*_&", true, []string{"\\`1'{}$\\@#!", "*&\\;)\\(*_&"}},
+		{"single", "  a	", false, true, []string{"a"}},
+		{"single quoted", `  "a   "	`, false, true, []string{"a   "}},
+		{"quoted contains backslash", `  "a \  "	`, false, true, []string{`a \  `}},
+		{"quoted contains quote", `  "a \"  "	`, false, true, []string{`a "  `}},
+		{"single quoted incomplete", `"a `, false, false, nil},
+		{"multiple", `a=b   c d  "e f" \"g`, false, true, []string{"a=b", "c", "d", "e f", "\"g"}},
+		{"arg chars", "`1'{}$@#! *&@)(*_&", false, true, []string{"`1'{}$@#!", "*&@)(*_&"}},
+		{"non-escapes", "\\`1'{}$\\@#! *&\\;)\\(*_&", false, true, []string{"\\`1'{}$\\@#!", "*&\\;)\\(*_&"}},
+		{"keep quotes", `echo "${MESSAGE}" "multi \"word"`, true, true, []string{"echo", `"${MESSAGE}"`, `"multi \"word"`}},
+		{"non-escapes", `echo "${MESSAGE}" "multi \"word"`, true, true, []string{"echo", `"${MESSAGE}"`, `"multi \"word"`}},
+		{"non-escapes", `echo "single argument \\\"keepQuotes\\\""`, true, true, []string{"echo", `"single argument \\\"keepQuotes\\\""`}},
 	}
 	for _, test := range tests {
 		t.Run(test.desc, func(t *testing.T) {
 			require := require.New(t)
-			result, err := splitArgs(test.input)
+			result, err := splitArgs(test.input, test.keepQuotes)
 			if test.succeed {
 				require.NoError(err)
 				require.Equal(test.output, result)