Merge e58ce34aa1 into ad93b7df63

internal/chart/v3/lint/lint_test.go

@@ -38,6 +38,7 @@ const goodChartDir = "rules/testdata/goodone"
 const subChartValuesDir = "rules/testdata/withsubchart"
 const malformedTemplate = "rules/testdata/malformed-template"
 const invalidChartFileDir = "rules/testdata/invalidchartfile"
+const duplicateKeysDir = "rules/testdata/duplicatekeys"
 
 func TestBadChartV3(t *testing.T) {
 	var values map[string]any
@@ -219,3 +220,26 @@ func TestMalformedTemplate(t *testing.T) {
 		assert.ErrorContains(t, m[0].Err, "invalid character '{'", "All didn't have the error for invalid character '{'")
 	}
 }
+
+// TestDuplicateKeysV3 tests that values.yaml with duplicate keys is detected
+// See https://github.com/helm/helm/issues/31102
+func TestDuplicateKeysV3(t *testing.T) {
+	var values map[string]any
+	m := RunAll(duplicateKeysDir, values, namespace).Messages
+	// Expect at least 1 message (the duplicate keys error)
+	if len(m) < 1 {
+		t.Fatalf("All didn't fail with expected errors, got %#v", m)
+	}
+	// Find the duplicate keys error message
+	found := false
+	for _, msg := range m {
+		if msg.Path == "values.yaml" && msg.Severity == support.ErrorSev &&
+			strings.Contains(msg.Err.Error(), "contains duplicate keys") {
+			found = true
+			break
+		}
+	}
+	if !found {
+		t.Errorf("All didn't have the error for duplicate YAML keys in values.yaml: %v", m)
+	}
+}

internal/chart/v3/lint/rules/testdata/duplicatekeys/Chart.yaml

@@ -0,0 +1,4 @@
+apiVersion: v3
+name: duplicatekeys
+description: testing chart with duplicate keys in values.yaml
+version: 0.1.0

internal/chart/v3/lint/rules/testdata/duplicatekeys/values.yaml

@@ -0,0 +1,4 @@
+invalid:
+  duplicate: default
+  duplicate: value-i-want
+  duplicate: last-one

internal/chart/v3/lint/rules/values.go

@@ -21,6 +21,9 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
+
+	yamlv2 "go.yaml.in/yaml/v2"
 
 	"helm.sh/helm/v4/internal/chart/v3/lint/support"
 	"helm.sh/helm/v4/pkg/chart/common"
