From 8ded5703dcb5bc8ea2ab3b679ee1769a6654c368 Mon Sep 17 00:00:00 2001 From: Amir Fowler Date: Thu, 10 Sep 2026 11:35:21 -0700 Subject: [PATCH 1/2] fix: don't run Sveltos templating over Helm values on chart-identity paths getInstantiatedChart instantiates every field of a HelmChart as a Sveltos Go template, including Values. Values that are not valid Sveltos templates (e.g. helm-style {{ .Values.x }} placeholders meant for the chart's own tpl rendering) made instantiation fail on paths that only need the chart identity: - updateChartMap: the ClusterSummary never registers its charts with the chartManager, so allMatchingProfilesProcessed never sees the profile as processed and canUninstallHelmChart returns WaitForProfileProcessingError forever: helm uninstall of every other profile on the cluster is blocked. - uninstallHelmCharts / getClusterSummaryWithInstantiatedCharts on the undeploy path: undeploy retries the template error forever and the ClusterSummary finalizer is never removed, wedging profile deletion. Values are already instantiated at the point of use (install/upgrade/hash) by getHelmChartInstantiatedValues, so instantiating them here was also a double templating. Leave Values untouched in getInstantiatedChart and let the deploy path surface the template error as a per-chart failure instead. Co-Authored-By: Claude Fable 5 --- controllers/handlers_helm.go | 12 ++++++++ controllers/handlers_helm_test.go | 46 +++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/controllers/handlers_helm.go b/controllers/handlers_helm.go index b15a4cf2..0be345fa 100644 --- a/controllers/handlers_helm.go +++ b/controllers/handlers_helm.go @@ -5429,6 +5429,16 @@ func getInstantiatedChart(ctx context.Context, dCtx *deploymentContext, // Create a deep copy of the chart to avoid modifying the original. instantiatedChart := currentChart.DeepCopy() + // Values are deliberately left out of the instantiation below: they are templated by + // getHelmChartInstantiatedValues at the point of use (install/upgrade/hash). This method + // is also called on paths that only need the chart identity (chartManager registration, + // uninstall): failing those paths on values that are not valid Sveltos templates (e.g. + // helm-style {{ .Values.x }} placeholders meant for the chart's own tpl rendering) would + // permanently wedge undeploy of every chart in the profile and, through + // allMatchingProfilesProcessed, block helm uninstalls of other profiles on the cluster. + values := instantiatedChart.Values + instantiatedChart.Values = "" + // Call the new recursive helper function to instantiate all fields. if err := instantiateStructFields(ctx, getManagementClusterConfig(), getManagementClusterClient(), instantiatedChart, dCtx.clusterSummary, dCtx.clusterObjects, dCtx.mgmtResources, logger); err != nil { @@ -5437,6 +5447,8 @@ func getInstantiatedChart(ctx context.Context, dCtx *deploymentContext, return nil, &configv1beta1.TemplateInstantiationError{Message: msg} } + instantiatedChart.Values = values + return instantiatedChart, nil } diff --git a/controllers/handlers_helm_test.go b/controllers/handlers_helm_test.go index a56a54e6..d2691d56 100644 --- a/controllers/handlers_helm_test.go +++ b/controllers/handlers_helm_test.go @@ -1329,6 +1329,52 @@ var _ = Describe("HandlersHelm", func() { Expect(instaniatedChart.ChartVersion).To(Equal("25.0.2")) }) + It("getInstantiatedChart leaves Values alone even when they are not a valid Sveltos template", func() { + helmChart := &configv1beta1.HelmChart{ + ReleaseName: randomString(), ReleaseNamespace: randomString(), + ChartName: randomString(), ChartVersion: randomString(), + RepositoryURL: randomString(), RepositoryName: randomString(), + HelmChartAction: configv1beta1.HelmChartActionInstall, + // Helm-style placeholders meant for the chart's own tpl rendering. They are not + // valid Sveltos templates (.Values does not exist in the Sveltos template context): + // instantiating them here used to fail and wedge chart registration and uninstall. + Values: `config: + service: | + [SERVICE] + Flush {{ .Values.flush }} + Log_Level {{ .Values.logLevel }}`, + } + + clusterSummary.Namespace = defaultNamespace + clusterSummary.Spec.ClusterNamespace = defaultNamespace + + cluster := &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterSummary.Spec.ClusterName, + Namespace: clusterSummary.Spec.ClusterNamespace, + }, + } + + Expect(testEnv.Create(context.TODO(), cluster)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, cluster)).To(Succeed()) + + Expect(testEnv.Create(context.TODO(), clusterSummary)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, clusterSummary)).To(Succeed()) + + clusterObjects, err := controllers.FetchClusterObjects(context.TODO(), testEnv.Config, testEnv.Client, + clusterSummary.Spec.ClusterNamespace, clusterSummary.Spec.ClusterName, libsveltosv1beta1.ClusterTypeCapi, + textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + + instaniatedChart, err := controllers.GetInstantiatedChart(context.TODO(), + controllers.NewDeploymentContext(clusterSummary, clusterObjects, nil), helmChart, + textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + Expect(instaniatedChart.Values).To(Equal(helmChart.Values)) + Expect(instaniatedChart.ReleaseName).To(Equal(helmChart.ReleaseName)) + Expect(instaniatedChart.ReleaseNamespace).To(Equal(helmChart.ReleaseNamespace)) + }) + It("updateClusterReportWithHelmReports updates ClusterReports with HelmReports", func() { helmChart := &configv1beta1.HelmChart{ ReleaseName: randomString(), ReleaseNamespace: randomString(), From 4892b83bcb7d3c0ee0614fd1e5d81f499a889d4f Mon Sep 17 00:00:00 2001 From: Amir Fowler Date: Fri, 11 Sep 2026 17:17:54 -0700 Subject: [PATCH 2/2] refactor: rename getInstantiatedChart to getInstantiatedChartIdentity Now that Values are deliberately excluded from instantiation, the name getInstantiatedChart overstates what the function does: it instantiates the chart identity fields (repo, name, version, release name/namespace, options) and leaves Values for getHelmChartInstantiatedValues at the point of use. Rename it, as suggested in review, so the contract is clear at every call site. Co-Authored-By: Claude Fable 5.1 --- controllers/clustersummary_controller.go | 2 +- controllers/export_test.go | 2 +- controllers/handlers_helm.go | 16 ++++++++-------- controllers/handlers_helm_test.go | 12 ++++++------ controllers/init_container_work.go | 2 +- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/controllers/clustersummary_controller.go b/controllers/clustersummary_controller.go index bbc33e01..662de454 100644 --- a/controllers/clustersummary_controller.go +++ b/controllers/clustersummary_controller.go @@ -2249,7 +2249,7 @@ func getClusterSummaryWithInstantiatedCharts(ctx context.Context, cs *configv1be for i := range cs.Spec.ClusterProfileSpec.HelmCharts { helmChart := &cs.Spec.ClusterProfileSpec.HelmCharts[i] - instantiateHelmChart, err := getInstantiatedChart(ctx, innerDCtx, helmChart, logger) + instantiateHelmChart, err := getInstantiatedChartIdentity(ctx, innerDCtx, helmChart, logger) if err != nil { return nil, err } diff --git a/controllers/export_test.go b/controllers/export_test.go index 6adf2b97..2e828d3a 100644 --- a/controllers/export_test.go +++ b/controllers/export_test.go @@ -157,7 +157,7 @@ var ( UpdateValueHashOnHelmChartSummary = updateValueHashOnHelmChartSummary DesiredValuesAreSubset = desiredValuesAreSubset GetCredentialsAndCAFiles = getCredentialsAndCAFiles - GetInstantiatedChart = getInstantiatedChart + GetInstantiatedChartIdentity = getInstantiatedChartIdentity GetHelmChartValuesFrom = getHelmChartValuesFrom GetHelmChartInstantiatedValues = getHelmChartInstantiatedValues LocateChartWithTimeout = locateChartWithTimeout diff --git a/controllers/handlers_helm.go b/controllers/handlers_helm.go index 0be345fa..faa83731 100644 --- a/controllers/handlers_helm.go +++ b/controllers/handlers_helm.go @@ -514,7 +514,7 @@ func walkAndUndeployHelmChartsInPullMode(ctx context.Context, c client.Client, c for i := range clusterSummary.Spec.ClusterProfileSpec.HelmCharts { currentChart := &clusterSummary.Spec.ClusterProfileSpec.HelmCharts[i] - instantiatedChart, err := getInstantiatedChart(ctx, dCtx, currentChart, logger) + instantiatedChart, err := getInstantiatedChartIdentity(ctx, dCtx, currentChart, logger) if err != nil { return err } @@ -966,7 +966,7 @@ func uninstallHelmCharts(ctx context.Context, c client.Client, clusterSummary *c for i := range clusterSummary.Spec.ClusterProfileSpec.HelmCharts { currentChart := &clusterSummary.Spec.ClusterProfileSpec.HelmCharts[i] - instantiatedChart, err := getInstantiatedChart(ctx, dCtx, currentChart, logger) + instantiatedChart, err := getInstantiatedChartIdentity(ctx, dCtx, currentChart, logger) if err != nil { return nil, err } @@ -1271,7 +1271,7 @@ func deploySingleChart(ctx context.Context, c client.Client, dCtx *deploymentCon currentChart *configv1beta1.HelmChart, kubeconfig string, isPullMode bool, logger logr.Logger) (*chartStepResult, error) { - instantiatedChart, err := getInstantiatedChart(ctx, dCtx, currentChart, logger) + instantiatedChart, err := getInstantiatedChartIdentity(ctx, dCtx, currentChart, logger) if err != nil { return nil, err } @@ -3219,7 +3219,7 @@ func buildReferencedHelmReleaseSummaries(ctx context.Context, c client.Client, for i := range currentClusterSummary.Spec.ClusterProfileSpec.HelmCharts { currentChart := ¤tClusterSummary.Spec.ClusterProfileSpec.HelmCharts[i] - instantiatedChart, err := getInstantiatedChart(ctx, dCtx, currentChart, logger) + instantiatedChart, err := getInstantiatedChartIdentity(ctx, dCtx, currentChart, logger) if err != nil { return nil, false, err } @@ -3349,7 +3349,7 @@ func updateStatusForNonReferencedHelmReleases(ctx context.Context, c client.Clie for i := range dCtx.clusterSummary.Spec.ClusterProfileSpec.HelmCharts { currentChart := dCtx.clusterSummary.Spec.ClusterProfileSpec.HelmCharts[i] - instantiatedChart, err := getInstantiatedChart(ctx, dCtx, ¤tChart, logger) + instantiatedChart, err := getInstantiatedChartIdentity(ctx, dCtx, ¤tChart, logger) if err != nil { return dCtx.clusterSummary, err } @@ -3593,7 +3593,7 @@ func collectResourcesFromManagedHelmChartsForDriftDetection(ctx context.Context, for i := range clusterSummary.Spec.ClusterProfileSpec.HelmCharts { currentChart := &clusterSummary.Spec.ClusterProfileSpec.HelmCharts[i] - instantiatedChart, err := getInstantiatedChart(ctx, innerDCtx, currentChart, logger) + instantiatedChart, err := getInstantiatedChartIdentity(ctx, innerDCtx, currentChart, logger) if err != nil { return nil, err } @@ -5423,7 +5423,7 @@ func generateReportForSameVersion(ctx context.Context, currentValues map[string] return report, nil } -func getInstantiatedChart(ctx context.Context, dCtx *deploymentContext, +func getInstantiatedChartIdentity(ctx context.Context, dCtx *deploymentContext, currentChart *configv1beta1.HelmChart, logger logr.Logger) (*configv1beta1.HelmChart, error) { // Create a deep copy of the chart to avoid modifying the original. @@ -5939,7 +5939,7 @@ func getStaleReleases(ctx context.Context, c client.Client, clusterSummary *conf currentlyReferencedReleases := make(map[string]bool) for i := range clusterSummary.Spec.ClusterProfileSpec.HelmCharts { currentChart := &clusterSummary.Spec.ClusterProfileSpec.HelmCharts[i] - instantiatedChart, err := getInstantiatedChart(ctx, dCtx, currentChart, logger) + instantiatedChart, err := getInstantiatedChartIdentity(ctx, dCtx, currentChart, logger) if err != nil { return nil, err } diff --git a/controllers/handlers_helm_test.go b/controllers/handlers_helm_test.go index d2691d56..a7af03f6 100644 --- a/controllers/handlers_helm_test.go +++ b/controllers/handlers_helm_test.go @@ -1236,7 +1236,7 @@ var _ = Describe("HandlersHelm", func() { Expect(report.ReleaseNamespace).To(Equal(helmChart.ReleaseNamespace)) }) - It("getInstantiatedChart returns instantiated HelmChart matching passed in chart", func() { + It("getInstantiatedChartIdentity returns instantiated HelmChart matching passed in chart", func() { helmChart := &configv1beta1.HelmChart{ ReleaseName: randomString(), ReleaseNamespace: randomString(), ChartName: randomString(), ChartVersion: randomString(), @@ -1265,7 +1265,7 @@ var _ = Describe("HandlersHelm", func() { textlogger.NewLogger(textlogger.NewConfig())) Expect(err).To(BeNil()) - instaniatedChart, err := controllers.GetInstantiatedChart(context.TODO(), + instaniatedChart, err := controllers.GetInstantiatedChartIdentity(context.TODO(), controllers.NewDeploymentContext(clusterSummary, clusterObjects, nil), helmChart, textlogger.NewLogger(textlogger.NewConfig())) Expect(err).To(BeNil()) @@ -1278,7 +1278,7 @@ var _ = Describe("HandlersHelm", func() { Expect(instaniatedChart.ChartVersion).To(Equal(helmChart.ChartVersion)) }) - It("getInstantiatedChart returns instantiated HelmChart", func() { + It("getInstantiatedChartIdentity returns instantiated HelmChart", func() { helmChart := &configv1beta1.HelmChart{ ReleaseName: randomString(), ReleaseNamespace: randomString(), ChartName: randomString(), RepositoryURL: randomString(), @@ -1316,7 +1316,7 @@ var _ = Describe("HandlersHelm", func() { textlogger.NewLogger(textlogger.NewConfig())) Expect(err).To(BeNil()) - instaniatedChart, err := controllers.GetInstantiatedChart(context.TODO(), + instaniatedChart, err := controllers.GetInstantiatedChartIdentity(context.TODO(), controllers.NewDeploymentContext(clusterSummary, clusterObjects, nil), helmChart, textlogger.NewLogger(textlogger.NewConfig())) Expect(err).To(BeNil()) @@ -1329,7 +1329,7 @@ var _ = Describe("HandlersHelm", func() { Expect(instaniatedChart.ChartVersion).To(Equal("25.0.2")) }) - It("getInstantiatedChart leaves Values alone even when they are not a valid Sveltos template", func() { + It("getInstantiatedChartIdentity leaves Values alone even when they are not a valid Sveltos template", func() { helmChart := &configv1beta1.HelmChart{ ReleaseName: randomString(), ReleaseNamespace: randomString(), ChartName: randomString(), ChartVersion: randomString(), @@ -1366,7 +1366,7 @@ var _ = Describe("HandlersHelm", func() { textlogger.NewLogger(textlogger.NewConfig())) Expect(err).To(BeNil()) - instaniatedChart, err := controllers.GetInstantiatedChart(context.TODO(), + instaniatedChart, err := controllers.GetInstantiatedChartIdentity(context.TODO(), controllers.NewDeploymentContext(clusterSummary, clusterObjects, nil), helmChart, textlogger.NewLogger(textlogger.NewConfig())) Expect(err).To(BeNil()) diff --git a/controllers/init_container_work.go b/controllers/init_container_work.go index 9cc0553d..895d544f 100644 --- a/controllers/init_container_work.go +++ b/controllers/init_container_work.go @@ -141,7 +141,7 @@ func updateClusterSummaryHelmHashes(ctx context.Context, directClient client.Cli clusterObjects: clusterObjects, mgmtResources: mgmtResources, } - instantiatedChart, err := getInstantiatedChart(ctx, innerDCtx, helmChart, logger) + instantiatedChart, err := getInstantiatedChartIdentity(ctx, innerDCtx, helmChart, logger) if err != nil { logger.V(logs.LogInfo).Error(err, "failed to get instantiated chart") return err