Merge pull request #5113 from daghack/invalid-default-arg-in-from

adds InvalidDefaultArgInFrom lint check

frontend/dockerfile/dockerfile2llb/convert.go

@@ -263,27 +263,15 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
 	shlex := shell.NewLex(dockerfile.EscapeToken)
 	outline := newOutlineCapture()
 
-	for _, cmd := range metaArgs {
-		for _, metaArg := range cmd.Args {
-			info := argInfo{definition: metaArg, location: cmd.Location()}
-			if v, ok := opt.BuildArgs[metaArg.Key]; !ok {
-				if metaArg.Value != nil {
-					result, err := shlex.ProcessWordWithMatches(*metaArg.Value, metaArgsToEnvs(optMetaArgs))
-					if err != nil {
-						return nil, parser.WithLocation(err, cmd.Location())
-					}
-					*metaArg.Value = result.Result
-					info.deps = result.Matched
-				}
-			} else {
-				metaArg.Value = &v
-			}
-			optMetaArgs = append(optMetaArgs, metaArg)
-			if metaArg.Value != nil {
-				info.value = *metaArg.Value
-			}
-			outline.allArgs[metaArg.Key] = info
-		}
+	// Validate that base images continue to be valid even
+	// when no build arguments are used.
+	validateBaseImagesWithDefaultArgs(stages, shlex, metaArgs, optMetaArgs, lint)
+
+	// Rebuild the arguments using the provided build arguments
+	// for the remainder of the build.
+	optMetaArgs, outline.allArgs, err = buildMetaArgs(optMetaArgs, shlex, metaArgs, opt.BuildArgs)
+	if err != nil {
+		return nil, err
 	}
 
 	metaResolver := opt.MetaResolver
@@ -2383,6 +2371,59 @@ func validateNoSecretKey(instruction, key string, location []parser.Range, lint
 	}
 }
 
+func validateBaseImagesWithDefaultArgs(stages []instructions.Stage, shlex *shell.Lex, metaArgs []instructions.ArgCommand, optMetaArgs []instructions.KeyValuePairOptional, lint *linter.Linter) {
+	// Build the arguments as if no build options were given
+	// and using only defaults.
+	optMetaArgs, _, err := buildMetaArgs(optMetaArgs, shlex, metaArgs, nil)
+	if err != nil {
+		// Abandon running the linter. We'll likely fail after this point
+		// with the same error but we shouldn't error here inside
+		// of the linting check.
+		return
+	}
+
+	for _, st := range stages {
+		nameMatch, err := shlex.ProcessWordWithMatches(st.BaseName, metaArgsToEnvs(optMetaArgs))
+		if err != nil {
+			return
+		}
+
+		// Verify the image spec is potentially valid.
+		if _, err := reference.ParseNormalizedNamed(nameMatch.Result); err != nil {
+			msg := linter.RuleInvalidDefaultArgInFrom.Format(st.BaseName)
+			lint.Run(&linter.RuleInvalidDefaultArgInFrom, st.Location, msg)
+		}
+	}
+}
+
+func buildMetaArgs(metaArgs []instructions.KeyValuePairOptional, shlex *shell.Lex, argCommands []instructions.ArgCommand, buildArgs map[string]string) ([]instructions.KeyValuePairOptional, map[string]argInfo, error) {
+	allArgs := make(map[string]argInfo)
+
+	for _, cmd := range argCommands {
+		for _, metaArg := range cmd.Args {
+			info := argInfo{definition: metaArg, location: cmd.Location()}
+			if v, ok := buildArgs[metaArg.Key]; !ok {
+				if metaArg.Value != nil {
+					result, err := shlex.ProcessWordWithMatches(*metaArg.Value, metaArgsToEnvs(metaArgs))
+					if err != nil {
+						return nil, nil, parser.WithLocation(err, cmd.Location())
+					}
+					metaArg.Value = &result.Result
+					info.deps = result.Matched
+				}
+			} else {
+				metaArg.Value = &v
+			}
+			metaArgs = append(metaArgs, metaArg)
+			if metaArg.Value != nil {
+				info.value = *metaArg.Value
+			}
+			allArgs[metaArg.Key] = info
+		}
+	}
+	return metaArgs, allArgs, nil
+}
+
 type emptyEnvs struct{}
 
 func (emptyEnvs) Get(string) (string, bool) {

frontend/dockerfile/dockerfile_lint_test.go

@@ -42,6 +42,7 @@ var lintTests = integration.TestFuncs(
 	testAllTargetUnmarshal,
 	testRedundantTargetPlatform,
 	testSecretsUsedInArgOrEnv,
+	testInvalidDefaultArgInFrom,
 )
 
 func testSecretsUsedInArgOrEnv(t *testing.T, sb integration.Sandbox) {
@@ -643,9 +644,9 @@ LABEL org.opencontainers.image.authors="[email protected]"
 
 func testWarningsBeforeError(t *testing.T, sb integration.Sandbox) {
 	dockerfile := []byte(`
-# warning: stage name should be lowercase
 FROM scratch AS BadStageName
-FROM ${BAR} AS base
+MAINTAINER [email protected]
+BADCMD
 `)
 	checkLinterWarnings(t, sb, &lintTestParams{
 		Dockerfile: dockerfile,
@@ -655,20 +656,20 @@ FROM ${BAR} AS base
 				Description: "Stage names should be lowercase",
 				URL:         "https://docs.docker.com/go/dockerfile/rule/stage-name-casing/",
 				Detail:      "Stage name 'BadStageName' should be lowercase",
-				Line:        3,
+				Line:        2,
 				Level:       1,
 			},
 			{
-				RuleName:    "UndefinedArgInFrom",
-				Description: "FROM command must use declared ARGs",
-				URL:         "https://docs.docker.com/go/dockerfile/rule/undefined-arg-in-from/",
-				Detail:      "FROM argument 'BAR' is not declared",
+				RuleName:    "MaintainerDeprecated",
+				Description: "The MAINTAINER instruction is deprecated, use a label instead to define an image author",
+				URL:         "https://docs.docker.com/go/dockerfile/rule/maintainer-deprecated/",
+				Detail:      "Maintainer instruction is deprecated in favor of using label",
 				Level:       1,
-				Line:        4,
+				Line:        3,
 			},
 		},
-		StreamBuildErr:    "failed to solve: base name (${BAR}) should not be blank",
-		UnmarshalBuildErr: "base name (${BAR}) should not be blank",
+		StreamBuildErr:    "failed to solve: dockerfile parse error on line 4: unknown instruction: BADCMD",
+		UnmarshalBuildErr: "dockerfile parse error on line 4: unknown instruction: BADCMD",
 		BuildErrLocation:  4,
 	})
 }