@@ -54,9 +57,13 @@ func validateValuesFileExistence(valuesPath string) error {
 }
 
 func validateValuesFile(valuesPath string, overrides map[string]any, skipSchemaValidation bool) error {
-	values, err := common.ReadValuesFile(valuesPath)
+	// Try strict parsing first to detect duplicate keys
-	if err != nil {
+	values, strictErr := common.ReadValuesFileStrict(valuesPath)
-		return fmt.Errorf("unable to parse YAML: %w", err)
+	if strictErr != nil {
+		if isDuplicateKeyError(strictErr) {
+			return fmt.Errorf("%s contains duplicate keys: %w", filepath.Base(valuesPath), strictErr)
+		}
+		return fmt.Errorf("unable to parse YAML: %w", strictErr)
 	}
 
 	// Helm 3.0.0 carried over the values linting from Helm 2.x, which only tests the top
@@ -83,3 +90,17 @@ func validateValuesFile(valuesPath string, overrides map[string]any, skipSchemaV
 
 	return nil
 }
+
+// isDuplicateKeyError checks if an error is related to duplicate YAML keys.
+func isDuplicateKeyError(err error) bool {
+	var typeErr *yamlv2.TypeError
+	if !errors.As(err, &typeErr) {
+		return false
+	}
+	for _, e := range typeErr.Errors {
+		if strings.Contains(e, "already set in map") {
+			return true
+		}
+	}
+	return false
+}

internal/chart/v3/lint/rules/values_test.go

@@ -17,11 +17,14 @@ limitations under the License.
 package rules
 
 import (
+	"errors"
+	"fmt"
 	"os"
 	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	yamlv2 "go.yaml.in/yaml/v2"
 
 	"helm.sh/helm/v4/internal/test/ensure"
 )
@@ -177,3 +180,58 @@ func createTestingSchema(t *testing.T, dir string) string {
 	}
 	return schemafile
 }
+
+func TestIsDuplicateKeyError(t *testing.T) {
+	tests := []struct {
+		name     string
+		err      error
+		expected bool
+	}{
+		{
+			name:     "duplicate key error",
+			err:      &yamlv2.TypeError{Errors: []string{`line 2: key "key" already set in map`}},
+			expected: true,
+		},
+		{
+			name:     "wrapped duplicate key error",
+			err:      fmt.Errorf("error converting YAML to JSON: %w", &yamlv2.TypeError{Errors: []string{`line 2: key "key" already set in map`}}),
+			expected: true,
+		},
+		{
+			name:     "non-duplicate key error",
+			err:      errors.New("some other error"),
+			expected: false,
+		},
+		{
+			name:     "type error but not duplicate-key related",
+			err:      &yamlv2.TypeError{Errors: []string{"cannot unmarshal !!seq into map[string]interface {}"}},
+			expected: false,
+		},
+		{
+			name:     "nil error",
+			err:      nil,
+			expected: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := isDuplicateKeyError(tt.err)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}
+
+func TestValidateValuesFileDuplicateKeys(t *testing.T) {
+	duplicateYaml := `key: value1
+key: value2
+`
+	tmpdir := ensure.TempFile(t, "values.yaml", []byte(duplicateYaml))
+	valfile := filepath.Join(tmpdir, "values.yaml")
+
+	err := validateValuesFile(valfile, map[string]any{}, false)
+	if err == nil {
+		t.Fatal("expected values file with duplicate keys to fail parsing")
+	}
+	assert.Contains(t, err.Error(), "contains duplicate keys")
+}

pkg/chart/common/values.go

@@ -118,6 +118,26 @@ func ReadValuesFile(filename string) (Values, error) {
 	return ReadValues(data)
 }
 
+// ReadValuesFileStrict will parse a YAML file into a map of values using strict unmarshaling.
+// This will detect duplicate keys in the YAML.
+func ReadValuesFileStrict(filename string) (Values, error) {
+	data, err := os.ReadFile(filename)
+	if err != nil {
+		return map[string]any{}, err
+	}
+	return ReadValuesStrict(data)
+}
+
+// ReadValuesStrict will parse YAML byte data into a Values using strict unmarshaling.
+// This will detect duplicate keys in the YAML.
+func ReadValuesStrict(data []byte) (vals Values, err error) {
+	err = yaml.UnmarshalStrict(data, &vals)
+	if len(vals) == 0 {
+		vals = Values{}
+	}
+	return vals, err
+}
+
 // ReleaseOptions represents the additional release options needed
 // for the composition of the final values struct
 type ReleaseOptions struct {

pkg/chart/common/values_test.go

@@ -19,6 +19,7 @@ package common
 import (
 	"bytes"
 	"fmt"
+	"strings"
 	"testing"
 	"text/template"
 )
@@ -203,3 +204,34 @@ chapter:
 		}
 	}
 }
+
+func TestReadValuesStrict(t *testing.T) {
+	doc := `# Test YAML parse
+poet: "Coleridge"
+title: "Rime of the Ancient Mariner"
+`
+
+	data, err := ReadValuesStrict([]byte(doc))
+	if err != nil {
+		t.Fatalf("Error parsing bytes: %s", err)
+	}
+	if data["poet"] != "Coleridge" {
+		t.Errorf("Unexpected poet: %v", data["poet"])
+	}
+}
+
+func TestReadValuesStrictDuplicateKeys(t *testing.T) {
+	doc := `invalid:
+  duplicate: default
+  duplicate: value-i-want
+  duplicate: last-one
+`
+
+	_, err := ReadValuesStrict([]byte(doc))
+	if err == nil {
+		t.Fatal("Expected error for duplicate keys, got nil")
+	}
+	if !strings.Contains(err.Error(), "already") && !strings.Contains(err.Error(), "duplicate") {
+		t.Fatalf("Expected duplicate-key error, got: %s", err)
+	}
+}

pkg/chart/v2/lint/lint_test.go

@@ -38,6 +38,7 @@ const goodChartDir = "rules/testdata/goodone"
 const subChartValuesDir = "rules/testdata/withsubchart"
 const malformedTemplate = "rules/testdata/malformed-template"
 const invalidChartFileDir = "rules/testdata/invalidchartfile"
+const duplicateKeysDir = "rules/testdata/duplicatekeys"
 
 func TestBadChart(t *testing.T) {
 	var values map[string]any
@@ -246,3 +247,26 @@ func TestMalformedTemplate(t *testing.T) {
 		}
 	}
 }
