Merge pull request moby#5091 from jsternberg/lint-redundant-target-platform

checks: add redundant target platform check

Author: tonistiigi

frontend/dockerfile/dockerfile2llb/convert.go

@@ -289,7 +289,8 @@ 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, metaArgsToEnvs(optMetaArgs))
+		env := metaArgsToEnvs(optMetaArgs)
+		nameMatch, err := shlex.ProcessWordWithMatches(st.BaseName, env)
 		reportUnusedFromArgs(metaArgsKeys(optMetaArgs), nameMatch.Unmatched, st.Location, lint)
 		used := nameMatch.Matched
 		if used == nil {
@@ -316,8 +317,10 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
 		}
 
 		if v := st.Platform; v != "" {
-			platMatch, err := shlex.ProcessWordWithMatches(v, metaArgsToEnvs(optMetaArgs))
+			platEnv := metaArgsToEnvs(optMetaArgs)
+			platMatch, err := shlex.ProcessWordWithMatches(v, platEnv)
 			reportUnusedFromArgs(metaArgsKeys(optMetaArgs), platMatch.Unmatched, st.Location, lint)
+			reportRedundantTargetPlatform(st.Platform, platMatch, st.Location, platEnv, lint)
 
 			if err != nil {
 				return nil, parser.WithLocation(errors.Wrapf(err, "failed to process arguments for platform %s", platMatch.Result), st.Location)
@@ -2281,6 +2284,26 @@ func reportUnusedFromArgs(values []string, unmatched map[string]struct{}, locati
 	}
 }
 
+func reportRedundantTargetPlatform(platformVar string, nameMatch shell.ProcessWordResult, location []parser.Range, env shell.EnvGetter, lint *linter.Linter) {
+	// Only match this rule if there was only one matched name.
+	// It's psosible there were multiple args and that one of them expanded to an empty
+	// string and we don't want to report a warning when that happens.
+	if len(nameMatch.Matched) == 1 && len(nameMatch.Unmatched) == 0 {
+		const targetPlatform = "TARGETPLATFORM"
+		// If target platform is the only environment variable that was substituted and the result
+		// matches the target platform exactly, we can infer that the input was ${TARGETPLATFORM} or
+		// $TARGETPLATFORM.
+		if _, ok := nameMatch.Matched[targetPlatform]; !ok {
+			return
+		}
+
+		if result, _ := env.Get(targetPlatform); nameMatch.Result == result {
+			msg := linter.RuleRedundantTargetPlatform.Format(platformVar)
+			lint.Run(&linter.RuleRedundantTargetPlatform, location, msg)
+		}
+	}
+}
+
 type instructionTracker struct {
 	Loc   []parser.Range
 	IsSet bool

frontend/dockerfile/dockerfile_lint_test.go

@@ -40,6 +40,7 @@ var lintTests = integration.TestFuncs(
 	testLegacyKeyValueFormat,
 	testBaseImagePlatformMismatch,
 	testAllTargetUnmarshal,
+	testRedundantTargetPlatform,
 )
 
 func testAllTargetUnmarshal(t *testing.T, sb integration.Sandbox) {
@@ -923,6 +924,42 @@ FROM a AS c
 	})
 }
 
