diff --git a/controllers/clustersummary_deployer_test.go b/controllers/clustersummary_deployer_test.go index 508192f5..8a2dcd79 100644 --- a/controllers/clustersummary_deployer_test.go +++ b/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)) diff --git a/controllers/handlers_helm.go b/controllers/handlers_helm.go index faa83731..d79d1e6a 100644 --- a/controllers/handlers_helm.go +++ b/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 := ¤tClusterSummary.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) } } diff --git a/controllers/handlers_helm_test.go b/controllers/handlers_helm_test.go index a7af03f6..7011a0f8 100644 --- a/controllers/handlers_helm_test.go +++ b/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{ @@ -2882,3 +3080,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 +}