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

Test IPsec + KPR #31760

Merged
merged 3 commits into from
May 2, 2024
Merged

Test IPsec + KPR #31760

merged 3 commits into from
May 2, 2024

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Apr 3, 2024

This pull request adds end-to-end CI coverage for IPsec + KPR, including coverage of key rotations and up/downgrades.

@pchaigno pchaigno added release-note/major This PR introduces major new functionality to Cilium. feature/ipsec Relates to Cilium's IPsec feature labels Apr 3, 2024
@pchaigno pchaigno force-pushed the pr/pchaigno/ipsec-kpr branch 4 times, most recently from f7cf28a to 5069a4a Compare April 9, 2024 08:19
@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 Apr 23, 2024
@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 Apr 23, 2024
@pchaigno pchaigno added area/CI Continuous Integration testing issue or flake area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. release-note/ci This PR makes changes to the CI. and removed release-note/major This PR introduces major new functionality to Cilium. labels May 1, 2024
With previous fixes, we can now have IPsec enabled together with KPR.
IPsec will encrypt traffic between pods as usual. Note that requests to
a NodePort that are being forwarded from the receiving node to a node
with a backend won't be encrypted.

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
We can reuse the two configs that had --devices set because KPR will
cause devices to be autodetected anyway. We then need to add one other
config to cover VXLAN.

Upgrade tests are not extended to cover KPR because it isn't supported
in the previous stable. We will need to wait for the next minor release
to be able to extend those tests.

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
To be able to cover this configuration without removing coverage for
others, we need to add one more test case. Fortunately, it will run in
parallel to other test case.

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
@pchaigno pchaigno requested a review from rgo3 May 2, 2024 09:33
@pchaigno pchaigno marked this pull request as ready for review May 2, 2024 09:34
@pchaigno pchaigno requested review from a team as code owners May 2, 2024 09:34
@pchaigno pchaigno requested review from jibi, aanm and christarazi May 2, 2024 09:34
@pchaigno pchaigno added this pull request to the merge queue May 2, 2024
Merged via the queue into main with commit 92dfbc5 May 2, 2024
270 checks passed
@pchaigno pchaigno deleted the pr/pchaigno/ipsec-kpr branch May 2, 2024 11:53
Comment on lines -72 to -75
if option.Config.EnableIPSec {
return fmt.Errorf("IPSec cannot be used with BPF NodePort")
}

Copy link
Member

Choose a reason for hiding this comment

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

From a look at what additional feature combinations this allows (with features that depend on EnableNodePort, and thus were previously incompatible), the two that stand out are:

  • XDP NodePort acceleration
  • BPF Masquerade (and all of its dependencies, like Egress Gateway)

Should we explicitly reject these two, until we have fully understood the implications and added sufficient test coverage?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's probably best. Should I already disable for WireGuard or do we have coverage in that case?

Copy link
Member

Choose a reason for hiding this comment

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

I'm note quite parsing

Should I already disable for WireGuard or do we have coverage in that case?

You mean whether WireGuard should also conflict against XDP and/or BPF Masquerade? I think that ship has sailed already, breaking potentially existing configs is a much tougher decision. Coverage-wise, I see that we have WireGuard + EGW in the e2e test.

I would also expect that those features have much more user exposure to WireGuard already (because IPsec was not an option until now).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI Continuous Integration testing issue or flake area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. feature/ipsec Relates to Cilium's IPsec feature release-note/ci This PR makes changes to the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants