From 8ac0c43bdc9643a086a6aca8bcd31197b3439b48 Mon Sep 17 00:00:00 2001 From: Viktor Voytovych Date: Wed, 11 Nov 2026 18:34:45 +0200 Subject: [PATCH 1/4] test(fabricidentity): Reproduce a gone network holding its quota unit The fake address service now models the two things a released claim meets: the platform project's quota on claims, refused before the address service runs, and Retain on delete, which keeps the allocation under the claim's name. A gone network's claim then holds the only quota unit, and the next network is refused, as the platform refuses it. --- .../networkfabricidentity_controller_test.go | 72 +++++++++++--- .../networkfabricidentity_quota_test.go | 93 +++++++++++++++++++ 2 files changed, 150 insertions(+), 15 deletions(-) create mode 100644 internal/cloud/controller/networkfabricidentity_quota_test.go diff --git a/internal/cloud/controller/networkfabricidentity_controller_test.go b/internal/cloud/controller/networkfabricidentity_controller_test.go index b90f90fc..61d9bae2 100644 --- a/internal/cloud/controller/networkfabricidentity_controller_test.go +++ b/internal/cloud/controller/networkfabricidentity_controller_test.go @@ -65,6 +65,10 @@ type fakeIdentityIPAM struct { // claim was deleted under Retain. It is what makes a second claim of the // same name a conflict rather than a fresh allocation. retained map[string]string + // limit is the platform project's quota on claims, when set. Quota + // admission counts live claims and refuses a create before the address + // service sees it, so a retained allocation holds no unit. + limit int } func allocationNameFor(claimName string) string { return "alloc-" + claimName } @@ -114,6 +118,9 @@ func newFakeIdentityIPAM(t *testing.T) *fakeIdentityIPAM { } f.created = append(f.created, ipClaim.Name) + if err := f.refuseOverQuota(ctx, c, ipClaim); err != nil { + return err + } if err := refuseWhatTheServerWouldRefuse(ipClaim); err != nil { return err } @@ -134,10 +141,55 @@ func newFakeIdentityIPAM(t *testing.T) *fakeIdentityIPAM { ipClaim.Status.AllocatedCIDR = blockForIndex(index) return c.Create(ctx, obj, opts...) }, + Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + if _, ok := obj.(*ipamv1alpha1.IPClaim); !ok { + return c.Delete(ctx, obj, opts...) + } + return f.retain(ctx, c, obj.GetName(), opts...) + }, }).Build() return f } +// refuseOverQuota mirrors the platform's quota admission on claims: a create +// that would pass the limit is refused before the address service runs. +func (f *fakeIdentityIPAM) refuseOverQuota(ctx context.Context, c client.WithWatch, ipClaim *ipamv1alpha1.IPClaim) error { + if f.limit == 0 { + return nil + } + var live ipamv1alpha1.IPClaimList + if err := c.List(ctx, &live, client.InNamespace(ipClaim.Namespace)); err != nil { + return err + } + if len(live.Items) >= f.limit { + return apierrors.NewForbidden(ipamv1alpha1.SchemeGroupVersion.WithResource("ipclaims").GroupResource(), + ipClaim.Name, fmt.Errorf("You've reached your quota of %d", f.limit)) + } + return nil +} + +// retain deletes a claim the way the address service does under Retain: the +// claim goes and its allocation stays, holding the block under the claim's +// name so that no other claim is ever handed it. +func (f *fakeIdentityIPAM) retain(ctx context.Context, c client.WithWatch, claimName string, opts ...client.DeleteOption) error { + var ipClaim ipamv1alpha1.IPClaim + if err := c.Get(ctx, client.ObjectKey{Namespace: "default", Name: claimName}, &ipClaim); err != nil { + return err + } + allocation := &ipamv1alpha1.IPAllocation{} + allocation.Namespace = "default" + allocation.Name = allocationNameFor(claimName) + allocation.Status.AllocatedCIDR = ipClaim.Status.AllocatedCIDR + if err := c.Create(ctx, allocation); err != nil { + return err + } + if err := c.Delete(ctx, &ipClaim, opts...); err != nil { + return err + } + f.retained[allocation.Name] = ipClaim.Status.AllocatedCIDR + return nil +} + // blockForIndex renders the block a pool rooted at fc00::/32 hands out for an // index, which is what the identity is read back out of. func blockForIndex(index int) string { @@ -161,22 +213,12 @@ func hex16(v int) string { // stays, and the block it holds becomes readable through the allocation. func (f *fakeIdentityIPAM) release(t *testing.T, ctx context.Context, claimName string) { t.Helper() - var ipClaim ipamv1alpha1.IPClaim - if err := f.client.Get(ctx, client.ObjectKey{Namespace: "default", Name: claimName}, &ipClaim); err != nil { - t.Fatalf("read the claim being released: %v", err) + ipClaim := &ipamv1alpha1.IPClaim{} + ipClaim.Namespace = "default" + ipClaim.Name = claimName + if err := f.client.Delete(ctx, ipClaim); err != nil { + t.Fatalf("release the claim: %v", err) } - - allocation := &ipamv1alpha1.IPAllocation{} - allocation.Namespace = "default" - allocation.Name = allocationNameFor(claimName) - allocation.Status.AllocatedCIDR = ipClaim.Status.AllocatedCIDR - if err := f.client.Create(ctx, allocation); err != nil { - t.Fatalf("retain the allocation: %v", err) - } - if err := f.client.Delete(ctx, &ipClaim); err != nil { - t.Fatalf("delete the claim: %v", err) - } - f.retained[allocation.Name] = ipClaim.Status.AllocatedCIDR } func (f *fakeIdentityIPAM) ClientForPlatform() (client.Client, error) { return f.client, nil } diff --git a/internal/cloud/controller/networkfabricidentity_quota_test.go b/internal/cloud/controller/networkfabricidentity_quota_test.go new file mode 100644 index 00000000..b04022b7 --- /dev/null +++ b/internal/cloud/controller/networkfabricidentity_quota_test.go @@ -0,0 +1,93 @@ +/* +Copyright © 2026 Datum Technology, Inc. All rights reserved. + +This program is free software: you can redistribute it and/or modify +it under the terms of the GNU Affero General Public License as +published by the Free Software Foundation, either version 3 of the +License, or (at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU Affero General Public License for more details. + +You should have received a copy of the GNU Affero General Public License +along with this program. If not, see . +*/ + +package controller + +import ( + "testing" + + ipamv1alpha1 "go.miloapis.com/ipam/pkg/apis/ipam/v1alpha1" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// Every network claims its identity in the platform's one project, under that +// project's quota on claims. A claim kept after its network is gone holds a +// unit forever, so the quota can only fill, and then no new network anywhere +// gets an identity. +func TestAGoneNetworkGivesItsQuotaUnitBack(t *testing.T) { + f := newIdentityFixture(t, "us-central-1") + f.ipam.limit = 1 + f.reconcile() + + f.deleteNetwork() + f.reconcile() + + f.addNetwork("next", "us-central-1") + if err := f.reconcileNamed("next"); err != nil { + t.Fatalf("a new network must get an identity once the old one is gone: %v", err) + } + if n := f.liveClaims(); n != 1 { + t.Fatalf("only the new network may hold a claim, got %d", n) + } +} + +// Giving the quota unit back must not give the identity back. The block stays +// retained under the network's name, so a network of that name returns to the +// same identity and no other network can ever be handed it. +func TestAReturningNetworkAdoptsTheBlockItGaveUp(t *testing.T) { + f := newIdentityFixture(t, "us-central-1") + f.ipam.limit = 1 + f.reconcile() + + before, ok := f.published() + if !ok { + t.Fatal("expected an identity to start from") + } + + f.deleteNetwork() + f.reconcile() + f.createNetwork() + f.reconcile() + + after, ok := f.published() + if !ok { + t.Fatal("the identity must be republished when the network comes back") + } + if after.Spec.Identity != before.Spec.Identity { + t.Fatalf("a returning network must adopt its retained block, got %d then %d", + before.Spec.Identity, after.Spec.Identity) + } +} + +func (f *identityFixture) reconcileNamed(name string) error { + f.t.Helper() + _, err := f.reconciler.Reconcile(f.ctx, ctrl.Request{ + NamespacedName: types.NamespacedName{Namespace: testNamespace, Name: name}, + }) + return err +} + +func (f *identityFixture) liveClaims() int { + f.t.Helper() + var live ipamv1alpha1.IPClaimList + if err := f.ipam.client.List(f.ctx, &live, client.InNamespace("default")); err != nil { + f.t.Fatalf("list the claims: %v", err) + } + return len(live.Items) +} From a6c9cff752a187d52c42a7fb9a3ac1b8924eeb99 Mon Sep 17 00:00:00 2001 From: Viktor Voytovych Date: Wed, 11 Nov 2026 18:37:49 +0200 Subject: [PATCH 2/4] fix(fabricidentity): Release a gone network's claim Every network claims its identity in the platform's one project, under that project's quota on claims. collect() removed the identity object but kept the claim, so each gone network held a quota unit forever and the quota could only fill; then no new network anywhere got an identity. collect() now releases the claim after unplacing the identity. Under Retain the address service keeps the allocation, so the block stays held under the network's name: it is never reissued, and a network of the same name that returns adopts it. A claim already gone counts as released; a refused release is an error, so the collection is retried. The platform identity needs delete on ipclaims. --- .../networkfabricidentity_controller.go | 31 ++++++++++++--- internal/cloud/fabricidentity/identity.go | 17 ++++++++ .../cloud/fabricidentity/identity_test.go | 39 +++++++++++++++++++ 3 files changed, 81 insertions(+), 6 deletions(-) diff --git a/internal/cloud/controller/networkfabricidentity_controller.go b/internal/cloud/controller/networkfabricidentity_controller.go index fc87309b..a74f444b 100644 --- a/internal/cloud/controller/networkfabricidentity_controller.go +++ b/internal/cloud/controller/networkfabricidentity_controller.go @@ -323,11 +323,11 @@ func (r *NetworkFabricIdentityReconciler) claim( // collect removes the published identity once the network it belongs to is // gone. // -// The IPAM claim is deliberately not released. A Route Target still installed -// in a remote location's import policy would silently merge a new network into -// a dead one's routes, so the allocation is retained forever and the identity -// is never reissued. Only the object goes; the per-location policies are shared -// and stay. +// The claim is released last, and only its quota unit goes with it: under +// Retain the allocation stays held under the network's name, so the identity +// is never reissued. A Route Target still installed in a remote location's +// import policy can then never merge a new network into a dead one's routes. +// The per-location policies are shared and stay. func (r *NetworkFabricIdentityReconciler) collect( ctx context.Context, namespace string, @@ -342,7 +342,26 @@ func (r *NetworkFabricIdentityReconciler) collect( } // 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) + if err := r.unplace(ctx, namespace, networkName); err != nil { + return err + } + // Every network claims in the platform's one project, under its quota, so + // a claim kept after its network would hold a unit until the quota is full + // and no new network anywhere gets an identity. + return r.release(ctx, namespace, networkName) +} + +// release gives the gone network's claim back to the platform's quota. +func (r *NetworkFabricIdentityReconciler) release(ctx context.Context, namespace, networkName string) error { + ipamClient, err := r.IPAM.ClientForPlatform() + if err != nil { + return fmt.Errorf("reach the address service to release network %q: %w", networkName, err) + } + return fabricidentity.Release(ctx, ipamClient, fabricidentity.Request{ + Namespace: r.IdentityNamespace, + NetworkNamespace: namespace, + NetworkName: networkName, + }) } // placeLocations keeps one policy per location, not one per network. diff --git a/internal/cloud/fabricidentity/identity.go b/internal/cloud/fabricidentity/identity.go index 9a8b5e98..20c08169 100644 --- a/internal/cloud/fabricidentity/identity.go +++ b/internal/cloud/fabricidentity/identity.go @@ -168,6 +168,23 @@ func Claim(ctx context.Context, ipamClient client.Client, request Request) (int6 return FromBlock(ipClaim.Status.AllocatedCIDR) } +// Release gives a gone network's claim back. +// +// Under Retain, the address service keeps the claim's allocation, so the block +// stays held under the network's name and is never handed to another network. +// What goes is the claim, and with it the quota unit it held in the platform's +// one project. A network of the same name that returns adopts the block it gave +// up. A claim already gone is released. +func Release(ctx context.Context, ipamClient client.Client, request Request) error { + ipClaim := &ipamv1alpha1.IPClaim{} + ipClaim.Namespace = request.Namespace + ipClaim.Name = ClaimName(request.NetworkNamespace, request.NetworkName) + if err := ipamClient.Delete(ctx, ipClaim); err != nil && !apierrors.IsNotFound(err) { + return fmt.Errorf("release the identity claim %q: %w", ipClaim.Name, err) + } + return nil +} + // adopt reads the identity out of an allocation this network already holds. // The allocation outlives the claim that made it, which is what retention is // for, so the block it names is the same one this network has always had. diff --git a/internal/cloud/fabricidentity/identity_test.go b/internal/cloud/fabricidentity/identity_test.go index 395c5068..be51a4be 100644 --- a/internal/cloud/fabricidentity/identity_test.go +++ b/internal/cloud/fabricidentity/identity_test.go @@ -226,3 +226,42 @@ func admit(ipClaim *ipamv1alpha1.IPClaim) error { } return nil } + +// A claim already gone has nothing left to give back, and a collection that +// retries after a crash must not fail on it. +func TestReleasingAClaimAlreadyGoneIsReleased(t *testing.T) { + scheme := runtime.NewScheme() + if err := ipamv1alpha1.AddToScheme(scheme); err != nil { + t.Fatalf("build the IPAM scheme: %v", err) + } + ipamClient := fake.NewClientBuilder().WithScheme(scheme).Build() + + err := Release(context.Background(), ipamClient, Request{ + Namespace: "default", NetworkNamespace: "ns-project", NetworkName: "prod", + }) + if err != nil { + t.Fatalf("a claim already gone must count as released: %v", err) + } +} + +// A refused release is an error, so the collection is retried rather than +// leaving the quota unit held without a word. +func TestARefusedReleaseIsAnError(t *testing.T) { + scheme := runtime.NewScheme() + if err := ipamv1alpha1.AddToScheme(scheme); err != nil { + t.Fatalf("build the IPAM scheme: %v", err) + } + ipamClient := fake.NewClientBuilder().WithScheme(scheme).WithInterceptorFuncs(interceptor.Funcs{ + Delete: func(context.Context, client.WithWatch, client.Object, ...client.DeleteOption) error { + return apierrors.NewForbidden(ipamv1alpha1.SchemeGroupVersion.WithResource("ipclaims").GroupResource(), + "fabric-identity.ns-project.prod", errors.New("delete is not granted")) + }, + }).Build() + + err := Release(context.Background(), ipamClient, Request{ + Namespace: "default", NetworkNamespace: "ns-project", NetworkName: "prod", + }) + if !apierrors.IsForbidden(err) { + t.Fatalf("a refused release must be returned as the refusal, got %v", err) + } +} From 480b7414152dd19cdfdda8c576f88e9e84674e2c Mon Sep 17 00:00:00 2001 From: Viktor Voytovych Date: Wed, 11 Nov 2026 19:11:16 +0200 Subject: [PATCH 3/4] fix(fabricidentity): Keep a live network's claim on a stale read The network is read from the hub's cache. A network deleted and recreated under one name can still read as gone there while it lives; releasing its claim then would leave its block held with no claim, uncounted by the quota for as long as it lives. release() now confirms the network is gone with a read past the cache, keeps the claim if it lives, and fails the reconcile if that read fails. Also covered: a release refused once is retried by the next reconcile of the same network, although the identity it starts from is already gone. --- cmd/galactic-controller/fabric.go | 2 +- .../networkfabricidentity_controller.go | 17 ++++ .../networkfabricidentity_controller_test.go | 7 ++ .../networkfabricidentity_quota_test.go | 77 +++++++++++++++++++ 4 files changed, 102 insertions(+), 1 deletion(-) diff --git a/cmd/galactic-controller/fabric.go b/cmd/galactic-controller/fabric.go index 1a9105b9..b74fda0f 100644 --- a/cmd/galactic-controller/fabric.go +++ b/cmd/galactic-controller/fabric.go @@ -86,7 +86,7 @@ func runFabric(parent context.Context, o *options) error { return fmt.Errorf("add federation hub cache: %w", err) } if err := (&cloudcontroller.NetworkFabricIdentityReconciler{ - Networks: hub.GetClient(), Hub: hub.GetClient(), HubCluster: hub, + Networks: hub.GetClient(), NetworkReader: hub.GetAPIReader(), Hub: hub.GetClient(), HubCluster: hub, IPAM: ipamClients, IdentityClass: o.identityClass, IdentityNamespace: o.identityNamespace, }).SetupWithManager(mgr); err != nil { return fmt.Errorf("set up fabric identity controller: %w", err) diff --git a/internal/cloud/controller/networkfabricidentity_controller.go b/internal/cloud/controller/networkfabricidentity_controller.go index a74f444b..8b142f13 100644 --- a/internal/cloud/controller/networkfabricidentity_controller.go +++ b/internal/cloud/controller/networkfabricidentity_controller.go @@ -104,6 +104,11 @@ type NetworkFabricIdentityReconciler struct { // withdrawal in this controller is conditioned on a read having succeeded. Networks client.Client + // NetworkReader reads a network past any cache. Networks is served from the + // hub's cache, where a network deleted and recreated under one name can still + // read as gone; a claim is released only once this read agrees. + NetworkReader client.Reader + // Hub is where the identity and its placement are written, and from where // federation carries them to the cells. Hub client.Client @@ -353,6 +358,15 @@ func (r *NetworkFabricIdentityReconciler) collect( // release gives the gone network's claim back to the platform's quota. func (r *NetworkFabricIdentityReconciler) release(ctx context.Context, namespace, networkName string) error { + // A live network keeps its claim, whatever the cache said: released, its + // block would stay held with no claim, uncounted by the quota while it lives. + err := r.NetworkReader.Get(ctx, client.ObjectKey{Namespace: namespace, Name: networkName}, &networkingv1alpha.Network{}) + if err == nil { + return nil + } + if !apierrors.IsNotFound(err) { + return fmt.Errorf("confirm network %q is gone before releasing its claim: %w", networkName, err) + } ipamClient, err := r.IPAM.ClientForPlatform() if err != nil { return fmt.Errorf("reach the address service to release network %q: %w", networkName, err) @@ -545,6 +559,9 @@ func (r *NetworkFabricIdentityReconciler) SetupWithManager(mgr ctrl.Manager) err if r.HubCluster == nil { return errors.New("a hub to watch is required") } + if r.NetworkReader == nil { + return errors.New("a direct network reader is required") + } hubCache := r.HubCluster.GetCache() diff --git a/internal/cloud/controller/networkfabricidentity_controller_test.go b/internal/cloud/controller/networkfabricidentity_controller_test.go index 61d9bae2..01168e81 100644 --- a/internal/cloud/controller/networkfabricidentity_controller_test.go +++ b/internal/cloud/controller/networkfabricidentity_controller_test.go @@ -69,6 +69,8 @@ type fakeIdentityIPAM struct { // admission counts live claims and refuses a create before the address // service sees it, so a retained allocation holds no unit. limit int + // refuseDelete stands in for an identity not yet granted delete on claims. + refuseDelete bool } func allocationNameFor(claimName string) string { return "alloc-" + claimName } @@ -145,6 +147,10 @@ func newFakeIdentityIPAM(t *testing.T) *fakeIdentityIPAM { if _, ok := obj.(*ipamv1alpha1.IPClaim); !ok { return c.Delete(ctx, obj, opts...) } + if f.refuseDelete { + return apierrors.NewForbidden(ipamv1alpha1.SchemeGroupVersion.WithResource("ipclaims").GroupResource(), + obj.GetName(), errors.New("delete is not granted")) + } return f.retain(ctx, c, obj.GetName(), opts...) }, }).Build() @@ -278,6 +284,7 @@ func newIdentityFixture(t *testing.T, presences ...string) *identityFixture { ipam: ipam, reconciler: &NetworkFabricIdentityReconciler{ Networks: networks, + NetworkReader: networks, Hub: hub, IPAM: ipam, IdentityClass: testClass, diff --git a/internal/cloud/controller/networkfabricidentity_quota_test.go b/internal/cloud/controller/networkfabricidentity_quota_test.go index b04022b7..abbea8e0 100644 --- a/internal/cloud/controller/networkfabricidentity_quota_test.go +++ b/internal/cloud/controller/networkfabricidentity_quota_test.go @@ -18,9 +18,12 @@ along with this program. If not, see . package controller import ( + "context" "testing" ipamv1alpha1 "go.miloapis.com/ipam/pkg/apis/ipam/v1alpha1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -91,3 +94,77 @@ func (f *identityFixture) liveClaims() int { } return len(live.Items) } + +// A release refused once, by an identity not yet granted delete on claims, is +// retried by the next reconcile of the same network, although the identity it +// would start from is already gone. +func TestARefusedReleaseIsRetriedByTheNextReconcile(t *testing.T) { + f := newIdentityFixture(t, "us-central-1") + f.reconcile() + + f.deleteNetwork() + f.ipam.refuseDelete = true + if err := f.reconcileNamed(testNetworkName); err == nil { + t.Fatal("a refused release must fail the reconcile, so it is retried") + } + if _, ok := f.published(); ok { + t.Fatal("the identity is collected before the release is tried") + } + + f.ipam.refuseDelete = false + if err := f.reconcileNamed(testNetworkName); err != nil { + t.Fatalf("the retry must release the claim: %v", err) + } + if n := f.liveClaims(); n != 0 { + t.Fatalf("the retry must leave no claim, got %d", n) + } +} + +// The network is read from the hub's cache. A network deleted and recreated +// under the same name can still read as gone there, while it lives. Its claim +// must not be released on that read, or the network would hold its block with +// no claim, uncounted by the quota for as long as it lives. +func TestALiveNetworkThatReadsGoneKeepsItsClaim(t *testing.T) { + f := newIdentityFixture(t, "us-central-1") + f.reconcile() + + f.reconciler.Networks = staleGetter{Client: f.networks} + if err := f.reconcileNamed(testNetworkName); err != nil { + t.Fatalf("reconcile: %v", err) + } + if n := f.liveClaims(); n != 1 { + t.Fatalf("a network that lives must keep its claim, got %d claims", n) + } + + f.reconciler.Networks = f.networks + f.reconcile() + if _, ok := f.published(); !ok { + t.Fatal("the next reconcile must publish the identity again") + } +} + +// staleGetter stands in for a cache that has not seen a network come back: +// every network reads as gone. +type staleGetter struct { + client.Client +} + +func (staleGetter) Get(_ context.Context, key client.ObjectKey, _ client.Object, _ ...client.GetOption) error { + return apierrors.NewNotFound(schema.GroupResource{Group: "networking.datumapis.com", Resource: "networks"}, key.Name) +} + +// A direct read that fails cannot confirm the network is gone, so the claim is +// kept and the reconcile fails, to be retried. +func TestAnUnconfirmedGoneNetworkKeepsItsClaim(t *testing.T) { + f := newIdentityFixture(t, "us-central-1") + f.reconcile() + + f.reconciler.Networks = staleGetter{Client: f.networks} + f.reconciler.NetworkReader = failingGetter{Client: f.networks} + if err := f.reconcileNamed(testNetworkName); err == nil { + t.Fatal("a release that cannot confirm the network is gone must fail the reconcile") + } + if n := f.liveClaims(); n != 1 { + t.Fatalf("an unconfirmed release must keep the claim, got %d claims", n) + } +} From 6e1c64f830b030c0de5acd4eeea97819aa706e50 Mon Sep 17 00:00:00 2001 From: Viktor Voytovych Date: Wed, 11 Nov 2026 20:52:14 +0200 Subject: [PATCH 4/4] fix(fabricidentity): Release only a retained claim, collect only a confirmed gone network The block is kept from reissue by the policy the address service recorded on the allocation when the claim bound, not by the claim's own policy, which can change. Release now reads the claim and its bound allocation, and deletes the claim, conditioned on its UID, only when the allocation reads Retain. Otherwise it keeps the claim and returns an error that names the allocation and its policy. The read past the cache now opens collect(), so a network the cache reads as gone while it lives keeps its identity and its placement as well as its claim. The fake address service now binds an allocation on create, records the claim's policy or a class default, and acts on the recorded policy at delete, as the address service does. --- .../networkfabricidentity_controller.go | 32 ++++++++---- .../networkfabricidentity_controller_test.go | 46 ++++++++++++---- .../networkfabricidentity_quota_test.go | 40 ++++++++++++++ internal/cloud/fabricidentity/identity.go | 42 +++++++++++++-- .../cloud/fabricidentity/identity_test.go | 52 ++++++++++++++++--- 5 files changed, 183 insertions(+), 29 deletions(-) diff --git a/internal/cloud/controller/networkfabricidentity_controller.go b/internal/cloud/controller/networkfabricidentity_controller.go index 8b142f13..49f102e1 100644 --- a/internal/cloud/controller/networkfabricidentity_controller.go +++ b/internal/cloud/controller/networkfabricidentity_controller.go @@ -106,7 +106,7 @@ type NetworkFabricIdentityReconciler struct { // NetworkReader reads a network past any cache. Networks is served from the // hub's cache, where a network deleted and recreated under one name can still - // read as gone; a claim is released only once this read agrees. + // read as gone; a network is collected only once this read agrees. NetworkReader client.Reader // Hub is where the identity and its placement are written, and from where @@ -338,6 +338,13 @@ func (r *NetworkFabricIdentityReconciler) collect( namespace string, networkName string, ) error { + // The cache can read a network as gone while it lives, after a delete and a + // create under one name. Nothing is withdrawn until a read past it agrees. + gone, err := r.confirmedGone(ctx, namespace, networkName) + if err != nil || !gone { + return err + } + object := &cloudv1alpha1.NetworkFabricIdentity{} object.Namespace = namespace object.Name = networkName @@ -356,17 +363,22 @@ func (r *NetworkFabricIdentityReconciler) collect( return r.release(ctx, namespace, networkName) } -// release gives the gone network's claim back to the platform's quota. -func (r *NetworkFabricIdentityReconciler) release(ctx context.Context, namespace, networkName string) error { - // A live network keeps its claim, whatever the cache said: released, its - // block would stay held with no claim, uncounted by the quota while it lives. +// confirmedGone reads the network past the cache. A network that lives, or a +// read that fails, collects nothing. +func (r *NetworkFabricIdentityReconciler) confirmedGone(ctx context.Context, namespace, networkName string) (bool, error) { err := r.NetworkReader.Get(ctx, client.ObjectKey{Namespace: namespace, Name: networkName}, &networkingv1alpha.Network{}) - if err == nil { - return nil - } - if !apierrors.IsNotFound(err) { - return fmt.Errorf("confirm network %q is gone before releasing its claim: %w", networkName, err) + switch { + case err == nil: + return false, nil + case apierrors.IsNotFound(err): + return true, nil + default: + return false, fmt.Errorf("confirm network %q is gone before collecting it: %w", networkName, err) } +} + +// release gives the gone network's claim back to the platform's quota. +func (r *NetworkFabricIdentityReconciler) release(ctx context.Context, namespace, networkName string) error { ipamClient, err := r.IPAM.ClientForPlatform() if err != nil { return fmt.Errorf("reach the address service to release network %q: %w", networkName, err) diff --git a/internal/cloud/controller/networkfabricidentity_controller_test.go b/internal/cloud/controller/networkfabricidentity_controller_test.go index 01168e81..6283fffb 100644 --- a/internal/cloud/controller/networkfabricidentity_controller_test.go +++ b/internal/cloud/controller/networkfabricidentity_controller_test.go @@ -71,6 +71,10 @@ type fakeIdentityIPAM struct { limit int // refuseDelete stands in for an identity not yet granted delete on claims. refuseDelete bool + // recordPolicy, when set, is the policy the address service records on a + // new allocation instead of the claim's own: a class default that differs, + // or a claim whose policy was changed after it bound. + recordPolicy ipamv1alpha1.ReclaimPolicy } func allocationNameFor(claimName string) string { return "alloc-" + claimName } @@ -141,7 +145,11 @@ func newFakeIdentityIPAM(t *testing.T) *fakeIdentityIPAM { f.next++ ipClaim.Status.Phase = ipamv1alpha1.ClaimBound ipClaim.Status.AllocatedCIDR = blockForIndex(index) - return c.Create(ctx, obj, opts...) + ipClaim.Status.BoundAllocationRef = &ipamv1alpha1.LocalRef{Name: allocationName} + if err := c.Create(ctx, obj, opts...); err != nil { + return err + } + return c.Create(ctx, f.allocationFor(ipClaim)) }, Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { if _, ok := obj.(*ipamv1alpha1.IPClaim); !ok { @@ -182,18 +190,38 @@ func (f *fakeIdentityIPAM) retain(ctx context.Context, c client.WithWatch, claim if err := c.Get(ctx, client.ObjectKey{Namespace: "default", Name: claimName}, &ipClaim); err != nil { return err } - allocation := &ipamv1alpha1.IPAllocation{} - allocation.Namespace = "default" - allocation.Name = allocationNameFor(claimName) - allocation.Status.AllocatedCIDR = ipClaim.Status.AllocatedCIDR - if err := c.Create(ctx, allocation); err != nil { + var allocation ipamv1alpha1.IPAllocation + if err := c.Get(ctx, client.ObjectKey{Namespace: "default", Name: allocationNameFor(claimName)}, &allocation); err != nil { return err } - if err := c.Delete(ctx, &ipClaim, opts...); err != nil { + // The policy recorded on the allocation at bind time decides, not the + // claim's: under Retain the allocation stays with its claim reference + // cleared, and under anything else its block goes back to the pool. + if allocation.Spec.ReclaimPolicy == ipamv1alpha1.ReclaimRetain { + allocation.Spec.ClaimRef = nil + if err := c.Update(ctx, &allocation); err != nil { + return err + } + f.retained[allocation.Name] = allocation.Status.AllocatedCIDR + } else if err := c.Delete(ctx, &allocation); err != nil { return err } - f.retained[allocation.Name] = ipClaim.Status.AllocatedCIDR - return nil + return c.Delete(ctx, &ipClaim, opts...) +} + +// allocationFor is the allocation the address service binds a new claim to, +// recording the policy that will decide its fate when the claim goes. +func (f *fakeIdentityIPAM) allocationFor(ipClaim *ipamv1alpha1.IPClaim) *ipamv1alpha1.IPAllocation { + allocation := &ipamv1alpha1.IPAllocation{} + allocation.Namespace = ipClaim.Namespace + allocation.Name = allocationNameFor(ipClaim.Name) + allocation.Spec.ClaimRef = &ipamv1alpha1.LocalRef{Name: ipClaim.Name} + allocation.Spec.ReclaimPolicy = ipClaim.Spec.ReclaimPolicy + if f.recordPolicy != "" { + allocation.Spec.ReclaimPolicy = f.recordPolicy + } + allocation.Status.AllocatedCIDR = ipClaim.Status.AllocatedCIDR + return allocation } // blockForIndex renders the block a pool rooted at fc00::/32 hands out for an diff --git a/internal/cloud/controller/networkfabricidentity_quota_test.go b/internal/cloud/controller/networkfabricidentity_quota_test.go index abbea8e0..77f413fa 100644 --- a/internal/cloud/controller/networkfabricidentity_quota_test.go +++ b/internal/cloud/controller/networkfabricidentity_quota_test.go @@ -168,3 +168,43 @@ func TestAnUnconfirmedGoneNetworkKeepsItsClaim(t *testing.T) { t.Fatalf("an unconfirmed release must keep the claim, got %d claims", n) } } + +// The block is kept from reissue by the policy the address service recorded on +// the allocation when the claim bound, not by the claim's own. A claim whose +// allocation is recorded as anything but Retain is kept: deleting it would put +// the network's identity back in the pool for another network. +func TestAClaimWhoseAllocationIsNotRetainedIsKept(t *testing.T) { + f := newIdentityFixture(t, "us-central-1") + f.ipam.recordPolicy = ipamv1alpha1.ReclaimDelete + f.reconcile() + + f.deleteNetwork() + if err := f.reconcileNamed(testNetworkName); err == nil { + t.Fatal("a claim that would free its block must fail the reconcile, so the log names it") + } + if n := f.liveClaims(); n != 1 { + t.Fatalf("a claim whose allocation is not retained must be kept, got %d claims", n) + } +} + +// A network the cache reads as gone while it lives is not collected at all: its +// identity, its placement and its claim all wait for a read past the cache. +func TestALiveNetworkThatReadsGoneIsNotCollected(t *testing.T) { + f := newIdentityFixture(t, "us-central-1") + f.reconcile() + before, _ := f.placement() + + f.reconciler.Networks = staleGetter{Client: f.networks} + if err := f.reconcileNamed(testNetworkName); err != nil { + t.Fatalf("reconcile: %v", err) + } + if _, ok := f.published(); !ok { + t.Fatal("a network that lives must keep its identity") + } + if after, placed := f.placement(); !placed || len(after) != len(before) { + t.Fatalf("a network that lives must keep its placement, got %v then %v", before, after) + } + if n := f.liveClaims(); n != 1 { + t.Fatalf("a network that lives must keep its claim, got %d claims", n) + } +} diff --git a/internal/cloud/fabricidentity/identity.go b/internal/cloud/fabricidentity/identity.go index 20c08169..94724ec0 100644 --- a/internal/cloud/fabricidentity/identity.go +++ b/internal/cloud/fabricidentity/identity.go @@ -175,12 +175,46 @@ func Claim(ctx context.Context, ipamClient client.Client, request Request) (int6 // What goes is the claim, and with it the quota unit it held in the platform's // one project. A network of the same name that returns adopts the block it gave // up. A claim already gone is released. +// +// The policy that keeps the block is the one recorded on the allocation when +// the claim bound, not the claim's own, which can change. So the claim is +// deleted only once that allocation reads Retain, and only the claim that was +// read. func Release(ctx context.Context, ipamClient client.Client, request Request) error { ipClaim := &ipamv1alpha1.IPClaim{} - ipClaim.Namespace = request.Namespace - ipClaim.Name = ClaimName(request.NetworkNamespace, request.NetworkName) - if err := ipamClient.Delete(ctx, ipClaim); err != nil && !apierrors.IsNotFound(err) { - return fmt.Errorf("release the identity claim %q: %w", ipClaim.Name, err) + key := client.ObjectKey{Namespace: request.Namespace, Name: ClaimName(request.NetworkNamespace, request.NetworkName)} + if err := ipamClient.Get(ctx, key, ipClaim); err != nil { + if apierrors.IsNotFound(err) { + return nil + } + return fmt.Errorf("read the identity claim %q before releasing it: %w", key.Name, err) + } + if err := requireRetained(ctx, ipamClient, ipClaim); err != nil { + return err + } + if err := ipamClient.Delete(ctx, ipClaim, client.Preconditions{UID: &ipClaim.UID}); err != nil && + !apierrors.IsNotFound(err) { + return fmt.Errorf("release the identity claim %q: %w", key.Name, err) + } + return nil +} + +// requireRetained refuses a claim whose bound allocation is not recorded as +// Retain: deleting that claim would put the network's block back in the pool, +// where any network could be handed it. +func requireRetained(ctx context.Context, ipamClient client.Client, ipClaim *ipamv1alpha1.IPClaim) error { + ref := ipClaim.Status.BoundAllocationRef + if ref == nil { + return fmt.Errorf("keep the identity claim %q: it names no bound allocation, so its policy cannot be read", + ipClaim.Name) + } + allocation := &ipamv1alpha1.IPAllocation{} + if err := ipamClient.Get(ctx, client.ObjectKey{Namespace: ipClaim.Namespace, Name: ref.Name}, allocation); err != nil { + return fmt.Errorf("keep the identity claim %q: read its allocation %q: %w", ipClaim.Name, ref.Name, err) + } + if allocation.Spec.ReclaimPolicy != ipamv1alpha1.ReclaimRetain { + return fmt.Errorf("keep the identity claim %q: its allocation %q is recorded %q, not Retain, "+ + "so a delete would free the block for reissue", ipClaim.Name, ref.Name, allocation.Spec.ReclaimPolicy) } return nil } diff --git a/internal/cloud/fabricidentity/identity_test.go b/internal/cloud/fabricidentity/identity_test.go index be51a4be..0ae9fef8 100644 --- a/internal/cloud/fabricidentity/identity_test.go +++ b/internal/cloud/fabricidentity/identity_test.go @@ -251,12 +251,14 @@ func TestARefusedReleaseIsAnError(t *testing.T) { if err := ipamv1alpha1.AddToScheme(scheme); err != nil { t.Fatalf("build the IPAM scheme: %v", err) } - ipamClient := fake.NewClientBuilder().WithScheme(scheme).WithInterceptorFuncs(interceptor.Funcs{ - Delete: func(context.Context, client.WithWatch, client.Object, ...client.DeleteOption) error { - return apierrors.NewForbidden(ipamv1alpha1.SchemeGroupVersion.WithResource("ipclaims").GroupResource(), - "fabric-identity.ns-project.prod", errors.New("delete is not granted")) - }, - }).Build() + ipamClient := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(boundClaim("prod", ipamv1alpha1.ReclaimRetain)...). + WithInterceptorFuncs(interceptor.Funcs{ + Delete: func(context.Context, client.WithWatch, client.Object, ...client.DeleteOption) error { + return apierrors.NewForbidden(ipamv1alpha1.SchemeGroupVersion.WithResource("ipclaims").GroupResource(), + "fabric-identity.ns-project.prod", errors.New("delete is not granted")) + }, + }).Build() err := Release(context.Background(), ipamClient, Request{ Namespace: "default", NetworkNamespace: "ns-project", NetworkName: "prod", @@ -265,3 +267,41 @@ func TestARefusedReleaseIsAnError(t *testing.T) { t.Fatalf("a refused release must be returned as the refusal, got %v", err) } } + +// A claim whose allocation is recorded as anything but Retain would free the +// network's block for reissue when deleted, so it is kept and named. +func TestAClaimWhoseAllocationIsNotRetainedIsNotReleased(t *testing.T) { + scheme := runtime.NewScheme() + if err := ipamv1alpha1.AddToScheme(scheme); err != nil { + t.Fatalf("build the IPAM scheme: %v", err) + } + ipamClient := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(boundClaim("prod", ipamv1alpha1.ReclaimDelete)...).Build() + + err := Release(context.Background(), ipamClient, Request{ + Namespace: "default", NetworkNamespace: "ns-project", NetworkName: "prod", + }) + if err == nil || !strings.Contains(err.Error(), "not Retain") { + t.Fatalf("a claim whose allocation is not retained must be kept with an error naming why, got %v", err) + } + var kept ipamv1alpha1.IPClaim + if err := ipamClient.Get(context.Background(), + client.ObjectKey{Namespace: "default", Name: ClaimName("ns-project", "prod")}, &kept); err != nil { + t.Fatalf("the claim must still be there: %v", err) + } +} + +// boundClaim is a network's claim and the allocation it is bound to, recorded +// with the given policy. +func boundClaim(networkName string, policy ipamv1alpha1.ReclaimPolicy) []client.Object { + ipClaim := &ipamv1alpha1.IPClaim{} + ipClaim.Namespace = "default" + ipClaim.Name = ClaimName("ns-project", networkName) + ipClaim.Status.BoundAllocationRef = &ipamv1alpha1.LocalRef{Name: "alloc-" + networkName} + allocation := &ipamv1alpha1.IPAllocation{} + allocation.Namespace = "default" + allocation.Name = "alloc-" + networkName + allocation.Spec.ClaimRef = &ipamv1alpha1.LocalRef{Name: ipClaim.Name} + allocation.Spec.ReclaimPolicy = policy + return []client.Object{ipClaim, allocation} +}