Skip to content

Commit

Permalink
Adds linter warning for using variables defined in parent stages
Browse files Browse the repository at this point in the history 
Signed-off-by: Talon Bowler <[email protected]>
  • Loading branch information
@daghack
daghack committed May 16, 2024
1 parent 28264b4 commit 1df6503
Show file tree
Hide file tree
Showing 5 changed files with 152 additions and 31 deletions.
84 changes: 71 additions & 13 deletions frontend/dockerfile/dockerfile2llb/convert.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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))
}
Expand Down Expand Up @@ -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),
)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
61 changes: 59 additions & 2 deletions frontend/dockerfile/dockerfile_lint_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ var lintTests = integration.TestFuncs(
testWorkdirRelativePath,
testUnmatchedVars,
testMultipleInstructionsDisallowed,
testOutOfScopeVar,
)

func testStageName(t *testing.T, sb integration.Sandbox) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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: 5,
},
{
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: 10,
},
},
})
}

func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestParams) {
destDir, err := os.MkdirTemp("", "buildkit")
require.NoError(t, err)
Expand Down
7 changes: 7 additions & 0 deletions frontend/dockerfile/linter/ruleset.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
},
}
)
4 changes: 2 additions & 2 deletions frontend/dockerfile/shell/lex.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
27 changes: 13 additions & 14 deletions frontend/dockerfile/shell/lex_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"}
Expand All @@ -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)

Check failure on line 80 in frontend/dockerfile/shell/lex_test.go

View workflow job for this annotation

GitHub Actions / test / run (./..., nydus, 1, integration)

Failed: frontend/dockerfile/shell/TestShellParserMandatoryEnvVars 

=== RUN   TestShellParserMandatoryEnvVars
    lex_test.go:80: 
        	Error Trace:	/src/frontend/dockerfile/shell/lex_test.go:80
        	Error:      	Should be empty, but was plain
        	Test:       	TestShellParserMandatoryEnvVars
--- FAIL: TestShellParserMandatoryEnvVars (0.00s)

Check failure on line 80 in frontend/dockerfile/shell/lex_test.go

View workflow job for this annotation

GitHub Actions / test / run (./..., 1, integration gateway)

Failed: frontend/dockerfile/shell/TestShellParserMandatoryEnvVars 

=== RUN   TestShellParserMandatoryEnvVars
    lex_test.go:80: 
        	Error Trace:	/src/frontend/dockerfile/shell/lex_test.go:80
        	Error:      	Should be empty, but was plain
        	Test:       	TestShellParserMandatoryEnvVars
--- FAIL: TestShellParserMandatoryEnvVars (0.00s)

_, _, err = shlex.ProcessWord(noUnset, unsetEnvs)
_, err = shlex.ProcessWord(noUnset, unsetEnvs)
require.ErrorContains(t, err, "message herex")
}

Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit 1df6503

Please sign in to comment.