Skeleton for stateful NAT #560
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
daniel-noland
previously requested changes
Jun 7, 2025
dataplane/src/nat/stateful/mod.rs
Outdated
| } | ||
|
|
||
| impl NatState { | ||
| fn new(net: &Net, pool: &allocator::NatPool) -> Self { |
There was a problem hiding this comment.
Are we expecting to clone net and pool? I would expect these to be transferred in as owned
There was a problem hiding this comment.
Yep, that's some detail I didn't pay too much attention to when drafting the skeleton, but you're absolutely correct. I'll change.
There was a problem hiding this comment.
Wait I replied too fast, why should they be passed as owned? They won't be part of the generated NatState, they'll just be used read-only to create the new state. So by reference should be the way to go? Otherwise, I've got to clone them before passing them to the NatState::new() method.
qmonnet
force-pushed
the
pr/qmonnet/stateful-nat-skeleton
branch
3 times, most recently
from
6a64bf4 to
eba3905
Compare
... In prevision for stateful NAT support. Signed-off-by: Quentin Monnet <[email protected]>
Make room for stateful NAT: Move all the code dedicated to stateless NAT into a dedicated file, in preparation for stateful NAT support. Signed-off-by: Quentin Monnet <[email protected]>
It makes more sense to expose these headers directly as part of the Net implementation, so that other components can reuse them. Signed-off-by: Quentin Monnet <[email protected]>
Get started with stateful NAT. There's nothing functional yet, this is just a very early skeleton for the logic. Committing this part first allows us to later create Pull Requests to focus on the more complex components (in particular, IP/port allocation and session management). Signed-off-by: Quentin Monnet <[email protected]>
Having a trait for the NAT pool (that handles IP and port allocation) makes it easier to try alternative implementations in the future. Signed-off-by: Quentin Monnet <[email protected]>
Similarly to what we did for the NAT IP/port allocator, implement the sesson manager as a trait so that we can quickly experiment with various implementations in the future. Signed-off-by: Quentin Monnet <[email protected]>
qmonnet
force-pushed
the
pr/qmonnet/stateful-nat-skeleton
branch
from
eba3905 to
f5884fb
Compare
Instead of using an IpAddr object, make the IP/port allocator and the session manager use a sealed NatIp trait, to avoid multiple matches for the exact IP version in the core of the NAT logics. We need to pass two distinct types to the stateful_nat() function: one is the type for the original IP address, the second one for the target IP address. Suggested-by: Daniel Noland <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>
|
@daniel-noland your feedback should have been addressed, can you please take another look? Are there some additional refinements you were talking about? |
mvachhar
approved these changes
Jun 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce a very simple skeleton for stateful NAT. There's nothing functional yet, this is just a very early skeleton for the logic. Committing this part first allows us to later create Pull Requests to focus on the more complex components (in particular, IP/port allocation and session management).
It turns out we don't share that much code with the stateless NAT portion of the pipeline stage; we may split stateless and stateful NAT into distinct pipeline stages in the future, this is yet to be determined (and doesn't matter much at this stage).
Follow-ups: