diff --git a/cmd/helm/lint.go b/cmd/helm/lint.go index c52b23c78e7..50790b016b2 100644 --- a/cmd/helm/lint.go +++ b/cmd/helm/lint.go @@ -97,7 +97,7 @@ func (l *lintCmd) run() error { var total int var failures int for _, path := range l.paths { - linter, err := lintChart(path, rvals, l.namespace, l.strict) + linter, err := lintChart(path, rvals, l.namespace) if err != nil { failures = failures + 1 fmt.Println("==> Skipping", path) @@ -132,7 +132,7 @@ func (l *lintCmd) run() error { return nil } -func lintChart(path string, vals []byte, namespace string, strict bool) (support.Linter, error) { +func lintChart(path string, vals []byte, namespace string) (support.Linter, error) { var chartPath string linter := support.Linter{} @@ -171,7 +171,7 @@ func lintChart(path string, vals []byte, namespace string, strict bool) (support return linter, fmt.Errorf("unable to check Chart.yaml file in chart: %s", err.Error()) } - return lint.All(chartPath, vals, namespace, strict), nil + return lint.All(chartPath, vals, namespace), nil } // vals merges values from files specified via -f/--values and diff --git a/cmd/helm/lint_test.go b/cmd/helm/lint_test.go index 42487a34f47..097365bb66c 100644 --- a/cmd/helm/lint_test.go +++ b/cmd/helm/lint_test.go @@ -58,11 +58,10 @@ func TestLintChart(t *testing.T) { values := []byte{} namespace := "testNamespace" - strict := false for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - _, err := lintChart(tt.chartPath, values, namespace, strict) + _, err := lintChart(tt.chartPath, values, namespace) switch { case err != nil && !tt.err: t.Errorf("%s", err) @@ -107,3 +106,45 @@ func TestLinRunForNonExistentChart(t *testing.T) { } }) } + +func TestLintRunForStrictErrors(t *testing.T) { + out := bytes.NewBufferString("") + tests := []struct { + name string + chartPath string + strict bool + errStr string + wantErr bool + }{ + { + name: "non-strict should not error", + chartPath: "testdata/testcharts/bad.name", + strict: false, + wantErr: false, + }, + { + name: "strict should error", + chartPath: "testdata/testcharts/bad.name", + strict: true, + errStr: "1 chart(s) linted, 1 chart(s) failed", + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + testLint := &lintCmd{ + paths: []string{tt.chartPath}, + strict: tt.strict, + out: out, + } + expectedErr := fmt.Errorf(tt.errStr) + err := testLint.run() + if tt.wantErr && err == nil { + t.Errorf("expected error but got no error") + } + if err != nil && (err.Error() != expectedErr.Error()) { + t.Errorf("expected: \"%v\" , but got: \"%v\"", expectedErr, err) + } + }) + } +} diff --git a/cmd/helm/testdata/testcharts/bad.name/.helmignore b/cmd/helm/testdata/testcharts/bad.name/.helmignore new file mode 100644 index 00000000000..50af0317254 --- /dev/null +++ b/cmd/helm/testdata/testcharts/bad.name/.helmignore @@ -0,0 +1,22 @@ +# Patterns to ignore when building packages. +# This supports shell glob matching, relative path matching, and +# negation (prefixed with !). Only one pattern per line. +.DS_Store +# Common VCS dirs +.git/ +.gitignore +.bzr/ +.bzrignore +.hg/ +.hgignore +.svn/ +# Common backup files +*.swp +*.bak +*.tmp +*~ +# Various IDEs +.project +.idea/ +*.tmproj +.vscode/ diff --git a/cmd/helm/testdata/testcharts/bad.name/Chart.yaml b/cmd/helm/testdata/testcharts/bad.name/Chart.yaml new file mode 100644 index 00000000000..2554907dbc4 --- /dev/null +++ b/cmd/helm/testdata/testcharts/bad.name/Chart.yaml @@ -0,0 +1,5 @@ +apiVersion: v1 +appVersion: "1.0" +description: A Helm chart for Kubernetes +name: bad.name +version: 0.1.0 diff --git a/cmd/helm/testdata/testcharts/bad.name/templates/NOTES.txt b/cmd/helm/testdata/testcharts/bad.name/templates/NOTES.txt new file mode 100644 index 00000000000..ebb29d1d5a8 --- /dev/null +++ b/cmd/helm/testdata/testcharts/bad.name/templates/NOTES.txt @@ -0,0 +1,21 @@ +1. Get the application URL by running these commands: +{{- if .Values.ingress.enabled }} +{{- range $host := .Values.ingress.hosts }} + {{- range .paths }} + http{{ if $.Values.ingress.tls }}s{{ end }}://{{ $host.host }}{{ . }} + {{- end }} +{{- end }} +{{- else if contains "NodePort" .Values.service.type }} + export NODE_PORT=$(kubectl get --namespace {{ .Release.Namespace }} -o jsonpath="{.spec.ports[0].nodePort}" services {{ include "bad.name.fullname" . }}) + export NODE_IP=$(kubectl get nodes --namespace {{ .Release.Namespace }} -o jsonpath="{.items[0].status.addresses[0].address}") + echo http://$NODE_IP:$NODE_PORT +{{- else if contains "LoadBalancer" .Values.service.type }} + NOTE: It may take a few minutes for the LoadBalancer IP to be available. + You can watch the status of by running 'kubectl get --namespace {{ .Release.Namespace }} svc -w {{ include "bad.name.fullname" . }}' + export SERVICE_IP=$(kubectl get svc --namespace {{ .Release.Namespace }} {{ include "bad.name.fullname" . }} --template "{{"{{ range (index .status.loadBalancer.ingress 0) }}{{.}}{{ end }}"}}") + echo http://$SERVICE_IP:{{ .Values.service.port }} +{{- else if contains "ClusterIP" .Values.service.type }} + export POD_NAME=$(kubectl get pods --namespace {{ .Release.Namespace }} -l "app.kubernetes.io/name={{ include "bad.name.name" . }},app.kubernetes.io/instance={{ .Release.Name }}" -o jsonpath="{.items[0].metadata.name}") + echo "Visit http://127.0.0.1:8080 to use your application" + kubectl port-forward $POD_NAME 8080:80 +{{- end }} diff --git a/cmd/helm/testdata/testcharts/bad.name/templates/_helpers.tpl b/cmd/helm/testdata/testcharts/bad.name/templates/_helpers.tpl new file mode 100644 index 00000000000..6d33b003ae7 --- /dev/null +++ b/cmd/helm/testdata/testcharts/bad.name/templates/_helpers.tpl @@ -0,0 +1,56 @@ +{{/* vim: set filetype=mustache: */}} +{{/* +Expand the name of the chart. +*/}} +{{- define "bad.name.name" -}} +{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" -}} +{{- end -}} + +{{/* +Create a default fully qualified app name. +We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec). +If release name contains chart name it will be used as a full name. +*/}} +{{- define "bad.name.fullname" -}} +{{- if .Values.fullnameOverride -}} +{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" -}} +{{- else -}} +{{- $name := default .Chart.Name .Values.nameOverride -}} +{{- if contains $name .Release.Name -}} +{{- .Release.Name | trunc 63 | trimSuffix "-" -}} +{{- else -}} +{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" -}} +{{- end -}} +{{- end -}} +{{- end -}} + +{{/* +Create chart name and version as used by the chart label. +*/}} +{{- define "bad.name.chart" -}} +{{- printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" -}} +{{- end -}} + +{{/* +Common labels +*/}} +{{- define "bad.name.labels" -}} +app.kubernetes.io/name: {{ include "bad.name.name" . }} +helm.sh/chart: {{ include "bad.name.chart" . }} +app.kubernetes.io/instance: {{ .Release.Name }} +{{- if .Chart.AppVersion }} +app.kubernetes.io/version: {{ .Chart.AppVersion | quote }} +{{- end }} +app.kubernetes.io/managed-by: {{ .Release.Service }} +{{- end -}} + +{{/* +Create the name of the service account to use +*/}} +{{- define "bad.name.serviceAccountName" -}} +{{- if .Values.serviceAccount.create -}} + {{ default (include "bad.name.fullname" .) .Values.serviceAccount.name }} +{{- else -}} + {{ default "default" .Values.serviceAccount.name }} +{{- end -}} +{{- end -}} diff --git a/cmd/helm/testdata/testcharts/bad.name/templates/deployment.yaml b/cmd/helm/testdata/testcharts/bad.name/templates/deployment.yaml new file mode 100644 index 00000000000..49e79c0e3b9 --- /dev/null +++ b/cmd/helm/testdata/testcharts/bad.name/templates/deployment.yaml @@ -0,0 +1,57 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: {{ include "bad.name.fullname" . }} + labels: +{{ include "bad.name.labels" . | indent 4 }} +spec: + replicas: {{ .Values.replicaCount }} + selector: + matchLabels: + app.kubernetes.io/name: {{ include "bad.name.name" . }} + app.kubernetes.io/instance: {{ .Release.Name }} + template: + metadata: + labels: + app.kubernetes.io/name: {{ include "bad.name.name" . }} + app.kubernetes.io/instance: {{ .Release.Name }} + spec: + {{- with .Values.imagePullSecrets }} + imagePullSecrets: + {{- toYaml . | nindent 8 }} + {{- end }} + serviceAccountName: {{ template "bad.name.serviceAccountName" . }} + securityContext: + {{- toYaml .Values.podSecurityContext | nindent 8 }} + containers: + - name: {{ .Chart.Name }} + securityContext: + {{- toYaml .Values.securityContext | nindent 12 }} + image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}" + imagePullPolicy: {{ .Values.image.pullPolicy }} + ports: + - name: http + containerPort: 80 + protocol: TCP + livenessProbe: + httpGet: + path: / + port: http + readinessProbe: + httpGet: + path: / + port: http + resources: + {{- toYaml .Values.resources | nindent 12 }} + {{- with .Values.nodeSelector }} + nodeSelector: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.affinity }} + affinity: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.tolerations }} + tolerations: + {{- toYaml . | nindent 8 }} + {{- end }} diff --git a/cmd/helm/testdata/testcharts/bad.name/templates/ingress.yaml b/cmd/helm/testdata/testcharts/bad.name/templates/ingress.yaml new file mode 100644 index 00000000000..557075cc4a3 --- /dev/null +++ b/cmd/helm/testdata/testcharts/bad.name/templates/ingress.yaml @@ -0,0 +1,41 @@ +{{- if .Values.ingress.enabled -}} +{{- $fullName := include "bad.name.fullname" . -}} +{{- $svcPort := .Values.service.port -}} +{{- if semverCompare ">=1.14-0" .Capabilities.KubeVersion.GitVersion -}} +apiVersion: networking.k8s.io/v1beta1 +{{- else -}} +apiVersion: extensions/v1beta1 +{{- end }} +kind: Ingress +metadata: + name: {{ $fullName }} + labels: +{{ include "bad.name.labels" . | indent 4 }} + {{- with .Values.ingress.annotations }} + annotations: + {{- toYaml . | nindent 4 }} + {{- end }} +spec: +{{- if .Values.ingress.tls }} + tls: + {{- range .Values.ingress.tls }} + - hosts: + {{- range .hosts }} + - {{ . | quote }} + {{- end }} + secretName: {{ .secretName }} + {{- end }} +{{- end }} + rules: + {{- range .Values.ingress.hosts }} + - host: {{ .host | quote }} + http: + paths: + {{- range .paths }} + - path: {{ . }} + backend: + serviceName: {{ $fullName }} + servicePort: {{ $svcPort }} + {{- end }} + {{- end }} +{{- end }} diff --git a/cmd/helm/testdata/testcharts/bad.name/templates/service.yaml b/cmd/helm/testdata/testcharts/bad.name/templates/service.yaml new file mode 100644 index 00000000000..5587ba307ab --- /dev/null +++ b/cmd/helm/testdata/testcharts/bad.name/templates/service.yaml @@ -0,0 +1,16 @@ +apiVersion: v1 +kind: Service +metadata: + name: {{ include "bad.name.fullname" . }} + labels: +{{ include "bad.name.labels" . | indent 4 }} +spec: + type: {{ .Values.service.type }} + ports: + - port: {{ .Values.service.port }} + targetPort: http + protocol: TCP + name: http + selector: + app.kubernetes.io/name: {{ include "bad.name.name" . }} + app.kubernetes.io/instance: {{ .Release.Name }} diff --git a/cmd/helm/testdata/testcharts/bad.name/templates/serviceaccount.yaml b/cmd/helm/testdata/testcharts/bad.name/templates/serviceaccount.yaml new file mode 100644 index 00000000000..d637f77faae --- /dev/null +++ b/cmd/helm/testdata/testcharts/bad.name/templates/serviceaccount.yaml @@ -0,0 +1,8 @@ +{{- if .Values.serviceAccount.create -}} +apiVersion: v1 +kind: ServiceAccount +metadata: + name: {{ template "bad.name.serviceAccountName" . }} + labels: +{{ include "bad.name.labels" . | indent 4 }} +{{- end -}} diff --git a/cmd/helm/testdata/testcharts/bad.name/templates/tests/test-connection.yaml b/cmd/helm/testdata/testcharts/bad.name/templates/tests/test-connection.yaml new file mode 100644 index 00000000000..f99ac7b1e53 --- /dev/null +++ b/cmd/helm/testdata/testcharts/bad.name/templates/tests/test-connection.yaml @@ -0,0 +1,15 @@ +apiVersion: v1 +kind: Pod +metadata: + name: "{{ include "bad.name.fullname" . }}-test-connection" + labels: +{{ include "bad.name.labels" . | indent 4 }} + annotations: + "helm.sh/hook": test-success +spec: + containers: + - name: wget + image: busybox + command: ['wget'] + args: ['{{ include "bad.name.fullname" . }}:{{ .Values.service.port }}'] + restartPolicy: Never diff --git a/cmd/helm/testdata/testcharts/bad.name/values.yaml b/cmd/helm/testdata/testcharts/bad.name/values.yaml new file mode 100644 index 00000000000..36bc3981039 --- /dev/null +++ b/cmd/helm/testdata/testcharts/bad.name/values.yaml @@ -0,0 +1,68 @@ +# Default values for bad.name. +# This is a YAML-formatted file. +# Declare variables to be passed into your templates. + +replicaCount: 1 + +image: + repository: nginx + tag: stable + pullPolicy: IfNotPresent + +imagePullSecrets: [] +nameOverride: "" +fullnameOverride: "" + +serviceAccount: + # Specifies whether a service account should be created + create: true + # The name of the service account to use. + # If not set and create is true, a name is generated using the fullname template + name: "" + +podSecurityContext: {} + # fsGroup: 2000 + +securityContext: {} + # capabilities: + # drop: + # - ALL + # readOnlyRootFilesystem: true + # runAsNonRoot: true + # runAsUser: 1000 + +service: + type: ClusterIP + port: 80 + +ingress: + enabled: false + annotations: {} + # kubernetes.io/ingress.class: nginx + # kubernetes.io/tls-acme: "true" + hosts: + - host: chart-example.local + paths: [] + + tls: [] + # - secretName: chart-example-tls + # hosts: + # - chart-example.local + +resources: {} + # We usually recommend not to specify default resources and to leave this as a conscious + # choice for the user. This also increases chances charts run on environments with little + # resources, such as Minikube. If you do want to specify resources, uncomment the following + # lines, adjust them as necessary, and remove the curly braces after 'resources:'. + # limits: + # cpu: 100m + # memory: 128Mi + # requests: + # cpu: 100m + # memory: 128Mi + +nodeSelector: {} + +tolerations: [] + +affinity: {} diff --git a/pkg/lint/lint.go b/pkg/lint/lint.go index aa8df58143f..d1a3b1c70cf 100644 --- a/pkg/lint/lint.go +++ b/pkg/lint/lint.go @@ -24,13 +24,13 @@ import ( ) // All runs all of the available linters on the given base directory. -func All(basedir string, values []byte, namespace string, strict bool) support.Linter { +func All(basedir string, values []byte, namespace string) support.Linter { // Using abs path to get directory context chartDir, _ := filepath.Abs(basedir) linter := support.Linter{ChartDir: chartDir} rules.Chartfile(&linter) rules.Values(&linter) - rules.Templates(&linter, values, namespace, strict) + rules.Templates(&linter, values, namespace) return linter } diff --git a/pkg/lint/lint_test.go b/pkg/lint/lint_test.go index acd67adac2b..33edc976d8a 100644 --- a/pkg/lint/lint_test.go +++ b/pkg/lint/lint_test.go @@ -31,7 +31,6 @@ var values = []byte{} const ( namespace = "testNamespace" - strict = false badChartDir = "rules/testdata/badchartfile" badValuesFileDir = "rules/testdata/badvaluesfile" badYamlFileDir = "rules/testdata/albatross" @@ -39,7 +38,7 @@ const ( ) func TestBadChart(t *testing.T) { - m := All(badChartDir, values, namespace, strict).Messages + m := All(badChartDir, values, namespace).Messages if len(m) != 6 { t.Errorf("Number of errors %v", len(m)) t.Errorf("All didn't fail with expected errors, got %#v", m) @@ -79,7 +78,7 @@ func TestBadChart(t *testing.T) { } func TestInvalidYaml(t *testing.T) { - m := All(badYamlFileDir, values, namespace, strict).Messages + m := All(badYamlFileDir, values, namespace).Messages if len(m) != 1 { t.Fatalf("All didn't fail with expected errors, got %#v", m) } @@ -89,7 +88,7 @@ func TestInvalidYaml(t *testing.T) { } func TestBadValues(t *testing.T) { - m := All(badValuesFileDir, values, namespace, strict).Messages + m := All(badValuesFileDir, values, namespace).Messages if len(m) != 1 { t.Fatalf("All didn't fail with expected errors, got %#v", m) } @@ -99,7 +98,7 @@ func TestBadValues(t *testing.T) { } func TestGoodChart(t *testing.T) { - m := All(goodChartDir, values, namespace, strict).Messages + m := All(goodChartDir, values, namespace).Messages if len(m) != 0 { t.Errorf("All failed but shouldn't have: %#v", m) } @@ -130,7 +129,7 @@ func TestHelmCreateChart(t *testing.T) { return } - m := All(createdChart, values, namespace, true).Messages + m := All(createdChart, values, namespace).Messages if ll := len(m); ll != 1 { t.Errorf("All should have had exactly 1 error. Got %d", ll) } else if msg := m[0].Err.Error(); !strings.Contains(msg, "icon is recommended") { diff --git a/pkg/lint/rules/template.go b/pkg/lint/rules/template.go index 26c548baced..ab3f154ef71 100644 --- a/pkg/lint/rules/template.go +++ b/pkg/lint/rules/template.go @@ -32,7 +32,7 @@ import ( ) // Templates lints the templates in the Linter. -func Templates(linter *support.Linter, values []byte, namespace string, strict bool) { +func Templates(linter *support.Linter, values []byte, namespace string) { path := "templates/" templatesPath := filepath.Join(linter.ChartDir, path) @@ -77,9 +77,6 @@ func Templates(linter *support.Linter, values []byte, namespace string, strict b } e := engine.New() e.LintMode = true - if strict { - e.Strict = true - } renderedContentMap, err := e.Render(chart, valuesToRender) renderOk := linter.RunLinterRule(support.ErrorSev, path, err) diff --git a/pkg/lint/rules/template_test.go b/pkg/lint/rules/template_test.go index a294d3c573e..23a7fc4ae0f 100644 --- a/pkg/lint/rules/template_test.go +++ b/pkg/lint/rules/template_test.go @@ -17,12 +17,15 @@ limitations under the License. package rules import ( + "io/ioutil" "os" "path/filepath" "strings" "testing" + "k8s.io/helm/pkg/chartutil" "k8s.io/helm/pkg/lint/support" + "k8s.io/helm/pkg/proto/hapi/chart" ) const ( @@ -52,7 +55,7 @@ var values = []byte("nameOverride: ''\nhttpPort: 80") func TestTemplateParsing(t *testing.T) { linter := support.Linter{ChartDir: templateTestBasedir} - Templates(&linter, values, namespace, strict) + Templates(&linter, values, namespace) res := linter.Messages if len(res) != 1 { @@ -75,10 +78,65 @@ func TestTemplateIntegrationHappyPath(t *testing.T) { defer os.Rename(ignoredTemplatePath, wrongTemplatePath) linter := support.Linter{ChartDir: templateTestBasedir} - Templates(&linter, values, namespace, strict) + Templates(&linter, values, namespace) res := linter.Messages if len(res) != 0 { t.Fatalf("Expected no error, got %d, %v", len(res), res) } } + +// TestSTrictTemplatePrasingMapError is a regression test. +// +// The template engine should not produce an error when a map in values.yaml does +// not contain all possible keys. +// +// See https://github.com/helm/helm/issues/7483 (helm v3) +// and https://github.com/helm/helm/issues/6705 (helm v2) +func TestStrictTemplateParsingMapError(t *testing.T) { + dir, err := ioutil.TempDir("", "helm-test-") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + var vals = []byte(` +mymap: + key1: nestedValue +`) + var manifest = `apiVersion: v1 +kind: ConfigMap +metadata: + name: foo +data: + myval1: {{default "val" .Values.mymap.key1 }} + myval2: {{default "val" .Values.mymap.key2 }} +` + ch := chart.Chart{ + Metadata: &chart.Metadata{ + Name: "regression.6705", + ApiVersion: "v1", + Version: "0.1.0", + }, + Templates: []*chart.Template{ + { + Name: "templates/configmap.yaml", + Data: []byte(manifest), + }, + }, + } + + if err := chartutil.SaveDir(&ch, dir); err != nil { + t.Fatal(err) + } + linter := &support.Linter{ + ChartDir: filepath.Join(dir, ch.Metadata.Name), + } + Templates(linter, vals, namespace) + if len(linter.Messages) != 0 { + t.Errorf("expected zero messages, got %d", len(linter.Messages)) + for i, msg := range linter.Messages { + t.Logf("Message %d: %q", i, msg) + } + } +}