Merge pull request moby#5213 from daghack/experimental-rule-checks

Proposal: Experimental Rulechecks

Author: tonistiigi

frontend/dockerfile/dockerfile2llb/convert.go

@@ -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

frontend/dockerfile/dockerfile_lint_test.go

@@ -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{

frontend/dockerfile/docs/rules/_index.md

@@ -97,7 +97,7 @@ $ docker build --check .
       <td>FROM --platform flag should not use a constant value</td>
     </tr>
     <tr>
-      <td><a href="./copy-ignored-file/">CopyIgnoredFile</a></td>
+      <td><a href="./copy-ignored-file/">CopyIgnoredFile (experimental)</a></td>
       <td>Attempting to Copy file that is excluded by .dockerignore</td>
     </tr>
   </tbody>

frontend/dockerfile/docs/rules/copy-ignored-file.md

@@ -5,6 +5,11 @@ aliases:
   - /go/dockerfile/rule/copy-ignored-file/
 ---
 
+> **Note**
+>
+> This check is experimental and is not enabled by default. To enable it, see
+> [Experimental checks](https://docs.docker.com/go/build-checks-experimental/).
+
 ## Output
 
 ```text

frontend/dockerfile/linter/docs/_index.md

@@ -30,7 +30,7 @@ $ docker build --check .
   <tbody>
     {{- range .Rules }}
     <tr>
-      <td><a href="./{{ .PageName }}/">{{ .Name }}</a></td>
+      <td><a href="./{{ .PageName }}/">{{ .Name }}{{- if .Experimental }} (experimental){{- end}}</a></td>
       <td>{{ .Description }}</td>
     </tr>
     {{- end }}

frontend/dockerfile/linter/generate.go

@@ -20,11 +20,12 @@ import (
 )
 
 type Rule struct {
-	Name        string
-	Description string
-	URL         *url.URL
-	PageName    string
-	URLAlias    string
+	Name         string
+	Description  string
+	URL          *url.URL
+	PageName     string
+	URLAlias     string
+	Experimental bool
 }
 
 const tmplStr = `---
@@ -35,6 +36,13 @@ aliases:
   - {{ .Rule.URLAlias }}
 {{- end }}
 ---
+{{- if .Rule.Experimental }}
+
+> **Note**
+>
+> This check is experimental and is not enabled by default. To enable it, see
+> [Experimental checks](https://docs.docker.com/go/build-checks-experimental/).
+{{- end }}
 
 {{ .Content }}
 `
@@ -160,6 +168,10 @@ func listRules() ([]Rule, error) {
 										}
 										rule.URL = u
 									}
+								case "Experimental":
+									if basicLit, ok := kv.Value.(*ast.Ident); ok {
+										rule.Experimental = basicLit.Name == "true"
+									}
 								}
 							}
 						}

frontend/dockerfile/linter/linter.go

@@ -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...)
 }
@@ -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 {
@@ -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 {
@@ -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 {
@@ -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 {
@@ -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
 }

frontend/dockerfile/linter/ruleset.go

@@ -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,
 	}
 )

frontend/dockerui/config.go

@@ -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
@@ -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