From 161fc508c6fe3195274554676e5317da4907e896 Mon Sep 17 00:00:00 2001 From: Talon Bowler Date: Wed, 2 Oct 2024 11:51:38 -0700 Subject: [PATCH] Refactor various rulecheck related code to properly handle env vars. after EnvGetter refactoring Signed-off-by: Talon Bowler --- frontend/dockerfile/dockerfile2llb/convert.go | 34 ++++++++++++++---- frontend/dockerfile/dockerfile_lint_test.go | 36 +++++++++++++++++++ 2 files changed, 63 insertions(+), 7 deletions(-) diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index bbcb9b224bd2..8cf7d69e64d1 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -265,7 +265,7 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS // Validate that base images continue to be valid even // when no build arguments are used. - validateBaseImagesWithDefaultArgs(stages, shlex, argCmds, globalArgs, lint) + validateBaseImagesWithDefaultArgs(stages, shlex, globalArgs, argCmds, lint) // Rebuild the arguments using the provided build arguments // for the remainder of the build. @@ -284,7 +284,8 @@ 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, globalArgs) - reportUnusedFromArgs(globalArgs, nameMatch.Unmatched, st.Location, lint) + argKeys := unusedFromArgsCheckKeys(globalArgs, outline.allArgs) + reportUnusedFromArgs(argKeys, nameMatch.Unmatched, st.Location, lint) used := nameMatch.Matched if used == nil { used = map[string]struct{}{} @@ -311,7 +312,8 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS if v := st.Platform; v != "" { platMatch, err := shlex.ProcessWordWithMatches(v, globalArgs) - reportUnusedFromArgs(globalArgs, platMatch.Unmatched, st.Location, lint) + argKeys := unusedFromArgsCheckKeys(globalArgs, outline.allArgs) + reportUnusedFromArgs(argKeys, platMatch.Unmatched, st.Location, lint) reportRedundantTargetPlatform(st.Platform, platMatch, st.Location, globalArgs, lint) reportConstPlatformDisallowed(st.Name, platMatch, st.Location, lint) @@ -2332,9 +2334,27 @@ func toPBLocation(sourceIndex int, location []parser.Range) pb.Location { } } -func reportUnusedFromArgs(args shell.EnvGetter, unmatched map[string]struct{}, location []parser.Range, lint *linter.Linter) { +func unusedFromArgsCheckKeys(env shell.EnvGetter, args map[string]argInfo) map[string]struct{} { + matched := make(map[string]struct{}) + for _, arg := range args { + matched[arg.definition.Key] = struct{}{} + } + for _, k := range env.Keys() { + matched[k] = struct{}{} + } + return matched +} + +func reportUnusedFromArgs(testArgKeys map[string]struct{}, unmatched map[string]struct{}, location []parser.Range, lint *linter.Linter) { + var argKeys []string + for arg := range testArgKeys { + argKeys = append(argKeys, arg) + } for arg := range unmatched { - suggest, _ := suggest.Search(arg, args.Keys(), true) + if _, ok := testArgKeys[arg]; ok { + continue + } + suggest, _ := suggest.Search(arg, argKeys, true) msg := linter.RuleUndefinedArgInFrom.Format(arg, suggest) lint.Run(&linter.RuleUndefinedArgInFrom, location, msg) } @@ -2456,10 +2476,10 @@ func validateNoSecretKey(instruction, key string, location []parser.Range, lint } } -func validateBaseImagesWithDefaultArgs(stages []instructions.Stage, shlex *shell.Lex, argsCmds []instructions.ArgCommand, args *llb.EnvList, lint *linter.Linter) { +func validateBaseImagesWithDefaultArgs(stages []instructions.Stage, shlex *shell.Lex, env *llb.EnvList, argCmds []instructions.ArgCommand, lint *linter.Linter) { // Build the arguments as if no build options were given // and using only defaults. - args, _, err := buildMetaArgs(args, shlex, argsCmds, nil) + args, _, err := buildMetaArgs(env, shlex, argCmds, nil) if err != nil { // Abandon running the linter. We'll likely fail after this point // with the same error but we shouldn't error here inside diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index 5be29b922715..de926d5f8619 100644 --- a/frontend/dockerfile/dockerfile_lint_test.go +++ b/frontend/dockerfile/dockerfile_lint_test.go @@ -805,6 +805,27 @@ COPY Dockerfile . `) checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) + dockerfile = []byte(` +ARG DEBUG +FROM scratch${DEBUG} +COPY Dockerfile . +`) + checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) + + dockerfile = []byte(` +ARG DEBUG +FROM scra${DEBUG:-tch} +COPY Dockerfile . +`) + checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) + + dockerfile = []byte(` +ARG DEBUG="" +FROM scratch${DEBUG-@bogus} +COPY Dockerfile . +`) + checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) + dockerfile = []byte(` FROM --platform=$BULIDPLATFORM scratch COPY Dockerfile . @@ -1456,6 +1477,9 @@ func checkProgressStream(t *testing.T, sb integration.Sandbox, lintTest *lintTes }, }, status) if lintTest.StreamBuildErr == "" && lintTest.StreamBuildErrRegexp == nil { + if err != nil { + t.Logf("expected no error, received: %v", err) + } require.NoError(t, err) } else { if lintTest.StreamBuildErr != "" { @@ -1471,6 +1495,18 @@ func checkProgressStream(t *testing.T, sb integration.Sandbox, lintTest *lintTes t.Fatalf("timed out waiting for statusDone") } + if len(lintTest.Warnings) != len(warnings) { + t.Logf("expected %d warnings, received:", len(lintTest.Warnings)) + t.Logf("\texpected:") + for i, w := range lintTest.Warnings { + t.Logf("\t\t%d: %s", i, w.Detail) + } + + t.Logf("\treceived:") + for i, w := range warnings { + t.Logf("\t%d: %s", i, w.Short) + } + } require.Equal(t, len(lintTest.Warnings), len(warnings)) sort.Slice(warnings, func(i, j int) bool { w1 := warnings[i]