From fd19678380a6190cfd1a0980c35cdbdf9ea44077 Mon Sep 17 00:00:00 2001 From: Quentin Monnet Date: Wed, 22 Apr 2026 16:26:10 +0100 Subject: [PATCH 1/2] chore(nat): Improve AllocatorError variants and messages AllocatorError::NoFreePort is not about no port block being available, but not ports being available in a block. The message is confusing: let's fix it. It's been so confusing that we used the error at a location where the error is not related (passing it an incorrect argument): let's add a new variant for that location, to make it easier to troubleshoot errors. Signed-off-by: Quentin Monnet --- nat/src/stateful/allocation.rs | 4 +++- nat/src/stateful/apalloc/port_alloc.rs | 2 +- nat/src/stateful/nf.rs | 1 + 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/nat/src/stateful/allocation.rs b/nat/src/stateful/allocation.rs index 79780ef8b..78e605271 100644 --- a/nat/src/stateful/allocation.rs +++ b/nat/src/stateful/allocation.rs @@ -14,10 +14,12 @@ pub enum AllocatorError { NoFreeIp, #[error("failed to allocate port block")] NoPortBlock, - #[error("no free port block available (base: {0})")] + #[error("no free port available in port block (block base index: {0})")] NoFreePort(u16), #[error("failed to allocate port: {0}")] PortAllocationFailed(NatPortError), + #[error("failed to reserve port: {0}")] + PortReservationFailed(u16), #[error("unsupported protocol: {0:?}")] UnsupportedProtocol(NextHeader), #[error("missing VPC discriminant")] diff --git a/nat/src/stateful/apalloc/port_alloc.rs b/nat/src/stateful/apalloc/port_alloc.rs index 21d52e101..d768cee6e 100644 --- a/nat/src/stateful/apalloc/port_alloc.rs +++ b/nat/src/stateful/apalloc/port_alloc.rs @@ -499,7 +499,7 @@ impl AllocatedPortBlock { ) })?, ) - .map_err(|()| AllocatorError::NoFreePort(port.as_u16()))?; + .map_err(|()| AllocatorError::PortReservationFailed(port.as_u16()))?; Ok(AllocatedPort::new(port, self.clone())) } diff --git a/nat/src/stateful/nf.rs b/nat/src/stateful/nf.rs index a9e42110f..d8f0f13ad 100644 --- a/nat/src/stateful/nf.rs +++ b/nat/src/stateful/nf.rs @@ -528,6 +528,7 @@ fn translate_error(error: &StatefulNatError) -> DoneReason { | StatefulNatError::IcmpError | StatefulNatError::AllocationFailure( AllocatorError::PortAllocationFailed(_) + | AllocatorError::PortReservationFailed(_) | AllocatorError::MissingDiscriminant | AllocatorError::UnsupportedDiscriminant, ) => DoneReason::NatFailure, From 3fd0605a7cee79baaf128c0e890d998665380827 Mon Sep 17 00:00:00 2001 From: Quentin Monnet Date: Mon, 27 Apr 2026 09:38:41 +0100 Subject: [PATCH 2/2] chore(nat): Move NF-related tests to nf.rs In commit 19af92e251e0 ("feat(stateful-nat): move NF to nf.rs") we moved the code for the NetworkFunction for masquerading NAT to a dedicated file, but we left the related tests behind; move them along, as it makes no sense to keep them in the near-empty mod.rs. Also remove the tracing imports from mod.rs given that there's no more code in that file. Signed-off-by: Quentin Monnet --- nat/src/stateful/mod.rs | 63 ----------------------------------------- nat/src/stateful/nf.rs | 60 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 63 deletions(-) diff --git a/nat/src/stateful/mod.rs b/nat/src/stateful/mod.rs index 24a8047ac..f6c42de15 100644 --- a/nat/src/stateful/mod.rs +++ b/nat/src/stateful/mod.rs @@ -16,68 +16,5 @@ pub use allocator_writer::NatAllocatorWriter; pub use allocator_writer::StatefulNatConfig; pub use nf::StatefulNat; -#[allow(unused)] -use tracing::{debug, error, warn}; - use tracectl::trace_target; trace_target!("stateful-nat", LevelFilter::INFO, &["nat", "pipeline"]); - -#[cfg(test)] -mod tests { - use crate::NatPort; - use net::headers::Transport; - use net::tcp::Tcp; - use net::tcp::port::TcpPort; - use net::udp::Udp; - use net::udp::port::UdpPort; - - #[test] - fn test_set_tcp_ports() { - let mut transport = Transport::Tcp(Tcp::new( - TcpPort::try_from(80).expect("Invalid port"), - TcpPort::try_from(443).expect("Invalid port"), - )); - let target_port = NatPort::new_port_checked(1234).expect("Invalid port"); - - transport - .try_set_source(target_port.try_into().unwrap()) - .unwrap(); - let Transport::Tcp(ref mut tcp) = transport else { - unreachable!() - }; - assert_eq!(tcp.source(), TcpPort::try_from(1234).unwrap()); - - transport - .try_set_destination(target_port.try_into().unwrap()) - .unwrap(); - let Transport::Tcp(ref mut tcp) = transport else { - unreachable!() - }; - assert_eq!(tcp.destination(), TcpPort::try_from(1234).unwrap()); - } - - #[test] - fn test_set_udp_port() { - let mut transport = Transport::Udp(Udp::new( - UdpPort::try_from(80).expect("Invalid port"), - UdpPort::try_from(443).expect("Invalid port"), - )); - let target_port = NatPort::new_port_checked(1234).expect("Invalid port"); - - transport - .try_set_source(target_port.try_into().unwrap()) - .unwrap(); - let Transport::Udp(ref mut udp) = transport else { - unreachable!() - }; - assert_eq!(udp.source(), UdpPort::try_from(1234).unwrap()); - - transport - .try_set_destination(target_port.try_into().unwrap()) - .unwrap(); - let Transport::Udp(ref mut udp) = transport else { - unreachable!() - }; - assert_eq!(udp.destination(), UdpPort::try_from(1234).unwrap()); - } -} diff --git a/nat/src/stateful/nf.rs b/nat/src/stateful/nf.rs index d8f0f13ad..a0ad4d0a7 100644 --- a/nat/src/stateful/nf.rs +++ b/nat/src/stateful/nf.rs @@ -568,3 +568,63 @@ impl NetworkFunction for StatefulNat { self.pipeline_data = data; } } + +#[cfg(test)] +mod tests { + use crate::NatPort; + use net::headers::Transport; + use net::tcp::Tcp; + use net::tcp::port::TcpPort; + use net::udp::Udp; + use net::udp::port::UdpPort; + + #[test] + fn test_set_tcp_ports() { + let mut transport = Transport::Tcp(Tcp::new( + TcpPort::try_from(80).expect("Invalid port"), + TcpPort::try_from(443).expect("Invalid port"), + )); + let target_port = NatPort::new_port_checked(1234).expect("Invalid port"); + + transport + .try_set_source(target_port.try_into().unwrap()) + .unwrap(); + let Transport::Tcp(ref mut tcp) = transport else { + unreachable!() + }; + assert_eq!(tcp.source(), TcpPort::try_from(1234).unwrap()); + + transport + .try_set_destination(target_port.try_into().unwrap()) + .unwrap(); + let Transport::Tcp(ref mut tcp) = transport else { + unreachable!() + }; + assert_eq!(tcp.destination(), TcpPort::try_from(1234).unwrap()); + } + + #[test] + fn test_set_udp_port() { + let mut transport = Transport::Udp(Udp::new( + UdpPort::try_from(80).expect("Invalid port"), + UdpPort::try_from(443).expect("Invalid port"), + )); + let target_port = NatPort::new_port_checked(1234).expect("Invalid port"); + + transport + .try_set_source(target_port.try_into().unwrap()) + .unwrap(); + let Transport::Udp(ref mut udp) = transport else { + unreachable!() + }; + assert_eq!(udp.source(), UdpPort::try_from(1234).unwrap()); + + transport + .try_set_destination(target_port.try_into().unwrap()) + .unwrap(); + let Transport::Udp(ref mut udp) = transport else { + unreachable!() + }; + assert_eq!(udp.destination(), UdpPort::try_from(1234).unwrap()); + } +}