linter: lint rule for using the legacy key/value format with whitespace

The `ENV` and `LABEL` commands both support using either a whitespace delimited token or one delimited with the equals token. The equals token is preferred because it is more explicit and less ambiguous than using whitespace. The linter rule emits a warning when it sees the whitespace separator used. To facilitate this, the parser was modified to include the separator as a node in the tree. The associated parsing code has also been changed for 3 arguments instead of only 2 per key/value pair. Signed-off-by: Jonathan A. Sternberg <[email protected]>
jsternberg · May 30, 2024 · 6cfa459 · 6cfa459
1 parent 6acf12f
commit 6cfa459
Show file tree

Hide file tree

Showing 21 changed files with 143 additions and 78 deletions.
diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go
@@ -36,6 +36,7 @@ var lintTests = integration.TestFuncs(
 	testWorkdirRelativePath,
 	testUnmatchedVars,
 	testMultipleInstructionsDisallowed,
+	testLegacyKeyValueFormat,
 )
 
 func testStageName(t *testing.T, sb integration.Sandbox) {
@@ -604,7 +605,7 @@ EOF
 ENTRYPOINT ["/myotherapp"]
 CMD ["/myotherapp"]
 HEALTHCHECK CMD ["/myotherapp"]
-	`)
+`)
 	checkLinterWarnings(t, sb, &lintTestParams{
 		Dockerfile: dockerfile,
 		Warnings: []expectedLintWarning{
@@ -628,6 +629,40 @@ HEALTHCHECK CMD ["/myotherapp"]
 	checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
 }
 
+func testLegacyKeyValueFormat(t *testing.T, sb integration.Sandbox) {
+	dockerfile := []byte(`
+FROM scratch
+ENV key value
+LABEL key value
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{
+		Dockerfile: dockerfile,
+		Warnings: []expectedLintWarning{
+			{
+				RuleName:    "LegacyKeyValueFormat",
+				Description: "Legacy key/value format with whitespace separator should not be used",
+				Detail:      "\"ENV key=value\" should be used instead of legacy \"ENV key value\" format",
+				Line:        3,
+				Level:       1,
+			},
+			{
+				RuleName:    "LegacyKeyValueFormat",
+				Description: "Legacy key/value format with whitespace separator should not be used",
+				Detail:      "\"LABEL key=value\" should be used instead of legacy \"LABEL key value\" format",
+				Line:        4,
+				Level:       1,
+			},
+		},
+	})
+
+	dockerfile = []byte(`
+FROM scratch
+ENV key=value
+LABEL key=value
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
+}
+
 func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestParams) {
 	destDir, err := os.MkdirTemp("", "buildkit")
 	require.NoError(t, err)

diff --git a/frontend/dockerfile/instructions/parse.go b/frontend/dockerfile/instructions/parse.go
@@ -78,15 +78,15 @@ func ParseInstructionWithLinter(node *parser.Node, lintWarn linter.LintWarnFunc)
 	req := newParseRequestFromNode(node)
 	switch strings.ToLower(node.Value) {
 	case command.Env:
-		return parseEnv(req)
+		return parseEnv(req, lintWarn)
 	case command.Maintainer:
 		if lintWarn != nil {
 			msg := linter.RuleMaintainerDeprecated.Format()
 			linter.RuleMaintainerDeprecated.Run(lintWarn, node.Location(), msg)
 		}
 		return parseMaintainer(req)
 	case command.Label:
-		return parseLabel(req)
+		return parseLabel(req, lintWarn)
 	case command.Add:
 		return parseAdd(req)
 	case command.Copy:
@@ -195,31 +195,34 @@ func Parse(ast *parser.Node, lint linter.LintWarnFunc) (stages []Stage, metaArgs
 	return stages, metaArgs, nil
 }
 
-func parseKvps(args []string, cmdName string) (KeyValuePairs, error) {
+func parseKvps(args []string, cmdName string, location []parser.Range, lint linter.LintWarnFunc) (KeyValuePairs, error) {
 	if len(args) == 0 {
 		return nil, errAtLeastOneArgument(cmdName)
 	}
-	if len(args)%2 != 0 {
+	if len(args)%3 != 0 {
 		// should never get here, but just in case
 		return nil, errTooManyArguments(cmdName)
 	}
 	var res KeyValuePairs
-	for j := 0; j < len(args); j += 2 {
+	for j := 0; j < len(args); j += 3 {
 		if len(args[j]) == 0 {
 			return nil, errBlankCommandNames(cmdName)
 		}
-		name := args[j]
-		value := args[j+1]
+		name, value, sep := args[j], args[j+1], args[j+2]
+		if sep == "" {
+			msg := linter.RuleLegacyKeyValueFormat.Format(cmdName)
+			linter.RuleLegacyKeyValueFormat.Run(lint, location, msg)
+		}
 		res = append(res, KeyValuePair{Key: name, Value: value})
 	}
 	return res, nil
 }
 
-func parseEnv(req parseRequest) (*EnvCommand, error) {
+func parseEnv(req parseRequest, lint linter.LintWarnFunc) (*EnvCommand, error) {
 	if err := req.flags.Parse(); err != nil {
 		return nil, err
 	}
-	envs, err := parseKvps(req.args, "ENV")
+	envs, err := parseKvps(req.args, "ENV", req.location, lint)
 	if err != nil {
 		return nil, err
 	}
@@ -243,12 +246,12 @@ func parseMaintainer(req parseRequest) (*MaintainerCommand, error) {
 	}, nil
 }
 
-func parseLabel(req parseRequest) (*LabelCommand, error) {
+func parseLabel(req parseRequest, lint linter.LintWarnFunc) (*LabelCommand, error) {
 	if err := req.flags.Parse(); err != nil {
 		return nil, err
 	}
 
-	labels, err := parseKvps(req.args, "LABEL")
+	labels, err := parseKvps(req.args, "LABEL", req.location, lint)
 	if err != nil {
 		return nil, err
 	}

diff --git a/frontend/dockerfile/instructions/parse_test.go b/frontend/dockerfile/instructions/parse_test.go
@@ -74,6 +74,9 @@ func TestCommandsTooManyArguments(t *testing.T) {
 					Value: "arg2",
 					Next: &parser.Node{
 						Value: "arg3",
+						Next: &parser.Node{
+							Value: "",
+						},
 					},
 				},
 			},
@@ -97,6 +100,9 @@ func TestCommandsBlankNames(t *testing.T) {
 				Value: "",
 				Next: &parser.Node{
 					Value: "arg2",
+					Next: &parser.Node{
+						Value: "=",
+					},
 				},
 			},
 		}

diff --git a/frontend/dockerfile/linter/ruleset.go b/frontend/dockerfile/linter/ruleset.go
@@ -105,4 +105,11 @@ var (
 			return fmt.Sprintf("Multiple %s instructions should not be used in the same stage because only the last one will be used", instructionName)
 		},
 	}
+	RuleLegacyKeyValueFormat = LinterRule[func(cmdName string) string]{
+		Name:        "LegacyKeyValueFormat",
+		Description: "Legacy key/value format with whitespace separator should not be used",
+		Format: func(cmdName string) string {
+			return fmt.Sprintf("\"%s key=value\" should be used instead of legacy \"%s key value\" format", cmdName, cmdName)
+		},
+	}
 )
diff --git a/frontend/dockerfile/parser/line_parsers.go b/frontend/dockerfile/parser/line_parsers.go
@@ -154,7 +154,7 @@ func parseNameVal(rest string, key string, d *directives) (*Node, error) {
 		if len(parts) < 2 {
 			return nil, errors.Errorf("%s must have two arguments", key)
 		}
-		return newKeyValueNode(parts[0], parts[1]), nil
+		return newKeyValueNode(parts[0], parts[1], ""), nil
 	}
 
 	var rootNode *Node
@@ -165,17 +165,20 @@ func parseNameVal(rest string, key string, d *directives) (*Node, error) {
 		}
 
 		parts := strings.SplitN(word, "=", 2)
-		node := newKeyValueNode(parts[0], parts[1])
+		node := newKeyValueNode(parts[0], parts[1], "=")
 		rootNode, prevNode = appendKeyValueNode(node, rootNode, prevNode)
 	}
 
 	return rootNode, nil
 }
 
-func newKeyValueNode(key, value string) *Node {
+func newKeyValueNode(key, value, sep string) *Node {
 	return &Node{
 		Value: key,
-		Next:  &Node{Value: value},
+		Next: &Node{
+			Value: value,
+			Next:  &Node{Value: sep},
+		},
 	}
 }
 
@@ -187,7 +190,9 @@ func appendKeyValueNode(node, rootNode, prevNode *Node) (*Node, *Node) {
 		prevNode.Next = node
 	}
 
-	prevNode = node.Next
+	for prevNode = node.Next; prevNode.Next != nil; {
+		prevNode = prevNode.Next
+	}
 	return rootNode, prevNode
 }
 

diff --git a/frontend/dockerfile/parser/line_parsers_test.go b/frontend/dockerfile/parser/line_parsers_test.go
@@ -14,7 +14,10 @@ func TestParseNameValOldFormat(t *testing.T) {
 
 	expected := &Node{
 		Value: "foo",
-		Next:  &Node{Value: "bar"},
+		Next: &Node{
+			Value: "bar",
+			Next:  &Node{Value: ""},
+		},
 	}
 	require.Equal(t, expected, node, cmpNodeOpt)
 }
@@ -31,9 +34,15 @@ func TestParseNameValNewFormat(t *testing.T) {
 		Next: &Node{
 			Value: "bar",
 			Next: &Node{
-				Value: "thing",
+				Value: "=",
 				Next: &Node{
-					Value: "star",
+					Value: "thing",
+					Next: &Node{
+						Value: "star",
+						Next: &Node{
+							Value: "=",
+						},
+					},
 				},
 			},
 		},

diff --git a/frontend/dockerfile/parser/testfiles/ADD-COPY-with-JSON/result b/frontend/dockerfile/parser/testfiles/ADD-COPY-with-JSON/result
@@ -1,5 +1,5 @@
 (from "ubuntu:14.04")
-(label "maintainer" "Seongyeol Lim <[email protected]>")
+(label "maintainer" "Seongyeol Lim <[email protected]>" "")
 (copy "." "/go/src/github.com/docker/docker")
 (add "." "/")
 (add "null" "/")

diff --git a/frontend/dockerfile/parser/testfiles/brimstone-consuldock/result b/frontend/dockerfile/parser/testfiles/brimstone-consuldock/result
@@ -1,5 +1,5 @@
 (from "brimstone/ubuntu:14.04")
-(label "maintainer" "[email protected]")
-(env "GOPATH" "/go")
+(label "maintainer" "[email protected]" "")
+(env "GOPATH" "/go" "")
 (entrypoint "/usr/local/bin/consuldock")
 (run "apt-get update \t&& dpkg -l | awk '/^ii/ {print $2}' > /tmp/dpkg.clean     && apt-get --no-install-recommends install -y 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/frontend/dockerfile/parser/testfiles/brimstone-docker-consul/result b/frontend/dockerfile/parser/testfiles/brimstone-docker-consul/result
@@ -5,5 +5,5 @@
 (run "apt-get update     && apt-get --no-install-recommends 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 \t&& dpkg -l | awk '/^ii/ {print $2}' > /tmp/dpkg.clean     && apt-get --no-install-recommends install -y 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")
+(env "GOPATH" "/go" "")
 (run "apt-get update \t&& dpkg -l | awk '/^ii/ {print $2}' > /tmp/dpkg.clean     && apt-get --no-install-recommends install -y 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/frontend/dockerfile/parser/testfiles/cpuguy83-nagios/result b/frontend/dockerfile/parser/testfiles/cpuguy83-nagios/result
@@ -1,14 +1,14 @@
 (from "cpuguy83/ubuntu")
-(env "NAGIOS_HOME" "/opt/nagios")
-(env "NAGIOS_USER" "nagios")
-(env "NAGIOS_GROUP" "nagios")
-(env "NAGIOS_CMDUSER" "nagios")
-(env "NAGIOS_CMDGROUP" "nagios")
-(env "NAGIOSADMIN_USER" "nagiosadmin")
-(env "NAGIOSADMIN_PASS" "nagios")
-(env "APACHE_RUN_USER" "nagios")
-(env "APACHE_RUN_GROUP" "nagios")
-(env "NAGIOS_TIMEZONE" "UTC")
+(env "NAGIOS_HOME" "/opt/nagios" "")
+(env "NAGIOS_USER" "nagios" "")
+(env "NAGIOS_GROUP" "nagios" "")
+(env "NAGIOS_CMDUSER" "nagios" "")
+(env "NAGIOS_CMDGROUP" "nagios" "")
+(env "NAGIOSADMIN_USER" "nagiosadmin" "")
+(env "NAGIOSADMIN_PASS" "nagios" "")
+(env "APACHE_RUN_USER" "nagios" "")
+(env "APACHE_RUN_GROUP" "nagios" "")
+(env "NAGIOS_TIMEZONE" "UTC" "")
 (run "sed -i 's/universe/universe multiverse/' /etc/apt/sources.list")
 (run "apt-get update && apt-get --no-install-recommends install -y iputils-ping netcat build-essential snmp snmpd snmp-mibs-downloader php5-cli apache2 libapache2-mod-php5 runit bc postfix bsd-mailx")
 (run "( egrep -i  \"^${NAGIOS_GROUP}\" /etc/group || groupadd $NAGIOS_GROUP ) && ( egrep -i \"^${NAGIOS_CMDGROUP}\" /etc/group || groupadd $NAGIOS_CMDGROUP )")
@@ -33,8 +33,8 @@
 (add "postfix.init" "/etc/sv/postfix/run")
 (add "postfix.stop" "/etc/sv/postfix/finish")
 (add "start.sh" "/usr/local/bin/start_nagios")
-(env "APACHE_LOCK_DIR" "/var/run")
-(env "APACHE_LOG_DIR" "/var/log/apache2")
+(env "APACHE_LOCK_DIR" "/var/run" "")
+(env "APACHE_LOG_DIR" "/var/log/apache2" "")
 (expose "80")
 (volume "/opt/nagios/var" "/opt/nagios/etc" "/opt/nagios/libexec" "/var/log/apache2" "/usr/share/snmp/mibs")
 (cmd "/usr/local/bin/start_nagios")
diff --git a/frontend/dockerfile/parser/testfiles/docker/result b/frontend/dockerfile/parser/testfiles/docker/result
@@ -1,14 +1,14 @@
 (from "ubuntu:14.04")
-(label "maintainer" "Tianon Gravi <[email protected]> (@tianon)")
+(label "maintainer" "Tianon Gravi <[email protected]> (@tianon)" "")
 (run "apt-get update && DEBIAN_FRONTEND=noninteractive apt-get --no-install-recommends install -yq \tapt-utils \taufs-tools \tautomake \tbtrfs-tools \tbuild-essential \tcurl \tdpkg-sig \tgit \tiptables \tlibapparmor-dev \tlibcap-dev \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")
+(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 \tdarwin/amd64 darwin/386 \tfreebsd/amd64 freebsd/386 freebsd/arm")
-(env "GOARM" "5")
+(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")
 (run "gem install --no-rdoc --no-ri fpm --version 1.0.2")
@@ -19,6 +19,6 @@
 (run "useradd --create-home --gid docker unprivilegeduser")
 (volume "/var/lib/docker")
 (workdir "/go/src/github.com/docker/docker")
-(env "DOCKER_BUILDTAGS" "apparmor selinux")
+(env "DOCKER_BUILDTAGS" "apparmor selinux" "")
 (entrypoint "hack/dind")
 (copy "." "/go/src/github.com/docker/docker")
diff --git a/frontend/dockerfile/parser/testfiles/env/result b/frontend/dockerfile/parser/testfiles/env/result
@@ -1,16 +1,16 @@
 (from "ubuntu")
-(env "name" "value")
-(env "name" "value")
-(env "name" "value" "name2" "value2")
-(env "name" "\"value value1\"")
-(env "name" "value\\ value2")
-(env "name" "\"value'quote space'value2\"")
-(env "name" "'value\"double quote\"value2'")
-(env "name" "value\\ value2" "name2" "value2\\ value3")
-(env "name" "\"a\\\"b\"")
-(env "name" "\"a\\'b\"")
-(env "name" "'a\\'b'")
-(env "name" "'a\\'b''")
-(env "name" "'a\\\"b'")
-(env "name" "\"''\"")
-(env "name" "value" "name1" "value1" "name2" "\"value2a            value2b\"" "name3" "\"value3a\\n\\\"value3b\\\"\"" "name4" "\"value4a\\\\nvalue4b\"")
+(env "name" "value" "")
+(env "name" "value" "=")
+(env "name" "value" "=" "name2" "value2" "=")
+(env "name" "\"value value1\"" "=")
+(env "name" "value\\ value2" "=")
+(env "name" "\"value'quote space'value2\"" "=")
+(env "name" "'value\"double quote\"value2'" "=")
+(env "name" "value\\ value2" "=" "name2" "value2\\ value3" "=")
+(env "name" "\"a\\\"b\"" "=")
+(env "name" "\"a\\'b\"" "=")
+(env "name" "'a\\'b'" "=")
+(env "name" "'a\\'b''" "=")
+(env "name" "'a\\\"b'" "=")
+(env "name" "\"''\"" "=")
+(env "name" "value" "=" "name1" "value1" "=" "name2" "\"value2a            value2b\"" "=" "name3" "\"value3a\\n\\\"value3b\\\"\"" "=" "name4" "\"value4a\\\\nvalue4b\"" "=")
diff --git a/frontend/dockerfile/parser/testfiles/escape-after-comment/result b/frontend/dockerfile/parser/testfiles/escape-after-comment/result
@@ -1,3 +1,3 @@
 (from "image")
-(label "maintainer" "[email protected]")
-(env "GOPATH" "\\go")
+(label "maintainer" "[email protected]" "")
+(env "GOPATH" "\\go" "")
diff --git a/frontend/dockerfile/parser/testfiles/escape-nonewline/result b/frontend/dockerfile/parser/testfiles/escape-nonewline/result
@@ -1,3 +1,3 @@
 (from "image")
-(label "maintainer" "[email protected]")
-(env "GOPATH" "\\go")
+(label "maintainer" "[email protected]" "")
+(env "GOPATH" "\\go" "")
diff --git a/frontend/dockerfile/parser/testfiles/escape/result b/frontend/dockerfile/parser/testfiles/escape/result
@@ -1,3 +1,3 @@
 (from "image")
-(label "maintainer" "[email protected]")
-(env "GOPATH" "\\go")
+(label "maintainer" "[email protected]" "")
+(env "GOPATH" "\\go" "")
diff --git a/frontend/dockerfile/parser/testfiles/escapes/result b/frontend/dockerfile/parser/testfiles/escapes/result
@@ -1,5 +1,5 @@
 (from "ubuntu:14.04")
-(label "maintainer" "Erik \\\\Hollensbe <[email protected]>\\\"")
+(label "maintainer" "Erik \\\\Hollensbe <[email protected]>\\\"" "")
 (run "apt-get \\update &&   apt-get \\\"install znc -y")
 (add "\\conf\\\\\"" "/.znc")
 (run "foo bar baz")

diff --git a/frontend/dockerfile/parser/testfiles/kartar-entrypoint-oddities/result b/frontend/dockerfile/parser/testfiles/kartar-entrypoint-oddities/result
@@ -1,6 +1,6 @@
 (from "ubuntu:14.04")
-(label "maintainer" "James Turnbull \"[email protected]\"")
-(env "REFRESHED_AT" "2014-06-01")
+(label "maintainer" "James Turnbull \"[email protected]\"" "")
+(env "REFRESHED_AT" "2014-06-01" "")
 (run "apt-get update")
 (run "apt-get --no-install-recommends install -y redis-server redis-tools")
 (expose "6379")