From b00c2bb39f2f8c7e89c663f063edaae53d504e67 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 | 1 - 1 file changed, 1 deletion(-) diff --git a/k8s-intf/src/bolero/crd.rs b/k8s-intf/src/bolero/crd.rs index 532f646502..18037f6eeb 100644 --- a/k8s-intf/src/bolero/crd.rs +++ b/k8s-intf/src/bolero/crd.rs @@ -99,7 +99,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 287c5d2e8ef6e008eb7f9c1994a778478093b41e 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 54dbcfd51f..1e58fd4713 100644 --- a/config/src/external/overlay/vpcpeering.rs +++ b/config/src/external/overlay/vpcpeering.rs @@ -1171,31 +1171,58 @@ pub mod contract { pub family: Family, } + #[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 = self.family.is_v4(driver)?; - 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))? { @@ -1220,19 +1247,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 a78c00af9e58420245824e95bbeeabd24680695f 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 | 392 ++++++++++++++++++++++++++++++++++++ nat/src/static_nat/mod.rs | 2 + nat/src/static_nat/probe.rs | 320 +++++++++++++++++++++++++++++ 3 files changed, 714 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..57a28d5377 --- /dev/null +++ b/nat/src/static_nat/fuzz.rs @@ -0,0 +1,392 @@ +// 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) +} + +/// Whether this run saw enough to judge the ratios below, or only to print them. +/// +/// Three kinds of run are too small, and they are not interchangeable: +/// +/// - **instrumentation.** A coverage run of these properties got through a +/// single configuration. The ratios then measure the build, not the property. +/// - **emulation.** miri and qemu-user see roughly two orders of magnitude +/// fewer cases; the same stand-down appears in `clock` and in the config +/// algebra's completeness table. +/// - **a corpus replay.** bolero runs exactly the inputs it is handed, so +/// `BOLERO_RANDOM_ITERATIONS=0` over one saved entry is *one case*. An +/// aggregate rate over one case reports how that case happened to fall -- +/// and that is precisely the run a developer makes to confirm a fix, so +/// failing it there is worse than not checking at all. +/// +/// The floor separates the last of those from a real campaign. It has to sit +/// below what the smallest honest run reaches, which is why it is measured +/// rather than picked. +/// +/// Measured natively 2026-09-06: 1060 to 2863 configurations per property here, +/// where the masquerade twin reaches 12 to 28. The floor matches the twin's +/// rather than this crate's own headroom, so the two do not drift apart, and it +/// is deliberately the smallest sample a rate can be computed from at all rather +/// than a fraction of either measurement -- a floor scaled to a fast machine +/// stands down on a slow one, which is the failure mode of a guard. +fn judged(built: usize) -> bool { + /// One case cannot support a rate; anything above that, the ratios can speak to. + const ENOUGH_CONFIGURATIONS: usize = 2; + !cfg!(instrumented) && !cfg!(emulated) && built >= ENOUGH_CONFIGURATIONS +} + +#[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"); + if !judged(built) { + println!(" {what}: not judged -- {built} configurations is too small a sample"); + return; + } + 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 e090c43bf3efc0b23076680da60da3cf783a307a 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 1e58fd4713..2cb094c70e 100644 --- a/config/src/external/overlay/vpcpeering.rs +++ b/config/src/external/overlay/vpcpeering.rs @@ -1172,11 +1172,26 @@ pub mod contract { } #[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 } } } @@ -1198,9 +1213,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() } } @@ -1223,6 +1244,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 57a28d5377..d4fae3b80e 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), + ) +} + /// Whether this run saw enough to judge the ratios below, or only to print them. /// /// Three kinds of run are too small, and they are not interchangeable: @@ -117,14 +153,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; @@ -134,41 +167,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; @@ -176,44 +214,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; @@ -238,12 +284,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" ); @@ -252,21 +293,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; @@ -280,31 +336,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; @@ -324,9 +388,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" @@ -340,19 +403,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; @@ -362,31 +428,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 519a8ce8707a05a065e9297ceef3b40f9cb26acb 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 | 87 +++-- nat/src/masquerade/fuzz.rs | 417 ++++++++++++++++++++++ nat/src/masquerade/mod.rs | 2 + nat/src/masquerade/probe.rs | 239 +++++++++++++ 4 files changed, 723 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 2cb094c70e..f9b2795345 100644 --- a/config/src/external/overlay/vpcpeering.rs +++ b/config/src/external/overlay/vpcpeering.rs @@ -1110,37 +1110,80 @@ pub mod contract { pub family: Family, } + #[derive(Debug, Clone, Copy)] + pub struct MasqueradeExposes(pub u8); + + impl Default for MasqueradeExposes { + fn default() -> Self { + Self(3) + } + } + + const MASQUERADE_SLOT: u8 = 4; + + /// The most exposes one manifest can be given distinct address blocks. + /// + /// Each expose is placed at `slot * MASQUERADE_SLOT` in a `u8`, so there are + /// exactly `256 / MASQUERADE_SLOT` of them. Asking for more used to wrap: slot + /// 64 landed back on block 0, duplicating the first expose's prefixes, and the + /// manifest was then rejected for overlapping. A generator of *legal* values + /// that quietly starts emitting illegal ones is the worst way to fail, because + /// the property does not break -- it just stops testing anything. + /// + /// `+ 1` because the domain has 256 values and `u8::MAX` is 255: slots run + /// `0..64`, and the last of them starts at `63 * 4 == 252`, which still fits. + /// Without it the cap is 63 and the last legal request is unreachable -- a + /// quieter version of the same fault this constant exists to prevent. + const MAX_MASQUERADE_EXPOSES: u8 = u8::MAX / MASQUERADE_SLOT + 1; + + impl ValueGenerator for MasqueradeExposes { + type Output = Vec; + + fn generate(&self, driver: &mut D) -> Option> { + let v4 = driver.produce::()?; + let most = self.0.clamp(1, MAX_MASQUERADE_EXPOSES); + let count = driver.gen_u8(Included(&1), Included(&most))?; + (0..count) + .map(|slot| masquerade_expose(driver, v4, slot * MASQUERADE_SLOT)) + .collect() + } + } + impl ValueGenerator for MasqueradeExpose { type Output = VpcExpose; fn generate(&self, driver: &mut D) -> Option { let v4 = self.family.is_v4(driver)?; - 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..b312254bdc --- /dev/null +++ b/nat/src/masquerade/fuzz.rs @@ -0,0 +1,417 @@ +// 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), + ) +} + +/// Whether this run saw enough to judge the ratios below, or only to print them. +/// +/// Three kinds of run are too small, and they are not interchangeable: +/// +/// - **instrumentation.** A coverage run of these properties got through a +/// single configuration. The ratios then measure the build, not the property. +/// - **emulation.** miri and qemu-user see roughly two orders of magnitude +/// fewer cases; the same stand-down appears in `clock` and in the config +/// algebra's completeness table. +/// - **a corpus replay.** bolero runs exactly the inputs it is handed, so +/// `BOLERO_RANDOM_ITERATIONS=0` over one saved entry is *one case*. An +/// aggregate rate over one case reports how that case happened to fall -- +/// and that is precisely the run a developer makes to confirm a fix, so +/// failing it there is worse than not checking at all. +/// +/// The floor separates the last of those from a real campaign. It has to sit +/// below what the smallest honest run reaches, which is why it is measured +/// rather than picked. +/// +/// Measured natively 2026-09-06: 12 to 28 configurations per property, against +/// 1060 to 2863 for the static-NAT twin. The difference is the cost of a case +/// here -- a flow table and the timers that go with it -- and not, as an earlier +/// version of this comment guessed, anything about how the runtime is driven: the +/// counts are the same either side of the commit that changed that. +/// +/// The floor is therefore *two*, not a fraction of the measured run. A CI runner +/// at a third of this machine's throughput would sit under any floor calibrated +/// on the numbers above, and a guard that stands down everywhere it runs is worse +/// than no guard. Two is the smallest sample a rate can be computed from at all, +/// which is exactly the corpus-replay case and nothing else. The ratio is the +/// part that carries meaning -- see the commit that removed the previous absolute +/// floor for why a bigger number measures the machine rather than the property. +fn judged(built: usize) -> bool { + /// One case cannot support a rate; anything above that, the ratios can speak to. + const ENOUGH_CONFIGURATIONS: usize = 2; + !cfg!(instrumented) && !cfg!(emulated) && built >= ENOUGH_CONFIGURATIONS +} + +#[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"); + if !judged(built) { + println!(" {what}: not judged -- {built} configurations is too small a sample"); + return; + } + 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() { + // Not `reached`: a dropped packet never gets as far as the + // assertion below, so counting it here would let the vacuity + // guard be satisfied entirely by packets that checked nothing. + // `exclusivity` above is the shape to copy -- count past the + // assert, never before it. + 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..bc8ba812c4 --- /dev/null +++ b/nat/src/masquerade/probe.rs @@ -0,0 +1,239 @@ +// 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}; + +// `FlowTable::new` takes a *shard* count, not a capacity -- capacity is set +// separately, by `set_capacity`. Named `FLOW_CAPACITY` and set to 4096, this +// asked for 4096 DashMap shards per case and set no capacity whatsoever. 16 is +// what every other call site in the tree passes. +const FLOW_SHARDS: usize = 16; + +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_SHARDS)); + 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 3007e735404d64dab90dc07e30258379f33e140e 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 | 300 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 312 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..b3fc6402a3 --- /dev/null +++ b/acl-filter/src/nf_fuzz.rs @@ -0,0 +1,300 @@ +// 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()) +} + +/// Whether this run saw enough to judge the ratios below, or only to print them. +/// +/// Three kinds of run are too small, and they are not interchangeable: +/// +/// - **instrumentation.** A coverage run of these properties got through a +/// single configuration. The ratios then measure the build, not the property. +/// - **emulation.** miri and qemu-user see roughly two orders of magnitude +/// fewer cases; the same stand-down appears in `clock` and in the config +/// algebra's completeness table. +/// - **a corpus replay.** bolero runs exactly the inputs it is handed, so +/// `BOLERO_RANDOM_ITERATIONS=0` over one saved entry is *one case*. An +/// aggregate rate over one case reports how that case happened to fall -- +/// and that is precisely the run a developer makes to confirm a fix, so +/// failing it there is worse than not checking at all. +/// +/// The floor separates the last of those from a real campaign. It has to sit +/// below what the smallest honest run reaches, which is why it is measured +/// rather than picked. +/// +/// Measured natively 2026-09-06: 27480 to 31616 probes drawn per property, at +/// `PROBES` per case, of which 24% were denied. One replayed corpus entry draws +/// `PROBES` and can legitimately deny none of them -- which is the failure this +/// floor exists to prevent. +fn judged(drawn: usize) -> bool { + /// Sixteen cases' worth: far under any real run, far over a single replay. + const ENOUGH_PROBES: usize = PROBES * 16; + !cfg!(instrumented) && !cfg!(emulated) && drawn >= ENOUGH_PROBES +} + +#[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"); + if !judged(drawn) { + println!(" {what}: not judged -- {drawn} probes is too small a sample"); + return; + } + 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 c99f306d3cbf5e530c179f75c3daeb8205eee8e9 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 | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/acl-filter/src/nf_fuzz.rs b/acl-filter/src/nf_fuzz.rs index b3fc6402a3..51072b318e 100644 --- a/acl-filter/src/nf_fuzz.rs +++ b/acl-filter/src/nf_fuzz.rs @@ -118,12 +118,35 @@ 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"); + if !judged(drawn) { + println!(" {what}: not judged -- {drawn} probes is too small a sample"); + return; + } + 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"); if !judged(drawn) { println!(" {what}: not judged -- {drawn} probes is too small a sample"); @@ -227,11 +250,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] @@ -259,11 +281,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] @@ -292,9 +313,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 45a7ee3f222f07aeb245d2dcc92fe3e4ca404bab 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 bc8ba812c4..171e667733 100644 --- a/nat/src/masquerade/probe.rs +++ b/nat/src/masquerade/probe.rs @@ -46,14 +46,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 213ff704ce73097f78c9e6327719c07e39206b3d 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 b312254bdc..1db52d6753 100644 --- a/nat/src/masquerade/fuzz.rs +++ b/nat/src/masquerade/fuzz.rs @@ -127,6 +127,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"); if !judged(built) { println!(" {what}: not judged -- {built} configurations is too small a sample"); From e01d78f3d67e6d3d713f4f750a2c023f56b605ac 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 171e667733..23fca29fa1 100644 --- a/nat/src/masquerade/probe.rs +++ b/nat/src/masquerade/probe.rs @@ -49,7 +49,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 5086b82cf6d1815f557d28040a691696636b6c35 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 51072b318e..971eb6f5cf 100644 --- a/acl-filter/src/nf_fuzz.rs +++ b/acl-filter/src/nf_fuzz.rs @@ -182,7 +182,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 f5173fcd3d240b17f8d41d8727185299e4744f1d 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 d4fae3b80e..021cd432c6 100644 --- a/nat/src/static_nat/fuzz.rs +++ b/nat/src/static_nat/fuzz.rs @@ -135,6 +135,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"); if !judged(built) { println!(" {what}: not judged -- {built} configurations is too small a sample"); From 17052a871176b9a13eec44bda430eb3cc767347c 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 021cd432c6..924f9f9e00 100644 --- a/nat/src/static_nat/fuzz.rs +++ b/nat/src/static_nat/fuzz.rs @@ -156,10 +156,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 { @@ -193,22 +194,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 { @@ -246,131 +249,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 { @@ -410,17 +421,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 { @@ -456,14 +469,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 c57245248452c39388c0b6758264a111fddd9122 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 971eb6f5cf..a493194900 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> { @@ -132,7 +130,7 @@ impl Tally { return; } 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" ); @@ -153,7 +151,7 @@ impl Tally { return; } 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 1db52d6753..0e55bf933d 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)] @@ -141,7 +139,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 924f9f9e00..06a8b5ef21 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)] @@ -149,7 +147,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" );