Merge pull request #4843 from daghack/basic-arg-lint-rules

add linting rules for undeclared args in from

frontend/dockerfile/dockerfile2llb/convert.go

@@ -236,7 +236,9 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
 			info := argInfo{definition: metaArg, location: cmd.Location()}
 			if v, ok := opt.BuildArgs[metaArg.Key]; !ok {
 				if metaArg.Value != nil {
-					*metaArg.Value, info.deps, _ = shlex.ProcessWordWithMatches(*metaArg.Value, metaArgsToMap(optMetaArgs))
+					result, _ := shlex.ProcessWordWithMatches(*metaArg.Value, metaArgsToMap(optMetaArgs))
+					*metaArg.Value = result.Result
+					info.deps = result.Matched
 				}
 			} else {
 				metaArg.Value = &v
@@ -258,14 +260,17 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
 
 	// set base state for every image
 	for i, st := range stages {
-		name, used, err := shlex.ProcessWordWithMatches(st.BaseName, metaArgsToMap(optMetaArgs))
+		nameMatch, err := shlex.ProcessWordWithMatches(st.BaseName, metaArgsToMap(optMetaArgs))
+		reportUnusedFromArgs(nameMatch.Unmatched, st.Location, opt.Warn)
+		used := nameMatch.Matched
+
 		if err != nil {
 			return nil, parser.WithLocation(err, st.Location)
 		}
-		if name == "" {
+		if nameMatch.Result == "" {
 			return nil, parser.WithLocation(errors.Errorf("base name (%s) should not be blank", st.BaseName), st.Location)
 		}
-		st.BaseName = name
+		st.BaseName = nameMatch.Result
 
 		ds := &dispatchState{
 			stage:          st,
@@ -279,18 +284,22 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
 		}
 
 		if v := st.Platform; v != "" {
-			v, u, err := shlex.ProcessWordWithMatches(v, metaArgsToMap(optMetaArgs))
+			platMatch, err := shlex.ProcessWordWithMatches(v, metaArgsToMap(optMetaArgs))
+			reportUnusedFromArgs(platMatch.Unmatched, st.Location, opt.Warn)
+
 			if err != nil {
-				return nil, parser.WithLocation(errors.Wrapf(err, "failed to process arguments for platform %s", v), st.Location)
+				return nil, parser.WithLocation(errors.Wrapf(err, "failed to process arguments for platform %s", platMatch.Result), st.Location)
 			}
 
-			p, err := platforms.Parse(v)
+			p, err := platforms.Parse(platMatch.Result)
 			if err != nil {
-				return nil, parser.WithLocation(errors.Wrapf(err, "failed to parse platform %s", v), st.Location)
+				return nil, parser.WithLocation(errors.Wrapf(err, "failed to parse platform %s", platMatch.Result), st.Location)
 			}
-			for k := range u {
+
+			for k := range platMatch.Matched {
 				used[k] = struct{}{}
 			}
+
 			ds.platform = &p
 		}
 
@@ -2074,3 +2083,10 @@ func toPBLocation(sourceIndex int, location []parser.Range) pb.Location {
 		Ranges:      loc,
 	}
 }
+
+func reportUnusedFromArgs(unmatched map[string]struct{}, location []parser.Range, warn linter.LintWarnFunc) {
+	for arg := range unmatched {
+		msg := linter.RuleUndeclaredArgInFrom.Format(arg)
+		linter.RuleUndeclaredArgInFrom.Run(warn, location, msg)
+	}
+}

frontend/dockerfile/dockerfile_lint_test.go

@@ -30,6 +30,7 @@ var lintTests = integration.TestFuncs(
 	testReservedStageName,
 	testMaintainerDeprecated,
 	testWarningsBeforeError,
+	testUndeclaredArg,
 )
 
 func testStageName(t *testing.T, sb integration.Sandbox) {
@@ -343,13 +344,80 @@ FROM ${BAR} AS base
 				Line:        3,
 				Level:       1,
 			},
+			{
+				RuleName:    "UndeclaredArgInFrom",
+				Description: "FROM command must use declared ARGs",
+				Detail:      "FROM argument 'BAR' is not declared",
+				Level:       1,
+				Line:        4,
+			},
 		},
 		StreamBuildErr:    "failed to solve: base name (${BAR}) should not be blank",
 		UnmarshalBuildErr: "base name (${BAR}) should not be blank",
 		BuildErrLocation:  4,
 	})
 }
 
