[privateip]

Fixes #978.

Summary

A failed grant-record write no longer strands a connector's /96 allocation.

A retry with the same connector keeps its /96, a lost reply points the operator at rotate, and retire tombstones an allocation that has no record.

The lab suite also gains a check that return traffic reaches addresses in several subdivisions of one /96 through its single aggregate route.

Test plan

• The new regression tests fail on main and pass here under the race detector
• Lint and the race-enabled unit suite pass
• The new lab row passed on the lab built from this branch, with all 12 source and destination pairs answered and one advertisement for the /96
• Three lab rows that fail the same way on unmodified main are unrelated and tracked in MASQUE lab rows maintenance-during-use, partial-setup and recreation fail on main #1197

[polylane]

Warning

Polylane could not verify the production impact of this pull request.

I checked the production account: no workload runs masque-authority (633 deployments, 1041 pods, 560 namespaces live) and only its CLI imports the changed package. Cloud Monitoring and log reads for the account returned no series.

View the full analysis →

Also considered · 3 refuted

• Refuted · Retire's new ledger scan needs ConfigMap list rights an authority pod may not hold · main already lists ConfigMaps with the identical label selector in Ledgers.Init (ledger.go:143), so any identity that runs the authority's ledger commands already needs configmaps list.
• Refuted · The new Retire path tombstones an allocation that is still serving traffic · The authority service retires EdgeService objects in its own path (authority.go:534) and never calls Grants.Retire, so the only caller is the explicit operator command.
• Refuted · A new masque-authority revision deploys to production on merge · Nothing builds, publishes or runs this binary in the connected account, so no deploy follows the merge.

Analysed against 1 cloud account and 1 repository

• Cloud account: datum-cloud-prod
• Repository: datum-cloud/galactic

Polylane could not find the cloud resources this repository manages, so this review looked at the entire cloud account. Connect this repository to its resources and the next review will focus on exactly what this code deploys to.

_{Polylane analysed 09b0f3d for production impact. You can ask follow-ups by mentioning @polylane in a comment.}

Rate this review: 👍 helpful · 👎 not helpful · 😕 confusing

[privateip]

Second-pass verdicts on head 09b0f3d.

pr-rereviewer: VERDICT: merge. It ran gh pr view 1195 --json title,body,isDraft,headRefOid; git rev-parse HEAD; git log --show-signature main..HEAD.

pr-conventions-reviewer: VERDICT: merge. It ran gh pr view 1195 --json title,body,commits,files,baseRefName,isDraft; git diff --stat 6b333af...09b0f3d; git diff of docs and authority files; gh pr list --state open.

First pass: pr-adversary returned merge after running go test -race ./internal/edge/masque/authority/, go vet ./tests/edgeconformance/..., and reading the Create, Retire and ledger paths. The conventions reviewer's first pass held on PR text only, and those findings were applied in the fix phase (title, body, draft state), so no code changed between passes.

CI is green on the head: every check passed, including Build, Lint, Unit Tests, Unit Tests (root), E2E Tests and Manifests. The debug-image and service-api publish jobs were skipped by design.

Auto-merge is enabled with the merge method, the only one the main ruleset allows. The ruleset requires one approving review, a code owner review and approval of the last push, so the PR waits for a human approval.

[ecv]

Approve.

The fix keeps the stranded /96 and makes it recoverable instead of reclaiming it, which is the safe choice: no path here frees a /96 a live grant could hold. Ledgers.Allocate already returns an existing entry for the same connector, so a retry reuses it, and Retire now tombstones a record-less allocation.

• Create with the same connector and VPC/cell after a Secret create failure reuses the entry (Allocate is idempotent for a non-retired connector), so the 256 cap is not consumed again. Ledger writes still go through Ledgers.update, a conditional Update on the read's resourceVersion with conflict retry; the PR adds only a read (holding) and no new write path.
• retireUnrecorded runs only when Disable returns NotFound, then tombstones and re-runs Disable to catch a racing Create. A Create whose record lands after that second Disable leaves an enabled record on a retired allocation. The recorded Retire has the same window (Disable, then ledger Retire), so this is no worse than before.
• Context cancel mid-Create after Allocate leaves the entry exactly like a Secret failure; the same retry or Retire recovers it. A lost reply on the Secret create gives AlreadyExists on retry, and the error points at grant rotate.
• Conformance changes only add the M12-aggregate-routing row and multi--src support in edgeprobe connectip. No assertion is removed, and with one or no -src the send loop behaves as before.

Follow-ups:

• The grant Secret is named by connector alone (RecordName), but the ledger is keyed by VPC and cell. A --connector that already has a record in another VPC or cell passes Allocate in the new ledger and then hits AlreadyExists, which strands an entry there. The new message tells the user to rotate, which is wrong for that case, and Retire of the recorded connector never tombstones the other ledger's entry. Check for an existing record before allocating, or make Retire sweep every ledger holding the name.
• A stranded entry is only recovered if the operator reads the error and retries or retires. Nothing sweeps record-less allocations, so a Create that dies (process kill) before returning the error still leaks until someone finds it. A periodic reconcile of ledger entries without records would close it.