Skip to content

feat(nat): Use actual source-VPC VNI with NAT #599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 18, 2025

Conversation

qmonnet
Copy link
Member

@qmonnet qmonnet commented Jun 17, 2025

Based on #598

Based on Fredi's recent work to add the VNI from the source VPC to packet metadata, here's an update to the NAT code to read the VNI from metadata (rather than the hardcoded placeholder that we had).

We also update unit tests accordingly.

@qmonnet qmonnet added this to the GW R1 milestone Jun 17, 2025
@qmonnet qmonnet requested a review from Fredi-raspall June 17, 2025 10:27
@qmonnet qmonnet self-assigned this Jun 17, 2025
@qmonnet qmonnet added the area/nat Related to Network Address Translation (NAT) label Jun 17, 2025
Base automatically changed from pr/fredi/metadata_vni to main June 17, 2025 12:11
@qmonnet qmonnet marked this pull request as ready for review June 17, 2025 13:55
@qmonnet qmonnet requested a review from a team as a code owner June 17, 2025 13:55
@qmonnet qmonnet requested a review from mvachhar June 17, 2025 16:07
Copy link
Collaborator

@daniel-noland daniel-noland left a comment

Choose a reason for hiding this comment

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

This code looks fine to me.

My only concern is that one we have already discussed: using VNI where it would make more sense to me to use VRF id.

@qmonnet
Copy link
Member Author

qmonnet commented Jun 18, 2025

My only concern is that one we have already discussed: using VNI where it would make more sense to me to use VRF id.

That discussion happened in the context of stateful NAT. I understand we probably want to use the same for stateless NAT as well, but the current implementation (for stateless) has been using the VNI from the beginning, and changing it to the VRF id is beyond the scope of the current PR (we'd need to change the stateless NAT tables to use the VRF id as a key rather than the VNI when we build the tables). I'll merge this, but create a ticket to track it.

Thanks for the review!

Based on Fredi's recent work to add the VNI from the source VPC to
packet metadata, here's an update to the NAT code to read the VNI from
metadata (rather than the hardcoded placeholder that we had).

We also update unit tests accordingly.

Signed-off-by: Quentin Monnet <[email protected]>
@qmonnet qmonnet force-pushed the pr/qmonnet/nat_vni_lookup branch from 4d58c00 to 2fd9636 Compare June 18, 2025 08:47
@qmonnet qmonnet enabled auto-merge June 18, 2025 08:49
@qmonnet qmonnet disabled auto-merge June 18, 2025 08:50
@qmonnet qmonnet enabled auto-merge June 18, 2025 08:50
@qmonnet qmonnet added this pull request to the merge queue Jun 18, 2025
Merged via the queue into main with commit 3d9db2f Jun 18, 2025
16 checks passed
@qmonnet qmonnet deleted the pr/qmonnet/nat_vni_lookup branch June 18, 2025 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/nat Related to Network Address Translation (NAT)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants