From f4a9b72a6af6923d6bab0e3b0f789844427c91ad Mon Sep 17 00:00:00 2001 From: Gianluca Mardente Date: Thu, 2 Apr 2026 08:12:09 +0200 Subject: [PATCH] (bug) Helm deployment: react to patches changes Previously, when Helm charts were deployed with patches, the ClusterSummary would enter the Provisioning state, but Sveltos would fail to trigger a redeployment of the helm chart. This occurred because the reconciliation logic only compared the deployed Helm chart version and its values; since these remained unchanged, Sveltos assumed no action was required. Consequently, any modifications introduced via patches were effectively ignored. This PR fixes this behavior by introducing a hash for patches. Sveltos now calculates and stores a hash of the patches defined in the profile and compares it against the previously deployed state. By tracking this hash alongside chart versions and values, Sveltos can accurately detect when patches have changed and trigger the necessary Helm redeployment to bring the cluster into the desired state. --- api/v1beta1/clustersummary_types.go | 4 + api/v1beta1/zz_generated.deepcopy.go | 5 + ...ig.projectsveltos.io_clustersummaries.yaml | 5 + controllers/handlers_helm.go | 57 +++++- manifest/manifest.yaml | 5 + test/fv/helm_patches_test.go | 164 ++++++++++++++++++ 6 files changed, 236 insertions(+), 4 deletions(-) create mode 100644 test/fv/helm_patches_test.go diff --git a/api/v1beta1/clustersummary_types.go b/api/v1beta1/clustersummary_types.go index 42ab8749..e80162f9 100644 --- a/api/v1beta1/clustersummary_types.go +++ b/api/v1beta1/clustersummary_types.go @@ -110,6 +110,10 @@ type HelmChartSummary struct { // +optional ValuesHash []byte `json:"valuesHash,omitempty"` + // PatchesHash represents of a unique value for the patches section + // +optional + PatchesHash []byte `json:"patchesHash,omitempty"` + // Status indicates whether ClusterSummary can manage the helm // chart or there is a conflict // +optional diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index 3504036e..be924ef0 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -816,6 +816,11 @@ func (in *HelmChartSummary) DeepCopyInto(out *HelmChartSummary) { *out = make([]byte, len(*in)) copy(*out, *in) } + if in.PatchesHash != nil { + in, out := &in.PatchesHash, &out.PatchesHash + *out = make([]byte, len(*in)) + copy(*out, *in) + } if in.FailureMessage != nil { in, out := &in.FailureMessage, &out.FailureMessage *out = new(string) diff --git a/config/crd/bases/config.projectsveltos.io_clustersummaries.yaml b/config/crd/bases/config.projectsveltos.io_clustersummaries.yaml index e1d2db8b..8f3cd29f 100644 --- a/config/crd/bases/config.projectsveltos.io_clustersummaries.yaml +++ b/config/crd/bases/config.projectsveltos.io_clustersummaries.yaml @@ -1674,6 +1674,11 @@ spec: description: FailureMessage provides the specific error from the Helm engine for this release type: string + patchesHash: + description: PatchesHash represents of a unique value for the + patches section + format: byte + type: string releaseName: description: ReleaseName is the chart release minLength: 1 diff --git a/controllers/handlers_helm.go b/controllers/handlers_helm.go index bca37e7a..51efe17a 100644 --- a/controllers/handlers_helm.go +++ b/controllers/handlers_helm.go @@ -1105,9 +1105,9 @@ func getFailureMessageFromHelmChartSummary(requestedChart *configv1beta1.HelmCha return nil } -// getValueHashFromHelmChartSummary returns the valueHash stored for this chart +// getValuesHashFromHelmChartSummary returns the valueHash stored for this chart // in the ClusterSummary -func getValueHashFromHelmChartSummary(requestedChart *configv1beta1.HelmChart, +func getValuesHashFromHelmChartSummary(requestedChart *configv1beta1.HelmChart, clusterSummary *configv1beta1.ClusterSummary) []byte { for i := range clusterSummary.Status.HelmReleaseSummaries { @@ -1122,6 +1122,23 @@ func getValueHashFromHelmChartSummary(requestedChart *configv1beta1.HelmChart, return nil } +// getPatchesHashFromHelmChartSummary returns the patchesHash stored for this chart +// in the ClusterSummary +func getPatchesHashFromHelmChartSummary(requestedChart *configv1beta1.HelmChart, + clusterSummary *configv1beta1.ClusterSummary) []byte { + + for i := range clusterSummary.Status.HelmReleaseSummaries { + rs := &clusterSummary.Status.HelmReleaseSummaries[i] + if rs.ReleaseName == requestedChart.ReleaseName && + rs.ReleaseNamespace == requestedChart.ReleaseNamespace { + + return rs.PatchesHash + } + } + + return nil +} + func generateConflictForHelmChart(ctx context.Context, clusterSummary *configv1beta1.ClusterSummary, instantiatedChart *configv1beta1.HelmChart) string { @@ -2297,10 +2314,18 @@ func shouldUpgrade(ctx context.Context, currentRelease *releaseInfo, instantiate return false } + currentPatchesHash, err := getHelmChartPatchesHash(ctx, clusterSummary, logger) + if err != nil { + logger.V(logs.LogInfo).Info(fmt.Sprintf("failed to get current patches hash: %v", err)) + currentPatchesHash = []byte("") + } + if clusterSummary.Spec.ClusterProfileSpec.SyncMode != configv1beta1.SyncModeContinuousWithDriftDetection { if clusterSummary.Spec.ClusterProfileSpec.SyncMode != configv1beta1.SyncModeDryRun { - oldValueHash := getValueHashFromHelmChartSummary(instantiatedChart, clusterSummary) + oldValueHash := getValuesHashFromHelmChartSummary(instantiatedChart, clusterSummary) + oldPatchesHash := getPatchesHashFromHelmChartSummary(instantiatedChart, clusterSummary) + // Compare Values c := getManagementClusterClient() currentValueHash, err := getHelmChartValuesHash(ctx, c, instantiatedChart, clusterSummary, mgmtResources, logger) if err != nil { @@ -2310,6 +2335,11 @@ func shouldUpgrade(ctx context.Context, currentRelease *releaseInfo, instantiate if !reflect.DeepEqual(oldValueHash, currentValueHash) { return true } + + // Compare patches + if !reflect.DeepEqual(oldPatchesHash, currentPatchesHash) { + return true + } } if currentRelease != nil { @@ -2560,6 +2590,11 @@ func updateStatusForReferencedHelmReleases(ctx context.Context, c client.Client, conflict := false + patchesHash, err := getPatchesHash(ctx, clusterSummary, logger) + if err != nil { + return clusterSummary, false, err + } + currentClusterSummary := &configv1beta1.ClusterSummary{} err = retry.RetryOnConflict(retry.DefaultRetry, func() error { err = c.Get(ctx, @@ -2587,7 +2622,8 @@ func updateStatusForReferencedHelmReleases(ctx context.Context, c client.Client, ReleaseNamespace: instantiatedChart.ReleaseNamespace, Status: configv1beta1.HelmChartStatusManaging, FailureMessage: getFailureMessageFromHelmChartSummary(instantiatedChart, clusterSummary), - ValuesHash: getValueHashFromHelmChartSummary(instantiatedChart, clusterSummary), // if a value is currently stored, keep it. + PatchesHash: []byte(patchesHash), + ValuesHash: getValuesHashFromHelmChartSummary(instantiatedChart, clusterSummary), // if a value is currently stored, keep it. // after chart is deployed such value will be updated } currentlyReferenced[helmInfo(instantiatedChart.ReleaseNamespace, instantiatedChart.ReleaseName)] = true @@ -3868,6 +3904,19 @@ func updateValueHashOnHelmChartSummary(ctx context.Context, requestedChart *conf return helmChartValuesHash, err } +func getHelmChartPatchesHash(ctx context.Context, clusterSummary *configv1beta1.ClusterSummary, + logger logr.Logger) ([]byte, error) { + + patchesHash, err := getPatchesHash(ctx, clusterSummary, logger) + if err != nil { + return nil, err + } + + h := sha256.New() + h.Write([]byte(patchesHash)) + return h.Sum(nil), nil +} + func getCredentialsAndCAFiles(ctx context.Context, c client.Client, clusterSummary *configv1beta1.ClusterSummary, requestedChart *configv1beta1.HelmChart) (credentialsPath, caPath string, err error) { diff --git a/manifest/manifest.yaml b/manifest/manifest.yaml index 0db0b06e..19ed2f94 100644 --- a/manifest/manifest.yaml +++ b/manifest/manifest.yaml @@ -6054,6 +6054,11 @@ spec: description: FailureMessage provides the specific error from the Helm engine for this release type: string + patchesHash: + description: PatchesHash represents of a unique value for the + patches section + format: byte + type: string releaseName: description: ReleaseName is the chart release minLength: 1 diff --git a/test/fv/helm_patches_test.go b/test/fv/helm_patches_test.go new file mode 100644 index 00000000..9aa24356 --- /dev/null +++ b/test/fv/helm_patches_test.go @@ -0,0 +1,164 @@ +/* +Copyright 2026. projectsveltos.io. All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package fv_test + +import ( + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + appsv1 "k8s.io/api/apps/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/retry" + + configv1beta1 "github.com/projectsveltos/addon-controller/api/v1beta1" + "github.com/projectsveltos/addon-controller/lib/clusterops" + libsveltosv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" +) + +var _ = Describe("Helm with patches", func() { + const ( + namePrefix = "helm-patches-" + ) + + It("Deploy and updates helm charts with patches correctly", Label("FV", "FV-PULLMODE"), func() { + Byf("Create a ClusterProfile matching Cluster %s/%s", kindWorkloadCluster.GetNamespace(), kindWorkloadCluster.GetName()) + clusterProfile := getClusterProfile(namePrefix, map[string]string{key: value}) + clusterProfile.Spec.SyncMode = configv1beta1.SyncModeContinuous + Expect(k8sClient.Create(context.TODO(), clusterProfile)).To(Succeed()) + + verifyClusterProfileMatches(clusterProfile) + + verifyClusterSummary(clusterops.ClusterProfileLabelName, + clusterProfile.Name, &clusterProfile.Spec, kindWorkloadCluster.GetNamespace(), + kindWorkloadCluster.GetName(), getClusterType()) + + Byf("Update ClusterProfile %s to deploy helm charts", clusterProfile.Name) + currentClusterProfile := &configv1beta1.ClusterProfile{} + + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + Expect(k8sClient.Get(context.TODO(), + types.NamespacedName{Name: clusterProfile.Name}, currentClusterProfile)).To(Succeed()) + currentClusterProfile.Spec.HelmCharts = []configv1beta1.HelmChart{ + { + RepositoryURL: "https://argoproj.github.io/argo-helm", + RepositoryName: "argo", + ChartName: "argo/argo-cd", + ChartVersion: "3.35.4", + ReleaseName: "argocd", + ReleaseNamespace: "argocd", + HelmChartAction: configv1beta1.HelmChartActionInstall, + }, + } + + currentClusterProfile.Spec.Patches = []libsveltosv1beta1.Patch{ + { + Target: &libsveltosv1beta1.PatchSelector{ + Kind: "Deployment", + Group: "apps", + Version: "v1", + }, + Patch: `- op: add + path: /metadata/annotations/test + value: ok`, + }, + } + return k8sClient.Update(context.TODO(), currentClusterProfile) + }) + Expect(err).To(BeNil()) + + Expect(k8sClient.Get(context.TODO(), + types.NamespacedName{Name: clusterProfile.Name}, currentClusterProfile)).To(Succeed()) + + clusterSummary := verifyClusterSummary(clusterops.ClusterProfileLabelName, + currentClusterProfile.Name, ¤tClusterProfile.Spec, + kindWorkloadCluster.GetNamespace(), kindWorkloadCluster.GetName(), getClusterType()) + + Byf("Getting client to access the workload cluster") + workloadClient, err := getKindWorkloadClusterKubeconfig() + Expect(err).To(BeNil()) + Expect(workloadClient).ToNot(BeNil()) + + Byf("Verifying argocd deployment is created in the workload cluster") + Eventually(func() bool { + depl := &appsv1.Deployment{} + err = workloadClient.Get(context.TODO(), + types.NamespacedName{Namespace: "argocd", Name: "argocd-server"}, depl) + if err != nil { + return false + } + if len(depl.Annotations) == 0 { + return false + } + return depl.Annotations["test"] == "ok" + }, timeout, pollingInterval).Should(BeTrue()) + + charts := []configv1beta1.Chart{ + {ReleaseName: "argocd", ChartVersion: "3.35.4", Namespace: "argocd"}, + } + + verifyClusterConfiguration(configv1beta1.ClusterProfileKind, clusterProfile.Name, + clusterSummary.Spec.ClusterNamespace, clusterSummary.Spec.ClusterName, libsveltosv1beta1.FeatureHelm, + nil, charts) + + Byf("Update ClusterProfile %s patches", clusterProfile.Name) + + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + Expect(k8sClient.Get(context.TODO(), + types.NamespacedName{Name: clusterProfile.Name}, currentClusterProfile)).To(Succeed()) + currentClusterProfile.Spec.Patches = []libsveltosv1beta1.Patch{ + { + Target: &libsveltosv1beta1.PatchSelector{ + Kind: "Deployment", + Group: "apps", + Version: "v1", + }, + Patch: `- op: add + path: /metadata/annotations/test2 + value: ok2`, + }, + } + + return k8sClient.Update(context.TODO(), currentClusterProfile) + }) + Expect(err).To(BeNil()) + + Expect(k8sClient.Get(context.TODO(), + types.NamespacedName{Name: clusterProfile.Name}, currentClusterProfile)).To(Succeed()) + + verifyClusterSummary(clusterops.ClusterProfileLabelName, + currentClusterProfile.Name, ¤tClusterProfile.Spec, + kindWorkloadCluster.GetNamespace(), kindWorkloadCluster.GetName(), getClusterType()) + + Byf("Verifying argocd deployment is updated in the workload cluster") + Eventually(func() bool { + depl := &appsv1.Deployment{} + err = workloadClient.Get(context.TODO(), + types.NamespacedName{Namespace: "argocd", Name: "argocd-server"}, depl) + if err != nil { + return false + } + if len(depl.Annotations) == 0 { + return false + } + return depl.Annotations["test2"] == "ok2" + }, timeout, pollingInterval).Should(BeTrue()) + + deleteClusterProfile(clusterProfile) + }) +})