From 97b09821d5b3d5a79a7a27e4564a0b62335aaeff Mon Sep 17 00:00:00 2001 From: John Howard Date: Mon, 13 Nov 2023 09:50:28 -0800 Subject: [PATCH] Support overwritting values from `import-values` dependencies Alternative to https://github.com/helm/helm/issues/12466 This allows the above requirement to be implemented in terms of existing Helm functionality. Instead of built in support for a "bundled value set", a chart can expose a dependency on a chart which provides some sets of values. These can be conditionally enabled with `import-values` and `conditions` on the dependency. The one gap in this approach is the parent values take precedence. This allows a user to *opt in* to making children high precedence. Additionally, a unit test and additional validation for the field is added. Note: overall, this is a worse experience than #12466 could provide for this use case. So I would prefer to still do that if feasible. However, this is a low-cost approach Signed-off-by: John Howard --- pkg/chart/dependency.go | 25 +++++++++++ pkg/chartutil/dependencies.go | 54 ++++++++++++++++------- pkg/chartutil/dependencies_test.go | 9 ++++ pkg/chartutil/testdata/subpop/Chart.yaml | 3 ++ pkg/chartutil/testdata/subpop/values.yaml | 6 +++ 5 files changed, 81 insertions(+), 16 deletions(-) diff --git a/pkg/chart/dependency.go b/pkg/chart/dependency.go index 4ef5eeb32..ee740e4a4 100644 --- a/pkg/chart/dependency.go +++ b/pkg/chart/dependency.go @@ -66,6 +66,31 @@ func (d *Dependency) Validate() error { if d.Alias != "" && !aliasNameFormat.MatchString(d.Alias) { return ValidationErrorf("dependency %q has disallowed characters in the alias", d.Name) } + for _, riv := range d.ImportValues { + switch iv := riv.(type) { + case map[string]interface{}: + for k, v := range iv { + switch k { + case "child": + if _, ok := v.(string); !ok { + return ValidationErrorf("dependency %q has invalid importValues; child must be a string", d.Name) + } + case "parent": + if _, ok := v.(string); !ok { + return ValidationErrorf("dependency %q has invalid importValues; parent must be a string", d.Name) + } + case "overwrite": + if _, ok := v.(bool); !ok { + return ValidationErrorf("dependency %q has invalid importValues; overwrite must be a boolean", d.Name) + } + } + } + case string: + // String is always valid + default: + return ValidationErrorf("dependency %q has invalid importValues of type %T", d.Name, iv) + } + } return nil } diff --git a/pkg/chartutil/dependencies.go b/pkg/chartutil/dependencies.go index a87f0fbe8..79bf933b4 100644 --- a/pkg/chartutil/dependencies.go +++ b/pkg/chartutil/dependencies.go @@ -249,7 +249,11 @@ func processImportValues(c *chart.Chart, merge bool) error { if err != nil { return err } - b := make(map[string]interface{}) + // Imports can be lower or higher precedence from the primary chart + // beforeParent are applied first, with lower precedence + beforeParent := []map[string]interface{}{} + // afterParent are applied last, with higher precedence + afterParent := []map[string]interface{}{} // import values from each dependency if specified in import-values for _, r := range c.Metadata.Dependencies { var outiv []interface{} @@ -258,11 +262,18 @@ func processImportValues(c *chart.Chart, merge bool) error { case map[string]interface{}: child := iv["child"].(string) parent := iv["parent"].(string) - - outiv = append(outiv, map[string]string{ + var overwrite bool + if o, f := iv["overwrite"]; f { + overwrite = o.(bool) + } + m := map[string]interface{}{ "child": child, "parent": parent, - }) + } + if overwrite { + m["overwrite"] = overwrite + } + outiv = append(outiv, m) // get child table vv, err := cvals.Table(r.Name + "." + child) @@ -270,15 +281,14 @@ func processImportValues(c *chart.Chart, merge bool) error { log.Printf("Warning: ImportValues missing table from chart %s: %v", r.Name, err) continue } - // create value map from child to be merged into parent - if merge { - b = MergeTables(b, pathToMap(parent, vv.AsMap())) + if overwrite { + afterParent = append(afterParent, pathToMap(parent, vv.AsMap())) } else { - b = CoalesceTables(b, pathToMap(parent, vv.AsMap())) + beforeParent = append(beforeParent, pathToMap(parent, vv.AsMap())) } case string: child := "exports." + iv - outiv = append(outiv, map[string]string{ + outiv = append(outiv, map[string]interface{}{ "child": child, "parent": ".", }) @@ -287,16 +297,20 @@ func processImportValues(c *chart.Chart, merge bool) error { log.Printf("Warning: ImportValues missing table: %v", err) continue } - if merge { - b = MergeTables(b, vm.AsMap()) - } else { - b = CoalesceTables(b, vm.AsMap()) - } + beforeParent = append(beforeParent, vm.AsMap()) } } r.ImportValues = outiv } + b := make(map[string]interface{}) + for _, mv := range beforeParent { + if merge { + b = MergeTables(b, mv) + } else { + b = CoalesceTables(b, mv) + } + } // Imported values from a child to a parent chart have a lower priority than // the parents values. This enables parent charts to import a large section // from a child and then override select parts. This is why b is merged into @@ -305,15 +319,23 @@ func processImportValues(c *chart.Chart, merge bool) error { // deep copying the cvals as there are cases where pointers can end // up in the cvals when they are copied onto b in ways that break things. cvals = deepCopyMap(cvals) - c.Values = MergeTables(cvals, b) + b = MergeTables(cvals, b) } else { // Trimming the nil values from cvals is needed for backwards compatibility. // Previously, the b value had been populated with cvals along with some // overrides. This caused the coalescing functionality to remove the // nil/null values. This trimming is for backwards compat. cvals = trimNilValues(cvals) - c.Values = CoalesceTables(cvals, b) + b = CoalesceTables(cvals, b) + } + for _, mv := range afterParent { + if merge { + b = MergeTables(mv, b) + } else { + b = CoalesceTables(mv, b) + } } + c.Values = b return nil } diff --git a/pkg/chartutil/dependencies_test.go b/pkg/chartutil/dependencies_test.go index 7e035be5f..3cf0576aa 100644 --- a/pkg/chartutil/dependencies_test.go +++ b/pkg/chartutil/dependencies_test.go @@ -190,6 +190,15 @@ func TestProcessDependencyImportValues(t *testing.T) { e["overridden-chart1.SC1string"] = "pollywog" e["overridden-chart1.SPextra2"] = "42" + // These values are imported from the child chart to the parent. Parent values are present. + // Child is configred to take precedence + e["child-overridden-chart1.SC1bool"] = "true" + e["child-overridden-chart1.SC1float"] = "3.14" + e["child-overridden-chart1.SC1int"] = "100" + e["child-overridden-chart1.SC1string"] = "dollywood" + e["child-overridden-chart1.SC1extra1"] = "11" + e["child-overridden-chart1.SPextra2"] = "42" + e["overridden-chartA.SCAbool"] = "true" e["overridden-chartA.SCAfloat"] = "41.3" e["overridden-chartA.SCAint"] = "808" diff --git a/pkg/chartutil/testdata/subpop/Chart.yaml b/pkg/chartutil/testdata/subpop/Chart.yaml index 27118672a..1ab5962c7 100644 --- a/pkg/chartutil/testdata/subpop/Chart.yaml +++ b/pkg/chartutil/testdata/subpop/Chart.yaml @@ -15,6 +15,9 @@ dependencies: parent: imported-chart1 - child: SC1data parent: overridden-chart1 + - child: SC1data + parent: child-overridden-chart1 + overwrite: true - child: imported-chartA parent: imported-chartA - child: imported-chartA-B diff --git a/pkg/chartutil/testdata/subpop/values.yaml b/pkg/chartutil/testdata/subpop/values.yaml index ba70ed406..70c31b37e 100644 --- a/pkg/chartutil/testdata/subpop/values.yaml +++ b/pkg/chartutil/testdata/subpop/values.yaml @@ -10,6 +10,12 @@ overridden-chart1: SC1string: "pollywog" SPextra2: 42 +child-overridden-chart1: + SC1bool: false + SC1float: 3.141592 + SC1int: 99 + SC1string: "pollywog" + SPextra2: 42 imported-chartA: SPextra3: 1.337