From dc19ab4dd5ab2118cb0f69949dc298199e8080a3 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Thu, 27 Aug 2026 16:58:55 -0500 Subject: [PATCH] fix: let the fabric identity sweep run without stopping the manager The sweep added with the per-identity placement policies deletes the per-location policies it replaced, but the federation credential could only create and patch them. The forbidden delete came back from a manager runnable, where an error stops the manager, so the controller crash-looped and nothing was reconciled at all -- worse than the leftover policies it was clearing. Grant the delete the sweep and collect both already needed, and make the sweep best-effort: a policy it cannot remove is logged loudly and the manager still starts. A controller that refuses to run reconciles nothing, where a leftover policy is a correctness problem that is at least visible and recoverable. Co-Authored-By: Claude Opus 5 (1M context) --- config/rbac/role.yaml | 1 + .../networkfabricidentity_controller.go | 17 +++++-- .../networkfabricidentity_controller_test.go | 45 +++++++++++++++++++ 3 files changed, 59 insertions(+), 4 deletions(-) diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index a1a49b5..8600ec8 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -102,6 +102,7 @@ rules: - clusterpropagationpolicies verbs: - create + - delete - get - list - patch diff --git a/internal/controller/networkfabricidentity_controller.go b/internal/controller/networkfabricidentity_controller.go index 7c35fa1..49b0e48 100644 --- a/internal/controller/networkfabricidentity_controller.go +++ b/internal/controller/networkfabricidentity_controller.go @@ -426,11 +426,18 @@ func (r *NetworkFabricIdentityReconciler) unplace(ctx context.Context, namespace // 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 @@ func (r *NetworkFabricIdentityReconciler) sweepLegacyPlacements(ctx context.Cont 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 @@ func FabricIdentityPolicyName(namespace, networkName string) string { // +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. // diff --git a/internal/controller/networkfabricidentity_controller_test.go b/internal/controller/networkfabricidentity_controller_test.go index 1175ce3..5699582 100644 --- a/internal/controller/networkfabricidentity_controller_test.go +++ b/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 }