[2/3] [nat] introduce NatAddress trait - #335
Conversation
2c7f158 to
ce64fd3
Compare
nicolaskagami
changed the title
NatFamily traitNatAddress trait
| } | ||
| } | ||
|
|
||
| impl NatAddress for Ipv4Addr { |
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?
There was a problem hiding this comment.
We could, but I didn't want to add new functionality on these PRs.
There was a problem hiding this comment.
additional validations could be added later
There was a problem hiding this comment.
I really feel like such a structure should be added on a dedicated PR, where we can discuss the semantics we want to enforce.
a01c3aa to
d7008db
Compare
22d9c68 to
1b86758
Compare
d7008db to
25b94af
Compare
1b86758 to
6265bc6
Compare
| } | ||
| } | ||
|
|
||
| impl NatAddress for Ipv4Addr { |
There was a problem hiding this comment.
additional validations could be added later
6265bc6 to
8293a3e
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.
8293a3e to
c597b42
Compare
bnaecker
left a comment
bnaecker
left a comment
There was a problem hiding this comment.
Looks good, nice cleanup. Just a few questions / nits.
| nat.ipv6_mappings.clear(); | ||
| if let Err(e) = nat::reset_ipv6(switch) { | ||
| error!(switch.log, "failed to reset ipv6 nat table: {:?}", e); | ||
| Err(e) | ||
| } else { | ||
| Ok(()) | ||
| } | ||
| table::nat::reset::<Ipv6Addr>(switch) |
There was a problem hiding this comment.
This predates your PR, but it might make sense to swap the order of operations here. If we fail to delete the entries on switch table, we'll have already cleared the local mappings, and have some torn state. Could we move the nat.ipv6_mappings.clear() call after a successful call to table::nat::reset?
There was a problem hiding this comment.
Good catch. Definitely an underlying bug I can happily fix. This could've made the mapping representation drift from the actual switch state.
| } else { | ||
| Ok(()) | ||
| } | ||
| table::nat::reset::<Ipv4Addr>(switch) |
There was a problem hiding this comment.
Same here, I'd argue we should swap clearing the in-memory and switch-table mappings.
| s.table_dump::<A::MatchKey, A::Action>(A::TABLE, from_hardware) | ||
| } | ||
|
|
||
| pub(crate) fn counter_fetch<A: NatAddress>( |
There was a problem hiding this comment.
Small nit, but I think the counter- and dump-related functions can be private to the table mod. They're only called from dpd/src/table/mod.rs (e.g., line 333), not more generally in the dpd crate.
There was a problem hiding this comment.
Yeah, they could be pub(super)
3 participants
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.