more proxy tests and fixes #822
Merged
Conversation
This file contains 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
|
At this point conntrack_local set via 'cilium config conntrack_local={false|true}' no longer appears to impact anything. If we want to remove it then the attached test case modifications are not needed and a couple additional deletes from endpoint.go could be added. |
|
Still need to change tuples on the Go side. See 9cf0ee2. Can you elaborate on |
|
OK, I need a few more fixes on the go side like @borkmann notes. |
tgraf
reviewed
May 26, 2017
| @@ -16,26 +16,27 @@ CLIENT_LABEL="id.client" | |||
| cleanup | |||
| logs_clear | |||
|
|
|||
| docker network inspect $TEST_NET 2> /dev/null || { | |||
There was a problem hiding this comment.
Can we merge 10-proxy.sh and 14-proxy-ingress.sh together?
There was a problem hiding this comment.
sure. patches collapse easy enough.
There was a problem hiding this comment.
aha you are not worried about the patches you mean the actual *.sh files, sure I can do that. Follow up PR though.
Proxy tests for conntrack local case. Signed-off-by: John Fastabend <[email protected]>
Proxy tests for conntrack local case. Note, I avoided building a larger unified test function and bash helpers because I like being able to see an entire test in a single file when debugging. Signed-off-by: John Fastabend <[email protected]>
There is a bug in the proxy code when local conntrack is enabled.
The issue is the proxy{4,6} map uses the original source IP in the key
when configuring the proxy. This allows the key to be matched by
the proxy on the 'from-netdev' side by using the 'dip' (which is
the client's IP) to lookup the address translation. The result
from the lookup is then used to translate the ports and source IP.
In the global CT case the ct_lookup{4,6} path reverses the saddr and
daddr in the ipv{4,6}_ct_tuple and, assuming no match is found in
the table, calls ct_create{4,6} to add a new key/value pair to the
proxy{4,6} map. When the key is created for this entry the 'daddr'
in the tuple is used which is actually the 'source' IP from the
packet (in other words its the clients IP address).
However, in the local CT case the tuple only has a single addr
and it is not swapped by the tuple reverse logic in ct_lookup{4,6}.
The result is ct_create{4,6} creates a key using the only available
address, which is the destination IP from the packet which corresponds
to the server. Then when the proxy sends packets, using the
'from-netdev' code path, a key is built using the destination IP
(the client IP from this direction) but there is no match in the
proxy{4,6} map due to the above bug (specifically the key being built
with the server IP).
To resolve this we came up with two solutions. One is to pass
the original source IP and use this in ct_create{4,6} as the source
address. But this gets ugly and we already pass by value the
orig_dip and orig_was_proxy so adding yet another value is getting
a bit awkward. The other option, and the one we implemented, is
to convert local CT over to use the same tuple structure as the
global CT implementation so that both modes have the same data
structures available to build the key from.
This solves the above issue and simplifies the code instead
of complicating it furher. With the nice benefit of being a patch
full of removals and creating one less #define to worry about.
The disadvantage is the key size is sizeof('address') larger in
the conntrack local case. However, this is not the default case
so I believe the trade-off is worth it.
Signed-off-by: John Fastabend <[email protected]>
The keys are the same for both local and glocal case now so we can remove at least a few additional switch statements. Signed-off-by: John Fastabend <[email protected]>
Signed-off-by: John Fastabend <[email protected]>
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.
The previous pull request fixed up the build error this pull request adds additional tests and then fixes the code to handle the additional cases.
Note I made a design decision here to drop the local/global CT distinction. The caveat is in the local case we now use an 'IP address' worth of extra memory in the local CT case. The result is the code is much simpler and easier to read.
Thanks!