adds InvalidDefaultArgInFrom lint check

Signed-off-by: Talon Bowler <[email protected]>

frontend/dockerfile/dockerfile2llb/convert.go

@@ -257,27 +257,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
@@ -2344,6 +2332,59 @@ func validateBaseImagePlatform(name string, expected, actual ocispecs.Platform,
  }
 }
 
+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

@@ -41,6 +41,7 @@ var lintTests = integration.TestFuncs(
  testBaseImagePlatformMismatch,
  testAllTargetUnmarshal,
  testRedundantTargetPlatform,
+ testInvalidDefaultArgInFrom,
 )
 
 func testAllTargetUnmarshal(t *testing.T, sb integration.Sandbox) {
@@ -561,9 +562,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,
@@ -573,20 +574,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,
  })
 }
@@ -960,6 +961,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: "Using ARG with default value results in an empty or invalid base image name",
+ URL: "https://docs.docker.com/go/dockerfile/rule/invalid-default-arg-in-from/",
+ Detail: "Using ARG for busybox:$VERSION with default values 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: "Using ARG with default value results in an empty or invalid base image name",
+ URL: "https://docs.docker.com/go/dockerfile/rule/invalid-default-arg-in-from/",
+ Detail: "Using ARG for $IMAGE with default values 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: "Using ARG with default value results in an empty or invalid base image name",
+ URL: "https://docs.docker.com/go/dockerfile/rule/invalid-default-arg-in-from/",
+ Detail: "Using ARG for busy${SFX} with default values 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

@@ -84,5 +84,9 @@ $ docker build --check .
   <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>
+ <tr>
+  <td><a href="./invalid-default-arg-in-from/">InvalidDefaultArgInFrom</a></td>
+  <td>Using ARG with default value 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,40 @@
+---
+title: InvalidDefaultArgInFrom
+description: Using ARG with default value 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}
+```
+

frontend/dockerfile/linter/docs/InvalidDefaultArgInFrom.md

@@ -0,0 +1,32 @@
+## 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}
+```

frontend/dockerfile/linter/ruleset.go

@@ -132,4 +132,12 @@ var (
  return fmt.Sprintf("Setting platform to predefined %s in FROM is redundant as this is the default behavior", platformVar)
  },
  }
+ RuleInvalidDefaultArgInFrom = LinterRule[func(string) string]{
+ Name: "InvalidDefaultArgInFrom",
+ Description: "Using ARG with default value 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("Using ARG for %v with default values results in empty or invalid base image name", baseName)
+ },
+ }
 )