+
+// TestDuplicateKeys tests that values.yaml with duplicate keys is detected
+// See https://github.com/helm/helm/issues/31102
+func TestDuplicateKeys(t *testing.T) {
+	var values map[string]any
+	m := RunAll(duplicateKeysDir, values, namespace).Messages
+	// Expect at least 1 message (the duplicate keys error)
+	if len(m) < 1 {
+		t.Fatalf("All didn't fail with expected errors, got %#v", m)
+	}
+	// Find the duplicate keys error message
+	found := false
+	for _, msg := range m {
+		if msg.Path == "values.yaml" && msg.Severity == support.ErrorSev &&
+			strings.Contains(msg.Err.Error(), "contains duplicate keys") {
+			found = true
+			break
+		}
+	}
+	if !found {
+		t.Errorf("All didn't have the error for duplicate YAML keys in values.yaml: %v", m)
+	}
+}

pkg/chart/v2/lint/rules/testdata/duplicatekeys/Chart.yaml

@@ -0,0 +1,4 @@
+apiVersion: v2
+name: duplicatekeys
+description: testing chart with duplicate keys in values.yaml
+version: 0.1.0

pkg/chart/v2/lint/rules/testdata/duplicatekeys/values.yaml

@@ -0,0 +1,4 @@
+invalid:
+  duplicate: default
+  duplicate: value-i-want
+  duplicate: last-one

pkg/chart/v2/lint/rules/values.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 
 	"helm.sh/helm/v4/pkg/chart/common"
 	"helm.sh/helm/v4/pkg/chart/common/util"
@@ -54,9 +55,13 @@ func validateValuesFileExistence(valuesPath string) error {
 }
 
 func validateValuesFile(valuesPath string, overrides map[string]any, skipSchemaValidation bool) error {
-	values, err := common.ReadValuesFile(valuesPath)
+	// Try strict parsing first to detect duplicate keys
-	if err != nil {
+	values, strictErr := common.ReadValuesFileStrict(valuesPath)
-		return fmt.Errorf("unable to parse YAML: %w", err)
+	if strictErr != nil {
+		if isDuplicateKeyError(strictErr) {
+			return fmt.Errorf("%s contains duplicate keys: %w", filepath.Base(valuesPath), strictErr)
+		}
+		return fmt.Errorf("unable to parse YAML: %w", strictErr)
 	}
 
 	// Helm 3.0.0 carried over the values linting from Helm 2.x, which only tests the top
@@ -83,3 +88,11 @@ func validateValuesFile(valuesPath string, overrides map[string]any, skipSchemaV
 
 	return nil
 }
+
+// isDuplicateKeyError checks if an error is related to duplicate YAML keys
+func isDuplicateKeyError(err error) bool {
+	errStr := err.Error()
+	return strings.Contains(errStr, "already set in map") ||
+		strings.Contains(errStr, "already defined") ||
+		strings.Contains(errStr, "duplicate")
+}