[2/3] [nat] introduce NatAddress trait - #335
Open
nicolaskagami wants to merge 1 commit into
Open
Conversation
This was referenced Aug 6, 2026
nicolaskagami
force-pushed
the
nsk/nat-2-nat-family
branch
from
August 6, 2026 13:44
2c7f158 to
ce64fd3
Compare
Tie each IP address family to its p4 table, match key, and action types via a trait, with the table operations provided as default methods. Replaces the duplicated per-family entry points.
nicolaskagami
changed the title
NatFamily traitNatAddress trait
nicolaskagami
marked this pull request as ready for review
August 6, 2026 18:00
| }; | ||
|
|
||
| match nat::add_ipv6_entry(switch, nat_ip, low, high, tgt) { | ||
| match nat_ip.add_entry(switch, low, high, tgt) { |
Contributor
There was a problem hiding this comment.
🤔
- We're validating the port range with a newtype but not using it here
- We're also passing in the
NatTarget - Both the
PortRangeand theNatTargetwere used to construct theIpv6NatEntrytype earlier
Does it make sense to pass the Ipv6NatEntry here as well? It wouldn't surprise me if there were additional validations we could perform with that type as well.
Same sentiment with other similar calls.
Author
There was a problem hiding this comment.
- Some of what we're doing here is in service of [3/3] [nat] unify per-family NAT mapping #336. This is one place where it could hold this type but it wouldn't be of much use since we would immediately de-structure it, effectively adding another type to the trait just so we could de-structure it one level below.
- This PR was as semantic-preserving as possible and avoided bigger refactors. I actually think
PortRangecould be separated off and we could have a more bespoke struct / search algorithms for it (as mentioned on the description of [1/3] [nat] movePortRangeto struct #334).
| } | ||
| } | ||
|
|
||
| impl NatAddress for Ipv4Addr { |
Contributor
There was a problem hiding this comment.
Instead of implementing NatAddress for a raw V4 or V6 addr, would it make sense to create newtypes around them that provides additional validation (i.e. do we want to allow creation of a NAT entry for any Ipv4 / Ipv6 address?), and implement this trait for the newtype?
Author
There was a problem hiding this comment.
We could, but I didn't want to add new functionality on these PRs.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This PR:
NatAddresstrait, tying each IP address family to its p4 table, match key, and action types. Replaces duplicated per-family entry points.This is the second of 3 PRs simplifying and de-duplicating some of the
nat.rscode.PortRangeto struct #334NatAddresstrait #335Obs: Changes are almost entirely equivalent, except for the ordering of some things and the log message
nat tables->nat table.