Skip to content

Commit

Permalink
add linting rules for undeclared args in from
Browse files Browse the repository at this point in the history 
Signed-off-by: Talon Bowler <[email protected]>
  • Loading branch information
@daghack
daghack committed Apr 15, 2024
1 parent 723a4d5 commit 87dd6f5
Show file tree
Hide file tree
Showing 6 changed files with 136 additions and 32 deletions.
25 changes: 22 additions & 3 deletions frontend/dockerfile/dockerfile2llb/convert.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,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.Word
info.deps = result.Matched
}
} else {
metaArg.Value = &v
Expand All @@ -248,7 +250,15 @@ 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))
result, err := shlex.ProcessWordWithMatches(st.BaseName, metaArgsToMap(optMetaArgs))
name := result.Word
used := result.Matched
if len(result.Unmatched) > 0 {
for unmatched := range result.Unmatched {
msg := linter.RuleUndeclaredArgInFrom.Format(unmatched)
linter.RuleUndeclaredArgInFrom.Run(opt.Warn, st.Location, msg)
}
}
if err != nil {
return nil, parser.WithLocation(err, st.Location)
}
Expand All @@ -269,7 +279,16 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
}

if v := st.Platform; v != "" {
v, u, err := shlex.ProcessWordWithMatches(v, metaArgsToMap(optMetaArgs))
result, err := shlex.ProcessWordWithMatches(v, metaArgsToMap(optMetaArgs))
v := result.Word
u := result.Matched

if len(result.Unmatched) > 0 {
for unmatched := range result.Unmatched {
msg := linter.RuleUndeclaredArgInFrom.Format(unmatched)
linter.RuleUndeclaredArgInFrom.Run(opt.Warn, st.Location, msg)
}
}
if err != nil {
return nil, parser.WithLocation(errors.Wrapf(err, "failed to process arguments for platform %s", v), st.Location)
}
Expand Down
98 changes: 78 additions & 20 deletions frontend/dockerfile/dockerfile_lint_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ var lintTests = integration.TestFuncs(
testNoEmptyContinuations,
testSelfConsistentCommandCasing,
testFileConsistentCommandCasing,
testLintUndeclaredArg,
)

func testStageName(t *testing.T, sb integration.Sandbox) {
Expand All @@ -38,7 +39,7 @@ FROM scratch as base2
FROM scratch AS base3
`)
checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{
checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{
{
RuleName: "StageNameCasing",
Description: "Stage names should be lowercase",
Expand All @@ -61,7 +62,7 @@ from scratch AS base
from scratch as base2
`)
checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{
checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{
{
RuleName: "FromAsCasing",
Description: "The 'as' keyword should match the case of the 'from' keyword",
Expand All @@ -83,7 +84,7 @@ COPY Dockerfile \
.
`)

checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{
checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{
{
RuleName: "NoEmptyContinuations",
Description: "Empty continuation lines will become errors in a future release",
Expand All @@ -101,7 +102,7 @@ func testSelfConsistentCommandCasing(t *testing.T, sb integration.Sandbox) {
From scratch as base
FROM scratch AS base2
`)
checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{
checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{
{
RuleName: "SelfConsistentCommandCasing",
Description: "Commands should be in consistent casing (all lower or all upper)",
Expand All @@ -115,7 +116,7 @@ FROM scratch AS base2
frOM scratch as base
from scratch as base2
`)
checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{
checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{
{
RuleName: "SelfConsistentCommandCasing",
Description: "Commands should be in consistent casing (all lower or all upper)",
Expand All @@ -133,7 +134,7 @@ FROM scratch
copy Dockerfile /foo
COPY Dockerfile /bar
`)
checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{
checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{
{
RuleName: "FileConsistentCommandCasing",
Description: "All commands within the Dockerfile should use the same casing (either upper or lower)",
Expand All @@ -149,7 +150,7 @@ from scratch
COPY Dockerfile /foo
copy Dockerfile /bar
`)
checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{
checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{
{
RuleName: "FileConsistentCommandCasing",
Description: "All commands within the Dockerfile should use the same casing (either upper or lower)",
Expand All @@ -166,7 +167,7 @@ COPY Dockerfile /foo
COPY Dockerfile /bar
COPY Dockerfile /baz
`)
checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{
checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{
{
RuleName: "FileConsistentCommandCasing",
Description: "All commands within the Dockerfile should use the same casing (either upper or lower)",
Expand All @@ -183,7 +184,7 @@ copy Dockerfile /foo
copy Dockerfile /bar
copy Dockerfile /baz
`)
checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{
checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{
{
RuleName: "FileConsistentCommandCasing",
Description: "All commands within the Dockerfile should use the same casing (either upper or lower)",
Expand All @@ -198,14 +199,67 @@ from scratch
copy Dockerfile /foo
copy Dockerfile /bar
`)
checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{})
checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{})

dockerfile = []byte(`
FROM scratch
COPY Dockerfile /foo
COPY Dockerfile /bar
`)
checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{})
checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{})
}

func testLintUndeclaredArg(t *testing.T, sb integration.Sandbox) {
dockerfile := []byte(`
ARG base=scratch
FROM $base
COPY Dockerfile .
`)
checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{})

dockerfile = []byte(`
# warning: undeclared ARG $base
FROM $base
COPY Dockerfile .
`)
checkLinterWarnings(t, sb, true, dockerfile, []expectedLintWarning{
{
RuleName: "UndeclaredArgInFrom",
Description: "FROM command must use declared ARGs",
Detail: "FROM argument 'base' is not declared",
Level: 1,
Line: 3,
},
})

dockerfile = []byte(`
ARG platform=linux/amd64
FROM --platform=$platform scratch
COPY Dockerfile .
`)
checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{})

dockerfile = []byte(`
ARG platform=linux/amd64
FROM --platform=$platform scratch
COPY Dockerfile .
`)
checkLinterWarnings(t, sb, false, dockerfile, []expectedLintWarning{})

dockerfile = []byte(`
# warning: undeclared ARG $platform
FROM --platform=$platform scratch
COPY Dockerfile .
`)
checkLinterWarnings(t, sb, true, dockerfile, []expectedLintWarning{
{
RuleName: "UndeclaredArgInFrom",
Description: "FROM command must use declared ARGs",
Detail: "FROM argument 'platform' is not declared",
Level: 1,
Line: 3,
},
})
}

func checkUnmarshal(t *testing.T, sb integration.Sandbox, c *client.Client, dir *integration.TmpDirWithName, expected []expectedLintWarning) {
Expand Down Expand Up @@ -237,7 +291,7 @@ func checkUnmarshal(t *testing.T, sb integration.Sandbox, c *client.Client, dir
})
// Compare expectedLintWarning with actual lint results
for i, w := range lintResults.Warnings {
checkLintWarning(t, w, expected[i])
checkLinterWarning(t, w, expected[i])
}
called = true
return nil, nil
Expand All @@ -253,7 +307,7 @@ func checkUnmarshal(t *testing.T, sb integration.Sandbox, c *client.Client, dir
require.True(t, called)
}

func checkProgressStream(t *testing.T, sb integration.Sandbox, c *client.Client, dir *integration.TmpDirWithName, expected []expectedLintWarning) {
func checkProgressStream(t *testing.T, sb integration.Sandbox, c *client.Client, dir *integration.TmpDirWithName, ignoreSolveErr bool, expected []expectedLintWarning) {
status := make(chan *client.SolveStatus)
statusDone := make(chan struct{})
done := make(chan struct{})
Expand Down Expand Up @@ -286,7 +340,9 @@ func checkProgressStream(t *testing.T, sb integration.Sandbox, c *client.Client,
dockerui.DefaultLocalNameContext: dir,
},
}, status)
require.NoError(t, err)
if !ignoreSolveErr {
require.NoError(t, err)
}

select {
case <-statusDone:
Expand All @@ -311,7 +367,7 @@ func checkProgressStream(t *testing.T, sb integration.Sandbox, c *client.Client,
}
}

func checkLinterWarnings(t *testing.T, sb integration.Sandbox, dockerfile []byte, expected []expectedLintWarning) {
func checkLinterWarnings(t *testing.T, sb integration.Sandbox, ignoreSolveErr bool, dockerfile []byte, expected []expectedLintWarning) {
// As a note, this test depends on the `expected` lint
// warnings to be in order of appearance in the Dockerfile.

Expand All @@ -327,11 +383,13 @@ func checkLinterWarnings(t *testing.T, sb integration.Sandbox, dockerfile []byte
defer c.Close()

t.Run("warntype=progress", func(t *testing.T) {
checkProgressStream(t, sb, c, dir, expected)
})
t.Run("warntype=unmarshal", func(t *testing.T) {
checkUnmarshal(t, sb, c, dir, expected)
checkProgressStream(t, sb, c, dir, ignoreSolveErr, expected)
})
if !ignoreSolveErr {
t.Run("warntype=unmarshal", func(t *testing.T) {
checkUnmarshal(t, sb, c, dir, expected)
})
}
}

func checkVertexWarning(t *testing.T, warning *client.VertexWarning, expected expectedLintWarning) {
Expand All @@ -342,7 +400,7 @@ func checkVertexWarning(t *testing.T, warning *client.VertexWarning, expected ex
require.Equal(t, expected.Level, warning.Level)
}

func checkLintWarning(t *testing.T, warning lint.Warning, expected expectedLintWarning) {
func checkLinterWarning(t *testing.T, warning lint.Warning, expected expectedLintWarning) {
short := linter.LintFormatShort(expected.RuleName, expected.Detail, expected.Line)
require.Equal(t, expected.RuleName, warning.RuleName)
require.Equal(t, expected.Description, warning.Description)
Expand Down
4 changes: 4 additions & 0 deletions frontend/dockerfile/dockerfile_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,10 @@ func init() {
}
}

func TestLint(t *testing.T) {
integration.Run(t, lintTests, opts...)
}

func TestIntegration(t *testing.T) {
integration.Run(t, allTests, opts...)

Expand Down
7 changes: 7 additions & 0 deletions frontend/dockerfile/linter/ruleset.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,11 @@ var (
return fmt.Sprintf("Command '%s' should match the case of the command majority (%s)", violatingCommand, correctCasing)
},
}
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)
},
}
)
18 changes: 16 additions & 2 deletions frontend/dockerfile/shell/lex.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,23 @@ func (s *Lex) ProcessWordWithMap(word string, env map[string]string) (string, er
return word, err
}


type ProcessWordMatchesResult struct {
Word 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{
Word: word,
Matched: sw.matches,
Unmatched: sw.nonmatches,
}, err
}

func (s *Lex) ProcessWordsWithMap(word string, env map[string]string) ([]string, error) {
Expand All @@ -79,6 +90,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 +110,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 +524,7 @@ func (sw *shellWord) getEnv(name string) (string, bool) {
return value, true
}
}
sw.nonmatches[name] = struct{}{}
return "", false
}

Expand Down
16 changes: 9 additions & 7 deletions frontend/dockerfile/shell/lex_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,9 @@ func TestProcessWithMatches(t *testing.T) {
for _, c := range tc {
c := c
t.Run(c.input, func(t *testing.T) {
w, matches, err := shlex.ProcessWordWithMatches(c.input, c.envs)
result, err := shlex.ProcessWordWithMatches(c.input, c.envs)
w := result.Word
matches := result.Matched
if c.expectedErr {
require.Error(t, err)
return
Expand All @@ -505,30 +507,30 @@ func TestProcessWithMatchesPlatform(t *testing.T) {
version = "v1.2.3"
)

w, _, err := shlex.ProcessWordWithMatches(release, map[string]string{
results, err := shlex.ProcessWordWithMatches(release, map[string]string{
"VERSION": version,
"TARGETOS": "linux",
"TARGETARCH": "arm",
"TARGETVARIANT": "v7",
})
require.NoError(t, err)
require.Equal(t, "something-v1.2.3.linux-arm-v7.tar.gz", w)
require.Equal(t, "something-v1.2.3.linux-arm-v7.tar.gz", results.Word)

w, _, err = shlex.ProcessWordWithMatches(release, map[string]string{
results, err = shlex.ProcessWordWithMatches(release, map[string]string{
"VERSION": version,
"TARGETOS": "linux",
"TARGETARCH": "arm64",
"TARGETVARIANT": "",
})
require.NoError(t, err)
require.Equal(t, "something-v1.2.3.linux-arm64.tar.gz", w)
require.Equal(t, "something-v1.2.3.linux-arm64.tar.gz", results.Word)

w, _, err = shlex.ProcessWordWithMatches(release, map[string]string{
results, err = shlex.ProcessWordWithMatches(release, map[string]string{
"VERSION": version,
"TARGETOS": "linux",
"TARGETARCH": "arm64",
// No "TARGETVARIANT": "",
})
require.NoError(t, err)
require.Equal(t, "something-v1.2.3.linux-arm64.tar.gz", w)
require.Equal(t, "something-v1.2.3.linux-arm64.tar.gz", results.Word)
}

0 comments on commit 87dd6f5

Please sign in to comment.