[chart/loader] use strict yaml unmarshaling for chart files

This updates the helm chart loader to use strict yaml unmarshaling (UnmarshalStrict) instead of the non-strict mode (Unmarshal). The main benefit of this is to prevent duplicate keys within the Values.yaml file which seems like that would never be intentionally done by the user. Right now if there are duplicate keys within Values.yaml, the last one will be used and the other ones will be silently ignored.

Signed-off-by: Aadesh Patel <aadeshdpatel@gmail.com>

pkg/chart/loader/load.go

@@ -81,7 +81,7 @@ func LoadFiles(files []*BufferedFile) (*chart.Chart, error) {
 			if c.Metadata == nil {
 				c.Metadata = new(chart.Metadata)
 			}
-			if err := yaml.Unmarshal(f.Data, c.Metadata); err != nil {
+			if err := yaml.UnmarshalStrict(f.Data, c.Metadata); err != nil {
 				return c, errors.Wrap(err, "cannot load Chart.yaml")
 			}
 			// NOTE(bacongobbler): while the chart specification says that APIVersion must be set,
@@ -99,12 +99,12 @@ func LoadFiles(files []*BufferedFile) (*chart.Chart, error) {
 			continue
 		case f.Name == "Chart.lock":
 			c.Lock = new(chart.Lock)
-			if err := yaml.Unmarshal(f.Data, &c.Lock); err != nil {
+			if err := yaml.UnmarshalStrict(f.Data, &c.Lock); err != nil {
 				return c, errors.Wrap(err, "cannot load Chart.lock")
 			}
 		case f.Name == "values.yaml":
 			c.Values = make(map[string]interface{})
-			if err := yaml.Unmarshal(f.Data, &c.Values); err != nil {
+			if err := yaml.UnmarshalStrict(f.Data, &c.Values); err != nil {
 				return c, errors.Wrap(err, "cannot load values.yaml")
 			}
 		case f.Name == "values.schema.json":
@@ -119,7 +119,7 @@ func LoadFiles(files []*BufferedFile) (*chart.Chart, error) {
 			if c.Metadata.APIVersion != chart.APIVersionV1 {
 				log.Printf("Warning: Dependencies are handled in Chart.yaml since apiVersion \"v2\". We recommend migrating dependencies to Chart.yaml.")
 			}
-			if err := yaml.Unmarshal(f.Data, c.Metadata); err != nil {
+			if err := yaml.UnmarshalStrict(f.Data, c.Metadata); err != nil {
 				return c, errors.Wrap(err, "cannot load requirements.yaml")
 			}
 			if c.Metadata.APIVersion == chart.APIVersionV1 {
@@ -128,7 +128,7 @@ func LoadFiles(files []*BufferedFile) (*chart.Chart, error) {
 		// Deprecated: requirements.lock is deprecated use Chart.lock.
 		case f.Name == "requirements.lock":
 			c.Lock = new(chart.Lock)
-			if err := yaml.Unmarshal(f.Data, &c.Lock); err != nil {
+			if err := yaml.UnmarshalStrict(f.Data, &c.Lock); err != nil {
 				return c, errors.Wrap(err, "cannot load requirements.lock")
 			}
 			if c.Metadata == nil {

pkg/chart/loader/load_test.go

@@ -489,6 +489,17 @@ func TestLoadInvalidArchive(t *testing.T) {
 	}
 }
 
+func TestLoadWithDuplicateKeysInValuesFile(t *testing.T) {
+	l, err := Loader("testdata/frobnitz_with_duplicate_keys")
+	if err != nil {
+		t.Fatalf("Failed to load testdata: %s", err)
+	}
+
+	if _, err = l.Load(); err == nil {
+		t.Errorf("packages with a value yaml file with duplicate keys should not load")
+	}
+}
+
 func verifyChart(t *testing.T, c *chart.Chart) {
 	t.Helper()
 	if c.Name() == "" {

pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/.helmignore

@@ -0,0 +1 @@
+ignore/

pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/Chart.lock

@@ -0,0 +1,8 @@
+dependencies:
+  - name: alpine
+    version: "0.1.0"
+    repository: https://example.com/charts
+  - name: mariner
+    version: "4.3.2"
+    repository: https://example.com/charts
+digest: invalid

pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/Chart.yaml

@@ -0,0 +1,27 @@
+apiVersion: v1
+name: frobnitz
+description: This is a frobnitz.
+version: "1.2.3"
+keywords:
+  - frobnitz
+  - sprocket
+  - dodad
+maintainers:
+  - name: The Helm Team
+    email: helm@example.com
+  - name: Someone Else
+    email: nobody@example.com
+sources:
+  - https://example.com/foo/bar
+home: http://example.com
+icon: https://example.com/64x64.png
+annotations:
+  extrakey: extravalue
+  anotherkey: anothervalue
+dependencies:
+  - name: alpine
+    version: "0.1.0"
+    repository: https://example.com/charts
+  - name: mariner
+    version: "4.3.2"
+    repository: https://example.com/charts

pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/INSTALL.txt

@@ -0,0 +1 @@
+This is an install document. The client may display this.

pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/LICENSE

@@ -0,0 +1 @@
+LICENSE placeholder.

pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/README.md

@@ -0,0 +1,11 @@
+# Frobnitz
+
+This is an example chart.
+
+## Usage
+
+This is an example. It has no usage.
+
+## Development
+
+For developer info, see the top-level repository.

pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/_ignore_me

@@ -0,0 +1 @@
+This should be ignored by the loader, but may be included in a chart.

pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/Chart.yaml

@@ -0,0 +1,5 @@
+apiVersion: v1
+name: alpine
+description: Deploy a basic Alpine Linux pod
+version: 0.1.0
+home: https://helm.sh/helm

pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/README.md

@@ -0,0 +1,9 @@
+This example was generated using the command `helm create alpine`.
+
+The `templates/` directory contains a very simple pod resource with a
+couple of parameters.
+
+The `values.toml` file contains the default values for the
+`alpine-pod.yaml` template.
+
+You can install this example using `helm install ./alpine`.

pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/charts/mast1/Chart.yaml

@@ -0,0 +1,5 @@
+apiVersion: v1
+name: mast1
+description: A Helm chart for Kubernetes
+version: 0.1.0
+home: ""

pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/charts/mast1/values.yaml

@@ -0,0 +1,4 @@
+# Default values for mast1.
+# This is a YAML-formatted file.
+# Declare name/value pairs to be passed into your templates.
+# name = "value"

pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/templates/alpine-pod.yaml

@@ -0,0 +1,14 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: {{.Release.Name}}-{{.Chart.Name}}
+  labels:
+    app.kubernetes.io/managed-by: {{.Release.Service}}
+    app.kubernetes.io/name: {{.Chart.Name}}
+    helm.sh/chart: "{{.Chart.Name}}-{{.Chart.Version}}"
+spec:
+  restartPolicy: {{default "Never" .restart_policy}}
+  containers:
+  - name: waiter
+    image: "alpine:3.9"
+    command: ["/bin/sleep","9000"]

pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/values.yaml

@@ -0,0 +1,2 @@
+# The pod name
+name: "my-alpine"

pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/docs/README.md

@@ -0,0 +1 @@
+This is a placeholder for documentation.

pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/icon.svg

@@ -0,0 +1,8 @@
+<?xml version="1.0"?>
+<svg xmlns:svg="http://www.w3.org/2000/svg" xmlns="http://www.w3.org/2000/svg"
+    xmlns:xlink="http://www.w3.org/1999/xlink"
+    version="1.0" width="256" height="256" id="test">
+  <desc>Example icon</desc>
+  <rect id="first" x="2" y="2" width="40" height="60" fill="navy"/>
+  <rect id="second" x="15" y="4" width="40" height="60" fill="red"/>
+</svg>

pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/templates/template.tpl

@@ -0,0 +1 @@
+Hello {{.Name | default "world"}}

pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/values.yaml

@@ -0,0 +1,9 @@
+# A values file contains configuration.
+
+name: "Some Name"
+
+section:
+  name: "Name in a section"
+
+section:
+  name: "Duplicate section name"