From 89ce7469cb157ddc32eca63fdc0d0cc99a76e26e Mon Sep 17 00:00:00 2001 From: "Jonathan A. Sternberg" Date: Fri, 10 May 2024 16:11:10 -0500 Subject: [PATCH] linter: add rule to catch multiple conflicting instructions used in the same stage This linter rule catches when ENTRYPOINT, CMD, and HEALTHCHECK are used multiple times in the same stage. This is because the last usage of each of these overrides the other usages which makes it so the earlier usage is ignored and unnecessary. Signed-off-by: Jonathan A. Sternberg --- frontend/dockerfile/dockerfile2llb/convert.go | 37 ++++++++-- frontend/dockerfile/dockerfile_lint_test.go | 68 +++++++++++++++++++ frontend/dockerfile/linter/ruleset.go | 7 ++ 3 files changed, 107 insertions(+), 5 deletions(-) diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 2bb9219db15f..3f0282c21a3f 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -819,7 +819,7 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error { case *instructions.EntrypointCommand: err = dispatchEntrypoint(d, c, opt.lintWarn) case *instructions.HealthCheckCommand: - err = dispatchHealthcheck(d, c) + err = dispatchHealthcheck(d, c, opt.lintWarn) case *instructions.ExposeCommand: err = dispatchExpose(d, c, opt.shlex) case *instructions.UserCommand: @@ -891,7 +891,6 @@ type dispatchState struct { // paths marks the paths that are used by this dispatchState. paths map[string]struct{} ignoreCache bool - cmdSet bool unregistered bool stageName string cmdIndex int @@ -904,6 +903,10 @@ type dispatchState struct { // workdirSet is set to true if a workdir has been set // within the current dockerfile. workdirSet bool + + entrypoint instructionTracker + cmd instructionTracker + healthcheck instructionTracker } func (ds *dispatchState) asyncLocalOpts() []llb.LocalOption { @@ -1473,6 +1476,8 @@ func dispatchOnbuild(d *dispatchState, c *instructions.OnbuildCommand) error { } func dispatchCmd(d *dispatchState, c *instructions.CmdCommand, warn linter.LintWarnFunc) error { + validateUsedOnce(c, &d.cmd, warn) + var args []string = c.CmdLine if c.PrependShell { if len(d.image.Config.Shell) == 0 { @@ -1483,11 +1488,12 @@ func dispatchCmd(d *dispatchState, c *instructions.CmdCommand, warn linter.LintW } d.image.Config.Cmd = args d.image.Config.ArgsEscaped = true //nolint:staticcheck // ignore SA1019: field is deprecated in OCI Image spec, but used for backward-compatibility with Docker image spec. - d.cmdSet = true 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) + var args []string = c.CmdLine if c.PrependShell { if len(d.image.Config.Shell) == 0 { @@ -1497,13 +1503,14 @@ func dispatchEntrypoint(d *dispatchState, c *instructions.EntrypointCommand, war args = withShell(d.image, args) } d.image.Config.Entrypoint = args - if !d.cmdSet { + if !d.cmd.IsSet { d.image.Config.Cmd = nil } return commitToHistory(&d.image, fmt.Sprintf("ENTRYPOINT %q", args), false, nil, d.epoch) } -func dispatchHealthcheck(d *dispatchState, c *instructions.HealthCheckCommand) error { +func dispatchHealthcheck(d *dispatchState, c *instructions.HealthCheckCommand, warn linter.LintWarnFunc) error { + validateUsedOnce(c, &d.healthcheck, warn) d.image.Config.Healthcheck = &dockerspec.HealthcheckConfig{ Test: c.Health.Test, Interval: c.Health.Interval, @@ -2151,3 +2158,23 @@ func reportUnusedFromArgs(unmatched map[string]struct{}, location []parser.Range linter.RuleUndeclaredArgInFrom.Run(warn, location, msg) } } + +type instructionTracker struct { + Loc []parser.Range + IsSet bool +} + +func (v *instructionTracker) MarkUsed(loc []parser.Range) { + v.Loc = loc + v.IsSet = true +} + +func validateUsedOnce(c instructions.Command, loc *instructionTracker, warn linter.LintWarnFunc) { + 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) + } + loc.MarkUsed(c.Location()) +} diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index 58246e65b289..eecacc56eb63 100644 --- a/frontend/dockerfile/dockerfile_lint_test.go +++ b/frontend/dockerfile/dockerfile_lint_test.go @@ -3,6 +3,7 @@ package dockerfile import ( "context" "encoding/json" + "fmt" "os" "sort" "testing" @@ -34,6 +35,7 @@ var lintTests = integration.TestFuncs( testUndeclaredArg, testWorkdirRelativePath, testUnmatchedVars, + testMultipleInstructionsDisallowed, ) func testStageName(t *testing.T, sb integration.Sandbox) { @@ -560,6 +562,72 @@ RUN echo $foo }) } +func testMultipleInstructionsDisallowed(t *testing.T, sb integration.Sandbox) { + makeLintWarning := func(instructionName string, line int) expectedLintWarning { + return expectedLintWarning{ + RuleName: "MultipleInstructionsDisallowed", + Description: "Multiple instructions of the same type should not be used in the same stage", + Detail: fmt.Sprintf("Multiple %s instructions should not be used in the same stage because only the last one will be used", instructionName), + Level: 1, + Line: line, + } + } + + dockerfile := []byte(` +FROM scratch +ENTRYPOINT ["/myapp"] +ENTRYPOINT ["/myotherapp"] +CMD ["/myapp"] +CMD ["/myotherapp"] +HEALTHCHECK CMD ["/myapp"] +HEALTHCHECK CMD ["/myotherapp"] +`) + checkLinterWarnings(t, sb, &lintTestParams{ + Dockerfile: dockerfile, + Warnings: []expectedLintWarning{ + makeLintWarning("ENTRYPOINT", 3), + makeLintWarning("CMD", 5), + makeLintWarning("HEALTHCHECK", 7), + }, + }) + + // Still a linter warning even when broken up with another + // command. Entrypoint is only used by the resulting image. + dockerfile = []byte(` +FROM scratch +ENTRYPOINT ["/myapp"] +CMD ["/myapp"] +HEALTHCHECK CMD ["/myapp"] +COPY <