From f93c2554ada89f04315a79cc3bd5e61d8a498779 Mon Sep 17 00:00:00 2001 From: "Daniel J. Pritchett" Date: Fri, 16 Aug 2024 14:41:34 -0500 Subject: [PATCH] HIP-0019 adds .helmlintignore capability See HIP-0019 proposal at helm/community: https://github.com/helm/community/pull/353 Co-authored-by: Danilo Patrucco Signed-off-by: Daniel J. Pritchett --- pkg/lint/ignore/doc.go | 24 ++++ pkg/lint/ignore/ignorer.go | 62 +++++++++ pkg/lint/ignore/matchers.go | 78 +++++++++++ pkg/lint/ignore/rule_loaders.go | 113 +++++++++++++++ pkg/lint/ignore/rule_test.go | 240 ++++++++++++++++++++++++++++++++ 5 files changed, 517 insertions(+) create mode 100644 pkg/lint/ignore/doc.go create mode 100644 pkg/lint/ignore/ignorer.go create mode 100644 pkg/lint/ignore/matchers.go create mode 100644 pkg/lint/ignore/rule_loaders.go create mode 100644 pkg/lint/ignore/rule_test.go diff --git a/pkg/lint/ignore/doc.go b/pkg/lint/ignore/doc.go new file mode 100644 index 000000000..76606b952 --- /dev/null +++ b/pkg/lint/ignore/doc.go @@ -0,0 +1,24 @@ +/* +Copyright The Helm Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +//Package ignore +/* +Package ignore contains tools for linting charts. + +Linting is the process of testing charts for errors or warnings regarding +formatting, compilation, or standards compliance. +*/ +package ignore // import "helm.sh/helm/v3/pkg/lint/ignore" diff --git a/pkg/lint/ignore/ignorer.go b/pkg/lint/ignore/ignorer.go new file mode 100644 index 000000000..98baf25e9 --- /dev/null +++ b/pkg/lint/ignore/ignorer.go @@ -0,0 +1,62 @@ +package ignore + +import ( + "fmt" + "helm.sh/helm/v3/pkg/lint/support" + "log" +) + +// DefaultIgnoreFileName is the name of the lint ignore file +const DefaultIgnoreFileName = ".helmlintignore" + +var debugFn func(format string, v ...interface{}) + +type Ignorer struct { + ChartPath string + Matchers []MatchesErrors +} + +func NewIgnorer(chartPath string, lintIgnorePath string, debugLogFn func(string, ...interface{})) (*Ignorer, error) { + debugFn = debugLogFn + matchers, err := LoadFromFilePath(chartPath, lintIgnorePath) + if err != nil { + return nil, err + } + + return &Ignorer{ChartPath: chartPath, Matchers: matchers}, nil +} + +// FilterMessages Verify what messages can be kept in the output, using also the error as a verification +func (i *Ignorer) FilterMessages(messages []support.Message) []support.Message { + out := make([]support.Message, 0, len(messages)) + for _, msg := range messages { + if i.ShouldKeepMessage(msg) { + out = append(out, msg) + } + } + return out +} + +func (i *Ignorer) ShouldKeepMessage(msg support.Message) bool { + return i.ShouldKeepError(msg.Err) +} + +// ShouldKeepError is used to verify if the error associated with the message need to be kept, or it can be ignored, called by FilterMessages and in the pkg/action/lint.go Run main function +func (i *Ignorer) ShouldKeepError(err error) bool { + // if any of our Matchers match the rule, we can discard it + for _, rule := range i.Matchers { + if rule.Match(err) { + debug("lint ignore rule matched this error, we should suppress it.", "errText", err.Error(), rule.LogAttrs()) + return false + } + } + + // if we can't find a reason to discard it, we keep it + debug("no lint ignore rules matched this error, we should NOT suppress it.", "errText", err.Error()) + return true +} + +var defaultDebugFn = func(format string, v ...interface{}) { + format = fmt.Sprintf("[debug] %s\n", format) + _ = log.Output(2, fmt.Sprintf(format, v...)) +} diff --git a/pkg/lint/ignore/matchers.go b/pkg/lint/ignore/matchers.go new file mode 100644 index 000000000..46a91620b --- /dev/null +++ b/pkg/lint/ignore/matchers.go @@ -0,0 +1,78 @@ +package ignore + +import ( + "log/slog" + "path/filepath" + "strings" +) + +const pathlessRulePrefix = "error_lint_ignore=" + +type MatchesErrors interface { + Match(error) bool + LogAttrs() slog.Attr +} + +type BadTemplateRule struct { + RuleText string + MessageText string + BadTemplatePath string +} + +type PathlessRule struct { + RuleText string + MessageText string +} + +// Match errors that have no file path in their body with ignorer rules. +// An examples of errors with no file path in their body is chart metadata errors `chart metadata is missing these dependencies` +func (pr PathlessRule) Match(err error) bool { + errText := err.Error() + matchableParts := strings.SplitN(pr.MessageText, ":", 2) + matchablePrefix := strings.TrimSpace(matchableParts[0]) + + if match, _ := filepath.Match(pr.MessageText, errText); match { + return true + } + if matched, _ := filepath.Match(matchablePrefix, errText); matched { + return true + } + + return false +} + +// LogAttrs Used for troubleshooting and gathering data +func (pr PathlessRule) LogAttrs() slog.Attr { + return slog.Group("BadTemplateRule", + slog.String("rule_text", pr.RuleText), + slog.String("value", pr.MessageText), + ) +} + +// LogAttrs Used for troubleshooting and gathering data +func (btr BadTemplateRule) LogAttrs() slog.Attr { + return slog.Group("BadTemplateRule", + slog.String("rule_text", btr.RuleText), + slog.String("key", btr.BadTemplatePath), + slog.String("value", btr.MessageText), + ) +} + +// Match errors that have a file path in their body with ignorer rules. +func (btr BadTemplateRule) Match(err error) bool { + errText := err.Error() + pathToOffendingFile, err := pathToOffendingFile(errText) + if err != nil { + return false + } + + cleanRulePath := filepath.Clean(btr.BadTemplatePath) + + if strings.Contains(pathToOffendingFile, cleanRulePath) { + if strings.Contains(errText, btr.MessageText) { + return true + } + } + + return false +} diff --git a/pkg/lint/ignore/rule_loaders.go b/pkg/lint/ignore/rule_loaders.go new file mode 100644 index 000000000..cff16c77b --- /dev/null +++ b/pkg/lint/ignore/rule_loaders.go @@ -0,0 +1,113 @@ +package ignore + +import ( + "bufio" + "fmt" + "io" + "os" + "path/filepath" + "strings" +) + +func LoadFromFilePath(chartPath, ignoreFilePath string) ([]MatchesErrors, error) { + if ignoreFilePath == "" { + ignoreFilePath = filepath.Join(chartPath, DefaultIgnoreFileName) + debug("\nNo helm lint ignore filepath specified, will try and use the following default: %s\n", ignoreFilePath) + } + + // attempt to load ignore patterns from ignoreFilePath. + // if none are found, return an empty ignorer so the program can keep running. + debug("\nTrying to load helm lint ignore file at %s\n", ignoreFilePath) + file, err := os.Open(ignoreFilePath) + if err != nil { + debug("failed to open helm lint ignore file: %s", ignoreFilePath) + return []MatchesErrors{}, nil + } + defer func() { + err := file.Close() + if err != nil { + debug("failed to close helm lint ignore file: %s", ignoreFilePath) + } + }() + + matchers := LoadFromReader(file) + return matchers, nil +} + +// debug provides [pkg/lint/ignore] with a runtime-overridable logging function +// intended to match the behavior of the top level debug() method from package main. +// +// When no debugFn is set for the package at runtime then debug will fall back to +// defaultDebugFn. +func debug(format string, args ...interface{}) { + if debugFn == nil { + defaultDebugFn(format, args...) + } else { + debugFn(format, args...) + } + return +} + +// TODO: figure out & fix or remove +func pathToOffendingFile(errText string) (string, error) { + delimiter := ":" + // splits into N parts delimited by colons + parts := strings.Split(errText, delimiter) + // if 3 or more parts, return the second part, after trimming its spaces + if len(parts) > 2 { + return strings.TrimSpace(parts[1]), nil + } + // if fewer than 3 parts, return empty string + return "", fmt.Errorf("fewer than three [%s]-delimited parts found, no path here: %s", delimiter, errText) +} + +func LoadFromReader(rdr io.Reader) []MatchesErrors { + matchers := make([]MatchesErrors, 0) + + scanner := bufio.NewScanner(rdr) + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + + if strings.HasPrefix(line, pathlessRulePrefix) { + matchers = append(matchers, buildPathlessRule(line, pathlessRulePrefix)) + } else { + matchers = append(matchers, buildBadTemplateRule(line)) + } + } + + return matchers +} + +func buildPathlessRule(line string, pathlessRulePrefix string) PathlessRule { + // handle chart-level errors + // Drop 'error_lint_ignore=' prefix from rule before saving it + const numSplits = 2 + tokens := strings.SplitN(line[len(pathlessRulePrefix):], pathlessRulePrefix, numSplits) + if len(tokens) == numSplits { + // TODO: find an example for this one - not sure we still use it + messageText, _ := tokens[0], tokens[1] + return PathlessRule{RuleText: line, MessageText: messageText} + } else { + messageText := tokens[0] + return PathlessRule{RuleText: line, MessageText: messageText} + } +} + +func buildBadTemplateRule(line string) BadTemplateRule { + const noMessageText = "" + const separator = " " + const numSplits = 2 + + // handle chart yaml file errors in specific template files + parts := strings.SplitN(line, separator, numSplits) + if len(parts) == numSplits { + messagePath, messageText := parts[0], parts[1] + return BadTemplateRule{RuleText: line, BadTemplatePath: messagePath, MessageText: messageText} + } else { + messagePath := parts[0] + return BadTemplateRule{RuleText: line, BadTemplatePath: messagePath, MessageText: noMessageText} + } +} diff --git a/pkg/lint/ignore/rule_test.go b/pkg/lint/ignore/rule_test.go new file mode 100644 index 000000000..04e88bb73 --- /dev/null +++ b/pkg/lint/ignore/rule_test.go @@ -0,0 +1,240 @@ +package ignore + +import ( + "fmt" + "github.com/stretchr/testify/assert" + "helm.sh/helm/v3/pkg/lint/support" + "strings" + "testing" +) + +func TestRule_ShouldKeepMessage(t *testing.T) { + type testCase struct { + Scenario string + RuleText string + Ignorables []support.Message + } + + testCases := []testCase{ + { + Scenario: "subchart template not defined", + RuleText: "gitlab/charts/webservice/templates/tests/tests.yaml <{{template \"fullname\" .}}>", + Ignorables: []support.Message{{ + Path: "templates/", + Err: fmt.Errorf("template: gitlab/charts/webservice/templates/tests/tests.yaml:5:20: executing \"gitlab/charts/webservice/templates/tests/tests.yaml\" at <{{template \"fullname\" .}}>: template \"fullname\" not defined"), + }}, + }, { + Scenario: "subchart template include template not found", + RuleText: "gitaly/templates/statefulset.yml \n", + Ignorables: []support.Message{{ + Path: "templates/", + Err: fmt.Errorf("template: gitaly/templates/statefulset.yml:1:11: executing \"gitaly/templates/statefulset.yml\" at : error calling include: template: no template \"gitlab.gitaly.includeInternalResources\" associated with template \"gotpl\""), + }}, + }, + { + Scenario: "subchart template evaluation has a nil pointer", + RuleText: "gitlab-exporter/templates/serviceaccount.yaml <.Values.global.serviceAccount.enabled>\n", + Ignorables: []support.Message{{ + Path: "templates/", + Err: fmt.Errorf("template: gitlab-exporter/templates/serviceaccount.yaml:1:57: executing \"gitlab-exporter/templates/serviceaccount.yaml\" at <.Values.global.serviceAccount.enabled>: nil pointer evaluating interface {}.enabled"), + }}, + }, { + Scenario: "webservice path only", + RuleText: "webservice/templates/tests/tests.yaml", + Ignorables: []support.Message{{ + Path: "templates/", + Err: fmt.Errorf("template: webservice/templates/tests/tests.yaml:8:8: executing \"webservice/templates/tests/tests.yaml\" at : error calling include: template: no template \"gitlab.standardLabels\" associated with template \"gotpl\""), + }}, + }, { + Scenario: "geo-logcursor path only", + RuleText: "geo-logcursor/templates/serviceaccount.yaml <.Values.global.serviceAccount.enabled>", + Ignorables: []support.Message{{ + Path: "templates/", + Err: fmt.Errorf("template: geo-logcursor/templates/serviceaccount.yaml:1:57: executing \"geo-logcursor/templates/serviceaccount.yaml\" at <.Values.global.serviceAccount.enabled>: nil pointer evaluating interface {}.enabled"), + }}, + }, { + Scenario: "webservice path only", + RuleText: "webservice/templates/service.yaml ", + Ignorables: []support.Message{{ + Path: "templates/", + Err: fmt.Errorf("template: gitlab/charts/gitlab/charts/webservice/templates/service.yaml:14:11: executing \"gitlab/charts/gitlab/charts/webservice/templates/service.yaml\" at : error calling include: template: gitlab/templates/_helpers.tpl:14:27: executing \"fullname\" at <.Chart.Name>: nil pointer evaluating interface {}.Name"), + }}, + }, { + Scenario: "certmanager-issuer path only", + RuleText: "certmanager-issuer/templates/rbac-config.yaml <.Values.global.ingress>", + Ignorables: []support.Message{{ + Path: "templates/", + Err: fmt.Errorf("template: certmanager-issuer/templates/rbac-config.yaml:1:67: executing \"certmanager-issuer/templates/rbac-config.yaml\" at <.Values.global.ingress>: nil pointer evaluating interface {}.ingress"), + }}, + }, { + Scenario: "gitlab-pages path only", + RuleText: "gitlab-pages/templates/serviceaccount.yaml <.Values.global.serviceAccount.enabled>", + Ignorables: []support.Message{{ + Path: "templates/", + Err: fmt.Errorf("template: gitlab-pages/templates/serviceaccount.yaml:1:57: executing \"gitlab-pages/templates/serviceaccount.yaml\" at <.Values.global.serviceAccount.enabled>: nil pointer evaluating interface {}.enabled"), + }}, + }, { + Scenario: "gitlab-shell path only", + RuleText: "gitlab-shell/templates/traefik-tcp-ingressroute.yaml <.Values.global.ingress.provider>", + Ignorables: []support.Message{{ + Path: "templates/", + Err: fmt.Errorf("template: gitlab-shell/templates/traefik-tcp-ingressroute.yaml:2:17: executing \"gitlab-shell/templates/traefik-tcp-ingressroute.yaml\" at <.Values.global.ingress.provider>: nil pointer evaluating interface {}.provider"), + }}, + }, { + Scenario: "kas path only", + RuleText: "kas/templates/serviceaccount.yaml <.Values.global.serviceAccount.enabled>", + Ignorables: []support.Message{{ + Path: "templates/", + Err: fmt.Errorf("template: kas/templates/serviceaccount.yaml:1:57: executing \"kas/templates/serviceaccount.yaml\" at <.Values.global.serviceAccount.enabled>: nil pointer evaluating interface {}.enabled"), + }}, + }, { + Scenario: "kas path only", + RuleText: "kas/templates/serviceaccount.yaml <.Values.global.serviceAccount.enabled>", + Ignorables: []support.Message{{ + Path: "templates/", + Err: fmt.Errorf("template: kas/templates/serviceaccount.yaml:1:57: executing \"kas/templates/serviceaccount.yaml\" at <.Values.global.serviceAccount.enabled>: nil pointer evaluating interface {}.enabled"), + }}, + }, { + Scenario: "mailroom path only", + RuleText: "mailroom/templates/serviceaccount.yaml <.Values.global.serviceAccount.enabled>", + Ignorables: []support.Message{{ + Path: "templates/", + Err: fmt.Errorf("template: mailroom/templates/serviceaccount.yaml:1:57: executing \"mailroom/templates/serviceaccount.yaml\" at <.Values.global.serviceAccount.enabled>: nil pointer evaluating interface {}.enabled"), + }}, + }, { + Scenario: "migrations path only", + RuleText: "migrations/templates/job.yaml ", + Ignorables: []support.Message{{ + Path: "templates/", + Err: fmt.Errorf("template: migrations/templates/job.yaml:2:3: executing \"migrations/templates/job.yaml\" at : error calling include: template: migrations/templates/_serviceaccountspec.yaml:1:57: executing \"migrations/templates/_serviceaccountspec.yaml\" at <.Values.global.serviceAccount.enabled>: nil pointer evaluating interface {}.enabled"), + }}, + }, { + Scenario: "praefect path only", + RuleText: "praefect/templates/statefulset.yaml <.Values.global.image>", + Ignorables: []support.Message{{ + Path: "templates/", + Err: fmt.Errorf("template: praefect/templates/statefulset.yaml:1:38: executing \"praefect/templates/statefulset.yaml\" at <.Values.global.image>: nil pointer evaluating interface {}.image"), + }}, + }, { + Scenario: "sidekiq path only", + RuleText: "sidekiq/templates/serviceaccount.yaml <.Values.global.serviceAccount.enabled>", + Ignorables: []support.Message{{ + Path: "templates/", + Err: fmt.Errorf("template: sidekiq/templates/serviceaccount.yaml:1:57: executing \"sidekiq/templates/serviceaccount.yaml\" at <.Values.global.serviceAccount.enabled>: nil pointer evaluating interface {}.enabled"), + }}, + }, { + Scenario: "spamcheck path only", + RuleText: "spamcheck/templates/serviceaccount.yaml <.Values.global.serviceAccount.enabled>", + Ignorables: []support.Message{{ + Path: "templates/", + Err: fmt.Errorf("template: spamcheck/templates/serviceaccount.yaml:1:57: executing \"spamcheck/templates/serviceaccount.yaml\" at <.Values.global.serviceAccount.enabled>: nil pointer evaluating interface {}.serviceAccount"), + }}, + }, { + Scenario: "toolbox path only", + RuleText: "toolbox/templates/serviceaccount.yaml <.Values.global.serviceAccount.enabled>", + Ignorables: []support.Message{{ + Path: "templates/", + Err: fmt.Errorf("template: toolbox/templates/serviceaccount.yaml:1:57: executing \"toolbox/templates/serviceaccount.yaml\" at <.Values.global.serviceAccount.enabled>: nil pointer evaluating interface {}.enabled"), + }}, + }, { + Scenario: "minio path only", + RuleText: "minio/templates/pdb.yaml <{{template \"gitlab.pdb.apiVersion\" $pdbCfg}}>", + Ignorables: []support.Message{{ + Path: "templates/", + Err: fmt.Errorf("template: minio/templates/pdb.yaml:3:24: executing \"minio/templates/pdb.yaml\" at <{{template \"gitlab.pdb.apiVersion\" $pdbCfg}}>: template \"gitlab.pdb.apiVersion\" not defined"), + }}, + }, { + Scenario: "nginx-ingress path only", + RuleText: "nginx-ingress/templates/admission-webhooks/job-patch/serviceaccount.yaml <.Values.admissionWebhooks.serviceAccount.automountServiceAccountToken>", + Ignorables: []support.Message{{ + Path: "templates/", + Err: fmt.Errorf("template: nginx-ingress/templates/admission-webhooks/job-patch/serviceaccount.yaml:13:40: executing \"nginx-ingress/templates/admission-webhooks/job-patch/serviceaccount.yaml\" at <.Values.admissionWebhooks.serviceAccount.automountServiceAccountToken>: nil pointer evaluating interface {}.serviceAccount"), + }}, + }, { + Scenario: "registry path only", + RuleText: "registry/templates/serviceaccount.yaml <.Values.global.serviceAccount.enabled>", + Ignorables: []support.Message{{ + Path: "templates/", + Err: fmt.Errorf("template: registry/templates/serviceaccount.yaml:1:57: executing \"registry/templates/serviceaccount.yaml\" at <.Values.global.serviceAccount.enabled>: nil pointer evaluating interface {}.enabled"), + }}, + }, + + { + Scenario: "subchart metadata missing dependencies", + RuleText: "error_lint_ignore=chart metadata is missing these dependencies**", + Ignorables: []support.Message{ + { + Path: "gitlab/chart/charts/gitlab", + Err: fmt.Errorf("chart metadata is missing these dependencies: sidekiq,spamcheck,gitaly,gitlab-shell,kas,mailroom,migrations,toolbox,geo-logcursor,gitlab-exporter,webservice"), + }, + }, + }, + { + Scenario: "subchart icon is recommended", + RuleText: "error_lint_ignore=icon is recommended", + Ignorables: []support.Message{{ + Path: "Chart.yaml", + Err: fmt.Errorf("icon is recommended"), + }}, + }, + { + Scenario: "subchart values file does not exist", + RuleText: "error_lint_ignore=file does not exist", + Ignorables: []support.Message{ + {Path: "values.yaml", Err: fmt.Errorf("file does not exist")}, + }, + }, + { + Scenario: "Chart.yaml missing apiVersion", + RuleText: "error_lint_ignore=apiVersion is required. The value must be either \"v1\" or \"v2\"", + Ignorables: []support.Message{ + { + Severity: support.ErrorSev, + Path: "Chart.yaml", + Err: fmt.Errorf("apiVersion is required. The value must be either \"v1\" or \"v2\""), + }, + }, + }, + { + Scenario: "values.yaml does not exist", + RuleText: "error_lint_ignore=file does not exist", + Ignorables: []support.Message{ + { + Severity: support.InfoSev, + Path: "values.yaml", + Err: fmt.Errorf("file does not exist"), + }, + }, + }, + { + Scenario: "missing dependencies in chart directory", + RuleText: "error_lint_ignore=chart directory is missing these dependencies: mariadb", + Ignorables: []support.Message{ + { + Severity: support.WarningSev, + Path: "/Users/daniel/radius/bb/helm/pkg/action/testdata/charts/chart-missing-deps-but-ignorable", + Err: fmt.Errorf("chart directory is missing these dependencies: mariadb"), + }, + }, + }, + } + for _, testCase := range testCases { + t.Run(testCase.Scenario, func(t *testing.T) { + matchers := LoadFromReader(strings.NewReader(testCase.RuleText)) + if len(matchers) == 0 { + t.Fatal("Expected a match, got none.", testCase.Scenario) + } + assert.Equal(t, 1, len(matchers)) + matcher := matchers[0] + + for _, ignorableMessage := range testCase.Ignorables { + got := matcher.Match(ignorableMessage.Err) + assert.True(t, got, testCase.Scenario) + } + + keepableMessage := support.NewMessage(support.ErrorSev, "wow/", fmt.Errorf("incredible: something just happened")) + got := matcher.Match(keepableMessage.Err) + assert.False(t, got, testCase.Scenario) + }) + } +}