Skip to content

(2)-masquerade: port forwarding claims - #1698

Draft
daniel-noland wants to merge 8 commits into
pr/daniel-noland/port-allocator-fixesfrom
pr/daniel-noland/portfw-reservation-fixes
Draft

(2)-masquerade: port forwarding claims#1698
daniel-noland wants to merge 8 commits into
pr/daniel-noland/port-allocator-fixesfrom
pr/daniel-noland/portfw-reservation-fixes

Conversation

@daniel-noland

@daniel-noland daniel-noland commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Warning

AI assisted, not yet ready for external review by other humans.
Please do not spend review time on this yet. It is pushed to run CI and to keep
the stack visible, not to attract review. The dont-merge label stays on until
that changes.

Two defects in the ports masquerade must keep off because port forwarding has claimed them.
Both were reachable. The first was found by the property tests added in the previous PR, which
is why that PR carries two tests committed #[ignore]d. The second was found while writing
them rather than by them: the pools are handed claims already expressed in public space, so no
pool-level property can see a mistake in translating the configuration into those claims. It
took the config-level test added here.

  • Only one claim per address was honoured. The limit was baked in three times over: a map
    keyed by address range where inserting twice overwrote, a pool resolving one range per
    address out of it, and a port allocator storing one range. Each had to change together.
  • Claims were computed in the wrong address space. They came from intersecting private
    prefixes, while the pools are asked about public addresses, so effectively nothing was
    ever reserved. Claims are now gathered per peer VPC rather than per manifest, since the
    public space towards a peer is shared between every VPC masquerading onto it.

