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

Proposal: Experimental Rulechecks #5213

Merged
merged 2 commits into from
Aug 13, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion frontend/dockerfile/dockerfile2llb/convert.go
Original file line number Diff line number Diff line change
Expand Up @@ -596,7 +596,7 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
ctxPaths := map[string]struct{}{}

var dockerIgnoreMatcher *patternmatcher.PatternMatcher
if opt.Client != nil && opt.Client.CopyIgnoredCheckEnabled {
if opt.Client != nil {
dockerIgnorePatterns, err := opt.Client.DockerIgnorePatterns(ctx)
if err != nil {
return nil, err
Expand Down
42 changes: 38 additions & 4 deletions frontend/dockerfile/dockerfile_lint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,46 @@ ADD Dockerfile /windy
StreamBuildErrRegexp: regexp.MustCompile(`failed to solve: failed to compute cache key: failed to calculate checksum of ref [^\s]+ "/Dockerfile": not found`),
})

dockerfile = []byte(`# check=experimental=CopyIgnoredFile
FROM scratch
COPY Dockerfile .
ADD Dockerfile /windy
`)

checkLinterWarnings(t, sb, &lintTestParams{
Dockerfile: dockerfile,
DockerIgnore: dockerignore,
FrontendAttrs: map[string]string{
"build-arg:BUILDKIT_DOCKERFILE_CHECK_COPYIGNORED_EXPERIMENT": "true",
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`),
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,
},
},
})

dockerfile = []byte(`# check=skip=all;experimental=CopyIgnoredFile
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`),
Warnings: []expectedLintWarning{
Expand Down
89 changes: 63 additions & 26 deletions frontend/dockerfile/linter/linter.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,42 +10,61 @@ import (
)

type Config struct {
Warn LintWarnFunc
SkipRules []string
SkipAll bool
ReturnAsError bool
ExperimentalAll bool
ExperimentalRules []string
ReturnAsError bool
SkipAll bool
SkipRules []string
Warn LintWarnFunc
}

type Linter struct {
SkippedRules map[string]struct{}
CalledRules []string
SkipAll bool
ReturnAsError bool
Warn LintWarnFunc
CalledRules []string
ExperimentalAll bool
ExperimentalRules map[string]struct{}
ReturnAsError bool
SkipAll bool
SkippedRules map[string]struct{}
Warn LintWarnFunc
}

func New(config *Config) *Linter {
toret := &Linter{
SkippedRules: map[string]struct{}{},
CalledRules: []string{},
Warn: config.Warn,
SkippedRules: map[string]struct{}{},
ExperimentalRules: map[string]struct{}{},
CalledRules: []string{},
Warn: config.Warn,
}
toret.SkipAll = config.SkipAll
toret.ExperimentalAll = config.ExperimentalAll
toret.ReturnAsError = config.ReturnAsError
for _, rule := range config.SkipRules {
toret.SkippedRules[rule] = struct{}{}
}
for _, rule := range config.ExperimentalRules {
toret.ExperimentalRules[rule] = struct{}{}
}
return toret
}

func (lc *Linter) Run(rule LinterRuleI, location []parser.Range, txt ...string) {
if lc == nil || lc.Warn == nil || lc.SkipAll || rule.IsDeprecated() {
if lc == nil || lc.Warn == nil || rule.IsDeprecated() {
return
}

rulename := rule.RuleName()
if _, ok := lc.SkippedRules[rulename]; ok {
return
if rule.IsExperimental() {
_, experimentalOk := lc.ExperimentalRules[rulename]
if !(lc.ExperimentalAll || experimentalOk) {
return
}
} else {
_, skipOk := lc.SkippedRules[rulename]
if lc.SkipAll || skipOk {
return
}
}

lc.CalledRules = append(lc.CalledRules, rulename)
rule.Run(lc.Warn, location, txt...)
}
Expand All @@ -72,14 +91,16 @@ type LinterRuleI interface {
RuleName() string
Run(warn LintWarnFunc, location []parser.Range, txt ...string)
IsDeprecated() bool
IsExperimental() bool
}

type LinterRule[F any] struct {
Name string
Description string
Deprecated bool
URL string
Format F
Name string
Description string
Deprecated bool
Experimental bool
URL string
Format F
}

func (rule *LinterRule[F]) RuleName() string {
Expand All @@ -98,6 +119,10 @@ func (rule *LinterRule[F]) IsDeprecated() bool {
return rule.Deprecated
}

func (rule *LinterRule[F]) IsExperimental() bool {
return rule.Experimental
}

func LintFormatShort(rulename, msg string, line int) string {
msg = fmt.Sprintf("%s: %s", rulename, msg)
if line > 0 {
Expand All @@ -114,9 +139,9 @@ func ParseLintOptions(checkStr string) (*Config, error) {
return &Config{}, nil
}

parts := strings.SplitN(checkStr, ";", 2)
var skipSet []string
var errorOnWarn, skipAll bool
parts := strings.SplitN(checkStr, ";", 3)
var skipSet, experimentalSet []string
var errorOnWarn, skipAll, experimentalAll bool
for _, p := range parts {
k, v, ok := strings.Cut(p, "=")
if !ok {
Expand All @@ -134,6 +159,16 @@ func ParseLintOptions(checkStr string) (*Config, error) {
skipSet[i] = strings.TrimSpace(rule)
}
}
case "experimental":
v = strings.TrimSpace(v)
if v == "all" {
experimentalAll = true
} else {
experimentalSet = strings.Split(v, ",")
for i, rule := range experimentalSet {
experimentalSet[i] = strings.TrimSpace(rule)
}
}
case "error":
v, err := strconv.ParseBool(strings.TrimSpace(v))
if err != nil {
Expand All @@ -145,8 +180,10 @@ func ParseLintOptions(checkStr string) (*Config, error) {
}
}
return &Config{
SkipRules: skipSet,
SkipAll: skipAll,
ReturnAsError: errorOnWarn,
ExperimentalAll: experimentalAll,
ExperimentalRules: experimentalSet,
SkipRules: skipSet,
SkipAll: skipAll,
ReturnAsError: errorOnWarn,
}, nil
}
1 change: 1 addition & 0 deletions frontend/dockerfile/linter/ruleset.go
Original file line number Diff line number Diff line change
Expand Up @@ -163,5 +163,6 @@ var (
Format: func(cmd, file string) string {
return fmt.Sprintf("Attempting to %s file %q that is excluded by .dockerignore", cmd, file)
},
Experimental: true,
daghack marked this conversation as resolved.
Show resolved Hide resolved
}
)
50 changes: 19 additions & 31 deletions frontend/dockerui/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,30 +46,28 @@ const (

// Don't forget to update frontend documentation if you add
// a new build-arg: frontend/dockerfile/docs/reference.md
keyCacheNSArg = "build-arg:BUILDKIT_CACHE_MOUNT_NS"
keyMultiPlatformArg = "build-arg:BUILDKIT_MULTI_PLATFORM"
keyHostnameArg = "build-arg:BUILDKIT_SANDBOX_HOSTNAME"
keyDockerfileLintArg = "build-arg:BUILDKIT_DOCKERFILE_CHECK"
keyContextKeepGitDirArg = "build-arg:BUILDKIT_CONTEXT_KEEP_GIT_DIR"
keySourceDateEpoch = "build-arg:SOURCE_DATE_EPOCH"
keyCopyIgnoredCheckEnabled = "build-arg:BUILDKIT_DOCKERFILE_CHECK_COPYIGNORED_EXPERIMENT"
keyCacheNSArg = "build-arg:BUILDKIT_CACHE_MOUNT_NS"
keyMultiPlatformArg = "build-arg:BUILDKIT_MULTI_PLATFORM"
keyHostnameArg = "build-arg:BUILDKIT_SANDBOX_HOSTNAME"
keyDockerfileLintArg = "build-arg:BUILDKIT_DOCKERFILE_CHECK"
keyContextKeepGitDirArg = "build-arg:BUILDKIT_CONTEXT_KEEP_GIT_DIR"
keySourceDateEpoch = "build-arg:SOURCE_DATE_EPOCH"
)

type Config struct {
BuildArgs map[string]string
CacheIDNamespace string
CgroupParent string
Epoch *time.Time
ExtraHosts []llb.HostIP
Hostname string
ImageResolveMode llb.ResolveMode
Labels map[string]string
NetworkMode pb.NetMode
ShmSize int64
Target string
Ulimits []pb.Ulimit
LinterConfig *linter.Config
CopyIgnoredCheckEnabled bool
BuildArgs map[string]string
CacheIDNamespace string
CgroupParent string
Epoch *time.Time
ExtraHosts []llb.HostIP
Hostname string
ImageResolveMode llb.ResolveMode
Labels map[string]string
NetworkMode pb.NetMode
ShmSize int64
Target string
Ulimits []pb.Ulimit
LinterConfig *linter.Config

CacheImports []client.CacheOptionsEntry
TargetPlatforms []ocispecs.Platform // nil means default
Expand Down Expand Up @@ -291,16 +289,6 @@ func (bc *Client) init() error {
}
}

// CopyIgnoredCheckEnabled is an experimental feature to check if COPY is ignored by .dockerignore,
// and it is disabled by default. It is expected that this feature will be enabled by default in a future
// release, and this build-arg will be removed.
if v, ok := opts[keyCopyIgnoredCheckEnabled]; ok {
bc.CopyIgnoredCheckEnabled, err = strconv.ParseBool(v)
if err != nil {
return errors.Wrapf(err, "failed to parse %s", keyCopyIgnoredCheckEnabled)
}
}

bc.localsSessionIDs = parseLocalSessionIDs(opts)

return nil
Expand Down
Loading