Repository: helm/helm (go)

Issue:
"helm lint" does not complain on duplicate keys in chartfile YAML
Currently, `helm lint` accepts chartfiles which have duplicate mapping keys (this constitutes invalid YAML), e.g. it accepts the following:

```yaml
apiVersion: v2
name: test
description: A Helm chart for Kubernetes
type: application
version: 0.1.0
appVersion: "1.16.0"
appVersion: "1.2.3"      <------ DUPLICATE KEY
dependencies:
  - name: trivy-operator
    repository: https://aquasecurity.github.io/helm-charts/
    version: 0.18.0
    version: 0.17.1      <------ DUPLICATE KEY
 ```
 
I think to fix this we can simply:
 - change [this function call](https://github.com/helm/helm/blob/main/pkg/chartutil/chartfile.go#L36) in pkg/chartutil to the [strict version](https://github.com/kubernetes-sigs/yaml/blob/master/yaml.go#L63) (`yaml.UnmarshalStrict`)
  - ~~check the resulting Metadata struct has a non-empty Name, a required field [according to this comment](https://github.com/helm/helm/blob/main/pkg/chart/metadata.go#L48)~~ this shouldn't be necessary, we can just use the error return value of the unmarshal function as we currently do

Happy to submit a PR with this change!

Minimal steps to reproduce:
  - `helm create testy-mctest`
  - update `Chart.yaml` with example above (or any key duplicated)
   - `helm lint`
   
Output of `helm version`:

```
version.BuildInfo{Version:"v3.10.2", GitCommit:"50f003e5ee8704ec937a756c646870227d7c8b58", GitTreeState:"clean", GoVersion:"go1.18.8"}
```

Pull request description:
fix(pkg/lint): unmarshals Chart.yaml strictly
When "helm lint" is run, it now errors on invalid Chart.yaml files, e.g. those with duplicate keys

Closes #12381

**What this PR does / why we need it**: Fixes a bug with the current `helm lint` command, where it does not error on Chart.yaml with duplicate mapping keys

**Special notes for your reviewer**:

**If applicable**:
- [ ] this PR contains documentation
- [x] this PR contains unit tests
- [ ] this PR has been tested for backwards compatibility


Patch:
diff --git a/pkg/chart/v2/util/chartfile.go b/pkg/chart/v2/util/chartfile.go
index 87323c201..b48687d55 100644
--- a/pkg/chart/v2/util/chartfile.go
+++ b/pkg/chart/v2/util/chartfile.go
@@ -37,6 +37,17 @@ func LoadChartfile(filename string) (*chart.Metadata, error) {
 	return y, err
 }
 
+// StrictLoadChartFile loads a Chart.yaml into a *chart.Metadata using a strict unmarshaling
+func StrictLoadChartfile(filename string) (*chart.Metadata, error) {
+	b, err := os.ReadFile(filename)
+	if err != nil {
+		return nil, err
+	}
+	y := new(chart.Metadata)
+	err = yaml.UnmarshalStrict(b, y)
+	return y, err
+}
+
 // SaveChartfile saves the given metadata as a Chart.yaml file at the given path.
 //
 // 'filename' should be the complete path and filename ('foo/Chart.yaml')
diff --git a/pkg/lint/rules/chartfile.go b/pkg/lint/rules/chartfile.go
index 598557a97..13ae77222 100644
--- a/pkg/lint/rules/chartfile.go
+++ b/pkg/lint/rules/chartfile.go
@@ -46,6 +46,9 @@ func Chartfile(linter *support.Linter) {
 		return
 	}
 
+	_, err = chartutil.StrictLoadChartfile(chartPath)
+	linter.RunLinterRule(support.WarningSev, chartFileName, validateChartYamlStrictFormat(err))
+
 	// type check for Chart.yaml . ignoring error as any parse
 	// errors would already be caught in the above load function
 	chartFileForTypeCheck, _ := loadChartFileForTypeCheck(chartPath)
@@ -102,6 +105,13 @@ func validateChartYamlFormat(chartFileError error) error {
 	return nil
 }
 
+func validateChartYamlStrictFormat(chartFileError error) error {
+	if chartFileError != nil {
+		return errors.Errorf("failed to strictly parse chart metadata file\n\t%s", chartFileError.Error())
+	}
+	return nil
+}
+
 func validateChartName(cf *chart.Metadata) error {
 	if cf.Name == "" {
 		return errors.New("name is required")


Test patch:
diff --git a/pkg/lint/lint_test.go b/pkg/lint/lint_test.go
index 067d140f6..888d3dfe6 100644
--- a/pkg/lint/lint_test.go
+++ b/pkg/lint/lint_test.go
@@ -35,6 +35,7 @@ const badYamlFileDir = "rules/testdata/albatross"
 const goodChartDir = "rules/testdata/goodone"
 const subChartValuesDir = "rules/testdata/withsubchart"
 const malformedTemplate = "rules/testdata/malformed-template"
+const invalidChartFileDir = "rules/testdata/invalidchartfile"
 
 func TestBadChart(t *testing.T) {
 	m := RunAll(badChartDir, values, namespace).Messages
@@ -90,6 +91,16 @@ func TestInvalidYaml(t *testing.T) {
 	}
 }
 
+func TestInvalidChartYaml(t *testing.T) {
+	m := RunAll(invalidChartFileDir, values, namespace).Messages
+	if len(m) != 1 {
+		t.Fatalf("All didn't fail with expected errors, got %#v", m)
+	}
+	if !strings.Contains(m[0].Err.Error(), "failed to strictly parse chart metadata file") {
+		t.Errorf("All didn't have the error for duplicate YAML keys")
+	}
+}
+
 func TestBadValues(t *testing.T) {
 	m := RunAll(badValuesFileDir, values, namespace).Messages
 	if len(m) < 1 {
diff --git a/pkg/lint/rules/testdata/invalidchartfile/Chart.yaml b/pkg/lint/rules/testdata/invalidchartfile/Chart.yaml
new file mode 100644
index 000000000..0fd58d1d4
--- /dev/null
+++ b/pkg/lint/rules/testdata/invalidchartfile/Chart.yaml
@@ -0,0 +1,6 @@
+name: some-chart
+apiVersion: v2
+apiVersion: v1
+description: A Helm chart for Kubernetes
+version: 1.3.0
+icon: http://example.com