Stateless NAT: Fix reverse path #648
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
qmonnet
force-pushed
the
pr/qmonnet/stateless-nat-reverse-path
branch
2 times, most recently
from
169d620 to
a10c5ca
Compare
|
The PR needs commit re-ordering and some clean-up, but I Think It Works ™️ |
This reverts commit 05bca3d. Turns out it's really too complex to have the tests far from the NAT config code, because it means we need to update the huge intermediate struct each time we want to test something new. Let's keep the tests in the mgmt crate, even if it looks less clean. Signed-off-by: Quentin Monnet <[email protected]>
This reverts commit 46e32fa. Turns out it's really too complex to have the tests far from the NAT config code, because it means we need to update the huge intermediate struct each time we want to test something new. Let's keep the tests in the mgmt crate, even if it looks less clean. Signed-off-by: Quentin Monnet <[email protected]>
qmonnet
force-pushed
the
pr/qmonnet/stateless-nat-reverse-path
branch
from
a10c5ca to
9f13b2f
Compare
It's better to have distinct source and destination IP addresses in the test packet that we generate with build_test_ipv4_packet(), to avoid any confusion as to what address we're processing (and because it makes little sense to use the same addresses in the first place). Adjust related tests accordingly: these are the tests for stateless NAT, in the "mgmt" package. Suggested-by: Fredi Raspall <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>
Rather than having two different tests for source and destination NAT, make sure that we try both the reply and request packet, thus validating NAT in both directions. Note that the test is not working at this stage: our objective is to make it work by fixing the code. We temporarily compile out the "tests" module to avoid failing tests. Signed-off-by: Quentin Monnet <[email protected]>
Remove the enum NatDirection, and all related logics. Instead, have the stateless NAT pipeline stage do both source and destination NAT, based on the contents of the NAT tables. The NAT tables need to be updated accordingly, in a future commit. Translation in stateful NAT has not been adjusted yet, this will also come in a future commit. Signed-off-by: Quentin Monnet <[email protected]>
We've been using the wrong prefixes when building the peers table for the first lookup for source NAT: we've used the "ips" blocks instead of the "as_range". We missed it in the tests, because we also use the wrong address for the lookup: we would do source IP when we should use the destination IP to find the remote VPC, of course. Fix both the table and the lookup. Reported-by: Manish Vachharajani <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>
In the previous commit, we fixed one bug for the source NAT lookups: we made the NAT table use the "as_range" prefixes rather than the "ips" prefixes for the virtual "remote peer VPC" lookup, in order to match the destination address of packets against public IPs for the remote VPC. This was not enough to fix NAT! We have two other issues: - First, the key we use for the "peering_index" in the PrefixTrie lookup are the "as_range" of the _local manifest_, which is wrong. We should have the prefixes for the "as_range" of the exposes of the _remote_ manifest, so we need to move the "peering_index" insertion (when building the table) to the loop where we process the _remote_ exposes. - This leads to the second issue: we don't have a 1:1 mapping between the VpcExpose objects of the two manifests, which means that a destination address corresponding to a given remote peer VPC can correspond to any of the configured VpcExpose objects for the peering involving this remote VPC. So we can't retrieve a single index, but a list of indices. Let's fix the code accordingly. Move the insertion of peer indices to the section in "add_peering()" where we process the remote "expose" blocks. There, make sure we insert a vector containing indices for all "expose" objects of the local VPC for the peering. Adjust the insertion and lookup logics accordingly in the "nat" crate, too. Signed-off-by: Quentin Monnet <[email protected]>
qmonnet
force-pushed
the
pr/qmonnet/stateless-nat-reverse-path
branch
from
9f13b2f to
32bc309
Compare
mvachhar
approved these changes
Jun 29, 2025
There was a problem hiding this comment.
I think this looks right now, and the tests which are way more complete seem to confirm it. I will refrain from merging until @Fredi-raspall confirms this can work with routing.
We have yet another issue when building the tables: if we do NAT on one side only, we won't populate the data necessary for doing source NAT for the remote VPC, because we don't find any as_range to use. What we need to do is to fall back on "ips" when there's no as_range prefixes, because this is where the public prefixes are contained in that case. We also have an issue where we don't find a mapping in the list for destination NAT, which is easily solved by not creating destination NAT rules at all if we don't have any as_range for a given expose. Reported-by: Manish Vachharajani <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>
Add the missing PerVniTable for the reverse path TODO: Complete this description Signed-off-by: Quentin Monnet <[email protected]>
Add a test to generate the NAT tables from a minimal but standalone GwConfig object, using four different VPCs for the overlay, to test stateless NAT in both directions between the exposes of multiple peerings. Signed-off-by: Quentin Monnet <[email protected]>
We don't currently support such configuration in NAT. Let's reject such exposes at validation time. Link: #650 Signed-off-by: Quentin Monnet <[email protected]>
qmonnet
force-pushed
the
pr/qmonnet/stateless-nat-reverse-path
branch
from
32bc309 to
a2432f0
Compare
mvachhar
approved these changes
Jun 29, 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.
No description provided.