Skip to content

Commit

Permalink
move validateCommandVar to happen during dispatch and add additional …
Browse files Browse the repository at this point in the history 
…test cases

Signed-off-by: Talon Bowler <talon.bowler@docker.com>
  • Loading branch information
@daghack
daghack committed May 3, 2024
1 parent a865a74 commit 5a3351e
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 46 deletions.
58 changes: 22 additions & 36 deletions frontend/dockerfile/dockerfile2llb/convert.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -364,8 +364,6 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
}
}

validateCommandArgs(allDispatchStates, shlex, opt.Warn)

var target *dispatchState
if opt.Target == "" {
target = allDispatchStates.lastTarget()
Expand Down Expand Up @@ -613,6 +611,8 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
cgroupParent: opt.CgroupParent,
llbCaps: opt.LLBCaps,
sourceMap: opt.SourceMap,
warn: opt.Warn,
argCmdVars: make(map[string]struct{}),
}

if err = dispatchOnBuildTriggers(d, d.image.Config.OnBuild, opt); err != nil {
Expand Down Expand Up @@ -748,15 +748,17 @@ type dispatchOpt struct {
cgroupParent string
llbCaps *apicaps.CapSet
sourceMap *llb.SourceMap
warn linter.LintWarnFunc
argCmdVars map[string]struct{}
}

func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error {
env, err := d.state.Env(context.TODO())
if err != nil {
return err
}
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
}
newword, _, err := opt.shlex.ProcessWord(word, env)
return newword, err
})
Expand All @@ -766,11 +768,6 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error {
}
if ex, ok := cmd.Command.(instructions.SupportsSingleWordExpansionRaw); ok {
err := ex.ExpandRaw(func(word string) (string, error) {
env, err := d.state.Env(context.TODO())
if err != nil {
return "", err
}

lex := shell.NewLex('\\')
lex.SkipProcessQuotes = true
newword, _, err := lex.ProcessWord(word, env)
Expand All @@ -780,8 +777,8 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error {
return err
}
}
validateCommandVar(cmd.Command, env, &opt)

var err error
switch c := cmd.Command.(type) {
case *instructions.MaintainerCommand:
err = dispatchMaintainer(d, c)
Expand Down Expand Up @@ -840,7 +837,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.metaArgs, &opt)
case *instructions.CopyCommand:
l := opt.buildContext
if len(cmd.sources) != 0 {
Expand Down Expand Up @@ -1539,10 +1536,11 @@ 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, metaArgs []instructions.KeyValuePairOptional, opt *dispatchOpt) error {
commitStrs := make([]string, 0, len(c.Args))
for _, arg := range c.Args {
buildArg := setKVValue(arg, buildArgValues)
opt.argCmdVars[arg.Key] = struct{}{}
buildArg := setKVValue(arg, opt.buildArgValues)

commitStr := arg.Key
if arg.Value != nil {
Expand Down Expand Up @@ -2038,27 +2036,15 @@ func validateStageNames(stages []instructions.Stage, warn linter.LintWarnFunc) {
}
}

func validateCommandArgs(allDispatchStates *dispatchStates, shlex *shell.Lex, warn linter.LintWarnFunc) {
nonEnvArgs := map[string]struct{}{}
for _, d := range allDispatchStates.states {
env, err := d.state.Env(context.TODO())
if err != nil {
continue
}
for _, cmd := range d.stage.Commands {
if argCmd, ok := cmd.(*instructions.ArgCommand); ok {
for _, arg := range argCmd.Args {
nonEnvArgs[arg.Key] = struct{}{}
}
}
if cmdstr, ok := cmd.(fmt.Stringer); ok {
_, unmatched, _ := shlex.ProcessWord(cmdstr.String(), env)
for arg := range unmatched {
if _, ok := nonEnvArgs[arg]; !ok {
msg := linter.RuleUndefinedArg.Format(arg)
linter.RuleUndefinedArg.Run(warn, cmd.Location(), msg)
}
}
func validateCommandVar(cmd instructions.Command, env []string, opt *dispatchOpt) {
if cmdstr, ok := cmd.(fmt.Stringer); ok {
_, unmatched, _ := opt.shlex.ProcessWord(cmdstr.String(), env)
for arg := range unmatched {
_, argCmdOk := opt.argCmdVars[arg]
_, nonEnvOk := nonEnvArgs[arg]
if !argCmdOk && !nonEnvOk {
msg := linter.RuleUndefinedVar.Format(arg)
linter.RuleUndefinedVar.Run(opt.warn, cmd.Location(), msg)
}
}
}
Expand Down
28 changes: 22 additions & 6 deletions frontend/dockerfile/dockerfile_lint_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ var lintTests = integration.TestFuncs(
testMaintainerDeprecated,
testWarningsBeforeError,
testUndeclaredArg,
testUndefinedArg,
testUndefinedVars,
)

func testStageName(t *testing.T, sb integration.Sandbox) {
Expand Down Expand Up @@ -419,7 +419,7 @@ COPY Dockerfile .
})
}

func testUndefinedArg(t *testing.T, sb integration.Sandbox) {
func testUndefinedVars(t *testing.T, sb integration.Sandbox) {
dockerfile := []byte(`
FROM scratch
ARG foo
Expand All @@ -428,7 +428,22 @@ COPY Dockerfile${foo} .
checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})

dockerfile = []byte(`
FROM busybox
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
Expand All @@ -437,9 +452,9 @@ RUN echo $foo
Dockerfile: dockerfile,
Warnings: []expectedLintWarning{
{
RuleName: "UndefinedArg",
Description: "ARGs should be defined before their use",
Detail: "Usage of undefined ARG 'foo'",
RuleName: "UndefinedVar",
Description: "Variables should be defined before their use",
Detail: "Usage of undefined variable '$foo'",
Level: 1,
Line: 3,
},
Expand Down Expand Up @@ -476,6 +491,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]
Expand Down
8 changes: 4 additions & 4 deletions frontend/dockerfile/linter/ruleset.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,11 @@ var (
return fmt.Sprintf("FROM argument '%s' is not declared", baseArg)
},
}
RuleUndefinedArg = LinterRule[func(string) string]{
Name: "UndefinedArg",
Description: "ARGs should be defined before their use",
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 ARG '%s'", arg)
return fmt.Sprintf("Usage of undefined variable '$%s'", arg)
},
}
)

0 comments on commit 5a3351e

Please sign in to comment.