-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[IPsec] refactor xfrm policy state handling oss #35210
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] refactor xfrm policy state handling oss #35210
Conversation
ldelossa
commented
Oct 3, 2024
/test |
bdada1d
to
96be97f
Compare
/test |
There was a problem hiding this 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.
Hmm:
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. |
96be97f
to
939863b
Compare
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
I've switched up the release note categorization as this is not intended to have user-facing impact. Thanks! |
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. |
This comment was marked as resolved.
This comment was marked as resolved.
2a8a9b2
to
d74a15e
Compare
/test |
f26b400
to
36e5eee
Compare
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>
36e5eee
to
29e20ad
Compare
/test |
Think I handled all your concerns |