Config apply concurrency - #1664
Draft
daniel-noland wants to merge 3 commits into
Draft
Conversation
The validation-legal generator mode produced no NAT at all, so `MasqueradeConfig` was always empty and the NAT allocator was never installed for any generated config. That makes the whole masquerade path -- allocator installation, flow migration, the genid protocol -- unreachable from a generated config, which is what a concurrency test of that path needs. Masquerade is the one NAT mode reachable without also controlling prefix sizes: it needs a non-empty `as` range in the same family as `ips` and no port ranges anywhere, all of which the shared pool already provides. Static NAT and port forwarding additionally require the two sides to cover the same number of addresses, which needs a pool that hands out sized blocks; the FIXME says so. Masquerade placement turns out to be another whole-peering decision rather than a per-expose one: `Peering::check_nat_modes` forbids masquerade on both sides of a peering, so the caller that owns both sides picks at most one to masquerade -- the same shape as the existing default-expose rule, and independent of it, since one manifest may hold both a default expose and a masquerading one. Recorded in the non-vacuity guard so that a future change which stops generating masquerade fails loudly here rather than silently hollowing out the concurrency test that depends on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Nothing covered replacing the masquerade allocator underneath a running data path. The existing concurrency tests allocate against a *fixed* allocator; the config tests reconfigure with nothing else running. Reconfiguration is where the two meet: `update_nat_allocator` walks the live flow table re-reserving each active flow's allocation into a freshly built allocator, then publishes it, so two structures are read-modify-written with no single atomic step covering both. The invariant asserted is the one that matters regardless of how the window is synchronised: no two active flows may hold the same masquerading allocation, since two flows translated to the same address and port cannot have their replies told apart. Config shapes come from the `k8s-intf` generators, so what reaches the allocator is what the CRD can express. The window is narrower than it first appears, which is worth recording: the migration walk holds the flow table's read guard across the publish and insertion takes that lock exclusively, so no flow can appear mid-swap. What the guard does not cover is an already-present flow taking its allocation late, since per-flow NAT state sits behind the flow's own lock. Generated configs immediately turned up an unrelated live defect, captured here as an ignored reproducer: a validated config carrying an IPv6 masquerade range wider than a /96 panics while the allocator is *built*, with no packets and no threads involved. `apalloc::alloc::map_address` indexes a range's addresses with a `u32`, and validation never checks that a masquerade range is narrow enough to index, so this is reachable from operator input and aborts the process under the shipped `panic = "abort"`. The race test skips such configs so that it reports on the race it is about; the fix is a design decision (reject at validation, or cap the indexable range) and is left alone here. This is not a shuttle test, and the module says why at length: `FlowTable::insert` arms a per-flow expiry timer via `tokio::task::spawn`, so every path that creates a flow needs a live tokio runtime, which shuttle cannot schedule. Anything downstream of flow insertion is therefore outside a model checker's reach today -- a property of the flow table, not of this test. Real threads on a tokio runtime still buy a regression test for the invariant and a race detector under tsan, but they find a narrow window by luck rather than by construction. Giving the flow table a way to insert without arming a timer would let this move to the shuttle pattern unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
A validated config carrying an IPv6 masquerade `as` range wider than a /96 took the whole process down while the allocator was *built* -- no packets, no threads. Validation does not reject such a range, so this was reachable straight from operator input, and under the shipped `panic = "abort"` a config push could stop the dataplane. The cap was already the documented intent. The comment above `create_ipv6_bitmap_mappings` has always said that the bitmap indexes addresses with a `u32`, that at most 2^32 addresses per expose are therefore usable, and that anything beyond "we'll just ignore". The code did not do that: it capped by stopping iteration over *prefixes*, having already inserted the oversized one into the mapping, and the bitmap was then populated from the prefix's own extent. For a /64 that asked `map_address` for offset 2^64-1, and the `u32::try_from(..).unwrap()` there was the crash. Implement the cap that was described: - lay each IPv6 prefix out in the offset space with the number of addresses that actually fit, truncating the prefix that straddles the limit and dropping any that follow, and log a warning either way so an operator can see they got a subset of what they configured; - populate the bitmap from those windows rather than from prefix extents, which removes the only caller that could ask for an out-of-range offset; - make `map_address` fallible and drop its `expect`/`unwrap`. The last one matters beyond tidiness. `re_reserve_ip_and_port` feeds it an address allocated under the *previous* allocator during a config change, and a config change can move or drop the prefix that address came from. An error there invalidates one flow; a panic took the process. The windows are carried as inclusive `(first, last)` offsets rather than counts, because a full budget is 2^32 addresses -- one more than a `u32` holds. An earlier cut of this fix counted addresses and so silently dropped any prefix of exactly a /96 rather than truncating it; `test_prefix_of_exactly_the_budget_fills_the_bitmap` covers that boundary. The `nat` reconfiguration race test no longer skips configs with IPv6 masquerade ranges, so generated v6 configs reach the allocator again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds targeted concurrency testing around masquerade allocator reconfiguration and hardens IPv6 bitmap handling so allocator builds/migrations don’t panic on very wide prefixes (e.g., IPv6 /64), while expanding k8s-intf bolero generators to produce masquerade shapes needed by the new test.
Changes:
- Add a multi-threaded, property-based regression test that races
NatAllocatorWriter::update_nat_allocatoragainst datapath flow creation and asserts allocation uniqueness. - Rework IPv6 bitmap mapping to truncate to the
u32index budget instead of panicking, and adjust bitmap initialization to use mapped offset ranges. - Extend bolero CRD generators/coverage markers to generate and track masquerade exposes.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| nat/src/masquerade/test_concurrency.rs | New concurrency/property test racing allocator migration vs datapath and checking uniqueness invariant. |
| nat/src/masquerade/mod.rs | Wires the new test module into the masquerade module tree. |
| nat/src/masquerade/apalloc/setup.rs | Introduces Ipv6BitmapMappings, truncation logic, and uses mapped offset windows to populate the bitmap safely. |
| nat/src/masquerade/apalloc/natip_with_bitmap.rs | Adapts IPv6 offset mapping to the now-fallible map_address. |
| nat/src/masquerade/apalloc/alloc.rs | Adds IPv6 bitmap range helpers and makes map_address return Result instead of panicking. |
| nat/Cargo.toml | Adds dev-dependency on k8s-intf (bolero feature) and enables tokio multi-thread runtime for the new test. |
| k8s-intf/src/bolero/peering.rs | Adds generator control to allow masquerade on at most one side of a peering. |
| k8s-intf/src/bolero/expose.rs | Adds generator support for masquerade exposes in the “from_pool” (validation-legal subset) mode. |
| k8s-intf/src/bolero/crd.rs | Extends reachability/coverage tracking to ensure masquerade exposes are generated. |
| Cargo.lock | Adds dataplane-k8s-intf to the dependency graph (lockfile update). |
| let remaining = BUDGET - index; | ||
| if remaining == 0 { | ||
| warn!("Ran out of NAT bitmap space before prefix {p}; it will not be used"); | ||
| continue; |
Comment on lines
+499
to
+503
| .range(..=address_bits) | ||
| .next_back() | ||
| .expect("This should never fail"); | ||
| .ok_or_else(|| { | ||
| AllocatorError::InternalIssue(format!("Address {address} is below every mapped prefix")) | ||
| })?; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.