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

Active connection tracking in ebpf #32584

Merged

Conversation

AwesomePatrol
Copy link
Contributor

@AwesomePatrol AwesomePatrol commented May 16, 2024

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

@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 May 16, 2024
@AwesomePatrol AwesomePatrol force-pushed the active-connection-tracking-in-ebpf branch from e8f0244 to beecf87 Compare May 16, 2024 14:13
@maintainer-s-little-helper

This comment was marked as resolved.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 16, 2024
@AwesomePatrol AwesomePatrol force-pushed the active-connection-tracking-in-ebpf branch from c7a9677 to 58f239e Compare May 16, 2024 17:02
@maintainer-s-little-helper

This comment was marked as resolved.

@AwesomePatrol AwesomePatrol force-pushed the active-connection-tracking-in-ebpf branch from 58f239e to 6bd2077 Compare May 17, 2024 07:03
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 17, 2024
@AwesomePatrol AwesomePatrol force-pushed the active-connection-tracking-in-ebpf branch from 6bd2077 to 66e1d80 Compare May 17, 2024 07:12
@AwesomePatrol

This comment was marked as outdated.

@AwesomePatrol AwesomePatrol marked this pull request as ready for review May 17, 2024 08:08
@AwesomePatrol AwesomePatrol requested review from a team as code owners May 17, 2024 08:08
@AwesomePatrol

This comment was marked as outdated.

Copy link
Contributor

@lmb lmb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@tommyp1ckles tommyp1ckles requested review from julianwiedmann and removed request for danehans May 20, 2024 04:20
@tommyp1ckles
Copy link
Contributor

@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 AwesomePatrol force-pushed the active-connection-tracking-in-ebpf branch from 76f4e16 to 27af564 Compare May 21, 2024 16:25
@AwesomePatrol
Copy link
Contributor Author

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
Copy link
Contributor

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

pkg/maps/lrs/lrs.go Outdated Show resolved Hide resolved
pkg/option/config.go Outdated Show resolved Hide resolved
@julianwiedmann
Copy link
Member

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! 🙏

@julianwiedmann julianwiedmann marked this pull request as draft May 22, 2024 10:32
@pchaigno pchaigno added the sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. label May 23, 2024
@pchaigno
Copy link
Member

pchaigno commented Jun 3, 2024

/test

@pchaigno pchaigno enabled auto-merge June 3, 2024 10:07
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 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.

bpf/lib/conntrack.h Outdated Show resolved Hide resolved
auto-merge was automatically disabled June 5, 2024 11:08

Head branch was pushed to by a user without write access

@AwesomePatrol AwesomePatrol force-pushed the active-connection-tracking-in-ebpf branch 2 times, most recently from 8aa2cc8 to 3cc35fe Compare June 5, 2024 11:09
bpf/lib/conntrack.h Outdated Show resolved Hide resolved
@julianwiedmann
Copy link
Member

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

@AwesomePatrol AwesomePatrol force-pushed the active-connection-tracking-in-ebpf branch from 3cc35fe to 3bc306b Compare June 7, 2024 11:03
@AwesomePatrol AwesomePatrol marked this pull request as draft June 7, 2024 11:42
@AwesomePatrol AwesomePatrol force-pushed the active-connection-tracking-in-ebpf branch from 3bc306b to b1f5513 Compare June 7, 2024 13:32
@AwesomePatrol

This comment was marked as outdated.

Add new map - LB_ACT_MAP - behind ENABLE_ACTIVE_CONNECTION_TRACKING flag
with counters of opened and closed connections. Behavior of eBPF remains
completely unchanged when ENABLE_ACTIVE_CONNECTION_TRACKING flag is not
set.

When an entry, to conntrack table is created, an entry in
LB_ACT_MAP.opened is incremented by one. When connection is closed, the
related LB_ACT_MAP.closed is incremented by one. This works only for
traffic originating from the local pods.

LB_ACT_MAP is keyed by svc_id (also known as rev_nat_index) and zone,
which is obtained from backend entry. Zone field in backend is populated
only when EndpointSlice contains a reference to zone in FixedZoneMapping
(so it is possible to convert between uint8 ID and string).

Signed-off-by: Aleksander Mistewicz <amistewicz@google.com>
@AwesomePatrol AwesomePatrol force-pushed the active-connection-tracking-in-ebpf branch from b1f5513 to 999920e Compare June 10, 2024 07:42
@AwesomePatrol
Copy link
Contributor Author

/test

@AwesomePatrol AwesomePatrol marked this pull request as ready for review June 10, 2024 08:54
@julianwiedmann julianwiedmann self-requested a review June 10, 2024 09:12
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, the conntrack integration feels much simpler now. Thank you!

@pchaigno pchaigno enabled auto-merge June 10, 2024 10:01
@pchaigno pchaigno added this pull request to the merge queue Jun 10, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 10, 2024
Merged via the queue into cilium:main with commit 03afbcc Jun 10, 2024
63 of 64 checks passed
@julianwiedmann julianwiedmann added the area/loadbalancing Impacts load-balancing and Kubernetes service implementations label Jun 10, 2024
@joestringer
Copy link
Member

Relating to release-note/major, I'm trying to understand the impact on users. Currently the release note is the following:

Add cilium_lb_act BPF map with counters of opened and closed connections

This sounds to me like a low-level detail that operators might consult during debugging, rather than unlocking for instance better load balancing across the cluster.

What's the user-facing impact of this change? Could it be broader and we can describe it better in the release note, or is this pretty accurate, in which case do we think this has a major impact on users?

@pchaigno
Copy link
Member

I'm the one who added this release note. I added it because my understanding is that this is a new feature being exposed to advanced users (who would poll the new map). In the past, I've used release-note/major for all new features, including things like internalTrafficPolicy=Local that may not be considered major in other contexts.

@AwesomePatrol
Copy link
Contributor Author

This sounds to me like a low-level detail that operators might consult during debugging

I agree. I work on exposing these as prometheus metrics, so I believe this PR is an implementation detail and not really a new feature. Hopefully I will publish new PR this week. It could be marked major then (and this one minor).

What's the user-facing impact of this change?

There should be no impact. Even with flags set (both fixed-zone-mapping and enable-active-connection-tracking) the only change is that you can query a new map with some data in it.

this is a new feature being exposed to advanced users (who would poll the new map)

I don't think that map polling is very user friendly, but are there many examples of usage of cilium maps outside of cilium binary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/loadbalancing Impacts load-balancing and Kubernetes service implementations feature/conntrack ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/major This PR introduces major new functionality to Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants