Merge pull request moby#5001 from jsternberg/legacy-env-lint-dispatch-only

dockerfile: only report legacy key/value format when stage is reachable

Author: tonistiigi

frontend/dockerfile/dockerfile2llb/convert.go

@@ -881,7 +881,7 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error {
 	case *instructions.MaintainerCommand:
 		err = dispatchMaintainer(d, c)
 	case *instructions.EnvCommand:
-		err = dispatchEnv(d, c)
+		err = dispatchEnv(d, c, opt.lint)
 	case *instructions.RunCommand:
 		err = dispatchRun(d, c, opt.proxyEnv, cmd.sources, opt)
 	case *instructions.WorkdirCommand:
@@ -915,7 +915,7 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error {
 			}
 		}
 	case *instructions.LabelCommand:
-		err = dispatchLabel(d, c)
+		err = dispatchLabel(d, c, opt.lint)
 	case *instructions.OnbuildCommand:
 		err = dispatchOnbuild(d, c)
 	case *instructions.CmdCommand:
@@ -1104,9 +1104,13 @@ func dispatchOnBuildTriggers(d *dispatchState, triggers []string, opt dispatchOp
 	return nil
 }
 
-func dispatchEnv(d *dispatchState, c *instructions.EnvCommand) error {
+func dispatchEnv(d *dispatchState, c *instructions.EnvCommand, lint *linter.Linter) error {
 	commitMessage := bytes.NewBufferString("ENV")
 	for _, e := range c.Env {
+		if e.NoDelim {
+			msg := linter.RuleLegacyKeyValueFormat.Format(c.Name())
+			lint.Run(&linter.RuleLegacyKeyValueFormat, c.Location(), msg)
+		}
 		commitMessage.WriteString(" " + e.String())
 		d.state = d.state.AddEnv(e.Key, e.Value)
 		d.image.Config.Env = addEnv(d.image.Config.Env, e.Key, e.Value)
@@ -1562,12 +1566,16 @@ func dispatchMaintainer(d *dispatchState, c *instructions.MaintainerCommand) err
 	return commitToHistory(&d.image, fmt.Sprintf("MAINTAINER %v", c.Maintainer), false, nil, d.epoch)
 }
 
-func dispatchLabel(d *dispatchState, c *instructions.LabelCommand) error {
+func dispatchLabel(d *dispatchState, c *instructions.LabelCommand, lint *linter.Linter) error {
 	commitMessage := bytes.NewBufferString("LABEL")
 	if d.image.Config.Labels == nil {
 		d.image.Config.Labels = make(map[string]string, len(c.Labels))
 	}
 	for _, v := range c.Labels {
+		if v.NoDelim {
+			msg := linter.RuleLegacyKeyValueFormat.Format(c.Name())
+			lint.Run(&linter.RuleLegacyKeyValueFormat, c.Location(), msg)
+		}
 		d.image.Config.Labels[v.Key] = v.Value
 		commitMessage.WriteString(" " + v.String())
 	}

frontend/dockerfile/dockerfile_lint_test.go

@@ -858,6 +858,37 @@ ENV key=value
 LABEL key=value
 `)
 	checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
+
+	// Warnings only happen in unmarshal if the lint happens in a
+	// stage that's not reachable.
+	dockerfile = []byte(`
+FROM scratch AS a
+
+FROM a AS b
+ENV key value
+LABEL key value
+
+FROM a AS c
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{
+		Dockerfile: dockerfile,
+		UnmarshalWarnings: []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,
+			},
+		},
+	})
 }
 
 func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestParams) {

frontend/dockerfile/instructions/commands.go

@@ -13,8 +13,9 @@ import (
 // This is useful for commands containing key-value maps that want to preserve
 // the order of insertion, instead of map[string]string which does not.
 type KeyValuePair struct {
-	Key   string
-	Value string
+	Key     string
+	Value   string
+	NoDelim bool
 }
 
 func (kvp *KeyValuePair) String() string {
@@ -108,8 +109,9 @@ func expandKvp(kvp KeyValuePair, expander SingleWordExpander) (KeyValuePair, err
 	if err != nil {
 		return KeyValuePair{}, err
 	}
-	return KeyValuePair{Key: key, Value: value}, nil
+	return KeyValuePair{Key: key, Value: value, NoDelim: kvp.NoDelim}, nil
 }
+
 func expandKvpsInPlace(kvps KeyValuePairs, expander SingleWordExpander) error {
 	for i, kvp := range kvps {
 		newKvp, err := expandKvp(kvp, expander)

frontend/dockerfile/instructions/parse.go

@@ -78,13 +78,13 @@ func ParseInstructionWithLinter(node *parser.Node, lint *linter.Linter) (v inter
 	req := newParseRequestFromNode(node)
 	switch strings.ToLower(node.Value) {
 	case command.Env:
-		return parseEnv(req, lint)
+		return parseEnv(req)
 	case command.Maintainer:
 		msg := linter.RuleMaintainerDeprecated.Format()
 		lint.Run(&linter.RuleMaintainerDeprecated, node.Location(), msg)
 		return parseMaintainer(req)
 	case command.Label:
-		return parseLabel(req, lint)
+		return parseLabel(req)
 	case command.Add:
 		return parseAdd(req)
 	case command.Copy:
@@ -193,7 +193,7 @@ func Parse(ast *parser.Node, lint *linter.Linter) (stages []Stage, metaArgs []Ar
 	return stages, metaArgs, nil
 }
 
-func parseKvps(args []string, cmdName string, location []parser.Range, lint *linter.Linter) (KeyValuePairs, error) {
+func parseKvps(args []string, cmdName string) (KeyValuePairs, error) {
 	if len(args) == 0 {
 		return nil, errAtLeastOneArgument(cmdName)
 	}
@@ -206,21 +206,17 @@ func parseKvps(args []string, cmdName string, location []parser.Range, lint *lin
 		if len(args[j]) == 0 {
 			return nil, errBlankCommandNames(cmdName)
 		}
-		name, value, sep := args[j], args[j+1], args[j+2]
-		if sep == "" {
-			msg := linter.RuleLegacyKeyValueFormat.Format(cmdName)
-			lint.Run(&linter.RuleLegacyKeyValueFormat, location, msg)
-		}
-		res = append(res, KeyValuePair{Key: name, Value: value})
+		name, value, delim := args[j], args[j+1], args[j+2]
+		res = append(res, KeyValuePair{Key: name, Value: value, NoDelim: delim == ""})
 	}
 	return res, nil
 }
 
-func parseEnv(req parseRequest, lint *linter.Linter) (*EnvCommand, error) {
+func parseEnv(req parseRequest) (*EnvCommand, error) {
 	if err := req.flags.Parse(); err != nil {
 		return nil, err
 	}
-	envs, err := parseKvps(req.args, "ENV", req.location, lint)
+	envs, err := parseKvps(req.args, "ENV")
 	if err != nil {
 		return nil, err
 	}
@@ -244,12 +240,12 @@ func parseMaintainer(req parseRequest) (*MaintainerCommand, error) {
 	}, nil
 }
 
-func parseLabel(req parseRequest, lint *linter.Linter) (*LabelCommand, error) {
+func parseLabel(req parseRequest) (*LabelCommand, error) {
 	if err := req.flags.Parse(); err != nil {
 		return nil, err
 	}
 
-	labels, err := parseKvps(req.args, "LABEL", req.location, lint)
+	labels, err := parseKvps(req.args, "LABEL")
 	if err != nil {
 		return nil, err
 	}