Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add linting rules for undeclared args in from #4843

Merged
merged 2 commits into from
May 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 25 additions & 9 deletions frontend/dockerfile/dockerfile2llb/convert.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand All @@ -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
}

Expand Down Expand Up @@ -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)
}
}
70 changes: 69 additions & 1 deletion frontend/dockerfile/dockerfile_lint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ var lintTests = integration.TestFuncs(
testReservedStageName,
testMaintainerDeprecated,
testWarningsBeforeError,
testUndeclaredArg,
)

func testStageName(t *testing.T, sb integration.Sandbox) {
Expand Down Expand Up @@ -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
daghack marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand Down Expand Up @@ -399,7 +467,6 @@ func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestPara
},
}, "", frontend, nil)
require.NoError(t, err)

require.True(t, called)
}

Expand Down Expand Up @@ -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
})
Expand Down
7 changes: 7 additions & 0 deletions frontend/dockerfile/linter/ruleset.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
},
}
)
17 changes: 15 additions & 2 deletions frontend/dockerfile/shell/lex.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
daghack marked this conversation as resolved.
Show resolved Hide resolved
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) {
Expand All @@ -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
Expand All @@ -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) {
Expand Down Expand Up @@ -511,6 +523,7 @@ func (sw *shellWord) getEnv(name string) (string, bool) {
return value, true
}
}
sw.nonmatches[name] = struct{}{}
return "", false
}

Expand Down
Loading