@@ -1042,6 +1043,104 @@ FROM --platform=${TARGETPLATFORM} scratch
 	})
 }
 
+func testInvalidDefaultArgInFrom(t *testing.T, sb integration.Sandbox) {
+	dockerfile := []byte(`
+ARG VERSION
+FROM busybox:$VERSION
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{
+		Dockerfile: dockerfile,
+		FrontendAttrs: map[string]string{
+			"build-arg:VERSION": "latest",
+		},
+		Warnings: []expectedLintWarning{
+			{
+				RuleName:    "InvalidDefaultArgInFrom",
+				Description: "Default value for global ARG results in an empty or invalid base image name",
+				URL:         "https://docs.docker.com/go/dockerfile/rule/invalid-default-arg-in-from/",
+				Detail:      "Default value for ARG busybox:$VERSION results in empty or invalid base image name",
+				Line:        3,
+				Level:       1,
+			},
+		},
+	})
+
+	dockerfile = []byte(`
+ARG IMAGE
+FROM $IMAGE
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{
+		Dockerfile: dockerfile,
+		FrontendAttrs: map[string]string{
+			"build-arg:IMAGE": "busybox:latest",
+		},
+		Warnings: []expectedLintWarning{
+			{
+				RuleName:    "InvalidDefaultArgInFrom",
+				Description: "Default value for global ARG results in an empty or invalid base image name",
+				URL:         "https://docs.docker.com/go/dockerfile/rule/invalid-default-arg-in-from/",
+				Detail:      "Default value for ARG $IMAGE results in empty or invalid base image name",
+				Line:        3,
+				Level:       1,
+			},
+		},
+	})
+
+	dockerfile = []byte(`
+ARG SFX="box:"
+FROM busy${SFX}
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{
+		Dockerfile: dockerfile,
+		FrontendAttrs: map[string]string{
+			"build-arg:SFX": "box:latest",
+		},
+		Warnings: []expectedLintWarning{
+			{
+				RuleName:    "InvalidDefaultArgInFrom",
+				Description: "Default value for global ARG results in an empty or invalid base image name",
+				URL:         "https://docs.docker.com/go/dockerfile/rule/invalid-default-arg-in-from/",
+				Detail:      "Default value for ARG busy${SFX} results in empty or invalid base image name",
+				Line:        3,
+				Level:       1,
+			},
+		},
+	})
+
+	dockerfile = []byte(`
+ARG VERSION="latest"
+FROM busybox:${VERSION}
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{
+		Dockerfile: dockerfile,
+		FrontendAttrs: map[string]string{
+			"build-arg:VERSION": "latest",
+		},
+	})
+
+	dockerfile = []byte(`
+ARG BUSYBOX_VARIANT=""
+FROM busybox:stable${BUSYBOX_VARIANT}
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{
+		Dockerfile: dockerfile,
+		FrontendAttrs: map[string]string{
+			"build-arg:BUSYBOX_VARIANT": "-musl",
+		},
+	})
+
+	dockerfile = []byte(`
+ARG BUSYBOX_VARIANT
+FROM busybox:stable${BUSYBOX_VARIANT}
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{
+		Dockerfile: dockerfile,
+		FrontendAttrs: map[string]string{
+			"build-arg:BUSYBOX_VARIANT": "-musl",
+		},
+	})
+}
+
 func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestParams) {
 	destDir, err := os.MkdirTemp("", "buildkit")
 	require.NoError(t, err)

frontend/dockerfile/docs/rules/_index.md

@@ -88,5 +88,9 @@ $ docker build --check .
       <td><a href="./secrets-used-in-arg-or-env/">SecretsUsedInArgOrEnv</a></td>
       <td>Sensitive data should not be used in the ARG or ENV commands</td>
     </tr>
+    <tr>
+      <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>
   </tbody>
 </table>

frontend/dockerfile/docs/rules/invalid-default-arg-in-from.md

@@ -0,0 +1,47 @@
+---
+title: InvalidDefaultArgInFrom
+description: Default value for global ARG results in an empty or invalid base image name
+aliases:
+  - /go/dockerfile/rule/invalid-default-arg-in-from/
+---
+
+## Output
+
+```text
+Using the global ARGs with default values should produce a valid build.
+```
+
+## Description
+
+An `ARG` used in an image reference should be valid when no build arguments are used. An image build should not require `--build-arg` to be used to produce a valid build.
+
+## Examples
+
+❌ Bad: don't rely on an ARG being set for an image reference to be valid
+
+```dockerfile
+ARG TAG
+FROM busybox:${TAG}
+```
+
+✅ Good: include a default for the ARG
+
+```dockerfile
+ARG TAG=latest
+FROM busybox:${TAG}
+```
+
+✅ Good: ARG can be empty if the image would be valid with it empty
+
+```dockerfile
+ARG VARIANT
+FROM busybox:stable${VARIANT}
+```
+
+✅ Good: Use a default value if the build arg is not present
+
+```dockerfile
+ARG TAG
+FROM alpine:${TAG:-3.14}
+```
+

frontend/dockerfile/linter/docs/InvalidDefaultArgInFrom.md

@@ -0,0 +1,39 @@
+## Output
+
+```text
+Using the global ARGs with default values should produce a valid build.
+```
+
+## Description
+
+An `ARG` used in an image reference should be valid when no build arguments are used. An image build should not require `--build-arg` to be used to produce a valid build.
+
+## Examples
+
+❌ Bad: don't rely on an ARG being set for an image reference to be valid
+
+```dockerfile
+ARG TAG
+FROM busybox:${TAG}
+```
+
+✅ Good: include a default for the ARG
+
+```dockerfile
+ARG TAG=latest
+FROM busybox:${TAG}
+```
+
+✅ Good: ARG can be empty if the image would be valid with it empty
+
+```dockerfile
+ARG VARIANT
+FROM busybox:stable${VARIANT}
+```
+
+✅ Good: Use a default value if the build arg is not present
+
+```dockerfile
+ARG TAG
+FROM alpine:${TAG:-3.14}
+```

frontend/dockerfile/linter/ruleset.go

@@ -140,4 +140,12 @@ var (
 			return fmt.Sprintf("Do not use ARG or ENV instructions for sensitive data (%s %q)", instruction, secretKey)
 		},
 	}
+	RuleInvalidDefaultArgInFrom = LinterRule[func(string) string]{
+		Name:        "InvalidDefaultArgInFrom",
+		Description: "Default value for global ARG results in an empty or invalid base image name",
+		URL:         "https://docs.docker.com/go/dockerfile/rule/invalid-default-arg-in-from/",
+		Format: func(baseName string) string {
+			return fmt.Sprintf("Default value for ARG %v results in empty or invalid base image name", baseName)
+		},
+	}
 )