Edit Helm values during release upgrades (#1139)

Author: hisco

internal/helm/client.go

@@ -26,6 +26,7 @@ import (
 	"github.com/skyhook-io/radar/pkg/helmhistory"
 
 	"helm.sh/helm/v3/pkg/action"
+	"helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/chart/loader"
 	"helm.sh/helm/v3/pkg/cli"
 	"helm.sh/helm/v3/pkg/registry"
@@ -2315,6 +2316,32 @@ func (c *Client) UpgradeAsUser(namespace, name, targetVersion, repositoryName st
 	return c.upgradeWith(actionConfig, name, targetVersion, repositoryName, noop)
 }
 
+// UpgradeWithValuesProgress upgrades a release to a target version applying the
+// supplied user values (WYSIWYG), reporting progress via a channel.
+func (c *Client) UpgradeWithValuesProgress(namespace, name, targetVersion, repositoryName string, newValues map[string]any, progressCh chan<- InstallProgress) error {
+	sendProgress := progressSender(progressCh)
+	sendProgress("preparing", fmt.Sprintf("Getting current release %s...", name), "")
+
+	actionConfig, err := c.getActionConfig(namespace)
+	if err != nil {
+		return err
+	}
+	return c.upgradeWithValues(actionConfig, name, targetVersion, repositoryName, newValues, sendProgress)
+}
+
+// UpgradeWithValuesProgressAsUser upgrades a release to a target version applying
+// the supplied user values with K8s impersonation and progress reporting.
+func (c *Client) UpgradeWithValuesProgressAsUser(namespace, name, targetVersion, repositoryName string, newValues map[string]any, username string, groups []string, progressCh chan<- InstallProgress) error {
+	sendProgress := progressSender(progressCh)
+	sendProgress("preparing", fmt.Sprintf("Getting current release %s...", name), "")
+
+	actionConfig, err := c.getActionConfigForUser(namespace, username, groups)
+	if err != nil {
+		return err
+	}
+	return c.upgradeWithValues(actionConfig, name, targetVersion, repositoryName, newValues, sendProgress)
+}
+
 func progressSender(progressCh chan<- InstallProgress) func(phase, message, detail string) {
 	return func(phase, message, detail string) {
 		if progressCh == nil {
@@ -2335,16 +2362,11 @@ func (c *Client) upgradeWith(actionConfig *action.Configuration, name, targetVer
 		return fmt.Errorf("failed to get current release: %w", err)
 	}
 
-	chartName := rel.Chart.Metadata.Name
-	sendProgress("resolving", fmt.Sprintf("Finding %s version %s in repositories...", chartName, targetVersion), "")
-
-	chartPath, resolvedRepo, err := c.resolveUpgradeChartPath(chartName, targetVersion, repositoryName, chartSourceHosts(rel.Chart.Metadata.Home, rel.Chart.Metadata.Sources))
+	targetChart, err := c.chartForUpgradeTarget(actionConfig, rel, targetVersion, repositoryName, sendProgress)
 	if err != nil {
 		return err
 	}
 
-	sendProgress("downloading", fmt.Sprintf("Downloading %s-%s from %s...", chartName, targetVersion, resolvedRepo), chartPath)
-
 	// Create upgrade action — don't use Wait=true because Radar already
 	// shows real-time resource status via SSE. Waiting blocks the dialog
 	// for minutes with zero feedback; users can monitor the rollout in the UI.
@@ -2357,49 +2379,105 @@ func (c *Client) upgradeWith(actionConfig *action.Configuration, name, targetVer
 	// for keys a newer chart added (a cross-version upgrade footgun).
 	upgradeAction.ResetThenReuseValues = true
 
+	sendProgress("upgrading", fmt.Sprintf("Applying %s %s...", rel.Chart.Metadata.Name, targetVersion), "")
+
+	// Run the upgrade
+	_, err = upgradeAction.Run(name, targetChart, rel.Config)
+	if err != nil {
+		return fmt.Errorf("upgrade failed: %w", err)
+	}
+
+	sendProgress("complete", fmt.Sprintf("Successfully upgraded %s to %s", name, targetVersion), "")
+	return nil
+}
+
+// upgradeWithValues upgrades a release to a target chart version applying exactly
+// the supplied user values (WYSIWYG — ResetValues, no merge with prior overrides).
+// The plain upgradeWith path keeps ResetThenReuseValues for the blind carry-over case.
+func (c *Client) upgradeWithValues(actionConfig *action.Configuration, name, targetVersion, repositoryName string, newValues map[string]any, sendProgress func(phase, message, detail string)) error {
+	getAction := action.NewGet(actionConfig)
+	rel, err := getAction.Run(name)
+	if err != nil {
+		return fmt.Errorf("failed to get current release: %w", err)
+	}
+
+	targetChart, err := c.chartForUpgradeTarget(actionConfig, rel, targetVersion, repositoryName, sendProgress)
+	if err != nil {
+		return err
+	}
+
+	upgradeAction := action.NewUpgrade(actionConfig)
+	upgradeAction.Namespace = rel.Namespace
+	upgradeAction.Timeout = 120 * time.Second
+	upgradeAction.ResetValues = true // WYSIWYG: apply only the edited values
+
+	sendProgress("upgrading", fmt.Sprintf("Applying %s %s...", rel.Chart.Metadata.Name, targetVersion), "")
+
+	_, err = upgradeAction.Run(name, targetChart, newValues)
+	if err != nil {
+		return fmt.Errorf("upgrade failed: %w", err)
+	}
+
+	sendProgress("complete", fmt.Sprintf("Successfully upgraded %s to %s", name, targetVersion), "")
+	return nil
+}
+
+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.loadTargetChart(actionConfig, rel, targetVersion, repositoryName, sendProgress)
+}
+
+// loadTargetChart resolves, downloads and loads the chart for a target upgrade
+// version, refusing a silent chart-swap (the resolved source must publish the
+// same chart the release runs). Shared by the upgrade and preview paths so they
+// resolve the target version identically.
+func (c *Client) loadTargetChart(actionConfig *action.Configuration, rel *release.Release, targetVersion, repositoryName string, sendProgress func(phase, message, detail string)) (*chart.Chart, error) {
+	chartName := rel.Chart.Metadata.Name
+	sendProgress("resolving", fmt.Sprintf("Finding %s version %s in repositories...", chartName, targetVersion), "")
+
+	chartPath, resolvedRepo, err := c.resolveUpgradeChartPath(chartName, targetVersion, repositoryName, chartSourceHosts(rel.Chart.Metadata.Home, rel.Chart.Metadata.Sources))
+	if err != nil {
+		return nil, err
+	}
+
+	sendProgress("downloading", fmt.Sprintf("Downloading %s-%s from %s...", chartName, targetVersion, resolvedRepo), chartPath)
+
 	// Use ChartPathOptions to locate/download the chart
-	client := action.NewInstall(actionConfig)
-	client.Version = targetVersion
+	locate := action.NewInstall(actionConfig)
+	locate.Version = targetVersion
 
 	// OCI pulls need a registry client on the action; Radar's action config
 	// doesn't carry one by default. Wire it from the user's helm registry login.
 	if registry.IsOCI(chartPath) {
 		rc, err := c.newRegistryClientConcrete()
 		if err != nil {
-			return fmt.Errorf("failed to build OCI registry client: %w", err)
+			return nil, fmt.Errorf("failed to build OCI registry client: %w", err)
 		}
-		client.SetRegistryClient(rc)
+		locate.SetRegistryClient(rc)
 	}
 
-	cp, err := client.ChartPathOptions.LocateChart(chartPath, c.settings)
+	cp, err := locate.ChartPathOptions.LocateChart(chartPath, c.settings)
 	if err != nil {
-		return fmt.Errorf("failed to locate chart: %w", err)
+		return nil, fmt.Errorf("failed to locate chart: %w", err)
 	}
 
 	sendProgress("loading", "Loading chart...", cp)
 
-	chart, err := loader.Load(cp)
+	targetChart, err := loader.Load(cp)
 	if err != nil {
-		return fmt.Errorf("failed to load chart: %w", err)
+		return nil, fmt.Errorf("failed to load chart: %w", err)
 	}
 
 	// Refuse a silent chart-swap: the resolved source must publish the SAME chart
 	// the release runs, not merely a chart at the same version. Matters most for
 	// OCI prefix probing, where "<prefix>/<chartName>" is derived, not asserted.
-	if chart.Metadata != nil && chart.Metadata.Name != chartName {
-		return fmt.Errorf("resolved chart is %q but release %q runs chart %q — refusing to swap charts", chart.Metadata.Name, name, chartName)
-	}
-
-	sendProgress("upgrading", fmt.Sprintf("Applying %s %s...", chartName, targetVersion), "")
-
-	// Run the upgrade
-	_, err = upgradeAction.Run(name, chart, rel.Config)
-	if err != nil {
-		return fmt.Errorf("upgrade failed: %w", err)
+	if targetChart.Metadata != nil && targetChart.Metadata.Name != chartName {
+		return nil, fmt.Errorf("resolved chart is %q but release %q runs chart %q — refusing to swap charts", targetChart.Metadata.Name, rel.Name, chartName)
 	}
 
-	sendProgress("complete", fmt.Sprintf("Successfully upgraded %s to %s", name, targetVersion), "")
-	return nil
+	return targetChart, nil
 }
 
 type chartPathCandidate struct {
@@ -2445,7 +2523,7 @@ func (c *Client) resolveUpgradeChartPathWithOCIResolver(chartName, targetVersion
 					continue
 				}
 				path := entry.URLs[0]
-				if !strings.HasPrefix(path, "http://") && !strings.HasPrefix(path, "https://") {
+				if !isAbsoluteChartURL(path) {
 					path = strings.TrimSuffix(r.URL, "/") + "/" + path
 				}
 				candidates = append(candidates, chartPathCandidate{repoName: r.Name, repoURL: r.URL, chartPath: path})
@@ -2492,6 +2570,10 @@ func (c *Client) resolveUpgradeChartPathWithOCIResolver(chartName, targetVersion
 	return "", "", fmt.Errorf("chart %s version %s not found in configured repositories or registered OCI sources", chartName, targetVersion)
 }
 
+func isAbsoluteChartURL(path string) bool {
+	return strings.HasPrefix(path, "http://") || strings.HasPrefix(path, "https://") || registry.IsOCI(path)
+}
+
 // BatchCheckUpgrades checks for upgrades for all releases at once (more efficient)
 func (c *Client) BatchCheckUpgrades(namespace string) (*BatchUpgradeInfo, error) {
 	return c.batchCheckUpgrades(namespace, "", nil)
@@ -2689,13 +2771,26 @@ func (c *Client) batchCheckUpgrades(namespace, username string, groups []string)
 	return result, nil
 }
 
-// PreviewValuesChange previews the effect of new values on a release via dry-run
-func (c *Client) PreviewValuesChange(namespace, name string, newValues map[string]any) (*ValuesPreviewResponse, error) {
+// PreviewValuesChange previews the effect of new values on a release via dry-run.
+// When targetVersion is non-empty and differs from the running chart version, the
+// dry-run renders against that target chart instead of the current chart.
+func (c *Client) PreviewValuesChange(namespace, name string, newValues map[string]any, targetVersion, repositoryName string) (*ValuesPreviewResponse, error) {
 	actionConfig, err := c.getActionConfig(namespace)
 	if err != nil {
 		return nil, err
 	}
+	return c.previewValuesChangeWith(actionConfig, name, newValues, targetVersion, repositoryName)
+}
+
+func (c *Client) PreviewValuesChangeAsUser(namespace, name string, newValues map[string]any, targetVersion, repositoryName string, username string, groups []string) (*ValuesPreviewResponse, error) {
+	actionConfig, err := c.getActionConfigForUser(namespace, username, groups)
+	if err != nil {
+		return nil, err
+	}
+	return c.previewValuesChangeWith(actionConfig, name, newValues, targetVersion, repositoryName)
+}
 
+func (c *Client) previewValuesChangeWith(actionConfig *action.Configuration, name string, newValues map[string]any, targetVersion, repositoryName string) (*ValuesPreviewResponse, error) {
 	// Get the current release
 	getAction := action.NewGet(actionConfig)
 	rel, err := getAction.Run(name)
@@ -2713,6 +2808,12 @@ func (c *Client) PreviewValuesChange(namespace, name string, newValues map[strin
 	// Get current manifest
 	currentManifest := rel.Manifest
 
+	noop := func(phase, message, detail string) {}
+	previewChart, err := c.chartForUpgradeTarget(actionConfig, rel, targetVersion, repositoryName, noop)
+	if err != nil {
+		return nil, err
+	}
+
 	// Perform a dry-run upgrade with the new values
 	upgradeAction := action.NewUpgrade(actionConfig)
 	upgradeAction.Namespace = rel.Namespace
@@ -2721,7 +2822,7 @@ func (c *Client) PreviewValuesChange(namespace, name string, newValues map[strin
 	upgradeAction.ResetValues = true // Use only the provided values, don't merge
 
 	// Run the dry-run upgrade
-	newRel, err := upgradeAction.Run(name, rel.Chart, newValues)
+	newRel, err := upgradeAction.Run(name, previewChart, newValues)
 	if err != nil {
 		return nil, fmt.Errorf("failed to preview values change: %w", err)
 	}

internal/helm/client_test.go

@@ -637,6 +637,22 @@ func TestGetValuesWithAllValuesKeepsComputedWhenUserValuesReadFails(t *testing.T
 	}
 }
 
+func TestChartForUpgradeTargetReusesReleaseChartForSameVersion(t *testing.T) {
+	client := testHelmClientWithRepoConfigOnly(t)
+	rel := helmTestRelease("argo-cd", "demo", 1, release.StatusDeployed, "deployed")
+	rel.Chart.Metadata.Version = "9.5.11"
+
+	got, err := client.chartForUpgradeTarget(nil, rel, "9.5.11", "missing-repo", func(phase, message, detail string) {
+		t.Fatalf("same-version chart selection should not resolve/download chart, got progress %q %q %q", phase, message, detail)
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got != rel.Chart {
+		t.Fatal("chartForUpgradeTarget returned a different chart, want current release chart")
+	}
+}
+
 func TestDiffResourceRefs(t *testing.T) {
 	left := []ResourceRef{
 		{APIVersion: "apps/v1", Kind: "Deployment", Namespace: "demo", Name: "cart"},
@@ -1437,6 +1453,49 @@ func TestResolveUpgradeChartPath_UsesRepositoryHint(t *testing.T) {
 	}
 }
 
+func TestResolveUpgradeChartPath_RepositoryIndexOCIURLIsAbsolute(t *testing.T) {
+	dir := t.TempDir()
+	cacheDir := filepath.Join(dir, "cache")
+	if err := os.Mkdir(cacheDir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	repoFile := filepath.Join(dir, "repositories.yaml")
+	if err := os.WriteFile(repoFile, []byte(`apiVersion: v1
+generated: "2026-05-05T00:00:00Z"
+repositories:
+- name: bitnami
+  url: https://charts.bitnami.com/bitnami
+`), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(cacheDir, "bitnami-index.yaml"), []byte(`apiVersion: v1
+entries:
+  nginx:
+  - name: nginx
+    version: 25.0.5
+    urls:
+    - oci://registry-1.docker.io/bitnamicharts/nginx
+generated: "2026-05-05T00:00:00Z"
+`), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	client := &Client{settings: &cli.EnvSettings{
+		RepositoryConfig: repoFile,
+		RepositoryCache:  cacheDir,
+	}}
+
+	chartPath, repoName, err := client.resolveUpgradeChartPath("nginx", "25.0.5", "bitnami", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if repoName != "bitnami" {
+		t.Fatalf("repo = %q, want bitnami", repoName)
+	}
+	if chartPath != "oci://registry-1.docker.io/bitnamicharts/nginx" {
+		t.Fatalf("chart path = %q, want OCI URL unchanged", chartPath)
+	}
+}
+
 func TestResolveUpgradeChartPath_AmbiguousWithoutHintOrAffinity(t *testing.T) {
 	client := testHelmClientWithRepos(t)