Skip to content

Conversation

jschwinger233
Copy link
Member

@jschwinger233 jschwinger233 commented May 10, 2024

Commit messages elaborate details. Although this is to fix an upgrade issue from 1.13, it doesn't harm main.

If the cilium is upgraded from an old version, it's possible a latest version cilium cluster has the old xfrm states left over. In this case, this PR performs unnecessary xfrm churn, but won't affect any traffic.

If the cilium is installed from scratch, it indeed doesn't need this fix at all. In this situation, the SafelyDeleteXfrmState() doesn't even cause xfrm churn because it can't find the old style xfrm state to delete.

This can be removed along with https://github.com/cilium/cilium/blob/v1.15.4/pkg/datapath/linux/ipsec/ipsec_linux.go#L351.

@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 10, 2024
@jschwinger233 jschwinger233 force-pushed the gray/non-atomic-delete-ipsec-sa branch 2 times, most recently from 4a47d10 to 6ba68e9 Compare May 10, 2024 04:50
@jschwinger233
Copy link
Member Author

/test

No functional change, just to make further revision easier.

Signed-off-by: gray <gray.liang@isovalent.com>
@jschwinger233 jschwinger233 force-pushed the gray/non-atomic-delete-ipsec-sa branch from 6ba68e9 to 30f969e Compare May 10, 2024 11:12
@jschwinger233 jschwinger233 added the release-note/bug This PR fixes an issue in a previous release of Cilium. label May 10, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 10, 2024
@jschwinger233 jschwinger233 added area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. area/agent Cilium agent related. feature/ipsec Relates to Cilium's IPsec feature labels May 10, 2024
@jschwinger233
Copy link
Member Author

/test

@jschwinger233 jschwinger233 added upgrade-impact This PR has potential upgrade or downgrade impact. needs-backport/1.13 labels May 10, 2024
@julianwiedmann julianwiedmann added the kind/bug This is a bug in the Cilium logic. label May 10, 2024
@jschwinger233 jschwinger233 marked this pull request as ready for review May 13, 2024 03:22
@jschwinger233 jschwinger233 requested a review from a team as a code owner May 13, 2024 03:22
@jschwinger233 jschwinger233 requested a review from pchaigno May 13, 2024 03:22
@jschwinger233 jschwinger233 force-pushed the gray/non-atomic-delete-ipsec-sa branch 2 times, most recently from 1eb9e26 to 71f722f Compare May 14, 2024 04:11
@jschwinger233 jschwinger233 requested a review from rgo3 May 14, 2024 04:17
@jschwinger233
Copy link
Member Author

/test

@jschwinger233 jschwinger233 force-pushed the gray/non-atomic-delete-ipsec-sa branch from 71f722f to a7903f4 Compare May 15, 2024 09:50
@jschwinger233
Copy link
Member Author

/test

@julianwiedmann julianwiedmann self-requested a review May 16, 2024 08:19
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

😭

Thanks Gray! Neat pull request!

@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 May 16, 2024
@jschwinger233 jschwinger233 removed the upgrade-impact This PR has potential upgrade or downgrade impact. label May 17, 2024
@julianwiedmann julianwiedmann added this pull request to the merge queue May 17, 2024
Merged via the queue into cilium:main with commit 9a68ec2 May 17, 2024
@jschwinger233 jschwinger233 deleted the gray/non-atomic-delete-ipsec-sa branch May 17, 2024 07:47
@YutaroHayakawa YutaroHayakawa mentioned this pull request May 23, 2024
15 tasks
@YutaroHayakawa YutaroHayakawa added backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. and removed needs-backport/1.15 labels May 23, 2024
@github-actions github-actions bot added backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. and removed backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. labels May 25, 2024
@julianwiedmann julianwiedmann added backport-pending/1.13 backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.13 labels May 27, 2024
@github-actions github-actions bot added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. backport-pending/1.13 labels May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/agent Cilium agent related. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. 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. feature/ipsec Relates to Cilium's IPsec feature kind/bug This is a bug in the Cilium logic. 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
Status: Needs backport from main
Status: Needs backport from main
Status: Released
Status: Released
Status: Released
Development

Successfully merging this pull request may close these issues.

5 participants