HIP-0019 adds .helmlintignore capability

See HIP-0019 proposal at helm/community: https://github.com/helm/community/pull/353 Co-authored-by: Danilo Patrucco <danilo.patrucco@gmail.com> Signed-off-by: Daniel J. Pritchett <dpritchett@radiusmethod.com>
1 year ago · f93c2554ad
parent c86e0d3ad9
commit f93c2554ad
5 changed files with 517 additions and 0 deletions
--- a/pkg/lint/ignore/doc.go
+++ 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"
--- a/pkg/lint/ignore/ignorer.go
+++ 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...))
+}
--- a/pkg/lint/ignore/matchers.go
+++ 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
+}
--- a/pkg/lint/ignore/rule_loaders.go
+++ 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}
+	}
+}
--- a/pkg/lint/ignore/rule_test.go
+++ 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 <include \"gitlab.gitaly.includeInternalResources\" $>\n",
+			Ignorables: []support.Message{{
+				Path: "templates/",
+				Err:  fmt.Errorf("template: gitaly/templates/statefulset.yml:1:11: executing \"gitaly/templates/statefulset.yml\" at <include \"gitlab.gitaly.includeInternalResources\" $>: 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 <include \"gitlab.standardLabels\" .>: 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 <include \"fullname\" .>",
+			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 <include \"fullname\" .>: 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 <include (print $.Template.BasePath \"/_serviceaccountspec.yaml\") .>",
+			Ignorables: []support.Message{{
+				Path: "templates/",
+				Err:  fmt.Errorf("template: migrations/templates/job.yaml:2:3: executing \"migrations/templates/job.yaml\" at <include (print $.Template.BasePath \"/_serviceaccountspec.yaml\") .>: 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)
+		})
+	}
+}