Skip to content

Commit 6cfa459

Browse files
jsternbergjsternberg
committed
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]>
1 parent 6acf12f commit 6cfa459

21 files changed

Lines changed: 143 additions & 78 deletions

File tree

‎frontend/dockerfile/dockerfile_lint_test.go‎

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ var lintTests = integration.TestFuncs(
3636
testWorkdirRelativePath,
3737
testUnmatchedVars,
3838
testMultipleInstructionsDisallowed,
39+
testLegacyKeyValueFormat,
3940
)
4041

4142
func testStageName(t *testing.T, sb integration.Sandbox) {
@@ -604,7 +605,7 @@ EOF
604605
ENTRYPOINT ["/myotherapp"]
605606
CMD ["/myotherapp"]
606607
HEALTHCHECK CMD ["/myotherapp"]
607-
`)
608+
`)
608609
checkLinterWarnings(t, sb, &lintTestParams{
609610
Dockerfile: dockerfile,
610611
Warnings: []expectedLintWarning{
@@ -628,6 +629,40 @@ HEALTHCHECK CMD ["/myotherapp"]
628629
checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
629630
}
630631

632+
func testLegacyKeyValueFormat(t *testing.T, sb integration.Sandbox) {
633+
dockerfile := []byte(`
634+
FROM scratch
635+
ENV key value
636+
LABEL key value
637+
`)
638+
checkLinterWarnings(t, sb, &lintTestParams{
639+
Dockerfile: dockerfile,
640+
Warnings: []expectedLintWarning{
641+
{
642+
RuleName: "LegacyKeyValueFormat",
643+
Description: "Legacy key/value format with whitespace separator should not be used",
644+
Detail: "\"ENV key=value\" should be used instead of legacy \"ENV key value\" format",
645+
Line: 3,
646+
Level: 1,
647+
},
648+
{
649+
RuleName: "LegacyKeyValueFormat",
650+
Description: "Legacy key/value format with whitespace separator should not be used",
651+
Detail: "\"LABEL key=value\" should be used instead of legacy \"LABEL key value\" format",
652+
Line: 4,
653+
Level: 1,
654+
},
655+
},
656+
})
657+
658+
dockerfile = []byte(`
659+
FROM scratch
660+
ENV key=value
661+
LABEL key=value
662+
`)
663+
checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
664+
}
665+
631666
func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestParams) {
632667
destDir, err := os.MkdirTemp("", "buildkit")
633668
require.NoError(t, err)

‎frontend/dockerfile/instructions/parse.go‎

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -78,15 +78,15 @@ func ParseInstructionWithLinter(node *parser.Node, lintWarn linter.LintWarnFunc)
7878
req := newParseRequestFromNode(node)
7979
switch strings.ToLower(node.Value) {
8080
case command.Env:
81-
return parseEnv(req)
81+
return parseEnv(req, lintWarn)
8282
case command.Maintainer:
8383
if lintWarn != nil {
8484
msg := linter.RuleMaintainerDeprecated.Format()
8585
linter.RuleMaintainerDeprecated.Run(lintWarn, node.Location(), msg)
8686
}
8787
return parseMaintainer(req)
8888
case command.Label:
89-
return parseLabel(req)
89+
return parseLabel(req, lintWarn)
9090
case command.Add:
9191
return parseAdd(req)
9292
case command.Copy:
@@ -195,31 +195,34 @@ func Parse(ast *parser.Node, lint linter.LintWarnFunc) (stages []Stage, metaArgs
195195
return stages, metaArgs, nil
196196
}
197197

198-
func parseKvps(args []string, cmdName string) (KeyValuePairs, error) {
198+
func parseKvps(args []string, cmdName string, location []parser.Range, lint linter.LintWarnFunc) (KeyValuePairs, error) {
199199
if len(args) == 0 {
200200
return nil, errAtLeastOneArgument(cmdName)
201201
}
202-
if len(args)%2 != 0 {
202+
if len(args)%3 != 0 {
203203
// should never get here, but just in case
204204
return nil, errTooManyArguments(cmdName)
205205
}
206206
var res KeyValuePairs
207-
for j := 0; j < len(args); j += 2 {
207+
for j := 0; j < len(args); j += 3 {
208208
if len(args[j]) == 0 {
209209
return nil, errBlankCommandNames(cmdName)
210210
}
211-
name := args[j]
212-
value := args[j+1]
211+
name, value, sep := args[j], args[j+1], args[j+2]
212+
if sep == "" {
213+
msg := linter.RuleLegacyKeyValueFormat.Format(cmdName)
214+
linter.RuleLegacyKeyValueFormat.Run(lint, location, msg)
215+
}
213216
res = append(res, KeyValuePair{Key: name, Value: value})
214217
}
215218
return res, nil
216219
}
217220

218-
func parseEnv(req parseRequest) (*EnvCommand, error) {
221+
func parseEnv(req parseRequest, lint linter.LintWarnFunc) (*EnvCommand, error) {
219222
if err := req.flags.Parse(); err != nil {
220223
return nil, err
221224
}
222-
envs, err := parseKvps(req.args, "ENV")
225+
envs, err := parseKvps(req.args, "ENV", req.location, lint)
223226
if err != nil {
224227
return nil, err
225228
}
@@ -243,12 +246,12 @@ func parseMaintainer(req parseRequest) (*MaintainerCommand, error) {
243246
}, nil
244247
}
245248

246-
func parseLabel(req parseRequest) (*LabelCommand, error) {
249+
func parseLabel(req parseRequest, lint linter.LintWarnFunc) (*LabelCommand, error) {
247250
if err := req.flags.Parse(); err != nil {
248251
return nil, err
249252
}
250253

251-
labels, err := parseKvps(req.args, "LABEL")
254+
labels, err := parseKvps(req.args, "LABEL", req.location, lint)
252255
if err != nil {
253256
return nil, err
254257
}

‎frontend/dockerfile/instructions/parse_test.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,9 @@ func TestCommandsTooManyArguments(t *testing.T) {
7474
Value: "arg2",
7575
Next: &parser.Node{
7676
Value: "arg3",
77+
Next: &parser.Node{
78+
Value: "",
79+
},
7780
},
7881
},
7982
},
@@ -97,6 +100,9 @@ func TestCommandsBlankNames(t *testing.T) {
97100
Value: "",
98101
Next: &parser.Node{
99102
Value: "arg2",
103+
Next: &parser.Node{
104+
Value: "=",
105+
},
100106
},
101107
},
102108
}

‎frontend/dockerfile/linter/ruleset.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,4 +105,11 @@ var (
105105
return fmt.Sprintf("Multiple %s instructions should not be used in the same stage because only the last one will be used", instructionName)
106106
},
107107
}
108+
RuleLegacyKeyValueFormat = LinterRule[func(cmdName string) string]{
109+
Name: "LegacyKeyValueFormat",
110+
Description: "Legacy key/value format with whitespace separator should not be used",
111+
Format: func(cmdName string) string {
112+
return fmt.Sprintf("\"%s key=value\" should be used instead of legacy \"%s key value\" format", cmdName, cmdName)
113+
},
114+
}
108115
)

‎frontend/dockerfile/parser/line_parsers.go‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ func parseNameVal(rest string, key string, d *directives) (*Node, error) {
154154
if len(parts) < 2 {
155155
return nil, errors.Errorf("%s must have two arguments", key)
156156
}
157-
return newKeyValueNode(parts[0], parts[1]), nil
157+
return newKeyValueNode(parts[0], parts[1], ""), nil
158158
}
159159

160160
var rootNode *Node
@@ -165,17 +165,20 @@ func parseNameVal(rest string, key string, d *directives) (*Node, error) {
165165
}
166166

167167
parts := strings.SplitN(word, "=", 2)
168-
node := newKeyValueNode(parts[0], parts[1])
168+
node := newKeyValueNode(parts[0], parts[1], "=")
169169
rootNode, prevNode = appendKeyValueNode(node, rootNode, prevNode)
170170
}
171171

172172
return rootNode, nil
173173
}
174174

175-
func newKeyValueNode(key, value string) *Node {
175+
func newKeyValueNode(key, value, sep string) *Node {
176176
return &Node{
177177
Value: key,
178-
Next: &Node{Value: value},
178+
Next: &Node{
179+
Value: value,
180+
Next: &Node{Value: sep},
181+
},
179182
}
180183
}
181184

@@ -187,7 +190,9 @@ func appendKeyValueNode(node, rootNode, prevNode *Node) (*Node, *Node) {
187190
prevNode.Next = node
188191
}
189192

190-
prevNode = node.Next
193+
for prevNode = node.Next; prevNode.Next != nil; {
194+
prevNode = prevNode.Next
195+
}
191196
return rootNode, prevNode
192197
}
193198

‎frontend/dockerfile/parser/line_parsers_test.go‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,10 @@ func TestParseNameValOldFormat(t *testing.T) {
1414

1515
expected := &Node{
1616
Value: "foo",
17-
Next: &Node{Value: "bar"},
17+
Next: &Node{
18+
Value: "bar",
19+
Next: &Node{Value: ""},
20+
},
1821
}
1922
require.Equal(t, expected, node, cmpNodeOpt)
2023
}
@@ -31,9 +34,15 @@ func TestParseNameValNewFormat(t *testing.T) {
3134
Next: &Node{
3235
Value: "bar",
3336
Next: &Node{
34-
Value: "thing",
37+
Value: "=",
3538
Next: &Node{
36-
Value: "star",
39+
Value: "thing",
40+
Next: &Node{
41+
Value: "star",
42+
Next: &Node{
43+
Value: "=",
44+
},
45+
},
3746
},
3847
},
3948
},

‎frontend/dockerfile/parser/testfiles/ADD-COPY-with-JSON/result‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
(from "ubuntu:14.04")
2-
(label "maintainer" "Seongyeol Lim <[email protected]>")
2+
(label "maintainer" "Seongyeol Lim <[email protected]>" "")
33
(copy "." "/go/src/github.com/docker/docker")
44
(add "." "/")
55
(add "null" "/")

‎frontend/dockerfile/parser/testfiles/brimstone-consuldock/result‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
(from "brimstone/ubuntu:14.04")
2-
(label "maintainer" "[email protected]")
3-
(env "GOPATH" "/go")
2+
(label "maintainer" "[email protected]" "")
3+
(env "GOPATH" "/go" "")
44
(entrypoint "/usr/local/bin/consuldock")
55
(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")

‎frontend/dockerfile/parser/testfiles/brimstone-docker-consul/result‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,5 +5,5 @@
55
(run "apt-get update && apt-get --no-install-recommends install -y unzip wget \t&& apt-get clean \t&& rm -rf /var/lib/apt/lists")
66
(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")
77
(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.*")
8-
(env "GOPATH" "/go")
8+
(env "GOPATH" "/go" "")
99
(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")

‎frontend/dockerfile/parser/testfiles/cpuguy83-nagios/result‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,14 @@
11
(from "cpuguy83/ubuntu")
2-
(env "NAGIOS_HOME" "/opt/nagios")
3-
(env "NAGIOS_USER" "nagios")
4-
(env "NAGIOS_GROUP" "nagios")
5-
(env "NAGIOS_CMDUSER" "nagios")
6-
(env "NAGIOS_CMDGROUP" "nagios")
7-
(env "NAGIOSADMIN_USER" "nagiosadmin")
8-
(env "NAGIOSADMIN_PASS" "nagios")
9-
(env "APACHE_RUN_USER" "nagios")
10-
(env "APACHE_RUN_GROUP" "nagios")
11-
(env "NAGIOS_TIMEZONE" "UTC")
2+
(env "NAGIOS_HOME" "/opt/nagios" "")
3+
(env "NAGIOS_USER" "nagios" "")
4+
(env "NAGIOS_GROUP" "nagios" "")
5+
(env "NAGIOS_CMDUSER" "nagios" "")
6+
(env "NAGIOS_CMDGROUP" "nagios" "")
7+
(env "NAGIOSADMIN_USER" "nagiosadmin" "")
8+
(env "NAGIOSADMIN_PASS" "nagios" "")
9+
(env "APACHE_RUN_USER" "nagios" "")
10+
(env "APACHE_RUN_GROUP" "nagios" "")
11+
(env "NAGIOS_TIMEZONE" "UTC" "")
1212
(run "sed -i 's/universe/universe multiverse/' /etc/apt/sources.list")
1313
(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")
1414
(run "( egrep -i \"^${NAGIOS_GROUP}\" /etc/group || groupadd $NAGIOS_GROUP ) && ( egrep -i \"^${NAGIOS_CMDGROUP}\" /etc/group || groupadd $NAGIOS_CMDGROUP )")
@@ -33,8 +33,8 @@
3333
(add "postfix.init" "/etc/sv/postfix/run")
3434
(add "postfix.stop" "/etc/sv/postfix/finish")
3535
(add "start.sh" "/usr/local/bin/start_nagios")
36-
(env "APACHE_LOCK_DIR" "/var/run")
37-
(env "APACHE_LOG_DIR" "/var/log/apache2")
36+
(env "APACHE_LOCK_DIR" "/var/run" "")
37+
(env "APACHE_LOG_DIR" "/var/log/apache2" "")
3838
(expose "80")
3939
(volume "/opt/nagios/var" "/opt/nagios/etc" "/opt/nagios/libexec" "/var/log/apache2" "/usr/share/snmp/mibs")
4040
(cmd "/usr/local/bin/start_nagios")

0 commit comments

Comments
 (0)