From 856dbdc7707898914bca304897921dd4f3264cc8 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 24 Jul 2026 16:39:16 -0600 Subject: [PATCH 1/8] test(k8s-intf): add name, port, and reserved-safe prefix generators Four generator primitives the config generators need, none of which existed: `K8sName` produces an RFC 1123 DNS label. `bolero`'s `String` generator produces arbitrary Unicode, which is not a legal k8s object name, so every CRD field naming an object was violating the "never produce an illegal value" half of the `TypeGenerator` contract in `development/code/property-testing.md`. `generate_port_range`/`generate_port_list` produce the textual port forms the converters parse, covering both the single-port and range spellings. `RoutableV4CidrGenerator`/`RoutableV6CidrGenerator` produce prefixes that do not overlap a reserved block. `VpcExpose` validation rejects a prefix overlapping one -- overlap, not containment, so a `/0` is illegal -- which the existing `UniqueV4CidrGenerator` happily produces. Reserved blocks are skipped whole rather than stepped through, since `240.0.0.0/4` alone spans 2^28 `/32`s. `PrefixPool` hands out mutually non-overlapping prefixes from one shared allocator, for the manifest-level non-overlap rule that cannot be satisfied by generating each expose independently. It uses interior mutability so it can be shared by `&` through a generator tree, since `ValueGenerator::generate` takes `&self`. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- k8s-intf/src/bolero/support.rs | 501 ++++++++++++++++++++++++++++++++- 1 file changed, 500 insertions(+), 1 deletion(-) diff --git a/k8s-intf/src/bolero/support.rs b/k8s-intf/src/bolero/support.rs index b1880a381d..35b6ddac01 100644 --- a/k8s-intf/src/bolero/support.rs +++ b/k8s-intf/src/bolero/support.rs @@ -4,7 +4,7 @@ use std::net::{Ipv4Addr, Ipv6Addr}; use std::ops::Bound; -use bolero::{Driver, ValueGenerator}; +use bolero::{Driver, TypeGenerator, ValueGenerator}; fn v4cdir_from_bytes(addr_bytes: u32, mask: u8) -> String { let and_mask = u32::MAX.unbounded_shl(32 - u32::from(mask)); @@ -254,6 +254,138 @@ pub fn choose(d: &mut D, choices: &[T]) -> Option { Some(choices[index].clone()) } +/// Characters legal anywhere in an RFC 1123 DNS label. +const NAME_ALNUM: &[u8] = b"abcdefghijklmnopqrstuvwxyz0123456789"; +/// Characters legal in the interior of an RFC 1123 DNS label. +const NAME_INNER: &[u8] = b"abcdefghijklmnopqrstuvwxyz0123456789-"; + +/// Maximum length of an RFC 1123 DNS label. +const NAME_MAX_LEN: usize = 63; + +/// A syntactically legal k8s object name. +/// +/// k8s object names are RFC 1123 DNS labels: lowercase alphanumerics plus `-`, starting and +/// ending with an alphanumeric, at most 63 characters. `bolero`'s [`String`] generator produces +/// arbitrary Unicode, which is *not* a legal name, so any CRD field naming a k8s object must draw +/// from here instead — a `TypeGenerator` must never produce an illegal value (see +/// `development/code/property-testing.md`). +#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] +pub struct K8sName(String); + +impl K8sName { + #[must_use] + pub fn take(self) -> String { + self.0 + } + + #[must_use] + pub fn as_str(&self) -> &str { + self.0.as_str() + } +} + +impl AsRef for K8sName { + fn as_ref(&self) -> &str { + self.0.as_str() + } +} + +impl std::fmt::Display for K8sName { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) + } +} + +/// Covers every legal RFC 1123 DNS label. +impl TypeGenerator for K8sName { + fn generate(d: &mut D) -> Option { + K8sNameGenerator::new(1, NAME_MAX_LEN).generate(d) + } +} + +/// A [`K8sName`] restricted to a length range. +/// +/// Prefer this over [`K8sName`]'s [`TypeGenerator`] where 63-character names would only bloat the +/// generated config without exercising anything. +pub struct K8sNameGenerator { + min_len: usize, + max_len: usize, +} + +impl K8sNameGenerator { + /// # Panics + /// + /// Panics if the requested length range is not a sub-range of `1..=63`. + #[must_use] + pub fn new(min_len: usize, max_len: usize) -> Self { + assert!( + min_len >= 1 && min_len <= max_len && max_len <= NAME_MAX_LEN, + "illegal k8s name length range {min_len}..={max_len}" + ); + Self { min_len, max_len } + } +} + +impl ValueGenerator for K8sNameGenerator { + type Output = K8sName; + + fn generate(&self, d: &mut D) -> Option { + let len = d.gen_usize( + Bound::Included(&self.min_len), + Bound::Included(&self.max_len), + )?; + let mut name = String::with_capacity(len); + for position in 0..len { + // Only the interior may hold a `-`; the first and last character must be alphanumeric. + let alphabet = if position == 0 || position == len - 1 { + NAME_ALNUM + } else { + NAME_INNER + }; + let index = d.gen_usize(Bound::Included(&0), Bound::Excluded(&alphabet.len()))?; + name.push(alphabet[index] as char); + } + Some(K8sName(name)) + } +} + +/// Generate the textual form of a legal port range: `""` or `"-"`. +/// +/// Both forms parse to an `lpm::prefix::PortRange`, which requires only `start <= end`. +pub fn generate_port_range(d: &mut D) -> Option { + let a = d.produce::()?; + let b = d.produce::()?; + if a == b && d.gen_bool(None)? { + // Cover the single-port spelling, which takes a distinct branch in `PortRange::from_str`. + return Some(format!("{a}")); + } + Some(format!("{}-{}", a.min(b), a.max(b))) +} + +/// Generate a legal `ports` list entry: one or more comma-separated port ranges. +/// +/// The converters split these on `,` before parsing each element, and reject a present-but-empty +/// list, so this never yields an empty string. +pub fn generate_port_list(d: &mut D) -> Option { + let count = d.gen_usize(Bound::Included(&1), Bound::Included(&4))?; + let mut ranges = Vec::with_capacity(count); + for _ in 0..count { + ranges.push(generate_port_range(d)?); + } + Some(ranges.join(",")) +} + +/// [`generate_port_list`] as a standalone [`ValueGenerator`]. +pub struct PortListGenerator; + +impl ValueGenerator for PortListGenerator { + type Output = String; + + fn generate(&self, d: &mut D) -> Option { + generate_port_list(d) + } +} + 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))?); @@ -266,6 +398,280 @@ pub fn generate_v6_prefixes(d: &mut D, count: u16) -> Option bool { + let len = u32::from(a_len.min(b_len)); + let mask = u32::MAX.unbounded_shl(32 - len); + (a_base & mask) == (b_base & mask) +} + +fn v6_overlaps(a_base: u128, a_len: u8, b_base: u128, b_len: u8) -> bool { + let len = u32::from(a_len.min(b_len)); + let mask = u128::MAX.unbounded_shl(128 - len); + (a_base & mask) == (b_base & mask) +} + +/// Whether a prefix overlaps any reserved block. Exposed for this module's own tests only; the +/// generators go through the skip-ahead helpers below. +#[cfg(test)] +pub(crate) fn test_only_v4_is_reserved(base: u32, len: u8) -> bool { + RESERVED_V4 + .iter() + .any(|&(r_base, r_len)| v4_overlaps(base, len, r_base, r_len)) +} + +#[cfg(test)] +pub(crate) fn test_only_v6_is_reserved(base: u128, len: u8) -> bool { + RESERVED_V6 + .iter() + .any(|&(r_base, r_len)| v6_overlaps(base, len, r_base, r_len)) +} + +/// The first prefix base at or after `base` that is not reserved. +/// +/// Stepping one prefix at a time would be unusable: `240.0.0.0/4` alone holds 2^28 `/32`s. So on +/// a hit, jump past the whole offending block instead, aligned back down to a prefix boundary. +fn v4_skip_reserved(mut base: u32, len: u8) -> u32 { + let align = u32::MAX.unbounded_shl(u32::from(32 - len)); + let step = 1_u32.unbounded_shl(u32::from(32 - len)); + // At most one pass per reserved block: each jump lands strictly past the block it skipped, and + // the only way to revisit lower addresses is the single wrap at the top of the space. + for _ in 0..=RESERVED_V4.len() { + let Some(&(r_base, r_len)) = RESERVED_V4 + .iter() + .find(|&&(r_base, r_len)| v4_overlaps(base, len, r_base, r_len)) + else { + return base; + }; + // Round *up* to the next prefix boundary past the reserved block. Rounding down would + // land back inside the block whenever the block is narrower than the prefix being + // generated -- a `/8` containing `169.254.0.0/16`, for instance. + let block_end = r_base | !u32::MAX.unbounded_shl(u32::from(32 - r_len)); + base = block_end.wrapping_add(step) & align; + } + // Falling out of the loop would mean returning a reserved base, which surfaces downstream as a + // validation failure a long way from its cause. Say so here instead. + unreachable!( + "v4 reserved-block skip did not converge at /{len}; the loop bound assumes every jump \ + lands strictly past the block it skipped, so a new entry in RESERVED_V4 has broken that" + ) +} + +fn v6_skip_reserved(mut base: u128, len: u8) -> u128 { + let align = u128::MAX.unbounded_shl(u32::from(128 - len)); + let step = 1_u128.unbounded_shl(u32::from(128 - len)); + for _ in 0..=RESERVED_V6.len() { + let Some(&(r_base, r_len)) = RESERVED_V6 + .iter() + .find(|&&(r_base, r_len)| v6_overlaps(base, len, r_base, r_len)) + else { + return base; + }; + let block_end = r_base | !u128::MAX.unbounded_shl(u32::from(128 - r_len)); + base = block_end.wrapping_add(step) & align; + } + // See [`v4_skip_reserved`]. + unreachable!( + "v6 reserved-block skip did not converge at /{len}; the loop bound assumes every jump \ + lands strictly past the block it skipped, so a new entry in RESERVED_V6 has broken that" + ) +} + +/// Generate `count` distinct IPv4 prefixes, none overlapping a reserved block. +/// +/// Use this wherever a prefix ends up in a `VpcExpose`; [`UniqueV4CidrGenerator`] is fine for +/// contexts with no such restriction (ACL matches, for instance). +pub struct RoutableV4CidrGenerator { + count: u16, + mask: u8, +} + +impl RoutableV4CidrGenerator { + /// # Panics + /// + /// Panics unless `mask` is in `MIN_ROUTABLE_MASK..=32`. + #[must_use] + pub fn new(count: u16, mask: u8) -> Self { + assert!( + (MIN_ROUTABLE_MASK..=32).contains(&mask), + "illegal v4 prefix length /{mask}" + ); + Self { count, mask } + } +} + +impl ValueGenerator for RoutableV4CidrGenerator { + type Output = Vec; + + fn generate(&self, d: &mut D) -> Option { + if self.count == 0 { + return Some(Vec::new()); + } + let step = 1_u32.unbounded_shl(u32::from(32 - self.mask)); + let seed = d.gen_u32(Bound::Included(&0), Bound::Included(&u32::MAX))?; + // Align the seed down to a prefix boundary so the walk stays aligned. + let mut base = seed & u32::MAX.unbounded_shl(u32::from(32 - self.mask)); + + let mut cidrs = Vec::with_capacity(usize::from(self.count)); + for _ in 0..self.count { + base = v4_skip_reserved(base, self.mask); + cidrs.push(format!("{}/{}", Ipv4Addr::from(base), self.mask)); + base = base.wrapping_add(step); + } + Some(cidrs) + } +} + +/// Generate `count` distinct IPv6 prefixes, none overlapping a reserved block. +pub struct RoutableV6CidrGenerator { + count: u16, + mask: u8, +} + +impl RoutableV6CidrGenerator { + /// # Panics + /// + /// Panics unless `mask` is in `MIN_ROUTABLE_MASK..=128`. + #[must_use] + pub fn new(count: u16, mask: u8) -> Self { + assert!( + (MIN_ROUTABLE_MASK..=128).contains(&mask), + "illegal v6 prefix length /{mask}" + ); + Self { count, mask } + } +} + +impl ValueGenerator for RoutableV6CidrGenerator { + type Output = Vec; + + fn generate(&self, d: &mut D) -> Option { + if self.count == 0 { + return Some(Vec::new()); + } + let step = 1_u128.unbounded_shl(u32::from(128 - self.mask)); + let seed = d.gen_u128(Bound::Included(&0), Bound::Included(&u128::MAX))?; + let mut base = seed & u128::MAX.unbounded_shl(u32::from(128 - self.mask)); + + let mut cidrs = Vec::with_capacity(usize::from(self.count)); + for _ in 0..self.count { + base = v6_skip_reserved(base, self.mask); + cidrs.push(format!("{}/{}", Ipv6Addr::from(base), self.mask)); + base = base.wrapping_add(step); + } + Some(cidrs) + } +} + +/// Hands out mutually non-overlapping, non-reserved prefixes. +/// +/// `VpcManifest::validate` rejects a manifest whose exposes overlap each other, so prefixes cannot +/// be drawn independently per expose -- something has to own the address space for the whole +/// config. Every prefix a pool returns has the same length as its siblings and a distinct base, +/// which makes non-overlap immediate rather than something to check. +/// +/// Uses interior mutability so it can be shared by `&` through the generator tree, since +/// [`ValueGenerator::generate`] takes `&self`. +pub struct PrefixPool { + v4_next: std::cell::Cell, + v6_next: std::cell::Cell, +} + +impl PrefixPool { + /// Block sizes handed out. Small enough that a config can hold plenty, large enough to be + /// realistic tenant blocks. + const V4_MASK: u8 = 24; + const V6_MASK: u8 = 64; + + /// Seed a pool at a driver-chosen point in the address space. + pub fn new(d: &mut D) -> Option { + let v4_seed = d.gen_u32(Bound::Included(&0), Bound::Included(&u32::MAX))?; + let v6_seed = d.gen_u128(Bound::Included(&0), Bound::Included(&u128::MAX))?; + Some(Self { + v4_next: std::cell::Cell::new( + v4_seed & u32::MAX.unbounded_shl(u32::from(32 - Self::V4_MASK)), + ), + v6_next: std::cell::Cell::new( + v6_seed & u128::MAX.unbounded_shl(u32::from(128 - Self::V6_MASK)), + ), + }) + } + + /// The next unused IPv4 prefix. + pub fn next_v4(&self) -> String { + let base = v4_skip_reserved(self.v4_next.get(), Self::V4_MASK); + let step = 1_u32.unbounded_shl(u32::from(32 - Self::V4_MASK)); + self.v4_next.set(base.wrapping_add(step)); + format!("{}/{}", Ipv4Addr::from(base), Self::V4_MASK) + } + + /// The next unused IPv6 prefix. + pub fn next_v6(&self) -> String { + let base = v6_skip_reserved(self.v6_next.get(), Self::V6_MASK); + let step = 1_u128.unbounded_shl(u32::from(128 - Self::V6_MASK)); + self.v6_next.set(base.wrapping_add(step)); + format!("{}/{}", Ipv6Addr::from(base), Self::V6_MASK) + } + + /// `count` unused prefixes, all of one family. + pub fn take(&self, count: u16, v4: bool) -> Vec { + (0..count) + .map(|_| if v4 { self.next_v4() } else { self.next_v6() }) + .collect() + } +} + +/// [`generate_prefixes`], restricted to prefixes legal inside a `VpcExpose`. +pub fn generate_routable_prefixes( + d: &mut D, + v4_count: u16, + v6_count: u16, +) -> Option> { + let mut prefixes = Vec::with_capacity(usize::from(v4_count) + usize::from(v6_count)); + if v4_count > 0 { + let mask = d.gen_u8(Bound::Included(&MIN_ROUTABLE_MASK), Bound::Included(&32))?; + prefixes.extend(RoutableV4CidrGenerator::new(v4_count, mask).generate(d)?); + } + if v6_count > 0 { + let mask = d.gen_u8(Bound::Included(&MIN_ROUTABLE_MASK), Bound::Included(&128))?; + prefixes.extend(RoutableV6CidrGenerator::new(v6_count, mask).generate(d)?); + } + Some(prefixes) +} + pub fn generate_prefixes( d: &mut D, v4_count: u16, @@ -294,6 +700,99 @@ mod test { _ => 1000, }; + /// A generated name must be a legal RFC 1123 DNS label, which is the whole reason this + /// generator exists in place of `produce::()`. + #[test] + fn test_k8s_name_generator_is_rfc1123() { + bolero::check!() + .with_type::() + .for_each(|name| { + let name = name.as_str(); + assert!(!name.is_empty(), "empty name"); + assert!(name.len() <= 63, "name too long: {name}"); + assert!( + name.bytes() + .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == b'-'), + "illegal character in {name}" + ); + let first = name.bytes().next().unwrap(); + let last = name.bytes().next_back().unwrap(); + assert!(first.is_ascii_alphanumeric(), "leading '-' in {name}"); + assert!(last.is_ascii_alphanumeric(), "trailing '-' in {name}"); + }); + } + + /// Both spellings a port list can take must parse, and the range spelling must be ordered. + #[test] + fn test_generated_port_lists_parse() { + bolero::check!() + .with_generator(crate::bolero::support::PortListGenerator) + .for_each(|ports| { + for range in ports.split(',') { + let range = range + .parse::() + .unwrap_or_else(|e| panic!("failed to parse port range {range}: {e}")); + assert!(range.start() <= range.end()); + } + }); + } + + /// Routable prefixes must be distinct and must avoid every reserved block. + /// + /// `config`'s validation is the real authority here; this test exists so that a regression in + /// the skip-ahead logic fails locally and quickly rather than as a mystifying failure (or, as + /// happened once, a hang) inside a downstream config property test. + #[test] + fn test_routable_v4_prefixes_avoid_reserved() { + for mask in 8..=32 { + let generator = crate::bolero::support::RoutableV4CidrGenerator::new(8, mask); + bolero::check!() + .with_generator(generator) + .with_iterations(ITERATIONS) + .for_each(|cidrs| { + let mut seen = std::collections::HashSet::new(); + assert_eq!(cidrs.len(), 8); + for cidr in cidrs { + assert!(seen.insert(cidr), "duplicate prefix {cidr}"); + let (addr, len) = cidr.split_once('/').unwrap(); + let addr = addr.parse::().unwrap(); + let len = len.parse::().unwrap(); + assert_eq!(len, mask); + assert!( + !crate::bolero::support::test_only_v4_is_reserved(addr.to_bits(), len), + "generated reserved prefix {cidr}" + ); + } + }); + } + } + + #[test] + #[cfg_attr(miri, ignore = "just too slow on miri")] + fn test_routable_v6_prefixes_avoid_reserved() { + for mask in [8, 16, 32, 48, 64, 96, 127, 128] { + let generator = crate::bolero::support::RoutableV6CidrGenerator::new(8, mask); + bolero::check!() + .with_generator(generator) + .with_iterations(ITERATIONS) + .for_each(|cidrs| { + let mut seen = std::collections::HashSet::new(); + assert_eq!(cidrs.len(), 8); + for cidr in cidrs { + assert!(seen.insert(cidr), "duplicate prefix {cidr}"); + let (addr, len) = cidr.split_once('/').unwrap(); + let addr = addr.parse::().unwrap(); + let len = len.parse::().unwrap(); + assert_eq!(len, mask); + assert!( + !crate::bolero::support::test_only_v6_is_reserved(addr.to_bits(), len), + "generated reserved prefix {cidr}" + ); + } + }); + } + } + #[test] fn test_unique_v4_cidr_generator() { for mask in 0..=32 { From 1c799ec5e1dff8205c38a266548259b5bedf500e Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 24 Jul 2026 16:40:12 -0600 Subject: [PATCH 2/8] test(k8s-intf): generate gateway groups with multiple members `TypeGenerator for GatewayAgentGroups` drew a member count in `0..=10` and then pushed exactly one member, so no generated group ever had more than one. That left `GwGroup::add_member`'s duplicate-name and duplicate-address checks unexercised, and the rank-to-community lookup in `validate_gw_groups` only ever reached rank 0. It also spent a driver draw on a value it discarded, which degrades shrinking. Generate the requested number of members, repairing name and address collisions in place (lengthen, walk) rather than redrawing, so every value the driver produced still contributes. Also move these impls under `LegalValue` and draw names from `K8sName`. They were bare `TypeGenerator` impls on the CRD types while restricting priority to `0..=10` -- narrower than "all legal values" -- and simultaneously drawing names from `produce::()`, which is wider: arbitrary Unicode is not a legal k8s name. They were on the wrong side of both rules at once. `LegalValueGroupsTableGenerator` builds the whole `spec.groups` map and returns the group names alongside it, which a later commit needs so that peerings can reference a group that exists. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- k8s-intf/src/bolero/gwgroups.rs | 114 ++++++++++++++++++++++++++++---- k8s-intf/src/bolero/spec.rs | 21 +++--- 2 files changed, 111 insertions(+), 24 deletions(-) diff --git a/k8s-intf/src/bolero/gwgroups.rs b/k8s-intf/src/bolero/gwgroups.rs index 5cf5dc82c2..d620829500 100644 --- a/k8s-intf/src/bolero/gwgroups.rs +++ b/k8s-intf/src/bolero/gwgroups.rs @@ -1,45 +1,135 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright Open Network Fabric Authors -use bolero::{Driver, TypeGenerator}; +use bolero::{Driver, TypeGenerator, ValueGenerator}; use net::ipv4::UnicastIpv4Addr; +use std::collections::BTreeSet; +use std::net::Ipv4Addr; use std::ops::Bound; use crate::bolero::LegalValue; +use crate::bolero::support::{K8sName, K8sNameGenerator}; use crate::gateway_agent_crd::{ GatewayAgentGatewayGroups, GatewayAgentGroups, GatewayAgentGroupsMembers, }; +/// Largest number of members a generated group may have. +/// +/// A group's members are ranked by position after sorting, and `ExternalConfig::validate` demands +/// a community for every rank, so `LegalValue` must supply at least this many +/// communities for the resulting config to validate. +pub const MAX_GROUP_MEMBERS: usize = 10; + impl TypeGenerator for LegalValue { fn generate(driver: &mut D) -> Option { let g = GatewayAgentGatewayGroups { - name: driver.produce::(), + name: driver + .produce::>()? + .map(crate::bolero::support::K8sName::take), priority: Some(driver.gen_u32(Bound::Included(&0), Bound::Included(&10))?), }; Some(LegalValue(g)) } } -impl TypeGenerator for GatewayAgentGroupsMembers { +/// Generate one member of a gateway group. +/// +/// `priority` is deliberately narrow (`0..=10`) so that generated groups contain ties, which is +/// what exercises the name/address tiebreak in `GwGroupMember`'s `Ord`. That restriction is why +/// this is a `LegalValue` rather than a bare `TypeGenerator` on the CRD type. +impl TypeGenerator for LegalValue { fn generate(driver: &mut D) -> Option { let gmember = GatewayAgentGroupsMembers { - name: driver.produce::()?, + name: driver.produce::()?.take(), priority: driver.gen_u32(Bound::Included(&0), Bound::Included(&10))?, vtep_ip: driver.produce::()?.to_string(), }; - Some(gmember) + Some(LegalValue(gmember)) } } -impl TypeGenerator for GatewayAgentGroups { +/// Generate a gateway group with `0..=MAX_GROUP_MEMBERS` members. +/// +/// `GwGroup::add_member` rejects duplicate member names *and* duplicate member addresses, so both +/// have to be unique within a group for the value to be legal. Collisions are repaired in place +/// (lengthen the name, walk the address) rather than by redrawing, so that every value the driver +/// produced still contributes and a collision costs a bounded amount of work. +impl TypeGenerator for LegalValue { fn generate(driver: &mut D) -> Option { - let num_members = driver.gen_usize(Bound::Included(&0), Bound::Included(&10))?; - let mut members = vec![]; - if num_members > 0 { - members.push(driver.produce::()?); + let num_members = + driver.gen_usize(Bound::Included(&0), Bound::Included(&MAX_GROUP_MEMBERS))?; + + let mut members = Vec::with_capacity(num_members); + let mut names = BTreeSet::new(); + let mut addrs = BTreeSet::new(); + + for _ in 0..num_members { + let mut member = driver + .produce::>()? + .take(); + + while !names.insert(member.name.clone()) { + member.name.push('x'); + } + + // `LegalValue` draws a unicast address, so parsing it back + // cannot fail. Walk it on collision, skipping over the non-unicast space. + #[allow(clippy::unwrap_used)] + let mut addr = member.vtep_ip.parse::().unwrap().to_bits(); + while !addrs.insert(addr) { + addr = addr.wrapping_add(1); + let candidate = Ipv4Addr::from(addr); + if candidate.is_multicast() || candidate.is_broadcast() { + addr = 0x0100_0000; + } + } + member.vtep_ip = Ipv4Addr::from(addr).to_string(); + + members.push(member); } - Some(GatewayAgentGroups { + + Some(LegalValue(GatewayAgentGroups { members: Some(members), - }) + })) + } +} + +/// Generate the `spec.groups` map along with the group names, which peerings must reference. +/// +/// Returns the map plus the sorted list of names, so the caller can hand the names to +/// [`crate::bolero::peering::LegalValuePeeringsGenerator`]: a peering naming a group that is not +/// in this table fails `ExternalConfig::validate`. +pub struct LegalValueGroupsTableGenerator { + count: usize, +} + +impl LegalValueGroupsTableGenerator { + #[must_use] + pub fn new(count: usize) -> Self { + Self { count } + } +} + +impl ValueGenerator for LegalValueGroupsTableGenerator { + type Output = ( + std::collections::BTreeMap, + Vec, + ); + + fn generate(&self, d: &mut D) -> Option { + let mut groups = std::collections::BTreeMap::new(); + let name_gen = K8sNameGenerator::new(1, 12); + let mut names = Vec::with_capacity(self.count); + for i in 0..self.count { + // Suffix with the index so group names are distinct without a retry loop; + // `GwGroupTable::add_group` rejects duplicates. + let name = format!("{}-{i}", name_gen.generate(d)?); + groups.insert( + name.clone(), + d.produce::>()?.take(), + ); + names.push(name); + } + Some((groups, names)) } } diff --git a/k8s-intf/src/bolero/spec.rs b/k8s-intf/src/bolero/spec.rs index e89abbda46..4792bf00cf 100644 --- a/k8s-intf/src/bolero/spec.rs +++ b/k8s-intf/src/bolero/spec.rs @@ -8,11 +8,10 @@ use bolero::{Driver, TypeGenerator, ValueGenerator}; use lpm::prefix::Prefix; +use crate::bolero::gwgroups::{LegalValueGroupsTableGenerator, MAX_GROUP_MEMBERS}; use crate::bolero::peering::LegalValuePeeringsGenerator; use crate::bolero::{LegalValue, SubnetMap, VpcSubnetMap}; -use crate::gateway_agent_crd::{ - GatewayAgentGateway, GatewayAgentGroups, GatewayAgentSpec, GatewayAgentVpcs, -}; +use crate::gateway_agent_crd::{GatewayAgentGateway, GatewayAgentSpec, GatewayAgentVpcs}; fn extract_subnets(vpcs: &BTreeMap) -> VpcSubnetMap { let mut vpc_subnets = VpcSubnetMap::new(); @@ -86,17 +85,15 @@ 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_groups = d.gen_usize(Bound::Included(&1), Bound::Included(&6))?; + let (groups, _) = LegalValueGroupsTableGenerator::new(num_groups).generate(d)?; - let num_communities = d.gen_usize(Bound::Included(&0), Bound::Included(&9))?; + // A group of N members occupies ranks `0..N`, and `ExternalConfig::validate` requires a + // community at every rank it finds. Sizing the table to the largest group a generated + // spec can hold keeps that satisfied without coupling to how many members were drawn. let mut communities = BTreeMap::new(); - for i in 0..=num_communities { - let community = format!("65000:{}", 100 + i); - communities.insert(i.to_string(), community); + for i in 0..MAX_GROUP_MEMBERS { + communities.insert(i.to_string(), format!("65000:{}", 100 + i)); } Some(LegalValue(GatewayAgentSpec { From 76d827edec125e2b14df068da8696116c5ddb858 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 24 Jul 2026 16:41:40 -0600 Subject: [PATCH 3/8] test(k8s-intf): add peering-scoped ACL generator The peerings generator set `acl: None` with a FIXME, so the ACL converter -- the largest single piece of the intake path -- was covered only by hand-written examples. An ACL rule is only legal relative to its peering, which is why this is a `ValueGenerator` carrying the two VPC names and the subnet map as borrowed context rather than a `TypeGenerator` on the CRD type: - `from`/`to` must name the peering's two VPCs, or be blank to be inferred, so the generator enumerates the legal shapes and knows which VPC each resolves to; both spellings of blank (absent and empty string) are covered; - a `vpcSubnet` reference in `match.src` resolves against the from-side VPC's subnets and one in `match.dst` against the to-side VPC's, so each side must draw from the map its own resolved VPC. Ports are only emitted for TCP and UDP, and one address family is chosen per rule, because `AclPattern::validate` rejects ports on other protocols and rejects a rule whose `src` and `dst` disagree on IP version. The side names come from the generated map rather than from the chosen pair, since `VpcPeering::try_from` derives `left`/`right` by iterating it and so orders them lexicographically. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- config/src/converters/k8s/config/peering.rs | 7 +- k8s-intf/src/bolero/acl.rs | 314 ++++++++++++++++++++ k8s-intf/src/bolero/mod.rs | 1 + k8s-intf/src/bolero/peering.rs | 16 +- 4 files changed, 335 insertions(+), 3 deletions(-) create mode 100644 k8s-intf/src/bolero/acl.rs diff --git a/config/src/converters/k8s/config/peering.rs b/config/src/converters/k8s/config/peering.rs index 45ae4a8e29..8dbb28d046 100644 --- a/config/src/converters/k8s/config/peering.rs +++ b/config/src/converters/k8s/config/peering.rs @@ -174,8 +174,11 @@ mod test { .with_generator(generator) .for_each(|peering| { let peering_name = "test-peering"; - let peering = VpcPeering::try_from((&subnets, peering_name, peering)).unwrap(); - assert_eq!(peering.name, peering_name); + let converted = VpcPeering::try_from((&subnets, peering_name, peering)).unwrap(); + assert_eq!(converted.name, peering_name); + // The ACL the generator produced is relative to this peering's two VPCs, so it + // must survive conversion rather than being dropped. + assert_eq!(converted.acl.is_some(), peering.acl.is_some()); // Rest of the assertions come from the types and the unwrap in the conversion above }); } diff --git a/k8s-intf/src/bolero/acl.rs b/k8s-intf/src/bolero/acl.rs new file mode 100644 index 0000000000..bd1e2ed89f --- /dev/null +++ b/k8s-intf/src/bolero/acl.rs @@ -0,0 +1,314 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +//! Generators for the peering-scoped ACL section of the CRD. +//! +//! An ACL rule is only legal *relative to its peering*: `from`/`to` must name the peering's two +//! VPCs (or be blank, to be inferred), and a `vpcSubnet` reference in `match.src` resolves against +//! the from-side VPC's subnets while one in `match.dst` resolves against the to-side VPC's. None +//! of that is expressible as a `TypeGenerator` on the CRD type, so this is a `ValueGenerator` +//! carrying the peering's two VPC names and the subnet map as borrowed context. + +use std::ops::Bound; + +use bolero::{Driver, ValueGenerator}; + +use crate::bolero::support::{choose, generate_port_list, generate_prefixes}; +use crate::bolero::{SubnetMap, VpcSubnetMap}; +use crate::gateway_agent_crd::{ + GatewayAgentPeeringsAcl, GatewayAgentPeeringsAclDefault, GatewayAgentPeeringsAclRules, + GatewayAgentPeeringsAclRulesAction, GatewayAgentPeeringsAclRulesMatch, + GatewayAgentPeeringsAclRulesMatchDst, GatewayAgentPeeringsAclRulesMatchSrc, + GatewayAgentPeeringsAclRulesScope, +}; + +/// Largest number of rules a generated ACL may hold. +const MAX_RULES: usize = 8; +/// Largest number of match entries per rule side. +const MAX_MATCH_ENTRIES: u16 = 4; + +/// Every legal spelling of a rule's `from`/`to` pair, given the peering's two VPC names. +/// +/// `complete_from_to` accepts a matching pair in either direction, or one side matching with the +/// other blank (inferred). Blank is spelled two ways in the CRD -- absent and empty-string -- and +/// the converter treats them identically via `unwrap_or_default`, so both are generated. +#[derive(Debug, Clone, Copy)] +enum FromToShape { + LeftToRight, + RightToLeft, + LeftOnlyAbsent, + LeftOnlyEmpty, + RightOnlyAbsent, + OnlyToLeftAbsent, + OnlyToRightEmpty, +} + +const FROM_TO_SHAPES: &[FromToShape] = &[ + FromToShape::LeftToRight, + FromToShape::RightToLeft, + FromToShape::LeftOnlyAbsent, + FromToShape::LeftOnlyEmpty, + FromToShape::RightOnlyAbsent, + FromToShape::OnlyToLeftAbsent, + FromToShape::OnlyToRightEmpty, +]; + +impl FromToShape { + /// The `(from, to)` field values for this shape. + fn fields(self, left: &str, right: &str) -> (Option, Option) { + match self { + FromToShape::LeftToRight => (Some(left.to_string()), Some(right.to_string())), + FromToShape::RightToLeft => (Some(right.to_string()), Some(left.to_string())), + FromToShape::LeftOnlyAbsent => (Some(left.to_string()), None), + FromToShape::LeftOnlyEmpty => (Some(left.to_string()), Some(String::new())), + FromToShape::RightOnlyAbsent => (Some(right.to_string()), None), + FromToShape::OnlyToLeftAbsent => (None, Some(left.to_string())), + FromToShape::OnlyToRightEmpty => (Some(String::new()), Some(right.to_string())), + } + } + + /// The VPC names the converter will resolve this shape to, in `(from, to)` order. + /// + /// This mirrors `complete_from_to`'s inference so the generator knows which subnet map each + /// side of `match` must draw its `vpcSubnet` references from. + fn resolved<'a>(self, left: &'a str, right: &'a str) -> (&'a str, &'a str) { + match self { + FromToShape::LeftToRight + | FromToShape::LeftOnlyAbsent + | FromToShape::LeftOnlyEmpty + | FromToShape::OnlyToRightEmpty => (left, right), + FromToShape::RightToLeft + | FromToShape::RightOnlyAbsent + | FromToShape::OnlyToLeftAbsent => (right, left), + } + } +} + +/// One entry of a rule's `match.src` / `match.dst` list. +/// +/// The converter accepts `cidr` xor `vpcSubnet` (or neither, meaning "any address"), each with an +/// optional non-empty `ports` list. +struct MatchEntry { + cidr: Option, + vpc_subnet: Option, + ports: Option>, +} + +impl From for GatewayAgentPeeringsAclRulesMatchSrc { + fn from(e: MatchEntry) -> Self { + Self { + cidr: e.cidr, + ports: e.ports, + vpc_subnet: e.vpc_subnet, + } + } +} + +impl From for GatewayAgentPeeringsAclRulesMatchDst { + fn from(e: MatchEntry) -> Self { + Self { + cidr: e.cidr, + ports: e.ports, + vpc_subnet: e.vpc_subnet, + } + } +} + +/// Whether a rule's generated protocol supports port matching. +/// +/// `AclPattern::validate` rejects ports on anything but TCP and UDP, and a numeric `6`/`17` +/// normalizes to those variants, so this is decided from the *converted* protocol, not the string. +fn proto_supports_ports(proto: Option<&str>) -> bool { + matches!(proto, Some("tcp" | "udp" | "6" | "17")) +} + +/// Generate one side (`src` or `dst`) of a rule's match block. +/// +/// `subnets` must be the subnet map of the VPC that this side's `vpcSubnet` references resolve +/// against -- the from-side VPC for `src`, the to-side VPC for `dst`. `want_v4` fixes the address +/// family: `AclPattern::validate` requires `src` and `dst` to agree on it. `allow_ports` reflects +/// whether the rule's protocol supports port matching at all. +fn generate_match_side( + d: &mut D, + subnets: &SubnetMap, + want_v4: bool, + allow_ports: bool, +) -> Option> { + let count = d.gen_u16(Bound::Included(&0), Bound::Included(&MAX_MATCH_ENTRIES))?; + if count == 0 { + return Some(Vec::new()); + } + + let subnet_names: Vec<&String> = subnets.keys().collect(); + // One prefix per entry, all distinct, so a `cidr` entry never duplicates another. + let prefixes = if want_v4 { + generate_prefixes(d, count, 0)? + } else { + generate_prefixes(d, 0, count)? + }; + + let mut entries = Vec::with_capacity(usize::from(count)); + for prefix in prefixes { + // A present-but-empty ports list is rejected by the converter, so only ever emit + // `None` (match all ports) or a non-empty list. + let ports = if allow_ports && d.gen_bool(None)? { + Some(vec![generate_port_list(d)?]) + } else { + None + }; + + // Three legal address shapes: a CIDR, a named VPC subnet, or neither ("any address + // within the peering", which the converter routes through a separate code path). + let shape = d.gen_usize(Bound::Included(&0), Bound::Included(&2))?; + let (cidr, vpc_subnet) = match shape { + 0 => (Some(prefix), None), + 1 if !subnet_names.is_empty() => (None, Some(choose(d, &subnet_names)?.clone())), + // Falls through to "any address" when the VPC has no subnets to reference. + _ => (None, None), + }; + + // "Any address" with no ports means "match everything"; the converter short-circuits the + // whole side on it, discarding any other entry. Skip it unless it is the only entry, so + // that generated multi-entry sides stay meaningful. + if cidr.is_none() && vpc_subnet.is_none() && ports.is_none() && count > 1 { + continue; + } + + entries.push(MatchEntry { + cidr, + vpc_subnet, + ports, + }); + } + Some(entries) +} + +/// One of the protocol spellings the converter accepts. +/// +/// `Absent` is a distinct case from any string: it means "match any protocol", and it also decides +/// whether ports are legal on the rule. +enum ProtoChoice { + Absent, + Named(String), +} + +impl From for Option { + fn from(choice: ProtoChoice) -> Self { + match choice { + ProtoChoice::Absent => None, + ProtoChoice::Named(name) => Some(name), + } + } +} + +/// Protocols the converter accepts: absent, the two names, or a numeric value. +fn generate_proto(d: &mut D) -> Option { + match d.gen_usize(Bound::Included(&0), Bound::Included(&3))? { + 0 => Some(ProtoChoice::Absent), + 1 => Some(ProtoChoice::Named("tcp".to_string())), + 2 => Some(ProtoChoice::Named("udp".to_string())), + // Any u8 is accepted; 6 and 17 normalize to the Tcp/Udp variants rather than Other. + _ => Some(ProtoChoice::Named(d.produce::()?.to_string())), + } +} + +/// Generate a legal, peering-relative ACL. +/// +/// The generated ACL is legal with respect to *conversion*: `Acl::try_from` accepts it for the +/// peering it was built against. +pub struct LegalValueAclGenerator<'a> { + vpc_subnets: &'a VpcSubnetMap, + left_name: &'a str, + right_name: &'a str, +} + +impl<'a> LegalValueAclGenerator<'a> { + /// `left_name` and `right_name` must be the two VPC names of the peering this ACL belongs to. + #[must_use] + pub fn new(vpc_subnets: &'a VpcSubnetMap, left_name: &'a str, right_name: &'a str) -> Self { + Self { + vpc_subnets, + left_name, + right_name, + } + } +} + + +impl ValueGenerator for LegalValueAclGenerator<'_> { + type Output = GatewayAgentPeeringsAcl; + + fn generate(&self, d: &mut D) -> Option { + let default = match d.gen_usize(Bound::Included(&0), Bound::Included(&2))? { + 0 => GatewayAgentPeeringsAclDefault::Deny, + 1 => GatewayAgentPeeringsAclDefault::DenyUnlessExposed, + // The CRD's `""` member: documented to mean the same as "deny-unless-exposed". + _ => GatewayAgentPeeringsAclDefault::KopiumEmpty, + }; + + // `Acl::validate` rejects an ACL with no rules at all, so a present ACL always has one. + let num_rules = d.gen_usize(Bound::Included(&1), Bound::Included(&MAX_RULES))?; + let empty_map = SubnetMap::new(); + let mut rules = Vec::with_capacity(num_rules); + + for index in 0..num_rules { + let shape = choose(d, FROM_TO_SHAPES)?; + let (from, to) = shape.fields(self.left_name, self.right_name); + let (from_vpc, to_vpc) = shape.resolved(self.left_name, self.right_name); + + let src_subnets = self.vpc_subnets.get(from_vpc).unwrap_or(&empty_map); + let dst_subnets = self.vpc_subnets.get(to_vpc).unwrap_or(&empty_map); + + let r#match = if d.gen_bool(None)? { + let proto: Option = generate_proto(d)?.into(); + let allow_ports = proto_supports_ports(proto.as_deref()); + // One family per rule: `AclPattern::validate` rejects a rule whose `src` and `dst` + // disagree on IP version. + let want_v4 = d.gen_bool(None)?; + let src = generate_match_side(d, src_subnets, want_v4, allow_ports)?; + let dst = generate_match_side(d, dst_subnets, want_v4, allow_ports)?; + Some(GatewayAgentPeeringsAclRulesMatch { + src: Some(src.into_iter().map(Into::into).collect::>()) + .filter(|s: &Vec| !s.is_empty()), + dst: Some(dst.into_iter().map(Into::into).collect::>()) + .filter(|s: &Vec| !s.is_empty()), + proto, + }) + } else { + None + }; + + // Rule names must be unique within an ACL when non-empty (empty ones are exempt), so + // suffix with the index rather than relying on the name space being large. + let name = d + .produce::>()? + .map(|n| format!("{n}-{index}")); + + let scope = match d.gen_usize(Bound::Included(&0), Bound::Included(&3))? { + 0 => None, + 1 => Some(GatewayAgentPeeringsAclRulesScope::Flow), + 2 => Some(GatewayAgentPeeringsAclRulesScope::Packet), + _ => Some(GatewayAgentPeeringsAclRulesScope::KopiumEmpty), + }; + + rules.push(GatewayAgentPeeringsAclRules { + action: if d.gen_bool(None)? { + GatewayAgentPeeringsAclRulesAction::Allow + } else { + GatewayAgentPeeringsAclRulesAction::Deny + }, + from, + to, + log: d.produce::>()?, + r#match, + name, + scope, + }); + } + + Some(GatewayAgentPeeringsAcl { + default, + rules: Some(rules), + }) + } +} diff --git a/k8s-intf/src/bolero/mod.rs b/k8s-intf/src/bolero/mod.rs index 078ef00a18..fa18f725d1 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 cf7ad87af1..227a84beb2 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::LegalValueAclGenerator; use crate::bolero::expose::LegalValueExposeGenerator; use crate::bolero::{SubnetMap, VpcSubnetMap}; use crate::gateway_agent_crd::{GatewayAgentPeerings, GatewayAgentPeeringsPeering}; @@ -91,10 +92,23 @@ impl ValueGenerator for LegalValuePeeringsGenerator<'_> { .map(|i| Some((vpc_names[i].clone(), peerings_gens[i].generate(d)?))) .collect::>>()?; + // `VpcPeering::try_from` names the sides by iterating the `peering` map, so `left` is the + // lexicographically smaller of the two VPC names. The ACL's `from`/`to` must agree with + // that, so derive the names from the map rather than from `vpc_names`. + let mut side_names = peering.keys(); + let left_name = side_names.next()?; + let right_name = side_names.next()?; + + let acl = if d.gen_bool(None)? { + Some(LegalValueAclGenerator::new(self.vpc_subnets, left_name, right_name).generate(d)?) + } else { + None + }; + Some(GatewayAgentPeerings { gateway_group: Some(d.produce::()?), peering: Some(peering), - acl: None, // FIXME: Add a proper implementation when used + acl, }) } } From 0471be7d7beab88b96b94399766a826cdb809e2b Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 24 Jul 2026 16:42:17 -0600 Subject: [PATCH 4/8] test(k8s-intf): generate the remaining gateway and spec fields Four fields were pinned to `None`, two of them with FIXMEs, leaving converter code unreachable from any generated input: - `flowTableCapacity`, whose converter has a fallible `u32` -> `NonZero` narrowing. Zero is illegal, so it is clamped away here and left to the hostile generators; - `profiling` and `agentVersion`, which were simply never populated; - `config.fabricBFD`, which the converter fans out over every underlay BGP neighbor after the underlay has already been built. That post-hoc mutation is invisible to a per-subtree converter test, so it needs to come from the top. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- k8s-intf/src/bolero/gateway.rs | 12 +++++++++--- k8s-intf/src/bolero/spec.rs | 15 ++++++++++++--- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/k8s-intf/src/bolero/gateway.rs b/k8s-intf/src/bolero/gateway.rs index ceb92d1b32..39bcc61027 100644 --- a/k8s-intf/src/bolero/gateway.rs +++ b/k8s-intf/src/bolero/gateway.rs @@ -14,7 +14,7 @@ use crate::bolero::Normalize; use crate::gateway_agent_crd::{ GatewayAgentGateway, GatewayAgentGatewayGroups, GatewayAgentGatewayInterfaces, - GatewayAgentGatewayLogs, GatewayAgentGatewayNeighbors, + GatewayAgentGatewayLogs, GatewayAgentGatewayNeighbors, GatewayAgentGatewayProfiling, }; impl TypeGenerator for LegalValue { @@ -42,12 +42,18 @@ impl TypeGenerator for LegalValue { Some(LegalValue(GatewayAgentGateway { asn: Some(d.gen_u32(Bound::Included(&1), Bound::Unbounded)?), - flow_table_capacity: None, + // The converter requires this to be non-zero and to fit in a `usize`; zero is an + // illegal value, so it is left to the hostile generators to produce. + flow_table_capacity: d.produce::>()?.map(|capacity| capacity.max(1)), groups: Some(groups), logs: Some(d.produce::>()?.take()), interfaces: Some(interfaces).filter(|i| !i.is_empty()), neighbors: Some(neighbors).filter(|n| !n.is_empty()), - profiling: None, // FIXME(mvachhar) Add a proper implementation + profiling: d + .produce::>()? + .map(|enabled| GatewayAgentGatewayProfiling { + enabled: Some(enabled), + }), protocol_ip: Some(format!( "{}/{}", d.produce::()?, diff --git a/k8s-intf/src/bolero/spec.rs b/k8s-intf/src/bolero/spec.rs index 4792bf00cf..e4d433abc5 100644 --- a/k8s-intf/src/bolero/spec.rs +++ b/k8s-intf/src/bolero/spec.rs @@ -10,8 +10,11 @@ use lpm::prefix::Prefix; use crate::bolero::gwgroups::{LegalValueGroupsTableGenerator, MAX_GROUP_MEMBERS}; use crate::bolero::peering::LegalValuePeeringsGenerator; +use crate::bolero::support::K8sName; use crate::bolero::{LegalValue, SubnetMap, VpcSubnetMap}; -use crate::gateway_agent_crd::{GatewayAgentGateway, GatewayAgentSpec, GatewayAgentVpcs}; +use crate::gateway_agent_crd::{ + GatewayAgentConfig, GatewayAgentGateway, GatewayAgentSpec, GatewayAgentVpcs, +}; fn extract_subnets(vpcs: &BTreeMap) -> VpcSubnetMap { let mut vpc_subnets = VpcSubnetMap::new(); @@ -97,8 +100,14 @@ impl TypeGenerator for LegalValue { } Some(LegalValue(GatewayAgentSpec { - agent_version: None, - config: None, + agent_version: d.produce::>()?.map(K8sName::take), + // `fabricBFD` fans out over every underlay BGP neighbor in the converter, so it is + // worth generating rather than pinning to `None`. + config: d + .produce::>()? + .map(|fabric_bfd| GatewayAgentConfig { + fabric_bfd: Some(fabric_bfd), + }), groups: Some(groups), communities: Some(communities), gateway: Some(d.produce::>()?.take()), From 108cd14d620c4c88f40e732a60cbc5299cbb9d26 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 24 Jul 2026 16:42:54 -0600 Subject: [PATCH 5/8] test(k8s-intf): make generated specs pass config validation `LegalValue` produced specs that converted but could not validate, so no generated input had ever reached `ExternalConfig::validate`. The blocker was `gatewayGroup`, drawn from `produce::()` while the group table used generated names, so `check_peering_gwgroups_exist` rejected almost every spec. "Legal" meant "convertible", which is strictly weaker than what the system means by legal. Fixing that exposed six more rules that are not local to any one subtree, so each is now upheld by whichever generator owns the relevant scope: - a peering's `gatewayGroup` must name a group in the table, so the group table is generated first and its names handed down; - `Vpc::check_peering_count` rejects a VPC pair peering twice, so pair selection moves up to the spec generator, which draws pairs without replacement; - `VpcManifest::validate` allows at most one default expose per manifest and `Peering::validate` forbids one on both sides of a peering, so default placement is decided by the caller that owns both sides; - `VpcRouteTable::validate` allows a VPC only one default destination across all its peerings, and a default expose on one side is a default destination for the other, so the spec generator tracks which VPCs have been offered one; - that same rule requires overlapping destinations to share a gateway group, and a default expose reaches the route table as `0.0.0.0/0`, which overlaps every v4 destination. So a spec can have default exposes or a diversity of gateway groups, but not both; each shape is generated half the time; - `VpcManifest::validate` rejects overlapping exposes, so addresses come from one `PrefixPool` shared by the whole spec, making non-overlap hold by construction. Two restrictions are deliberate and documented at the call sites, since lifting them needs context the generators do not have yet: ACL address matches and `scope: flow` (coupled to the exposes via `manifest_coverage_set`), and NAT-mode exposes and `vpcSubnet` references (coupled to prefix sizing and to the VPC subnet generator). Both remain covered at the conversion level. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- config/src/converters/k8s/config/peering.rs | 8 +- k8s-intf/src/bolero/acl.rs | 108 +++++++++---- k8s-intf/src/bolero/expose.rs | 102 +++++++++++- k8s-intf/src/bolero/peering.rs | 165 ++++++++++++++++++-- k8s-intf/src/bolero/spec.rs | 90 +++++++++-- 5 files changed, 409 insertions(+), 64 deletions(-) diff --git a/config/src/converters/k8s/config/peering.rs b/config/src/converters/k8s/config/peering.rs index 8dbb28d046..4b8d0c6f6d 100644 --- a/config/src/converters/k8s/config/peering.rs +++ b/config/src/converters/k8s/config/peering.rs @@ -169,15 +169,17 @@ mod test { ]), ), ]); - let generator = LegalValuePeeringsGenerator::new(&subnets).unwrap(); + let gwgroups = ["group-a".to_string(), "group-b".to_string()]; + let generator = LegalValuePeeringsGenerator::new(&subnets, &gwgroups).unwrap(); bolero::check!() .with_generator(generator) .for_each(|peering| { let peering_name = "test-peering"; let converted = VpcPeering::try_from((&subnets, peering_name, peering)).unwrap(); assert_eq!(converted.name, peering_name); - // The ACL the generator produced is relative to this peering's two VPCs, so it - // must survive conversion rather than being dropped. + // The generator only ever names a group it was given, and the ACL it generates is + // relative to this peering's two VPCs, so both must survive conversion. + assert!(gwgroups.contains(&converted.gwgroup)); assert_eq!(converted.acl.is_some(), peering.acl.is_some()); // Rest of the assertions come from the types and the unwrap in the conversion above }); diff --git a/k8s-intf/src/bolero/acl.rs b/k8s-intf/src/bolero/acl.rs index bd1e2ed89f..acc8b83cba 100644 --- a/k8s-intf/src/bolero/acl.rs +++ b/k8s-intf/src/bolero/acl.rs @@ -140,15 +140,14 @@ fn generate_match_side( } let subnet_names: Vec<&String> = subnets.keys().collect(); - // One prefix per entry, all distinct, so a `cidr` entry never duplicates another. - let prefixes = if want_v4 { - generate_prefixes(d, count, 0)? - } else { - generate_prefixes(d, 0, count)? - }; - let mut entries = Vec::with_capacity(usize::from(count)); - for prefix in prefixes { + // Draw each entry's shape and ports first, then generate exactly as many prefixes as the + // `cidr` entries need. Generating one per entry up front would spend driver draws on + // prefixes the `vpcSubnet` and "any address" entries discard, which costs nothing in output + // but blunts shrinking; asking for them in one call is what keeps them distinct, so a `cidr` + // entry never duplicates another. + let mut shapes = Vec::with_capacity(usize::from(count)); + for _ in 0..count { // A present-but-empty ports list is rejected by the converter, so only ever emit // `None` (match all ports) or a non-empty list. let ports = if allow_ports && d.gen_bool(None)? { @@ -157,15 +156,30 @@ fn generate_match_side( None }; - // Three legal address shapes: a CIDR, a named VPC subnet, or neither ("any address - // within the peering", which the converter routes through a separate code path). + // Three legal address shapes: shape 0 is a CIDR, shape 1 a named VPC subnet, and shape 2 + // neither ("any address within the peering", which the converter routes through a separate + // code path). Shape 1 falls through to "any address" when the VPC has no subnets. let shape = d.gen_usize(Bound::Included(&0), Bound::Included(&2))?; - let (cidr, vpc_subnet) = match shape { - 0 => (Some(prefix), None), - 1 if !subnet_names.is_empty() => (None, Some(choose(d, &subnet_names)?.clone())), - // Falls through to "any address" when the VPC has no subnets to reference. - _ => (None, None), + let vpc_subnet = if shape == 1 && !subnet_names.is_empty() { + Some(choose(d, &subnet_names)?.clone()) + } else { + None }; + shapes.push((shape == 0, vpc_subnet, ports)); + } + + let cidr_count = u16::try_from(shapes.iter().filter(|(cidr, _, _)| *cidr).count()) + .unwrap_or_else(|_| unreachable!("at most MAX_MATCH_ENTRIES entries")); + let mut prefixes = if want_v4 { + generate_prefixes(d, cidr_count, 0)? + } else { + generate_prefixes(d, 0, cidr_count)? + } + .into_iter(); + + let mut entries = Vec::with_capacity(usize::from(count)); + for (wants_cidr, vpc_subnet, ports) in shapes { + let cidr = if wants_cidr { prefixes.next() } else { None }; // "Any address" with no ports means "match everything"; the converter short-circuits the // whole side on it, discarding any other entry. Skip it unless it is the only entry, so @@ -214,12 +228,14 @@ fn generate_proto(d: &mut D) -> Option { /// Generate a legal, peering-relative ACL. /// -/// The generated ACL is legal with respect to *conversion*: `Acl::try_from` accepts it for the -/// peering it was built against. +/// By default the generated ACL is legal with respect to *conversion*: `Acl::try_from` accepts it +/// for the peering it was built against. It is not necessarily legal with respect to +/// `ValidatedAcl` -- see [`LegalValueAclGenerator::independent_of_exposes`]. pub struct LegalValueAclGenerator<'a> { vpc_subnets: &'a VpcSubnetMap, left_name: &'a str, right_name: &'a str, + independent_of_exposes: bool, } impl<'a> LegalValueAclGenerator<'a> { @@ -230,10 +246,35 @@ impl<'a> LegalValueAclGenerator<'a> { vpc_subnets, left_name, right_name, + independent_of_exposes: false, } } -} + /// Restrict generation to ACLs whose legality does not depend on the peering's exposes. + /// + /// Two validation rules couple an ACL to the exposes on either side of its peering, neither of + /// which is visible from the ACL section alone: + /// + /// - `ValidatedAclRule::validate_patterns_coverage` requires a rule's `src`/`dst` addresses to + /// intersect what the corresponding side actually exposes. An empty side is exempt: it is + /// read as "everything the manifest exposes". + /// - `ValidatedAclRule::validate_scope` rejects `scope: flow` -- which is also the default -- + /// unless every expose on one side of the peering uses masquerade or port forwarding. + /// + /// In this mode, rules carry no address matches (so coverage is trivially satisfied) and always + /// set `scope: packet`. Everything else -- `from`/`to` inference, actions, protocols, logging, + /// the default action -- is still generated. + /// + /// FIXME: a generator that also covers address matches and `scope: flow` needs the *generated + /// exposes* of both sides as further context, and would have to mirror `manifest_coverage_set` + /// to know which prefixes are in scope. Until then, address matches are exercised at the + /// conversion level only (`converters::k8s::config::peering`). + #[must_use] + pub fn independent_of_exposes(mut self) -> Self { + self.independent_of_exposes = true; + self + } +} impl ValueGenerator for LegalValueAclGenerator<'_> { type Output = GatewayAgentPeeringsAcl; @@ -262,11 +303,18 @@ impl ValueGenerator for LegalValueAclGenerator<'_> { let r#match = if d.gen_bool(None)? { let proto: Option = generate_proto(d)?.into(); let allow_ports = proto_supports_ports(proto.as_deref()); - // One family per rule: `AclPattern::validate` rejects a rule whose `src` and `dst` - // disagree on IP version. - let want_v4 = d.gen_bool(None)?; - let src = generate_match_side(d, src_subnets, want_v4, allow_ports)?; - let dst = generate_match_side(d, dst_subnets, want_v4, allow_ports)?; + let (src, dst) = if self.independent_of_exposes { + (Vec::new(), Vec::new()) + } else { + // One family per rule: `AclPattern::validate` rejects a rule whose `src` and + // `dst` disagree on IP version. Drawn here rather than above so that the + // no-address-match mode does not spend a draw on it. + let want_v4 = d.gen_bool(None)?; + ( + generate_match_side(d, src_subnets, want_v4, allow_ports)?, + generate_match_side(d, dst_subnets, want_v4, allow_ports)?, + ) + }; Some(GatewayAgentPeeringsAclRulesMatch { src: Some(src.into_iter().map(Into::into).collect::>()) .filter(|s: &Vec| !s.is_empty()), @@ -284,11 +332,15 @@ impl ValueGenerator for LegalValueAclGenerator<'_> { .produce::>()? .map(|n| format!("{n}-{index}")); - let scope = match d.gen_usize(Bound::Included(&0), Bound::Included(&3))? { - 0 => None, - 1 => Some(GatewayAgentPeeringsAclRulesScope::Flow), - 2 => Some(GatewayAgentPeeringsAclRulesScope::Packet), - _ => Some(GatewayAgentPeeringsAclRulesScope::KopiumEmpty), + let scope = if self.independent_of_exposes { + Some(GatewayAgentPeeringsAclRulesScope::Packet) + } else { + match d.gen_usize(Bound::Included(&0), Bound::Included(&3))? { + 0 => None, + 1 => Some(GatewayAgentPeeringsAclRulesScope::Flow), + 2 => Some(GatewayAgentPeeringsAclRulesScope::Packet), + _ => Some(GatewayAgentPeeringsAclRulesScope::KopiumEmpty), + } }; rules.push(GatewayAgentPeeringsAclRules { diff --git a/k8s-intf/src/bolero/expose.rs b/k8s-intf/src/bolero/expose.rs index 8750ed9c29..180094eb55 100644 --- a/k8s-intf/src/bolero/expose.rs +++ b/k8s-intf/src/bolero/expose.rs @@ -5,7 +5,7 @@ use std::ops::Bound; use bolero::{Driver, TypeGenerator, ValueGenerator}; -use crate::bolero::support::generate_prefixes; +use crate::bolero::support::{PrefixPool, generate_routable_prefixes}; use crate::bolero::{LegalValue, SubnetMap}; use crate::gateway_agent_crd::{ GatewayAgentPeeringsPeeringExpose, GatewayAgentPeeringsPeeringExposeAs, @@ -17,19 +17,102 @@ use crate::gateway_agent_crd::{ GatewayAgentPeeringsPeeringExposeNatStatic, }; +/// The one legal shape of a "default" expose. +/// +/// A default expose carries no prefixes at all -- the converter rejects `ips`/`as` on one, and +/// `VpcExpose::validate` rejects `nots`/`nat` as well -- so there is nothing here to randomize. +/// A manifest may hold at most one, which is why this is a plain constructor for the caller that +/// owns the expose list rather than a branch inside [`LegalValueExposeGenerator`]. +#[must_use] +pub fn default_expose() -> GatewayAgentPeeringsPeeringExpose { + GatewayAgentPeeringsPeeringExpose { + r#as: None, + ips: None, + default: Some(true), + nat: None, + } +} + /// 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. +/// +/// Never produces a "default" expose; see [`default_expose`]. +/// +/// By default the result is legal with respect to *conversion* only. For one that also survives +/// `VpcExpose::validate`, see [`LegalValueExposeGenerator::from_pool`]. +/// +/// Both modes draw addresses from the reserved-block-avoiding generators in +/// [`crate::bolero::support`], even though conversion accepts any parseable prefix and only +/// validation rejects reserved space. That makes the default mode narrower than it needs to be, +/// deliberately: nothing on the conversion path branches on prefix length or on which block a +/// prefix falls in -- `process_ip_block` parses the string and stores it -- so the wider range +/// buys no coverage, and one address-generation path is easier to reason about than two. pub struct LegalValueExposeGenerator<'a> { subnets: &'a SubnetMap, + pool: Option<&'a PrefixPool>, } impl<'a> LegalValueExposeGenerator<'a> { #[must_use] pub fn new(subnets: &'a SubnetMap) -> Self { - Self { subnets } + Self { + subnets, + pool: None, + } + } + + /// Draw addresses from `pool`, and restrict generation to exposes that pass validation. + /// + /// `VpcExpose::validate` couples an expose's fields together in ways that cannot be satisfied + /// by generating each field independently: + /// + /// - `ips`, `nots`, `as` and `notAs` must all be the same IP version; + /// - static NAT requires `ips` and `as` to cover the same *number of addresses*; + /// - port forwarding requires exactly one prefix per side, no exclusions, and a port range on + /// each side; + /// - masquerade forbids port ranges entirely; + /// - and, across the whole manifest, no two exposes may overlap. + /// + /// This mode satisfies all of them by generating the tractable subset: one address family per + /// expose, `ips` drawn from the shared pool (so overlap is impossible by construction), and no + /// exclusions, NAT, or `vpcSubnet` references. + /// + /// FIXME: extending this to NAT means generating `ips`/`as` in matched sizes per NAT mode, and + /// to `vpcSubnet` references means coordinating the VPC subnet generator with the same pool. + /// Until then, those shapes are exercised at the conversion level only + /// (`converters::k8s::config::expose`). + #[must_use] + pub fn from_pool(mut self, pool: &'a PrefixPool) -> Self { + self.pool = Some(pool); + self + } + + /// The validation-legal subset: a single family, pool-allocated `ips`, nothing else. + fn generate_from_pool( + d: &mut D, + pool: &PrefixPool, + ) -> Option { + let v4 = d.gen_bool(None)?; + let count = d.gen_u16(Bound::Included(&1), Bound::Included(&4))?; + let ips = pool + .take(count, v4) + .into_iter() + .map(|cidr| GatewayAgentPeeringsPeeringExposeIps { + cidr: Some(cidr), + not: None, + vpc_subnet: None, + }) + .collect(); + Some(GatewayAgentPeeringsPeeringExpose { + r#as: None, + ips: Some(ips), + // Explicit `false` is legal and takes the same path as absent; cover both spellings. + default: d.gen_bool(None)?.then_some(false), + nat: None, + }) } } @@ -37,6 +120,10 @@ impl ValueGenerator for LegalValueExposeGenerator<'_> { type Output = GatewayAgentPeeringsPeeringExpose; fn generate(&self, d: &mut D) -> Option { + if let Some(pool) = self.pool { + return Self::generate_from_pool(d, pool); + } + 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( @@ -57,7 +144,7 @@ impl ValueGenerator for LegalValueExposeGenerator<'_> { 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 ips = generate_prefixes(d, num_v4_ips, num_v6_ips)? + let ips = generate_routable_prefixes(d, num_v4_ips, num_v6_ips)? .into_iter() .map(|p| GatewayAgentPeeringsPeeringExposeIps { cidr: Some(p), @@ -65,7 +152,7 @@ impl ValueGenerator for LegalValueExposeGenerator<'_> { vpc_subnet: None, }) .collect::>(); - let nots = generate_prefixes(d, num_v4_nots, num_v6_nots)? + let nots = generate_routable_prefixes(d, num_v4_nots, num_v6_nots)? .into_iter() .map(|p| GatewayAgentPeeringsPeeringExposeIps { cidr: None, @@ -73,13 +160,13 @@ impl ValueGenerator for LegalValueExposeGenerator<'_> { vpc_subnet: None, }) .collect::>(); - let r#as = generate_prefixes(d, num_v4_as, num_v6_as)? + let r#as = generate_routable_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)? + let not_as = generate_routable_prefixes(d, num_v4_not_as, num_v6_not_as)? .into_iter() .map(|p| GatewayAgentPeeringsPeeringExposeAs { cidr: None, @@ -112,7 +199,8 @@ impl ValueGenerator for LegalValueExposeGenerator<'_> { Some(GatewayAgentPeeringsPeeringExpose { r#as: Some(final_as).filter(|f| !f.is_empty()), ips: Some(final_ips).filter(|f| !f.is_empty()), - default: None, + // Explicit `false` is legal and takes the same path as absent; cover both spellings. + default: d.gen_bool(None)?.then_some(false), nat: if has_as { Some( d.produce::>()? diff --git a/k8s-intf/src/bolero/peering.rs b/k8s-intf/src/bolero/peering.rs index 227a84beb2..60eb20ec30 100644 --- a/k8s-intf/src/bolero/peering.rs +++ b/k8s-intf/src/bolero/peering.rs @@ -8,6 +8,7 @@ use bolero::{Driver, ValueGenerator}; use crate::bolero::acl::LegalValueAclGenerator; use crate::bolero::expose::LegalValueExposeGenerator; +use crate::bolero::support::{PrefixPool, choose}; use crate::bolero::{SubnetMap, VpcSubnetMap}; use crate::gateway_agent_crd::{GatewayAgentPeerings, GatewayAgentPeeringsPeering}; @@ -15,14 +16,43 @@ use crate::gateway_agent_crd::{GatewayAgentPeerings, GatewayAgentPeeringsPeering /// /// This does not attempt to be exhaustive for vpc names, just generate relevant legal values. /// In particular, subnet names are restricted. Lengths of various lists is also limited to 16 +/// +/// By default this never generates a "default" expose, because legality of one is not decidable +/// from a single side of a peering: `VpcManifest::validate` allows at most one per manifest, and +/// `Peering::validate` forbids a default expose on *both* sides of the same peering. Use +/// [`LegalValuePeeringsPeeringGenerator::allow_default_expose`] from a caller that owns both +/// sides and can uphold that. pub struct LegalValuePeeringsPeeringGenerator<'a> { subnets: &'a SubnetMap, + allow_default: bool, + pool: Option<&'a PrefixPool>, } impl<'a> LegalValuePeeringsPeeringGenerator<'a> { #[must_use] pub fn new(subnets: &'a SubnetMap) -> Self { - Self { subnets } + Self { + subnets, + allow_default: false, + pool: None, + } + } + + /// Permit this side to carry (at most one) default expose. + /// + /// The caller must not enable this for both sides of the same peering. + #[must_use] + pub fn allow_default_expose(mut self) -> Self { + self.allow_default = true; + self + } + + /// Draw expose addresses from `pool`; see + /// [`crate::bolero::expose::LegalValueExposeGenerator::from_pool`]. + #[must_use] + pub fn from_pool(mut self, pool: &'a PrefixPool) -> Self { + self.pool = Some(pool); + self } } @@ -32,10 +62,18 @@ impl ValueGenerator for LegalValuePeeringsPeeringGenerator<'_> { 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) + let expose_gen = match self.pool { + Some(pool) => expose_gen.from_pool(pool), + None => expose_gen, + }; + let mut expose = (0..num_expose) .map(|_| expose_gen.generate(d)) .collect::>>()?; + if self.allow_default && d.gen_bool(None)? { + expose.push(crate::bolero::expose::default_expose()); + } + Some(GatewayAgentPeeringsPeering { expose: Some(expose).filter(|e| !e.is_empty()), }) @@ -45,9 +83,15 @@ impl ValueGenerator for LegalValuePeeringsPeeringGenerator<'_> { /// Generate legal values for `GatewayAgentPeerings` /// /// This does not attempt to be exhaustive for vpc names, just generate relevant legal values. +/// +/// `gwgroup_names` must be the names present in the spec's `groups` table: +/// `ExternalConfig::validate` rejects a peering whose `gatewayGroup` is not in that table, so a +/// peering generated against an unrelated name is convertible but not *valid*. pub struct LegalValuePeeringsGenerator<'a> { vpc_subnets: &'a VpcSubnetMap, vpc_names: Vec<&'a String>, + gwgroup_names: &'a [String], + pool: Option<&'a PrefixPool>, } impl<'a> LegalValuePeeringsGenerator<'a> { @@ -55,17 +99,37 @@ 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 { + /// Returns an error if there are less than two VPCs in the subnet map, or if no gateway group + /// names were supplied for the peering to reference. + pub fn new(vpc_subnets: &'a VpcSubnetMap, gwgroup_names: &'a [String]) -> Result { if vpc_subnets.len() < 2 { return Err("At least two VPCs are required to generate peerings".to_string()); } + if gwgroup_names.is_empty() { + return Err("At least one gateway group is required to generate peerings".to_string()); + } let vpc_names = vpc_subnets.keys().collect(); Ok(Self { vpc_subnets, vpc_names, + gwgroup_names, + pool: None, }) } + + /// Restrict generation to peerings that survive `ExternalConfig::validate`, drawing expose + /// addresses from `pool`. + /// + /// Without this, a generated peering is legal with respect to conversion only: validation + /// couples exposes to each other and ACL rules to the exposes. See + /// [`crate::bolero::expose::LegalValueExposeGenerator::from_pool`] and + /// [`crate::bolero::acl::LegalValueAclGenerator::independent_of_exposes`] for what each + /// restriction costs in coverage. + #[must_use] + pub fn validation_legal(mut self, pool: &'a PrefixPool) -> Self { + self.pool = Some(pool); + self + } } fn pick2<'a, D: Driver, T>(d: &mut D, items: &[&'a T]) -> Option<[&'a T; 2]> { @@ -79,18 +143,58 @@ 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<'_> { + /// Generate a peering between two named VPCs, optionally letting one side carry a default + /// expose. + /// + /// Prefer this over [`ValueGenerator::generate`] when generating several peerings for one + /// spec: two validation rules span the whole overlay and only the caller building the whole set + /// can uphold them. + /// + /// - `Vpc::check_peering_count` rejects an overlay in which the same pair of VPCs peers more + /// than once, so the caller must not repeat a pair. + /// - `VpcRouteTable::validate` rejects a VPC with more than one default destination, and a + /// default expose on one side of a peering *is* a default destination for the other side. So + /// `default_side` must be `None` unless the opposite VPC has no default from any peering. + pub fn generate_for_pair( + &self, + d: &mut D, + vpc_a: &str, + vpc_b: &str, + default_side: Option, + ) -> Option { + self.generate_inner(d, [vpc_a, vpc_b], default_side) + } - fn generate(&self, d: &mut D) -> Option { - let vpc_names = pick2(d, &self.vpc_names)?; + fn generate_inner( + &self, + d: &mut D, + vpc_names: [&str; 2], + default_side: Option, + ) -> Option { 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)?))) - .collect::>>()?; + + let side_gen = |name: &str| { + let side = LegalValuePeeringsPeeringGenerator::new( + self.vpc_subnets.get(name).unwrap_or(&empty_map), + ); + match self.pool { + Some(pool) => side.from_pool(pool), + None => side, + } + }; + let first = side_gen(vpc_names[0]); + let second = side_gen(vpc_names[1]); + let (first, second) = match default_side { + Some(0) => (first.allow_default_expose(), second), + Some(1) => (first, second.allow_default_expose()), + _ => (first, second), + }; + + let peering = BTreeMap::from([ + (vpc_names[0].to_string(), first.generate(d)?), + (vpc_names[1].to_string(), second.generate(d)?), + ]); // `VpcPeering::try_from` names the sides by iterating the `peering` map, so `left` is the // lexicographically smaller of the two VPC names. The ACL's `from`/`to` must agree with @@ -100,15 +204,44 @@ impl ValueGenerator for LegalValuePeeringsGenerator<'_> { let right_name = side_names.next()?; let acl = if d.gen_bool(None)? { - Some(LegalValueAclGenerator::new(self.vpc_subnets, left_name, right_name).generate(d)?) + let acl_gen = LegalValueAclGenerator::new(self.vpc_subnets, left_name, right_name); + let acl_gen = if self.pool.is_some() { + acl_gen.independent_of_exposes() + } else { + acl_gen + }; + Some(acl_gen.generate(d)?) } else { None }; Some(GatewayAgentPeerings { - gateway_group: Some(d.produce::()?), + gateway_group: Some(choose(d, self.gwgroup_names)?), peering: Some(peering), acl, }) } } + +/// Generates a peering between a randomly chosen pair of the known VPCs. +/// +/// Safe for a spec holding a single peering; use +/// [`LegalValuePeeringsGenerator::generate_for_pair`] when generating several, so that no pair of +/// VPCs peers twice and no VPC collects two default destinations. +impl ValueGenerator for LegalValuePeeringsGenerator<'_> { + type Output = GatewayAgentPeerings; + + fn generate(&self, d: &mut D) -> Option { + let vpc_names = pick2(d, &self.vpc_names)?; + // With only one peering in play, either side may carry a default. + let default_side = match d.gen_usize(Bound::Included(&0), Bound::Included(&2))? { + side @ (0 | 1) => Some(side), + _ => None, + }; + self.generate_inner( + d, + [vpc_names[0].as_str(), vpc_names[1].as_str()], + default_side, + ) + } +} diff --git a/k8s-intf/src/bolero/spec.rs b/k8s-intf/src/bolero/spec.rs index e4d433abc5..100406994f 100644 --- a/k8s-intf/src/bolero/spec.rs +++ b/k8s-intf/src/bolero/spec.rs @@ -10,7 +10,7 @@ use lpm::prefix::Prefix; use crate::bolero::gwgroups::{LegalValueGroupsTableGenerator, MAX_GROUP_MEMBERS}; use crate::bolero::peering::LegalValuePeeringsGenerator; -use crate::bolero::support::K8sName; +use crate::bolero::support::{K8sName, PrefixPool}; use crate::bolero::{LegalValue, SubnetMap, VpcSubnetMap}; use crate::gateway_agent_crd::{ GatewayAgentConfig, GatewayAgentGateway, GatewayAgentSpec, GatewayAgentVpcs, @@ -48,6 +48,14 @@ fn increment_string(s: &mut str) { /// Generate a random legal `GatewayAgentSpec` /// +/// "Legal" here means the spec converts *and* validates: the whole value has to hold together, +/// not just each subtree on its own. Two cross-references make that non-local: +/// +/// - a peering's `gatewayGroup` must name a group in `groups`, so the group table is generated +/// first and its names handed to the peering generator; +/// - `ExternalConfig::validate` demands a community for every member rank within a group, so the +/// community table is sized to the largest group that could be generated. +/// /// This does not cover all legal `GatewayAgentSpecs`, /// it is limited by the underlying generators and it generates /// vpcs and peerings with a fixed name pattern and not all @@ -80,20 +88,82 @@ impl TypeGenerator for LegalValue { let vpc_subnet_map = extract_subnets(&vpcs); + // Generate the group table first: peerings have to reference it by name. + let num_groups = d.gen_usize(Bound::Included(&1), Bound::Included(&6))?; + let (groups, gwgroup_names) = + LegalValueGroupsTableGenerator::new(num_groups).generate(d)?; + 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)?); + // One pool for the whole spec, so no two exposes anywhere in it can overlap -- a + // stronger invariant than validation demands, and a cheaper one to be sure of. + let pool = PrefixPool::new(d)?; + + // A default expose reaches the route table as `0.0.0.0/0`, which overlaps every other + // v4 destination, and `VpcRouteTable::validate` requires overlapping destinations to + // share a gateway group. So a spec can have default exposes or a diversity of gateway + // groups, but not both. Generate each shape half the time rather than dropping one. + let allow_defaults = d.gen_bool(None)?; + let groups_in_play: &[String] = if allow_defaults { + let pick = + d.gen_usize(Bound::Included(&0), Bound::Excluded(&gwgroup_names.len()))?; + &gwgroup_names[pick..=pick] + } else { + &gwgroup_names + }; + + #[allow(clippy::unwrap_used)] + // Cannot fail: `num_peerings > 0` implies `num_vpcs > 1`, and `num_groups >= 1`. + let peering_gen = LegalValuePeeringsGenerator::new(&vpc_subnet_map, groups_in_play) + .unwrap() + .validation_legal(&pool); + + // `Vpc::check_peering_count` rejects an overlay where the same pair of VPCs peers more + // than once, so draw peerings from the set of distinct pairs without replacement. + let names: Vec<&String> = vpc_subnet_map.keys().collect(); + let mut pairs: Vec<(usize, usize)> = (0..names.len()) + .flat_map(|a| ((a + 1)..names.len()).map(move |b| (a, b))) + .collect(); + + // A default expose on one side of a peering is a default *destination* for the other + // side, and `VpcRouteTable::validate` allows a VPC only one of those. Track which + // VPCs have been offered one and refuse to offer a second. The bookkeeping is + // conservative: a VPC is marked when a default is *permitted*, not when one is + // actually emitted, since that choice is the side generator's to make. + let mut offered_default: HashSet = HashSet::new(); + + for i in 0..num_peerings.min(pairs.len()) { + let choice = d.gen_usize(Bound::Included(&0), Bound::Excluded(&pairs.len()))?; + let (a, b) = pairs.swap_remove(choice); + + // Side 0 carrying a default targets `b`, and side 1 targets `a`. + let mut candidates = Vec::with_capacity(2); + if allow_defaults && !offered_default.contains(&b) { + candidates.push((0_usize, b)); + } + if allow_defaults && !offered_default.contains(&a) { + candidates.push((1_usize, a)); + } + let default_side = if candidates.is_empty() || d.gen_bool(None)? { + None + } else { + let pick = + d.gen_usize(Bound::Included(&0), Bound::Excluded(&candidates.len()))?; + let (side, target) = candidates[pick]; + offered_default.insert(target); + Some(side) + }; + + peerings.insert( + format!("peering{i}"), + peering_gen.generate_for_pair(d, names[a], names[b], default_side)?, + ); } } - let num_groups = d.gen_usize(Bound::Included(&1), Bound::Included(&6))?; - let (groups, _) = LegalValueGroupsTableGenerator::new(num_groups).generate(d)?; - - // A group of N members occupies ranks `0..N`, and `ExternalConfig::validate` requires a - // community at every rank it finds. Sizing the table to the largest group a generated - // spec can hold keeps that satisfied without coupling to how many members were drawn. + // A group of N members occupies ranks `0..N`, and validation requires a community at + // every rank it finds. Sizing the table to the largest group a generated spec can hold + // keeps that satisfied without coupling to how many members were actually drawn. let mut communities = BTreeMap::new(); for i in 0..MAX_GROUP_MEMBERS { communities.insert(i.to_string(), format!("65000:{}", 100 + i)); From 306ecea90a63b2c055b54783eb61058857730d79 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 24 Jul 2026 16:43:58 -0600 Subject: [PATCH 6/8] test(config): assert generated configs validate, not just convert The top-level conversion test stopped at `try_from`, so every rule in `ExternalConfig::validate` -- and everything it delegates to, notably the overlay and its peerings -- went unfuzzed. Now that generated specs are legal by the system's own definition, take the property all the way to `ValidatedGwConfig`. This is the test that keeps `LegalValue` honest: a cross-reference bug in a generator now fails here instead of hiding as silent under-coverage. Two narrower properties for the fields the previous commits unpinned: `fabricBFD` must reach every underlay BGP neighbor or none of them, and `flowTableCapacity` must survive the `u32` -> `NonZero` narrowing intact. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- .../converters/k8s/config/gateway_config.rs | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/config/src/converters/k8s/config/gateway_config.rs b/config/src/converters/k8s/config/gateway_config.rs index 044a60a5fc..27d9a258b9 100644 --- a/config/src/converters/k8s/config/gateway_config.rs +++ b/config/src/converters/k8s/config/gateway_config.rs @@ -152,6 +152,8 @@ impl TryFrom<&GatewayAgent> for ExternalConfig { #[cfg(test)] mod test { + use std::num::NonZero; + use k8s_intf::bolero::LegalValue; use k8s_intf::gateway_agent_crd::GatewayAgent; @@ -169,4 +171,83 @@ mod test { // Other assertions are implicit via the unwrap of the conversion }); } + + /// A legal `GatewayAgent` must not merely convert -- it must validate. + /// + /// `LegalValue` is only worth the name if it agrees with the system's own definition of a + /// valid config. Stopping at `try_from` would leave every rule in `ExternalConfig::validate` + /// (and everything it delegates to, notably the overlay and its peerings) unfuzzed, and would + /// let cross-reference bugs in the generators hide as silent under-coverage. + #[test] + fn test_gateway_config_validates() { + bolero::check!() + .with_type::>() + .for_each(|ga| { + let ga = ga.as_ref(); + let external_config = ExternalConfig::try_from(ga).unwrap(); + let genid = external_config.genid; + + let validated = external_config + .validate() + .unwrap_or_else(|e| panic!("legal config failed validation: {e}")); + + assert_eq!(validated.genid(), genid); + assert_eq!( + validated.external().gwname(), + ga.metadata.name.as_ref().unwrap() + ); + }); + } + + /// `fabricBFD` must reach every underlay BGP neighbor, or none of them. + /// + /// The flag is applied by a fan-out loop after the underlay has already been converted, so it + /// is exactly the kind of post-hoc mutation that a per-subtree converter test cannot see. + #[test] + fn test_fabric_bfd_fans_out_to_all_neighbors() { + bolero::check!() + .with_type::>() + .for_each(|ga| { + let ga = ga.as_ref(); + let expected = ga.spec.config.as_ref().and_then(|c| c.fabric_bfd); + let external_config = ExternalConfig::try_from(ga).unwrap(); + + let Some(bgp) = external_config.underlay.vrf.bgp.as_ref() else { + return; + }; + match expected { + // Only `Some(true)` turns BFD on; absent and `Some(false)` both leave the + // neighbours as the underlay converter produced them. + Some(true) => assert!( + bgp.neighbors.iter().all(|n| n.bfd), + "fabricBFD set but some neighbors have bfd off" + ), + _ => assert!( + bgp.neighbors.iter().all(|n| !n.bfd), + "fabricBFD unset but some neighbors have bfd on" + ), + } + }); + } + + /// `flowTableCapacity` must survive the `u32` -> `NonZero` narrowing intact. + #[test] + fn test_flow_table_capacity_conversion() { + bolero::check!() + .with_type::>() + .for_each(|ga| { + let ga = ga.as_ref(); + let expected = ga + .spec + .gateway + .as_ref() + .and_then(|gw| gw.flow_table_capacity); + let external_config = ExternalConfig::try_from(ga).unwrap(); + + assert_eq!( + external_config.flow_table_capacity.map(NonZero::get), + expected.map(|c| c as usize), + ); + }); + } } From 2abfa125d5da2faad16ad71c023a05c7a0145334 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 24 Jul 2026 16:44:28 -0600 Subject: [PATCH 7/8] test(k8s-intf): cover the CRD deserialization boundary Nothing fed YAML or JSON text into `GatewayAgentSpec`, yet that parse is the real trust boundary in both intake modes: k8s-less reads YAML from a watched file via `load_crd_from_file`, and the k8s watcher parses JSON off the API server. Both land in the same kopium-generated types, whose integer widths `build.rs` rewrites textually (`i64` -> `u64`, `i32` -> `u32`, with specific fields patched back). Nothing checked that those rewrites round-trip. Round-trip each encoding, and additionally require the two to agree: a field that survives one but not the other would make the k8s and k8s-less modes disagree about the same configuration. Also assert the spec generator is not vacuous. A property test passes just as easily against a generator that has quietly degenerated -- as `GatewayAgentGroups` had, drawing a member count in `0..=10` and emitting exactly one -- and nothing detects that from the outside. So require each interesting shape (multi-member groups, ACLs, default exposes, both `fabricBFD` settings, and the rest) to actually show up in the sample. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- k8s-intf/src/bolero/crd.rs | 193 +++++++++++++++++++++++++++++++++++++ 1 file changed, 193 insertions(+) diff --git a/k8s-intf/src/bolero/crd.rs b/k8s-intf/src/bolero/crd.rs index 92fc069b5a..781a0407b0 100644 --- a/k8s-intf/src/bolero/crd.rs +++ b/k8s-intf/src/bolero/crd.rs @@ -41,3 +41,196 @@ impl TypeGenerator for LegalValue { })) } } + +/// Tests for the deserialization boundary itself. +/// +/// This is the real trust boundary in both intake modes: k8s-less parses YAML from a watched file +/// (`k8s_less::kubeless_watch_gateway_agent_crd` -> [`crate::utils::load_crd_from_file`]) and the +/// k8s watcher parses JSON off the API server. Both land in the same kopium-generated types, and +/// `build.rs` rewrites those types' integer widths textually (`i64` -> `u64`, `i32` -> `u32`, with +/// specific fields patched back). Nothing else checks that those rewrites round-trip. +#[cfg(test)] +mod test { + use crate::bolero::LegalValue; + use crate::gateway_agent_crd::GatewayAgentSpec; + + /// A spec must survive YAML serialization, which is what k8s-less reads. + #[test] + fn test_spec_yaml_round_trip() { + bolero::check!() + .with_type::>() + .for_each(|spec| { + let spec = spec.as_ref(); + let yaml = serde_yaml_ng::to_string(spec).expect("failed to serialize as YAML"); + let parsed: GatewayAgentSpec = serde_yaml_ng::from_str(&yaml) + .unwrap_or_else(|e| panic!("failed to parse YAML: {e}\n{yaml}")); + assert_eq!(*spec, parsed, "YAML round-trip changed the spec:\n{yaml}"); + }); + } + + /// A spec must survive JSON serialization, which is what the k8s watcher receives. + #[test] + fn test_spec_json_round_trip() { + bolero::check!() + .with_type::>() + .for_each(|spec| { + let spec = spec.as_ref(); + let json = serde_json::to_string(spec).expect("failed to serialize as JSON"); + let parsed: GatewayAgentSpec = serde_json::from_str(&json) + .unwrap_or_else(|e| panic!("failed to parse JSON: {e}\n{json}")); + assert_eq!(*spec, parsed, "JSON round-trip changed the spec:\n{json}"); + }); + } + + /// The spec generator must actually reach the shapes it claims to cover. + /// + /// A property test passes just as easily against a generator that has quietly degenerated -- + /// `GatewayAgentGroups` used to draw a member count in `0..=10` and then emit exactly one, so + /// nothing ever exercised a multi-member group. Nothing detects that from the outside, so + /// assert reachability directly: sample specs and require each interesting shape to show up. + /// + /// Ignored under emulation -- miri, and qemu-user for a cross-arch test archive -- because + /// `bolero`'s budget is wall-clock (one second unless iterations are pinned), and a second of + /// emulation buys about one sample, which cannot exhibit every shape. The iteration count is + /// pinned so that what this asserts does not depend on how fast the machine running it happens + /// to be. + #[test] + #[cfg_attr(emulated, ignore = "needs many samples; emulation buys too few")] + fn test_spec_generator_is_not_vacuous() { + // Deliberately `std::sync` rather than the `concurrency::sync` facade: under the `shuttle` + // and `loom` features that facade hands out model-checker atomics, which panic outside a + // model-checked execution, and this is an ordinary `#[test]`. + use std::sync::atomic::{AtomicU32, Ordering}; // nosemgrep: rust-no-direct-std-sync-import + + /// Samples to draw. Every shape below is reachable with probability well over 1/16, so + /// this is many orders of magnitude more than "reliable". + const ITERATIONS: usize = 512; + + /// Shapes a generated spec can exhibit; every one must show up somewhere in the sample. + /// Index in this array is the bit position used to record it. + const SHAPES: &[&str] = &[ + "multiple vpcs", + "peerings", + "acl", + "multi-rule acl", + "default expose", + "multiple exposes", + "empty group", + "multi-member group", + "fabricBFD on", + "fabricBFD off", + "fabricBFD absent", + "flow table capacity", + "profiling", + // `config::converters::k8s::config::gateway_config`'s + // `test_fabric_bfd_fans_out_to_all_neighbors` asserts over every underlay BGP + // neighbor, which the converter builds one-for-one from `gateway.neighbors`. An + // empty list satisfies that assertion vacuously, so require a non-empty one to be + // reachable here, where the reachability claims live. + "bgp neighbors", + ]; + + // An atomic bitmask rather than a collection behind a lock: `bolero` runs each case inside + // `catch_unwind`, so whatever the closure captures has to be `RefUnwindSafe`, and the + // project disallows `std::sync::Mutex`. + let seen = AtomicU32::new(0); + let record = |shape: &str| { + #[allow(clippy::expect_used)] + let bit = SHAPES + .iter() + .position(|s| *s == shape) + .expect("shape not listed in SHAPES"); + seen.fetch_or(1 << bit, Ordering::Relaxed); + }; + + bolero::check!() + .with_type::>() + .with_iterations(ITERATIONS) + .for_each(|spec| { + let spec = spec.as_ref(); + + if spec.vpcs.as_ref().is_some_and(|v| v.len() > 1) { + record("multiple vpcs"); + } + if let Some(peerings) = spec.peerings.as_ref().filter(|p| !p.is_empty()) { + record("peerings"); + for peering in peerings.values() { + if let Some(acl) = peering.acl.as_ref() { + record("acl"); + if acl.rules.as_ref().is_some_and(|r| r.len() > 1) { + record("multi-rule acl"); + } + } + for side in peering.peering.iter().flat_map(|p| p.values()) { + let Some(exposes) = side.expose.as_ref() else { + continue; + }; + if exposes.iter().any(|e| e.default == Some(true)) { + record("default expose"); + } + if exposes.len() > 1 { + record("multiple exposes"); + } + } + } + } + for group in spec.groups.iter().flat_map(|g| g.values()) { + match group.members.as_ref().map_or(0, Vec::len) { + 0 => record("empty group"), + 1 => {} + _ => record("multi-member group"), + } + } + match spec.config.as_ref().and_then(|c| c.fabric_bfd) { + Some(true) => record("fabricBFD on"), + Some(false) => record("fabricBFD off"), + None => record("fabricBFD absent"), + } + if let Some(gw) = spec.gateway.as_ref() { + if gw.flow_table_capacity.is_some() { + record("flow table capacity"); + } + if gw.profiling.is_some() { + record("profiling"); + } + if gw.neighbors.as_ref().is_some_and(|n| !n.is_empty()) { + record("bgp neighbors"); + } + } + }); + + let seen = seen.load(Ordering::Relaxed); + let missing: Vec<&str> = SHAPES + .iter() + .enumerate() + .filter(|(bit, _)| seen & (1 << bit) == 0) + .map(|(_, shape)| *shape) + .collect(); + assert!( + missing.is_empty(), + "the spec generator never produced: {missing:?}" + ); + } + + /// The two intake encodings must agree. + /// + /// A field that survives one encoding but not the other would make the k8s and k8s-less modes + /// disagree about the same configuration, which is the failure this pins down. + #[test] + fn test_yaml_and_json_agree() { + bolero::check!() + .with_type::>() + .for_each(|spec| { + let spec = spec.as_ref(); + let from_yaml: GatewayAgentSpec = serde_yaml_ng::from_str( + &serde_yaml_ng::to_string(spec).expect("failed to serialize as YAML"), + ) + .expect("failed to parse YAML"); + let from_json: GatewayAgentSpec = serde_json::from_str( + &serde_json::to_string(spec).expect("failed to serialize as JSON"), + ) + .expect("failed to parse JSON"); + assert_eq!(from_yaml, from_json, "YAML and JSON intake disagree"); + }); + } +} From d5b237024d7a9f53dbdd56432e7886f5f5d62acd Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 24 Jul 2026 16:45:45 -0600 Subject: [PATCH 8/8] test(k8s-intf,config): reject configs with one thing deliberately wrong Every generator so far produces values the system should accept, which leaves the rejection paths -- the roughly thirty `FromK8sConversionError` and `ConfigError` variants a malformed CRD can provoke -- covered only by hand-written cases. Those paths matter: config arrives from outside the dataplane, so a malformed `GatewayAgent` must be refused cleanly rather than panicking or half-applying. `Mutated` takes a legal `GatewayAgent` and applies exactly one of 23 named corruptions: dangling references, duplicate VNIs and internal ids, malformed CIDRs and protocols, inverted port ranges, a three-sided peering, a zero generation, the wrong namespace, and so on. The mutation is carried in the generated value rather than applied by a loop in the test body, so `bolero` can shrink to the smallest config that still exhibits a problem and the failure names the mutation responsible. The property is deliberately coarse: rejected, never panicking. Asserting a specific error variant per mutation would pin down the current phrasing of every guard and break on harmless refactors, while asserting only "does not panic" would pass even if a dangling reference were silently accepted. A mutation that finds nothing to corrupt reports itself as a no-op and is required to still validate, rather than being resampled -- resampling would bias the distribution and complicate shrinking. One such case is load-bearing: `Vpc::try_from` ignores `subnets` entirely, and the CIDRs are parsed only by the overlay converter's `extract_subnets`, which runs only when the spec has both VPCs and peerings. So a malformed subnet CIDR on an unpeered VPC is accepted as-is today. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- .../converters/k8s/config/gateway_config.rs | 34 ++ k8s-intf/src/bolero/mod.rs | 1 + k8s-intf/src/bolero/mutate.rs | 423 ++++++++++++++++++ 3 files changed, 458 insertions(+) create mode 100644 k8s-intf/src/bolero/mutate.rs diff --git a/config/src/converters/k8s/config/gateway_config.rs b/config/src/converters/k8s/config/gateway_config.rs index 27d9a258b9..a6f42fe7c7 100644 --- a/config/src/converters/k8s/config/gateway_config.rs +++ b/config/src/converters/k8s/config/gateway_config.rs @@ -155,6 +155,7 @@ mod test { use std::num::NonZero; use k8s_intf::bolero::LegalValue; + use k8s_intf::bolero::mutate::Mutated; use k8s_intf::gateway_agent_crd::GatewayAgent; use crate::ExternalConfig; @@ -230,6 +231,39 @@ mod test { }); } + /// A config with one thing deliberately wrong must be *rejected*, never panic. + /// + /// The property is deliberately coarse. Asserting a specific error variant per mutation would + /// pin down the current phrasing of every guard and break on harmless refactors; asserting only + /// "does not panic" would pass even if the config plane silently accepted a dangling reference. + /// Rejection is the property that actually matters: bad config from outside the dataplane must + /// not be applied. + /// + /// A mutation that found nothing to corrupt (no peerings to break a group reference in, say) + /// leaves the config legal, and is required to still validate. + #[test] + fn test_mutated_config_is_rejected() { + bolero::check!().with_type::().for_each(|mutated| { + let result = ExternalConfig::try_from(mutated.agent()) + .and_then(|c| c.validate().map_err(Into::into)); + + if mutated.is_noop() { + assert!( + result.is_ok(), + "{:?} changed nothing, so the config should still be valid: {:?}", + mutated.mutation(), + result.err(), + ); + } else { + assert!( + result.is_err(), + "{:?} was applied but the config was still accepted", + mutated.mutation(), + ); + } + }); + } + /// `flowTableCapacity` must survive the `u32` -> `NonZero` narrowing intact. #[test] fn test_flow_table_capacity_conversion() { diff --git a/k8s-intf/src/bolero/mod.rs b/k8s-intf/src/bolero/mod.rs index fa18f725d1..e43739b4d3 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..a8188043dd --- /dev/null +++ b/k8s-intf/src/bolero/mutate.rs @@ -0,0 +1,423 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +//! Hostile generators: legal configs with one thing deliberately wrong. +//! +//! Every other generator in this module produces values the system should *accept*. That leaves +//! the rejection paths -- the ~30 `FromK8sConversionError` and `ConfigError` variants a malformed +//! CRD can provoke -- covered only by hand-written cases. Those paths matter: config arrives from +//! outside the dataplane, so a malformed `GatewayAgent` must be refused cleanly rather than +//! panicking or half-applying. +//! +//! The mutation is carried *in the generated value* rather than applied by a loop in the test body. +//! That is what makes failures useful: `bolero` can shrink to the smallest config that still +//! exhibits the problem, and the failure report names the mutation that caused it. +//! +//! For that naming to mean anything, a mutation has to trip the guard it is named after rather than +//! some unrelated one. The ACL mutations are the case worth checking, since they graft a `match` +//! onto a rule whose other fields are whatever the generator drew: a `proto` of, say, `58` makes +//! ports illegal on any rule, and an address match has to intersect what its side exposes. Both of +//! those are `ValidatedAcl` rules, and every guard these mutations aim at -- `resolve_prefix` for a +//! dangling or doubly-specified subnet, `parse_entry_ports` for an empty list, `parse_port_ranges` +//! for an inverted range -- lives in `AclPattern::try_from`, which runs during conversion and so +//! fails first. Nothing here needs to pin `proto` or the addresses to keep the labels honest. + +use std::collections::BTreeMap; +use std::ops::Bound; + +use bolero::{Driver, TypeGenerator}; + +use crate::bolero::LegalValue; +use crate::gateway_agent_crd::{GatewayAgent, GatewayAgentPeeringsAclRulesMatchSrc}; + +/// A single, named way to corrupt an otherwise legal [`GatewayAgent`]. +/// +/// Each variant targets a specific guard in the intake path. Variants whose target is absent from +/// a given config are no-ops for that config -- see [`Mutated::is_noop`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Mutation { + /// Clear `metadata.generation`, which `validate_metadata` requires. + DropGeneration, + /// Set `metadata.generation` to zero, which `validate_metadata` rejects explicitly. + ZeroGeneration, + /// Clear `metadata.name`. + DropName, + /// Blank `metadata.name`; distinct from absent, and separately rejected. + EmptyName, + /// Move the object out of the `default` namespace, the only one accepted. + WrongNamespace, + /// Clear `spec.gateway`, without which there is no device or underlay to build. + DropGateway, + /// Point a peering at a gateway group that does not exist. + DanglingGatewayGroup, + /// Name a VPC subnet that does not exist from an ACL match. + DanglingAclSubnet, + /// Give two VPCs the same VNI. + DuplicateVni, + /// Give two VPCs the same internal id. + DuplicateVpcInternalId, + /// Set a VPC's VNI to zero, outside the legal 1..2^24 range. + ZeroVni, + /// Corrupt a VPC subnet CIDR's mask beyond its address family. + OversizeSubnetMask, + /// Replace a VPC subnet CIDR with text that is not a prefix at all. + MalformedSubnetCidr, + /// Set `flowTableCapacity` to zero, which must be a non-zero value. + ZeroFlowTableCapacity, + /// Declare a peering over three VPCs; a peering is strictly between two. + ThreeSidedPeering, + /// Give an ACL rule a `from` naming neither of the peering's VPCs. + AclFromUnknownVpc, + /// Give an ACL match an inverted port range. + InvertedAclPortRange, + /// Give an ACL match an unparseable protocol. + MalformedAclProto, + /// Present an empty `ports` list, which is rejected rather than read as "all ports". + EmptyAclPortsList, + /// Set both `cidr` and `vpcSubnet` on an ACL match entry; they are mutually exclusive. + AclCidrAndSubnet, + /// Map a community to a non-numeric priority. + MalformedCommunityPriority, + /// Give a gateway group two members with the same name. + DuplicateGroupMember, + /// Give a gateway group member an unparseable VTEP address. + MalformedGroupMemberAddress, +} + +/// Every mutation, for the generator to draw from. +const MUTATIONS: &[Mutation] = &[ + Mutation::DropGeneration, + Mutation::ZeroGeneration, + Mutation::DropName, + Mutation::EmptyName, + Mutation::WrongNamespace, + Mutation::DropGateway, + Mutation::DanglingGatewayGroup, + Mutation::DanglingAclSubnet, + Mutation::DuplicateVni, + Mutation::DuplicateVpcInternalId, + Mutation::ZeroVni, + Mutation::OversizeSubnetMask, + Mutation::MalformedSubnetCidr, + Mutation::ZeroFlowTableCapacity, + Mutation::ThreeSidedPeering, + Mutation::AclFromUnknownVpc, + Mutation::InvertedAclPortRange, + Mutation::MalformedAclProto, + Mutation::EmptyAclPortsList, + Mutation::AclCidrAndSubnet, + Mutation::MalformedCommunityPriority, + Mutation::DuplicateGroupMember, + Mutation::MalformedGroupMemberAddress, +]; + +/// A [`GatewayAgent`] that was legal until one [`Mutation`] was applied. +/// +/// The system must reject this -- or, when the mutation found nothing to corrupt, accept it. Which +/// of the two is expected is reported by [`Mutated::is_noop`], so a test can assert the strong +/// property (rejected, with a specific error) without treating "nothing to mutate" as a failure. +#[derive(Debug, Clone)] +pub struct Mutated { + agent: GatewayAgent, + mutation: Mutation, + applied: bool, +} + +impl Mutated { + #[must_use] + pub fn agent(&self) -> &GatewayAgent { + &self.agent + } + + #[must_use] + pub fn mutation(&self) -> Mutation { + self.mutation + } + + /// Whether the mutation found nothing to corrupt, leaving the config legal. + /// + /// A config with no peerings has no gateway group reference to break, for instance. Rather + /// than resample -- which would bias the distribution and complicate shrinking -- the value is + /// returned as-is and flagged. + #[must_use] + pub fn is_noop(&self) -> bool { + !self.applied + } +} + +/// Mutate the first VPC subnet CIDR found, via `edit`. +/// +/// Reports "not applied" when the spec has no peerings, because nothing then reads the subnet: +/// `Vpc::try_from` ignores `subnets` outright, and the CIDRs are parsed only by the overlay +/// converter's `extract_subnets`, which runs only when both VPCs and peerings are present. A +/// malformed CIDR on an unpeered VPC is therefore accepted as-is, so a mutation there has corrupted +/// nothing the intake path looks at. +fn edit_first_subnet_cidr(agent: &mut GatewayAgent, edit: impl FnOnce(&mut String)) -> bool { + if agent.spec.peerings.as_ref().is_none_or(BTreeMap::is_empty) { + return false; + } + let Some(vpcs) = agent.spec.vpcs.as_mut() else { + return false; + }; + for vpc in vpcs.values_mut() { + let Some(subnets) = vpc.subnets.as_mut() else { + continue; + }; + for subnet in subnets.values_mut() { + if let Some(cidr) = subnet.cidr.as_mut() { + edit(cidr); + return true; + } + } + } + false +} + +/// Mutate the first ACL rule found, via `edit`. +fn edit_first_acl_rule( + agent: &mut GatewayAgent, + edit: impl FnOnce(&mut crate::gateway_agent_crd::GatewayAgentPeeringsAclRules), +) -> bool { + let Some(peerings) = agent.spec.peerings.as_mut() else { + return false; + }; + for peering in peerings.values_mut() { + let Some(acl) = peering.acl.as_mut() else { + continue; + }; + let Some(rules) = acl.rules.as_mut() else { + continue; + }; + if let Some(rule) = rules.first_mut() { + edit(rule); + return true; + } + } + false +} + +/// Give the first ACL rule found a `match.src` consisting of the single supplied entry. +fn set_first_acl_src(agent: &mut GatewayAgent, src: GatewayAgentPeeringsAclRulesMatchSrc) -> bool { + edit_first_acl_rule(agent, |rule| { + let m = rule.r#match.get_or_insert( + crate::gateway_agent_crd::GatewayAgentPeeringsAclRulesMatch { + dst: None, + proto: None, + src: None, + }, + ); + m.src = Some(vec![src]); + }) +} + +/// Apply `mutation` to `agent`, reporting whether it found anything to change. +#[allow(clippy::too_many_lines)] +fn apply(agent: &mut GatewayAgent, mutation: Mutation) -> bool { + match mutation { + Mutation::DropGeneration => agent.metadata.generation.take().is_some(), + Mutation::ZeroGeneration => { + agent.metadata.generation = Some(0); + true + } + Mutation::DropName => agent.metadata.name.take().is_some(), + Mutation::EmptyName => { + agent.metadata.name = Some(String::new()); + true + } + Mutation::WrongNamespace => { + agent.metadata.namespace = Some("kube-system".to_string()); + true + } + Mutation::DropGateway => agent.spec.gateway.take().is_some(), + Mutation::DanglingGatewayGroup => { + let Some(peerings) = agent.spec.peerings.as_mut() else { + return false; + }; + let Some(peering) = peerings.values_mut().next() else { + return false; + }; + peering.gateway_group = Some("no-such-group".to_string()); + true + } + Mutation::DanglingAclSubnet => set_first_acl_src( + agent, + GatewayAgentPeeringsAclRulesMatchSrc { + cidr: None, + ports: None, + vpc_subnet: Some("no-such-subnet".to_string()), + }, + ), + Mutation::AclCidrAndSubnet => set_first_acl_src( + agent, + GatewayAgentPeeringsAclRulesMatchSrc { + cidr: Some("10.0.0.0/24".to_string()), + ports: None, + vpc_subnet: Some("also-a-subnet".to_string()), + }, + ), + Mutation::EmptyAclPortsList => set_first_acl_src( + agent, + GatewayAgentPeeringsAclRulesMatchSrc { + cidr: Some("10.0.0.0/24".to_string()), + ports: Some(Vec::new()), + vpc_subnet: None, + }, + ), + Mutation::InvertedAclPortRange => set_first_acl_src( + agent, + GatewayAgentPeeringsAclRulesMatchSrc { + cidr: Some("10.0.0.0/24".to_string()), + ports: Some(vec!["9000-80".to_string()]), + vpc_subnet: None, + }, + ), + Mutation::MalformedAclProto => edit_first_acl_rule(agent, |rule| { + let m = rule.r#match.get_or_insert( + crate::gateway_agent_crd::GatewayAgentPeeringsAclRulesMatch { + dst: None, + proto: None, + src: None, + }, + ); + m.proto = Some("not-a-protocol".to_string()); + }), + Mutation::AclFromUnknownVpc => edit_first_acl_rule(agent, |rule| { + rule.from = Some("no-such-vpc".to_string()); + rule.to = None; + }), + Mutation::DuplicateVni => { + let Some(vpcs) = agent.spec.vpcs.as_mut() else { + return false; + }; + let Some(first_vni) = vpcs.values().next().and_then(|vpc| vpc.vni) else { + return false; + }; + // Needs a second VPC to collide with. + if vpcs.len() < 2 { + return false; + } + #[allow(clippy::unwrap_used)] // len >= 2 checked above + let second = vpcs.values_mut().nth(1).unwrap(); + if second.vni == Some(first_vni) { + return false; + } + second.vni = Some(first_vni); + true + } + Mutation::DuplicateVpcInternalId => { + let Some(vpcs) = agent.spec.vpcs.as_mut() else { + return false; + }; + if vpcs.len() < 2 { + return false; + } + let Some(first_id) = vpcs.values().next().and_then(|vpc| vpc.internal_id.clone()) + else { + return false; + }; + #[allow(clippy::unwrap_used)] // len >= 2 checked above + let second = vpcs.values_mut().nth(1).unwrap(); + if second.internal_id.as_ref() == Some(&first_id) { + return false; + } + second.internal_id = Some(first_id); + true + } + Mutation::ZeroVni => { + let Some(vpcs) = agent.spec.vpcs.as_mut() else { + return false; + }; + let Some(vpc) = vpcs.values_mut().next() else { + return false; + }; + vpc.vni = Some(0); + true + } + Mutation::OversizeSubnetMask => edit_first_subnet_cidr(agent, |cidr| { + // A v4 prefix cannot have a /33, nor a v6 one a /129. + let oversize = if cidr.contains(':') { "/129" } else { "/33" }; + if let Some((addr, _)) = cidr.split_once('/') { + *cidr = format!("{addr}{oversize}"); + } + }), + Mutation::MalformedSubnetCidr => edit_first_subnet_cidr(agent, |cidr| { + *cidr = "definitely/not/a/prefix".to_string(); + }), + Mutation::ZeroFlowTableCapacity => { + let Some(gateway) = agent.spec.gateway.as_mut() else { + return false; + }; + gateway.flow_table_capacity = Some(0); + true + } + Mutation::ThreeSidedPeering => { + let Some(peerings) = agent.spec.peerings.as_mut() else { + return false; + }; + let Some(peering) = peerings.values_mut().next() else { + return false; + }; + let Some(sides) = peering.peering.as_mut() else { + return false; + }; + let Some(extra) = sides.values().next().cloned() else { + return false; + }; + sides.insert("a-third-vpc".to_string(), extra); + true + } + Mutation::MalformedCommunityPriority => { + let communities = agent + .spec + .communities + .get_or_insert_with(std::collections::BTreeMap::new); + communities.insert("not-a-number".to_string(), "65000:999".to_string()); + true + } + Mutation::DuplicateGroupMember => { + let Some(groups) = agent.spec.groups.as_mut() else { + return false; + }; + for group in groups.values_mut() { + let Some(members) = group.members.as_mut() else { + continue; + }; + let Some(first) = members.first().cloned() else { + continue; + }; + members.push(first); + return true; + } + false + } + Mutation::MalformedGroupMemberAddress => { + let Some(groups) = agent.spec.groups.as_mut() else { + return false; + }; + for group in groups.values_mut() { + let Some(members) = group.members.as_mut() else { + continue; + }; + if let Some(member) = members.first_mut() { + member.vtep_ip = "300.1.2.3".to_string(); + return true; + } + } + false + } + } +} + +impl TypeGenerator for Mutated { + fn generate(d: &mut D) -> Option { + let mut agent = d.produce::>()?.take(); + let index = d.gen_usize(Bound::Included(&0), Bound::Excluded(&MUTATIONS.len()))?; + let mutation = MUTATIONS[index]; + let applied = apply(&mut agent, mutation); + Some(Self { + agent, + mutation, + applied, + }) + } +}