From 05782a788c98593c9df7df31e7419b2124687b72 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Mon, 13 May 2024 22:56:05 -0700 Subject: [PATCH 1/3] dockerfile: improve error messages and add suggest to platform flag parsing Signed-off-by: Tonis Tiigi --- frontend/dockerfile/dockerfile2llb/convert.go | 30 ++++++++++++++++++- frontend/dockerfile/dockerfile_lint_test.go | 26 ++++++++++++++-- util/suggest/error.go | 13 +++++--- 3 files changed, 62 insertions(+), 7 deletions(-) diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 3f0282c21a3f..301de585b27d 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -291,9 +291,18 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS 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 { @@ -687,6 +696,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 { @@ -2178,3 +2195,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 + err, ok = suggest.WrapErrorMaybe(err, k, options, true) + if ok { + break + } + } + return err +} diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index 186b63ad88e1..c3efe9174ba4 100644 --- a/frontend/dockerfile/dockerfile_lint_test.go +++ b/frontend/dockerfile/dockerfile_lint_test.go @@ -467,11 +467,33 @@ COPY Dockerfile . 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", + 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 diff --git a/util/suggest/error.go b/util/suggest/error.go index 0ad7b5c8679a..1c3d41e6f965 100644 --- a/util/suggest/error.go +++ b/util/suggest/error.go @@ -8,8 +8,13 @@ import ( // 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) (error, bool) { if err == nil { - return nil + return nil, false } orig := val if !caseSensitive { @@ -23,7 +28,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 err, false } dist := levenshtein.Distance(val, opt, nil) if dist < mindist { @@ -37,13 +42,13 @@ func WrapError(err error, val string, options []string, caseSensitive bool) erro } if match == "" { - return err + return err, false } return &suggestError{ err: err, match: match, - } + }, true } type suggestError struct { From 555ea7d152d9e37853db60f0fc8d4a3e36b6577c Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Mon, 13 May 2024 23:16:53 -0700 Subject: [PATCH 2/3] dockerfile: add hint suggestions to UndeclaredArgInFrom Signed-off-by: Tonis Tiigi --- frontend/dockerfile/dockerfile2llb/convert.go | 11 +++--- frontend/dockerfile/dockerfile_lint_test.go | 4 +-- frontend/dockerfile/linter/ruleset.go | 10 ++++-- util/suggest/error.go | 34 +++++++++++-------- 4 files changed, 34 insertions(+), 25 deletions(-) diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 301de585b27d..fe3ba2ac0e4d 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -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 { @@ -285,7 +285,7 @@ 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) @@ -2169,9 +2169,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) } } @@ -2199,7 +2200,7 @@ func validateUsedOnce(c instructions.Command, loc *instructionTracker, warn lint func wrapSuggestAny(err error, keys map[string]struct{}, options []string) error { for k := range keys { var ok bool - err, ok = suggest.WrapErrorMaybe(err, k, options, true) + ok, err = suggest.WrapErrorMaybe(err, k, options, true) if ok { break } diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index c3efe9174ba4..7343b8300c1e 100644 --- a/frontend/dockerfile/dockerfile_lint_test.go +++ b/frontend/dockerfile/dockerfile_lint_test.go @@ -462,7 +462,7 @@ 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, }, @@ -484,7 +484,7 @@ COPY Dockerfile . { RuleName: "UndeclaredArgInFrom", Description: "FROM command must use declared ARGs", - Detail: "FROM argument 'MYARCH' is not declared", + Detail: "FROM argument 'MYARCH' is not declared (did you mean MY_ARCH?)", Level: 1, Line: 4, }, diff --git a/frontend/dockerfile/linter/ruleset.go b/frontend/dockerfile/linter/ruleset.go index 3c15dccab12d..ba96fc744485 100644 --- a/frontend/dockerfile/linter/ruleset.go +++ b/frontend/dockerfile/linter/ruleset.go @@ -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]{ diff --git a/util/suggest/error.go b/util/suggest/error.go index 1c3d41e6f965..8b23f48a3eee 100644 --- a/util/suggest/error.go +++ b/util/suggest/error.go @@ -6,16 +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 { - err, _ = WrapErrorMaybe(err, val, options, caseSensitive) - return err -} - -func WrapErrorMaybe(err error, val string, options []string, caseSensitive bool) (error, bool) { - if err == nil { - return nil, false - } +func Search(val string, options []string, caseSensitive bool) (string, bool) { orig := val if !caseSensitive { val = strings.ToLower(val) @@ -28,7 +19,7 @@ func WrapErrorMaybe(err error, val string, options []string, caseSensitive bool) } if val == opt { // exact match means error was unrelated to the value - return err, false + return "", false } dist := levenshtein.Distance(val, opt, nil) if dist < mindist { @@ -40,15 +31,28 @@ func WrapErrorMaybe(err error, val string, options []string, caseSensitive bool) mindist = dist } } + return match, match != "" +} - if match == "" { - return err, false +// 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, - }, true + } } type suggestError struct { From 94ee93a6e68c25953ba0b68a3b7674d3b82e8055 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Mon, 13 May 2024 23:39:17 -0700 Subject: [PATCH 3/3] dockerfile: add hint suggestions to UndefinedVar Signed-off-by: Tonis Tiigi --- frontend/dockerfile/dockerfile2llb/convert.go | 24 +++++++----- frontend/dockerfile/dockerfile_lint_test.go | 37 +++++++++++++++++++ frontend/dockerfile/linter/ruleset.go | 10 +++-- 3 files changed, 59 insertions(+), 12 deletions(-) diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index fe3ba2ac0e4d..9bc90b55936e 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -766,7 +766,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 { @@ -782,7 +782,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 { @@ -2103,19 +2103,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) } } diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index 7343b8300c1e..9018f9f5e34e 100644 --- a/frontend/dockerfile/dockerfile_lint_test.go +++ b/frontend/dockerfile/dockerfile_lint_test.go @@ -582,6 +582,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) { diff --git a/frontend/dockerfile/linter/ruleset.go b/frontend/dockerfile/linter/ruleset.go index ba96fc744485..fd558d6eb748 100644 --- a/frontend/dockerfile/linter/ruleset.go +++ b/frontend/dockerfile/linter/ruleset.go @@ -95,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]{