Preserve Helm release values on rad upgrade kubernetes (#11218)#12029
Preserve Helm release values on rad upgrade kubernetes (#11218)#12029nicolejms wants to merge 9 commits into
rad upgrade kubernetes (#11218)#12029Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Pull request overview
This PR fixes rad upgrade kubernetes overwriting previously stored Helm release values by separating chart defaults from CLI-provided overrides and enabling Helm “reset-then-reuse-values” behavior by default, with an explicit --reset-values opt-out.
Changes:
- Parse
--set/--set-fileinto a freshvalsmap (no longer mutatinghelmChart.Values) and passvalsinto Helm install/upgrade calls. - Default upgrades to
ResetThenReuseValuessemantics (preserve previously stored user overrides while picking up new chart defaults), with--reset-valuesto discard stored values. - Apply the same “defaults vs overrides” separation for the Contour chart; update mocks and tests for the new Helm client signatures/behavior.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/helm/radius.go | Returns a separate user-values map parsed from CLI instead of mutating chart defaults. |
| pkg/cli/helm/helmclient.go | Extends HelmClient install/upgrade APIs to accept vals and a reuseValues toggle; sets ResetThenReuseValues. |
| pkg/cli/helm/helmaction.go | Threads vals into install/upgrade and defaults reinstall upgrades to reuse prior stored values. |
| pkg/cli/helm/cluster.go | Adds ResetValues option, plumbs it through upgrade logic, and passes per-invocation vals maps. |
| pkg/cli/cmd/upgrade/kubernetes/kubernetes.go | Adds --reset-values flag and documents the new default upgrade value-preservation behavior. |
| pkg/cli/helm/contour.go | Builds a fresh Contour override map (no chart mutation) for HostNetwork-related settings. |
| pkg/cli/helm/contour_test.go | Updates tests to validate the new buildContourValues behavior. |
| pkg/cli/helm/helmaction_test.go | Updates tests for the new CLI-values parsing helper and adds a regression test (see review comment). |
| pkg/cli/helm/cluster_test.go | Updates mocks/expectations and adds coverage for the --reset-values behavior. |
| pkg/cli/helm/mock_helmclient.go | Regenerated mock to match updated HelmClient method signatures. |
| pkg/cli/helm/mock_cluster.go | Regenerated mock (parameter naming/marker field updates). |
Files not reviewed (2)
- pkg/cli/helm/mock_cluster.go: Language not supported
- pkg/cli/helm/mock_helmclient.go: Language not supported
`rad upgrade kubernetes` was passing chart defaults + current --set args to `helm upgrade` with no reuse flags, so any previously-stored user values (e.g. `global.azureWorkloadIdentity.enabled=true`) were silently reset to chart defaults on every upgrade. Fix: - Stop mutating `helmChart.Values` from --set/--set-file. Parse user overrides into a separate map and pass them to Helm as `vals`. - Set `upgradeClient.ResetThenReuseValues = true` by default so Helm starts from new chart defaults, overlays previously-stored user values, and then overlays the current --set overrides. - Add a `--reset-values` flag on `rad upgrade kubernetes` as an explicit opt-out (matches the `helm` CLI escape hatch). - Apply the same separation to Contour so HostNetwork no longer pollutes stored release values. Fixes #11218 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: aeroNJneer <nicolej@microsoft.com>
nicolejms
force-pushed
the
nicolejms/fix-11218-rad-upgrade-reuse-values
branch
from
37044f3 to
651acf5
Compare
Unit Tests 2 files ± 0 438 suites ±0 7m 30s ⏱️ +14s Results for commit 267f0ec. ± Comparison against base commit 1dbd9a1. This pull request removes 3 and adds 21 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12029 +/- ##
==========================================
+ Coverage 52.13% 52.25% +0.12%
==========================================
Files 734 734
Lines 46704 46717 +13
==========================================
+ Hits 24350 24414 +64
+ Misses 20017 19992 -25
+ Partials 2337 2311 -26 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@copilot add dev unit tests to bring code coverage too 100% for the changes |
- parseUserValuesFromCLI: error paths for invalid --set and --set-file args - prepareRadiusChart: error paths for chart load and value parse failures - prepareContourChart: error path and immutability assertion - ApplyHelmChart: install error, reinstall path, reinstall error, no-op, query error - PopulateDefaultClusterOptions: Contour overrides branch - UpgradeRadius: Radius upgrade error, Contour upgrade error, CheckInstall error
Added comprehensive unit tests in commit
The only uncovered lines are either OS-gated ( |
Radius functional test overviewClick here to see the test run details
Test Status⌛ Building Radius and pushing container images for functional tests... |
willdavsmith
left a comment
willdavsmith
left a comment
There was a problem hiding this comment.
looking pretty good. left a couple of comments/questions
| // By default it preserves the user-supplied values stored on the previous Helm release (ResetThenReuseValues | ||
| // semantics): the upgrade starts from the new chart defaults, re-applies the previously stored user overrides, | ||
| // and then overlays the current invocation's --set / --set-file values. Set ClusterOptions.ResetValues to true | ||
| // to opt out and use only the current invocation's overrides. |
There was a problem hiding this comment.
is there a Helm docs link that we could add here on the ResetThenReuseValues semantics?
There was a problem hiding this comment.
Added in 8a6a9c2. Added the link https://helm.sh/docs/helm/helm_upgrade/#options to the UpgradeRadius function doc comment, the RunHelmUpgrade interface doc, and the implementation doc.
| // Mock Helm Install | ||
| mockHelmClient.EXPECT().RunHelmInstall(gomock.AssignableToTypeOf(&helm.Configuration{}), gomock.AssignableToTypeOf(&chart.Chart{}), "radius", "radius-system", true).Return(radiusRelease, nil).Times(1) | ||
| mockHelmClient.EXPECT().RunHelmInstall(gomock.AssignableToTypeOf(&helm.Configuration{}), gomock.AssignableToTypeOf(&chart.Chart{}), "contour", "radius-system", false).Return(contourRelease, nil).Times(1) | ||
| mockHelmClient.EXPECT().RunHelmInstall(gomock.AssignableToTypeOf(&helm.Configuration{}), gomock.AssignableToTypeOf(&chart.Chart{}), gomock.Any(), "radius", "radius-system", true).Return(radiusRelease, nil).Times(1) |
There was a problem hiding this comment.
why were these changed to gomock.Any()?
There was a problem hiding this comment.
The vals parameter (map[string]any) is now dynamically built by prepareRadiusChart/prepareContourChart from the chart options at runtime, so the exact map contents depend on the test's CLIClusterOptions. Using gomock.Any() avoids brittle tests that break if we add or rename a default value. The important assertions about vals content are covered in the dedicated Test_prepareRadiusChart_DoesNotMutateChartValues and Test_prepareContourChart_DoesNotMutateChartValues tests which verify the exact map contents returned by the prepare functions.
| "dnsPolicy": "ClusterFirstWithHostNet", | ||
| // Container ports for the Envoy pod must be 80/443 to allow Envoy to bind | ||
| // directly on the host network. | ||
| "containerPorts": map[string]any{ |
There was a problem hiding this comment.
@copilot are these changes in contour.go going to cause regressions? or are they correct w.r.t. the previous implementation
There was a problem hiding this comment.
These changes are correct and won't cause regressions. The old addContourValues mutated helmChart.Values in place (navigating into nested maps like helmChart.Values["envoy"].(map[string]any) and writing keys directly). The new buildContourValues produces the exact same key/value structure but as a standalone map that gets passed to Helm as user overrides via the vals parameter.
The end result at render time is identical — Helm merges vals on top of chart defaults — but now the chart's default Values map stays clean. This means ResetThenReuseValues works correctly on subsequent upgrades (only the explicit overrides get stored as "user values" on the release, not the entire mutated defaults tree).
The old code also had defensive nil checks that would return errors if chart values didn't have the expected nested structure (e.g. envoy.containerPorts node not found). Those can't fail in the new code because we're building the map from scratch rather than navigating an existing tree.
| // RunHelmInstall installs the Helm chart using the supplied user-values map as the | ||
| // override set. The map should contain only user-supplied overrides (not the chart | ||
| // defaults); Helm merges them on top of the chart's defaults at render time. | ||
| RunHelmInstall(helmConf *helm.Configuration, helmChart *chart.Chart, vals map[string]any, releaseName, namespace string, wait bool) (*release.Release, error) |
There was a problem hiding this comment.
is there a better data model for vals besides map[string]any? is there a type/interface to use from the helm package? I assume that we would only accept specific values here (helm cli values)
There was a problem hiding this comment.
Helm's own SDK uses map[string]interface{} (i.e., map[string]any) throughout — see action.Install.Run() and action.Upgrade.Run() which both accept vals map[string]interface{}. There's no typed struct or interface in the Helm package for values; they're always an untyped nested map matching the YAML structure. Using map[string]any here is consistent with what the Helm SDK expects and what strvals.Parse() returns.
Description
rad upgrade kuberneteswas passingchart defaults + current --set argstohelm upgradewith no reuse flags, so any previously-stored user values (e.g.global.azureWorkloadIdentity.enabled=true,database.enabled=true) were silently reset to chart defaults on every upgrade.Root cause
pkg/cli/helm/radius.go::addArgsFromCLIparsed--set/--set-filedirectly intohelmChart.Values, mutating the chart-default tree.pkg/cli/helm/helmclient.go::RunHelmUpgradecalledupgradeClient.Run(name, chart, helmChart.Values)without setting anyReuseValues/ResetThenReuseValues. With no reuse flag, Helm treats the passed map as the complete user-supplied override set and discards anything stored on the previous release.Fix
helmChart.Valuesfrom--set/--set-file. Parse user overrides into a separatemap[string]anyand pass them to Helm asvals.upgradeClient.ResetThenReuseValues = trueso Helm starts from the new chart defaults, overlays the user-supplied values stored on the previous release, and then overlays thevalsfrom this upgrade — exactly the semantics requested in the issue. See the Helm upgrade docs for details on--reset-then-reuse-valuesbehavior.--reset-valuesflag onrad upgrade kubernetesas the explicit opt-out (mirrors thehelmCLI escape hatch).HostNetworkno longer pollutes stored release values.Behavioral notes
helmChart.Valuesas "user-supplied values". On the first upgrade after the fix, those previously-polluted values are replayed once. From the next upgrade onward the stored user values are clean. This matcheshelm upgrade --reuse-valuessemantics and is the right trade-off (preserves user intent, no surprise resets).--reset-values.Tests
Test_prepareRadiusChart_DoesNotMutateChartValuesas a regression guard — callsprepareRadiusChartwith a mockedHelmClientreturning a chart with non-emptyValues, asserts (1) the returnedvalscontains the CLI overrides and (2)helmChart.Valuesremains unchanged.Test_Helm_UpgradeRadius_ResetValuesto verify that--reset-valuesflipsreuseValuestofalseon the helm client call.Test_parseUserValuesFromCLI_InvalidSetArg,Test_parseUserValuesFromCLI_InvalidSetFileArg,Test_parseUserValuesFromCLI_EmptyArgs— error and edge-case paths for CLI value parsing.Test_prepareRadiusChart_LoadChartError,Test_prepareRadiusChart_ParseValuesError— error paths in chart preparation.Test_prepareContourChart_LoadChartError,Test_prepareContourChart_DoesNotMutateChartValues— Contour chart error path and immutability assertion.Test_ApplyHelmChart_InstallError,Test_ApplyHelmChart_ReinstallPath,Test_ApplyHelmChart_ReinstallError,Test_ApplyHelmChart_AlreadyInstalled_NoReinstall,Test_ApplyHelmChart_QueryReleaseError— full coverage of install/reinstall/no-op/error paths.Test_PopulateDefaultClusterOptions_ContourOverrides— Contour option override branches.Test_Helm_UpgradeRadius_RadiusUpgradeError,Test_Helm_UpgradeRadius_ContourUpgradeError,Test_Helm_UpgradeRadius_CheckInstallError— upgrade error paths.valsandreuseValuesparameters.go test ./pkg/cli/helm/...andgo test ./pkg/cli/cmd/upgrade/kubernetes/...pass.Type of change
Contributor checklist
Please verify that the PR meets the following requirements, where applicable:
eng/design-notes/in this repository, if new APIs are being introduced.