Propagate context through TCP packets #1161

grcevski · 2024-09-11T19:38:09Z

This PR finishes the work required to propagate context with trace context injection through TCP packets. The feature is disabled by default. I'm proposing the PR to ensure we can pass all integration tests without this enabled.

To complete the work, I plan to follow-up with another PR that will do the following:

Add option to disable black-box context propagation
Add tests without black-box context propagation and TCP packet propagation.

Relates to #871

grcevski · 2024-09-11T19:39:57Z

bpf/http_types.h

@@ -175,12 +175,13 @@ const u8 ip4ip6_prefix[] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0xff, 0xff};

 #ifdef BPF_DEBUG
 static __always_inline void dbg_print_http_connection_info(connection_info_t *info) {
-    bpf_dbg_printk("[http] s_h = %llx, s_l = %llx, d_h = %llx, d_l = %llx, s_port=%d, d_port=%d",
+    bpf_dbg_printk("[conn] s_h = %llx, s_l = %llx, s_port=%d",


I split this so that we can use it on older kernels where bpf_printk allowed for at most 3 arguments.

grcevski · 2024-09-11T19:42:13Z

bpf/k_tracer.c

@@ -186,6 +186,33 @@ int BPF_KPROBE(kprobe_tcp_connect, struct sock *sk) {

    bpf_dbg_printk("=== tcp connect %llx ===", id);

+    tp_info_pid_t *tp_p = tp_buf();


Connect runs before the SYN packet is sent. We use this opportunity to setup a trace context information for the connection. We'll later query the trace information in tc_egress, and serialize it on the TCP packet.

grcevski · 2024-09-11T19:43:23Z

bpf/k_tracer.c

-    if (tcp_syn(&tcp) && !tcp_ack(&tcp)) {
-        bpf_skb_load_bytes(skb, tcp.hdr_len, &buf, 4);
+    if (skb->len - tcp.hdr_len < sizeof(tp_info_pid_t)) {
+        bpf_printk("SYN packet without tp info");


these prints will have to be removed, but keeping them for now since this is disabled. bpf_dbg_printk doesnt' work because we need to fix it to not use bpf_get_pid_tid, but bpf_get_task info

grcevski · 2024-09-11T19:44:43Z

bpf/k_tracer.c

+    make_tp_string(tp_buf, &tp.tp);
+    bpf_printk("tp: %s", tp_buf);
+
+    bpf_map_update_elem(&incoming_trace_map, &conn, &tp, BPF_ANY);


Once we receive a traceID over the wire (TCP packet) we store it for later to be used by the trace code.

codecov-commenter · 2024-09-11T19:46:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.35%. Comparing base (77cba46) to head (06ddc8d).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1161      +/-   ##
==========================================
- Coverage   81.07%   78.35%   -2.73%     
==========================================
  Files         136      136              
  Lines       11416    11416              
==========================================
- Hits         9256     8945     -311     
- Misses       1636     1920     +284     
- Partials      524      551      +27

Flag	Coverage Δ
integration-test	`?`
k8s-integration-test	`58.81% <ø> (-0.12%)`	⬇️
oats-test	`35.77% <ø> (ø)`
unittests	`53.23% <ø> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

grcevski · 2024-09-11T19:47:33Z

bpf/trace_common.h

@@ -196,10 +204,16 @@ static __always_inline void get_or_create_trace_info(http_connection_metadata_t

    if (meta) {
        if (meta->type == EVENT_HTTP_CLIENT) {
+            tp_info_pid_t *in_tp = bpf_map_lookup_elem(&outgoing_trace_map, conn);


Before this change the client code only looked for a server trace and if it doesn't find it it will generate the trace information later. Now we look if the TCP egress has setup trace info for us, if we find it we mark as having trace info, we must not regenerate it. If we have our own TCP egress info, but there's server incoming request that wrapped this client call, then we'll overwrite this info.

I'd actually put that in a comment

grcevski · 2024-09-11T19:48:00Z

bpf/trace_common.h

-            tp_info_pid_t *existing_tp = trace_info_for_connection(conn);
-
-            if (correlated_requests(tp_p, existing_tp)) {
+            tp_info_pid_t *existing_tp = bpf_map_lookup_elem(&incoming_trace_map, conn);


For server requests, we first look for TCP info and then we fall back to black-box info.

rafaelroquetto

This is great, awesome! Very clever!

My comments are very minor (pedantic) nits, feel free to ignore.

rafaelroquetto · 2024-09-11T22:06:39Z

bpf/k_tracer.c

@@ -232,7 +259,7 @@ int BPF_KRETPROBE(kretprobe_sys_connect, int fd)
        sort_connection_info(&info.p_conn.conn);
        info.p_conn.pid = pid_from_pid_tgid(id);
        info.orig_dport = orig_dport;
-
+        


nit: trailing whitespace

rafaelroquetto · 2024-09-11T22:32:43Z

bpf/k_tracer.c


-    if (tcp_syn(&tcp) && !tcp_ack(&tcp)) {
-        bpf_skb_load_bytes(skb, tcp.hdr_len, &buf, 4);
+    if (skb->len - tcp.hdr_len < sizeof(tp_info_pid_t)) {


I was looking into this, and stumbled upon this check in read_sk_buff()

if ((skb->len - tcp->hdr_len) < 0) { // less than 0 is a packet we can't parse

both len and hdr_len are unsigned, so I think, to complement this check, we should change read_sk_buff() to

if ((skb->len - tcp->hdr_len) > skb->len) { // underflow, hdr_len is > len, packet too big

or even better

if (tcp->hdr_len > skb->len) { // package too big

rafaelroquetto · 2024-09-11T23:03:00Z

bpf/k_tracer.c

+        bpf_printk("tot_len = %d, new_tot_len = %d", bpf_ntohs(tcp.tot_len), bpf_ntohs(new_tot_len));
+        bpf_printk("h_proto = %d, skb->len = %d", tcp.h_proto, skb->len);


Suggested change

bpf_printk("tot_len = %d, new_tot_len = %d", bpf_ntohs(tcp.tot_len), bpf_ntohs(new_tot_len));

bpf_printk("h_proto = %d, skb->len = %d", tcp.h_proto, skb->len);

bpf_printk("tot_len = %u, new_tot_len = %u", bpf_ntohs(tcp.tot_len), bpf_ntohs(new_tot_len));

bpf_printk("h_proto = %u, skb->len = %u", tcp.h_proto, skb->len);

rafaelroquetto · 2024-09-11T23:13:29Z

bpf/trace_common.h

@@ -196,10 +204,16 @@ static __always_inline void get_or_create_trace_info(http_connection_metadata_t

    if (meta) {
        if (meta->type == EVENT_HTTP_CLIENT) {
+            tp_info_pid_t *in_tp = bpf_map_lookup_elem(&outgoing_trace_map, conn);


I'd actually put that in a comment

rafaelroquetto · 2024-09-11T23:30:16Z

bpf/k_tracer.c

-        bpf_printk("SYN packed len = %d, offset = %d, hdr_len %d", skb->len, len, tcp.hdr_len);
+    tp_info_pid_t tp = {0};
+
+    bpf_skb_load_bytes(skb, tcp.hdr_len, &tp, sizeof(tp_info_pid_t));


I wonder if it may be a good idea to have tp_info_pid_t store some sort of magic number - it's highly unlikely, but if someone stores the exact same amount of bytes there, we will be fooled. Who knows how many other people are doing crazy things like this?

Something like [0xb3l4][tp_info_pid_t]

Good idea! I initially thought of this and said to myself, so if they do, we'll read something random and it's no worse than generating new ID. However, on a second thought, the value someone else puts in could be not random, but a constant and it will make all of our traceIDs be the same. I took your advice to put an ID on the PID (we don't use this field for TCP and it's later overwritten with -1). So I'll check the PID on ingress and if it doesn't match our magic value we'll not take in the packet.

rafaelroquetto · 2024-09-11T23:41:02Z

bpf/k_tracer.c

@@ -703,17 +741,25 @@ int app_egress(struct __sk_buff *skb) {
        return 0;
    }

+    if (!tcp_syn(&tcp) || tcp_ack(&tcp)) {


I'd add a comment such as:

// handle SYN only, ignore SYN+ACK packets

mariomac

Amazing! As Rafael suggests, I'd write your GitHub explanations as actual code comments.

mariomac

LGTM

grcevski added 2 commits September 11, 2024 13:01

Add client setup of tp

8fa34e1

Propagate trace context with TCP

3f15c50

grcevski requested review from mariomac, marctc and rafaelroquetto as code owners September 11, 2024 19:38

grcevski commented Sep 11, 2024

View reviewed changes

grcevski added 3 commits September 11, 2024 15:55

Fix bug

55df69d

Merge branch 'main' into propagate_tc_tcp

022c6a0

rebuild bpfs

0811044

rafaelroquetto approved these changes Sep 11, 2024

View reviewed changes

mariomac reviewed Sep 12, 2024

View reviewed changes

Address feedback comments

06ddc8d

mariomac approved these changes Sep 12, 2024

View reviewed changes

grcevski merged commit 6a94e5e into grafana:main Sep 12, 2024
9 checks passed

grcevski deleted the propagate_tc_tcp branch September 12, 2024 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate context through TCP packets #1161

Propagate context through TCP packets #1161

grcevski commented Sep 11, 2024 •

edited

Loading

grcevski Sep 11, 2024

grcevski Sep 11, 2024

grcevski Sep 11, 2024

grcevski Sep 11, 2024

codecov-commenter commented Sep 11, 2024 •

edited

Loading

grcevski Sep 11, 2024

rafaelroquetto Sep 11, 2024

grcevski Sep 11, 2024

rafaelroquetto left a comment

rafaelroquetto Sep 11, 2024

rafaelroquetto Sep 11, 2024

rafaelroquetto Sep 11, 2024

rafaelroquetto Sep 11, 2024

rafaelroquetto Sep 11, 2024

grcevski Sep 12, 2024

rafaelroquetto Sep 11, 2024

mariomac left a comment

mariomac left a comment

		@@ -186,6 +186,33 @@ int BPF_KPROBE(kprobe_tcp_connect, struct sock *sk) {

		bpf_dbg_printk("=== tcp connect %llx ===", id);

		tp_info_pid_t *tp_p = tp_buf();

		bpf_printk("tot_len = %d, new_tot_len = %d", bpf_ntohs(tcp.tot_len), bpf_ntohs(new_tot_len));
		bpf_printk("h_proto = %d, skb->len = %d", tcp.h_proto, skb->len);

Propagate context through TCP packets #1161

Propagate context through TCP packets #1161

Conversation

grcevski commented Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 11, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelroquetto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariomac left a comment

Choose a reason for hiding this comment

mariomac left a comment

Choose a reason for hiding this comment

grcevski commented Sep 11, 2024 •

edited

Loading

codecov-commenter commented Sep 11, 2024 •

edited

Loading