From 4dc51677760b42b623b867d357ac501a5978bf31 Mon Sep 17 00:00:00 2001 From: Talon Bowler Date: Wed, 15 May 2024 15:34:58 -0700 Subject: [PATCH] Adds linter warning for using variables defined in parent stages Signed-off-by: Talon Bowler --- frontend/dockerfile/dockerfile2llb/convert.go | 84 ++++++++++++++++--- frontend/dockerfile/dockerfile_lint_test.go | 61 +++++++++++++- frontend/dockerfile/linter/ruleset.go | 7 ++ frontend/dockerfile/shell/lex.go | 4 +- frontend/dockerfile/shell/lex_test.go | 27 +++--- 5 files changed, 152 insertions(+), 31 deletions(-) diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 3f0282c21a3fe..616c930578f75 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -748,9 +748,9 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error { return "", err } - newword, unmatched, err := opt.shlex.ProcessWord(word, env) - reportUnmatchedVariables(cmd, d.buildArgs, unmatched, &opt) - return newword, err + result, err := opt.shlex.ProcessWord(word, env) + reportUnmatchedVariables(cmd, d.buildArgs, result.Unmatched, &opt) + return result.Result, err }) if err != nil { return err @@ -764,9 +764,9 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error { } lex := shell.NewLex('\\') lex.SkipProcessQuotes = true - newword, unmatched, err := lex.ProcessWord(word, env) - reportUnmatchedVariables(cmd, d.buildArgs, unmatched, &opt) - return newword, err + result, err := lex.ProcessWord(word, env) + reportUnmatchedVariables(cmd, d.buildArgs, result.Unmatched, &opt) + return result.Result, err }) if err != nil { return err @@ -1125,7 +1125,17 @@ func dispatchRun(d *dispatchState, c *instructions.RunCommand, proxy *llb.ProxyE if err != nil { return err } - opt = append(opt, llb.WithCustomName(prefixCommand(d, uppercaseCmd(processCmdEnv(&shlex, customname, env)), d.prefixPlatform, pl, env))) + + name := processCmdEnv(processCmdArgs{ + cmd: customname, + env: env, + dispatchState: d, + dispatchOpt: &dopt, + location: c.Location(), + shlex: &shlex, + }) + + opt = append(opt, llb.WithCustomName(prefixCommand(d, uppercaseCmd(name), d.prefixPlatform, pl, env))) for _, h := range dopt.extraHosts { opt = append(opt, llb.AddExtraHost(h.Host, h.IP)) } @@ -1193,8 +1203,18 @@ func dispatchWorkdir(d *dispatchState, c *instructions.WorkdirCommand, commit bo if err != nil { return err } + + name := processCmdEnv(processCmdArgs{ + cmd: c.String(), + env: env, + dispatchState: d, + dispatchOpt: opt, + location: c.Location(), + shlex: opt.shlex, + }) + d.state = d.state.File(llb.Mkdir(wd, 0755, mkdirOpt...), - llb.WithCustomName(prefixCommand(d, uppercaseCmd(processCmdEnv(opt.shlex, c.String(), env)), d.prefixPlatform, &platform, env)), + llb.WithCustomName(prefixCommand(d, uppercaseCmd(name), d.prefixPlatform, &platform, env)), location(opt.sourceMap, c.Location()), llb.Platform(*d.platform), ) @@ -1272,7 +1292,15 @@ func dispatchCopy(d *dispatchState, cfg copyConfig) error { return err } - name := uppercaseCmd(processCmdEnv(cfg.opt.shlex, cfg.cmdToPrint.String(), env)) + name := uppercaseCmd(processCmdEnv(processCmdArgs{ + cmd: cfg.cmdToPrint.String(), + env: env, + dispatchState: d, + dispatchOpt: &cfg.opt, + location: cfg.location, + shlex: cfg.opt.shlex, + })) + pgName := prefixCommand(d, name, d.prefixPlatform, &platform, env) var a *llb.FileAction @@ -1886,12 +1914,24 @@ func uppercaseCmd(str string) string { return strings.Join(p, " ") } -func processCmdEnv(shlex *shell.Lex, cmd string, env []string) string { - w, _, err := shlex.ProcessWord(cmd, env) +type processCmdArgs struct { + cmd string + dispatchOpt *dispatchOpt + dispatchState *dispatchState + env []string + location []parser.Range + shlex *shell.Lex +} + +func processCmdEnv(args processCmdArgs) string { + results, err := args.shlex.ProcessWord(args.cmd, args.env) if err != nil { - return cmd + return args.cmd } - return w + if args.dispatchOpt.lintWarn != nil { + reportOutOfScopeVars(args, results.Matched) + } + return results.Result } func prefixCommand(ds *dispatchState, str string, prefixPlatform bool, platform *ocispecs.Platform, env []string) string { @@ -2086,6 +2126,24 @@ func validateStageNames(stages []instructions.Stage, warn linter.LintWarnFunc) { } } +func reportOutOfScopeVars(args processCmdArgs, matched map[string]struct{}) { + parentBuildArgs := make(map[string]struct{}) + for base := args.dispatchState.base; base != nil; base = base.base { + for _, ba := range base.buildArgs { + parentBuildArgs[ba.Key] = struct{}{} + } + } + for _, ba := range args.dispatchState.buildArgs { + delete(parentBuildArgs, ba.Key) + } + for match := range matched { + if _, ok := parentBuildArgs[match]; ok { + msg := linter.RuleOutOfScopeVar.Format(match, args.dispatchState.stageName) + linter.RuleOutOfScopeVar.Run(args.dispatchOpt.lintWarn, args.location, msg) + } + } +} + func reportUnmatchedVariables(cmd instructions.Command, buildArgs []instructions.KeyValuePairOptional, unmatched map[string]struct{}, opt *dispatchOpt) { if len(unmatched) == 0 { return diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index 186b63ad88e18..b550479e81f2c 100644 --- 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, + testOutOfScopeVar, ) func testStageName(t *testing.T, sb integration.Sandbox) { @@ -531,10 +532,24 @@ COPY Dockerfile${foo} . FROM alpine AS base ARG foo=Dockerfile -FROM base +FROM base AS first +COPY $foo . +ARG foo=Dockerfile COPY $foo . + `) - checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) + checkLinterWarnings(t, sb, &lintTestParams{ + Dockerfile: dockerfile, + Warnings: []expectedLintWarning{ + { + RuleName: "OutOfScopeVar", + Description: "Variables should be defined within the same stage", + Detail: "Variable 'foo' is out of scope for stage 'first', it should be defined within the same stage", + Level: 1, + Line: 6, + }, + }, + }) dockerfile = []byte(` FROM alpine @@ -628,6 +643,48 @@ HEALTHCHECK CMD ["/myotherapp"] checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) } +func testOutOfScopeVar(t *testing.T, sb integration.Sandbox) { + dockerfile := []byte(` +FROM alpine AS base +ARG foo=bar +`) + checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) + + dockerfile = []byte(` +FROM alpine AS base +ARG foo=bar + +FROM base AS first +RUN echo $foo +ARG foo=baz +RUN echo $foo + +FROM first AS second +RUN echo $foo +ARG foo=baz +RUN echo $foo +`) + checkLinterWarnings(t, sb, &lintTestParams{ + Dockerfile: dockerfile, + Warnings: []expectedLintWarning{ + { + RuleName: "OutOfScopeVar", + Description: "Variables should be defined within the same stage", + Detail: "Variable 'foo' is out of scope for stage 'first', it should be defined within the same stage", + Level: 1, + Line: 6, + }, + { + RuleName: "OutOfScopeVar", + Description: "Variables should be defined within the same stage", + Detail: "Variable 'foo' is out of scope for stage 'second', it should be defined within the same stage", + Level: 1, + Line: 11, + }, + }, + }) +} + func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestParams) { destDir, err := os.MkdirTemp("", "buildkit") require.NoError(t, err) diff --git a/frontend/dockerfile/linter/ruleset.go b/frontend/dockerfile/linter/ruleset.go index 3c15dccab12d9..505a7f43f2656 100644 --- 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) }, } + RuleOutOfScopeVar = LinterRule[func(string, string) string]{ + Name: "OutOfScopeVar", + Description: "Variables should be defined within the same stage", + Format: func(word, stageName string) string { + return fmt.Sprintf("Variable '%s' is out of scope for stage '%s', it should be defined within the same stage", word, stageName) + }, + } ) diff --git a/frontend/dockerfile/shell/lex.go b/frontend/dockerfile/shell/lex.go index 306eb5e81939f..2878de85ba6f2 100644 --- a/frontend/dockerfile/shell/lex.go +++ b/frontend/dockerfile/shell/lex.go @@ -35,9 +35,9 @@ func NewLex(escapeToken rune) *Lex { // 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) { +func (s *Lex) ProcessWord(word string, env []string) (*ProcessWordResult, error) { result, err := s.process(word, BuildEnvs(env)) - return result.Result, result.Unmatched, err + return result, err } // ProcessWords will use the 'env' list of environment variables, diff --git a/frontend/dockerfile/shell/lex_test.go b/frontend/dockerfile/shell/lex_test.go index 9e8f05a5b67cc..bd8386df38b4e 100644 --- a/frontend/dockerfile/shell/lex_test.go +++ b/frontend/dockerfile/shell/lex_test.go @@ -50,7 +50,6 @@ func TestReversePattern(t *testing.T) { } func TestShellParserMandatoryEnvVars(t *testing.T) { - var newWord string var err error shlex := NewLex('\\') setEnvs := []string{"VAR=plain", "ARG=x"} @@ -61,26 +60,26 @@ func TestShellParserMandatoryEnvVars(t *testing.T) { noUnset := "${VAR?message here$ARG}" // disallow empty - newWord, _, err = shlex.ProcessWord(noEmpty, setEnvs) + result, err := shlex.ProcessWord(noEmpty, setEnvs) require.NoError(t, err) - require.Equal(t, "plain", newWord) + require.Equal(t, "plain", result.Result) - _, _, 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) + result, err = shlex.ProcessWord(noUnset, setEnvs) require.NoError(t, err) - require.Equal(t, "plain", newWord) + require.Equal(t, "plain", result.Result) - newWord, _, err = shlex.ProcessWord(noUnset, emptyEnvs) + result, err = shlex.ProcessWord(noUnset, setEnvs) require.NoError(t, err) - require.Empty(t, newWord) + require.Empty(t, result.Result) - _, _, err = shlex.ProcessWord(noUnset, unsetEnvs) + _, err = shlex.ProcessWord(noUnset, unsetEnvs) require.ErrorContains(t, err, "message herex") } @@ -123,15 +122,15 @@ 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) + result, err := shlex.ProcessWord(source, envs) if expected == "error" { - require.Errorf(t, err, "input: %q, result: %q", source, newWord) + require.Errorf(t, err, "input: %q, result: %q", source, result.Result) } else { require.NoError(t, err, "at line %d of %s", lineCount, fn) - require.Equal(t, expected, newWord, "at line %d of %s", lineCount, fn) + require.Equal(t, expected, result.Result, "at line %d of %s", lineCount, fn) } - newWord, err = shlex.ProcessWordWithMap(source, envsMap) + newWord, err := shlex.ProcessWordWithMap(source, envsMap) if expected == "error" { require.Errorf(t, err, "input: %q, result: %q", source, newWord) } else {