diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 4b26a4938a80..84716ec3ee91 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/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) { diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index 6814b53542be..a23b2b6a1c2b 100644 --- a/frontend/dockerfile/dockerfile_lint_test.go +++ b/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="me@example.org" 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 me@example.org +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) diff --git a/frontend/dockerfile/docs/rules/_index.md b/frontend/dockerfile/docs/rules/_index.md index 3158072d5558..0980d5d398ec 100644 --- a/frontend/dockerfile/docs/rules/_index.md +++ b/frontend/dockerfile/docs/rules/_index.md @@ -88,5 +88,9 @@ $ docker build --check . SecretsUsedInArgOrEnv Sensitive data should not be used in the ARG or ENV commands + + InvalidDefaultArgInFrom + Default value for global ARG results in an empty or invalid base image name + diff --git a/frontend/dockerfile/docs/rules/invalid-default-arg-in-from.md b/frontend/dockerfile/docs/rules/invalid-default-arg-in-from.md new file mode 100644 index 000000000000..2e6ff309d3d6 --- /dev/null +++ b/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} +``` + diff --git a/frontend/dockerfile/linter/docs/InvalidDefaultArgInFrom.md b/frontend/dockerfile/linter/docs/InvalidDefaultArgInFrom.md new file mode 100644 index 000000000000..faaf337f6edc --- /dev/null +++ b/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} +``` diff --git a/frontend/dockerfile/linter/ruleset.go b/frontend/dockerfile/linter/ruleset.go index f52147200310..b333c4c7c225 100644 --- a/frontend/dockerfile/linter/ruleset.go +++ b/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) + }, + } )