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

checks: add redundant target platform check

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