Skip to content

Commit

Permalink
Merge pull request #5135 from daghack/copy-ignored-file-check
Browse files Browse the repository at this point in the history 
Rule Check: CopyIgnoredFiles
  • Loading branch information
@tonistiigi
tonistiigi committed Jul 10, 2024
2 parents 62ba6fe + 07fe324 commit e83d79a
Show file tree
Hide file tree
Showing 7 changed files with 364 additions and 103 deletions.
111 changes: 79 additions & 32 deletions frontend/dockerfile/dockerfile2llb/convert.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import (
"github.com/moby/buildkit/util/suggest"
"github.com/moby/buildkit/util/system"
dockerspec "github.com/moby/docker-image-spec/specs-go/v1"
"github.com/moby/patternmatcher"
"github.com/moby/sys/signal"
digest "github.com/opencontainers/go-digest"
ocispecs "github.com/opencontainers/image-spec/specs-go/v1"
Expand Down Expand Up @@ -594,6 +595,20 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
buildContext := &mutableOutput{}
ctxPaths := map[string]struct{}{}

var dockerIgnoreMatcher *patternmatcher.PatternMatcher
if opt.Client != nil && opt.Client.CopyIgnoredCheckEnabled {
dockerIgnorePatterns, err := opt.Client.DockerIgnorePatterns(ctx)
if err != nil {
return nil, err
}
if len(dockerIgnorePatterns) > 0 {
dockerIgnoreMatcher, err = patternmatcher.New(dockerIgnorePatterns)
if err != nil {
return nil, err
}
}
}

for _, d := range allDispatchStates.states {
if !opt.AllStages {
if _, ok := allReachable[d]; !ok || d.noinit {
Expand Down Expand Up @@ -634,24 +649,27 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
return nil, parser.WithLocation(err, d.stage.Location)
}
}

d.state = d.state.Network(opt.NetworkMode)

opt := dispatchOpt{
allDispatchStates: allDispatchStates,
metaArgs: optMetaArgs,
buildArgValues: opt.BuildArgs,
shlex: shlex,
buildContext: llb.NewState(buildContext),
proxyEnv: proxyEnv,
cacheIDNamespace: opt.CacheIDNamespace,
buildPlatforms: platformOpt.buildPlatforms,
targetPlatform: platformOpt.targetPlatform,
extraHosts: opt.ExtraHosts,
shmSize: opt.ShmSize,
ulimit: opt.Ulimits,
cgroupParent: opt.CgroupParent,
llbCaps: opt.LLBCaps,
sourceMap: opt.SourceMap,
lint: lint,
allDispatchStates: allDispatchStates,
metaArgs: optMetaArgs,
buildArgValues: opt.BuildArgs,
shlex: shlex,
buildContext: llb.NewState(buildContext),
proxyEnv: proxyEnv,
cacheIDNamespace: opt.CacheIDNamespace,
buildPlatforms: platformOpt.buildPlatforms,
targetPlatform: platformOpt.targetPlatform,
extraHosts: opt.ExtraHosts,
shmSize: opt.ShmSize,
ulimit: opt.Ulimits,
cgroupParent: opt.CgroupParent,
llbCaps: opt.LLBCaps,
sourceMap: opt.SourceMap,
lint: lint,
dockerIgnoreMatcher: dockerIgnoreMatcher,
}

if err = dispatchOnBuildTriggers(d, d.image.Config.OnBuild, opt); err != nil {
Expand Down Expand Up @@ -810,22 +828,23 @@ func toCommand(ic instructions.Command, allDispatchStates *dispatchStates) (comm
}

type dispatchOpt struct {
allDispatchStates *dispatchStates
metaArgs []instructions.KeyValuePairOptional
buildArgValues map[string]string
shlex *shell.Lex
buildContext llb.State
proxyEnv *llb.ProxyEnv
cacheIDNamespace string
targetPlatform ocispecs.Platform
buildPlatforms []ocispecs.Platform
extraHosts []llb.HostIP
shmSize int64
ulimit []pb.Ulimit
cgroupParent string
llbCaps *apicaps.CapSet
sourceMap *llb.SourceMap
lint *linter.Linter
allDispatchStates *dispatchStates
metaArgs []instructions.KeyValuePairOptional
buildArgValues map[string]string
shlex *shell.Lex
buildContext llb.State
proxyEnv *llb.ProxyEnv
cacheIDNamespace string
targetPlatform ocispecs.Platform
buildPlatforms []ocispecs.Platform
extraHosts []llb.HostIP
shmSize int64
ulimit []pb.Ulimit
cgroupParent string
llbCaps *apicaps.CapSet
sourceMap *llb.SourceMap
lint *linter.Linter
dockerIgnoreMatcher *patternmatcher.PatternMatcher
}

func getEnv(state llb.State) shell.EnvGetter {
Expand Down Expand Up @@ -912,6 +931,7 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error {
keepGitDir: c.KeepGitDir,
checksum: checksum,
location: c.Location(),
ignoreMatcher: opt.dockerIgnoreMatcher,
opt: opt,
})
}
Expand Down Expand Up @@ -946,12 +966,15 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error {
err = dispatchArg(d, c, &opt)
case *instructions.CopyCommand:
l := opt.buildContext
var ignoreMatcher *patternmatcher.PatternMatcher
if len(cmd.sources) != 0 {
src := cmd.sources[0]
if !src.noinit {
return errors.Errorf("cannot copy from stage %q, it needs to be defined before current stage %q", c.From, d.stageName)
}
l = src.state
} else {
ignoreMatcher = opt.dockerIgnoreMatcher
}
err = dispatchCopy(d, copyConfig{
params: c.SourcesAndDest,
Expand All @@ -964,6 +987,7 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error {
link: c.Link,
parents: c.Parents,
location: c.Location(),
ignoreMatcher: ignoreMatcher,
opt: opt,
})
if err == nil {
Expand Down Expand Up @@ -1442,6 +1466,7 @@ func dispatchCopy(d *dispatchState, cfg copyConfig) error {
a = a.Copy(st, f, dest, opts...)
}
} else {
validateCopySourcePath(src, &cfg)
var patterns []string
if cfg.parents {
// detect optional pivot point
Expand Down Expand Up @@ -1558,6 +1583,7 @@ type copyConfig struct {
checksum digest.Digest
parents bool
location []parser.Range
ignoreMatcher *patternmatcher.PatternMatcher
opt dispatchOpt
}

Expand Down Expand Up @@ -1871,6 +1897,27 @@ func addReachableStages(s *dispatchState, stages map[*dispatchState]struct{}) {
}
}

func validateCopySourcePath(src string, cfg *copyConfig) error {
if cfg.ignoreMatcher == nil {
return nil
}
cmd := "Copy"
if cfg.isAddCommand {
cmd = "Add"
}

ok, err := cfg.ignoreMatcher.MatchesOrParentMatches(src)
if err != nil {
return err
}
if ok {
msg := linter.RuleCopyIgnoredFile.Format(cmd, src)
cfg.opt.lint.Run(&linter.RuleCopyIgnoredFile, cfg.location, msg)
}

return nil
}

func validateCircularDependency(states []*dispatchState) error {
var visit func(*dispatchState, []instructions.Command) []instructions.Command
if states == nil {
Expand Down
113 changes: 99 additions & 14 deletions frontend/dockerfile/dockerfile_lint_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"maps"
"os"
"regexp"
"sort"
"testing"
"time"
Expand Down Expand Up @@ -44,8 +45,70 @@ var lintTests = integration.TestFuncs(
testSecretsUsedInArgOrEnv,
testInvalidDefaultArgInFrom,
testFromPlatformFlagConstDisallowed,
testCopyIgnoredFiles,
)

func testCopyIgnoredFiles(t *testing.T, sb integration.Sandbox) {
dockerignore := []byte(`
Dockerfile
`)
dockerfile := []byte(`
FROM scratch
COPY Dockerfile .
ADD Dockerfile /windy
`)
checkLinterWarnings(t, sb, &lintTestParams{
Dockerfile: dockerfile,
DockerIgnore: dockerignore,
BuildErrLocation: 3,
StreamBuildErrRegexp: regexp.MustCompile(`failed to solve: failed to compute cache key: failed to calculate checksum of ref [^\s]+ "/Dockerfile": not found`),
})

checkLinterWarnings(t, sb, &lintTestParams{
Dockerfile: dockerfile,
DockerIgnore: dockerignore,
FrontendAttrs: map[string]string{
"build-arg:BUILDKIT_DOCKERFILE_CHECK_COPYIGNORED_EXPERIMENT": "true",
},
BuildErrLocation: 3,
StreamBuildErrRegexp: regexp.MustCompile(`failed to solve: failed to compute cache key: failed to calculate checksum of ref [^\s]+ "/Dockerfile": not found`),
Warnings: []expectedLintWarning{
{
RuleName: "CopyIgnoredFile",
Description: "Attempting to Copy file that is excluded by .dockerignore",
Detail: `Attempting to Copy file "Dockerfile" that is excluded by .dockerignore`,
URL: "https://docs.docker.com/go/dockerfile/rule/copy-ignored-file/",
Level: 1,
Line: 3,
},
{
RuleName: "CopyIgnoredFile",
Description: "Attempting to Copy file that is excluded by .dockerignore",
Detail: `Attempting to Add file "Dockerfile" that is excluded by .dockerignore`,
URL: "https://docs.docker.com/go/dockerfile/rule/copy-ignored-file/",
Level: 1,
Line: 4,
},
},
})

dockerignore = []byte(`
foobar
`)
dockerfile = []byte(`
FROM scratch AS base
COPY Dockerfile /foobar
ADD Dockerfile /windy
FROM base
COPY --from=base /foobar /Dockerfile
`)
checkLinterWarnings(t, sb, &lintTestParams{
Dockerfile: dockerfile,
DockerIgnore: dockerignore,
})
}

func testSecretsUsedInArgOrEnv(t *testing.T, sb integration.Sandbox) {
dockerfile := []byte(`
FROM scratch
Expand Down Expand Up @@ -1206,11 +1269,19 @@ func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestPara
lintResults, err := unmarshalLintResults(res)
require.NoError(t, err)

if lintResults.Error != nil {
require.Equal(t, lintTest.UnmarshalBuildErr, lintResults.Error.Message)
if lintTest.UnmarshalBuildErr == "" && lintTest.UnmarshalBuildErrRegexp == nil {
require.Nil(t, lintResults.Error)
} else {
require.NotNil(t, lintResults.Error)
if lintTest.UnmarshalBuildErr != "" {
require.Equal(t, lintTest.UnmarshalBuildErr, lintResults.Error.Message)
} else if !lintTest.UnmarshalBuildErrRegexp.MatchString(lintResults.Error.Message) {
t.Fatalf("error %q does not match %q", lintResults.Error.Message, lintTest.UnmarshalBuildErrRegexp.String())
}
require.Greater(t, lintResults.Error.Location.SourceIndex, int32(-1))
require.Less(t, lintResults.Error.Location.SourceIndex, int32(len(lintResults.Sources)))
}

require.Equal(t, len(warnings), len(lintResults.Warnings))

sort.Slice(lintResults.Warnings, func(i, j int) bool {
Expand All @@ -1230,6 +1301,7 @@ func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestPara
_, err = lintTest.Client.Build(sb.Context(), client.SolveOpt{
LocalMounts: map[string]fsutil.FS{
dockerui.DefaultLocalNameDockerfile: lintTest.TmpDir,
dockerui.DefaultLocalNameContext: lintTest.TmpDir,
},
}, "", frontend, nil)
require.NoError(t, err)
Expand Down Expand Up @@ -1276,10 +1348,14 @@ func checkProgressStream(t *testing.T, sb integration.Sandbox, lintTest *lintTes
dockerui.DefaultLocalNameContext: lintTest.TmpDir,
},
}, status)
if lintTest.StreamBuildErr == "" {
if lintTest.StreamBuildErr == "" && lintTest.StreamBuildErrRegexp == nil {
require.NoError(t, err)
} else {
require.EqualError(t, err, lintTest.StreamBuildErr)
if lintTest.StreamBuildErr != "" {
require.EqualError(t, err, lintTest.StreamBuildErr)
} else if !lintTest.StreamBuildErrRegexp.MatchString(err.Error()) {
t.Fatalf("error %q does not match %q", err.Error(), lintTest.StreamBuildErrRegexp.String())
}
}

select {
Expand Down Expand Up @@ -1313,9 +1389,15 @@ func checkLinterWarnings(t *testing.T, sb integration.Sandbox, lintTest *lintTes
integration.SkipOnPlatform(t, "windows")

if lintTest.TmpDir == nil {
testfiles := []fstest.Applier{
fstest.CreateFile("Dockerfile", lintTest.Dockerfile, 0600),
}
if lintTest.DockerIgnore != nil {
testfiles = append(testfiles, fstest.CreateFile(".dockerignore", lintTest.DockerIgnore, 0600))
}
lintTest.TmpDir = integration.Tmpdir(
t,
fstest.CreateFile("Dockerfile", lintTest.Dockerfile, 0600),
testfiles...,
)
}

Expand Down Expand Up @@ -1374,13 +1456,16 @@ type expectedLintWarning struct {
}

type lintTestParams struct {
Client *client.Client
TmpDir *integration.TmpDirWithName
Dockerfile []byte
Warnings []expectedLintWarning
UnmarshalWarnings []expectedLintWarning
StreamBuildErr string
UnmarshalBuildErr string
BuildErrLocation int32
FrontendAttrs map[string]string
Client *client.Client
TmpDir *integration.TmpDirWithName
Dockerfile []byte
DockerIgnore []byte
Warnings []expectedLintWarning
UnmarshalWarnings []expectedLintWarning
StreamBuildErr string
StreamBuildErrRegexp *regexp.Regexp
UnmarshalBuildErr string
UnmarshalBuildErrRegexp *regexp.Regexp
BuildErrLocation int32
FrontendAttrs map[string]string
}
4 changes: 4 additions & 0 deletions frontend/dockerfile/docs/rules/_index.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,5 +96,9 @@ $ docker build --check .
<td><a href="./from-platform-flag-const-disallowed/">FromPlatformFlagConstDisallowed</a></td>
<td>FROM --platform flag should not use a constant value</td>
</tr>
<tr>
<td><a href="./copy-ignored-file/">CopyIgnoredFile</a></td>
<td>Attempting to Copy file that is excluded by .dockerignore</td>
</tr>
</tbody>
</table>
Loading

0 comments on commit e83d79a

Please sign in to comment.