diff --git a/frontend/dockerfile/builder/build.go b/frontend/dockerfile/builder/build.go index 6c6fdc48a7c8..09a36ebd08ae 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" @@ -73,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)) }, } @@ -85,6 +86,9 @@ 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) { + return dockerfile2llb.DockerfileLint(ctx, src.Data, convertOpt) + }, }); err != nil { return nil, err } else if ok { diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index ea5e8601536b..4975f26f113e 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" @@ -63,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 { @@ -110,6 +111,19 @@ func Dockefile2Outline(ctx context.Context, dt []byte, opt ConvertOpt) (*outline return &o, nil } +func DockerfileLint(ctx context.Context, dt []byte, opt ConvertOpt) (*lint.LintResults, error) { + results := &lint.LintResults{} + 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 { + return nil, err + } + return results, nil +} + func ListTargets(ctx context.Context, dt []byte) (*targets.List, error) { dockerfile, err := parser.Parse(bytes.NewReader(dt)) if err != nil { @@ -162,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 { @@ -1946,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 new file mode 100644 index 000000000000..495313b5aa2b --- /dev/null +++ b/frontend/dockerfile/dockerfile_lint_test.go @@ -0,0 +1,371 @@ +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/dockerfile/linter" + "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/pkg/errors" + "github.com/stretchr/testify/require" + "github.com/tonistiigi/fsutil" +) + +var lintTests = integration.TestFuncs( + testStageName, + testNoEmptyContinuations, + testSelfConsistentCommandCasing, + testFileConsistentCommandCasing, +) + +func testStageName(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{ + { + RuleName: "StageNameCasing", + Description: "Stage names should be lowercase", + Detail: "Stage name 'BadStageName' should be lowercase", + Line: 3, + Level: 1, + }, + { + RuleName: "FromAsCasing", + Description: "The 'as' keyword should match the case of the 'from' keyword", + Detail: "'as' and 'FROM' keywords' casing do not match", + Line: 6, + Level: 1, + }, + }) + + dockerfile = []byte(` +# warning: 'AS' should match 'from' cmd casing. +from scratch AS base + +from scratch as base2 +`) + checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + { + RuleName: "FromAsCasing", + 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 testNoEmptyContinuations(t *testing.T, sb integration.Sandbox) { + dockerfile := []byte(` +FROM scratch +# warning: empty continuation line +COPY Dockerfile \ + +. +COPY Dockerfile \ +. +`) + + checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + { + RuleName: "NoEmptyContinuations", + Description: "Empty continuation lines will become errors in a future release", + Detail: "Empty continuation line", + Level: 1, + Line: 6, + URL: "https://github.com/moby/moby/pull/33719", + }, + }) +} + +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{ + { + RuleName: "SelfConsistentCommandCasing", + 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(` +# warning: 'FROM' should be either lowercased or uppercased +frOM scratch as base +from scratch as base2 +`) + checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ + { + RuleName: "SelfConsistentCommandCasing", + Description: "Commands should be in consistent casing (all lower or all upper)", + Detail: "Command 'frOM' should be consistently cased", + Line: 3, + 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{ + { + RuleName: "FileConsistentCommandCasing", + 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, + 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{ + { + RuleName: "FileConsistentCommandCasing", + 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, + 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{ + { + RuleName: "FileConsistentCommandCasing", + 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, + 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{ + { + RuleName: "FileConsistentCommandCasing", + 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, + 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 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) + + 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) + + lintResults, err := unmarshalLintResults(res) + require.NoError(t, err) + + 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.Ranges[0] + secondRange := lintResults.Warnings[j].Location.Ranges[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) + + 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{}) + + 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 { + checkVertexWarning(t, w, expected[i]) + } +} + +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() + + 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 := linter.LintFormatShort(expected.RuleName, expected.Detail, expected.Line) + require.Equal(t, short, string(warning.Short)) + 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) + 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) { + 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 +} + +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 fe448ed30b0d..2bf962c04c2e 100644 --- a/frontend/dockerfile/dockerfile_test.go +++ b/frontend/dockerfile/dockerfile_test.go @@ -175,10 +175,6 @@ var allTests = integration.TestFuncs( testDuplicatePlatformProvenance, testDockerIgnoreMissingProvenance, testSBOMScannerArgs, - testLintStageName, - testLintNoEmptyContinuations, - testSelfConsistentCommandCasing, - testFileConsistentCommandCasing, testNilContextInSolveGateway, testCopyUnicodePath, testFrontendDeduplicateSources, @@ -247,6 +243,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 +6764,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 +6920,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) @@ -7535,13 +7298,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/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/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..cfe1420b274b --- /dev/null +++ b/frontend/subrequests/lint/lint.go @@ -0,0 +1,163 @@ +package lint + +import ( + "bytes" + "encoding/json" + "fmt" + "io" + "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" + +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 Source struct { + Filename string `json:"fileName"` + Language string `json:"language"` + Definition *pb.Definition `json:"definition"` + Data []byte `json:"data"` +} + +type Warning struct { + 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 { + Warnings []Warning `json:"warnings"` + Sources []Source `json:"sources"` +} + +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, newSource) { + return i + } + } + results.Sources = append(results.Sources, newSource) + return len(results.Sources) - 1 +} + +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, + Description: description, + URL: url, + Detail: fmtmsg, + Location: pbLocation, + }) +} + +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 (results *LintResults) ToResult() (*client.Result, error) { + res := client.NewResult() + dt, err := json.MarshalIndent(results, "", " ") + 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] { + source := warnings.Sources[warning.Location.SourceIndex] + sourceData := bytes.Split(source.Data, []byte("\n")) + 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.Ranges { + 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) + } + fmt.Fprintln(tw) + } + + return tw.Flush() +}