fix: let the fabric identity sweep run without stopping the manager

config/rbac/role.yaml

@@ -102,6 +102,7 @@ rules:
   - clusterpropagationpolicies
   verbs:
   - create
+  - delete
   - get
   - list
   - patch

internal/controller/networkfabricidentity_controller.go

@@ -426,11 +426,18 @@
 // Identified by shape rather than by name: a policy this controller owns whose
 // selectors carry no resource name is selecting by label, which only the old
 // form did.
+// It is deliberately best-effort and never returns an error. It runs as a
+// manager runnable, where an error stops the manager, and a leftover policy is
+// a correctness problem worth logging loudly where a controller that will not
+// start reconciles nothing at all.
 func (r *NetworkFabricIdentityReconciler) sweepLegacyPlacements(ctx context.Context) error {
+	log := ctrl.LoggerFrom(ctx)
+
 	var policies unstructured.UnstructuredList
 	policies.SetGroupVersionKind(clusterPropagationPolicyGVK.GroupVersion().WithKind("ClusterPropagationPolicyList"))
 	if err := r.Hub.List(ctx, &policies, client.MatchingLabels{FabricIdentityPolicyLabel: "true"}); err != nil {
-		return fmt.Errorf("read the fabric identity placement policies: %w", err)
+		log.Error(err, "could not read the fabric identity placement policies to sweep")
+		return nil
 	}
 
 	for i := range policies.Items {
@@ -439,9 +446,11 @@
 			continue
 		}
 		if err := r.Hub.Delete(ctx, policy); err != nil && !apierrors.IsNotFound(err) {
-			return fmt.Errorf("remove the legacy placement policy %q: %w", policy.GetName(), err)
+			log.Error(err, "could not remove a legacy per-location placement policy, it will keep competing for identities",
+				"policy", policy.GetName())
+			continue
 		}
-		ctrl.LoggerFrom(ctx).Info("removed a legacy per-location placement policy", "policy", policy.GetName())
+		log.Info("removed a legacy per-location placement policy", "policy", policy.GetName())
 	}
 	return nil
 }
@@ -486,7 +495,7 @@
 // +kubebuilder:rbac:groups="",resources=events,verbs=create;patch
 // +kubebuilder:rbac:groups=networking.datumapis.com,resources=networks;networkcontexts,verbs=get;list;watch
 // +kubebuilder:rbac:groups=cloud.datumapis.com,resources=networkfabricidentities,verbs=create;delete;get;list;patch;update;watch
-// +kubebuilder:rbac:groups=policy.karmada.io,resources=clusterpropagationpolicies,verbs=create;get;list;patch;update;watch
+// +kubebuilder:rbac:groups=policy.karmada.io,resources=clusterpropagationpolicies,verbs=create;delete;get;list;patch;update;watch
 
 // SetupWithManager registers the reconciler.
 //

internal/controller/networkfabricidentity_controller_test.go

@@ -31,6 +31,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -768,6 +769,50 @@ func TestPlacementSurvivesAnUnreadablePresence(t *testing.T) {
 // failingLister stands in for a control plane that cannot answer. Every list is
 // an error, which is the case a placement must never mistake for "the network
 // is required nowhere".
+// A sweep that cannot delete must not stop the manager: a controller that
+// refuses to start reconciles nothing, which is strictly worse than a leftover
+// policy it complains about.
+func TestASweepThatCannotDeleteStillStarts(t *testing.T) {
+	f := newIdentityFixture(t, "us-central-1")
+
+	legacy := &unstructured.Unstructured{Object: map[string]any{
+		"spec": map[string]any{
+			"resourceSelectors": []any{map[string]any{
+				"apiVersion":    cloudv1alpha1.GroupVersion.String(),
+				"kind":          "NetworkFabricIdentity",
+				"labelSelector": map[string]any{"matchLabels": map[string]any{LocationLabel("us-central-1"): "true"}},
+			}},
+		},
+	}}
+	legacy.SetGroupVersionKind(clusterPropagationPolicyGVK)
+	legacy.SetName("cloud-fabric-identity-us-central-1")
+	legacy.SetLabels(map[string]string{FabricIdentityPolicyLabel: "true"})
+	if err := f.hub.Create(f.ctx, legacy); err != nil {
+		t.Fatalf("create the legacy policy: %v", err)
+	}
+
+	f.reconciler.Hub = failingDeleter{Client: f.hub}
+	if err := f.reconciler.sweepLegacyPlacements(f.ctx); err != nil {
+		t.Fatalf("a sweep that cannot delete must not be an error, got %v", err)
+	}
+
+	// A hub it cannot even read is equally survivable.
+	f.reconciler.Hub = failingLister{Client: f.hub}
+	if err := f.reconciler.sweepLegacyPlacements(f.ctx); err != nil {
+		t.Fatalf("a sweep that cannot read must not be an error, got %v", err)
+	}
+}
+
+type failingDeleter struct {
+	client.Client
+}
+
+func (failingDeleter) Delete(context.Context, client.Object, ...client.DeleteOption) error {
+	return apierrors.NewForbidden(schema.GroupResource{
+		Group: "policy.karmada.io", Resource: "clusterpropagationpolicies",
+	}, "cloud-fabric-identity-us-central-1", errors.New("not permitted"))
+}
+
 type failingLister struct {
 	client.Client
 }