Skip to content

Commit 1eb2a03

Browse files
tonistiigitonistiigi
authored
Merge pull request moby#4925 from tonistiigi/dockerfile-platform-validation
dockerfile: improve error messages and add suggest to flag parsing
2 parents 265c38c + 94ee93a commit 1eb2a03

File tree

4 files changed

+143
-32
lines changed

4 files changed

+143
-32
lines changed

‎frontend/dockerfile/dockerfile2llb/convert.go

Lines changed: 49 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,7 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
261261
// set base state for every image
262262
for i, st := range stages {
263263
nameMatch, err := shlex.ProcessWordWithMatches(st.BaseName, metaArgsToMap(optMetaArgs))
264-
reportUnusedFromArgs(nameMatch.Unmatched, st.Location, opt.Warn)
264+
reportUnusedFromArgs(metaArgsKeys(optMetaArgs), nameMatch.Unmatched, st.Location, opt.Warn)
265265
used := nameMatch.Matched
266266

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

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

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

294+
if platMatch.Result == "" {
295+
err := errors.Errorf("empty platform value from expression %s", v)
296+
err = parser.WithLocation(err, st.Location)
297+
err = wrapSuggestAny(err, platMatch.Unmatched, metaArgsKeys(optMetaArgs))
298+
return nil, err
299+
}
300+
294301
p, err := platforms.Parse(platMatch.Result)
295302
if err != nil {
296-
return nil, parser.WithLocation(errors.Wrapf(err, "failed to parse platform %s", platMatch.Result), st.Location)
303+
err = parser.WithLocation(err, st.Location)
304+
err = wrapSuggestAny(err, platMatch.Unmatched, metaArgsKeys(optMetaArgs))
305+
return nil, parser.WithLocation(errors.Wrapf(err, "failed to parse platform %s", v), st.Location)
297306
}
298307

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

700+
func metaArgsKeys(metaArgs []instructions.KeyValuePairOptional) []string {
701+
s := make([]string, 0, len(metaArgs))
702+
for _, arg := range metaArgs {
703+
s = append(s, arg.Key)
704+
}
705+
return s
706+
}
707+
691708
func toCommand(ic instructions.Command, allDispatchStates *dispatchStates) (command, error) {
692709
cmd := command{Command: ic}
693710
if c, ok := ic.(*instructions.CopyCommand); ok {
@@ -750,7 +767,7 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error {
750767
}
751768

752769
newword, unmatched, err := opt.shlex.ProcessWord(word, env)
753-
reportUnmatchedVariables(cmd, d.buildArgs, unmatched, &opt)
770+
reportUnmatchedVariables(cmd, d.buildArgs, env, unmatched, &opt)
754771
return newword, err
755772
})
756773
if err != nil {
@@ -766,7 +783,7 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error {
766783
lex := shell.NewLex('\\')
767784
lex.SkipProcessQuotes = true
768785
newword, unmatched, err := lex.ProcessWord(word, env)
769-
reportUnmatchedVariables(cmd, d.buildArgs, unmatched, &opt)
786+
reportUnmatchedVariables(cmd, d.buildArgs, env, unmatched, &opt)
770787
return newword, err
771788
})
772789
if err != nil {
@@ -2077,19 +2094,25 @@ func validateStageNames(stages []instructions.Stage, warn linter.LintWarnFunc) {
20772094
}
20782095
}
20792096

2080-
func reportUnmatchedVariables(cmd instructions.Command, buildArgs []instructions.KeyValuePairOptional, unmatched map[string]struct{}, opt *dispatchOpt) {
2097+
func reportUnmatchedVariables(cmd instructions.Command, buildArgs []instructions.KeyValuePairOptional, env []string, unmatched map[string]struct{}, opt *dispatchOpt) {
2098+
for _, buildArg := range buildArgs {
2099+
delete(unmatched, buildArg.Key)
2100+
}
20812101
if len(unmatched) == 0 {
20822102
return
20832103
}
2084-
for _, buildArg := range buildArgs {
2085-
delete(unmatched, buildArg.Key)
2104+
options := metaArgsKeys(opt.metaArgs)
2105+
for _, envVar := range env {
2106+
key, _ := parseKeyValue(envVar)
2107+
options = append(options, key)
20862108
}
20872109
for cmdVar := range unmatched {
2088-
_, nonEnvOk := nonEnvArgs[cmdVar]
2089-
if !nonEnvOk {
2090-
msg := linter.RuleUndefinedVar.Format(cmdVar)
2091-
linter.RuleUndefinedVar.Run(opt.lintWarn, cmd.Location(), msg)
2110+
if _, nonEnvOk := nonEnvArgs[cmdVar]; nonEnvOk {
2111+
continue
20922112
}
2113+
match, _ := suggest.Search(cmdVar, options, true)
2114+
msg := linter.RuleUndefinedVar.Format(cmdVar, match)
2115+
linter.RuleUndefinedVar.Run(opt.lintWarn, cmd.Location(), msg)
20932116
}
20942117
}
20952118

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

2146-
func reportUnusedFromArgs(unmatched map[string]struct{}, location []parser.Range, warn linter.LintWarnFunc) {
2169+
func reportUnusedFromArgs(values []string, unmatched map[string]struct{}, location []parser.Range, warn linter.LintWarnFunc) {
21472170
for arg := range unmatched {
2148-
msg := linter.RuleUndeclaredArgInFrom.Format(arg)
2171+
suggest, _ := suggest.Search(arg, values, true)
2172+
msg := linter.RuleUndeclaredArgInFrom.Format(arg, suggest)
21492173
linter.RuleUndeclaredArgInFrom.Run(warn, location, msg)
21502174
}
21512175
}
@@ -2169,3 +2193,14 @@ func validateUsedOnce(c instructions.Command, loc *instructionTracker, warn lint
21692193
}
21702194
loc.MarkUsed(c.Location())
21712195
}
2196+
2197+
func wrapSuggestAny(err error, keys map[string]struct{}, options []string) error {
2198+
for k := range keys {
2199+
var ok bool
2200+
ok, err = suggest.WrapErrorMaybe(err, k, options, true)
2201+
if ok {
2202+
break
2203+
}
2204+
}
2205+
return err
2206+
}

‎frontend/dockerfile/dockerfile_lint_test.go

Lines changed: 62 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -463,16 +463,38 @@ COPY Dockerfile .
463463
{
464464
RuleName: "UndeclaredArgInFrom",
465465
Description: "FROM command must use declared ARGs",
466-
Detail: "FROM argument 'BULIDPLATFORM' is not declared",
466+
Detail: "FROM argument 'BULIDPLATFORM' is not declared (did you mean BUILDPLATFORM?)",
467467
Level: 1,
468468
Line: 2,
469469
},
470470
},
471-
StreamBuildErr: "failed to solve: failed to parse platform : \"\" is an invalid component of \"\": platform specifier component must match \"^[A-Za-z0-9_-]+$\": invalid argument",
472-
UnmarshalBuildErr: "failed to parse platform : \"\" is an invalid component of \"\": platform specifier component must match \"^[A-Za-z0-9_-]+$\": invalid argument",
471+
StreamBuildErr: "failed to solve: empty platform value from expression $BULIDPLATFORM (did you mean BUILDPLATFORM?)",
472+
UnmarshalBuildErr: "empty platform value from expression $BULIDPLATFORM (did you mean BUILDPLATFORM?)",
473473
BuildErrLocation: 2,
474474
})
475475

476+
dockerfile = []byte(`
477+
ARG MY_OS=linux
478+
ARG MY_ARCH=amd64
479+
FROM --platform=linux/${MYARCH} busybox
480+
COPY Dockerfile .
481+
`)
482+
checkLinterWarnings(t, sb, &lintTestParams{
483+
Dockerfile: dockerfile,
484+
Warnings: []expectedLintWarning{
485+
{
486+
RuleName: "UndeclaredArgInFrom",
487+
Description: "FROM command must use declared ARGs",
488+
Detail: "FROM argument 'MYARCH' is not declared (did you mean MY_ARCH?)",
489+
Level: 1,
490+
Line: 4,
491+
},
492+
},
493+
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?)",
494+
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?)",
495+
BuildErrLocation: 4,
496+
})
497+
476498
dockerfile = []byte(`
477499
ARG tag=latest
478500
FROM busybox:${tag}${version} AS b
@@ -561,6 +583,43 @@ RUN echo $foo
561583
},
562584
},
563585
})
586+
587+
dockerfile = []byte(`
588+
FROM alpine
589+
ARG DIR_BINARIES=binaries/
590+
ARG DIR_ASSETS=assets/
591+
ARG DIR_CONFIG=config/
592+
COPY $DIR_ASSET .
593+
`)
594+
checkLinterWarnings(t, sb, &lintTestParams{
595+
Dockerfile: dockerfile,
596+
Warnings: []expectedLintWarning{
597+
{
598+
RuleName: "UndefinedVar",
599+
Description: "Variables should be defined before their use",
600+
Detail: "Usage of undefined variable '$DIR_ASSET' (did you mean $DIR_ASSETS?)",
601+
Level: 1,
602+
Line: 6,
603+
},
604+
},
605+
})
606+
607+
dockerfile = []byte(`
608+
FROM alpine
609+
ENV PATH=$PAHT:/tmp/bin
610+
`)
611+
checkLinterWarnings(t, sb, &lintTestParams{
612+
Dockerfile: dockerfile,
613+
Warnings: []expectedLintWarning{
614+
{
615+
RuleName: "UndefinedVar",
616+
Description: "Variables should be defined before their use",
617+
Detail: "Usage of undefined variable '$PAHT' (did you mean $PATH?)",
618+
Level: 1,
619+
Line: 3,
620+
},
621+
},
622+
})
564623
}
565624

566625
func testMultipleInstructionsDisallowed(t *testing.T, sb integration.Sandbox) {

‎frontend/dockerfile/linter/ruleset.go

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -70,11 +70,15 @@ var (
7070
return "Maintainer instruction is deprecated in favor of using label"
7171
},
7272
}
73-
RuleUndeclaredArgInFrom = LinterRule[func(string) string]{
73+
RuleUndeclaredArgInFrom = LinterRule[func(string, string) string]{
7474
Name: "UndeclaredArgInFrom",
7575
Description: "FROM command must use declared ARGs",
76-
Format: func(baseArg string) string {
77-
return fmt.Sprintf("FROM argument '%s' is not declared", baseArg)
76+
Format: func(baseArg, suggest string) string {
77+
out := fmt.Sprintf("FROM argument '%s' is not declared", baseArg)
78+
if suggest != "" {
79+
out += fmt.Sprintf(" (did you mean %s?)", suggest)
80+
}
81+
return out
7882
},
7983
}
8084
RuleWorkdirRelativePath = LinterRule[func(workdir string) string]{
@@ -91,11 +95,15 @@ var (
9195
return fmt.Sprintf("Usage of undefined variable '$%s'", arg)
9296
},
9397
}
94-
RuleUndefinedVar = LinterRule[func(string) string]{
98+
RuleUndefinedVar = LinterRule[func(string, string) string]{
9599
Name: "UndefinedVar",
96100
Description: "Variables should be defined before their use",
97-
Format: func(arg string) string {
98-
return fmt.Sprintf("Usage of undefined variable '$%s'", arg)
101+
Format: func(arg, suggest string) string {
102+
out := fmt.Sprintf("Usage of undefined variable '$%s'", arg)
103+
if suggest != "" {
104+
out += fmt.Sprintf(" (did you mean $%s?)", suggest)
105+
}
106+
return out
99107
},
100108
}
101109
RuleMultipleInstructionsDisallowed = LinterRule[func(instructionName string) string]{

‎util/suggest/error.go

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,7 @@ import (
66
"github.com/agext/levenshtein"
77
)
88

9-
// WrapError wraps error with a suggestion for fixing it
10-
func WrapError(err error, val string, options []string, caseSensitive bool) error {
11-
if err == nil {
12-
return nil
13-
}
9+
func Search(val string, options []string, caseSensitive bool) (string, bool) {
1410
orig := val
1511
if !caseSensitive {
1612
val = strings.ToLower(val)
@@ -23,7 +19,7 @@ func WrapError(err error, val string, options []string, caseSensitive bool) erro
2319
}
2420
if val == opt {
2521
// exact match means error was unrelated to the value
26-
return err
22+
return "", false
2723
}
2824
dist := levenshtein.Distance(val, opt, nil)
2925
if dist < mindist {
@@ -35,12 +31,25 @@ func WrapError(err error, val string, options []string, caseSensitive bool) erro
3531
mindist = dist
3632
}
3733
}
34+
return match, match != ""
35+
}
3836

39-
if match == "" {
40-
return err
37+
// WrapError wraps error with a suggestion for fixing it
38+
func WrapError(err error, val string, options []string, caseSensitive bool) error {
39+
_, err = WrapErrorMaybe(err, val, options, caseSensitive)
40+
return err
41+
}
42+
43+
func WrapErrorMaybe(err error, val string, options []string, caseSensitive bool) (bool, error) {
44+
if err == nil {
45+
return false, nil
46+
}
47+
match, ok := Search(val, options, caseSensitive)
48+
if match == "" || !ok {
49+
return false, err
4150
}
4251

43-
return &suggestError{
52+
return true, &suggestError{
4453
err: err,
4554
match: match,
4655
}

0 commit comments

Comments
 (0)