fix(helm): preserve umbrella dependencies during values updates

Author: StefanSa

internal/helm/client.go

@@ -2561,10 +2561,96 @@ func (c *Client) upgradeWithValues(actionConfig *action.Configuration, name, tar
 }
 
 func (c *Client) chartForUpgradeTarget(actionConfig *action.Configuration, rel *release.Release, targetVersion, repositoryName string, sendProgress func(phase, message, detail string)) (*chart.Chart, error) {
-	if targetVersion == "" || targetVersion == rel.Chart.Metadata.Version {
-		return rel.Chart, nil
+	return c.chartForUpgradeTargetWithLoader(actionConfig, rel, targetVersion, repositoryName, sendProgress, c.loadTargetChart)
+}
+
+type targetChartLoader func(*action.Configuration, *release.Release, string, string, func(string, string, string)) (*chart.Chart, error)
+
+// chartForUpgradeTargetWithLoader returns a chart that is safe to render. Helm's
+// stored release representation does not serialize Chart.dependencies, so an
+// umbrella release can retain dependency declarations in Chart.yaml while all
+// child chart bodies are absent. Reusing that object for a same-version values
+// operation renders only the parent and makes the resulting upgrade destructive.
+//
+// A complete in-memory release chart remains reusable. An incomplete one is
+// reconstructed through the same configured-source resolver used by upgrades,
+// pinned to the installed chart version. The loaded chart is checked again so a
+// source package that merely declares (but does not vendor) dependencies fails
+// closed.
+func (c *Client) chartForUpgradeTargetWithLoader(actionConfig *action.Configuration, rel *release.Release, targetVersion, repositoryName string, sendProgress func(phase, message, detail string), load targetChartLoader) (*chart.Chart, error) {
+	if rel == nil || rel.Chart == nil || rel.Chart.Metadata == nil {
+		return nil, fmt.Errorf("release has no usable chart metadata")
+	}
+
+	currentVersion := rel.Chart.Metadata.Version
+	if targetVersion == "" {
+		targetVersion = currentVersion
+	}
+
+	reconstructingStoredChart := false
+	if targetVersion == currentVersion {
+		if err := validateChartDependencyBodies(rel.Chart); err == nil {
+			return rel.Chart, nil
+		}
+		reconstructingStoredChart = true
+	}
+
+	loaded, err := load(actionConfig, rel, targetVersion, repositoryName, sendProgress)
+	if err != nil {
+		if reconstructingStoredChart {
+			return nil, fmt.Errorf("could not reconstruct complete chart %s version %s for values operation: %w", rel.Chart.Metadata.Name, targetVersion, err)
+		}
+		return nil, err
+	}
+	if err := validateChartDependencyBodies(loaded); err != nil {
+		return nil, fmt.Errorf("refusing to render incomplete chart %s version %s: %w", rel.Chart.Metadata.Name, targetVersion, err)
+	}
+	return loaded, nil
+}
+
+// validateChartDependencyBodies verifies that every dependency declared by each
+// chart in the tree has a corresponding loaded chart body. Alias names are
+// accepted because Helm rewrites an aliased child name while processing values.
+func validateChartDependencyBodies(ch *chart.Chart) error {
+	if ch == nil || ch.Metadata == nil {
+		return fmt.Errorf("chart metadata is missing")
+	}
+
+	children := ch.Dependencies()
+	matchedChildren := make([]bool, len(children))
+	for _, declared := range ch.Metadata.Dependencies {
+		if declared == nil {
+			continue
+		}
+		found := false
+		for i, child := range children {
+			if matchedChildren[i] {
+				continue
+			}
+			if child == nil || child.Metadata == nil {
+				continue
+			}
+			if child.Metadata.Name == declared.Name || (declared.Alias != "" && child.Metadata.Name == declared.Alias) {
+				matchedChildren[i] = true
+				found = true
+				break
+			}
+		}
+		if !found {
+			name := declared.Name
+			if declared.Alias != "" {
+				name += " (alias " + declared.Alias + ")"
+			}
+			return fmt.Errorf("dependency body %q declared by chart %q is missing", name, ch.Metadata.Name)
+		}
+	}
+
+	for _, child := range children {
+		if err := validateChartDependencyBodies(child); err != nil {
+			return err
+		}
 	}
-	return c.loadTargetChart(actionConfig, rel, targetVersion, repositoryName, sendProgress)
+	return nil
 }
 
 // loadTargetChart resolves, downloads and loads the chart for a target upgrade
@@ -2994,21 +3080,27 @@ func (c *Client) ApplyValuesAsUser(namespace, name string, newValues map[string]
 }
 
 func (c *Client) applyValuesWith(actionConfig *action.Configuration, name string, newValues map[string]any) error {
-	// Get the current release to reuse its chart
+	// Get the current release and reconstruct its exact installed chart when
+	// Helm storage omitted vendored dependency bodies.
 	getAction := action.NewGet(actionConfig)
 	rel, err := getAction.Run(name)
 	if err != nil {
 		return fmt.Errorf("failed to get current release: %w", err)
 	}
+	noop := func(phase, message, detail string) {}
+	applyChart, err := c.chartForUpgradeTarget(actionConfig, rel, rel.Chart.Metadata.Version, "", noop)
+	if err != nil {
+		return err
+	}
 
 	// Create upgrade action — no Wait, Radar shows resource status in real-time
 	upgradeAction := action.NewUpgrade(actionConfig)
 	upgradeAction.Namespace = rel.Namespace
 	upgradeAction.Timeout = 120 * time.Second
 	upgradeAction.ResetValues = true // Use only the provided values, don't merge
 
-	// Run the upgrade with the existing chart and new values
-	_, err = upgradeAction.Run(name, rel.Chart, newValues)
+	// Run the upgrade with the complete installed-version chart and new values.
+	_, err = upgradeAction.Run(name, applyChart, newValues)
 	if err != nil {
 		return fmt.Errorf("failed to apply values: %w", err)
 	}

internal/helm/client_test.go

@@ -11,6 +11,7 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"sort"
 	"strings"
 	"testing"
 	"time"
@@ -19,7 +20,9 @@ import (
 
 	"helm.sh/helm/v3/pkg/action"
 	"helm.sh/helm/v3/pkg/chart"
+	"helm.sh/helm/v3/pkg/chartutil"
 	"helm.sh/helm/v3/pkg/cli"
+	"helm.sh/helm/v3/pkg/engine"
 	kubefake "helm.sh/helm/v3/pkg/kube/fake"
 	"helm.sh/helm/v3/pkg/release"
 	helmstorage "helm.sh/helm/v3/pkg/storage"
@@ -654,6 +657,161 @@ func TestChartForUpgradeTargetReusesReleaseChartForSameVersion(t *testing.T) {
 	}
 }
 
+// Regression for Values Preview/Apply on umbrella releases. Helm storage keeps
+// Chart.yaml's dependency declarations but JSON serialization drops the chart
+// dependency bodies. Both values paths select their chart through
+// chartForUpgradeTarget, so same-version selection must reconstruct the exact
+// installed version before rendering or applying.
+func TestValuesPreviewApplyReconstructSameVersionUmbrella(t *testing.T) {
+	complete := valuesIsolationUmbrellaChart()
+	stored := roundTripStoredChart(t, complete)
+	if len(stored.Metadata.Dependencies) != 1 || len(stored.Dependencies()) != 0 {
+		t.Fatalf("test precondition failed: stored chart declarations=%d bodies=%d, want 1 and 0", len(stored.Metadata.Dependencies), len(stored.Dependencies()))
+	}
+
+	rel := helmTestRelease("umbrella", "demo", 1, release.StatusDeployed, "deployed")
+	rel.Chart = stored
+	client := &Client{}
+	loadCalls := 0
+	selected, err := client.chartForUpgradeTargetWithLoader(nil, rel, "", "", func(string, string, string) {}, func(_ *action.Configuration, gotRel *release.Release, version, repository string, _ func(string, string, string)) (*chart.Chart, error) {
+		loadCalls++
+		if gotRel != rel || version != "1.0.4" || repository != "" {
+			t.Fatalf("loader got release=%p version=%q repository=%q", gotRel, version, repository)
+		}
+		return complete, nil
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if loadCalls != 1 || selected != complete {
+		t.Fatalf("loader calls=%d selected=%p, want one call and complete chart %p", loadCalls, selected, complete)
+	}
+
+	before := renderValuesIsolationChart(t, selected, map[string]any{"parentTag": "1.0.0", "child": map[string]any{"imageTag": "2.0.0"}})
+	after := renderValuesIsolationChart(t, selected, map[string]any{"parentTag": "1.0.1", "child": map[string]any{"imageTag": "2.0.0"}})
+	if len(before) != 2 || len(after) != 2 {
+		t.Fatalf("rendered inventory before=%v after=%v, want parent and child", mapKeys(before), mapKeys(after))
+	}
+	for name, body := range before {
+		afterBody, ok := after[name]
+		if !ok {
+			t.Fatalf("object inventory changed: %q missing after values edit", name)
+		}
+		if strings.Contains(name, "/charts/child/") {
+			if afterBody != body {
+				t.Fatalf("child workload changed for parent-only values override\nbefore:\n%s\nafter:\n%s", body, afterBody)
+			}
+		} else if afterBody == body || !strings.Contains(afterBody, "parent:1.0.1") {
+			t.Fatalf("parent workload did not receive intended image override\nbefore:\n%s\nafter:\n%s", body, afterBody)
+		}
+	}
+}
+
+func TestValuesPreviewApplyFailsClosedWhenDependencyBodiesUnavailable(t *testing.T) {
+	stored := roundTripStoredChart(t, valuesIsolationUmbrellaChart())
+	rel := helmTestRelease("umbrella", "demo", 1, release.StatusDeployed, "deployed")
+	rel.Chart = stored
+
+	t.Run("source cannot resolve exact installed version", func(t *testing.T) {
+		_, err := (&Client{}).chartForUpgradeTargetWithLoader(nil, rel, "1.0.4", "", func(string, string, string) {}, func(*action.Configuration, *release.Release, string, string, func(string, string, string)) (*chart.Chart, error) {
+			return nil, errors.New("chart not found in configured sources")
+		})
+		if err == nil || !strings.Contains(err.Error(), "could not reconstruct complete chart umbrella version 1.0.4") {
+			t.Fatalf("error = %v, want fail-closed reconstruction error", err)
+		}
+	})
+
+	t.Run("resolved package also lacks dependency body", func(t *testing.T) {
+		_, err := (&Client{}).chartForUpgradeTargetWithLoader(nil, rel, "1.0.4", "", func(string, string, string) {}, func(*action.Configuration, *release.Release, string, string, func(string, string, string)) (*chart.Chart, error) {
+			return stored, nil
+		})
+		if err == nil || !strings.Contains(err.Error(), "refusing to render incomplete chart") || !strings.Contains(err.Error(), "dependency body \"child\"") {
+			t.Fatalf("error = %v, want explicit missing-dependency refusal", err)
+		}
+	})
+}
+
+func valuesIsolationUmbrellaChart() *chart.Chart {
+	child := &chart.Chart{
+		Metadata: &chart.Metadata{Name: "child", Version: "2.0.0", Type: "application"},
+		Values:   map[string]any{"imageTag": "2.0.0"},
+		Templates: []*chart.File{{Name: "templates/deployment.yaml", Data: []byte(`apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: child
+spec:
+  selector:
+    matchLabels: {app: child}
+  template:
+    metadata:
+      labels: {app: child}
+    spec:
+      containers:
+      - name: child
+        image: child:{{ .Values.imageTag }}
+`)}},
+	}
+	parent := &chart.Chart{
+		Metadata: &chart.Metadata{
+			Name: "umbrella", Version: "1.0.4", Type: "application",
+			Dependencies: []*chart.Dependency{{Name: "child", Version: "2.0.0"}},
+		},
+		Values: map[string]any{"parentTag": "1.0.0", "child": map[string]any{"imageTag": "2.0.0"}},
+		Templates: []*chart.File{{Name: "templates/deployment.yaml", Data: []byte(`apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: parent
+spec:
+  selector:
+    matchLabels: {app: parent}
+  template:
+    metadata:
+      labels: {app: parent}
+    spec:
+      containers:
+      - name: parent
+        image: parent:{{ .Values.parentTag }}
+`)}},
+	}
+	parent.SetDependencies(child)
+	return parent
+}
+
+func roundTripStoredChart(t *testing.T, ch *chart.Chart) *chart.Chart {
+	t.Helper()
+	b, err := json.Marshal(ch)
+	if err != nil {
+		t.Fatal(err)
+	}
+	var stored chart.Chart
+	if err := json.Unmarshal(b, &stored); err != nil {
+		t.Fatal(err)
+	}
+	return &stored
+}
+
+func renderValuesIsolationChart(t *testing.T, ch *chart.Chart, values map[string]any) map[string]string {
+	t.Helper()
+	renderValues, err := chartutil.ToRenderValues(ch, values, chartutil.ReleaseOptions{Name: "umbrella", Namespace: "demo", IsUpgrade: true}, chartutil.DefaultCapabilities)
+	if err != nil {
+		t.Fatal(err)
+	}
+	rendered, err := engine.Render(ch, renderValues)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return rendered
+}
+
+func mapKeys[V any](m map[string]V) []string {
+	keys := make([]string, 0, len(m))
+	for key := range m {
+		keys = append(keys, key)
+	}
+	sort.Strings(keys)
+	return keys
+}
+
 func TestDiffResourceRefs(t *testing.T) {
 	left := []ResourceRef{
 		{APIVersion: "apps/v1", Kind: "Deployment", Namespace: "demo", Name: "cart"},