diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index b4a3892462c1d..243a2a688d289 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -139,10 +139,6 @@ func ListTargets(ctx context.Context, dt []byte) (*targets.List, error) { return l, nil } -func consistentCasing(s string) bool { - return s == strings.ToLower(s) || s == strings.ToUpper(s) -} - func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchState, error) { if len(dt) == 0 { return nil, errors.Errorf("the Dockerfile cannot be empty") @@ -202,7 +198,7 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS } for _, node := range dockerfile.AST.Children { - if !consistentCasing(node.Value) { + if !isConsistentCasing(node.Value) { msg := linter.RuleCommandCasing.Format(node.Value) linter.RuleCommandCasing.Run(opt.Warn, node.Location(), msg) } @@ -1952,3 +1948,7 @@ func isEnabledForStage(stage string, value string) bool { } return false } + +func isConsistentCasing(s string) bool { + return s == strings.ToLower(s) || s == strings.ToUpper(s) +} diff --git a/frontend/dockerfile/dockerfile_test.go b/frontend/dockerfile/dockerfile_test.go index 845bfecb63847..785ff60cb2458 100644 --- a/frontend/dockerfile/dockerfile_test.go +++ b/frontend/dockerfile/dockerfile_test.go @@ -6766,99 +6766,79 @@ ARG BUILDKIT_SBOM_SCAN_STAGE=true } func testLintWarnings(t *testing.T, sb integration.Sandbox) { - integration.SkipOnPlatform(t, "windows") - f := getFrontend(t, sb) + testLintStageName(t, sb) + testLintNoEmptyContinuations(t, sb) + testCommandCasing(t, sb) +} - // empty line in here is intentional to cause line continuation warning +func testLintStageName(t *testing.T, sb integration.Sandbox) { dockerfile := []byte(` -# set of commands which should cause warnings -FROM scratch as BadStageName -Copy Dockerfile . -COPY Dockerfile \ - -. - -# set of commands which should not cause warnings -FROM scratch as base -copy Dockerfile . -COPY Dockerfile . -`) - - dir := integration.Tmpdir( - t, - fstest.CreateFile("Dockerfile", dockerfile, 0600), - ) - - c, err := client.New(sb.Context(), sb.Address()) - require.NoError(t, err) - defer c.Close() +# warning: stage name should be lowercase +FROM scratch AS BadStageName - status := make(chan *client.SolveStatus) - statusDone := make(chan struct{}) - done := make(chan struct{}) - - var warnings []*client.VertexWarning +# warning: 'as' should match 'FROM' cmd casing. +FROM scratch as base2 - go func() { - defer close(statusDone) - for { - select { - case st, ok := <-status: - if !ok { - return - } - warnings = append(warnings, st.Warnings...) - case <-done: - return - } - } - }() +# warning: 'AS' should match 'from' cmd casing. +from scratch AS base3 - _, err = f.Solve(sb.Context(), c, client.SolveOpt{ - FrontendAttrs: map[string]string{ - "platform": "linux/amd64,linux/arm64", +FROM scratch AS base4 +from scratch as base5 +`) + compareLintWarnings(t, sb, dockerfile, []expectedLintWarning{ + { + Short: "Lint Rule 'StageNameCasing': Stage name 'BadStageName' should be lowercase (line 3)", + Detail: "Stage names should be lowercase", + Level: 1, }, - LocalMounts: map[string]fsutil.FS{ - dockerui.DefaultLocalNameDockerfile: dir, - dockerui.DefaultLocalNameContext: dir, + { + 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, }, - }, status) - require.NoError(t, err) + { + Short: "Lint Rule 'FromAsCasing': 'AS' and 'from' keywords' casing do not match (line 9)", + Detail: "The 'as' keyword should match the case of the 'from' keyword", + Level: 1, + }, + }) +} - select { - case <-statusDone: - case <-time.After(10 * time.Second): - close(done) - } +func testLintNoEmptyContinuations(t *testing.T, sb integration.Sandbox) { + dockerfile := []byte(` +FROM scratch +# warning: empty continuation line +COPY Dockerfile \ - <-statusDone +. +COPY Dockerfile \ +. +`) - // two platforms only show one warning - require.Equal(t, 3, 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 + compareLintWarnings(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, + }, }) +} - require.Equal(t, "Lint Rule 'StageNameCasing': Stage name 'BadStageName' should be lowercase (line 3)", string(warnings[0].Short)) - require.Equal(t, "Stage names should be lowercase", string(warnings[0].Detail[0])) - require.Equal(t, "", warnings[0].URL) - require.Equal(t, 1, warnings[0].Level) - - require.Equal(t, "Lint Rule 'CommandCasing': Command 'Copy' should be consistently cased (line 4)", string(warnings[1].Short)) - require.Equal(t, "Commands should be in consistent casing (all lower or all upper)", string(warnings[1].Detail[0])) - require.Equal(t, "", warnings[1].URL) - require.Equal(t, 1, warnings[1].Level) - - require.Equal(t, "Lint Rule 'NoEmptyContinuations': Empty continuation line (line 7)", string(warnings[2].Short)) - require.Equal(t, "Empty continuation lines will become errors in a future release", string(warnings[2].Detail[0])) - require.Equal(t, "https://github.com/moby/moby/pull/33719", warnings[2].URL) +func testCommandCasing(t *testing.T, sb integration.Sandbox) { + dockerfile := []byte(` +# warning: 'FROM' should be either lowercased or uppercased +From scratch as base +FROM scratch AS base2 +from scratch as base3 +`) + compareLintWarnings(t, sb, dockerfile, []expectedLintWarning{ + { + Short: "Lint Rule 'CommandCasing': Command 'From' should be consistently cased (line 3)", + Detail: "Commands should be in consistent casing (all lower or all upper)", + Level: 1, + }, + }) } // #3495 @@ -7059,6 +7039,82 @@ RUN rm -f /foo-2030.1 } } +func compareLintWarnings(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): + close(done) + } + + <-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) @@ -7429,6 +7485,13 @@ 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/instructions/parse.go b/frontend/dockerfile/instructions/parse.go index 95a54357276ca..37fec6ea3d9b5 100644 --- a/frontend/dockerfile/instructions/parse.go +++ b/frontend/dockerfile/instructions/parse.go @@ -67,14 +67,6 @@ func newParseRequestFromNode(node *parser.Node) parseRequest { } } -func validStageNameCasing(cmdArgs []string) bool { - if len(cmdArgs) != 3 { - return true - } - stageName := cmdArgs[2] - return stageName == strings.ToLower(stageName) -} - func ParseInstruction(node *parser.Node) (v interface{}, err error) { return ParseInstructionWithLinter(node, nil) } @@ -97,10 +89,14 @@ func ParseInstructionWithLinter(node *parser.Node, lintWarn linter.LintWarnFunc) case command.Copy: return parseCopy(req) case command.From: - if lintWarn != nil && !validStageNameCasing(req.args) { + if lintWarn != nil && !isValidStageNameCasing(req.args) { msg := linter.RuleStageNameCasing.Format(req.args[2]) linter.RuleStageNameCasing.Run(lintWarn, node.Location(), msg) } + if lintWarn != nil && !doesFromCaseMatchAsCase(req) { + msg := linter.RuleFromAsCasing.Format(req.command, req.args[1]) + linter.RuleFromAsCasing.Run(lintWarn, node.Location(), msg) + } return parseFrom(req) case command.Onbuild: return parseOnBuild(req) @@ -832,3 +828,41 @@ func allInstructionNames() []string { } return out } + +func isLowerCased(s string) bool { + return s == strings.ToLower(s) +} + +func isUpperCased(s string) bool { + return s == strings.ToUpper(s) +} + +func isConsistentCasing(s string) bool { + return isLowerCased(s) || isUpperCased(s) +} + +func isValidStageNameCasing(cmdArgs []string) bool { + if len(cmdArgs) != 3 { + return true + } + stageName := cmdArgs[2] + return isLowerCased(stageName) +} + +func doesFromCaseMatchAsCase(req parseRequest) bool { + if len(req.args) < 3 { + return true + } + // consistent casing for the command is handled elsewhere. + // If the command is not consistent, there's no need to + // add an additional lint warning for the `as` argument. + if !isConsistentCasing(req.command) { + return true + } + var lowerCaseCmd = isLowerCased(req.command) + if lowerCaseCmd { + return isLowerCased(req.args[1]) + } else { + return isUpperCased(req.args[1]) + } +} diff --git a/frontend/dockerfile/linter/ruleset.go b/frontend/dockerfile/linter/ruleset.go index c1203fd967de2..3a3cd5ff6b579 100644 --- a/frontend/dockerfile/linter/ruleset.go +++ b/frontend/dockerfile/linter/ruleset.go @@ -12,6 +12,13 @@ var ( return fmt.Sprintf("Stage name '%s' should be lowercase", stageName) }, } + RuleFromAsCasing = LinterRule[func(string, string) string]{ + Name: "FromAsCasing", + Description: "The 'as' keyword should match the case of the 'from' keyword", + Format: func(from, as string) string { + return fmt.Sprintf("'%s' and '%s' keywords' casing do not match", as, from) + }, + } RuleNoEmptyContinuations = LinterRule[func() string]{ Name: "NoEmptyContinuations", Description: "Empty continuation lines will become errors in a future release",