Merge pull request #4897 from daghack/undeclared-args

Add undefined arg lint rule

frontend/dockerfile/dockerfile2llb/convert.go

@@ -740,13 +740,17 @@ type dispatchOpt struct {
 }
 
 func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error {
+	var err error
 	if ex, ok := cmd.Command.(instructions.SupportsSingleWordExpansion); ok {
 		err := ex.Expand(func(word string) (string, error) {
 			env, err := d.state.Env(context.TODO())
 			if err != nil {
 				return "", err
 			}
-			return opt.shlex.ProcessWord(word, env)
+
+			newword, unmatched, err := opt.shlex.ProcessWord(word, env)
+			reportUnmatchedVariables(cmd, d.buildArgs, unmatched, &opt)
+			return newword, err
 		})
 		if err != nil {
 			return err
@@ -758,17 +762,17 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error {
 			if err != nil {
 				return "", err
 			}
-
 			lex := shell.NewLex('\\')
 			lex.SkipProcessQuotes = true
-			return lex.ProcessWord(word, env)
+			newword, unmatched, err := lex.ProcessWord(word, env)
+			reportUnmatchedVariables(cmd, d.buildArgs, unmatched, &opt)
+			return newword, err
 		})
 		if err != nil {
 			return err
 		}
 	}
 
-	var err error
 	switch c := cmd.Command.(type) {
 	case *instructions.MaintainerCommand:
 		err = dispatchMaintainer(d, c)
@@ -827,7 +831,7 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error {
 	case *instructions.ShellCommand:
 		err = dispatchShell(d, c)
 	case *instructions.ArgCommand:
-		err = dispatchArg(d, c, opt.metaArgs, opt.buildArgValues)
+		err = dispatchArg(d, c, &opt)
 	case *instructions.CopyCommand:
 		l := opt.buildContext
 		if len(cmd.sources) != 0 {
@@ -1576,10 +1580,10 @@ func dispatchShell(d *dispatchState, c *instructions.ShellCommand) error {
 	return commitToHistory(&d.image, fmt.Sprintf("SHELL %v", c.Shell), false, nil, d.epoch)
 }
 
-func dispatchArg(d *dispatchState, c *instructions.ArgCommand, metaArgs []instructions.KeyValuePairOptional, buildArgValues map[string]string) error {
+func dispatchArg(d *dispatchState, c *instructions.ArgCommand, opt *dispatchOpt) error {
 	commitStrs := make([]string, 0, len(c.Args))
 	for _, arg := range c.Args {
-		buildArg := setKVValue(arg, buildArgValues)
+		buildArg := setKVValue(arg, opt.buildArgValues)
 
 		commitStr := arg.Key
 		if arg.Value != nil {
@@ -1589,7 +1593,7 @@ func dispatchArg(d *dispatchState, c *instructions.ArgCommand, metaArgs []instru
 
 		skipArgInfo := false // skip the arg info if the arg is inherited from global scope
 		if buildArg.Value == nil {
-			for _, ma := range metaArgs {
+			for _, ma := range opt.metaArgs {
 				if ma.Key == buildArg.Key {
 					buildArg.Value = ma.Value
 					skipArgInfo = true
@@ -1876,7 +1880,7 @@ func uppercaseCmd(str string) string {
 }
 
 func processCmdEnv(shlex *shell.Lex, cmd string, env []string) string {
-	w, err := shlex.ProcessWord(cmd, env)
+	w, _, err := shlex.ProcessWord(cmd, env)
 	if err != nil {
 		return cmd
 	}
@@ -2075,6 +2079,22 @@ func validateStageNames(stages []instructions.Stage, warn linter.LintWarnFunc) {
 	}
 }
 
+func reportUnmatchedVariables(cmd instructions.Command, buildArgs []instructions.KeyValuePairOptional, unmatched map[string]struct{}, opt *dispatchOpt) {
+	if len(unmatched) == 0 {
+		return
+	}
+	for _, buildArg := range buildArgs {
+		delete(unmatched, buildArg.Key)
+	}
+	for cmdVar := range unmatched {
+		_, nonEnvOk := nonEnvArgs[cmdVar]
+		if !nonEnvOk {
+			msg := linter.RuleUndefinedVar.Format(cmdVar)
+			linter.RuleUndefinedVar.Run(opt.lintWarn, cmd.Location(), msg)
+		}
+	}
+}
+
 func mergeLocations(locations ...[]parser.Range) []parser.Range {
 	allRanges := []parser.Range{}
 	for _, ranges := range locations {

frontend/dockerfile/dockerfile_lint_test.go

@@ -33,6 +33,7 @@ var lintTests = integration.TestFuncs(
 	testWarningsBeforeError,
 	testUndeclaredArg,
 	testWorkdirRelativePath,
+	testUnmatchedVars,
 )
 
 func testStageName(t *testing.T, sb integration.Sandbox) {
@@ -516,6 +517,49 @@ WORKDIR subdir/
 	checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
 }
 
+func testUnmatchedVars(t *testing.T, sb integration.Sandbox) {
+	dockerfile := []byte(`
+FROM scratch
+ARG foo
+COPY Dockerfile${foo} .
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
+
+	dockerfile = []byte(`
+FROM alpine AS base
+ARG foo=Dockerfile
+
+FROM base
+COPY $foo .
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
+
+	dockerfile = []byte(`
+FROM alpine
+RUN echo $PATH
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
+
+	dockerfile = []byte(`
+FROM alpine
+COPY $foo .
+ARG foo=bar
+RUN echo $foo
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{
+		Dockerfile: dockerfile,
+		Warnings: []expectedLintWarning{
+			{
+				RuleName:    "UndefinedVar",
+				Description: "Variables should be defined before their use",
+				Detail:      "Usage of undefined variable '$foo'",
+				Level:       1,
+				Line:        3,
+			},
+		},
+	})
+}
+
 func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestParams) {
 	destDir, err := os.MkdirTemp("", "buildkit")
 	require.NoError(t, err)
@@ -545,6 +589,7 @@ func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestPara
 			require.Less(t, lintResults.Error.Location.SourceIndex, int32(len(lintResults.Sources)))
 		}
 		require.Equal(t, len(lintTest.Warnings), len(lintResults.Warnings))
+
 		sort.Slice(lintResults.Warnings, func(i, j int) bool {
 			// sort by line number in ascending order
 			firstRange := lintResults.Warnings[i].Location.Ranges[0]

frontend/dockerfile/linter/ruleset.go

@@ -84,4 +84,18 @@ var (
 			return fmt.Sprintf("Relative workdir %q can have unexpected results if the base image changes", workdir)
 		},
 	}
+	RuleUndefinedArg = LinterRule[func(string) string]{
+		Name:        "UndefinedArg",
+		Description: "ARGs should be defined before their use",
+		Format: func(arg string) string {
+			return fmt.Sprintf("Usage of undefined variable '$%s'", arg)
+		},
+	}
+	RuleUndefinedVar = LinterRule[func(string) string]{
+		Name:        "UndefinedVar",
+		Description: "Variables should be defined before their use",
+		Format: func(arg string) string {
+			return fmt.Sprintf("Usage of undefined variable '$%s'", arg)
+		},
+	}
 )

frontend/dockerfile/shell/lex.go

@@ -32,10 +32,12 @@ func NewLex(escapeToken rune) *Lex {
 }
 
 // ProcessWord will use the 'env' list of environment variables,
-// and replace any env var references in 'word'.
-func (s *Lex) ProcessWord(word string, env []string) (string, error) {
-	word, _, err := s.process(word, BuildEnvs(env))
-	return word, err
+// and replace any env var references in 'word'. It will also
+// return variables in word which were not found in the 'env' list,
+// which is useful in later linting.
+func (s *Lex) ProcessWord(word string, env []string) (string, map[string]struct{}, error) {
+	result, err := s.process(word, BuildEnvs(env))
+	return result.Result, result.Unmatched, err
 }
 
 // ProcessWords will use the 'env' list of environment variables,
@@ -46,38 +48,40 @@ func (s *Lex) ProcessWord(word string, env []string) (string, error) {
 // Note, each one is trimmed to remove leading and trailing spaces (unless
 // they are quoted", but ProcessWord retains spaces between words.
 func (s *Lex) ProcessWords(word string, env []string) ([]string, error) {
-	_, words, err := s.process(word, BuildEnvs(env))
-	return words, err
+	result, err := s.process(word, BuildEnvs(env))
+	return result.Words, err
 }
 
 // ProcessWordWithMap will use the 'env' list of environment variables,
 // and replace any env var references in 'word'.
 func (s *Lex) ProcessWordWithMap(word string, env map[string]string) (string, error) {
-	word, _, err := s.process(word, env)
-	return word, err
+	result, err := s.process(word, env)
+	return result.Result, err
 }
 
-type ProcessWordMatchesResult struct {
+type ProcessWordResult struct {
 	Result    string
+	Words     []string
 	Matched   map[string]struct{}
 	Unmatched map[string]struct{}
 }
 
 // ProcessWordWithMatches will use the 'env' list of environment variables,
 // replace any env var references in 'word' and return the env that were used.
-func (s *Lex) ProcessWordWithMatches(word string, env map[string]string) (ProcessWordMatchesResult, error) {
+func (s *Lex) ProcessWordWithMatches(word string, env map[string]string) (ProcessWordResult, error) {
 	sw := s.init(word, env)
-	word, _, err := sw.process(word)
-	return ProcessWordMatchesResult{
+	word, words, err := sw.process(word)
+	return ProcessWordResult{
 		Result:    word,
+		Words:     words,
 		Matched:   sw.matches,
 		Unmatched: sw.nonmatches,
 	}, err
 }
 
 func (s *Lex) ProcessWordsWithMap(word string, env map[string]string) ([]string, error) {
-	_, words, err := s.process(word, env)
-	return words, err
+	result, err := s.process(word, env)
+	return result.Words, err
 }
 
 func (s *Lex) init(word string, env map[string]string) *shellWord {
@@ -95,9 +99,15 @@ func (s *Lex) init(word string, env map[string]string) *shellWord {
 	return sw
 }
 
-func (s *Lex) process(word string, env map[string]string) (string, []string, error) {
+func (s *Lex) process(word string, env map[string]string) (*ProcessWordResult, error) {
 	sw := s.init(word, env)
-	return sw.process(word)
+	word, words, err := sw.process(word)
+	return &ProcessWordResult{
+		Result:    word,
+		Words:     words,
+		Matched:   sw.matches,
+		Unmatched: sw.nonmatches,
+	}, err
 }
 
 type shellWord struct {

frontend/dockerfile/shell/lex_test.go

@@ -61,26 +61,26 @@ func TestShellParserMandatoryEnvVars(t *testing.T) {
 	noUnset := "${VAR?message here$ARG}"
 
 	// disallow empty
-	newWord, err = shlex.ProcessWord(noEmpty, setEnvs)
+	newWord, _, err = shlex.ProcessWord(noEmpty, setEnvs)
 	require.NoError(t, err)
 	require.Equal(t, "plain", newWord)
 
-	_, err = shlex.ProcessWord(noEmpty, emptyEnvs)
+	_, _, err = shlex.ProcessWord(noEmpty, emptyEnvs)
 	require.ErrorContains(t, err, "message herex")
 
-	_, err = shlex.ProcessWord(noEmpty, unsetEnvs)
+	_, _, err = shlex.ProcessWord(noEmpty, unsetEnvs)
 	require.ErrorContains(t, err, "message herex")
 
 	// disallow unset
-	newWord, err = shlex.ProcessWord(noUnset, setEnvs)
+	newWord, _, err = shlex.ProcessWord(noUnset, setEnvs)
 	require.NoError(t, err)
 	require.Equal(t, "plain", newWord)
 
-	newWord, err = shlex.ProcessWord(noUnset, emptyEnvs)
+	newWord, _, err = shlex.ProcessWord(noUnset, emptyEnvs)
 	require.NoError(t, err)
 	require.Empty(t, newWord)
 
-	_, err = shlex.ProcessWord(noUnset, unsetEnvs)
+	_, _, err = shlex.ProcessWord(noUnset, unsetEnvs)
 	require.ErrorContains(t, err, "message herex")
 }
 
@@ -123,7 +123,7 @@ func TestShellParser4EnvVars(t *testing.T) {
 
 		if ((platform == "W" || platform == "A") && runtime.GOOS == "windows") ||
 			((platform == "U" || platform == "A") && runtime.GOOS != "windows") {
-			newWord, err := shlex.ProcessWord(source, envs)
+			newWord, _, err := shlex.ProcessWord(source, envs)
 			if expected == "error" {
 				require.Errorf(t, err, "input: %q, result: %q", source, newWord)
 			} else {