[DRAFT][RFC] L2 announce: add IPv6 support by msune · Pull Request #34983 · cilium/cilium

msune · 2024-09-20T11:32:22Z

This (incomplete) patchset adds IPv6 support for L2 announcements, effectively removing one of the Beta limitations of the L2 announce feature.

The PR is in a RFC state, before I get any deeper. In particular, I would like to know if the overall approach makes sense.

Notes:

This PR removes the requirement to define ENABLE_IPV6 to compile (unused when not defined) ICMPv6 code.

Missing work:

Make unit pass (untested)
Add support for gratuitous NA (see below)

Questions

Not particulary happy with the golang code under l2responder.go. It's a bit ugly with the if(v6) else ...; I am new to golang, and I haven't seen this pattern used in Maps, but would it make sense to have a common interface for both l2respondermaps?
To implement the Gratuitous Neighbour Advertisments (needed in l2responder.go), I am wondering if the best course of action is to take the current garp cell and add the NA part (and probably rename it to something like gneigh/gneigh.go).
Is there E2E coverage for L2 announce IPv4? I couldn't find it.

Thanks

msune · 2024-09-20T11:33:46Z

cc @borkmann

msune · 2024-10-02T07:42:33Z

Any input on the open questions?

borkmann · 2024-10-21T12:51:22Z

This (incomplete) patchset adds IPv6 support for L2 announcements, effectively removing one of the Beta limitations of the L2 announce feature.

🚀

The PR is in a RFC state, before I get any deeper. In particular, I would like to know if the overall approach makes sense.

Yes, this LGTM!

Notes:

This PR removes the requirement to define ENABLE_IPV6 to compile (unused when not defined) ICMPv6 code.

Ack, it's odd that this is part of the header guard.

Missing work:

Make unit pass (untested)

Add support for gratuitous NA (see below)

Questions

Not particulary happy with the golang code under l2responder.go. It's a bit ugly with the if(v6) else ...; I am new to golang, and I haven't seen this pattern used in Maps, but would it make sense to have a common interface for both l2respondermaps?

Cc @dylandreimerink for visibility. Imho, looks ok.

To implement the Gratuitous Neighbour Advertisments (needed in l2responder.go), I am wondering if the best course of action is to take the current garp cell and add the NA part (and probably rename it to something like gneigh/gneigh.go).

Cc @mhofstetter wrt separate cell?

Is there E2E coverage for L2 announce IPv4? I couldn't find it.

At least not that I can find it as part of #25471 which contains unit tests though. https://github.com/cilium/cilium-cli seems not to have them.

dylandreimerink · 2024-10-29T14:18:34Z

Not particulary happy with the golang code under l2responder.go. It's a bit ugly with the if(v6) else ...; I am new to golang, and I haven't seen this pattern used in Maps, but would it make sense to have a common interface for both l2respondermaps?

I think this code isn't that ugly. Writing it some other way would likely end up with more boilerplate.

To implement the Gratuitous Neighbour Advertisments (needed in l2responder.go), I am wondering if the best course of action is to take the current garp cell and add the NA part (and probably rename it to something like gneigh/gneigh.go).

I think that is a good idea. You can likely reuse a lot of logic. Renaming also seems good to me, just take the Pod L2 announcement use case into account to.

Is there E2E coverage for L2 announce IPv4? I couldn't find it.

Can confirm there is no E2E testing, only the _test.go and BPF tests.

tsndqst · 2024-10-29T14:45:21Z

It looks like the author of the arp package in garp.go also has an ndp package: https://github.com/mdlayher/ndp. Perhaps the interface is similar.

msune · 2024-11-05T16:03:42Z

msune To implement the Gratuitous Neighbour Advertisments (needed in l2responder.go), I am wondering if the best course of action is to take the current garp cell and add the NA part (and probably rename it to something like gneigh/gneigh.go).

dylandreimerink I think that is a good idea. You can likely reuse a lot of logic. Renaming also seems good to me, just take the Pod L2 announcement use case into account to.

ACK.

@dylandreimerink Wondering if it's better to add v6 Pod L2 announcement as part of this patchset or in a subsequent PR. Thoughts?

Is there E2E coverage for L2 announce IPv4? I couldn't find it.

borkmann At least not that I can find it as part of #25471 which contains unit tests though. https://github.com/cilium/cilium-cli seems not to have them.

So just to be clear, the only test that exists today is the BPF unit test here, so an addition bpf unit test - already part of this PR - should suffice, correct?

tsndqst It looks like the author of the arp package in garp.go also has an ndp package: https://github.com/mdlayher/ndp. Perhaps the interface is similar.

I will give it a try, thx. We only need Gratuitous NAs here, so it should be fairly easy.

Do not conditionally compile ICMPv6 code based on ENABLE_IPV6, to simplify codebase and be consistent with ARP code. Signed-off-by: Marc Suñé <[email protected]>

bpf code Signed-off-by: Marc Suñé <[email protected]>

Define v6_svc_one and v6_ext_one addresses required by the V6 l2_announce test. Signed-off-by: Marc Suñé <[email protected]>

This commit adds tc_l2_announce6 unit test, to cover IPv6 support for L2 announce. Signed-off-by: Marc Suñé <[email protected]>

control code Signed-off-by: Marc Suñé <[email protected]>

This commit: * Removes l2_announce IPv6 limitation from docs. * Minor cleanup on that page. Signed-off-by: Marc Suñé <[email protected]>

dylandreimerink · 2024-11-05T16:25:59Z

Wondering if it's better to add v6 Pod L2 announcement as part of this patchset or in a subsequent PR. Thoughts?

