Merge pull request #5140 from jsternberg/lint-constant-platform-disal…

…lowed checks: add check for constant in from platform flag
moby · Jul 10, 2024 · 4096e93 · 4096e93
2 parents 5b5afdb + 58753e8
commit 4096e93
Show file tree

Hide file tree

Showing 6 changed files with 182 additions and 0 deletions.
diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go
@@ -315,6 +315,7 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
 			platMatch, err := shlex.ProcessWordWithMatches(v, platEnv)
 			reportUnusedFromArgs(metaArgsKeys(optMetaArgs), platMatch.Unmatched, st.Location, lint)
 			reportRedundantTargetPlatform(st.Platform, platMatch, st.Location, platEnv, lint)
+			reportConstPlatformDisallowed(st.Name, platMatch, st.Location, lint)
 
 			if err != nil {
 				return nil, parser.WithLocation(errors.Wrapf(err, "failed to process arguments for platform %s", platMatch.Result), st.Location)
@@ -2300,6 +2301,31 @@ func reportRedundantTargetPlatform(platformVar string, nameMatch shell.ProcessWo
 	}
 }
 
+func reportConstPlatformDisallowed(stageName string, nameMatch shell.ProcessWordResult, location []parser.Range, lint *linter.Linter) {
+	if len(nameMatch.Matched) > 0 || len(nameMatch.Unmatched) > 0 {
+		// Some substitution happened so the platform was not a constant.
+		// Disable checking for this warning.
+		return
+	}
+
+	// Attempt to parse the platform result. If this fails, then it will fail
+	// later so just ignore.
+	p, err := platforms.Parse(nameMatch.Result)
+	if err != nil {
+		return
+	}
+
+	// Check if the platform os or architecture is used in the stage name
+	// at all. If it is, then disable this warning.
+	if strings.Contains(stageName, p.OS) || strings.Contains(stageName, p.Architecture) {
+		return
+	}
+
+	// Report the linter warning.
+	msg := linter.RuleFromPlatformFlagConstDisallowed.Format(nameMatch.Result)
+	lint.Run(&linter.RuleFromPlatformFlagConstDisallowed, location, msg)
+}
+
 type instructionTracker struct {
 	Loc   []parser.Range
 	IsSet bool

diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go
@@ -43,6 +43,7 @@ var lintTests = integration.TestFuncs(
 	testRedundantTargetPlatform,
 	testSecretsUsedInArgOrEnv,
 	testInvalidDefaultArgInFrom,
+	testFromPlatformFlagConstDisallowed,
 )
 
 func testSecretsUsedInArgOrEnv(t *testing.T, sb integration.Sandbox) {
@@ -1141,6 +1142,39 @@ FROM busybox:stable${BUSYBOX_VARIANT}
 	})
 }
 
+func testFromPlatformFlagConstDisallowed(t *testing.T, sb integration.Sandbox) {
+	dockerfile := []byte(`
+FROM --platform=linux/amd64 scratch
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{
+		Dockerfile: dockerfile,
+		Warnings: []expectedLintWarning{
+			{
+				RuleName:    "FromPlatformFlagConstDisallowed",
+				Description: "FROM --platform flag should not use a constant value",
+				URL:         "https://docs.docker.com/go/dockerfile/rule/from-platform-flag-const-disallowed/",
+				Detail:      "FROM --platform flag should not use constant value \"linux/amd64\"",
+				Line:        2,
+				Level:       1,
+			},
+		},
+	})
+
+	dockerfile = []byte(`
+FROM --platform=linux/amd64 scratch AS my_amd64_stage
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{
+		Dockerfile: dockerfile,
+	})
+
+	dockerfile = []byte(`
+FROM --platform=linux/amd64 scratch AS linux
+`)
+	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)

diff --git a/frontend/dockerfile/docs/rules/_index.md b/frontend/dockerfile/docs/rules/_index.md
@@ -92,5 +92,9 @@ $ docker build --check .
       <td><a href="./invalid-default-arg-in-from/">InvalidDefaultArgInFrom</a></td>
       <td>Default value for global ARG results in an empty or invalid base image name</td>
     </tr>
+    <tr>
+      <td><a href="./from-platform-flag-const-disallowed/">FromPlatformFlagConstDisallowed</a></td>
+      <td>FROM --platform flag should not use a constant value</td>
+    </tr>
   </tbody>
 </table>
