Skip to content

Commit

Permalink
Merge pull request #4925 from tonistiigi/dockerfile-platform-validation
Browse files Browse the repository at this point in the history 
dockerfile: improve error messages and add suggest to flag parsing
  • Loading branch information
@tonistiigi
tonistiigi committed May 30, 2024
2 parents 265c38c + 94ee93a commit 1eb2a03
Show file tree
Hide file tree
Showing 4 changed files with 143 additions and 32 deletions.
63 changes: 49 additions & 14 deletions frontend/dockerfile/dockerfile2llb/convert.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
// set base state for every image
for i, st := range stages {
nameMatch, err := shlex.ProcessWordWithMatches(st.BaseName, metaArgsToMap(optMetaArgs))
reportUnusedFromArgs(nameMatch.Unmatched, st.Location, opt.Warn)
reportUnusedFromArgs(metaArgsKeys(optMetaArgs), nameMatch.Unmatched, st.Location, opt.Warn)
used := nameMatch.Matched

if err != nil {
Expand All @@ -285,15 +285,24 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS

if v := st.Platform; v != "" {
platMatch, err := shlex.ProcessWordWithMatches(v, metaArgsToMap(optMetaArgs))
reportUnusedFromArgs(platMatch.Unmatched, st.Location, opt.Warn)
reportUnusedFromArgs(metaArgsKeys(optMetaArgs), platMatch.Unmatched, st.Location, opt.Warn)

if err != nil {
return nil, parser.WithLocation(errors.Wrapf(err, "failed to process arguments for platform %s", platMatch.Result), st.Location)
}

if platMatch.Result == "" {
err := errors.Errorf("empty platform value from expression %s", v)
err = parser.WithLocation(err, st.Location)
err = wrapSuggestAny(err, platMatch.Unmatched, metaArgsKeys(optMetaArgs))
return nil, err
}

p, err := platforms.Parse(platMatch.Result)
if err != nil {
return nil, parser.WithLocation(errors.Wrapf(err, "failed to parse platform %s", platMatch.Result), st.Location)
err = parser.WithLocation(err, st.Location)
err = wrapSuggestAny(err, platMatch.Unmatched, metaArgsKeys(optMetaArgs))
return nil, parser.WithLocation(errors.Wrapf(err, "failed to parse platform %s", v), st.Location)
}

for k := range platMatch.Matched {
Expand Down Expand Up @@ -688,6 +697,14 @@ func metaArgsToMap(metaArgs []instructions.KeyValuePairOptional) map[string]stri
return m
}

func metaArgsKeys(metaArgs []instructions.KeyValuePairOptional) []string {
s := make([]string, 0, len(metaArgs))
for _, arg := range metaArgs {
s = append(s, arg.Key)
}
return s
}

func toCommand(ic instructions.Command, allDispatchStates *dispatchStates) (command, error) {
cmd := command{Command: ic}
if c, ok := ic.(*instructions.CopyCommand); ok {
Expand Down Expand Up @@ -750,7 +767,7 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error {
}

newword, unmatched, err := opt.shlex.ProcessWord(word, env)
reportUnmatchedVariables(cmd, d.buildArgs, unmatched, &opt)
reportUnmatchedVariables(cmd, d.buildArgs, env, unmatched, &opt)
return newword, err
})
if err != nil {
Expand All @@ -766,7 +783,7 @@ 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)
reportUnmatchedVariables(cmd, d.buildArgs, env, unmatched, &opt)
return newword, err
})
if err != nil {
Expand Down Expand Up @@ -2077,19 +2094,25 @@ func validateStageNames(stages []instructions.Stage, warn linter.LintWarnFunc) {
}
}

func reportUnmatchedVariables(cmd instructions.Command, buildArgs []instructions.KeyValuePairOptional, unmatched map[string]struct{}, opt *dispatchOpt) {
func reportUnmatchedVariables(cmd instructions.Command, buildArgs []instructions.KeyValuePairOptional, env []string, unmatched map[string]struct{}, opt *dispatchOpt) {
for _, buildArg := range buildArgs {
delete(unmatched, buildArg.Key)
}
if len(unmatched) == 0 {
return
}
for _, buildArg := range buildArgs {
delete(unmatched, buildArg.Key)
options := metaArgsKeys(opt.metaArgs)
for _, envVar := range env {
key, _ := parseKeyValue(envVar)
options = append(options, key)
}
for cmdVar := range unmatched {
_, nonEnvOk := nonEnvArgs[cmdVar]
if !nonEnvOk {
msg := linter.RuleUndefinedVar.Format(cmdVar)
linter.RuleUndefinedVar.Run(opt.lintWarn, cmd.Location(), msg)
if _, nonEnvOk := nonEnvArgs[cmdVar]; nonEnvOk {
continue
}
match, _ := suggest.Search(cmdVar, options, true)
msg := linter.RuleUndefinedVar.Format(cmdVar, match)
linter.RuleUndefinedVar.Run(opt.lintWarn, cmd.Location(), msg)
}
}

Expand Down Expand Up @@ -2143,9 +2166,10 @@ func toPBLocation(sourceIndex int, location []parser.Range) pb.Location {
}
}

func reportUnusedFromArgs(unmatched map[string]struct{}, location []parser.Range, warn linter.LintWarnFunc) {
func reportUnusedFromArgs(values []string, unmatched map[string]struct{}, location []parser.Range, warn linter.LintWarnFunc) {
for arg := range unmatched {
msg := linter.RuleUndeclaredArgInFrom.Format(arg)
suggest, _ := suggest.Search(arg, values, true)
msg := linter.RuleUndeclaredArgInFrom.Format(arg, suggest)
linter.RuleUndeclaredArgInFrom.Run(warn, location, msg)
}
}
Expand All @@ -2169,3 +2193,14 @@ func validateUsedOnce(c instructions.Command, loc *instructionTracker, warn lint
}
loc.MarkUsed(c.Location())
}

func wrapSuggestAny(err error, keys map[string]struct{}, options []string) error {
for k := range keys {
var ok bool
ok, err = suggest.WrapErrorMaybe(err, k, options, true)
if ok {
break
}
}
return err
}
65 changes: 62 additions & 3 deletions frontend/dockerfile/dockerfile_lint_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -463,16 +463,38 @@ COPY Dockerfile .
{
RuleName: "UndeclaredArgInFrom",
Description: "FROM command must use declared ARGs",
Detail: "FROM argument 'BULIDPLATFORM' is not declared",
Detail: "FROM argument 'BULIDPLATFORM' is not declared (did you mean BUILDPLATFORM?)",
Level: 1,
Line: 2,
},
},
StreamBuildErr: "failed to solve: failed to parse platform : \"\" is an invalid component of \"\": platform specifier component must match \"^[A-Za-z0-9_-]+$\": invalid argument",
UnmarshalBuildErr: "failed to parse platform : \"\" is an invalid component of \"\": platform specifier component must match \"^[A-Za-z0-9_-]+$\": invalid argument",
StreamBuildErr: "failed to solve: empty platform value from expression $BULIDPLATFORM (did you mean BUILDPLATFORM?)",
UnmarshalBuildErr: "empty platform value from expression $BULIDPLATFORM (did you mean BUILDPLATFORM?)",
BuildErrLocation: 2,
})

