fix: clear stale Helm Conflict entry when ownership is won mid-deploy…

controllers/clustersummary_deployer_test.go

@@ -333,6 +333,98 @@ var _ = Describe("ClustersummaryDeployer", func() {
 		Expect(dep.IsKeyInProgress(key)).To(BeFalse())
 	})
 
+	It("deployFeature does not revisit a stale Helm Conflict once the feature is Provisioned", func() {
+		// Documents the latch the Helm-side fix exists to prevent. A ClusterSummary that ends a
+		// pass Provisioned while still holding a Conflict entry never runs the Helm handler
+		// again, so the entry is never recomputed and the profile reports a conflict forever.
+		// shouldRedeploy returns false for a deployed feature whose hash has not changed, and
+		// helmConflictResolved is only consulted on a deployer error, so neither path recovers.
+		releaseName := randomString()
+		releaseNamespace := randomString()
+		clusterSummary.Spec.ClusterProfileSpec.HelmCharts = []configv1beta1.HelmChart{
+			{
+				RepositoryURL:    randomString(),
+				RepositoryName:   randomString(),
+				ChartName:        randomString(),
+				ChartVersion:     randomString(),
+				ReleaseName:      releaseName,
+				ReleaseNamespace: releaseNamespace,
+			},
+		}
+
+		initObjects := []client.Object{
+			clusterSummary,
+			clusterProfile,
+			cluster,
+		}
+
+		c := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(initObjects...).WithObjects(initObjects...).Build()
+
+		clusterSummaryScope := getClusterSummaryScope(c, logger, clusterProfile, clusterSummary)
+
+		// helmHash instantiates the chart values, and that resolves the Cluster through the
+		// management cluster client rather than the client passed in. So the Cluster has to
+		// exist there too, not only in the fake client driving the reconcile.
+		ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}}
+		Expect(testEnv.Create(ctx, ns)).To(Succeed())
+		Expect(waitForObject(ctx, testEnv.Client, ns)).To(Succeed())
+		mgmtCluster := &clusterv1.Cluster{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: clusterSummary.Spec.ClusterNamespace,
+				Name:      clusterSummary.Spec.ClusterName,
+			},
+		}
+		Expect(testEnv.Create(ctx, mgmtCluster)).To(Succeed())
+		Expect(waitForObject(ctx, testEnv.Client, mgmtCluster)).To(Succeed())
+
+		helmHash, err := controllers.HelmHash(ctx, c, clusterSummary, textlogger.NewLogger(textlogger.NewConfig()))
+		Expect(err).To(BeNil())
+
+		clusterSummary.Status.FeatureSummaries = []configv1beta1.FeatureSummary{
+			{
+				FeatureID: libsveltosv1beta1.FeatureHelm,
+				Hash:      helmHash,
+				Status:    libsveltosv1beta1.FeatureStatusProvisioned,
+			},
+		}
+		// The conflict is over. The ClusterSummary named here has been deleted, so this entry
+		// can only be corrected by running the Helm handler again.
+		clusterSummary.Status.HelmReleaseSummaries = []configv1beta1.HelmChartSummary{
+			{
+				ReleaseName:      releaseName,
+				ReleaseNamespace: releaseNamespace,
+				Status:           configv1beta1.HelmChartStatusConflict,
+				ConflictMessage:  fmt.Sprintf("ClusterSummary %s managing it", randomString()),
+			},
+		}
+
+		Expect(c.Status().Update(context.TODO(), clusterSummary)).To(Succeed())
+
+		dep := fakedeployer.GetClient(context.TODO(), textlogger.NewLogger(textlogger.NewConfig()), c)
+
+		reconciler := getClusterSummaryReconciler(c, dep)
+
+		f := controllers.GetHandlersForFeature(libsveltosv1beta1.FeatureHelm)
+
+		err = controllers.DeployFeature(reconciler, context.TODO(), clusterSummaryScope, f,
+			textlogger.NewLogger(textlogger.NewConfig()))
+		Expect(err).To(BeNil())
+
+		// No deploy is submitted, so nothing recomputes HelmReleaseSummaries and the Conflict
+		// entry survives untouched.
+		key := deployer.GetKey(clusterSummary.Spec.ClusterNamespace, clusterSummary.Spec.ClusterName,
+			clusterSummary.Name, string(libsveltosv1beta1.FeatureHelm), libsveltosv1beta1.ClusterTypeCapi, false)
+		Expect(dep.IsKeyInProgress(key)).To(BeFalse())
+
+		currentClusterSummary := &configv1beta1.ClusterSummary{}
+		Expect(c.Get(context.TODO(),
+			types.NamespacedName{Namespace: clusterSummary.Namespace, Name: clusterSummary.Name},
+			currentClusterSummary)).To(Succeed())
+		Expect(currentClusterSummary.Status.HelmReleaseSummaries).To(HaveLen(1))
+		Expect(currentClusterSummary.Status.HelmReleaseSummaries[0].Status).To(
+			Equal(configv1beta1.HelmChartStatusConflict))
+	})
+
 	It("deployFeature when feature is deployed and hash has changed, calls Deploy", func() {
 		clusterRoleName := randomString()
 		configMap := createConfigMapWithPolicy("default", randomString(), fmt.Sprintf(viewClusterRole, clusterRoleName))

controllers/handlers_helm.go

@@ -4736,6 +4736,11 @@ func updateValueHashOnHelmChartSummary(ctx context.Context, requestedChart *conf
 		return nil, err
 	}
 
+	chartManager, err := chartmanager.GetChartManagerInstance(ctx, c)
+	if err != nil {
+		return nil, err
+	}
+
 	err = retry.RetryOnConflict(retry.DefaultRetry, func() error {
 		currentClusterSummary := &configv1beta1.ClusterSummary{}
 		err = c.Get(ctx,
@@ -4744,6 +4749,17 @@ func updateValueHashOnHelmChartSummary(ctx context.Context, requestedChart *conf
 			return err
 		}
 
+		// Correct a Conflict left over from the start of this pass, but only while this
+		// ClusterSummary really is the chart's manager. deploySingleChart checks ownership
+		// before deploying; re-check it here, at the write, so the guarantee does not rest on
+		// that staying the only caller. Without the correction the entry latches: the feature
+		// ends Provisioned with an unchanged hash, so shouldRedeploy never runs the Helm
+		// handler again to recompute it. DryRun is excluded because it can pass the ownership
+		// check without ever registering, and it no-ops the two functions that otherwise
+		// maintain these entries.
+		isManager := dCtx.clusterSummary.Spec.ClusterProfileSpec.SyncMode != configv1beta1.SyncModeDryRun &&
+			chartManager.CanManageChart(dCtx.clusterSummary, requestedChart)
+
 		for i := range currentClusterSummary.Status.HelmReleaseSummaries {
 			rs := &currentClusterSummary.Status.HelmReleaseSummaries[i]
 			if rs.ReleaseName == requestedChart.ReleaseName &&
@@ -4752,6 +4768,10 @@ func updateValueHashOnHelmChartSummary(ctx context.Context, requestedChart *conf
 				rs.ValuesHash = helmChartValuesHash
 				rs.PatchesHash = helmChartPatchesHash
 				rs.NeedsRedeploy = false
+				if isManager {
+					rs.Status = configv1beta1.HelmChartStatusManaging
+					rs.ConflictMessage = ""
+				}
 				setResolvedHelmChartIdentity(ctx, c, dCtx.clusterSummary, rs, requestedChart, currentRelease, logger)
 			}
 		}

controllers/handlers_helm_test.go

@@ -914,6 +914,204 @@ var _ = Describe("HandlersHelm", func() {
 		Expect(bytes.Equal(updatedClusterSummary.Status.HelmReleaseSummaries[0].PatchesHash, stalePatchesHash)).To(BeTrue())
 	})
 
+	It("UpdateValueHashOnHelmChartSummary clears a stale Conflict once this ClusterSummary owns the release", func() {
+		// Regression test for a latched conflict. buildReferencedHelmReleaseSummaries computes
+		// Status once, at the start of handleCharts. If the ClusterSummary holding the release
+		// unregisters after that write but before walkChartsAndDeploy re-checks ownership, this
+		// ClusterSummary deploys the chart while its entry still says Conflict. The pass then
+		// ends Provisioned with an unchanged hash, so shouldRedeploy never runs the Helm handler
+		// again and the stale entry stays forever.
+		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())
+
+		// The stale entry, naming a ClusterSummary that has since been deleted, plus a second
+		// release that is genuinely owned by someone else. Only the first may be touched.
+		otherConflictMessage := fmt.Sprintf("ClusterSummary %s managing it", randomString())
+		clusterSummary.Status = configv1beta1.ClusterSummaryStatus{
+			HelmReleaseSummaries: []configv1beta1.HelmChartSummary{
+				{
+					ReleaseName:      nginxChart.ReleaseName,
+					ReleaseNamespace: nginxChart.ReleaseNamespace,
+					Status:           configv1beta1.HelmChartStatusConflict,
+					ConflictMessage:  fmt.Sprintf("ClusterSummary %s managing it", randomString()),
+				},
+				{
+					ReleaseName:      testReleaseNameKyverno,
+					ReleaseNamespace: testReleaseNameKyverno,
+					Status:           configv1beta1.HelmChartStatusConflict,
+					ConflictMessage:  otherConflictMessage,
+				},
+			},
+		}
+		Expect(testEnv.Status().Update(context.TODO(), clusterSummary)).To(Succeed())
+
+		createClusterForClusterSummary(clusterSummary)
+
+		// This ClusterSummary is the only registered manager, so it owns the release.
+		manager, err := chartmanager.GetChartManagerInstance(context.TODO(), testEnv.Client)
+		Expect(err).To(BeNil())
+		manager.RegisterClusterSummaryForCharts(clusterSummary)
+
+		dCtx := controllers.NewDeploymentContext(clusterSummary, nil, nil)
+		_, err = controllers.UpdateValueHashOnHelmChartSummary(context.TODO(), nginxChart, nil, dCtx,
+			textlogger.NewLogger(textlogger.NewConfig()))
+		Expect(err).To(BeNil())
+
+		current := readClusterSummaryUncached(clusterSummary)
+
+		owned := findHelmChartSummary(current, nginxChart.ReleaseNamespace, nginxChart.ReleaseName)
+		Expect(owned).ToNot(BeNil())
+		Expect(owned.Status).To(Equal(configv1beta1.HelmChartStatusManaging))
+		Expect(owned.ConflictMessage).To(BeEmpty())
+
+		// The write must be scoped to the release that was deployed.
+		untouched := findHelmChartSummary(current, testReleaseNameKyverno, testReleaseNameKyverno)
+		Expect(untouched).ToNot(BeNil())
+		Expect(untouched.Status).To(Equal(configv1beta1.HelmChartStatusConflict))
+		Expect(untouched.ConflictMessage).To(Equal(otherConflictMessage))
+	})
+
+	It("UpdateValueHashOnHelmChartSummary keeps the Conflict when another ClusterSummary owns the release", func() {
+		// The correction above is only safe while this ClusterSummary is the chart's manager.
+		// deploySingleChart gates on that before deploying, but the write re-checks it so the
+		// guarantee does not rest on that staying the only caller.
+		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())
+
+		conflictMessage := fmt.Sprintf("ClusterSummary %s managing it", randomString())
+		clusterSummary.Status = configv1beta1.ClusterSummaryStatus{
+			HelmReleaseSummaries: []configv1beta1.HelmChartSummary{
+				{
+					ReleaseName:      nginxChart.ReleaseName,
+					ReleaseNamespace: nginxChart.ReleaseNamespace,
+					Status:           configv1beta1.HelmChartStatusConflict,
+					ConflictMessage:  conflictMessage,
+				},
+			},
+		}
+		Expect(testEnv.Status().Update(context.TODO(), clusterSummary)).To(Succeed())
+
+		createClusterForClusterSummary(clusterSummary)
+
+		// Register a different ClusterSummary first, so it holds the release and ours does not.
+		otherClusterSummary := &configv1beta1.ClusterSummary{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      randomString(),
+				Namespace: clusterSummary.Namespace,
+			},
+			Spec: configv1beta1.ClusterSummarySpec{
+				ClusterNamespace:   clusterSummary.Spec.ClusterNamespace,
+				ClusterName:        clusterSummary.Spec.ClusterName,
+				ClusterType:        clusterSummary.Spec.ClusterType,
+				ClusterProfileSpec: configv1beta1.Spec{HelmCharts: []configv1beta1.HelmChart{*nginxChart}},
+			},
+		}
+		manager, err := chartmanager.GetChartManagerInstance(context.TODO(), testEnv.Client)
+		Expect(err).To(BeNil())
+		manager.RegisterClusterSummaryForCharts(otherClusterSummary)
+		manager.RegisterClusterSummaryForCharts(clusterSummary)
+		Expect(manager.CanManageChart(clusterSummary, nginxChart)).To(BeFalse())
+
+		dCtx := controllers.NewDeploymentContext(clusterSummary, nil, nil)
+		_, err = controllers.UpdateValueHashOnHelmChartSummary(context.TODO(), nginxChart, nil, dCtx,
+			textlogger.NewLogger(textlogger.NewConfig()))
+		Expect(err).To(BeNil())
+
+		current := readClusterSummaryUncached(clusterSummary)
+		summary := findHelmChartSummary(current, nginxChart.ReleaseNamespace, nginxChart.ReleaseName)
+		Expect(summary).ToNot(BeNil())
+		Expect(summary.Status).To(Equal(configv1beta1.HelmChartStatusConflict))
+		Expect(summary.ConflictMessage).To(Equal(conflictMessage))
+	})
+
+	It("UpdateValueHashOnHelmChartSummary keeps the Conflict in DryRun mode", func() {
+		// DryRun can pass the ownership check without ever registering, and it no-ops the two
+		// functions that otherwise maintain these entries. It must change nothing here either.
+		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},
+			SyncMode:   configv1beta1.SyncModeDryRun,
+		}
+		clusterSummary.Namespace = defaultNamespace
+		clusterSummary.Spec.ClusterNamespace = defaultNamespace
+
+		Expect(testEnv.Create(context.TODO(), clusterSummary)).To(Succeed())
+		Expect(waitForObject(context.TODO(), testEnv.Client, clusterSummary)).To(Succeed())
+
+		conflictMessage := fmt.Sprintf("ClusterSummary %s managing it", randomString())
+		clusterSummary.Status = configv1beta1.ClusterSummaryStatus{
+			HelmReleaseSummaries: []configv1beta1.HelmChartSummary{
+				{
+					ReleaseName:      nginxChart.ReleaseName,
+					ReleaseNamespace: nginxChart.ReleaseNamespace,
+					Status:           configv1beta1.HelmChartStatusConflict,
+					ConflictMessage:  conflictMessage,
+				},
+			},
+		}
+		Expect(testEnv.Status().Update(context.TODO(), clusterSummary)).To(Succeed())
+
+		createClusterForClusterSummary(clusterSummary)
+
+		// Registered, so only the DryRun check can stop the write.
+		manager, err := chartmanager.GetChartManagerInstance(context.TODO(), testEnv.Client)
+		Expect(err).To(BeNil())
+		manager.RegisterClusterSummaryForCharts(clusterSummary)
+		Expect(manager.CanManageChart(clusterSummary, nginxChart)).To(BeTrue())
+
+		dCtx := controllers.NewDeploymentContext(clusterSummary, nil, nil)
+		_, err = controllers.UpdateValueHashOnHelmChartSummary(context.TODO(), nginxChart, nil, dCtx,
+			textlogger.NewLogger(textlogger.NewConfig()))
+		Expect(err).To(BeNil())
+
+		current := readClusterSummaryUncached(clusterSummary)
+		summary := findHelmChartSummary(current, nginxChart.ReleaseNamespace, nginxChart.ReleaseName)
+		Expect(summary).ToNot(BeNil())
+		Expect(summary.Status).To(Equal(configv1beta1.HelmChartStatusConflict))
+		Expect(summary.ConflictMessage).To(Equal(conflictMessage))
+	})
+
 	It("updateStatusForeferencedHelmReleases is no-op in DryRun mode", func() {
 		clusterSummary.Spec.ClusterProfileSpec = configv1beta1.Spec{
 			HelmCharts: []configv1beta1.HelmChart{
@@ -2836,3 +3034,44 @@ var _ = Describe("locateChartWithTimeout", func() {
 		Expect(err).To(Equal(expectedErr))
 	})
 })
+
+// createClusterForClusterSummary creates the Cluster that clusterSummary points at.
+// getHelmChartValuesHash resolves it via clusterproxy.GetCluster, so it must exist.
+func createClusterForClusterSummary(clusterSummary *configv1beta1.ClusterSummary) {
+	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())
+}
+
+// readClusterSummaryUncached reads straight from the API server. testEnv.Client is cached and
+// can still serve the pre-update object for a beat after a status write returns, which would
+// make an assertion about a just-written value flaky.
+func readClusterSummaryUncached(clusterSummary *configv1beta1.ClusterSummary) *configv1beta1.ClusterSummary {
+	uncached, err := client.New(testEnv.Config, client.Options{Scheme: scheme})
+	Expect(err).To(BeNil())
+
+	current := &configv1beta1.ClusterSummary{}
+	Expect(uncached.Get(context.TODO(),
+		types.NamespacedName{Namespace: clusterSummary.Namespace, Name: clusterSummary.Name},
+		current)).To(Succeed())
+
+	return current
+}
+
+func findHelmChartSummary(clusterSummary *configv1beta1.ClusterSummary,
+	releaseNamespace, releaseName string) *configv1beta1.HelmChartSummary {
+
+	for i := range clusterSummary.Status.HelmReleaseSummaries {
+		summary := &clusterSummary.Status.HelmReleaseSummaries[i]
+		if summary.ReleaseNamespace == releaseNamespace && summary.ReleaseName == releaseName {
+			return summary
+		}
+	}
+
+	return nil
+}