Update NAT example to illustrate "shared IPs" issue on session collisions #1086
+164
−90
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
added
area/nat
This comment was marked as resolved.
This comment was marked as resolved.
Add a variant of the method to update an allocator, only compiled for tests, so that we can turn off randomness for port block ordering for stateful NAT port allocation in tests. This matters, for example, when we want to check whether, all other items being equal, we can have collisions on allocated ports, and hence on allocated NAT sessions, in specific test cases. Signed-off-by: Quentin Monnet <[email protected]>
We recently added support for "shared IPs" in stateful NAT (for example, when two different VPCs expose the same NAT-ed IPs to a given VPC), but the design has an issue: when the exposed IPs on both sides of the two peerings of our example are identical, we risk creating identical flow table entries for the destination VPC lookup stage (the keys for the entries for reverse connections from the VPC seeing shared IPs would normally differ on their destination VPC discriminant, but this field is set to None for the entries for destination VPC lookup, precisely because we don't know them at the lookup stage). Update our test to illustrate this scenario. Why didn't the test catch it sooner? Two reasons: - We had "random" port allocation turned on, meaning that collisions on the source port for packets from VPC-1 to VPC-2 and VPC-3 to VPC-2 were unlikely in the first place - although I did try turning it off manually in the code before merging the test. - The main problem is that we first try to see if a packet from VPC-3 with different IPs and ports from the one sent from VPC-1 triggers an issue. It doesn't, of course, the flow table entries are different. But this packet "picks" the first available port from the port range for single allocated IP address, which means that when we later try with a packet from VPC-3 with the same IPs and ports as the packets from VPC-1, we cannot get the same port allocated (we get the second of a port block, while the packets from VPC-1 use the first port of their port block). To make sure we trigger the issue, turn off "random" port allocation, and remove the intermediate step that used different IPs and ports. Make sure to document the issue so we can come back and adjust the test once the issue is fixed. The issue itself is tracked at #1083. Signed-off-by: Quentin Monnet <[email protected]>
Additional logs turned out to be helpful to understand what was going on during the test. Let's keep them on in order to obtain more information if this test ever fails again. Given that we need to remove the "#[traced_test]" annotation, we re-add logs for target "stateful-nat" manually. Signed-off-by: Quentin Monnet <[email protected]>
qmonnet
force-pushed
the
pr/qmonnet/shared-ips-nat-pb
branch
from
d469c45 to
826ab45
Compare
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.
We recently added support for "shared IPs" in stateful NAT (for example, when two different VPCs expose the same NAT-ed IPs to a given VPC), but the design has an issue: when the exposed IPs on both sides of the two peerings of our example are identical, we risk creating identical flow table entries for the destination VPC lookup stage (the keys for the entries for reverse connections from the VPC seeing shared IPs would normally differ on their destination VPC discriminant, but this field is set to None for the entries for destination VPC lookup, precisely because we don't know them at the lookup stage).
Here we want to update the related stateful NAT test to illustrate and acknowledge the issue.
Related issue: