Active connection tracking in ebpf

[AwesomePatrol]

As described in cilium/design-cfps#31, this is part of #31752

Example configuration (in cilium-config) for KIND cluster:

  fixed-zone-mapping: zone-a=123,zone-b=129
  enable-active-connection-tracking: "true"

where topology labels were set on the nodes:

kubectl label node kind-control-plane topology.kubernetes.io/zone=zone-a
kubectl label node kind-worker topology.kubernetes.io/zone=zone-b

Zone mapping can be verified with cilium map get cilium_lb4_backends_v3:

Key   Value                        State   Error
16    ANY://10.244.1.74[zone-b]    sync
17    ANY://10.244.1.74[zone-b]    sync
18    ANY://10.244.1.202[zone-b]   sync
19    ANY://10.244.1.202[zone-b]   sync
13    ANY://192.168.11.2[zone-b]   sync
14    ANY://10.244.0.178[zone-a]   sync
15    ANY://10.244.0.178[zone-a]   sync

LRS accounting can be verified with cilium map get cilium_lb_act:

Key                       Value     State   Error
10.96.138.80:80[zone-b]   +72 -72
10.96.138.80:80[zone-a]   +28 -28

Add cilium_lb_act BPF map with counters of opened and closed connections

[lmb]

Loader changes are fine. As a rando reviewer, it's not at all clear to me what lrs stands for however.

[tommyp1ckles]

@julianwiedmann would you be able to review this for @cilium/sig-datapath, I'm assigned sig-cli but my review would also count towards datapath - and I'm not familiar enough with this code to provide a thorough review.

[AwesomePatrol]

Loader changes are fine. As a rando reviewer, it's not at all clear to me what lrs stands for however.

LRS stands for Load Reporting Service from Envoy. Please see: https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/load_stats/v3/lrs.proto. I am bad at naming stuff, so I am open to suggestions

[tommyp1ckles]

@AwesomePatrol Does the TODO list in the second commits message represent work that's still to be done?

[julianwiedmann]

Based on the TODOs and the missing description in the second patch, this looks like it still needs some more work. Flipping to draft for now to get it off my queue, please flip back when you're ready! 🙏

[pchaigno]

I removed TODOs from the commit message as to not confuse any other reviewers. What descriptions are missing?

I'm guessing Julian means the commit descriptions. They should describe what the commit is doing, but more importantly why and any subtleties to help reviewers understand what is going on. See examples in the bpf/ commit history. Of course, depending on the changes in the commit, that can be very short (if what and why are obvious with no subtleties). With hundreds of LoC, I'm going to venture your changes aren't trivial enough to have empty commits 😁

[joamaki]

Approving as I'm going on vacation and mostly had nits about unused methods.

[pchaigno]

/test

[julianwiedmann]

👋 thank you! I'd want to address the integration into the BPF codepath below.

Besides that, could you please squash the last two patches in the series into the initial series? I don't think we need to merge code that immediately gets refactored again.

[julianwiedmann]

Picking on this part in particular, as it's clearly misplaced. ct_create_fill_entry() is meant to fill the CT entry (from the info in the ct_state representation), but here we don't even touch the CT entry. So at best this part should just live in ct_create{4,6}() instead.

But as more general question - why are these changes intertwined with the actual CT lookup? If we call the feature Active connection counting instead, would it still fit into conntrack.h? We're not actually tracking any connection state, after all ...

My expectation would be to have this logic live here, where it would be included into the BPF program only once. What's missing to make that happen? Do we just need to pass back a few bits of connection-internal state to detect reopened / closed connections?

[AwesomePatrol]

it's clearly misplaced. ct_create_fill_entry() is meant to fill the CT entry

Agreed. I moved all calls to a single function, __ct_lookup.

If we call the feature Active connection counting instead, would it still fit into conntrack.h?

Yes, the decision to place it in conntrack.h is a technical requirement of what information is available in each function. Please, feel free to suggest other places that are suitable for this, if there are any.

My expectation would be to have this logic live here

This handles counting new connections only, where do you want to handle closing connections?

[julianwiedmann]

what information is available in each function. Please, feel free to suggest other places that are suitable for this, if there are any.

Let's use the ct_state then to provide this detailed level of information to callers :).

This handles counting new connections only, where do you want to handle closing connections?

How about the following?

ct_result = ct_lazy_lookup4(...);
if (ct_result == CT_NEW || ct_state->reopened)
	// account for opened connection
else if (ct_state->closing)
	// account for closed connection

[julianwiedmann]

I believe this would happen too early in the code flow now. For a new connection, we haven't selected a backend yet.

[julianwiedmann]

Note that you'll probably need a rebase to pick up the changes from #32653.