+func testUndeclaredArg(t *testing.T, sb integration.Sandbox) {
+	dockerfile := []byte(`
+ARG base=scratch
+FROM $base
+COPY Dockerfile .
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
+
+	dockerfile = []byte(`
+ARG BUILDPLATFORM=linux/amd64
+FROM --platform=$BUILDPLATFORM scratch
+COPY Dockerfile .
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
+
+	dockerfile = []byte(`
+FROM --platform=$BUILDPLATFORM scratch
+COPY Dockerfile .
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
+
+	dockerfile = []byte(`
+FROM --platform=$BULIDPLATFORM scratch
+COPY Dockerfile .
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{
+		Dockerfile: dockerfile,
+		Warnings: []expectedLintWarning{
+			{
+				RuleName:    "UndeclaredArgInFrom",
+				Description: "FROM command must use declared ARGs",
+				Detail:      "FROM argument 'BULIDPLATFORM' is not declared",
+				Level:       1,
+				Line:        2,
+			},
+		},
+		StreamBuildErr:    "failed to solve: failed to parse platform : \"\" is an invalid component of \"\": platform specifier component must match \"^[A-Za-z0-9_-]+$\": invalid argument",
+		UnmarshalBuildErr: "failed to parse platform : \"\" is an invalid component of \"\": platform specifier component must match \"^[A-Za-z0-9_-]+$\": invalid argument",
+		BuildErrLocation:  2,
+	})
+
+	dockerfile = []byte(`
+ARG tag=latest
+FROM busybox:${tag}${version} AS b
+COPY Dockerfile .
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{
+		Dockerfile: dockerfile,
+		Warnings: []expectedLintWarning{
+			{
+				RuleName:    "UndeclaredArgInFrom",
+				Description: "FROM command must use declared ARGs",
+				Detail:      "FROM argument 'version' is not declared",
+				Level:       1,
+				Line:        3,
+			},
+		},
+	})
+}
+
 func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestParams) {
 	destDir, err := os.MkdirTemp("", "buildkit")
 	require.NoError(t, err)
@@ -399,7 +467,6 @@ func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestPara
 		},
 	}, "", frontend, nil)
 	require.NoError(t, err)
-
 	require.True(t, called)
 }
 
@@ -468,6 +535,7 @@ func checkProgressStream(t *testing.T, sb integration.Sandbox, lintTest *lintTes
 }
 
 func checkLinterWarnings(t *testing.T, sb integration.Sandbox, lintTest *lintTestParams) {
+	t.Helper()
 	sort.Slice(lintTest.Warnings, func(i, j int) bool {
 		return lintTest.Warnings[i].Line < lintTest.Warnings[j].Line
 	})

frontend/dockerfile/linter/ruleset.go

@@ -63,4 +63,11 @@ var (
 			return "Maintainer instruction is deprecated in favor of using label"
 		},
 	}
+	RuleUndeclaredArgInFrom = LinterRule[func(string) string]{
+		Name:        "UndeclaredArgInFrom",
+		Description: "FROM command must use declared ARGs",
+		Format: func(baseArg string) string {
+			return fmt.Sprintf("FROM argument '%s' is not declared", baseArg)
+		},
+	}
 )

frontend/dockerfile/shell/lex.go

@@ -57,12 +57,22 @@ func (s *Lex) ProcessWordWithMap(word string, env map[string]string) (string, er
 	return word, err
 }
 
+type ProcessWordMatchesResult struct {
+	Result    string
+	Matched   map[string]struct{}
+	Unmatched map[string]struct{}
+}
+
 // ProcessWordWithMatches will use the 'env' list of environment variables,
 // replace any env var references in 'word' and return the env that were used.
-func (s *Lex) ProcessWordWithMatches(word string, env map[string]string) (string, map[string]struct{}, error) {
+func (s *Lex) ProcessWordWithMatches(word string, env map[string]string) (ProcessWordMatchesResult, error) {
 	sw := s.init(word, env)
 	word, _, err := sw.process(word)
-	return word, sw.matches, err
+	return ProcessWordMatchesResult{
+		Result:    word,
+		Matched:   sw.matches,
+		Unmatched: sw.nonmatches,
+	}, err
 }
 
 func (s *Lex) ProcessWordsWithMap(word string, env map[string]string) ([]string, error) {
@@ -79,6 +89,7 @@ func (s *Lex) init(word string, env map[string]string) *shellWord {
 		rawQuotes:         s.RawQuotes,
 		rawEscapes:        s.RawEscapes,
 		matches:           make(map[string]struct{}),
+		nonmatches:        make(map[string]struct{}),
 	}
 	sw.scanner.Init(strings.NewReader(word))
 	return sw
@@ -98,6 +109,7 @@ type shellWord struct {
 	skipUnsetEnv      bool
 	skipProcessQuotes bool
 	matches           map[string]struct{}
+	nonmatches        map[string]struct{}
 }
 
 func (sw *shellWord) process(source string) (string, []string, error) {
@@ -511,6 +523,7 @@ func (sw *shellWord) getEnv(name string) (string, bool) {
 			return value, true
 		}
 	}
+	sw.nonmatches[name] = struct{}{}
 	return "", false
 }