Skip to content

bpf: support ipv6 egressgateway policies #37713

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 2 commits into from
Mar 20, 2025
Merged

bpf: support ipv6 egressgateway policies #37713

merged 2 commits into from
Mar 20, 2025

Conversation

rgo3
Copy link
Contributor

@rgo3 rgo3 commented Feb 18, 2025 •
edited
Loading

This PR enables ipv6 egressgateway policies at the datapath level, ensuring that IPv6 traffic can be matched when respective policies are in place. It should be noted that intra cluster traffic that needs to be redirected to the correct gateway node, is still going through an IPv4 tunnel.

Apart from that, all the newly IPv6 related changes in this commit should be akin to the already existing code paths that provide the egressgateway functionality for IPv4 traffic.

It also adds the same tests as we currently have for ipv4 policies.

@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 Feb 18, 2025
@rgo3 rgo3 force-pushed the pr/rgo3/ipv6-egw-datapath branch from c67c8e3 to fc118e8 Compare February 18, 2025 15:11
rgo3
rgo3 commented Feb 18, 2025
View reviewed changes
julianwiedmann
julianwiedmann reviewed Feb 18, 2025
View reviewed changes
Copy link
Member

@julianwiedmann julianwiedmann 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! I just took a first pass to sketch out the packet flow, so we're aligned on what those parts do. It looks complete 🙂.

One subtle thing to check is whether when masquerading (on the gateway node), we already pass back the correct CTX_ACT_REDIRECT (when bouncing the packet to a different egress interface) and orig_addr for send_trace_notify6(). I remember taking some shortcuts back then, because we only needed it for IPv4 ...

@rgo3 rgo3 force-pushed the pr/rgo3/ipv6-egw-datapath branch 5 times, most recently from 0a5c0e3 to 1f75f98 Compare February 21, 2025 11:14
@julianwiedmann
Copy link
Member

julianwiedmann commented Feb 21, 2025 •
edited
Loading

One subtle thing to check is whether when masquerading (on the gateway node), we already pass back the correct CTX_ACT_REDIRECT (when bouncing the packet to a different egress interface) and orig_addr for send_trace_notify6(). I remember taking some shortcuts back then, because we only needed it for IPv4 ...

Found it, this is what I meant:

cilium/bpf/lib/nodeport_egress.h

Line 120 in 457fdd8

