diff --git a/internal/chart/v3/lint/lint_test.go b/internal/chart/v3/lint/lint_test.go index afacb8052..9dad14851 100644 --- a/internal/chart/v3/lint/lint_test.go +++ b/internal/chart/v3/lint/lint_test.go @@ -37,6 +37,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 @@ -241,3 +242,26 @@ func TestMalformedTemplate(t *testing.T) { } } } + +// 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) + } +} diff --git a/internal/chart/v3/lint/rules/testdata/duplicatekeys/Chart.yaml b/internal/chart/v3/lint/rules/testdata/duplicatekeys/Chart.yaml new file mode 100644 index 000000000..a57e72256 --- /dev/null +++ b/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 diff --git a/internal/chart/v3/lint/rules/testdata/duplicatekeys/values.yaml b/internal/chart/v3/lint/rules/testdata/duplicatekeys/values.yaml new file mode 100644 index 000000000..1c23dd36e --- /dev/null +++ b/internal/chart/v3/lint/rules/testdata/duplicatekeys/values.yaml @@ -0,0 +1,4 @@ +invalid: + duplicate: default + duplicate: value-i-want + duplicate: last-one diff --git a/internal/chart/v3/lint/rules/values.go b/internal/chart/v3/lint/rules/values.go index b4a2edb0c..a91e70363 100644 --- a/internal/chart/v3/lint/rules/values.go +++ b/internal/chart/v3/lint/rules/values.go @@ -21,6 +21,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "helm.sh/helm/v4/internal/chart/v3/lint/support" "helm.sh/helm/v4/pkg/chart/common" @@ -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) - if err != nil { - return fmt.Errorf("unable to parse YAML: %w", err) + // Try strict parsing first to detect duplicate keys + values, strictErr := common.ReadValuesFileStrict(valuesPath) + 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") +} diff --git a/pkg/chart/common/values.go b/pkg/chart/common/values.go index 17a067790..ffe77eb52 100644 --- a/pkg/chart/common/values.go +++ b/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 { diff --git a/pkg/chart/common/values_test.go b/pkg/chart/common/values_test.go index 9743869ec..609db24b8 100644 --- a/pkg/chart/common/values_test.go +++ b/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) + } +} diff --git a/pkg/chart/v2/lint/lint_test.go b/pkg/chart/v2/lint/lint_test.go index 4256281e0..df7d2f6c4 100644 --- a/pkg/chart/v2/lint/lint_test.go +++ b/pkg/chart/v2/lint/lint_test.go @@ -37,6 +37,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 @@ -245,3 +246,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) + } +} diff --git a/pkg/chart/v2/lint/rules/testdata/duplicatekeys/Chart.yaml b/pkg/chart/v2/lint/rules/testdata/duplicatekeys/Chart.yaml new file mode 100644 index 000000000..b9a42ac9b --- /dev/null +++ b/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 diff --git a/pkg/chart/v2/lint/rules/testdata/duplicatekeys/values.yaml b/pkg/chart/v2/lint/rules/testdata/duplicatekeys/values.yaml new file mode 100644 index 000000000..1c23dd36e --- /dev/null +++ b/pkg/chart/v2/lint/rules/testdata/duplicatekeys/values.yaml @@ -0,0 +1,4 @@ +invalid: + duplicate: default + duplicate: value-i-want + duplicate: last-one diff --git a/pkg/chart/v2/lint/rules/values.go b/pkg/chart/v2/lint/rules/values.go index 2c766068c..6cbd170ec 100644 --- a/pkg/chart/v2/lint/rules/values.go +++ b/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) - if err != nil { - return fmt.Errorf("unable to parse YAML: %w", err) + // Try strict parsing first to detect duplicate keys + values, strictErr := common.ReadValuesFileStrict(valuesPath) + 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") +}