Preliminary work for static NAT source/destination split by qmonnet · Pull Request #1419 · githedgehog/dataplane

qmonnet · 2026-03-27T20:53:27Z

As the title says, this is some preliminary work for splitting the current static NAT stage into two steps, source and destination NAT. This will be necessary for supporting static NAT + masquerade at opposite ends of a peering. Here we do not split the stage, but we prepare for that by moving source and destination translation into individual functions. The biggest chunk of these changes is the modifications brought to the code for translating ICMP Error messages' inner packets (and the biggest part of the diff is a not-so-related clean-up in NAT tests, but it's mostly an indentation change).

~~Will need to be rebased on top of #1414 and possibly #1389.~~ I'll handle conflicts if necessary.

Copilot

Pull request overview

Prepares the stateless NAT implementation for an eventual split between source-NAT and destination-NAT by refactoring ICMP error-message inner-packet translation into dedicated src/dst helpers, and by tightening/clarifying related packet/test utilities.

Changes:

Refactors ICMP error inner-packet translation into *_src and *_dst functions and updates stateless NAT to call them from separate source_nat() / destination_nat() steps.
Adds Packet::is_icmp_inner_translation_candidate() to centralize the “ICMP error with embedded packet” predicate.
Cleans up ICMP error-message tests and exposes a shared Ethernet test header builder.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
net/src/packet/utils.rs	Adds an ICMP-inner translation candidate predicate on `Packet`.
net/src/packet/test_utils.rs	Documents and exports `make_default_for_eth()` for reuse in tests.
nat/src/stateless/mod.rs	Splits translation into `source_nat()` / `destination_nat()` and uses new ICMP inner src/dst translation helpers.
nat/src/icmp_handler/icmp_error_msg.rs	Splits ICMP inner translation into src/dst functions; changes checksum validation API and updates tests accordingly.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

daniel-noland

I'm really confused by the whole approach taken here. Maybe there is some reason for this approach I can't see, but I think that

completely refusing to use the qualified types from net and
dealing with ipv4 and ipv6 at the same time and
taking Packet where you could take Headers

has made life hard here for no real reason.

I would also really like to see fuzz tests to catch edge cases in this type of logic.

Granted, many of these problems (if problems they are, I may have missed something fundamental) predate this PR, but some of it does not.

I'm concerned about the whole direction here. Especially as regards the global reasoning and use of unreachable!() when the methods are not

marked unsafe or
structurally defended against arbitrary crash by the type system.

unreachable!() isn't for code that is actually unreachable. Not for code which is simply not reached in the current impl or tests.

That said, do let me know if I missed something major here. I'm happy to chat about it

daniel-noland · 2026-04-13T21:41:29Z