Verified by mutation: restoring the private prefixes makes the new end-to-end test hand out
the first forwarded port. Two test oracles were also strengthened here rather than higher in
the stack: the reserved-ports property now unions every claim as the implementation does,
instead of skipping claims whose own expose did not declare the address (the gated version
survived ten thousand inputs against a pool built from only each expose's own claims), and the
masquerade-over-forwarded-ports fixture is now the same-manifest configuration production
validation actually accepts, rather than a cross-VPC one it rejects.

The lifecycle of a claimed block

Claiming ports gives the allocator three states it never had before -- a block claimed in full,
an address whose every block is claimed, and a run of such addresses -- and it mishandled all
three. These were found by review of this PR and the fixes have been moved down from #1700 to
sit with the change that creates the states, so this PR does not ship them:

  • A fully claimed first address disabled the whole region. Allocation draws the lowest free
    address; if port forwarding had claimed every port on it, the attempt failed and the address
    went straight back into the pool for the next packet to find. Masquerade over 10.1.0.0/31
    with forwarding over 10.1.0.0/32:1024-65535 could allocate nothing at all, though
    10.1.0.1 had 64k ports free. Such addresses are now kept out of the pool as it is built.
  • A fully claimed block left the free-block count overstated. The count was taken before
    claims were applied and never decremented when a block was skipped, so an address reported
    room it did not have, reuse failed on it, and every later flow drew a fresh address. With four
    addresses and one claim, the 260th allocation failed with NoFreeIp while 193,533 usable
    pairs sat untouched.
  • An ordinary policy conflict was reported as allocator corruption. A skipped claimed block
    is not free and has no allocated block, so a reservation aimed at it returned InternalIssue
    rather than PortReservationFailed. The concurrency model treats InternalIssue as fatal,
    and a late carry-over can aim at exactly such a block.

Each was reproduced against this PR's previous head and passes now.

Reachability, stated plainly

Cross-VPC claim sharing -- one VPC's port forwarding binding another VPC's masquerade pools --
turns out to be unreachable from a validated configuration: the peer's route table rejects a
public overlap between masquerade and port forwarding coming from two different VPCs
(VpcRoute::can_overlap exempts masquerade-with-masquerade, not this). The live case is both
modes in one manifest, which validate_expose_collisions explicitly allows and which the
tests here now model. Collecting claims per peer VPC stays, as defence of the same kind as #1697:
the guarantee lives in another crate, and nothing near the pools says they rest on it.

Also here

  • Flow timeouts stretch when the tests run under an emulator. The end-to-end test added here
    never answers its SYNs, so its flows carry the five-second one-way timeout, and it does two more
    packets and four assertions between the last thing that refreshes them and the flow count at the
    end. Natively that is about a second; under miri it is well past five, the flows have expired,
    and the count is zero. That is why check/miri/powerpc64 goes red from this PR onwards while
    (1)-masquerade: allocate each public pair once #1696 is green -- (1)-masquerade: allocate each public pair once #1696 does not have the test. Reproduced natively with no emulator involved, by
    sleeping six seconds rather than one: same assertion, same values.

    The constants and the sleeps against them are older than this stack, and the other masquerade
    tests sit on the same cliff, so the timeouts are stretched rather than the one test being
    patched. emulated is set only by the miri and qemu-user paths, so a real data plane always gets
    the native values.

Stack

Merge bottom to top. Each PR is based on the one above it in this list.

#1697 was folded into #1696 and closed; its one commit belonged next to the other pool-table work.

Every commit in the stack builds and passes cargo nextest run and cargo clippy --all-targets
on its own.

@daniel-noland
daniel-noland requested a review from a team as a code owner August 6, 2026 02:28
@daniel-noland
daniel-noland requested review from sergeymatov and removed request for a team August 6, 2026 02:28
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fd392355-5329-4c1e-b732-0e34ae5ce708

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@daniel-noland daniel-noland added the dont-merge Do not merge this Pull Request label Aug 6, 2026
@daniel-noland daniel-noland changed the title masquerade: port forwarding reservation fixes (2)-masquerade: port forwarding reservation fixes Aug 6, 2026
@daniel-noland
daniel-noland marked this pull request as draft August 6, 2026 02:32
@daniel-noland
daniel-noland force-pushed the pr/daniel-noland/portfw-reservation-fixes branch from 58bf053 to ceb138c Compare August 6, 2026 04:35
@daniel-noland
daniel-noland requested a review from Copilot August 6, 2026 05:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes masquerade port-allocation so that ports claimed by port forwarding are consistently reserved (including multiple claims per address) and reserved in the correct (public) address space, preventing masquerade from allocating (public_ip, port) pairs already spoken for by port forwarding. It also adjusts masquerade flow timeouts under emulation to keep time-based tests stable under miri/qemu-user.

Changes:

  • Rework how port-forwarding claims are gathered and applied: claims are collected per peer VPC (public space) and propagated into pool/allocator reservation logic with support for multiple claims per address.
  • Introduce a ReservedPorts/PortClaims model to keep all claims (including overlaps) and correctly apply them per address and per 256-port block.
  • Stretch masquerade flow timeouts when built with cfg(emulated) to avoid spurious expirations in emulated test runs; add tests for shared-private-prefix re-reservation and forwarded-port avoidance.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
nat/src/ranges.rs Make IpRange::contains available outside cfg(test) so production code can query reserved-address coverage.
nat/src/masquerade/test.rs Add an end-to-end reconfiguration test ensuring two VPCs sharing a private prefix keep distinct public ranges across config updates.
nat/src/masquerade/nf.rs Scale internal flow timeouts under cfg(emulated) to stabilize time-sensitive tests under emulation.
nat/src/masquerade/apalloc/test_alloc.rs Add allocator-level test ensuring masquerade never allocates ports claimed by port forwarding on shared public space.
nat/src/masquerade/apalloc/setup.rs Gather port-forwarding claims per peer VPC (public space) and pass them into pool construction; remove per-expose overlap computation.
nat/src/masquerade/apalloc/reserved.rs New data structures for keeping multiple reserved port ranges per address and evaluating per-block coverage/clipping.
nat/src/masquerade/apalloc/port_alloc.rs Change PortAllocator to accept multiple claims (PortClaims), skip fully-claimed blocks, and reserve multiple clipped ranges in new blocks.
nat/src/masquerade/apalloc/pool_fuzz.rs Update fuzz/property harness to pass claimed sets into pool construction and unignore previously failing reservation properties.
nat/src/masquerade/apalloc/mod.rs Wire in the new reserved module.
nat/src/masquerade/apalloc/display.rs Update allocator/pool display output to reflect multiple reserved ranges instead of a single range.
nat/src/masquerade/apalloc/concurrent_fuzz.rs Update concurrent fuzz harness to pass an explicit claimed set to pool_sets_for_specs.
nat/src/masquerade/apalloc/alloc.rs Replace single-range reservation model with ReservedPorts/PortClaims across NatPool and AllocatedIp creation and reservation paths.

Comment thread nat/src/masquerade/apalloc/pool_fuzz.rs Outdated
Comment on lines +471 to +473
/// This is a property of the pools, which are given claims already expressed in public space.
/// Computing those claims from the configuration is a separate step, and gets the address space
/// wrong; see the note on `find_masquerade_portfw_overlap`.
@daniel-noland
daniel-noland force-pushed the pr/daniel-noland/port-allocator-fixes branch from 33a926f to 191e004 Compare August 6, 2026 21:25
@daniel-noland
daniel-noland force-pushed the pr/daniel-noland/portfw-reservation-fixes branch 2 times, most recently from a17cbd1 to cf59591 Compare August 6, 2026 21:47
daniel-noland added a commit that referenced this pull request Aug 6, 2026
The pool-level guarantee behind the Clone removal in #1696. That change
makes the sequence which broke it unwriteable, so what is left to check is
the guarantee itself: a pair a live allocation holds cannot be reserved,
and can be once it is released.

Lives here rather than with the fix because it needs the claims argument
`pool_sets_for_specs` grows in #1698.
daniel-noland added a commit that referenced this pull request Aug 6, 2026
The pool-level guarantee behind the Clone removal in #1696. That change
makes the sequence which broke it unwriteable, so what is left to check is
the guarantee itself: a pair a live allocation holds cannot be reserved,
and can be once it is released.

Lives here rather than with the fix because it needs the claims argument
`pool_sets_for_specs` grows in #1698.
daniel-noland added a commit that referenced this pull request Aug 6, 2026
The pool-level guarantee behind the Clone removal in #1696. That change
makes the sequence which broke it unwriteable, so what is left to check is
the guarantee itself: a pair a live allocation holds cannot be reserved,
and can be once it is released.

Lives here rather than with the fix because it needs the claims argument
`pool_sets_for_specs` grows in #1698.
@daniel-noland
daniel-noland force-pushed the pr/daniel-noland/portfw-reservation-fixes branch from a5c7af6 to c8219f9 Compare August 6, 2026 22:33
daniel-noland added a commit that referenced this pull request Aug 6, 2026
The pool-level guarantee behind the Clone removal in #1696. That change
makes the sequence which broke it unwriteable, so what is left to check is
the guarantee itself: a pair a live allocation holds cannot be reserved,
and can be once it is released.

Lives here rather than with the fix because it needs the claims argument
`pool_sets_for_specs` grows in #1698.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
@daniel-noland
daniel-noland force-pushed the pr/daniel-noland/portfw-reservation-fixes branch from c8219f9 to 51bc9db Compare August 6, 2026 22:49
daniel-noland added a commit that referenced this pull request Aug 6, 2026
The pool-level guarantee behind the Clone removal in #1696. That change
makes the sequence which broke it unwriteable, so what is left to check is
the guarantee itself: a pair a live allocation holds cannot be reserved,
and can be once it is released.

Lives here rather than with the fix because it needs the claims argument
`pool_sets_for_specs` grows in #1698.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
daniel-noland added a commit that referenced this pull request Aug 6, 2026
The pool-level guarantee behind the Clone removal in #1696. That change
makes the sequence which broke it unwriteable, so what is left to check is
the guarantee itself: a pair a live allocation holds cannot be reserved,
and can be once it is released.

Lives here rather than with the fix because it needs the claims argument
`pool_sets_for_specs` grows in #1698.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
@daniel-noland
daniel-noland force-pushed the pr/daniel-noland/port-allocator-fixes branch from 5173d8e to 1ca53df Compare August 6, 2026 23:51
@daniel-noland
daniel-noland force-pushed the pr/daniel-noland/portfw-reservation-fixes branch from 51bc9db to 29a8636 Compare August 6, 2026 23:51
daniel-noland added a commit that referenced this pull request Aug 7, 2026
The pool-level guarantee behind the Clone removal in #1696. That change
makes the sequence which broke it unwriteable, so what is left to check is
the guarantee itself: a pair a live allocation holds cannot be reserved,
and can be once it is released.

Lives here rather than with the fix because it needs the claims argument
`pool_sets_for_specs` grows in #1698.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
daniel-noland added a commit that referenced this pull request Aug 7, 2026
The pool-level guarantee behind the Clone removal in #1696. That change
makes the sequence which broke it unwriteable, so what is left to check is
the guarantee itself: a pair a live allocation holds cannot be reserved,
and can be once it is released.

Lives here rather than with the fix because it needs the claims argument
`pool_sets_for_specs` grows in #1698.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
daniel-noland added a commit that referenced this pull request Aug 7, 2026
The pool-level guarantee behind the Clone removal in #1696. That change
makes the sequence which broke it unwriteable, so what is left to check is
the guarantee itself: a pair a live allocation holds cannot be reserved,
and can be once it is released.

Lives here rather than with the fix because it needs the claims argument
`pool_sets_for_specs` grows in #1698.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
@daniel-noland
daniel-noland force-pushed the pr/daniel-noland/port-allocator-fixes branch from 1ca53df to 44c59e4 Compare August 7, 2026 01:07
@daniel-noland
daniel-noland force-pushed the pr/daniel-noland/portfw-reservation-fixes branch from 29a8636 to 7026f44 Compare August 7, 2026 01:07
daniel-noland added a commit that referenced this pull request Aug 7, 2026
The pool-level guarantee behind the Clone removal in #1696. That change
makes the sequence which broke it unwriteable, so what is left to check is
the guarantee itself: a pair a live allocation holds cannot be reserved,
and can be once it is released.

Lives here rather than with the fix because it needs the claims argument
`pool_sets_for_specs` grows in #1698.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
@daniel-noland
daniel-noland force-pushed the pr/daniel-noland/port-allocator-fixes branch from 44c59e4 to 090a2f7 Compare August 7, 2026 01:30
@daniel-noland
daniel-noland force-pushed the pr/daniel-noland/portfw-reservation-fixes branch from 7026f44 to 67f2ea5 Compare August 7, 2026 01:30
daniel-noland and others added 8 commits August 6, 2026 19:44
A masquerade flow expires against the wall clock, while a test refreshes
it by doing work: sending a packet. Under miri or qemu-user that work runs
something like two orders of magnitude slower, so the gap between one
packet and the next stops being a fraction of a flow's life and becomes
several times it. A flow a native run keeps comfortably alive is one an
emulated run finds long dead.

That is what has been failing check/miri/powerpc64 on this stack.
test_masquerade_reconfig_two_vpcs_sharing_a_private_prefix never answers
its SYNs, so its flows carry the one-way timeout of five seconds, and it
does two packets and four assertions between the last refresh and the
count. Natively that is about a second. Under miri it is well past five,
all four flows are gone, and active_len() is 0 where the test wants 4.

Nothing to do with the allocator. The test arrives in the second PR of
this stack, which is the whole of why the job passes on the first and
fails on every one after it: the first does not have the test. Not a
matter of which seed a run drew, as I first supposed.

The constants and the sleeps against them are older than the stack, and
the same cliff is under the other masquerade tests that use them.
test_masquerade_reconfig_keep_flow finished within a second of the one
that failed, so this was going to spread.

Confirmed by reproducing it natively, with no emulator involved: sleeping
six seconds rather than one in that test gives the identical failure,
Some(0) against Some(4). With the timeouts stretched, the same six-second
stall passes.

`emulated` is set only by the miri and qemu-user paths, so a real data
plane is unaffected. The whole nat suite passes with the cfg forced on,
which also shows the three tests that assert a flow count of zero are
driven by invalidation rather than by a timeout firing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Keying the pool table by source VPC made a private address mean something
only inside its own VPC. The allocator tests cover that directly, but
nothing drove it through the pipeline, where the address has to survive
being looked up on the packet path and again when the configuration is
applied.

Two VPCs both using 1.1.0.0/16 and both masquerading towards a third,
onto public ranges of their own. Each is translated onto the range its
own expose declares, and both keep that address across a configuration
change rather than one of them landing in the other's pool.

The change applied here is an identical configuration, which the writer
answers by keeping the allocator it has and only advancing the flows'
generation. That is the common case and worth covering, but it is not the
carry-over path: re-reservation is reached only when the configuration
really differs, and a later commit in this series covers that.

The flow-creating packet does not carry the flow it creates, so the
assertions are made on a follow-up packet, which is also what shows the
generation advancing.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
A public address could carry only one claimed port range. Port
forwarding names a public range and a port range per expose, and nothing
stops two exposes naming the same address with different ports, so the
second claim replaced the first and masquerade went on handing out ports
that port forwarding had statically mapped elsewhere.

The single range was baked in three times over: the claims were
collected into a map keyed by address range, where inserting twice under
one key overwrote; the pool then resolved one range per address out of
that map, since the lookup returns a single entry; and the port allocator
stored one range. Each had to change together, which is why the tests
covering this were committed ignored rather than fixed piecemeal.

Carry the claims as a list instead, in a type of their own. A list rather
than a map because claims may be made on overlapping address ranges and
every claim covering an address applies, not merely the innermost or the
last recorded, and reserving a port in a block bitmap is an idempotent
OR, so overlapping claims need no merging.

Two things needed more than a mechanical change:

Skipping a fully claimed port block used to ask whether one claim covered
it. Several claims may cover a block between them while no single one of
them does, so this now walks the union. The old special case for port 0,
which is never handed out for TCP or UDP and so need not be covered for
the first block to be useless, is kept.

Claims are clipped to a block before they reach its bitmap, which indexes
ports modulo 256 and silently reserves nothing when handed a range
crossing its end. That clipping already existed for the single range;
it now happens per claim, and also on the path where an address enters
the pool through a reservation. That path passed no claims at all, so a
flow carried across a config change could bring an address in
unencumbered and let masquerade hand out the ports port forwarding held
on it.

Both previously ignored tests now run: the pools honour claims expressed
in public space. Computing those claims from the configuration is the
separate defect noted on find_masquerade_portfw_overlap, and is next.

The property that states this needed its oracle widened the same way.

The property says a claim binds the public space it names whoever made
it, and the pool is built that way: the claims of every expose are
unioned and applied to every region over that space.

The oracle did not check it that way. It skipped a claim unless the
expose that made it also declared the address the allocation came from,
so a claim made through one expose was never checked against an
allocation made through another -- the one case the property exists to
state.

Building the pool from only each expose's own claims leaves the property
passing over ten thousand inputs before this, and fails on the second
input after it.

Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
The ports masquerade had to keep off were worked out by intersecting the
private prefixes of a port-forwarding expose with the private prefixes of
a masquerade expose. The pools are then asked about them by public
address, because a public address and port is what an allocation is, so
the claims described a space nothing ever looks in and effectively
nothing was reserved.

A port-forwarding expose is validated down to one prefix on each side,
of equal size and with a port range on both, so its public claim needs no
offset arithmetic: it is exactly its as_range. Record that instead.

Claims are also collected per peer VPC now, rather than per manifest.
The public space towards a peer is shared between every VPC masquerading
onto it, which is what the region decomposition already exists for, and
return traffic carries nothing that says which VPC it belongs to. A
claim therefore binds the space rather than the expose that declared it:
one VPC's port forwarding has to be honoured by another VPC's pools, and
a peering that port-forwards without masquerading no longer has its
claims dropped for want of a masquerade expose to hang them on.

That makes the claims a property of the public space rather than of an
expose, so they move off PoolSpec and are passed to the pool builder
once per peer VPC and protocol.

The masquerade expose's own protocol no longer narrows a claim. Pools are
built for TCP, UDP and ICMP whatever an expose declares, so a TCP claim
belongs in the TCP pool regardless, and intersecting the two protocols
only dropped claims that were still live.

Verified by mutation: putting the private prefixes back makes the new
end-to-end test hand out 10.1.0.0:1024, the first port the forwarding
expose has taken.

The fixture the end-to-end test runs on had to be a configuration that
production would actually accept.

The fixture put the masquerade expose in VPC-1's peering and the
port-forwarding expose in VPC-2's, both onto one public address towards
VPC-3. Production rejects that: config validation builds each VPC a route
table from its peerings, and VPC-3 cannot route the shared address to one
peering when the exposes come from two -- masquerade with masquerade may
overlap there, masquerade with port forwarding may not. The fixture passed
validation only because building a VpcTable by hand skips collecting
peerings into the peer, so VPC-3's route table was empty and the check
never ran.

Both exposes now sit in one manifest, which is the shape validation
accepts (validate_expose_collisions allows the overlap within a manifest,
each mode implying a direction) and the shape the end-to-end reconfig test
already uses. The test over the fixture is unchanged and still bites: the
first port handed out is 1031, and without the claims it would be 1024.

Cross-VPC claim sharing in gather_exposes stays, and this is worth being
clear about: with the overlap rejected across VPCs, no validated
configuration currently reaches it. It is defence of the same kind as the
pool lookup that stopped relying on disjoint prefixes -- the guarantee
lives in another crate, and nothing near the pools says they rest on it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Allocation drew the lowest free address from a region and, if no port
could be had from it, handed it straight back and gave up. The same
address was lowest next time, so the pool served nothing at all for as
long as it stayed there. One public address whose every port is claimed
by port forwarding therefore took a whole region out of service, and the
claims only had to land on the lowest address in it: the same claim one
address higher was harmless, because allocation never reached it.

This became reachable when claims started being computed in the public
space. Before that they described private addresses the pools are never
asked about, so nothing was ever reserved and no address could be fully
claimed. The configuration it needs is allowed: validation permits a
masquerade expose and a port-forwarding expose to overlap, which is the
whole reason masquerade keeps off the ports port forwarding has taken.

Draw another address instead. An address that comes fresh out of the
pool and yields no port is one whose every port is spoken for, which does
not change while the pool lives, so it is taken out rather than handed
back and the walk moves on.

The walk is bounded, because a data plane cannot search without a bound
on the packet path. The bound costs nothing when the first address
serves, and since a useless address is taken out as it is found, a long
claimed run is worked through over successive packets rather than being
walked again by each of them.

The tests reach exhaustion by claiming every port of an address, which is
what makes it testable at all: allocation stays on one address for 64k
ports, so a handful of allocations never leaves the first address of the
first region. That was the substance of a review comment on the property
tests, and it hid this. They now cover an address being passed over, a
region with nothing left reporting exhaustion, every address of a region
being reachable in turn, and an expose falling back to shared space once
the space it has to itself is gone.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
A block port forwarding had claimed in full was skipped when picking one
to allocate from, and the skip marked it non-free without taking it off
the count of blocks still usable. The count only ever went down when a
block was actually allocated, so it stood permanently higher than the
truth.

That count is what decides whether an address is worth trying. An address
whose remaining blocks were all claimed still reported room, so reuse
tried it, and the attempt failed with "no port block" rather than "no
free port" -- and the walk over addresses already in hand returned on
that error instead of moving to the next one. Every allocation from then
on drew a fresh address while the addresses in hand sat with tens of
thousands of free ports.

A region of four addresses with one claim on one of them ran out after
259 allocations, where it holds room for over a quarter of a million.

Decide which blocks are unusable once, when the allocator is built: the
well-known range, as before, and now also the blocks claimed in full.
Then count the usable ones from the blocks themselves rather than
computing what the count ought to be, so the two cannot disagree. Picking
a block becomes just claiming the first free flag, which takes the claims
off the allocation path entirely.

Reuse also moves on now when an address turns out to have nothing left,
whichever way it says so, rather than giving up on the whole walk.

Either change alone is enough for the case above, and the test passes
with either reverted; it pins the outcome rather than the mechanism. The
second is worth keeping regardless, since an address can be emptied by
another thread between being judged worth trying and being tried.

Two more ways the count could stop describing the blocks, found by review
of this PR. Giving a block back raised its flag before adding to the
count, so a claimant winning the flag in between subtracted from a count
that was still short -- on a `u16`, past zero to 65535, which says an
address has room it does not have. The count is given back first now, so
the transient error is one too many rather than a wrap. And a block whose
construction failed after its flag was taken and the count lowered gave
neither back: no `Arc` existed, so no `Drop` was coming, and the block was
claimed by nobody for the life of the allocator. Both paths hand it back.

The well-known-range refusal in `reserve_port` also no longer depends on
knowing what `setup.rs` does. The range is a port-number convention and
says nothing about an ICMP identifier; identifiers cannot reach it today
because ICMP pools are built with the exclusion off, but nothing here
said so.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
…g error

Reserving a port looks the block holding it up, and reads a block that is
not free as one that is allocated and therefore present in the list of
allocated blocks. Absent from that list, it concluded the allocator's own
bookkeeping was broken and returned InternalIssue.

That stopped being true once a block port forwarding has claimed in full
could be marked non-free without ever being allocated. Such a block never
joins the list, so its absence says nothing. A configuration reaches this:
keep a masquerade expose, add port forwarding covering the whole 256-port
block around a port some flow holds, and carry that flow over. The flow is
dropped, which is right, but a legal conflict between two parts of a valid
configuration is logged as a bug in the allocator, and reported upwards as
InternalFailure rather than as a NAT failure.

Answer as a claim on part of the same block already does, where the block
stays allocatable and its own bitmap refuses the port. How much of a block
an operator happened to claim is not something a caller should be able to
tell apart, and it is certainly not the difference between a policy
conflict and broken bookkeeping.

The condition mirrors the one that marks blocks unusable when the allocator
is built, and has to keep mirroring it, so it is written as a single
predicate next to the lookup that needs it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
…e pool

Allocation draws the lowest free address, so a port-forwarded prefix at
the bottom of a region is what it meets first. Every one of those
addresses was drawn, found to have no port to give, and taken back out --
and an allocation may work through only MAX_ADDRESSES_PER_ALLOCATION of
them before it gives up and the packet is dropped. A run of N therefore
cost floor(N/8) dropped packets: sixteen for a claimed /25, measured, on
flows the region had tens of thousands of ports waiting for. Retirement
kept it from being worse than that, since the next packet carried on
where the last stopped, but a fresh allocator starts with a fresh pool,
so the whole run was rediscovered after every config change, per protocol.

Which addresses those are is known when the pool is built. Sweep the
claims rather than the addresses -- a region may hold billions of the
latter and only as many of the former as there are port-forwarding
exposes towards one peer -- cutting where a claim begins or just past
where one ends, since coverage cannot change anywhere else. One address
then decides each stretch, and the stretches that can serve nothing are
taken out of the bitmap before anything is allocated.

Whether an address can serve is asked through the same predicate the port
allocator uses to rule a block out, applied to all 256 of them, rather
than by a second definition of the same thing written over the port
space. The two drifting apart would mean either dropping an address that
still had a block to give, which is capacity silently thrown away, or
keeping one that had none, which is the walk this removes.

The exclusion needs somewhere to live that a drop cannot undo. Reserving
reaches addresses allocation never touches: a flow carried across a
config change presents the address it holds, and the pool takes it into
use to try to give the port back. Where the new configuration has claimed
that address the reservation fails, correctly -- but the address has been
through the pool by then, and deallocate_from_pool would hand it to the
bitmap on the way out, undoing the exclusion on the first config change
that needed it. Unusable offsets are therefore tracked apart from free
ones, and consulted before an address is given back. That closes the same
hole for retire_ip, where it was latent: an address retired for having
nothing to give could be put back by a carry-over that failed on it.

The bound stays, for the case configuration cannot produce: an address
emptied by another thread between being drawn and being drawn upon.

The property over the new sweep needed an oracle written independently
of it.

The property checked what the sweep reports against `every_block_is_unusable`
applied per address -- the same function the sweep calls. It verified the
cutting into intervals, which is worth verifying, but it could say nothing
about the predicate underneath, because it was comparing that predicate
with itself. Mutation testing showed it: truncating the block loop, and
excusing one block from being claimed, both left it green.

It now decides the same question a different way, walking the claims over
the port space to ask whether they cover it end to end, with no notion of
a block anywhere in it. Both mutations fail against that, as does the one
that stops excluding addresses altogether.

The generator also draws port ranges in three shapes rather than two, the
new one being a claim that stops exactly on a block boundary. Left to
chance a range that ends on a multiple of 256 essentially never appears,
and that is the shape the block-level and address-level answers can
disagree on.

Comparing the production code against itself is the risk that comes with
routing both answers through one predicate so they cannot drift. The
answer is not to give up the single definition, it is to test it against
something written independently.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
daniel-noland added a commit that referenced this pull request Aug 7, 2026
The pool-level guarantee behind the Clone removal in #1696. That change
makes the sequence which broke it unwriteable, so what is left to check is
the guarantee itself: a pair a live allocation holds cannot be reserved,
and can be once it is released.

Lives here rather than with the fix because it needs the claims argument
`pool_sets_for_specs` grows in #1698.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
@daniel-noland
daniel-noland force-pushed the pr/daniel-noland/port-allocator-fixes branch from 090a2f7 to d42aea5 Compare August 7, 2026 01:50
@daniel-noland
daniel-noland force-pushed the pr/daniel-noland/portfw-reservation-fixes branch from 67f2ea5 to 3014ecf Compare August 7, 2026 01:50
@daniel-noland daniel-noland changed the title (2)-masquerade: port forwarding reservation fixes (2)-masquerade: port forwarding claims Aug 7, 2026

// A TCP packet towards VPC-3, from a given source VPC and private address. A SYN opens a flow;
// anything else is only translated if one already exists.
fn tcp_from(src_vni_id: u32, src_ip: &str, syn: bool) -> Packet<TestBuffer> {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dont-merge Do not merge this Pull Request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants