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.

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

bpf: fib: fix issues with L2 resolution #30128

Merged
merged 4 commits into from
Jan 16, 2024
Merged

bpf: fib: fix issues with L2 resolution #30128

merged 4 commits into from
Jan 16, 2024

Conversation

julianwiedmann
Copy link
Member

@julianwiedmann julianwiedmann commented Jan 8, 2024 •
edited

Clarify in which scenarios the fallback to the neigh-map makes sense.

Then fix a recent regression where some code paths are no longer able to obtain a L2 resolution on BPF redirect. This would for instance affect the forwarding of LBed requests to remote backends, when running in XDP or on a pre-5.10 kernel.

@julianwiedmann julianwiedmann added kind/bug This is a bug in the Cilium logic. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. kind/regression This functionality worked fine before, but was broken in a newer release of Cilium. area/loadbalancing Impacts load-balancing and Kubernetes service implementations needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Jan 8, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jan 8, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in v1.15.0-rc.1 Jan 8, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.6 Jan 8, 2024
@julianwiedmann julianwiedmann added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jan 8, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 8, 2024
@julianwiedmann julianwiedmann force-pushed the 1.16-bpf-fib-redirect branch 2 times, most recently from b96793a to 6ee9489 Compare January 8, 2024 07:52
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann
Copy link
Member Author

/test

borkmann
borkmann reviewed Jan 8, 2024
View reviewed changes
bpf/lib/fib.h Outdated Show resolved Hide resolved
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann
bpf: fib: require opt-in for neighour map fallback in fib_do_redirect()
afe3a06 
The neighbour map is populated by the inbound nodeport path, and used to
cache the client's MAC address. Therefore it only makes sense to use this
fallback in the LB's reply path.

Opt-out from using it in
- the LB NAT forward path
- the LB DSR forward path
- the outbound EgressGW paths
- bpf_lxc's reply path, as that's only used with ENABLE_HOST_ROUTING and
  thus can always use the neigh-resolver.

Note that callers which can't use the neigh-map will need *some* sort of
toleration for failed L2 resolution / DROP_NO_FIB result.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: fib: fix DMAC rewrite with ENABLE_SKIP_FIB
76b6be4 
A recent FIB refactor introduced a bug, where fib_redirect*() no longer
performs a FIB lookup if ENABLE_SKIP_FIB is set.

But for configs without neigh-resolver, some code paths (that can't fall
back to the neigh map) strictly require this FIB lookup to obtain the
next-hop's MAC address.

Fix things by reintroducing the FIB lookup when neigh_resolver_available()
returns false.

Fixes: e30e18b ("bpf,fib: use fib_do_redirect in fib_redirect")
Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: fib: refactor fib_do_redirect()
fc41a7f 
Clarify the different paths of L2 resolution:
1. when the neigh-resolver is available, always use it. Forward the
   next-hop info from a preceding FIB lookup where available.
2. otherwise fallback to the neigh map, for callers that have opted in.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: nodeport: opt-out from neighbour map when XDP-forwarding via tunnel
4c46cde 
When XDP manually builds the tunnel headers and forwards to a remote node,
it makes no sense to rely on the neighbour map for L2 resolution.

We have to trust that the agent installs managed neigh entries for all
other nodes, and thus the FIB lookup will always return a L2 resolution.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann julianwiedmann added release-note/misc This PR makes changes that have no direct user impact. and removed release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Jan 12, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Needs backport from main in 1.14.6 Jan 12, 2024
@julianwiedmann julianwiedmann changed the title bpf: FIB fixes bpf: fib: fix issues with L2 resolution Jan 12, 2024
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann marked this pull request as ready for review January 12, 2024 09:19
@julianwiedmann julianwiedmann requested review from a team as code owners January 12, 2024 09:19
@julianwiedmann julianwiedmann added release-blocker/1.15 This issue will prevent the release of the next version of Cilium. feature/egress-gateway Impacts the egress IP gateway feature. labels Jan 12, 2024
ldelossa
ldelossa approved these changes Jan 15, 2024
View reviewed changes
Copy link
Contributor

@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

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

Read through these changes.

They look good to me. Think we still have a bit of refactoring to go as we talked a bit about out of band, but this fixes the issue at hand.

jschwinger233
jschwinger233 approved these changes Jan 16, 2024
View reviewed changes
Copy link
Member

@jschwinger233 jschwinger233 left a comment

Choose a reason for hiding this comment

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

Thanks! Just a minor question regarding fib_params.

bpf/lib/fib.h Show resolved Hide resolved
@julianwiedmann julianwiedmann added this pull request to the merge queue Jan 16, 2024
Merged via the queue into cilium:main with commit cc25b91 Jan 16, 2024
62 checks passed
@julianwiedmann julianwiedmann deleted the 1.16-bpf-fib-redirect branch January 16, 2024 17:21
This was referenced Jan 17, 2024
Merged
Open
@giorio94 giorio94 mentioned this pull request Jan 22, 2024
Merged
12 tasks
@giorio94 giorio94 added backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. and removed needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Jan 22, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.15 in v1.15.0-rc.1 Jan 22, 2024
@julianwiedmann julianwiedmann mentioned this pull request Jan 23, 2024
Merged
1 task
@julianwiedmann julianwiedmann added the backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. label Jan 23, 2024
@github-actions github-actions bot added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Jan 24, 2024
@giorio94 giorio94 added backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. and removed backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. labels Jan 29, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.15 to Backport done to v1.15 in v1.15.0-rc.1 Jan 29, 2024
@michi-covalent michi-covalent mentioned this pull request Feb 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ldelossa ldelossa ldelossa approved these changes

@jschwinger233 jschwinger233 jschwinger233 approved these changes

@borkmann borkmann Awaiting requested review from borkmann

Assignees
No one assigned
Labels
area/loadbalancing Impacts load-balancing and Kubernetes service implementations backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. feature/egress-gateway Impacts the egress IP gateway feature. kind/bug This is a bug in the Cilium logic. kind/regression This functionality worked fine before, but was broken in a newer release of Cilium. release-blocker/1.15 This issue will prevent the release of the next version of Cilium. release-note/misc This PR makes changes that have no direct user impact. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
v1.15.0-rc.1
Backport done to v1.15
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@julianwiedmann @borkmann @ldelossa @jschwinger233 @giorio94