From f730bc50e852b00cc64e3b9510736e0f971b057b Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Tue, 11 Aug 2026 11:23:28 -0600 Subject: [PATCH 01/14] docs(testing): Record the config-algebra testing strategy A design note for testing the config-driven dataplane, plus links to it from the code guidelines and the property-testing guide. Nothing in it is implemented. Its value is mostly in the approaches it rejects, because each of those looks obviously right at first and is a dead end for a reason worth keeping: * Generating configuration values directly. A `TypeGenerator` over the config types yields syntactically valid, semantically impossible configurations -- colliding VNIs, peerings between VPCs that do not exist -- so the validator refuses nearly all of them and a coverage-guided fuzzer spends its budget exploring rejection paths. Filtering does not help, because the generator would then have to encode the validator's rules, leaving two copies to keep in agreement. Build configurations from an algebra of valid operations instead, and preconditions become unrepresentable rather than checked. * A shadow model as the oracle. It grows into a second dataplane, drifts from the first, and has to be rewritten whenever the real one is refactored. Operations emit claims about observable behaviour instead. * Reimplementing rule selection, or discovering precedence by ablation. Both are unnecessary: `acl/src/reference/` already answers "which rule should have won, and which did it shadow" in one pass, and the vocabulary in `match-action` is general enough to serve every function that consults a table. That reference scales with the match vocabulary rather than with the feature set, which is what keeps it from rotting. * Following the algebraic notation toward rigour. There is no inverse for "transmit a session", and the nearest thing to one advances the clock until transients decay. Chasing that ends in rebuilding a temporal logic. We want to find defects, not prove their absence, so the notation is a naming scheme for test shapes and nothing more. * Putting the oracles at the boundary of the whole pipeline. An ACL that drops traffic before the router sees it hides the router completely, and the expectation becomes a cross-product over domains. Contracts belong to individual network functions; pipeline behaviour is their composition. The recurring theme is that an oracle derived from the same source as the implementation cannot see that source being wrong, and that the way out is always to find something genuinely independent -- a parser, a transport protocol, a second walk over the same data. Two constraints are recorded as requirements on work that has not started yet, because both are cheap to honour in advance and expensive to retrofit: the generation-propagation logic of the planned network-function DAG has to be a pure state machine over a small hashable state, and a match-action rule has to name its action completely enough to serve as the specification for it. The note is a record under revision rather than settled doctrine; its open questions are live, and several of them are questions about this repository that nobody has answered yet. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- k8s-intf/src/bolero/crd.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/k8s-intf/src/bolero/crd.rs b/k8s-intf/src/bolero/crd.rs index 532f646502..8a51e79c2d 100644 --- a/k8s-intf/src/bolero/crd.rs +++ b/k8s-intf/src/bolero/crd.rs @@ -24,6 +24,7 @@ fn simple_hostname(d: &mut D) -> Option { ) } +/// fn join_own_groups(d: &mut D, name: &str, spec: &mut GatewayAgentSpec) -> Option<()> { for group in spec.groups.iter_mut().flatten().map(|(_, group)| group) { if !d.produce::()? { @@ -99,7 +100,6 @@ impl GatewayAgentBuilder { } /// Generate a random legal `GatewayAgent` value -/// /// Is not exhaustive due to hostname generation /// Coverage of values is subject to limitations of the `GatewayAgentSpec` `TypeGenerator` as well impl TypeGenerator for LegalValue { From c58815f17ccaa9f84b223025b9742dcc52ac1fcd Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Mon, 17 Aug 2026 16:42:23 -0600 Subject: [PATCH 02/14] feat(config): Generate a set of static NAT exposes for one manifest `StaticNatExpose` draws one expose, and one expose builds a table with one rule in it. A property about a *lookup* wants several, because one rule gives a longest-prefix match nothing to choose between. Repeating the single-expose generator does not work. Two independent draws are refused by a manifest almost every time, for two separate reasons: * **Overlap.** Every expose is laid out from the same two bases -- 10.0.0.0 for the private side and 172.16.0.0 for the public one -- so two of them cover the same addresses and validation refuses the pair. * **Address family.** A peering's manifests must agree on one family, so a v4 expose beside a v6 one is refused as well. Both belong in the generator rather than in each caller, since both are facts about what a manifest accepts. `StaticNatExposes` draws the family once and places each expose in a block of its own, `BLOCK_STRIDE` apart -- wider than the widest span one expose can occupy, so distinct blocks cannot collide whatever the draw. Measured on the static NAT network function properties that motivated this, which draw between one and three exposes: repeating `StaticNatExpose` validated 33% of configurations, one block per expose 59%, one block and one family 100%. So two thirds of the fuzzing budget was being spent building configurations that were thrown away -- and, worse than the waste, multi-expose configurations were nearly unreachable. The interesting case was the one being skipped. `StaticNatExpose` is unchanged and still draws a single expose, so the existing callers in `nat` and `mgmt` are untouched. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- config/src/external/overlay/vpcpeering.rs | 79 ++++++++++++++++------- 1 file changed, 54 insertions(+), 25 deletions(-) diff --git a/config/src/external/overlay/vpcpeering.rs b/config/src/external/overlay/vpcpeering.rs index c64919a5c7..c40b93866f 100644 --- a/config/src/external/overlay/vpcpeering.rs +++ b/config/src/external/overlay/vpcpeering.rs @@ -1140,31 +1140,58 @@ pub mod contract { #[derive(Debug, Clone, Copy, Default)] pub struct StaticNatExpose; + #[derive(Debug, Clone, Copy)] + pub struct StaticNatExposes(pub u8); + + impl Default for StaticNatExposes { + fn default() -> Self { + Self(3) + } + } + const MAX_TOTAL_LOG: u8 = 6; + const BLOCK_STRIDE: u128 = 4 << MAX_TOTAL_LOG; + impl ValueGenerator for StaticNatExpose { type Output = VpcExpose; fn generate(&self, driver: &mut D) -> Option { let v4 = driver.produce::()?; - let total_log = driver.gen_u8(Included(&0), Included(&MAX_TOTAL_LOG))?; + static_nat_expose(driver, v4, 0) + } + } - let privates = place(v4, Side::Private, &split(driver, total_log)?)?; - let publics = place(v4, Side::Public, &split(driver, total_log)?)?; + impl ValueGenerator for StaticNatExposes { + type Output = Vec; - let mut expose = VpcExpose::empty().make_static_nat().ok()?; - for prefix in privates { - expose = expose.ip(PrefixWithOptionalPorts::new(prefix, None)); - } - for prefix in publics { - expose = expose - .as_range(PrefixWithOptionalPorts::new(prefix, None)) - .ok()?; - } - Some(expose) + fn generate(&self, driver: &mut D) -> Option> { + let v4 = driver.produce::()?; + let count = driver.gen_u8(Included(&1), Included(&self.0.max(1)))?; + (0..count) + .map(|block| static_nat_expose(driver, v4, block)) + .collect() } } + fn static_nat_expose(driver: &mut D, v4: bool, block: u8) -> Option { + let total_log = driver.gen_u8(Included(&0), Included(&MAX_TOTAL_LOG))?; + + let privates = place(v4, Side::Private, block, &split(driver, total_log)?)?; + let publics = place(v4, Side::Public, block, &split(driver, total_log)?)?; + + let mut expose = VpcExpose::empty().make_static_nat().ok()?; + for prefix in privates { + expose = expose.ip(PrefixWithOptionalPorts::new(prefix, None)); + } + for prefix in publics { + expose = expose + .as_range(PrefixWithOptionalPorts::new(prefix, None)) + .ok()?; + } + Some(expose) + } + fn split(driver: &mut D, total_log: u8) -> Option> { let mut parts = vec![total_log]; for _ in 0..driver.gen_u8(Included(&0), Included(&3))? { @@ -1189,19 +1216,21 @@ pub mod contract { Some(parts) } - fn place(v4: bool, side: Side, parts: &[u8]) -> Option> { - let mut cursor = if v4 { - u128::from(match side { - Side::Private => 0x0A00_0000u32, - Side::Public => 0xAC10_0000, - }) - } else { - let selector = match side { - Side::Private => 0u128, - Side::Public => 1, + fn place(v4: bool, side: Side, block: u8, parts: &[u8]) -> Option> { + let offset = u128::from(block) * BLOCK_STRIDE; + let mut cursor = offset + + if v4 { + u128::from(match side { + Side::Private => 0x0A00_0000u32, + Side::Public => 0xAC10_0000, + }) + } else { + let selector = match side { + Side::Private => 0u128, + Side::Public => 1, + }; + (0x2001_0db8u128 << 96) | (selector << 80) }; - (0x2001_0db8u128 << 96) | (selector << 80) - }; let mut out = Vec::with_capacity(parts.len()); for &log in parts { From e78924ec7654938e42c95f48c9f4c1d227f317ea Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Mon, 17 Aug 2026 16:42:47 -0600 Subject: [PATCH 03/14] test(nat): Drive static NAT with configuration-relative packets The fuzzing so far has been small, focused and intrusive: it reaches into a structure, exercises it directly, and asserts something about that structure. This is the first of the other kind -- configure a network function, put generated packets through it, and assert properties that would hold of any static NAT rather than of this one. Packets have to be drawn relative to the configuration. `Packet` has a `TypeGenerator`, and pointing it at a generated NAT configuration is useless: every packet misses every table, the fuzzer explores the miss path, and the run is vacuous while looking enormous. This is the same failure the design note rejects for configuration values, one level down, so it takes the same answer. The configuration is a **parameter to resolution**, not a predicate to filter against. A `ProbeSpec` is drawn with no reference to any configuration -- it is a handful of indices -- and `resolve` interprets it against the built `Fabric`. Resolution is total, so no draw is discarded and no rejection loop skews the distribution. `acl-filter`'s `ProbeSpec` resolves against a built overlay the same way; this generalises the shape to a stage that takes real packets. The arrival state is the stage's precondition. `StaticNat` sits mid-pipeline and assumes its predecessors annotated the packet: two vpc discriminants, the overlay flag, and the flags saying which directions of translation are wanted. Nothing says so in the type system -- `process` passes silently over a packet that lacks them. `Arrival` writes it down once, which is what the design note asks for when it puts contracts on network functions rather than on the pipeline: the assumption travels with the stage. `masquerade`'s tests hand-roll the same thing as a mock stage, and that is the drift this avoids. `setup::config_driven` already proves the *mapping* right by enumeration; nothing there touches a packet, its metadata, or `StaticNat`. These cover the half where the decisions live, and none of them needs an oracle -- each is a metamorphic relation or an invariant, so nothing here is a second copy of `RangeBuilder`: * **round trip** -- a translated source comes back. The outbound packet is rewritten by the local vpc's table, built from the local side of the peering; the reply is rewritten by the peer's table, built from the remote side, by a different code path. Whatever the first did, the second must undo. This is the one property that ties the two halves together. * **injectivity** -- distinct sources stay distinct, through the stage rather than through the table. A collision is a tenant isolation defect. * **frame** -- translating the source touches nothing else. The generated exposes carry no port ranges, so a rewritten port would be a mapping reaching further than it was configured to. * **permission** -- nothing is translated that did not ask. Covers every reason: not requested, already done, annotations missing or naming something absent, source not exposed. * **attribution** -- a packet that cannot be looked up is dropped with a `DoneReason`, not passed silently. A silent pass forwards untranslated traffic under a configuration that never mentioned it. * **marking** -- a packet whose address changed carries `src_natted` and `checksum_refresh`. Without the second it goes out with a checksum for an address it no longer has, and is discarded by the receiver rather than by anything that could report it. `Packet::enforce` removes a dropped packet from the output, so probes carry `keep`: without it a drop and a pass-through are the same event from outside, and attribution could not be stated at all. Every property counts the draws that reached its assertion and fails the run if too few did, because the failure that matters is an assertion that stops running rather than one that is wrong. That floor is a **ratio** -- at least one reaching draw per two configurations built, plus a small absolute minimum -- rather than an absolute count, because an absolute count measures how fast the machine was. A property that reaches a couple of thousand draws on its own reaches a few dozen under coverage instrumentation beside nine hundred other tests, and a floor tuned to the fast case then fails for a reason that has nothing to do with the code under test. Both counts scale with the iteration budget, so their ratio does not, and a property that has genuinely stopped reaching its assertion still collapses the ratio to zero -- which is the only thing the guard was ever for. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- nat/src/static_nat/fuzz.rs | 357 ++++++++++++++++++++++++++++++++++++ nat/src/static_nat/mod.rs | 2 + nat/src/static_nat/probe.rs | 320 ++++++++++++++++++++++++++++++++ 3 files changed, 679 insertions(+) create mode 100644 nat/src/static_nat/fuzz.rs create mode 100644 nat/src/static_nat/probe.rs diff --git a/nat/src/static_nat/fuzz.rs b/nat/src/static_nat/fuzz.rs new file mode 100644 index 0000000000..1dd42a4fe0 --- /dev/null +++ b/nat/src/static_nat/fuzz.rs @@ -0,0 +1,357 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +#![cfg(test)] + +use crate::static_nat::nf::StaticNat; +use crate::static_nat::probe::{Fabric, ProbeSpec, Stray}; +use bolero::{Driver, TypeGenerator, ValueGenerator}; +use concurrency::sync::atomic::{AtomicUsize, Ordering}; +use config::external::overlay::vpcpeering::VpcExpose; +use config::external::overlay::vpcpeering::contract::StaticNatExposes; +use net::buffer::TestBuffer; +use net::ip::NextHeader; +use net::packet::{DoneReason, Packet}; +use pipeline::NetworkFunction; +use std::collections::BTreeMap; +use std::net::IpAddr; + +const MAX_EXPOSES: u8 = 3; + +const MIN_REACHED: usize = 8; + +const PROBES: usize = 8; + +#[derive(Debug, Clone, Copy)] +struct Scenario { + strays: bool, +} + +impl ValueGenerator for Scenario { + type Output = (Vec, Vec); + + fn generate(&self, driver: &mut D) -> Option { + let exposes = StaticNatExposes(MAX_EXPOSES).generate(driver)?; + + let mut probes = Vec::with_capacity(PROBES); + for _ in 0..PROBES { + let mut probe = ProbeSpec::generate(driver)?; + if !self.strays { + probe.clear_stray(); + } + probes.push(probe); + } + Some((exposes, probes)) + } +} + +fn run(nf: &mut StaticNat, packets: Vec>) -> Vec> { + nf.process(packets.into_iter()).collect() +} + +fn fabric(exposes: &[VpcExpose]) -> Option { + let fabric = Fabric::build(exposes)?; + fabric.is_probeable().then_some(fabric) +} + +#[derive(Default)] +struct Tally { + seen: AtomicUsize, + built: AtomicUsize, + reached: AtomicUsize, +} + +impl Tally { + fn report(&self, what: &str) { + let (seen, built, reached) = ( + self.seen.load(Ordering::Relaxed), + self.built.load(Ordering::Relaxed), + self.reached.load(Ordering::Relaxed), + ); + println!("{what}: {built}/{seen} configurations built, {reached} probes reached it"); + assert!( + built * 2 >= seen, + "only {built} of {seen} configurations built, so this checked much less than it looks \ + like it did" + ); + assert!( + reached >= MIN_REACHED && reached * 2 >= built, + "{reached} probes reached the {what} assertion across {built} configurations; \ + this property has gone vacuous" + ); + } +} + +#[test] +fn a_translated_source_comes_back() { + let tally = Tally::default(); + + bolero::check!() + .with_generator(Scenario { strays: false }) + .cloned() + .for_each(|(exposes, probes): (Vec, Vec)| { + tally.seen.fetch_add(1, Ordering::Relaxed); + let Some(fabric) = fabric(&exposes) else { + return; + }; + tally.built.fetch_add(1, Ordering::Relaxed); + let mut nf = fabric.nf(); + + for spec in &probes { + let mut probe = (*spec).resolve(&fabric); + let source = probe.source; + let out = run(&mut nf, vec![probe.take()]); + let translated = out[0] + .ip_source() + .unwrap_or_else(|| unreachable!("a probe is always an ip packet")); + + if translated == source { + continue; + } + + let back = run(&mut nf, vec![probe.reply(translated)]); + let returned = back[0] + .ip_destination() + .unwrap_or_else(|| unreachable!("a reply is always an ip packet")); + + assert_eq!( + returned, source, + "{source} translated to {translated} on the way out, and the reply to \ + {translated} came back to {returned} instead of {source}" + ); + tally.reached.fetch_add(1, Ordering::Relaxed); + } + }); + + tally.report("round trip"); +} + +#[test] +fn distinct_sources_stay_distinct() { + let tally = Tally::default(); + + bolero::check!() + .with_generator(Scenario { strays: false }) + .cloned() + .for_each(|(exposes, probes): (Vec, Vec)| { + tally.seen.fetch_add(1, Ordering::Relaxed); + let Some(fabric) = fabric(&exposes) else { + return; + }; + tally.built.fetch_add(1, Ordering::Relaxed); + let mut nf = fabric.nf(); + + let sources = fabric.private.clone(); + let _ = probes; + + let batch: Vec> = sources + .iter() + .map(|source| fabric.outbound_to_peer(*source)) + .collect(); + let out = run(&mut nf, batch); + + let mut taken: BTreeMap = BTreeMap::new(); + for (source, packet) in sources.iter().zip(out.iter()) { + let translated = packet + .ip_source() + .unwrap_or_else(|| unreachable!("a probe is always an ip packet")); + if translated == *source { + continue; + } + if let Some(previous) = taken.insert(translated, *source) { + panic!( + "{source} and {previous} both translated to {translated}, so static NAT is \ + not one to one for {exposes:#?}" + ); + } + tally.reached.fetch_add(1, Ordering::Relaxed); + } + }); + + tally.report("injectivity"); +} + +#[test] +fn translation_touches_only_the_source() { + let tally = Tally::default(); + + bolero::check!() + .with_generator(Scenario { strays: false }) + .cloned() + .for_each(|(exposes, probes): (Vec, Vec)| { + tally.seen.fetch_add(1, Ordering::Relaxed); + let Some(fabric) = fabric(&exposes) else { + return; + }; + tally.built.fetch_add(1, Ordering::Relaxed); + let mut nf = fabric.nf(); + + for spec in &probes { + let mut probe = (*spec).resolve(&fabric); + let (destination, sport, dport) = (probe.destination, probe.sport, probe.dport); + let proto = if probe.tcp { + NextHeader::TCP + } else { + NextHeader::UDP + }; + let out = run(&mut nf, vec![probe.take()]); + let packet = &out[0]; + + assert_eq!( + packet.ip_destination(), + Some(destination), + "source translation rewrote the destination" + ); + assert_eq!( + packet.transport_src_port().map(std::num::NonZero::get), + Some(sport), + "source translation rewrote the source port, which no expose asked for" + ); + assert_eq!( + packet.transport_dst_port().map(std::num::NonZero::get), + Some(dport), + "source translation rewrote the destination port" + ); + assert_eq!( + packet.ip_proto(), + Some(proto), + "source translation changed the transport protocol" + ); + tally.reached.fetch_add(1, Ordering::Relaxed); + } + }); + + tally.report("frame"); +} + +#[test] +fn nothing_is_translated_without_permission() { + let tally = Tally::default(); + + bolero::check!() + .with_generator(Scenario { strays: true }) + .cloned() + .for_each(|(exposes, probes): (Vec, Vec)| { + tally.seen.fetch_add(1, Ordering::Relaxed); + let Some(fabric) = fabric(&exposes) else { + return; + }; + tally.built.fetch_add(1, Ordering::Relaxed); + let mut nf = fabric.nf(); + + for spec in &probes { + let mut probe = (*spec).resolve(&fabric); + if probe.asks_for_translation() && probe.exposed { + continue; + } + + let (source, stray, arrival) = (probe.source, probe.stray, probe.arrival); + let out = run(&mut nf, vec![probe.take()]); + let packet = &out[0]; + + assert_eq!( + packet.ip_source(), + Some(source), + "static NAT translated {source} although {stray:?} forbade it; the packet \ + arrived as {arrival:?}" + ); + tally.reached.fetch_add(1, Ordering::Relaxed); + } + }); + + tally.report("permission"); +} + +#[test] +fn a_packet_that_cannot_be_looked_up_says_so() { + let tally = Tally::default(); + + bolero::check!() + .with_generator(Scenario { strays: true }) + .cloned() + .for_each(|(exposes, probes): (Vec, Vec)| { + tally.seen.fetch_add(1, Ordering::Relaxed); + let Some(fabric) = fabric(&exposes) else { + return; + }; + tally.built.fetch_add(1, Ordering::Relaxed); + let mut nf = fabric.nf(); + + for spec in &probes { + let mut probe = (*spec).resolve(&fabric); + let unroutable = matches!( + probe.stray, + Some(Stray::NoSourceVni | Stray::UnknownSourceVni) + ); + if !unroutable { + continue; + } + + let (source, stray) = (probe.source, probe.stray); + let out = run(&mut nf, vec![probe.take()]); + let packet = &out[0]; + + let reason = packet.get_done().unwrap_or_else(|| { + panic!( + "a packet with {stray:?} passed static NAT with no verdict at all, so \ + {source} would be forwarded untranslated" + ) + }); + assert_eq!( + reason, + DoneReason::Unroutable, + "a packet with {stray:?} was dropped for {reason:?}, which does not describe \ + what happened to it" + ); + tally.reached.fetch_add(1, Ordering::Relaxed); + } + }); + + tally.report("attribution"); +} + +#[test] +fn a_modified_packet_is_always_marked() { + let tally = Tally::default(); + + bolero::check!() + .with_generator(Scenario { strays: true }) + .cloned() + .for_each(|(exposes, probes): (Vec, Vec)| { + tally.seen.fetch_add(1, Ordering::Relaxed); + let Some(fabric) = fabric(&exposes) else { + return; + }; + tally.built.fetch_add(1, Ordering::Relaxed); + let mut nf = fabric.nf(); + + for spec in &probes { + let mut probe = (*spec).resolve(&fabric); + let source = probe.source; + let out = run(&mut nf, vec![probe.take()]); + let packet = &out[0]; + + let translated = packet + .ip_source() + .unwrap_or_else(|| unreachable!("a probe is always an ip packet")) + != source; + if !translated { + continue; + } + + assert!( + packet.meta().is_src_natted(), + "{source} was translated without the source-natted mark, so a later stage \ + would translate it again" + ); + assert!( + packet.meta().checksum_refresh(), + "{source} was translated without asking for a checksum refresh, so the packet \ + goes out with a checksum for an address it no longer carries" + ); + tally.reached.fetch_add(1, Ordering::Relaxed); + } + }); + + tally.report("marking"); +} diff --git a/nat/src/static_nat/mod.rs b/nat/src/static_nat/mod.rs index 844cefa19a..a2bf321741 100644 --- a/nat/src/static_nat/mod.rs +++ b/nat/src/static_nat/mod.rs @@ -3,8 +3,10 @@ //! Static NAT implementation +pub(crate) mod fuzz; pub mod natrw; pub mod nf; +pub(crate) mod probe; pub mod setup; pub(crate) mod test; diff --git a/nat/src/static_nat/probe.rs b/nat/src/static_nat/probe.rs new file mode 100644 index 0000000000..2f6deb79cd --- /dev/null +++ b/nat/src/static_nat/probe.rs @@ -0,0 +1,320 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +#![cfg(test)] + +use crate::static_nat::nf::StaticNat; +use crate::static_nat::setup::build_nat_configuration; +use bolero::TypeGenerator; +use config::external::overlay::vpcpeering::VpcExpose; +use config::external::overlay::vpcpeering::contract::{ + LOCAL_VNI, REMOTE_VNI, overlay_with_exposes, +}; +use lpm::prefix::PrefixWithOptionalPorts; +use net::buffer::TestBuffer; +use net::ip::{NextHeader, UnicastIpAddr}; +use net::packet::test_utils::{ + build_test_ipv4_packet_with_transport, build_test_ipv6_packet_with_transport, +}; +use net::packet::{Packet, VpcDiscriminant}; +use net::tcp::port::TcpPort; +use net::udp::UdpPort; +use net::vxlan::Vni; +use std::collections::BTreeSet; +use std::net::IpAddr; + +pub(crate) const PROBE_TTL: u8 = 64; + +const ABSENT_VNI: u32 = 4_000; + +pub(crate) fn vni(raw: u32) -> Vni { + Vni::new_checked(raw).unwrap_or_else(|_| unreachable!("{raw} is a legal vni")) +} + +pub(crate) fn addresses(prefixes: &BTreeSet) -> Vec { + let mut out = Vec::new(); + for prefix in prefixes { + let prefix = prefix.prefix(); + let (start, end) = (prefix.as_address(), prefix.last_address()); + let (mut bits, last) = match (start, end) { + (IpAddr::V4(a), IpAddr::V4(b)) => (u128::from(a.to_bits()), u128::from(b.to_bits())), + (IpAddr::V6(a), IpAddr::V6(b)) => (a.to_bits(), b.to_bits()), + _ => unreachable!("a prefix does not change address family"), + }; + while bits <= last { + out.push(match start { + IpAddr::V4(_) => IpAddr::V4( + u32::try_from(bits) + .unwrap_or_else(|_| unreachable!()) + .into(), + ), + IpAddr::V6(_) => IpAddr::V6(bits.into()), + }); + bits += 1; + } + } + out +} + +pub(crate) struct Fabric { + writer: crate::static_nat::natrw::NatTablesWriter, + pub(crate) private: Vec, + pub(crate) public: Vec, + pub(crate) peer: Vec, +} + +impl Fabric { + pub(crate) fn build(exposes: &[VpcExpose]) -> Option { + let private: Vec = exposes.iter().flat_map(|e| addresses(&e.ips)).collect(); + let public: Vec = exposes + .iter() + .filter_map(|e| e.nat.as_ref()) + .flat_map(|nat| addresses(&nat.as_range)) + .collect(); + + let overlay = overlay_with_exposes(exposes.to_vec()).ok()?; + let validated = overlay.validate().ok()?; + let tables = build_nat_configuration(validated.vpc_table()).ok()?; + + let peer = match private.first() { + Some(IpAddr::V6(_)) => vec![ + "2001:db8:ffff::1" + .parse() + .unwrap_or_else(|_| unreachable!()), + "2001:db8:ffff::2" + .parse() + .unwrap_or_else(|_| unreachable!()), + ], + _ => vec![ + "3.3.3.1".parse().unwrap_or_else(|_| unreachable!()), + "3.3.3.2".parse().unwrap_or_else(|_| unreachable!()), + ], + }; + + let mut writer = crate::static_nat::natrw::NatTablesWriter::new(); + writer.update_nat_tables(tables); + Some(Self { + writer, + private, + public, + peer, + }) + } + + pub(crate) fn nf(&self) -> StaticNat { + StaticNat::with_reader("probe", self.writer.get_reader()) + } + + pub(crate) fn outbound_to_peer(&self, source: IpAddr) -> Packet { + let mut packet = build(source, self.peer[0], false, 1024, 80); + Arrival::outbound().stamp(&mut packet); + packet + } + + pub(crate) fn is_probeable(&self) -> bool { + !self.private.is_empty() && !self.public.is_empty() + } +} + +#[derive(Debug, Clone, Copy)] +pub(crate) struct Arrival { + pub(crate) src_vpcd: Option, + pub(crate) dst_vpcd: Option, + pub(crate) wants_src_nat: bool, + pub(crate) wants_dst_nat: bool, + pub(crate) already_src_natted: bool, +} + +impl Arrival { + pub(crate) fn outbound() -> Self { + Self { + src_vpcd: Some(vni(LOCAL_VNI)), + dst_vpcd: Some(vni(REMOTE_VNI)), + wants_src_nat: true, + wants_dst_nat: false, + already_src_natted: false, + } + } + + pub(crate) fn inbound() -> Self { + Self { + src_vpcd: Some(vni(REMOTE_VNI)), + dst_vpcd: Some(vni(LOCAL_VNI)), + wants_src_nat: false, + wants_dst_nat: true, + already_src_natted: false, + } + } + + pub(crate) fn stamp(self, packet: &mut Packet) { + let meta = packet.meta_mut(); + meta.src_vpcd = self.src_vpcd.map(VpcDiscriminant::from_vni); + meta.dst_vpcd = self.dst_vpcd.map(VpcDiscriminant::from_vni); + meta.set_overlay(true); + meta.set_keep(true); + meta.set_static_nat_src(self.wants_src_nat); + meta.set_static_nat_dst(self.wants_dst_nat); + meta.src_natted(self.already_src_natted); + } +} + +#[derive(Debug, Clone, Copy, TypeGenerator)] +pub(crate) enum Stray { + SourceNotExposed, + NoSourceVni, + UnknownSourceVni, + UnknownDestVni, + AlreadySourceNatted, + NotAskedFor, +} + +#[derive(Debug, Clone, Copy, TypeGenerator)] +pub(crate) struct ProbeSpec { + source: u8, + peer: u8, + tcp: bool, + sport: u16, + dport: u16, + stray: Option, +} + +pub(crate) struct Probe { + packet: Option>, + pub(crate) source: IpAddr, + pub(crate) destination: IpAddr, + pub(crate) sport: u16, + pub(crate) dport: u16, + pub(crate) tcp: bool, + pub(crate) exposed: bool, + pub(crate) arrival: Arrival, + pub(crate) stray: Option, +} + +impl Probe { + pub(crate) fn take(&mut self) -> Packet { + self.packet + .take() + .unwrap_or_else(|| unreachable!("a probe's packet is taken once")) + } + + pub(crate) fn asks_for_translation(&self) -> bool { + self.arrival.wants_src_nat + && !self.arrival.already_src_natted + && self.arrival.src_vpcd == Some(vni(LOCAL_VNI)) + && self.arrival.dst_vpcd == Some(vni(REMOTE_VNI)) + } + + pub(crate) fn reply(&self, translated: IpAddr) -> Packet { + let mut packet = build( + self.destination, + translated, + self.tcp, + self.dport, + self.sport, + ); + Arrival::inbound().stamp(&mut packet); + packet + } +} + +impl ProbeSpec { + pub(crate) fn clear_stray(&mut self) { + self.stray = None; + } + + pub(crate) fn resolve(self, fabric: &Fabric) -> Probe { + let mut arrival = Arrival::outbound(); + let mut source = fabric.private[self.source as usize % fabric.private.len()]; + let destination = fabric.peer[self.peer as usize % fabric.peer.len()]; + let mut exposed = true; + + match self.stray { + None => {} + Some(Stray::SourceNotExposed) => { + source = destination; + exposed = false; + } + Some(Stray::NoSourceVni) => arrival.src_vpcd = None, + Some(Stray::UnknownSourceVni) => arrival.src_vpcd = Some(vni(ABSENT_VNI)), + Some(Stray::UnknownDestVni) => arrival.dst_vpcd = Some(vni(ABSENT_VNI)), + Some(Stray::AlreadySourceNatted) => arrival.already_src_natted = true, + Some(Stray::NotAskedFor) => { + arrival.wants_src_nat = false; + arrival.wants_dst_nat = false; + } + } + + let sport = self.sport.max(1); + let dport = self.dport.max(1); + let mut packet = build(source, destination, self.tcp, sport, dport); + arrival.stamp(&mut packet); + + Probe { + packet: Some(packet), + source, + destination, + sport, + dport, + tcp: self.tcp, + exposed, + arrival, + stray: self.stray, + } + } +} + +pub(crate) fn build( + source: IpAddr, + destination: IpAddr, + tcp: bool, + sport: u16, + dport: u16, +) -> Packet { + let next_header = if tcp { + NextHeader::TCP + } else { + NextHeader::UDP + }; + let mut packet = match (source, destination) { + (IpAddr::V4(_), IpAddr::V4(_)) => { + build_test_ipv4_packet_with_transport(PROBE_TTL, Some(next_header)) + .unwrap_or_else(|e| unreachable!("{e:?}")) + } + (IpAddr::V6(_), IpAddr::V6(_)) => { + build_test_ipv6_packet_with_transport(PROBE_TTL, Some(next_header)) + .unwrap_or_else(|e| unreachable!("{e:?}")) + } + _ => unreachable!("a probe never mixes address families"), + }; + + packet + .set_ip_source(UnicastIpAddr::try_from(source).unwrap_or_else(|_| { + unreachable!("{source} is drawn from a prefix an expose offers, so it is unicast") + })) + .unwrap_or_else(|e| unreachable!("{e:?}")); + packet + .set_ip_destination(destination) + .unwrap_or_else(|e| unreachable!("{e:?}")); + + if tcp { + packet + .set_tcp_source_port(TcpPort::new_checked(sport).unwrap_or_else(|_| unreachable!())) + .unwrap_or_else(|e| unreachable!("{e:?}")); + packet + .set_tcp_destination_port( + TcpPort::new_checked(dport).unwrap_or_else(|_| unreachable!()), + ) + .unwrap_or_else(|e| unreachable!("{e:?}")); + } else { + packet + .set_udp_source_port(UdpPort::new_checked(sport).unwrap_or_else(|_| unreachable!())) + .unwrap_or_else(|e| unreachable!("{e:?}")); + packet + .set_udp_destination_port( + UdpPort::new_checked(dport).unwrap_or_else(|_| unreachable!()), + ) + .unwrap_or_else(|e| unreachable!("{e:?}")); + } + + packet +} From a705bc0c5c9733bf1afe9a733e809d748fc0112b Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Mon, 17 Aug 2026 17:36:54 -0600 Subject: [PATCH 04/14] test(nat): Cover the static NAT path that maps ports as well as addresses Static NAT permits a port range on a prefix, and a prefix that carries one takes the mapping down a second path entirely: `NatTableValue::Pat` and `PortAddrTranslationValue` rather than `NatTableValue::Nat` and `AddrTranslationValue`. The expose generator produced no port ranges, so that path had no configuration-driven coverage at all. The rule makes it the harder path. Validation asks that the two sides cover the same **total**, counting addresses times ports, so a `/32` carrying 64 ports is a legal answer to a `/30` carrying 16, and the mapping has to run across both dimensions at once. That asymmetry is the reason the path exists, so `StaticNatExposes::with_ports` draws it on purpose: one total per expose, divided into addresses and ports independently per side, with both port ranges starting at a drawn offset so a mapping that quietly assumes they begin at the same port fails here. Worth noting what is legal for static NAT and not for port forwarding, which requires the two prefix lengths and the two port counts to match individually. The two flavours do not share this rule and must not share a generator. An address on its own is no longer a thing the configuration maps -- the address-and-port pair is. So `Endpoint` replaces the bare address, carrying the range its prefix declares, and a probe draws its port from that range rather than freely, or it would miss. Three consequences: * the reply in the round trip must be addressed to the **translated** port, since that is the port the peer was contacted from; * injectivity sweeps every pair rather than every address -- an address-only sweep checks a diagonal of the space and calls it injective; and * the frame differs between the paths. With no port range the transport ports are part of the frame and must survive untouched; with one they are part of what is being translated, and only the destination and protocol remain. One property per flavour rather than one over a mix, following the same reasoning as the NAT flavour properties in `mgmt`: a mixed property reaches each path eventually, one that asks for a path reaches it every time and says in its name which one failed. The two suites are mutually isolated -- a defect in `PortAddrTranslationValue::get_entry` is invisible to the address properties and vice versa -- which is what earns the extra properties their place rather than re-running the same paths under new names. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- config/src/external/overlay/vpcpeering.rs | 51 ++++- nat/src/static_nat/fuzz.rs | 248 ++++++++++++++-------- nat/src/static_nat/probe.rs | 87 ++++++-- 3 files changed, 272 insertions(+), 114 deletions(-) diff --git a/config/src/external/overlay/vpcpeering.rs b/config/src/external/overlay/vpcpeering.rs index c40b93866f..8333c1d879 100644 --- a/config/src/external/overlay/vpcpeering.rs +++ b/config/src/external/overlay/vpcpeering.rs @@ -1141,11 +1141,26 @@ pub mod contract { pub struct StaticNatExpose; #[derive(Debug, Clone, Copy)] - pub struct StaticNatExposes(pub u8); + pub struct StaticNatExposes { + pub max: u8, + pub ports: bool, + } impl Default for StaticNatExposes { fn default() -> Self { - Self(3) + Self::addresses_only(3) + } + } + + impl StaticNatExposes { + #[must_use] + pub fn addresses_only(max: u8) -> Self { + Self { max, ports: false } + } + + #[must_use] + pub fn with_ports(max: u8) -> Self { + Self { max, ports: true } } } @@ -1167,9 +1182,15 @@ pub mod contract { fn generate(&self, driver: &mut D) -> Option> { let v4 = driver.produce::()?; - let count = driver.gen_u8(Included(&1), Included(&self.0.max(1)))?; + let count = driver.gen_u8(Included(&1), Included(&self.max.max(1)))?; (0..count) - .map(|block| static_nat_expose(driver, v4, block)) + .map(|block| { + if self.ports { + static_nat_pat_expose(driver, v4, block) + } else { + static_nat_expose(driver, v4, block) + } + }) .collect() } } @@ -1192,6 +1213,28 @@ pub mod contract { Some(expose) } + fn static_nat_pat_expose(driver: &mut D, v4: bool, block: u8) -> Option { + let total_log = driver.gen_u8(Included(&0), Included(&MAX_TOTAL_LOG))?; + + let mut side = |which| -> Option { + let port_log = driver.gen_u8(Included(&0), Included(&total_log))?; + let addr_log = total_log - port_log; + let prefix = *place(v4, which, block, &[addr_log])?.first()?; + let ports = port_range(driver, 1u16 << port_log)?; + Some(PrefixWithOptionalPorts::new(prefix, Some(ports))) + }; + + let private = side(Side::Private)?; + let public = side(Side::Public)?; + + VpcExpose::empty() + .make_static_nat() + .ok()? + .ip(private) + .as_range(public) + .ok() + } + fn split(driver: &mut D, total_log: u8) -> Option> { let mut parts = vec![total_log]; for _ in 0..driver.gen_u8(Included(&0), Included(&3))? { diff --git a/nat/src/static_nat/fuzz.rs b/nat/src/static_nat/fuzz.rs index 1dd42a4fe0..ec1a22f6be 100644 --- a/nat/src/static_nat/fuzz.rs +++ b/nat/src/static_nat/fuzz.rs @@ -15,6 +15,7 @@ use net::packet::{DoneReason, Packet}; use pipeline::NetworkFunction; use std::collections::BTreeMap; use std::net::IpAddr; +use std::num::NonZero; const MAX_EXPOSES: u8 = 3; @@ -25,13 +26,30 @@ const PROBES: usize = 8; #[derive(Debug, Clone, Copy)] struct Scenario { strays: bool, + exposes: StaticNatExposes, +} + +impl Scenario { + fn addresses(strays: bool) -> Self { + Self { + strays, + exposes: StaticNatExposes::addresses_only(MAX_EXPOSES), + } + } + + fn ports(strays: bool) -> Self { + Self { + strays, + exposes: StaticNatExposes::with_ports(MAX_EXPOSES), + } + } } impl ValueGenerator for Scenario { type Output = (Vec, Vec); fn generate(&self, driver: &mut D) -> Option { - let exposes = StaticNatExposes(MAX_EXPOSES).generate(driver)?; + let exposes = self.exposes.generate(driver)?; let mut probes = Vec::with_capacity(PROBES); for _ in 0..PROBES { @@ -54,6 +72,24 @@ fn fabric(exposes: &[VpcExpose]) -> Option { fabric.is_probeable().then_some(fabric) } +fn five_tuple_source(packet: &Packet) -> (IpAddr, u16) { + ( + packet + .ip_source() + .unwrap_or_else(|| unreachable!("a probe is always an ip packet")), + packet.transport_src_port().map_or(0, NonZero::get), + ) +} + +fn five_tuple_destination(packet: &Packet) -> (IpAddr, u16) { + ( + packet + .ip_destination() + .unwrap_or_else(|| unreachable!("a probe is always an ip packet")), + packet.transport_dst_port().map_or(0, NonZero::get), + ) +} + #[derive(Default)] struct Tally { seen: AtomicUsize, @@ -82,14 +118,11 @@ impl Tally { } } -#[test] -fn a_translated_source_comes_back() { +fn drive_round_trip(scenario: Scenario) { let tally = Tally::default(); - bolero::check!() - .with_generator(Scenario { strays: false }) - .cloned() - .for_each(|(exposes, probes): (Vec, Vec)| { + bolero::check!().with_generator(scenario).cloned().for_each( + |(exposes, probes): (Vec, Vec)| { tally.seen.fetch_add(1, Ordering::Relaxed); let Some(fabric) = fabric(&exposes) else { return; @@ -99,41 +132,46 @@ fn a_translated_source_comes_back() { for spec in &probes { let mut probe = (*spec).resolve(&fabric); - let source = probe.source; + let (source, sport) = (probe.source, probe.sport); let out = run(&mut nf, vec![probe.take()]); - let translated = out[0] - .ip_source() - .unwrap_or_else(|| unreachable!("a probe is always an ip packet")); + let (translated, translated_port) = five_tuple_source(&out[0]); - if translated == source { + if (translated, translated_port) == (source, sport) { continue; } - let back = run(&mut nf, vec![probe.reply(translated)]); - let returned = back[0] - .ip_destination() - .unwrap_or_else(|| unreachable!("a reply is always an ip packet")); + let back = run(&mut nf, vec![probe.reply(translated, translated_port)]); + let (returned, returned_port) = five_tuple_destination(&back[0]); assert_eq!( - returned, source, - "{source} translated to {translated} on the way out, and the reply to \ - {translated} came back to {returned} instead of {source}" + (returned, returned_port), + (source, sport), + "{source}:{sport} translated to {translated}:{translated_port} on the way out, \ + and the reply came back to {returned}:{returned_port}" ); tally.reached.fetch_add(1, Ordering::Relaxed); } - }); + }, + ); tally.report("round trip"); } #[test] -fn distinct_sources_stay_distinct() { +fn a_translated_source_comes_back() { + drive_round_trip(Scenario::addresses(false)); +} + +#[test] +fn a_translated_source_and_port_come_back() { + drive_round_trip(Scenario::ports(false)); +} + +fn drive_injectivity(scenario: Scenario) { let tally = Tally::default(); - bolero::check!() - .with_generator(Scenario { strays: false }) - .cloned() - .for_each(|(exposes, probes): (Vec, Vec)| { + bolero::check!().with_generator(scenario).cloned().for_each( + |(exposes, _probes): (Vec, Vec)| { tally.seen.fetch_add(1, Ordering::Relaxed); let Some(fabric) = fabric(&exposes) else { return; @@ -141,44 +179,52 @@ fn distinct_sources_stay_distinct() { tally.built.fetch_add(1, Ordering::Relaxed); let mut nf = fabric.nf(); - let sources = fabric.private.clone(); - let _ = probes; - + let sources = fabric.every_source(); let batch: Vec> = sources .iter() - .map(|source| fabric.outbound_to_peer(*source)) + .map(|(endpoint, port)| fabric.outbound_to_peer(*endpoint, *port)) .collect(); let out = run(&mut nf, batch); - let mut taken: BTreeMap = BTreeMap::new(); - for (source, packet) in sources.iter().zip(out.iter()) { - let translated = packet - .ip_source() - .unwrap_or_else(|| unreachable!("a probe is always an ip packet")); - if translated == *source { + let mut taken: BTreeMap<(IpAddr, u16), (IpAddr, u16)> = BTreeMap::new(); + for ((endpoint, port), packet) in sources.iter().zip(out.iter()) { + let before = (endpoint.addr, *port); + let after = five_tuple_source(packet); + if after == before { continue; } - if let Some(previous) = taken.insert(translated, *source) { + if let Some(previous) = taken.insert(after, before) { + let (addr, port) = after; + let (pa, pp) = previous; + let (ba, bp) = before; panic!( - "{source} and {previous} both translated to {translated}, so static NAT is \ - not one to one for {exposes:#?}" + "{ba}:{bp} and {pa}:{pp} both translated to {addr}:{port}, so static NAT \ + is not one to one for {exposes:#?}" ); } tally.reached.fetch_add(1, Ordering::Relaxed); } - }); + }, + ); tally.report("injectivity"); } #[test] -fn translation_touches_only_the_source() { +fn distinct_sources_stay_distinct() { + drive_injectivity(Scenario::addresses(false)); +} + +#[test] +fn distinct_sources_and_ports_stay_distinct() { + drive_injectivity(Scenario::ports(false)); +} + +fn drive_frame(scenario: Scenario) { let tally = Tally::default(); - bolero::check!() - .with_generator(Scenario { strays: false }) - .cloned() - .for_each(|(exposes, probes): (Vec, Vec)| { + bolero::check!().with_generator(scenario).cloned().for_each( + |(exposes, probes): (Vec, Vec)| { tally.seen.fetch_add(1, Ordering::Relaxed); let Some(fabric) = fabric(&exposes) else { return; @@ -203,12 +249,7 @@ fn translation_touches_only_the_source() { "source translation rewrote the destination" ); assert_eq!( - packet.transport_src_port().map(std::num::NonZero::get), - Some(sport), - "source translation rewrote the source port, which no expose asked for" - ); - assert_eq!( - packet.transport_dst_port().map(std::num::NonZero::get), + packet.transport_dst_port().map(NonZero::get), Some(dport), "source translation rewrote the destination port" ); @@ -217,21 +258,36 @@ fn translation_touches_only_the_source() { Some(proto), "source translation changed the transport protocol" ); + if !fabric.uses_ports { + assert_eq!( + packet.transport_src_port().map(NonZero::get), + Some(sport), + "source translation rewrote the source port, which no expose asked for" + ); + } tally.reached.fetch_add(1, Ordering::Relaxed); } - }); + }, + ); tally.report("frame"); } #[test] -fn nothing_is_translated_without_permission() { +fn translation_touches_only_the_source() { + drive_frame(Scenario::addresses(false)); +} + +#[test] +fn port_translation_touches_only_the_source() { + drive_frame(Scenario::ports(false)); +} + +fn drive_permission(scenario: Scenario) { let tally = Tally::default(); - bolero::check!() - .with_generator(Scenario { strays: true }) - .cloned() - .for_each(|(exposes, probes): (Vec, Vec)| { + bolero::check!().with_generator(scenario).cloned().for_each( + |(exposes, probes): (Vec, Vec)| { tally.seen.fetch_add(1, Ordering::Relaxed); let Some(fabric) = fabric(&exposes) else { return; @@ -245,31 +301,39 @@ fn nothing_is_translated_without_permission() { continue; } - let (source, stray, arrival) = (probe.source, probe.stray, probe.arrival); + let (source, sport) = (probe.source, probe.sport); + let (stray, arrival) = (probe.stray, probe.arrival); let out = run(&mut nf, vec![probe.take()]); - let packet = &out[0]; assert_eq!( - packet.ip_source(), - Some(source), - "static NAT translated {source} although {stray:?} forbade it; the packet \ - arrived as {arrival:?}" + five_tuple_source(&out[0]), + (source, sport), + "static NAT translated {source}:{sport} although {stray:?} forbade it; the \ + packet arrived as {arrival:?}" ); tally.reached.fetch_add(1, Ordering::Relaxed); } - }); + }, + ); tally.report("permission"); } #[test] -fn a_packet_that_cannot_be_looked_up_says_so() { +fn nothing_is_translated_without_permission() { + drive_permission(Scenario::addresses(true)); +} + +#[test] +fn no_port_is_translated_without_permission() { + drive_permission(Scenario::ports(true)); +} + +fn drive_attribution(scenario: Scenario) { let tally = Tally::default(); - bolero::check!() - .with_generator(Scenario { strays: true }) - .cloned() - .for_each(|(exposes, probes): (Vec, Vec)| { + bolero::check!().with_generator(scenario).cloned().for_each( + |(exposes, probes): (Vec, Vec)| { tally.seen.fetch_add(1, Ordering::Relaxed); let Some(fabric) = fabric(&exposes) else { return; @@ -289,9 +353,8 @@ fn a_packet_that_cannot_be_looked_up_says_so() { let (source, stray) = (probe.source, probe.stray); let out = run(&mut nf, vec![probe.take()]); - let packet = &out[0]; - let reason = packet.get_done().unwrap_or_else(|| { + let reason = out[0].get_done().unwrap_or_else(|| { panic!( "a packet with {stray:?} passed static NAT with no verdict at all, so \ {source} would be forwarded untranslated" @@ -305,19 +368,22 @@ fn a_packet_that_cannot_be_looked_up_says_so() { ); tally.reached.fetch_add(1, Ordering::Relaxed); } - }); + }, + ); tally.report("attribution"); } #[test] -fn a_modified_packet_is_always_marked() { +fn a_packet_that_cannot_be_looked_up_says_so() { + drive_attribution(Scenario::addresses(true)); +} + +fn drive_marking(scenario: Scenario) { let tally = Tally::default(); - bolero::check!() - .with_generator(Scenario { strays: true }) - .cloned() - .for_each(|(exposes, probes): (Vec, Vec)| { + bolero::check!().with_generator(scenario).cloned().for_each( + |(exposes, probes): (Vec, Vec)| { tally.seen.fetch_add(1, Ordering::Relaxed); let Some(fabric) = fabric(&exposes) else { return; @@ -327,31 +393,39 @@ fn a_modified_packet_is_always_marked() { for spec in &probes { let mut probe = (*spec).resolve(&fabric); - let source = probe.source; + let before = (probe.source, probe.sport); let out = run(&mut nf, vec![probe.take()]); let packet = &out[0]; - let translated = packet - .ip_source() - .unwrap_or_else(|| unreachable!("a probe is always an ip packet")) - != source; - if !translated { + if five_tuple_source(packet) == before { continue; } + let (source, sport) = before; assert!( packet.meta().is_src_natted(), - "{source} was translated without the source-natted mark, so a later stage \ - would translate it again" + "{source}:{sport} was translated without the source-natted mark, so a later \ + stage would translate it again" ); assert!( packet.meta().checksum_refresh(), - "{source} was translated without asking for a checksum refresh, so the packet \ - goes out with a checksum for an address it no longer carries" + "{source}:{sport} was translated without asking for a checksum refresh, so the \ + packet goes out with a checksum for headers it no longer carries" ); tally.reached.fetch_add(1, Ordering::Relaxed); } - }); + }, + ); tally.report("marking"); } + +#[test] +fn a_modified_packet_is_always_marked() { + drive_marking(Scenario::addresses(true)); +} + +#[test] +fn a_port_modified_packet_is_always_marked() { + drive_marking(Scenario::ports(true)); +} diff --git a/nat/src/static_nat/probe.rs b/nat/src/static_nat/probe.rs index 2f6deb79cd..d349c88da3 100644 --- a/nat/src/static_nat/probe.rs +++ b/nat/src/static_nat/probe.rs @@ -10,7 +10,7 @@ use config::external::overlay::vpcpeering::VpcExpose; use config::external::overlay::vpcpeering::contract::{ LOCAL_VNI, REMOTE_VNI, overlay_with_exposes, }; -use lpm::prefix::PrefixWithOptionalPorts; +use lpm::prefix::{PortRange, PrefixWithOptionalPorts}; use net::buffer::TestBuffer; use net::ip::{NextHeader, UnicastIpAddr}; use net::packet::test_utils::{ @@ -31,10 +31,30 @@ pub(crate) fn vni(raw: u32) -> Vni { Vni::new_checked(raw).unwrap_or_else(|_| unreachable!("{raw} is a legal vni")) } -pub(crate) fn addresses(prefixes: &BTreeSet) -> Vec { +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +pub(crate) struct Endpoint { + pub(crate) addr: IpAddr, + pub(crate) ports: Option, +} + +impl Endpoint { + pub(crate) fn port(&self, index: u16) -> u16 { + match self.ports { + None => index.max(1), + Some(range) => { + let len = u32::try_from(range.len()).unwrap_or(u32::from(u16::MAX)); + let offset = u32::from(index) % len.max(1); + u16::try_from(u32::from(range.start()) + offset).unwrap_or(range.end()) + } + } + } +} + +pub(crate) fn endpoints(prefixes: &BTreeSet) -> Vec { let mut out = Vec::new(); - for prefix in prefixes { - let prefix = prefix.prefix(); + for prefix_with_ports in prefixes { + let ports = prefix_with_ports.ports(); + let prefix = prefix_with_ports.prefix(); let (start, end) = (prefix.as_address(), prefix.last_address()); let (mut bits, last) = match (start, end) { (IpAddr::V4(a), IpAddr::V4(b)) => (u128::from(a.to_bits()), u128::from(b.to_bits())), @@ -42,13 +62,16 @@ pub(crate) fn addresses(prefixes: &BTreeSet) -> Vec unreachable!("a prefix does not change address family"), }; while bits <= last { - out.push(match start { - IpAddr::V4(_) => IpAddr::V4( - u32::try_from(bits) - .unwrap_or_else(|_| unreachable!()) - .into(), - ), - IpAddr::V6(_) => IpAddr::V6(bits.into()), + out.push(Endpoint { + addr: match start { + IpAddr::V4(_) => IpAddr::V4( + u32::try_from(bits) + .unwrap_or_else(|_| unreachable!()) + .into(), + ), + IpAddr::V6(_) => IpAddr::V6(bits.into()), + }, + ports, }); bits += 1; } @@ -58,25 +81,27 @@ pub(crate) fn addresses(prefixes: &BTreeSet) -> Vec, - pub(crate) public: Vec, + pub(crate) private: Vec, + pub(crate) public: Vec, pub(crate) peer: Vec, + pub(crate) uses_ports: bool, } impl Fabric { pub(crate) fn build(exposes: &[VpcExpose]) -> Option { - let private: Vec = exposes.iter().flat_map(|e| addresses(&e.ips)).collect(); - let public: Vec = exposes + let private: Vec = exposes.iter().flat_map(|e| endpoints(&e.ips)).collect(); + let public: Vec = exposes .iter() .filter_map(|e| e.nat.as_ref()) - .flat_map(|nat| addresses(&nat.as_range)) + .flat_map(|nat| endpoints(&nat.as_range)) .collect(); + let uses_ports = private.iter().chain(&public).any(|e| e.ports.is_some()); let overlay = overlay_with_exposes(exposes.to_vec()).ok()?; let validated = overlay.validate().ok()?; let tables = build_nat_configuration(validated.vpc_table()).ok()?; - let peer = match private.first() { + let peer = match private.first().map(|e| e.addr) { Some(IpAddr::V6(_)) => vec![ "2001:db8:ffff::1" .parse() @@ -98,6 +123,7 @@ impl Fabric { private, public, peer, + uses_ports, }) } @@ -105,12 +131,24 @@ impl Fabric { StaticNat::with_reader("probe", self.writer.get_reader()) } - pub(crate) fn outbound_to_peer(&self, source: IpAddr) -> Packet { - let mut packet = build(source, self.peer[0], false, 1024, 80); + pub(crate) fn outbound_to_peer(&self, source: Endpoint, port: u16) -> Packet { + let mut packet = build(source.addr, self.peer[0], false, port, 80); Arrival::outbound().stamp(&mut packet); packet } + pub(crate) fn every_source(&self) -> Vec<(Endpoint, u16)> { + self.private + .iter() + .flat_map(|endpoint| match endpoint.ports { + None => vec![(*endpoint, 1024)], + Some(range) => (range.start()..=range.end()) + .map(|port| (*endpoint, port)) + .collect(), + }) + .collect() + } + pub(crate) fn is_probeable(&self) -> bool { !self.private.is_empty() && !self.public.is_empty() } @@ -204,13 +242,13 @@ impl Probe { && self.arrival.dst_vpcd == Some(vni(REMOTE_VNI)) } - pub(crate) fn reply(&self, translated: IpAddr) -> Packet { + pub(crate) fn reply(&self, translated: IpAddr, translated_port: u16) -> Packet { let mut packet = build( self.destination, translated, self.tcp, self.dport, - self.sport, + translated_port, ); Arrival::inbound().stamp(&mut packet); packet @@ -224,14 +262,18 @@ impl ProbeSpec { pub(crate) fn resolve(self, fabric: &Fabric) -> Probe { let mut arrival = Arrival::outbound(); - let mut source = fabric.private[self.source as usize % fabric.private.len()]; + let endpoint = fabric.private[self.source as usize % fabric.private.len()]; + let destination = fabric.peer[self.peer as usize % fabric.peer.len()]; + let mut source = endpoint.addr; + let mut sport = endpoint.port(self.sport); let mut exposed = true; match self.stray { None => {} Some(Stray::SourceNotExposed) => { source = destination; + sport = self.sport.max(1); exposed = false; } Some(Stray::NoSourceVni) => arrival.src_vpcd = None, @@ -244,7 +286,6 @@ impl ProbeSpec { } } - let sport = self.sport.max(1); let dport = self.dport.max(1); let mut packet = build(source, destination, self.tcp, sport, dport); arrival.stamp(&mut packet); From 66a1ce6817daab3c9bed6ae7fa4a7afd6916970e Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Mon, 17 Aug 2026 17:46:28 -0600 Subject: [PATCH 05/14] test(nat): Drive masquerade with configuration-relative flows The second network function, and the first stateful one. Static NAT's answer for a packet is fixed by its tables; masquerade's is whatever the allocator handed out the first time it saw the flow, kept in the flow table and reused after. Every property here is really about that state being kept consistently, which is a different subject from anything the allocator's own tests can reach. A probe is a flow, not a packet. The first packet of a flow allocates and writes a flow entry; the second finds that entry and reuses it. Different code, and the interesting properties relate the two. So `Probe::packet` hands back a fresh packet on every call rather than being consumed once, because sending the same flow twice is how the hot path is reached at all. The stage order is load-bearing. `FlowLookup` attaches a flow entry only to a packet whose `dst_vpcd` is **absent**, and the flow filter that sets `dst_vpcd` runs *after* it. `Masquerade` then requires `dst_vpcd` to be present. So the annotation has to arrive between the two stages -- not before both, not after. Stamping both up front, the way the static NAT harness does, crashes nothing: it quietly gives no packet any flow state, sends every packet down the allocation path, and makes a flow look re-allocated on each packet. That is the sharper form of the arrival-state point from the static NAT work. A network function's precondition is not always a stamp a test can apply in one go -- here part of it is supplied by a stage that must run *after* another stage that requires its absence, and no test that ignores the ordering describes the real thing. The three prerequisites the design note lists for comparing a stateful stage at all, handled rather than assumed: * **Seeded non-determinism** -- `set_randomize(false)`, or two fabrics built from one configuration disagree on every flow. * **Timers** -- rather than fake a clock, every property completes inside one flow lifetime, so none depends on expiry either happening or not. Expiry is a separate subject and wants the explicitly driven clock the note asks for, not a wall clock a property happens to outrun. * **Projections, not state** -- nothing here inspects the allocator or the flow table. Every assertion is over what came out of the pipeline. The properties also need a tokio runtime, since `FlowTable::insert` spawns a per-flow expiry timer. The existing tests get one from `#[tokio::test]`; a bolero body is synchronous, so it enters a runtime instead. None of the properties predicts which address and port a flow will be given -- that is the allocator's business and predicting it would be a second copy of it: * **reversibility** -- the reply comes back to where the flow started. Unlike static NAT there is no second table built from the other side of the peering: the reverse translation exists only because the forward packet recorded it. A forward translation not faithfully recorded is a connection that never gets an answer. * **stability** -- a flow keeps the translation it was first given. A stage that re-allocated would produce a legal-looking packet every time, and the connection would break in a way no allocator-level test could see, because the two allocations are individually correct. * **exclusivity** -- two live flows never share a translation. Distinct source ports as well as addresses, since masquerade collapses many private addresses onto few public ones and the port is what keeps them apart after. * **containment** -- every translation lands inside a range the configuration named. The one property that consults the configuration, and legitimately: a membership test, not a prediction of which member. An address from outside the declared set is unroutable, so the flow is a blackhole that looks like success from inside the box. * **permission** and **attribution** -- as for static NAT. Permission matters more here, because a translation is not merely applied but *recorded*: a packet masqueraded without permission leaves an entry behind that keeps translating its successors. Reversibility is deliberately blind to an allocation outside the declared range: it asserts the reverse undoes the forward, which stays true when both use the same wrong address. Containment is what covers that. `MasqueradeExposes` gets the same treatment `StaticNatExposes` did, for the same two reasons -- `MasqueradeExpose` draws its base index freely, so two exposes collide whenever their index ranges intersect, and independent draws mix address families. One slot of four indices per expose, family drawn once. The vacuity guard is a ratio rather than an absolute count, for the reason recorded with the static NAT properties. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- config/src/external/overlay/vpcpeering.rs | 71 +++-- nat/src/masquerade/fuzz.rs | 371 ++++++++++++++++++++++ nat/src/masquerade/mod.rs | 2 + nat/src/masquerade/probe.rs | 235 ++++++++++++++ 4 files changed, 657 insertions(+), 22 deletions(-) create mode 100644 nat/src/masquerade/fuzz.rs create mode 100644 nat/src/masquerade/probe.rs diff --git a/config/src/external/overlay/vpcpeering.rs b/config/src/external/overlay/vpcpeering.rs index 8333c1d879..da855cd2f7 100644 --- a/config/src/external/overlay/vpcpeering.rs +++ b/config/src/external/overlay/vpcpeering.rs @@ -1081,37 +1081,64 @@ pub mod contract { #[derive(Debug, Clone, Copy, Default)] pub struct MasqueradeExpose; + #[derive(Debug, Clone, Copy)] + pub struct MasqueradeExposes(pub u8); + + impl Default for MasqueradeExposes { + fn default() -> Self { + Self(3) + } + } + + const MASQUERADE_SLOT: u8 = 4; + + impl ValueGenerator for MasqueradeExposes { + type Output = Vec; + + fn generate(&self, driver: &mut D) -> Option> { + let v4 = driver.produce::()?; + let count = driver.gen_u8(Included(&1), Included(&self.0.max(1)))?; + (0..count) + .map(|slot| masquerade_expose(driver, v4, slot.wrapping_mul(MASQUERADE_SLOT))) + .collect() + } + } + impl ValueGenerator for MasqueradeExpose { type Output = VpcExpose; fn generate(&self, driver: &mut D) -> Option { let v4 = driver.produce::()?; - let privates = driver.gen_u8(Included(&1), Included(&3))?; - let publics = driver.gen_u8(Included(&1), Included(&2))?; let base = driver.produce::()?; - let idle_timeout = match driver.gen_u8(Included(&0), Included(&2))? { - 0 => None, - 1 => Some(Duration::from_secs(30)), - _ => Some(Duration::from_mins(2)), - }; + masquerade_expose(driver, v4, base) + } + } + + fn masquerade_expose(driver: &mut D, v4: bool, base: u8) -> Option { + let privates = driver.gen_u8(Included(&1), Included(&3))?; + let publics = driver.gen_u8(Included(&1), Included(&2))?; + let idle_timeout = match driver.gen_u8(Included(&0), Included(&2))? { + 0 => None, + 1 => Some(Duration::from_secs(30)), + _ => Some(Duration::from_mins(2)), + }; - let mut expose = VpcExpose::empty().make_masquerade(idle_timeout).ok()?; - for index in 0..privates { - expose = expose.ip(PrefixWithOptionalPorts::new( - block(v4, Side::Private, base.wrapping_add(index))?, + let mut expose = VpcExpose::empty().make_masquerade(idle_timeout).ok()?; + for index in 0..privates { + expose = expose.ip(PrefixWithOptionalPorts::new( + block(v4, Side::Private, base.wrapping_add(index))?, + None, + )); + } + for index in 0..publics { + expose = expose + .as_range(PrefixWithOptionalPorts::new( + block(v4, Side::Public, base.wrapping_add(index))?, None, - )); - } - for index in 0..publics { - expose = expose - .as_range(PrefixWithOptionalPorts::new( - block(v4, Side::Public, base.wrapping_add(index))?, - None, - )) - .ok()?; - } - Some(expose) + )) + .ok()?; } + Some(expose) } #[derive(Clone, Copy)] diff --git a/nat/src/masquerade/fuzz.rs b/nat/src/masquerade/fuzz.rs new file mode 100644 index 0000000000..809ff8e670 --- /dev/null +++ b/nat/src/masquerade/fuzz.rs @@ -0,0 +1,371 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +#![cfg(test)] + +use crate::masquerade::probe::{Arrival, Fabric, ProbeSpec, Stray, run}; +use bolero::{Driver, TypeGenerator, ValueGenerator}; +use concurrency::sync::atomic::{AtomicUsize, Ordering}; +use config::external::overlay::vpcpeering::VpcExpose; +use config::external::overlay::vpcpeering::contract::MasqueradeExposes; +use net::buffer::TestBuffer; +use net::packet::Packet; +use std::collections::BTreeMap; +use std::net::IpAddr; +use std::num::NonZero; + +const MAX_EXPOSES: u8 = 3; + +const MIN_REACHED: usize = 8; + +const PROBES: usize = 8; + +#[derive(Debug, Clone, Copy)] +struct Scenario { + strays: bool, +} + +impl ValueGenerator for Scenario { + type Output = (Vec, Vec); + + fn generate(&self, driver: &mut D) -> Option { + let exposes = MasqueradeExposes(MAX_EXPOSES).generate(driver)?; + + let mut probes = Vec::with_capacity(PROBES); + for _ in 0..PROBES { + let mut probe = ProbeSpec::generate(driver)?; + if !self.strays { + probe.clear_stray(); + } + probes.push(probe); + } + Some((exposes, probes)) + } +} + +fn with_runtime(body: impl FnOnce()) { + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_time() + .build() + .unwrap_or_else(|e| unreachable!("{e}")); + let _guard = runtime.enter(); + body(); +} + +fn fabric(exposes: &[VpcExpose]) -> Option { + let fabric = Fabric::build(exposes)?; + fabric.is_probeable().then_some(fabric) +} + +fn source_of(packet: &Packet) -> (IpAddr, u16) { + ( + packet + .ip_source() + .unwrap_or_else(|| unreachable!("a probe is always an ip packet")), + packet.transport_src_port().map_or(0, NonZero::get), + ) +} + +fn destination_of(packet: &Packet) -> (IpAddr, u16) { + ( + packet + .ip_destination() + .unwrap_or_else(|| unreachable!("a probe is always an ip packet")), + packet.transport_dst_port().map_or(0, NonZero::get), + ) +} + +#[derive(Default)] +struct Tally { + seen: AtomicUsize, + built: AtomicUsize, + reached: AtomicUsize, +} + +impl Tally { + fn report(&self, what: &str) { + let (seen, built, reached) = ( + self.seen.load(Ordering::Relaxed), + self.built.load(Ordering::Relaxed), + self.reached.load(Ordering::Relaxed), + ); + println!("{what}: {built}/{seen} configurations built, {reached} flows reached it"); + assert!( + built * 2 >= seen, + "only {built} of {seen} configurations built, so this checked much less than it looks \ + like it did" + ); + assert!( + reached >= MIN_REACHED && reached * 2 >= built, + "{reached} flows reached the {what} assertion across {built} configurations; \ + this property has gone vacuous" + ); + } +} + +#[test] +fn a_masqueraded_flow_comes_back() { + let tally = Tally::default(); + + with_runtime(|| { + bolero::check!() + .with_generator(Scenario { strays: false }) + .cloned() + .for_each(|(exposes, probes): (Vec, Vec)| { + tally.seen.fetch_add(1, Ordering::Relaxed); + let Some(fabric) = fabric(&exposes) else { + return; + }; + tally.built.fetch_add(1, Ordering::Relaxed); + let (mut lookup, mut masq) = fabric.stages(); + + for spec in &probes { + let probe = (*spec).resolve(&fabric); + let before = (probe.source, probe.sport); + let out = run( + &mut lookup, + &mut masq, + vec![probe.packet()], + probe.arrival.dst_vpcd, + ); + let after = source_of(&out[0]); + if after == before || out[0].is_done() { + continue; + } + + let back = run( + &mut lookup, + &mut masq, + vec![probe.reply(after.0, after.1)], + Arrival::inbound().dst_vpcd, + ); + assert_eq!( + destination_of(&back[0]), + before, + "{:?} was masqueraded to {after:?}, and the reply came back to {:?}", + before, + destination_of(&back[0]) + ); + tally.reached.fetch_add(1, Ordering::Relaxed); + } + }); + }); + + tally.report("reversibility"); +} + +#[test] +fn a_flow_keeps_its_translation() { + let tally = Tally::default(); + + with_runtime(|| { + bolero::check!() + .with_generator(Scenario { strays: false }) + .cloned() + .for_each(|(exposes, probes): (Vec, Vec)| { + tally.seen.fetch_add(1, Ordering::Relaxed); + let Some(fabric) = fabric(&exposes) else { + return; + }; + tally.built.fetch_add(1, Ordering::Relaxed); + let (mut lookup, mut masq) = fabric.stages(); + + for spec in &probes { + let probe = (*spec).resolve(&fabric); + let before = (probe.source, probe.sport); + let first = run(&mut lookup, &mut masq, vec![probe.packet()], probe.arrival.dst_vpcd); + if out_unchanged(&first, before) { + continue; + } + let second = run(&mut lookup, &mut masq, vec![probe.packet()], probe.arrival.dst_vpcd); + + assert_eq!( + source_of(&second[0]), + source_of(&first[0]), + "the same flow from {before:?} was given {:?} and then {:?}, so its reply can \ + only reach one of them", + source_of(&first[0]), + source_of(&second[0]) + ); + tally.reached.fetch_add(1, Ordering::Relaxed); + } + }); + }); + + tally.report("stability"); +} + +fn out_unchanged(out: &[Packet], before: (IpAddr, u16)) -> bool { + out[0].is_done() || source_of(&out[0]) == before +} + +#[test] +fn distinct_flows_do_not_share_a_translation() { + let tally = Tally::default(); + + with_runtime(|| { + bolero::check!() + .with_generator(Scenario { strays: false }) + .cloned() + .for_each(|(exposes, probes): (Vec, Vec)| { + tally.seen.fetch_add(1, Ordering::Relaxed); + let Some(fabric) = fabric(&exposes) else { + return; + }; + tally.built.fetch_add(1, Ordering::Relaxed); + let (mut lookup, mut masq) = fabric.stages(); + + let mut taken: BTreeMap<(IpAddr, u16), (IpAddr, u16)> = BTreeMap::new(); + for (index, spec) in probes.iter().enumerate() { + let mut probe = (*spec).resolve(&fabric); + probe.sport = u16::try_from(1024 + index).unwrap_or(1024); + let before = (probe.source, probe.sport); + let out = run(&mut lookup, &mut masq, vec![probe.packet()], probe.arrival.dst_vpcd); + if out_unchanged(&out, before) { + continue; + } + let after = source_of(&out[0]); + + if let Some(previous) = taken.insert(after, before) { + assert_eq!( + previous, before, + "flows from {previous:?} and {before:?} were both masqueraded to {after:?}, \ + so a reply can only reach one of them" + ); + } + tally.reached.fetch_add(1, Ordering::Relaxed); + } + }); + }); + + tally.report("exclusivity"); +} + +#[test] +fn a_translation_stays_inside_the_public_range() { + let tally = Tally::default(); + + with_runtime(|| { + bolero::check!() + .with_generator(Scenario { strays: false }) + .cloned() + .for_each(|(exposes, probes): (Vec, Vec)| { + tally.seen.fetch_add(1, Ordering::Relaxed); + let Some(fabric) = fabric(&exposes) else { + return; + }; + tally.built.fetch_add(1, Ordering::Relaxed); + let (mut lookup, mut masq) = fabric.stages(); + + for spec in &probes { + let probe = (*spec).resolve(&fabric); + let before = (probe.source, probe.sport); + let out = run( + &mut lookup, + &mut masq, + vec![probe.packet()], + probe.arrival.dst_vpcd, + ); + if out_unchanged(&out, before) { + continue; + } + let (addr, port) = source_of(&out[0]); + + assert!( + fabric.is_public(addr), + "{before:?} was masqueraded to {addr}:{port}, which no expose offers; the \ + fabric has no route back to it" + ); + tally.reached.fetch_add(1, Ordering::Relaxed); + } + }); + }); + + tally.report("containment"); +} + +#[test] +fn nothing_is_masqueraded_without_permission() { + let tally = Tally::default(); + + with_runtime(|| { + bolero::check!() + .with_generator(Scenario { strays: true }) + .cloned() + .for_each(|(exposes, probes): (Vec, Vec)| { + tally.seen.fetch_add(1, Ordering::Relaxed); + let Some(fabric) = fabric(&exposes) else { + return; + }; + tally.built.fetch_add(1, Ordering::Relaxed); + let (mut lookup, mut masq) = fabric.stages(); + + for spec in &probes { + let probe = (*spec).resolve(&fabric); + if probe.asks_for_translation() && probe.exposed { + continue; + } + let before = (probe.source, probe.sport); + let (stray, arrival) = (probe.stray, probe.arrival); + let out = run(&mut lookup, &mut masq, vec![probe.packet()], probe.arrival.dst_vpcd); + + if out[0].is_done() { + tally.reached.fetch_add(1, Ordering::Relaxed); + continue; + } + assert_eq!( + source_of(&out[0]), + before, + "masquerade translated {before:?} although {stray:?} forbade it; the packet \ + arrived as {arrival:?}" + ); + tally.reached.fetch_add(1, Ordering::Relaxed); + } + }); + }); + + tally.report("permission"); +} + +#[test] +fn a_flow_that_cannot_be_masqueraded_says_so() { + let tally = Tally::default(); + + with_runtime(|| { + bolero::check!() + .with_generator(Scenario { strays: true }) + .cloned() + .for_each(|(exposes, probes): (Vec, Vec)| { + tally.seen.fetch_add(1, Ordering::Relaxed); + let Some(fabric) = fabric(&exposes) else { + return; + }; + tally.built.fetch_add(1, Ordering::Relaxed); + let (mut lookup, mut masq) = fabric.stages(); + + for spec in &probes { + let probe = (*spec).resolve(&fabric); + let unplaceable = matches!( + probe.stray, + Some(Stray::SourceNotExposed | Stray::UnknownSourceVni | Stray::UnknownDestVni) + ); + if !unplaceable { + continue; + } + let before = (probe.source, probe.sport); + let stray = probe.stray; + let out = run(&mut lookup, &mut masq, vec![probe.packet()], probe.arrival.dst_vpcd); + let packet = &out[0]; + + assert!( + packet.is_done(), + "a flow from {before:?} with {stray:?} passed masquerade with no verdict, so \ + a private address reaches the fabric untranslated" + ); + tally.reached.fetch_add(1, Ordering::Relaxed); + } + }); + }); + + tally.report("attribution"); +} diff --git a/nat/src/masquerade/mod.rs b/nat/src/masquerade/mod.rs index d7b2861dba..5e149bb06f 100644 --- a/nat/src/masquerade/mod.rs +++ b/nat/src/masquerade/mod.rs @@ -5,10 +5,12 @@ pub(crate) mod allocation; mod allocator_writer; pub mod apalloc; pub(crate) mod flows; +mod fuzz; pub(crate) mod icmp_handling; mod natip; mod nf; mod packet; +mod probe; mod protocol; mod state; mod test; diff --git a/nat/src/masquerade/probe.rs b/nat/src/masquerade/probe.rs new file mode 100644 index 0000000000..aa1aeeb0fa --- /dev/null +++ b/nat/src/masquerade/probe.rs @@ -0,0 +1,235 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +#![cfg(test)] + +use crate::masquerade::{MasqueradeConfig, NatAllocatorWriter}; +use bolero::TypeGenerator; +use concurrency::sync::Arc; +use config::external::overlay::vpcpeering::VpcExpose; +use config::external::overlay::vpcpeering::contract::{ + LOCAL_VNI, REMOTE_VNI, overlay_with_exposes, +}; +use flow_entry::flow_table::{FlowLookup, FlowTable}; +use lpm::prefix::Prefix; +use net::buffer::TestBuffer; +use net::packet::{Packet, VpcDiscriminant}; +use net::vxlan::Vni; +use pipeline::NetworkFunction; +use std::net::IpAddr; + +use crate::Masquerade; +use crate::static_nat::probe::{build, vni}; + +const FLOW_CAPACITY: usize = 4096; + +const ABSENT_VNI: u32 = 4_000; + +pub(crate) struct Fabric { + flow_table: Arc, + allocator: NatAllocatorWriter, + pub(crate) private: Vec, + pub(crate) public: Vec, + pub(crate) peer: Vec, +} + +impl Fabric { + pub(crate) fn build(exposes: &[VpcExpose]) -> Option { + let overlay = overlay_with_exposes(exposes.to_vec()).ok()?; + let validated = overlay.validate().ok()?; + + let private: Vec = exposes + .iter() + .flat_map(|e| e.ips.iter().map(|p| p.prefix().as_address())) + .collect(); + let public: Vec = exposes + .iter() + .filter_map(|e| e.nat.as_ref()) + .flat_map(|nat| { + nat.as_range + .iter() + .map(lpm::prefix::PrefixWithOptionalPorts::prefix) + }) + .collect(); + + let peer = match private.first() { + Some(IpAddr::V6(_)) => vec![ + "2001:db8:ffff::1" + .parse() + .unwrap_or_else(|_| unreachable!()), + "2001:db8:ffff::2" + .parse() + .unwrap_or_else(|_| unreachable!()), + ], + _ => vec![ + "3.3.3.1".parse().unwrap_or_else(|_| unreachable!()), + "3.3.3.2".parse().unwrap_or_else(|_| unreachable!()), + ], + }; + + let flow_table = Arc::new(FlowTable::new(FLOW_CAPACITY)); + let mut allocator = NatAllocatorWriter::new(); + let config = MasqueradeConfig::new(validated.vpc_table()).set_randomize(false); + allocator.update_nat_allocator(config, 1, &flow_table); + + Some(Self { + flow_table, + allocator, + private, + public, + peer, + }) + } + + pub(crate) fn stages(&self) -> (FlowLookup, Masquerade) { + ( + FlowLookup::new("flow-lookup", self.flow_table.clone()), + Masquerade::new( + "masquerade", + self.flow_table.clone(), + self.allocator.get_reader(), + ), + ) + } + + pub(crate) fn is_probeable(&self) -> bool { + !self.private.is_empty() && !self.public.is_empty() + } + + pub(crate) fn is_public(&self, addr: IpAddr) -> bool { + self.public.iter().any(|p| p.covers_addr(&addr)) + } +} + +pub(crate) fn run( + lookup: &mut FlowLookup, + masq: &mut Masquerade, + packets: Vec>, + dst_vpcd: Option, +) -> Vec> { + let mut looked: Vec<_> = lookup.process(packets.into_iter()).collect(); + for packet in &mut looked { + packet.meta_mut().dst_vpcd = dst_vpcd.map(VpcDiscriminant::from_vni); + } + masq.process(looked.into_iter()).collect() +} + +#[derive(Debug, Clone, Copy)] +pub(crate) struct Arrival { + pub(crate) src_vpcd: Option, + pub(crate) dst_vpcd: Option, + pub(crate) wants_masquerade: bool, +} + +impl Arrival { + pub(crate) fn outbound() -> Self { + Self { + src_vpcd: Some(vni(LOCAL_VNI)), + dst_vpcd: Some(vni(REMOTE_VNI)), + wants_masquerade: true, + } + } + + pub(crate) fn inbound() -> Self { + Self { + src_vpcd: Some(vni(REMOTE_VNI)), + dst_vpcd: Some(vni(LOCAL_VNI)), + wants_masquerade: true, + } + } + + pub(crate) fn stamp(self, packet: &mut Packet) { + let meta = packet.meta_mut(); + meta.src_vpcd = self.src_vpcd.map(VpcDiscriminant::from_vni); + meta.set_overlay(true); + meta.set_keep(true); + meta.set_masquerade(self.wants_masquerade); + } +} + +#[derive(Debug, Clone, Copy, TypeGenerator)] +pub(crate) enum Stray { + SourceNotExposed, + UnknownSourceVni, + UnknownDestVni, + NotAskedFor, +} + +#[derive(Debug, Clone, Copy, TypeGenerator)] +pub(crate) struct ProbeSpec { + source: u8, + peer: u8, + sport: u16, + dport: u16, + stray: Option, +} + +pub(crate) struct Probe { + pub(crate) source: IpAddr, + pub(crate) destination: IpAddr, + pub(crate) sport: u16, + pub(crate) dport: u16, + pub(crate) exposed: bool, + pub(crate) arrival: Arrival, + pub(crate) stray: Option, +} + +impl Probe { + pub(crate) fn asks_for_translation(&self) -> bool { + self.arrival.wants_masquerade + && self.arrival.src_vpcd == Some(vni(LOCAL_VNI)) + && self.arrival.dst_vpcd == Some(vni(REMOTE_VNI)) + } + + pub(crate) fn packet(&self) -> Packet { + let mut packet = build(self.source, self.destination, false, self.sport, self.dport); + self.arrival.stamp(&mut packet); + packet + } + + pub(crate) fn reply(&self, translated: IpAddr, translated_port: u16) -> Packet { + let mut packet = build( + self.destination, + translated, + false, + self.dport, + translated_port, + ); + Arrival::inbound().stamp(&mut packet); + packet + } +} + +impl ProbeSpec { + pub(crate) fn clear_stray(&mut self) { + self.stray = None; + } + + pub(crate) fn resolve(self, fabric: &Fabric) -> Probe { + let mut arrival = Arrival::outbound(); + let mut source = fabric.private[self.source as usize % fabric.private.len()]; + let destination = fabric.peer[self.peer as usize % fabric.peer.len()]; + let mut exposed = true; + + match self.stray { + None => {} + Some(Stray::SourceNotExposed) => { + source = destination; + exposed = false; + } + Some(Stray::UnknownSourceVni) => arrival.src_vpcd = Some(vni(ABSENT_VNI)), + Some(Stray::UnknownDestVni) => arrival.dst_vpcd = Some(vni(ABSENT_VNI)), + Some(Stray::NotAskedFor) => arrival.wants_masquerade = false, + } + + Probe { + source, + destination, + sport: self.sport.max(1), + dport: self.dport.max(1), + exposed, + arrival, + stray: self.stray, + } + } +} From 71d609c971ea606d3a83ae1ac2d9915044b82977 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Mon, 17 Aug 2026 17:50:45 -0600 Subject: [PATCH 06/14] test(acl): Carry the acl properties from the lookup to the network function `acl-filter/src/fuzz.rs` has the strongest oracle in this codebase: it evaluates the validated configuration directly and compares that against the lowered tables, so a lowering mistake cannot hide behind the thing it produced. What it never touches is a packet. Every probe there is a `PacketSummary` handed straight to `lookup`. Two pieces of production code sit between a packet and that summary, and neither had any coverage from a generated configuration: * **`PacketSummary::try_from`**, which reads the five-tuple and both discriminants out of the headers; and * **`AclFilter::process_packet`**, which turns a verdict into a fate -- `DoneReason::AclDropped`, `invalidate_flows`, and the `is_overlay` gate deciding whether any of it happens. A field misread in the first of those is invisible to every existing property, because none of them builds the packet that would be misread. This re-points the existing generators rather than writing new ones. The `OverlaySpec` and `ProbeSpec` are unchanged; a probe now becomes a packet and the answer is read off the packet's fate. The oracle is the same `oracle_resolved_action`, asked the same question, so this is a differential test over the packet path rather than a second ACL. * **stage verdict** -- a packet the configuration denies is dropped with `AclDropped`; one it allows survives untouched. This tests the extraction implicitly: a field read from the wrong place makes the stage judge a different tuple from the one the oracle judged, and they disagree wherever that field decides the answer. * **summary round trip** -- the five-tuple read back is the one the packet was built with. Direct rather than implicit, so it also catches the misread that happens to be harmless for the ruleset drawn. * **missing discriminant** -- a packet naming no destination vpc is refused as `Unroutable`. An ACL is indexed by the vpc pair, so such a packet cannot be judged at all, and letting it through applies no policy whatsoever. * **underlay gate** -- traffic that is not overlay traffic is left alone. No ACL in the configuration describes it. Only TCP and UDP become packets. A probe drawing ICMP or an arbitrary next header is counted and skipped rather than approximated, because a packet whose headers did not match the summary it came from would make every disagreement meaningless. The same goes for the generator's `CrossVersion` stray, which asks for a v4 source with a v6 destination -- there is no such packet, and that case stays with the summary-level properties where it belongs. The vacuity guard here counts denials as well as arrivals: a run that only ever saw permits would pass while the drop path -- the only path where the stage does anything -- went entirely unexercised. Around a quarter of probes are denied in practice. It is a ratio rather than an absolute count, for the reason recorded with the static NAT properties. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- acl-filter/src/fuzz.rs | 10 ++ acl-filter/src/lib.rs | 2 + acl-filter/src/nf_fuzz.rs | 267 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 279 insertions(+) create mode 100644 acl-filter/src/nf_fuzz.rs diff --git a/acl-filter/src/fuzz.rs b/acl-filter/src/fuzz.rs index 156fbe82c9..ddd052191d 100644 --- a/acl-filter/src/fuzz.rs +++ b/acl-filter/src/fuzz.rs @@ -127,6 +127,16 @@ fn resolved_action(rule: Option, default: Option) -> A rule.map_or_else(|| default.unwrap_or(AclAction::Allow), |v| v.action) } +pub(crate) fn oracle_resolved_action( + overlay: &ValidatedOverlay, + packet: &PacketSummary, +) -> AclAction { + resolved_action( + oracle_lookup(overlay, packet), + oracle_default_action(overlay, packet.src_vni, packet.dst_vni), + ) +} + // ------------------------------------------------------------------------------------------------- // Properties. diff --git a/acl-filter/src/lib.rs b/acl-filter/src/lib.rs index 3e17f71453..d4fe2c6f40 100644 --- a/acl-filter/src/lib.rs +++ b/acl-filter/src/lib.rs @@ -26,6 +26,8 @@ mod fuzz; #[cfg(test)] mod fuzz_gen; #[cfg(test)] +mod nf_fuzz; +#[cfg(test)] mod tests; pub use access::{ diff --git a/acl-filter/src/nf_fuzz.rs b/acl-filter/src/nf_fuzz.rs new file mode 100644 index 0000000000..f0d2bc3665 --- /dev/null +++ b/acl-filter/src/nf_fuzz.rs @@ -0,0 +1,267 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +#![cfg(test)] + +use crate::fuzz::oracle_resolved_action; +use crate::fuzz_gen::{OverlaySpec, ProbeSpec}; +use crate::{AclFilter, AclFilterContext, AclFilterContextWriter, PacketSummary}; +use concurrency::sync::atomic::{AtomicUsize, Ordering}; +use config::external::overlay::acl::AclAction; +use net::buffer::TestBuffer; +use net::ip::{NextHeader, UnicastIpAddr}; +use net::packet::test_utils::{ + build_test_ipv4_packet_with_transport, build_test_ipv6_packet_with_transport, +}; +use net::packet::{DoneReason, Packet, VpcDiscriminant}; +use net::tcp::port::TcpPort; +use net::udp::UdpPort; +use pipeline::NetworkFunction; +use std::net::IpAddr; + +const MIN_REACHED: usize = 8; + +const PROBES: usize = 8; + +fn packet_for(summary: &PacketSummary) -> Option> { + let (sport, dport) = summary.ports?; + let tcp = match summary.proto { + NextHeader::TCP => true, + NextHeader::UDP => false, + _ => return None, + }; + + let mut packet = match (summary.src_ip, summary.dst_ip) { + (IpAddr::V4(_), IpAddr::V4(_)) => { + build_test_ipv4_packet_with_transport(64, Some(summary.proto)).ok()? + } + (IpAddr::V6(_), IpAddr::V6(_)) => { + build_test_ipv6_packet_with_transport(64, Some(summary.proto)).ok()? + } + _ => return None, + }; + + packet + .set_ip_source(UnicastIpAddr::try_from(summary.src_ip).ok()?) + .ok()?; + packet.set_ip_destination(summary.dst_ip).ok()?; + if tcp { + packet + .set_tcp_source_port(TcpPort::new_checked(sport.max(1)).ok()?) + .ok()?; + packet + .set_tcp_destination_port(TcpPort::new_checked(dport.max(1)).ok()?) + .ok()?; + } else { + packet + .set_udp_source_port(UdpPort::new_checked(sport.max(1)).ok()?) + .ok()?; + packet + .set_udp_destination_port(UdpPort::new_checked(dport.max(1)).ok()?) + .ok()?; + } + + let meta = packet.meta_mut(); + meta.src_vpcd = Some(VpcDiscriminant::from_vni(summary.src_vni)); + meta.dst_vpcd = Some(VpcDiscriminant::from_vni(summary.dst_vni)); + meta.set_overlay(true); + meta.set_keep(true); + Some(packet) +} + +fn expected_summary(summary: &PacketSummary) -> PacketSummary { + let mut expected = summary.clone(); + expected.ports = summary.ports.map(|(s, d)| (s.max(1), d.max(1))); + expected +} + +fn filter(built: &crate::fuzz_gen::BuiltOverlay) -> AclFilter { + let writer = AclFilterContextWriter::new(); + writer.store(AclFilterContext::for_test(&built.overlay)); + AclFilter::new("nf-fuzz-acl-filter", writer.get_reader()) +} + +#[derive(Default)] +struct Tally { + drawn: AtomicUsize, + reached: AtomicUsize, + denied: AtomicUsize, +} + +impl Tally { + fn report(&self, what: &str) { + let (drawn, reached, denied) = ( + self.drawn.load(Ordering::Relaxed), + self.reached.load(Ordering::Relaxed), + self.denied.load(Ordering::Relaxed), + ); + println!("{what}: {reached}/{drawn} probes became packets, {denied} of them denied"); + assert!( + reached >= MIN_REACHED && reached * 4 >= drawn, + "only {reached} of {drawn} probes became packets, so the {what} assertion is barely \ + running" + ); + assert!( + denied * 20 >= reached, + "only {denied} of {reached} probes were denied, so the drop path is barely exercised \ + and this property is mostly checking that nothing happens" + ); + } +} + +#[test] +fn the_stage_agrees_with_the_configuration() { + let tally = Tally::default(); + + bolero::check!() + .with_type::<(OverlaySpec, [ProbeSpec; PROBES])>() + .for_each(|(overlay_spec, probe_specs)| { + let built = overlay_spec.build(); + let mut acl = filter(&built); + + for probe_spec in probe_specs { + tally.drawn.fetch_add(1, Ordering::Relaxed); + let summary = probe_spec.resolve(&built); + let Some(packet) = packet_for(&summary) else { + continue; + }; + + let want = oracle_resolved_action(&built.overlay, &summary); + let out: Vec<_> = acl.process(std::iter::once(packet)).collect(); + let got = out[0].get_done(); + + match want { + AclAction::Deny => { + assert_eq!( + got, + Some(DoneReason::AclDropped), + "the configuration denies {summary:?} and the stage let it through \ + with {got:?}\nspec: {overlay_spec:?}" + ); + tally.denied.fetch_add(1, Ordering::Relaxed); + } + AclAction::Allow => { + assert_eq!( + got, None, + "the configuration allows {summary:?} and the stage dropped it for \ + {got:?}\nspec: {overlay_spec:?}" + ); + } + } + tally.reached.fetch_add(1, Ordering::Relaxed); + } + }); + + tally.report("stage verdict"); +} + +#[test] +fn the_summary_survives_the_round_trip_through_a_packet() { + let tally = Tally::default(); + + bolero::check!() + .with_type::<(OverlaySpec, [ProbeSpec; PROBES])>() + .for_each(|(overlay_spec, probe_specs)| { + let built = overlay_spec.build(); + + for probe_spec in probe_specs { + tally.drawn.fetch_add(1, Ordering::Relaxed); + let summary = probe_spec.resolve(&built); + let Some(packet) = packet_for(&summary) else { + continue; + }; + + let read = PacketSummary::try_from(&packet) + .unwrap_or_else(|e| panic!("a built packet did not yield a summary: {e:?}")); + let expected = expected_summary(&summary); + + assert_eq!( + (read.src_vni, read.dst_vni), + (expected.src_vni, expected.dst_vni), + "discriminants came back swapped or wrong\nspec: {overlay_spec:?}" + ); + assert_eq!( + (read.src_ip, read.dst_ip), + (expected.src_ip, expected.dst_ip), + "addresses came back swapped or wrong\nspec: {overlay_spec:?}" + ); + assert_eq!( + read.proto, expected.proto, + "protocol came back wrong\nspec: {overlay_spec:?}" + ); + assert_eq!( + read.ports, expected.ports, + "ports came back swapped or wrong\nspec: {overlay_spec:?}" + ); + tally.reached.fetch_add(1, Ordering::Relaxed); + tally.denied.fetch_add(1, Ordering::Relaxed); + } + }); + + tally.report("summary round trip"); +} + +#[test] +fn a_packet_with_no_discriminants_is_dropped() { + let tally = Tally::default(); + + bolero::check!() + .with_type::<(OverlaySpec, [ProbeSpec; PROBES])>() + .for_each(|(overlay_spec, probe_specs)| { + let built = overlay_spec.build(); + let mut acl = filter(&built); + + for probe_spec in probe_specs { + tally.drawn.fetch_add(1, Ordering::Relaxed); + let summary = probe_spec.resolve(&built); + let Some(mut packet) = packet_for(&summary) else { + continue; + }; + packet.meta_mut().dst_vpcd = None; + + let out: Vec<_> = acl.process(std::iter::once(packet)).collect(); + assert_eq!( + out[0].get_done(), + Some(DoneReason::Unroutable), + "a packet with no destination vpc was not refused\nspec: {overlay_spec:?}" + ); + tally.reached.fetch_add(1, Ordering::Relaxed); + tally.denied.fetch_add(1, Ordering::Relaxed); + } + }); + + tally.report("missing discriminant"); +} + +#[test] +fn underlay_traffic_is_not_judged() { + let tally = Tally::default(); + + bolero::check!() + .with_type::<(OverlaySpec, [ProbeSpec; PROBES])>() + .for_each(|(overlay_spec, probe_specs)| { + let built = overlay_spec.build(); + let mut acl = filter(&built); + + for probe_spec in probe_specs { + tally.drawn.fetch_add(1, Ordering::Relaxed); + let summary = probe_spec.resolve(&built); + let Some(mut packet) = packet_for(&summary) else { + continue; + }; + packet.meta_mut().set_overlay(false); + + let out: Vec<_> = acl.process(std::iter::once(packet)).collect(); + assert_eq!( + out[0].get_done(), + None, + "a packet that is not overlay traffic was judged by an overlay acl\nspec: \ + {overlay_spec:?}" + ); + tally.reached.fetch_add(1, Ordering::Relaxed); + tally.denied.fetch_add(1, Ordering::Relaxed); + } + }); + + tally.report("underlay gate"); +} From 78bb83ef035bb73c16cdc9417e4407cfc691b408 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 27 Aug 2026 01:54:15 -0600 Subject: [PATCH 07/14] fix(acl-filter): Let the acl properties be selected, and stop miscounting denials Two defects in one helper, both of which `nat`'s two copies of it already avoid. `cargo bolero` runs the test binary once with `CARGO_BOLERO_SELECT` set to find out which targets it holds; `check!()` registers and returns without drawing, so `report` ran with every count at zero and the vacuity guard refused the *selection*. These targets could not be fuzzed at all. The denial ratio is documented as catching a run that only ever saw permits. Three of the four properties bumped `denied` on the same line as `reached`, which reduces it to `reached * 20 >= reached`; `underlay_traffic_is_not_judged` did so having just asserted the packet was *not* denied. They now use a report that measures arrivals and does not claim to measure anything else. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- acl-filter/src/nf_fuzz.rs | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/acl-filter/src/nf_fuzz.rs b/acl-filter/src/nf_fuzz.rs index f0d2bc3665..51c81262c8 100644 --- a/acl-filter/src/nf_fuzz.rs +++ b/acl-filter/src/nf_fuzz.rs @@ -89,12 +89,31 @@ struct Tally { } impl Tally { + fn report_arrivals_only(&self, what: &str) { + let (drawn, reached) = ( + self.drawn.load(Ordering::Relaxed), + self.reached.load(Ordering::Relaxed), + ); + if drawn == 0 { + return; + } + println!("{what}: {reached}/{drawn} probes became packets"); + assert!( + reached >= MIN_REACHED && reached * 4 >= drawn, + "only {reached} of {drawn} probes became packets, so the {what} assertion is barely \ + running" + ); + } + fn report(&self, what: &str) { let (drawn, reached, denied) = ( self.drawn.load(Ordering::Relaxed), self.reached.load(Ordering::Relaxed), self.denied.load(Ordering::Relaxed), ); + if drawn == 0 { + return; + } println!("{what}: {reached}/{drawn} probes became packets, {denied} of them denied"); assert!( reached >= MIN_REACHED && reached * 4 >= drawn, @@ -194,11 +213,10 @@ fn the_summary_survives_the_round_trip_through_a_packet() { "ports came back swapped or wrong\nspec: {overlay_spec:?}" ); tally.reached.fetch_add(1, Ordering::Relaxed); - tally.denied.fetch_add(1, Ordering::Relaxed); } }); - tally.report("summary round trip"); + tally.report_arrivals_only("summary round trip"); } #[test] @@ -226,11 +244,10 @@ fn a_packet_with_no_discriminants_is_dropped() { "a packet with no destination vpc was not refused\nspec: {overlay_spec:?}" ); tally.reached.fetch_add(1, Ordering::Relaxed); - tally.denied.fetch_add(1, Ordering::Relaxed); } }); - tally.report("missing discriminant"); + tally.report_arrivals_only("missing discriminant"); } #[test] @@ -259,9 +276,8 @@ fn underlay_traffic_is_not_judged() { {overlay_spec:?}" ); tally.reached.fetch_add(1, Ordering::Relaxed); - tally.denied.fetch_add(1, Ordering::Relaxed); } }); - tally.report("underlay gate"); + tally.report_arrivals_only("underlay gate"); } From 18450675029d3d7e7d535a019410df3a6fc16dde Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 27 Aug 2026 01:55:57 -0600 Subject: [PATCH 08/14] test(nat): Take the declared public set from the validated overlay `Fabric::public` is what `a_translation_stays_inside_the_public_range` tests membership against, and it was computed from the *unvalidated* exposes. Validation collapses exclusion prefixes, so the raw `as_range` is a superset of what the allocator's pool is built from: a translation to an address the operator explicitly excluded would have passed. Latent, because the masquerade generator emits no exclusions -- which is the only reason the two agree, and not something the property should depend on. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- nat/src/masquerade/probe.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/nat/src/masquerade/probe.rs b/nat/src/masquerade/probe.rs index aa1aeeb0fa..67674db05f 100644 --- a/nat/src/masquerade/probe.rs +++ b/nat/src/masquerade/probe.rs @@ -42,14 +42,13 @@ impl Fabric { .iter() .flat_map(|e| e.ips.iter().map(|p| p.prefix().as_address())) .collect(); - let public: Vec = exposes - .iter() - .filter_map(|e| e.nat.as_ref()) - .flat_map(|nat| { - nat.as_range - .iter() - .map(lpm::prefix::PrefixWithOptionalPorts::prefix) - }) + let public: Vec = validated + .vpc_table() + .values() + .flat_map(|vpc| vpc.peerings()) + .flat_map(|peering| peering.local().valexp()) + .flat_map(|expose| expose.as_range_or_empty().iter()) + .map(lpm::prefix::PrefixWithOptionalPorts::prefix) .collect(); let peer = match private.first() { From 78e97aa9602dc381e9d97f8395033bf33fd5c8de Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 27 Aug 2026 13:06:25 -0600 Subject: [PATCH 09/14] fix(nat): Let the masquerade properties be selected `fix(acl-filter): Let the acl properties be selected` says `nat`'s two copies of this helper already avoid the defect. Only one does, and by accident: `static_nat::fuzz` puts `check!()` at the body scope of `drive_*`, so bolero's `return` under `CARGO_BOLERO_SELECT` leaves the function before `report` runs. Here `check!()` is inside a `with_runtime` closure, so the `return` leaves only the closure and the vacuity guard fires on every count at zero. All six masquerade properties therefore failed `cargo bolero`'s target enumeration and could not be fuzzed at all. Signed-off-by: Daniel Noland --- nat/src/masquerade/fuzz.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/nat/src/masquerade/fuzz.rs b/nat/src/masquerade/fuzz.rs index 809ff8e670..97968b0664 100644 --- a/nat/src/masquerade/fuzz.rs +++ b/nat/src/masquerade/fuzz.rs @@ -89,6 +89,9 @@ impl Tally { self.built.load(Ordering::Relaxed), self.reached.load(Ordering::Relaxed), ); + if seen == 0 { + return; + } println!("{what}: {built}/{seen} configurations built, {reached} flows reached it"); assert!( built * 2 >= seen, From 76d8e21df17b39256b2b22d4ccaecbf41b1d4a63 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 27 Aug 2026 13:06:25 -0600 Subject: [PATCH 10/14] test(nat): Take static NAT's declared sets from the validated overlay too The commit before last made this argument for masquerade and left the identical code in static NAT: validation collapses exclusion prefixes, so the raw expose lists are supersets of what the tables were built from. It is worse on this side. `private` is what `every_source` sweeps, so an excluded address in it fails `a_translated_source_comes_back` and `distinct_sources_stay_distinct` against a correct implementation -- a property that lies rather than one that misses. Latent either way: the generator emits no exclusions, which is the only reason the two agree. Both walks now take the offering vpc alone. `overlay_with_exposes` gives the peer a manifest of its own, and folding it in would have swept its 256 addresses as sources this configuration maps. Signed-off-by: Daniel Noland --- nat/src/masquerade/probe.rs | 3 ++- nat/src/static_nat/probe.rs | 30 ++++++++++++++++++++---------- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/nat/src/masquerade/probe.rs b/nat/src/masquerade/probe.rs index 67674db05f..b689e831df 100644 --- a/nat/src/masquerade/probe.rs +++ b/nat/src/masquerade/probe.rs @@ -45,7 +45,8 @@ impl Fabric { let public: Vec = validated .vpc_table() .values() - .flat_map(|vpc| vpc.peerings()) + .filter(|vpc| vpc.vni() == vni(LOCAL_VNI)) + .flat_map(config::external::overlay::vpc::ValidatedVpc::peerings) .flat_map(|peering| peering.local().valexp()) .flat_map(|expose| expose.as_range_or_empty().iter()) .map(lpm::prefix::PrefixWithOptionalPorts::prefix) diff --git a/nat/src/static_nat/probe.rs b/nat/src/static_nat/probe.rs index d349c88da3..1272192830 100644 --- a/nat/src/static_nat/probe.rs +++ b/nat/src/static_nat/probe.rs @@ -20,7 +20,6 @@ use net::packet::{Packet, VpcDiscriminant}; use net::tcp::port::TcpPort; use net::udp::UdpPort; use net::vxlan::Vni; -use std::collections::BTreeSet; use std::net::IpAddr; pub(crate) const PROBE_TTL: u8 = 64; @@ -50,7 +49,9 @@ impl Endpoint { } } -pub(crate) fn endpoints(prefixes: &BTreeSet) -> Vec { +pub(crate) fn endpoints<'a>( + prefixes: impl IntoIterator, +) -> Vec { let mut out = Vec::new(); for prefix_with_ports in prefixes { let ports = prefix_with_ports.ports(); @@ -89,18 +90,27 @@ pub(crate) struct Fabric { impl Fabric { pub(crate) fn build(exposes: &[VpcExpose]) -> Option { - let private: Vec = exposes.iter().flat_map(|e| endpoints(&e.ips)).collect(); - let public: Vec = exposes - .iter() - .filter_map(|e| e.nat.as_ref()) - .flat_map(|nat| endpoints(&nat.as_range)) - .collect(); - let uses_ports = private.iter().chain(&public).any(|e| e.ports.is_some()); - let overlay = overlay_with_exposes(exposes.to_vec()).ok()?; let validated = overlay.validate().ok()?; let tables = build_nat_configuration(validated.vpc_table()).ok()?; + let local: Vec<&config::external::overlay::vpcpeering::ValidatedExpose> = validated + .vpc_table() + .values() + .filter(|vpc| vpc.vni() == vni(LOCAL_VNI)) + .flat_map(config::external::overlay::vpc::ValidatedVpc::peerings) + .flat_map(|peering| peering.local().valexp()) + .collect(); + let private: Vec = local + .iter() + .flat_map(|expose| endpoints(expose.ips().iter())) + .collect(); + let public: Vec = local + .iter() + .flat_map(|expose| endpoints(expose.as_range_or_empty().iter())) + .collect(); + let uses_ports = private.iter().chain(&public).any(|e| e.ports.is_some()); + let peer = match private.first().map(|e| e.addr) { Some(IpAddr::V6(_)) => vec![ "2001:db8:ffff::1" From 7645b9731090a47f6278fc7e28cd2ba89063955a Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 27 Aug 2026 13:21:07 -0600 Subject: [PATCH 11/14] fix(acl-filter): Judge the summary the packet actually carries `packet_for` clamps port 0 to 1, because 0 is not a port either transport can carry. The oracle was still asked about the drawn summary, so its verdict on port 0 was compared against the stage's verdict on port 1 -- and a ruleset that distinguishes the two fails a correct implementation. `expected_summary` already existed for exactly this, and was used only by the round-trip property. Using it here restates nothing: that the packet carries this summary is what the round-trip property establishes. Signed-off-by: Daniel Noland --- acl-filter/src/nf_fuzz.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/acl-filter/src/nf_fuzz.rs b/acl-filter/src/nf_fuzz.rs index 51c81262c8..48b0cf7ce5 100644 --- a/acl-filter/src/nf_fuzz.rs +++ b/acl-filter/src/nf_fuzz.rs @@ -145,7 +145,8 @@ fn the_stage_agrees_with_the_configuration() { continue; }; - let want = oracle_resolved_action(&built.overlay, &summary); + let judged = expected_summary(&summary); + let want = oracle_resolved_action(&built.overlay, &judged); let out: Vec<_> = acl.process(std::iter::once(packet)).collect(); let got = out[0].get_done(); From ea0e0d7f88af8f045f3780f980d5b867de1df9b6 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 27 Aug 2026 13:51:21 -0600 Subject: [PATCH 12/14] fix(nat): Let the static NAT properties be selected The same guard as the masquerade one two commits back, and the same reason. Backported from `fix(nat): Let the nat properties be fuzzed at all` on pr/daniel-noland/driven-clock, whose other two files do not exist yet here. Not load-bearing today: `check!()` sits at the body scope of the `drive_*` helpers, so its `return` leaves the helper before `report` runs. It is here so that wrapping a property body in a closure later cannot quietly make these targets unselectable again, which is exactly how masquerade acquired the fault. Signed-off-by: Daniel Noland --- nat/src/static_nat/fuzz.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/nat/src/static_nat/fuzz.rs b/nat/src/static_nat/fuzz.rs index ec1a22f6be..704f776d4e 100644 --- a/nat/src/static_nat/fuzz.rs +++ b/nat/src/static_nat/fuzz.rs @@ -104,6 +104,9 @@ impl Tally { self.built.load(Ordering::Relaxed), self.reached.load(Ordering::Relaxed), ); + if seen == 0 { + return; + } println!("{what}: {built}/{seen} configurations built, {reached} probes reached it"); assert!( built * 2 >= seen, From b86c9c5c2eba4cc1b4d3d70f517bb9055972dd76 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Mon, 24 Aug 2026 20:49:06 -0600 Subject: [PATCH 13/14] fix(nat): Give each static nat property a target name of its own `bolero::check!()` names its target after the function it is written in, so the six drivers shared by these eleven tests registered six targets named after functions that are not tests -- listed by `cargo bolero list`, resolvable by nothing. Expanding the driver at each test site is what makes the name a test's. The case bodies are unchanged; only where the macro expands moved. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) (cherry picked from commit c00f66de9166850a3714069a9a9ab90c526b9c3f) --- nat/src/static_nat/fuzz.rs | 200 ++++++++++++++++++++----------------- 1 file changed, 107 insertions(+), 93 deletions(-) diff --git a/nat/src/static_nat/fuzz.rs b/nat/src/static_nat/fuzz.rs index 704f776d4e..6360641059 100644 --- a/nat/src/static_nat/fuzz.rs +++ b/nat/src/static_nat/fuzz.rs @@ -121,10 +121,11 @@ impl Tally { } } -fn drive_round_trip(scenario: Scenario) { +macro_rules! drive_round_trip { + ($scenario:expr) => {{ let tally = Tally::default(); - bolero::check!().with_generator(scenario).cloned().for_each( + bolero::check!().with_generator($scenario).cloned().for_each( |(exposes, probes): (Vec, Vec)| { tally.seen.fetch_add(1, Ordering::Relaxed); let Some(fabric) = fabric(&exposes) else { @@ -158,22 +159,24 @@ fn drive_round_trip(scenario: Scenario) { ); tally.report("round trip"); +}}; } #[test] fn a_translated_source_comes_back() { - drive_round_trip(Scenario::addresses(false)); + drive_round_trip!(Scenario::addresses(false)); } #[test] fn a_translated_source_and_port_come_back() { - drive_round_trip(Scenario::ports(false)); + drive_round_trip!(Scenario::ports(false)); } -fn drive_injectivity(scenario: Scenario) { +macro_rules! drive_injectivity { + ($scenario:expr) => {{ let tally = Tally::default(); - bolero::check!().with_generator(scenario).cloned().for_each( + bolero::check!().with_generator($scenario).cloned().for_each( |(exposes, _probes): (Vec, Vec)| { tally.seen.fetch_add(1, Ordering::Relaxed); let Some(fabric) = fabric(&exposes) else { @@ -211,131 +214,139 @@ fn drive_injectivity(scenario: Scenario) { ); tally.report("injectivity"); +}}; } #[test] fn distinct_sources_stay_distinct() { - drive_injectivity(Scenario::addresses(false)); + drive_injectivity!(Scenario::addresses(false)); } #[test] fn distinct_sources_and_ports_stay_distinct() { - drive_injectivity(Scenario::ports(false)); + drive_injectivity!(Scenario::ports(false)); } -fn drive_frame(scenario: Scenario) { - let tally = Tally::default(); - - bolero::check!().with_generator(scenario).cloned().for_each( - |(exposes, probes): (Vec, Vec)| { - tally.seen.fetch_add(1, Ordering::Relaxed); - let Some(fabric) = fabric(&exposes) else { - return; - }; - tally.built.fetch_add(1, Ordering::Relaxed); - let mut nf = fabric.nf(); - - for spec in &probes { - let mut probe = (*spec).resolve(&fabric); - let (destination, sport, dport) = (probe.destination, probe.sport, probe.dport); - let proto = if probe.tcp { - NextHeader::TCP - } else { - NextHeader::UDP +macro_rules! drive_frame { + ($scenario:expr) => {{ + let tally = Tally::default(); + + bolero::check!() + .with_generator($scenario) + .cloned() + .for_each(|(exposes, probes): (Vec, Vec)| { + tally.seen.fetch_add(1, Ordering::Relaxed); + let Some(fabric) = fabric(&exposes) else { + return; }; - let out = run(&mut nf, vec![probe.take()]); - let packet = &out[0]; + tally.built.fetch_add(1, Ordering::Relaxed); + let mut nf = fabric.nf(); + + for spec in &probes { + let mut probe = (*spec).resolve(&fabric); + let (destination, sport, dport) = (probe.destination, probe.sport, probe.dport); + let proto = if probe.tcp { + NextHeader::TCP + } else { + NextHeader::UDP + }; + let out = run(&mut nf, vec![probe.take()]); + let packet = &out[0]; - assert_eq!( - packet.ip_destination(), - Some(destination), - "source translation rewrote the destination" - ); - assert_eq!( - packet.transport_dst_port().map(NonZero::get), - Some(dport), - "source translation rewrote the destination port" - ); - assert_eq!( - packet.ip_proto(), - Some(proto), - "source translation changed the transport protocol" - ); - if !fabric.uses_ports { assert_eq!( - packet.transport_src_port().map(NonZero::get), - Some(sport), - "source translation rewrote the source port, which no expose asked for" + packet.ip_destination(), + Some(destination), + "source translation rewrote the destination" ); + assert_eq!( + packet.transport_dst_port().map(NonZero::get), + Some(dport), + "source translation rewrote the destination port" + ); + assert_eq!( + packet.ip_proto(), + Some(proto), + "source translation changed the transport protocol" + ); + if !fabric.uses_ports { + assert_eq!( + packet.transport_src_port().map(NonZero::get), + Some(sport), + "source translation rewrote the source port, which no expose asked for" + ); + } + tally.reached.fetch_add(1, Ordering::Relaxed); } - tally.reached.fetch_add(1, Ordering::Relaxed); - } - }, - ); + }); - tally.report("frame"); + tally.report("frame"); + }}; } #[test] fn translation_touches_only_the_source() { - drive_frame(Scenario::addresses(false)); + drive_frame!(Scenario::addresses(false)); } #[test] fn port_translation_touches_only_the_source() { - drive_frame(Scenario::ports(false)); + drive_frame!(Scenario::ports(false)); } -fn drive_permission(scenario: Scenario) { - let tally = Tally::default(); +macro_rules! drive_permission { + ($scenario:expr) => {{ + let tally = Tally::default(); + + bolero::check!() + .with_generator($scenario) + .cloned() + .for_each(|(exposes, probes): (Vec, Vec)| { + tally.seen.fetch_add(1, Ordering::Relaxed); + let Some(fabric) = fabric(&exposes) else { + return; + }; + tally.built.fetch_add(1, Ordering::Relaxed); + let mut nf = fabric.nf(); - bolero::check!().with_generator(scenario).cloned().for_each( - |(exposes, probes): (Vec, Vec)| { - tally.seen.fetch_add(1, Ordering::Relaxed); - let Some(fabric) = fabric(&exposes) else { - return; - }; - tally.built.fetch_add(1, Ordering::Relaxed); - let mut nf = fabric.nf(); + for spec in &probes { + let mut probe = (*spec).resolve(&fabric); + if probe.asks_for_translation() && probe.exposed { + continue; + } - for spec in &probes { - let mut probe = (*spec).resolve(&fabric); - if probe.asks_for_translation() && probe.exposed { - continue; - } + let (source, sport) = (probe.source, probe.sport); + let (stray, arrival) = (probe.stray, probe.arrival); + let out = run(&mut nf, vec![probe.take()]); - let (source, sport) = (probe.source, probe.sport); - let (stray, arrival) = (probe.stray, probe.arrival); - let out = run(&mut nf, vec![probe.take()]); - - assert_eq!( - five_tuple_source(&out[0]), - (source, sport), - "static NAT translated {source}:{sport} although {stray:?} forbade it; the \ + assert_eq!( + five_tuple_source(&out[0]), + (source, sport), + "static NAT translated {source}:{sport} although {stray:?} forbade it; the \ packet arrived as {arrival:?}" - ); - tally.reached.fetch_add(1, Ordering::Relaxed); - } - }, - ); + ); + tally.reached.fetch_add(1, Ordering::Relaxed); + } + }); - tally.report("permission"); + tally.report("permission"); + }}; } #[test] fn nothing_is_translated_without_permission() { - drive_permission(Scenario::addresses(true)); + drive_permission!(Scenario::addresses(true)); } #[test] fn no_port_is_translated_without_permission() { - drive_permission(Scenario::ports(true)); + drive_permission!(Scenario::ports(true)); } -fn drive_attribution(scenario: Scenario) { +macro_rules! drive_attribution { + ($scenario:expr) => {{ let tally = Tally::default(); - bolero::check!().with_generator(scenario).cloned().for_each( + bolero::check!().with_generator($scenario).cloned().for_each( |(exposes, probes): (Vec, Vec)| { tally.seen.fetch_add(1, Ordering::Relaxed); let Some(fabric) = fabric(&exposes) else { @@ -375,17 +386,19 @@ fn drive_attribution(scenario: Scenario) { ); tally.report("attribution"); +}}; } #[test] fn a_packet_that_cannot_be_looked_up_says_so() { - drive_attribution(Scenario::addresses(true)); + drive_attribution!(Scenario::addresses(true)); } -fn drive_marking(scenario: Scenario) { +macro_rules! drive_marking { + ($scenario:expr) => {{ let tally = Tally::default(); - bolero::check!().with_generator(scenario).cloned().for_each( + bolero::check!().with_generator($scenario).cloned().for_each( |(exposes, probes): (Vec, Vec)| { tally.seen.fetch_add(1, Ordering::Relaxed); let Some(fabric) = fabric(&exposes) else { @@ -421,14 +434,15 @@ fn drive_marking(scenario: Scenario) { ); tally.report("marking"); +}}; } #[test] fn a_modified_packet_is_always_marked() { - drive_marking(Scenario::addresses(true)); + drive_marking!(Scenario::addresses(true)); } #[test] fn a_port_modified_packet_is_always_marked() { - drive_marking(Scenario::ports(true)); + drive_marking!(Scenario::ports(true)); } From c49f3b8dc2d9038b9da58de93c47e20c7a691ef6 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 27 Aug 2026 15:23:13 -0600 Subject: [PATCH 14/14] test(nat,acl-filter): Drop the absolute floor the vacuity guards carried The comment three lines above these asserts says an absolute floor measures how fast the machine was rather than anything about the property. The asserts carried one anyway, and coverage instrumentation duly failed it: `a_flow_that_cannot_be_masqueraded_says_so` reached 3 flows across 1 configuration, which satisfies `reached * 2 >= built` and misses `reached >= 8`. The ratio is the part that means something, and a property that has stopped reaching its assertion collapses to zero, which `reached > 0` catches. Signed-off-by: Daniel Noland --- acl-filter/src/nf_fuzz.rs | 6 ++---- nat/src/masquerade/fuzz.rs | 4 +--- nat/src/static_nat/fuzz.rs | 4 +--- 3 files changed, 4 insertions(+), 10 deletions(-) diff --git a/acl-filter/src/nf_fuzz.rs b/acl-filter/src/nf_fuzz.rs index 48b0cf7ce5..2ab77ecd3b 100644 --- a/acl-filter/src/nf_fuzz.rs +++ b/acl-filter/src/nf_fuzz.rs @@ -19,8 +19,6 @@ use net::udp::UdpPort; use pipeline::NetworkFunction; use std::net::IpAddr; -const MIN_REACHED: usize = 8; - const PROBES: usize = 8; fn packet_for(summary: &PacketSummary) -> Option> { @@ -99,7 +97,7 @@ impl Tally { } println!("{what}: {reached}/{drawn} probes became packets"); assert!( - reached >= MIN_REACHED && reached * 4 >= drawn, + reached > 0 && reached * 4 >= drawn, "only {reached} of {drawn} probes became packets, so the {what} assertion is barely \ running" ); @@ -116,7 +114,7 @@ impl Tally { } println!("{what}: {reached}/{drawn} probes became packets, {denied} of them denied"); assert!( - reached >= MIN_REACHED && reached * 4 >= drawn, + reached > 0 && reached * 4 >= drawn, "only {reached} of {drawn} probes became packets, so the {what} assertion is barely \ running" ); diff --git a/nat/src/masquerade/fuzz.rs b/nat/src/masquerade/fuzz.rs index 97968b0664..ca7d004bc9 100644 --- a/nat/src/masquerade/fuzz.rs +++ b/nat/src/masquerade/fuzz.rs @@ -16,8 +16,6 @@ use std::num::NonZero; const MAX_EXPOSES: u8 = 3; -const MIN_REACHED: usize = 8; - const PROBES: usize = 8; #[derive(Debug, Clone, Copy)] @@ -99,7 +97,7 @@ impl Tally { like it did" ); assert!( - reached >= MIN_REACHED && reached * 2 >= built, + reached > 0 && reached * 2 >= built, "{reached} flows reached the {what} assertion across {built} configurations; \ this property has gone vacuous" ); diff --git a/nat/src/static_nat/fuzz.rs b/nat/src/static_nat/fuzz.rs index 6360641059..7987784ea3 100644 --- a/nat/src/static_nat/fuzz.rs +++ b/nat/src/static_nat/fuzz.rs @@ -19,8 +19,6 @@ use std::num::NonZero; const MAX_EXPOSES: u8 = 3; -const MIN_REACHED: usize = 8; - const PROBES: usize = 8; #[derive(Debug, Clone, Copy)] @@ -114,7 +112,7 @@ impl Tally { like it did" ); assert!( - reached >= MIN_REACHED && reached * 2 >= built, + reached > 0 && reached * 2 >= built, "{reached} probes reached the {what} assertion across {built} configurations; \ this property has gone vacuous" );