From d635c7eb8ce2fa95c17f1d8b96abcab67ea0abb3 Mon Sep 17 00:00:00 2001 From: Talon Bowler Date: Fri, 26 Apr 2024 19:21:26 -0700 Subject: [PATCH] update lint subrequest to return build error and warnings up to error rather than a failed grpc response Signed-off-by: Talon Bowler --- frontend/dockerfile/dockerfile2llb/convert.go | 61 ++- frontend/dockerfile/dockerfile_lint_test.go | 352 +++++++++++------- frontend/subrequests/lint/lint.go | 6 + 3 files changed, 288 insertions(+), 131 deletions(-) diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 7828a15eed472..6fed838409b5d 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -118,8 +118,17 @@ func DockerfileLint(ctx context.Context, dt []byte, opt ConvertOpt) (*lint.LintR results.AddWarning(rulename, description, url, fmtmsg, sourceIndex, location) } _, err := toDispatchState(ctx, dt, opt) + + var errLoc *parser.ErrorLocation if err != nil { - return nil, err + buildErr := &lint.BuildError{ + Message: err.Error(), + } + if errors.As(err, &errLoc) { + ranges := mergeLocations(errLoc.Locations...) + buildErr.Location = toPBLocation(sourceIndex, ranges) + } + results.Error = buildErr } return results, nil } @@ -2015,3 +2024,53 @@ func validateStageNames(stages []instructions.Stage, warn linter.LintWarnFunc) { } } } + +func mergeLocations(locations ...[]parser.Range) []parser.Range { + allRanges := []parser.Range{} + for _, ranges := range locations { + allRanges = append(allRanges, ranges...) + } + if len(allRanges) == 0 { + return []parser.Range{} + } + if len(allRanges) == 1 { + return allRanges + } + + sort.Slice(allRanges, func(i, j int) bool { + return allRanges[i].Start.Line < allRanges[j].Start.Line + }) + + location := []parser.Range{} + currentRange := allRanges[0] + for _, r := range allRanges[1:] { + if r.Start.Line <= currentRange.End.Line { + currentRange.End = r.End + } else { + location = append(location, currentRange) + currentRange = r + } + } + location = append(location, currentRange) + return location +} + +func toPBLocation(sourceIndex int, location []parser.Range) pb.Location { + loc := make([]*pb.Range, 0, len(location)) + for _, l := range location { + loc = append(loc, &pb.Range{ + Start: pb.Position{ + Line: int32(l.Start.Line), + Character: int32(l.Start.Character), + }, + End: pb.Position{ + Line: int32(l.End.Line), + Character: int32(l.End.Character), + }, + }) + } + return pb.Location{ + SourceIndex: int32(sourceIndex), + Ranges: loc, + } +} diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index 09803269a1e4d..b184691736f43 100644 --- a/frontend/dockerfile/dockerfile_lint_test.go +++ b/frontend/dockerfile/dockerfile_lint_test.go @@ -29,6 +29,7 @@ var lintTests = integration.TestFuncs( testDuplicateStageName, testReservedStageName, testMaintainerDeprecated, + testWarningsBeforeError, ) func testStageName(t *testing.T, sb integration.Sandbox) { @@ -41,20 +42,23 @@ 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, + checkLinterWarnings(t, sb, &lintTestParams{ + Dockerfile: dockerfile, + Warnings: []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, + }, }, }) @@ -64,13 +68,17 @@ 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, + + checkLinterWarnings(t, sb, &lintTestParams{ + Dockerfile: dockerfile, + Warnings: []expectedLintWarning{ + { + RuleName: "FromAsCasing", + Description: "The 'as' keyword should match the case of the 'from' keyword", + Detail: "'AS' and 'from' keywords' casing do not match", + Line: 3, + Level: 1, + }, }, }) } @@ -86,14 +94,17 @@ 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", + checkLinterWarnings(t, sb, &lintTestParams{ + Dockerfile: dockerfile, + Warnings: []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", + }, }, }) } @@ -104,27 +115,34 @@ func testSelfConsistentCommandCasing(t *testing.T, sb integration.Sandbox) { 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, + checkLinterWarnings(t, sb, &lintTestParams{ + Dockerfile: dockerfile, + Warnings: []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, + checkLinterWarnings(t, sb, &lintTestParams{ + Dockerfile: dockerfile, + Warnings: []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, + }, }, }) } @@ -136,13 +154,17 @@ FROM scratch 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, + + checkLinterWarnings(t, sb, &lintTestParams{ + Dockerfile: dockerfile, + Warnings: []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, + }, }, }) @@ -152,13 +174,16 @@ from scratch 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, + checkLinterWarnings(t, sb, &lintTestParams{ + Dockerfile: dockerfile, + Warnings: []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, + }, }, }) @@ -169,13 +194,16 @@ 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, + checkLinterWarnings(t, sb, &lintTestParams{ + Dockerfile: dockerfile, + Warnings: []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, + }, }, }) @@ -186,13 +214,16 @@ 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, + checkLinterWarnings(t, sb, &lintTestParams{ + Dockerfile: dockerfile, + Warnings: []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, + }, }, }) @@ -201,14 +232,14 @@ from scratch copy Dockerfile /foo copy Dockerfile /bar `) - checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{}) + checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) dockerfile = []byte(` FROM scratch COPY Dockerfile /foo COPY Dockerfile /bar `) - checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{}) + checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) } func testDuplicateStageName(t *testing.T, sb integration.Sandbox) { @@ -216,13 +247,16 @@ func testDuplicateStageName(t *testing.T, sb integration.Sandbox) { FROM scratch AS b FROM scratch AS b `) - checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ - { - RuleName: "DuplicateStageName", - Description: "Stage names should be unique", - Detail: "Duplicate stage name \"b\", stage names should be unique", - Level: 1, - Line: 3, + checkLinterWarnings(t, sb, &lintTestParams{ + Dockerfile: dockerfile, + Warnings: []expectedLintWarning{ + { + RuleName: "DuplicateStageName", + Description: "Stage names should be unique", + Detail: "Duplicate stage name \"b\", stage names should be unique", + Level: 1, + Line: 3, + }, }, }) @@ -230,7 +264,7 @@ FROM scratch AS b FROM scratch AS b1 FROM scratch AS b2 `) - checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{}) + checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) } func testReservedStageName(t *testing.T, sb integration.Sandbox) { @@ -238,20 +272,23 @@ func testReservedStageName(t *testing.T, sb integration.Sandbox) { FROM scratch AS scratch FROM scratch AS context `) - checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ - { - RuleName: "ReservedStageName", - Description: "Reserved stage names should not be used to name a stage", - Detail: "Stage name should not use the same name as reserved stage \"scratch\"", - Level: 1, - Line: 2, - }, - { - RuleName: "ReservedStageName", - Description: "Reserved stage names should not be used to name a stage", - Detail: "Stage name should not use the same name as reserved stage \"context\"", - Level: 1, - Line: 3, + checkLinterWarnings(t, sb, &lintTestParams{ + Dockerfile: dockerfile, + Warnings: []expectedLintWarning{ + { + RuleName: "ReservedStageName", + Description: "Reserved stage names should not be used to name a stage", + Detail: "Stage name should not use the same name as reserved stage \"scratch\"", + Level: 1, + Line: 2, + }, + { + RuleName: "ReservedStageName", + Description: "Reserved stage names should not be used to name a stage", + Detail: "Stage name should not use the same name as reserved stage \"context\"", + Level: 1, + Line: 3, + }, }, }) @@ -261,7 +298,7 @@ FROM scratch AS context FROM scratch FROM scratch AS a `) - checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{}) + checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) } func testMaintainerDeprecated(t *testing.T, sb integration.Sandbox) { @@ -269,14 +306,17 @@ func testMaintainerDeprecated(t *testing.T, sb integration.Sandbox) { FROM scratch MAINTAINER me@example.org `) - checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ - { - RuleName: "MaintainerDeprecated", - Description: "The maintainer instruction is deprecated, use a label instead to define an image author", - Detail: "Maintainer instruction is deprecated in favor of using label", - URL: "https://docs.docker.com/reference/dockerfile/#maintainer-deprecated", - Level: 1, - Line: 3, + checkLinterWarnings(t, sb, &lintTestParams{ + Dockerfile: dockerfile, + Warnings: []expectedLintWarning{ + { + RuleName: "MaintainerDeprecated", + Description: "The maintainer instruction is deprecated, use a label instead to define an image author", + Detail: "Maintainer instruction is deprecated in favor of using label", + URL: "https://docs.docker.com/reference/dockerfile/#maintainer-deprecated", + Level: 1, + Line: 3, + }, }, }) @@ -284,10 +324,33 @@ MAINTAINER me@example.org FROM scratch LABEL org.opencontainers.image.authors="me@example.org" `) - checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{}) + checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) } -func checkUnmarshal(t *testing.T, sb integration.Sandbox, c *client.Client, dir *integration.TmpDirWithName, expected []expectedLintWarning) { +func testWarningsBeforeError(t *testing.T, sb integration.Sandbox) { + dockerfile := []byte(` +# warning: stage name should be lowercase +FROM scratch AS BadStageName +FROM ${BAR} AS base +`) + checkLinterWarnings(t, sb, &lintTestParams{ + Dockerfile: dockerfile, + Warnings: []expectedLintWarning{ + { + RuleName: "StageNameCasing", + Description: "Stage names should be lowercase", + Detail: "Stage name 'BadStageName' should be lowercase", + Line: 3, + Level: 1, + }, + }, + StreamBuildErr: "failed to solve: base name (${BAR}) should not be blank", + UnmarshalBuildErr: "base name (${BAR}) should not be blank", + BuildErrLocation: 4, + }) +} + +func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestParams) { destDir, err := os.MkdirTemp("", "buildkit") require.NoError(t, err) defer os.RemoveAll(destDir) @@ -302,12 +365,18 @@ func checkUnmarshal(t *testing.T, sb integration.Sandbox, c *client.Client, dir }, Frontend: "dockerfile.v0", }) - require.NoError(t, err) + if err != nil { + return nil, err + } lintResults, err := unmarshalLintResults(res) require.NoError(t, err) - require.Equal(t, len(expected), len(lintResults.Warnings)) + if lintResults.Error != nil { + require.Equal(t, lintTest.UnmarshalBuildErr, lintResults.Error.Message) + require.Equal(t, lintTest.BuildErrLocation, lintResults.Error.Location.Ranges[0].Start.Line) + } + require.Equal(t, len(lintTest.Warnings), 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] @@ -316,15 +385,15 @@ func checkUnmarshal(t *testing.T, sb integration.Sandbox, c *client.Client, dir }) // Compare expectedLintWarning with actual lint results for i, w := range lintResults.Warnings { - checkLintWarning(t, w, expected[i]) + checkLintWarning(t, w, lintTest.Warnings[i]) } called = true return nil, nil } - _, err = c.Build(sb.Context(), client.SolveOpt{ + _, err = lintTest.Client.Build(sb.Context(), client.SolveOpt{ LocalMounts: map[string]fsutil.FS{ - dockerui.DefaultLocalNameDockerfile: dir, + dockerui.DefaultLocalNameDockerfile: lintTest.TmpDir, }, }, "", frontend, nil) require.NoError(t, err) @@ -332,7 +401,7 @@ func checkUnmarshal(t *testing.T, sb integration.Sandbox, c *client.Client, dir require.True(t, called) } -func checkProgressStream(t *testing.T, sb integration.Sandbox, c *client.Client, dir *integration.TmpDirWithName, expected []expectedLintWarning) { +func checkProgressStream(t *testing.T, sb integration.Sandbox, lintTest *lintTestParams) { t.Helper() status := make(chan *client.SolveStatus) @@ -358,16 +427,20 @@ func checkProgressStream(t *testing.T, sb integration.Sandbox, c *client.Client, f := getFrontend(t, sb) - _, err := f.Solve(sb.Context(), c, client.SolveOpt{ + _, err := f.Solve(sb.Context(), lintTest.Client, client.SolveOpt{ FrontendAttrs: map[string]string{ "platform": "linux/amd64,linux/arm64", }, LocalMounts: map[string]fsutil.FS{ - dockerui.DefaultLocalNameDockerfile: dir, - dockerui.DefaultLocalNameContext: dir, + dockerui.DefaultLocalNameDockerfile: lintTest.TmpDir, + dockerui.DefaultLocalNameContext: lintTest.TmpDir, }, }, status) - require.NoError(t, err) + if lintTest.StreamBuildErr == "" { + require.NoError(t, err) + } else { + require.EqualError(t, err, lintTest.StreamBuildErr) + } select { case <-statusDone: @@ -376,7 +449,7 @@ func checkProgressStream(t *testing.T, sb integration.Sandbox, c *client.Client, } // two platforms only show one warning - require.Equal(t, len(expected), len(warnings)) + require.Equal(t, len(lintTest.Warnings), len(warnings)) sort.Slice(warnings, func(i, j int) bool { w1 := warnings[i] w2 := warnings[j] @@ -388,30 +461,39 @@ func checkProgressStream(t *testing.T, sb integration.Sandbox, c *client.Client, return w1.Range[0].Start.Line < w2.Range[0].Start.Line }) for i, w := range warnings { - checkVertexWarning(t, w, expected[i]) + checkVertexWarning(t, w, lintTest.Warnings[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. +func checkLinterWarnings(t *testing.T, sb integration.Sandbox, lintTest *lintTestParams) { + if lintTest.Warnings != nil { + sort.Slice(lintTest.Warnings, func(i, j int) bool { + return lintTest.Warnings[i].Line < lintTest.Warnings[j].Line + }) + } integration.SkipOnPlatform(t, "windows") - dir := integration.Tmpdir( - t, - fstest.CreateFile("Dockerfile", dockerfile, 0600), - ) + if lintTest.TmpDir == nil { + lintTest.TmpDir = integration.Tmpdir( + t, + fstest.CreateFile("Dockerfile", lintTest.Dockerfile, 0600), + ) + } - c, err := client.New(sb.Context(), sb.Address()) - require.NoError(t, err) - defer c.Close() + if lintTest.Client == nil { + var err error + lintTest.Client, err = client.New(sb.Context(), sb.Address()) + require.NoError(t, err) + defer lintTest.Client.Close() + } t.Run("warntype=progress", func(t *testing.T) { - checkProgressStream(t, sb, c, dir, expected) + checkProgressStream(t, sb, lintTest) }) + t.Run("warntype=unmarshal", func(t *testing.T) { - checkUnmarshal(t, sb, c, dir, expected) + checkUnmarshal(t, sb, lintTest) }) } @@ -452,3 +534,13 @@ type expectedLintWarning struct { URL string Level int } + +type lintTestParams struct { + Client *client.Client + TmpDir *integration.TmpDirWithName + Dockerfile []byte + Warnings []expectedLintWarning + StreamBuildErr string + UnmarshalBuildErr string + BuildErrLocation int +} diff --git a/frontend/subrequests/lint/lint.go b/frontend/subrequests/lint/lint.go index 6237eb3c1c21e..b280836c5e7d8 100644 --- a/frontend/subrequests/lint/lint.go +++ b/frontend/subrequests/lint/lint.go @@ -39,9 +39,15 @@ type Warning struct { Location pb.Location `json:"location,omitempty"` } +type BuildError struct { + Message string `json:"message"` + Location pb.Location +} + type LintResults struct { Warnings []Warning `json:"warnings"` Sources []*pb.SourceInfo `json:"sources"` + Error *BuildError `json:"buildError,omitempty"` } func (results *LintResults) AddSource(sourceMap *llb.SourceMap) int {