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.

Sign up for GitHub

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

Jump to bottom

ipsec: Improve logging #28642

Merged
merged 3 commits into from Oct 31, 2023
Merged

ipsec: Improve logging #28642

merged 3 commits into from Oct 31, 2023

Conversation

pchaigno
Copy link
Member

See commits for details.

Fix IPsec error logs to always have all information needed to identify the XFRM configuration on which the error happened.

@pchaigno pchaigno added 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 needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Oct 17, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.3 Oct 17, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.12.15 Oct 17, 2023
@pchaigno pchaigno marked this pull request as ready for review October 17, 2023 11:17
@pchaigno pchaigno requested review from a team as code owners October 17, 2023 11:17
@pchaigno pchaigno requested review from asauber and brb October 17, 2023 11:17
@jrajahalme jrajahalme added this to Needs backport from main in 1.12.16 Oct 17, 2023
@jrajahalme jrajahalme removed this from Needs backport from main in 1.12.15 Oct 17, 2023
@pchaigno
ipsec: Helper function to get direction from XFRM mark
fcbcef2 
This is useful for XFRM states which do not have a built-in direction
field. Instead, we encode the direction in the packet mark and can
therefore rely on that when logging. The same function can be used for
XFRM policies, even though they do have a built-in Dir field as well.

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
@pchaigno
ipsec: Log node ID and direction whenever possible
cc913d4 
The SPI and the source and destination IP addresses (or CIDRs for XFRM
policies) are not enough anymore to uniquely identify XFRM states and
policies. We additionally need the node ID.

This commit therefore ensures that we always log the five contextual
information bits whenever possible: SPI, source, destination, direction,
and node ID.

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
@pchaigno
ipsec: Log node ID in hex format for consistency
a98a6db 
The node ID is reported in hexadecimal format in the XFRM states and
policies, as well as in the node ID map dump. To make it easier to match
the node ID across different sources, we should also dump it in hex
format in the agent logs.

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
@pchaigno
Copy link
Member Author

/test

@jrajahalme jrajahalme added this to Needs backport from main in 1.14.4 Oct 18, 2023
@jrajahalme jrajahalme removed this from Needs backport from main in 1.14.3 Oct 18, 2023
asauber
asauber previously requested changes Oct 20, 2023
View reviewed changes
pkg/datapath/linux/ipsec/ipsec_linux.go Show resolved Hide resolved
pkg/datapath/linux/node.go Show resolved Hide resolved
@pchaigno pchaigno dismissed asauber’s stale review October 31, 2023 08:10

Several nitpicks, probably not worth pushing again and rerunning CI.

@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 Oct 31, 2023
@aditighag aditighag merged commit 4506c76 into cilium:main Oct 31, 2023
62 checks passed
@pchaigno pchaigno deleted the improve-ipsec-logging branch October 31, 2023 15:37
@derailed derailed added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Nov 1, 2023
@derailed derailed mentioned this pull request Nov 2, 2023
Closed
6 tasks
@jibi jibi mentioned this pull request Nov 7, 2023
Merged
15 tasks
@pchaigno pchaigno added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Nov 7, 2023
@pchaigno pchaigno mentioned this pull request Nov 7, 2023
Merged
7 tasks
@pchaigno pchaigno added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Nov 7, 2023
@pchaigno pchaigno mentioned this pull request Nov 7, 2023
Merged
6 tasks
@github-actions github-actions bot added 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.12 The backport for Cilium 1.12.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. backport-pending/1.12 labels Nov 8, 2023
@thorn3r thorn3r moved this from Needs backport from main to Backport done to v1.14 in 1.14.4 Nov 9, 2023
@thorn3r thorn3r mentioned this pull request Nov 9, 2023
Merged
@nathanjsweet nathanjsweet moved this from Needs backport from main to Backport done to v1.12 in 1.12.16 Nov 10, 2023
This was referenced Nov 12, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brb brb brb approved these changes

@asauber asauber asauber left review comments

Assignees
No one assigned
Labels
area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. 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. 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.
Projects
No open projects
1.12.16
Backport done to v1.12
1.14.4
Backport done to v1.14
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pchaigno @brb @asauber @derailed @aditighag