Skip to content

ipsec: guard vxlan-in-esp policies by routing mode correctly. #35549

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
Merged
merged 1 commit into from
Oct 25, 2024
Merged

ipsec: guard vxlan-in-esp policies by routing mode correctly. #35549

merged 1 commit into from
Oct 25, 2024

Conversation

ldelossa
Copy link
Contributor

See commit message for details.

Fix a potential issue where VXLAN-in-ESP policies are installed erroneously when EGW is enabled.

@ldelossa ldelossa added area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. 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. labels Oct 25, 2024
@ldelossa ldelossa requested review from a team as code owners October 25, 2024 14:41
@ldelossa ldelossa requested a review from pchaigno October 25, 2024 14:41
pchaigno
pchaigno reviewed Oct 25, 2024
View reviewed changes
@pchaigno
Copy link
Member

Doesn't this need backporting to v1.16?

@ldelossa
ipsec: guard vxlan-in-esp policies by routing mode correctly.
4bacb14 
Use the appropriate configuration option when deciding to install
VXLAN-in-ESP related policies and states.

Previous to this commit this was decided by checking whether a tunnel device
was configured.

This is too general.
EgressGateway configures a tunnel device depsite the cluster being in
native routing mode.

It would be an error to install the VXLAN-in-ESP related policies and
states in this scenario.

Fix this by wiring the RoutingMode configuration into `LocalNodeConfiguration`
and referring to this in ipsec.go

Currently, this cannot actually case a bug in Cilium.
This is because of the subsequent check on
`EnableIPSecEncryptedOverlay` configuration flag.
This cannot be enabled without tunnel mode being enabled.

However, in future commits this restriction will be going away.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
@ldelossa ldelossa force-pushed the ldelossa/vxlan-in-esp-policies-fix branch from 1677c31 to 4bacb14 Compare October 25, 2024 15:02
@ldelossa ldelossa added release-note/misc This PR makes changes that have no direct user impact. and removed release-note/bug This PR fixes an issue in a previous release of Cilium. labels Oct 25, 2024
@ldelossa
Copy link
Contributor Author

@pchaigno

No backport necessary.
The code that's being fixed here was introduced recently in:

commit 7cb25827d61d14543dcbbf3639caa6a322916135
Author: Louis DeLosSantos <louis.delos@isovalent.com>
Date:   Thu Oct 3 14:47:28 2024 -0400
upstream/pr/v1.14-backport-2024-09-13-02-13                                                              
    ipsec: utilize declarative XFRM policy/state creation
    
    This commit updates the methods which create XFRM states and policies to
    use the declarative structure introduced in the previous commit.
    
    Each policy is now well-defined before passing the declarative structure
    down to the methods which do the heavy lifting of policy creation.
    
    The heavy-lifting structures no longer impose a constraint on how/what
    arguments are provided and instead refer to the declarative
    IPSecParameter structure for the information they need.
    
    This allows the developer to reason about the XFRM state and policy they
    are creating at a higher level, and in one place.
    
    Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>

No release has occurred since that.

@pchaigno pchaigno enabled auto-merge October 25, 2024 15:11
@ldelossa
Copy link
Contributor Author

/test

@pchaigno pchaigno added this pull request to the merge queue Oct 25, 2024
@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 25, 2024
Merged via the queue into cilium:main with commit 4f0f10e Oct 25, 2024
64 checks passed
@ldelossa ldelossa deleted the ldelossa/vxlan-in-esp-policies-fix branch October 26, 2024 02:48
julianwiedmann
julianwiedmann reviewed Jan 9, 2025
View reviewed changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julianwiedmann julianwiedmann julianwiedmann left review comments

@pchaigno pchaigno pchaigno approved these changes

Assignees
No one assigned
Labels
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. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ldelossa @pchaigno @julianwiedmann