Merge pull request #4913 from jsternberg/lint-multiple-instructions-d…

…isallowed

linter: add rule to catch multiple conflicting instructions used in the same stage

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())
+}

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 <<EOF /a.txt
+Hello, World!
+EOF
+ENTRYPOINT ["/myotherapp"]
+CMD ["/myotherapp"]
+HEALTHCHECK CMD ["/myotherapp"]
+	`)
+	checkLinterWarnings(t, sb, &lintTestParams{
+		Dockerfile: dockerfile,
+		Warnings: []expectedLintWarning{
+			makeLintWarning("ENTRYPOINT", 3),
+			makeLintWarning("CMD", 4),
+			makeLintWarning("HEALTHCHECK", 5),
+		},
+	})
+
+	dockerfile = []byte(`
+FROM scratch AS a
+ENTRYPOINT ["/myapp"]
+CMD ["/myapp"]
+HEALTHCHECK CMD ["/myapp"]
+
+FROM a AS b
+ENTRYPOINT ["/myotherapp"]
+CMD ["/myotherapp"]
+HEALTHCHECK CMD ["/myotherapp"]
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
+}
+
 func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestParams) {
 	destDir, err := os.MkdirTemp("", "buildkit")
 	require.NoError(t, err)

frontend/dockerfile/linter/ruleset.go

@@ -98,4 +98,11 @@ var (
 			return fmt.Sprintf("Usage of undefined variable '$%s'", arg)
 		},
 	}
+	RuleMultipleInstructionsDisallowed = LinterRule[func(instructionName string) string]{
+		Name:        "MultipleInstructionsDisallowed",
+		Description: "Multiple instructions of the same type should not be used in the same stage",
+		Format: func(instructionName string) string {
+			return fmt.Sprintf("Multiple %s instructions should not be used in the same stage because only the last one will be used", instructionName)
+		},
+	}
 )