From 72d7aa7e4b083983f2831880fc39ee3fbd8de241 Mon Sep 17 00:00:00 2001 From: Talon Bowler Date: Tue, 9 Apr 2024 14:23:29 -0700 Subject: [PATCH 1/3] update print to include lint subcommand Signed-off-by: Talon Bowler --- frontend/dockerfile/builder/build.go | 5 + frontend/dockerfile/dockerfile2llb/convert.go | 29 ++ frontend/dockerfile/dockerfile_lint_test.go | 381 ++++++++++++++++++ frontend/dockerfile/dockerfile_test.go | 237 +---------- frontend/dockerui/requests.go | 14 + frontend/subrequests/lint/lint.go | 93 +++++ 6 files changed, 523 insertions(+), 236 deletions(-) create mode 100644 frontend/dockerfile/dockerfile_lint_test.go create mode 100644 frontend/subrequests/lint/lint.go diff --git a/frontend/dockerfile/builder/build.go b/frontend/dockerfile/builder/build.go index 6c6fdc48a7c8..6e32c65feb09 100644 --- a/frontend/dockerfile/builder/build.go +++ b/frontend/dockerfile/builder/build.go @@ -15,6 +15,7 @@ import ( "github.com/moby/buildkit/frontend/dockerui" "github.com/moby/buildkit/frontend/gateway/client" gwpb "github.com/moby/buildkit/frontend/gateway/pb" + "github.com/moby/buildkit/frontend/subrequests/lint" "github.com/moby/buildkit/frontend/subrequests/outline" "github.com/moby/buildkit/frontend/subrequests/targets" "github.com/moby/buildkit/solver/errdefs" @@ -85,6 +86,10 @@ func Build(ctx context.Context, c client.Client) (_ *client.Result, err error) { ListTargets: func(ctx context.Context) (*targets.List, error) { return dockerfile2llb.ListTargets(ctx, src.Data) }, + Lint: func(ctx context.Context) (*lint.LintResults, error) { + warnings, err := dockerfile2llb.DockerfileLint(ctx, src.Data, convertOpt) + return &lint.LintResults{Warnings: warnings}, err + }, }); err != nil { return nil, err } else if ok { diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index ea5e8601536b..08e6b2009d35 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -26,6 +26,7 @@ import ( "github.com/moby/buildkit/frontend/dockerfile/parser" "github.com/moby/buildkit/frontend/dockerfile/shell" "github.com/moby/buildkit/frontend/dockerui" + "github.com/moby/buildkit/frontend/subrequests/lint" "github.com/moby/buildkit/frontend/subrequests/outline" "github.com/moby/buildkit/frontend/subrequests/targets" "github.com/moby/buildkit/identity" @@ -110,6 +111,34 @@ func Dockefile2Outline(ctx context.Context, dt []byte, opt ConvertOpt) (*outline return &o, nil } +func DockerfileLint(ctx context.Context, dt []byte, opt ConvertOpt) ([]lint.Warning, error) { + warnings := []lint.Warning{} + sourceData := strings.Split(string(opt.SourceMap.Data), "\n") + opt.Warn = func(short, url string, detail [][]byte, location []parser.Range) { + var rulename, ruledetail string + if strings.HasPrefix(short, "Lint Rule ") { + rulename = strings.TrimPrefix(short, "Lint Rule ") + ruleParts := strings.Split(rulename, ":") + rulename = strings.Trim(ruleParts[0], "'") + ruledetail = strings.TrimSpace(ruleParts[1]) + } else { + return + } + warnings = append(warnings, lint.Warning{ + RuleName: rulename, + Detail: ruledetail, + Filename: opt.SourceMap.Filename, + Source: sourceData[location[0].Start.Line-1 : location[0].End.Line], + StartLine: location[0].Start.Line, + }) + } + _, err := toDispatchState(ctx, dt, opt) + if err != nil { + return nil, err + } + return warnings, nil +} + func ListTargets(ctx context.Context, dt []byte) (*targets.List, error) { dockerfile, err := parser.Parse(bytes.NewReader(dt)) if err != nil { diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go new file mode 100644 index 000000000000..6d8ac6ee8a77 --- /dev/null +++ b/frontend/dockerfile/dockerfile_lint_test.go @@ -0,0 +1,381 @@ +package dockerfile + +import ( + "context" + "encoding/json" + "os" + "sort" + "testing" + "time" + + "github.com/containerd/continuity/fs/fstest" + "github.com/moby/buildkit/client" + "github.com/moby/buildkit/frontend/dockerui" + gateway "github.com/moby/buildkit/frontend/gateway/client" + + "github.com/moby/buildkit/frontend/subrequests/lint" + "github.com/moby/buildkit/util/testutil/integration" + "github.com/moby/buildkit/util/testutil/workers" + "github.com/pkg/errors" + "github.com/stretchr/testify/require" + "github.com/tonistiigi/fsutil" +) + +var lintTests = integration.TestFuncs( + testLintRequest, + testLintStageName, + testLintNoEmptyContinuations, +) + +func testLintRequest(t *testing.T, sb integration.Sandbox) { + integration.SkipOnPlatform(t, "windows") + workers.CheckFeatureCompat(t, sb, workers.FeatureFrontendOutline) + f := getFrontend(t, sb) + if _, ok := f.(*clientFrontend); !ok { + t.Skip("only test with client frontend") + } + + dockerfile := []byte(` + FROM scratch as target + COPY Dockerfile \ + + . + ARG inherited=box + copy Dockerfile /foo + + FROM scratch AS target2 + COPY Dockerfile /Dockerfile + `) + + dir := integration.Tmpdir( + t, + fstest.CreateFile("Dockerfile", []byte(dockerfile), 0600), + ) + + c, err := client.New(sb.Context(), sb.Address()) + require.NoError(t, err) + defer c.Close() + + destDir, err := os.MkdirTemp("", "buildkit") + require.NoError(t, err) + defer os.RemoveAll(destDir) + + called := false + frontend := func(ctx context.Context, c gateway.Client) (*gateway.Result, error) { + res, err := c.Solve(ctx, gateway.SolveRequest{ + FrontendOpt: map[string]string{ + "frontend.caps": "moby.buildkit.frontend.subrequests", + "requestid": "frontend.lint", + "build-arg:BAR": "678", + "target": "target", + }, + Frontend: "dockerfile.v0", + }) + require.NoError(t, err) + + lintResults, err := unmarshalLintResults(res) + require.NoError(t, err) + + require.Equal(t, 3, len(lintResults.Warnings)) + sort.Slice(lintResults.Warnings, func(i, j int) bool { + // sort by line number in ascending order + return lintResults.Warnings[i].StartLine < lintResults.Warnings[j].StartLine + }) + checkLintRequestWarnings(t, lintResults.Warnings, []lint.Warning{ + { + RuleName: "FromAsCasing", + Detail: "'as' and 'FROM' keywords' casing do not match (line 2)", + Filename: "Dockerfile", + StartLine: 2, + Source: []string{"\tFROM scratch as target"}, + }, + { + RuleName: "NoEmptyContinuations", + Detail: "Empty continuation line (line 5)", + Filename: "Dockerfile", + StartLine: 5, + Source: []string{"\t."}, + }, + { + RuleName: "FileConsistentCommandCasing", + Detail: "Command 'copy' should match the case of the command majority (uppercase) (line 7)", + Filename: "Dockerfile", + StartLine: 7, + Source: []string{"\tcopy Dockerfile /foo"}, + }, + }) + called = true + return nil, nil + } + + _, err = c.Build(sb.Context(), client.SolveOpt{ + LocalMounts: map[string]fsutil.FS{ + dockerui.DefaultLocalNameDockerfile: dir, + }, + }, "", frontend, nil) + require.NoError(t, err) + + require.True(t, called) +} + +func testLintStageName(t *testing.T, sb integration.Sandbox) { + dockerfile := []byte(` +# warning: stage name should be lowercase +FROM scratch AS BadStageName + +# warning: 'as' should match 'FROM' cmd casing. +FROM scratch as base2 + +FROM scratch AS base3 +`) + checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + { + Short: "Lint Rule 'StageNameCasing': Stage name 'BadStageName' should be lowercase (line 3)", + Detail: "Stage names should be lowercase", + Level: 1, + }, + { + Short: "Lint Rule 'FromAsCasing': 'as' and 'FROM' keywords' casing do not match (line 6)", + Detail: "The 'as' keyword should match the case of the 'from' keyword", + Level: 1, + }, + }) + + dockerfile = []byte(` +# warning: 'AS' should match 'from' cmd casing. +from scratch AS base + +from scratch as base2 +`) + checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + { + Short: "Lint Rule 'FromAsCasing': 'AS' and 'from' keywords' casing do not match (line 3)", + Detail: "The 'as' keyword should match the case of the 'from' keyword", + Level: 1, + }, + }) +} + +func testLintNoEmptyContinuations(t *testing.T, sb integration.Sandbox) { + dockerfile := []byte(` +FROM scratch +# warning: empty continuation line +COPY Dockerfile \ + +. +COPY Dockerfile \ +. +`) + + checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + { + Short: "Lint Rule 'NoEmptyContinuations': Empty continuation line (line 6)", + Detail: "Empty continuation lines will become errors in a future release", + URL: "https://github.com/moby/moby/pull/33719", + Level: 1, + }, + }) +} + +func testSelfConsistentCommandCasing(t *testing.T, sb integration.Sandbox) { + dockerfile := []byte(` +# warning: 'FROM' should be either lowercased or uppercased +From scratch as base +FROM scratch AS base2 +`) + checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + { + Short: "Lint Rule 'SelfConsistentCommandCasing': Command 'From' should be consistently cased (line 3)", + Detail: "Commands should be in consistent casing (all lower or all upper)", + Level: 1, + }, + }) + dockerfile = []byte(` +# warning: 'FROM' should be either lowercased or uppercased +frOM scratch as base +from scratch as base2 +`) + checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + { + Short: "Lint Rule 'SelfConsistentCommandCasing': Command 'frOM' should be consistently cased (line 3)", + Detail: "Commands should be in consistent casing (all lower or all upper)", + Level: 1, + }, + }) +} + +func testFileConsistentCommandCasing(t *testing.T, sb integration.Sandbox) { + dockerfile := []byte(` +FROM scratch +# warning: 'copy' should match command majority's casing (uppercase) +copy Dockerfile /foo +COPY Dockerfile /bar +`) + checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + { + Short: "Lint Rule 'FileConsistentCommandCasing': Command 'copy' should match the case of the command majority (uppercase) (line 4)", + Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", + Level: 1, + }, + }) + + dockerfile = []byte(` +from scratch +# warning: 'COPY' should match command majority's casing (lowercase) +COPY Dockerfile /foo +copy Dockerfile /bar +`) + checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + { + Short: "Lint Rule 'FileConsistentCommandCasing': Command 'COPY' should match the case of the command majority (lowercase) (line 4)", + Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", + Level: 1, + }, + }) + + dockerfile = []byte(` +# warning: 'from' should match command majority's casing (uppercase) +from scratch +COPY Dockerfile /foo +COPY Dockerfile /bar +COPY Dockerfile /baz +`) + checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + { + Short: "Lint Rule 'FileConsistentCommandCasing': Command 'from' should match the case of the command majority (uppercase) (line 3)", + Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", + Level: 1, + }, + }) + + dockerfile = []byte(` +# warning: 'FROM' should match command majority's casing (lowercase) +FROM scratch +copy Dockerfile /foo +copy Dockerfile /bar +copy Dockerfile /baz +`) + checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + { + Short: "Lint Rule 'FileConsistentCommandCasing': Command 'FROM' should match the case of the command majority (lowercase) (line 3)", + Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", + Level: 1, + }, + }) + + dockerfile = []byte(` +from scratch +copy Dockerfile /foo +copy Dockerfile /bar +`) + checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{}) + + dockerfile = []byte(` +FROM scratch +COPY Dockerfile /foo +COPY Dockerfile /bar +`) + checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{}) +} + +func checkLinterWarnings(t *testing.T, sb integration.Sandbox, dockerfile []byte, expected []expectedLintWarning) { + // As a note, this test depends on the `expected` lint + // warnings to be in order of appearance in the Dockerfile. + + integration.SkipOnPlatform(t, "windows") + f := getFrontend(t, sb) + + dir := integration.Tmpdir( + t, + fstest.CreateFile("Dockerfile", dockerfile, 0600), + ) + + c, err := client.New(sb.Context(), sb.Address()) + require.NoError(t, err) + defer c.Close() + + status := make(chan *client.SolveStatus) + statusDone := make(chan struct{}) + done := make(chan struct{}) + + var warnings []*client.VertexWarning + + go func() { + defer close(statusDone) + for { + select { + case st, ok := <-status: + if !ok { + return + } + warnings = append(warnings, st.Warnings...) + case <-done: + return + } + } + }() + + _, err = f.Solve(sb.Context(), c, client.SolveOpt{ + FrontendAttrs: map[string]string{ + "platform": "linux/amd64,linux/arm64", + }, + LocalMounts: map[string]fsutil.FS{ + dockerui.DefaultLocalNameDockerfile: dir, + dockerui.DefaultLocalNameContext: dir, + }, + }, status) + require.NoError(t, err) + + select { + case <-statusDone: + case <-time.After(10 * time.Second): + t.Fatalf("timed out waiting for statusDone") + } + + // two platforms only show one warning + require.Equal(t, len(expected), len(warnings)) + sort.Slice(warnings, func(i, j int) bool { + w1 := warnings[i] + w2 := warnings[j] + if len(w1.Range) == 0 { + return true + } else if len(w2.Range) == 0 { + return false + } + return w1.Range[0].Start.Line < w2.Range[0].Start.Line + }) + for i, w := range warnings { + require.Equal(t, expected[i].Short, string(w.Short)) + require.Equal(t, expected[i].Detail, string(w.Detail[0])) + require.Equal(t, expected[i].URL, w.URL) + require.Equal(t, expected[i].Level, w.Level) + } +} + +func checkLintRequestWarnings(t *testing.T, actual, expected []lint.Warning) { + require.Equal(t, len(expected), len(actual)) + + for i, expected := range expected { + actual := actual[i] + require.Equal(t, expected.RuleName, actual.RuleName) + require.Equal(t, expected.Detail, actual.Detail) + require.Equal(t, expected.Filename, actual.Filename) + require.Equal(t, expected.StartLine, actual.StartLine) + require.Equal(t, len(expected.Source), len(actual.Source)) + for j, expectedSource := range expected.Source { + require.Equal(t, expectedSource, actual.Source[j]) + } + } +} + +func unmarshalLintResults(res *gateway.Result) (*lint.LintResults, error) { + dt, ok := res.Metadata["result.json"] + if !ok { + return nil, errors.Errorf("missing frontend.outline") + } + var l lint.LintResults + if err := json.Unmarshal(dt, &l); err != nil { + return nil, err + } + return &l, nil +} diff --git a/frontend/dockerfile/dockerfile_test.go b/frontend/dockerfile/dockerfile_test.go index fe448ed30b0d..53d3eae46cd0 100644 --- a/frontend/dockerfile/dockerfile_test.go +++ b/frontend/dockerfile/dockerfile_test.go @@ -175,8 +175,6 @@ var allTests = integration.TestFuncs( testDuplicatePlatformProvenance, testDockerIgnoreMissingProvenance, testSBOMScannerArgs, - testLintStageName, - testLintNoEmptyContinuations, testSelfConsistentCommandCasing, testFileConsistentCommandCasing, testNilContextInSolveGateway, @@ -247,6 +245,7 @@ func TestIntegration(t *testing.T) { "granted": networkHostGranted, "denied": networkHostDenied, }))...) + integration.Run(t, lintTests, opts...) integration.Run(t, heredocTests, opts...) integration.Run(t, outlineTests, opts...) integration.Run(t, targetsTests, opts...) @@ -6767,166 +6766,6 @@ ARG BUILDKIT_SBOM_SCAN_STAGE=true require.Equal(t, 1, len(att.LayersRaw)) } -func testLintStageName(t *testing.T, sb integration.Sandbox) { - dockerfile := []byte(` -# warning: stage name should be lowercase -FROM scratch AS BadStageName - -# warning: 'as' should match 'FROM' cmd casing. -FROM scratch as base2 - -FROM scratch AS base3 -`) - checkLintWarnings(t, sb, dockerfile, []expectedLintWarning{ - { - Short: "Lint Rule 'StageNameCasing': Stage name 'BadStageName' should be lowercase (line 3)", - Detail: "Stage names should be lowercase", - Level: 1, - }, - { - Short: "Lint Rule 'FromAsCasing': 'as' and 'FROM' keywords' casing do not match (line 6)", - Detail: "The 'as' keyword should match the case of the 'from' keyword", - Level: 1, - }, - }) - - dockerfile = []byte(` -# warning: 'AS' should match 'from' cmd casing. -from scratch AS base - -from scratch as base2 -`) - checkLintWarnings(t, sb, dockerfile, []expectedLintWarning{ - { - Short: "Lint Rule 'FromAsCasing': 'AS' and 'from' keywords' casing do not match (line 3)", - Detail: "The 'as' keyword should match the case of the 'from' keyword", - Level: 1, - }, - }) -} - -func testLintNoEmptyContinuations(t *testing.T, sb integration.Sandbox) { - dockerfile := []byte(` -FROM scratch -# warning: empty continuation line -COPY Dockerfile \ - -. -COPY Dockerfile \ -. -`) - - checkLintWarnings(t, sb, dockerfile, []expectedLintWarning{ - { - Short: "Lint Rule 'NoEmptyContinuations': Empty continuation line (line 6)", - Detail: "Empty continuation lines will become errors in a future release", - URL: "https://github.com/moby/moby/pull/33719", - Level: 1, - }, - }) -} - -func testSelfConsistentCommandCasing(t *testing.T, sb integration.Sandbox) { - dockerfile := []byte(` -# warning: 'FROM' should be either lowercased or uppercased -From scratch as base -FROM scratch AS base2 -`) - checkLintWarnings(t, sb, dockerfile, []expectedLintWarning{ - { - Short: "Lint Rule 'SelfConsistentCommandCasing': Command 'From' should be consistently cased (line 3)", - Detail: "Commands should be in consistent casing (all lower or all upper)", - Level: 1, - }, - }) - dockerfile = []byte(` -# warning: 'FROM' should be either lowercased or uppercased -frOM scratch as base -from scratch as base2 -`) - checkLintWarnings(t, sb, dockerfile, []expectedLintWarning{ - { - Short: "Lint Rule 'SelfConsistentCommandCasing': Command 'frOM' should be consistently cased (line 3)", - Detail: "Commands should be in consistent casing (all lower or all upper)", - Level: 1, - }, - }) -} - -func testFileConsistentCommandCasing(t *testing.T, sb integration.Sandbox) { - dockerfile := []byte(` -FROM scratch -# warning: 'copy' should match command majority's casing (uppercase) -copy Dockerfile /foo -COPY Dockerfile /bar -`) - checkLintWarnings(t, sb, dockerfile, []expectedLintWarning{ - { - Short: "Lint Rule 'FileConsistentCommandCasing': Command 'copy' should match the case of the command majority (uppercase) (line 4)", - Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", - Level: 1, - }, - }) - - dockerfile = []byte(` -from scratch -# warning: 'COPY' should match command majority's casing (lowercase) -COPY Dockerfile /foo -copy Dockerfile /bar -`) - checkLintWarnings(t, sb, dockerfile, []expectedLintWarning{ - { - Short: "Lint Rule 'FileConsistentCommandCasing': Command 'COPY' should match the case of the command majority (lowercase) (line 4)", - Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", - Level: 1, - }, - }) - - dockerfile = []byte(` -# warning: 'from' should match command majority's casing (uppercase) -from scratch -COPY Dockerfile /foo -COPY Dockerfile /bar -COPY Dockerfile /baz -`) - checkLintWarnings(t, sb, dockerfile, []expectedLintWarning{ - { - Short: "Lint Rule 'FileConsistentCommandCasing': Command 'from' should match the case of the command majority (uppercase) (line 3)", - Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", - Level: 1, - }, - }) - - dockerfile = []byte(` -# warning: 'FROM' should match command majority's casing (lowercase) -FROM scratch -copy Dockerfile /foo -copy Dockerfile /bar -copy Dockerfile /baz -`) - checkLintWarnings(t, sb, dockerfile, []expectedLintWarning{ - { - Short: "Lint Rule 'FileConsistentCommandCasing': Command 'FROM' should match the case of the command majority (lowercase) (line 3)", - Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", - Level: 1, - }, - }) - - dockerfile = []byte(` -from scratch -copy Dockerfile /foo -copy Dockerfile /bar -`) - checkLintWarnings(t, sb, dockerfile, []expectedLintWarning{}) - - dockerfile = []byte(` -FROM scratch -COPY Dockerfile /foo -COPY Dockerfile /bar -`) - checkLintWarnings(t, sb, dockerfile, []expectedLintWarning{}) -} - func testReproSourceDateEpoch(t *testing.T, sb integration.Sandbox) { integration.SkipOnPlatform(t, "windows") workers.CheckFeatureCompat(t, sb, workers.FeatureOCIExporter, workers.FeatureSourceDateEpoch) @@ -7083,80 +6922,6 @@ COPY --link foo foo } } -func checkLintWarnings(t *testing.T, sb integration.Sandbox, dockerfile []byte, expected []expectedLintWarning) { - // As a note, this test depends on the `expected` lint - // warnings to be in order of appearance in the Dockerfile. - - integration.SkipOnPlatform(t, "windows") - f := getFrontend(t, sb) - - dir := integration.Tmpdir( - t, - fstest.CreateFile("Dockerfile", dockerfile, 0600), - ) - - c, err := client.New(sb.Context(), sb.Address()) - require.NoError(t, err) - defer c.Close() - - status := make(chan *client.SolveStatus) - statusDone := make(chan struct{}) - done := make(chan struct{}) - - var warnings []*client.VertexWarning - - go func() { - defer close(statusDone) - for { - select { - case st, ok := <-status: - if !ok { - return - } - warnings = append(warnings, st.Warnings...) - case <-done: - return - } - } - }() - - _, err = f.Solve(sb.Context(), c, client.SolveOpt{ - FrontendAttrs: map[string]string{ - "platform": "linux/amd64,linux/arm64", - }, - LocalMounts: map[string]fsutil.FS{ - dockerui.DefaultLocalNameDockerfile: dir, - dockerui.DefaultLocalNameContext: dir, - }, - }, status) - require.NoError(t, err) - - select { - case <-statusDone: - case <-time.After(10 * time.Second): - t.Fatalf("timed out waiting for statusDone") - } - - // two platforms only show one warning - require.Equal(t, len(expected), len(warnings)) - sort.Slice(warnings, func(i, j int) bool { - w1 := warnings[i] - w2 := warnings[j] - if len(w1.Range) == 0 { - return true - } else if len(w2.Range) == 0 { - return false - } - return w1.Range[0].Start.Line < w2.Range[0].Start.Line - }) - for i, w := range warnings { - require.Equal(t, expected[i].Short, string(w.Short)) - require.Equal(t, expected[i].Detail, string(w.Detail[0])) - require.Equal(t, expected[i].URL, w.URL) - require.Equal(t, expected[i].Level, w.Level) - } -} - func timeMustParse(t *testing.T, layout, value string) time.Time { tm, err := time.Parse(layout, value) require.NoError(t, err) diff --git a/frontend/dockerui/requests.go b/frontend/dockerui/requests.go index 7900a0c7a5c5..0cf069aa6483 100644 --- a/frontend/dockerui/requests.go +++ b/frontend/dockerui/requests.go @@ -7,6 +7,7 @@ import ( "github.com/moby/buildkit/frontend/gateway/client" "github.com/moby/buildkit/frontend/subrequests" + "github.com/moby/buildkit/frontend/subrequests/lint" "github.com/moby/buildkit/frontend/subrequests/outline" "github.com/moby/buildkit/frontend/subrequests/targets" "github.com/moby/buildkit/solver/errdefs" @@ -19,6 +20,7 @@ const ( type RequestHandler struct { Outline func(context.Context) (*outline.Outline, error) ListTargets func(context.Context) (*targets.List, error) + Lint func(context.Context) (*lint.LintResults, error) AllowOther bool } @@ -55,6 +57,18 @@ func (bc *Client) HandleSubrequest(ctx context.Context, h RequestHandler) (*clie res, err := targets.ToResult() return res, true, err } + case lint.SubrequestLintDefinition.Name: + if f := h.Lint; f != nil { + warnings, err := f(ctx) + if err != nil { + return nil, false, err + } + if warnings == nil { + return nil, true, nil + } + res, err := warnings.ToResult() + return res, true, err + } } if h.AllowOther { return nil, false, nil diff --git a/frontend/subrequests/lint/lint.go b/frontend/subrequests/lint/lint.go new file mode 100644 index 000000000000..8ef6892740b6 --- /dev/null +++ b/frontend/subrequests/lint/lint.go @@ -0,0 +1,93 @@ +package lint + +import ( + "bytes" + "encoding/json" + "fmt" + "io" + "sort" + "text/tabwriter" + + "github.com/moby/buildkit/frontend/gateway/client" + "github.com/moby/buildkit/frontend/subrequests" +) + +const RequestLint = "frontend.lint" + +var SubrequestLintDefinition = subrequests.Request{ + Name: RequestLint, + Version: "1.0.0", + Type: subrequests.TypeRPC, + Description: "Lint a Dockerfile", + Opts: []subrequests.Named{}, + Metadata: []subrequests.Named{ + {Name: "result.json"}, + {Name: "result.txt"}, + }, +} + +type Warning struct { + RuleName string `json:"rule_name"` + Detail string `json:"detail"` + Filename string `json:"filename"` + Source []string `json:"source"` + StartLine int `json:"start_line"` +} + +type LintResults struct { + Warnings []Warning `json:"warnings"` +} + +func (warns LintResults) ToResult() (*client.Result, error) { + res := client.NewResult() + dt, err := json.MarshalIndent(warns, "", " ") + if err != nil { + return nil, err + } + res.AddMeta("result.json", dt) + + b := bytes.NewBuffer(nil) + if err := PrintLintViolations(dt, b); err != nil { + return nil, err + } + res.AddMeta("result.txt", b.Bytes()) + + res.AddMeta("version", []byte(SubrequestLintDefinition.Version)) + return res, nil +} + +func PrintLintViolations(dt []byte, w io.Writer) error { + var warnings LintResults + + if err := json.Unmarshal(dt, &warnings); err != nil { + return err + } + + // Here, we're grouping the warnings by rule name + lintWarnings := make(map[string][]Warning) + lintWarningRules := []string{} + for _, warning := range warnings.Warnings { + if _, ok := lintWarnings[warning.RuleName]; !ok { + lintWarningRules = append(lintWarningRules, warning.RuleName) + lintWarnings[warning.RuleName] = []Warning{} + } + lintWarnings[warning.RuleName] = append(lintWarnings[warning.RuleName], warning) + } + sort.Strings(lintWarningRules) + + tw := tabwriter.NewWriter(w, 0, 0, 2, ' ', 0) + for _, rule := range lintWarningRules { + fmt.Fprintf(tw, "Lint Rule %s\n", rule) + for _, warning := range lintWarnings[rule] { + fmt.Fprintf(tw, "\t%s:%d\n", warning.Filename, warning.StartLine) + fmt.Fprintf(tw, "\t%s\n", warning.Detail) + for offset, source := range warning.Source { + fmt.Fprintf(tw, "\t\t%d\t|\t%s\n", warning.StartLine+offset, source) + } + fmt.Fprintln(tw) + } + fmt.Fprintln(tw) + } + + return tw.Flush() +} From 81fc6a355deee405f43a38cc84a8d9198d492d56 Mon Sep 17 00:00:00 2001 From: Talon Bowler Date: Tue, 9 Apr 2024 22:35:09 -0700 Subject: [PATCH 2/3] Consolidate tests and update Warning output to include source data Signed-off-by: Talon Bowler --- frontend/dockerfile/builder/build.go | 3 +- frontend/dockerfile/dockerfile2llb/convert.go | 39 ++- frontend/dockerfile/dockerfile_lint_test.go | 290 ++++++++---------- frontend/dockerfile/dockerfile_test.go | 7 - frontend/subrequests/lint/lint.go | 76 ++++- 5 files changed, 228 insertions(+), 187 deletions(-) diff --git a/frontend/dockerfile/builder/build.go b/frontend/dockerfile/builder/build.go index 6e32c65feb09..a49b97ae4ca6 100644 --- a/frontend/dockerfile/builder/build.go +++ b/frontend/dockerfile/builder/build.go @@ -87,8 +87,7 @@ func Build(ctx context.Context, c client.Client) (_ *client.Result, err error) { return dockerfile2llb.ListTargets(ctx, src.Data) }, Lint: func(ctx context.Context) (*lint.LintResults, error) { - warnings, err := dockerfile2llb.DockerfileLint(ctx, src.Data, convertOpt) - return &lint.LintResults{Warnings: warnings}, err + return dockerfile2llb.DockerfileLint(ctx, src.Data, convertOpt) }, }); err != nil { return nil, err diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 08e6b2009d35..63c4158931a8 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -111,32 +111,39 @@ func Dockefile2Outline(ctx context.Context, dt []byte, opt ConvertOpt) (*outline return &o, nil } -func DockerfileLint(ctx context.Context, dt []byte, opt ConvertOpt) ([]lint.Warning, error) { - warnings := []lint.Warning{} - sourceData := strings.Split(string(opt.SourceMap.Data), "\n") +func DockerfileLint(ctx context.Context, dt []byte, opt ConvertOpt) (*lint.LintResults, error) { + results := &lint.LintResults{} opt.Warn = func(short, url string, detail [][]byte, location []parser.Range) { - var rulename, ruledetail string + var ruleName, ruleDetail string if strings.HasPrefix(short, "Lint Rule ") { - rulename = strings.TrimPrefix(short, "Lint Rule ") - ruleParts := strings.Split(rulename, ":") - rulename = strings.Trim(ruleParts[0], "'") - ruledetail = strings.TrimSpace(ruleParts[1]) + ruleName = strings.TrimPrefix(short, "Lint Rule ") + ruleParts := strings.Split(ruleName, ":") + ruleName = strings.Trim(ruleParts[0], "'") + ruleDetail = strings.TrimSpace(ruleParts[1]) } else { return } - warnings = append(warnings, lint.Warning{ - RuleName: rulename, - Detail: ruledetail, - Filename: opt.SourceMap.Filename, - Source: sourceData[location[0].Start.Line-1 : location[0].End.Line], - StartLine: location[0].Start.Line, - }) + sourceIndex := results.AddSource(opt.SourceMap.Filename, "Dockerfile", opt.SourceMap.Data) + sourceLocation := []lint.Range{} + for _, loc := range location { + sourceLocation = append(sourceLocation, lint.Range{ + Start: lint.Position{ + Line: loc.Start.Line, + Column: loc.Start.Character, + }, + End: lint.Position{ + Line: loc.End.Line, + Column: loc.End.Character, + }, + }) + } + results.AddWarning(ruleName, ruleDetail, sourceIndex, sourceLocation) } _, err := toDispatchState(ctx, dt, opt) if err != nil { return nil, err } - return warnings, nil + return results, nil } func ListTargets(ctx context.Context, dt []byte) (*targets.List, error) { diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index 6d8ac6ee8a77..6c89b7785561 100644 --- a/frontend/dockerfile/dockerfile_lint_test.go +++ b/frontend/dockerfile/dockerfile_lint_test.go @@ -3,6 +3,7 @@ package dockerfile import ( "context" "encoding/json" + "fmt" "os" "sort" "testing" @@ -15,109 +16,16 @@ import ( "github.com/moby/buildkit/frontend/subrequests/lint" "github.com/moby/buildkit/util/testutil/integration" - "github.com/moby/buildkit/util/testutil/workers" "github.com/pkg/errors" "github.com/stretchr/testify/require" "github.com/tonistiigi/fsutil" ) var lintTests = integration.TestFuncs( - testLintRequest, testLintStageName, testLintNoEmptyContinuations, ) -func testLintRequest(t *testing.T, sb integration.Sandbox) { - integration.SkipOnPlatform(t, "windows") - workers.CheckFeatureCompat(t, sb, workers.FeatureFrontendOutline) - f := getFrontend(t, sb) - if _, ok := f.(*clientFrontend); !ok { - t.Skip("only test with client frontend") - } - - dockerfile := []byte(` - FROM scratch as target - COPY Dockerfile \ - - . - ARG inherited=box - copy Dockerfile /foo - - FROM scratch AS target2 - COPY Dockerfile /Dockerfile - `) - - dir := integration.Tmpdir( - t, - fstest.CreateFile("Dockerfile", []byte(dockerfile), 0600), - ) - - c, err := client.New(sb.Context(), sb.Address()) - require.NoError(t, err) - defer c.Close() - - destDir, err := os.MkdirTemp("", "buildkit") - require.NoError(t, err) - defer os.RemoveAll(destDir) - - called := false - frontend := func(ctx context.Context, c gateway.Client) (*gateway.Result, error) { - res, err := c.Solve(ctx, gateway.SolveRequest{ - FrontendOpt: map[string]string{ - "frontend.caps": "moby.buildkit.frontend.subrequests", - "requestid": "frontend.lint", - "build-arg:BAR": "678", - "target": "target", - }, - Frontend: "dockerfile.v0", - }) - require.NoError(t, err) - - lintResults, err := unmarshalLintResults(res) - require.NoError(t, err) - - require.Equal(t, 3, len(lintResults.Warnings)) - sort.Slice(lintResults.Warnings, func(i, j int) bool { - // sort by line number in ascending order - return lintResults.Warnings[i].StartLine < lintResults.Warnings[j].StartLine - }) - checkLintRequestWarnings(t, lintResults.Warnings, []lint.Warning{ - { - RuleName: "FromAsCasing", - Detail: "'as' and 'FROM' keywords' casing do not match (line 2)", - Filename: "Dockerfile", - StartLine: 2, - Source: []string{"\tFROM scratch as target"}, - }, - { - RuleName: "NoEmptyContinuations", - Detail: "Empty continuation line (line 5)", - Filename: "Dockerfile", - StartLine: 5, - Source: []string{"\t."}, - }, - { - RuleName: "FileConsistentCommandCasing", - Detail: "Command 'copy' should match the case of the command majority (uppercase) (line 7)", - Filename: "Dockerfile", - StartLine: 7, - Source: []string{"\tcopy Dockerfile /foo"}, - }, - }) - called = true - return nil, nil - } - - _, err = c.Build(sb.Context(), client.SolveOpt{ - LocalMounts: map[string]fsutil.FS{ - dockerui.DefaultLocalNameDockerfile: dir, - }, - }, "", frontend, nil) - require.NoError(t, err) - - require.True(t, called) -} - func testLintStageName(t *testing.T, sb integration.Sandbox) { dockerfile := []byte(` # warning: stage name should be lowercase @@ -130,14 +38,18 @@ FROM scratch AS base3 `) checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { - Short: "Lint Rule 'StageNameCasing': Stage name 'BadStageName' should be lowercase (line 3)", - Detail: "Stage names should be lowercase", - Level: 1, + RuleName: "StageNameCasing", + Description: "Stage name 'BadStageName' should be lowercase", + Line: 3, + Detail: "Stage names should be lowercase", + Level: 1, }, { - Short: "Lint Rule 'FromAsCasing': 'as' and 'FROM' keywords' casing do not match (line 6)", - Detail: "The 'as' keyword should match the case of the 'from' keyword", - Level: 1, + RuleName: "FromAsCasing", + Description: "'as' and 'FROM' keywords' casing do not match", + Line: 6, + Detail: "The 'as' keyword should match the case of the 'from' keyword", + Level: 1, }, }) @@ -149,9 +61,11 @@ from scratch as base2 `) checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { - Short: "Lint Rule 'FromAsCasing': 'AS' and 'from' keywords' casing do not match (line 3)", - Detail: "The 'as' keyword should match the case of the 'from' keyword", - Level: 1, + RuleName: "FromAsCasing", + Description: "'AS' and 'from' keywords' casing do not match", + Line: 3, + Detail: "The 'as' keyword should match the case of the 'from' keyword", + Level: 1, }, }) } @@ -169,10 +83,12 @@ COPY Dockerfile \ checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { - Short: "Lint Rule 'NoEmptyContinuations': Empty continuation line (line 6)", - Detail: "Empty continuation lines will become errors in a future release", - URL: "https://github.com/moby/moby/pull/33719", - Level: 1, + RuleName: "NoEmptyContinuations", + Description: "Empty continuation line", + Line: 6, + Detail: "Empty continuation lines will become errors in a future release", + URL: "https://github.com/moby/moby/pull/33719", + Level: 1, }, }) } @@ -185,9 +101,11 @@ FROM scratch AS base2 `) checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { - Short: "Lint Rule 'SelfConsistentCommandCasing': Command 'From' should be consistently cased (line 3)", - Detail: "Commands should be in consistent casing (all lower or all upper)", - Level: 1, + RuleName: "SelfConsistentCommandCasing", + Description: "Command 'From' should be consistently cased", + Line: 3, + Detail: "Commands should be in consistent casing (all lower or all upper)", + Level: 1, }, }) dockerfile = []byte(` @@ -197,9 +115,11 @@ from scratch as base2 `) checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { - Short: "Lint Rule 'SelfConsistentCommandCasing': Command 'frOM' should be consistently cased (line 3)", - Detail: "Commands should be in consistent casing (all lower or all upper)", - Level: 1, + RuleName: "SelfConsistentCommandCasing", + Description: "Command 'frOM' should be consistently cased", + Line: 3, + Detail: "Commands should be in consistent casing (all lower or all upper)", + Level: 1, }, }) } @@ -213,9 +133,11 @@ COPY Dockerfile /bar `) checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { - Short: "Lint Rule 'FileConsistentCommandCasing': Command 'copy' should match the case of the command majority (uppercase) (line 4)", - Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", - Level: 1, + RuleName: "FileConsistentCommandCasing", + Description: "Command 'copy' should match the case of the command majority (uppercase)", + Line: 4, + Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", + Level: 1, }, }) @@ -227,9 +149,11 @@ copy Dockerfile /bar `) checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { - Short: "Lint Rule 'FileConsistentCommandCasing': Command 'COPY' should match the case of the command majority (lowercase) (line 4)", - Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", - Level: 1, + RuleName: "FileConsistentCommandCasing", + Description: "Command 'COPY' should match the case of the command majority (lowercase)", + Line: 4, + Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", + Level: 1, }, }) @@ -242,9 +166,11 @@ COPY Dockerfile /baz `) checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { - Short: "Lint Rule 'FileConsistentCommandCasing': Command 'from' should match the case of the command majority (uppercase) (line 3)", - Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", - Level: 1, + RuleName: "FileConsistentCommandCasing", + Description: "Command 'from' should match the case of the command majority (uppercase)", + Line: 3, + Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", + Level: 1, }, }) @@ -257,9 +183,11 @@ copy Dockerfile /baz `) checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { - Short: "Lint Rule 'FileConsistentCommandCasing': Command 'FROM' should match the case of the command majority (lowercase) (line 3)", - Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", - Level: 1, + RuleName: "FileConsistentCommandCasing", + Description: "Command 'FROM' should match the case of the command majority (lowercase)", + Line: 3, + Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", + Level: 1, }, }) @@ -278,22 +206,52 @@ COPY Dockerfile /bar checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{}) } -func checkLinterWarnings(t *testing.T, sb integration.Sandbox, dockerfile []byte, expected []expectedLintWarning) { - // As a note, this test depends on the `expected` lint - // warnings to be in order of appearance in the Dockerfile. +func checkUnmarshal(t *testing.T, sb integration.Sandbox, c *client.Client, f frontend, dir *integration.TmpDirWithName, dockerfile []byte, expected []expectedLintWarning) { + destDir, err := os.MkdirTemp("", "buildkit") + require.NoError(t, err) + defer os.RemoveAll(destDir) - integration.SkipOnPlatform(t, "windows") - f := getFrontend(t, sb) + called := false + frontend := func(ctx context.Context, c gateway.Client) (*gateway.Result, error) { + res, err := c.Solve(ctx, gateway.SolveRequest{ + FrontendOpt: map[string]string{ + "frontend.caps": "moby.buildkit.frontend.subrequests", + "requestid": "frontend.lint", + "build-arg:BAR": "678", + }, + Frontend: "dockerfile.v0", + }) + require.NoError(t, err) - dir := integration.Tmpdir( - t, - fstest.CreateFile("Dockerfile", dockerfile, 0600), - ) + lintResults, err := unmarshalLintResults(res) + require.NoError(t, err) - c, err := client.New(sb.Context(), sb.Address()) + require.Equal(t, len(expected), len(lintResults.Warnings)) + sort.Slice(lintResults.Warnings, func(i, j int) bool { + // sort by line number in ascending order + firstRange := lintResults.Warnings[i].Location[0] + secondRange := lintResults.Warnings[j].Location[0] + return firstRange.Start.Line < secondRange.Start.Line + }) + // Compare expectedLintWarning with actual lint results + for i, w := range lintResults.Warnings { + checkLintWarning(t, w, expected[i]) + } + called = true + return nil, nil + } + + _, err = c.Build(sb.Context(), client.SolveOpt{ + LocalMounts: map[string]fsutil.FS{ + dockerui.DefaultLocalNameDockerfile: dir, + }, + }, "", frontend, nil) require.NoError(t, err) - defer c.Close() + require.True(t, called) +} + +func checkProgressStream(t *testing.T, sb integration.Sandbox, c *client.Client, f frontend, dir *integration.TmpDirWithName, dockerfile []byte, expected []expectedLintWarning) { status := make(chan *client.SolveStatus) statusDone := make(chan struct{}) done := make(chan struct{}) @@ -315,7 +273,7 @@ func checkLinterWarnings(t *testing.T, sb integration.Sandbox, dockerfile []byte } }() - _, err = f.Solve(sb.Context(), c, client.SolveOpt{ + _, err := f.Solve(sb.Context(), c, client.SolveOpt{ FrontendAttrs: map[string]string{ "platform": "linux/amd64,linux/arm64", }, @@ -345,27 +303,42 @@ func checkLinterWarnings(t *testing.T, sb integration.Sandbox, dockerfile []byte return w1.Range[0].Start.Line < w2.Range[0].Start.Line }) for i, w := range warnings { - require.Equal(t, expected[i].Short, string(w.Short)) - require.Equal(t, expected[i].Detail, string(w.Detail[0])) - require.Equal(t, expected[i].URL, w.URL) - require.Equal(t, expected[i].Level, w.Level) + checkVertexWarning(t, w, expected[i]) } } -func checkLintRequestWarnings(t *testing.T, actual, expected []lint.Warning) { - require.Equal(t, len(expected), len(actual)) - - for i, expected := range expected { - actual := actual[i] - require.Equal(t, expected.RuleName, actual.RuleName) - require.Equal(t, expected.Detail, actual.Detail) - require.Equal(t, expected.Filename, actual.Filename) - require.Equal(t, expected.StartLine, actual.StartLine) - require.Equal(t, len(expected.Source), len(actual.Source)) - for j, expectedSource := range expected.Source { - require.Equal(t, expectedSource, actual.Source[j]) - } - } +func checkLinterWarnings(t *testing.T, sb integration.Sandbox, dockerfile []byte, expected []expectedLintWarning) { + // As a note, this test depends on the `expected` lint + // warnings to be in order of appearance in the Dockerfile. + + integration.SkipOnPlatform(t, "windows") + f := getFrontend(t, sb) + + dir := integration.Tmpdir( + t, + fstest.CreateFile("Dockerfile", dockerfile, 0600), + ) + + c, err := client.New(sb.Context(), sb.Address()) + require.NoError(t, err) + defer c.Close() + + checkProgressStream(t, sb, c, f, dir, dockerfile, expected) + checkUnmarshal(t, sb, c, f, dir, dockerfile, expected) +} + +func checkVertexWarning(t *testing.T, warning *client.VertexWarning, expected expectedLintWarning) { + short := fmt.Sprintf("Lint Rule '%s': %s (line %d)", expected.RuleName, expected.Description, expected.Line) + require.Equal(t, short, string(warning.Short)) + require.Equal(t, expected.Detail, string(warning.Detail[0])) + require.Equal(t, expected.URL, warning.URL) + require.Equal(t, expected.Level, warning.Level) +} + +func checkLintWarning(t *testing.T, warning lint.Warning, expected expectedLintWarning) { + require.Equal(t, expected.RuleName, warning.RuleName) + detail := fmt.Sprintf("%s (line %d)", expected.Description, expected.Line) + require.Equal(t, detail, warning.Detail) } func unmarshalLintResults(res *gateway.Result) (*lint.LintResults, error) { @@ -379,3 +352,12 @@ func unmarshalLintResults(res *gateway.Result) (*lint.LintResults, error) { } return &l, nil } + +type expectedLintWarning struct { + RuleName string + Description string + Line int + Detail string + URL string + Level int +} diff --git a/frontend/dockerfile/dockerfile_test.go b/frontend/dockerfile/dockerfile_test.go index 53d3eae46cd0..98485715f95d 100644 --- a/frontend/dockerfile/dockerfile_test.go +++ b/frontend/dockerfile/dockerfile_test.go @@ -7300,13 +7300,6 @@ type nopWriteCloser struct { func (nopWriteCloser) Close() error { return nil } -type expectedLintWarning struct { - Short string - Detail string - URL string - Level int -} - type secModeSandbox struct{} func (*secModeSandbox) UpdateConfigFile(in string) string { diff --git a/frontend/subrequests/lint/lint.go b/frontend/subrequests/lint/lint.go index 8ef6892740b6..d03e40656c1f 100644 --- a/frontend/subrequests/lint/lint.go +++ b/frontend/subrequests/lint/lint.go @@ -26,16 +26,67 @@ var SubrequestLintDefinition = subrequests.Request{ }, } +type Source struct { + Filename string `json:"filename"` + Language string `json:"language"` + Data []byte `json:"data"` +} + +type Range struct { + Start Position `json:"start"` + End Position `json:"end"` +} + +type Position struct { + Line int `json:"line"` + Column int `json:"column"` +} + type Warning struct { - RuleName string `json:"rule_name"` - Detail string `json:"detail"` - Filename string `json:"filename"` - Source []string `json:"source"` - StartLine int `json:"start_line"` + RuleName string `json:"rule_name"` + Detail string `json:"detail"` + SourceIndex int `json:"source_index"` + Location []Range `json:"location"` } type LintResults struct { Warnings []Warning `json:"warnings"` + Sources []Source `json:"sources"` +} + +func (results *LintResults) AddSource(filename string, language string, data []byte) int { + sourceE := Source{ + Filename: filename, + Language: language, + Data: data, + } + for i, source := range results.Sources { + if sourceEqual(source, sourceE) { + return i + } + } + results.Sources = append(results.Sources, Source{ + Filename: filename, + Language: language, + Data: data, + }) + return len(results.Sources) - 1 +} + +func (results *LintResults) AddWarning(ruleName, detail string, sourceIndex int, location []Range) { + results.Warnings = append(results.Warnings, Warning{ + RuleName: ruleName, + Detail: detail, + SourceIndex: sourceIndex, + Location: location, + }) +} + +func sourceEqual(a, b Source) bool { + if a.Filename != b.Filename || a.Language != b.Language { + return false + } + return bytes.Equal(a.Data, b.Data) } func (warns LintResults) ToResult() (*client.Result, error) { @@ -79,10 +130,19 @@ func PrintLintViolations(dt []byte, w io.Writer) error { for _, rule := range lintWarningRules { fmt.Fprintf(tw, "Lint Rule %s\n", rule) for _, warning := range lintWarnings[rule] { - fmt.Fprintf(tw, "\t%s:%d\n", warning.Filename, warning.StartLine) + source := warnings.Sources[warning.SourceIndex] + sourceData := bytes.Split(source.Data, []byte("\n")) + firstRange := warning.Location[0] + if firstRange.Start.Line != firstRange.End.Line { + fmt.Fprintf(tw, "\t%s:%d-%d\n", source.Filename, firstRange.Start.Line, firstRange.End.Line) + } else { + fmt.Fprintf(tw, "\t%s:%d\n", source.Filename, firstRange.Start.Line) + } fmt.Fprintf(tw, "\t%s\n", warning.Detail) - for offset, source := range warning.Source { - fmt.Fprintf(tw, "\t\t%d\t|\t%s\n", warning.StartLine+offset, source) + for _, r := range warning.Location { + for i := r.Start.Line; i <= r.End.Line; i++ { + fmt.Fprintf(tw, "\t%d\t|\t%s\n", i, sourceData[i-1]) + } } fmt.Fprintln(tw) } From e81c6c67c531e3b9c3559a848a3527b20b7b7406 Mon Sep 17 00:00:00 2001 From: Talon Bowler Date: Wed, 10 Apr 2024 08:46:35 -0700 Subject: [PATCH 3/3] refactored opt.Warn to simplify and updated warning data Signed-off-by: Talon Bowler --- frontend/dockerfile/builder/build.go | 4 +- frontend/dockerfile/dockerfile2llb/convert.go | 34 ++------ frontend/dockerfile/dockerfile_lint_test.go | 80 +++++++++-------- frontend/dockerfile/dockerfile_test.go | 2 - frontend/dockerfile/linter/linter.go | 11 ++- frontend/subrequests/lint/lint.go | 86 +++++++++++-------- 6 files changed, 107 insertions(+), 110 deletions(-) diff --git a/frontend/dockerfile/builder/build.go b/frontend/dockerfile/builder/build.go index a49b97ae4ca6..09a36ebd08ae 100644 --- a/frontend/dockerfile/builder/build.go +++ b/frontend/dockerfile/builder/build.go @@ -74,8 +74,8 @@ func Build(ctx context.Context, c client.Client) (_ *client.Result, err error) { Client: bc, SourceMap: src.SourceMap, MetaResolver: c, - Warn: func(msg, url string, detail [][]byte, location []parser.Range) { - src.Warn(ctx, msg, warnOpts(location, detail, url)) + Warn: func(rulename, description, url, msg string, location []parser.Range) { + src.Warn(ctx, msg, warnOpts(location, [][]byte{[]byte(description)}, url)) }, } diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 63c4158931a8..4975f26f113e 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -64,7 +64,7 @@ type ConvertOpt struct { TargetPlatform *ocispecs.Platform MetaResolver llb.ImageMetaResolver LLBCaps *apicaps.CapSet - Warn func(short, url string, detail [][]byte, location []parser.Range) + Warn linter.LintWarnFunc } type SBOMTargets struct { @@ -113,31 +113,9 @@ func Dockefile2Outline(ctx context.Context, dt []byte, opt ConvertOpt) (*outline func DockerfileLint(ctx context.Context, dt []byte, opt ConvertOpt) (*lint.LintResults, error) { results := &lint.LintResults{} - opt.Warn = func(short, url string, detail [][]byte, location []parser.Range) { - var ruleName, ruleDetail string - if strings.HasPrefix(short, "Lint Rule ") { - ruleName = strings.TrimPrefix(short, "Lint Rule ") - ruleParts := strings.Split(ruleName, ":") - ruleName = strings.Trim(ruleParts[0], "'") - ruleDetail = strings.TrimSpace(ruleParts[1]) - } else { - return - } - sourceIndex := results.AddSource(opt.SourceMap.Filename, "Dockerfile", opt.SourceMap.Data) - sourceLocation := []lint.Range{} - for _, loc := range location { - sourceLocation = append(sourceLocation, lint.Range{ - Start: lint.Position{ - Line: loc.Start.Line, - Column: loc.Start.Character, - }, - End: lint.Position{ - Line: loc.End.Line, - Column: loc.End.Character, - }, - }) - } - results.AddWarning(ruleName, ruleDetail, sourceIndex, sourceLocation) + sourceIndex := results.AddSource(opt.SourceMap) + opt.Warn = func(rulename, description, url, fmtmsg string, location []parser.Range) { + results.AddWarning(rulename, description, url, fmtmsg, sourceIndex, location) } _, err := toDispatchState(ctx, dt, opt) if err != nil { @@ -198,7 +176,7 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS } if opt.Warn == nil { - opt.Warn = func(string, string, [][]byte, []parser.Range) {} + opt.Warn = func(string, string, string, string, []parser.Range) {} } if opt.Client != nil && opt.LLBCaps == nil { @@ -1982,7 +1960,7 @@ func isSelfConsistentCasing(s string) bool { return s == strings.ToLower(s) || s == strings.ToUpper(s) } -func validateCommandCasing(dockerfile *parser.Result, warn func(short, url string, detail [][]byte, location []parser.Range)) { +func validateCommandCasing(dockerfile *parser.Result, warn linter.LintWarnFunc) { var lowerCount, upperCount int for _, node := range dockerfile.AST.Children { if isSelfConsistentCasing(node.Value) { diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index 6c89b7785561..495313b5aa2b 100644 --- a/frontend/dockerfile/dockerfile_lint_test.go +++ b/frontend/dockerfile/dockerfile_lint_test.go @@ -3,7 +3,6 @@ package dockerfile import ( "context" "encoding/json" - "fmt" "os" "sort" "testing" @@ -11,6 +10,7 @@ import ( "github.com/containerd/continuity/fs/fstest" "github.com/moby/buildkit/client" + "github.com/moby/buildkit/frontend/dockerfile/linter" "github.com/moby/buildkit/frontend/dockerui" gateway "github.com/moby/buildkit/frontend/gateway/client" @@ -22,11 +22,13 @@ import ( ) var lintTests = integration.TestFuncs( - testLintStageName, - testLintNoEmptyContinuations, + testStageName, + testNoEmptyContinuations, + testSelfConsistentCommandCasing, + testFileConsistentCommandCasing, ) -func testLintStageName(t *testing.T, sb integration.Sandbox) { +func testStageName(t *testing.T, sb integration.Sandbox) { dockerfile := []byte(` # warning: stage name should be lowercase FROM scratch AS BadStageName @@ -39,16 +41,16 @@ FROM scratch AS base3 checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { RuleName: "StageNameCasing", - Description: "Stage name 'BadStageName' should be lowercase", + Description: "Stage names should be lowercase", + Detail: "Stage name 'BadStageName' should be lowercase", Line: 3, - Detail: "Stage names should be lowercase", Level: 1, }, { RuleName: "FromAsCasing", - Description: "'as' and 'FROM' keywords' casing do not match", + Description: "The 'as' keyword should match the case of the 'from' keyword", + Detail: "'as' and 'FROM' keywords' casing do not match", Line: 6, - Detail: "The 'as' keyword should match the case of the 'from' keyword", Level: 1, }, }) @@ -62,15 +64,15 @@ from scratch as base2 checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { RuleName: "FromAsCasing", - Description: "'AS' and 'from' keywords' casing do not match", - Line: 3, - Detail: "The 'as' keyword should match the case of the 'from' keyword", + Description: "The 'as' keyword should match the case of the 'from' keyword", + Detail: "'AS' and 'from' keywords' casing do not match", Level: 1, + Line: 3, }, }) } -func testLintNoEmptyContinuations(t *testing.T, sb integration.Sandbox) { +func testNoEmptyContinuations(t *testing.T, sb integration.Sandbox) { dockerfile := []byte(` FROM scratch # warning: empty continuation line @@ -84,11 +86,11 @@ COPY Dockerfile \ checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { RuleName: "NoEmptyContinuations", - Description: "Empty continuation line", + Description: "Empty continuation lines will become errors in a future release", + Detail: "Empty continuation line", + Level: 1, Line: 6, - Detail: "Empty continuation lines will become errors in a future release", URL: "https://github.com/moby/moby/pull/33719", - Level: 1, }, }) } @@ -102,10 +104,10 @@ FROM scratch AS base2 checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { RuleName: "SelfConsistentCommandCasing", - Description: "Command 'From' should be consistently cased", - Line: 3, - Detail: "Commands should be in consistent casing (all lower or all upper)", + Description: "Commands should be in consistent casing (all lower or all upper)", + Detail: "Command 'From' should be consistently cased", Level: 1, + Line: 3, }, }) dockerfile = []byte(` @@ -116,9 +118,9 @@ from scratch as base2 checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { RuleName: "SelfConsistentCommandCasing", - Description: "Command 'frOM' should be consistently cased", + Description: "Commands should be in consistent casing (all lower or all upper)", + Detail: "Command 'frOM' should be consistently cased", Line: 3, - Detail: "Commands should be in consistent casing (all lower or all upper)", Level: 1, }, }) @@ -134,9 +136,9 @@ COPY Dockerfile /bar checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { RuleName: "FileConsistentCommandCasing", - Description: "Command 'copy' should match the case of the command majority (uppercase)", + Description: "All commands within the Dockerfile should use the same casing (either upper or lower)", + Detail: "Command 'copy' should match the case of the command majority (uppercase)", Line: 4, - Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", Level: 1, }, }) @@ -150,9 +152,9 @@ copy Dockerfile /bar checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { RuleName: "FileConsistentCommandCasing", - Description: "Command 'COPY' should match the case of the command majority (lowercase)", + Description: "All commands within the Dockerfile should use the same casing (either upper or lower)", + Detail: "Command 'COPY' should match the case of the command majority (lowercase)", Line: 4, - Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", Level: 1, }, }) @@ -167,9 +169,9 @@ COPY Dockerfile /baz checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { RuleName: "FileConsistentCommandCasing", - Description: "Command 'from' should match the case of the command majority (uppercase)", + Description: "All commands within the Dockerfile should use the same casing (either upper or lower)", + Detail: "Command 'from' should match the case of the command majority (uppercase)", Line: 3, - Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", Level: 1, }, }) @@ -184,9 +186,9 @@ copy Dockerfile /baz checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { RuleName: "FileConsistentCommandCasing", - Description: "Command 'FROM' should match the case of the command majority (lowercase)", + Description: "All commands within the Dockerfile should use the same casing (either upper or lower)", + Detail: "Command 'FROM' should match the case of the command majority (lowercase)", Line: 3, - Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", Level: 1, }, }) @@ -229,8 +231,8 @@ func checkUnmarshal(t *testing.T, sb integration.Sandbox, c *client.Client, f fr require.Equal(t, len(expected), len(lintResults.Warnings)) sort.Slice(lintResults.Warnings, func(i, j int) bool { // sort by line number in ascending order - firstRange := lintResults.Warnings[i].Location[0] - secondRange := lintResults.Warnings[j].Location[0] + firstRange := lintResults.Warnings[i].Location.Ranges[0] + secondRange := lintResults.Warnings[j].Location.Ranges[0] return firstRange.Start.Line < secondRange.Start.Line }) // Compare expectedLintWarning with actual lint results @@ -323,22 +325,28 @@ func checkLinterWarnings(t *testing.T, sb integration.Sandbox, dockerfile []byte require.NoError(t, err) defer c.Close() - checkProgressStream(t, sb, c, f, dir, dockerfile, expected) - checkUnmarshal(t, sb, c, f, dir, dockerfile, expected) + t.Run("warntype=progress", func(t *testing.T) { + checkProgressStream(t, sb, c, f, dir, dockerfile, expected) + }) + t.Run("warntype=unmarshal", func(t *testing.T) { + checkUnmarshal(t, sb, c, f, dir, dockerfile, expected) + }) } func checkVertexWarning(t *testing.T, warning *client.VertexWarning, expected expectedLintWarning) { - short := fmt.Sprintf("Lint Rule '%s': %s (line %d)", expected.RuleName, expected.Description, expected.Line) + short := linter.LintFormatShort(expected.RuleName, expected.Detail, expected.Line) require.Equal(t, short, string(warning.Short)) - require.Equal(t, expected.Detail, string(warning.Detail[0])) + require.Equal(t, expected.Description, string(warning.Detail[0])) require.Equal(t, expected.URL, warning.URL) require.Equal(t, expected.Level, warning.Level) } func checkLintWarning(t *testing.T, warning lint.Warning, expected expectedLintWarning) { + short := linter.LintFormatShort(expected.RuleName, expected.Detail, expected.Line) require.Equal(t, expected.RuleName, warning.RuleName) - detail := fmt.Sprintf("%s (line %d)", expected.Description, expected.Line) - require.Equal(t, detail, warning.Detail) + require.Equal(t, expected.Description, warning.Description) + require.Equal(t, expected.URL, warning.URL) + require.Equal(t, short, warning.Detail) } func unmarshalLintResults(res *gateway.Result) (*lint.LintResults, error) { diff --git a/frontend/dockerfile/dockerfile_test.go b/frontend/dockerfile/dockerfile_test.go index 98485715f95d..2bf962c04c2e 100644 --- a/frontend/dockerfile/dockerfile_test.go +++ b/frontend/dockerfile/dockerfile_test.go @@ -175,8 +175,6 @@ var allTests = integration.TestFuncs( testDuplicatePlatformProvenance, testDockerIgnoreMissingProvenance, testSBOMScannerArgs, - testSelfConsistentCommandCasing, - testFileConsistentCommandCasing, testNilContextInSolveGateway, testCopyUnicodePath, testFrontendDeduplicateSources, diff --git a/frontend/dockerfile/linter/linter.go b/frontend/dockerfile/linter/linter.go index 96a0d878f24b..d8d33e5e94cf 100644 --- a/frontend/dockerfile/linter/linter.go +++ b/frontend/dockerfile/linter/linter.go @@ -22,9 +22,12 @@ func (rule LinterRule[F]) Run(warn LintWarnFunc, location []parser.Range, txt .. if len(txt) == 0 { txt = []string{rule.Description} } - short := strings.Join(txt, " ") - short = fmt.Sprintf("Lint Rule '%s': %s (line %d)", rule.Name, short, startLine) - warn(short, rule.URL, [][]byte{[]byte(rule.Description)}, location) + short := LintFormatShort(rule.Name, strings.Join(txt, " "), startLine) + warn(rule.Name, rule.Description, rule.URL, short, location) } -type LintWarnFunc func(short, url string, detail [][]byte, location []parser.Range) +func LintFormatShort(rulename, msg string, startLine int) string { + return fmt.Sprintf("Lint Rule '%s': %s (line %d)", rulename, msg, startLine) +} + +type LintWarnFunc func(rulename, description, url, fmtmsg string, location []parser.Range) diff --git a/frontend/subrequests/lint/lint.go b/frontend/subrequests/lint/lint.go index d03e40656c1f..cfe1420b274b 100644 --- a/frontend/subrequests/lint/lint.go +++ b/frontend/subrequests/lint/lint.go @@ -8,8 +8,11 @@ import ( "sort" "text/tabwriter" + "github.com/moby/buildkit/client/llb" + "github.com/moby/buildkit/frontend/dockerfile/parser" "github.com/moby/buildkit/frontend/gateway/client" "github.com/moby/buildkit/frontend/subrequests" + "github.com/moby/buildkit/solver/pb" ) const RequestLint = "frontend.lint" @@ -27,26 +30,18 @@ var SubrequestLintDefinition = subrequests.Request{ } type Source struct { - Filename string `json:"filename"` - Language string `json:"language"` - Data []byte `json:"data"` -} - -type Range struct { - Start Position `json:"start"` - End Position `json:"end"` -} - -type Position struct { - Line int `json:"line"` - Column int `json:"column"` + Filename string `json:"fileName"` + Language string `json:"language"` + Definition *pb.Definition `json:"definition"` + Data []byte `json:"data"` } type Warning struct { - RuleName string `json:"rule_name"` - Detail string `json:"detail"` - SourceIndex int `json:"source_index"` - Location []Range `json:"location"` + RuleName string `json:"ruleName"` + Description string `json:"description,omitempty"` + URL string `json:"url,omitempty"` + Detail string `json:"detail,omitempty"` + Location pb.Location `json:"location,omitempty"` } type LintResults struct { @@ -54,31 +49,46 @@ type LintResults struct { Sources []Source `json:"sources"` } -func (results *LintResults) AddSource(filename string, language string, data []byte) int { - sourceE := Source{ - Filename: filename, - Language: language, - Data: data, +func (results *LintResults) AddSource(sourceMap *llb.SourceMap) int { + newSource := Source{ + Filename: sourceMap.Filename, + Language: sourceMap.Language, + Definition: sourceMap.Definition.ToPB(), + Data: sourceMap.Data, } for i, source := range results.Sources { - if sourceEqual(source, sourceE) { + if sourceEqual(source, newSource) { return i } } - results.Sources = append(results.Sources, Source{ - Filename: filename, - Language: language, - Data: data, - }) + results.Sources = append(results.Sources, newSource) return len(results.Sources) - 1 } -func (results *LintResults) AddWarning(ruleName, detail string, sourceIndex int, location []Range) { +func (results *LintResults) AddWarning(rulename, description, url, fmtmsg string, sourceIndex int, location []parser.Range) { + sourceLocation := []*pb.Range{} + for _, loc := range location { + sourceLocation = append(sourceLocation, &pb.Range{ + Start: pb.Position{ + Line: int32(loc.Start.Line), + Character: int32(loc.Start.Character), + }, + End: pb.Position{ + Line: int32(loc.End.Line), + Character: int32(loc.End.Character), + }, + }) + } + pbLocation := pb.Location{ + SourceIndex: int32(sourceIndex), + Ranges: sourceLocation, + } results.Warnings = append(results.Warnings, Warning{ - RuleName: ruleName, - Detail: detail, - SourceIndex: sourceIndex, - Location: location, + RuleName: rulename, + Description: description, + URL: url, + Detail: fmtmsg, + Location: pbLocation, }) } @@ -89,9 +99,9 @@ func sourceEqual(a, b Source) bool { return bytes.Equal(a.Data, b.Data) } -func (warns LintResults) ToResult() (*client.Result, error) { +func (results *LintResults) ToResult() (*client.Result, error) { res := client.NewResult() - dt, err := json.MarshalIndent(warns, "", " ") + dt, err := json.MarshalIndent(results, "", " ") if err != nil { return nil, err } @@ -130,16 +140,16 @@ func PrintLintViolations(dt []byte, w io.Writer) error { for _, rule := range lintWarningRules { fmt.Fprintf(tw, "Lint Rule %s\n", rule) for _, warning := range lintWarnings[rule] { - source := warnings.Sources[warning.SourceIndex] + source := warnings.Sources[warning.Location.SourceIndex] sourceData := bytes.Split(source.Data, []byte("\n")) - firstRange := warning.Location[0] + firstRange := warning.Location.Ranges[0] if firstRange.Start.Line != firstRange.End.Line { fmt.Fprintf(tw, "\t%s:%d-%d\n", source.Filename, firstRange.Start.Line, firstRange.End.Line) } else { fmt.Fprintf(tw, "\t%s:%d\n", source.Filename, firstRange.Start.Line) } fmt.Fprintf(tw, "\t%s\n", warning.Detail) - for _, r := range warning.Location { + for _, r := range warning.Location.Ranges { for i := r.Start.Line; i <= r.End.Line; i++ { fmt.Fprintf(tw, "\t%d\t|\t%s\n", i, sourceData[i-1]) }