Skip to content

[2/3] [nat] introduce NatAddress trait - #335

Open
nicolaskagami wants to merge 1 commit into
nsk/nat-1-portrangefrom
nsk/nat-2-nat-family
Open

[2/3] [nat] introduce NatAddress trait#335
nicolaskagami wants to merge 1 commit into
nsk/nat-1-portrangefrom
nsk/nat-2-nat-family

Conversation

@nicolaskagami

@nicolaskagami nicolaskagami commented Aug 6, 2026

Copy link
Copy Markdown

This PR:

  • Introduces a NatAddress trait, 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.rs code.

Obs: Changes are almost entirely equivalent, except for the ordering of some things and the log message nat tables -> nat table.

@nicolaskagami nicolaskagami self-assigned this Aug 6, 2026
@nicolaskagami nicolaskagami changed the title [2/3] [nat] introduce NatFamily trait [2/3] [nat] introduce NatAddress trait Aug 6, 2026
@nicolaskagami
nicolaskagami marked this pull request as ready for review August 6, 2026 18:00
Comment thread dpd/src/nat.rs Outdated
Comment thread dpd/src/table/nat.rs
}
}

impl NatAddress for Ipv4Addr {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but I didn't want to add new functionality on these PRs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

additional validations could be added later

@nicolaskagami
nicolaskagami force-pushed the nsk/nat-2-nat-family branch 2 times, most recently from 22d9c68 to 1b86758 Compare August 7, 2026 13:47
Comment thread dpd/src/nat.rs Outdated
} else {
Ok(())
}
Ipv6Addr::reset(switch)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 This feels less clear than the older representation. If nat::reset_ipv6 was called in the wrong context it would be clear. If Ipv6Addr::reset(switch) is called in any other context it would not be clear if we're doing the correct thing or not without looking into the function to see what it did.

I feel like this is a close-but-slightly-awkward abstraction and this should be something more akin to an Ipv6NatTable type that implements a NatTable trait, or something in that spirit. I can be convinced otherwise, but even after sleeping on it the proposed representation feels weird to me.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I split this and #336 in an unfortunate place since this does go away in the next PR. Should be easy to address it on this one though.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 8293a3e. Definitely better, thanks.

Comment thread dpd/src/nat.rs Outdated
} else {
Ok(())
}
Ipv4Addr::reset(switch)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Comment thread dpd/src/table/nat.rs
}
}

impl NatAddress for Ipv4Addr {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

additional validations could be added later

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants