From 5877e05677f18c977ee84d56245f11bb6d975e0e Mon Sep 17 00:00:00 2001 From: Gianluca Mardente Date: Thu, 13 Aug 2026 18:45:43 +0200 Subject: [PATCH] feat: Scope Helm redeploy on drift to only the chart that drifted When a ClusterProfile deploys multiple Helm charts with syncMode ContinuousWithDriftDetection, drift on a single resource used to mark the entire Helm feature for redeploy, and unconditionally upgraded every chart in the profile regardless of whether it had drifted or even changed. HelmChartSummary gains NeedsRedeploy, set per chart when drift is detected, and shouldUpgrade now checks it before falling through to the existing value/patches/version diff, instead of always returning true for drift-detection mode. A chart neither drifted nor changed is left alone. Falls back to marking every chart when no chart-scoped info is available so drift is never silently missed, just not always scoped. --- api/v1beta1/clustersummary_types.go | 7 + ...ig.projectsveltos.io_clustersummaries.yaml | 7 + controllers/export_test.go | 11 + controllers/handlers_helm.go | 122 +++--- controllers/handlers_helm_test.go | 360 +++++++++++++++++- controllers/init_container_work.go | 19 + controllers/init_container_work_test.go | 155 ++++++++ controllers/resourcesummary_collection.go | 31 ++ .../resourcesummary_collection_test.go | 157 ++++++-- controllers/test_constants_test.go | 6 + go.mod | 2 +- go.sum | 4 +- lib/crd/clustersummaries.go | 7 + manifest/manifest.yaml | 7 + ...drift-detection-manager-in-mgmt-cluster.go | 2 +- ...ift-detection-manager-in-mgmt-cluster.yaml | 2 +- .../drift-detection-manager.go | 2 +- .../drift-detection-manager.yaml | 2 +- 18 files changed, 799 insertions(+), 104 deletions(-) create mode 100644 controllers/init_container_work_test.go diff --git a/api/v1beta1/clustersummary_types.go b/api/v1beta1/clustersummary_types.go index d318e6d2..fdd472f4 100644 --- a/api/v1beta1/clustersummary_types.go +++ b/api/v1beta1/clustersummary_types.go @@ -166,6 +166,13 @@ type HelmChartSummary struct { // LastCheckedTime is when LatestVersion/LatestPatchVersion were last evaluated. // +optional LastCheckedTime *metav1.Time `json:"lastCheckedTime,omitempty"` + + // NeedsRedeploy is set to true when drift-detection reports that a resource deployed by + // this chart changed out of band. Only meaningful when SyncMode is ContinuousWithDriftDetection. + // When true, this chart is upgraded on the next reconciliation regardless of whether its + // desired values/version changed; cleared once the upgrade succeeds. + // +optional + NeedsRedeploy bool `json:"needsRedeploy,omitempty"` } // ClusterSummarySpec defines the desired state of ClusterSummary diff --git a/config/crd/bases/config.projectsveltos.io_clustersummaries.yaml b/config/crd/bases/config.projectsveltos.io_clustersummaries.yaml index db072f5c..bb96095a 100644 --- a/config/crd/bases/config.projectsveltos.io_clustersummaries.yaml +++ b/config/crd/bases/config.projectsveltos.io_clustersummaries.yaml @@ -2532,6 +2532,13 @@ spec: greater than ChartVersion. Populated by a periodic background check, independent of the reconcile loop. Detection only: Sveltos never mutates ChartVersion based on this. type: string + needsRedeploy: + description: |- + NeedsRedeploy is set to true when drift-detection reports that a resource deployed by + this chart changed out of band. Only meaningful when SyncMode is ContinuousWithDriftDetection. + When true, this chart is upgraded on the next reconciliation regardless of whether its + desired values/version changed; cleared once the upgrade succeeds. + type: boolean patchesHash: description: PatchesHash represents of a unique value for the patches section diff --git a/controllers/export_test.go b/controllers/export_test.go index 2c0f9d3b..26b82f4c 100644 --- a/controllers/export_test.go +++ b/controllers/export_test.go @@ -136,6 +136,7 @@ var ( ShouldInstall = shouldInstall ShouldUninstall = shouldUninstall ShouldUpgrade = shouldUpgrade + NeedsRedeployForDrift = needsRedeployForDrift UpdateChartsInClusterConfiguration = updateChartsInClusterConfiguration UpdateStatusForReferencedHelmReleases = updateStatusForReferencedHelmReleases UpdateStatusForNonReferencedHelmReleases = updateStatusForNonReferencedHelmReleases @@ -143,6 +144,9 @@ var ( UpdateClusterReportWithHelmReports = updateClusterReportWithHelmReports HandleCharts = handleCharts GetHelmChartValuesHash = getHelmChartValuesHash + GetHelmChartPatchesHash = getHelmChartPatchesHash + GetPatchesHash = getPatchesHash + UpdateValueHashOnHelmChartSummary = updateValueHashOnHelmChartSummary DesiredValuesAreSubset = desiredValuesAreSubset GetCredentialsAndCAFiles = getCredentialsAndCAFiles GetInstantiatedChart = getInstantiatedChart @@ -211,6 +215,8 @@ var ( resourceSummaryInstalledCache = make(map[corev1.ObjectReference]bool) resourceSummaryInstalledCacheMu.Unlock() } + ProcessResourceSummary = processResourceSummary + MarkDriftedHelmCharts = markDriftedHelmCharts ) var ( @@ -261,6 +267,11 @@ type ( RegistryClientOptions = registryClientOptions ) +var ( + UpdateClusterSummaryHelmHashes = updateClusterSummaryHelmHashes + SetHelmChartsPatchesHash = setHelmChartsPatchesHash +) + // NewDeploymentContext constructs a deploymentContext for use in tests. func NewDeploymentContext( clusterSummary *configv1beta1.ClusterSummary, diff --git a/controllers/handlers_helm.go b/controllers/handlers_helm.go index 4384ae29..f4d4eb9b 100644 --- a/controllers/handlers_helm.go +++ b/controllers/handlers_helm.go @@ -1455,6 +1455,27 @@ func getValuesHashFromHelmChartSummary(requestedChart *configv1beta1.HelmChart, return nil } +// getNeedsRedeployFromHelmChartSummary returns the NeedsRedeploy flag stored for this chart in +// clusterSummary. buildReferencedHelmReleaseSummaries rebuilds Status.HelmReleaseSummaries from +// scratch on every reconcile, before shouldUpgrade ever runs, so NeedsRedeploy (set by +// markDriftedHelmCharts when drift-detection reports this chart changed out of band) must be +// carried forward explicitly here, the same way ValuesHash already is, or it gets silently reset +// to false before shouldUpgrade gets a chance to read it. +func getNeedsRedeployFromHelmChartSummary(requestedChart *configv1beta1.HelmChart, + clusterSummary *configv1beta1.ClusterSummary) bool { + + for i := range clusterSummary.Status.HelmReleaseSummaries { + rs := &clusterSummary.Status.HelmReleaseSummaries[i] + if rs.ReleaseName == requestedChart.ReleaseName && + rs.ReleaseNamespace == requestedChart.ReleaseNamespace { + + return rs.NeedsRedeploy + } + } + + return false +} + // getPatchesHashFromHelmChartSummary returns the patchesHash stored for this chart // in the ClusterSummary func getPatchesHashFromHelmChartSummary(requestedChart *configv1beta1.HelmChart, @@ -2842,14 +2863,30 @@ func shouldUpgrade(ctx context.Context, currentRelease *releaseInfo, instantiate return shouldUpgradeForContinuousMode(ctx, currentRelease, instantiatedChart, dCtx, currentPatchesHash, logger) } - // ContinuousWithDriftDetection: always upgrade except on the very first reconciliation - // when the live release already matches desired state (no stored ValuesHash yet). - // Subsequent reconciliations always upgrade so the drift-detection agent can repair drift. + // ContinuousWithDriftDetection: upgrade if drift-detection reported this specific chart's + // deployed resources changed out of band (NeedsRedeploy, set by markDriftedHelmCharts in + // resourcesummary_collection.go), or if the desired chart spec itself changed, same check + // Continuous mode uses. A chart that neither drifted nor changed is left alone, instead of + // being upgraded on every single reconcile regardless of whether anything needs it. // ResourceSummary is deployed by postProcessDeployedHelmCharts regardless of this decision. - if driftDetectionFirstReconciliationCanSkip(ctx, currentRelease, instantiatedChart, dCtx, currentPatchesHash, logger) { - return false + if needsRedeployForDrift(instantiatedChart, dCtx.clusterSummary) { + return true } - return true + return shouldUpgradeForContinuousMode(ctx, currentRelease, instantiatedChart, dCtx, currentPatchesHash, logger) +} + +// needsRedeployForDrift returns true if drift-detection has flagged this chart's deployed +// resources as changed out of band since the last successful deploy of this chart. +func needsRedeployForDrift(instantiatedChart *configv1beta1.HelmChart, clusterSummary *configv1beta1.ClusterSummary) bool { + for i := range clusterSummary.Status.HelmReleaseSummaries { + summary := &clusterSummary.Status.HelmReleaseSummaries[i] + if summary.ReleaseName == instantiatedChart.ReleaseName && + summary.ReleaseNamespace == instantiatedChart.ReleaseNamespace { + + return summary.NeedsRedeploy + } + } + return false } // shouldUpgradeForContinuousMode handles the upgrade decision for Continuous and DryRun modes. @@ -2885,8 +2922,9 @@ func shouldUpgradeForContinuousMode(ctx context.Context, currentRelease *release } } if oldPatchesHash == nil { - emptyHash := sha256.Sum256([]byte("")) - if reflect.DeepEqual(currentPatchesHash, emptyHash[:]) { + // getHelmChartPatchesHash returns an empty []byte when no patches are configured + // (getPatchesHash short-circuits to "" before hashing anything). + if len(currentPatchesHash) == 0 { oldPatchesHash = currentPatchesHash } } @@ -2916,35 +2954,6 @@ func shouldUpgradeForContinuousMode(ctx context.Context, currentRelease *release return false } -// driftDetectionFirstReconciliationCanSkip returns true when the live release already -// reflects the desired state and this is the first reconciliation (no stored ValuesHash). -// Conditions: no stored state, release deployed, same version, desired values are a subset -// of the coalesced release values, and no patches configured. -// Patches are not skipped: we cannot infer from the release whether they were applied before. -func driftDetectionFirstReconciliationCanSkip(ctx context.Context, currentRelease *releaseInfo, - instantiatedChart *configv1beta1.HelmChart, dCtx *deploymentContext, - currentPatchesHash []byte, logger logr.Logger) bool { - - if getValuesHashFromHelmChartSummary(instantiatedChart, dCtx.clusterSummary) != nil { - return false - } - if currentRelease == nil || currentRelease.Status != releasecommon.StatusDeployed.String() { - return false - } - emptyPatchesHash := sha256.Sum256([]byte("")) - if !reflect.DeepEqual(currentPatchesHash, emptyPatchesHash[:]) { - return false - } - current, err1 := semver.NewVersion(currentRelease.ChartVersion) - expected, err2 := semver.NewVersion(instantiatedChart.ChartVersion) - if err1 != nil || err2 != nil || !current.Equal(expected) { - return false - } - desiredValues, err := getHelmChartInstantiatedValues(ctx, dCtx.clusterSummary, - dCtx.mgmtResources, instantiatedChart, logger) - return err == nil && desiredValuesAreSubset(desiredValues, currentRelease.FullValues) -} - // shouldUninstall returns true if action is uninstall there is a release installed currently func shouldUninstall(currentRelease *releaseInfo, requestedChart *configv1beta1.HelmChart) bool { if currentRelease == nil { @@ -3152,7 +3161,7 @@ type getManagerForChart func(clusterNamespace, clusterName string, func buildReferencedHelmReleaseSummaries(ctx context.Context, c client.Client, getManager getManagerForChart, clusterSummary *configv1beta1.ClusterSummary, currentClusterSummary *configv1beta1.ClusterSummary, dCtx *deploymentContext, - patchesHash string, logger logr.Logger, + logger logr.Logger, ) (summaries []configv1beta1.HelmChartSummary, conflict bool, err error) { helmInfo := func(releaseNamespace, releaseName string) string { @@ -3180,8 +3189,16 @@ func buildReferencedHelmReleaseSummaries(ctx context.Context, c client.Client, ReleaseNamespace: instantiatedChart.ReleaseNamespace, Status: configv1beta1.HelmChartStatusManaging, FailureMessage: getFailureMessageFromHelmChartSummary(instantiatedChart, clusterSummary), - PatchesHash: []byte(patchesHash), - ValuesHash: getValuesHashFromHelmChartSummary(instantiatedChart, clusterSummary), + // Carried forward, not recomputed: shouldUpgradeForContinuousMode compares this + // against a freshly computed value to detect a real patches change. Overwriting + // it with the fresh value here, before that comparison ever runs, would make the + // comparison structurally unable to see a difference (old and current would + // always match, since both come from the same current state). PatchesHash is + // only advanced to the current value on a successful deploy, in + // updateValueHashOnHelmChartSummary, the same point ValuesHash is. + PatchesHash: getPatchesHashFromHelmChartSummary(instantiatedChart, clusterSummary), + ValuesHash: getValuesHashFromHelmChartSummary(instantiatedChart, clusterSummary), + NeedsRedeploy: getNeedsRedeployFromHelmChartSummary(instantiatedChart, clusterSummary), } currentlyReferenced[helmInfo(instantiatedChart.ReleaseNamespace, instantiatedChart.ReleaseName)] = true } else { @@ -3235,11 +3252,6 @@ 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, @@ -3262,7 +3274,7 @@ func updateStatusForReferencedHelmReleases(ctx context.Context, c client.Client, var summaries []configv1beta1.HelmChartSummary summaries, conflict, err = buildReferencedHelmReleaseSummaries(ctx, c, chartManager.GetManagerForChart, - clusterSummary, currentClusterSummary, innerDCtx, patchesHash, logger) + clusterSummary, currentClusterSummary, innerDCtx, logger) if err != nil { return err } @@ -4669,6 +4681,14 @@ func updateValueHashOnHelmChartSummary(ctx context.Context, requestedChart *conf return nil, err } + // Advanced here, on a successful deploy, same as ValuesHash. buildReferencedHelmReleaseSummaries + // only ever carries the stored PatchesHash forward, it never recomputes it, so this is the only + // place PatchesHash catches up to the patches that were actually just applied. + helmChartPatchesHash, err := getHelmChartPatchesHash(ctx, dCtx.clusterSummary, logger) + if err != nil { + return nil, err + } + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { currentClusterSummary := &configv1beta1.ClusterSummary{} err = c.Get(ctx, @@ -4683,6 +4703,8 @@ func updateValueHashOnHelmChartSummary(ctx context.Context, requestedChart *conf rs.ReleaseNamespace == requestedChart.ReleaseNamespace { rs.ValuesHash = helmChartValuesHash + rs.PatchesHash = helmChartPatchesHash + rs.NeedsRedeploy = false setResolvedHelmChartIdentity(ctx, c, dCtx.clusterSummary, rs, requestedChart, currentRelease, logger) } } @@ -4774,6 +4796,10 @@ func clearStaleOutdatedVersionInfo(clusterSummary *configv1beta1.ClusterSummary, rs.LastCheckedTime = nil } +// getHelmChartPatchesHash returns the same hex-string-as-bytes encoding buildReferencedHelmReleaseSummaries +// stores in HelmChartSummary.PatchesHash (getPatchesHash already returns a hex-encoded sha256 digest), so +// the two are directly comparable. Do not hash patchesHash again here, that would compare a hex(sha256(...)) +// value against a sha256(hex(sha256(...))) value, which can never be equal regardless of whether patches changed. func getHelmChartPatchesHash(ctx context.Context, clusterSummary *configv1beta1.ClusterSummary, logger logr.Logger) ([]byte, error) { @@ -4782,9 +4808,7 @@ func getHelmChartPatchesHash(ctx context.Context, clusterSummary *configv1beta1. return nil, err } - h := sha256.New() - h.Write([]byte(patchesHash)) - return h.Sum(nil), nil + return []byte(patchesHash), nil } func getCredentialsAndCAFiles(ctx context.Context, c client.Client, clusterSummary *configv1beta1.ClusterSummary, diff --git a/controllers/handlers_helm_test.go b/controllers/handlers_helm_test.go index 8db5f942..2b926f42 100644 --- a/controllers/handlers_helm_test.go +++ b/controllers/handlers_helm_test.go @@ -17,6 +17,7 @@ limitations under the License. package controllers_test import ( + "bytes" "context" "crypto/sha256" "encoding/json" @@ -397,9 +398,10 @@ var _ = Describe("HandlersHelm", func() { textlogger.NewLogger(textlogger.NewConfig()))).To(BeFalse()) }) - It("shouldUpgrade returns true on second reconciliation with ContinuousWithDriftDetection (stored ValuesHash present)", func() { - // Once a ValuesHash is stored in HelmReleaseSummaries, every reconciliation must - // trigger an upgrade so the drift-detection agent can repair configuration drift. + // shouldUpgradeWithDriftDetection sets up a chart whose stored ValuesHash already matches + // what's actually deployed (nothing about the desired spec changed), with NeedsRedeploy set + // as requested, and returns what shouldUpgrade decides for it under ContinuousWithDriftDetection. + shouldUpgradeWithDriftDetection := func(needsRedeploy bool) bool { ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: clusterSummary.Spec.ClusterNamespace}} Expect(testEnv.Create(context.TODO(), ns)).To(Succeed()) Expect(waitForObject(context.TODO(), testEnv.Client, ns)).To(Succeed()) @@ -414,12 +416,25 @@ var _ = Describe("HandlersHelm", func() { Expect(waitForObject(context.TODO(), testEnv.Client, cluster)).To(Succeed()) clusterSummary.Spec.ClusterProfileSpec.SyncMode = configv1beta1.SyncModeContinuousWithDriftDetection + + requestChart := &configv1beta1.HelmChart{ + ReleaseName: testReleaseNameNginxLatest, + ReleaseNamespace: testNginxRepo, + ChartVersion: testChartVersion100, + HelmChartAction: configv1beta1.HelmChartActionInstall, + } + + storedValuesHash, err := controllers.GetHelmChartValuesHash(context.TODO(), testEnv, requestChart, + clusterSummary, nil, textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + clusterSummary.Status.HelmReleaseSummaries = []configv1beta1.HelmChartSummary{ { ReleaseName: testReleaseNameNginxLatest, ReleaseNamespace: testNginxRepo, - ValuesHash: []byte("previously-stored-hash"), + ValuesHash: storedValuesHash, Status: configv1beta1.HelmChartStatusManaging, + NeedsRedeploy: needsRedeploy, }, } @@ -428,6 +443,48 @@ var _ = Describe("HandlersHelm", func() { ChartVersion: testChartVersion100, FullValues: map[string]interface{}{testReplicaCountKey: 1}, } + + return controllers.ShouldUpgrade(context.TODO(), currentRelease, requestChart, + controllers.NewDeploymentContext(clusterSummary, nil, nil), + textlogger.NewLogger(textlogger.NewConfig())) + } + + It("shouldUpgrade returns false with ContinuousWithDriftDetection when stored state matches and NeedsRedeploy is false", func() { + // Nothing changed and drift-detection hasn't flagged this chart, so there's nothing to + // do: shouldUpgrade must not blindly re-upgrade every chart on every reconcile + // regardless of state. + Expect(shouldUpgradeWithDriftDetection(false)).To(BeFalse()) + }) + + It("shouldUpgrade returns true with ContinuousWithDriftDetection when NeedsRedeploy is set, even if stored state matches", func() { + // Same as above, nothing about the desired spec changed, but drift-detection flagged + // this chart's deployed resources as changed out of band. That alone must trigger the + // upgrade, scoped to this chart. + Expect(shouldUpgradeWithDriftDetection(true)).To(BeTrue()) + }) + + It("shouldUpgrade returns true when patches are added to a chart that previously had none", func() { + // Regression test: buildReferencedHelmReleaseSummaries used to overwrite PatchesHash + // with the freshly computed value on every reconcile, before shouldUpgrade ever compared + // it, so a real patches change was undetectable, old and current were always equal by + // construction, since both came from the same current state. PatchesHash now behaves + // like ValuesHash: carried forward until a successful deploy, so a genuine change here + // (patches added where there were none) must be visible to the comparison. + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: clusterSummary.Spec.ClusterNamespace}} + Expect(testEnv.Create(context.TODO(), ns)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, ns)).To(Succeed()) + + 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()) + + clusterSummary.Spec.ClusterProfileSpec.SyncMode = configv1beta1.SyncModeContinuous + requestChart := &configv1beta1.HelmChart{ ReleaseName: testReleaseNameNginxLatest, ReleaseNamespace: testNginxRepo, @@ -435,6 +492,41 @@ var _ = Describe("HandlersHelm", func() { HelmChartAction: configv1beta1.HelmChartActionInstall, } + // Deployed previously with no patches configured. + noPatchesHash, err := controllers.GetHelmChartPatchesHash(context.TODO(), clusterSummary, + textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + + storedValuesHash, err := controllers.GetHelmChartValuesHash(context.TODO(), testEnv, requestChart, + clusterSummary, nil, textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + + clusterSummary.Status.HelmReleaseSummaries = []configv1beta1.HelmChartSummary{ + { + ReleaseName: testReleaseNameNginxLatest, + ReleaseNamespace: testNginxRepo, + ValuesHash: storedValuesHash, + PatchesHash: noPatchesHash, + Status: configv1beta1.HelmChartStatusManaging, + }, + } + + // Patches just got added to the ClusterProfile. + clusterSummary.Spec.ClusterProfileSpec.Patches = []libsveltosv1beta1.Patch{ + { + Patch: testManagedAnnotationPatch, + Target: &libsveltosv1beta1.PatchSelector{ + Kind: testKindDeployment, + }, + }, + } + + currentRelease := &controllers.ReleaseInfo{ + Status: releasecommon.StatusDeployed.String(), + ChartVersion: testChartVersion100, + FullValues: map[string]interface{}{testReplicaCountKey: 1}, + } + Expect(controllers.ShouldUpgrade(context.TODO(), currentRelease, requestChart, controllers.NewDeploymentContext(clusterSummary, nil, nil), textlogger.NewLogger(textlogger.NewConfig()))).To(BeTrue()) @@ -460,9 +552,7 @@ var _ = Describe("HandlersHelm", func() { clusterSummary.Spec.ClusterProfileSpec.SyncMode = configv1beta1.SyncModeContinuousWithDriftDetection clusterSummary.Spec.ClusterProfileSpec.Patches = []libsveltosv1beta1.Patch{ { - Patch: `- op: add - path: /metadata/annotations/projectsveltos.io~1managed - value: "true"`, + Patch: testManagedAnnotationPatch, Target: &libsveltosv1beta1.PatchSelector{ Kind: testKindDeployment, }, @@ -486,6 +576,110 @@ var _ = Describe("HandlersHelm", func() { textlogger.NewLogger(textlogger.NewConfig()))).To(BeTrue()) }) + It("getHelmChartPatchesHash returns the same encoding stored by buildReferencedHelmReleaseSummaries", func() { + // buildReferencedHelmReleaseSummaries stores getPatchesHash's return value directly as + // []byte(patchesHash), a hex-encoded digest kept as text. getHelmChartPatchesHash must + // return that same encoding, not hash it again, otherwise the two are never equal and + // shouldUpgradeForContinuousMode reports "patches changed" on every single reconcile + // regardless of whether patches actually changed. + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: clusterSummary.Spec.ClusterNamespace}} + Expect(testEnv.Create(context.TODO(), ns)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, ns)).To(Succeed()) + + 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()) + + clusterSummary.Spec.ClusterProfileSpec.Patches = []libsveltosv1beta1.Patch{ + { + Patch: testManagedAnnotationPatch, + Target: &libsveltosv1beta1.PatchSelector{ + Kind: testKindDeployment, + }, + }, + } + + storedFormat, err := controllers.GetPatchesHash(context.TODO(), clusterSummary, + textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + Expect(storedFormat).ToNot(BeEmpty()) + + comparisonFormat, err := controllers.GetHelmChartPatchesHash(context.TODO(), clusterSummary, + textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + + Expect(comparisonFormat).To(Equal([]byte(storedFormat))) + }) + + It("shouldUpgrade returns false when patches are configured but unchanged", func() { + // Regression test: previously getHelmChartPatchesHash hashed the already-hex-encoded + // stored value a second time, so this always evaluated to true, forcing a redeploy of + // every chart with patches configured on every reconcile, in any sync mode, regardless + // of whether anything actually changed. + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: clusterSummary.Spec.ClusterNamespace}} + Expect(testEnv.Create(context.TODO(), ns)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, ns)).To(Succeed()) + + 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()) + + clusterSummary.Spec.ClusterProfileSpec.SyncMode = configv1beta1.SyncModeContinuous + clusterSummary.Spec.ClusterProfileSpec.Patches = []libsveltosv1beta1.Patch{ + { + Patch: testManagedAnnotationPatch, + Target: &libsveltosv1beta1.PatchSelector{ + Kind: testKindDeployment, + }, + }, + } + + requestChart := &configv1beta1.HelmChart{ + ReleaseName: testReleaseNameNginxLatest, + ReleaseNamespace: testNginxRepo, + ChartVersion: testChartVersion100, + HelmChartAction: configv1beta1.HelmChartActionInstall, + } + + storedValuesHash, err := controllers.GetHelmChartValuesHash(context.TODO(), testEnv, requestChart, + clusterSummary, nil, textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + + storedPatchesHash, err := controllers.GetHelmChartPatchesHash(context.TODO(), clusterSummary, + textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + + clusterSummary.Status.HelmReleaseSummaries = []configv1beta1.HelmChartSummary{ + { + ReleaseName: testReleaseNameNginxLatest, + ReleaseNamespace: testNginxRepo, + ValuesHash: storedValuesHash, + PatchesHash: storedPatchesHash, + Status: configv1beta1.HelmChartStatusManaging, + }, + } + + currentRelease := &controllers.ReleaseInfo{ + Status: releasecommon.StatusDeployed.String(), + ChartVersion: testChartVersion100, + FullValues: map[string]interface{}{testReplicaCountKey: 1}, + } + + Expect(controllers.ShouldUpgrade(context.TODO(), currentRelease, requestChart, + controllers.NewDeploymentContext(clusterSummary, nil, nil), + textlogger.NewLogger(textlogger.NewConfig()))).To(BeFalse()) + }) + It("UpdateStatusForeferencedHelmReleases updates ClusterSummary.Status.HelmReleaseSummaries", func() { calicoChart := &configv1beta1.HelmChart{ RepositoryURL: "https://projectcalico.docs.tigera.io/charts", @@ -570,6 +764,152 @@ var _ = Describe("HandlersHelm", func() { }) + It("UpdateStatusForReferencedHelmReleases preserves NeedsRedeploy across a rebuild", func() { + // Regression test: UpdateStatusForReferencedHelmReleases (via buildReferencedHelmReleaseSummaries) + // rebuilds Status.HelmReleaseSummaries from scratch on every reconcile, before shouldUpgrade + // ever runs. It already carries ValuesHash forward explicitly; it must do the same for + // NeedsRedeploy, set by markDriftedHelmCharts when drift-detection reports this chart + // changed out of band, otherwise the flag is silently wiped before shouldUpgrade can act on + // it, and a chart with a real, detected drift is never actually redeployed. + nginxChart := &configv1beta1.HelmChart{ + RepositoryURL: testRepoURLNginxStable, + RepositoryName: testRepoNameNginxStable, + ChartName: testChartNameNginxIngress, + ChartVersion: testChartVersion100, + ReleaseName: testReleaseNameNginxLatest, + ReleaseNamespace: testNginxRepo, + HelmChartAction: configv1beta1.HelmChartActionInstall, + } + + clusterSummary.Spec.ClusterProfileSpec = configv1beta1.Spec{ + HelmCharts: []configv1beta1.HelmChart{*nginxChart}, + } + clusterSummary.Namespace = defaultNamespace + clusterSummary.Spec.ClusterNamespace = defaultNamespace + + Expect(testEnv.Create(context.TODO(), clusterSummary)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, clusterSummary)).To(Succeed()) + + clusterSummary.Status = configv1beta1.ClusterSummaryStatus{ + HelmReleaseSummaries: []configv1beta1.HelmChartSummary{ + { + ReleaseName: nginxChart.ReleaseName, + ReleaseNamespace: nginxChart.ReleaseNamespace, + Status: configv1beta1.HelmChartStatusManaging, + NeedsRedeploy: true, + }, + }, + } + Expect(testEnv.Status().Update(context.TODO(), clusterSummary)).To(Succeed()) + + 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()) + + manager, err := chartmanager.GetChartManagerInstance(context.TODO(), testEnv.Client) + Expect(err).To(BeNil()) + manager.RegisterClusterSummaryForCharts(clusterSummary) + + _, conflict, err := controllers.UpdateStatusForReferencedHelmReleases(context.TODO(), + testEnv.Client, clusterSummary, nil, textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + Expect(conflict).To(BeFalse()) + + Eventually(func() bool { + currentClusterSummary := &configv1beta1.ClusterSummary{} + err = testEnv.Get(context.TODO(), + types.NamespacedName{Namespace: clusterSummary.Namespace, Name: clusterSummary.Name}, + currentClusterSummary) + if err != nil || len(currentClusterSummary.Status.HelmReleaseSummaries) != 1 { + return false + } + return currentClusterSummary.Status.HelmReleaseSummaries[0].NeedsRedeploy + }, timeout, pollingInterval).Should(BeTrue()) + }) + + It("UpdateStatusForReferencedHelmReleases carries PatchesHash forward instead of recomputing it", func() { + // Regression test: buildReferencedHelmReleaseSummaries used to store the freshly computed + // PatchesHash directly, on every reconcile, before shouldUpgrade ever ran. That made a + // real patches change undetectable: by the time shouldUpgrade compared old vs current, + // both had already been set to the same fresh value moments earlier in the same + // reconcile. PatchesHash must instead be carried forward here, the same way ValuesHash + // already is, and only advanced to the current value on a successful deploy + // (updateValueHashOnHelmChartSummary), so shouldUpgrade's comparison has something real + // to compare against. + nginxChart := &configv1beta1.HelmChart{ + RepositoryURL: testRepoURLNginxStable, + RepositoryName: testRepoNameNginxStable, + ChartName: testChartNameNginxIngress, + ChartVersion: testChartVersion100, + ReleaseName: testReleaseNameNginxLatest, + ReleaseNamespace: testNginxRepo, + HelmChartAction: configv1beta1.HelmChartActionInstall, + } + + clusterSummary.Spec.ClusterProfileSpec = configv1beta1.Spec{ + HelmCharts: []configv1beta1.HelmChart{*nginxChart}, + // Patches configured now. If PatchesHash were recomputed instead of carried + // forward, this reconcile would store the hash of *this* patch list, matching + // whatever shouldUpgrade computes as "current" right after, and the change below + // would never be visible. + Patches: []libsveltosv1beta1.Patch{ + { + Patch: testManagedAnnotationPatch, + Target: &libsveltosv1beta1.PatchSelector{ + Kind: testKindDeployment, + }, + }, + }, + } + clusterSummary.Namespace = defaultNamespace + clusterSummary.Spec.ClusterNamespace = defaultNamespace + + Expect(testEnv.Create(context.TODO(), clusterSummary)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, clusterSummary)).To(Succeed()) + + stalePatchesHash := []byte("stale-patches-hash-from-before-patches-were-added") + clusterSummary.Status = configv1beta1.ClusterSummaryStatus{ + HelmReleaseSummaries: []configv1beta1.HelmChartSummary{ + { + ReleaseName: nginxChart.ReleaseName, + ReleaseNamespace: nginxChart.ReleaseNamespace, + Status: configv1beta1.HelmChartStatusManaging, + PatchesHash: stalePatchesHash, + }, + }, + } + Expect(testEnv.Status().Update(context.TODO(), clusterSummary)).To(Succeed()) + + 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()) + + manager, err := chartmanager.GetChartManagerInstance(context.TODO(), testEnv.Client) + Expect(err).To(BeNil()) + manager.RegisterClusterSummaryForCharts(clusterSummary) + + // Checked on the function's direct return value, not via a Get against testEnv's cache: + // the cache can still be showing the pre-call state for a beat after this returns, which + // would let a broken (fresh-value-writing) implementation slip past an Eventually/Get + // check by accident, since the stale cache read looks like the pre-existing value too. + updatedClusterSummary, conflict, err := controllers.UpdateStatusForReferencedHelmReleases(context.TODO(), + testEnv.Client, clusterSummary, nil, textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + Expect(conflict).To(BeFalse()) + Expect(updatedClusterSummary.Status.HelmReleaseSummaries).To(HaveLen(1)) + Expect(bytes.Equal(updatedClusterSummary.Status.HelmReleaseSummaries[0].PatchesHash, stalePatchesHash)).To(BeTrue()) + }) + It("updateStatusForeferencedHelmReleases is no-op in DryRun mode", func() { clusterSummary.Spec.ClusterProfileSpec = configv1beta1.Spec{ HelmCharts: []configv1beta1.HelmChart{ @@ -1140,9 +1480,9 @@ var _ = Describe("Hash methods", func() { } nginxChart := configv1beta1.HelmChart{ - RepositoryURL: "https://helm.nginx.com/stable/", - RepositoryName: "nginx-stable", - ChartName: "nginx-stable/nginx-ingress", + RepositoryURL: testRepoURLNginxStable, + RepositoryName: testRepoNameNginxStable, + ChartName: testChartNameNginxIngress, ChartVersion: "0.17.1", ReleaseName: testReleaseNameNginxLatest, ReleaseNamespace: testNginxRepo, diff --git a/controllers/init_container_work.go b/controllers/init_container_work.go index 7e59fbb1..9cc0553d 100644 --- a/controllers/init_container_work.go +++ b/controllers/init_container_work.go @@ -124,6 +124,16 @@ func updateClusterSummaryHelmHashes(ctx context.Context, directClient client.Cli } setHelmHash(currentClusterSummary, helmHash) + // PatchesHash is one value for the whole ClusterProfileSpec.Patches list, shared across + // every chart (same as buildReferencedHelmReleaseSummaries), so it's computed once here, + // not per chart. + patchesHash, err := getHelmChartPatchesHash(ctx, currentClusterSummary, logger) + if err != nil { + logger.V(logs.LogInfo).Error(err, "failed to get helm patches hash") + return err + } + setHelmChartsPatchesHash(currentClusterSummary, patchesHash) + for i := range currentClusterSummary.Spec.ClusterProfileSpec.HelmCharts { helmChart := ¤tClusterSummary.Spec.ClusterProfileSpec.HelmCharts[i] innerDCtx := &deploymentContext{ @@ -171,3 +181,12 @@ func setHelmChartValueHash(clusterSummary *configv1beta1.ClusterSummary, helmCha } } } + +// setHelmChartsPatchesHash stores patchesHash on every HelmReleaseSummaries entry, mirroring +// buildReferencedHelmReleaseSummaries: PatchesHash is one value for the whole ClusterProfileSpec.Patches +// list, not per chart, so every chart gets the same value. +func setHelmChartsPatchesHash(clusterSummary *configv1beta1.ClusterSummary, patchesHash []byte) { + for i := range clusterSummary.Status.HelmReleaseSummaries { + clusterSummary.Status.HelmReleaseSummaries[i].PatchesHash = patchesHash + } +} diff --git a/controllers/init_container_work_test.go b/controllers/init_container_work_test.go new file mode 100644 index 00000000..66460216 --- /dev/null +++ b/controllers/init_container_work_test.go @@ -0,0 +1,155 @@ +/* +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 controllers_test + +import ( + "bytes" + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/klog/v2/textlogger" + clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" + + configv1beta1 "github.com/projectsveltos/addon-controller/api/v1beta1" + "github.com/projectsveltos/addon-controller/controllers" + libsveltosv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" +) + +var _ = Describe("Init container work", func() { + It("updateClusterSummaryHelmHashes refreshes ValuesHash and PatchesHash without triggering a redeploy", func() { + clusterNamespace := randomString() + + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: clusterNamespace}} + Expect(testEnv.Create(context.TODO(), ns)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, ns)).To(Succeed()) + + cluster := &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: randomString(), + Namespace: clusterNamespace, + }, + } + Expect(testEnv.Create(context.TODO(), cluster)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, cluster)).To(Succeed()) + + requestChart := configv1beta1.HelmChart{ + RepositoryURL: testRepoURLNginxStable, + RepositoryName: testRepoNameNginxStable, + ChartName: testChartNameNginxIngress, + ReleaseName: testReleaseNameNginxLatest, + ReleaseNamespace: testNginxRepo, + ChartVersion: testChartVersion100, + HelmChartAction: configv1beta1.HelmChartActionInstall, + } + + clusterSummary := &configv1beta1.ClusterSummary{ + ObjectMeta: metav1.ObjectMeta{ + Name: randomString(), + Namespace: clusterNamespace, + }, + Spec: configv1beta1.ClusterSummarySpec{ + ClusterNamespace: clusterNamespace, + ClusterName: cluster.Name, + ClusterType: libsveltosv1beta1.ClusterTypeCapi, + ClusterProfileSpec: configv1beta1.Spec{ + SyncMode: configv1beta1.SyncModeContinuousWithDriftDetection, + HelmCharts: []configv1beta1.HelmChart{requestChart}, + Patches: []libsveltosv1beta1.Patch{ + { + Patch: testManagedAnnotationPatch, + Target: &libsveltosv1beta1.PatchSelector{ + Kind: testKindDeployment, + }, + }, + }, + }, + }, + } + Expect(testEnv.Create(context.TODO(), clusterSummary)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, clusterSummary)).To(Succeed()) + + currentClusterSummary := &configv1beta1.ClusterSummary{} + Expect(testEnv.Get(context.TODO(), + types.NamespacedName{Namespace: clusterSummary.Namespace, Name: clusterSummary.Name}, + currentClusterSummary)).To(Succeed()) + + // Simulate the state right before an addon-controller upgrade: the feature is + // already Provisioned, and the stored hashes are stale/wrong (as if computed by an + // older, different hash implementation). + currentClusterSummary.Status.FeatureSummaries = []configv1beta1.FeatureSummary{ + { + FeatureID: libsveltosv1beta1.FeatureHelm, + Status: libsveltosv1beta1.FeatureStatusProvisioned, + Hash: []byte(randomString()), + }, + } + currentClusterSummary.Status.HelmReleaseSummaries = []configv1beta1.HelmChartSummary{ + { + ReleaseName: requestChart.ReleaseName, + ReleaseNamespace: requestChart.ReleaseNamespace, + Status: configv1beta1.HelmChartStatusManaging, + ValuesHash: []byte("stale-values-hash"), + PatchesHash: []byte("stale-patches-hash"), + }, + } + Expect(testEnv.Status().Update(context.TODO(), currentClusterSummary)).To(Succeed()) + Eventually(func() error { + return testEnv.Get(context.TODO(), + types.NamespacedName{Namespace: clusterSummary.Namespace, Name: clusterSummary.Name}, + currentClusterSummary) + }, timeout, pollingInterval).Should(Succeed()) + + logger := textlogger.NewLogger(textlogger.NewConfig()) + + expectedPatchesHash, err := controllers.GetHelmChartPatchesHash(context.TODO(), currentClusterSummary, logger) + Expect(err).To(BeNil()) + Expect(expectedPatchesHash).ToNot(Equal([]byte("stale-patches-hash"))) + + Expect(controllers.UpdateClusterSummaryHelmHashes(context.TODO(), testEnv.Client, "", + currentClusterSummary, logger)).To(Succeed()) + + Eventually(func() bool { + err := testEnv.Get(context.TODO(), + types.NamespacedName{Namespace: clusterSummary.Namespace, Name: clusterSummary.Name}, + currentClusterSummary) + if err != nil { + return false + } + if len(currentClusterSummary.Status.HelmReleaseSummaries) != 1 { + return false + } + rs := currentClusterSummary.Status.HelmReleaseSummaries[0] + return bytes.Equal(rs.PatchesHash, expectedPatchesHash) && + string(rs.ValuesHash) != "stale-values-hash" + }, timeout, pollingInterval).Should(BeTrue()) + + // Refreshing the stored hashes must not, by itself, mark the feature for redeploy: + // no reset of Status/Hash, that decision belongs to the main reconcile loop, not to + // this startup pre-warm step. + for i := range currentClusterSummary.Status.FeatureSummaries { + if currentClusterSummary.Status.FeatureSummaries[i].FeatureID == libsveltosv1beta1.FeatureHelm { + Expect(currentClusterSummary.Status.FeatureSummaries[i].Status).To( + Equal(libsveltosv1beta1.FeatureStatusProvisioned)) + } + } + }) +}) diff --git a/controllers/resourcesummary_collection.go b/controllers/resourcesummary_collection.go index afa2a58e..9346443e 100644 --- a/controllers/resourcesummary_collection.go +++ b/controllers/resourcesummary_collection.go @@ -532,6 +532,7 @@ func processResourceSummary(ctx context.Context, clusterClient client.Client, l.V(logs.LogDebug).Info("redeploy helm") clusterSummary.Status.FeatureSummaries[i].Hash = nil clusterSummary.Status.FeatureSummaries[i].Status = libsveltosv1beta1.FeatureStatusProvisioning + markDriftedHelmCharts(clusterSummary, rs.Status.DriftedHelmCharts, l) trackDrifts(clusterSummaryNamespace, clusterSummary.Spec.ClusterName, string(clusterSummary.Status.FeatureSummaries[i].FeatureID), string(clusterSummary.Spec.ClusterType), profileKind, profileNamespace, profileName, logger) } @@ -586,9 +587,39 @@ func resetResourceSummaryStatus(ctx context.Context, remoteClient client.Client, resourceSummary.Status.ResourcesChanged = false resourceSummary.Status.KustomizeResourcesChanged = false resourceSummary.Status.HelmResourcesChanged = false + resourceSummary.Status.DriftedHelmCharts = nil return remoteClient.Status().Update(ctx, resourceSummary) } +// markDriftedHelmCharts sets NeedsRedeploy on the HelmReleaseSummaries entries matching +// driftedCharts (by release name/namespace), so shouldUpgrade can later scope the redeploy +// to only those charts instead of the whole Helm feature. If driftedCharts is empty (e.g. an +// older drift-detection-manager that doesn't report chart ownership yet), every chart is +// marked, same as today's behavior, so drift is never silently missed. +func markDriftedHelmCharts(clusterSummary *configv1beta1.ClusterSummary, + driftedCharts []libsveltosv1beta1.HelmChartRef, logger logr.Logger) { + + if len(driftedCharts) == 0 { + logger.V(logs.LogDebug).Info("no chart-scoped drift info available, marking all charts for redeploy") + for i := range clusterSummary.Status.HelmReleaseSummaries { + clusterSummary.Status.HelmReleaseSummaries[i].NeedsRedeploy = true + } + return + } + + for i := range clusterSummary.Status.HelmReleaseSummaries { + summary := &clusterSummary.Status.HelmReleaseSummaries[i] + for j := range driftedCharts { + if summary.ReleaseName == driftedCharts[j].ReleaseName && + summary.ReleaseNamespace == driftedCharts[j].ReleaseNamespace { + + summary.NeedsRedeploy = true + break + } + } + } +} + func getListOfClusterWithDriftDetectionDeployed(ctx context.Context, c client.Client, ) (map[corev1.ObjectReference]bool, error) { diff --git a/controllers/resourcesummary_collection_test.go b/controllers/resourcesummary_collection_test.go index bb7589c9..7e9300bb 100644 --- a/controllers/resourcesummary_collection_test.go +++ b/controllers/resourcesummary_collection_test.go @@ -91,7 +91,7 @@ var _ = Describe("ResourceSummary Collection", func() { } Expect(waitForObject(context.TODO(), testEnv.Client, ns)).To(Succeed()) - resourceSummary := getResourceSummary(nil, nil) + resourceSummary := getResourceSummary() resourceSummary.Annotations = map[string]string{ libsveltosv1beta1.ClusterSummaryNameAnnotation: clusterSummary.Name, libsveltosv1beta1.ClusterSummaryNamespaceAnnotation: clusterSummary.Namespace, @@ -151,6 +151,118 @@ var _ = Describe("ResourceSummary Collection", func() { return err == nil && !currentResourceSummary.Status.HelmResourcesChanged }, timeout, pollingInterval).Should(BeTrue()) }) + It("processResourceSummary scopes NeedsRedeploy to only the drifted chart", func() { + cluster := prepareCluster() + + chartA := configv1beta1.HelmChartSummary{ + ReleaseName: randomString(), + ReleaseNamespace: randomString(), + Status: configv1beta1.HelmChartStatusManaging, + } + chartB := configv1beta1.HelmChartSummary{ + ReleaseName: randomString(), + ReleaseNamespace: randomString(), + Status: configv1beta1.HelmChartStatusManaging, + } + + clusterSummary := &configv1beta1.ClusterSummary{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: cluster.Namespace, + Name: clusterProfileNamePrefix + randomString(), + Labels: map[string]string{clusterops.ClusterProfileLabelName: randomString()}, + }, + Spec: configv1beta1.ClusterSummarySpec{ + ClusterType: libsveltosv1beta1.ClusterTypeCapi, + }, + } + Expect(testEnv.Create(context.TODO(), clusterSummary)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, clusterSummary)).To(Succeed()) + + currentClusterSummary := &configv1beta1.ClusterSummary{} + Expect(testEnv.Get(context.TODO(), + types.NamespacedName{Namespace: clusterSummary.Namespace, Name: clusterSummary.Name}, + currentClusterSummary)).To(Succeed()) + currentClusterSummary.Status.FeatureSummaries = []configv1beta1.FeatureSummary{ + { + FeatureID: libsveltosv1beta1.FeatureHelm, + Status: libsveltosv1beta1.FeatureStatusProvisioned, + Hash: []byte(randomString()), + }, + } + currentClusterSummary.Status.HelmReleaseSummaries = []configv1beta1.HelmChartSummary{chartA, chartB} + Expect(testEnv.Status().Update(context.TODO(), currentClusterSummary)).To(Succeed()) + + resourceSummary := getResourceSummary() + resourceSummary.Annotations = map[string]string{ + libsveltosv1beta1.ClusterSummaryNameAnnotation: clusterSummary.Name, + libsveltosv1beta1.ClusterSummaryNamespaceAnnotation: clusterSummary.Namespace, + } + Expect(testEnv.Create(context.TODO(), resourceSummary)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, resourceSummary)).To(Succeed()) + + currentResourceSummary := &libsveltosv1beta1.ResourceSummary{} + Expect(testEnv.Get(context.TODO(), + types.NamespacedName{Namespace: resourceSummary.Namespace, Name: resourceSummary.Name}, + currentResourceSummary)).To(Succeed()) + currentResourceSummary.Status.HelmResourcesChanged = true + // Only chartA drifted. chartB must be left alone. + currentResourceSummary.Status.DriftedHelmCharts = []libsveltosv1beta1.HelmChartRef{ + { + ChartName: randomString(), + ReleaseName: chartA.ReleaseName, + ReleaseNamespace: chartA.ReleaseNamespace, + }, + } + Expect(testEnv.Status().Update(context.TODO(), currentResourceSummary)).To(Succeed()) + + Eventually(func() bool { + err := testEnv.Get(context.TODO(), + types.NamespacedName{Namespace: resourceSummary.Namespace, Name: resourceSummary.Name}, + currentResourceSummary) + return err == nil && currentResourceSummary.Status.HelmResourcesChanged + }, timeout, pollingInterval).Should(BeTrue()) + + Expect(controllers.ProcessResourceSummary(context.TODO(), testEnv.Client, currentResourceSummary, + textlogger.NewLogger(textlogger.NewConfig()))).To(Succeed()) + + Eventually(func() bool { + err := testEnv.Get(context.TODO(), + types.NamespacedName{Namespace: clusterSummary.Namespace, Name: clusterSummary.Name}, + currentClusterSummary) + if err != nil { + return false + } + for i := range currentClusterSummary.Status.HelmReleaseSummaries { + rs := ¤tClusterSummary.Status.HelmReleaseSummaries[i] + if rs.ReleaseName == chartA.ReleaseName { + if !rs.NeedsRedeploy { + return false + } + } else if rs.NeedsRedeploy { + return false + } + } + return true + }, timeout, pollingInterval).Should(BeTrue()) + }) + + It("markDriftedHelmCharts marks every chart when no chart-scoped drift info is available", func() { + clusterSummary := &configv1beta1.ClusterSummary{ + Status: configv1beta1.ClusterSummaryStatus{ + HelmReleaseSummaries: []configv1beta1.HelmChartSummary{ + {ReleaseName: randomString(), ReleaseNamespace: randomString()}, + {ReleaseName: randomString(), ReleaseNamespace: randomString()}, + }, + }, + } + + controllers.MarkDriftedHelmCharts(clusterSummary, nil, textlogger.NewLogger(textlogger.NewConfig())) + + for i := range clusterSummary.Status.HelmReleaseSummaries { + Expect(clusterSummary.Status.HelmReleaseSummaries[i].NeedsRedeploy).To(BeTrue()) + } + }) + It("isResourceSummaryInstalledCached caches positive result", func() { controllers.ResetResourceSummaryInstalledCache() @@ -233,7 +345,7 @@ var _ = Describe("ResourceSummary Collection", func() { Expect(testEnv.Status().Update(context.TODO(), currentClusterSummary)).To(Succeed()) // RS labeled as CAPI cluster — drift detected. - capiRS := getResourceSummary(nil, nil) + capiRS := getResourceSummary() capiRS.Namespace = capiCluster.Namespace capiRS.Annotations = map[string]string{ libsveltosv1beta1.ClusterSummaryNameAnnotation: clusterSummary.Name, @@ -253,7 +365,7 @@ var _ = Describe("ResourceSummary Collection", func() { // RS labeled as Sveltos cluster — same namespace/name as the CAPI cluster, also with drift. // This must NOT be processed since its cluster type does not match the CAPI entry in clustersWithDD. - sveltosRS := getResourceSummary(nil, nil) + sveltosRS := getResourceSummary() sveltosRS.Namespace = capiCluster.Namespace sveltosRS.Annotations = map[string]string{ libsveltosv1beta1.ClusterSummaryNameAnnotation: clusterSummary.Name, @@ -311,44 +423,13 @@ var _ = Describe("ResourceSummary Collection", func() { }) }) -func getResourceSummary(resource, helmResource *corev1.ObjectReference) *libsveltosv1beta1.ResourceSummary { - rs := &libsveltosv1beta1.ResourceSummary{ +// getResourceSummary returns a bare ResourceSummary. None of the tests in this file need it +// pre-populated with Spec.Resources/ChartResources, they set Status directly instead. +func getResourceSummary() *libsveltosv1beta1.ResourceSummary { + return &libsveltosv1beta1.ResourceSummary{ ObjectMeta: metav1.ObjectMeta{ Name: randomString(), Namespace: sveltosNamespace, }, } - - if resource != nil { - rs.Spec.Resources = []libsveltosv1beta1.Resource{ - { - Name: resource.Name, - Namespace: resource.Namespace, - Kind: resource.Kind, - Group: resource.GroupVersionKind().Group, - Version: resource.GroupVersionKind().Version, - }, - } - } - - if helmResource != nil { - rs.Spec.ChartResources = []libsveltosv1beta1.HelmResources{ - { - ChartName: randomString(), - ReleaseName: randomString(), - ReleaseNamespace: randomString(), - Resources: []libsveltosv1beta1.ResourceSummaryResource{ - { - Name: helmResource.Name, - Namespace: helmResource.Namespace, - Kind: helmResource.Kind, - Group: helmResource.GroupVersionKind().Group, - Version: helmResource.GroupVersionKind().Version, - }, - }, - }, - } - } - - return rs } diff --git a/controllers/test_constants_test.go b/controllers/test_constants_test.go index e565ba56..508bb7c3 100644 --- a/controllers/test_constants_test.go +++ b/controllers/test_constants_test.go @@ -43,6 +43,9 @@ const ( testReleaseNameContour = "contour-latest" testRepoURLBitnami = "https://charts.bitnami.com/bitnami" testChartNameBitnamiContour = "bitnami/contour" + testRepoURLNginxStable = "https://helm.nginx.com/stable/" + testRepoNameNginxStable = "nginx-stable" + testChartNameNginxIngress = "nginx-stable/nginx-ingress" // Label keys and values testEnvLabelKey = "env" @@ -114,6 +117,9 @@ const ( testEnvLabelPatch = `- op: add path: /metadata/labels/environment value: production` + testManagedAnnotationPatch = `- op: add + path: /metadata/annotations/projectsveltos.io~1managed + value: "true"` // Verbs and resources testCreateVerb = "create" diff --git a/go.mod b/go.mod index c1058dfa..ed447ea9 100644 --- a/go.mod +++ b/go.mod @@ -20,7 +20,7 @@ require ( github.com/onsi/gomega v1.42.1 github.com/opencontainers/image-spec v1.1.1 github.com/pkg/errors v0.9.1 - github.com/projectsveltos/libsveltos v1.13.1-0.20260812123753-3148ff0e45f2 + github.com/projectsveltos/libsveltos v1.13.1-0.20260813141903-0bf44bc3c246 github.com/prometheus/client_golang v1.24.1 github.com/sigstore/cosign/v3 v3.1.3 github.com/sigstore/sigstore v1.10.9 diff --git a/go.sum b/go.sum index 511a1ed4..ea686e52 100644 --- a/go.sum +++ b/go.sum @@ -639,8 +639,8 @@ github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRI github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/poy/onpar v1.1.2 h1:QaNrNiZx0+Nar5dLgTVp5mXkyoVFIbepjyEoGSnhbAY= github.com/poy/onpar v1.1.2/go.mod h1:6X8FLNoxyr9kkmnlqpK6LSoiOtrO6MICtWwEuWkLjzg= -github.com/projectsveltos/libsveltos v1.13.1-0.20260812123753-3148ff0e45f2 h1:kPqbjM5zOj2IazbaCdjgO/IIyqMdWV8N60L5DIl/HjE= -github.com/projectsveltos/libsveltos v1.13.1-0.20260812123753-3148ff0e45f2/go.mod h1:dgQKoyCm3xr5TV0vJxbq+dkuBLmwm4mqt6tMAgJiP+s= +github.com/projectsveltos/libsveltos v1.13.1-0.20260813141903-0bf44bc3c246 h1:ByiXtT8ebkKCnsFl3iwHH8ZbK3hvgc8TwmT7U4O0UuM= +github.com/projectsveltos/libsveltos v1.13.1-0.20260813141903-0bf44bc3c246/go.mod h1:dgQKoyCm3xr5TV0vJxbq+dkuBLmwm4mqt6tMAgJiP+s= github.com/projectsveltos/lua-utils/glua-json v0.0.0-20251212200258-2b3cdcb7c0f5 h1:khnc+994UszxZYu69J+R5FKiLA/Nk1JQj0EYAkwTWz0= github.com/projectsveltos/lua-utils/glua-json v0.0.0-20251212200258-2b3cdcb7c0f5/go.mod h1:yVL8KQFa9tmcxgwl9nwIMtKgtmIVC1zaFRSCfOwYvPY= github.com/projectsveltos/lua-utils/glua-runes v0.0.0-20251212200258-2b3cdcb7c0f5 h1:YbsebwRwTRhV8QacvEAdFqxcxHdeu7JTVtsBovbkgos= diff --git a/lib/crd/clustersummaries.go b/lib/crd/clustersummaries.go index c1668b99..fe5d8d25 100644 --- a/lib/crd/clustersummaries.go +++ b/lib/crd/clustersummaries.go @@ -2551,6 +2551,13 @@ spec: greater than ChartVersion. Populated by a periodic background check, independent of the reconcile loop. Detection only: Sveltos never mutates ChartVersion based on this. type: string + needsRedeploy: + description: |- + NeedsRedeploy is set to true when drift-detection reports that a resource deployed by + this chart changed out of band. Only meaningful when SyncMode is ContinuousWithDriftDetection. + When true, this chart is upgraded on the next reconciliation regardless of whether its + desired values/version changed; cleared once the upgrade succeeds. + type: boolean patchesHash: description: PatchesHash represents of a unique value for the patches section diff --git a/manifest/manifest.yaml b/manifest/manifest.yaml index fdc5c81a..a602d56f 100644 --- a/manifest/manifest.yaml +++ b/manifest/manifest.yaml @@ -8929,6 +8929,13 @@ spec: greater than ChartVersion. Populated by a periodic background check, independent of the reconcile loop. Detection only: Sveltos never mutates ChartVersion based on this. type: string + needsRedeploy: + description: |- + NeedsRedeploy is set to true when drift-detection reports that a resource deployed by + this chart changed out of band. Only meaningful when SyncMode is ContinuousWithDriftDetection. + When true, this chart is upgraded on the next reconciliation regardless of whether its + desired values/version changed; cleared once the upgrade succeeds. + type: boolean patchesHash: description: PatchesHash represents of a unique value for the patches section diff --git a/pkg/drift-detection/drift-detection-manager-in-mgmt-cluster.go b/pkg/drift-detection/drift-detection-manager-in-mgmt-cluster.go index f68fbab6..4ec7734c 100644 --- a/pkg/drift-detection/drift-detection-manager-in-mgmt-cluster.go +++ b/pkg/drift-detection/drift-detection-manager-in-mgmt-cluster.go @@ -60,7 +60,7 @@ spec: valueFrom: fieldRef: fieldPath: metadata.namespace - image: docker.io/projectsveltos/drift-detection-manager@sha256:651ef898a5116d07f74fec85f533927f74af56f36422efed926d69ab481ff36d + image: docker.io/projectsveltos/drift-detection-manager@sha256:b69e6497fdb4bf2309b34d39412d10cd9eba158abe0cfff53a786509ebc38ddc livenessProbe: failureThreshold: 3 httpGet: diff --git a/pkg/drift-detection/drift-detection-manager-in-mgmt-cluster.yaml b/pkg/drift-detection/drift-detection-manager-in-mgmt-cluster.yaml index 7a932eb7..238c18bf 100644 --- a/pkg/drift-detection/drift-detection-manager-in-mgmt-cluster.yaml +++ b/pkg/drift-detection/drift-detection-manager-in-mgmt-cluster.yaml @@ -42,7 +42,7 @@ spec: valueFrom: fieldRef: fieldPath: metadata.namespace - image: docker.io/projectsveltos/drift-detection-manager@sha256:651ef898a5116d07f74fec85f533927f74af56f36422efed926d69ab481ff36d + image: docker.io/projectsveltos/drift-detection-manager@sha256:b69e6497fdb4bf2309b34d39412d10cd9eba158abe0cfff53a786509ebc38ddc livenessProbe: failureThreshold: 3 httpGet: diff --git a/pkg/drift-detection/drift-detection-manager.go b/pkg/drift-detection/drift-detection-manager.go index 41b78d20..e3f0a62b 100644 --- a/pkg/drift-detection/drift-detection-manager.go +++ b/pkg/drift-detection/drift-detection-manager.go @@ -162,7 +162,7 @@ spec: valueFrom: fieldRef: fieldPath: metadata.namespace - image: docker.io/projectsveltos/drift-detection-manager@sha256:651ef898a5116d07f74fec85f533927f74af56f36422efed926d69ab481ff36d + image: docker.io/projectsveltos/drift-detection-manager@sha256:b69e6497fdb4bf2309b34d39412d10cd9eba158abe0cfff53a786509ebc38ddc livenessProbe: failureThreshold: 3 httpGet: diff --git a/pkg/drift-detection/drift-detection-manager.yaml b/pkg/drift-detection/drift-detection-manager.yaml index 27070b94..8fe16854 100644 --- a/pkg/drift-detection/drift-detection-manager.yaml +++ b/pkg/drift-detection/drift-detection-manager.yaml @@ -144,7 +144,7 @@ spec: valueFrom: fieldRef: fieldPath: metadata.namespace - image: docker.io/projectsveltos/drift-detection-manager@sha256:651ef898a5116d07f74fec85f533927f74af56f36422efed926d69ab481ff36d + image: docker.io/projectsveltos/drift-detection-manager@sha256:b69e6497fdb4bf2309b34d39412d10cd9eba158abe0cfff53a786509ebc38ddc livenessProbe: failureThreshold: 3 httpGet: