From 65c3fc75815728bbfc9e795e8126a912034a3467 Mon Sep 17 00:00:00 2001 From: Kartik Suryavanshi <158498247+KartikSuryavanshi@users.noreply.github.com> Date: Wed, 15 Jul 2026 09:08:52 +0530 Subject: [PATCH 1/2] fix: add duplicate key detection for values.yaml Add lint rule to detect duplicate keys in values.yaml. This addresses the issue where Helm allows installing charts with invalid YAML that contains duplicate keys, which silently takes the last value. Changes: - Add ReadValuesFileStrict and ReadValuesStrict functions in pkg/chart/common/values.go that use yaml.UnmarshalStrict - Add validateValuesFileDuplicateKeys function in both v2 and v3 lint rules to check for duplicate keys - Add test data with duplicate keys for both v2 and v3 - Add tests to verify duplicate key detection works Closes #31102 Signed-off-by: Kartik Suryavanshi <158498247+KartikSuryavanshi@users.noreply.github.com> --- internal/chart/v3/lint/lint_test.go | 24 ++++++++++++++ .../rules/testdata/duplicatekeys/Chart.yaml | 4 +++ .../rules/testdata/duplicatekeys/values.yaml | 4 +++ internal/chart/v3/lint/rules/values.go | 19 +++++++++-- pkg/chart/common/values.go | 20 ++++++++++++ pkg/chart/common/values_test.go | 32 +++++++++++++++++++ pkg/chart/v2/lint/lint_test.go | 24 ++++++++++++++ .../rules/testdata/duplicatekeys/Chart.yaml | 4 +++ .../rules/testdata/duplicatekeys/values.yaml | 4 +++ pkg/chart/v2/lint/rules/values.go | 19 +++++++++-- 10 files changed, 148 insertions(+), 6 deletions(-) create mode 100644 internal/chart/v3/lint/rules/testdata/duplicatekeys/Chart.yaml create mode 100644 internal/chart/v3/lint/rules/testdata/duplicatekeys/values.yaml create mode 100644 pkg/chart/v2/lint/rules/testdata/duplicatekeys/Chart.yaml create mode 100644 pkg/chart/v2/lint/rules/testdata/duplicatekeys/values.yaml 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") +} From e58ce34aa1a4b763a2075025c75f3906b7a6050a Mon Sep 17 00:00:00 2001 From: Kartik Suryavanshi <158498247+KartikSuryavanshi@users.noreply.github.com> Date: Wed, 15 Jul 2026 16:54:11 +0530 Subject: [PATCH 2/2] fix: use errors.As with content check for duplicate key detection Replace fragile string matching in isDuplicateKeyError with a type-based check using errors.As and *yamlv2.TypeError, combined with content checking for "already set in map". The error from sigs.k8s.io/yaml wraps the underlying yaml.TypeError, and errors.As properly unwraps it. This eliminates the dead code branches for "already defined" and "duplicate" strings that were never produced by the yaml parser, while still being precise enough to not misclassify other yaml.TypeError variants as duplicate key errors. Signed-off-by: Kartik Suryavanshi Signed-off-by: Kartik Suryavanshi <158498247+KartikSuryavanshi@users.noreply.github.com> --- internal/chart/v3/lint/rules/values.go | 18 +++++-- internal/chart/v3/lint/rules/values_test.go | 58 +++++++++++++++++++++ 2 files changed, 71 insertions(+), 5 deletions(-) diff --git a/internal/chart/v3/lint/rules/values.go b/internal/chart/v3/lint/rules/values.go index a91e70363..9ebf405c4 100644 --- a/internal/chart/v3/lint/rules/values.go +++ b/internal/chart/v3/lint/rules/values.go @@ -23,6 +23,8 @@ import ( "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" "helm.sh/helm/v4/pkg/chart/common/util" @@ -89,10 +91,16 @@ func validateValuesFile(valuesPath string, overrides map[string]any, skipSchemaV return nil } -// isDuplicateKeyError checks if an error is related to duplicate YAML keys +// 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") + 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 } diff --git a/internal/chart/v3/lint/rules/values_test.go b/internal/chart/v3/lint/rules/values_test.go index 54c7e6457..0324930ed 100644 --- a/internal/chart/v3/lint/rules/values_test.go +++ b/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" ) @@ -181,3 +184,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") +}