diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 4b56dc7cbc5b8..fbaa38154492f 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/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) { diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index 736a468b03bd1..1dc695eb59573 100644 --- a/frontend/dockerfile/dockerfile_lint_test.go +++ b/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="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, @@ -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) diff --git a/frontend/dockerfile/docs/rules/_index.md b/frontend/dockerfile/docs/rules/_index.md index 7e7bb9a8d2392..cc15afa2881fe 100644 --- a/frontend/dockerfile/docs/rules/_index.md +++ b/frontend/dockerfile/docs/rules/_index.md @@ -84,5 +84,9 @@ $ docker build --check . RedundantTargetPlatform Setting platform to predefined $TARGETPLATFORM in FROM is redundant as this is the default behavior + + InvalidDefaultArgInFrom + Using ARG with default value 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 0000000000000..ce66d21395c5c --- /dev/null +++ b/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} +``` + diff --git a/frontend/dockerfile/linter/docs/InvalidDefaultArgInFrom.md b/frontend/dockerfile/linter/docs/InvalidDefaultArgInFrom.md new file mode 100644 index 0000000000000..0d2ccb717a20d --- /dev/null +++ b/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} +``` diff --git a/frontend/dockerfile/linter/ruleset.go b/frontend/dockerfile/linter/ruleset.go index 173dc434141a1..95fd617d4b863 100644 --- a/frontend/dockerfile/linter/ruleset.go +++ b/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) + }, + } )