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

[v1.15] re-introduce ICMPv6 NS responder on from-netdev #31155

Merged
merged 4 commits into from Mar 5, 2024

Conversation

jschwinger233
Copy link
Member

@jschwinger233 jschwinger233 commented Mar 5, 2024

Once this PR is merged, a GitHub action will update the labels of these PRs:

30837 31127

@maintainer-s-little-helper maintainer-s-little-helper bot added backport/1.15 This PR represents a backport for Cilium 1.15.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master. labels Mar 5, 2024
[ upstream commit: 60c5e76 ]

SKIP_ICMPV6_NS_HANDLING was there to pass bpf coverage test, which is
gone by cilium#28090.

In the meantime, removing SKIP_ICMPV6_NS_HANDLING from
tc_nodeport_l3_dev.c prevents "potential missed tailcall" errors
introduced by cilium#30467, as
tail_icmp6_handle_ns() doesn't exist when SKIP_ICMPV6_NS_HANDLING is
defined, but still gets tail-called by icmp6_handle_ns().

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
[ upstream commit: dc9dfd7 ]

[ backporter's notes: in v1.15 icmp6_handle_ns() doesn't have
  ext_err parameter, so icmp6_host_handle() doesn't need to accept
  it either. ]

This reverts commit 6580714, to fix
the breakage of "IPv6 NS responder for pod" introduced by
cilium#12086 (bpf: Reply NA when recv ND for local IPv6 endpoints).

6580714 was merged to solve cilium#14509.
To not revive cilium#14509, this commit also passes through ICMPv6 NS if the
target is native node IP (eth0's addr). By letting stack take care of
those NS-for-node-IP packets, we managed to:

1. Solve cilium#14509 again, but in a way keeping NS responder. The cause of
   cilium#14509 was NS responder always generates ND whose source IP is
   "router_ip" (cilium_internal_ip) rather than "node_ip". Once we pass
   those NS-for-node-IP packets to stack, the ND response would
   naturally have "node_ip" as source.

2. Avoid the fib_lookup failure mentioned at cilium#30837 (comment).

icmp6_host_handle() also has a new parameter `handle_ns` to control if
we want NS responder to be active. If it is called from `to-netdev` code
path, handle_ns is set to false. This is suggested by julianwiedmann.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
@jschwinger233
Copy link
Member Author

/test-backport-1.15

jschwinger233 and others added 2 commits March 5, 2024 19:04
[ upstream commit: 8d4db89 ]

[ backporter's notes: minor changes due to lack of cilium#30467 in v1.15 ]

This commit adds bpf/tests/ipv6_ndp_from_netdev_test.c to cover two
scenarios:
1. from_netdev receives IPv6 NS for a pod IP on the same host
2. from_netdev receives IPv6 NS for the node IP (eth0's addr)

For case 1, from_netdev should return a NA on behalf of the target pod
to avoid cilium#30926.
for case 2, it must return the NS to stack to address cilium#14509.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
[ upstream commit: 475a194 ]

[ backporter's notes: minor conflict due to v1.15 icmp6_host_handle()
  doesn't have ext_err parameter. ]

The ICMPv6 handling in handle_ipv6() is only required for the HostFW or by
from-netdev. Exclude it otherwise.

This is a minor optimization for
dc9dfd7 ("bpf: Re-introduce ICMPv6 NS responder on from-netdev").

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
@jschwinger233
Copy link
Member Author

/test-backport-1.15

@jschwinger233 jschwinger233 added feature/ipv6 Relates to IPv6 protocol support sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Mar 5, 2024
@jschwinger233 jschwinger233 marked this pull request as ready for review March 5, 2024 13:08
@jschwinger233 jschwinger233 requested a review from a team as a code owner March 5, 2024 13:08
@julianwiedmann julianwiedmann changed the title v1.15 Backports from-host's ICMPv6 path [v1.15] re-introduce ICMPv6 NS responder on from-netdev Mar 5, 2024
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.

lgtm, ty Gray!

@julianwiedmann julianwiedmann added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 5, 2024
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Deferring to Julian's ack as tophat.

@joestringer joestringer merged commit ccf6810 into cilium:v1.15 Mar 5, 2024
60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.15 This PR represents a backport for Cilium 1.15.x of a PR that was merged to main. feature/ipv6 Relates to IPv6 protocol support kind/backports This PR provides functionality previously merged into master. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants