From 362ef6dd9de764fafecd9e5f87fad555fd89074b Mon Sep 17 00:00:00 2001 From: Talon Bowler Date: Mon, 27 May 2024 22:24:55 -0700 Subject: [PATCH] Adds controls for checking dockerfile lint rules Signed-off-by: Talon Bowler --- frontend/dockerfile/dockerfile2llb/convert.go | 148 +++++++++++++----- frontend/dockerfile/dockerfile_lint_test.go | 114 +++++++++++++- frontend/dockerfile/instructions/parse.go | 28 ++-- frontend/dockerfile/linter/linter.go | 67 +++++++- frontend/dockerfile/parser/directives.go | 32 ++-- frontend/dockerfile/parser/directives_test.go | 65 ++++++++ frontend/dockerui/config.go | 6 + 7 files changed, 387 insertions(+), 73 deletions(-) diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 6e79a3aaf2196..4e199596538ce 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -162,6 +162,70 @@ func ListTargets(ctx context.Context, dt []byte) (*targets.List, error) { return l, nil } +func parseLintOptions(checkStr string) (*linter.Config, error) { + checkStr = strings.TrimSpace(checkStr) + if checkStr == "" { + return &linter.Config{}, nil + } + + parts := strings.SplitN(checkStr, ";", 2) + var skipSet []string + var errorOnWarn, skipAll bool + for _, p := range parts { + k, v, ok := strings.Cut(p, "=") + if !ok { + return nil, errors.Errorf("invalid check option %q", p) + } + k = strings.TrimSpace(k) + switch k { + case "skip": + v = strings.TrimSpace(v) + if v == "all" { + skipAll = true + } else { + skipSet = strings.Split(v, ",") + for i, rule := range skipSet { + skipSet[i] = strings.TrimSpace(rule) + } + } + case "error": + v, err := strconv.ParseBool(strings.TrimSpace(v)) + if err != nil { + return nil, errors.Wrapf(err, "failed to parse check option %q", p) + } + errorOnWarn = v + default: + return nil, errors.Errorf("invalid check option %q", k) + } + } + return &linter.Config{ + SkipRules: skipSet, + SkipAll: skipAll, + ReturnAsError: errorOnWarn, + }, nil +} + +func newRuleLinter(dt []byte, opt *ConvertOpt) (*linter.Linter, error) { + var lintOptionStr *string + if opt.Client != nil && opt.Client.LinterConfig != nil { + lintOptionStr = opt.Client.LinterConfig + } + if lintOptionStr == nil { + if dfLintOption, _, _, ok := parser.ParseDirective("check", dt); ok { + lintOptionStr = &dfLintOption + } else { + empty := "" + lintOptionStr = &empty + } + } + lintConfig, err := parseLintOptions(*lintOptionStr) + if err != nil { + return nil, errors.Wrapf(err, "failed to parse check options") + } + lintConfig.Warn = opt.Warn + return linter.New(lintConfig), nil +} + func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchState, error) { if len(dt) == 0 { return nil, errors.Errorf("the Dockerfile cannot be empty") @@ -184,8 +248,9 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS return nil, nil, nil } - if opt.Warn == nil { - opt.Warn = func(string, string, string, string, []parser.Range) {} + lint, err := newRuleLinter(dt, &opt) + if err != nil { + return nil, err } if opt.Client != nil && opt.LLBCaps == nil { @@ -214,19 +279,19 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS if warning.URL == linter.RuleNoEmptyContinuations.URL { location := []parser.Range{*warning.Location} msg := linter.RuleNoEmptyContinuations.Format() - linter.RuleNoEmptyContinuations.Run(opt.Warn, location, msg) + lint.Run(&linter.RuleNoEmptyContinuations, location, msg) } } - validateCommandCasing(dockerfile, opt.Warn) + validateCommandCasing(dockerfile, lint) proxyEnv := proxyEnvFromBuildArgs(opt.BuildArgs) - stages, metaArgs, err := instructions.Parse(dockerfile.AST, opt.Warn) + stages, metaArgs, err := instructions.Parse(dockerfile.AST, lint) if err != nil { return nil, err } - validateStageNames(stages, opt.Warn) + validateStageNames(stages, lint) shlex := shell.NewLex(dockerfile.EscapeToken) outline := newOutlineCapture() @@ -264,7 +329,7 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS // set base state for every image for i, st := range stages { nameMatch, err := shlex.ProcessWordWithMatches(st.BaseName, metaArgsToMap(optMetaArgs)) - reportUnusedFromArgs(metaArgsKeys(optMetaArgs), nameMatch.Unmatched, st.Location, opt.Warn) + reportUnusedFromArgs(metaArgsKeys(optMetaArgs), nameMatch.Unmatched, st.Location, lint) used := nameMatch.Matched if err != nil { @@ -288,7 +353,7 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS if v := st.Platform; v != "" { platMatch, err := shlex.ProcessWordWithMatches(v, metaArgsToMap(optMetaArgs)) - reportUnusedFromArgs(metaArgsKeys(optMetaArgs), platMatch.Unmatched, st.Location, opt.Warn) + reportUnusedFromArgs(metaArgsKeys(optMetaArgs), platMatch.Unmatched, st.Location, lint) if err != nil { return nil, parser.WithLocation(errors.Wrapf(err, "failed to process arguments for platform %s", platMatch.Result), st.Location) @@ -542,7 +607,7 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS llb.WithCustomName(prefixCommand(d, "FROM "+d.stage.BaseName, opt.MultiPlatformRequested, platform, nil)), location(opt.SourceMap, d.stage.Location), ) - validateBaseImagePlatform(origName, *platform, d.image.Platform, d.stage.Location, opt.Warn) + validateBaseImagePlatform(origName, *platform, d.image.Platform, d.stage.Location, lint) } d.platform = platform return nil @@ -614,7 +679,7 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS cgroupParent: opt.CgroupParent, llbCaps: opt.LLBCaps, sourceMap: opt.SourceMap, - lintWarn: opt.Warn, + lint: lint, } if err = dispatchOnBuildTriggers(d, d.image.Config.OnBuild, opt); err != nil { @@ -658,6 +723,14 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS target.image.Config.Labels[k] = v } + // If lint.Error() returns an error, it means that + // there were warnings, and that our linter has been + // configured to return an error on warnings, + // so we appropriately return that error here. + if err := lint.Error(); err != nil { + return nil, err + } + opts := filterPaths(ctxPaths) bctx := opt.MainContext if opt.Client != nil { @@ -758,7 +831,7 @@ type dispatchOpt struct { cgroupParent string llbCaps *apicaps.CapSet sourceMap *llb.SourceMap - lintWarn linter.LintWarnFunc + lint *linter.Linter } func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error { @@ -839,11 +912,11 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error { case *instructions.OnbuildCommand: err = dispatchOnbuild(d, c) case *instructions.CmdCommand: - err = dispatchCmd(d, c, opt.lintWarn) + err = dispatchCmd(d, c, opt.lint) case *instructions.EntrypointCommand: - err = dispatchEntrypoint(d, c, opt.lintWarn) + err = dispatchEntrypoint(d, c, opt.lint) case *instructions.HealthCheckCommand: - err = dispatchHealthcheck(d, c, opt.lintWarn) + err = dispatchHealthcheck(d, c, opt.lint) case *instructions.ExposeCommand: err = dispatchExpose(d, c, opt.shlex) case *instructions.UserCommand: @@ -1183,7 +1256,7 @@ func dispatchWorkdir(d *dispatchState, c *instructions.WorkdirCommand, commit bo // by fixing the first one. if !d.workdirSet && !system.IsAbs(c.Path, d.platform.OS) { msg := linter.RuleWorkdirRelativePath.Format(c.Path) - linter.RuleWorkdirRelativePath.Run(opt.lintWarn, c.Location(), msg) + opt.lint.Run(&linter.RuleWorkdirRelativePath, c.Location(), msg) } d.workdirSet = true } @@ -1499,14 +1572,14 @@ func dispatchOnbuild(d *dispatchState, c *instructions.OnbuildCommand) error { return nil } -func dispatchCmd(d *dispatchState, c *instructions.CmdCommand, warn linter.LintWarnFunc) error { - validateUsedOnce(c, &d.cmd, warn) +func dispatchCmd(d *dispatchState, c *instructions.CmdCommand, lint *linter.Linter) error { + validateUsedOnce(c, &d.cmd, lint) var args []string = c.CmdLine if c.PrependShell { if len(d.image.Config.Shell) == 0 { msg := linter.RuleJSONArgsRecommended.Format(c.Name()) - linter.RuleJSONArgsRecommended.Run(warn, c.Location(), msg) + lint.Run(&linter.RuleJSONArgsRecommended, c.Location(), msg) } args = withShell(d.image, args) } @@ -1515,14 +1588,14 @@ func dispatchCmd(d *dispatchState, c *instructions.CmdCommand, warn linter.LintW return commitToHistory(&d.image, fmt.Sprintf("CMD %q", args), false, nil, d.epoch) } -func dispatchEntrypoint(d *dispatchState, c *instructions.EntrypointCommand, warn linter.LintWarnFunc) error { - validateUsedOnce(c, &d.entrypoint, warn) +func dispatchEntrypoint(d *dispatchState, c *instructions.EntrypointCommand, lint *linter.Linter) error { + validateUsedOnce(c, &d.entrypoint, lint) var args []string = c.CmdLine if c.PrependShell { if len(d.image.Config.Shell) == 0 { msg := linter.RuleJSONArgsRecommended.Format(c.Name()) - linter.RuleJSONArgsRecommended.Run(warn, c.Location(), msg) + lint.Run(&linter.RuleJSONArgsRecommended, c.Location(), msg) } args = withShell(d.image, args) } @@ -1533,8 +1606,8 @@ func dispatchEntrypoint(d *dispatchState, c *instructions.EntrypointCommand, war return commitToHistory(&d.image, fmt.Sprintf("ENTRYPOINT %q", args), false, nil, d.epoch) } -func dispatchHealthcheck(d *dispatchState, c *instructions.HealthCheckCommand, warn linter.LintWarnFunc) error { - validateUsedOnce(c, &d.healthcheck, warn) +func dispatchHealthcheck(d *dispatchState, c *instructions.HealthCheckCommand, lint *linter.Linter) error { + validateUsedOnce(c, &d.healthcheck, lint) d.image.Config.Healthcheck = &dockerspec.HealthcheckConfig{ Test: c.Health.Test, Interval: c.Health.Interval, @@ -2058,7 +2131,7 @@ func isSelfConsistentCasing(s string) bool { return s == strings.ToLower(s) || s == strings.ToUpper(s) } -func validateCommandCasing(dockerfile *parser.Result, warn linter.LintWarnFunc) { +func validateCommandCasing(dockerfile *parser.Result, lint *linter.Linter) { var lowerCount, upperCount int for _, node := range dockerfile.AST.Children { if isSelfConsistentCasing(node.Value) { @@ -2077,7 +2150,7 @@ func validateCommandCasing(dockerfile *parser.Result, warn linter.LintWarnFunc) // command to the casing of the majority of commands. if !isSelfConsistentCasing(node.Value) { msg := linter.RuleSelfConsistentCommandCasing.Format(node.Value) - linter.RuleSelfConsistentCommandCasing.Run(warn, node.Location(), msg) + lint.Run(&linter.RuleSelfConsistentCommandCasing, node.Location(), msg) } else { var msg string var needsLintWarn bool @@ -2089,7 +2162,7 @@ func validateCommandCasing(dockerfile *parser.Result, warn linter.LintWarnFunc) needsLintWarn = true } if needsLintWarn { - linter.RuleFileConsistentCommandCasing.Run(warn, node.Location(), msg) + lint.Run(&linter.RuleFileConsistentCommandCasing, node.Location(), msg) } } } @@ -2100,18 +2173,18 @@ var reservedStageNames = map[string]struct{}{ "scratch": {}, } -func validateStageNames(stages []instructions.Stage, warn linter.LintWarnFunc) { +func validateStageNames(stages []instructions.Stage, lint *linter.Linter) { stageNames := make(map[string]struct{}) for _, stage := range stages { if stage.Name != "" { if _, ok := reservedStageNames[stage.Name]; ok { msg := linter.RuleReservedStageName.Format(stage.Name) - linter.RuleReservedStageName.Run(warn, stage.Location, msg) + lint.Run(&linter.RuleReservedStageName, stage.Location, msg) } if _, ok := stageNames[stage.Name]; ok { msg := linter.RuleDuplicateStageName.Format(stage.Name) - linter.RuleDuplicateStageName.Run(warn, stage.Location, msg) + lint.Run(&linter.RuleDuplicateStageName, stage.Location, msg) } stageNames[stage.Name] = struct{}{} } @@ -2119,6 +2192,9 @@ func validateStageNames(stages []instructions.Stage, warn linter.LintWarnFunc) { } func reportUnmatchedVariables(cmd instructions.Command, buildArgs []instructions.KeyValuePairOptional, env []string, unmatched map[string]struct{}, opt *dispatchOpt) { + if len(unmatched) == 0 { + return + } for _, buildArg := range buildArgs { delete(unmatched, buildArg.Key) } @@ -2136,7 +2212,7 @@ func reportUnmatchedVariables(cmd instructions.Command, buildArgs []instructions } match, _ := suggest.Search(cmdVar, options, true) msg := linter.RuleUndefinedVar.Format(cmdVar, match) - linter.RuleUndefinedVar.Run(opt.lintWarn, cmd.Location(), msg) + opt.lint.Run(&linter.RuleUndefinedVar, cmd.Location(), msg) } } @@ -2190,11 +2266,11 @@ func toPBLocation(sourceIndex int, location []parser.Range) pb.Location { } } -func reportUnusedFromArgs(values []string, unmatched map[string]struct{}, location []parser.Range, warn linter.LintWarnFunc) { +func reportUnusedFromArgs(values []string, unmatched map[string]struct{}, location []parser.Range, lint *linter.Linter) { for arg := range unmatched { suggest, _ := suggest.Search(arg, values, true) msg := linter.RuleUndeclaredArgInFrom.Format(arg, suggest) - linter.RuleUndeclaredArgInFrom.Run(warn, location, msg) + lint.Run(&linter.RuleUndeclaredArgInFrom, location, msg) } } @@ -2208,12 +2284,12 @@ func (v *instructionTracker) MarkUsed(loc []parser.Range) { v.IsSet = true } -func validateUsedOnce(c instructions.Command, loc *instructionTracker, warn linter.LintWarnFunc) { +func validateUsedOnce(c instructions.Command, loc *instructionTracker, lint *linter.Linter) { if loc.IsSet { msg := linter.RuleMultipleInstructionsDisallowed.Format(c.Name()) // Report the location of the previous invocation because it is the one // that will be ignored. - linter.RuleMultipleInstructionsDisallowed.Run(warn, loc.Loc, msg) + lint.Run(&linter.RuleMultipleInstructionsDisallowed, loc.Loc, msg) } loc.MarkUsed(c.Location()) } @@ -2229,11 +2305,11 @@ func wrapSuggestAny(err error, keys map[string]struct{}, options []string) error return err } -func validateBaseImagePlatform(name string, expected, actual ocispecs.Platform, location []parser.Range, warn linter.LintWarnFunc) { +func validateBaseImagePlatform(name string, expected, actual ocispecs.Platform, location []parser.Range, lint *linter.Linter) { if expected.OS != actual.OS || expected.Architecture != actual.Architecture { expectedStr := platforms.Format(platforms.Normalize(expected)) actualStr := platforms.Format(platforms.Normalize(actual)) msg := linter.RuleInvalidBaseImagePlatform.Format(name, expectedStr, actualStr) - linter.RuleInvalidBaseImagePlatform.Run(warn, location, msg) + lint.Run(&linter.RuleInvalidBaseImagePlatform, location, msg) } } diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index f456048d8789b..b9a98f2842731 100644 --- a/frontend/dockerfile/dockerfile_lint_test.go +++ b/frontend/dockerfile/dockerfile_lint_test.go @@ -23,6 +23,7 @@ import ( ) var lintTests = integration.TestFuncs( + testRuleCheckOption, testStageName, testNoEmptyContinuations, testSelfConsistentCommandCasing, @@ -40,6 +41,103 @@ var lintTests = integration.TestFuncs( testBaseImagePlatformMismatch, ) +func testRuleCheckOption(t *testing.T, sb integration.Sandbox) { + dockerfile := []byte(`#check=skip=all +FROM scratch as base +copy Dockerfile . +`) + checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) + + dockerfile = []byte(`#check=skip=all;error=true +FROM scratch as base +copy Dockerfile . +`) + checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) + + dockerfile = []byte(`#check=skip=FileConsistentCommandCasing,FromAsCasing +FROM scratch as base +copy Dockerfile . +`) + checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) + + dockerfile = []byte(`#check=skip=FileConsistentCommandCasing,FromAsCasing;error=true +FROM scratch as base +copy Dockerfile . +`) + checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) + + dockerfile = []byte(`#check=skip=FileConsistentCommandCasing +FROM scratch as base +copy Dockerfile . +`) + 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: 2, + Level: 1, + }, + }, + }) + + dockerfile = []byte(`#check=skip=FileConsistentCommandCasing;error=true +FROM scratch as base +copy Dockerfile . +`) + 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: 2, + Level: 1, + }, + }, + StreamBuildErr: "failed to solve: lint violation found for rules: FromAsCasing", + UnmarshalBuildErr: "lint violation found for rules: FromAsCasing", + BuildErrLocation: 2, + }) + + dockerfile = []byte(`#check=skip=all +FROM scratch as base +copy Dockerfile . +`) + 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: 2, + Level: 1, + }, + }, + StreamBuildErr: "failed to solve: lint violation found for rules: FromAsCasing", + UnmarshalBuildErr: "lint violation found for rules: FromAsCasing", + BuildErrLocation: 2, + FrontendAttrs: map[string]string{ + "build-arg:BUILDKIT_DOCKERFILE_CHECK": "skip=FileConsistentCommandCasing;error=true", + }, + }) + + dockerfile = []byte(`#check=error=true +FROM scratch as base +copy Dockerfile . +`) + checkLinterWarnings(t, sb, &lintTestParams{ + Dockerfile: dockerfile, + FrontendAttrs: map[string]string{ + "build-arg:BUILDKIT_DOCKERFILE_CHECK": "skip=all", + }, + }) +} + func testStageName(t *testing.T, sb integration.Sandbox) { dockerfile := []byte(` # warning: stage name should be lowercase @@ -730,12 +828,16 @@ func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestPara called := false frontend := func(ctx context.Context, c gateway.Client) (*gateway.Result, error) { + frontendOpts := map[string]string{ + "frontend.caps": "moby.buildkit.frontend.subrequests", + "requestid": "frontend.lint", + } + for k, v := range lintTest.FrontendAttrs { + frontendOpts[k] = v + } res, err := c.Solve(ctx, gateway.SolveRequest{ - FrontendOpt: map[string]string{ - "frontend.caps": "moby.buildkit.frontend.subrequests", - "requestid": "frontend.lint", - }, - Frontend: "dockerfile.v0", + FrontendOpt: frontendOpts, + Frontend: "dockerfile.v0", }) if err != nil { return nil, err @@ -746,7 +848,6 @@ func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestPara 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.Greater(t, lintResults.Error.Location.SourceIndex, int32(-1)) require.Less(t, lintResults.Error.Location.SourceIndex, int32(len(lintResults.Sources))) } @@ -827,7 +928,6 @@ func checkProgressStream(t *testing.T, sb integration.Sandbox, lintTest *lintTes t.Fatalf("timed out waiting for statusDone") } - // two platforms only show one warning require.Equal(t, len(lintTest.Warnings), len(warnings)) sort.Slice(warnings, func(i, j int) bool { w1 := warnings[i] diff --git a/frontend/dockerfile/instructions/parse.go b/frontend/dockerfile/instructions/parse.go index 8fb2b2d5a6fd1..9c1c7ea379b92 100644 --- a/frontend/dockerfile/instructions/parse.go +++ b/frontend/dockerfile/instructions/parse.go @@ -71,34 +71,34 @@ func ParseInstruction(node *parser.Node) (v interface{}, err error) { } // ParseInstruction converts an AST to a typed instruction (either a command or a build stage beginning when encountering a `FROM` statement) -func ParseInstructionWithLinter(node *parser.Node, lintWarn linter.LintWarnFunc) (v interface{}, err error) { +func ParseInstructionWithLinter(node *parser.Node, lint *linter.Linter) (v interface{}, err error) { defer func() { err = parser.WithLocation(err, node.Location()) }() req := newParseRequestFromNode(node) switch strings.ToLower(node.Value) { case command.Env: - return parseEnv(req, lintWarn) + return parseEnv(req, lint) case command.Maintainer: - if lintWarn != nil { + if lint != nil { msg := linter.RuleMaintainerDeprecated.Format() - linter.RuleMaintainerDeprecated.Run(lintWarn, node.Location(), msg) + lint.Run(&linter.RuleMaintainerDeprecated, node.Location(), msg) } return parseMaintainer(req) case command.Label: - return parseLabel(req, lintWarn) + return parseLabel(req, lint) case command.Add: return parseAdd(req) case command.Copy: return parseCopy(req) case command.From: - if lintWarn != nil && !isLowerCaseStageName(req.args) { + if lint != nil && !isLowerCaseStageName(req.args) { msg := linter.RuleStageNameCasing.Format(req.args[2]) - linter.RuleStageNameCasing.Run(lintWarn, node.Location(), msg) + lint.Run(&linter.RuleStageNameCasing, node.Location(), msg) } - if lintWarn != nil && !doesFromCaseMatchAsCase(req) { + if lint != nil && !doesFromCaseMatchAsCase(req) { msg := linter.RuleFromAsCasing.Format(req.command, req.args[1]) - linter.RuleFromAsCasing.Run(lintWarn, node.Location(), msg) + lint.Run(&linter.RuleFromAsCasing, node.Location(), msg) } return parseFrom(req) case command.Onbuild: @@ -166,7 +166,7 @@ func (e *parseError) Unwrap() error { // Parse a Dockerfile into a collection of buildable stages. // metaArgs is a collection of ARG instructions that occur before the first FROM. -func Parse(ast *parser.Node, lint linter.LintWarnFunc) (stages []Stage, metaArgs []ArgCommand, err error) { +func Parse(ast *parser.Node, lint *linter.Linter) (stages []Stage, metaArgs []ArgCommand, err error) { for _, n := range ast.Children { cmd, err := ParseInstructionWithLinter(n, lint) if err != nil { @@ -195,7 +195,7 @@ func Parse(ast *parser.Node, lint linter.LintWarnFunc) (stages []Stage, metaArgs return stages, metaArgs, nil } -func parseKvps(args []string, cmdName string, location []parser.Range, lint linter.LintWarnFunc) (KeyValuePairs, error) { +func parseKvps(args []string, cmdName string, location []parser.Range, lint *linter.Linter) (KeyValuePairs, error) { if len(args) == 0 { return nil, errAtLeastOneArgument(cmdName) } @@ -211,14 +211,14 @@ func parseKvps(args []string, cmdName string, location []parser.Range, lint lint name, value, sep := args[j], args[j+1], args[j+2] if sep == "" { msg := linter.RuleLegacyKeyValueFormat.Format(cmdName) - linter.RuleLegacyKeyValueFormat.Run(lint, location, msg) + lint.Run(&linter.RuleLegacyKeyValueFormat, location, msg) } res = append(res, KeyValuePair{Key: name, Value: value}) } return res, nil } -func parseEnv(req parseRequest, lint linter.LintWarnFunc) (*EnvCommand, error) { +func parseEnv(req parseRequest, lint *linter.Linter) (*EnvCommand, error) { if err := req.flags.Parse(); err != nil { return nil, err } @@ -246,7 +246,7 @@ func parseMaintainer(req parseRequest) (*MaintainerCommand, error) { }, nil } -func parseLabel(req parseRequest, lint linter.LintWarnFunc) (*LabelCommand, error) { +func parseLabel(req parseRequest, lint *linter.Linter) (*LabelCommand, error) { if err := req.flags.Parse(); err != nil { return nil, err } diff --git a/frontend/dockerfile/linter/linter.go b/frontend/dockerfile/linter/linter.go index e9f058ad474f2..9fc5c96ac5723 100644 --- a/frontend/dockerfile/linter/linter.go +++ b/frontend/dockerfile/linter/linter.go @@ -5,8 +5,69 @@ import ( "strings" "github.com/moby/buildkit/frontend/dockerfile/parser" + "github.com/pkg/errors" ) +type Config struct { + Warn LintWarnFunc + SkipRules []string + SkipAll bool + ReturnAsError bool +} + +type Linter struct { + SkippedRules map[string]struct{} + CalledRules map[string]struct{} + SkipAll bool + ReturnAsError bool + Warn LintWarnFunc +} + +func New(config *Config) *Linter { + toret := &Linter{ + SkippedRules: map[string]struct{}{}, + CalledRules: map[string]struct{}{}, + Warn: config.Warn, + } + toret.SkipAll = config.SkipAll + toret.ReturnAsError = config.ReturnAsError + for _, rule := range config.SkipRules { + toret.SkippedRules[rule] = struct{}{} + } + return toret +} + +func (lc *Linter) Run(rule LinterRuleI, location []parser.Range, txt ...string) { + if lc.Warn == nil || lc.SkipAll { + return + } + rulename := rule.RuleName() + if _, ok := lc.SkippedRules[rulename]; ok { + return + } + lc.CalledRules[rulename] = struct{}{} + rule.Run(lc.Warn, location, txt...) +} + +func (lc *Linter) Error() error { + if !lc.ReturnAsError { + return nil + } + if len(lc.CalledRules) == 0 { + return nil + } + var rules []string + for r := range lc.CalledRules { + rules = append(rules, r) + } + return errors.Errorf("lint violation found for rules: %s", strings.Join(rules, ", ")) +} + +type LinterRuleI interface { + RuleName() string + Run(warn LintWarnFunc, location []parser.Range, txt ...string) +} + type LinterRule[F any] struct { Name string Description string @@ -14,7 +75,11 @@ type LinterRule[F any] struct { Format F } -func (rule LinterRule[F]) Run(warn LintWarnFunc, location []parser.Range, txt ...string) { +func (rule *LinterRule[F]) RuleName() string { + return rule.Name +} + +func (rule *LinterRule[F]) Run(warn LintWarnFunc, location []parser.Range, txt ...string) { if len(txt) == 0 { txt = []string{rule.Description} } diff --git a/frontend/dockerfile/parser/directives.go b/frontend/dockerfile/parser/directives.go index db1668f252bf5..651b13148c3a5 100644 --- a/frontend/dockerfile/parser/directives.go +++ b/frontend/dockerfile/parser/directives.go @@ -13,12 +13,14 @@ import ( const ( keySyntax = "syntax" + keyCheck = "check" keyEscape = "escape" ) var validDirectives = map[string]struct{}{ keySyntax: {}, keyEscape: {}, + keyCheck: {}, } type Directive struct { @@ -110,6 +112,10 @@ func (d *DirectiveParser) ParseAll(data []byte) ([]*Directive, error) { // This allows for a flexible range of input formats, and appropriate syntax // selection. func DetectSyntax(dt []byte) (string, string, []Range, bool) { + return ParseDirective(keySyntax, dt) +} + +func ParseDirective(key string, dt []byte) (string, string, []Range, bool) { dt, hadShebang, err := discardShebang(dt) if err != nil { return "", "", nil, false @@ -119,42 +125,38 @@ func DetectSyntax(dt []byte) (string, string, []Range, bool) { line++ } - // use default directive parser, and search for #syntax= + // use default directive parser, and search for #key= directiveParser := DirectiveParser{line: line} - if syntax, cmdline, loc, ok := detectSyntaxFromParser(dt, directiveParser); ok { + if syntax, cmdline, loc, ok := detectDirectiveFromParser(key, dt, directiveParser); ok { return syntax, cmdline, loc, true } - // use directive with different comment prefix, and search for //syntax= + // use directive with different comment prefix, and search for //key= directiveParser = DirectiveParser{line: line} directiveParser.setComment("//") - if syntax, cmdline, loc, ok := detectSyntaxFromParser(dt, directiveParser); ok { + if syntax, cmdline, loc, ok := detectDirectiveFromParser(key, dt, directiveParser); ok { return syntax, cmdline, loc, true } - // search for possible json directives - var directive struct { - Syntax string `json:"syntax"` - } - if err := json.Unmarshal(dt, &directive); err == nil { - if directive.Syntax != "" { + // use json directive, and search for { "key": "..." } + jsonDirective := map[string]string{} + if err := json.Unmarshal(dt, &jsonDirective); err == nil { + if v, ok := jsonDirective[key]; ok { loc := []Range{{ Start: Position{Line: line}, End: Position{Line: line}, }} - return directive.Syntax, directive.Syntax, loc, true + return v, v, loc, true } } return "", "", nil, false } -func detectSyntaxFromParser(dt []byte, parser DirectiveParser) (string, string, []Range, bool) { +func detectDirectiveFromParser(key string, dt []byte, parser DirectiveParser) (string, string, []Range, bool) { directives, _ := parser.ParseAll(dt) for _, d := range directives { - // check for syntax directive before erroring out, since the error - // might have occurred *after* the syntax directive - if d.Name == keySyntax { + if d.Name == key { p, _, _ := strings.Cut(d.Value, " ") return p, d.Value, d.Location, true } diff --git a/frontend/dockerfile/parser/directives_test.go b/frontend/dockerfile/parser/directives_test.go index 31077977b2d26..74a1fe9dcc689 100644 --- a/frontend/dockerfile/parser/directives_test.go +++ b/frontend/dockerfile/parser/directives_test.go @@ -103,3 +103,68 @@ RUN ls _, _, _, ok = DetectSyntax([]byte(dt)) require.False(t, ok) } + +func TestParseDirective(t *testing.T) { + t.Parallel() + + dt := `#check = skip=all // opts +FROM busybox +` + ref, cmdline, loc, ok := ParseDirective("check", []byte(dt)) + require.True(t, ok) + require.Equal(t, ref, "skip=all") + require.Equal(t, cmdline, "skip=all // opts") + require.Equal(t, 1, loc[0].Start.Line) + require.Equal(t, 1, loc[0].End.Line) + + dt = `#!/bin/sh +# check = skip=all +FROM busybox +` + ref, _, loc, ok = ParseDirective("check", []byte(dt)) + require.True(t, ok) + require.Equal(t, ref, "skip=all") + require.Equal(t, 2, loc[0].Start.Line) + require.Equal(t, 2, loc[0].End.Line) + + dt = `#!/bin/sh + +# check = skip=all +` + _, _, _, ok = ParseDirective("check", []byte(dt)) + require.False(t, ok) + + dt = `FROM busybox +RUN ls +` + ref, cmdline, _, ok = ParseDirective("check", []byte(dt)) + require.False(t, ok) + require.Equal(t, ref, "") + require.Equal(t, cmdline, "") + + dt = `//check=skip=all +//key=value` + ref, _, _, ok = ParseDirective("check", []byte(dt)) + require.True(t, ok) + require.Equal(t, ref, "skip=all") + + dt = `#!/bin/sh +//check=skip=all` + ref, _, _, ok = ParseDirective("check", []byte(dt)) + require.True(t, ok) + require.Equal(t, ref, "skip=all") + + dt = `{"check": "skip=all"}` + ref, _, _, ok = ParseDirective("check", []byte(dt)) + require.True(t, ok) + require.Equal(t, ref, "skip=all") + + dt = `{"check": "foo"` + _, _, _, ok = ParseDirective("check", []byte(dt)) + require.False(t, ok) + + dt = `{"check": "foo"} +# syntax=bar` + _, _, _, ok = ParseDirective("check", []byte(dt)) + require.False(t, ok) +} diff --git a/frontend/dockerui/config.go b/frontend/dockerui/config.go index 476c9faf69e4b..d99b3affa35f8 100644 --- a/frontend/dockerui/config.go +++ b/frontend/dockerui/config.go @@ -47,6 +47,7 @@ const ( keyCacheNSArg = "build-arg:BUILDKIT_CACHE_MOUNT_NS" keyMultiPlatformArg = "build-arg:BUILDKIT_MULTI_PLATFORM" keyHostnameArg = "build-arg:BUILDKIT_SANDBOX_HOSTNAME" + keyDockerfileLintArg = "build-arg:BUILDKIT_DOCKERFILE_CHECK" keyContextKeepGitDirArg = "build-arg:BUILDKIT_CONTEXT_KEEP_GIT_DIR" keySourceDateEpoch = "build-arg:SOURCE_DATE_EPOCH" ) @@ -64,6 +65,7 @@ type Config struct { ShmSize int64 Target string Ulimits []pb.Ulimit + LinterConfig *string CacheImports []client.CacheOptionsEntry TargetPlatforms []ocispecs.Platform // nil means default @@ -277,6 +279,10 @@ func (bc *Client) init() error { opts[keyHostname] = v } bc.Hostname = opts[keyHostname] + + if v, ok := opts[keyDockerfileLintArg]; ok { + bc.LinterConfig = &v + } return nil }