From 87dd6f5706fabc93f0b89be1bb30ef0db08c93dd Mon Sep 17 00:00:00 2001 From: Talon Bowler Date: Tue, 9 Apr 2024 12:39:03 -0700 Subject: [PATCH] add linting rules for undeclared args in from Signed-off-by: Talon Bowler --- frontend/dockerfile/dockerfile2llb/convert.go | 25 ++++- frontend/dockerfile/dockerfile_lint_test.go | 98 +++++++++++++++---- frontend/dockerfile/dockerfile_test.go | 4 + frontend/dockerfile/linter/ruleset.go | 7 ++ frontend/dockerfile/shell/lex.go | 18 +++- frontend/dockerfile/shell/lex_test.go | 16 +-- 6 files changed, 136 insertions(+), 32 deletions(-) diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index cb537d4df0daa..9acbacbc7fade 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -226,7 +226,9 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS info := argInfo{definition: metaArg, location: cmd.Location()} if v, ok := opt.BuildArgs[metaArg.Key]; !ok { if metaArg.Value != nil { - *metaArg.Value, info.deps, _ = shlex.ProcessWordWithMatches(*metaArg.Value, metaArgsToMap(optMetaArgs)) + result, _ := shlex.ProcessWordWithMatches(*metaArg.Value, metaArgsToMap(optMetaArgs)) + *metaArg.Value = result.Word + info.deps = result.Matched } } else { metaArg.Value = &v @@ -248,7 +250,15 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS // set base state for every image for i, st := range stages { - name, used, err := shlex.ProcessWordWithMatches(st.BaseName, metaArgsToMap(optMetaArgs)) + result, err := shlex.ProcessWordWithMatches(st.BaseName, metaArgsToMap(optMetaArgs)) + name := result.Word + used := result.Matched + if len(result.Unmatched) > 0 { + for unmatched := range result.Unmatched { + msg := linter.RuleUndeclaredArgInFrom.Format(unmatched) + linter.RuleUndeclaredArgInFrom.Run(opt.Warn, st.Location, msg) + } + } if err != nil { return nil, parser.WithLocation(err, st.Location) } @@ -269,7 +279,16 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS } if v := st.Platform; v != "" { - v, u, err := shlex.ProcessWordWithMatches(v, metaArgsToMap(optMetaArgs)) + result, err := shlex.ProcessWordWithMatches(v, metaArgsToMap(optMetaArgs)) + v := result.Word + u := result.Matched + + if len(result.Unmatched) > 0 { + for unmatched := range result.Unmatched { + msg := linter.RuleUndeclaredArgInFrom.Format(unmatched) + linter.RuleUndeclaredArgInFrom.Run(opt.Warn, st.Location, msg) + } + } if err != nil { return nil, parser.WithLocation(errors.Wrapf(err, "failed to process arguments for platform %s", v), st.Location) } diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index 90e9dad48c1b1..08040fe85d324 100644 --- a/frontend/dockerfile/dockerfile_lint_test.go +++ b/frontend/dockerfile/dockerfile_lint_test.go @@ -26,6 +26,7 @@ var lintTests = integration.TestFuncs( testNoEmptyContinuations, testSelfConsistentCommandCasing, testFileConsistentCommandCasing, + testLintUndeclaredArg, ) func testStageName(t *testing.T, sb integration.Sandbox) { @@ -38,7 +39,7 @@ FROM scratch as base2 FROM scratch AS base3 `) - checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{ { RuleName: "StageNameCasing", Description: "Stage names should be lowercase", @@ -61,7 +62,7 @@ from scratch AS base from scratch as base2 `) - checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{ { RuleName: "FromAsCasing", Description: "The 'as' keyword should match the case of the 'from' keyword", @@ -83,7 +84,7 @@ COPY Dockerfile \ . `) - checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{ { RuleName: "NoEmptyContinuations", Description: "Empty continuation lines will become errors in a future release", @@ -101,7 +102,7 @@ func testSelfConsistentCommandCasing(t *testing.T, sb integration.Sandbox) { From scratch as base FROM scratch AS base2 `) - checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{ { RuleName: "SelfConsistentCommandCasing", Description: "Commands should be in consistent casing (all lower or all upper)", @@ -115,7 +116,7 @@ FROM scratch AS base2 frOM scratch as base from scratch as base2 `) - checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{ { RuleName: "SelfConsistentCommandCasing", Description: "Commands should be in consistent casing (all lower or all upper)", @@ -133,7 +134,7 @@ FROM scratch copy Dockerfile /foo COPY Dockerfile /bar `) - checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{ { RuleName: "FileConsistentCommandCasing", Description: "All commands within the Dockerfile should use the same casing (either upper or lower)", @@ -149,7 +150,7 @@ from scratch COPY Dockerfile /foo copy Dockerfile /bar `) - checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{ { RuleName: "FileConsistentCommandCasing", Description: "All commands within the Dockerfile should use the same casing (either upper or lower)", @@ -166,7 +167,7 @@ COPY Dockerfile /foo COPY Dockerfile /bar COPY Dockerfile /baz `) - checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{ { RuleName: "FileConsistentCommandCasing", Description: "All commands within the Dockerfile should use the same casing (either upper or lower)", @@ -183,7 +184,7 @@ copy Dockerfile /foo copy Dockerfile /bar copy Dockerfile /baz `) - checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{ { RuleName: "FileConsistentCommandCasing", Description: "All commands within the Dockerfile should use the same casing (either upper or lower)", @@ -198,14 +199,67 @@ from scratch copy Dockerfile /foo copy Dockerfile /bar `) - checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{}) + checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{}) dockerfile = []byte(` FROM scratch COPY Dockerfile /foo COPY Dockerfile /bar `) - checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{}) + checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{}) +} + +func testLintUndeclaredArg(t *testing.T, sb integration.Sandbox) { + dockerfile := []byte(` +ARG base=scratch +FROM $base +COPY Dockerfile . +`) + checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{}) + + dockerfile = []byte(` +# warning: undeclared ARG $base +FROM $base +COPY Dockerfile . +`) + checkLinterWarnings(t, sb, true, dockerfile, []expectedLintWarning{ + { + RuleName: "UndeclaredArgInFrom", + Description: "FROM command must use declared ARGs", + Detail: "FROM argument 'base' is not declared", + Level: 1, + Line: 3, + }, + }) + + dockerfile = []byte(` +ARG platform=linux/amd64 +FROM --platform=$platform scratch +COPY Dockerfile . +`) + checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{}) + + dockerfile = []byte(` +ARG platform=linux/amd64 +FROM --platform=$platform scratch +COPY Dockerfile . +`) + checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{}) + + dockerfile = []byte(` +# warning: undeclared ARG $platform +FROM --platform=$platform scratch +COPY Dockerfile . +`) + checkLinterWarnings(t, sb, true, dockerfile, []expectedLintWarning{ + { + RuleName: "UndeclaredArgInFrom", + Description: "FROM command must use declared ARGs", + Detail: "FROM argument 'platform' is not declared", + Level: 1, + Line: 3, + }, + }) } func checkUnmarshal(t *testing.T, sb integration.Sandbox, c *client.Client, dir *integration.TmpDirWithName, expected []expectedLintWarning) { @@ -237,7 +291,7 @@ func checkUnmarshal(t *testing.T, sb integration.Sandbox, c *client.Client, dir }) // Compare expectedLintWarning with actual lint results for i, w := range lintResults.Warnings { - checkLintWarning(t, w, expected[i]) + checkLinterWarning(t, w, expected[i]) } called = true return nil, nil @@ -253,7 +307,7 @@ func checkUnmarshal(t *testing.T, sb integration.Sandbox, c *client.Client, dir require.True(t, called) } -func checkProgressStream(t *testing.T, sb integration.Sandbox, c *client.Client, dir *integration.TmpDirWithName, expected []expectedLintWarning) { +func checkProgressStream(t *testing.T, sb integration.Sandbox, c *client.Client, dir *integration.TmpDirWithName, ignoreSolveErr bool, expected []expectedLintWarning) { status := make(chan *client.SolveStatus) statusDone := make(chan struct{}) done := make(chan struct{}) @@ -286,7 +340,9 @@ func checkProgressStream(t *testing.T, sb integration.Sandbox, c *client.Client, dockerui.DefaultLocalNameContext: dir, }, }, status) - require.NoError(t, err) + if !ignoreSolveErr { + require.NoError(t, err) + } select { case <-statusDone: @@ -311,7 +367,7 @@ func checkProgressStream(t *testing.T, sb integration.Sandbox, c *client.Client, } } -func checkLinterWarnings(t *testing.T, sb integration.Sandbox, dockerfile []byte, expected []expectedLintWarning) { +func checkLinterWarnings(t *testing.T, sb integration.Sandbox, ignoreSolveErr bool, dockerfile []byte, expected []expectedLintWarning) { // As a note, this test depends on the `expected` lint // warnings to be in order of appearance in the Dockerfile. @@ -327,11 +383,13 @@ func checkLinterWarnings(t *testing.T, sb integration.Sandbox, dockerfile []byte defer c.Close() t.Run("warntype=progress", func(t *testing.T) { - checkProgressStream(t, sb, c, dir, expected) - }) - t.Run("warntype=unmarshal", func(t *testing.T) { - checkUnmarshal(t, sb, c, dir, expected) + checkProgressStream(t, sb, c, dir, ignoreSolveErr, expected) }) + if !ignoreSolveErr { + t.Run("warntype=unmarshal", func(t *testing.T) { + checkUnmarshal(t, sb, c, dir, expected) + }) + } } func checkVertexWarning(t *testing.T, warning *client.VertexWarning, expected expectedLintWarning) { @@ -342,7 +400,7 @@ func checkVertexWarning(t *testing.T, warning *client.VertexWarning, expected ex require.Equal(t, expected.Level, warning.Level) } -func checkLintWarning(t *testing.T, warning lint.Warning, expected expectedLintWarning) { +func checkLinterWarning(t *testing.T, warning lint.Warning, expected expectedLintWarning) { short := linter.LintFormatShort(expected.RuleName, expected.Detail, expected.Line) require.Equal(t, expected.RuleName, warning.RuleName) require.Equal(t, expected.Description, warning.Description) diff --git a/frontend/dockerfile/dockerfile_test.go b/frontend/dockerfile/dockerfile_test.go index 2bf962c04c2eb..6f546b9222fc3 100644 --- a/frontend/dockerfile/dockerfile_test.go +++ b/frontend/dockerfile/dockerfile_test.go @@ -230,6 +230,10 @@ func init() { } } +func TestLint(t *testing.T) { + integration.Run(t, lintTests, opts...) +} + func TestIntegration(t *testing.T) { integration.Run(t, allTests, opts...) diff --git a/frontend/dockerfile/linter/ruleset.go b/frontend/dockerfile/linter/ruleset.go index 16cb719a82a50..d65119d7931de 100644 --- a/frontend/dockerfile/linter/ruleset.go +++ b/frontend/dockerfile/linter/ruleset.go @@ -41,4 +41,11 @@ var ( return fmt.Sprintf("Command '%s' should match the case of the command majority (%s)", violatingCommand, correctCasing) }, } + RuleUndeclaredArgInFrom = LinterRule[func(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) + }, + } ) diff --git a/frontend/dockerfile/shell/lex.go b/frontend/dockerfile/shell/lex.go index 7f6934a913566..91686caa0baca 100644 --- a/frontend/dockerfile/shell/lex.go +++ b/frontend/dockerfile/shell/lex.go @@ -57,12 +57,23 @@ func (s *Lex) ProcessWordWithMap(word string, env map[string]string) (string, er return word, err } + +type ProcessWordMatchesResult struct { + Word string + Matched map[string]struct{} + Unmatched map[string]struct{} +} + // ProcessWordWithMatches will use the 'env' list of environment variables, // replace any env var references in 'word' and return the env that were used. -func (s *Lex) ProcessWordWithMatches(word string, env map[string]string) (string, map[string]struct{}, error) { +func (s *Lex) ProcessWordWithMatches(word string, env map[string]string) (ProcessWordMatchesResult, error) { sw := s.init(word, env) word, _, err := sw.process(word) - return word, sw.matches, err + return ProcessWordMatchesResult{ + Word: word, + Matched: sw.matches, + Unmatched: sw.nonmatches, + }, err } func (s *Lex) ProcessWordsWithMap(word string, env map[string]string) ([]string, error) { @@ -79,6 +90,7 @@ func (s *Lex) init(word string, env map[string]string) *shellWord { rawQuotes: s.RawQuotes, rawEscapes: s.RawEscapes, matches: make(map[string]struct{}), + nonmatches: make(map[string]struct{}), } sw.scanner.Init(strings.NewReader(word)) return sw @@ -98,6 +110,7 @@ type shellWord struct { skipUnsetEnv bool skipProcessQuotes bool matches map[string]struct{} + nonmatches map[string]struct{} } func (sw *shellWord) process(source string) (string, []string, error) { @@ -511,6 +524,7 @@ func (sw *shellWord) getEnv(name string) (string, bool) { return value, true } } + sw.nonmatches[name] = struct{}{} return "", false } diff --git a/frontend/dockerfile/shell/lex_test.go b/frontend/dockerfile/shell/lex_test.go index 2dc01d8e0ecde..ead54b9466a49 100644 --- a/frontend/dockerfile/shell/lex_test.go +++ b/frontend/dockerfile/shell/lex_test.go @@ -480,7 +480,9 @@ func TestProcessWithMatches(t *testing.T) { for _, c := range tc { c := c t.Run(c.input, func(t *testing.T) { - w, matches, err := shlex.ProcessWordWithMatches(c.input, c.envs) + result, err := shlex.ProcessWordWithMatches(c.input, c.envs) + w := result.Word + matches := result.Matched if c.expectedErr { require.Error(t, err) return @@ -505,30 +507,30 @@ func TestProcessWithMatchesPlatform(t *testing.T) { version = "v1.2.3" ) - w, _, err := shlex.ProcessWordWithMatches(release, map[string]string{ + results, err := shlex.ProcessWordWithMatches(release, map[string]string{ "VERSION": version, "TARGETOS": "linux", "TARGETARCH": "arm", "TARGETVARIANT": "v7", }) require.NoError(t, err) - require.Equal(t, "something-v1.2.3.linux-arm-v7.tar.gz", w) + require.Equal(t, "something-v1.2.3.linux-arm-v7.tar.gz", results.Word) - w, _, err = shlex.ProcessWordWithMatches(release, map[string]string{ + results, err = shlex.ProcessWordWithMatches(release, map[string]string{ "VERSION": version, "TARGETOS": "linux", "TARGETARCH": "arm64", "TARGETVARIANT": "", }) require.NoError(t, err) - require.Equal(t, "something-v1.2.3.linux-arm64.tar.gz", w) + require.Equal(t, "something-v1.2.3.linux-arm64.tar.gz", results.Word) - w, _, err = shlex.ProcessWordWithMatches(release, map[string]string{ + results, err = shlex.ProcessWordWithMatches(release, map[string]string{ "VERSION": version, "TARGETOS": "linux", "TARGETARCH": "arm64", // No "TARGETVARIANT": "", }) require.NoError(t, err) - require.Equal(t, "something-v1.2.3.linux-arm64.tar.gz", w) + require.Equal(t, "something-v1.2.3.linux-arm64.tar.gz", results.Word) }