+func testRedundantTargetPlatform(t *testing.T, sb integration.Sandbox) {
+	dockerfile := []byte(`
+FROM --platform=$TARGETPLATFORM scratch
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{
+		Dockerfile: dockerfile,
+		Warnings: []expectedLintWarning{
+			{
+				RuleName:    "RedundantTargetPlatform",
+				Description: "Setting platform to predefined $TARGETPLATFORM in FROM is redundant as this is the default behavior",
+				URL:         "https://docs.docker.com/go/dockerfile/rule/redundant-target-platform/",
+				Detail:      "Setting platform to predefined $TARGETPLATFORM in FROM is redundant as this is the default behavior",
+				Line:        2,
+				Level:       1,
+			},
+		},
+	})
+
+	dockerfile = []byte(`
+FROM --platform=${TARGETPLATFORM} scratch
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{
+		Dockerfile: dockerfile,
+		Warnings: []expectedLintWarning{
+			{
+				RuleName:    "RedundantTargetPlatform",
+				Description: "Setting platform to predefined $TARGETPLATFORM in FROM is redundant as this is the default behavior",
+				URL:         "https://docs.docker.com/go/dockerfile/rule/redundant-target-platform/",
+				Detail:      "Setting platform to predefined ${TARGETPLATFORM} in FROM is redundant as this is the default behavior",
+				Line:        2,
+				Level:       1,
+			},
+		},
+	})
+}
+
 func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestParams) {
 	destDir, err := os.MkdirTemp("", "buildkit")
 	require.NoError(t, err)

frontend/dockerfile/dockerfile_test.go

@@ -122,7 +122,7 @@ var allTests = integration.TestFuncs(
 	testErrorsSourceMap,
 	testMultiArgs,
 	testFrontendSubrequests,
-	testDockefileCheckHostname,
+	testDockerfileCheckHostname,
 	testDefaultShellAndPath,
 	testDockerfileLowercase,
 	testExportCacheLoop,
@@ -201,8 +201,10 @@ var reproTests = integration.TestFuncs(
 	testReproSourceDateEpoch,
 )
 
-var opts []integration.TestOpt
-var securityOpts []integration.TestOpt
+var (
+	opts         []integration.TestOpt
+	securityOpts []integration.TestOpt
+)
 
 type frontend interface {
 	Solve(context.Context, *client.Client, client.SolveOpt, chan *client.SolveStatus) (*client.SolveResponse, error)
@@ -4387,6 +4389,7 @@ COPY --from=base unique /
 	require.NoError(t, err)
 	require.Equal(t, string(dt), string(dt2))
 }
+
 func testCacheImportExport(t *testing.T, sb integration.Sandbox) {
 	integration.SkipOnPlatform(t, "windows")
 	workers.CheckFeatureCompat(t, sb, workers.FeatureCacheExport, workers.FeatureCacheBackendLocal)
@@ -5285,7 +5288,7 @@ COPY Dockerfile Dockerfile
 }
 
 // moby/buildkit#1301
-func testDockefileCheckHostname(t *testing.T, sb integration.Sandbox) {
+func testDockerfileCheckHostname(t *testing.T, sb integration.Sandbox) {
 	integration.SkipOnPlatform(t, "windows")
 	f := getFrontend(t, sb)
 	dockerfile := []byte(`
@@ -7496,6 +7499,7 @@ func (f *clientFrontend) SolveGateway(ctx context.Context, c gateway.Client, req
 func (f *clientFrontend) DFCmdArgs(ctx, dockerfile string) (string, string) {
 	return "", ""
 }
+
 func (f *clientFrontend) RequiresBuildctl(t *testing.T) {
 	t.Skip()
 }
@@ -7556,8 +7560,10 @@ func (*secModeInsecure) UpdateConfigFile(in string) string {
 	return in + "\n\ninsecure-entitlements = [\"security.insecure\"]\n"
 }
 
-var securityInsecureGranted integration.ConfigUpdater = &secModeInsecure{}
-var securityInsecureDenied integration.ConfigUpdater = &secModeSandbox{}
+var (
+	securityInsecureGranted integration.ConfigUpdater = &secModeInsecure{}
+	securityInsecureDenied  integration.ConfigUpdater = &secModeSandbox{}
+)
 
 type networkModeHost struct{}
 
@@ -7571,8 +7577,10 @@ func (*networkModeSandbox) UpdateConfigFile(in string) string {
 	return in
 }
 
-var networkHostGranted integration.ConfigUpdater = &networkModeHost{}
-var networkHostDenied integration.ConfigUpdater = &networkModeSandbox{}
+var (
+	networkHostGranted integration.ConfigUpdater = &networkModeHost{}
+	networkHostDenied  integration.ConfigUpdater = &networkModeSandbox{}
+)
 
 func fixedWriteCloser(wc io.WriteCloser) filesync.FileOutputFunc {
 	return func(map[string]string) (io.WriteCloser, error) {

frontend/dockerfile/docs/rules/_index.md

@@ -80,5 +80,9 @@ $ docker build --check .
       <td><a href="./legacy-key-value-format/">LegacyKeyValueFormat</a></td>
       <td>Legacy key/value format with whitespace separator should not be used</td>
     </tr>
+    <tr>
+      <td><a href="./redundant-target-platform/">RedundantTargetPlatform</a></td>
+      <td>Setting platform to predefined $TARGETPLATFORM in FROM is redundant as this is the default behavior</td>
+    </tr>
   </tbody>
 </table>

frontend/dockerfile/docs/rules/redundant-target-platform.md

@@ -0,0 +1,35 @@
+---
+title: RedundantTargetPlatform
+description: Setting platform to predefined $TARGETPLATFORM in FROM is redundant as this is the default behavior
+aliases:
+  - /go/dockerfile/rule/redundant-target-platform/
+---
+
+## Output
+
+```text
+Setting platform to predefined $TARGETPLATFORM in FROM is redundant as this is the default behavior
+```
+
+## Description
+
+A custom platform can be used for a base image. The default platform is the
+same platform as the target output so setting the platform to `$TARGETPLATFORM`
+is redundant and unnecessary.
+
+## Examples
+
+❌ Bad: this usage of `--platform` is redundant since `$TARGETPLATFORM` is the default.
+
+```dockerfile
+FROM --platform=$TARGETPLATFORM alpine AS builder
+RUN apk add --no-cache git
+```
+
+✅ Good: omit the `--platform` argument.
+
+```dockerfile
+FROM alpine AS builder
+RUN apk add --no-cache git
+```
+

frontend/dockerfile/linter/docs/RedundantTargetPlatform.md

@@ -0,0 +1,27 @@
+## Output
+
+```text
+Setting platform to predefined $TARGETPLATFORM in FROM is redundant as this is the default behavior
+```
+
+## Description
+
+A custom platform can be used for a base image. The default platform is the
+same platform as the target output so setting the platform to `$TARGETPLATFORM`
+is redundant and unnecessary.
+
+## Examples
+
+❌ Bad: this usage of `--platform` is redundant since `$TARGETPLATFORM` is the default.
+
+```dockerfile
+FROM --platform=$TARGETPLATFORM alpine AS builder
+RUN apk add --no-cache git
+```
+
+✅ Good: omit the `--platform` argument.
+
+```dockerfile
+FROM alpine AS builder
+RUN apk add --no-cache git
+```

frontend/dockerfile/linter/ruleset.go

@@ -124,4 +124,12 @@ var (
 			return fmt.Sprintf("Base image %s was pulled with platform %q, expected %q for current build", image, actual, expected)
 		},
 	}
+	RuleRedundantTargetPlatform = LinterRule[func(string) string]{
+		Name:        "RedundantTargetPlatform",
+		Description: "Setting platform to predefined $TARGETPLATFORM in FROM is redundant as this is the default behavior",
+		URL:         "https://docs.docker.com/go/dockerfile/rule/redundant-target-platform/",
+		Format: func(platformVar string) string {
+			return fmt.Sprintf("Setting platform to predefined %s in FROM is redundant as this is the default behavior", platformVar)
+		},
+	}
 )