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

ipsec: fixes for key rotation #27319

Merged
merged 2 commits into from Aug 17, 2023
Merged

ipsec: fixes for key rotation #27319

merged 2 commits into from Aug 17, 2023

Conversation

jrfastab
Copy link
Contributor

@jrfastab jrfastab commented Aug 7, 2023

Fixups to ensure keys are correctly in place before deleting the old key.

  1. Instantiate all xfrm rules when new key shows up instead of waiting for nodeUpdate events through validator to happen. Its not always the case on larger clusters that the nodeUpdate logic will trigger before the key retention logic triggers to delete old keys. In this case we may be missing xfrm rules for new keys for some nodes.

  2. Ensure rules are in place before key is updated. Current logic updates datapath key before updating the BPF maps creating smallish window where pkts can be marked for the new key but the BPF maps and datapath wont have the mapping to do this yet resulting in dropped pkts.

Both of these intend to address an increased policy block counter in recent versions.

Fix bug that could cause packet drops of type XfrmOutPolBlock while rotating the IPsec key.

@jrfastab jrfastab requested review from a team as code owners August 7, 2023 14:51
@jrfastab jrfastab requested a review from brb August 7, 2023 14:51
@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 Aug 7, 2023
@jrfastab jrfastab added needs-backport/1.11 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 Aug 7, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.1 Aug 7, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.13.6 Aug 7, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.12.13 Aug 7, 2023
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

I briefly looked over the changes and nothing alarming stood out. I'd prefer someone with more familiarity with this part of the codebase to take a closer look.

pkg/datapath/linux/node.go Show resolved Hide resolved
@joestringer joestringer added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Aug 8, 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 Aug 8, 2023
@joestringer
Copy link
Member

/test

@nebril nebril added this to Needs backport from main in 1.13.7 Aug 10, 2023
@nebril nebril removed this from Needs backport from main in 1.13.6 Aug 10, 2023
@nebril nebril added this to Needs backport from main in 1.14.2 Aug 10, 2023
@nebril nebril removed this from Needs backport from main in 1.14.1 Aug 10, 2023
@jrfastab
Copy link
Contributor Author

/test

Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Thanks!

pkg/datapath/linux/ipsec/ipsec_linux.go Show resolved Hide resolved
@asauber asauber added this to Needs backport from main in 1.12.14 Aug 13, 2023
@rolinh rolinh removed the request for review from lambdanis August 17, 2023 07:19
@rolinh rolinh merged commit 4f3bc9f into main Aug 17, 2023
196 checks passed
@rolinh rolinh deleted the pr/jrfastab/rollkeys branch August 17, 2023 07:19
@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 Aug 17, 2023
@jrfastab jrfastab restored the pr/jrfastab/rollkeys branch August 17, 2023 22:42
@margamanterola margamanterola added backport-pending/1.11 backport/author The backport will be carried out by the author of the PR. and removed needs-backport/1.11 labels Aug 18, 2023
@margamanterola
Copy link
Member

Marking as backport/author to avoid this getting in the backporter bucket, as it's expected to have conflicts. @jschwinger233 will take care of the backports.

@jrfastab jrfastab added backport-done/1.11 The backport for Cilium 1.11.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.13 The backport for Cilium 1.13.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.11 needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Aug 21, 2023
@pchaigno pchaigno deleted the pr/jrfastab/rollkeys branch August 25, 2023 09:08
@pchaigno
Copy link
Member

I've added a release note.

@joestringer joestringer moved this from Needs backport from main to Backport done to v1.12 in 1.12.14 Aug 25, 2023
@joestringer joestringer moved this from Needs backport from main to Backport done to v1.13 in 1.13.7 Aug 25, 2023
@joestringer joestringer moved this from Needs backport from main to Backport done to v1.14 in 1.14.2 Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/author The backport will be carried out by the author of the PR. 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. 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.14
Backport done to v1.12
1.13.7
Backport done to v1.13
1.14.2
Backport done to v1.14
Development

Successfully merging this pull request may close these issues.

None yet

9 participants