Skip to content

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Nov 6, 2024

See commit descriptions. This bug made it only into a pre-release for v1.17.

Fixes: #35210.

Fix bug that would break all pod-to-pod connectivity when using the per-tunnel IPsec key system.

@pchaigno pchaigno added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. feature/ipsec Relates to Cilium's IPsec feature labels Nov 6, 2024
@pchaigno pchaigno force-pushed the pr/pchaigno/fix-ipsec-regression-bootid branch 3 times, most recently from fb872a5 to 3d802bb Compare November 6, 2024 13:26
Global keys have been deprecated, so in the EKS workflow, we are getting
a warning because we don't use per-tunnel keys yet:

    level=warn msg="global IPsec keys are deprecated and will be removed in v1.17. Use per-tunnel keys instead by adding a '+' sign after the SPI (15+ in your case)."

This commit switches the secret to use per-tunnel IPsec keys.

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
Until this commit, we would cover the transitions between single-key and
per-tunnel-key systems for IPsec. In v1.17, we will remove support for
single-key. There is thus no need to cover all of those transitions
anymore. All users are now expected to be (and remain) on
per-tunnel-key.

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
The GitHub action for IPsec key rotations currently assumes the cluster
has two nodes and that both IPv4 and IPv6 are enabled. These assumptions
don't hold for the EKS conformance test where IPv6 is disabled and three
nodes are created.

This commit therefore adds two new arguments to the GitHub action, to
support IPv6 being enabled/disabled and a variable number of nodes.

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
In the key derivation logic, commit 7cb2582 ("ipsec: utilize
declarative XFRM policy/state creation") removed the inversion of local
and remote node IP addresses based on XFRM direction because it's not
needed anymore. We however still need to invert the local and remote
boot IDs based on direction.

The consequence was that key derivation in per-tunnel key mode was
always broken. Kind would work because all nodes have the same boot ID
on Kind. This would result in packet drops of type XfrmInStateProtoError
because the computed keys end up being different on both ends of the
tunnels.

Fixes: 7cb2582 ("ipsec: utilize declarative XFRM policy/state creation")
Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
@pchaigno pchaigno force-pushed the pr/pchaigno/fix-ipsec-regression-bootid branch from 3d802bb to 6a9766b Compare November 6, 2024 13:27
@pchaigno pchaigno requested review from brlbil and smagnani96 November 6, 2024 14:28
@pchaigno pchaigno marked this pull request as ready for review November 6, 2024 14:28
@pchaigno pchaigno requested review from a team as code owners November 6, 2024 14:28
@pchaigno pchaigno enabled auto-merge November 6, 2024 14:28
@tommyp1ckles
Copy link
Contributor

/ci-gke

@tommyp1ckles
Copy link
Contributor

/ci-aks

Copy link
Contributor

@smagnani96 smagnani96 left a comment

Choose a reason for hiding this comment

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

Changes looks good, took me a bit to figure out CI -- my bad (I left a comment explaining where I was stuck).

@pchaigno pchaigno added this pull request to the merge queue Nov 7, 2024
Merged via the queue into main with commit 208550d Nov 7, 2024
301 checks passed
@pchaigno pchaigno deleted the pr/pchaigno/fix-ipsec-regression-bootid branch November 7, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/ipsec Relates to Cilium's IPsec feature kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: Conformance GKE fails on Wait for Cilium to be ready gh: ipsec: support arbitrary number of nodes in ipsec-key-rotate action
4 participants