diff --git a/frontend/dockerfile/docs/rules/from-platform-flag-const-disallowed.md b/frontend/dockerfile/docs/rules/from-platform-flag-const-disallowed.md
@@ -0,0 +1,59 @@
+---
+title: FromPlatformFlagConstDisallowed
+description: FROM --platform flag should not use a constant value
+aliases:
+  - /go/dockerfile/rule/from-platform-flag-const-disallowed/
+---
+
+## Output
+
+```text
+FROM --platform flag should not use constant value "linux/amd64"
+```
+
+## Description
+
+Specifying `--platform` in the Dockerfile `FROM` instruction forces the image to build on only one target platform. This prevents building a multi-platform image from this Dockerfile and you must build on the same platform as specified in `--platform`.
+
+The recommended approach is to:
+
+* Omit `FROM --platform` in the Dockerfile and use the `--platform` argument on the command line.
+* Use `$BUILDPLATFORM` or some other combination of variables for the `--platform` argument.
+* Stage name should include the platform, OS, or architecture name to indicate that it only contains platform-specific instructions.
+
+## Examples
+
+❌ Bad: using a constant argument for `--platform`
+
+```dockerfile
+FROM --platform=linux/amd64 alpine AS base
+RUN apk add --no-cache git
+```
+
+✅ Good: using the default platform
+
+```dockerfile
+FROM alpine AS base
+RUN apk add --no-cache git
+```
+
+✅ Good: using a meta variable
+
+```dockerfile
+FROM --platform=${BUILDPLATFORM} alpine AS base
+RUN apk add --no-cache git
+```
+
+✅ Good: used in a multi-stage build with a target architecture
+
+```dockerfile
+FROM --platform=linux/amd64 alpine AS build_amd64
+...
+
+FROM --platform=linux/arm64 alpine AS build_arm64
+...
+
+FROM build_${TARGETARCH} AS build
+...
+```
+
diff --git a/frontend/dockerfile/linter/docs/FromPlatformFlagConstDisallowed.md b/frontend/dockerfile/linter/docs/FromPlatformFlagConstDisallowed.md
@@ -0,0 +1,51 @@
+## Output
+
+```text
+FROM --platform flag should not use constant value "linux/amd64"
+```
+
+## Description
+
+Specifying `--platform` in the Dockerfile `FROM` instruction forces the image to build on only one target platform. This prevents building a multi-platform image from this Dockerfile and you must build on the same platform as specified in `--platform`.
+
+The recommended approach is to:
+
+* Omit `FROM --platform` in the Dockerfile and use the `--platform` argument on the command line.
+* Use `$BUILDPLATFORM` or some other combination of variables for the `--platform` argument.
+* Stage name should include the platform, OS, or architecture name to indicate that it only contains platform-specific instructions.
+
+## Examples
+
+❌ Bad: using a constant argument for `--platform`
+
+```dockerfile
+FROM --platform=linux/amd64 alpine AS base
+RUN apk add --no-cache git
+```
+
+✅ Good: using the default platform
+
+```dockerfile
+FROM alpine AS base
+RUN apk add --no-cache git
+```
+
+✅ Good: using a meta variable
+
+```dockerfile
+FROM --platform=${BUILDPLATFORM} alpine AS base
+RUN apk add --no-cache git
+```
+
+✅ Good: used in a multi-stage build with a target architecture
+
+```dockerfile
+FROM --platform=linux/amd64 alpine AS build_amd64
+...
+
+FROM --platform=linux/arm64 alpine AS build_arm64
+...
+
+FROM build_${TARGETARCH} AS build
+...
+```
diff --git a/frontend/dockerfile/linter/ruleset.go b/frontend/dockerfile/linter/ruleset.go
@@ -148,4 +148,12 @@ var (
 			return fmt.Sprintf("Default value for ARG %v results in empty or invalid base image name", baseName)
 		},
 	}
+	RuleFromPlatformFlagConstDisallowed = LinterRule[func(string) string]{
+		Name:        "FromPlatformFlagConstDisallowed",
+		Description: "FROM --platform flag should not use a constant value",
+		URL:         "https://docs.docker.com/go/dockerfile/rule/from-platform-flag-const-disallowed/",
+		Format: func(platform string) string {
+			return fmt.Sprintf("FROM --platform flag should not use constant value %q", platform)
+		},
+	}
 )