Skip to content
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

bpf:hubble: update trace/drop notify for L2-less packets #37097

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

smagnani96
Copy link
Contributor

@smagnani96 smagnani96 commented Jan 20, 2025

WIP. Need discussion first. This is just to showcase a potential solution.

Results achieved (tested upon a rebase to #34958):

# IPv4
## Cilium Monitor
<- crypto flow 0x0 , identity 37044->unknown state unknown ifindex cilium_wg0 orig-ip 0.0.0.0: 10.244.1.88 -> 10.244.2.35 EchoRequest
-> crypto flow 0x0 , identity 54684->unknown state unknown ifindex cilium_wg0 orig-ip 0.0.0.0: 10.244.2.35 -> 10.244.1.88 EchoReply
## Hubble Observe
Jan 31 14:34:38.876: cilium-test-1/client-7d44dd948b-cjh5k (ID:37044) <> cilium-test-1/echo-other-node-79787668d5-xwg9k (ID:54684) from-crypto FORWARDED (ICMPv4 EchoRequest)
Jan 31 14:34:38.877: cilium-test-1/echo-other-node-79787668d5-xwg9k (ID:54684) <> cilium-test-1/client-7d44dd948b-cjh5k (ID:37044) to-crypto FORWARDED (ICMPv4 EchoReply)

# IPv6
## Cilium Monitor
<- crypto flow 0x0 , identity 37044->unknown state unknown ifindex cilium_wg0 orig-ip ::: fd00:10:244:1::3c72 -> fd00:10:244:2::ca3f EchoRequest
-> crypto flow 0xc0318447 , identity 54684->unknown state unknown ifindex cilium_wg0 orig-ip ::: fd00:10:244:2::ca3f -> fd00:10:244:1::3c72 EchoReply
## Hubble Observe
Jan 31 14:26:26.430: cilium-test-1/client-7d44dd948b-cjh5k (ID:37044) <> cilium-test-1/echo-other-node-79787668d5-xwg9k (ID:54684) from-crypto FORWARDED (ICMPv6 EchoRequest)
Jan 31 14:26:26.430: cilium-test-1/echo-other-node-79787668d5-xwg9k (ID:54684) <> cilium-test-1/client-7d44dd948b-cjh5k (ID:37044) to-crypto FORWARDED (ICMPv6 EchoReply)

Fixes: #37095

@smagnani96 smagnani96 added area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. dont-merge/preview-only Only for preview or testing, don't merge it. labels Jan 20, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 20, 2025
@smagnani96 smagnani96 added the release-note/misc This PR makes changes that have no direct user impact. label Jan 20, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 20, 2025
GetConnectionInfo is not referenced nor used in any of our sources.
Public API should be `GetConnectionSummary`, which internally calls
`getConnectionInfoFromCache`.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
The packet decoder in our Cilium Monitor utility never checks for errors
returned from the DecodeLayers function. In Hubble, we do have identical
code, but with the additional checks on that returned error code.
We should try to keep these tool utilities as aligned as possible to avoid
misunderstanding: in case of a decode error, Cilium Monitor would still
try to use the decoded info.

Since v1.1.18, DecodeLayers returns a non-nil error for an empty packet.
Skip decoding and truncate layers to avoid accidental re-use.
See google/gopacket#846

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
This patch enabled the correct decoding of trace notifications when
emitted from an L3 device (ex. Wireguard). Prior to this patch, our
Monitor/Hubble logic always attempted to decode the raw bytes starting
from the Ethernet Layer. This of course doesn't hold true for L3 devices,
where we never see the L2 layer.

Since we already have a flag field in the trace notification with some
unused padding, let's borrow a bit to indicate whether the packet is being
logged from an L3 device or not. This could have been also achieved from
Monitor/Hubble directly, since the notification even has the ifindex, but
it would have required to add logic to keep ifindexes related to L3
interfaces up-to-date in Hubble/Monitor. IMHO, (1) this might not scale
well, and (2) it is much clearer to have all info forwarded from the
datapath. I'm not concerned about performance implication here since
no new data is being forwarded in the trace event.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
Similarly as the previous commit, this patch enabled the correct decoding
of drop notifications when emitted from an L3 device (ex. Wireguard).

Since we don't have a flag field or padding in drop notification struct,
let's increment the version and add a `__u32` field to hold flags.
For now, we use 1 bit to indicate IPv6 and 1 bit to indicate whether the
packet is being dropped from a L3 Device. The other 30 bits are kept as
padding the preserve the struct alignment.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96 smagnani96 force-pushed the pr/bpf-l3packet-tracing branch from e1bb0d2 to 8880c82 Compare January 31, 2025 15:02
@smagnani96
Copy link
Contributor Author

/test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. dont-merge/preview-only Only for preview or testing, don't merge it. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bpf: tracing: hubble: erroneously parse of L2-less packets in trace/drop notifications
1 participant