diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 43d3a2b7975b..2bb9219db15f 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/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 { diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index 3d69a5420890..58246e65b289 100644 --- a/frontend/dockerfile/dockerfile_lint_test.go +++ b/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] diff --git a/frontend/dockerfile/linter/ruleset.go b/frontend/dockerfile/linter/ruleset.go index 2d42b61cc331..cc29794d7c1b 100644 --- a/frontend/dockerfile/linter/ruleset.go +++ b/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) + }, + } ) diff --git a/frontend/dockerfile/shell/lex.go b/frontend/dockerfile/shell/lex.go index 3ae23f5ea668..306eb5e81939 100644 --- a/frontend/dockerfile/shell/lex.go +++ b/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 { diff --git a/frontend/dockerfile/shell/lex_test.go b/frontend/dockerfile/shell/lex_test.go index 08dd6f9b4cef..9e8f05a5b67c 100644 --- a/frontend/dockerfile/shell/lex_test.go +++ b/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 {