Skip to content

Conversation

ldelossa
Copy link
Contributor

@ldelossa ldelossa commented Oct 3, 2024

Refactor XFRM policy and state creation

@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 Oct 3, 2024
@ldelossa ldelossa added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Oct 3, 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 Oct 3, 2024
@ldelossa ldelossa 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 Oct 3, 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 Oct 3, 2024
@ldelossa ldelossa added area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Oct 3, 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 Oct 3, 2024
@ldelossa
Copy link
Contributor Author

ldelossa commented Oct 4, 2024

/test

@ldelossa ldelossa force-pushed the ldelossa/refactor-xfrm-policy-state-handling-oss branch 2 times, most recently from bdada1d to 96be97f Compare October 7, 2024 17:42
@ldelossa
Copy link
Contributor Author

ldelossa commented Oct 7, 2024

/test

@ldelossa ldelossa marked this pull request as ready for review October 7, 2024 19:52
@ldelossa ldelossa requested a review from a team as a code owner October 7, 2024 19:52
@ldelossa ldelossa requested a review from jschwinger233 October 7, 2024 19:52
@ldelossa ldelossa changed the title [WIP] refactor xfrm policy state handling oss [IPsec] refactor xfrm policy state handling oss Oct 7, 2024
Copy link
Member

@jschwinger233 jschwinger233 left a comment

Choose a reason for hiding this comment

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

High-level intent is clear, I left some nonblocking comments.

The commits in my opinion may not be split in a perfect way: the first commit introduces 3 functions representing 3 branches in enableIPsecIPvX(), but reviewer could have difficulty in confirming whether any logic has changed. Maybe a better way to organize these changes can be: introduce new functions commit by commit, remove the old branch replaced by new function in the same commit of new function.

New struct IPSecParameters looks good to me.

@ldelossa
Copy link
Contributor Author

ldelossa commented Oct 8, 2024

Hmm:

Maybe a better way to organize these changes can be: introduce new functions commit by commit, remove the old branch replaced by new function in the same commit of new function.

I don't immediately see a major concern here. The diff for the first commit only shows all green and I had thought, along with the commit message details, this made it clear that its add only. Was it not?

The reason they are split is because otherwise, you'd get a pretty nasty diff, since you'd be overwriting the same lines of the previous monolithic function by new ones. Adding the new functions first creates new lines, so that you can rip out the monolithic function in the next commit without too much craziness in the diff.

@ldelossa ldelossa force-pushed the ldelossa/refactor-xfrm-policy-state-handling-oss branch from 96be97f to 939863b Compare October 8, 2024 19:29
@ldelossa
Copy link
Contributor Author

ldelossa commented Oct 8, 2024

/test

@ldelossa ldelossa requested a review from jschwinger233 October 9, 2024 11:04
Copy link
Member

@jschwinger233 jschwinger233 left a comment

Choose a reason for hiding this comment

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

Thank you!

@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 9, 2024
@jschwinger233 jschwinger233 added the feature/ipsec Relates to Cilium's IPsec feature label Oct 9, 2024
@joestringer joestringer added release-note/misc This PR makes changes that have no direct user impact. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Oct 9, 2024
@joestringer
Copy link
Member

I've switched up the release note categorization as this is not intended to have user-facing impact. Thanks!

@joestringer
Copy link
Member

FWIW I do think that if we end up hitting a regression due to the refactoring, then it's going to be a lot more difficult to trace the changes if the commits introduce the brand new version then switch and remove the old version. I understand the nuances that in some cases it's not possible to retain a clear translation from old to new. I won't block the PR on this, but I did want to affirm one of the ideas where @jschwinger233 is coming from.

@joestringer joestringer added this pull request to the merge queue Oct 9, 2024
@maintainer-s-little-helper

This comment was marked as resolved.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 14, 2024
@ldelossa ldelossa force-pushed the ldelossa/refactor-xfrm-policy-state-handling-oss branch from 2a8a9b2 to d74a15e Compare October 14, 2024 10:29
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 14, 2024
@ldelossa ldelossa requested a review from pchaigno October 14, 2024 10:36
@ldelossa
Copy link
Contributor Author

/test

@ldelossa ldelossa force-pushed the ldelossa/refactor-xfrm-policy-state-handling-oss branch 3 times, most recently from f26b400 to 36e5eee Compare October 14, 2024 22:58
Introduce a new set of enableIPSec{4,6} related functions which split up
the duty of creating XFRM states and policies.

There are two logically separate code paths with regards to
creating/updating XFRM policies. One where "SubnetEncryption" is enabled
by IPAM and one where its disabled.

The introduced functions handle each case separately, reducing the
footprint of the original function which handles both cases.

A subsequent commit will cut over to using these new functions.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
Update both the enableIPsecIPv4 and 6 methods to utilize the helper
methods introduced in the previous commit.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
Prior to this commit the XFRM state and policy creation code would both
explicitly create XFRM FWD rules and implicitly (done during policy-in
creation).

This commit disambiguates where FWD policies are created. We opt to
always make them explicit. This will help with refactoring efforts in
a subsequent commit.

After this commit all FWD rules are created within the same function
that IN and OUT policies are created as well. We also add a new IPSecDir
constant to explicitly instruct the control plane to create a forward
rule.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
This commit introduces a new structure for declarative declaration of an
XFRM Policy and State.

This structure provides a consistent set of fields which are required
for creating an XFRM state and its associated policies.

Having a well defined structure allows us to declare exactly how an XFRM
policy should look at a higher level, and methods which are passed this
structure are in charge of creating the policy as the structure defines.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
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>
This function will be replaced by individual tests for IN, FWD, and OUT
policy creationg in the following commit.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
This commit introduces 3 new tests for specifically testing the creation
of XFRM states and policies in a particular direction, using the
declarative IPSecParamater struct.

Additionally, existing tests are updated to utilize the structure.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
Remove the use of IPSecDirBoth for creating both IN and OUT policies.

This option was only used in tests and had no functional use during
Cilium's runtime.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
@ldelossa ldelossa force-pushed the ldelossa/refactor-xfrm-policy-state-handling-oss branch from 36e5eee to 29e20ad Compare October 14, 2024 23:10
@ldelossa
Copy link
Contributor Author

/test

@ldelossa ldelossa removed the dont-merge/waiting-for-review Requires further review before merging. label Oct 15, 2024
@ldelossa
Copy link
Contributor Author

@pchaigno

Think I handled all your concerns

@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 21, 2024
@pchaigno pchaigno added this pull request to the merge queue Oct 21, 2024
Merged via the queue into cilium:main with commit 61563a4 Oct 21, 2024
63 checks passed
@ldelossa ldelossa deleted the ldelossa/refactor-xfrm-policy-state-handling-oss branch October 21, 2024 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. feature/ipsec Relates to Cilium's IPsec feature 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
Development

Successfully merging this pull request may close these issues.

4 participants