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: Split removeStaleXFRMOnce to fix deprioritization issue #26113

Merged
merged 1 commit into from
Jun 20, 2023
Merged

ipsec: Split removeStaleXFRMOnce to fix deprioritization issue #26113

merged 1 commit into from
Jun 20, 2023

Conversation

jschwinger233
Copy link
Member

@jschwinger233 jschwinger233 commented Jun 12, 2023 •
edited

We expect deprioritizeOldOutPolicy() to be executed for IPv4 and IPv6, but removeStaleXFRMOnce prevents the second call. If both IPv4 and IPv6 are enabled, v6 xfrm policy won't be deprioritized due to this issue.

This commit fixes it by spliting removeStaleXFRMOnce into removeStaleIPv4XFRMOnce and removeStaleIPv6XFRMOnce.

Fixes: 688dc9a

@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 Jun 12, 2023
@jschwinger233 jschwinger233 added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jun 12, 2023
@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 Jun 12, 2023
@jschwinger233 jschwinger233 added area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jun 12, 2023
@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 Jun 12, 2023
@jschwinger233 jschwinger233 marked this pull request as ready for review June 12, 2023 11:11
@jschwinger233 jschwinger233 requested a review from a team as a code owner June 12, 2023 11:11
@jschwinger233 jschwinger233 requested a review from jibi June 12, 2023 11:11
@jschwinger233
Copy link
Member Author

/test

pchaigno
pchaigno approved these changes Jun 19, 2023
View reviewed changes
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.

Nice catch!

Could you add in the commit description a reference to the commit that introduced the bug?

@jschwinger233
Copy link
Member Author

Thanks Paul, will make it more clear.

CI seems to have caught a bug cause by this change, let me investigate it.

@jschwinger233
ipsec: Split removeStaleXFRMOnce to fix deprioritization issue
ef08c74 
We expect deprioritizeOldOutPolicy() to be executed for IPv4 and IPv6,
but removeStaleXFRMOnce prevents the second call. If both IPv4 and IPv6
are enabled, v6 xfrm policy won't be deprioritized due to this issue.

This commit fixes it by spliting removeStaleXFRMOnce into
removeStaleIPv4XFRMOnce and removeStaleIPv6XFRMOnce.

Fixes: cilium@688dc9a

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
@pchaigno
Copy link
Member

CI seems to have caught a bug cause by this change, let me investigate it.

If it is the with bpf_host ginkgo IPsec test failing, do check open flake issues. We have a flake on that.

@pchaigno pchaigno added needs-backport/1.11 needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch feature/ipv6 Relates to IPv6 protocol support labels Jun 20, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.13.5 Jun 20, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.12.12 Jun 20, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.11.19 Jun 20, 2023
@jschwinger233
Copy link
Member Author

/test

@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 20, 2023
@borkmann borkmann merged commit f4f3656 into cilium:main Jun 20, 2023
60 of 61 checks passed
@nbusseneau nbusseneau mentioned this pull request Jun 22, 2023
Merged
7 tasks
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.11 in 1.11.19 Jun 22, 2023
@nbusseneau nbusseneau mentioned this pull request Jun 22, 2023
Merged
10 tasks
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.12 in 1.12.12 Jun 22, 2023
@nbusseneau nbusseneau mentioned this pull request Jun 22, 2023
Merged
19 tasks
@nbusseneau nbusseneau 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 Jun 22, 2023
@joestringer joestringer mentioned this pull request Jun 28, 2023
Merged
@tklauser tklauser added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Jun 29, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.13 in 1.13.5 Jun 29, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.11 to Backport done to v1.11 in 1.11.19 Jun 29, 2023
@tklauser tklauser added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed backport-pending/1.12 labels Jun 29, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.12 to Backport done to v1.12 in 1.12.12 Jun 29, 2023
@tklauser tklauser added backport-done/1.13 The backport for Cilium 1.13.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. labels Jun 29, 2023
@gentoo-root gentoo-root moved this from Backport pending to v1.13 to Backport done to v1.13 in 1.13.5 Jul 26, 2023
This was referenced Jul 26, 2023
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jibi jibi jibi approved these changes

@pchaigno pchaigno pchaigno approved these changes

Assignees
No one assigned
Labels
area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. 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. feature/ipv6 Relates to IPv6 protocol support 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.11.19
Backport done to v1.11
1.12.12
Backport done to v1.12
1.13.5
Backport done to v1.13
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jschwinger233 @pchaigno @jibi @tklauser @borkmann @nbusseneau