/* contrary to tail_handle_snat_fwd_ipv4, we don't check for

@rgo3
Copy link
Contributor Author

rgo3 commented Feb 21, 2025

I'll address the missing bits in tail_handle_snat_fwd_ipv6 as soon as I have at least one test working. I think the redirect from host tests shouldn't be affected by this.

Currently struggling quite a bit with getting the redirect from host to gateway test to run with ipv6. Verifier seems to dislike my ipv6 translation of the test quite a bit. Any suggestions?

julianwiedmann
julianwiedmann reviewed Feb 21, 2025
View reviewed changes
@rgo3 rgo3 force-pushed the pr/rgo3/ipv6-egw-datapath branch 2 times, most recently from 18b0201 to 766dadb Compare March 3, 2025 21:46
rgo3
rgo3 commented Mar 4, 2025
View reviewed changes
rgo3
rgo3 commented Mar 4, 2025
View reviewed changes
rgo3
rgo3 commented Mar 4, 2025
View reviewed changes
@rgo3 rgo3 force-pushed the pr/rgo3/ipv6-egw-datapath branch 2 times, most recently from f48954d to 1f3fa50 Compare March 4, 2025 11:21
@rgo3
Copy link
Contributor Author

rgo3 commented Mar 4, 2025

Ran verifier tests against main and my branch on minimum supported kernel version (5.4), no huge differences in complexity:

❯ diff complexity-main complexity-egwv6
5c5
<     verifier_test.go:263: cil_to_netdev: processed 46028 insns (limit 1000000) max_states_per_insn 19 total_states 2492 peak_states 1369 mark_read 49 (3594 unverified insns)
---
>     verifier_test.go:263: cil_to_netdev: processed 48006 insns (limit 1000000) max_states_per_insn 19 total_states 2578 peak_states 1488 mark_read 47 (4222 unverified insns)
18c18
<     verifier_test.go:263: tail_handle_snat_fwd_ipv6: processed 8682 insns (limit 1000000) max_states_per_insn 7 total_states 447 peak_states 423 mark_read 53 (2521 unverified insns)
---
>     verifier_test.go:263: tail_handle_snat_fwd_ipv6: processed 9517 insns (limit 1000000) max_states_per_insn 8 total_states 533 peak_states 503 mark_read 59 (2783 unverified insns)
30c30
<     verifier_test.go:263: tail_nodeport_rev_dnat_ingress_ipv6: processed 2638 insns (limit 1000000) max_states_per_insn 6 total_states 182 peak_states 181 mark_read 29 (964 unverified insns)
---
>     verifier_test.go:263: tail_nodeport_rev_dnat_ingress_ipv6: processed 3800 insns (limit 1000000) max_states_per_insn 4 total_states 209 peak_states 207 mark_read 29 (1163 unverified insns)

Given the limit of 1000000 ins, this shouldn't be a concern I hope.

@rgo3
Copy link
Contributor Author

rgo3 commented Mar 4, 2025

/test

@rgo3
Copy link
Contributor Author

rgo3 commented Mar 4, 2025

Looks like I've hit #32387 in https://github.com/cilium/cilium/actions/runs/13655381077/job/38173190618

@rgo3 rgo3 force-pushed the pr/rgo3/ipv6-egw-datapath branch from 1f3fa50 to 95d5260 Compare March 5, 2025 09:55
@rgo3 rgo3 marked this pull request as ready for review March 5, 2025 09:55
@rgo3 rgo3 requested review from a team as code owners March 5, 2025 09:55
@rgo3 rgo3 requested a review from ysksuzuki March 5, 2025 09:55
@rgo3
Copy link
Contributor Author

rgo3 commented Mar 5, 2025

/test

@julianwiedmann julianwiedmann added area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact. labels Mar 5, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Mar 5, 2025
@julianwiedmann julianwiedmann self-requested a review March 5, 2025 10:21
julianwiedmann
julianwiedmann reviewed Mar 5, 2025
View reviewed changes
Copy link
Member

@julianwiedmann julianwiedmann 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, looks close to ship already!

Took a first swing over the production code, will come back for the tests later :)

@rgo3 rgo3 force-pushed the pr/rgo3/ipv6-egw-datapath branch from 95d5260 to 13cc450 Compare March 5, 2025 19:41
julianwiedmann
julianwiedmann approved these changes Mar 9, 2025
View reviewed changes
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

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

Tests lgtm as well, thank you!

@rgo3 rgo3 force-pushed the pr/rgo3/ipv6-egw-datapath branch 2 times, most recently from 3172e25 to 3984364 Compare March 10, 2025 18:02
@rgo3
Copy link
Contributor Author

rgo3 commented Mar 10, 2025

/test

@rgo3 rgo3 force-pushed the pr/rgo3/ipv6-egw-datapath branch from 3984364 to 024846b Compare March 11, 2025 10:39
@rgo3
Copy link
Contributor Author

rgo3 commented Mar 11, 2025

/test

@julianwiedmann julianwiedmann added this pull request to the merge queue Mar 19, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 19, 2025
@julianwiedmann
Copy link
Member

@rgo3 looks like this needs a rebase and some love

@rgo3
Copy link
Contributor Author

rgo3 commented Mar 20, 2025

Looks like I need to make some general adjustments after Timos recent PR to remove dynamic map names.

rgo3 added 2 commits March 20, 2025 11:47
@rgo3
bpf: support ipv6 egressgateway policies
0388d83 
This commit enables ipv6 egressgateway policies at the datapath level,
ensuring that IPv6 traffic can be matched when respective policies are
in place. It should be noted that intra cluster traffic that needs to be
redirected to the correct gateway node, is still going through an IPv4
tunnel.
Apart from that, all the newly IPv6 related changes in this commit
should be akin to the already existing code paths that provide the
egressgateway functionality for IPv4 traffic.

Signed-off-by: Robin Gögge <r.goegge@isovalent.com>
@rgo3
bpf/tests: add egress gw tests for ipv6
8318a8a 
This commit provides a translation of the existing IPv4 BPF integration
test suites for egressgateway policies into IPv6 equivalent tests.

Signed-off-by: Robin Gögge <r.goegge@isovalent.com>
@rgo3 rgo3 force-pushed the pr/rgo3/ipv6-egw-datapath branch from 024846b to 8318a8a Compare March 20, 2025 10:56
@rgo3
Copy link
Contributor Author

rgo3 commented Mar 20, 2025

/test

@rgo3
Copy link
Contributor Author

rgo3 commented Mar 20, 2025

Looks good now :)

@julianwiedmann julianwiedmann added this pull request to the merge queue Mar 20, 2025
@ysksuzuki
Copy link
Member

Looks like the checkpatch is failing
https://github.com/cilium/cilium/actions/runs/13967644064/job/39101637308

WARNING:LONG_LINE: line length of 101 exceeds 100 columns
#711: FILE: bpf/lib/nodeport_egress.h:747:
+						      __and(is_defined(ENABLE_EGRESS_GATEWAY_COMMON),


NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] bpf: support ipv6 egressgateway policies" has style problems, please review.

Merged via the queue into main with commit 214f200 Mar 20, 2025
297 of 299 checks passed
@julianwiedmann julianwiedmann deleted the pr/rgo3/ipv6-egw-datapath branch March 20, 2025 13:46
@rgo3
Copy link
Contributor Author

rgo3 commented Mar 20, 2025

@ysksuzuki Yes, that is known and was discussed (I'll cc you on the resolved discussion). TL;DR, the equivalent ipv4 code looks exactly the same, so I figured it's fine to violate checkpatch in that one instance.

@ysksuzuki
Copy link
Member

Oh, I missed that comment. Thanks!

@rgo3 rgo3 mentioned this pull request Mar 28, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julianwiedmann julianwiedmann julianwiedmann approved these changes

@ysksuzuki ysksuzuki Awaiting requested review from ysksuzuki ysksuzuki is a code owner automatically assigned from cilium/egress-gateway

Assignees
No one assigned
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. feature/egress-gateway Impacts the egress IP gateway feature. feature/ipv6 Relates to IPv6 protocol support 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
@rgo3 @julianwiedmann @ysksuzuki