From 75d1f1fe8d89f0dfc5eb02456e2dfc0b7293c050 Mon Sep 17 00:00:00 2001 From: Talon Bowler Date: Wed, 29 May 2024 13:28:40 -0700 Subject: [PATCH] refactor the linter functionality to be based around passing a ruleChecker object rather than a callback Signed-off-by: Talon Bowler --- frontend/dockerfile/builder/build.go | 50 ++++----- frontend/dockerfile/dockerfile2llb/convert.go | 100 +++++++++++------- frontend/dockerfile/dockerfile_lint_test.go | 50 +++++++++ frontend/dockerfile/instructions/parse.go | 18 ++-- frontend/dockerfile/linter/linter.go | 82 ++++++++++---- frontend/dockerfile/linter/ruleset.go | 17 --- frontend/dockerfile/parser/directives.go | 32 +++--- frontend/dockerfile/parser/directives_test.go | 34 +++--- 8 files changed, 238 insertions(+), 145 deletions(-) diff --git a/frontend/dockerfile/builder/build.go b/frontend/dockerfile/builder/build.go index 551157a9d8ad4..fe0edc8624830 100644 --- a/frontend/dockerfile/builder/build.go +++ b/frontend/dockerfile/builder/build.go @@ -50,7 +50,6 @@ func Build(ctx context.Context, c client.Client) (_ *client.Result, err error) { return nil, err } - var errorFromLint bool if _, ok := opts["cmdline"]; !ok { if cmdline, ok := opts[keySyntaxArg]; ok { p := strings.SplitN(strings.TrimSpace(cmdline), " ", 2) @@ -66,42 +65,41 @@ func Build(ctx context.Context, c client.Client) (_ *client.Result, err error) { } return res, err } + } - var lintOptionStr string - if cmdline, ok := opts[keyDockerfileLintArg]; ok { - lintOptionStr = cmdline - } else if _, cmdline, _, ok := parser.DetectLint(src.Data); ok { - lintOptionStr = cmdline + var checkOptionStr string + var ruleChecker *linter.RuleChecker + if cmdline, ok := opts[keyDockerfileLintArg]; ok { + checkOptionStr = cmdline + } + if checkOptionStr != "" { + ruleChecker, err = linter.NewRuleCheckerFromStr(checkOptionStr) + ruleChecker.FromBuildArg = true + if err != nil { + return nil, errors.Wrapf(err, "failed to parse lint options") } - if lintOptionStr != "" { - skipSet, errorOn, err := parser.ParseLintOptions(lintOptionStr) - if err != nil { - return nil, errors.Wrapf(err, "failed to parse lint options") - } - linter.SetLintOptions(skipSet) - errorFromLint = errorOn + } else { + ruleChecker = linter.NewRuleChecker() + } + ruleChecker.Warn = func(rulename, description, url, msg string, location []parser.Range) { + startLine := 0 + if len(location) > 0 { + startLine = location[0].Start.Line } + msg = linter.LintFormatShort(rulename, msg, startLine) + src.Warn(ctx, msg, warnOpts(location, [][]byte{[]byte(description)}, url)) } if capsError != nil { return nil, capsError } - violatedRules := []string{} convertOpt := dockerfile2llb.ConvertOpt{ Config: bc.Config, Client: bc, SourceMap: src.SourceMap, MetaResolver: c, - Warn: func(rulename, description, url, msg string, location []parser.Range) { - startLine := 0 - if len(location) > 0 { - startLine = location[0].Start.Line - } - msg = linter.LintFormatShort(rulename, msg, startLine) - src.Warn(ctx, msg, warnOpts(location, [][]byte{[]byte(description)}, url)) - violatedRules = append(violatedRules, rulename) - }, + RuleChecker: ruleChecker, } if res, ok, err := bc.HandleSubrequest(ctx, dockerui.RequestHandler{ @@ -148,7 +146,7 @@ func Build(ctx context.Context, c client.Client) (_ *client.Result, err error) { opt := convertOpt opt.TargetPlatform = platform if idx != 0 { - opt.Warn = nil + opt.RuleChecker = nil } st, img, baseImg, scanTarget, err := dockerfile2llb.Dockerfile2LLB(ctx, src.Data, opt) @@ -156,10 +154,6 @@ func Build(ctx context.Context, c client.Client) (_ *client.Result, err error) { return nil, nil, nil, err } - if errorFromLint && len(violatedRules) > 0 { - return nil, nil, nil, errors.Errorf("violated lint rules: %s", strings.Join(violatedRules, ", ")) - } - def, err := st.Marshal(ctx) if err != nil { return nil, nil, nil, errors.Wrapf(err, "failed to marshal LLB definition") diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 3f0282c21a3fe..3ec9b396120f8 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -64,7 +64,7 @@ type ConvertOpt struct { TargetPlatform *ocispecs.Platform MetaResolver llb.ImageMetaResolver LLBCaps *apicaps.CapSet - Warn linter.LintWarnFunc + RuleChecker *linter.RuleChecker } type SBOMTargets struct { @@ -114,7 +114,7 @@ func Dockefile2Outline(ctx context.Context, dt []byte, opt ConvertOpt) (*outline func DockerfileLint(ctx context.Context, dt []byte, opt ConvertOpt) (*lint.LintResults, error) { results := &lint.LintResults{} sourceIndex := results.AddSource(opt.SourceMap) - opt.Warn = func(rulename, description, url, fmtmsg string, location []parser.Range) { + opt.RuleChecker.Warn = func(rulename, description, url, fmtmsg string, location []parser.Range) { results.AddWarning(rulename, description, url, fmtmsg, sourceIndex, location) } _, err := toDispatchState(ctx, dt, opt) @@ -171,6 +171,21 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS return nil, errors.Errorf("Client and MainContext cannot both be provided") } + if opt.RuleChecker == nil { + opt.RuleChecker = linter.NewRuleChecker() + } + + if !opt.RuleChecker.FromBuildArg { + if lintOptionStr, _, _, ok := parser.DetectCheck(dt); ok { + lc, err := linter.NewRuleCheckerFromStr(lintOptionStr) + if err != nil { + return nil, errors.Wrapf(err, "failed to parse check options") + } + lc.Warn = opt.RuleChecker.Warn + opt.RuleChecker = lc + } + } + namedContext := func(ctx context.Context, name string, copt dockerui.ContextOpt) (*llb.State, *dockerspec.DockerOCIImage, error) { if opt.Client == nil { return nil, nil, nil @@ -184,8 +199,8 @@ 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) {} + if opt.RuleChecker.Warn == nil { + opt.RuleChecker.Warn = func(string, string, string, string, []parser.Range) {} } if opt.Client != nil && opt.LLBCaps == nil { @@ -214,19 +229,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) + opt.RuleChecker.Run(&linter.RuleNoEmptyContinuations, location, msg) } } - validateCommandCasing(dockerfile, opt.Warn) + validateCommandCasing(dockerfile, opt.RuleChecker) proxyEnv := proxyEnvFromBuildArgs(opt.BuildArgs) - stages, metaArgs, err := instructions.Parse(dockerfile.AST, opt.Warn) + stages, metaArgs, err := instructions.Parse(dockerfile.AST, opt.RuleChecker) if err != nil { return nil, err } - validateStageNames(stages, opt.Warn) + validateStageNames(stages, opt.RuleChecker) shlex := shell.NewLex(dockerfile.EscapeToken) outline := newOutlineCapture() @@ -261,7 +276,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(nameMatch.Unmatched, st.Location, opt.Warn) + reportUnusedFromArgs(nameMatch.Unmatched, st.Location, opt.RuleChecker) used := nameMatch.Matched if err != nil { @@ -285,7 +300,7 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS if v := st.Platform; v != "" { platMatch, err := shlex.ProcessWordWithMatches(v, metaArgsToMap(optMetaArgs)) - reportUnusedFromArgs(platMatch.Unmatched, st.Location, opt.Warn) + reportUnusedFromArgs(platMatch.Unmatched, st.Location, opt.RuleChecker) if err != nil { return nil, parser.WithLocation(errors.Wrapf(err, "failed to process arguments for platform %s", platMatch.Result), st.Location) @@ -600,7 +615,7 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS cgroupParent: opt.CgroupParent, llbCaps: opt.LLBCaps, sourceMap: opt.SourceMap, - lintWarn: opt.Warn, + ruleCheck: opt.RuleChecker, } if err = dispatchOnBuildTriggers(d, d.image.Config.OnBuild, opt); err != nil { @@ -644,6 +659,15 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS target.image.Config.Labels[k] = v } + // If opt.RuleChecker.ViolationError() returns an error, it means that + // error if there were warnings, and that our RuleChecker has been + // configured to return an error on warnings, so we appropriately return + // that error here. + err = opt.RuleChecker.ViolationError() + if err != nil { + return nil, err + } + opts := filterPaths(ctxPaths) bctx := opt.MainContext if opt.Client != nil { @@ -736,7 +760,7 @@ type dispatchOpt struct { cgroupParent string llbCaps *apicaps.CapSet sourceMap *llb.SourceMap - lintWarn linter.LintWarnFunc + ruleCheck *linter.RuleChecker } func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error { @@ -749,7 +773,7 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error { } newword, unmatched, err := opt.shlex.ProcessWord(word, env) - reportUnmatchedVariables(cmd, d.buildArgs, unmatched, &opt) + reportUnmatchedVariables(cmd, d.buildArgs, unmatched, opt.ruleCheck) return newword, err }) if err != nil { @@ -765,7 +789,7 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error { lex := shell.NewLex('\\') lex.SkipProcessQuotes = true newword, unmatched, err := lex.ProcessWord(word, env) - reportUnmatchedVariables(cmd, d.buildArgs, unmatched, &opt) + reportUnmatchedVariables(cmd, d.buildArgs, unmatched, opt.ruleCheck) return newword, err }) if err != nil { @@ -815,11 +839,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.ruleCheck) case *instructions.EntrypointCommand: - err = dispatchEntrypoint(d, c, opt.lintWarn) + err = dispatchEntrypoint(d, c, opt.ruleCheck) case *instructions.HealthCheckCommand: - err = dispatchHealthcheck(d, c, opt.lintWarn) + err = dispatchHealthcheck(d, c, opt.ruleCheck) case *instructions.ExposeCommand: err = dispatchExpose(d, c, opt.shlex) case *instructions.UserCommand: @@ -1159,7 +1183,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.ruleCheck.Run(&linter.RuleWorkdirRelativePath, c.Location(), msg) } d.workdirSet = true } @@ -1475,14 +1499,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, ruleCheck *linter.RuleChecker) error { + validateUsedOnce(c, &d.cmd, ruleCheck) 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) + ruleCheck.Run(&linter.RuleJSONArgsRecommended, c.Location(), msg) } args = withShell(d.image, args) } @@ -1491,14 +1515,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, ruleCheck *linter.RuleChecker) error { + validateUsedOnce(c, &d.entrypoint, ruleCheck) 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) + ruleCheck.Run(&linter.RuleJSONArgsRecommended, c.Location(), msg) } args = withShell(d.image, args) } @@ -1509,8 +1533,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, ruleCheck *linter.RuleChecker) error { + validateUsedOnce(c, &d.healthcheck, ruleCheck) d.image.Config.Healthcheck = &dockerspec.HealthcheckConfig{ Test: c.Health.Test, Interval: c.Health.Interval, @@ -2026,7 +2050,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, ruleChecker *linter.RuleChecker) { var lowerCount, upperCount int for _, node := range dockerfile.AST.Children { if isSelfConsistentCasing(node.Value) { @@ -2045,7 +2069,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) + ruleChecker.Run(&linter.RuleSelfConsistentCommandCasing, node.Location(), msg) } else { var msg string var needsLintWarn bool @@ -2057,7 +2081,7 @@ func validateCommandCasing(dockerfile *parser.Result, warn linter.LintWarnFunc) needsLintWarn = true } if needsLintWarn { - linter.RuleFileConsistentCommandCasing.Run(warn, node.Location(), msg) + ruleChecker.Run(&linter.RuleFileConsistentCommandCasing, node.Location(), msg) } } } @@ -2068,25 +2092,25 @@ var reservedStageNames = map[string]struct{}{ "scratch": {}, } -func validateStageNames(stages []instructions.Stage, warn linter.LintWarnFunc) { +func validateStageNames(stages []instructions.Stage, ruleChecker *linter.RuleChecker) { 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) + ruleChecker.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) + ruleChecker.Run(&linter.RuleDuplicateStageName, stage.Location, msg) } stageNames[stage.Name] = struct{}{} } } } -func reportUnmatchedVariables(cmd instructions.Command, buildArgs []instructions.KeyValuePairOptional, unmatched map[string]struct{}, opt *dispatchOpt) { +func reportUnmatchedVariables(cmd instructions.Command, buildArgs []instructions.KeyValuePairOptional, unmatched map[string]struct{}, ruleChecker *linter.RuleChecker) { if len(unmatched) == 0 { return } @@ -2097,7 +2121,7 @@ func reportUnmatchedVariables(cmd instructions.Command, buildArgs []instructions _, nonEnvOk := nonEnvArgs[cmdVar] if !nonEnvOk { msg := linter.RuleUndefinedVar.Format(cmdVar) - linter.RuleUndefinedVar.Run(opt.lintWarn, cmd.Location(), msg) + ruleChecker.Run(&linter.RuleUndefinedVar, cmd.Location(), msg) } } } @@ -2152,10 +2176,10 @@ func toPBLocation(sourceIndex int, location []parser.Range) pb.Location { } } -func reportUnusedFromArgs(unmatched map[string]struct{}, location []parser.Range, warn linter.LintWarnFunc) { +func reportUnusedFromArgs(unmatched map[string]struct{}, location []parser.Range, ruleChecker *linter.RuleChecker) { for arg := range unmatched { msg := linter.RuleUndeclaredArgInFrom.Format(arg) - linter.RuleUndeclaredArgInFrom.Run(warn, location, msg) + ruleChecker.Run(&linter.RuleUndeclaredArgInFrom, location, msg) } } @@ -2169,12 +2193,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, ruleChecker *linter.RuleChecker) { 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) + ruleChecker.Run(&linter.RuleMultipleInstructionsDisallowed, loc.Loc, msg) } loc.MarkUsed(c.Location()) } diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index 186b63ad88e18..ca476b929b46a 100644 --- a/frontend/dockerfile/dockerfile_lint_test.go +++ b/frontend/dockerfile/dockerfile_lint_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "os" + "path/filepath" "sort" "testing" "time" @@ -489,6 +490,55 @@ COPY Dockerfile . }, }, }) + + testUndeclaredArgEnv(t, sb) +} + +func testUndeclaredArgEnv(t *testing.T, sb integration.Sandbox) { + t.Helper() + c, err := client.New(sb.Context(), sb.Address()) + require.NoError(t, err) + defer c.Close() + + dockerfile := []byte(` +FROM busybox +ARG FOO +ARG BAR= +RUN env | grep FOO > /env_foo || true +RUN env | grep BAR > /env_bar +`) + + dir := integration.Tmpdir( + t, + fstest.CreateFile("Dockerfile", dockerfile, 0600), + ) + + f := getFrontend(t, sb) + + destDir := t.TempDir() + + _, err = f.Solve(sb.Context(), c, client.SolveOpt{ + FrontendAttrs: map[string]string{}, + LocalMounts: map[string]fsutil.FS{ + dockerui.DefaultLocalNameDockerfile: dir, + dockerui.DefaultLocalNameContext: dir, + }, + Exports: []client.ExportEntry{ + { + Type: client.ExporterLocal, + OutputDir: destDir, + }, + }, + }, nil) + require.NoError(t, err) + + dt, err := os.ReadFile(filepath.Join(destDir, "env_foo")) + require.NoError(t, err) + require.Empty(t, dt) + + dt, err = os.ReadFile(filepath.Join(destDir, "env_bar")) + require.NoError(t, err) + require.Contains(t, string(dt), "BAR=") } func testWorkdirRelativePath(t *testing.T, sb integration.Sandbox) { diff --git a/frontend/dockerfile/instructions/parse.go b/frontend/dockerfile/instructions/parse.go index 9deb9c67275d8..f0fd689cb7b38 100644 --- a/frontend/dockerfile/instructions/parse.go +++ b/frontend/dockerfile/instructions/parse.go @@ -71,7 +71,7 @@ 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, ruleCheck *linter.RuleChecker) (v interface{}, err error) { defer func() { err = parser.WithLocation(err, node.Location()) }() @@ -80,9 +80,9 @@ func ParseInstructionWithLinter(node *parser.Node, lintWarn linter.LintWarnFunc) case command.Env: return parseEnv(req) case command.Maintainer: - if lintWarn != nil { + if ruleCheck != nil { msg := linter.RuleMaintainerDeprecated.Format() - linter.RuleMaintainerDeprecated.Run(lintWarn, node.Location(), msg) + ruleCheck.Run(&linter.RuleMaintainerDeprecated, node.Location(), msg) } return parseMaintainer(req) case command.Label: @@ -92,13 +92,13 @@ func ParseInstructionWithLinter(node *parser.Node, lintWarn linter.LintWarnFunc) case command.Copy: return parseCopy(req) case command.From: - if lintWarn != nil && !isLowerCaseStageName(req.args) { + if ruleCheck != nil && !isLowerCaseStageName(req.args) { msg := linter.RuleStageNameCasing.Format(req.args[2]) - linter.RuleStageNameCasing.Run(lintWarn, node.Location(), msg) + ruleCheck.Run(&linter.RuleStageNameCasing, node.Location(), msg) } - if lintWarn != nil && !doesFromCaseMatchAsCase(req) { + if ruleCheck != nil && !doesFromCaseMatchAsCase(req) { msg := linter.RuleFromAsCasing.Format(req.command, req.args[1]) - linter.RuleFromAsCasing.Run(lintWarn, node.Location(), msg) + ruleCheck.Run(&linter.RuleFromAsCasing, node.Location(), msg) } return parseFrom(req) case command.Onbuild: @@ -166,9 +166,9 @@ 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, ruleCheck *linter.RuleChecker) (stages []Stage, metaArgs []ArgCommand, err error) { for _, n := range ast.Children { - cmd, err := ParseInstructionWithLinter(n, lint) + cmd, err := ParseInstructionWithLinter(n, ruleCheck) if err != nil { return nil, nil, &parseError{inner: err, node: n} } diff --git a/frontend/dockerfile/linter/linter.go b/frontend/dockerfile/linter/linter.go index c75571b872614..d0eb2ebdaecbb 100644 --- a/frontend/dockerfile/linter/linter.go +++ b/frontend/dockerfile/linter/linter.go @@ -5,11 +5,68 @@ import ( "strings" "github.com/moby/buildkit/frontend/dockerfile/parser" + "github.com/pkg/errors" ) +type RuleChecker struct { + SkippedRules map[string]struct{} + CalledRules map[string]struct{} + SkipAll bool + ErrorFromViolation bool + FromBuildArg bool + Warn LintWarnFunc +} + +func NewRuleChecker() *RuleChecker { + return &RuleChecker{ + SkippedRules: map[string]struct{}{}, + CalledRules: map[string]struct{}{}, + } +} + +func NewRuleCheckerFromStr(lintOptionStr string) (*RuleChecker, error) { + ruleCheck := NewRuleChecker() + skipRules, skipAll, errOn, err := parser.ParseCheckOptions(lintOptionStr) + if err != nil { + return nil, err + } + ruleCheck.SkipAll = skipAll + for _, rule := range skipRules { + ruleCheck.SkippedRules[rule] = struct{}{} + } + ruleCheck.ErrorFromViolation = errOn + return ruleCheck, nil +} + +func (lc *RuleChecker) Run(rule LinterRuleI, location []parser.Range, txt ...string) { + if lc.Warn == nil { + return + } + rulename := rule.RuleName() + if _, ok := lc.SkippedRules[rulename]; ok { + return + } + lc.CalledRules[rulename] = struct{}{} + rule.Run(lc.Warn, location, txt...) +} + +func (lc *RuleChecker) ViolationError() error { + if !lc.ErrorFromViolation { + 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) - SetSkip(bool) } type LinterRule[F any] struct { @@ -17,17 +74,13 @@ type LinterRule[F any] struct { Description string URL string Format F - Skip bool } -func (rule *LinterRule[F]) SetSkip(skip bool) { - rule.Skip = skip +func (rule *LinterRule[F]) RuleName() string { + return rule.Name } func (rule *LinterRule[F]) Run(warn LintWarnFunc, location []parser.Range, txt ...string) { - if rule.Skip { - return - } if len(txt) == 0 { txt = []string{rule.Description} } @@ -40,18 +93,3 @@ func LintFormatShort(rulename, msg string, startLine int) string { } type LintWarnFunc func(rulename, description, url, fmtmsg string, location []parser.Range) - -func SetLintOptions(skipSet []string) { - if len(skipSet) == 1 && skipSet[0] == "all" { - for _, rule := range AllRules { - rule.SetSkip(true) - } - return - } - - for _, ruleName := range skipSet { - if rule, ok := AllRules[ruleName]; ok { - rule.SetSkip(true) - } - } -} diff --git a/frontend/dockerfile/linter/ruleset.go b/frontend/dockerfile/linter/ruleset.go index 9014df6643d63..98ef9eafd1f10 100644 --- a/frontend/dockerfile/linter/ruleset.go +++ b/frontend/dockerfile/linter/ruleset.go @@ -108,20 +108,3 @@ var ( }, } ) - -var AllRules = map[string]LinterRuleI{ - RuleStageNameCasing.Name: &RuleStageNameCasing, - RuleFromAsCasing.Name: &RuleFromAsCasing, - RuleNoEmptyContinuations.Name: &RuleNoEmptyContinuations, - RuleSelfConsistentCommandCasing.Name: &RuleSelfConsistentCommandCasing, - RuleFileConsistentCommandCasing.Name: &RuleFileConsistentCommandCasing, - RuleDuplicateStageName.Name: &RuleDuplicateStageName, - RuleReservedStageName.Name: &RuleReservedStageName, - RuleJSONArgsRecommended.Name: &RuleJSONArgsRecommended, - RuleMaintainerDeprecated.Name: &RuleMaintainerDeprecated, - RuleUndeclaredArgInFrom.Name: &RuleUndeclaredArgInFrom, - RuleWorkdirRelativePath.Name: &RuleWorkdirRelativePath, - RuleUndefinedArg.Name: &RuleUndefinedArg, - RuleUndefinedVar.Name: &RuleUndefinedVar, - RuleMultipleInstructionsDisallowed.Name: &RuleMultipleInstructionsDisallowed, -} diff --git a/frontend/dockerfile/parser/directives.go b/frontend/dockerfile/parser/directives.go index 8f708bf9f3903..023329b7e7fee 100644 --- a/frontend/dockerfile/parser/directives.go +++ b/frontend/dockerfile/parser/directives.go @@ -13,14 +13,14 @@ import ( const ( keySyntax = "syntax" - keyLint = "lint" + keyCheck = "check" keyEscape = "escape" ) var validDirectives = map[string]struct{}{ keySyntax: {}, keyEscape: {}, - keyLint: {}, + keyCheck: {}, } type Directive struct { @@ -115,37 +115,41 @@ func DetectSyntax(dt []byte) (string, string, []Range, bool) { return detectDirective(keySyntax, dt) } -func ParseLintOptions(lintStr string) ([]string, bool, error) { - parts := strings.SplitN(lintStr, ";", 2) +func ParseCheckOptions(checkStr string) ([]string, bool, bool, error) { + parts := strings.SplitN(checkStr, ";", 2) var skipSet []string - var errorOnWarn bool + var errorOnWarn, skipAll bool for _, p := range parts { k, v, ok := strings.Cut(p, "=") if !ok { - errors.Errorf("invalid lint option %s", p) + errors.Errorf("invalid check option %s", p) } k = strings.TrimSpace(k) switch k { case "skip": v = strings.TrimSpace(v) - skipSet = strings.Split(v, ",") - for i, rule := range skipSet { - skipSet[i] = strings.TrimSpace(rule) + if v == "all" { + skipAll = true + } else { + skipSet = strings.Split(v, ",") + for i, rule := range skipSet { + skipSet[i] = strings.TrimSpace(rule) + } } case "error": v = strings.TrimSpace(v) errorOnWarn = v == "true" default: - return nil, false, errors.Errorf("invalid lint option %s", k) + return nil, false, false, errors.Errorf("invalid check option %s", k) } } - return skipSet, errorOnWarn, nil + return skipSet, skipAll, errorOnWarn, nil } -// DetectLint returns the lint options of provided Dockerfile. +// DetectCheck returns the check options of provided Dockerfile. // Follows all the same rules as the syntax directive. -func DetectLint(dt []byte) (string, string, []Range, bool) { - return detectDirective(keyLint, dt) +func DetectCheck(dt []byte) (string, string, []Range, bool) { + return detectDirective(keyCheck, dt) } func detectDirective(key string, dt []byte) (string, string, []Range, bool) { diff --git a/frontend/dockerfile/parser/directives_test.go b/frontend/dockerfile/parser/directives_test.go index f5f4edfb94e5f..ddc14ac416696 100644 --- a/frontend/dockerfile/parser/directives_test.go +++ b/frontend/dockerfile/parser/directives_test.go @@ -107,10 +107,10 @@ RUN ls func TestDetectLint(t *testing.T) { t.Parallel() - dt := `#lint = skip=all // opts + dt := `#check = skip=all // opts FROM busybox ` - ref, cmdline, loc, ok := DetectLint([]byte(dt)) + ref, cmdline, loc, ok := DetectCheck([]byte(dt)) require.True(t, ok) require.Equal(t, ref, "skip=all") require.Equal(t, cmdline, "skip=all // opts") @@ -118,10 +118,10 @@ FROM busybox require.Equal(t, 1, loc[0].End.Line) dt = `#!/bin/sh -# lint = skip=all +# check = skip=all FROM busybox ` - ref, _, loc, ok = DetectLint([]byte(dt)) + ref, _, loc, ok = DetectCheck([]byte(dt)) require.True(t, ok) require.Equal(t, ref, "skip=all") require.Equal(t, 2, loc[0].Start.Line) @@ -129,42 +129,42 @@ FROM busybox dt = `#!/bin/sh -# lint = skip=all +# check = skip=all ` - _, _, _, ok = DetectLint([]byte(dt)) + _, _, _, ok = DetectCheck([]byte(dt)) require.False(t, ok) dt = `FROM busybox RUN ls ` - ref, cmdline, _, ok = DetectLint([]byte(dt)) + ref, cmdline, _, ok = DetectCheck([]byte(dt)) require.False(t, ok) require.Equal(t, ref, "") require.Equal(t, cmdline, "") - dt = `//lint=skip=all + dt = `//check=skip=all //key=value` - ref, _, _, ok = DetectLint([]byte(dt)) + ref, _, _, ok = DetectCheck([]byte(dt)) require.True(t, ok) require.Equal(t, ref, "skip=all") dt = `#!/bin/sh -//lint=skip=all` - ref, _, _, ok = DetectLint([]byte(dt)) +//check=skip=all` + ref, _, _, ok = DetectCheck([]byte(dt)) require.True(t, ok) require.Equal(t, ref, "skip=all") - dt = `{"lint": "skip=all"}` - ref, _, _, ok = DetectLint([]byte(dt)) + dt = `{"check": "skip=all"}` + ref, _, _, ok = DetectCheck([]byte(dt)) require.True(t, ok) require.Equal(t, ref, "skip=all") - dt = `{"lint": "foo"` - _, _, _, ok = DetectLint([]byte(dt)) + dt = `{"check": "foo"` + _, _, _, ok = DetectCheck([]byte(dt)) require.False(t, ok) - dt = `{"lint": "foo"} + dt = `{"check": "foo"} # syntax=bar` - _, _, _, ok = DetectLint([]byte(dt)) + _, _, _, ok = DetectCheck([]byte(dt)) require.False(t, ok) }