From b69db55b1f2fa6cc1379304a80714e34b94c60ed Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Thu, 27 Aug 2026 16:37:01 -0500 Subject: [PATCH] fix: carry a network's fabric identity to every location it reaches An identity required in two locations reached only one of them, so the two halves of one network were two networks on the fabric -- the exact split deriving the VPC identifier from a shared identity exists to prevent. Placement wrote one ClusterPropagationPolicy per location, each selecting identities by a location label. An identity needed in several locations carries several of those labels, so several policies select it. Karmada binds a resource to exactly one policy, so only the winner's placement took effect and the rest were silently ignored. Each identity now gets one policy that names it and lists every location it is required at, so no two policies can contend for one object. The per-location policies are swept on startup, identified by selecting without naming a resource, because they keep competing for as long as they exist. Co-Authored-By: Claude Opus 5 (1M context) --- .../networkfabricidentity_controller.go | 139 +++++++++++++---- .../networkfabricidentity_controller_test.go | 141 ++++++++++++++---- 2 files changed, 224 insertions(+), 56 deletions(-) diff --git a/internal/controller/networkfabricidentity_controller.go b/internal/controller/networkfabricidentity_controller.go index 9d7897a..7c35fa1 100644 --- a/internal/controller/networkfabricidentity_controller.go +++ b/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,16 +349,24 @@ 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", @@ -363,18 +374,19 @@ func (r *NetworkFabricIdentityReconciler) placeLocation(ctx context.Context, loc 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), + }, }, }, }, @@ -382,19 +394,87 @@ func (r *NetworkFabricIdentityReconciler) placeLocation(ctx context.Context, loc }, }} 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{}, diff --git a/internal/controller/networkfabricidentity_controller_test.go b/internal/controller/networkfabricidentity_controller_test.go index 387b213..1175ce3 100644 --- a/internal/controller/networkfabricidentity_controller_test.go +++ b/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") } }