hubble: add SNAT IP flow field and filter #32130
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
chancez
reviewed
Apr 22, 2024
chancez
requested changes
Apr 22, 2024
|
Oops just noticed this is a draft after submitting my review. Feel free to ignore as needed. |
89a9a26 to
8014b4a
Compare
8014b4a to
3ef8c5c
Compare
chancez
reviewed
Apr 30, 2024
chancez
reviewed
Apr 30, 2024
f536e36 to
b52e943
Compare
|
#31226 has been merged and a commit was added to add CLI support. Marking as ready to review. |
b52e943 to
540de42
Compare
|
/test |
In an egress gateway setup, the packet leaving the egress gateway to the destination is SNAT'ed with the egress gateway IP address. Before this patch, the corresponding flow would be reported wrongly as INGRESS by Hubble, as the traffic direction heuristic would see a non-reply flow emitted by a different BPF program that the one attached to the source lxc interface. This patch set the traffic direction to EGRESS when we detect that the packet was SNAT'ed, which also fix the lb case. Signed-off-by: Alexandre Perrin <alex@isovalent.com>
Since cel_expression is part of the new Experimental message, it doesn't have to follow the FlowFilter field number. Also fix drop_reason_desc field to use the highest unused field number. Both fields were introduced recently (i.e. after v1.15) and thus this commit doesn't break backward compatibility with any currently released Cilium version at the time of writing. Signed-off-by: Alexandre Perrin <alex@isovalent.com>
Move the ip != nil check earlier since the block is about setting ip related fields and the check is repeated before this patch. This will simplify the next commit introducing new logic related to ip.Source. Signed-off-by: Alexandre Perrin <alex@isovalent.com>
Before this patch, when TraceNotify.OriginalIP was set, the Hubble parser would use it as ip.Source discarding the source IP parsed from the packet header. This patch introduce a new IP.source_xlated field, preserving the parsed header's source IP, allowing to expose the actual IP used in the Egress Gateway and LB cases. Signed-off-by: Alexandre Perrin <alex@isovalent.com>
The previous commit introduce a new SNAT IP field (source_xlated) in Hubble flow data. This commit is a follow-up adding a corresponding Hubble filter by SNAT IP either with exact match or CIDR match. Note that unlike source and destination IP, source_xlated might be empty. Signed-off-by: Alexandre Perrin <alex@isovalent.com>
The previous commits introduced the IP.source_xlated flow field and a corresponding source_ip_xlated filter at the Hubble API level. This patch is a follow-up adding source_ip_xlated support to the Hubble CLI. After this patch, the --ip flag behaviour is arguably unintuitive as it will not take source_ip_xlated into account (i.e. only source_ip and destination_ip). Unfortunately, addressing this limitation would require reworking the Hubble CLI internal left/right filter setup, which it outside of the scope of this commit. The --ip help message was adapted accordingly with the intent of clearing the potential confusion. Signed-off-by: Alexandre Perrin <alex@isovalent.com>
540de42 to
5bd08e5
Compare
|
/test |
rolinh
approved these changes
May 3, 2024
gentoo-root
approved these changes
May 7, 2024
| @@ -189,6 +189,7 @@ func newFlowFilter() *flowFilter { | |||
| {"from-pod", "namespace", "all-namespaces"}, | |||
| {"to-service", "namespace", "all-namespaces"}, | |||
| {"from-service", "namespace", "all-namespaces"}, | |||
| {"snat-ip"}, | |||
There was a problem hiding this comment.
Maybe I'm missing something, but what's the point of conflict lists that consist of one item? What do they check?
There was a problem hiding this comment.
That's a good question 😅 I think there's no point in doing that although we apparently did it for other filters as well.
tklauser
approved these changes
May 8, 2024
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.
NOTE: draft until #31226 is merged, also currently missing the Hubble CLI bitsAdd a new
source_xlatedfield to HubbleFlow.IPand a corresponding IP filter.Fix the
FlowFilter.Experimental.cel_expressionandFlowFilter.drop_reason_descprotobuf field numbers on the way.