They are independent features, I think its fine to do that in a separate PR

maintainer-s-little-helper · 2024-11-06T09:12:26Z

Commit c17d882 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

github-actions · 2024-12-07T02:04:09Z

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

github-actions · 2024-12-22T02:03:30Z

This pull request has not seen any activity since it was marked stale.
Closing.

RonaldPhilipsen · 2025-01-04T22:21:36Z

pkg/datapath/l2responder/l2responder.go

+			if e.IP.Is6() {
+				err = arMap.Delete(e.IP, uint32(idx))
+			} else {
+				err = ndMap.Delete(e.IP, uint32(idx))
+			}


Perhaps im misreading this, but you define

arMap := p.params.L2ResponderMap ndMap := p.params.L2V6ResponderMap

shouldn´t this therefore be the other way around?

Suggested change

if e.IP.Is6() {

err = arMap.Delete(e.IP, uint32(idx))

} else {

err = ndMap.Delete(e.IP, uint32(idx))

}

if e.IP.Is6() {

err = ndMap.Delete(e.IP, uint32(idx))

} else {

err = arMap.Delete(e.IP, uint32(idx))

}

Indeed, thx. I plan to (finally) continue working on this.

RonaldPhilipsen · 2025-01-04T22:23:23Z

pkg/datapath/l2responder/l2responder.go

+		if e.IP.Is6() {
+			desiredMap[l2respondermap.L2ResponderKey{
+				IP:      types.IPv4(e.IP.As4()),
+				IfIndex: uint32(idx),
+			}] = desiredEntry{
+				entry: e,
+			}
+		} else {
+			desiredMap6[l2v6respondermap.L2V6ResponderKey{
+				IP:      types.IPv6(e.IP.As16()),
+				IfIndex: uint32(idx),
+			}] = desiredEntry{
+				entry: e,
+			}


Same thing here, looks like the IPV6/IPV4 logic is inverted

leptonyu · 2025-02-20T09:18:29Z

Is this PR still in review?

msune · 2025-05-20T08:35:59Z

Is this PR still in review?

There will be a follow up PR (final). I didn't have the time just yet to finalise it 😉. These PRs are prep work:

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 Sep 20, 2024

msune changed the title ~~[DRAFT] L2 announce: add IPv6 support~~ [DRAFT][RFC] L2 announce: add IPv6 support Sep 20, 2024

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Sep 20, 2024

msune force-pushed the l2_announce_ipv6 branch from 9cf1969 to 45bb330 Compare September 30, 2024 09:40

dylandreimerink mentioned this pull request Oct 29, 2024

L2-announcements lack ipv6-support #28985

Closed

msune added 6 commits November 5, 2024 17:16

bpf/lib/icmpv6: remove ifdef IPV6

89b9fe6

Do not conditionally compile ICMPv6 code based on ENABLE_IPV6, to simplify codebase and be consistent with ARP code. Signed-off-by: Marc Suñé <[email protected]>

bpf: tc l2_announce v6

519821f

bpf code Signed-off-by: Marc Suñé <[email protected]>

bpf/test/pktgen: add IPv6 {svc,ext}_one addresses

cfde29b

Define v6_svc_one and v6_ext_one addresses required by the V6 l2_announce test. Signed-off-by: Marc Suñé <[email protected]>

bpf/test: add L2 announce IPv6 unit test

5833d58

This commit adds tc_l2_announce6 unit test, to cover IPv6 support for L2 announce. Signed-off-by: Marc Suñé <[email protected]>

pkg: tc l2_announce v6

ea1fef2

control code Signed-off-by: Marc Suñé <[email protected]>

docs: remove IPv6 l2_announce limitation

9fb99fa

This commit: * Removes l2_announce IPv6 limitation from docs. * Minor cleanup on that page. Signed-off-by: Marc Suñé <[email protected]>

Tc

c17d882

msune force-pushed the l2_announce_ipv6 branch from 45bb330 to c17d882 Compare November 6, 2024 09:12

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Nov 6, 2024

msune mentioned this pull request Nov 18, 2024

bpf: misc ICMPv6 ND fixes (srcip, nollsrcopt...) #36024

Merged

github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Dec 7, 2024

github-actions bot closed this Dec 22, 2024

RonaldPhilipsen reviewed Jan 4, 2025

View reviewed changes

msune mentioned this pull request May 20, 2025

Add L2 announcement IPv6 support #39648

Merged

Conversation

msune commented Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes:

Missing work:

Questions

Uh oh!

msune commented Sep 20, 2024

Uh oh!

msune commented Oct 2, 2024

Uh oh!

borkmann commented Oct 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes:

Missing work:

Questions

Uh oh!

dylandreimerink commented Oct 29, 2024

Uh oh!

tsndqst commented Oct 29, 2024

Uh oh!

msune commented Nov 5, 2024

Uh oh!

dylandreimerink commented Nov 5, 2024

Uh oh!

maintainer-s-little-helper bot commented Nov 6, 2024

Uh oh!

github-actions bot commented Dec 7, 2024

Uh oh!

github-actions bot commented Dec 22, 2024

Uh oh!

RonaldPhilipsen Jan 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msune Jan 8, 2025

Choose a reason for hiding this comment

Uh oh!

RonaldPhilipsen Jan 4, 2025

Choose a reason for hiding this comment

Uh oh!

leptonyu commented Feb 20, 2025

Uh oh!

msune commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

msune commented Sep 20, 2024 •

edited

Loading

borkmann commented Oct 21, 2024 •

edited

Loading

RonaldPhilipsen Jan 4, 2025 •

edited

Loading

msune commented May 20, 2025 •

edited

Loading