From 9aaf1d853dbca8bd5817a078df35581c13940d40 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 00:21:05 -0600 Subject: [PATCH 01/23] fix(flow-entry): Add an insertion that will not displace a live flow Two packets of one new flow can reach a NAT stage at the same time. Packets of a 5-tuple usually land on one core, but nothing guarantees that, and each packet builds a pair of its own before inserting it. With a plain insert, whoever gets there second displaces the other's forward flow -- and only that half. The two reverse keys carry the allocations that made them, no two allocations agree, so the reverses never collide and the loser's is never displaced along with its partner. It stays in the table, live, mapping a translation whose allocation goes back to the pool as soon as the displaced forward half is collected. Return traffic for that public pair, once it has been handed out again, is then translated for whoever held it before. Two changes, either of which leaves a hole on its own. insert_if_absent stands aside when a live flow already holds the key, and reports that flow so the caller can go on with it. Arbitrating on one key is enough, because racing packets of a single flow share their forward key by construction: only whoever wins it inserts a reverse. A flow that is present but no longer live is displaced as before, since it is a corpse its timer has not swept yet and standing aside for one would drop a packet that could have replaced it. Displacing a flow now also invalidates the other half of its pair, wherever it happens. The race is not the only way to reach the orphan: the flow timer expires the two halves separately, so an expired forward half could be replaced by an ordinary insert while its partner was still live. That path needs no concurrency at all. Three tests, each of which fails against the code without its guard: a live flow keeps its key, a dead one does not, and displacing a flow takes its partner with it. Rebased onto a `related_pair` that is fallible and requires exactly one half of the pair to carry `INITIATOR`; both are invariants main gained after this was written. The test now marks the forward half and unwraps, matching the sibling test in `concurrent_fuzz.rs`. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- flow-entry/src/flow_table/mod.rs | 2 +- flow-entry/src/flow_table/table.rs | 181 ++++++++++++++++++++++++++--- 2 files changed, 164 insertions(+), 19 deletions(-) diff --git a/flow-entry/src/flow_table/mod.rs b/flow-entry/src/flow_table/mod.rs index a8d246a881..10307d6ace 100644 --- a/flow-entry/src/flow_table/mod.rs +++ b/flow-entry/src/flow_table/mod.rs @@ -9,7 +9,7 @@ pub mod table; mod concurrent_fuzz; pub use nf_lookup::FlowLookup; -pub use table::{FlowTable, FlowTableReadGuard}; +pub use table::{FlowTable, FlowTableReadGuard, Insertion}; pub use net::flows::atomic_instant::AtomicInstant; pub use net::flows::flow_info::*; diff --git a/flow-entry/src/flow_table/table.rs b/flow-entry/src/flow_table/table.rs index 27695322f7..9972072937 100644 --- a/flow-entry/src/flow_table/table.rs +++ b/flow-entry/src/flow_table/table.rs @@ -19,6 +19,12 @@ pub enum FlowTableError { CapacityExceeded, } +#[derive(Debug)] +pub enum Insertion { + Installed, + Occupied(Arc), +} + type Table = DashMap, RandomState>; #[derive(Debug)] @@ -212,25 +218,41 @@ impl FlowTable { }); } + fn admit(&self, table: &Table, val: &Arc) -> Result<(), FlowTableError> { + if table.len() < self.capacity.load(Ordering::Relaxed) { + return Ok(()); + } + let has_related_in_table = val + .related + .as_ref() + .and_then(Weak::upgrade) + .is_some_and(|rel| rel.is_active()); + + if has_related_in_table { + Ok(()) + } else { + Err(FlowTableError::CapacityExceeded) + } + } + + fn displace(old: Option<&Arc>) { + let Some(old) = old else { + return; + }; + old.update_status(FlowStatus::Detached); + old.token.cancel(); + if let Some(related) = old.related.as_ref().and_then(Weak::upgrade) { + debug!("insert: invalidating the partner of a displaced flow"); + related.invalidate(); + } + } + fn insert_common(&self, val: &Arc) -> Result>, FlowTableError> { let table = self.table.read(); - let capacity = self.capacity.load(Ordering::Relaxed); let flow_key = val.flowkey(); debug!("insert: inserting flow {flow_key}"); - // Reject new flows when at capacity. Exception: always admit the second half of a - // related pair (e.g. the reverse NAT flow) to avoid leaving a one-sided entry. - if table.len() >= capacity { - let has_related_in_table = val - .related - .as_ref() - .and_then(Weak::upgrade) - .is_some_and(|rel| rel.is_active()); - - if !has_related_in_table { - return Err(FlowTableError::CapacityExceeded); - } - } + self.admit(&table, val)?; let result = table.insert(*flow_key, val.clone()); // Set Active only after the insert so that the invariant holds: Active iff in the @@ -243,10 +265,7 @@ impl FlowTable { #[cfg(not(any(feature = "shuttle", feature = "loom")))] Self::start_timer(self.table.clone(), val.clone()); - if let Some(old) = result.as_ref() { - old.update_status(FlowStatus::Detached); - old.token.cancel(); - } + Self::displace(result.as_ref()); let Some(ret) = result else { return Ok(None); @@ -259,6 +278,46 @@ impl FlowTable { Ok(Some(ret)) } + pub fn insert_if_absent(&self, val: &Arc) -> Result { + let table = self.table.read(); + let flow_key = val.flowkey(); + debug!("insert: inserting flow {flow_key} unless it is already held"); + + self.admit(&table, val)?; + + let displaced = match table.entry(*flow_key) { + dashmap::Entry::Occupied(mut occupied) => { + if occupied.get().is_active() { + Err(occupied.get().clone()) + } else { + Ok(Some(occupied.insert(val.clone()))) + } + } + dashmap::Entry::Vacant(vacant) => { + vacant.insert(val.clone()); + Ok(None) + } + }; + let displaced = match displaced { + Ok(displaced) => displaced, + Err(held) => { + drop(table); + debug!("insert: flow {flow_key} is already held by a live flow"); + return Ok(Insertion::Occupied(held)); + } + }; + + val.update_status(FlowStatus::Active); + drop(table); + + #[cfg(not(any(feature = "shuttle", feature = "loom")))] + Self::start_timer(self.table.clone(), val.clone()); + + Self::displace(displaced.as_ref()); + + Ok(Insertion::Installed) + } + /// Lookup a flow in the table. /// /// # Panics @@ -412,6 +471,7 @@ mod tests { #[concurrency_mode(std)] mod std_tests { + use net::flows::FlowInfoFlags; use std::time::Instant; use tracing_test::traced_test; @@ -634,6 +694,91 @@ mod tests { assert_eq!(flow_table.active_len().unwrap(), 0); } + fn key_for(src_port: u16) -> FlowKey { + FlowKey::new( + Some(VpcDiscriminant::VNI(Vni::new_checked(1).unwrap())), + "1.2.3.4".parse::().unwrap(), + "4.5.6.7".parse::().unwrap(), + IpProtoKey::Tcp(TcpProtoKey { + src_port: TcpPort::new_checked(src_port).unwrap(), + dst_port: TcpPort::new_checked(2048).unwrap(), + }), + ) + } + + #[tokio::test] + async fn an_active_flow_holds_its_key_against_a_second_insertion() { + let flow_table = FlowTable::default(); + let key = key_for(1025); + let far_future = Instant::now() + Duration::from_hours(1); + + let first = Arc::new(FlowInfo::new(key, far_future)); + assert!(matches!( + flow_table.insert_if_absent(&first).unwrap(), + Insertion::Installed + )); + + let second = Arc::new(FlowInfo::new(key, far_future)); + let outcome = flow_table.insert_if_absent(&second).unwrap(); + let Insertion::Occupied(held) = outcome else { + panic!("a live flow was displaced by a second insertion: {outcome:?}"); + }; + assert!(Arc::ptr_eq(&held, &first), "the wrong flow was reported"); + + let found = flow_table.lookup(&key).expect("the key is still served"); + assert!(Arc::ptr_eq(&found, &first)); + assert_ne!(second.status(), FlowStatus::Active); + } + + #[tokio::test] + async fn a_flow_that_is_not_live_is_displaced() { + let flow_table = FlowTable::default(); + let key = key_for(1026); + let far_future = Instant::now() + Duration::from_hours(1); + + let first = Arc::new(FlowInfo::new(key, far_future)); + flow_table.insert_if_absent(&first).unwrap(); + first.invalidate(); + + let second = Arc::new(FlowInfo::new(key, far_future)); + assert!( + matches!( + flow_table.insert_if_absent(&second).unwrap(), + Insertion::Installed + ), + "a flow that was no longer live held its key" + ); + let found = flow_table.lookup(&key).expect("the key is served"); + assert!(Arc::ptr_eq(&found, &second)); + } + + #[tokio::test] + async fn displacing_a_flow_invalidates_its_partner() { + let flow_table = FlowTable::default(); + let (forward_key, reverse_key) = (key_for(1027), key_for(1028)); + let far_future = Instant::now() + Duration::from_hours(1); + + let (forward, reverse) = FlowInfo::related_pair( + far_future, + forward_key, + FlowInfoFlags::INITIATOR, + reverse_key, + FlowInfoFlags::default(), + ) + .expect("related_pair should succeed for distinct keys"); + flow_table.insert_from_arc(&forward).unwrap(); + flow_table.insert_from_arc(&reverse).unwrap(); + assert!(reverse.is_active()); + + let replacement = Arc::new(FlowInfo::new(forward_key, far_future)); + flow_table.insert_from_arc(&replacement).unwrap(); + + assert!( + !reverse.is_active(), + "the partner of a displaced flow was left live in the table" + ); + } + #[tokio::test] async fn test_flow_table_capacity_exceeded() { let flow_table = FlowTable::default(); From 0b0046fe1414d961a563b650cdf505ec1bb851ac Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 09:37:04 -0600 Subject: [PATCH 02/23] feat(config): Generate port-forwarding exposes for property tests The configuration types had no generators, so every test of the path from a configuration to a NAT table was driven by a handful of hand-written overlays. That is the largest untested surface in the NAT crate: the code that turns exposes into static, masquerade and port-forwarding tables is reached only by the shapes somebody thought to write down. This is the first generator, for the port-forwarding flavour, chosen because it has the tightest validity rules and the smallest surface downstream. `config` grows an optional bolero dependency and a feature to go with it, following what `net` and `lpm` already do. Valid by construction rather than generate-and-reject. A rejected configuration still counts as a run, so a generator that produces them quietly buys less coverage than its iteration count suggests -- hence one prefix per side of one family, drawn from blocks that are not special-use, with a bounded port range on each side and matching totals. Two tests in `config` hold it to that, and they are how the overflow in its own port arithmetic was found: `start + count - 1` adds before it subtracts, and the sum reaches 65536 at the top of the range. The generator is deliberately narrower than the legal space. Validation checks that the two sides have equal size, where size counts addresses times ports, so sides with different prefix lengths and compensating port counts satisfy it -- while `PortFwEntry` checks prefix length and port count separately and rejects them. Generating that case would find the disagreement rather than test anything past it, so it is left out and written down in the generator's documentation. The property in `nat` is that an expose becomes the rules it describes, and mostly that the two sides do not get crossed: `as_range` is what traffic arrives on, `ips` is where it goes, and a rule holding them the other way round forwards to the wrong place while passing every check the rule itself makes. One constraint that is not obvious from reading: both manifests of a peering must be of one IP version, so a fixed IPv4 remote side cannot stand opposite a generated IPv6 expose. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- config/Cargo.toml | 4 + config/src/external/overlay/vpcpeering.rs | 128 ++++++++++++++++++++++ nat/Cargo.toml | 2 +- nat/src/portfw/portfwtable/setup.rs | 89 +++++++++++++++ 4 files changed, 222 insertions(+), 1 deletion(-) diff --git a/config/Cargo.toml b/config/Cargo.toml index 4c6ace6c8e..ffe89a893a 100644 --- a/config/Cargo.toml +++ b/config/Cargo.toml @@ -5,6 +5,9 @@ license.workspace = true publish.workspace = true version.workspace = true +[features] +bolero = ["dep:bolero", "lpm/bolero"] + [dependencies] # internal common = { workspace = true } @@ -16,6 +19,7 @@ net = { workspace = true } # external arc-swap = { workspace = true } +bolero = { workspace = true, optional = true, default-features = false, features = ["alloc"] } chrono = { workspace = true, features = ["alloc", "std"] } derive_builder = { workspace = true, features = [] } ipnet = { workspace = true } diff --git a/config/src/external/overlay/vpcpeering.rs b/config/src/external/overlay/vpcpeering.rs index d5f641a26c..d4cc7af214 100644 --- a/config/src/external/overlay/vpcpeering.rs +++ b/config/src/external/overlay/vpcpeering.rs @@ -1005,3 +1005,131 @@ impl VpcPeeringTable { .filter(move |p| p.left.name == vpc || p.right.name == vpc) } } + +#[cfg(any(test, feature = "bolero"))] +pub mod contract { + + use super::{VpcExpose, VpcExposeNatConfig}; + use bolero::{Driver, ValueGenerator}; + use lpm::prefix::{ + IpPrefix, Ipv4Prefix, Ipv6Prefix, L4Protocol, PortRange, Prefix, PrefixWithOptionalPorts, + }; + use std::net::{Ipv4Addr, Ipv6Addr}; + use std::ops::Bound::Included; + use std::time::Duration; + + const MAX_HOST_BITS: u8 = 8; + + const MAX_PORTS: u16 = 1024; + + #[derive(Debug, Clone, Copy, Default)] + pub struct PortForwardingExpose; + + impl ValueGenerator for PortForwardingExpose { + type Output = VpcExpose; + + fn generate(&self, driver: &mut D) -> Option { + let host_bits = driver.gen_u8(Included(&0), Included(&MAX_HOST_BITS))?; + let (internal, external) = if driver.produce::()? { + v4_pair(driver, host_bits)? + } else { + v6_pair(driver, host_bits)? + }; + + let count = driver.gen_u16(Included(&1), Included(&MAX_PORTS))?; + let internal_ports = port_range(driver, count)?; + let external_ports = port_range(driver, count)?; + + let proto = match driver.gen_u8(Included(&0), Included(&2))? { + 0 => L4Protocol::Tcp, + 1 => L4Protocol::Udp, + _ => L4Protocol::Any, + }; + let idle_timeout = match driver.gen_u8(Included(&0), Included(&2))? { + 0 => None, + 1 => Some(Duration::from_secs(5)), + _ => Some(Duration::from_mins(5)), + }; + + VpcExpose::empty() + .make_port_forwarding(idle_timeout, Some(proto)) + .ok()? + .ip(PrefixWithOptionalPorts::new(internal, Some(internal_ports))) + .as_range(PrefixWithOptionalPorts::new(external, Some(external_ports))) + .ok() + } + } + + #[must_use] + pub fn nat_config(expose: &VpcExpose) -> Option<&VpcExposeNatConfig> { + expose.nat_config() + } + + fn v4_pair(driver: &mut D, host_bits: u8) -> Option<(Prefix, Prefix)> { + let len = 32 - host_bits; + let mask = u32::MAX.checked_shl(u32::from(host_bits)).unwrap_or(0); + let internal = (0x0A00_0000 | (driver.produce::()? & 0x00FF_FFFF)) & mask; + let external = (0xAC10_0000 | (driver.produce::()? & 0x000F_FFFF)) & mask; + Some((prefix_v4(internal, len)?, prefix_v4(external, len)?)) + } + + const INTERNAL_BASE: u128 = 0x2001_0db8_0000_0000_0000_0000_0000_0000; + const EXTERNAL_BASE: u128 = 0x2001_0db8_0001_0000_0000_0000_0000_0000; + + fn v6_pair(driver: &mut D, host_bits: u8) -> Option<(Prefix, Prefix)> { + let len = 128 - host_bits; + let mask = u128::MAX.checked_shl(u32::from(host_bits)).unwrap_or(0); + let internal = (INTERNAL_BASE | u128::from(driver.produce::()?)) & mask; + let external = (EXTERNAL_BASE | u128::from(driver.produce::()?)) & mask; + Some((prefix_v6(internal, len)?, prefix_v6(external, len)?)) + } + + fn prefix_v4(bits: u32, len: u8) -> Option { + Ipv4Prefix::new(Ipv4Addr::from_bits(bits), len) + .ok() + .map(Prefix::from) + } + + fn prefix_v6(bits: u128, len: u8) -> Option { + Ipv6Prefix::new(Ipv6Addr::from_bits(bits), len) + .ok() + .map(Prefix::from) + } + + fn port_range(driver: &mut D, count: u16) -> Option { + let last_start = u16::MAX - (count - 1); + let start = driver.gen_u16(Included(&1), Included(&last_start))?; + PortRange::new(start, start + (count - 1)).ok() + } + + #[cfg(test)] + mod tests { + use super::*; + + #[test] + fn every_generated_expose_validates() { + bolero::check!() + .with_generator(PortForwardingExpose) + .for_each(|expose: &VpcExpose| { + let validated = expose.validate(); + assert!( + validated.is_ok(), + "generated expose was rejected: {expose} -- {:?}", + validated.err() + ); + }); + } + + #[test] + fn a_generated_expose_survives_validation_as_port_forwarding() { + bolero::check!() + .with_generator(PortForwardingExpose) + .for_each(|expose: &VpcExpose| { + let validated = expose.validate().unwrap_or_else(|e| panic!("{e:?}")); + assert!(validated.has_port_forwarding()); + assert_eq!(validated.ips().len(), 1); + assert_eq!(validated.as_range_or_empty().len(), 1); + }); + } + } +} diff --git a/nat/Cargo.toml b/nat/Cargo.toml index e46d2740a2..b2e69b14e7 100644 --- a/nat/Cargo.toml +++ b/nat/Cargo.toml @@ -36,7 +36,7 @@ shuttle = { workspace = true, optional = true } [dev-dependencies] # internal -config = { workspace = true } +config = { workspace = true, features = ["bolero"] } fixin = { workspace = true } test-utils = { workspace = true } lpm = { workspace = true, features = ["testing"] } diff --git a/nat/src/portfw/portfwtable/setup.rs b/nat/src/portfw/portfwtable/setup.rs index 812e14b329..9005ecf8cd 100644 --- a/nat/src/portfw/portfwtable/setup.rs +++ b/nat/src/portfw/portfwtable/setup.rs @@ -111,3 +111,92 @@ pub fn build_port_forwarding_configuration( } Ok(ruleset) } + +#[cfg(test)] +mod tests { + use super::*; + use config::external::overlay::Overlay; + use config::external::overlay::vpc::{Vpc, VpcTable}; + use config::external::overlay::vpcpeering::contract::PortForwardingExpose; + use config::external::overlay::vpcpeering::{ + VpcExpose, VpcManifest, VpcPeering, VpcPeeringTable, + }; + use lpm::prefix::Prefix; + + const LOCAL_VNI: u32 = 100; + const REMOTE_VNI: u32 = 200; + + fn overlay_offering(expose: VpcExpose) -> config::external::overlay::ValidatedOverlay { + let mut vpc_table = VpcTable::new(); + vpc_table + .add(Vpc::new("VPC-1", "AAAAA", LOCAL_VNI).expect("local vpc")) + .expect("add local vpc"); + vpc_table + .add(Vpc::new("VPC-2", "BBBBB", REMOTE_VNI).expect("remote vpc")) + .expect("add remote vpc"); + + let remote_prefix = match expose.ips.first().expect("one prefix").prefix() { + Prefix::IPV4(_) => "3.3.3.0/24", + Prefix::IPV6(_) => "2001:db8:ffff::/64", + }; + let local = VpcManifest::new("VPC-1").exposing(expose); + let remote = + VpcManifest::new("VPC-2").exposing(VpcExpose::empty().ip(remote_prefix.into())); + let mut peerings = VpcPeeringTable::new(); + peerings + .add(VpcPeering::with_default_group( + "VPC-1--VPC-2", + local, + remote, + )) + .expect("add peering"); + + Overlay::new(vpc_table, peerings) + .validate() + .expect("the overlay around a valid expose should validate") + } + + #[test] + fn an_expose_becomes_the_rules_it_describes() { + bolero::check!() + .with_generator(PortForwardingExpose) + .cloned() + .for_each(|expose: VpcExpose| { + let nat = expose.nat.as_ref().expect("port forwarding sets nat"); + let proto = nat.proto; + let internal = *expose.ips.first().expect("one prefix"); + let external = *nat.as_range.first().expect("one prefix"); + + let overlay = overlay_offering(expose.clone()); + let rules = build_port_forwarding_configuration(overlay.vpc_table()) + .expect("a validated port-forwarding expose should build"); + + let expected = if proto == L4Protocol::Any { 2 } else { 1 }; + assert_eq!(rules.len(), expected, "for {expose}"); + + for rule in &rules { + assert_eq!(rule.ext_prefix, external.prefix(), "external prefix"); + assert_eq!(rule.int_prefix, internal.prefix(), "internal prefix"); + assert_eq!( + rule.ext_ports.first().get(), + external.ports().expect("ports").start(), + "external ports" + ); + assert_eq!( + rule.int_ports.first().get(), + internal.ports().expect("ports").start(), + "internal ports" + ); + assert_eq!(rule.dst_vpcd, VpcDiscriminant::from_vni(vni(LOCAL_VNI))); + assert_eq!( + rule.key.src_vpcd(), + VpcDiscriminant::from_vni(vni(REMOTE_VNI)) + ); + } + }); + } + + fn vni(raw: u32) -> net::vxlan::Vni { + net::vxlan::Vni::new_checked(raw).unwrap_or_else(|_| unreachable!()) + } +} From 5905bb1d70edb6767e4f7af584b954269646f3d4 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 10:01:20 -0600 Subject: [PATCH 03/23] feat(config): Generate masquerade exposes, and share the overlay around them Second generator, and the scaffolding both of them now sit on. Masquerade's rules are looser than port forwarding's: several prefixes per side, and their sizes need not agree, which is the point of it -- many private addresses behind few public ones. The one thing it forbids is a port range on either side. Prefixes within a side are carved so as not to overlap, since a manifest rejects overlapping ones, and the two sides come from separate blocks. `overlay_offering` moves into the generator module from the port-forwarding test that first needed it. Every property downstream of a configuration needs an overlay to put the expose in, and the two constraints it has to satisfy are not obvious from reading: a manifest with no exposes is rejected, so the remote side has to expose something, and a peering's two manifests must agree on address family, so what it exposes has to follow whichever family the generated expose came from. The property in `nat` is that masquerade only ever hands out an address the expose named. That runs through most of the allocator -- the pool table finding a pool for the private source, the public space being cut into regions, the expose being given regions of its own -- and a mistake anywhere along it shows up as an address from somewhere else. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- config/src/external/overlay/vpcpeering.rs | 118 +++++++++++++++++++++- nat/src/portfw/portfwtable/setup.rs | 44 +------- 2 files changed, 121 insertions(+), 41 deletions(-) diff --git a/config/src/external/overlay/vpcpeering.rs b/config/src/external/overlay/vpcpeering.rs index d4cc7af214..2c0995e31a 100644 --- a/config/src/external/overlay/vpcpeering.rs +++ b/config/src/external/overlay/vpcpeering.rs @@ -1009,7 +1009,10 @@ impl VpcPeeringTable { #[cfg(any(test, feature = "bolero"))] pub mod contract { - use super::{VpcExpose, VpcExposeNatConfig}; + use super::{VpcExpose, VpcExposeNatConfig, VpcManifest, VpcPeering, VpcPeeringTable}; + use crate::ConfigError; + use crate::external::overlay::vpc::{Vpc, VpcTable}; + use crate::external::overlay::{Overlay, ValidatedOverlay}; use bolero::{Driver, ValueGenerator}; use lpm::prefix::{ IpPrefix, Ipv4Prefix, Ipv6Prefix, L4Protocol, PortRange, Prefix, PrefixWithOptionalPorts, @@ -1060,6 +1063,91 @@ pub mod contract { } } + #[derive(Debug, Clone, Copy, Default)] + pub struct MasqueradeExpose; + + impl ValueGenerator for MasqueradeExpose { + type Output = VpcExpose; + + fn generate(&self, driver: &mut D) -> Option { + let v4 = driver.produce::()?; + let privates = driver.gen_u8(Included(&1), Included(&3))?; + let publics = driver.gen_u8(Included(&1), Included(&2))?; + let base = driver.produce::()?; + let idle_timeout = match driver.gen_u8(Included(&0), Included(&2))? { + 0 => None, + 1 => Some(Duration::from_secs(30)), + _ => Some(Duration::from_mins(2)), + }; + + 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, + )) + .ok()?; + } + Some(expose) + } + } + + #[derive(Clone, Copy)] + enum Side { + Private, + Public, + } + + fn block(v4: bool, side: Side, index: u8) -> Option { + if v4 { + let bits = match side { + Side::Private => 0x0A00_0000 | (u32::from(index) << 16), + Side::Public => 0xAC10_0000 | (u32::from(index) << 8), + }; + prefix_v4(bits, 24) + } else { + let selector = match side { + Side::Private => 0u128, + Side::Public => 1, + }; + let bits = (0x2001_0db8u128 << 96) | (selector << 80) | (u128::from(index) << 64); + prefix_v6(bits, 64) + } + } + + pub const LOCAL_VNI: u32 = 100; + pub const REMOTE_VNI: u32 = 200; + + pub fn overlay_offering(expose: VpcExpose) -> Result { + let remote_prefix = match expose.ips.first().map(PrefixWithOptionalPorts::prefix) { + Some(Prefix::IPV6(_)) => "2001:db8:ffff::/64", + _ => "3.3.3.0/24", + }; + + let mut vpc_table = VpcTable::new(); + vpc_table.add(Vpc::new("VPC-1", "AAAAA", LOCAL_VNI)?)?; + vpc_table.add(Vpc::new("VPC-2", "BBBBB", REMOTE_VNI)?)?; + + let local = VpcManifest::new("VPC-1").exposing(expose); + let remote = + VpcManifest::new("VPC-2").exposing(VpcExpose::empty().ip(remote_prefix.into())); + let mut peerings = VpcPeeringTable::new(); + peerings.add(VpcPeering::with_default_group( + "VPC-1--VPC-2", + local, + remote, + ))?; + + Overlay::new(vpc_table, peerings).validate() + } + #[must_use] pub fn nat_config(expose: &VpcExpose) -> Option<&VpcExposeNatConfig> { expose.nat_config() @@ -1120,6 +1208,34 @@ pub mod contract { }); } + #[test] + fn every_generated_masquerade_expose_validates() { + bolero::check!() + .with_generator(MasqueradeExpose) + .for_each(|expose: &VpcExpose| { + let validated = expose.validate().unwrap_or_else(|e| { + panic!("generated expose was rejected: {expose} -- {e:?}") + }); + assert!(validated.has_masquerade()); + assert!(!validated.ips().is_empty()); + assert!(!validated.as_range_or_empty().is_empty()); + }); + } + + #[test] + fn a_generated_expose_can_be_offered_in_an_overlay() { + bolero::check!() + .with_generator(MasqueradeExpose) + .cloned() + .for_each(|expose: VpcExpose| { + let shown = expose.to_string(); + assert!( + overlay_offering(expose).is_ok(), + "could not build an overlay around {shown}" + ); + }); + } + #[test] fn a_generated_expose_survives_validation_as_port_forwarding() { bolero::check!() diff --git a/nat/src/portfw/portfwtable/setup.rs b/nat/src/portfw/portfwtable/setup.rs index 9005ecf8cd..0f1c27e34c 100644 --- a/nat/src/portfw/portfwtable/setup.rs +++ b/nat/src/portfw/portfwtable/setup.rs @@ -115,46 +115,10 @@ pub fn build_port_forwarding_configuration( #[cfg(test)] mod tests { use super::*; - use config::external::overlay::Overlay; - use config::external::overlay::vpc::{Vpc, VpcTable}; - use config::external::overlay::vpcpeering::contract::PortForwardingExpose; - use config::external::overlay::vpcpeering::{ - VpcExpose, VpcManifest, VpcPeering, VpcPeeringTable, + use config::external::overlay::vpcpeering::VpcExpose; + use config::external::overlay::vpcpeering::contract::{ + LOCAL_VNI, PortForwardingExpose, REMOTE_VNI, overlay_offering, }; - use lpm::prefix::Prefix; - - const LOCAL_VNI: u32 = 100; - const REMOTE_VNI: u32 = 200; - - fn overlay_offering(expose: VpcExpose) -> config::external::overlay::ValidatedOverlay { - let mut vpc_table = VpcTable::new(); - vpc_table - .add(Vpc::new("VPC-1", "AAAAA", LOCAL_VNI).expect("local vpc")) - .expect("add local vpc"); - vpc_table - .add(Vpc::new("VPC-2", "BBBBB", REMOTE_VNI).expect("remote vpc")) - .expect("add remote vpc"); - - let remote_prefix = match expose.ips.first().expect("one prefix").prefix() { - Prefix::IPV4(_) => "3.3.3.0/24", - Prefix::IPV6(_) => "2001:db8:ffff::/64", - }; - let local = VpcManifest::new("VPC-1").exposing(expose); - let remote = - VpcManifest::new("VPC-2").exposing(VpcExpose::empty().ip(remote_prefix.into())); - let mut peerings = VpcPeeringTable::new(); - peerings - .add(VpcPeering::with_default_group( - "VPC-1--VPC-2", - local, - remote, - )) - .expect("add peering"); - - Overlay::new(vpc_table, peerings) - .validate() - .expect("the overlay around a valid expose should validate") - } #[test] fn an_expose_becomes_the_rules_it_describes() { @@ -167,7 +131,7 @@ mod tests { let internal = *expose.ips.first().expect("one prefix"); let external = *nat.as_range.first().expect("one prefix"); - let overlay = overlay_offering(expose.clone()); + let overlay = overlay_offering(expose.clone()).expect("overlay"); let rules = build_port_forwarding_configuration(overlay.vpc_table()) .expect("a validated port-forwarding expose should build"); From 7b6b416b4f2b37fbbc371fbb7a37431faebafbcf Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 10:05:53 -0600 Subject: [PATCH 04/23] feat(config): Generate static NAT exposes, and pin the mapping is a bijection The last of the three NAT flavours, and the one the generator was worth building for. Static NAT's rule is that the two sides hold the same number of addresses while being free to be cut up differently: a /26 on one side can be answered by four /28s on the other. Working out the mapping across boundaries that do not line up is the whole job of `RangeBuilder`, the most intricate code in the NAT crate, and until now it was reached by one bolero test over hand-built inputs and a handful of examples. So the generator picks one total and splits it independently per side. Two things had to be got right for that to mean anything. Parts are laid out with a gap of their own size after each, not end to end. Placed end to end they are aligned siblings, and validation normalizes those back into a single prefix -- so the differing shapes the generator had just worked out were collapsed away before anything saw them. The generator's own test asserts the shapes do differ; without it the suite would have looked healthy while only ever testing one prefix per side. Sizes stay under 64 addresses so the property can enumerate rather than sample. The property is that the mapping is a bijection: every private address lands somewhere public, no two land in the same place, and between them they cover the public side exactly. Port ranges are left out. Static NAT permits them and they take the mapping down a second path -- `PortAddrTranslationValue` rather than `AddrTranslationValue` -- which carries its own unfinished work, and wants a generator written for it rather than this one stretched. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- config/src/external/overlay/vpcpeering.rs | 100 ++++++++++++++++++++++ nat/src/static_nat/setup/mod.rs | 91 ++++++++++++++++++++ 2 files changed, 191 insertions(+) diff --git a/config/src/external/overlay/vpcpeering.rs b/config/src/external/overlay/vpcpeering.rs index 2c0995e31a..72fa55c075 100644 --- a/config/src/external/overlay/vpcpeering.rs +++ b/config/src/external/overlay/vpcpeering.rs @@ -1122,6 +1122,85 @@ pub mod contract { } } + #[derive(Debug, Clone, Copy, Default)] + pub struct StaticNatExpose; + + const MAX_TOTAL_LOG: u8 = 6; + + impl ValueGenerator for StaticNatExpose { + type Output = VpcExpose; + + fn generate(&self, driver: &mut D) -> Option { + let v4 = driver.produce::()?; + let total_log = driver.gen_u8(Included(&0), Included(&MAX_TOTAL_LOG))?; + + let privates = place(v4, Side::Private, &split(driver, total_log)?)?; + let publics = place(v4, Side::Public, &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))? { + let splittable: Vec = parts + .iter() + .enumerate() + .filter(|(_, log)| **log > 0) + .map(|(index, _)| index) + .collect(); + if splittable.is_empty() { + break; + } + let choice = usize::from(driver.gen_u8( + Included(&0), + Included(&u8::try_from(splittable.len() - 1).ok()?), + )?); + let log = parts.swap_remove(splittable[choice]); + parts.push(log - 1); + parts.push(log - 1); + } + parts.sort_unstable_by(|a, b| b.cmp(a)); + 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, + }; + (0x2001_0db8u128 << 96) | (selector << 80) + }; + + let mut out = Vec::with_capacity(parts.len()); + for &log in parts { + let prefix = if v4 { + prefix_v4(u32::try_from(cursor).ok()?, 32 - log)? + } else { + prefix_v6(cursor, 128 - log)? + }; + out.push(prefix); + cursor += 2u128 << log; + } + Some(out) + } + pub const LOCAL_VNI: u32 = 100; pub const REMOTE_VNI: u32 = 200; @@ -1236,6 +1315,27 @@ pub mod contract { }); } + #[test] + fn every_generated_static_nat_expose_validates() { + let mut shapes_differed = false; + bolero::check!() + .with_generator(StaticNatExpose) + .for_each(|expose: &VpcExpose| { + let validated = expose.validate().unwrap_or_else(|e| { + panic!("generated expose was rejected: {expose} -- {e:?}") + }); + assert!(validated.has_static_nat()); + if validated.ips().len() != validated.as_range_or_empty().len() { + shapes_differed = true; + } + }); + assert!( + shapes_differed, + "no generated expose had a different number of prefixes on each side, so the \ + mapping was never asked to fragment" + ); + } + #[test] fn a_generated_expose_survives_validation_as_port_forwarding() { bolero::check!() diff --git a/nat/src/static_nat/setup/mod.rs b/nat/src/static_nat/setup/mod.rs index 484f3f8d4e..df0e0c8e16 100644 --- a/nat/src/static_nat/setup/mod.rs +++ b/nat/src/static_nat/setup/mod.rs @@ -206,3 +206,94 @@ mod tests { .expect("Failed to build NAT tables"); } } + +#[cfg(test)] +mod config_driven { + use super::*; + use config::external::overlay::vpcpeering::VpcExpose; + use config::external::overlay::vpcpeering::contract::{ + LOCAL_VNI, REMOTE_VNI, StaticNatExpose, overlay_offering, + }; + use lpm::prefix::PrefixWithOptionalPorts; + use std::collections::BTreeSet; + use std::net::IpAddr; + + fn vni(raw: u32) -> Vni { + Vni::new_checked(raw).unwrap_or_else(|_| unreachable!()) + } + + 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 + } + + #[test] + fn the_two_sides_of_an_expose_map_one_to_one() { + bolero::check!() + .with_generator(StaticNatExpose) + .cloned() + .for_each(|expose: VpcExpose| { + let private = addresses(&expose.ips); + let public: BTreeSet = addresses( + &expose + .nat + .as_ref() + .expect("static nat sets nat") + .as_range + .clone(), + ) + .into_iter() + .collect(); + + let overlay = overlay_offering(expose.clone()).expect("overlay"); + let tables = build_nat_configuration(overlay.vpc_table()) + .expect("a validated expose builds"); + let table = tables + .get_table(vni(LOCAL_VNI)) + .expect("the offering vpc has a table"); + + let mut seen = BTreeSet::new(); + for source in &private { + let (mapped, _) = table + .find_src_mapping(source, None, vni(REMOTE_VNI)) + .unwrap_or_else(|| panic!("{source} has no mapping in {expose}")); + let mapped = mapped.inner(); + assert!( + public.contains(&mapped), + "{source} mapped to {mapped}, which the expose does not offer" + ); + assert!( + seen.insert(mapped), + "{source} mapped to {mapped}, which another address already took" + ); + } + assert_eq!( + seen.len(), + public.len(), + "the mapping left part of the public side unused, for {expose}" + ); + }); + } +} From 06ce0dc505cbe12fb67e920cb5d288e664f9f848 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 10:25:46 -0600 Subject: [PATCH 05/23] fix(config): Refuse a port-forwarding expose the dataplane cannot build Validation compared the two sides of a port-forwarding expose by total size, where a total is addresses times ports. That is the right check for static NAT, whose whole job is mapping between differently shaped sides -- but a port-forwarding rule maps one prefix onto another address for address and one port range onto another positionally, so it can only express matched lengths and matched port counts. A product is equally satisfied by a /32 carrying 100 ports opposite a /30 carrying 25, and that pairing validated. `PortFwEntry::is_valid` refused it, so the configuration never took effect. The trouble is where it refused it. Port forwarding is the last of the NAT stages in `apply_gw_config`, and the sequence is a linear chain with no staging, so by the time it fails the kernel interfaces, the flow filter, the ACL tables, the static NAT tables and the masquerade allocator have all been committed. The apply then returns an error and rolls back, and the rollback restores the configuration -- but not the masquerade flows that rebuilding the allocator has already judged against the rejected config and torn down. Established connections break for a configuration that was never applied, and the box takes two disruptive transitions instead of none. So the check moves to where rejecting is free. The two lengths and the two port counts are compared directly, which is strictly stronger than the product they replace: with one prefix on each side, equal lengths and equal counts imply equal totals, while the converse is what let this through. `PortFwEntry` keeps its own checks, which still guard callers that build a rule without going through a configuration. `MismatchedPrefixLengths` and `MismatchedPortRangeSizes` each carry what did not line up, naming the private and the public side rather than taking two positional numbers of one type, since which is which is the whole content of the error. `MismatchedPrefixSizes` cannot be reused for the length case, tempting as that is: it compares addresses times ports, and the pairing this rejects has that product equal on both sides, so it would print two numbers that are the same and ask the operator to reconcile them. One shape is therefore reported differently than before: a /24 opposite a /25 said `MismatchedPrefixSizes(256, 128)`, and now says that port forwarding requires prefixes of the same length. That error's own message is reworded while here, since it named neither what has to hold nor which side is which. The numbers stay behind `Debug` because `PrefixWithPortsSize` is a 145-bit bnum type with no `Display`, and `Debug` pads it into a run of digits that reads as gibberish, so they come last rather than mid-sentence. Giving that type a `Display` is worth doing in lpm. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- config/src/errors.rs | 15 +++- .../src/external/overlay/validation_tests.rs | 8 +- config/src/external/overlay/vpcpeering.rs | 84 +++++++++++++++++-- 3 files changed, 97 insertions(+), 10 deletions(-) diff --git a/config/src/errors.rs b/config/src/errors.rs index bbc6509359..c359f93211 100644 --- a/config/src/errors.rs +++ b/config/src/errors.rs @@ -75,8 +75,21 @@ pub enum ConfigError { #[error("Invalid ACL configuration: {0}")] InvalidAcl(String), // NAT-specific - #[error("Mismatched prefixes sizes for static NAT: {0:?} and {1:?}")] + #[error( + "Mismatched sizes for static NAT: the exposed prefixes and the range they translate to \ + must cover the same number of address-port pairs (they cover {0:?} and {1:?})" + )] MismatchedPrefixSizes(PrefixWithPortsSize, PrefixWithPortsSize), + #[error( + "Mismatched prefix lengths for port forwarding: /{private} exposed and /{public} \ + translated to; a rule maps addresses one for one, so the two must be the same length" + )] + MismatchedPrefixLengths { private: u8, public: u8 }, + #[error( + "Mismatched port range sizes for port forwarding: {private} ports exposed and {public} \ + translated to; a rule maps ports one for one, so the two must be the same size" + )] + MismatchedPortRangeSizes { private: usize, public: usize }, #[error("Peering {0} has manifests using incompatible NAT modes")] IncompatibleNatModes(String), #[error("Vpc {0} has a peering with no exposes")] diff --git a/config/src/external/overlay/validation_tests.rs b/config/src/external/overlay/validation_tests.rs index 571811ebc8..b1644b38e1 100644 --- a/config/src/external/overlay/validation_tests.rs +++ b/config/src/external/overlay/validation_tests.rs @@ -646,7 +646,13 @@ mod test { .unwrap(); let result = expose.validate(); assert!( - matches!(result, Err(ConfigError::MismatchedPrefixSizes(_, _))), + matches!( + result, + Err(ConfigError::MismatchedPrefixLengths { + private: 24, + public: 25 + }) + ), "{result:?}", ); } diff --git a/config/src/external/overlay/vpcpeering.rs b/config/src/external/overlay/vpcpeering.rs index 72fa55c075..dbbc8fbcd9 100644 --- a/config/src/external/overlay/vpcpeering.rs +++ b/config/src/external/overlay/vpcpeering.rs @@ -432,8 +432,6 @@ impl VpcExpose { // - we have no exclusion prefixes (note: we could relax this constraint now that we // collapse exclusion prefixes early) // - we have a single prefix on each side (private and public addresses) - // - we have the same number of addresses on each side - // - the list of associated port ranges also has the same size on each side if collapsed_expose.has_port_forwarding() { if !self.nots.is_empty() || !self.not_as_or_empty().is_empty() { return Err(ConfigError::Forbidden( @@ -446,12 +444,6 @@ impl VpcExpose { "Port forwarding requires a single prefix on each side", )); } - if ips_sizes != as_range_sizes { - return Err(ConfigError::MismatchedPrefixSizes( - ips_sizes, - as_range_sizes, - )); - } // For port forwarding, ensure that a port range is always present. Lack of port range would imply // all ports, which is not allowed since port 0 is forbidden in the implementation for prefixes in [collapsed_expose.ips(), collapsed_expose.as_range_or_empty()] { @@ -461,6 +453,29 @@ impl VpcExpose { )); } } + + let internal = collapsed_expose + .ips() + .first() + .unwrap_or_else(|| unreachable!()); + let external = collapsed_expose + .as_range_or_empty() + .first() + .unwrap_or_else(|| unreachable!()); + if internal.prefix().length() != external.prefix().length() { + return Err(ConfigError::MismatchedPrefixLengths { + private: internal.prefix().length(), + public: external.prefix().length(), + }); + } + let internal_ports = internal.ports().unwrap_or_else(|| unreachable!()); + let external_ports = external.ports().unwrap_or_else(|| unreachable!()); + if internal_ports.len() != external_ports.len() { + return Err(ConfigError::MismatchedPortRangeSizes { + private: internal_ports.len(), + public: external_ports.len(), + }); + } } // For masquerade, we don't support port ranges @@ -1287,6 +1302,59 @@ pub mod contract { }); } + fn forwarding(internal: (&str, u16, u16), external: (&str, u16, u16)) -> VpcExpose { + let side = |(prefix, first, last): (&str, u16, u16)| { + PrefixWithOptionalPorts::new( + prefix.into(), + Some(PortRange::new(first, last).unwrap_or_else(|_| unreachable!())), + ) + }; + VpcExpose::empty() + .make_port_forwarding(None, None) + .unwrap_or_else(|_| unreachable!()) + .ip(side(internal)) + .as_range(side(external)) + .unwrap_or_else(|_| unreachable!()) + } + + #[test] + fn compensating_sizes_do_not_make_a_valid_expose() { + let expose = forwarding(("10.0.0.0/32", 1000, 1099), ("172.16.0.0/30", 2000, 2024)); + assert!( + matches!( + expose.validate(), + Err(ConfigError::MismatchedPrefixLengths { + private: 32, + public: 30 + }) + ), + "a /32 with 100 ports opposite a /30 with 25 was accepted: {:?}", + expose.validate() + ); + } + + #[test] + fn port_ranges_of_different_sizes_do_not_make_a_valid_expose() { + let expose = forwarding(("10.0.0.0/32", 1000, 1099), ("172.16.0.0/32", 2000, 2049)); + assert!( + matches!( + expose.validate(), + Err(ConfigError::MismatchedPortRangeSizes { + private: 100, + public: 50 + }) + ), + "100 ports opposite 50 was accepted: {:?}", + expose.validate() + ); + } + + #[test] + fn matched_sides_still_make_a_valid_expose() { + let expose = forwarding(("10.0.0.0/30", 1000, 1099), ("172.16.0.0/30", 2000, 2099)); + assert!(expose.validate().is_ok(), "{:?}", expose.validate()); + } + #[test] fn every_generated_masquerade_expose_validates() { bolero::check!() From bc2696706418261dd6e2fa8ada03da0956904c7c Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 16:09:18 -0600 Subject: [PATCH 06/23] test(mgmt): Property-test the configuration chain, and unblock its generators MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A gateway configuration passes through four steps before the dataplane sees it: GatewayAgent (CRD) ─▶ ExternalConfig ─▶ validated ─▶ InternalConfig ─▶ FRR The converters, the validator and the renderers are all reasonably covered. The third arrow is not: `build_internal_config` was exercised by one hand-built sample in `check_frr_config`, a test that renders the result and prints it. So the step that turns a *validated* configuration into the one the dataplane applies had never seen a generated input -- and that is where a configuration which validates and cannot be built would live. It matters because `apply_gw_config` is a linear `?`-chain with no transaction. By the time a late step fails, kernel interfaces, the flow filter, ACLs, static NAT and the masquerade allocator have all been committed, and rolling the configuration back does not restore the masquerade flows already torn down. Three properties, on generated `LegalValue`: whatever validates builds and renders; the built configuration carries a vrf for exactly the vnis the overlay's vpcs have; and the whole chain is deterministic, which matters because `frr-reload.py` diffs the rendered text against what FRR is running. Every property is of the form "if it validates, then ...", so a fourth test measures how often that is rather than assuming. About a sixth of generated configurations validate, carrying three vpcs each -- and none of them has a peering. Twenty-four thousand peerings generated per four thousand configurations, and not one survived validation. Peerings are where the exposes, the NAT and the ACLs live, so the whole of that half of the model was being discarded before anything downstream could see it, while `k8s-intf`'s generators sat at 94% coverage and every per-converter property passed -- because those test the converters, which run before validation. Three causes fixed here, all in the generators: - **peering pairs were drawn independently.** `spec.rs` drew up to sixteen peerings and `pick2` chose a fresh vpc pair for each with no memory, so a duplicated pair was near-certain and one duplicate fails the whole configuration. Pair selection moves to the caller, which draws distinct ones. - **each expose drew a mix of address families.** It split every count into a v4 part and a v6 part, and a `VpcExpose` must be single-family. The family is now chosen once per expose, and named vpc subnets of the other family are left out too, since a named subnet contributes its own prefix. - **prefixes were drawn as short as `/0`.** A v4 `/0` covers loopback and a `/2` at 64 covers `127.0.0.0/8`, so a short prefix always overlaps a special-use range that an expose may not. Minimum masks are now `/8` and `/16`; longer prefixes can still land in a reserved range, they just are no longer guaranteed to. Also `min` rather than `max` when choosing how many vpc subnets an expose names: with `max` the count was always at least the number that exist and the loop stopped when they ran out, so every expose named all of them and the count never varied. The remaining failures share one root cause: the expose is built first and its NAT mode chosen afterwards, so the shape and the mode do not agree. Static NAT gets mismatched address-port counts, port forwarding gets the exclusion prefixes it forbids, and masquerade gets an empty `as` list. Fixing it means choosing the mode first and shaping the expose to fit, which is what `config`'s own `contract` module does for the same three modes. The vacuity test asserts a twentieth for now, and is written to be strengthened to require peerings once that lands. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- k8s-intf/src/bolero/expose.rs | 21 ++-- k8s-intf/src/bolero/peering.rs | 30 +++++- k8s-intf/src/bolero/spec.rs | 10 +- k8s-intf/src/bolero/support.rs | 15 ++- mgmt/Cargo.toml | 1 + mgmt/src/processor/confbuild/internal.rs | 120 +++++++++++++++++++++++ 6 files changed, 175 insertions(+), 22 deletions(-) diff --git a/k8s-intf/src/bolero/expose.rs b/k8s-intf/src/bolero/expose.rs index 8750ed9c29..be9e7d8312 100644 --- a/k8s-intf/src/bolero/expose.rs +++ b/k8s-intf/src/bolero/expose.rs @@ -39,7 +39,7 @@ impl ValueGenerator for LegalValueExposeGenerator<'_> { fn generate(&self, d: &mut D) -> Option { let num_ips = d.gen_u16(Bound::Included(&1), Bound::Included(&16))?; let num_nots = d.gen_u16(Bound::Included(&0), Bound::Included(&16))?; - let num_subnets = std::cmp::max( + let num_subnets = std::cmp::min( self.subnets.len(), d.gen_usize(Bound::Included(&0), Bound::Included(&16))?, ); @@ -47,15 +47,11 @@ impl ValueGenerator for LegalValueExposeGenerator<'_> { let num_as = d.gen_u16(Bound::Included(&0), Bound::Included(&16))?; let num_as_not = d.gen_u16(Bound::Included(&0), Bound::Included(&16))?; - let num_v4_ips = d.gen_u16(Bound::Included(&0), Bound::Included(&num_ips))?; - let num_v6_ips = num_ips - num_v4_ips; - let num_v4_nots = d.gen_u16(Bound::Included(&0), Bound::Included(&num_nots))?; - let num_v6_nots = num_nots - num_v4_nots; - - let num_v4_as = d.gen_u16(Bound::Included(&0), Bound::Included(&num_as))?; - let num_v6_as = num_as - num_v4_as; - let num_v4_not_as = d.gen_u16(Bound::Included(&0), Bound::Included(&num_as_not))?; - let num_v6_not_as = num_as_not - num_v4_not_as; + let v4 = d.produce::()?; + let (num_v4_ips, num_v6_ips) = if v4 { (num_ips, 0) } else { (0, num_ips) }; + let (num_v4_nots, num_v6_nots) = if v4 { (num_nots, 0) } else { (0, num_nots) }; + let (num_v4_as, num_v6_as) = if v4 { (num_as, 0) } else { (0, num_as) }; + let (num_v4_not_as, num_v6_not_as) = if v4 { (num_as_not, 0) } else { (0, num_as_not) }; let ips = generate_prefixes(d, num_v4_ips, num_v6_ips)? .into_iter() @@ -87,7 +83,10 @@ impl ValueGenerator for LegalValueExposeGenerator<'_> { }); let mut subnets = Vec::new(); - let mut subnet_iter = self.subnets.iter(); + let mut subnet_iter = self + .subnets + .iter() + .filter(|(_, prefix)| prefix.is_ipv4() == v4); for _ in 0..num_subnets { let Some((name, _)) = subnet_iter.next() else { break; diff --git a/k8s-intf/src/bolero/peering.rs b/k8s-intf/src/bolero/peering.rs index cf7ad87af1..fdd71f3360 100644 --- a/k8s-intf/src/bolero/peering.rs +++ b/k8s-intf/src/bolero/peering.rs @@ -78,11 +78,24 @@ fn pick2<'a, D: Driver, T>(d: &mut D, items: &[&'a T]) -> Option<[&'a T; 2]> { Some([items[index1], items[index2]]) } -impl ValueGenerator for LegalValuePeeringsGenerator<'_> { - type Output = GatewayAgentPeerings; +impl LegalValuePeeringsGenerator<'_> { + #[must_use] + pub fn pairs(&self) -> Vec<[&String; 2]> { + let names = &self.vpc_names; + let mut out = Vec::with_capacity(names.len() * names.len() / 2); + for (i, first) in names.iter().enumerate() { + for second in names.iter().skip(i + 1) { + out.push([*first, *second]); + } + } + out + } - fn generate(&self, d: &mut D) -> Option { - let vpc_names = pick2(d, &self.vpc_names)?; + pub fn generate_for( + &self, + d: &mut D, + vpc_names: [&String; 2], + ) -> Option { let empty_map = SubnetMap::new(); let peerings_gens = vpc_names.map(|n| { LegalValuePeeringsPeeringGenerator::new(self.vpc_subnets.get(n).unwrap_or(&empty_map)) @@ -98,3 +111,12 @@ impl ValueGenerator for LegalValuePeeringsGenerator<'_> { }) } } + +impl ValueGenerator for LegalValuePeeringsGenerator<'_> { + type Output = GatewayAgentPeerings; + + fn generate(&self, d: &mut D) -> Option { + let vpc_names = pick2(d, &self.vpc_names)?; + self.generate_for(d, vpc_names) + } +} diff --git a/k8s-intf/src/bolero/spec.rs b/k8s-intf/src/bolero/spec.rs index e89abbda46..dbd5b1aeb4 100644 --- a/k8s-intf/src/bolero/spec.rs +++ b/k8s-intf/src/bolero/spec.rs @@ -4,7 +4,7 @@ use std::collections::{BTreeMap, HashSet}; use std::ops::Bound; -use bolero::{Driver, TypeGenerator, ValueGenerator}; +use bolero::{Driver, TypeGenerator}; use lpm::prefix::Prefix; @@ -81,8 +81,12 @@ impl TypeGenerator for LegalValue { let mut peerings = BTreeMap::new(); if num_peerings > 0 { let peering_gen = LegalValuePeeringsGenerator::new(&vpc_subnet_map).unwrap(); - for i in 0..num_peerings { - peerings.insert(format!("peering{i}"), peering_gen.generate(d)?); + let mut available = peering_gen.pairs(); + let wanted = num_peerings.min(available.len()); + for i in 0..wanted { + let choice = d.gen_usize(Bound::Included(&0), Bound::Excluded(&available.len()))?; + let pair = available.swap_remove(choice); + peerings.insert(format!("peering{i}"), peering_gen.generate_for(d, pair)?); } } diff --git a/k8s-intf/src/bolero/support.rs b/k8s-intf/src/bolero/support.rs index b1880a381d..bf1ba01f61 100644 --- a/k8s-intf/src/bolero/support.rs +++ b/k8s-intf/src/bolero/support.rs @@ -254,15 +254,22 @@ pub fn choose(d: &mut D, choices: &[T]) -> Option { Some(choices[index].clone()) } +const MIN_V4_MASK: u8 = 8; +const MIN_V6_MASK: u8 = 16; + pub fn generate_v4_prefixes(d: &mut D, count: u16) -> Option> { - let cidr4_gen = - UniqueV4CidrGenerator::new(count, d.gen_u8(Bound::Included(&0), Bound::Included(&32))?); + let cidr4_gen = UniqueV4CidrGenerator::new( + count, + d.gen_u8(Bound::Included(&MIN_V4_MASK), Bound::Included(&32))?, + ); cidr4_gen.generate(d) } pub fn generate_v6_prefixes(d: &mut D, count: u16) -> Option> { - let cidr6_gen = - UniqueV6CidrGenerator::new(count, d.gen_u8(Bound::Included(&0), Bound::Included(&128))?); + let cidr6_gen = UniqueV6CidrGenerator::new( + count, + d.gen_u8(Bound::Included(&MIN_V6_MASK), Bound::Included(&128))?, + ); cidr6_gen.generate(d) } diff --git a/mgmt/Cargo.toml b/mgmt/Cargo.toml index 482e1a1396..a5988c10db 100644 --- a/mgmt/Cargo.toml +++ b/mgmt/Cargo.toml @@ -60,6 +60,7 @@ tracing-test = { workspace = true } dpdk = { workspace = true, features = ["test"] } # EAL for tests that build the rte_acl-backed ACL filter and flow-filter context fixin = { workspace = true } id = { workspace = true, features = ["bolero"] } +k8s-intf = { workspace = true, features = ["bolero"] } interface-manager = { workspace = true, features = ["bolero"] } lpm = { workspace = true, features = ["testing"] } n-vm = { workspace = true } diff --git a/mgmt/src/processor/confbuild/internal.rs b/mgmt/src/processor/confbuild/internal.rs index fc108e4871..eecac90947 100644 --- a/mgmt/src/processor/confbuild/internal.rs +++ b/mgmt/src/processor/confbuild/internal.rs @@ -392,3 +392,123 @@ pub fn build_internal_config( debug!("Successfully built internal config for genid {genid}"); Ok(internal) } + +#[cfg(test)] +mod chain_properties { + use super::*; + use config::{ExternalConfig, GenId}; + use k8s_intf::bolero::LegalValue; + use k8s_intf::gateway_agent_crd::GatewayAgent; + use routing::Render; + use std::collections::BTreeSet; + + fn chain(agent: &GatewayAgent) -> Option<(GenId, InternalConfig)> { + let external = ExternalConfig::try_from(agent) + .unwrap_or_else(|e| panic!("a schema-legal CRD did not convert: {e}")); + let validated = external.validate().ok()?; + let genid = validated.genid(); + let internal = build_internal_config(&validated, None).unwrap_or_else(|e| { + panic!("a validated configuration would not build: {e}\n{validated:#?}") + }); + Some((genid, internal)) + } + + #[test] + fn whatever_validates_builds_and_renders() { + bolero::check!() + .with_type::>() + .for_each(|agent| { + let Some((genid, internal)) = chain(agent.as_ref()) else { + return; + }; + let text = internal.render(&genid).to_string(); + assert!( + text.contains(&format!("! config for gen {genid}")), + "the rendered config does not say which generation it is for" + ); + }); + } + + #[test] + fn every_vpc_gets_a_vrf_and_no_more() { + bolero::check!() + .with_type::>() + .for_each(|agent| { + let external = ExternalConfig::try_from(agent.as_ref()) + .unwrap_or_else(|e| panic!("a schema-legal CRD did not convert: {e}")); + let Ok(validated) = external.validate() else { + return; + }; + let internal = build_internal_config(&validated, None) + .unwrap_or_else(|e| panic!("a validated configuration would not build: {e}")); + + let wanted: BTreeSet = validated + .external() + .overlay() + .vpc_table() + .values() + .map(|vpc| vpc.vni().as_u32()) + .collect(); + let built: BTreeSet = internal + .vrfs + .iter_by_name() + .filter_map(|vrf| vrf.vni.map(|vni| vni.as_u32())) + .collect(); + assert_eq!(built, wanted, "vrfs do not match the vpcs they come from"); + }); + } + + #[test] + fn the_properties_are_not_vacuous() { + use concurrency::sync::atomic::{AtomicUsize, Ordering}; + static SEEN: AtomicUsize = AtomicUsize::new(0); + static VALIDATED: AtomicUsize = AtomicUsize::new(0); + static VPCS: AtomicUsize = AtomicUsize::new(0); + + bolero::check!() + .with_type::>() + .for_each(|agent| { + SEEN.fetch_add(1, Ordering::Relaxed); + let external = ExternalConfig::try_from(agent.as_ref()) + .unwrap_or_else(|e| panic!("a schema-legal CRD did not convert: {e}")); + if let Ok(validated) = external.validate() { + VALIDATED.fetch_add(1, Ordering::Relaxed); + VPCS.fetch_add( + validated.external().overlay().vpc_table().len(), + Ordering::Relaxed, + ); + } + }); + + let seen = SEEN.load(Ordering::Relaxed); + let validated = VALIDATED.load(Ordering::Relaxed); + let vpcs = VPCS.load(Ordering::Relaxed); + println!("{validated}/{seen} configurations validated, carrying {vpcs} vpcs"); + assert!(seen > 0, "no configurations were generated"); + assert!( + validated * 20 >= seen, + "only {validated} of {seen} configurations validated: the properties above are \ + checking almost nothing" + ); + assert!(vpcs > validated, "validated configurations carry no vpcs"); + } + + #[test] + fn the_chain_is_deterministic() { + bolero::check!() + .with_type::>() + .for_each(|agent| { + let Some((genid, once)) = chain(agent.as_ref()) else { + return; + }; + let (_, twice) = chain(agent.as_ref()).unwrap_or_else(|| { + panic!("the same CRD validated once and not the second time") + }); + assert_eq!( + once.render(&genid).to_string(), + twice.render(&genid).to_string(), + "the configuration chain is not deterministic" + ); + }); + } +} From 91c46a24ee9f4fdc92b1be1709a59da24b828371 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 16:45:06 -0600 Subject: [PATCH 07/23] test(mgmt): Drive the config builder with generated NAT peerings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The third arrow of the configuration chain -- GatewayAgent (CRD) ─▶ ExternalConfig ─▶ validated ─▶ InternalConfig ─▶ FRR -- had never been given a peering. The CRD generators produce exposes that validation always refuses, so the half of the model where the exposes, the NAT and the ACLs live reached the builder never. Fixing the CRD generators is its own piece of work. This gets at the same question from the other side, and now rather than after it: `config`'s contract generators already produce exposes that are valid *by construction* for each of the three NAT flavours, so an overlay built around them and spliced into the sample underlay reaches `build_internal_config` with a peering in it. The claim is that a configuration which validates can be built and rendered. One that validates and then fails to build is a half-applied dataplane -- `apply_gw_config` is a linear `?`-chain with no transaction, so by the time a late step fails the kernel interfaces, the flow filter, the ACLs, static NAT and the masquerade allocator are all committed, and rolling the configuration back does not restore the masquerade flows already torn down. The port-forwarding expose that validated and could not be built is precedent for the class. It holds here across mixed NAT flavours; nothing found. Two supporting changes: - `contract::overlay_with` and `overlay_with_exposes` split out of `overlay_offering`, which validated the overlay and returned it validated. A caller assembling a whole `ExternalConfig` needs the unvalidated one, because validating the overlay alone skips every check that spans the underlay and the overlay together. One of those matters immediately: `VpcPeering::with_default_group` names a gateway group `default`, and whole-config validation checks that a peering's group exists -- a check overlay-only validation cannot make, since the group table sits beside the overlay rather than in it. So an overlay from these generators is not embeddable in a whole configuration without adding that group. - the contract module was gated `any(test, feature = "bolero")` but only ever compiled under `test`: it used `Prefix: From<&str>`, which the feature alone does not provide. Now it builds either way, which is what lets `mgmt` depend on it. The vni checks alone cannot see a build that skipped the overlay, the underlay vrf, the underlay's bgp peers or the community table, so the property asserts each of those directly rather than inferring them. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- config/src/external/overlay/vpcpeering.rs | 27 ++- mgmt/Cargo.toml | 1 + mgmt/src/processor/confbuild/internal.rs | 2 +- mgmt/src/tests/mgmt.rs | 198 +++++++++++++++++++++- 4 files changed, 218 insertions(+), 10 deletions(-) diff --git a/config/src/external/overlay/vpcpeering.rs b/config/src/external/overlay/vpcpeering.rs index dbbc8fbcd9..c64919a5c7 100644 --- a/config/src/external/overlay/vpcpeering.rs +++ b/config/src/external/overlay/vpcpeering.rs @@ -1220,7 +1220,18 @@ pub mod contract { pub const REMOTE_VNI: u32 = 200; pub fn overlay_offering(expose: VpcExpose) -> Result { - let remote_prefix = match expose.ips.first().map(PrefixWithOptionalPorts::prefix) { + overlay_with(expose)?.validate() + } + + pub fn overlay_with(expose: VpcExpose) -> Result { + overlay_with_exposes(vec![expose]) + } + + pub fn overlay_with_exposes(exposes: Vec) -> Result { + let remote_prefix = match exposes + .first() + .and_then(|expose| expose.ips.first().map(PrefixWithOptionalPorts::prefix)) + { Some(Prefix::IPV6(_)) => "2001:db8:ffff::/64", _ => "3.3.3.0/24", }; @@ -1229,9 +1240,15 @@ pub mod contract { vpc_table.add(Vpc::new("VPC-1", "AAAAA", LOCAL_VNI)?)?; vpc_table.add(Vpc::new("VPC-2", "BBBBB", REMOTE_VNI)?)?; - let local = VpcManifest::new("VPC-1").exposing(expose); - let remote = - VpcManifest::new("VPC-2").exposing(VpcExpose::empty().ip(remote_prefix.into())); + let local = exposes + .into_iter() + .fold(VpcManifest::new("VPC-1"), VpcManifest::exposing); + let remote = VpcManifest::new("VPC-2").exposing( + VpcExpose::empty().ip(remote_prefix + .parse::() + .unwrap_or_else(|_| unreachable!()) + .into()), + ); let mut peerings = VpcPeeringTable::new(); peerings.add(VpcPeering::with_default_group( "VPC-1--VPC-2", @@ -1239,7 +1256,7 @@ pub mod contract { remote, ))?; - Overlay::new(vpc_table, peerings).validate() + Ok(Overlay::new(vpc_table, peerings)) } #[must_use] diff --git a/mgmt/Cargo.toml b/mgmt/Cargo.toml index a5988c10db..da679f40b4 100644 --- a/mgmt/Cargo.toml +++ b/mgmt/Cargo.toml @@ -60,6 +60,7 @@ tracing-test = { workspace = true } dpdk = { workspace = true, features = ["test"] } # EAL for tests that build the rte_acl-backed ACL filter and flow-filter context fixin = { workspace = true } id = { workspace = true, features = ["bolero"] } +config = { workspace = true, features = ["bolero"] } k8s-intf = { workspace = true, features = ["bolero"] } interface-manager = { workspace = true, features = ["bolero"] } lpm = { workspace = true, features = ["testing"] } diff --git a/mgmt/src/processor/confbuild/internal.rs b/mgmt/src/processor/confbuild/internal.rs index eecac90947..78a15ac433 100644 --- a/mgmt/src/processor/confbuild/internal.rs +++ b/mgmt/src/processor/confbuild/internal.rs @@ -311,7 +311,7 @@ fn build_internal_overlay_config( Ok(()) } -const EVPN_RMAP_NO_ADV_COMM: &str = "EVPN-ROUTE-MAP-NO-ADV-COMM"; +pub(crate) const EVPN_RMAP_NO_ADV_COMM: &str = "EVPN-ROUTE-MAP-NO-ADV-COMM"; /// Create a route-map that adds community "no-advertise" to all routes fn route_map_add_noadv_comm() -> RouteMap { diff --git a/mgmt/src/tests/mgmt.rs b/mgmt/src/tests/mgmt.rs index 3aa26792a1..f72592b43a 100644 --- a/mgmt/src/tests/mgmt.rs +++ b/mgmt/src/tests/mgmt.rs @@ -171,7 +171,7 @@ pub mod test { } /* DEVICE configuration */ - fn sample_device_config() -> DeviceConfig { + pub(super) fn sample_device_config() -> DeviceConfig { DeviceConfig::new() } @@ -319,7 +319,7 @@ pub mod test { } /* build sample underlay config */ - fn sample_underlay_config() -> Underlay { + pub(super) fn sample_underlay_config() -> Underlay { /* main loopback for BGP and vtep */ let loopback = IpAddr::from_str("7.0.0.100").expect("Bad address"); let router_id = get_v4_addr(loopback); @@ -333,7 +333,7 @@ pub mod test { } #[rustfmt::skip] - fn sample_gw_groups() -> GwGroupTable { + pub(super) fn sample_gw_groups() -> GwGroupTable { let mut gwt = GwGroupTable::new(); let mut group = GwGroup::new("gw-group-1"); group.add_member(GwGroupMember::new("gw1", 1, IpAddr::from_str("172.128.0.1").unwrap())).unwrap(); @@ -348,7 +348,7 @@ pub mod test { gwt } - fn sample_community_table() -> PriorityCommunityTable { + pub(super) fn sample_community_table() -> PriorityCommunityTable { let mut comtable = PriorityCommunityTable::new(); comtable.insert(0, "65000:800").unwrap(); comtable.insert(1, "65000:801").unwrap(); @@ -508,3 +508,193 @@ pub mod test { router.stop(); } } + +#[cfg(test)] +mod peering_chain { + use bolero::{Driver, ValueGenerator}; + use config::ExternalConfig; + use config::external::ExternalConfigBuilder; + use config::external::gwgroup::{GwGroup, GwGroupMember, GwGroupTable}; + use config::external::overlay::vpcpeering::VpcExpose; + use config::external::overlay::vpcpeering::contract::{ + LOCAL_VNI, MasqueradeExpose, PortForwardingExpose, REMOTE_VNI, StaticNatExpose, + overlay_with_exposes, + }; + use routing::Render; + use std::net::IpAddr; + use std::ops::Bound::Included; + use std::str::FromStr; + + use super::test::{ + sample_community_table, sample_device_config, sample_gw_groups, sample_underlay_config, + }; + use crate::processor::confbuild::internal::{EVPN_RMAP_NO_ADV_COMM, build_internal_config}; + + #[derive(Debug, Clone, Copy, PartialEq, Eq)] + enum Flavour { + PortForwarding, + Masquerade, + Static, + } + + const MAX_EXPOSES: u8 = 3; + + const UNDERLAY_ASN: u32 = 65000; + + #[derive(Debug, Clone, Copy, Default)] + struct AnyNatExposes; + + impl ValueGenerator for AnyNatExposes { + type Output = Vec<(Flavour, VpcExpose)>; + + fn generate(&self, driver: &mut D) -> Option { + let count = driver.gen_u8(Included(&1), Included(&MAX_EXPOSES))?; + let mut out = Vec::with_capacity(usize::from(count)); + for _ in 0..count { + out.push(match driver.gen_u8(Included(&0), Included(&2))? { + 0 => ( + Flavour::PortForwarding, + PortForwardingExpose.generate(driver)?, + ), + 1 => (Flavour::Masquerade, MasqueradeExpose.generate(driver)?), + _ => (Flavour::Static, StaticNatExpose.generate(driver)?), + }); + } + Some(out) + } + } + + fn gw_groups_with_default() -> GwGroupTable { + let mut groups = sample_gw_groups(); + let mut default = GwGroup::new("default"); + default + .add_member(GwGroupMember::new( + "gw-default", + 1, + IpAddr::from_str("172.128.0.9").unwrap_or_else(|_| unreachable!()), + )) + .unwrap_or_else(|e| unreachable!("{e}")); + groups + .add_group(default) + .unwrap_or_else(|e| unreachable!("{e}")); + groups + } + + fn external_offering(exposes: Vec) -> ExternalConfig { + let overlay = overlay_with_exposes(exposes).unwrap_or_else(|e| unreachable!("{e}")); + ExternalConfigBuilder::default() + .gwname("test-gw".to_string()) + .genid(1) + .device(sample_device_config()) + .underlay(sample_underlay_config()) + .overlay(overlay) + .gwgroups(gw_groups_with_default()) + .communities(sample_community_table()) + .build() + .unwrap_or_else(|e| unreachable!("{e}")) + } + + #[test] + fn a_config_with_a_nat_peering_builds_and_renders() { + use concurrency::sync::atomic::{AtomicUsize, Ordering}; + static SEEN: AtomicUsize = AtomicUsize::new(0); + static BUILT: AtomicUsize = AtomicUsize::new(0); + static MULTI: AtomicUsize = AtomicUsize::new(0); + + bolero::check!() + .with_generator(AnyNatExposes) + .cloned() + .for_each(|offered: Vec<(Flavour, VpcExpose)>| { + SEEN.fetch_add(1, Ordering::Relaxed); + let flavours: Vec = offered.iter().map(|(f, _)| *f).collect(); + let exposes: Vec = offered.iter().map(|(_, e)| e.clone()).collect(); + let external = external_offering(exposes.clone()); + + let validated = match external.validate() { + Ok(validated) => validated, + Err(e) => { + assert!( + exposes.len() > 1, + "a single {:?} expose that is valid by construction was refused: {e}\n{exposes:#?}", + flavours[0] + ); + return; + } + }; + BUILT.fetch_add(1, Ordering::Relaxed); + if exposes.len() > 1 { + MULTI.fetch_add(1, Ordering::Relaxed); + } + + let internal = build_internal_config(&validated, None).unwrap_or_else(|e| { + panic!("a validated {flavours:?} configuration would not build: {e}\n{exposes:#?}") + }); + + let vnis: Vec = internal + .vrfs + .iter_by_name() + .filter_map(|vrf| vrf.vni.map(|vni| vni.as_u32())) + .collect(); + for vni in [LOCAL_VNI, REMOTE_VNI] { + assert!( + vnis.contains(&vni), + "vni {vni} of the peering is missing from the built config, got {vnis:?}" + ); + } + + assert!( + internal.vrfs.default_vrf_config().is_some(), + "the built config has no default vrf" + ); + + for order in 0..5 { + assert_eq!( + internal.commtable.get_community(order), + validated.external().communities().get_community(order), + "community {order} did not survive the build" + ); + } + + assert!( + internal + .rmap_table + .values() + .any(|rmap| rmap.name == EVPN_RMAP_NO_ADV_COMM), + "the evpn no-advertise route-map is missing from the built config" + ); + + let text = internal.render(&validated.genid()).to_string(); + assert!( + text.contains("! config for gen 1"), + "the rendered config does not say which generation it is for" + ); + assert!( + text.contains(EVPN_RMAP_NO_ADV_COMM), + "the evpn no-advertise route-map is missing from the rendered config" + ); + assert!( + text.contains(&format!("router bgp {UNDERLAY_ASN}")), + "the underlay bgp instance is missing from the rendered config" + ); + for vni in [LOCAL_VNI, REMOTE_VNI] { + assert!( + text.contains(&format!(" vni {vni}")), + "vni {vni} is missing from the rendered config" + ); + } + }); + + let seen = SEEN.load(Ordering::Relaxed); + let built = BUILT.load(Ordering::Relaxed); + let multi = MULTI.load(Ordering::Relaxed); + println!("{built}/{seen} configurations built, {multi} of them with several exposes"); + assert!( + built * 2 >= seen, + "most configurations were skipped: {built}/{seen}" + ); + assert!( + multi > 0, + "no configuration with more than one expose was built" + ); + } +} From f33fc25601a24a90f4272dfc9261f167c6a2749f Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 17:09:29 -0600 Subject: [PATCH 08/23] test(k8s-intf): Generate configurations that are valid by construction The CRD generators produced peerings in quantity and none survived validation. Three causes were fixed alongside the measurement that found it; this finds the rest, and turns the entry point into something a property can aim with. The principle is already written down in this repo, in `config`'s own contract module: Valid by construction rather than by generate-and-reject, so every case reaches the code under test. The CRD expose generator did the opposite: it drew the prefixes first and chose a NAT flavour afterwards. Since every flavour constrains the shape -- static NAT needs both sides to cover the same number of address-port pairs, port forwarding needs one prefix per side of equal length with matched port ranges and no exclusions at all, masquerade needs a non-empty translation range -- essentially nothing it produced could be accepted, and no amount of context passed down would have helped. The order was wrong. Four further causes, all cross-cutting rules that no per-expose generator can satisfy: - **the two manifests of a peering must agree on address family.** Each drew its own. - **only one manifest of a peering may use a stateful flavour.** Masquerade opposite masquerade, masquerade opposite port forwarding, and port forwarding opposite port forwarding are all refused. Both sides drew freely. The peering generator now draws which side may be stateful and restricts the other to the stateless flavours. - **a peering names a gateway group, and validation checks it exists.** The name was `d.produce::()`, so it never did. Groups are now generated before peerings, and a peering picks one of them. - **a vpc's subnets are subject to the same rules as an expose's prefixes,** because an expose can name a subnet and a named subnet contributes its prefix. They were drawn across the whole address space, so `127.0.0.0/8` and `224.0.0.0/4` subnets made every expose naming them invalid. They now come from the private block, carved consecutively so they are distinct and non-overlapping without a rejection loop. Prefixes throughout now come from blocks this validator does not treat as special-use -- `10.0.0.0/8` and `172.16.0.0/12` for v4, halves of `2001:db8::/32` for v6 -- with the private and public sides in different blocks so an expose's two sides can never be the same prefix. The same choice, for the same reason, as the contract module. 94% of generated configurations now validate, carrying peerings, against 17% carrying none. `LegalValue` implements `TypeGenerator`, which per `development/code/property-testing.md` must "**never** produce an illegal value". It did so on more than four draws in five, so the name asserted a property it did not have. The real generator is now `GatewayAgents`, a `ValueGenerator` produced by `GatewayAgentBuilder`, with knobs for the NAT flavours, the address families and the sizes. `LegalValue`'s `TypeGenerator` impls delegate to the defaults, so every existing user keeps working, and a property that wants to aim at one flavour or one family can now say so. The defaults are much smaller: four vpcs, three peerings, two exposes each, three prefixes a side. It was sixteen of everything nested four deep, which made a single case thousands of prefixes -- costly to run and unreadable when it failed. Three smaller things: - `start + size - 1` overflowed `u16` for a port range ending at 65535, since it groups as `(start + size) - 1`. Debug-mode overflow checks caught it. - `test_vpc_conversion`'s oracle had to learn that the conversion collects into a set-like structure, so a prefix written twice in one expose comes out once. Its expectation was only ever right because the previous generators drew from a uniqueness-preserving generator and never produced a repeat. - the vacuity test in `processor::confbuild::internal` now requires peerings, which is what it was written to be strengthened into. Residue, at about one in a thousand: two exposes in one peering drawing overlapping prefixes from the same block. Avoiding it needs coordination across exposes, and it is legitimate rejection rather than a defect. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- config/src/converters/k8s/config/expose.rs | 6 +- config/src/converters/k8s/config/peering.rs | 11 +- k8s-intf/src/bolero/crd.rs | 70 ++++- k8s-intf/src/bolero/expose.rs | 325 +++++++++++--------- k8s-intf/src/bolero/mod.rs | 57 +++- k8s-intf/src/bolero/peering.rs | 96 +++++- k8s-intf/src/bolero/spec.rs | 127 +++++++- k8s-intf/src/bolero/support.rs | 106 +++++++ k8s-intf/src/bolero/vpc.rs | 65 +++- mgmt/src/processor/confbuild/internal.rs | 22 +- 10 files changed, 684 insertions(+), 201 deletions(-) diff --git a/config/src/converters/k8s/config/expose.rs b/config/src/converters/k8s/config/expose.rs index 719ecf2e91..f6dd7708f3 100644 --- a/config/src/converters/k8s/config/expose.rs +++ b/config/src/converters/k8s/config/expose.rs @@ -491,7 +491,7 @@ mod test { "10.0.4.0/24".parse::().unwrap(), ), ]); - let expose_gen = k8s_intf::bolero::expose::LegalValueExposeGenerator::new(&subnets); + let expose_gen = k8s_intf::bolero::expose::AnyExposeGenerator::new(&subnets); bolero::check!() .with_generator(expose_gen) .for_each(|k8s_expose| { @@ -549,6 +549,7 @@ mod test { }) .unwrap_or(vec![]); k8s_nots.sort(); + k8s_nots.dedup(); let k8s_subnets = k8s_expose .ips .as_ref() @@ -566,6 +567,7 @@ mod test { .unwrap_or(vec![]); k8s_ips.extend(k8s_subnets); k8s_ips.sort(); + k8s_ips.dedup(); let k8s_as = k8s_expose.r#as.as_ref().map(|r#as| { let mut ret = r#as @@ -574,6 +576,7 @@ mod test { .map(|r#as| r#as.cidr.as_ref().unwrap().clone()) .collect::>(); ret.sort(); + ret.dedup(); ret }); @@ -584,6 +587,7 @@ mod test { .map(|r#as| r#as.not.as_ref().unwrap().clone()) .collect::>(); ret.sort(); + ret.dedup(); ret }); diff --git a/config/src/converters/k8s/config/peering.rs b/config/src/converters/k8s/config/peering.rs index 45ae4a8e29..956bf7a783 100644 --- a/config/src/converters/k8s/config/peering.rs +++ b/config/src/converters/k8s/config/peering.rs @@ -91,6 +91,7 @@ mod test { use k8s_intf::bolero::peering::{ LegalValuePeeringsGenerator, LegalValuePeeringsPeeringGenerator, }; + use k8s_intf::bolero::{AddressFamily, NatFlavour}; use lpm::prefix::Prefix; use crate::converters::k8s::config::{SubnetMap, VpcSubnetMap}; @@ -98,7 +99,9 @@ mod test { #[test] fn test_vpc_manifest_conversion() { let subnets = SubnetMap::new(); // Let this be empty since we are test subnet conversion elsewhere - let generator = LegalValuePeeringsPeeringGenerator::new(&subnets); + let flavours = NatFlavour::all(); + let generator = + LegalValuePeeringsPeeringGenerator::new(&subnets, &flavours, AddressFamily::V4, 3); bolero::check!() .with_generator(generator) .for_each(|peering| { @@ -169,7 +172,11 @@ mod test { ]), ), ]); - let generator = LegalValuePeeringsGenerator::new(&subnets).unwrap(); + let flavours = NatFlavour::all(); + let families = AddressFamily::all(); + let groups = vec!["gwgroup-0".to_string()]; + let generator = + LegalValuePeeringsGenerator::new(&subnets, &flavours, &families, 3, &groups).unwrap(); bolero::check!() .with_generator(generator) .for_each(|peering| { diff --git a/k8s-intf/src/bolero/crd.rs b/k8s-intf/src/bolero/crd.rs index 92fc069b5a..edba6f94b3 100644 --- a/k8s-intf/src/bolero/crd.rs +++ b/k8s-intf/src/bolero/crd.rs @@ -6,8 +6,9 @@ use std::ops::Bound; use bolero::{Driver, TypeGenerator, ValueGenerator, produce}; use kube::core::ObjectMeta; -use crate::bolero::LegalValue; -use crate::gateway_agent_crd::{GatewayAgent, GatewayAgentSpec}; +use crate::bolero::spec::{GatewayAgentSpecs, SpecBuilder}; +use crate::bolero::{AddressFamily, LegalValue, NatFlavour}; +use crate::gateway_agent_crd::GatewayAgent; const HOSTNAME_BASE: &str = "host-"; @@ -23,21 +24,70 @@ fn simple_hostname(d: &mut D) -> Option { ) } -/// 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 { - fn generate(d: &mut D) -> Option { - Some(LegalValue(GatewayAgent { +#[derive(Debug, Clone, Default)] +pub struct GatewayAgents(GatewayAgentSpecs); + +impl ValueGenerator for GatewayAgents { + type Output = GatewayAgent; + + fn generate(&self, d: &mut D) -> Option { + Some(GatewayAgent { metadata: ObjectMeta { name: Some(simple_hostname(d)?), generation: Some(d.gen_i64(Bound::Excluded(&0), Bound::Unbounded)?), namespace: Some("default".to_string()), ..Default::default() }, - spec: d.produce::>()?.take(), + spec: self.0.generate(d)?, status: None, // Add when we build a generator and converter for status - })) + }) + } +} + +#[derive(Debug, Clone, Default)] +pub struct GatewayAgentBuilder(SpecBuilder); + +impl GatewayAgentBuilder { + #[must_use] + pub fn new() -> Self { + Self::default() + } + + #[must_use] + pub fn flavours(mut self, flavours: Vec) -> Self { + self.0 = self.0.flavours(flavours); + self + } + + #[must_use] + pub fn families(mut self, families: Vec) -> Self { + self.0 = self.0.families(families); + self + } + + #[must_use] + pub fn sizes(mut self, vpcs: u8, peerings: u8, exposes: u8, subnets: u8) -> Self { + self.0 = self + .0 + .max_vpcs(vpcs) + .max_peerings(peerings) + .max_exposes(exposes) + .max_subnets(subnets); + self + } + + #[must_use] + pub fn build(self) -> GatewayAgents { + GatewayAgents(self.0.build()) + } +} + +/// 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 { + fn generate(d: &mut D) -> Option { + Some(LegalValue(GatewayAgents::default().generate(d)?)) } } diff --git a/k8s-intf/src/bolero/expose.rs b/k8s-intf/src/bolero/expose.rs index be9e7d8312..d2f7176676 100644 --- a/k8s-intf/src/bolero/expose.rs +++ b/k8s-intf/src/bolero/expose.rs @@ -3,10 +3,10 @@ use std::ops::Bound; -use bolero::{Driver, TypeGenerator, ValueGenerator}; +use bolero::{Driver, ValueGenerator}; -use crate::bolero::support::generate_prefixes; -use crate::bolero::{LegalValue, SubnetMap}; +use crate::bolero::support::blocks; +use crate::bolero::{AddressFamily, NatFlavour, SubnetMap}; use crate::gateway_agent_crd::{ GatewayAgentPeeringsPeeringExpose, GatewayAgentPeeringsPeeringExposeAs, GatewayAgentPeeringsPeeringExposeIps, GatewayAgentPeeringsPeeringExposeNat, @@ -17,174 +17,225 @@ use crate::gateway_agent_crd::{ GatewayAgentPeeringsPeeringExposeNatStatic, }; -/// Generate a legal value for `GatewayAgentPeeringsPeeringExpose` /// -/// This is not exhaustive over all legal values due to the complexity of doing this. For example, -/// the CIDR generators are not exhaustive; and we use a single port range for all CIDRs rather than -/// trying different combinations. -pub struct LegalValueExposeGenerator<'a> { +const MAX_PREFIXES: u8 = 3; + +const MAX_PORTS: u16 = 1024; + +#[derive(Debug, Clone)] +pub struct ExposeGenerator<'a> { + flavour: NatFlavour, + family: AddressFamily, subnets: &'a SubnetMap, } -impl<'a> LegalValueExposeGenerator<'a> { +impl<'a> ExposeGenerator<'a> { #[must_use] - pub fn new(subnets: &'a SubnetMap) -> Self { - Self { subnets } + pub fn new(flavour: NatFlavour, family: AddressFamily, subnets: &'a SubnetMap) -> Self { + Self { + flavour, + family, + subnets, + } } -} - -impl ValueGenerator for LegalValueExposeGenerator<'_> { - type Output = GatewayAgentPeeringsPeeringExpose; - fn generate(&self, d: &mut D) -> Option { - let num_ips = d.gen_u16(Bound::Included(&1), Bound::Included(&16))?; - let num_nots = d.gen_u16(Bound::Included(&0), Bound::Included(&16))?; - let num_subnets = std::cmp::min( - self.subnets.len(), - d.gen_usize(Bound::Included(&0), Bound::Included(&16))?, - ); - - let num_as = d.gen_u16(Bound::Included(&0), Bound::Included(&16))?; - let num_as_not = d.gen_u16(Bound::Included(&0), Bound::Included(&16))?; - - let v4 = d.produce::()?; - let (num_v4_ips, num_v6_ips) = if v4 { (num_ips, 0) } else { (0, num_ips) }; - let (num_v4_nots, num_v6_nots) = if v4 { (num_nots, 0) } else { (0, num_nots) }; - let (num_v4_as, num_v6_as) = if v4 { (num_as, 0) } else { (0, num_as) }; - let (num_v4_not_as, num_v6_not_as) = if v4 { (num_as_not, 0) } else { (0, num_as_not) }; - - let ips = generate_prefixes(d, num_v4_ips, num_v6_ips)? - .into_iter() - .map(|p| GatewayAgentPeeringsPeeringExposeIps { - cidr: Some(p), - not: None, - vpc_subnet: None, - }) - .collect::>(); - let nots = generate_prefixes(d, num_v4_nots, num_v6_nots)? - .into_iter() - .map(|p| GatewayAgentPeeringsPeeringExposeIps { - cidr: None, - not: Some(p), - vpc_subnet: None, - }) - .collect::>(); - let r#as = generate_prefixes(d, num_v4_as, num_v6_as)? - .into_iter() - .map(|p| GatewayAgentPeeringsPeeringExposeAs { - cidr: Some(p), - not: None, - }); - let not_as = generate_prefixes(d, num_v4_not_as, num_v6_not_as)? - .into_iter() - .map(|p| GatewayAgentPeeringsPeeringExposeAs { - cidr: None, - not: Some(p), - }); + fn length(&self, d: &mut D) -> Option { + d.gen_u8( + Bound::Included(&blocks::min_len(self.family)), + Bound::Included(&blocks::max_len(self.family)), + ) + } - let mut subnets = Vec::new(); - let mut subnet_iter = self - .subnets + fn matching_subnets(&self) -> Vec<&'a String> { + self.subnets .iter() - .filter(|(_, prefix)| prefix.is_ipv4() == v4); - for _ in 0..num_subnets { - let Some((name, _)) = subnet_iter.next() else { - break; - }; - subnets.push(GatewayAgentPeeringsPeeringExposeIps { - cidr: None, - not: None, - vpc_subnet: Some(name.clone()), - }); - } - - let mut final_ips = Vec::with_capacity(ips.len() + nots.len() + subnets.len()); - final_ips.extend(ips); - final_ips.extend(nots); - final_ips.extend(subnets); + .filter(|(_, prefix)| prefix.is_ipv4() == self.family.is_v4()) + .map(|(name, _)| name) + .collect() + } - let mut final_as = Vec::with_capacity(r#as.len() + not_as.len()); - final_as.extend(r#as); - final_as.extend(not_as); - let has_as = !final_as.is_empty(); + fn exclusion(&self, d: &mut D, parent: &str, private: bool) -> Option { + let (_, len) = parent.split_once('/')?; + let len: u8 = len.parse().ok()?; + let max = blocks::max_len(self.family); + if len >= max { + return None; + } + let longer = d.gen_u8(Bound::Excluded(&len), Bound::Included(&max))?; + if private { + blocks::private(d, self.family, longer) + } else { + blocks::public(d, self.family, longer) + } + } - Some(GatewayAgentPeeringsPeeringExpose { - r#as: Some(final_as).filter(|f| !f.is_empty()), - ips: Some(final_ips).filter(|f| !f.is_empty()), - default: None, - nat: if has_as { - Some( - d.produce::>()? - .take(), - ) - } else { - None - }, - }) + fn port_pair(d: &mut D) -> Option<(String, String)> { + let size = d.gen_u16(Bound::Included(&1), Bound::Included(&MAX_PORTS))?; + let first_start = d.gen_u16(Bound::Included(&1), Bound::Included(&(65535 - size + 1)))?; + let second_start = d.gen_u16(Bound::Included(&1), Bound::Included(&(65535 - size + 1)))?; + Some(( + format!("{first_start}-{}", first_start + (size - 1)), + format!("{second_start}-{}", second_start + (size - 1)), + )) } -} -// This is not exhaustive as it does not generate all possible time -// strings, just 0 to 2*3600 seconds. -// -impl TypeGenerator for LegalValue { - fn generate(d: &mut D) -> Option { - let nat_mode = d.produce::()? % 3; - let idle_timeout_secs = d.gen_u64(Bound::Included(&0), Bound::Included(&(2 * 3600)))?; - let idle_timeout = std::time::Duration::from_secs(idle_timeout_secs); - match nat_mode { - 0 => Some(LegalValue(GatewayAgentPeeringsPeeringExposeNat { + fn translation(&self, d: &mut D) -> Option { + let idle_secs = d.gen_u64(Bound::Included(&0), Bound::Included(&(2 * 3600)))?; + let idle = std::time::Duration::from_secs(idle_secs); + Some(match self.flavour { + NatFlavour::None => return None, + NatFlavour::Masquerade => GatewayAgentPeeringsPeeringExposeNat { masquerade: Some(GatewayAgentPeeringsPeeringExposeNatMasquerade { - idle_timeout: Some(idle_timeout.into()), + idle_timeout: Some(idle.into()), }), port_forward: None, r#static: None, - })), - 1 => Some(LegalValue(GatewayAgentPeeringsPeeringExposeNat { + }, + NatFlavour::Static => GatewayAgentPeeringsPeeringExposeNat { masquerade: None, port_forward: None, r#static: Some(GatewayAgentPeeringsPeeringExposeNatStatic {}), - })), - 2 => { - // Generate a valid port range - let bound1 = d.gen_u16(Bound::Included(&1), Bound::Included(&65535))?; - let bound2 = d.gen_u16(Bound::Included(&1), Bound::Included(&65535))?; - let start = bound1.min(bound2); - let end = bound1.max(bound2); - let port_range = format!("{start}-{end}"); - - // Generate another valid port range of the same size - let port_range_size = (end - start) as usize + 1; - let max_new_start = u16::try_from(65536 - port_range_size).unwrap(); - let new_bound = d.gen_u16(Bound::Included(&0), Bound::Included(&max_new_start))?; - let new_port_range = format!( - "{new_bound}-{}", - new_bound + u16::try_from(port_range_size - 1).unwrap() - ); - - Some(LegalValue(GatewayAgentPeeringsPeeringExposeNat { + }, + NatFlavour::PortForward => { + let (port, r#as) = Self::port_pair(d)?; + GatewayAgentPeeringsPeeringExposeNat { masquerade: None, port_forward: Some(GatewayAgentPeeringsPeeringExposeNatPortForward { - idle_timeout: Some(idle_timeout.into()), + idle_timeout: Some(idle.into()), ports: Some(vec![GatewayAgentPeeringsPeeringExposeNatPortForwardPorts { - r#as: Some(new_port_range), - port: Some(port_range), - proto: match d.produce::()? % 3 { + r#as: Some(r#as), + port: Some(port), + proto: match d.gen_u8(Bound::Included(&0), Bound::Included(&2))? { 0 => Some( GatewayAgentPeeringsPeeringExposeNatPortForwardPortsProto::Tcp, ), 1 => Some( GatewayAgentPeeringsPeeringExposeNatPortForwardPortsProto::Udp, ), - 2 => None, - _ => unreachable!(), + _ => None, }, }]), }), r#static: None, - })) + } + } + }) + } +} + +impl ValueGenerator for ExposeGenerator<'_> { + type Output = GatewayAgentPeeringsPeeringExpose; + + fn generate(&self, d: &mut D) -> Option { + let paired = matches!(self.flavour, NatFlavour::Static | NatFlavour::PortForward); + + let mut ips = Vec::new(); + let mut translations = Vec::new(); + + if paired { + let len = self.length(d)?; + let private = blocks::private(d, self.family, len)?; + let public = blocks::public(d, self.family, len)?; + ips.push(GatewayAgentPeeringsPeeringExposeIps { + cidr: Some(private), + not: None, + vpc_subnet: None, + }); + translations.push(GatewayAgentPeeringsPeeringExposeAs { + cidr: Some(public), + not: None, + }); + } else { + let count = d.gen_u8(Bound::Included(&1), Bound::Included(&MAX_PREFIXES))?; + for _ in 0..count { + let len = self.length(d)?; + ips.push(GatewayAgentPeeringsPeeringExposeIps { + cidr: Some(blocks::private(d, self.family, len)?), + not: None, + vpc_subnet: None, + }); + } + if self.flavour.needs_translation() { + let count = d.gen_u8(Bound::Included(&1), Bound::Included(&MAX_PREFIXES))?; + for _ in 0..count { + let len = self.length(d)?; + translations.push(GatewayAgentPeeringsPeeringExposeAs { + cidr: Some(blocks::public(d, self.family, len)?), + not: None, + }); + } + } + + let named = self.matching_subnets(); + if !named.is_empty() { + let take = d.gen_usize(Bound::Included(&0), Bound::Included(&named.len()))?; + for name in named.into_iter().take(take) { + ips.push(GatewayAgentPeeringsPeeringExposeIps { + cidr: None, + not: None, + vpc_subnet: Some(name.clone()), + }); + } } - _ => unreachable!(), } + + if self.flavour.allows_exclusions() && d.produce::()? { + let parents: Vec = ips.iter().filter_map(|e| e.cidr.clone()).collect(); + if let Some(parent) = parents.first() + && let Some(exclusion) = self.exclusion(d, parent, true) + { + ips.push(GatewayAgentPeeringsPeeringExposeIps { + cidr: None, + not: Some(exclusion), + vpc_subnet: None, + }); + } + let parents: Vec = translations.iter().filter_map(|e| e.cidr.clone()).collect(); + if let Some(parent) = parents.first() + && let Some(exclusion) = self.exclusion(d, parent, false) + { + translations.push(GatewayAgentPeeringsPeeringExposeAs { + cidr: None, + not: Some(exclusion), + }); + } + } + + Some(GatewayAgentPeeringsPeeringExpose { + r#as: Some(translations).filter(|t| !t.is_empty()), + ips: Some(ips).filter(|i| !i.is_empty()), + default: None, + nat: if self.flavour.needs_translation() { + Some(self.translation(d)?) + } else { + None + }, + }) + } +} + +#[derive(Debug, Clone)] +pub struct AnyExposeGenerator<'a> { + subnets: &'a SubnetMap, +} + +impl<'a> AnyExposeGenerator<'a> { + #[must_use] + pub fn new(subnets: &'a SubnetMap) -> Self { + Self { subnets } + } +} + +impl ValueGenerator for AnyExposeGenerator<'_> { + type Output = GatewayAgentPeeringsPeeringExpose; + + fn generate(&self, d: &mut D) -> Option { + let flavours = NatFlavour::all(); + let families = AddressFamily::all(); + let flavour = + flavours[d.gen_usize(Bound::Included(&0), Bound::Excluded(&flavours.len()))?]; + let family = + families[d.gen_usize(Bound::Included(&0), Bound::Excluded(&families.len()))?]; + ExposeGenerator::new(flavour, family, self.subnets).generate(d) } } diff --git a/k8s-intf/src/bolero/mod.rs b/k8s-intf/src/bolero/mod.rs index 078ef00a18..60698c33c6 100644 --- a/k8s-intf/src/bolero/mod.rs +++ b/k8s-intf/src/bolero/mod.rs @@ -17,6 +17,59 @@ use std::collections::BTreeMap; use lpm::prefix::Prefix; +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] +pub enum NatFlavour { + None, + Masquerade, + Static, + PortForward, +} + +impl NatFlavour { + #[must_use] + pub fn all() -> Vec { + vec![ + Self::None, + Self::Masquerade, + Self::Static, + Self::PortForward, + ] + } + + #[must_use] + pub fn allows_exclusions(self) -> bool { + matches!(self, Self::None | Self::Masquerade) + } + + #[must_use] + pub fn needs_translation(self) -> bool { + !matches!(self, Self::None) + } + + #[must_use] + pub fn is_stateful(self) -> bool { + matches!(self, Self::Masquerade | Self::PortForward) + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] +pub enum AddressFamily { + V4, + V6, +} + +impl AddressFamily { + #[must_use] + pub fn all() -> Vec { + vec![Self::V4, Self::V6] + } + + #[must_use] + pub fn is_v4(self) -> bool { + matches!(self, Self::V4) + } +} + /// A type on which implement `bolero::TypeGenerator` for legal values of `T` /// /// Generally, `bolero` type generators should generate all possible values of `T` so that it is possible to test validation logic, etc. @@ -70,9 +123,9 @@ where // This is distinct from the SubnetMap in config/converters/k8s // since this type is only for the test library. It should be // compatible with the SubnetMap in config/converters/k8s -type SubnetMap = BTreeMap; +pub(crate) type SubnetMap = BTreeMap; // This is distinct from the VpcSubnetMap in config/converters/k8s // since this type is only for the test library. It should be // compatible with the SubnetMap in config/converters/k8s -type VpcSubnetMap = BTreeMap; +pub(crate) type VpcSubnetMap = BTreeMap; diff --git a/k8s-intf/src/bolero/peering.rs b/k8s-intf/src/bolero/peering.rs index fdd71f3360..7f2cf45197 100644 --- a/k8s-intf/src/bolero/peering.rs +++ b/k8s-intf/src/bolero/peering.rs @@ -6,8 +6,8 @@ use std::ops::Bound; use bolero::{Driver, ValueGenerator}; -use crate::bolero::expose::LegalValueExposeGenerator; -use crate::bolero::{SubnetMap, VpcSubnetMap}; +use crate::bolero::expose::ExposeGenerator; +use crate::bolero::{AddressFamily, NatFlavour, SubnetMap, VpcSubnetMap}; use crate::gateway_agent_crd::{GatewayAgentPeerings, GatewayAgentPeeringsPeering}; /// Generate legal values for `GatewayAgentPeeringsPeering` @@ -16,12 +16,25 @@ use crate::gateway_agent_crd::{GatewayAgentPeerings, GatewayAgentPeeringsPeering /// In particular, subnet names are restricted. Lengths of various lists is also limited to 16 pub struct LegalValuePeeringsPeeringGenerator<'a> { subnets: &'a SubnetMap, + flavours: &'a [NatFlavour], + family: AddressFamily, + max_exposes: u8, } impl<'a> LegalValuePeeringsPeeringGenerator<'a> { #[must_use] - pub fn new(subnets: &'a SubnetMap) -> Self { - Self { subnets } + pub fn new( + subnets: &'a SubnetMap, + flavours: &'a [NatFlavour], + family: AddressFamily, + max_exposes: u8, + ) -> Self { + Self { + subnets, + flavours, + family, + max_exposes, + } } } @@ -29,11 +42,13 @@ impl ValueGenerator for LegalValuePeeringsPeeringGenerator<'_> { type Output = GatewayAgentPeeringsPeering; fn generate(&self, d: &mut D) -> Option { - let num_expose = d.gen_usize(Bound::Included(&1), Bound::Included(&16))?; - let expose_gen = LegalValueExposeGenerator::new(self.subnets); - let expose = (0..num_expose) - .map(|_| expose_gen.generate(d)) - .collect::>>()?; + let num_expose = d.gen_u8(Bound::Included(&1), Bound::Included(&self.max_exposes))?; + let mut expose = Vec::with_capacity(usize::from(num_expose)); + for _ in 0..num_expose { + let flavour = self.flavours + [d.gen_usize(Bound::Included(&0), Bound::Excluded(&self.flavours.len()))?]; + expose.push(ExposeGenerator::new(flavour, self.family, self.subnets).generate(d)?); + } Some(GatewayAgentPeeringsPeering { expose: Some(expose).filter(|e| !e.is_empty()), @@ -47,6 +62,10 @@ impl ValueGenerator for LegalValuePeeringsPeeringGenerator<'_> { pub struct LegalValuePeeringsGenerator<'a> { vpc_subnets: &'a VpcSubnetMap, vpc_names: Vec<&'a String>, + flavours: &'a [NatFlavour], + families: &'a [AddressFamily], + max_exposes: u8, + groups: &'a [String], } impl<'a> LegalValuePeeringsGenerator<'a> { @@ -55,16 +74,43 @@ impl<'a> LegalValuePeeringsGenerator<'a> { /// # Errors /// /// Returns an error if there are less than two VPCs in the subnet map. - pub fn new(vpc_subnets: &'a VpcSubnetMap) -> Result { + pub fn new( + vpc_subnets: &'a VpcSubnetMap, + flavours: &'a [NatFlavour], + families: &'a [AddressFamily], + max_exposes: u8, + groups: &'a [String], + ) -> Result { if vpc_subnets.len() < 2 { return Err("At least two VPCs are required to generate peerings".to_string()); } + if groups.is_empty() { + return Err("At least one gateway group is required".to_string()); + } let vpc_names = vpc_subnets.keys().collect(); Ok(Self { vpc_subnets, vpc_names, + flavours, + families, + max_exposes, + groups, }) } + + fn stateless_of(&self) -> Vec { + let stateless: Vec = self + .flavours + .iter() + .copied() + .filter(|flavour| !flavour.is_stateful()) + .collect(); + if stateless.is_empty() { + vec![NatFlavour::None] + } else { + stateless + } + } } fn pick2<'a, D: Driver, T>(d: &mut D, items: &[&'a T]) -> Option<[&'a T; 2]> { @@ -96,16 +142,36 @@ impl LegalValuePeeringsGenerator<'_> { d: &mut D, vpc_names: [&String; 2], ) -> Option { + let family = self.families + [d.gen_usize(Bound::Included(&0), Bound::Excluded(&self.families.len()))?]; + + let stateful_side = d.gen_usize(Bound::Included(&0), Bound::Included(&1))?; + let stateless = self.stateless_of(); + let empty_map = SubnetMap::new(); - let peerings_gens = vpc_names.map(|n| { - LegalValuePeeringsPeeringGenerator::new(self.vpc_subnets.get(n).unwrap_or(&empty_map)) - }); let peering = (0..=1) - .map(|i| Some((vpc_names[i].clone(), peerings_gens[i].generate(d)?))) + .map(|i| { + let flavours: &[NatFlavour] = if i == stateful_side { + self.flavours + } else { + &stateless + }; + let generator = LegalValuePeeringsPeeringGenerator::new( + self.vpc_subnets.get(vpc_names[i]).unwrap_or(&empty_map), + flavours, + family, + self.max_exposes, + ); + Some((vpc_names[i].clone(), generator.generate(d)?)) + }) .collect::>>()?; + let group = self.groups + [d.gen_usize(Bound::Included(&0), Bound::Excluded(&self.groups.len()))?] + .clone(); + Some(GatewayAgentPeerings { - gateway_group: Some(d.produce::()?), + gateway_group: Some(group), peering: Some(peering), acl: None, // FIXME: Add a proper implementation when used }) diff --git a/k8s-intf/src/bolero/spec.rs b/k8s-intf/src/bolero/spec.rs index dbd5b1aeb4..10aeb406e4 100644 --- a/k8s-intf/src/bolero/spec.rs +++ b/k8s-intf/src/bolero/spec.rs @@ -4,12 +4,12 @@ use std::collections::{BTreeMap, HashSet}; use std::ops::Bound; -use bolero::{Driver, TypeGenerator}; +use bolero::{Driver, TypeGenerator, ValueGenerator}; use lpm::prefix::Prefix; use crate::bolero::peering::LegalValuePeeringsGenerator; -use crate::bolero::{LegalValue, SubnetMap, VpcSubnetMap}; +use crate::bolero::{AddressFamily, LegalValue, NatFlavour, SubnetMap, VpcSubnetMap}; use crate::gateway_agent_crd::{ GatewayAgentGateway, GatewayAgentGroups, GatewayAgentSpec, GatewayAgentVpcs, }; @@ -44,6 +44,85 @@ fn increment_string(s: &mut str) { } } +#[derive(Debug, Clone)] +pub struct SpecBuilder { + max_vpcs: u8, + max_peerings: u8, + max_exposes: u8, + max_subnets: u8, + flavours: Vec, + families: Vec, +} + +impl Default for SpecBuilder { + fn default() -> Self { + Self { + max_vpcs: 4, + max_peerings: 3, + max_exposes: 2, + max_subnets: 3, + flavours: NatFlavour::all(), + families: AddressFamily::all(), + } + } +} + +impl SpecBuilder { + #[must_use] + pub fn max_vpcs(mut self, max: u8) -> Self { + self.max_vpcs = max; + self + } + + #[must_use] + pub fn max_peerings(mut self, max: u8) -> Self { + self.max_peerings = max; + self + } + + #[must_use] + pub fn max_exposes(mut self, max: u8) -> Self { + self.max_exposes = max; + self + } + + #[must_use] + pub fn max_subnets(mut self, max: u8) -> Self { + self.max_subnets = max; + self + } + + #[must_use] + pub fn flavours(mut self, flavours: Vec) -> Self { + if !flavours.is_empty() { + self.flavours = flavours; + } + self + } + + #[must_use] + pub fn families(mut self, families: Vec) -> Self { + if !families.is_empty() { + self.families = families; + } + self + } + + #[must_use] + pub fn build(self) -> GatewayAgentSpecs { + GatewayAgentSpecs(self) + } +} + +#[derive(Debug, Clone)] +pub struct GatewayAgentSpecs(pub(crate) SpecBuilder); + +impl Default for GatewayAgentSpecs { + fn default() -> Self { + SpecBuilder::default().build() + } +} + /// Generate a random legal `GatewayAgentSpec` /// /// This does not cover all legal `GatewayAgentSpecs`, @@ -52,9 +131,19 @@ fn increment_string(s: &mut str) { /// vni combinations are generated. impl TypeGenerator for LegalValue { fn generate(d: &mut D) -> Option { - let num_vpcs = d.gen_usize(Bound::Included(&0), Bound::Included(&16))?; + Some(LegalValue(GatewayAgentSpecs::default().generate(d)?)) + } +} + +impl ValueGenerator for GatewayAgentSpecs { + type Output = GatewayAgentSpec; + + fn generate(&self, d: &mut D) -> Option { + let knobs = &self.0; + let num_vpcs = + usize::from(d.gen_u8(Bound::Included(&0), Bound::Included(&knobs.max_vpcs))?); let num_peerings = if num_vpcs > 1 { - d.gen_usize(Bound::Included(&0), Bound::Included(&16))? + usize::from(d.gen_u8(Bound::Included(&0), Bound::Included(&knobs.max_peerings))?) } else { 0 }; @@ -64,8 +153,8 @@ impl TypeGenerator for LegalValue { let mut vpc_internal_ids = HashSet::new(); for i in 0..num_vpcs { let vni_offset = u32::try_from(i).expect("too many vpcs"); - let lv_vpc = d.produce::>()?; - let mut vpc = lv_vpc.take(); + let mut vpc = crate::bolero::vpc::VpcGenerator::new(knobs.max_subnets, &knobs.families) + .generate(d)?; let vpc_id = vpc.internal_id.as_mut().unwrap(); while !vpc_internal_ids.insert(vpc_id.clone()) { // We already have a VPC with this internal_id, "increment" the string to generate a @@ -78,9 +167,23 @@ impl TypeGenerator for LegalValue { let vpc_subnet_map = extract_subnets(&vpcs); + let num_groups = d.gen_usize(Bound::Included(&0), Bound::Included(&6))?; + let mut groups = BTreeMap::new(); + for i in 0..=num_groups { + groups.insert(format!("gwgroup-{i}"), d.produce::()?); + } + let group_names: Vec = groups.keys().cloned().collect(); + let mut peerings = BTreeMap::new(); if num_peerings > 0 { - let peering_gen = LegalValuePeeringsGenerator::new(&vpc_subnet_map).unwrap(); + let peering_gen = LegalValuePeeringsGenerator::new( + &vpc_subnet_map, + &knobs.flavours, + &knobs.families, + knobs.max_exposes, + &group_names, + ) + .unwrap(); let mut available = peering_gen.pairs(); let wanted = num_peerings.min(available.len()); for i in 0..wanted { @@ -90,12 +193,6 @@ impl TypeGenerator for LegalValue { } } - let num_groups = d.gen_usize(Bound::Included(&0), Bound::Included(&6))?; - let mut groups = BTreeMap::new(); - for i in 0..=num_groups { - groups.insert(format!("gwgroup-{i}"), d.produce::()?); - } - let num_communities = d.gen_usize(Bound::Included(&0), Bound::Included(&9))?; let mut communities = BTreeMap::new(); for i in 0..=num_communities { @@ -103,7 +200,7 @@ impl TypeGenerator for LegalValue { communities.insert(i.to_string(), community); } - Some(LegalValue(GatewayAgentSpec { + Some(GatewayAgentSpec { agent_version: None, config: None, groups: Some(groups), @@ -111,6 +208,6 @@ impl TypeGenerator for LegalValue { gateway: Some(d.produce::>()?.take()), vpcs: Some(vpcs).filter(|v| !v.is_empty()), peerings: Some(peerings).filter(|p| !p.is_empty()), - })) + }) } } diff --git a/k8s-intf/src/bolero/support.rs b/k8s-intf/src/bolero/support.rs index bf1ba01f61..2d21248e89 100644 --- a/k8s-intf/src/bolero/support.rs +++ b/k8s-intf/src/bolero/support.rs @@ -423,3 +423,109 @@ mod test { } } } + +pub mod blocks { + use crate::bolero::AddressFamily; + use bolero::Driver; + use std::net::{Ipv4Addr, Ipv6Addr}; + + pub const MIN_V4_LEN: u8 = 16; + pub const MIN_V6_LEN: u8 = 48; + + fn v4(base: u32, block_len: u8, host: u32, len: u8) -> String { + let block_host_bits = 32 - block_len; + let within = if block_host_bits >= 32 { + host + } else { + host & ((1u32 << block_host_bits) - 1) + }; + let mask = u32::MAX.checked_shl(u32::from(32 - len)).unwrap_or(0); + let addr = (base | within) & mask; + format!("{}/{len}", Ipv4Addr::from(addr)) + } + + fn v6(base: u128, block_len: u8, host: u128, len: u8) -> String { + let block_host_bits = 128 - block_len; + let within = if block_host_bits >= 128 { + host + } else { + host & ((1u128 << block_host_bits) - 1) + }; + let mask = u128::MAX.checked_shl(u32::from(128 - len)).unwrap_or(0); + let addr = (base | within) & mask; + format!("{}/{len}", Ipv6Addr::from(addr)) + } + + pub fn private(d: &mut D, family: AddressFamily, len: u8) -> Option { + Some(if family.is_v4() { + v4(0x0A00_0000, 8, d.produce::()?, len) + } else { + v6( + 0x2001_0db8_0000_0000_0000_0000_0000_0000, + 33, + d.produce::()?, + len, + ) + }) + } + + pub fn public(d: &mut D, family: AddressFamily, len: u8) -> Option { + Some(if family.is_v4() { + v4(0xAC10_0000, 12, d.produce::()?, len) + } else { + v6( + 0x2001_0db8_8000_0000_0000_0000_0000_0000, + 33, + d.produce::()?, + len, + ) + }) + } + + pub fn private_run( + d: &mut D, + family: AddressFamily, + len: u8, + count: u16, + ) -> Option> { + if count == 0 { + return Some(Vec::new()); + } + let mut out = Vec::with_capacity(usize::from(count)); + if family.is_v4() { + let slots = 1u32.checked_shl(u32::from(len) - 8).unwrap_or(u32::MAX); + let first = d.produce::()? % slots; + let shift = u32::from(32 - len); + for i in 0..u32::from(count) { + let slot = (first + i) % slots; + let addr = 0x0A00_0000 | slot.checked_shl(shift).unwrap_or(0); + out.push(format!("{}/{len}", Ipv4Addr::from(addr))); + } + } else { + let slots = 1u128.checked_shl(u32::from(len) - 33).unwrap_or(u128::MAX); + let first = d.produce::()? % slots; + let shift = u32::from(128 - len); + for i in 0..u128::from(count) { + let slot = (first + i) % slots; + let addr = 0x2001_0db8_0000_0000_0000_0000_0000_0000 + | slot.checked_shl(shift).unwrap_or(0); + out.push(format!("{}/{len}", Ipv6Addr::from(addr))); + } + } + Some(out) + } + + #[must_use] + pub fn min_len(family: AddressFamily) -> u8 { + if family.is_v4() { + MIN_V4_LEN + } else { + MIN_V6_LEN + } + } + + #[must_use] + pub fn max_len(family: AddressFamily) -> u8 { + if family.is_v4() { 32 } else { 128 } + } +} diff --git a/k8s-intf/src/bolero/vpc.rs b/k8s-intf/src/bolero/vpc.rs index 0eb84b6b92..c03ac81b36 100644 --- a/k8s-intf/src/bolero/vpc.rs +++ b/k8s-intf/src/bolero/vpc.rs @@ -7,8 +7,8 @@ use bolero::{Driver, TypeGenerator, ValueGenerator}; use net::vxlan::Vni; -use crate::bolero::LegalValue; -use crate::bolero::support::{UniqueV4CidrGenerator, UniqueV6CidrGenerator}; +use crate::bolero::support::blocks; +use crate::bolero::{AddressFamily, LegalValue}; use crate::gateway_agent_crd::{GatewayAgentVpcs, GatewayAgentVpcsSubnets}; fn generate_internal_id(d: &mut D) -> Option { @@ -21,20 +21,50 @@ fn generate_internal_id(d: &mut D) -> Option { Some(result) } -impl TypeGenerator for LegalValue { - fn generate(d: &mut D) -> Option { +#[derive(Debug, Clone)] +pub struct VpcGenerator<'a> { + max_subnets: u8, + families: &'a [AddressFamily], +} + +impl<'a> VpcGenerator<'a> { + #[must_use] + pub fn new(max_subnets: u8, families: &'a [AddressFamily]) -> Self { + Self { + max_subnets, + families, + } + } + + fn wants(&self, family: AddressFamily) -> bool { + self.families.contains(&family) + } +} + +impl ValueGenerator for VpcGenerator<'_> { + type Output = GatewayAgentVpcs; + + fn generate(&self, d: &mut D) -> Option { let internal_id = generate_internal_id(d)?; let vni = d.produce::()?; - let v4_masklen = d.gen_u8(Bound::Included(&0), Bound::Included(&32))?; - let num_v4_cidrs = d.gen_u16(Bound::Included(&0), Bound::Included(&16))?; + let v4_masklen = d.gen_u8(Bound::Included(&blocks::MIN_V4_LEN), Bound::Included(&32))?; + let v6_masklen = d.gen_u8(Bound::Included(&blocks::MIN_V6_LEN), Bound::Included(&128))?; + let num_v4_cidrs = if self.wants(AddressFamily::V4) { + u16::from(d.gen_u8(Bound::Included(&0), Bound::Included(&self.max_subnets))?) + } else { + 0 + }; + let num_v6_cidrs = if self.wants(AddressFamily::V6) { + u16::from(d.gen_u8(Bound::Included(&0), Bound::Included(&self.max_subnets))?) + } else { + 0 + }; - let v6_masklen = d.gen_u8(Bound::Included(&0), Bound::Included(&128))?; - let num_v6_cidrs = d.gen_u16(Bound::Included(&0), Bound::Included(&16))?; - let v4_gen = UniqueV4CidrGenerator::new(num_v4_cidrs, v4_masklen); - let v6_gen = UniqueV6CidrGenerator::new(num_v6_cidrs, v6_masklen); - - let subnets_cidrs = vec![v4_gen.generate(d)?, v6_gen.generate(d)?]; + let subnets_cidrs = vec![ + blocks::private_run(d, AddressFamily::V4, v4_masklen, num_v4_cidrs)?, + blocks::private_run(d, AddressFamily::V6, v6_masklen, num_v6_cidrs)?, + ]; let subnets = subnets_cidrs .into_iter() .flatten() @@ -47,10 +77,17 @@ impl TypeGenerator for LegalValue { }) .collect::>(); - Some(LegalValue(GatewayAgentVpcs { + Some(GatewayAgentVpcs { internal_id: Some(internal_id), vni: Some(vni.into()), subnets: Some(subnets).filter(|s| !s.is_empty()), - })) + }) + } +} + +impl TypeGenerator for LegalValue { + fn generate(d: &mut D) -> Option { + let families = AddressFamily::all(); + Some(LegalValue(VpcGenerator::new(3, &families).generate(d)?)) } } diff --git a/mgmt/src/processor/confbuild/internal.rs b/mgmt/src/processor/confbuild/internal.rs index 78a15ac433..653913b079 100644 --- a/mgmt/src/processor/confbuild/internal.rs +++ b/mgmt/src/processor/confbuild/internal.rs @@ -464,6 +464,7 @@ mod chain_properties { static SEEN: AtomicUsize = AtomicUsize::new(0); static VALIDATED: AtomicUsize = AtomicUsize::new(0); static VPCS: AtomicUsize = AtomicUsize::new(0); + static PEERINGS: AtomicUsize = AtomicUsize::new(0); bolero::check!() .with_type::>() @@ -473,8 +474,13 @@ mod chain_properties { .unwrap_or_else(|e| panic!("a schema-legal CRD did not convert: {e}")); if let Ok(validated) = external.validate() { VALIDATED.fetch_add(1, Ordering::Relaxed); - VPCS.fetch_add( - validated.external().overlay().vpc_table().len(), + let table = validated.external().overlay().vpc_table(); + VPCS.fetch_add(table.len(), Ordering::Relaxed); + PEERINGS.fetch_add( + table + .values() + .map(|vpc| vpc.peerings().len()) + .sum::(), Ordering::Relaxed, ); } @@ -483,14 +489,20 @@ mod chain_properties { let seen = SEEN.load(Ordering::Relaxed); let validated = VALIDATED.load(Ordering::Relaxed); let vpcs = VPCS.load(Ordering::Relaxed); - println!("{validated}/{seen} configurations validated, carrying {vpcs} vpcs"); + let peerings = PEERINGS.load(Ordering::Relaxed); + println!("{validated}/{seen} validated, carrying {vpcs} vpcs and {peerings} peerings"); assert!(seen > 0, "no configurations were generated"); assert!( - validated * 20 >= seen, + validated * 2 >= seen, "only {validated} of {seen} configurations validated: the properties above are \ - checking almost nothing" + checking much less than they look like they are" ); assert!(vpcs > validated, "validated configurations carry no vpcs"); + assert!( + peerings > 0, + "no validated configuration carries a peering, so nothing downstream of validation \ + has seen the exposes, the NAT or the ACLs" + ); } #[test] From fc3630e2e039dffd523a96d018a8dfde2ea8ef89 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 17:35:45 -0600 Subject: [PATCH 09/23] test(k8s-intf): Generate peering ACLs The peering generators carried `acl: None // FIXME: Add a proper implementation when used`, so no ACL ever reached the converter, the validator or anything past them. `config/src/converters/k8s/config/acl.rs` is the largest converter in the crate at 800 lines, with ten hand-written tests and no generated input. The ACL is built from the manifests rather than beside them, and that is the shape of the thing. A rule's `match` is checked against what the two sides of the peering actually expose -- the source prefixes have to intersect the *from* side's native addresses, the destination prefixes the *to* side's advertised ones -- and `scope: flow` is checked against how they translate. So the generator reads those facts back off the manifests the peering generator has just built (`SideFacts::of`) and names prefixes that are really there. Drawing them freely would produce rules that match nothing, which is refused outright. The rules satisfied by construction: - `from` and `to` name the peering's two vpcs, in either order, and sometimes only one of them -- the converter completes the other, and that completion is code worth running; - a named prefix comes from the corresponding side, and carries no ports of its own: coverage compares addresses *and* ports, so ports named against a prefix that already restricts them in the manifest would intersect nothing; - only TCP and UDP may carry ports at all, so any other protocol and any-protocol get none; - ports are only named on a side whose exposes do not restrict them, i.e. one with no port forwarding; - an ACL has at least one rule, since one with none says nothing its peering's default action does not; - `scope: flow` only where one side of the peering is stateful throughout. The scope default is worth its own paragraph. The CRD says a rule's scope "can be either 'flow' (default if empty) or 'packet'", so omitting the field asks for flow, and is refused in exactly the cases an explicit `flow` would be. Letting the flow-is-not-allowed case fall through to omitting the field therefore asks for flow by another name: naming `packet` explicitly took ACL yield from 24% of validated configurations to 64%. The vacuity test asserts that share rather than merely that some ACL survives. An ACL refused for `scope: flow` is refused for something other than what the rule says, so a generator that gets it wrong still produces some valid ACLs -- just far fewer, which an `acls > 0` assertion cannot see. Residue, about one in six thousand: a rule whose destination prefix does not intersect the *to* side's advertised set. The advertised set is read here as "the translation range where the expose has one, the native prefix otherwise", which is not quite what `all_public_ips` computes in every case. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- k8s-intf/src/bolero/acl.rs | 229 +++++++++++++++++++++++ k8s-intf/src/bolero/mod.rs | 1 + k8s-intf/src/bolero/peering.rs | 14 +- mgmt/src/processor/confbuild/internal.rs | 21 ++- 4 files changed, 262 insertions(+), 3 deletions(-) create mode 100644 k8s-intf/src/bolero/acl.rs diff --git a/k8s-intf/src/bolero/acl.rs b/k8s-intf/src/bolero/acl.rs new file mode 100644 index 0000000000..3cc3d528ca --- /dev/null +++ b/k8s-intf/src/bolero/acl.rs @@ -0,0 +1,229 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +use std::ops::Bound; + +use bolero::{Driver, ValueGenerator}; + +use crate::gateway_agent_crd::{ + GatewayAgentPeeringsAcl, GatewayAgentPeeringsAclDefault, GatewayAgentPeeringsAclRules, + GatewayAgentPeeringsAclRulesAction, GatewayAgentPeeringsAclRulesMatch, + GatewayAgentPeeringsAclRulesMatchDst, GatewayAgentPeeringsAclRulesMatchSrc, + GatewayAgentPeeringsAclRulesScope, GatewayAgentPeeringsPeering, +}; + +const MAX_RULES: u8 = 3; + +#[derive(Debug, Clone)] +pub struct SideFacts { + pub vpc: String, + pub native: Vec, + pub advertised: Vec, + pub restricts_ports: bool, + pub all_stateful: bool, +} + +impl SideFacts { + #[must_use] + pub fn of(vpc: &str, manifest: &GatewayAgentPeeringsPeering) -> Self { + let exposes = manifest.expose.as_deref().unwrap_or(&[]); + let mut native = Vec::new(); + let mut advertised = Vec::new(); + let mut restricts_ports = false; + let mut all_stateful = !exposes.is_empty(); + + for expose in exposes { + let ips: Vec = expose + .ips + .iter() + .flatten() + .filter_map(|ip| ip.cidr.clone()) + .collect(); + let translations: Vec = expose + .r#as + .iter() + .flatten() + .filter_map(|entry| entry.cidr.clone()) + .collect(); + + native.extend(ips.iter().cloned()); + if translations.is_empty() { + advertised.extend(ips); + } else { + advertised.extend(translations); + } + + let nat = expose.nat.as_ref(); + if nat.is_some_and(|nat| nat.port_forward.is_some()) { + restricts_ports = true; + } + if !nat.is_some_and(|nat| nat.masquerade.is_some() || nat.port_forward.is_some()) { + all_stateful = false; + } + } + + Self { + vpc: vpc.to_string(), + native, + advertised, + restricts_ports, + all_stateful, + } + } +} + +#[derive(Debug, Clone)] +pub struct AclGenerator { + left: SideFacts, + right: SideFacts, +} + +impl AclGenerator { + #[must_use] + pub fn new(left: SideFacts, right: SideFacts) -> Self { + Self { left, right } + } + + fn ports(d: &mut D) -> Option> { + let count = d.gen_u8(Bound::Included(&1), Bound::Included(&2))?; + let mut out = Vec::with_capacity(usize::from(count)); + for _ in 0..count { + let first = d.gen_u16(Bound::Included(&1), Bound::Included(&65535))?; + if d.produce::()? { + out.push(format!("{first}")); + } else { + let second = d.gen_u16(Bound::Included(&1), Bound::Included(&65535))?; + out.push(format!("{}-{}", first.min(second), first.max(second))); + } + } + Some(out) + } + + fn prefix(d: &mut D, choices: &[String]) -> Option { + if choices.is_empty() { + return None; + } + let index = d.gen_usize(Bound::Included(&0), Bound::Excluded(&choices.len()))?; + Some(choices[index].clone()) + } + + fn rule(&self, d: &mut D, index: u8) -> Option { + let (from, to) = if d.produce::()? { + (&self.left, &self.right) + } else { + (&self.right, &self.left) + }; + + let (from_field, to_field) = match d.gen_u8(Bound::Included(&0), Bound::Included(&2))? { + 0 => (Some(from.vpc.clone()), None), + 1 => (None, Some(to.vpc.clone())), + _ => (Some(from.vpc.clone()), Some(to.vpc.clone())), + }; + + let (proto, may_use_ports) = match d.gen_u8(Bound::Included(&0), Bound::Included(&3))? { + 0 => (None, false), + 1 => (Some("tcp".to_string()), true), + 2 => (Some("udp".to_string()), true), + _ => ( + Some( + d.gen_u8(Bound::Included(&1), Bound::Included(&254))? + .to_string(), + ), + false, + ), + }; + + let src_ports = may_use_ports && !from.restricts_ports && d.produce::()?; + let dst_ports = may_use_ports && !to.restricts_ports && d.produce::()?; + + let src_prefix = if d.produce::()? { + Self::prefix(d, &from.native) + } else { + None + }; + let dst_prefix = if d.produce::()? { + Self::prefix(d, &to.advertised) + } else { + None + }; + + let src = if src_prefix.is_some() || src_ports { + Some(vec![GatewayAgentPeeringsAclRulesMatchSrc { + cidr: src_prefix, + ports: if src_ports { + Self::ports(d)?.into() + } else { + None + }, + vpc_subnet: None, + }]) + } else { + None + }; + let dst = if dst_prefix.is_some() || dst_ports { + Some(vec![GatewayAgentPeeringsAclRulesMatchDst { + cidr: dst_prefix, + ports: if dst_ports { + Self::ports(d)?.into() + } else { + None + }, + vpc_subnet: None, + }]) + } else { + None + }; + + let r#match = if src.is_some() || dst.is_some() || proto.is_some() { + Some(GatewayAgentPeeringsAclRulesMatch { dst, proto, src }) + } else { + None + }; + + let flow_allowed = self.left.all_stateful || self.right.all_stateful; + let scope = if flow_allowed && d.produce::()? { + if d.produce::()? { + Some(GatewayAgentPeeringsAclRulesScope::Flow) + } else { + None + } + } else { + Some(GatewayAgentPeeringsAclRulesScope::Packet) + }; + + Some(GatewayAgentPeeringsAclRules { + action: if d.produce::()? { + GatewayAgentPeeringsAclRulesAction::Allow + } else { + GatewayAgentPeeringsAclRulesAction::Deny + }, + from: from_field, + log: Some(d.produce::()?), + r#match, + name: Some(format!("rule{index}")), + scope, + to: to_field, + }) + } +} + +impl ValueGenerator for AclGenerator { + type Output = GatewayAgentPeeringsAcl; + + fn generate(&self, d: &mut D) -> Option { + let count = d.gen_u8(Bound::Included(&1), Bound::Included(&MAX_RULES))?; + let mut rules = Vec::with_capacity(usize::from(count)); + for index in 0..count { + rules.push(self.rule(d, index)?); + } + + Some(GatewayAgentPeeringsAcl { + default: match d.gen_u8(Bound::Included(&0), Bound::Included(&2))? { + 0 => GatewayAgentPeeringsAclDefault::Deny, + 1 => GatewayAgentPeeringsAclDefault::DenyUnlessExposed, + _ => GatewayAgentPeeringsAclDefault::KopiumEmpty, + }, + rules: Some(rules).filter(|r| !r.is_empty()), + }) + } +} diff --git a/k8s-intf/src/bolero/mod.rs b/k8s-intf/src/bolero/mod.rs index 60698c33c6..5b1243df33 100644 --- a/k8s-intf/src/bolero/mod.rs +++ b/k8s-intf/src/bolero/mod.rs @@ -1,6 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright Open Network Fabric Authors +pub mod acl; pub mod bgp; pub mod crd; pub mod expose; diff --git a/k8s-intf/src/bolero/peering.rs b/k8s-intf/src/bolero/peering.rs index 7f2cf45197..90b6cd5e7d 100644 --- a/k8s-intf/src/bolero/peering.rs +++ b/k8s-intf/src/bolero/peering.rs @@ -6,6 +6,7 @@ use std::ops::Bound; use bolero::{Driver, ValueGenerator}; +use crate::bolero::acl::{AclGenerator, SideFacts}; use crate::bolero::expose::ExposeGenerator; use crate::bolero::{AddressFamily, NatFlavour, SubnetMap, VpcSubnetMap}; use crate::gateway_agent_crd::{GatewayAgentPeerings, GatewayAgentPeeringsPeering}; @@ -170,10 +171,21 @@ impl LegalValuePeeringsGenerator<'_> { [d.gen_usize(Bound::Included(&0), Bound::Excluded(&self.groups.len()))?] .clone(); + let acl = if d.produce::()? { + let facts: Vec = peering + .iter() + .map(|(vpc, manifest)| SideFacts::of(vpc, manifest)) + .collect(); + let [left, right] = <[SideFacts; 2]>::try_from(facts).ok()?; + Some(AclGenerator::new(left, right).generate(d)?) + } else { + None + }; + Some(GatewayAgentPeerings { gateway_group: Some(group), peering: Some(peering), - acl: None, // FIXME: Add a proper implementation when used + acl, }) } } diff --git a/mgmt/src/processor/confbuild/internal.rs b/mgmt/src/processor/confbuild/internal.rs index 653913b079..7121f7fe55 100644 --- a/mgmt/src/processor/confbuild/internal.rs +++ b/mgmt/src/processor/confbuild/internal.rs @@ -465,6 +465,7 @@ mod chain_properties { static VALIDATED: AtomicUsize = AtomicUsize::new(0); static VPCS: AtomicUsize = AtomicUsize::new(0); static PEERINGS: AtomicUsize = AtomicUsize::new(0); + static ACLS: AtomicUsize = AtomicUsize::new(0); bolero::check!() .with_type::>() @@ -483,6 +484,14 @@ mod chain_properties { .sum::(), Ordering::Relaxed, ); + ACLS.fetch_add( + table + .values() + .flat_map(|vpc| vpc.peerings()) + .filter(|peering| peering.acl().is_some()) + .count(), + Ordering::Relaxed, + ); } }); @@ -490,7 +499,10 @@ mod chain_properties { let validated = VALIDATED.load(Ordering::Relaxed); let vpcs = VPCS.load(Ordering::Relaxed); let peerings = PEERINGS.load(Ordering::Relaxed); - println!("{validated}/{seen} validated, carrying {vpcs} vpcs and {peerings} peerings"); + let acls = ACLS.load(Ordering::Relaxed); + println!( + "{validated}/{seen} validated, carrying {vpcs} vpcs, {peerings} peerings, {acls} acls" + ); assert!(seen > 0, "no configurations were generated"); assert!( validated * 2 >= seen, @@ -501,7 +513,12 @@ mod chain_properties { assert!( peerings > 0, "no validated configuration carries a peering, so nothing downstream of validation \ - has seen the exposes, the NAT or the ACLs" + has seen the exposes or the NAT" + ); + assert!( + acls * 2 >= validated, + "only {acls} of {validated} validated configurations carry an ACL: most generated ACLs \ + are being refused for something other than what they say" ); } From a9a2943bf62cf493c798b1c1669eb6f61e4a35ee Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 17:43:54 -0600 Subject: [PATCH 10/23] test(mgmt): Build every dataplane table a validated config implies `build_internal_config` turns a validated configuration into the FRR half of it, and the chain properties cover that. The other half is the dataplane's own tables, built from the same validated configuration by - `build_nat_configuration` -- static NAT, - `MasqueradeConfig::new` and `update_nat_allocator` -- masquerade, - `build_port_forwarding_configuration` and `PortFwTableWriter::update_table` -- port forwarding. All of them are fallible from a configuration that has already validated, and the last is where the port-forwarding expose that validated and could not be built actually fired. So the claim is the same one carried a step further: a configuration that validates builds every table it implies. That defect was found by reading the code. This is what would have found it, from generated CRD input, at the point it fires in production -- during apply, at the last of the NAT stages, after the kernel interfaces, the flow filter, the ACLs, the static NAT tables and the masquerade allocator have all been committed. The class is now guarded by machine rather than by having noticed it. One property per NAT flavour, using the generator knobs: a property over the default flavour mix reaches each flavour eventually, one that asks for a flavour reaches it in every case and says in its name which one failed. Each asserts its own yield so it cannot quietly go vacuous. Masquerade runs at about a thirtieth of the others' rate, because rebuilding the allocator walks the address-port pools. Worth knowing before anyone wonders why that one test is slow. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- mgmt/src/tests/mgmt.rs | 86 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/mgmt/src/tests/mgmt.rs b/mgmt/src/tests/mgmt.rs index f72592b43a..6a8d230018 100644 --- a/mgmt/src/tests/mgmt.rs +++ b/mgmt/src/tests/mgmt.rs @@ -698,3 +698,89 @@ mod peering_chain { ); } } + +#[cfg(test)] +mod dataplane_tables { + use config::{ExternalConfig, ValidatedGwConfig}; + use flow_entry::flow_table::FlowTable; + use k8s_intf::bolero::NatFlavour; + use k8s_intf::bolero::crd::GatewayAgentBuilder; + use nat::masquerade::{MasqueradeConfig, NatAllocatorWriter}; + use nat::portfw::{PortFwTableWriter, build_port_forwarding_configuration}; + use nat::static_nat::NatTablesWriter; + use nat::static_nat::setup::build_nat_configuration; + + fn build_tables(validated: &ValidatedGwConfig, flavour: NatFlavour) { + let vpc_table = validated.external().overlay().vpc_table(); + + let nat_tables = build_nat_configuration(vpc_table).unwrap_or_else(|e| { + panic!("a validated {flavour:?} configuration would not build static NAT: {e}") + }); + let mut nattablesw = NatTablesWriter::new(); + nattablesw.update_nat_tables(nat_tables); + + let masquerade = MasqueradeConfig::new(vpc_table, validated.genid()).set_randomize(false); + let mut natallocatorw = NatAllocatorWriter::new(); + let flow_table = FlowTable::new(16); + natallocatorw.update_nat_allocator(masquerade, &flow_table); + + let ruleset = build_port_forwarding_configuration(vpc_table).unwrap_or_else(|e| { + panic!("a validated {flavour:?} configuration would not build port forwarding: {e}") + }); + let mut portfw_w = PortFwTableWriter::new(); + portfw_w.update_table(&ruleset).unwrap_or_else(|e| { + panic!("a validated {flavour:?} port-forwarding ruleset was refused by the table: {e}") + }); + } + + fn drive(flavour: NatFlavour) { + use concurrency::sync::atomic::{AtomicUsize, Ordering}; + let seen = AtomicUsize::new(0); + let built = AtomicUsize::new(0); + + let generator = GatewayAgentBuilder::new().flavours(vec![flavour]).build(); + + bolero::check!() + .with_generator(generator) + .cloned() + .for_each(|agent| { + seen.fetch_add(1, Ordering::Relaxed); + let external = ExternalConfig::try_from(&agent) + .unwrap_or_else(|e| panic!("a schema-legal CRD did not convert: {e}")); + let Ok(validated) = external.validate() else { + return; + }; + built.fetch_add(1, Ordering::Relaxed); + build_tables(&validated, flavour); + }); + + let seen = seen.load(Ordering::Relaxed); + let built = built.load(Ordering::Relaxed); + println!("{flavour:?}: {built}/{seen} configurations validated and built their tables"); + assert!( + built * 2 >= seen, + "only {built} of {seen} {flavour:?} configurations validated, so this checked much \ + less than it looks like it did" + ); + } + + #[test] + fn a_static_nat_configuration_builds_its_tables() { + drive(NatFlavour::Static); + } + + #[test] + fn a_masquerade_configuration_builds_its_tables() { + drive(NatFlavour::Masquerade); + } + + #[test] + fn a_port_forwarding_configuration_builds_its_tables() { + drive(NatFlavour::PortForward); + } + + #[test] + fn a_configuration_with_no_nat_builds_its_tables() { + drive(NatFlavour::None); + } +} From 4001ca879db0f6a5d4b7bbff2d2bf2cee413ef7b Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 18:19:10 -0600 Subject: [PATCH 11/23] test(config): Hunt validator permissiveness with near-miss configurations The validator ships as a wasm module in a process of its own. Its entire surface is ExternalConfig::try_from(&crd)?.validate()? -- convert, then validate, and nothing else. If it blesses a configuration, that process writes the configuration to Kubernetes. The dataplane runs the same two steps later, but it has no way to tell anyone something is wrong: by then the configuration is the desired state. There is no path back to the user. So the requirement is not "the dataplane reports bad configurations well". It is that **anything the validator accepts must be enactable**, and every check living only in a downstream builder is a hole in it. A validator that is too strict is a nuisance -- the user sees an error and fixes their input. One that is too permissive is unrecoverable. A panic is the same failure wearing a different coat: in wasm it traps, so the calling process gets a failure with no `ValidateError` in it, and the user gets nothing to act on. Everything up to here generates configurations that are legal *by construction*, which exercises everything downstream of validation and nothing of validation itself. This adds the other kind: a legal configuration with **one** rule deliberately broken. Near-miss rather than arbitrary, because a configuration wrong in one way is far more likely to slip past than one wrong in twenty. Thirteen mutations, each naming a rule the validator is supposed to enforce -- mismatched port-forwarding prefixes, mismatched static-NAT sizes, an exclusion on a port-forwarding expose, mixed address families, a reserved prefix, an empty private list, a dropped translation range, both manifests stateful, a missing gateway group, a stranger in a rule's `from`, flow scope without state, port zero -- and a control that changes nothing. What it asserts: - **whatever the validator accepts, the dataplane can enact**: the internal config builds and renders, and the static NAT tables, masquerade allocator and port-forwarding table all build and are accepted; - **it never panics**, since reaching the assertions at all means it returned; - **a rejection is never `InternalFailure`**, because "this is our bug" is not something a user can act on. Plus enough bookkeeping that the generator cannot quietly stop working: every mutation must be drawn, the control must rarely be refused (otherwise the mutated cases are being refused for the wrong reasons), and a mutation that finds a target must usually be refused. "Usually" rather than "always" because `DemandFlowScope` has a legitimate exception -- asking for flow scope on a peering that *is* stateful throughout is legal. No gap found yet. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- k8s-intf/src/bolero/mod.rs | 1 + k8s-intf/src/bolero/mutate.rs | 343 ++++++++++++++++++++++++++++++++++ mgmt/src/tests/mgmt.rs | 117 ++++++++++++ 3 files changed, 461 insertions(+) create mode 100644 k8s-intf/src/bolero/mutate.rs diff --git a/k8s-intf/src/bolero/mod.rs b/k8s-intf/src/bolero/mod.rs index 5b1243df33..c4fb5c006a 100644 --- a/k8s-intf/src/bolero/mod.rs +++ b/k8s-intf/src/bolero/mod.rs @@ -9,6 +9,7 @@ pub mod gateway; pub mod gwgroups; pub mod interface; pub mod logs; +pub mod mutate; pub mod peering; pub mod spec; pub mod support; diff --git a/k8s-intf/src/bolero/mutate.rs b/k8s-intf/src/bolero/mutate.rs new file mode 100644 index 0000000000..60c9fb9cfa --- /dev/null +++ b/k8s-intf/src/bolero/mutate.rs @@ -0,0 +1,343 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +use std::ops::Bound; + +use bolero::{Driver, ValueGenerator}; + +use crate::bolero::crd::GatewayAgents; +use crate::gateway_agent_crd::{ + GatewayAgent, GatewayAgentPeeringsAclRulesScope, GatewayAgentPeeringsPeeringExpose, + GatewayAgentPeeringsPeeringExposeAs, GatewayAgentPeeringsPeeringExposeIps, + GatewayAgentPeeringsPeeringExposeNatMasquerade, +}; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Mutation { + None, + MismatchPortForwardPrefixes, + MismatchStaticNatPrefixes, + ExcludeFromPortForwarding, + MixAddressFamilies, + UseReservedPrefix, + EmptyPrivatePrefixes, + DropTranslationRange, + MakeBothSidesStateful, + NameAMissingGroup, + NameAStrangerInARule, + DemandFlowScope, + UsePortZero, +} + +impl Mutation { + pub const COUNT: usize = 13; + + #[must_use] + pub fn index(self) -> usize { + Self::all() + .iter() + .position(|other| *other == self) + .unwrap_or_else(|| unreachable!()) + } + + #[must_use] + pub fn all() -> Vec { + vec![ + Self::None, + Self::MismatchPortForwardPrefixes, + Self::MismatchStaticNatPrefixes, + Self::ExcludeFromPortForwarding, + Self::MixAddressFamilies, + Self::UseReservedPrefix, + Self::EmptyPrivatePrefixes, + Self::DropTranslationRange, + Self::MakeBothSidesStateful, + Self::NameAMissingGroup, + Self::NameAStrangerInARule, + Self::DemandFlowScope, + Self::UsePortZero, + ] + } +} + +fn lengthen(cidr: &str, by: u8) -> Option { + let (address, len) = cidr.split_once('/')?; + let len: u8 = len.parse().ok()?; + let max = if address.contains(':') { 128 } else { 32 }; + let longer = len.saturating_add(by).min(max); + if longer == len { + return None; + } + Some(format!("{address}/{longer}")) +} + +fn exposes_mut(agent: &mut GatewayAgent) -> Vec<&mut GatewayAgentPeeringsPeeringExpose> { + agent + .spec + .peerings + .iter_mut() + .flatten() + .flat_map(|(_, peerings)| peerings.peering.iter_mut().flatten()) + .flat_map(|(_, manifest)| manifest.expose.iter_mut().flatten()) + .collect() +} + +fn is_port_forwarding(expose: &GatewayAgentPeeringsPeeringExpose) -> bool { + expose + .nat + .as_ref() + .is_some_and(|nat| nat.port_forward.is_some()) +} + +fn is_static(expose: &GatewayAgentPeeringsPeeringExpose) -> bool { + expose + .nat + .as_ref() + .is_some_and(|nat| nat.r#static.is_some()) +} + +#[allow(clippy::too_many_lines)] +pub fn apply(d: &mut D, agent: &mut GatewayAgent, mutation: Mutation) -> Option { + let bit = match mutation { + Mutation::None => false, + + Mutation::MismatchPortForwardPrefixes | Mutation::MismatchStaticNatPrefixes => { + let wanted: fn(&GatewayAgentPeeringsPeeringExpose) -> bool = + if mutation == Mutation::MismatchPortForwardPrefixes { + is_port_forwarding + } else { + is_static + }; + let mut done = false; + for expose in exposes_mut(agent) { + if !wanted(expose) { + continue; + } + if let Some(entry) = expose.r#as.iter_mut().flatten().next() + && let Some(cidr) = entry.cidr.as_ref() + && let Some(longer) = lengthen(cidr, 2) + { + entry.cidr = Some(longer); + done = true; + break; + } + } + done + } + + Mutation::ExcludeFromPortForwarding => { + let mut done = false; + for expose in exposes_mut(agent) { + if !is_port_forwarding(expose) { + continue; + } + let Some(inside) = expose + .ips + .iter() + .flatten() + .find_map(|ip| ip.cidr.as_ref()) + .and_then(|cidr| lengthen(cidr, 1)) + else { + continue; + }; + expose.ips.get_or_insert_with(Vec::new).push( + GatewayAgentPeeringsPeeringExposeIps { + cidr: None, + not: Some(inside), + vpc_subnet: None, + }, + ); + done = true; + break; + } + done + } + + Mutation::MixAddressFamilies => { + let mut done = false; + for expose in exposes_mut(agent) { + let Some(entry) = expose.ips.iter_mut().flatten().next() else { + continue; + }; + let Some(cidr) = entry.cidr.as_ref() else { + continue; + }; + let other = if cidr.contains(':') { + "10.99.0.0/16" + } else { + "2001:db8:9999::/48" + }; + expose.ips.get_or_insert_with(Vec::new).push( + GatewayAgentPeeringsPeeringExposeIps { + cidr: Some(other.to_string()), + not: None, + vpc_subnet: None, + }, + ); + done = true; + break; + } + done + } + + Mutation::UseReservedPrefix => { + let reserved = ["127.0.0.0/8", "224.0.0.0/4", "0.0.0.0/8", "ff00::/8"]; + let choice = + reserved[d.gen_usize(Bound::Included(&0), Bound::Excluded(&reserved.len()))?]; + let mut done = false; + for expose in exposes_mut(agent) { + if let Some(entry) = expose.ips.iter_mut().flatten().next() + && entry.cidr.is_some() + { + entry.cidr = Some(choice.to_string()); + done = true; + break; + } + } + done + } + + Mutation::EmptyPrivatePrefixes => { + let mut done = false; + for expose in exposes_mut(agent) { + if expose.ips.is_some() { + expose.ips = None; + done = true; + break; + } + } + done + } + + Mutation::DropTranslationRange => { + let mut done = false; + for expose in exposes_mut(agent) { + if expose.nat.is_some() && expose.r#as.is_some() { + expose.r#as = None; + done = true; + break; + } + } + done + } + + Mutation::MakeBothSidesStateful => { + let mut done = false; + for (_, peerings) in agent.spec.peerings.iter_mut().flatten() { + let manifests = peerings.peering.iter_mut().flatten(); + let mut touched = 0; + for (_, manifest) in manifests { + for expose in manifest.expose.iter_mut().flatten() { + let nat = expose.nat.get_or_insert( + crate::gateway_agent_crd::GatewayAgentPeeringsPeeringExposeNat { + masquerade: None, + port_forward: None, + r#static: None, + }, + ); + nat.port_forward = None; + nat.r#static = None; + nat.masquerade = Some(GatewayAgentPeeringsPeeringExposeNatMasquerade { + idle_timeout: None, + }); + if expose.r#as.is_none() { + expose.r#as = Some(vec![GatewayAgentPeeringsPeeringExposeAs { + cidr: Some("172.31.0.0/16".to_string()), + not: None, + }]); + } + } + touched += 1; + } + if touched == 2 { + done = true; + break; + } + } + done + } + + Mutation::NameAMissingGroup => { + if let Some((_, peerings)) = agent.spec.peerings.iter_mut().flatten().next() { + peerings.gateway_group = Some("no-such-group".to_string()); + true + } else { + false + } + } + + Mutation::NameAStrangerInARule => { + let mut done = false; + for (_, peerings) in agent.spec.peerings.iter_mut().flatten() { + let Some(acl) = peerings.acl.as_mut() else { + continue; + }; + if let Some(rule) = acl.rules.iter_mut().flatten().next() { + rule.from = Some("not-in-this-peering".to_string()); + done = true; + break; + } + } + done + } + + Mutation::DemandFlowScope => { + let mut done = false; + for (_, peerings) in agent.spec.peerings.iter_mut().flatten() { + let Some(acl) = peerings.acl.as_mut() else { + continue; + }; + for rule in acl.rules.iter_mut().flatten() { + rule.scope = Some(GatewayAgentPeeringsAclRulesScope::Flow); + done = true; + } + if done { + break; + } + } + done + } + + Mutation::UsePortZero => { + let mut done = false; + for expose in exposes_mut(agent) { + let Some(nat) = expose.nat.as_mut() else { + continue; + }; + let Some(pf) = nat.port_forward.as_mut() else { + continue; + }; + if let Some(ports) = pf.ports.iter_mut().flatten().next() { + ports.port = Some("0-100".to_string()); + ports.r#as = Some("0-100".to_string()); + done = true; + break; + } + } + done + } + }; + Some(bit) +} + +#[derive(Debug, Clone, Default)] +pub struct MutatedAgents(GatewayAgents); + +impl MutatedAgents { + #[must_use] + pub fn new(agents: GatewayAgents) -> Self { + Self(agents) + } +} + +impl ValueGenerator for MutatedAgents { + type Output = (Mutation, bool, GatewayAgent); + + fn generate(&self, d: &mut D) -> Option { + let mut agent = self.0.generate(d)?; + let all = Mutation::all(); + let mutation = all[d.gen_usize(Bound::Included(&0), Bound::Excluded(&all.len()))?]; + let bit = apply(d, &mut agent, mutation)?; + Some((mutation, bit, agent)) + } +} diff --git a/mgmt/src/tests/mgmt.rs b/mgmt/src/tests/mgmt.rs index 6a8d230018..6ea92f3f6e 100644 --- a/mgmt/src/tests/mgmt.rs +++ b/mgmt/src/tests/mgmt.rs @@ -784,3 +784,120 @@ mod dataplane_tables { drive(NatFlavour::None); } } + +#[cfg(test)] +mod validator_completeness { + use concurrency::sync::atomic::{AtomicUsize, Ordering}; + use config::{ConfigError, ExternalConfig, ValidatedGwConfig}; + use flow_entry::flow_table::FlowTable; + use k8s_intf::bolero::mutate::{MutatedAgents, Mutation}; + use k8s_intf::gateway_agent_crd::GatewayAgent; + use nat::masquerade::{MasqueradeConfig, NatAllocatorWriter}; + use nat::portfw::{PortFwTableWriter, build_port_forwarding_configuration}; + use nat::static_nat::NatTablesWriter; + use nat::static_nat::setup::build_nat_configuration; + use routing::Render; + + use crate::processor::confbuild::internal::build_internal_config; + + fn validator(crd: &GatewayAgent) -> Result { + let external = ExternalConfig::try_from(crd) + .map_err(|e| ConfigError::Invalid(format!("conversion: {e}")))?; + external.validate() + } + + fn enact(validated: &ValidatedGwConfig, mutation: Mutation) { + let genid = validated.genid(); + + let internal = build_internal_config(validated, None).unwrap_or_else(|e| { + panic!("{mutation:?}: validator accepted a config the builder rejects: {e}") + }); + let _ = internal.render(&genid).to_string(); + + let vpc_table = validated.external().overlay().vpc_table(); + + let nat_tables = build_nat_configuration(vpc_table).unwrap_or_else(|e| { + panic!("{mutation:?}: validator accepted a config static NAT rejects: {e}") + }); + NatTablesWriter::new().update_nat_tables(nat_tables); + + let masquerade = MasqueradeConfig::new(vpc_table, genid).set_randomize(false); + NatAllocatorWriter::new().update_nat_allocator(masquerade, &FlowTable::new(16)); + + let ruleset = build_port_forwarding_configuration(vpc_table).unwrap_or_else(|e| { + panic!("{mutation:?}: validator accepted a config port forwarding rejects: {e}") + }); + PortFwTableWriter::new() + .update_table(&ruleset) + .unwrap_or_else(|e| { + panic!("{mutation:?}: validator accepted a ruleset the table rejects: {e}") + }); + } + + #[test] + fn whatever_the_validator_accepts_can_be_enacted() { + const N: usize = Mutation::COUNT; + #[allow(clippy::declare_interior_mutable_const)] + const ZERO: AtomicUsize = AtomicUsize::new(0); + static DRAWN: [AtomicUsize; N] = [ZERO; N]; + static APPLIED: [AtomicUsize; N] = [ZERO; N]; + static REFUSED: [AtomicUsize; N] = [ZERO; N]; + + bolero::check!() + .with_generator(MutatedAgents::default()) + .cloned() + .for_each(|(mutation, bit, agent): (Mutation, bool, GatewayAgent)| { + let outcome = validator(&agent); + let accepted = outcome.is_ok(); + + if let Ok(validated) = &outcome { + enact(validated, mutation); + } else if let Err(e) = &outcome { + assert!( + !matches!(e, ConfigError::InternalFailure(_)), + "{mutation:?}: rejected with an internal failure, which tells the user \ + nothing they can act on: {e}" + ); + } + + let slot = mutation.index(); + DRAWN[slot].fetch_add(1, Ordering::Relaxed); + if bit { + APPLIED[slot].fetch_add(1, Ordering::Relaxed); + } + if !accepted { + REFUSED[slot].fetch_add(1, Ordering::Relaxed); + } + }); + + let mut total_applied = 0; + let mut total_refused = 0; + for mutation in Mutation::all() { + let slot = mutation.index(); + let drawn = DRAWN[slot].load(Ordering::Relaxed); + let applied = APPLIED[slot].load(Ordering::Relaxed); + let refused = REFUSED[slot].load(Ordering::Relaxed); + println!("{mutation:<32?} {drawn:>7} drawn {applied:>7} applied {refused:>7} refused"); + assert!(drawn > 0, "{mutation:?} was never drawn"); + if mutation != Mutation::None { + total_applied += applied; + total_refused += refused; + } + } + + let control = Mutation::None.index(); + let drawn = DRAWN[control].load(Ordering::Relaxed); + let refused = REFUSED[control].load(Ordering::Relaxed); + assert!( + refused * 4 <= drawn, + "the unmutated control was refused {refused} times in {drawn}: the near-miss generator \ + is not starting from legal configurations" + ); + + assert!( + total_applied > 0 && total_refused * 2 >= total_applied, + "only {total_refused} of {total_applied} applied mutations were refused: the near-miss \ + generator is mostly producing legal configurations" + ); + } +} From 229ad768e10b28502b05f541926ce1b00b253ae6 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 19:46:25 -0600 Subject: [PATCH 12/23] test(mgmt): Let a fuzzing engine drive the near-miss property The generator-health assertions -- every mutation was drawn, the control is rarely refused, an applied mutation is usually refused -- are now `#[cfg(not(fuzzing))]`. They describe the *distribution* of the inputs, which is the random engine's contract. A coverage-guided engine deliberately skews that distribution: libfuzzer keeps a corpus and steers toward inputs that reach new code, so it will happily spend a run replaying one mutation ten thousand times. That is the right behaviour for finding a gap, and fatal to a check that every mutation gets drawn. The property itself -- whatever the validator accepts, the dataplane can enact -- is what a fuzzer is here to break, and it runs under both engines. `cargo bolero` sets `--cfg fuzzing` for every engine it drives, so that cfg is exactly the right question to ask; registered in `[lints.rust]` following `id`'s precedent for `cfg(kani)`. With this the property runs under libfuzzer: just sanitize=NONE fuzz \ tests::mgmt::validator_completeness::whatever_the_validator_accepts_can_be_enacted \ 1800s -p dataplane-mgmt -j 60 -E=-workers=60 Note the `-E`: `-j 60` alone gives 32 workers, because libFuzzer defaults `-workers` to `ncores/2` and only `-jobs` follows `-j`. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- mgmt/Cargo.toml | 3 +++ mgmt/src/tests/mgmt.rs | 23 ++++++++++++++++++----- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/mgmt/Cargo.toml b/mgmt/Cargo.toml index da679f40b4..366466fa0f 100644 --- a/mgmt/Cargo.toml +++ b/mgmt/Cargo.toml @@ -55,6 +55,9 @@ tokio = { workspace = true, features = ["macros", "rt", "sync", "time"] } tracing = { workspace = true, features = ["attributes"] } tracing-test = { workspace = true } +[lints.rust] +unexpected_cfgs = { level = "warn", check-cfg = ['cfg(fuzzing)'] } + [dev-dependencies] # internal dpdk = { workspace = true, features = ["test"] } # EAL for tests that build the rte_acl-backed ACL filter and flow-filter context diff --git a/mgmt/src/tests/mgmt.rs b/mgmt/src/tests/mgmt.rs index 6ea92f3f6e..1a60a5d491 100644 --- a/mgmt/src/tests/mgmt.rs +++ b/mgmt/src/tests/mgmt.rs @@ -870,13 +870,26 @@ mod validator_completeness { } }); + #[cfg(fuzzing)] + println!("under a coverage-guided engine: skipping the generator-health checks"); + + #[cfg(not(fuzzing))] + check_generator_health(&DRAWN, &APPLIED, &REFUSED); + } + + #[cfg(not(fuzzing))] + fn check_generator_health( + drawn_at: &[AtomicUsize; Mutation::COUNT], + applied_at: &[AtomicUsize; Mutation::COUNT], + refused_at: &[AtomicUsize; Mutation::COUNT], + ) { let mut total_applied = 0; let mut total_refused = 0; for mutation in Mutation::all() { let slot = mutation.index(); - let drawn = DRAWN[slot].load(Ordering::Relaxed); - let applied = APPLIED[slot].load(Ordering::Relaxed); - let refused = REFUSED[slot].load(Ordering::Relaxed); + let drawn = drawn_at[slot].load(Ordering::Relaxed); + let applied = applied_at[slot].load(Ordering::Relaxed); + let refused = refused_at[slot].load(Ordering::Relaxed); println!("{mutation:<32?} {drawn:>7} drawn {applied:>7} applied {refused:>7} refused"); assert!(drawn > 0, "{mutation:?} was never drawn"); if mutation != Mutation::None { @@ -886,8 +899,8 @@ mod validator_completeness { } let control = Mutation::None.index(); - let drawn = DRAWN[control].load(Ordering::Relaxed); - let refused = REFUSED[control].load(Ordering::Relaxed); + let drawn = drawn_at[control].load(Ordering::Relaxed); + let refused = refused_at[control].load(Ordering::Relaxed); assert!( refused * 4 <= drawn, "the unmutated control was refused {refused} times in {drawn}: the near-miss generator \ From e2821d0065dffd1607d13225cfbf532146cd0e93 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 19:46:42 -0600 Subject: [PATCH 13/23] test(k8s-intf): Draw prefixes from slots so exposes cannot overlap The near-miss property's control is an *unmutated* configuration: legal by construction, and expected to validate. Under uniform random input it was refused 6% of the time. Replaying libfuzzer's corpus, 22.5% -- and 89% of those rejections were a single error, `VPC prefixes overlap`. That gap between the two numbers is the whole point of running a coverage-guided engine. A generator flaw that shows up in one random draw in a thousand looks like noise. A fuzzer finds it, saves the input, and mutates around it, because "the validator rejects this" is new code and new code is what it is hunting. **The corpus is a map of the generator's blind spots**, and reading it off is cheaper than reasoning about where the generator might be weak. The flaw: every expose of a manifest drew its prefixes from one shared block, so whether two of them overlapped was a matter of chance. `validate_expose_collisions` refuses that for most pairs of NAT modes. Overlap is broken by *sharing an address range*, so the fix is to make sharing impossible rather than unlikely. Each prefix is confined to a nested box, and two prefixes in different boxes cannot overlap however long they are: * **block** -- private or public. Already there; keeps an expose's two sides from being the same prefix. * **slot** -- one per expose of a manifest. * **sub-slot** -- one per prefix of an expose's own list, since a private list may hold several and those have to be disjoint from each other too. `MIN_V4_LEN` goes 16 -> 20 to make room: `172.16.0.0/12` holds 256 slots of /20, which a `u8` index cannot exceed. v6 keeps /48, which leaves 32,768. Two consequences fall out of the same rule. A vpc's subnets get a reserved region at the bottom of each private block, because a *named* subnet contributes its prefix just as surely as a written-out one does, so it must not land in a slot an expose draws from -- and the subnets are dealt out round-robin, since a subnet named by two exposes of one manifest is a prefix those two exposes share. And `VpcGenerator` now draws the subnet count *before* the mask length: the other order lets the region run short at that length, and `private_run` would wrap and hand back the same prefix twice, which is two overlapping subnets. The control's rejection rate falls from 6% to 2.3%. The residual is not explained yet. It is still `VPC prefixes overlap`, but the offending prefixes do not appear literally in the CRD -- a `/51` reported against a `/91` that *is* in the input, where no `/51` is. So it comes from the converter's own output: either the post-exclusion decomposition, since subtracting a `not` from a prefix yields a fan of longer ones, or `collapse_prefixes`. Worth picking up separately, because if the converter can manufacture an overlap the input did not have, that is a question about the converter. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- config/src/converters/k8s/config/expose.rs | 2 +- k8s-intf/src/bolero/expose.rs | 65 +++++++++---- k8s-intf/src/bolero/peering.rs | 7 +- k8s-intf/src/bolero/support.rs | 108 +++++++++++++++++---- k8s-intf/src/bolero/vpc.rs | 11 ++- 5 files changed, 148 insertions(+), 45 deletions(-) diff --git a/config/src/converters/k8s/config/expose.rs b/config/src/converters/k8s/config/expose.rs index f6dd7708f3..2861b37192 100644 --- a/config/src/converters/k8s/config/expose.rs +++ b/config/src/converters/k8s/config/expose.rs @@ -491,7 +491,7 @@ mod test { "10.0.4.0/24".parse::().unwrap(), ), ]); - let expose_gen = k8s_intf::bolero::expose::AnyExposeGenerator::new(&subnets); + let expose_gen = k8s_intf::bolero::expose::AnyExposeGenerator::new(0, &subnets); bolero::check!() .with_generator(expose_gen) .for_each(|k8s_expose| { diff --git a/k8s-intf/src/bolero/expose.rs b/k8s-intf/src/bolero/expose.rs index d2f7176676..1bb366121a 100644 --- a/k8s-intf/src/bolero/expose.rs +++ b/k8s-intf/src/bolero/expose.rs @@ -26,22 +26,32 @@ const MAX_PORTS: u16 = 1024; pub struct ExposeGenerator<'a> { flavour: NatFlavour, family: AddressFamily, + slot: u8, + slots: u8, subnets: &'a SubnetMap, } impl<'a> ExposeGenerator<'a> { #[must_use] - pub fn new(flavour: NatFlavour, family: AddressFamily, subnets: &'a SubnetMap) -> Self { + pub fn new( + flavour: NatFlavour, + family: AddressFamily, + slot: u8, + slots: u8, + subnets: &'a SubnetMap, + ) -> Self { Self { flavour, family, + slot, + slots: slots.max(slot.saturating_add(1)), subnets, } } - fn length(&self, d: &mut D) -> Option { + fn length(&self, d: &mut D, at: blocks::At) -> Option { d.gen_u8( - Bound::Included(&blocks::min_len(self.family)), + Bound::Included(&blocks::min_len_at(self.family, at)), Bound::Included(&blocks::max_len(self.family)), ) } @@ -51,10 +61,19 @@ impl<'a> ExposeGenerator<'a> { .iter() .filter(|(_, prefix)| prefix.is_ipv4() == self.family.is_v4()) .map(|(name, _)| name) + .enumerate() + .filter(|(index, _)| index % usize::from(self.slots) == usize::from(self.slot)) + .map(|(_, name)| name) .collect() } - fn exclusion(&self, d: &mut D, parent: &str, private: bool) -> Option { + fn exclusion( + &self, + d: &mut D, + parent: &str, + at: blocks::At, + private: bool, + ) -> Option { let (_, len) = parent.split_once('/')?; let len: u8 = len.parse().ok()?; let max = blocks::max_len(self.family); @@ -63,9 +82,9 @@ impl<'a> ExposeGenerator<'a> { } let longer = d.gen_u8(Bound::Excluded(&len), Bound::Included(&max))?; if private { - blocks::private(d, self.family, longer) + blocks::private(d, self.family, at, longer) } else { - blocks::public(d, self.family, longer) + blocks::public(d, self.family, at, longer) } } @@ -133,9 +152,10 @@ impl ValueGenerator for ExposeGenerator<'_> { let mut translations = Vec::new(); if paired { - let len = self.length(d)?; - let private = blocks::private(d, self.family, len)?; - let public = blocks::public(d, self.family, len)?; + let at = blocks::At::whole(self.slot); + let len = self.length(d, at)?; + let private = blocks::private(d, self.family, at, len)?; + let public = blocks::public(d, self.family, at, len)?; ips.push(GatewayAgentPeeringsPeeringExposeIps { cidr: Some(private), not: None, @@ -147,20 +167,22 @@ impl ValueGenerator for ExposeGenerator<'_> { }); } else { let count = d.gen_u8(Bound::Included(&1), Bound::Included(&MAX_PREFIXES))?; - for _ in 0..count { - let len = self.length(d)?; + for sub in 0..count { + let at = blocks::At::nth(self.slot, sub, count); + let len = self.length(d, at)?; ips.push(GatewayAgentPeeringsPeeringExposeIps { - cidr: Some(blocks::private(d, self.family, len)?), + cidr: Some(blocks::private(d, self.family, at, len)?), not: None, vpc_subnet: None, }); } if self.flavour.needs_translation() { let count = d.gen_u8(Bound::Included(&1), Bound::Included(&MAX_PREFIXES))?; - for _ in 0..count { - let len = self.length(d)?; + for sub in 0..count { + let at = blocks::At::nth(self.slot, sub, count); + let len = self.length(d, at)?; translations.push(GatewayAgentPeeringsPeeringExposeAs { - cidr: Some(blocks::public(d, self.family, len)?), + cidr: Some(blocks::public(d, self.family, at, len)?), not: None, }); } @@ -181,8 +203,9 @@ impl ValueGenerator for ExposeGenerator<'_> { if self.flavour.allows_exclusions() && d.produce::()? { let parents: Vec = ips.iter().filter_map(|e| e.cidr.clone()).collect(); + let first = blocks::At::nth(self.slot, 0, u8::try_from(parents.len()).unwrap_or(1)); if let Some(parent) = parents.first() - && let Some(exclusion) = self.exclusion(d, parent, true) + && let Some(exclusion) = self.exclusion(d, parent, first, true) { ips.push(GatewayAgentPeeringsPeeringExposeIps { cidr: None, @@ -191,8 +214,9 @@ impl ValueGenerator for ExposeGenerator<'_> { }); } let parents: Vec = translations.iter().filter_map(|e| e.cidr.clone()).collect(); + let first = blocks::At::nth(self.slot, 0, u8::try_from(parents.len()).unwrap_or(1)); if let Some(parent) = parents.first() - && let Some(exclusion) = self.exclusion(d, parent, false) + && let Some(exclusion) = self.exclusion(d, parent, first, false) { translations.push(GatewayAgentPeeringsPeeringExposeAs { cidr: None, @@ -216,13 +240,14 @@ impl ValueGenerator for ExposeGenerator<'_> { #[derive(Debug, Clone)] pub struct AnyExposeGenerator<'a> { + slot: u8, subnets: &'a SubnetMap, } impl<'a> AnyExposeGenerator<'a> { #[must_use] - pub fn new(subnets: &'a SubnetMap) -> Self { - Self { subnets } + pub fn new(slot: u8, subnets: &'a SubnetMap) -> Self { + Self { slot, subnets } } } @@ -236,6 +261,6 @@ impl ValueGenerator for AnyExposeGenerator<'_> { flavours[d.gen_usize(Bound::Included(&0), Bound::Excluded(&flavours.len()))?]; let family = families[d.gen_usize(Bound::Included(&0), Bound::Excluded(&families.len()))?]; - ExposeGenerator::new(flavour, family, self.subnets).generate(d) + ExposeGenerator::new(flavour, family, self.slot, 1, self.subnets).generate(d) } } diff --git a/k8s-intf/src/bolero/peering.rs b/k8s-intf/src/bolero/peering.rs index 90b6cd5e7d..70ca154eb4 100644 --- a/k8s-intf/src/bolero/peering.rs +++ b/k8s-intf/src/bolero/peering.rs @@ -45,10 +45,13 @@ impl ValueGenerator for LegalValuePeeringsPeeringGenerator<'_> { fn generate(&self, d: &mut D) -> Option { let num_expose = d.gen_u8(Bound::Included(&1), Bound::Included(&self.max_exposes))?; let mut expose = Vec::with_capacity(usize::from(num_expose)); - for _ in 0..num_expose { + for slot in 0..num_expose { let flavour = self.flavours [d.gen_usize(Bound::Included(&0), Bound::Excluded(&self.flavours.len()))?]; - expose.push(ExposeGenerator::new(flavour, self.family, self.subnets).generate(d)?); + expose.push( + ExposeGenerator::new(flavour, self.family, slot, num_expose, self.subnets) + .generate(d)?, + ); } Some(GatewayAgentPeeringsPeering { diff --git a/k8s-intf/src/bolero/support.rs b/k8s-intf/src/bolero/support.rs index 2d21248e89..513cc39b9e 100644 --- a/k8s-intf/src/bolero/support.rs +++ b/k8s-intf/src/bolero/support.rs @@ -429,8 +429,68 @@ pub mod blocks { use bolero::Driver; use std::net::{Ipv4Addr, Ipv6Addr}; - pub const MIN_V4_LEN: u8 = 16; - pub const MIN_V6_LEN: u8 = 48; + pub const SLOT_V4_LEN: u8 = 20; + pub const SLOT_V6_LEN: u8 = 48; + pub const MIN_V4_LEN: u8 = SLOT_V4_LEN; + pub const MIN_V6_LEN: u8 = SLOT_V6_LEN; + + pub const SUBNET_SLOTS: u8 = 16; + + fn subnet_region_len(family: AddressFamily) -> u8 { + let exponent = u8::try_from(SUBNET_SLOTS.trailing_zeros()).unwrap_or(0); + min_len(family) - exponent + } + + #[derive(Debug, Clone, Copy)] + pub struct At { + pub slot: u8, + pub sub: u8, + pub subs: u8, + } + + impl At { + #[must_use] + pub fn whole(slot: u8) -> Self { + Self { + slot, + sub: 0, + subs: 1, + } + } + + #[must_use] + pub fn nth(slot: u8, sub: u8, subs: u8) -> Self { + Self { + slot, + sub, + subs: subs.max(sub.saturating_add(1)), + } + } + + fn sub_bits(self) -> u8 { + u8::try_from(self.subs.max(1).next_power_of_two().trailing_zeros()).unwrap_or(0) + } + + fn level(self, family: AddressFamily) -> u8 { + min_len(family) + .saturating_add(self.sub_bits()) + .min(max_len(family)) + } + + fn place(self, family: AddressFamily, block_base: u128, slot_index: u32) -> (u128, u8) { + let width = max_len(family); + let level = self.level(family); + let slot = u128::from(slot_index) << (width - min_len(family)); + let sub_mask = (1u128 << self.sub_bits()) - 1; + let sub = (u128::from(self.sub) & sub_mask) << (width - level); + (block_base | slot | sub, level) + } + } + + #[must_use] + pub fn min_len_at(family: AddressFamily, at: At) -> u8 { + at.level(family) + } fn v4(base: u32, block_len: u8, host: u32, len: u8) -> String { let block_host_bits = 32 - block_len; @@ -456,32 +516,35 @@ pub mod blocks { format!("{}/{len}", Ipv6Addr::from(addr)) } - pub fn private(d: &mut D, family: AddressFamily, len: u8) -> Option { + pub fn private(d: &mut D, family: AddressFamily, at: At, len: u8) -> Option { + let slot = u32::from(SUBNET_SLOTS) + u32::from(at.slot); Some(if family.is_v4() { - v4(0x0A00_0000, 8, d.produce::()?, len) + let (base, level) = at.place(family, 0x0A00_0000, slot); + v4(u32::try_from(base).ok()?, level, d.produce::()?, len) } else { - v6( - 0x2001_0db8_0000_0000_0000_0000_0000_0000, - 33, - d.produce::()?, - len, - ) + let (base, level) = at.place(family, 0x2001_0db8_0000_0000_0000_0000_0000_0000, slot); + v6(base, level, d.produce::()?, len) }) } - pub fn public(d: &mut D, family: AddressFamily, len: u8) -> Option { + pub fn public(d: &mut D, family: AddressFamily, at: At, len: u8) -> Option { + let slot = u32::from(at.slot); Some(if family.is_v4() { - v4(0xAC10_0000, 12, d.produce::()?, len) + let (base, level) = at.place(family, 0xAC10_0000, slot); + v4(u32::try_from(base).ok()?, level, d.produce::()?, len) } else { - v6( - 0x2001_0db8_8000_0000_0000_0000_0000_0000, - 33, - d.produce::()?, - len, - ) + let (base, level) = at.place(family, 0x2001_0db8_8000_0000_0000_0000_0000_0000, slot); + v6(base, level, d.produce::()?, len) }) } + #[must_use] + pub fn min_subnet_len(family: AddressFamily, count: u16) -> u8 { + let region = subnet_region_len(family); + let bits = u8::try_from(count.next_power_of_two().trailing_zeros()).unwrap_or(u8::MAX); + region.saturating_add(bits).min(max_len(family)) + } + pub fn private_run( d: &mut D, family: AddressFamily, @@ -491,9 +554,12 @@ pub mod blocks { if count == 0 { return Some(Vec::new()); } + let region = u32::from(subnet_region_len(family)); let mut out = Vec::with_capacity(usize::from(count)); if family.is_v4() { - let slots = 1u32.checked_shl(u32::from(len) - 8).unwrap_or(u32::MAX); + let slots = 1u32 + .checked_shl(u32::from(len) - region) + .unwrap_or(u32::MAX); let first = d.produce::()? % slots; let shift = u32::from(32 - len); for i in 0..u32::from(count) { @@ -502,7 +568,9 @@ pub mod blocks { out.push(format!("{}/{len}", Ipv4Addr::from(addr))); } } else { - let slots = 1u128.checked_shl(u32::from(len) - 33).unwrap_or(u128::MAX); + let slots = 1u128 + .checked_shl(u32::from(len) - region) + .unwrap_or(u128::MAX); let first = d.produce::()? % slots; let shift = u32::from(128 - len); for i in 0..u128::from(count) { diff --git a/k8s-intf/src/bolero/vpc.rs b/k8s-intf/src/bolero/vpc.rs index c03ac81b36..29b70ed32a 100644 --- a/k8s-intf/src/bolero/vpc.rs +++ b/k8s-intf/src/bolero/vpc.rs @@ -48,8 +48,6 @@ impl ValueGenerator for VpcGenerator<'_> { let internal_id = generate_internal_id(d)?; let vni = d.produce::()?; - let v4_masklen = d.gen_u8(Bound::Included(&blocks::MIN_V4_LEN), Bound::Included(&32))?; - let v6_masklen = d.gen_u8(Bound::Included(&blocks::MIN_V6_LEN), Bound::Included(&128))?; let num_v4_cidrs = if self.wants(AddressFamily::V4) { u16::from(d.gen_u8(Bound::Included(&0), Bound::Included(&self.max_subnets))?) } else { @@ -61,6 +59,15 @@ impl ValueGenerator for VpcGenerator<'_> { 0 }; + let v4_masklen = d.gen_u8( + Bound::Included(&blocks::min_subnet_len(AddressFamily::V4, num_v4_cidrs)), + Bound::Included(&32), + )?; + let v6_masklen = d.gen_u8( + Bound::Included(&blocks::min_subnet_len(AddressFamily::V6, num_v6_cidrs)), + Bound::Included(&128), + )?; + let subnets_cidrs = vec![ blocks::private_run(d, AddressFamily::V4, v4_masklen, num_v4_cidrs)?, blocks::private_run(d, AddressFamily::V6, v6_masklen, num_v6_cidrs)?, From 8d808c8c45d43a8144de3b7c4b3efb862915c5b0 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 21:10:35 -0600 Subject: [PATCH 14/23] fix(k8s-intf): Give every vpc its own slots, and assert the control validates Two changes that belong together: the scope the slot scheme has to have, and the assertion whose absence made that expensive to find. The near-miss property *counted* rejections of its unmutated control and checked the rate stayed under 25%. It sat at 6%, which reads as tolerable noise. It was not noise, and a tally is a terrible instrument for finding out why: it says a rate and nothing about which configuration or which prefix. Asserted instead -- an unmutated configuration must validate -- bolero shrinks the failure, and the counterexample is one expose: ips: [ cidr 10.1.0.0/20, not 10.1.0.0/21 ] in two manifests, plus the error naming `10.1.8.0/21` twice. `10.1.0.0/20` minus `10.1.0.0/21` *is* `10.1.8.0/21`, and it appeared twice because two peers of one vpc both exposed it. If a thing must hold, assert it, and let the shrinker do the reading. The rule is not what a per-manifest scheme expresses. Keeping the exposes of a *manifest* apart is not enough. `VpcRouteTable::build` is per vpc, over the exposes its **peers** advertise to it, and `validate` refuses overlap among them -- because a vpc with one destination and two places to send it is ambiguous. So prefixes must be disjoint **across vpcs**, not merely within a manifest, and slot 0 belonged to every vpc at once. The vpc becomes the outermost level of the scheme: `blocks::expose_slot(vpc, slots_per_vpc, expose)`, and each vpc's subnets get a slot of their own rather than sharing one region. `pairs()` and `generate_for` now deal in indices, since a vpc's *position* is what decides its slots. The control's rejection rate goes from 2.3% to none observed. The rule now has a mutation of its own. `OverlapWithAnotherPeer` breaks it deliberately, and about one in six of the cases it builds is legitimately legal, because `can_overlap` permits masqueraded and default routes to overlap within one gateway group. Worth having, because until now this validator path was reached *only* by the generator's accident, and fixing the accident would have left it untested. It also exposes an edge of the enactability property. Delete the `OverlappingPrefixes` check and "whatever validates, builds" still passes: two routes to one destination build fine and the dataplane picks one. That rule is about *ambiguity*, not *feasibility*, so this property structurally cannot police it -- a gap in the property, not in the validator. Policing it needs a companion property, "a mutation that breaks a rule must be refused", which needs each mutation to say whether the case it built is certainly illegal. Recorded at both sites. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- config/src/converters/k8s/config/peering.rs | 2 +- k8s-intf/src/bolero/expose.rs | 61 +++++++++++---- k8s-intf/src/bolero/mutate.rs | 83 ++++++++++++++++++++- k8s-intf/src/bolero/peering.rs | 47 ++++++------ k8s-intf/src/bolero/spec.rs | 8 +- k8s-intf/src/bolero/support.rs | 26 ++++--- k8s-intf/src/bolero/vpc.rs | 10 ++- mgmt/src/tests/mgmt.rs | 15 ++-- 8 files changed, 187 insertions(+), 65 deletions(-) diff --git a/config/src/converters/k8s/config/peering.rs b/config/src/converters/k8s/config/peering.rs index 956bf7a783..dc82803ab5 100644 --- a/config/src/converters/k8s/config/peering.rs +++ b/config/src/converters/k8s/config/peering.rs @@ -101,7 +101,7 @@ mod test { let subnets = SubnetMap::new(); // Let this be empty since we are test subnet conversion elsewhere let flavours = NatFlavour::all(); let generator = - LegalValuePeeringsPeeringGenerator::new(&subnets, &flavours, AddressFamily::V4, 3); + LegalValuePeeringsPeeringGenerator::new(&subnets, &flavours, AddressFamily::V4, 3, 0); bolero::check!() .with_generator(generator) .for_each(|peering| { diff --git a/k8s-intf/src/bolero/expose.rs b/k8s-intf/src/bolero/expose.rs index 1bb366121a..f5e7436e2a 100644 --- a/k8s-intf/src/bolero/expose.rs +++ b/k8s-intf/src/bolero/expose.rs @@ -26,25 +26,49 @@ const MAX_PORTS: u16 = 1024; pub struct ExposeGenerator<'a> { flavour: NatFlavour, family: AddressFamily, - slot: u8, - slots: u8, + which: Which, subnets: &'a SubnetMap, } +#[derive(Debug, Clone, Copy)] +pub struct Which { + pub slot: u8, + pub index: u8, + pub count: u8, +} + +impl Which { + #[must_use] + pub fn only(slot: u8) -> Self { + Self { + slot, + index: 0, + count: 1, + } + } + + #[must_use] + pub fn nth(slot: u8, index: u8, count: u8) -> Self { + Self { + slot, + index, + count: count.max(index.saturating_add(1)), + } + } +} + impl<'a> ExposeGenerator<'a> { #[must_use] pub fn new( flavour: NatFlavour, family: AddressFamily, - slot: u8, - slots: u8, + which: Which, subnets: &'a SubnetMap, ) -> Self { Self { flavour, family, - slot, - slots: slots.max(slot.saturating_add(1)), + which, subnets, } } @@ -62,7 +86,9 @@ impl<'a> ExposeGenerator<'a> { .filter(|(_, prefix)| prefix.is_ipv4() == self.family.is_v4()) .map(|(name, _)| name) .enumerate() - .filter(|(index, _)| index % usize::from(self.slots) == usize::from(self.slot)) + .filter(|(index, _)| { + index % usize::from(self.which.count) == usize::from(self.which.index) + }) .map(|(_, name)| name) .collect() } @@ -152,7 +178,7 @@ impl ValueGenerator for ExposeGenerator<'_> { let mut translations = Vec::new(); if paired { - let at = blocks::At::whole(self.slot); + let at = blocks::At::whole(self.which.slot); let len = self.length(d, at)?; let private = blocks::private(d, self.family, at, len)?; let public = blocks::public(d, self.family, at, len)?; @@ -168,7 +194,7 @@ impl ValueGenerator for ExposeGenerator<'_> { } else { let count = d.gen_u8(Bound::Included(&1), Bound::Included(&MAX_PREFIXES))?; for sub in 0..count { - let at = blocks::At::nth(self.slot, sub, count); + let at = blocks::At::nth(self.which.slot, sub, count); let len = self.length(d, at)?; ips.push(GatewayAgentPeeringsPeeringExposeIps { cidr: Some(blocks::private(d, self.family, at, len)?), @@ -179,7 +205,7 @@ impl ValueGenerator for ExposeGenerator<'_> { if self.flavour.needs_translation() { let count = d.gen_u8(Bound::Included(&1), Bound::Included(&MAX_PREFIXES))?; for sub in 0..count { - let at = blocks::At::nth(self.slot, sub, count); + let at = blocks::At::nth(self.which.slot, sub, count); let len = self.length(d, at)?; translations.push(GatewayAgentPeeringsPeeringExposeAs { cidr: Some(blocks::public(d, self.family, at, len)?), @@ -203,7 +229,8 @@ impl ValueGenerator for ExposeGenerator<'_> { if self.flavour.allows_exclusions() && d.produce::()? { let parents: Vec = ips.iter().filter_map(|e| e.cidr.clone()).collect(); - let first = blocks::At::nth(self.slot, 0, u8::try_from(parents.len()).unwrap_or(1)); + let first = + blocks::At::nth(self.which.slot, 0, u8::try_from(parents.len()).unwrap_or(1)); if let Some(parent) = parents.first() && let Some(exclusion) = self.exclusion(d, parent, first, true) { @@ -214,7 +241,8 @@ impl ValueGenerator for ExposeGenerator<'_> { }); } let parents: Vec = translations.iter().filter_map(|e| e.cidr.clone()).collect(); - let first = blocks::At::nth(self.slot, 0, u8::try_from(parents.len()).unwrap_or(1)); + let first = + blocks::At::nth(self.which.slot, 0, u8::try_from(parents.len()).unwrap_or(1)); if let Some(parent) = parents.first() && let Some(exclusion) = self.exclusion(d, parent, first, false) { @@ -240,14 +268,17 @@ impl ValueGenerator for ExposeGenerator<'_> { #[derive(Debug, Clone)] pub struct AnyExposeGenerator<'a> { - slot: u8, + which: Which, subnets: &'a SubnetMap, } impl<'a> AnyExposeGenerator<'a> { #[must_use] pub fn new(slot: u8, subnets: &'a SubnetMap) -> Self { - Self { slot, subnets } + Self { + which: Which::only(slot), + subnets, + } } } @@ -261,6 +292,6 @@ impl ValueGenerator for AnyExposeGenerator<'_> { flavours[d.gen_usize(Bound::Included(&0), Bound::Excluded(&flavours.len()))?]; let family = families[d.gen_usize(Bound::Included(&0), Bound::Excluded(&families.len()))?]; - ExposeGenerator::new(flavour, family, self.slot, 1, self.subnets).generate(d) + ExposeGenerator::new(flavour, family, self.which, self.subnets).generate(d) } } diff --git a/k8s-intf/src/bolero/mutate.rs b/k8s-intf/src/bolero/mutate.rs index 60c9fb9cfa..a5a16407b9 100644 --- a/k8s-intf/src/bolero/mutate.rs +++ b/k8s-intf/src/bolero/mutate.rs @@ -27,10 +27,11 @@ pub enum Mutation { NameAStrangerInARule, DemandFlowScope, UsePortZero, + OverlapWithAnotherPeer, } impl Mutation { - pub const COUNT: usize = 13; + pub const COUNT: usize = 14; #[must_use] pub fn index(self) -> usize { @@ -56,6 +57,7 @@ impl Mutation { Self::NameAStrangerInARule, Self::DemandFlowScope, Self::UsePortZero, + Self::OverlapWithAnotherPeer, ] } } @@ -316,6 +318,85 @@ pub fn apply(d: &mut D, agent: &mut GatewayAgent, mutation: Mutation) } done } + + Mutation::OverlapWithAnotherPeer => { + let mut donor: Option<(String, String, String)> = None; + for (key, peering) in agent.spec.peerings.iter().flatten() { + let Some(manifests) = peering.peering.as_ref() else { + continue; + }; + for shared in manifests.keys() { + let elsewhere = agent + .spec + .peerings + .iter() + .flatten() + .any(|(other, peering)| { + other != key + && peering + .peering + .as_ref() + .is_some_and(|m| m.contains_key(shared)) + }); + if !elsewhere { + continue; + } + let prefix = manifests + .iter() + .filter(|(name, _)| *name != shared) + .flat_map(|(_, manifest)| manifest.expose.iter().flatten()) + .flat_map(|expose| expose.ips.iter().flatten()) + .find_map(|ip| ip.cidr.clone()); + if let Some(prefix) = prefix { + donor = Some((key.clone(), shared.clone(), prefix)); + break; + } + } + if donor.is_some() { + break; + } + } + + let mut done = false; + if let Some((donor_key, shared, prefix)) = donor { + for (key, peering) in agent.spec.peerings.iter_mut().flatten() { + if *key == donor_key { + continue; + } + let Some(manifests) = peering.peering.as_mut() else { + continue; + }; + if !manifests.contains_key(&shared) { + continue; + } + let names: Vec = manifests + .keys() + .filter(|name| **name != shared) + .cloned() + .collect(); + for name in names { + let Some(manifest) = manifests.get_mut(&name) else { + continue; + }; + if let Some(ip) = manifest + .expose + .iter_mut() + .flatten() + .flat_map(|expose| expose.ips.iter_mut().flatten()) + .find(|ip| ip.cidr.is_some()) + { + ip.cidr = Some(prefix.clone()); + done = true; + break; + } + } + if done { + break; + } + } + } + done + } }; Some(bit) } diff --git a/k8s-intf/src/bolero/peering.rs b/k8s-intf/src/bolero/peering.rs index 70ca154eb4..78bb62f7b9 100644 --- a/k8s-intf/src/bolero/peering.rs +++ b/k8s-intf/src/bolero/peering.rs @@ -7,7 +7,8 @@ use std::ops::Bound; use bolero::{Driver, ValueGenerator}; use crate::bolero::acl::{AclGenerator, SideFacts}; -use crate::bolero::expose::ExposeGenerator; +use crate::bolero::expose::{ExposeGenerator, Which}; +use crate::bolero::support::blocks; use crate::bolero::{AddressFamily, NatFlavour, SubnetMap, VpcSubnetMap}; use crate::gateway_agent_crd::{GatewayAgentPeerings, GatewayAgentPeeringsPeering}; @@ -20,6 +21,7 @@ pub struct LegalValuePeeringsPeeringGenerator<'a> { flavours: &'a [NatFlavour], family: AddressFamily, max_exposes: u8, + slot_base: u8, } impl<'a> LegalValuePeeringsPeeringGenerator<'a> { @@ -29,12 +31,14 @@ impl<'a> LegalValuePeeringsPeeringGenerator<'a> { flavours: &'a [NatFlavour], family: AddressFamily, max_exposes: u8, + vpc: u8, ) -> Self { Self { subnets, flavours, family, max_exposes, + slot_base: blocks::expose_slot(vpc, max_exposes, 0), } } } @@ -45,13 +49,12 @@ impl ValueGenerator for LegalValuePeeringsPeeringGenerator<'_> { fn generate(&self, d: &mut D) -> Option { let num_expose = d.gen_u8(Bound::Included(&1), Bound::Included(&self.max_exposes))?; let mut expose = Vec::with_capacity(usize::from(num_expose)); - for slot in 0..num_expose { + for index in 0..num_expose { let flavour = self.flavours [d.gen_usize(Bound::Included(&0), Bound::Excluded(&self.flavours.len()))?]; - expose.push( - ExposeGenerator::new(flavour, self.family, slot, num_expose, self.subnets) - .generate(d)?, - ); + let which = Which::nth(self.slot_base.saturating_add(index), index, num_expose); + expose + .push(ExposeGenerator::new(flavour, self.family, which, self.subnets).generate(d)?); } Some(GatewayAgentPeeringsPeering { @@ -117,25 +120,25 @@ impl<'a> LegalValuePeeringsGenerator<'a> { } } -fn pick2<'a, D: Driver, T>(d: &mut D, items: &[&'a T]) -> Option<[&'a T; 2]> { - assert!(items.len() >= 2); +fn pick2(d: &mut D, len: usize) -> Option<[usize; 2]> { + assert!(len >= 2); - let index1 = d.gen_usize(Bound::Included(&0), Bound::Excluded(&items.len()))?; - let mut index2 = d.gen_usize(Bound::Included(&0), Bound::Excluded(&items.len()))?; + let index1 = d.gen_usize(Bound::Included(&0), Bound::Excluded(&len))?; + let mut index2 = d.gen_usize(Bound::Included(&0), Bound::Excluded(&len))?; if index1 == index2 { - index2 = (index2 + 1) % items.len(); + index2 = (index2 + 1) % len; } - Some([items[index1], items[index2]]) + Some([index1, index2]) } impl LegalValuePeeringsGenerator<'_> { #[must_use] - pub fn pairs(&self) -> Vec<[&String; 2]> { - let names = &self.vpc_names; - let mut out = Vec::with_capacity(names.len() * names.len() / 2); - for (i, first) in names.iter().enumerate() { - for second in names.iter().skip(i + 1) { - out.push([*first, *second]); + pub fn pairs(&self) -> Vec<[usize; 2]> { + let n = self.vpc_names.len(); + let mut out = Vec::with_capacity(n * n / 2); + for first in 0..n { + for second in (first + 1)..n { + out.push([first, second]); } } out @@ -144,8 +147,9 @@ impl LegalValuePeeringsGenerator<'_> { pub fn generate_for( &self, d: &mut D, - vpc_names: [&String; 2], + vpcs: [usize; 2], ) -> Option { + let vpc_names = [*self.vpc_names.get(vpcs[0])?, *self.vpc_names.get(vpcs[1])?]; let family = self.families [d.gen_usize(Bound::Included(&0), Bound::Excluded(&self.families.len()))?]; @@ -165,6 +169,7 @@ impl LegalValuePeeringsGenerator<'_> { flavours, family, self.max_exposes, + u8::try_from(vpcs[i]).unwrap_or(u8::MAX), ); Some((vpc_names[i].clone(), generator.generate(d)?)) }) @@ -197,7 +202,7 @@ impl ValueGenerator for LegalValuePeeringsGenerator<'_> { type Output = GatewayAgentPeerings; fn generate(&self, d: &mut D) -> Option { - let vpc_names = pick2(d, &self.vpc_names)?; - self.generate_for(d, vpc_names) + let pair = pick2(d, self.vpc_names.len())?; + self.generate_for(d, pair) } } diff --git a/k8s-intf/src/bolero/spec.rs b/k8s-intf/src/bolero/spec.rs index 10aeb406e4..82e8beacec 100644 --- a/k8s-intf/src/bolero/spec.rs +++ b/k8s-intf/src/bolero/spec.rs @@ -153,8 +153,12 @@ impl ValueGenerator for GatewayAgentSpecs { let mut vpc_internal_ids = HashSet::new(); for i in 0..num_vpcs { let vni_offset = u32::try_from(i).expect("too many vpcs"); - let mut vpc = crate::bolero::vpc::VpcGenerator::new(knobs.max_subnets, &knobs.families) - .generate(d)?; + let mut vpc = crate::bolero::vpc::VpcGenerator::new( + u8::try_from(i).unwrap_or(u8::MAX), + knobs.max_subnets, + &knobs.families, + ) + .generate(d)?; let vpc_id = vpc.internal_id.as_mut().unwrap(); while !vpc_internal_ids.insert(vpc_id.clone()) { // We already have a VPC with this internal_id, "increment" the string to generate a diff --git a/k8s-intf/src/bolero/support.rs b/k8s-intf/src/bolero/support.rs index 513cc39b9e..06f570fbd7 100644 --- a/k8s-intf/src/bolero/support.rs +++ b/k8s-intf/src/bolero/support.rs @@ -436,9 +436,9 @@ pub mod blocks { pub const SUBNET_SLOTS: u8 = 16; - fn subnet_region_len(family: AddressFamily) -> u8 { - let exponent = u8::try_from(SUBNET_SLOTS.trailing_zeros()).unwrap_or(0); - min_len(family) - exponent + #[must_use] + pub fn expose_slot(vpc: u8, slots_per_vpc: u8, expose: u8) -> u8 { + vpc.saturating_mul(slots_per_vpc).saturating_add(expose) } #[derive(Debug, Clone, Copy)] @@ -540,43 +540,45 @@ pub mod blocks { #[must_use] pub fn min_subnet_len(family: AddressFamily, count: u16) -> u8 { - let region = subnet_region_len(family); - let bits = u8::try_from(count.next_power_of_two().trailing_zeros()).unwrap_or(u8::MAX); - region.saturating_add(bits).min(max_len(family)) + let bits = u8::try_from(count.max(1).next_power_of_two().trailing_zeros()).unwrap_or(0); + min_len(family).saturating_add(bits).min(max_len(family)) } pub fn private_run( d: &mut D, family: AddressFamily, + vpc: u8, len: u8, count: u16, ) -> Option> { if count == 0 { return Some(Vec::new()); } - let region = u32::from(subnet_region_len(family)); + let slot_len = u32::from(min_len(family)); let mut out = Vec::with_capacity(usize::from(count)); if family.is_v4() { + let base = 0x0A00_0000 | (u32::from(vpc) << (32 - slot_len)); let slots = 1u32 - .checked_shl(u32::from(len) - region) + .checked_shl(u32::from(len) - slot_len) .unwrap_or(u32::MAX); let first = d.produce::()? % slots; let shift = u32::from(32 - len); for i in 0..u32::from(count) { let slot = (first + i) % slots; - let addr = 0x0A00_0000 | slot.checked_shl(shift).unwrap_or(0); + let addr = base | slot.checked_shl(shift).unwrap_or(0); out.push(format!("{}/{len}", Ipv4Addr::from(addr))); } } else { + let base = + 0x2001_0db8_0000_0000_0000_0000_0000_0000 | (u128::from(vpc) << (128 - slot_len)); let slots = 1u128 - .checked_shl(u32::from(len) - region) + .checked_shl(u32::from(len) - slot_len) .unwrap_or(u128::MAX); let first = d.produce::()? % slots; let shift = u32::from(128 - len); for i in 0..u128::from(count) { let slot = (first + i) % slots; - let addr = 0x2001_0db8_0000_0000_0000_0000_0000_0000 - | slot.checked_shl(shift).unwrap_or(0); + let addr = base | slot.checked_shl(shift).unwrap_or(0); out.push(format!("{}/{len}", Ipv6Addr::from(addr))); } } diff --git a/k8s-intf/src/bolero/vpc.rs b/k8s-intf/src/bolero/vpc.rs index 29b70ed32a..eaf5c74fd8 100644 --- a/k8s-intf/src/bolero/vpc.rs +++ b/k8s-intf/src/bolero/vpc.rs @@ -23,14 +23,16 @@ fn generate_internal_id(d: &mut D) -> Option { #[derive(Debug, Clone)] pub struct VpcGenerator<'a> { + vpc: u8, max_subnets: u8, families: &'a [AddressFamily], } impl<'a> VpcGenerator<'a> { #[must_use] - pub fn new(max_subnets: u8, families: &'a [AddressFamily]) -> Self { + pub fn new(vpc: u8, max_subnets: u8, families: &'a [AddressFamily]) -> Self { Self { + vpc, max_subnets, families, } @@ -69,8 +71,8 @@ impl ValueGenerator for VpcGenerator<'_> { )?; let subnets_cidrs = vec![ - blocks::private_run(d, AddressFamily::V4, v4_masklen, num_v4_cidrs)?, - blocks::private_run(d, AddressFamily::V6, v6_masklen, num_v6_cidrs)?, + blocks::private_run(d, AddressFamily::V4, self.vpc, v4_masklen, num_v4_cidrs)?, + blocks::private_run(d, AddressFamily::V6, self.vpc, v6_masklen, num_v6_cidrs)?, ]; let subnets = subnets_cidrs .into_iter() @@ -95,6 +97,6 @@ impl ValueGenerator for VpcGenerator<'_> { impl TypeGenerator for LegalValue { fn generate(d: &mut D) -> Option { let families = AddressFamily::all(); - Some(LegalValue(VpcGenerator::new(3, &families).generate(d)?)) + Some(LegalValue(VpcGenerator::new(0, 3, &families).generate(d)?)) } } diff --git a/mgmt/src/tests/mgmt.rs b/mgmt/src/tests/mgmt.rs index 1a60a5d491..8274d8188b 100644 --- a/mgmt/src/tests/mgmt.rs +++ b/mgmt/src/tests/mgmt.rs @@ -853,6 +853,12 @@ mod validator_completeness { if let Ok(validated) = &outcome { enact(validated, mutation); } else if let Err(e) = &outcome { + assert!( + mutation != Mutation::None, + "an unmutated configuration was refused, so the generator is producing \ + illegal input and every mutated case is suspect: {e}" + ); + assert!( !matches!(e, ConfigError::InternalFailure(_)), "{mutation:?}: rejected with an internal failure, which tells the user \ @@ -898,15 +904,6 @@ mod validator_completeness { } } - let control = Mutation::None.index(); - let drawn = drawn_at[control].load(Ordering::Relaxed); - let refused = refused_at[control].load(Ordering::Relaxed); - assert!( - refused * 4 <= drawn, - "the unmutated control was refused {refused} times in {drawn}: the near-miss generator \ - is not starting from legal configurations" - ); - assert!( total_applied > 0 && total_refused * 2 >= total_applied, "only {total_refused} of {total_applied} applied mutations were refused: the near-miss \ From fb6b57ee8ba20e394425271fd8bd5943fe4bad18 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 21:48:36 -0600 Subject: [PATCH 15/23] test(mgmt): Check that a validated configuration has only one meaning The near-miss property asks whether an accepted configuration can be *enacted*. Its edge on `OverlappingPrefixes` shows the goal has a second half it cannot reach: whether an accepted configuration can be enacted only **one way**. The two failures are nothing alike from where the user stands. An unenactable configuration fails to build and somebody gets an error. An ambiguous one builds perfectly -- two readings, both valid outputs of the code as written -- and the chain takes whichever its containers hand it first. There is no error to report, so nothing reports it. Only traffic going somewhere nobody chose, found much later. A CRD's `expose` list, and the `ips` and `as` lists inside it, are *sets*: their order is not part of what the configuration means. Nor is which name a peering carries, since peering names reach no artifact -- the names in the rendered config come from vpcs. So reordering all of that must leave every artifact the dataplane installs identical. The virtue of permutation as the oracle is that it restates no rule, so it can notice an ambiguity nobody thought to forbid. An ACL's `rules` are deliberately left alone: those are ordered by definition, first match wins, and permuting them would assert something false. Driven by `MutatedAgents`, not by legal configurations alone, and that is the point. The generator now keeps every vpc's prefixes disjoint, so it *cannot* produce an overlapping-route ambiguity by itself; a permutation property fed only clean input would pass without ever meeting the case it exists for -- it would be measuring its own generator. Near-misses put the question where it belongs: when the validator lets a rule slide, is the result still unambiguous? Comparison is over sorted lines, because some of these tables are hash maps whose iteration order is not part of the configuration's meaning. That costs nothing that matters: the artifacts whose order *is* semantic carry their sequence numbers in the text, so reordering them changes the lines themselves. It does not catch run-time ambiguity. With the `OverlappingPrefixes` check gone, so that two peers of one vpc may advertise the same destination, this property stays silent -- and structurally must, because an import prefix-list is rendered per peer, so both routes are installed, in two lists, and the rendered configuration is the same whichever order the peerings are walked. Nothing is silently picked at build time. The picking happens later, in the forwarding plane, on a packet. So the concern splits, and this covers one half: * **build-time** -- one artifact, two possible contents. Covered here. * **run-time** -- one artifact, two rules inside it matching one packet. Not covered by anything, and it is the half that misbehaves in production rather than in a build. Recorded at the property, since the next person to read it should know its edge. The second half needs a check over the installed tables, and it is worth knowing before writing it that for a rule with no downstream consumer such a check is necessarily a second statement of the requirement rather than an independent one. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- k8s-intf/src/bolero/mod.rs | 1 + k8s-intf/src/bolero/permute.rs | 112 +++++++++++++++++++++++++++++++++ mgmt/src/tests/mgmt.rs | 108 +++++++++++++++++++++++++++++++ 3 files changed, 221 insertions(+) create mode 100644 k8s-intf/src/bolero/permute.rs diff --git a/k8s-intf/src/bolero/mod.rs b/k8s-intf/src/bolero/mod.rs index c4fb5c006a..8c29d9547d 100644 --- a/k8s-intf/src/bolero/mod.rs +++ b/k8s-intf/src/bolero/mod.rs @@ -11,6 +11,7 @@ pub mod interface; pub mod logs; pub mod mutate; pub mod peering; +pub mod permute; pub mod spec; pub mod support; pub mod vpc; diff --git a/k8s-intf/src/bolero/permute.rs b/k8s-intf/src/bolero/permute.rs new file mode 100644 index 0000000000..779e411dd1 --- /dev/null +++ b/k8s-intf/src/bolero/permute.rs @@ -0,0 +1,112 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +use std::collections::BTreeMap; +use std::ops::Bound; + +use bolero::{Driver, ValueGenerator}; + +use crate::bolero::mutate::{MutatedAgents, Mutation}; +use crate::gateway_agent_crd::{ + GatewayAgent, GatewayAgentPeerings, GatewayAgentPeeringsPeeringExpose, +}; + +fn reorder(d: &mut D, items: &mut [T]) -> Option<()> { + if items.len() < 2 { + return Some(()); + } + let by = d.gen_usize(Bound::Included(&0), Bound::Excluded(&items.len()))?; + items.rotate_left(by); + let first = d.gen_usize(Bound::Included(&0), Bound::Excluded(&items.len()))?; + let second = d.gen_usize(Bound::Included(&0), Bound::Excluded(&items.len()))?; + items.swap(first, second); + Some(()) +} + +fn reorder_expose( + d: &mut D, + expose: &mut GatewayAgentPeeringsPeeringExpose, +) -> Option<()> { + if let Some(ips) = expose.ips.as_mut() { + reorder(d, ips)?; + } + if let Some(translations) = expose.r#as.as_mut() { + reorder(d, translations)?; + } + if let Some(ports) = expose + .nat + .as_mut() + .and_then(|nat| nat.port_forward.as_mut()) + .and_then(|forward| forward.ports.as_mut()) + { + reorder(d, ports)?; + } + Some(()) +} + +fn reorder_peering(d: &mut D, peering: &mut GatewayAgentPeerings) -> Option<()> { + for manifest in peering.peering.iter_mut().flatten().map(|(_, m)| m) { + if let Some(exposes) = manifest.expose.as_mut() { + reorder(d, exposes)?; + for expose in exposes.iter_mut() { + reorder_expose(d, expose)?; + } + } + } + + for rule in peering + .acl + .iter_mut() + .flat_map(|acl| acl.rules.iter_mut()) + .flatten() + { + let Some(pattern) = rule.r#match.as_mut() else { + continue; + }; + if let Some(src) = pattern.src.as_mut() { + reorder(d, src)?; + } + if let Some(dst) = pattern.dst.as_mut() { + reorder(d, dst)?; + } + } + Some(()) +} + +fn reorder_agent(d: &mut D, agent: &mut GatewayAgent) -> Option { + let before = format!("{:?}", agent.spec.peerings); + + if let Some(peerings) = agent.spec.peerings.as_mut() { + let names: Vec = peerings.keys().cloned().collect(); + let mut bodies: Vec = peerings.values().cloned().collect(); + reorder(d, &mut bodies)?; + *peerings = names.into_iter().zip(bodies).collect::>(); + + for peering in peerings.values_mut() { + reorder_peering(d, peering)?; + } + } + + Some(before != format!("{:?}", agent.spec.peerings)) +} + +#[derive(Debug, Default, Clone)] +pub struct PermutedAgents(MutatedAgents); + +impl PermutedAgents { + #[must_use] + pub fn new(agents: MutatedAgents) -> Self { + Self(agents) + } +} + +impl ValueGenerator for PermutedAgents { + type Output = (Mutation, GatewayAgent, GatewayAgent, bool); + + fn generate(&self, d: &mut D) -> Option { + let (mutation, _applied, agent) = self.0.generate(d)?; + let mut permuted = agent.clone(); + let moved = reorder_agent(d, &mut permuted)?; + Some((mutation, agent, permuted, moved)) + } +} diff --git a/mgmt/src/tests/mgmt.rs b/mgmt/src/tests/mgmt.rs index 8274d8188b..175a944f47 100644 --- a/mgmt/src/tests/mgmt.rs +++ b/mgmt/src/tests/mgmt.rs @@ -911,3 +911,111 @@ mod validator_completeness { ); } } + +mod ambiguity { + use concurrency::sync::atomic::{AtomicUsize, Ordering}; + use config::{ConfigError, ExternalConfig, ValidatedGwConfig}; + use flow_entry::flow_table::FlowTable; + use k8s_intf::bolero::mutate::Mutation; + use k8s_intf::bolero::permute::PermutedAgents; + use k8s_intf::gateway_agent_crd::GatewayAgent; + use nat::masquerade::{MasqueradeConfig, NatAllocatorWriter}; + use nat::portfw::{PortFwTableWriter, build_port_forwarding_configuration}; + use nat::static_nat::setup::build_nat_configuration; + use routing::Render; + + use crate::processor::confbuild::internal::build_internal_config; + + fn validator(crd: &GatewayAgent) -> Result { + let external = ExternalConfig::try_from(crd) + .map_err(|e| ConfigError::Invalid(format!("conversion: {e}")))?; + external.validate() + } + + fn artifacts(validated: &ValidatedGwConfig) -> Option> { + let genid = validated.genid(); + let internal = build_internal_config(validated, None).ok()?; + let vpc_table = validated.external().overlay().vpc_table(); + + let nat_tables = build_nat_configuration(vpc_table).ok()?; + let ruleset = build_port_forwarding_configuration(vpc_table).ok()?; + let mut portfw = PortFwTableWriter::new(); + portfw.update_table(&ruleset).ok()?; + + let masquerade = MasqueradeConfig::new(vpc_table, genid).set_randomize(false); + let mut allocator = NatAllocatorWriter::new(); + allocator.update_nat_allocator(masquerade, &FlowTable::new(16)); + + let mut lines: Vec = + format!("{}\n{nat_tables}\n{ruleset:#?}", internal.render(&genid)) + .lines() + .map(|line| line.trim_end().to_string()) + .filter(|line| !line.is_empty()) + .collect(); + lines.sort(); + Some(lines) + } + + #[test] + fn a_configuration_has_only_one_meaning() { + static MOVED: AtomicUsize = AtomicUsize::new(0); + static COMPARED: AtomicUsize = AtomicUsize::new(0); + + bolero::check!() + .with_generator(PermutedAgents::default()) + .cloned() + .for_each( + |(mutation, agent, permuted, moved): (Mutation, GatewayAgent, GatewayAgent, bool)| { + let Ok(first) = validator(&agent) else { + return; + }; + + let second = validator(&permuted).unwrap_or_else(|e| { + panic!( + "{mutation:?}: reordering a configuration's sets made the validator refuse \ + it, so it is treating list order as meaning: {e}" + ) + }); + + let (Some(before), Some(after)) = (artifacts(&first), artifacts(&second)) else { + return; + }; + + if moved { + MOVED.fetch_add(1, Ordering::Relaxed); + } + COMPARED.fetch_add(1, Ordering::Relaxed); + + if before != after { + let mut differences: Vec = Vec::new(); + for line in &before { + if !after.contains(line) { + differences.push(format!(" only before: {line}")); + } + } + for line in &after { + if !before.contains(line) { + differences.push(format!(" only after: {line}")); + } + } + differences.truncate(20); + panic!( + "{mutation:?}: reordering a configuration's sets changed what the dataplane \ + installs, so the configuration had more than one meaning and the chain \ + picked one:\n{}", + differences.join("\n") + ); + } + }, + ); + + let compared = COMPARED.load(Ordering::Relaxed); + let moved = MOVED.load(Ordering::Relaxed); + println!("{moved} of {compared} comparisons were of a genuinely reordered configuration"); + assert!( + compared > 0 && moved * 10 >= compared, + "only {moved} of {compared} comparisons actually reordered anything: the permutation is \ + not doing any work" + ); + } +} From 79737d8f3f22ec5de13418e200c95ea57f55a220 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 22:15:41 -0600 Subject: [PATCH 16/23] test(config): Assert the validator refuses what a mutation certainly breaks Nothing guards the validator against growing *more permissive* about a rule no downstream builder enforces. "Whatever validates, builds" cannot: the whole point of that class of rule is that the thing builds fine. The guard is the obvious one, and what it needed was not a new property but a stronger contract on the generator: **a mutation now reports `true` only when the result is certainly illegal**, so the near-miss property can assert the validator refuses whatever was touched. Certainty is the generator's job. Two mutations broke rules that have legitimate exceptions, so both now check the exception does not apply before touching anything, rather than producing a case whose legality is arguable: - `DemandFlowScope` skips peerings where a side is stateful throughout, since flow scope is legal there. Mirrors `Acl::validate_scope`, and the two being separate statements of one rule is the point. - `OverlapWithAnotherPeer` copies a prefix only between exposes that advertise their `ips` verbatim -- no translation, no exclusions, not a default. That second condition is narrower than it looks like it needs to be. Route destinations come from `VpcExpose::public_ips`, which is the **translation range** for anything that translates, so excluding masquerade is not enough: a static-NAT expose's `ips` are its private side and never become a route at all, which makes the "overlap" no overlap and the mutation a liar. Exclusions are out for the same reason -- `public_ips` subtracts the `not`s, which can carve away the very prefix copied. Worth noting the shape of that: an assertion about the validator finds a defect in the *generator's model of the validator*. That is the differential working in the direction one does not plan for. The thirteen equalities between applied and refused were previously an observation printed in a tally. They are now enforced per case, and shrinkable. `OverlapWithAnotherPeer` applies to about one draw in forty -- it needs two peerings sharing a vpc and a plain expose on each side. Low, so the new `applied > 0` health check needs a big sample, and the health checks are now tiered by sample size: the default one-second run says what it was too small to check rather than either failing spuriously or looking like it checked. The old ratio assertions are gone, since the per-case assertions are strictly stronger. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- k8s-intf/src/bolero/mutate.rs | 369 +++++++++++++++++++--------------- mgmt/src/tests/mgmt.rs | 52 +++-- 2 files changed, 248 insertions(+), 173 deletions(-) diff --git a/k8s-intf/src/bolero/mutate.rs b/k8s-intf/src/bolero/mutate.rs index a5a16407b9..3c3d7c1321 100644 --- a/k8s-intf/src/bolero/mutate.rs +++ b/k8s-intf/src/bolero/mutate.rs @@ -7,9 +7,9 @@ use bolero::{Driver, ValueGenerator}; use crate::bolero::crd::GatewayAgents; use crate::gateway_agent_crd::{ - GatewayAgent, GatewayAgentPeeringsAclRulesScope, GatewayAgentPeeringsPeeringExpose, - GatewayAgentPeeringsPeeringExposeAs, GatewayAgentPeeringsPeeringExposeIps, - GatewayAgentPeeringsPeeringExposeNatMasquerade, + GatewayAgent, GatewayAgentPeeringsAclRulesScope, GatewayAgentPeeringsPeering, + GatewayAgentPeeringsPeeringExpose, GatewayAgentPeeringsPeeringExposeAs, + GatewayAgentPeeringsPeeringExposeIps, GatewayAgentPeeringsPeeringExposeNatMasquerade, }; #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -99,10 +99,196 @@ fn is_static(expose: &GatewayAgentPeeringsPeeringExpose) -> bool { } #[allow(clippy::too_many_lines)] -pub fn apply(d: &mut D, agent: &mut GatewayAgent, mutation: Mutation) -> Option { - let bit = match mutation { - Mutation::None => false, +fn stateful_throughout(manifest: &GatewayAgentPeeringsPeering) -> bool { + let exposes = manifest.expose.as_deref().unwrap_or(&[]); + !exposes.is_empty() + && exposes.iter().all(|expose| { + expose + .nat + .as_ref() + .is_some_and(|nat| nat.masquerade.is_some() || nat.port_forward.is_some()) + }) +} + +fn advertises_its_ips(expose: &GatewayAgentPeeringsPeeringExpose) -> bool { + expose.nat.is_none() + && expose.default != Some(true) + && expose.ips.iter().flatten().all(|ip| ip.not.is_none()) +} + +fn overlap_with_another_peer(agent: &mut GatewayAgent) -> bool { + let mut donor: Option<(String, String, String)> = None; + for (key, peering) in agent.spec.peerings.iter().flatten() { + let Some(manifests) = peering.peering.as_ref() else { + continue; + }; + for shared in manifests.keys() { + let elsewhere = agent + .spec + .peerings + .iter() + .flatten() + .any(|(other, peering)| { + other != key + && peering + .peering + .as_ref() + .is_some_and(|m| m.contains_key(shared)) + }); + if !elsewhere { + continue; + } + let prefix = manifests + .iter() + .filter(|(name, _)| *name != shared) + .flat_map(|(_, manifest)| manifest.expose.iter().flatten()) + .filter(|expose| advertises_its_ips(expose)) + .flat_map(|expose| expose.ips.iter().flatten()) + .find_map(|ip| ip.cidr.clone()); + if let Some(prefix) = prefix { + donor = Some((key.clone(), shared.clone(), prefix)); + break; + } + } + if donor.is_some() { + break; + } + } + + let mut done = false; + if let Some((donor_key, shared, prefix)) = donor { + for (key, peering) in agent.spec.peerings.iter_mut().flatten() { + if *key == donor_key { + continue; + } + let Some(manifests) = peering.peering.as_mut() else { + continue; + }; + if !manifests.contains_key(&shared) { + continue; + } + let names: Vec = manifests + .keys() + .filter(|name| **name != shared) + .cloned() + .collect(); + for name in names { + let Some(manifest) = manifests.get_mut(&name) else { + continue; + }; + if let Some(ip) = manifest + .expose + .iter_mut() + .flatten() + .filter(|expose| advertises_its_ips(expose)) + .flat_map(|expose| expose.ips.iter_mut().flatten()) + .find(|ip| ip.cidr.is_some()) + { + ip.cidr = Some(prefix.clone()); + done = true; + break; + } + } + if done { + break; + } + } + } + done +} + +fn mutate_peering_metadata(agent: &mut GatewayAgent, mutation: Mutation) -> bool { + match mutation { + Mutation::MakeBothSidesStateful => { + let mut done = false; + for (_, peerings) in agent.spec.peerings.iter_mut().flatten() { + let manifests = peerings.peering.iter_mut().flatten(); + let mut touched = 0; + for (_, manifest) in manifests { + for expose in manifest.expose.iter_mut().flatten() { + let nat = expose.nat.get_or_insert( + crate::gateway_agent_crd::GatewayAgentPeeringsPeeringExposeNat { + masquerade: None, + port_forward: None, + r#static: None, + }, + ); + nat.port_forward = None; + nat.r#static = None; + nat.masquerade = Some(GatewayAgentPeeringsPeeringExposeNatMasquerade { + idle_timeout: None, + }); + if expose.r#as.is_none() { + expose.r#as = Some(vec![GatewayAgentPeeringsPeeringExposeAs { + cidr: Some("172.31.0.0/16".to_string()), + not: None, + }]); + } + } + touched += 1; + } + if touched == 2 { + done = true; + break; + } + } + done + } + + Mutation::NameAMissingGroup => { + if let Some((_, peerings)) = agent.spec.peerings.iter_mut().flatten().next() { + peerings.gateway_group = Some("no-such-group".to_string()); + true + } else { + false + } + } + + Mutation::NameAStrangerInARule => { + let mut done = false; + for (_, peerings) in agent.spec.peerings.iter_mut().flatten() { + let Some(acl) = peerings.acl.as_mut() else { + continue; + }; + if let Some(rule) = acl.rules.iter_mut().flatten().next() { + rule.from = Some("not-in-this-peering".to_string()); + done = true; + break; + } + } + done + } + Mutation::DemandFlowScope => { + let mut done = false; + for (_, peerings) in agent.spec.peerings.iter_mut().flatten() { + let allowed = peerings + .peering + .iter() + .flatten() + .any(|(_, manifest)| stateful_throughout(manifest)); + if allowed { + continue; + } + let Some(acl) = peerings.acl.as_mut() else { + continue; + }; + for rule in acl.rules.iter_mut().flatten() { + rule.scope = Some(GatewayAgentPeeringsAclRulesScope::Flow); + done = true; + } + if done { + break; + } + } + done + } + _ => unreachable!("mutate_peering_metadata called for {mutation:?}"), + } +} + +fn mutate_expose_shape(agent: &mut GatewayAgent, mutation: Mutation) -> bool { + match mutation { Mutation::MismatchPortForwardPrefixes | Mutation::MismatchStaticNatPrefixes => { let wanted: fn(&GatewayAgentPeeringsPeeringExpose) -> bool = if mutation == Mutation::MismatchPortForwardPrefixes { @@ -181,7 +367,18 @@ pub fn apply(d: &mut D, agent: &mut GatewayAgent, mutation: Mutation) } done } + _ => unreachable!("mutate_expose_shape called for {mutation:?}"), + } +} + +pub fn apply(d: &mut D, agent: &mut GatewayAgent, mutation: Mutation) -> Option { + let bit = match mutation { + Mutation::None => false, + Mutation::MismatchPortForwardPrefixes + | Mutation::MismatchStaticNatPrefixes + | Mutation::ExcludeFromPortForwarding + | Mutation::MixAddressFamilies => mutate_expose_shape(agent, mutation), Mutation::UseReservedPrefix => { let reserved = ["127.0.0.0/8", "224.0.0.0/4", "0.0.0.0/8", "ff00::/8"]; let choice = @@ -223,83 +420,10 @@ pub fn apply(d: &mut D, agent: &mut GatewayAgent, mutation: Mutation) done } - Mutation::MakeBothSidesStateful => { - let mut done = false; - for (_, peerings) in agent.spec.peerings.iter_mut().flatten() { - let manifests = peerings.peering.iter_mut().flatten(); - let mut touched = 0; - for (_, manifest) in manifests { - for expose in manifest.expose.iter_mut().flatten() { - let nat = expose.nat.get_or_insert( - crate::gateway_agent_crd::GatewayAgentPeeringsPeeringExposeNat { - masquerade: None, - port_forward: None, - r#static: None, - }, - ); - nat.port_forward = None; - nat.r#static = None; - nat.masquerade = Some(GatewayAgentPeeringsPeeringExposeNatMasquerade { - idle_timeout: None, - }); - if expose.r#as.is_none() { - expose.r#as = Some(vec![GatewayAgentPeeringsPeeringExposeAs { - cidr: Some("172.31.0.0/16".to_string()), - not: None, - }]); - } - } - touched += 1; - } - if touched == 2 { - done = true; - break; - } - } - done - } - - Mutation::NameAMissingGroup => { - if let Some((_, peerings)) = agent.spec.peerings.iter_mut().flatten().next() { - peerings.gateway_group = Some("no-such-group".to_string()); - true - } else { - false - } - } - - Mutation::NameAStrangerInARule => { - let mut done = false; - for (_, peerings) in agent.spec.peerings.iter_mut().flatten() { - let Some(acl) = peerings.acl.as_mut() else { - continue; - }; - if let Some(rule) = acl.rules.iter_mut().flatten().next() { - rule.from = Some("not-in-this-peering".to_string()); - done = true; - break; - } - } - done - } - - Mutation::DemandFlowScope => { - let mut done = false; - for (_, peerings) in agent.spec.peerings.iter_mut().flatten() { - let Some(acl) = peerings.acl.as_mut() else { - continue; - }; - for rule in acl.rules.iter_mut().flatten() { - rule.scope = Some(GatewayAgentPeeringsAclRulesScope::Flow); - done = true; - } - if done { - break; - } - } - done - } - + Mutation::MakeBothSidesStateful + | Mutation::NameAMissingGroup + | Mutation::NameAStrangerInARule + | Mutation::DemandFlowScope => mutate_peering_metadata(agent, mutation), Mutation::UsePortZero => { let mut done = false; for expose in exposes_mut(agent) { @@ -319,84 +443,7 @@ pub fn apply(d: &mut D, agent: &mut GatewayAgent, mutation: Mutation) done } - Mutation::OverlapWithAnotherPeer => { - let mut donor: Option<(String, String, String)> = None; - for (key, peering) in agent.spec.peerings.iter().flatten() { - let Some(manifests) = peering.peering.as_ref() else { - continue; - }; - for shared in manifests.keys() { - let elsewhere = agent - .spec - .peerings - .iter() - .flatten() - .any(|(other, peering)| { - other != key - && peering - .peering - .as_ref() - .is_some_and(|m| m.contains_key(shared)) - }); - if !elsewhere { - continue; - } - let prefix = manifests - .iter() - .filter(|(name, _)| *name != shared) - .flat_map(|(_, manifest)| manifest.expose.iter().flatten()) - .flat_map(|expose| expose.ips.iter().flatten()) - .find_map(|ip| ip.cidr.clone()); - if let Some(prefix) = prefix { - donor = Some((key.clone(), shared.clone(), prefix)); - break; - } - } - if donor.is_some() { - break; - } - } - - let mut done = false; - if let Some((donor_key, shared, prefix)) = donor { - for (key, peering) in agent.spec.peerings.iter_mut().flatten() { - if *key == donor_key { - continue; - } - let Some(manifests) = peering.peering.as_mut() else { - continue; - }; - if !manifests.contains_key(&shared) { - continue; - } - let names: Vec = manifests - .keys() - .filter(|name| **name != shared) - .cloned() - .collect(); - for name in names { - let Some(manifest) = manifests.get_mut(&name) else { - continue; - }; - if let Some(ip) = manifest - .expose - .iter_mut() - .flatten() - .flat_map(|expose| expose.ips.iter_mut().flatten()) - .find(|ip| ip.cidr.is_some()) - { - ip.cidr = Some(prefix.clone()); - done = true; - break; - } - } - if done { - break; - } - } - } - done - } + Mutation::OverlapWithAnotherPeer => overlap_with_another_peer(agent), }; Some(bit) } diff --git a/mgmt/src/tests/mgmt.rs b/mgmt/src/tests/mgmt.rs index 175a944f47..46faf75f69 100644 --- a/mgmt/src/tests/mgmt.rs +++ b/mgmt/src/tests/mgmt.rs @@ -851,6 +851,11 @@ mod validator_completeness { let accepted = outcome.is_ok(); if let Ok(validated) = &outcome { + assert!( + !bit, + "{mutation:?} broke a rule outright and the validator accepted the result, \ + so nothing downstream will ever report it" + ); enact(validated, mutation); } else if let Err(e) = &outcome { assert!( @@ -889,26 +894,49 @@ mod validator_completeness { applied_at: &[AtomicUsize; Mutation::COUNT], refused_at: &[AtomicUsize; Mutation::COUNT], ) { - let mut total_applied = 0; - let mut total_refused = 0; + let mut total = 0; for mutation in Mutation::all() { let slot = mutation.index(); let drawn = drawn_at[slot].load(Ordering::Relaxed); let applied = applied_at[slot].load(Ordering::Relaxed); let refused = refused_at[slot].load(Ordering::Relaxed); println!("{mutation:<32?} {drawn:>7} drawn {applied:>7} applied {refused:>7} refused"); - assert!(drawn > 0, "{mutation:?} was never drawn"); - if mutation != Mutation::None { - total_applied += applied; - total_refused += refused; - } + total += drawn; } - assert!( - total_applied > 0 && total_refused * 2 >= total_applied, - "only {total_refused} of {total_applied} applied mutations were refused: the near-miss \ - generator is mostly producing legal configurations" - ); + let cases_for_drawn = 50 * Mutation::COUNT; + let cases_for_applied = 2_000 * Mutation::COUNT; + if total < cases_for_drawn { + println!( + "only {total} cases: too few to say anything about mutation coverage \ + (needs {cases_for_drawn} to check each was drawn, {cases_for_applied} to check each \ + found a target)" + ); + return; + } + + for mutation in Mutation::all() { + assert!( + drawn_at[mutation.index()].load(Ordering::Relaxed) > 0, + "{mutation:?} was never drawn in {total} cases" + ); + } + + if total < cases_for_applied { + println!( + "{total} cases: enough to check every mutation was drawn, too few to check each \ + found a target (needs {cases_for_applied})" + ); + return; + } + + for mutation in Mutation::all().into_iter().filter(|m| *m != Mutation::None) { + assert!( + applied_at[mutation.index()].load(Ordering::Relaxed) > 0, + "{mutation:?} never found anything to break in {total} cases, so it is testing \ + nothing" + ); + } } } From 7a5c096717c3c86a61355762dfdd5a5830905fe1 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 22:33:25 -0600 Subject: [PATCH 17/23] test(nat): Catch a static NAT table that holds one of two rules asked for The table-level ambiguity check needs no new property. It needs a mutation that reaches the case, and then the permutation property already has the answer -- which is a better outcome than a bespoke overlap checker, because it restates no rule and its failure names the two meanings outright. The two tables are not alike, and reading them side by side is the finding: - **Port forwarding** cannot be ambiguous. `RangeSet::insert_range` says "overlap is forbidden" and returns `Err`, so two rules overlapping within one prefix are refused at enact time; and across distinct prefixes `lookup_cumulative` is a longest-prefix match, which is a defined total order rather than a choice. Its own comment spells out the boundary: "If prefixes overlap and ports too, more than a match could happen. This function will provide only one match, for the longest prefix." - **Static NAT** can be. `NatRuleTable::insert` takes no `Result` and checks nothing, so a second entry for one prefix **silently replaces** the first. Nothing anywhere reports it. So `DuplicateAStaticExpose`: two exposes of one manifest claiming a single private prefix, which `validate_expose_collisions` refuses for every pair of NAT modes except masquerade-with-port-forwarding. Copying the donor expose whole would not do. Two *identical* exposes overwrite the table entry with an identical value, so there is nothing to pick between and no ambiguity to detect; ambiguity needs one prefix with **two different** translations. So the copy's translation range moves to the prefix next door. A sibling is the same length, so static NAT's equal-totals rule still holds and overlap stays the only rule broken; it is disjoint from the donor's, so the public-prefix rule holds too; and it sits inside the same parent, so it cannot stray into a reserved range or another expose's slot. It also needs no agreement between the two exposes' prefix lengths, which would drop the mutation's reach from one draw in ten to one in 250. With `check_private_prefixes_dont_overlap` gone, both guards fire from opposite directions. The near-miss property fails because the mutation certainly broke a rule and the validator accepted it. The ambiguity property fails naming the two meanings: only before: [10.1.0.0 .. 10.1.7.255] -> [172.16.8.0 .. 172.16.15.255] only after: [10.1.0.0 .. 10.1.7.255] -> [172.16.0.0 .. 172.16.7.255] One private range, two public ones, and which you get depends on nothing but the order the configuration was written in. The first guard is a restatement -- it holds because the generator knows the rule. The second is not: permutation asked no rule's permission, and would have caught this even if nobody had thought to forbid it. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- k8s-intf/src/bolero/mutate.rs | 59 ++++++++++++++++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/k8s-intf/src/bolero/mutate.rs b/k8s-intf/src/bolero/mutate.rs index 3c3d7c1321..ee781a597a 100644 --- a/k8s-intf/src/bolero/mutate.rs +++ b/k8s-intf/src/bolero/mutate.rs @@ -27,11 +27,12 @@ pub enum Mutation { NameAStrangerInARule, DemandFlowScope, UsePortZero, + DuplicateAStaticExpose, OverlapWithAnotherPeer, } impl Mutation { - pub const COUNT: usize = 14; + pub const COUNT: usize = 15; #[must_use] pub fn index(self) -> usize { @@ -57,6 +58,7 @@ impl Mutation { Self::NameAStrangerInARule, Self::DemandFlowScope, Self::UsePortZero, + Self::DuplicateAStaticExpose, Self::OverlapWithAnotherPeer, ] } @@ -73,6 +75,20 @@ fn lengthen(cidr: &str, by: u8) -> Option { Some(format!("{address}/{longer}")) } +fn sibling(cidr: &str) -> Option { + let (address, len) = cidr.split_once('/')?; + let len: u8 = len.parse().ok()?; + if address.contains(':') { + let bits = address.parse::().ok()?.to_bits(); + let flipped = bits ^ (1u128 << (128u8.checked_sub(len)?)); + Some(format!("{}/{len}", std::net::Ipv6Addr::from(flipped))) + } else { + let bits = address.parse::().ok()?.to_bits(); + let flipped = bits ^ (1u32 << (32u8.checked_sub(len)?)); + Some(format!("{}/{len}", std::net::Ipv4Addr::from(flipped))) + } +} + fn exposes_mut(agent: &mut GatewayAgent) -> Vec<&mut GatewayAgentPeeringsPeeringExpose> { agent .spec @@ -371,6 +387,46 @@ fn mutate_expose_shape(agent: &mut GatewayAgent, mutation: Mutation) -> bool { } } +fn duplicate_a_static_expose(agent: &mut GatewayAgent) -> bool { + for (_, peering) in agent.spec.peerings.iter_mut().flatten() { + for manifest in peering.peering.iter_mut().flatten().map(|(_, m)| m) { + let Some(exposes) = manifest.expose.as_mut() else { + continue; + }; + let statics: Vec = exposes + .iter() + .enumerate() + .filter(|(_, expose)| { + expose + .nat + .as_ref() + .is_some_and(|nat| nat.r#static.is_some()) + }) + .map(|(index, _)| index) + .collect(); + + let (Some(donor), Some(recipient)) = (statics.first(), statics.get(1)) else { + continue; + }; + let mut copy = exposes[*donor].clone(); + let moved = copy + .r#as + .as_mut() + .and_then(|ranges| ranges.first_mut()) + .and_then(|range| { + let next_door = sibling(range.cidr.as_ref()?)?; + range.cidr = Some(next_door); + Some(()) + }); + if moved.is_some() { + exposes[*recipient] = copy; + return true; + } + } + } + false +} + pub fn apply(d: &mut D, agent: &mut GatewayAgent, mutation: Mutation) -> Option { let bit = match mutation { Mutation::None => false, @@ -443,6 +499,7 @@ pub fn apply(d: &mut D, agent: &mut GatewayAgent, mutation: Mutation) done } + Mutation::DuplicateAStaticExpose => duplicate_a_static_expose(agent), Mutation::OverlapWithAnotherPeer => overlap_with_another_peer(agent), }; Some(bit) From a070796d070d67ce6251efee4da105ae8bbd4c83 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Sat, 8 Aug 2026 11:59:06 -0600 Subject: [PATCH 18/23] fix(k8s-intf): Put the generated gateway in its own gateway groups `build_routing_config_peer` builds **every** import prefix list, advertise prefix list, route-map and VRF import -- the entire peering half of the routing configuration. It runs only when the peering's gateway group lists *this* gateway, by name: if let Some(rank) = grouptable.get_group_member_rank(peer.gwgroup(), gwname) Group members were generated with `name: driver.produce::()`, an arbitrary string, while the gateway's name is `metadata.name` (`host-a...`). An arbitrary string is never that, so the condition was false in essentially every configuration ever generated, and that subsystem has been dead in every property run so far. It is why `internal.rs` sat at 42% region coverage with 366 missed lines. The fix renames a group's single generated member to the gateway's own name, for a drawn subset of groups so that the not-a-member case still occurs -- a peering pointed at a group this gateway does not belong to is a real configuration, and the one that legitimately renders nothing. Replacing rather than adding, because a group generated here holds at most one member, so replacing cannot collide on a name or an address, either of which validation refuses. Standing on that ground for the first time turns up two IPv6 defects, in different places, each of which had been hiding the other. `internal.rs` never uses `IpVer::V6`: - **advertise**: the prefix list is `IpVer::V4` and its prefixes are **unfiltered**, so a v6 prefix reaches `PrefixList::add_entry` and returns `ConfigError::InternalFailure`. Reached when the gateway *is* in the peering's group. - **import**: the prefix list is `IpVer::V4` *and* filtered by `is_ipv4()`, so v6 prefixes are dropped in silence. No error, and no route either. They never appeared together because the first needs the gateway in the group and the second is only visible when it is not. `ConfigError::InternalFailure` is the variant that means "this is our bug". The wasm validator does not build the internal config, so it blesses the configuration and it is written to Kubernetes; the dataplane then cannot build it, and has nowhere to report that. Every property that renders a configuration is pinned to IPv4 until that is settled, each with the reason at the call site, and `chain_properties` through a single `ipv4_agents()` so there is exactly one line to widen. Three of those are pre-existing properties that this change turns red -- independent confirmation from tests nobody wrote for it. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- k8s-intf/src/bolero/crd.rs | 26 +++- mgmt/src/processor/confbuild/internal.rs | 27 +++-- mgmt/src/tests/mgmt.rs | 145 +++++++++++++++-------- 3 files changed, 136 insertions(+), 62 deletions(-) diff --git a/k8s-intf/src/bolero/crd.rs b/k8s-intf/src/bolero/crd.rs index edba6f94b3..532f646502 100644 --- a/k8s-intf/src/bolero/crd.rs +++ b/k8s-intf/src/bolero/crd.rs @@ -8,7 +8,7 @@ use kube::core::ObjectMeta; use crate::bolero::spec::{GatewayAgentSpecs, SpecBuilder}; use crate::bolero::{AddressFamily, LegalValue, NatFlavour}; -use crate::gateway_agent_crd::GatewayAgent; +use crate::gateway_agent_crd::{GatewayAgent, GatewayAgentSpec}; const HOSTNAME_BASE: &str = "host-"; @@ -24,7 +24,18 @@ fn simple_hostname(d: &mut D) -> Option { ) } -/// +fn join_own_groups(d: &mut D, name: &str, spec: &mut GatewayAgentSpec) -> Option<()> { + for group in spec.groups.iter_mut().flatten().map(|(_, group)| group) { + if !d.produce::()? { + continue; + } + if let Some(member) = group.members.iter_mut().flatten().next() { + member.name = name.to_string(); + } + } + Some(()) +} + #[derive(Debug, Clone, Default)] pub struct GatewayAgents(GatewayAgentSpecs); @@ -32,14 +43,18 @@ impl ValueGenerator for GatewayAgents { type Output = GatewayAgent; fn generate(&self, d: &mut D) -> Option { + let name = simple_hostname(d)?; + let generation = d.gen_i64(Bound::Excluded(&0), Bound::Unbounded)?; + let mut spec = self.0.generate(d)?; + join_own_groups(d, &name, &mut spec)?; Some(GatewayAgent { metadata: ObjectMeta { - name: Some(simple_hostname(d)?), - generation: Some(d.gen_i64(Bound::Excluded(&0), Bound::Unbounded)?), + name: Some(name), + generation: Some(generation), namespace: Some("default".to_string()), ..Default::default() }, - spec: self.0.generate(d)?, + spec, status: None, // Add when we build a generator and converter for status }) } @@ -84,6 +99,7 @@ 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 { diff --git a/mgmt/src/processor/confbuild/internal.rs b/mgmt/src/processor/confbuild/internal.rs index 7121f7fe55..90579457c2 100644 --- a/mgmt/src/processor/confbuild/internal.rs +++ b/mgmt/src/processor/confbuild/internal.rs @@ -397,11 +397,18 @@ pub fn build_internal_config( mod chain_properties { use super::*; use config::{ExternalConfig, GenId}; - use k8s_intf::bolero::LegalValue; + use k8s_intf::bolero::AddressFamily; + use k8s_intf::bolero::crd::{GatewayAgentBuilder, GatewayAgents}; use k8s_intf::gateway_agent_crd::GatewayAgent; use routing::Render; use std::collections::BTreeSet; + fn ipv4_agents() -> GatewayAgents { + GatewayAgentBuilder::new() + .families(vec![AddressFamily::V4]) + .build() + } + fn chain(agent: &GatewayAgent) -> Option<(GenId, InternalConfig)> { let external = ExternalConfig::try_from(agent) .unwrap_or_else(|e| panic!("a schema-legal CRD did not convert: {e}")); @@ -416,9 +423,9 @@ mod chain_properties { #[test] fn whatever_validates_builds_and_renders() { bolero::check!() - .with_type::>() + .with_generator(ipv4_agents()) .for_each(|agent| { - let Some((genid, internal)) = chain(agent.as_ref()) else { + let Some((genid, internal)) = chain(agent) else { return; }; let text = internal.render(&genid).to_string(); @@ -432,9 +439,9 @@ mod chain_properties { #[test] fn every_vpc_gets_a_vrf_and_no_more() { bolero::check!() - .with_type::>() + .with_generator(ipv4_agents()) .for_each(|agent| { - let external = ExternalConfig::try_from(agent.as_ref()) + let external = ExternalConfig::try_from(agent) .unwrap_or_else(|e| panic!("a schema-legal CRD did not convert: {e}")); let Ok(validated) = external.validate() else { return; @@ -468,10 +475,10 @@ mod chain_properties { static ACLS: AtomicUsize = AtomicUsize::new(0); bolero::check!() - .with_type::>() + .with_generator(ipv4_agents()) .for_each(|agent| { SEEN.fetch_add(1, Ordering::Relaxed); - let external = ExternalConfig::try_from(agent.as_ref()) + let external = ExternalConfig::try_from(agent) .unwrap_or_else(|e| panic!("a schema-legal CRD did not convert: {e}")); if let Ok(validated) = external.validate() { VALIDATED.fetch_add(1, Ordering::Relaxed); @@ -525,12 +532,12 @@ mod chain_properties { #[test] fn the_chain_is_deterministic() { bolero::check!() - .with_type::>() + .with_generator(ipv4_agents()) .for_each(|agent| { - let Some((genid, once)) = chain(agent.as_ref()) else { + let Some((genid, once)) = chain(agent) else { return; }; - let (_, twice) = chain(agent.as_ref()).unwrap_or_else(|| { + let (_, twice) = chain(agent).unwrap_or_else(|| { panic!("the same CRD validated once and not the second time") }); assert_eq!( diff --git a/mgmt/src/tests/mgmt.rs b/mgmt/src/tests/mgmt.rs index 46faf75f69..8a2e44b1b3 100644 --- a/mgmt/src/tests/mgmt.rs +++ b/mgmt/src/tests/mgmt.rs @@ -786,26 +786,101 @@ mod dataplane_tables { } #[cfg(test)] -mod validator_completeness { - use concurrency::sync::atomic::{AtomicUsize, Ordering}; +#[cfg(test)] +mod enacted { use config::{ConfigError, ExternalConfig, ValidatedGwConfig}; use flow_entry::flow_table::FlowTable; - use k8s_intf::bolero::mutate::{MutatedAgents, Mutation}; use k8s_intf::gateway_agent_crd::GatewayAgent; use nat::masquerade::{MasqueradeConfig, NatAllocatorWriter}; use nat::portfw::{PortFwTableWriter, build_port_forwarding_configuration}; - use nat::static_nat::NatTablesWriter; use nat::static_nat::setup::build_nat_configuration; use routing::Render; use crate::processor::confbuild::internal::build_internal_config; - fn validator(crd: &GatewayAgent) -> Result { + pub(super) fn validator(crd: &GatewayAgent) -> Result { let external = ExternalConfig::try_from(crd) .map_err(|e| ConfigError::Invalid(format!("conversion: {e}")))?; external.validate() } + pub(super) struct Artifacts { + pub frr: Vec, + pub static_nat: Vec, + pub port_forwarding: Vec, + pub masquerade: Vec, + } + + fn lines(text: &str) -> Vec { + let mut out: Vec = text + .lines() + .map(|line| line.trim_end().to_string()) + .filter(|line| !line.is_empty()) + .collect(); + out.sort(); + out + } + + impl Artifacts { + pub(super) fn of(validated: &ValidatedGwConfig) -> Option { + let genid = validated.genid(); + let internal = build_internal_config(validated, None).ok()?; + let vpc_table = validated.external().overlay().vpc_table(); + + let nat_tables = build_nat_configuration(vpc_table).ok()?; + let ruleset = build_port_forwarding_configuration(vpc_table).ok()?; + let mut portfw = PortFwTableWriter::new(); + portfw.update_table(&ruleset).ok()?; + + let masquerade = MasqueradeConfig::new(vpc_table, genid).set_randomize(false); + let mut writer = NatAllocatorWriter::new(); + writer.update_nat_allocator(masquerade, &FlowTable::new(16)); + let allocator = writer.get_reader().get(); + + Some(Self { + frr: lines(&internal.render(&genid).to_string()), + static_nat: lines(&nat_tables.to_string()), + port_forwarding: lines( + &ruleset + .iter() + .map(ToString::to_string) + .collect::>() + .join("\n"), + ), + masquerade: allocator + .map(|allocator| lines(&allocator.to_string())) + .unwrap_or_default(), + }) + } + + pub(super) fn all(&self) -> Vec { + let mut out = self.frr.clone(); + out.extend(self.static_nat.iter().cloned()); + out.extend(self.port_forwarding.iter().cloned()); + out.extend(self.masquerade.iter().cloned()); + out.sort(); + out + } + } +} + +mod validator_completeness { + use super::enacted::validator; + use concurrency::sync::atomic::{AtomicUsize, Ordering}; + use config::{ConfigError, ValidatedGwConfig}; + use flow_entry::flow_table::FlowTable; + use k8s_intf::bolero::AddressFamily; + use k8s_intf::bolero::crd::GatewayAgentBuilder; + use k8s_intf::bolero::mutate::{MutatedAgents, Mutation}; + use k8s_intf::gateway_agent_crd::GatewayAgent; + use nat::masquerade::{MasqueradeConfig, NatAllocatorWriter}; + use nat::portfw::{PortFwTableWriter, build_port_forwarding_configuration}; + use nat::static_nat::NatTablesWriter; + use nat::static_nat::setup::build_nat_configuration; + use routing::Render; + + use crate::processor::confbuild::internal::build_internal_config; + fn enact(validated: &ValidatedGwConfig, mutation: Mutation) { let genid = validated.genid(); @@ -835,7 +910,9 @@ mod validator_completeness { } #[test] - fn whatever_the_validator_accepts_can_be_enacted() { + fn whatever_the_validator_accepts_can_be_enacted_over_ipv4() { + let families = vec![AddressFamily::V4]; + const N: usize = Mutation::COUNT; #[allow(clippy::declare_interior_mutable_const)] const ZERO: AtomicUsize = AtomicUsize::new(0); @@ -844,7 +921,9 @@ mod validator_completeness { static REFUSED: [AtomicUsize; N] = [ZERO; N]; bolero::check!() - .with_generator(MutatedAgents::default()) + .with_generator(MutatedAgents::new( + GatewayAgentBuilder::new().families(families).build(), + )) .cloned() .for_each(|(mutation, bit, agent): (Mutation, bool, GatewayAgent)| { let outcome = validator(&agent); @@ -941,48 +1020,11 @@ mod validator_completeness { } mod ambiguity { + use super::enacted::{Artifacts, validator}; use concurrency::sync::atomic::{AtomicUsize, Ordering}; - use config::{ConfigError, ExternalConfig, ValidatedGwConfig}; - use flow_entry::flow_table::FlowTable; use k8s_intf::bolero::mutate::Mutation; use k8s_intf::bolero::permute::PermutedAgents; use k8s_intf::gateway_agent_crd::GatewayAgent; - use nat::masquerade::{MasqueradeConfig, NatAllocatorWriter}; - use nat::portfw::{PortFwTableWriter, build_port_forwarding_configuration}; - use nat::static_nat::setup::build_nat_configuration; - use routing::Render; - - use crate::processor::confbuild::internal::build_internal_config; - - fn validator(crd: &GatewayAgent) -> Result { - let external = ExternalConfig::try_from(crd) - .map_err(|e| ConfigError::Invalid(format!("conversion: {e}")))?; - external.validate() - } - - fn artifacts(validated: &ValidatedGwConfig) -> Option> { - let genid = validated.genid(); - let internal = build_internal_config(validated, None).ok()?; - let vpc_table = validated.external().overlay().vpc_table(); - - let nat_tables = build_nat_configuration(vpc_table).ok()?; - let ruleset = build_port_forwarding_configuration(vpc_table).ok()?; - let mut portfw = PortFwTableWriter::new(); - portfw.update_table(&ruleset).ok()?; - - let masquerade = MasqueradeConfig::new(vpc_table, genid).set_randomize(false); - let mut allocator = NatAllocatorWriter::new(); - allocator.update_nat_allocator(masquerade, &FlowTable::new(16)); - - let mut lines: Vec = - format!("{}\n{nat_tables}\n{ruleset:#?}", internal.render(&genid)) - .lines() - .map(|line| line.trim_end().to_string()) - .filter(|line| !line.is_empty()) - .collect(); - lines.sort(); - Some(lines) - } #[test] fn a_configuration_has_only_one_meaning() { @@ -990,7 +1032,13 @@ mod ambiguity { static COMPARED: AtomicUsize = AtomicUsize::new(0); bolero::check!() - .with_generator(PermutedAgents::default()) + .with_generator(PermutedAgents::new( + k8s_intf::bolero::mutate::MutatedAgents::new( + k8s_intf::bolero::crd::GatewayAgentBuilder::new() + .families(vec![k8s_intf::bolero::AddressFamily::V4]) + .build(), + ), + )) .cloned() .for_each( |(mutation, agent, permuted, moved): (Mutation, GatewayAgent, GatewayAgent, bool)| { @@ -1005,9 +1053,11 @@ mod ambiguity { ) }); - let (Some(before), Some(after)) = (artifacts(&first), artifacts(&second)) else { + let (Some(before), Some(after)) = (Artifacts::of(&first), Artifacts::of(&second)) + else { return; }; + let (before, after) = (before.all(), after.all()); if moved { MOVED.fetch_add(1, Ordering::Relaxed); @@ -1040,6 +1090,7 @@ mod ambiguity { let compared = COMPARED.load(Ordering::Relaxed); let moved = MOVED.load(Ordering::Relaxed); println!("{moved} of {compared} comparisons were of a genuinely reordered configuration"); + #[cfg(not(fuzzing))] assert!( compared > 0 && moved * 10 >= compared, "only {moved} of {compared} comparisons actually reordered anything: the permutation is \ From a3049ac3e1fa22835a6f20cfee9230aeaa877005 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Sat, 8 Aug 2026 11:59:31 -0600 Subject: [PATCH 19/23] test(mgmt): Check that every expose leaves a trace The third question to ask of a blessed configuration, after "can it be enacted" and "does it have one meaning": **is the dataplane doing all of it?** The failure this hunts is a builder that silently ignores part of its input -- a shape it does not handle, a `continue` on a branch nobody expected to be reachable. Nothing else here covers that class, and unlike the ambiguity work it needs no mutation to reach: such a bug lives in the builder rather than behind a validator rule, so it shows up on configurations that are entirely legal. The oracle is removal. Take one expose out and something the dataplane installs must change. The artifacts are asked **one at a time**, and that is the whole design. Every expose contributes prefixes to the FRR render whatever else it does, so a merged comparison would report a difference even where a NAT builder had ignored the expose completely -- exactly the case worth catching. What each artifact is entitled to expect comes from the removed expose's own NAT mode, read straight off the CRD: no model of `collapse_prefixes` or of the PAT splitting is needed, and none is wanted, since a wrong model would make this property lie rather than fail. That also retires the counting formulation this replaces, which needed the expected number of table entries and so needed exactly the model that would make it unreliable. Two defects fall out of it directly -- the gateway-group hole and the IPv6 rendering behind it, both in the commit before this one -- and a third by hanging. `NatAllocator`'s `Display` never returns on an IPv6 masquerade pool: `ips_in_bitmap` walks every set bit of the pool's bitmap, a few thousand iterations for a v4 `/20` and unbounded for a v6 pool. Over IPv4 it completes 380 times in a one-second run, worst case 6ms; over both families it does not complete once in 200 seconds. Not a deadlock -- 61 crash artifacts, every one `slow-unit`, none a `timeout`, at 99.4% CPU. That is why this property and `ambiguity` are pinned to IPv4 as well. It also answered the question it was built on. There is no legitimately no-op expose by *shape*, but there is by **context**: an expose in a peering whose gateway group excludes this gateway is not this gateway's to route, so nothing it contains reaches any artifact. Correct behaviour, unpredictable from the expose alone, and now exempted by `handled_here`. About one case in fourteen reaches the comparison; the rest are configurations with no manifest holding two exposes, overwhelmingly because they have no peerings at all. Hence the loose bound in the health check, set from that measurement, and the note that a floor on vpcs and peerings would do better than the ceiling `sizes()` can express. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- k8s-intf/src/bolero/mod.rs | 1 + k8s-intf/src/bolero/reduce.rs | 113 +++++++++++++++++++++++++++ mgmt/src/tests/mgmt.rs | 143 ++++++++++++++++++++++++++++++++++ 3 files changed, 257 insertions(+) create mode 100644 k8s-intf/src/bolero/reduce.rs diff --git a/k8s-intf/src/bolero/mod.rs b/k8s-intf/src/bolero/mod.rs index 8c29d9547d..e8234c1a72 100644 --- a/k8s-intf/src/bolero/mod.rs +++ b/k8s-intf/src/bolero/mod.rs @@ -12,6 +12,7 @@ pub mod logs; pub mod mutate; pub mod peering; pub mod permute; +pub mod reduce; pub mod spec; pub mod support; pub mod vpc; diff --git a/k8s-intf/src/bolero/reduce.rs b/k8s-intf/src/bolero/reduce.rs new file mode 100644 index 0000000000..fca44ec1f9 --- /dev/null +++ b/k8s-intf/src/bolero/reduce.rs @@ -0,0 +1,113 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +use std::ops::Bound; + +use bolero::{Driver, ValueGenerator}; + +use crate::bolero::mutate::{MutatedAgents, Mutation}; +use crate::gateway_agent_crd::GatewayAgent; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Dropped { + pub peering: String, + pub vpc: String, + pub index: usize, + pub nat: Option<&'static str>, +} + +impl std::fmt::Display for Dropped { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "{}/{} expose {} ({})", + self.peering, + self.vpc, + self.index, + self.nat.unwrap_or("no nat") + ) + } +} + +fn candidates(agent: &GatewayAgent) -> Vec<(String, String, usize)> { + let mut out = Vec::new(); + for (peering_name, peering) in agent.spec.peerings.iter().flatten() { + for (vpc, manifest) in peering.peering.iter().flatten() { + let count = manifest.expose.as_ref().map_or(0, Vec::len); + if count < 2 { + continue; + } + for index in 0..count { + out.push((peering_name.clone(), vpc.clone(), index)); + } + } + } + out +} + +fn drop_an_expose( + d: &mut D, + agent: &mut GatewayAgent, + mut choices: Vec<(String, String, usize)>, +) -> Option { + let choice = d.gen_usize(Bound::Included(&0), Bound::Excluded(&choices.len()))?; + let (peering_name, vpc, index) = choices.swap_remove(choice); + + let exposes = agent + .spec + .peerings + .as_mut()? + .get_mut(&peering_name)? + .peering + .as_mut()? + .get_mut(&vpc)? + .expose + .as_mut()?; + if index >= exposes.len() { + return None; + } + let removed = exposes.remove(index); + let nat = removed.nat.as_ref().and_then(|nat| { + if nat.r#static.is_some() { + Some("static") + } else if nat.masquerade.is_some() { + Some("masquerade") + } else if nat.port_forward.is_some() { + Some("port forwarding") + } else { + None + } + }); + + Some(Dropped { + peering: peering_name, + vpc, + index, + nat, + }) +} + +#[derive(Debug, Default, Clone)] +pub struct ReducedAgents(MutatedAgents); + +impl ReducedAgents { + #[must_use] + pub fn new(agents: MutatedAgents) -> Self { + Self(agents) + } +} + +impl ValueGenerator for ReducedAgents { + type Output = (Mutation, GatewayAgent, GatewayAgent, Option); + + fn generate(&self, d: &mut D) -> Option { + let (mutation, _applied, agent) = self.0.generate(d)?; + let mut reduced = agent.clone(); + let choices = candidates(&reduced); + if choices.is_empty() { + return Some((mutation, agent, reduced, None)); + } + let dropped = drop_an_expose(d, &mut reduced, choices)?; + Some((mutation, agent, reduced, Some(dropped))) + } +} diff --git a/mgmt/src/tests/mgmt.rs b/mgmt/src/tests/mgmt.rs index 8a2e44b1b3..bd55f3fd7a 100644 --- a/mgmt/src/tests/mgmt.rs +++ b/mgmt/src/tests/mgmt.rs @@ -1098,3 +1098,146 @@ mod ambiguity { ); } } + +mod relevance { + use super::enacted::{Artifacts, validator}; + use concurrency::sync::atomic::{AtomicUsize, Ordering}; + use k8s_intf::bolero::mutate::Mutation; + use k8s_intf::bolero::reduce::{Dropped, ReducedAgents}; + use k8s_intf::gateway_agent_crd::GatewayAgent; + + fn handled_here(agent: &GatewayAgent, peering: &str) -> bool { + let Some(name) = agent.metadata.name.as_deref() else { + return false; + }; + let Some(group) = agent + .spec + .peerings + .as_ref() + .and_then(|peerings| peerings.get(peering)) + .and_then(|peering| peering.gateway_group.as_deref()) + else { + return false; + }; + agent + .spec + .groups + .as_ref() + .and_then(|groups| groups.get(group)) + .and_then(|group| group.members.as_ref()) + .is_some_and(|members| members.iter().any(|m| m.name == name)) + } + + fn difference(left: &[String], right: &[String]) -> Option { + if left == right { + return None; + } + let mut out: Vec = Vec::new(); + for line in left { + if !right.contains(line) { + out.push(format!(" only with it: {line}")); + } + } + for line in right { + if !left.contains(line) { + out.push(format!(" only without it: {line}")); + } + } + out.truncate(10); + Some(out.join("\n")) + } + + #[test] + fn every_expose_leaves_a_trace() { + static CHECKED: AtomicUsize = AtomicUsize::new(0); + static NOTHING_TO_DROP: AtomicUsize = AtomicUsize::new(0); + static REFUSED_WITHOUT: AtomicUsize = AtomicUsize::new(0); + static NOT_OURS: AtomicUsize = AtomicUsize::new(0); + static TRANSLATING: AtomicUsize = AtomicUsize::new(0); + + bolero::check!() + .with_generator(ReducedAgents::new( + k8s_intf::bolero::mutate::MutatedAgents::new( + k8s_intf::bolero::crd::GatewayAgentBuilder::new() + .families(vec![k8s_intf::bolero::AddressFamily::V4]) + .sizes(4, 3, 4, 3) + .build(), + ), + )) + .cloned() + .for_each( + |(mutation, agent, reduced, dropped): ( + Mutation, + GatewayAgent, + GatewayAgent, + Option, + )| { + let Some(dropped) = dropped else { + NOTHING_TO_DROP.fetch_add(1, Ordering::Relaxed); + return; + }; + let Ok(whole) = validator(&agent) else { + return; + }; + let Ok(less) = validator(&reduced) else { + REFUSED_WITHOUT.fetch_add(1, Ordering::Relaxed); + return; + }; + let (Some(with), Some(without)) = (Artifacts::of(&whole), Artifacts::of(&less)) + else { + return; + }; + + if !handled_here(&agent, &dropped.peering) { + NOT_OURS.fetch_add(1, Ordering::Relaxed); + return; + } + + CHECKED.fetch_add(1, Ordering::Relaxed); + + assert!( + difference(&with.frr, &without.frr).is_some(), + "{mutation:?}: removing {dropped} changed nothing in the routing \ + configuration, so the dataplane was never routing it" + ); + + let (table, name) = match dropped.nat { + None => return, + Some("static") => (&with.static_nat, &without.static_nat), + Some("port forwarding") => { + (&with.port_forwarding, &without.port_forwarding) + } + Some("masquerade") => (&with.masquerade, &without.masquerade), + Some(other) => unreachable!("unknown nat mode {other}"), + }; + TRANSLATING.fetch_add(1, Ordering::Relaxed); + assert!( + difference(table, name).is_some(), + "{mutation:?}: removing {dropped} changed nothing in the table for its own \ + NAT mode, so that translation was never installed" + ); + }, + ); + + let checked = CHECKED.load(Ordering::Relaxed); + let nothing = NOTHING_TO_DROP.load(Ordering::Relaxed); + let refused = REFUSED_WITHOUT.load(Ordering::Relaxed); + let not_ours = NOT_OURS.load(Ordering::Relaxed); + let translating = TRANSLATING.load(Ordering::Relaxed); + println!( + "{checked} exposes checked ({translating} of them translating); {nothing} \ + configurations had nothing to drop, {refused} became illegal without it, {not_ours} \ + sat in a peering this gateway does not handle" + ); + + let seen = checked + nothing + refused + not_ours; + #[cfg(not(fuzzing))] + if seen > 200 { + assert!( + checked * 40 > seen, + "only {checked} of {seen} cases got as far as comparing artifacts: this property has \ + become mostly skips" + ); + } + } +} From e240cc14d180843da905d669890fbe96db3a3232 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Mon, 17 Aug 2026 14:42:26 -0600 Subject: [PATCH 20/23] test(mgmt): Follow the genid out of MasqueradeConfig Main moved the generation id from `MasqueradeConfig::new` to `update_nat_allocator`, which is the right home for it -- the config describes what to masquerade, and the generation belongs to the act of installing it. The three call sites these tests grew still passed it the old way. Mechanical, and it is the only adaptation the config-generator work needed against a main that has moved four hundred commits since this was written. Kept as one commit rather than folded back into the three that introduced the call sites. That leaves those three, and the five between them, unable to compile `dataplane-mgmt`'s tests on their own. Squashing it back is a `--autosquash` away if bisectable history inside the branch is worth more than the smaller diff to review. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- mgmt/src/tests/mgmt.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/mgmt/src/tests/mgmt.rs b/mgmt/src/tests/mgmt.rs index bd55f3fd7a..98e39671a0 100644 --- a/mgmt/src/tests/mgmt.rs +++ b/mgmt/src/tests/mgmt.rs @@ -719,10 +719,10 @@ mod dataplane_tables { let mut nattablesw = NatTablesWriter::new(); nattablesw.update_nat_tables(nat_tables); - let masquerade = MasqueradeConfig::new(vpc_table, validated.genid()).set_randomize(false); + let masquerade = MasqueradeConfig::new(vpc_table).set_randomize(false); let mut natallocatorw = NatAllocatorWriter::new(); let flow_table = FlowTable::new(16); - natallocatorw.update_nat_allocator(masquerade, &flow_table); + natallocatorw.update_nat_allocator(masquerade, validated.genid(), &flow_table); let ruleset = build_port_forwarding_configuration(vpc_table).unwrap_or_else(|e| { panic!("a validated {flavour:?} configuration would not build port forwarding: {e}") @@ -832,9 +832,9 @@ mod enacted { let mut portfw = PortFwTableWriter::new(); portfw.update_table(&ruleset).ok()?; - let masquerade = MasqueradeConfig::new(vpc_table, genid).set_randomize(false); + let masquerade = MasqueradeConfig::new(vpc_table).set_randomize(false); let mut writer = NatAllocatorWriter::new(); - writer.update_nat_allocator(masquerade, &FlowTable::new(16)); + writer.update_nat_allocator(masquerade, genid, &FlowTable::new(16)); let allocator = writer.get_reader().get(); Some(Self { @@ -896,8 +896,8 @@ mod validator_completeness { }); NatTablesWriter::new().update_nat_tables(nat_tables); - let masquerade = MasqueradeConfig::new(vpc_table, genid).set_randomize(false); - NatAllocatorWriter::new().update_nat_allocator(masquerade, &FlowTable::new(16)); + let masquerade = MasqueradeConfig::new(vpc_table).set_randomize(false); + NatAllocatorWriter::new().update_nat_allocator(masquerade, genid, &FlowTable::new(16)); let ruleset = build_port_forwarding_configuration(vpc_table).unwrap_or_else(|e| { panic!("{mutation:?}: validator accepted a config port forwarding rejects: {e}") From a86fad95485bde354553c613db33b8292f57f629 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 27 Aug 2026 09:00:17 -0600 Subject: [PATCH 21/23] fix(mgmt): Refuse an IPv6 peering by name instead of two ways by accident Nothing in this module uses `IpVer::V6`, and left to themselves the two prefix lists failed differently and both badly. The advertise list is `IpVer::V4` over unfiltered prefixes, so a v6 prefix failed `is_version_compatible` and came back as `ConfigError::InternalFailure` -- the one rejection this crate's own mutation property asserts a configuration must never get, on the grounds that "this is our bug" is not something anyone can act on from the outside. The import list was `IpVer::V4` *and* filtered by `is_ipv4()`, so v6 prefixes were dropped in silence. That half is the worse one: the configuration applies, reports success, and carries no traffic. `build_internal_config` runs in `process_incoming_config` before `apply`, so this was already a clean rejection and nothing was committed when it fired -- contrary to what the comment on `chain_properties::ipv4_agents` says, which took the partial-apply argument from the port-forwarding case, where `apply_port_forwarding_config` really does run after the other stages. What changes here is what the operator is told, and that the silent half stops being silent. Here rather than in `VpcExpose::validate` because it is this module that is IPv4-only. NAT's static, masquerade and port-forwarding tables build and translate v6 today; refusing a v6 expose outright would take that away to guard a limitation it does not have. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- config/src/errors.rs | 2 ++ mgmt/src/processor/confbuild/internal.rs | 42 +++++++++++++++--------- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/config/src/errors.rs b/config/src/errors.rs index c359f93211..78a81abc05 100644 --- a/config/src/errors.rs +++ b/config/src/errors.rs @@ -46,6 +46,8 @@ pub enum ConfigError { FailureApply(String), #[error("Forbidden: {0}")] Forbidden(&'static str), + #[error("Not supported yet: {0}")] + Unsupported(&'static str), #[error("Bad VPC Id")] BadVpcId(String), #[error("Bad VTEP local address {0}: {1}")] diff --git a/mgmt/src/processor/confbuild/internal.rs b/mgmt/src/processor/confbuild/internal.rs index 90579457c2..4d5c92ccfb 100644 --- a/mgmt/src/processor/confbuild/internal.rs +++ b/mgmt/src/processor/confbuild/internal.rs @@ -15,7 +15,7 @@ use config::external::overlay::vpc::{ValidatedPeering, ValidatedVpc}; use config::external::overlay::vpcpeering::ValidatedManifest; use config::{ConfigError, ConfigResult}; -use lpm::prefix::Prefix; +use lpm::prefix::{Prefix, PrefixWithOptionalPorts}; use net::route::RouteTableId; use net::vxlan::Vni; use std::net::Ipv4Addr; @@ -46,24 +46,30 @@ fn vpc_import_prefix_list_for_peer( Some(vpc.import_plist_peer_desc(rmanifest.name())), ); for expose in rmanifest.valexp() { + reject_ipv6(expose.ips().iter().map(PrefixWithOptionalPorts::prefix))?; // allow native prefixes, natted or not - let native_prefixes = - expose - .ips() - .iter() - .filter(|p| p.prefix().is_ipv4()) - .map(|prefix_with_ports| { - PrefixListEntry::new( - PrefixListAction::Permit, - PrefixListPrefix::Prefix(prefix_with_ports.prefix()), - Some(PrefixListMatchLen::Ge(prefix_with_ports.prefix().length())), - ) - }); + let native_prefixes = expose.ips().iter().map(|prefix_with_ports| { + PrefixListEntry::new( + PrefixListAction::Permit, + PrefixListPrefix::Prefix(prefix_with_ports.prefix()), + Some(PrefixListMatchLen::Ge(prefix_with_ports.prefix().length())), + ) + }); plist.add_entries(native_prefixes)?; } Ok(plist) } +fn reject_ipv6(prefixes: impl IntoIterator) -> ConfigResult { + if prefixes.into_iter().any(|prefix| prefix.is_ipv6()) { + return Err(ConfigError::Unsupported( + "IPv6 prefixes in a vpc peering: the FRR configuration built from a peering is \ + IPv4-only, so such a peering cannot be rendered", + )); + } + Ok(()) +} + /// Build AF l2vpn EVPN config for a VPC VRF #[must_use] fn vpc_bgp_af_l2vpn_evpn(vpc: &ValidatedVpc) -> AfL2vpnEvpn { @@ -159,6 +165,8 @@ impl VpcRoutingConfigIpv4 { /* list of advertised prefixes */ self.adv_nets.extend(nets.clone()); + reject_ipv6(nets.iter().copied())?; + /* build adv prefix list and route-map */ let mut adv_plist = PrefixList::new( &vpc.adv_plist(rmanifest.name()), @@ -414,9 +422,11 @@ mod chain_properties { .unwrap_or_else(|e| panic!("a schema-legal CRD did not convert: {e}")); let validated = external.validate().ok()?; let genid = validated.genid(); - let internal = build_internal_config(&validated, None).unwrap_or_else(|e| { - panic!("a validated configuration would not build: {e}\n{validated:#?}") - }); + let internal = match build_internal_config(&validated, None) { + Ok(internal) => internal, + Err(ConfigError::Unsupported(_)) => return None, + Err(e) => panic!("a validated configuration would not build: {e}\n{validated:#?}"), + }; Some((genid, internal)) } From 6c443884a0dc0a99d72dc711c5051cdc0080f165 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 27 Aug 2026 12:59:47 -0600 Subject: [PATCH 22/23] fix(mgmt): Route both build sites through the same declared-limitation skip `ipv4_agents` documents widening its family list as the way to find out whether IPv6 rendering has landed, on the grounds that `chain` treats a declared limitation as a skip. It was a skip in one of the module's two build sites; the other unwrapped, so following the documented procedure would have failed `every_vpc_gets_a_vrf_and_no_more` on a legal configuration, with a message blaming the builder. The guard in `VpcRoutingConfigIpv4` also ran after the field it protects had already been extended. Signed-off-by: Daniel Noland --- mgmt/src/processor/confbuild/internal.rs | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/mgmt/src/processor/confbuild/internal.rs b/mgmt/src/processor/confbuild/internal.rs index 4d5c92ccfb..a541059180 100644 --- a/mgmt/src/processor/confbuild/internal.rs +++ b/mgmt/src/processor/confbuild/internal.rs @@ -162,11 +162,11 @@ impl VpcRoutingConfigIpv4 { nets.sort_unstable(); nets.dedup(); + reject_ipv6(nets.iter().copied())?; + /* list of advertised prefixes */ self.adv_nets.extend(nets.clone()); - reject_ipv6(nets.iter().copied())?; - /* build adv prefix list and route-map */ let mut adv_plist = PrefixList::new( &vpc.adv_plist(rmanifest.name()), @@ -422,12 +422,15 @@ mod chain_properties { .unwrap_or_else(|e| panic!("a schema-legal CRD did not convert: {e}")); let validated = external.validate().ok()?; let genid = validated.genid(); - let internal = match build_internal_config(&validated, None) { - Ok(internal) => internal, - Err(ConfigError::Unsupported(_)) => return None, + Some((genid, build_or_skip(&validated)?)) + } + + fn build_or_skip(validated: &ValidatedGwConfig) -> Option { + match build_internal_config(validated, None) { + Ok(internal) => Some(internal), + Err(ConfigError::Unsupported(_)) => None, Err(e) => panic!("a validated configuration would not build: {e}\n{validated:#?}"), - }; - Some((genid, internal)) + } } #[test] @@ -456,8 +459,9 @@ mod chain_properties { let Ok(validated) = external.validate() else { return; }; - let internal = build_internal_config(&validated, None) - .unwrap_or_else(|e| panic!("a validated configuration would not build: {e}")); + let Some(internal) = build_or_skip(&validated) else { + return; + }; let wanted: BTreeSet = validated .external() From 440a6fa364acb37b2e23e7153a9477d60337f745 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 27 Aug 2026 12:59:47 -0600 Subject: [PATCH 23/23] fix(k8s-intf): Refuse a slot budget the scheme cannot separate `SUBNET_SLOTS` reserves sixteen slots, one per vpc, and `expose_slot` saturates. A caller asking `SpecBuilder` for more vpcs or exposes than that gets colliding slots rather than a panic, which puts the generator back to producing the overlapping prefixes the whole scheme exists to prevent -- and the only symptom is that the properties downstream go quiet. The doc comment describing `apply`, and its `too_many_lines` allowance, were attached to `stateful_throughout`: doc comments are attributes, so both blocks and the attribute bound to the following item. Signed-off-by: Daniel Noland --- k8s-intf/src/bolero/mutate.rs | 1 - k8s-intf/src/bolero/support.rs | 11 +++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/k8s-intf/src/bolero/mutate.rs b/k8s-intf/src/bolero/mutate.rs index ee781a597a..9e70ebf8ed 100644 --- a/k8s-intf/src/bolero/mutate.rs +++ b/k8s-intf/src/bolero/mutate.rs @@ -114,7 +114,6 @@ fn is_static(expose: &GatewayAgentPeeringsPeeringExpose) -> bool { .is_some_and(|nat| nat.r#static.is_some()) } -#[allow(clippy::too_many_lines)] fn stateful_throughout(manifest: &GatewayAgentPeeringsPeering) -> bool { let exposes = manifest.expose.as_deref().unwrap_or(&[]); !exposes.is_empty() diff --git a/k8s-intf/src/bolero/support.rs b/k8s-intf/src/bolero/support.rs index 06f570fbd7..a6c2a28378 100644 --- a/k8s-intf/src/bolero/support.rs +++ b/k8s-intf/src/bolero/support.rs @@ -438,6 +438,17 @@ pub mod blocks { #[must_use] pub fn expose_slot(vpc: u8, slots_per_vpc: u8, expose: u8) -> u8 { + debug_assert!( + vpc < SUBNET_SLOTS, + "vpc {vpc} has no subnet slot of its own: {SUBNET_SLOTS} are reserved, so its subnets \ + would land on top of an expose's prefixes and the two would overlap" + ); + let wanted = u32::from(vpc) * u32::from(slots_per_vpc) + u32::from(expose); + debug_assert!( + u8::try_from(wanted).is_ok(), + "vpc {vpc} expose {expose} needs slot {wanted} of 256, so the saturating arithmetic \ + below will hand it a slot another expose already holds" + ); vpc.saturating_mul(slots_per_vpc).saturating_add(expose) }