+
+pub(crate) fn nat_translate_icmp_inner_src<Buf: PacketBufferMut>(
+    packet: &mut Packet<Buf>,
+    target_addr: IpAddr,


why not accept UnicastIpAddr instead of all the extra error handling and branches?

It would move the same error handling (and branches) to the caller, possibly at multiple calling locations, and I'm not sure what the advantage of that would be ?

daniel-noland · 2026-04-13T21:48:23Z

+pub(crate) fn nat_translate_icmp_inner_dst<Buf: PacketBufferMut>(
+    packet: &mut Packet<Buf>,
+    target_addr: IpAddr,
+    target_port: Option<NatPort>,
+) -> Result<(), IcmpErrorMsgError> {


this type of thing would be much easier as a method which accepted an Ipv4, an Icmp4 and a UnicastIpv4Addr + Option<NatPort> + another method with ipv6 equivalents

As is, this

requires the method to be generic over the buffer (which is not needed)

requires a bunch of error handling which could be avoided by moving it closer to the front of the pipeline

doesn't structurally catch errors regarding various packet shapes.

I don't really understand why we are trying to handle ipv4 and ipv6 at the same time. That is making this much harder than is needed (and likely slowing it down a fair bit)

Not sure I follow all of this. We need to chat.

daniel-noland · 2026-04-13T21:51:32Z

+    let old_port = transport
+        .destination()
+        .unwrap_or_else(|| unreachable!())
+        .into();


crash on invalid data!?!

We need to have a discussion about this.

daniel-noland · 2026-04-13T21:58:28Z

+    fn destination_nat<Buf: PacketBufferMut>(
+        &self,
+        table: &PerVniTable,
+        packet: &mut Packet<Buf>,
+    ) -> Result<bool, StatelessNatError> {


why accept Packet instead of Headers? Aren't we just making our lives hard here?

I don't know. Are we? Without the Packet we need to pass down all the different header layers, plus the packet metadata, plus the attached flow information, to all subfunctions that need it, I think? And it doesn't sound easier, so I'm not sure I follow what you're suggesting. Let's chat?

I'm ok with this provisionally (mostly because it is an "in flight" project and many of my concerns are out of scope).

That said, I would like to address some of these patterns for future work

daniel-noland

approved under the theory that we discuss and address the patterns in this code tomorrow.

We don't need to change instantly, but I would like this pattern to change

We can make the function slightly less verbose by setting ports directly from the packet object, rather than the transport layer. Signed-off-by: Quentin Monnet <qmo@qmon.net>

Not setting the EtherType for the packet results in a failure to parse the IP header, and to actually test the different code paths in validate_checksums_icmp(). Signed-off-by: Quentin Monnet <qmo@qmon.net>

This is in preparation for splitting source and destination NAT in the static NAT pipeline stage. To do so, we need the translation of an ICMP Error inner packet header to happen in two steps, as well. So in this commit, we split nat_translate_icmp_inner() into two functions, nat_translate_icmp_inner_src() and nat_translate_icmp_inner_dst(), taking care of the source and destination translation, respectively. This requires splitting translate_inner_tcp_udp() into two functions as well; translate_inner_icmp() needs to be called only once and does not need as many changes. Note that we refactor _but keep_ the original function nat_translate_icmp_inner() as well, because it is still in use in the masquerading pipeline stage. It now delegates to the two source and destination functions. Signed-off-by: Quentin Monnet <qmo@qmon.net>

Function validate_checksums_icmp() used to return a Result<bool, IcmpErrorMsgError> to indicate, in case of success, whether the packet should go through translation for the inner packet embedded in an ICMP Error outer packet. This is not very readable. Instead, split this logic into two steps, the check on whether we should attempt translation, and the checksum validation. As part of this we make the function return a Result<(), IcmpErrorMsgError> instead; and we call the newly-defined method is_icmp_inner_translation_candidate(packet) to check whether the inner packet should endure translation and checksum validation at all, before that. Signed-off-by: Quentin Monnet <qmo@qmon.net>

In preparation for splitting the code for static NAT into source NAT and destination NAT, we want to perform translation of ICMP Error messages' embedded headers in two steps, destination and source. To this aim, we replace translate_icmp_inner_packet_if_any() into two sub-step variants, translate_icmp_inner_packet_dst_if_any() (for the destination) and translate_icmp_inner_packet_src_if_any() (for the source). Method find_translation_icmp_inner() gets factored in the others in the process. Signed-off-by: Quentin Monnet <qmo@qmon.net>

In order to support static NAT on one end of a peering, and masquerading on the other end, we'll need to split and reorganise the static NAT code. In preparation for this work, move toward an accentuated dissociation of the source and destination translations for the stateless NAT code: move each part, source and destination translation, including ICMP Error messages' inner packets translation for each direction, into its own method for the StatelessNat object, source_nat() and destination_nat(), respectively. Signed-off-by: Quentin Monnet <qmo@qmon.net>

Daniel reports that we lose information by using error variant StatelessNatError::UnsupportedTranslation at several locations, where distinct events cause the error to happen. Add other enum variants instead, and log the error before converting it to more generic DoneReason variants. Reported-by: Daniel Noland <daniel@githedgehog.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>

Keep more information about the error we hit when propagating the result from ICMP Error messages processing, by adding more variants to the error enum to better distinguish the different error cases. Reported-by: Daniel Noland <daniel@githedgehog.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>

qmonnet requested a review from Fredi-raspall March 27, 2026 20:53

qmonnet added the area/nat Related to Network Address Translation (NAT) label Mar 27, 2026

qmonnet force-pushed the pr/qmonnet/split-static-nat branch 2 times, most recently from 420bcdb to 69d870b Compare April 10, 2026 14:08

qmonnet mentioned this pull request Apr 13, 2026

test(nat): Remove unnecessary "tests" submodules #1455

Merged

qmonnet force-pushed the pr/qmonnet/split-static-nat branch from 69d870b to f794ef5 Compare April 13, 2026 11:03

qmonnet marked this pull request as ready for review April 13, 2026 11:04

qmonnet requested a review from a team as a code owner April 13, 2026 11:04

Copilot AI review requested due to automatic review settings April 13, 2026 11:04

qmonnet self-assigned this Apr 13, 2026

Copilot started reviewing on behalf of qmonnet April 13, 2026 11:05 View session

Copilot AI reviewed Apr 13, 2026

View reviewed changes

Comment thread nat/src/icmp_handler/icmp_error_msg.rs

Comment thread nat/src/stateless/mod.rs Outdated

qmonnet force-pushed the pr/qmonnet/split-static-nat branch 2 times, most recently from fcfd79f to 14a72f4 Compare April 13, 2026 20:10

daniel-noland requested a review from Copilot April 13, 2026 21:23

Copilot started reviewing on behalf of daniel-noland April 13, 2026 21:24 View session

Copilot AI reviewed Apr 13, 2026

View reviewed changes

Comment thread nat/src/stateless/mod.rs Outdated

Comment thread nat/src/stateless/mod.rs Outdated

daniel-noland previously requested changes Apr 13, 2026

View reviewed changes

qmonnet force-pushed the pr/qmonnet/split-static-nat branch from 14a72f4 to 95339c5 Compare April 15, 2026 14:17

qmonnet requested a review from daniel-noland April 15, 2026 17:01

daniel-noland approved these changes Apr 15, 2026

View reviewed changes

qmonnet force-pushed the pr/qmonnet/split-static-nat branch from 95339c5 to 53ad645 Compare April 16, 2026 10:02

qmonnet added 2 commits April 17, 2026 10:50

refactor(nat): Collapse cases for translating ports in static NAT

4024c1e

We can make the function slightly less verbose by setting ports directly from the packet object, rather than the transport layer. Signed-off-by: Quentin Monnet <qmo@qmon.net>

fix(nat): Fix packet creation in ICMP tests

4d53a4a

Not setting the EtherType for the packet results in a failure to parse the IP header, and to actually test the different code paths in validate_checksums_icmp(). Signed-off-by: Quentin Monnet <qmo@qmon.net>

qmonnet force-pushed the pr/qmonnet/split-static-nat branch from 53ad645 to b7411e9 Compare April 17, 2026 09:55

qmonnet added 4 commits April 17, 2026 10:59

qmonnet added 2 commits April 17, 2026 10:59

qmonnet force-pushed the pr/qmonnet/split-static-nat branch from b7411e9 to 018ce1f Compare April 17, 2026 09:59

Fredi-raspall added the dont-merge Do not merge this Pull Request label Apr 20, 2026

Conversation

qmonnet commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

daniel-noland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daniel-noland Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

qmonnet Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

daniel-noland Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

qmonnet Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

daniel-noland Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

qmonnet Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

daniel-noland Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

qmonnet Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

daniel-noland left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

qmonnet commented Mar 27, 2026 •

edited

Loading

qmonnet Apr 14, 2026 •

edited

Loading