Skip to content

Commit 8bcc375

Browse files
daghackdaghack
committed
adds InvalidDefaultArgInFrom lint check
Signed-off-by: Talon Bowler <[email protected]>
1 parent 965ff52 commit 8bcc375

File tree

6 files changed

+269
-31
lines changed

6 files changed

+269
-31
lines changed

‎frontend/dockerfile/dockerfile2llb/convert.go

Lines changed: 62 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -263,27 +263,15 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
263263
shlex := shell.NewLex(dockerfile.EscapeToken)
264264
outline := newOutlineCapture()
265265

266-
for _, cmd := range metaArgs {
267-
for _, metaArg := range cmd.Args {
268-
info := argInfo{definition: metaArg, location: cmd.Location()}
269-
if v, ok := opt.BuildArgs[metaArg.Key]; !ok {
270-
if metaArg.Value != nil {
271-
result, err := shlex.ProcessWordWithMatches(*metaArg.Value, metaArgsToEnvs(optMetaArgs))
272-
if err != nil {
273-
return nil, parser.WithLocation(err, cmd.Location())
274-
}
275-
*metaArg.Value = result.Result
276-
info.deps = result.Matched
277-
}
278-
} else {
279-
metaArg.Value = &v
280-
}
281-
optMetaArgs = append(optMetaArgs, metaArg)
282-
if metaArg.Value != nil {
283-
info.value = *metaArg.Value
284-
}
285-
outline.allArgs[metaArg.Key] = info
286-
}
266+
// Validate that base images continue to be valid even
267+
// when no build arguments are used.
268+
validateBaseImagesWithDefaultArgs(stages, shlex, metaArgs, optMetaArgs, lint)
269+
270+
// Rebuild the arguments using the provided build arguments
271+
// for the remainder of the build.
272+
optMetaArgs, outline.allArgs, err = buildMetaArgs(optMetaArgs, shlex, metaArgs, opt.BuildArgs)
273+
if err != nil {
274+
return nil, err
287275
}
288276

289277
metaResolver := opt.MetaResolver
@@ -2383,6 +2371,59 @@ func validateNoSecretKey(instruction, key string, location []parser.Range, lint
23832371
}
23842372
}
23852373

2374+
func validateBaseImagesWithDefaultArgs(stages []instructions.Stage, shlex *shell.Lex, metaArgs []instructions.ArgCommand, optMetaArgs []instructions.KeyValuePairOptional, lint *linter.Linter) {
2375+
// Build the arguments as if no build options were given
2376+
// and using only defaults.
2377+
optMetaArgs, _, err := buildMetaArgs(optMetaArgs, shlex, metaArgs, nil)
2378+
if err != nil {
2379+
// Abandon running the linter. We'll likely fail after this point
2380+
// with the same error but we shouldn't error here inside
2381+
// of the linting check.
2382+
return
2383+
}
2384+
2385+
for _, st := range stages {
2386+
nameMatch, err := shlex.ProcessWordWithMatches(st.BaseName, metaArgsToEnvs(optMetaArgs))
2387+
if err != nil {
2388+
return
2389+
}
2390+
2391+
// Verify the image spec is potentially valid.
2392+
if _, err := reference.ParseNormalizedNamed(nameMatch.Result); err != nil {
2393+
msg := linter.RuleInvalidDefaultArgInFrom.Format(st.BaseName)
2394+
lint.Run(&linter.RuleInvalidDefaultArgInFrom, st.Location, msg)
2395+
}
2396+
}
2397+
}
2398+
2399+
func buildMetaArgs(metaArgs []instructions.KeyValuePairOptional, shlex *shell.Lex, argCommands []instructions.ArgCommand, buildArgs map[string]string) ([]instructions.KeyValuePairOptional, map[string]argInfo, error) {
2400+
allArgs := make(map[string]argInfo)
2401+
2402+
for _, cmd := range argCommands {
2403+
for _, metaArg := range cmd.Args {
2404+
info := argInfo{definition: metaArg, location: cmd.Location()}
2405+
if v, ok := buildArgs[metaArg.Key]; !ok {
2406+
if metaArg.Value != nil {
2407+
result, err := shlex.ProcessWordWithMatches(*metaArg.Value, metaArgsToEnvs(metaArgs))
2408+
if err != nil {
2409+
return nil, nil, parser.WithLocation(err, cmd.Location())
2410+
}
2411+
metaArg.Value = &result.Result
2412+
info.deps = result.Matched
2413+
}
2414+
} else {
2415+
metaArg.Value = &v
2416+
}
2417+
metaArgs = append(metaArgs, metaArg)
2418+
if metaArg.Value != nil {
2419+
info.value = *metaArg.Value
2420+
}
2421+
allArgs[metaArg.Key] = info
2422+
}
2423+
}
2424+
return metaArgs, allArgs, nil
2425+
}
2426+
23862427
type emptyEnvs struct{}
23872428

