Skip to content

ipsec: ensure all trace events are discarded when mon. agg. is enabled#29616

Merged
pchaigno merged 2 commits into
cilium:mainfrom
learnitall:pr/learnitall/fix-encrypt-events
Dec 19, 2023
Merged

ipsec: ensure all trace events are discarded when mon. agg. is enabled#29616
pchaigno merged 2 commits into
cilium:mainfrom
learnitall:pr/learnitall/fix-encrypt-events

Conversation

@learnitall

@learnitall learnitall commented Dec 4, 2023

Copy link
Copy Markdown
Contributor

This commit modifies calls to send_trace_notify in the datapath to drop trace events related to encrypted packets when monitor aggregation is enabled. More specifically, this commit ensures that whenever send_trace_notify is called with a trace_reason of TRACE_REASON_ENCRYPTED, the monitor argument is set to zero. A Coccinelle script is provided in this commit to add a build-time check for this requirement moving forward.

This change helps to reduce the overall CPU usage of Cilium Agents when IPSec encryption is enabled, by reducing the number of trace events emitted by the datapath. Normally monitor aggregation can be used in order to reduce the number of trace events, however IPSec-related trace events are not able to be aggregated since they lack the necessary connection tracking information. See the function emit_trace_notify in bpf/lib/trace.h for more information.

This endeavor was first started in cilium/cilium#27168, however, in said PR I missed a couple of spots where this type of change could also be applied. This PR acts as a follow-up to ensure that all places where TRACE_REASON_ENCRYPTED appears in a call to send_trace_notify, the argument monitor is set to zero.

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!
Fix and prevent future bugs limiting pod-to-pod network performance under high load when tunneling and IPSec are both enabled.
@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 Dec 4, 2023
@learnitall learnitall force-pushed the pr/learnitall/fix-encrypt-events branch 2 times, most recently from 7b5c4bc to 01a2645 Compare December 5, 2023 00:06
@learnitall learnitall changed the title bpf: Do not trace encrypted packets when monitor agg is enabled ipsec: ensure all trace events are discarded when mon. agg. is enabled Dec 5, 2023
@learnitall learnitall force-pushed the pr/learnitall/fix-encrypt-events branch from 01a2645 to 2e06a1c Compare December 6, 2023 17:02
@learnitall learnitall added kind/performance There is a performance impact of this. area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. release-note/bug This PR fixes an issue in a previous release of Cilium. area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. needs-backport/1.12 labels Dec 6, 2023
@maintainer-s-little-helper maintainer-s-little-helper Bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Dec 6, 2023
@learnitall learnitall marked this pull request as ready for review December 6, 2023 17:33
@learnitall learnitall requested a review from a team as a code owner December 6, 2023 17:33
@learnitall learnitall requested a review from ldelossa December 6, 2023 17:33
@julianwiedmann julianwiedmann requested review from a team and pchaigno and removed request for a team December 6, 2023 17:40

@pchaigno pchaigno left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice fix, perfect commit description, and impressive work on Coccinelle! 🚀

@julianwiedmann

Copy link
Copy Markdown
Member

/test

@learnitall learnitall requested review from a team as code owners December 9, 2023 00:13
Comment thread bpf/bpf_network.c
@learnitall

Copy link
Copy Markdown
Contributor Author

/test

@learnitall

Copy link
Copy Markdown
Contributor Author

/test

@pchaigno pchaigno enabled auto-merge December 18, 2023 11:27
@pchaigno

Copy link
Copy Markdown
Member

@learnitall You'll need to rebase to catch https://github.com/cilium/cilium/pull/29958/commits and pass the CI.

@pchaigno pchaigno added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Dec 18, 2023
@julianwiedmann julianwiedmann added the feature/ipsec Relates to Cilium's IPsec feature label Dec 19, 2023

@ldelossa ldelossa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. feature/ipsec Relates to Cilium's IPsec feature kind/performance There is a performance impact of this. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.