fix: carry a network's fabric identity to every location it reaches

internal/controller/networkfabricidentity_controller.go

@@ -34,6 +34,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/log"
+	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	"sigs.k8s.io/controller-runtime/pkg/source"
 
@@ -55,11 +56,11 @@ const (
 	// the location it serves. Placement selects on it.
 	servingLocationTopologyLabel = "topology.datum.net/location"
 
-	// fabricIdentityLocationLabelPrefix marks an identity as required at one
-	// location. It follows the per-location convention the existing policies
-	// already select on, with the location in the key rather than the value: a
-	// label key holds one value, and one network is required in several
-	// locations at once.
+	// fabricIdentityLocationLabelPrefix records that an identity is required at
+	// one location, with the location in the key rather than the value: a label
+	// key holds one value, and one network is required in several locations at
+	// once. Placement no longer selects on it -- see place -- but it stays as
+	// the readable record of where an identity is needed.
 	fabricIdentityLocationLabelPrefix = "cloud.datumapis.com/location-"
 )
 
@@ -165,7 +166,7 @@ func (r *NetworkFabricIdentityReconciler) Reconcile(ctx context.Context, req ctr
 		return ctrl.Result{}, err
 	}
 
-	return ctrl.Result{}, r.placeLocations(ctx, locations)
+	return ctrl.Result{}, r.place(ctx, req.Namespace, req.Name, locations)
 }
 
 // presences reads the contexts declaring the network is required somewhere.
@@ -332,7 +333,9 @@ func (r *NetworkFabricIdentityReconciler) collect(
 	if err := r.Hub.Delete(ctx, object); err != nil && !apierrors.IsNotFound(err) {
 		return fmt.Errorf("collect the fabric identity for network %q: %w", networkName, err)
 	}
-	return nil
+	// The policy names this one identity, so nothing else can be left holding
+	// it and it would otherwise outlive the network forever.
+	return r.unplace(ctx, namespace, networkName)
 }
 
 // placeLocations keeps one policy per location, not one per network.
@@ -346,55 +349,132 @@ func (r *NetworkFabricIdentityReconciler) collect(
 //
 // Placing it fleet-wide is not an option: the identity is capability-like, and
 // what holds it can name a network's forwarding state.
-func (r *NetworkFabricIdentityReconciler) placeLocations(ctx context.Context, locations []string) error {
-	for _, location := range locations {
-		if err := r.placeLocation(ctx, location); err != nil {
-			return err
-		}
+// place carries one identity to every location it is required at, with a
+// single policy that names that one object.
+//
+// One policy per identity, not one per location: Karmada binds a resource to
+// exactly one policy, so per-location policies all selecting the same identity
+// by label compete for it, and only the winner's placement takes effect. An
+// identity required in two locations then reaches one of them, silently, which
+// is exactly the split this whole mechanism exists to prevent.
+func (r *NetworkFabricIdentityReconciler) place(
+	ctx context.Context,
+	namespace string,
+	networkName string,
+	locations []string,
+) error {
+	if len(locations) == 0 {
+		return r.unplace(ctx, namespace, networkName)
 	}
-	return nil
-}
 
-func (r *NetworkFabricIdentityReconciler) placeLocation(ctx context.Context, location string) error {
 	policy := &unstructured.Unstructured{Object: map[string]any{
 		"spec": map[string]any{
 			"conflictResolution": "Overwrite",
 			"resourceSelectors": []any{
 				map[string]any{
 					"apiVersion": cloudv1alpha1.GroupVersion.String(),
 					"kind":       "NetworkFabricIdentity",
-					"labelSelector": map[string]any{
-						"matchLabels": map[string]any{
-							LocationLabel(location): "true",
-						},
-					},
+					"namespace":  namespace,
+					"name":       networkName,
 				},
 			},
 			"placement": map[string]any{
 				"clusterAffinity": map[string]any{
 					"labelSelector": map[string]any{
-						"matchLabels": map[string]any{
-							servingLocationTopologyLabel: location,
+						"matchExpressions": []any{
+							map[string]any{
+								"key":      servingLocationTopologyLabel,
+								"operator": "In",
+								"values":   locationValues(locations),
+							},
 						},
 					},
 				},
 			},
 		},
 	}}
 	policy.SetGroupVersionKind(clusterPropagationPolicyGVK)
-	policy.SetName(FabricIdentityPolicyName(location))
+	policy.SetName(FabricIdentityPolicyName(namespace, networkName))
 	policy.SetLabels(map[string]string{FabricIdentityPolicyLabel: "true"})
 
 	if err := r.Hub.Patch(ctx, policy, client.Apply, //nolint:staticcheck // SA1019: the typed Apply API needs a generated ApplyConfiguration this unstructured policy has none of
 		client.FieldOwner(fabricIdentityFieldManager), client.ForceOwnership); err != nil {
-		return fmt.Errorf("place fabric identities for location %q: %w", location, err)
+		return fmt.Errorf("place the fabric identity for network %q: %w", networkName, err)
 	}
 	return nil
 }
 
-// FabricIdentityPolicyName names the placement for one location.
-func FabricIdentityPolicyName(location string) string {
-	return "cloud-fabric-identity-" + location
+// unplace removes the policy for an identity required nowhere. The identity
+// itself stays: the next context to appear must find the value the fabric
+// already knows the network by.
+func (r *NetworkFabricIdentityReconciler) unplace(ctx context.Context, namespace, networkName string) error {
+	policy := &unstructured.Unstructured{}
+	policy.SetGroupVersionKind(clusterPropagationPolicyGVK)
+	policy.SetName(FabricIdentityPolicyName(namespace, networkName))
+
+	if err := r.Hub.Delete(ctx, policy); err != nil && !apierrors.IsNotFound(err) {
+		return fmt.Errorf("unplace the fabric identity for network %q: %w", networkName, err)
+	}
+	return nil
+}
+
+// sweepLegacyPlacements removes the per-location policies this controller used
+// to write. They select identities by label, so they keep competing with the
+// per-identity policies that replaced them for as long as they exist, and a
+// resource Karmada binds to the wrong one reaches the wrong cells.
+//
+// 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.
+func (r *NetworkFabricIdentityReconciler) sweepLegacyPlacements(ctx context.Context) error {
+	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)
+	}
+
+	for i := range policies.Items {
+		policy := &policies.Items[i]
+		if !selectsByLabel(policy) {
+			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)
+		}
+		ctrl.LoggerFrom(ctx).Info("removed a legacy per-location placement policy", "policy", policy.GetName())
+	}
+	return nil
+}
+
+// selectsByLabel reports whether a policy selects resources without naming one.
+func selectsByLabel(policy *unstructured.Unstructured) bool {
+	selectors, found, err := unstructured.NestedSlice(policy.Object, "spec", "resourceSelectors")
+	if err != nil || !found || len(selectors) == 0 {
+		return false
+	}
+	for _, entry := range selectors {
+		selector, ok := entry.(map[string]any)
+		if !ok {
+			return false
+		}
+		if name, _, _ := unstructured.NestedString(selector, "name"); name == "" {
+			return true
+		}
+	}
+	return false
+}
+
+// locationValues is locations as the []any an unstructured policy needs.
+func locationValues(locations []string) []any {
+	values := make([]any, 0, len(locations))
+	for _, location := range locations {
+		values = append(values, location)
+	}
+	return values
+}
+
+func FabricIdentityPolicyName(namespace, networkName string) string {
+	return "cloud-fabric-identity-" + namespace + "-" + networkName
 }
 
 // The manager runs locally, so what this ServiceAccount has to be able to do is
@@ -433,6 +513,11 @@ func (r *NetworkFabricIdentityReconciler) SetupWithManager(mgr ctrl.Manager) err
 
 	hubCache := r.HubCluster.GetCache()
 
+	// Once, on the leader, before anything is placed under the new scheme.
+	if err := mgr.Add(manager.RunnableFunc(r.sweepLegacyPlacements)); err != nil {
+		return fmt.Errorf("schedule the legacy placement sweep: %w", err)
+	}
+
 	return ctrl.NewControllerManagedBy(mgr).
 		Named("networkfabricidentity").
 		WatchesRawSource(source.Kind(hubCache, &networkingv1alpha.Network{},

internal/controller/networkfabricidentity_controller_test.go

@@ -21,6 +21,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"slices"
 	"sort"
 	"strings"
 	"testing"
@@ -329,18 +330,39 @@ func (f *identityFixture) placement() ([]string, bool) {
 	return locations, len(locations) > 0
 }
 
-// policyFor reads the one policy that carries every identity required at a
-// location. There is one of these per location, not per network.
-func (f *identityFixture) policyFor(location string) (*unstructured.Unstructured, bool) {
+// policyFor reads the one policy that carries one identity to every location
+// it is required at. There is one of these per network, not per location.
+func (f *identityFixture) policyFor(networkName string) (*unstructured.Unstructured, bool) {
 	f.t.Helper()
 	policy := &unstructured.Unstructured{}
 	policy.SetGroupVersionKind(clusterPropagationPolicyGVK)
-	if err := f.hub.Get(f.ctx, client.ObjectKey{Name: FabricIdentityPolicyName(location)}, policy); err != nil {
+	key := client.ObjectKey{Name: FabricIdentityPolicyName(testNamespace, networkName)}
+	if err := f.hub.Get(f.ctx, key, policy); err != nil {
 		return nil, false
 	}
 	return policy, true
 }
 
+// placementLocations reads the locations a policy places its identity on.
+func placementLocations(t *testing.T, policy *unstructured.Unstructured) []string {
+	t.Helper()
+	expressions, _, err := unstructured.NestedSlice(policy.Object,
+		"spec", "placement", "clusterAffinity", "labelSelector", "matchExpressions")
+	if err != nil || len(expressions) != 1 {
+		t.Fatalf("expected one placement expression, got %v (%v)", expressions, err)
+	}
+	expression, _ := expressions[0].(map[string]any)
+	if key, _, _ := unstructured.NestedString(expression, "key"); key != servingLocationTopologyLabel {
+		t.Fatalf("placement must select the label a cell claims its location with, got %q", key)
+	}
+	if op, _, _ := unstructured.NestedString(expression, "operator"); op != "In" {
+		t.Fatalf("placement must match any of the locations, got operator %q", op)
+	}
+	values, _, _ := unstructured.NestedStringSlice(expression, "values")
+	sort.Strings(values)
+	return values
+}
+
 // The identity is published on a cloud object, not on the Network. Nothing a
 // consumer reads carries it.
 func TestIdentityIsPublishedOnItsOwnObject(t *testing.T) {
@@ -588,37 +610,42 @@ func newPresence(location string) *networkingv1alpha.NetworkContext {
 	return presence
 }
 
-// One policy per location, selecting every identity required there. The policy
-// count is the number of locations, not the number of networks.
-func TestOnePolicyPerLocationCarriesEveryIdentityRequiredThere(t *testing.T) {
+// One policy per identity, naming that one object and listing every location
+// it is required at.
+//
+// Karmada binds a resource to exactly one policy. Per-location policies each
+// selecting the same identity by label therefore compete for it, and only the
+// winner's placement takes effect -- an identity required in two locations
+// reaches one of them, silently. Naming the object is what makes that
+// impossible.
+func TestOnePolicyPerIdentityCarriesItToEveryLocation(t *testing.T) {
 	f := newIdentityFixture(t, "us-central-1", "us-east-1")
 	f.reconcile()
 
-	for _, location := range []string{"us-central-1", "us-east-1"} {
-		policy, ok := f.policyFor(location)
-		if !ok {
-			t.Fatalf("expected a policy for %q", location)
-		}
+	policy, ok := f.policyFor(testNetworkName)
+	if !ok {
+		t.Fatalf("expected a policy for %q", testNetworkName)
+	}
 
-		selectors, _, err := unstructured.NestedSlice(policy.Object, "spec", "resourceSelectors")
-		if err != nil || len(selectors) != 1 {
-			t.Fatalf("expected one resource selector, got %v (%v)", selectors, err)
-		}
-		entry, _ := selectors[0].(map[string]any)
-		labels, _, _ := unstructured.NestedStringMap(entry, "labelSelector", "matchLabels")
-		if labels[LocationLabel(location)] != "true" {
-			t.Fatalf("the policy for %q must select identities required there, got %v", location, labels)
-		}
+	selectors, _, err := unstructured.NestedSlice(policy.Object, "spec", "resourceSelectors")
+	if err != nil || len(selectors) != 1 {
+		t.Fatalf("expected one resource selector, got %v (%v)", selectors, err)
+	}
+	entry, _ := selectors[0].(map[string]any)
+	name, _, _ := unstructured.NestedString(entry, "name")
+	namespace, _, _ := unstructured.NestedString(entry, "namespace")
+	if name != testNetworkName || namespace != testNamespace {
+		t.Fatalf("the policy must name the one identity it carries, got %s/%s", namespace, name)
+	}
+	if _, found, _ := unstructured.NestedMap(entry, "labelSelector"); found {
+		t.Fatal("selecting by label is what lets two policies contend for one identity")
+	}
 
-		placement, _, _ := unstructured.NestedStringMap(policy.Object,
-			"spec", "placement", "clusterAffinity", "labelSelector", "matchLabels")
-		if placement[servingLocationTopologyLabel] != location {
-			t.Fatalf("the policy for %q must place on the cell serving it, got %v", location, placement)
-		}
+	if locations := placementLocations(t, policy); !slices.Equal(locations, []string{"us-central-1", "us-east-1"}) {
+		t.Fatalf("the policy must place on every location the network reaches, got %v", locations)
 	}
 
-	// A second network in the same location reuses the same policy rather than
-	// adding one.
+	// A second network needs its own policy: one identity, one policy.
 	f.addNetwork("staging", "us-central-1")
 	if _, err := f.reconciler.Reconcile(f.ctx, ctrl.Request{
 		NamespacedName: types.NamespacedName{Namespace: testNamespace, Name: "staging"},
@@ -632,7 +659,63 @@ func TestOnePolicyPerLocationCarriesEveryIdentityRequiredThere(t *testing.T) {
 		t.Fatalf("list policies: %v", err)
 	}
 	if len(policies.Items) != 2 {
-		t.Fatalf("two locations must need two policies however many networks there are, got %d", len(policies.Items))
+		t.Fatalf("two networks must have two policies however many locations they reach, got %d", len(policies.Items))
+	}
+
+	// No identity may be selected by more than one policy.
+	named := map[string]int{}
+	for i := range policies.Items {
+		selectors, _, _ := unstructured.NestedSlice(policies.Items[i].Object, "spec", "resourceSelectors")
+		for _, selector := range selectors {
+			entry, _ := selector.(map[string]any)
+			namespace, _, _ := unstructured.NestedString(entry, "namespace")
+			name, _, _ := unstructured.NestedString(entry, "name")
+			named[namespace+"/"+name]++
+		}
+	}
+	for identity, count := range named {
+		if count != 1 {
+			t.Fatalf("identity %s is selected by %d policies; Karmada honours only one", identity, count)
+		}
+	}
+}
+
+// The per-location policies this replaces select by label, so they keep
+// competing for identities until they are gone.
+func TestLegacyPerLocationPoliciesAreSweptAway(t *testing.T) {
+	f := newIdentityFixture(t, "us-central-1", "us-east-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.reconcile()
+	if err := f.reconciler.sweepLegacyPlacements(f.ctx); err != nil {
+		t.Fatalf("sweep: %v", err)
+	}
+
+	swept := &unstructured.Unstructured{}
+	swept.SetGroupVersionKind(clusterPropagationPolicyGVK)
+	err := f.hub.Get(f.ctx, client.ObjectKey{Name: "cloud-fabric-identity-us-central-1"}, swept)
+	if !apierrors.IsNotFound(err) {
+		t.Fatalf("the legacy policy must be gone, got %v", err)
+	}
+
+	// The policy written under the new scheme is left alone.
+	if _, ok := f.policyFor(testNetworkName); !ok {
+		t.Fatal("the sweep must not remove a policy that names its identity")
 	}
 }