23882429
func (emptyEnvs) Get(string) (string, bool) {

‎frontend/dockerfile/dockerfile_lint_test.go

Lines changed: 109 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ var lintTests = integration.TestFuncs(
4242
testAllTargetUnmarshal,
4343
testRedundantTargetPlatform,
4444
testSecretsUsedInArgOrEnv,
45+
testInvalidDefaultArgInFrom,
4546
)
4647

4748
func testSecretsUsedInArgOrEnv(t *testing.T, sb integration.Sandbox) {
@@ -643,9 +644,9 @@ LABEL org.opencontainers.image.authors="[email protected]"
643644

644645
func testWarningsBeforeError(t *testing.T, sb integration.Sandbox) {
645646
dockerfile := []byte(`
646-
# warning: stage name should be lowercase
647647
FROM scratch AS BadStageName
648-
FROM ${BAR} AS base
648+
MAINTAINER [email protected]
649+
BADCMD
649650
`)
650651
checkLinterWarnings(t, sb, &lintTestParams{
651652
Dockerfile: dockerfile,
@@ -655,20 +656,20 @@ FROM ${BAR} AS base
655656
Description: "Stage names should be lowercase",
656657
URL: "https://docs.docker.com/go/dockerfile/rule/stage-name-casing/",
657658
Detail: "Stage name 'BadStageName' should be lowercase",
658-
Line: 3,
659+
Line: 2,
659660
Level: 1,
660661
},
661662
{
662-
RuleName: "UndefinedArgInFrom",
663-
Description: "FROM command must use declared ARGs",
664-
URL: "https://docs.docker.com/go/dockerfile/rule/undefined-arg-in-from/",
665-
Detail: "FROM argument 'BAR' is not declared",
663+
RuleName: "MaintainerDeprecated",
664+
Description: "The MAINTAINER instruction is deprecated, use a label instead to define an image author",
665+
URL: "https://docs.docker.com/go/dockerfile/rule/maintainer-deprecated/",
666+
Detail: "Maintainer instruction is deprecated in favor of using label",
666667
Level: 1,
667-
Line: 4,
668+
Line: 3,
668669
},
669670
},
670-
StreamBuildErr: "failed to solve: base name (${BAR}) should not be blank",
671-
UnmarshalBuildErr: "base name (${BAR}) should not be blank",
671+
StreamBuildErr: "failed to solve: dockerfile parse error on line 4: unknown instruction: BADCMD",
672+
UnmarshalBuildErr: "dockerfile parse error on line 4: unknown instruction: BADCMD",
672673
BuildErrLocation: 4,
673674
})
674675
}
@@ -1042,6 +1043,104 @@ FROM --platform=${TARGETPLATFORM} scratch
10421043
})
10431044
}
10441045

