(fix) four ClusterSummary reconcile bugs

controllers/clustersummary_controller.go

@@ -367,6 +367,18 @@ func (r *ClusterSummaryReconciler) reconcileDelete(
 			}
 		}
 
+		// Mirror the dependsOn ordering enforced on deploy: do not undeploy a prerequisite
+		// while a dependent ClusterSummary for the same cluster still exists and still
+		// requires it.
+		allRemoved, dependentMsg, err := r.areDependentsRemoved(ctx, clusterSummaryScope, logger)
+		if err != nil {
+			return reconcile.Result{Requeue: true, RequeueAfter: deleteRequeueAfter}, nil
+		}
+		clusterSummaryScope.SetDependenciesMessage(&dependentMsg)
+		if !allRemoved {
+			return reconcile.Result{Requeue: true, RequeueAfter: deleteRequeueAfter}, nil
+		}
+
 		// still call undeploy even if cluster is deleted. Sveltos might have deployed resources
 		// in the management cluster and those need to be removed.
 		err = r.undeploy(ctx, clusterSummaryScope, logger)
@@ -902,15 +914,15 @@ func (r *ClusterSummaryReconciler) isClusterPresent(ctx context.Context,
 
 	cs := clusterSummaryScope.ClusterSummary
 
-	var cluster client.Object
-	cluster, err = clusterproxy.GetCluster(ctx, r.Client, cs.Spec.ClusterNamespace, cs.Spec.ClusterName, cs.Spec.ClusterType)
+	cluster, err := clusterproxy.GetCluster(ctx, r.Client, cs.Spec.ClusterNamespace, cs.Spec.ClusterName, cs.Spec.ClusterType)
 	if err != nil {
 		if apierrors.IsNotFound(err) {
 			return false, false, nil
 		}
+		return false, false, err
 	}
 
-	return true, !cluster.GetDeletionTimestamp().IsZero(), err
+	return true, !cluster.GetDeletionTimestamp().IsZero(), nil
 }
 
 func (r *ClusterSummaryReconciler) undeploy(ctx context.Context, clusterSummaryScope *scope.ClusterSummaryScope,
@@ -1755,6 +1767,64 @@ func (r *ClusterSummaryReconciler) areDependenciesDeployed(ctx context.Context,
 	return true, dependencyMessage, nil
 }
 
+// areDependentsRemoved is the mirror check of areDependenciesDeployed, run on delete instead of
+// deploy. dependsOn is a two-way contract, not just a one-time deploy-ordering hint:
+// prepareForDeployment blocks a dependent from deploying until its prerequisites are Provisioned,
+// and symmetrically, a prerequisite (e.g. cert-manager) must not be undeployed while a dependent
+// that still needs it (e.g. kyverno) is still around - deleting it out from under a live dependent
+// would silently break that dependent, with no error at the moment that actually caused it. This
+// mirrors how Kubernetes itself protects still-referenced objects (a CRD while CRs of that type
+// exist, a PVC while a pod mounts it): the block is deliberate, and lifts once the dependent is
+// gone or no longer lists this profile as a dependency.
+//
+// Returns false, with a message, if another ClusterSummary for the same cluster still exists and
+// still lists this ClusterSummary's profile in its own DependsOn.
+func (r *ClusterSummaryReconciler) areDependentsRemoved(ctx context.Context, clusterSummaryScope *scope.ClusterSummaryScope,
+	logger logr.Logger) (allRemoved bool, dependentMessage string, err error) {
+
+	profileReference, err := configv1beta1.GetProfileOwnerReference(clusterSummaryScope.ClusterSummary)
+	if err != nil {
+		logger.V(logs.LogInfo).Info(fmt.Sprintf("failed to get profile owner: %v", err))
+		return false, "", fmt.Errorf("failed to get profile owner: %w", err)
+	}
+
+	if profileReference == nil {
+		return false, "", fmt.Errorf("profile owner not found: %w", err)
+	}
+
+	clusterSummary := clusterSummaryScope.ClusterSummary
+
+	listOptions := []client.ListOption{
+		client.InNamespace(clusterSummary.Spec.ClusterNamespace),
+		client.MatchingLabels{
+			configv1beta1.ClusterNameLabel: clusterSummary.Spec.ClusterName,
+			configv1beta1.ClusterTypeLabel: string(clusterSummary.Spec.ClusterType),
+		},
+	}
+
+	clusterSummaryList := &configv1beta1.ClusterSummaryList{}
+	if err := r.List(ctx, clusterSummaryList, listOptions...); err != nil {
+		return false, "", err
+	}
+
+	for i := range clusterSummaryList.Items {
+		other := &clusterSummaryList.Items[i]
+		if other.Name == clusterSummary.Name {
+			continue
+		}
+
+		for j := range other.Spec.ClusterProfileSpec.DependsOn {
+			if other.Spec.ClusterProfileSpec.DependsOn[j] == profileReference.Name {
+				msg := fmt.Sprintf("%s still depends on this profile and has not been removed yet", other.Name)
+				logger.V(logs.LogInfo).Info(msg)
+				return false, msg, nil
+			}
+		}
+	}
+
+	return true, "", nil
+}
+
 func (r *ClusterSummaryReconciler) setFailureMessage(clusterSummaryScope *scope.ClusterSummaryScope, failureMessage string) {
 	if clusterSummaryScope.ClusterSummary.Spec.ClusterProfileSpec.HelmCharts != nil {
 		clusterSummaryScope.SetFailureMessage(libsveltosv1beta1.FeatureHelm, &failureMessage)

controllers/clustersummary_controller_test.go

@@ -1108,6 +1108,79 @@ var _ = Describe("ClustersummaryController", func() {
 		Expect(deployed).To(BeTrue())
 	})
 
+	It("areDependentsRemoved returns false while a dependent ClusterSummary still exists", func() {
+		// clusterSummary/clusterProfile (from BeforeEach) play the role of the prerequisite
+		// (e.g. cert-manager). dependentSummary plays the dependent (e.g. kyverno): it still
+		// lists clusterProfile.Name in its own DependsOn, so the prerequisite must not be
+		// undeployed while it's still around - dependsOn protects the prerequisite from
+		// deletion, it's not just a one-time deploy-ordering hint.
+		dependentProfileName := randomString()
+		dependentSummaryName := clusterops.GetClusterSummaryName(configv1beta1.ClusterProfileKind,
+			dependentProfileName, clusterName, false)
+		dependentSummary := &configv1beta1.ClusterSummary{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      dependentSummaryName,
+				Namespace: namespace,
+				Labels: map[string]string{
+					clusterops.ClusterProfileLabelName: dependentProfileName,
+					configv1beta1.ClusterNameLabel:     clusterName,
+					configv1beta1.ClusterTypeLabel:     string(libsveltosv1beta1.ClusterTypeCapi),
+				},
+			},
+			Spec: configv1beta1.ClusterSummarySpec{
+				ClusterNamespace: cluster.Namespace,
+				ClusterName:      cluster.Name,
+				ClusterType:      libsveltosv1beta1.ClusterTypeCapi,
+				ClusterProfileSpec: configv1beta1.Spec{
+					DependsOn: []string{clusterProfile.Name},
+				},
+			},
+		}
+
+		initObjects := []client.Object{
+			dependentSummary,
+			clusterSummary,
+			clusterProfile,
+		}
+
+		c := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(initObjects...).WithObjects(initObjects...).Build()
+
+		addOwnerReference(context.TODO(), c, clusterSummary, clusterProfile)
+
+		deployer := fakedeployer.GetClient(context.TODO(), textlogger.NewLogger(textlogger.NewConfig()), c)
+		reconciler := &controllers.ClusterSummaryReconciler{
+			Client:       c,
+			Scheme:       scheme,
+			Deployer:     deployer,
+			ClusterMap:   make(map[corev1.ObjectReference]*libsveltosset.Set),
+			ReferenceMap: make(map[corev1.ObjectReference]*libsveltosset.Set),
+			PolicyMux:    sync.Mutex{},
+		}
+
+		clusterSummaryScope, err := scope.NewClusterSummaryScope(&scope.ClusterSummaryScopeParams{
+			Client:         c,
+			Logger:         textlogger.NewLogger(textlogger.NewConfig()),
+			ClusterSummary: clusterSummary,
+			ControllerName: testControllerNameSummary,
+		})
+		Expect(err).To(BeNil())
+
+		// because the dependent ClusterSummary still exists and still depends on this profile
+		removed, msg, err := controllers.AreDependentsRemoved(reconciler, context.TODO(), clusterSummaryScope,
+			textlogger.NewLogger(textlogger.NewConfig()))
+		Expect(err).To(BeNil())
+		Expect(removed).To(BeFalse())
+		Expect(msg).To(ContainSubstring(dependentSummaryName))
+
+		// once the dependent ClusterSummary is gone, the prerequisite is free to undeploy
+		Expect(c.Delete(context.TODO(), dependentSummary)).To(Succeed())
+
+		removed, _, err = controllers.AreDependentsRemoved(reconciler, context.TODO(), clusterSummaryScope,
+			textlogger.NewLogger(textlogger.NewConfig()))
+		Expect(err).To(BeNil())
+		Expect(removed).To(BeTrue())
+	})
+
 	It("processUndeployError requeues with deleteRequeueAfter and no error for WaitForProfileProcessingError", func() {
 		initObjects := []client.Object{
 			clusterProfile,

controllers/export_test.go

@@ -70,6 +70,7 @@ var (
 	CanRemoveFinalizer                   = (*ClusterSummaryReconciler).canRemoveFinalizer
 	ReconcileDelete                      = (*ClusterSummaryReconciler).reconcileDelete
 	AreDependenciesDeployed              = (*ClusterSummaryReconciler).areDependenciesDeployed
+	AreDependentsRemoved                 = (*ClusterSummaryReconciler).areDependentsRemoved
 	SetFailureMessage                    = (*ClusterSummaryReconciler).setFailureMessage
 	ResetFeatureStatus                   = (*ClusterSummaryReconciler).resetFeatureStatus
 
@@ -152,6 +153,7 @@ var (
 	GetInstantiatedChart                     = getInstantiatedChart
 	GetHelmChartValuesFrom                   = getHelmChartValuesFrom
 	GetHelmChartInstantiatedValues           = getHelmChartInstantiatedValues
+	LocateChartWithTimeout                   = locateChartWithTimeout
 
 	InstantiateTemplateValues = instantiateTemplateValues
 	FetchClusterObjects       = fetchClusterObjects

controllers/handlers_helm.go

@@ -2052,6 +2052,40 @@ func installRelease(ctx context.Context, clusterSummary *configv1beta1.ClusterSu
 	return rel, nil
 }
 
+const (
+	// locateChartTimeout bounds a single LocateChart call. Like repo.ChartRepository.DownloadIndexFile,
+	// action.Install/Upgrade's LocateChart takes no context.Context - it downloads the chart tarball with
+	// its own HTTP client, so nothing here can cancel a stalled call. Without this timeout, a repository
+	// that accepts the connection but never completes the response wedges the deployer worker processing
+	// this request forever: the request only leaves the deployer's inProgress tracking once the handler
+	// call returns, so a call that never returns permanently occupies one of the worker pool's fixed
+	// slots and its ClusterSummary/feature is stuck reporting "still being provisioned" with no further
+	// retries, indefinitely.
+	locateChartTimeout = 90 * time.Second
+)
+
+func locateChartWithTimeout(locateFn func(string, *cli.EnvSettings) (string, error),
+	chartName string, settings *cli.EnvSettings) (string, error) {
+
+	type locateResult struct {
+		path string
+		err  error
+	}
+
+	done := make(chan locateResult, 1)
+	go func() {
+		cp, err := locateFn(chartName, settings)
+		done <- locateResult{cp, err}
+	}()
+
+	select {
+	case r := <-done:
+		return r.path, r.err
+	case <-time.After(locateChartTimeout):
+		return "", fmt.Errorf("timed out locating chart %s", chartName)
+	}
+}
+
 // locateChartWithCacheRetry calls locateFn to resolve the chart path. If the call fails (e.g.
 // because the local repository index is stale and does not yet contain a newly published version),
 // the cache is cleared and the call is retried once so that the current reconciliation can succeed
@@ -2062,12 +2096,12 @@ func locateChartWithCacheRetry(
 	requestedChart *configv1beta1.HelmChart, registryOptions *registryClientOptions,
 	logger logr.Logger) (string, error) {
 
-	cp, err := locateFn(chartName, settings)
+	cp, err := locateChartWithTimeout(locateFn, chartName, settings)
 	if err != nil {
 		logger.V(logs.LogInfo).Info(fmt.Sprintf("LocateChart failed: %v; refreshing cache and retrying", err))
 		removeCachedData(settings, requestedChart.RepositoryName, requestedChart.RepositoryURL,
 			registryOptions, logger)
-		cp, err = locateFn(chartName, settings)
+		cp, err = locateChartWithTimeout(locateFn, chartName, settings)
 	}
 	return cp, err
 }

controllers/handlers_helm_test.go

@@ -32,6 +32,7 @@ import (
 	. "github.com/onsi/gomega"
 
 	"helm.sh/helm/v4/pkg/action"
+	"helm.sh/helm/v4/pkg/cli"
 	releasecommon "helm.sh/helm/v4/pkg/release/common"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -2755,3 +2756,26 @@ var _ = Describe("getHelmUpgradeClient Force", func() {
 		Expect(upgradeClient.ForceConflicts).To(BeTrue())
 	})
 })
+
+var _ = Describe("locateChartWithTimeout", func() {
+	It("returns the result once locateFn returns", func() {
+		expectedPath := randomString()
+		locateFn := func(chartName string, settings *cli.EnvSettings) (string, error) {
+			return expectedPath, nil
+		}
+
+		cp, err := controllers.LocateChartWithTimeout(locateFn, randomString(), &cli.EnvSettings{})
+		Expect(err).To(BeNil())
+		Expect(cp).To(Equal(expectedPath))
+	})
+
+	It("propagates the error returned by locateFn", func() {
+		expectedErr := errors.New(randomString())
+		locateFn := func(chartName string, settings *cli.EnvSettings) (string, error) {
+			return "", expectedErr
+		}
+
+		_, err := controllers.LocateChartWithTimeout(locateFn, randomString(), &cli.EnvSettings{})
+		Expect(err).To(Equal(expectedErr))
+	})
+})

controllers/utils.go

@@ -458,14 +458,27 @@ func removeStaleResourceSummary(ctx context.Context, clusterNamespace, clusterNa
 		return err
 	}
 
+	// rsListOptions only scopes by cluster name/type, not namespace - client-go label
+	// selectors can't match on ClusterSummary namespace, so multiple clusters sharing the
+	// same name across different namespaces (a normal setup) all come back in one List.
+	// matchingCount tracks only the ones that are ours to wait on: known to belong to
+	// clusterNamespace, or - conservatively - ones whose namespace we can't determine at
+	// all. That keeps an unrelated cluster's still-finalizing ResourceSummary (a namespace
+	// we can positively rule out) from blocking this one's cleanup, without letting an
+	// unidentifiable ResourceSummary be ignored outright.
+	matchingCount := 0
 	for i := range resourceSummaries.Items {
 		rs := &resourceSummaries.Items[i]
 
 		ns, ok := getClusterSummaryNamespaceFromResourceSummary(rs, logger)
-		if !ok {
+		if ok && ns != clusterNamespace {
 			continue
 		}
-		if ns != clusterNamespace {
+		matchingCount++
+
+		if !ok {
+			// Can't tell which cluster this belongs to, so don't risk deleting it - just
+			// count it, so the caller waits rather than proceeding as if it weren't there.
 			continue
 		}
 
@@ -477,7 +490,7 @@ func removeStaleResourceSummary(ctx context.Context, clusterNamespace, clusterNa
 		}
 	}
 
-	if len(resourceSummaries.Items) > 0 {
+	if matchingCount > 0 {
 		// The drift-detection-manager adds a finalizer to ResourceSummary instances and removes
 		// it only after a successful deletion. If ResourceSummary instances still exist, we return
 		// an error to prevent the drift-detection-manager deployment from being deleted, ensuring