Repository navigation
fix(fabricidentity): Release a gone network's claim - #1381
vvoytovych wants to merge 4 commits into
Conversation
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.
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.
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.
ecv
left a comment
There was a problem hiding this comment.
Approve.
Releasing the claim last, behind a direct read of the network, gives the quota unit back while ipam keeps the block retained. Checked against milo-os/ipam v0.5.5, the version galactic pins: a Retain delete nulls the claim key and keeps the allocation, so the block is not reissued. All checks pass at 480b741, and removing the direct-read guard fails TestALiveNetworkThatReadsGoneKeepsItsClaim and TestAnUnconfirmedGoneNetworkKeepsItsClaim.
- The direct read uses
hub.GetAPIReader()under the existinggetonnetworks. Any error other than NotFound fails the reconcile before an ipam call, so the claim is kept. - A same-name network re-created mid-release gets 409
AllocationRetainedfrom ipam and adopts the same block. No second block is claimed. - A refused release returns an error and requeues on per-key backoff. It does not hot-loop, block other networks or recreate the collected identity.
Follow-ups:
- Production's Milo RBAC grants only
get, createonipclaims; infra 9217 addsdeletein staging only. Grant it in production before the galactic pin moves, or every deleted network retries forever and the leak stays. - A restart while a release is failing leaks the quota unit for good. The controller watches only
NetworkandNetworkContext, and a deleted network is not in the initial list, so nothing re-enqueues it. A startup sweep that releasesfabric-identity.*claims with no network, likesweepLegacyPlacements, would close it. - A network that adopts its retained block seems to hold it with no IPClaim, so quota does not count it while it lives. Worth confirming and recreating the claim on adopt.
- Quota admission runs before the adopt path, so a returning network at the limit may be refused even though it adds no block.
TestAReturningNetworkAdoptsTheBlockItGaveUpcannot show this. Decide the expected behavior and test it.
|
Each follow-up, and where it lives:
|
…nfirmed 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.
ecv
left a comment
There was a problem hiding this comment.
Approve.
The delta since 480b741 tightens both ends. The direct read now gates the whole collect, so nothing is withdrawn on a stale cache. Release deletes only a claim whose bound allocation is recorded Retain, under a UID precondition. A freed block can't be reissued, and a claim re-created mid-release isn't the one deleted.
- The new read of the allocation needs
getonipallocations, which Milo's RBAC already grants.
Follow-ups:
- A claim with no bound allocation, or one bound to a non-Retain allocation, now errors on every reconcile and retries on backoff forever. If either can happen in practice, treat it as terminal: log it once, emit a metric and stop requeueing.
- The earlier follow-ups still stand: production's
deletegrant, the restart leak, and the adopted block with no claim.
|
On the follow-up: neither case arises through IPAM's API, so I keep the retry.
The paths left are a hand edit of |
scotwells
left a comment
There was a problem hiding this comment.
I need a bit more time to review this. When I built out the network identity stuff originally, I intentionally had IP claims be retained so that a network identity would never be re-used across VPCs.
|
Thought through this one a bit more and I think I'd be comfortable with a Release behavior after some sort of cooldown so that VPC IDs are released immediately by the IPAM system. I'd want this capability built into the IPAM system though so it can be a generic capability that's re-used across all services. I'd also like to wait for @privateip's thoughts here so he has a chance to chime in. |
Every network claims its fabric identity as an IPClaim in the platform's one project, under that project's quota on claims.
collect()removed a gone network's identity and kept its claim, so each gone network held a quota unit forever. Once the quota is full, no new network anywhere gets an identity, while existing networks keep working. In our production this filled the platform project's claim quota, and new networks waited for an identity.Summary
Releasing the claim keeps what keeping it protected. Under Retain the address service keeps the allocation under the claim's name, so the block is never reissued. The policy that decides is the one recorded on the allocation when the claim bound, so
Releasechecks that one. A network of the same name that returns adopts it, through the existingRetainedAllocationpath.Test plan
TestAGoneNetworkGivesItsQuotaUnitBackfails onmain, where the next network is refused for quota, and passes with the fix.go test ./internal/cloud/...passes, also under-race, andgolangci-lintv2.12.2 reports 0 issues on both changed packages. The full lint, with its eBPF build, runs on this draft.Merge danger
The direct read closes one window: a network deleted and recreated under one name can read as gone in the cache while it lives. Collected then, it would lose its identity and placement for a reconcile. Its block would also stay held with no claim, uncounted by the quota.
The platform identity needs
deleteonipclaims. Without it,collect()returns the refusal and retries; the identity and its placement are already gone.A two-way door: a revert keeps claims again. A claim released meanwhile is not restored.
Releasedeletes only a claim whose allocation is recorded Retain, so its block stays held and no identity is lost or reissued. The blast radius is the fabric identity controller.