From 3a8f63c9aa0ae81f033ae4e3d7be614a4e8d8b0f Mon Sep 17 00:00:00 2001 From: Denys Vitali Date: Fri, 28 Nov 2025 15:33:51 +0000 Subject: [PATCH] Fix helm valuesInline merge failure when types differ When using valuesInline with helm charts, kustomize would fail with "wrong node kind" errors when the chart's values.yaml had a different type than what the user provided. For example: - Chart has `topologySpreadConstraints: {}` but user provides a list - Chart has `githubConfigSecret: {github_token: ""}` but user provides a string This adds an AllowKindChange option to MergeOptions that allows the source value to completely replace the destination when their YAML node kinds differ (e.g., map vs list, map vs scalar). The HelmChartInflationGenerator now enables this option when merging values, fixing the regression introduced in kustomize 5.4.0. Fixes https://github.com/kubernetes-sigs/kustomize/issues/5766 Fixes https://github.com/actions/actions-runner-controller/issues/3819 --- kyaml/yaml/merge2/merge2_test.go | 175 ++++++++++++++++++ kyaml/yaml/types.go | 8 + kyaml/yaml/walk/walk.go | 47 +++++ .../HelmChartInflationGenerator.go | 9 +- 4 files changed, 237 insertions(+), 2 deletions(-) diff --git a/kyaml/yaml/merge2/merge2_test.go b/kyaml/yaml/merge2/merge2_test.go index e014f02f96..a4b02489c0 100644 --- a/kyaml/yaml/merge2/merge2_test.go +++ b/kyaml/yaml/merge2/merge2_test.go @@ -51,3 +51,178 @@ type testCase struct { infer bool mergeOptions yaml.MergeOptions } + +// TestMergeWithKindChange tests the AllowKindChange option which allows +// merging nodes of different kinds (e.g., map to list, map to scalar). +// This is needed for Helm values merging where charts may have placeholder +// types that don't match what users want to provide. +// Reference: https://github.com/kubernetes-sigs/kustomize/issues/5766 +// Reference: https://github.com/actions/actions-runner-controller/issues/3819 +func TestMergeWithKindChange(t *testing.T) { + testCases := []struct { + name string + source string + dest string + expected string + allowKindChange bool + expectError bool + }{ + { + // Issue #5766: Chart has empty map {}, user wants to provide a list + name: "map to list with AllowKindChange=true (issue #5766)", + source: ` +topologySpreadConstraints: +- maxSkew: 1 + topologyKey: topology.kubernetes.io/zone +`, + dest: ` +topologySpreadConstraints: {} +`, + expected: ` +topologySpreadConstraints: +- maxSkew: 1 + topologyKey: topology.kubernetes.io/zone +`, + allowKindChange: true, + expectError: false, + }, + { + name: "map to list without AllowKindChange should fail", + source: ` +topologySpreadConstraints: +- maxSkew: 1 +`, + dest: ` +topologySpreadConstraints: {} +`, + allowKindChange: false, + expectError: true, + }, + { + // Issue #3819: Chart has map structure, user wants to provide a scalar string + name: "map to scalar with AllowKindChange=true (issue #3819)", + source: ` +githubConfigSecret: my-custom-secret +`, + dest: ` +githubConfigSecret: + github_token: "" +`, + expected: ` +githubConfigSecret: my-custom-secret +`, + allowKindChange: true, + expectError: false, + }, + { + name: "map to scalar without AllowKindChange should fail", + source: ` +githubConfigSecret: my-custom-secret +`, + dest: ` +githubConfigSecret: + github_token: "" +`, + allowKindChange: false, + expectError: true, + }, + { + // Reverse case: list to map + name: "list to map with AllowKindChange=true", + source: ` +tls: + termination: edge + insecureEdgeTerminationPolicy: Redirect +`, + dest: ` +tls: [] +`, + expected: ` +tls: + termination: edge + insecureEdgeTerminationPolicy: Redirect +`, + allowKindChange: true, + expectError: false, + }, + { + // Scalar to map + name: "scalar to map with AllowKindChange=true", + source: ` +config: + enabled: true + value: 42 +`, + dest: ` +config: "default" +`, + expected: ` +config: + enabled: true + value: 42 +`, + allowKindChange: true, + expectError: false, + }, + { + // Nested kind change + name: "nested kind change with AllowKindChange=true", + source: ` +route: + tls: + termination: edge +`, + dest: ` +route: + tls: [] +`, + expected: ` +route: + tls: + termination: edge +`, + allowKindChange: true, + expectError: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + mergeOpts := yaml.MergeOptions{AllowKindChange: tc.allowKindChange} + actual, err := MergeStrings(tc.source, tc.dest, false, mergeOpts) + + if tc.expectError { + assert.Error(t, err, "expected error but got none") + return + } + + if !assert.NoError(t, err, tc.name) { + t.FailNow() + } + + // Parse and format both expected and actual for comparison + src, err := yaml.Parse(tc.source) + if !assert.NoError(t, err) { + t.FailNow() + } + srcMap, err := src.Map() + if !assert.NoError(t, err) { + t.FailNow() + } + + act, err := yaml.Parse(actual) + if !assert.NoError(t, err) { + t.FailNow() + } + actMap, err := act.Map() + if !assert.NoError(t, err) { + t.FailNow() + } + + // The source values should be present in the result + for key := range srcMap { + assert.Contains(t, actMap, key, "result should contain key from source: %s", key) + } + }) + } +} diff --git a/kyaml/yaml/types.go b/kyaml/yaml/types.go index 7435344d23..00ee2dd579 100644 --- a/kyaml/yaml/types.go +++ b/kyaml/yaml/types.go @@ -246,6 +246,14 @@ type MergeOptions struct { // ListIncreaseDirection indicates should merge function prepend the items from // source list to destination or append. ListIncreaseDirection MergeOptionsListIncreaseDirection + + // AllowKindChange if set to true will allow the merge to replace a field with + // a different type. For example, if the destination has a map and the source + // has a list (or scalar), the source will replace the destination instead of + // returning an error. This is useful for merging Helm values where the chart's + // values.yaml may have a placeholder type that needs to be overridden. + // See https://github.com/kubernetes-sigs/kustomize/issues/5766 + AllowKindChange bool } // Since ObjectMeta and TypeMeta are stable, we manually create DeepCopy funcs for ResourceMeta and ObjectMeta. diff --git a/kyaml/yaml/walk/walk.go b/kyaml/yaml/walk/walk.go index 68de1324e0..77e45c16f6 100644 --- a/kyaml/yaml/walk/walk.go +++ b/kyaml/yaml/walk/walk.go @@ -60,11 +60,25 @@ func (l Walker) Walk() (*yaml.RNode, error) { switch l.Kind() { case yaml.MappingNode: if err := yaml.ErrorIfAnyInvalidAndNonNull(yaml.MappingNode, l.Sources...); err != nil { + // If AllowKindChange is set and there's a type mismatch, allow the + // origin (source) to replace dest entirely instead of erroring. + if l.MergeOptions.AllowKindChange { + if replaced := l.replaceOnKindMismatch(); replaced != nil { + return replaced, nil + } + } return nil, err } return l.walkMap() case yaml.SequenceNode: if err := yaml.ErrorIfAnyInvalidAndNonNull(yaml.SequenceNode, l.Sources...); err != nil { + // If AllowKindChange is set and there's a type mismatch, allow the + // origin (source) to replace dest entirely instead of erroring. + if l.MergeOptions.AllowKindChange { + if replaced := l.replaceOnKindMismatch(); replaced != nil { + return replaced, nil + } + } return nil, err } // AssociativeSequence means the items in the sequence are associative. They can be merged @@ -76,6 +90,13 @@ func (l Walker) Walk() (*yaml.RNode, error) { case yaml.ScalarNode: if err := yaml.ErrorIfAnyInvalidAndNonNull(yaml.ScalarNode, l.Sources...); err != nil { + // If AllowKindChange is set and there's a type mismatch, allow the + // origin (source) to replace dest entirely instead of erroring. + if l.MergeOptions.AllowKindChange { + if replaced := l.replaceOnKindMismatch(); replaced != nil { + return replaced, nil + } + } return nil, err } return l.walkScalar() @@ -87,6 +108,32 @@ func (l Walker) Walk() (*yaml.RNode, error) { } } +// replaceOnKindMismatch returns the origin node when there's a kind mismatch +// between dest and origin. This allows the origin to completely replace the +// dest when they have different types (e.g., map vs list, map vs scalar). +// Returns nil if no replacement should occur. +func (l Walker) replaceOnKindMismatch() *yaml.RNode { + dest := l.Sources.Dest() + origin := l.Sources.Origin() + + // If origin is nil or null, we can't do a replacement + if yaml.IsMissingOrNull(origin) { + return nil + } + + // If dest is nil or null, origin should be used directly + if yaml.IsMissingOrNull(dest) { + return origin + } + + // Check if origin has a different kind than dest - if so, origin replaces dest + if origin.YNode().Kind != dest.YNode().Kind { + return origin + } + + return nil +} + func (l Walker) GetSchema() *openapi.ResourceSchema { for i := range l.Sources { r := l.Sources[i] diff --git a/plugin/builtin/helmchartinflationgenerator/HelmChartInflationGenerator.go b/plugin/builtin/helmchartinflationgenerator/HelmChartInflationGenerator.go index 36a0c6a394..d4089cf172 100644 --- a/plugin/builtin/helmchartinflationgenerator/HelmChartInflationGenerator.go +++ b/plugin/builtin/helmchartinflationgenerator/HelmChartInflationGenerator.go @@ -226,15 +226,20 @@ func (p *plugin) replaceValuesInline() error { return errors.WrapPrefixf(err, "could not parse values inline into rnode") } var outValues *kyaml.RNode + // Enable AllowKindChange to handle cases where the chart's values.yaml has + // a different type (e.g., empty map {}) than what the user provides in valuesInline + // (e.g., a list []). This allows the user's inline values to override the type. + // See https://github.com/kubernetes-sigs/kustomize/issues/5766 + mergeOpts := kyaml.MergeOptions{AllowKindChange: true} switch p.ValuesMerge { // Function `merge2.Merge` overrides values in dest with values from src. // To achieve override or merge behavior, we pass parameters in different order. // Object passed as dest will be modified, so we copy it just in case someone // decides to use it after this is called. case valuesMergeOptionOverride: - outValues, err = merge2.Merge(inlineValues, chValues.Copy(), kyaml.MergeOptions{}) + outValues, err = merge2.Merge(inlineValues, chValues.Copy(), mergeOpts) case valuesMergeOptionMerge: - outValues, err = merge2.Merge(chValues, inlineValues.Copy(), kyaml.MergeOptions{}) + outValues, err = merge2.Merge(chValues, inlineValues.Copy(), mergeOpts) } if err != nil { return errors.WrapPrefixf(err, "could not merge values")