From 97729f0e3859b19b0166b6e8924c37180f51cf6d Mon Sep 17 00:00:00 2001 From: Quentin Monnet Date: Thu, 29 Jan 2026 12:51:02 +0000 Subject: [PATCH 1/4] test(nat): Remove double stateful NAT in tests We don't support stateful NAT on both ends of a peering yet, although we can make it work internally. We'll add some validation to reject such configuration in a follow-up commit, so here we adjust the tests that go through the validation step to use stateful NAT on one end only. We might want to reverse this change when we support NAT on both ends. Signed-off-by: Quentin Monnet --- nat/src/stateful/test.rs | 307 +++++---------------------------------- 1 file changed, 37 insertions(+), 270 deletions(-) diff --git a/nat/src/stateful/test.rs b/nat/src/stateful/test.rs index 73c5b5bdb..7b726e31c 100644 --- a/nat/src/stateful/test.rs +++ b/nat/src/stateful/test.rs @@ -43,7 +43,6 @@ mod tests { use std::time::Duration; use tracing_test::traced_test; - const FIVE_MINUTES: Duration = Duration::from_secs(5 * 60); const ONE_MINUTE: Duration = Duration::from_secs(60); fn addr_v4(addr: &str) -> Ipv4Addr { @@ -115,7 +114,7 @@ mod tests { // VPC1 <-> VPC2 let expose121 = VpcExpose::empty() - .make_stateful_nat(Some(FIVE_MINUTES)) + .make_stateful_nat(Some(ONE_MINUTE)) .unwrap() .ip("1.1.0.0/16".into()) .as_range("10.12.0.0/16".into()); @@ -130,26 +129,10 @@ mod tests { .unwrap() .ip("1.3.0.0/24".into()) .as_range("10.100.0.0/24".into()); - let expose211 = VpcExpose::empty() - .make_stateful_nat(Some(ONE_MINUTE)) - .unwrap() - .ip("1.2.2.0/24".into()) - .as_range("10.201.201.0/24".into()); - let expose212 = VpcExpose::empty() - .make_stateful_nat(None) - .unwrap() - .ip("1.2.3.0/24".into()) - .as_range("10.201.202.0/24".into()); - let expose213 = VpcExpose::empty() - .make_stateful_nat(None) - .unwrap() - .ip("2.0.0.0/24".into()) - .as_range("10.201.203.0/24".into()); - let expose214 = VpcExpose::empty() - .make_stateful_nat(None) - .unwrap() - .ip("2.0.1.0/28".into()) - .as_range("10.201.204.192/28".into()); + let expose211 = VpcExpose::empty().ip("10.201.201.0/24".into()); + let expose212 = VpcExpose::empty().ip("10.201.202.0/24".into()); + let expose213 = VpcExpose::empty().ip("10.201.203.0/24".into()); + let expose214 = VpcExpose::empty().ip("10.201.204.192/28".into()); // VPC1 <-> VPC3 let expose131 = VpcExpose::empty() @@ -164,11 +147,7 @@ mod tests { .as_range("3.1.0.0/16".into()) .not_as("3.1.128.0/17".into()) .as_range("3.2.0.0/17".into()); - let expose311 = VpcExpose::empty() - .make_stateful_nat(None) - .unwrap() - .ip("192.168.128.0/24".into()) - .as_range("3.3.3.0/24".into()); + let expose311 = VpcExpose::empty().ip("3.3.3.0/24".into()); // VPC1 <-> VPC4 let expose141 = VpcExpose::empty() @@ -176,11 +155,7 @@ mod tests { .unwrap() .ip("1.1.0.0/16".into()) .as_range("4.4.0.0/16".into()); - let expose411 = VpcExpose::empty() - .make_stateful_nat(None) - .unwrap() - .ip("1.1.0.0/16".into()) - .as_range("4.5.0.0/16".into()); + let expose411 = VpcExpose::empty().ip("4.5.0.0/16".into()); // VPC2 <-> VPC4 let expose241 = VpcExpose::empty() @@ -191,12 +166,8 @@ mod tests { .as_range("44.0.0.0/16".into()) .not_as("44.0.200.0/24".into()); let expose421 = VpcExpose::empty() - .make_stateful_nat(None) - .unwrap() - .ip("4.4.0.0/16".into()) - .not("4.4.128.0/18".into()) - .as_range("44.4.0.0/16".into()) - .not_as("44.4.64.0/18".into()); + .ip("44.4.0.0/16".into()) + .not("44.4.64.0/18".into()); // VPC3 <-> VPC4 let expose341 = VpcExpose::empty() @@ -204,10 +175,7 @@ mod tests { .unwrap() .ip("192.168.100.0/24".into()) .as_range("34.34.34.0/24".into()); - let expose431 = VpcExpose::empty() - .make_stateful_nat(None) - .unwrap() - .ip("4.4.0.0/24".into()); + let expose431 = VpcExpose::empty().ip("4.4.0.0/24".into()); // VPC1 <-> VPC2 let mut manifest12 = VpcManifest::new("VPC-1"); @@ -275,11 +243,7 @@ mod tests { .unwrap() .ip("1.1.0.0/16".into()) .as_range("2.2.0.0/16".into()); - let expose211 = VpcExpose::empty() - .make_stateful_nat(None) - .unwrap() - .ip("1.2.2.0/24".into()) - .as_range("3.3.3.0/24".into()); + let expose211 = VpcExpose::empty().ip("3.3.3.0/24".into()); let mut manifest12 = VpcManifest::new("VPC-1"); add_expose(&mut manifest12, expose121); @@ -368,13 +332,13 @@ mod tests { // NAT: expose121 <-> expose211 let (orig_src, orig_dst) = ("1.1.2.3", "10.201.201.18"); - let (target_src, target_dst) = ("10.12.0.0", "1.2.2.0"); + let target_src = "10.12.0.0"; let (output_src, output_dst, output_src_port, output_dst_port, done_reason) = check_packet(&mut nat, vni(100), vni(200), orig_src, orig_dst, 9998, 443); assert_eq!(done_reason, None); assert_eq!(output_src, addr_v4(target_src)); - assert_eq!(output_dst, addr_v4(target_dst)); + assert_eq!(output_dst, addr_v4(orig_dst)); // Reverse path let ( return_output_src, @@ -386,7 +350,7 @@ mod tests { &mut nat, vni(200), vni(100), - target_dst, + orig_dst, target_src, output_dst_port, output_src_port, @@ -410,12 +374,11 @@ mod tests { ) else { unreachable!() }; - // We expect to find the minimum (non-empty) value between the two VPCs involved assert_eq!(idle_timeout, ONE_MINUTE); // Reverse path let Some((_, idle_timeout)) = nat.get_session::( VpcDiscriminant::VNI(vni(200)), - IpAddr::from_str(target_dst).unwrap(), + IpAddr::from_str(orig_dst).unwrap(), VpcDiscriminant::VNI(vni(100)), IpAddr::from_str(target_src).unwrap(), IpProtoKey::Udp(UdpProtoKey { @@ -438,42 +401,11 @@ mod tests { // TODO: We should drop this connection after updating the allocator in the future, as a // result these steps should fail let (orig_src, orig_dst) = ("1.1.2.3", "10.201.201.18"); - let (target_src, target_dst) = ("10.12.0.0", "1.2.2.0"); + let target_src = "10.12.0.0"; let (output_src, output_dst, output_src_port, output_dst_port, done_reason) = check_packet(&mut nat, vni(100), vni(200), orig_src, orig_dst, 9998, 443); assert_eq!(output_src, addr_v4(target_src)); - assert_eq!(output_dst, addr_v4(target_dst)); - assert_eq!(done_reason, None); - // Reverse path - let ( - return_output_src, - return_output_dst, - return_output_src_port, - return_output_dst_port, - done_reason, - ) = check_packet( - &mut nat, - vni(200), - vni(100), - target_dst, - target_src, - output_dst_port, - output_src_port, - ); - assert_eq!(return_output_src, addr_v4(orig_dst)); - assert_eq!(return_output_dst, addr_v4(orig_src)); - assert_eq!(return_output_src_port, 443); - assert_eq!(return_output_dst_port, 9998); - assert_eq!(done_reason, None); - - // Check new connection: valid source NAT but invalid destination NAT - let (orig_src, orig_dst) = ("1.1.2.3", "10.201.201.17"); - let target_src = "2.2.0.0"; - let (output_src, output_dst, output_src_port, output_dst_port, done_reason) = - check_packet(&mut nat, vni(100), vni(200), orig_src, orig_dst, 9998, 80); - assert_eq!(output_src, addr_v4(target_src)); assert_eq!(output_dst, addr_v4(orig_dst)); - assert_eq!(output_dst_port, 80); assert_eq!(done_reason, None); // Reverse path let ( @@ -493,99 +425,17 @@ mod tests { ); assert_eq!(return_output_src, addr_v4(orig_dst)); assert_eq!(return_output_dst, addr_v4(orig_src)); - assert_eq!(return_output_src_port, 80); + assert_eq!(return_output_src_port, 443); assert_eq!(return_output_dst_port, 9998); assert_eq!(done_reason, None); // Check new valid connection let (orig_src, orig_dst) = ("1.1.2.3", "3.3.3.3"); - let (target_src, target_dst) = ("2.2.0.0", "1.2.2.0"); + let target_src = "2.2.0.0"; let (output_src, output_dst, output_src_port, output_dst_port, done_reason) = check_packet(&mut nat, vni(100), vni(200), orig_src, orig_dst, 9998, 80); assert_eq!(output_src, addr_v4(target_src)); - assert_eq!(output_dst, addr_v4(target_dst)); - assert_eq!(done_reason, None); - // Reverse path - let ( - return_output_src, - return_output_dst, - return_output_src_port, - return_output_dst_port, - done_reason, - ) = check_packet( - &mut nat, - vni(200), - vni(100), - target_dst, - target_src, - output_dst_port, - output_src_port, - ); - assert_eq!(return_output_src, addr_v4(orig_dst)); - assert_eq!(return_output_dst, addr_v4(orig_src)); - assert_eq!(return_output_src_port, 80); - assert_eq!(return_output_dst_port, 9998); - assert_eq!(done_reason, None); - } - - fn build_overlay_2vpcs_unidirectional_nat() -> Overlay { - fn add_expose(manifest: &mut VpcManifest, expose: VpcExpose) { - manifest.add_expose(expose).expect("Failed to add expose"); - } - - let mut vpc_table = VpcTable::new(); - let _ = vpc_table.add(Vpc::new("VPC-1", "AAAAA", 100).expect("Failed to add VPC")); - let _ = vpc_table.add(Vpc::new("VPC-2", "BBBBB", 200).expect("Failed to add VPC")); - - let expose121 = VpcExpose::empty() - .make_stateful_nat(None) - .unwrap() - .ip("1.1.0.0/16".into()) - .as_range("2.2.0.0/16".into()); - let expose211 = VpcExpose::empty().ip("5.0.0.0/8".into()); - - let mut manifest12 = VpcManifest::new("VPC-1"); - add_expose(&mut manifest12, expose121); - let mut manifest21 = VpcManifest::new("VPC-2"); - add_expose(&mut manifest21, expose211); - - let peering12 = VpcPeering::new("VPC-1--VPC-2", manifest12, manifest21, None); - - let mut peering_table = VpcPeeringTable::new(); - peering_table.add(peering12).expect("Failed to add peering"); - - Overlay::new(vpc_table, peering_table) - } - - #[test] - #[traced_test] - fn test_full_config_unidirectional_nat() { - let mut config = build_sample_config(build_overlay_2vpcs_unidirectional_nat()); - config.validate().unwrap(); - - // Check that we can validate the allocator - let (mut nat, mut allocator) = StatefulNat::new_with_defaults(); - allocator - .update_allocator(&config.external.overlay.vpc_table) - .unwrap(); - - // No NAT - let (orig_src, orig_dst) = ("8.8.8.8", "9.9.9.9"); - let (output_src, output_dst, output_src_port, output_dst_port, done_reason) = - check_packet(&mut nat, vni(100), vni(200), orig_src, orig_dst, 9998, 443); - assert_eq!(output_src, addr_v4(orig_src)); assert_eq!(output_dst, addr_v4(orig_dst)); - assert_eq!(output_src_port, 9998); - assert_eq!(output_dst_port, 443); - assert_eq!(done_reason, Some(DoneReason::Filtered)); - - // NAT: expose121 <-> expose211 (valid source NAT, no destination NAT) - let (orig_src, orig_dst) = ("1.1.2.3", "5.0.0.5"); - let (target_src, target_dst) = ("2.2.0.0", "5.0.0.5"); - let (output_src, output_dst, output_src_port, output_dst_port, done_reason) = - check_packet(&mut nat, vni(100), vni(200), orig_src, orig_dst, 9998, 443); - assert_eq!(output_src, addr_v4(target_src)); - assert_eq!(output_dst, addr_v4(target_dst)); assert_eq!(done_reason, None); // Reverse path let ( @@ -598,14 +448,14 @@ mod tests { &mut nat, vni(200), vni(100), - target_dst, + orig_dst, target_src, output_dst_port, output_src_port, ); assert_eq!(return_output_src, addr_v4(orig_dst)); assert_eq!(return_output_dst, addr_v4(orig_src)); - assert_eq!(return_output_src_port, 443); + assert_eq!(return_output_src_port, 80); assert_eq!(return_output_dst_port, 9998); assert_eq!(done_reason, None); } @@ -709,7 +559,7 @@ mod tests { // NAT: expose121 <-> expose211 let (orig_src, orig_dst, orig_identifier) = (addr_v4("1.1.2.3"), addr_v4("3.3.3.3"), 1337); - let (target_src, target_dst) = (addr_v4("2.2.0.0"), addr_v4("1.2.2.0")); + let target_src = addr_v4("2.2.0.0"); let (output_src, output_dst, output_identifier_1, done_reason) = check_packet_icmp_echo( &mut nat, vni(100), @@ -720,7 +570,7 @@ mod tests { orig_identifier, ); assert_eq!(output_src, target_src); - assert_eq!(output_dst, target_dst); + assert_eq!(output_dst, orig_dst); assert!(output_identifier_1.is_multiple_of(256)); // First port of a 256-port "port block" from allocator assert_eq!(done_reason, None); @@ -730,7 +580,7 @@ mod tests { &mut nat, vni(200), vni(100), - target_dst, + orig_dst, target_src, IcmpEchoDirection::Reply, output_identifier_1, @@ -742,7 +592,7 @@ mod tests { // Second request with same identifier: no reallocation let (orig_src, orig_dst) = (addr_v4("1.1.2.3"), addr_v4("3.3.3.3")); - let (target_src, target_dst) = (addr_v4("2.2.0.0"), addr_v4("1.2.2.0")); + let target_src = addr_v4("2.2.0.0"); let (output_src, output_dst, output_identifier_2, done_reason) = check_packet_icmp_echo( &mut nat, vni(100), @@ -753,13 +603,13 @@ mod tests { orig_identifier, ); assert_eq!(output_src, target_src); - assert_eq!(output_dst, target_dst); + assert_eq!(output_dst, orig_dst); assert_eq!(output_identifier_2, output_identifier_1); // Same identifier as before assert_eq!(done_reason, None); // NAT: expose121 <-> expose211 again, but with identifier 0 (corner case) let (orig_src, orig_dst, orig_identifier) = (addr_v4("1.1.2.3"), addr_v4("3.3.3.3"), 0); - let (target_src, target_dst) = (addr_v4("2.2.0.0"), addr_v4("1.2.2.0")); + let target_src = addr_v4("2.2.0.0"); let (output_src, output_dst, output_identifier_3, done_reason) = check_packet_icmp_echo( &mut nat, vni(100), @@ -771,87 +621,8 @@ mod tests { ); assert_eq!(output_src, target_src); - assert_eq!(output_dst, target_dst); - assert_eq!(output_identifier_3, output_identifier_1 + 1); // Second port of the same 256-port "port block" from allocator - assert_eq!(done_reason, None); - } - - #[test] - #[traced_test] - fn test_icmp_echo_unidirectional_nat() { - let mut config = build_sample_config(build_overlay_2vpcs_unidirectional_nat()); - config.validate().unwrap(); - - // Check that we can validate the allocator - let (mut nat, mut allocator) = StatefulNat::new_with_defaults(); - allocator - .update_allocator(&config.external.overlay.vpc_table) - .unwrap(); - - // No NAT - let (orig_src, orig_dst, orig_identifier) = (addr_v4("8.8.8.8"), addr_v4("9.9.9.9"), 1337); - let (output_src, output_dst, output_identifier, done_reason) = check_packet_icmp_echo( - &mut nat, - vni(100), - vni(200), - orig_src, - orig_dst, - IcmpEchoDirection::Request, - orig_identifier, - ); - assert_eq!(output_src, orig_src); assert_eq!(output_dst, orig_dst); - assert_eq!(output_identifier, orig_identifier); - assert_eq!(done_reason, Some(DoneReason::Filtered)); - - // NAT: expose121 <-> expose211 - let (orig_src, orig_dst, orig_identifier) = (addr_v4("1.1.2.3"), addr_v4("5.0.0.5"), 1337); - let (target_src, target_dst) = (addr_v4("2.2.0.0"), addr_v4("5.0.0.5")); - let (output_src, output_dst, output_identifier_1, done_reason) = check_packet_icmp_echo( - &mut nat, - vni(100), - vni(200), - orig_src, - orig_dst, - IcmpEchoDirection::Request, - orig_identifier, - ); - assert_eq!(output_src, target_src); - assert_eq!(output_dst, target_dst); - assert!(output_identifier_1.is_multiple_of(256)); // First port of a 256-port "port block" from allocator - assert_eq!(done_reason, None); - - // Reverse path - let (return_output_src, return_output_dst, return_output_identifier, done_reason) = - check_packet_icmp_echo( - &mut nat, - vni(200), - vni(100), - target_dst, - target_src, - IcmpEchoDirection::Reply, - output_identifier_1, - ); - assert_eq!(return_output_src, orig_dst); - assert_eq!(return_output_dst, orig_src); - assert_eq!(return_output_identifier, orig_identifier); - assert_eq!(done_reason, None); - - // Second request with same identifier: no reallocation - let (orig_src, orig_dst) = (addr_v4("1.1.2.3"), addr_v4("5.0.0.5")); - let (target_src, target_dst) = (addr_v4("2.2.0.0"), addr_v4("5.0.0.5")); - let (output_src, output_dst, output_identifier_2, done_reason) = check_packet_icmp_echo( - &mut nat, - vni(100), - vni(200), - orig_src, - orig_dst, - IcmpEchoDirection::Request, - orig_identifier, - ); - assert_eq!(output_src, target_src); - assert_eq!(output_dst, target_dst); - assert_eq!(output_identifier_2, output_identifier_1); // Same identifier as before + assert_eq!(output_identifier_3, output_identifier_1 + 1); // Second port of the same 256-port "port block" from allocator assert_eq!(done_reason, None); } @@ -947,13 +718,13 @@ mod tests { orig_echo_seq_number, ) = ( // Host 1.1.2.3 in VPC1 sent imaginary ICMP Echo packet to 3.3.3.3 in VPC2, - // which imaginarily got translated as 2.2.0.0 -> 1.2.2.0. + // which imaginarily got translated as 2.2.0.0 -> 3.3.3.3. // Router 1.2.2.18 from VPC2 returns Destination Unreachable to 2.2.0.0 with initial // datagram embedded in it addr_v4("1.2.2.18"), addr_v4("2.2.0.0"), addr_v4("2.2.0.0"), - addr_v4("1.2.2.0"), + addr_v4("3.3.3.3"), 1337, 0, ); @@ -990,7 +761,7 @@ mod tests { addr_v4("1.1.2.3"), addr_v4("3.3.3.3"), addr_v4("2.2.0.0"), - addr_v4("1.2.2.0"), + addr_v4("3.3.3.3"), ); let (output_echo_src, output_echo_dst, output_echo_identifier, done_reason) = check_packet_icmp_echo( @@ -1063,11 +834,7 @@ mod tests { .unwrap() .ip("1.1.0.0/16".into()) .as_range("2.2.0.0/16".into()); - let expose211 = VpcExpose::empty() - .make_stateful_nat(None) - .unwrap() - .ip("1.2.2.0/24".into()) - .as_range("3.3.3.0/24".into()); + let expose211 = VpcExpose::empty().ip("3.3.3.0/24".into()); let expose212 = VpcExpose::empty().set_default(); let mut manifest12 = VpcManifest::new("VPC-1"); @@ -1097,7 +864,7 @@ mod tests { // Using the expose with a prefix let (orig_src, orig_dst, orig_src_port, orig_dst_port) = ("1.1.0.1", "3.3.3.3", 9999, 443); - let (target_src, target_dst) = ("2.2.0.0", "1.2.2.0"); + let target_src = "2.2.0.0"; let (output_src, output_dst, output_src_port, output_dst_port, done_reason) = check_packet( &mut nat, vni(100), @@ -1110,7 +877,7 @@ mod tests { assert_eq!(done_reason, None); assert_eq!(output_src, addr_v4(target_src)); - assert_eq!(output_dst, addr_v4(target_dst)); + assert_eq!(output_dst, addr_v4(orig_dst)); // Reverse path let ( return_output_src, @@ -1122,7 +889,7 @@ mod tests { &mut nat, vni(200), vni(100), - target_dst, + orig_dst, target_src, output_dst_port, output_src_port, @@ -1136,7 +903,7 @@ mod tests { // Using the default expose let (orig_src, orig_dst, orig_src_port, orig_dst_port) = ("1.1.0.1", "10.11.12.13", 9999, 443); - let (target_src, target_dst) = ("2.2.0.0", "10.11.12.13"); + let target_src = "2.2.0.0"; let (output_src, output_dst, output_src_port, output_dst_port, done_reason) = check_packet( &mut nat, vni(100), @@ -1149,7 +916,7 @@ mod tests { assert_eq!(done_reason, None); assert_eq!(output_src, addr_v4(target_src)); - assert_eq!(output_dst, addr_v4(target_dst)); + assert_eq!(output_dst, addr_v4(orig_dst)); // Reverse path let ( return_output_src, @@ -1161,7 +928,7 @@ mod tests { &mut nat, vni(200), vni(100), - target_dst, + orig_dst, target_src, output_dst_port, output_src_port, From 9d63a717454c63baf9def6934be1caf5647f9adf Mon Sep 17 00:00:00 2001 From: Quentin Monnet Date: Thu, 29 Jan 2026 14:59:45 +0000 Subject: [PATCH 2/4] test(config,flow-filter): Build VPC table without validating, in test In test test_flow_filter_table_check_nat_requirements() we validate the overlay we use to generate the peerings in the VPC table we work with; but in a future commit, we'll introduce additional validation that will make our peerings no longer valid. We could adjust the peerings to use stateful NAT on one side only, and without stateless NAT, but it would significantly decrease the interest of the test (they would be fewer risks to find a mistake). Instead, skip validation, and only collect the peerings. To do that, we make the relevant code public from Overlay's implementation. This comes at the risk of other locations calling the code in the future; but given that the fields for the Overlay, and method vpcid_map() are public already, it would be trivial to replicate this code elsewhere anyway. To lower the risk, we also add a comment to discourage calling this function elsewhere. Signed-off-by: Quentin Monnet --- config/src/external/overlay/mod.rs | 13 ++++++++++--- flow-filter/src/lib.rs | 4 ++-- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/config/src/external/overlay/mod.rs b/config/src/external/overlay/mod.rs index 0ca4dea09..9f1591de6 100644 --- a/config/src/external/overlay/mod.rs +++ b/config/src/external/overlay/mod.rs @@ -63,15 +63,22 @@ impl Overlay { debug!("Validating overlay configuration..."); self.validate_peerings()?; - let id_map = self.vpcid_map(); // collect peerings for every vpc. - self.vpc_table - .collect_peerings(&self.peering_table, &id_map); + self.collect_peerings(); self.vpc_table.validate()?; debug!("Overlay configuration is VALID:\n{self}"); Ok(()) } + + /// Collect peerings from the peering table for every VPC. + /// + /// Should only be called in `validate`, or in tests. + pub fn collect_peerings(&mut self) { + let id_map = self.vpcid_map(); + self.vpc_table + .collect_peerings(&self.peering_table, &id_map); + } } diff --git a/flow-filter/src/lib.rs b/flow-filter/src/lib.rs index dc5498609..ef719e895 100644 --- a/flow-filter/src/lib.rs +++ b/flow-filter/src/lib.rs @@ -820,8 +820,8 @@ mod tests { .unwrap(); let mut overlay = Overlay::new(vpc_table, peering_table); - // Validation is necessary to build overlay.vpc_table's peerings from peering_table - overlay.validate().unwrap(); + // Build overlay.vpc_table's peerings from peering_table, with no validation + overlay.collect_peerings(); let table = FlowFilterTable::build_from_overlay(&overlay).unwrap(); From 552bca4ed29f69e30f8e275351fe2aeecdc5ff48 Mon Sep 17 00:00:00 2001 From: Quentin Monnet Date: Wed, 28 Jan 2026 16:37:26 +0000 Subject: [PATCH 3/4] feat(config): Return error when Peering has stateful NAT on both sides We do not currently support stateful NAT on both sides of a peering. Make sure to return a config error if users attempt to do it anyway. Link: https://github.com/githedgehog/internal/issues/260 Reported-by: Pau Capdevila Signed-off-by: Quentin Monnet --- config/src/errors.rs | 4 +++ config/src/external/overlay/tests.rs | 47 ++++++++++++++++++++++++++++ config/src/external/overlay/vpc.rs | 23 ++++++++++++++ 3 files changed, 74 insertions(+) diff --git a/config/src/errors.rs b/config/src/errors.rs index 67d4f54f8..78c1fc97f 100644 --- a/config/src/errors.rs +++ b/config/src/errors.rs @@ -75,6 +75,10 @@ pub enum ConfigError { // NAT-specific #[error("Mismatched prefixes sizes for static NAT: {0:?} and {1:?}")] MismatchedPrefixSizes(PrefixWithPortsSize, PrefixWithPortsSize), + #[error( + "Stateful NAT is only supported on one side of a peering, but peering {0} has stateful NAT enabled on both sides" + )] + StatefulNatOnBothSides(String), // Interface addresses #[error("Invalid interface address format: {0}")] diff --git a/config/src/external/overlay/tests.rs b/config/src/external/overlay/tests.rs index 517e8eff9..0248f6986 100644 --- a/config/src/external/overlay/tests.rs +++ b/config/src/external/overlay/tests.rs @@ -288,6 +288,53 @@ pub mod test { ); } + #[test] + fn test_peering_nat_both_sides() { + // Build VPCs and VPC table + let vpc1 = Vpc::new("VPC-1", "VPC01", 1).unwrap(); + let vpc2 = Vpc::new("VPC-2", "VPC02", 2).unwrap(); + let mut vpc_table = VpcTable::new(); + vpc_table.add(vpc1).unwrap(); + vpc_table.add(vpc2).unwrap(); + + // Build peering with stateful NAT on both sides + let peering = VpcPeering::new( + "Peering-1", + VpcManifest { + name: "VPC-1".to_owned(), + exposes: vec![ + VpcExpose::empty() + .make_stateful_nat(None) + .unwrap() + .ip("1.0.0.0/8".into()) + .as_range("2.0.0.0/8".into()), + ], + }, + VpcManifest { + name: "VPC-2".to_owned(), + exposes: vec![ + VpcExpose::empty() + .make_stateful_nat(None) + .unwrap() + .ip("3.0.0.0/8".into()) + .as_range("4.0.0.0/8".into()), + ], + }, + None, + ); + + // Build VPC pering table and add peering + let mut peering_table = VpcPeeringTable::new(); + peering_table.add(peering).unwrap(); + + // Build overlay object and validate it + let mut overlay = Overlay::new(vpc_table, peering_table); + assert_eq!( + overlay.validate(), + Err(ConfigError::StatefulNatOnBothSides("Peering-1".to_owned())) + ); + } + #[test] fn test_overlay_missing_vpc() { /* build VPCs */ diff --git a/config/src/external/overlay/vpc.rs b/config/src/external/overlay/vpc.rs index 09565ce41..891f10aaa 100644 --- a/config/src/external/overlay/vpc.rs +++ b/config/src/external/overlay/vpc.rs @@ -45,6 +45,29 @@ impl Peering { // not needed will be validated when validating the remote vpc self.remote.validate()?; } + + self.validate_nat_combinations() + } + + fn validate_nat_combinations(&self) -> ConfigResult { + // We do not support stateful NAT on both sides of a peering + let mut local_has_stateful_nat = false; + for expose in &self.local.exposes { + if expose.has_stateful_nat() { + local_has_stateful_nat = true; + break; + } + } + + if !local_has_stateful_nat { + return Ok(()); + } + + for expose in &self.remote.exposes { + if expose.has_stateful_nat() { + return Err(ConfigError::StatefulNatOnBothSides(self.name.clone())); + } + } Ok(()) } } From 0a47a012118d9f86b00c5be3954042642a47657f Mon Sep 17 00:00:00 2001 From: Quentin Monnet Date: Thu, 29 Jan 2026 14:19:46 +0000 Subject: [PATCH 4/4] feat(config): Return error when peering uses stateful + stateless NAT Return an error when one side of a peering uses stateful NAT, and the other side uses stateless NAT. It should probably not be difficult to make it work, but at the very least we need to change the checks at the entry points of the two NAT stages: currently we don't apply stateful NAT if the packet has been processed through the stateless NAT stage. We may also need additional checks to determine whether source or destination needs to be NAT-ed, to avoid a double-NAT operation. Anyway, as long as we don't support this properly, let's reject such configurations. Signed-off-by: Quentin Monnet --- config/src/errors.rs | 4 +++ config/src/external/overlay/tests.rs | 49 ++++++++++++++++++++++++++++ config/src/external/overlay/vpc.rs | 12 +++++-- 3 files changed, 63 insertions(+), 2 deletions(-) diff --git a/config/src/errors.rs b/config/src/errors.rs index 78c1fc97f..6538117a4 100644 --- a/config/src/errors.rs +++ b/config/src/errors.rs @@ -79,6 +79,10 @@ pub enum ConfigError { "Stateful NAT is only supported on one side of a peering, but peering {0} has stateful NAT enabled on both sides" )] StatefulNatOnBothSides(String), + #[error( + "Stateful NAT is not compatible with stateless NAT at the other side of a peering, but peering {0} has stateful and stateless NAT enabled on different sides" + )] + StatefulPlusStatelessNat(String), // Interface addresses #[error("Invalid interface address format: {0}")] diff --git a/config/src/external/overlay/tests.rs b/config/src/external/overlay/tests.rs index 0248f6986..292999518 100644 --- a/config/src/external/overlay/tests.rs +++ b/config/src/external/overlay/tests.rs @@ -335,6 +335,55 @@ pub mod test { ); } + #[test] + fn test_peering_nat_stateful_plus_stateless() { + // Build VPCs and VPC table + let vpc1 = Vpc::new("VPC-1", "VPC01", 1).unwrap(); + let vpc2 = Vpc::new("VPC-2", "VPC02", 2).unwrap(); + let mut vpc_table = VpcTable::new(); + vpc_table.add(vpc1).unwrap(); + vpc_table.add(vpc2).unwrap(); + + // Build peering with stateful NAT on one side and stateless NAT on the other side + let peering = VpcPeering::new( + "Peering-1", + VpcManifest { + name: "VPC-1".to_owned(), + exposes: vec![ + VpcExpose::empty() + .make_stateful_nat(None) + .unwrap() + .ip("1.0.0.0/8".into()) + .as_range("2.0.0.0/8".into()), + ], + }, + VpcManifest { + name: "VPC-2".to_owned(), + exposes: vec![ + VpcExpose::empty() + .make_stateless_nat() + .unwrap() + .ip("3.0.0.0/8".into()) + .as_range("4.0.0.0/8".into()), + ], + }, + None, + ); + + // Build VPC pering table and add peering + let mut peering_table = VpcPeeringTable::new(); + peering_table.add(peering).unwrap(); + + // Build overlay object and validate it + let mut overlay = Overlay::new(vpc_table, peering_table); + assert_eq!( + overlay.validate(), + Err(ConfigError::StatefulPlusStatelessNat( + "Peering-1".to_owned() + )) + ); + } + #[test] fn test_overlay_missing_vpc() { /* build VPCs */ diff --git a/config/src/external/overlay/vpc.rs b/config/src/external/overlay/vpc.rs index 891f10aaa..cde45ad0e 100644 --- a/config/src/external/overlay/vpc.rs +++ b/config/src/external/overlay/vpc.rs @@ -50,16 +50,21 @@ impl Peering { } fn validate_nat_combinations(&self) -> ConfigResult { - // We do not support stateful NAT on both sides of a peering + // If stateful NAT is set up on one side of the peering, we don't support NAT (stateless or + // stateful) on the other side. + let mut local_has_nat = false; let mut local_has_stateful_nat = false; for expose in &self.local.exposes { if expose.has_stateful_nat() { local_has_stateful_nat = true; + local_has_nat = true; break; + } else if expose.has_stateless_nat() { + local_has_nat = true; } } - if !local_has_stateful_nat { + if !local_has_nat { return Ok(()); } @@ -67,6 +72,9 @@ impl Peering { if expose.has_stateful_nat() { return Err(ConfigError::StatefulNatOnBothSides(self.name.clone())); } + if expose.has_stateless_nat() && local_has_stateful_nat { + return Err(ConfigError::StatefulPlusStatelessNat(self.name.clone())); + } } Ok(()) }