1046+
func testInvalidDefaultArgInFrom(t *testing.T, sb integration.Sandbox) {
1047+
dockerfile := []byte(`
1048+
ARG VERSION
1049+
FROM busybox:$VERSION
1050+
`)
1051+
checkLinterWarnings(t, sb, &lintTestParams{
1052+
Dockerfile: dockerfile,
1053+
FrontendAttrs: map[string]string{
1054+
"build-arg:VERSION": "latest",
1055+
},
1056+
Warnings: []expectedLintWarning{
1057+
{
1058+
RuleName: "InvalidDefaultArgInFrom",
1059+
Description: "Default value for global ARG results in an empty or invalid base image name",
1060+
URL: "https://docs.docker.com/go/dockerfile/rule/invalid-default-arg-in-from/",
1061+
Detail: "Default value for ARG busybox:$VERSION results in empty or invalid base image name",
1062+
Line: 3,
1063+
Level: 1,
1064+
},
1065+
},
1066+
})
1067+
1068+
dockerfile = []byte(`
1069+
ARG IMAGE
1070+
FROM $IMAGE
1071+
`)
1072+
checkLinterWarnings(t, sb, &lintTestParams{
1073+
Dockerfile: dockerfile,
1074+
FrontendAttrs: map[string]string{
1075+
"build-arg:IMAGE": "busybox:latest",
1076+
},
1077+
Warnings: []expectedLintWarning{
1078+
{
1079+
RuleName: "InvalidDefaultArgInFrom",
1080+
Description: "Default value for global ARG results in an empty or invalid base image name",
1081+
URL: "https://docs.docker.com/go/dockerfile/rule/invalid-default-arg-in-from/",
1082+
Detail: "Default value for ARG $IMAGE results in empty or invalid base image name",
1083+
Line: 3,
1084+
Level: 1,
1085+
},
1086+
},
1087+
})
1088+
1089+
dockerfile = []byte(`
1090+
ARG SFX="box:"
1091+
FROM busy${SFX}
1092+
`)
1093+
checkLinterWarnings(t, sb, &lintTestParams{
1094+
Dockerfile: dockerfile,
1095+
FrontendAttrs: map[string]string{
1096+
"build-arg:SFX": "box:latest",
1097+
},
1098+
Warnings: []expectedLintWarning{
1099+
{
1100+
RuleName: "InvalidDefaultArgInFrom",
1101+
Description: "Default value for global ARG results in an empty or invalid base image name",
1102+
URL: "https://docs.docker.com/go/dockerfile/rule/invalid-default-arg-in-from/",
1103+
Detail: "Default value for ARG busy${SFX} results in empty or invalid base image name",
1104+
Line: 3,
1105+
Level: 1,
1106+
},
1107+
},
1108+
})
1109+
1110+
dockerfile = []byte(`
1111+
ARG VERSION="latest"
1112+
FROM busybox:${VERSION}
1113+
`)
1114+
checkLinterWarnings(t, sb, &lintTestParams{
1115+
Dockerfile: dockerfile,
1116+
FrontendAttrs: map[string]string{
1117+
"build-arg:VERSION": "latest",
1118+
},
1119+
})
1120+
1121+
dockerfile = []byte(`
1122+
ARG BUSYBOX_VARIANT=""
1123+
FROM busybox:stable${BUSYBOX_VARIANT}
1124+
`)
1125+
checkLinterWarnings(t, sb, &lintTestParams{
1126+
Dockerfile: dockerfile,
1127+
FrontendAttrs: map[string]string{
1128+
"build-arg:BUSYBOX_VARIANT": "-musl",
1129+
},
1130+
})
1131+
1132+
dockerfile = []byte(`
1133+
ARG BUSYBOX_VARIANT
1134+
FROM busybox:stable${BUSYBOX_VARIANT}
1135+
`)
1136+
checkLinterWarnings(t, sb, &lintTestParams{
1137+
Dockerfile: dockerfile,
1138+
FrontendAttrs: map[string]string{
1139+
"build-arg:BUSYBOX_VARIANT": "-musl",
1140+
},
1141+
})
1142+
}
1143+
10451144
func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestParams) {
10461145
destDir, err := os.MkdirTemp("", "buildkit")
10471146
require.NoError(t, err)

‎frontend/dockerfile/docs/rules/_index.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,5 +88,9 @@ $ docker build --check .
8888
<td><a href="./secrets-used-in-arg-or-env/">SecretsUsedInArgOrEnv</a></td>
8989
<td>Sensitive data should not be used in the ARG or ENV commands</td>
9090
</tr>
91+
<tr>
92+
<td><a href="./invalid-default-arg-in-from/">InvalidDefaultArgInFrom</a></td>
93+
<td>Default value for global ARG results in an empty or invalid base image name</td>
94+
</tr>
9195
</tbody>
9296
</table>

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
---
2+
title: InvalidDefaultArgInFrom
3+
description: Default value for global ARG results in an empty or invalid base image name
4+
aliases:
5+
- /go/dockerfile/rule/invalid-default-arg-in-from/
6+
---
7+
8+
## Output
9+
10+
```text
11+
Using the global ARGs with default values should produce a valid build.
12+
```
13+
14+
## Description
15+
16+
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.
17+
18+
## Examples
19+
20+
❌ Bad: don't rely on an ARG being set for an image reference to be valid
21+
22+
```dockerfile
23+
ARG TAG
24+
FROM busybox:${TAG}
25+
```
26+
27+
✅ Good: include a default for the ARG
28+
29+
```dockerfile
30+
ARG TAG=latest
31+
FROM busybox:${TAG}
32+
```
33+
34+
✅ Good: ARG can be empty if the image would be valid with it empty
35+
36+
```dockerfile
37+
ARG VARIANT
38+
FROM busybox:stable${VARIANT}
39+
```
40+
41+
✅ Good: Use a default value if the build arg is not present
42+
43+
```dockerfile
44+
ARG TAG
45+
FROM alpine:${TAG:-3.14}
46+
```
47+

‎frontend/dockerfile/linter/docs/InvalidDefaultArgInFrom.md

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
## Output
2+
3+
```text
4+
Using the global ARGs with default values should produce a valid build.
5+
```
6+
7+
## Description
8+
9+
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.
10+
11+
## Examples
12+
13+
❌ Bad: don't rely on an ARG being set for an image reference to be valid
14+
15+
```dockerfile
16+
ARG TAG
17+
FROM busybox:${TAG}
18+
```
19+
20+
✅ Good: include a default for the ARG
21+
22+
```dockerfile
23+
ARG TAG=latest
24+
FROM busybox:${TAG}
25+
```
26+
27+
✅ Good: ARG can be empty if the image would be valid with it empty
28+
29+
```dockerfile
30+
ARG VARIANT
31+
FROM busybox:stable${VARIANT}
32+
```
33+
34+
✅ Good: Use a default value if the build arg is not present
35+
36+
```dockerfile
37+
ARG TAG
38+
FROM alpine:${TAG:-3.14}
39+
```

‎frontend/dockerfile/linter/ruleset.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,4 +140,12 @@ var (
140140
return fmt.Sprintf("Do not use ARG or ENV instructions for sensitive data (%s %q)", instruction, secretKey)
141141
},
142142
}
143+
RuleInvalidDefaultArgInFrom = LinterRule[func(string) string]{
144+
Name: "InvalidDefaultArgInFrom",
145+
Description: "Default value for global ARG results in an empty or invalid base image name",
146+
URL: "https://docs.docker.com/go/dockerfile/rule/invalid-default-arg-in-from/",
147+
Format: func(baseName string) string {
148+
return fmt.Sprintf("Default value for ARG %v results in empty or invalid base image name", baseName)
149+
},
150+
}
143151
)

0 commit comments

Comments
 (0)