dockerfile = []byte(`
ARG MY_OS=linux
ARG MY_ARCH=amd64
FROM --platform=linux/${MYARCH} busybox
COPY Dockerfile .
`)
checkLinterWarnings(t, sb, &lintTestParams{
Dockerfile: dockerfile,
Warnings: []expectedLintWarning{
{
RuleName: "UndeclaredArgInFrom",
Description: "FROM command must use declared ARGs",
Detail: "FROM argument 'MYARCH' is not declared (did you mean MY_ARCH?)",
Level: 1,
Line: 4,
},
},
StreamBuildErr: "failed to solve: failed to parse platform linux/${MYARCH}: \"\" is an invalid component of \"linux/\": platform specifier component must match \"^[A-Za-z0-9_-]+$\": invalid argument (did you mean MY_ARCH?)",
UnmarshalBuildErr: "failed to parse platform linux/${MYARCH}: \"\" is an invalid component of \"linux/\": platform specifier component must match \"^[A-Za-z0-9_-]+$\": invalid argument (did you mean MY_ARCH?)",
BuildErrLocation: 4,
})

dockerfile = []byte(`
ARG tag=latest
FROM busybox:${tag}${version} AS b
Expand Down Expand Up @@ -561,6 +583,43 @@ RUN echo $foo
},
},
})

dockerfile = []byte(`
FROM alpine
ARG DIR_BINARIES=binaries/
ARG DIR_ASSETS=assets/
ARG DIR_CONFIG=config/
COPY $DIR_ASSET .
`)
checkLinterWarnings(t, sb, &lintTestParams{
Dockerfile: dockerfile,
Warnings: []expectedLintWarning{
{
RuleName: "UndefinedVar",
Description: "Variables should be defined before their use",
Detail: "Usage of undefined variable '$DIR_ASSET' (did you mean $DIR_ASSETS?)",
Level: 1,
Line: 6,
},
},
})

dockerfile = []byte(`
FROM alpine
ENV PATH=$PAHT:/tmp/bin
`)
checkLinterWarnings(t, sb, &lintTestParams{
Dockerfile: dockerfile,
Warnings: []expectedLintWarning{
{
RuleName: "UndefinedVar",
Description: "Variables should be defined before their use",
Detail: "Usage of undefined variable '$PAHT' (did you mean $PATH?)",
Level: 1,
Line: 3,
},
},
})
}

func testMultipleInstructionsDisallowed(t *testing.T, sb integration.Sandbox) {
Expand Down
20 changes: 14 additions & 6 deletions frontend/dockerfile/linter/ruleset.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,15 @@ var (
return "Maintainer instruction is deprecated in favor of using label"
},
}
RuleUndeclaredArgInFrom = LinterRule[func(string) string]{
RuleUndeclaredArgInFrom = LinterRule[func(string, string) string]{
Name: "UndeclaredArgInFrom",
Description: "FROM command must use declared ARGs",
Format: func(baseArg string) string {
return fmt.Sprintf("FROM argument '%s' is not declared", baseArg)
Format: func(baseArg, suggest string) string {
out := fmt.Sprintf("FROM argument '%s' is not declared", baseArg)
if suggest != "" {
out += fmt.Sprintf(" (did you mean %s?)", suggest)
}
return out
},
}
RuleWorkdirRelativePath = LinterRule[func(workdir string) string]{
Expand All @@ -91,11 +95,15 @@ var (
return fmt.Sprintf("Usage of undefined variable '$%s'", arg)
},
}
RuleUndefinedVar = LinterRule[func(string) string]{
RuleUndefinedVar = LinterRule[func(string, 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)
Format: func(arg, suggest string) string {
out := fmt.Sprintf("Usage of undefined variable '$%s'", arg)
if suggest != "" {
out += fmt.Sprintf(" (did you mean $%s?)", suggest)
}
return out
},
}
RuleMultipleInstructionsDisallowed = LinterRule[func(instructionName string) string]{
Expand Down
27 changes: 18 additions & 9 deletions util/suggest/error.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,7 @@ import (
"github.com/agext/levenshtein"
)

// WrapError wraps error with a suggestion for fixing it
func WrapError(err error, val string, options []string, caseSensitive bool) error {
if err == nil {
return nil
}
func Search(val string, options []string, caseSensitive bool) (string, bool) {
orig := val
if !caseSensitive {
val = strings.ToLower(val)
Expand All @@ -23,7 +19,7 @@ func WrapError(err error, val string, options []string, caseSensitive bool) erro
}
if val == opt {
// exact match means error was unrelated to the value
return err
return "", false
}
dist := levenshtein.Distance(val, opt, nil)
if dist < mindist {
Expand All @@ -35,12 +31,25 @@ func WrapError(err error, val string, options []string, caseSensitive bool) erro
mindist = dist
}
}
return match, match != ""
}

if match == "" {
return err
// WrapError wraps error with a suggestion for fixing it
func WrapError(err error, val string, options []string, caseSensitive bool) error {
_, err = WrapErrorMaybe(err, val, options, caseSensitive)
return err
}

func WrapErrorMaybe(err error, val string, options []string, caseSensitive bool) (bool, error) {
if err == nil {
return false, nil
}
match, ok := Search(val, options, caseSensitive)
if match == "" || !ok {
return false, err
}

return &suggestError{
return true, &suggestError{
err: err,
match: match,
}
Expand Down

0 comments on commit 1eb2a03

Please sign in to comment.