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

cilium: fib lookup consolidation #23139

Closed
wants to merge 24 commits into from
Closed

cilium: fib lookup consolidation #23139

wants to merge 24 commits into from

Conversation

borkmann
Copy link
Member

@borkmann borkmann commented Jan 17, 2023

This series reworks and consolidates all fib lookups in our BPF code to utilize common functionality.

For tc BPF, this means that redirect_neigh code can be used whenever neighbors cannot be resolved. XDP utilizes the BPF neighbor map as we cannot do this ad-hoc in this layer.

This affects KPR, standalone LB, NAT46x64 GW, DSR code.

Fixes: #22782

Out of scope in here: EDT fix

@borkmann borkmann added sig/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. needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Jan 17, 2023
@borkmann borkmann force-pushed the pr/nat46x64-batch branch 9 times, most recently from 1be1cd4 to f084edc Compare January 27, 2023 13:06
@borkmann borkmann force-pushed the pr/nat46x64-batch branch 2 times, most recently from 4ba56f7 to 511d55f Compare February 13, 2023 21:13
The NAT4->6 via services should only have the IPv6 entry, the NAT6->4 via
services should only have the IPv4 entry.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Remove for now to simplify later refactoring. Later commits add a reworked
version for single-node devices.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Add a neigh_resolver_available() helper which is true for skb and false for XDP.
For XDP we need to reuse the BPF neighbor map. Add logic in here, so we have
everything in a single location.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Small rename into fib_redirect_v{4,6}, no change in functionality.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Pass in iif as a parameter for fib_redirect_v{4,6}.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
For stateless NAT46x64, reuse the fib_redirect_v{4,6} code. Now the
tc BPF layer is actually able to resolve neighbors on demand.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Pull in the needs_l2_check functionality into fib_redirect_v{4,6} and
control this via const bool needs_l2_check given not all call-sites
need this. Mainly only when we push from bpf_lxc to an L3 device to
outside world such as wireguard.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
As next step, move all the DSR fib_lookup() open coding to reuse the
fib_redirect_v{4,6} helpers. This now enables tc BPF layer to resolve
neighbors on demand which it was previously not able to do.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
To allow for visibility, pass in ext_err and propagate fib lookup error codes
to call-sites, so that they are able to pass these to send_drop_notify*().

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
…elpers

Rework fib_redirect_v{4,6} to not return CTX_ACT_* but actual DROP_* errors
instead. Also, add a new fib_err for the BPF map, namely BPF_FIB_MAP_NO_NEIGH
when no neighbor could be found.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Provide more fib visibility for BPF host routing, and pass in ext_err to
fib_redirect_v{4,6} so that drop notifications will see them. Before that
we've ignored more fine-grained errors.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Since we removed ENABLE_SKIP_FIB.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Get rid of code duplication in fib_redirect_v{4,6} helpers, we only really
need to prep fib_params and common code can reside in fib_redirect(). This
is also needed for some call-sites for NAT46x64 which prep fib_params and
should not use fib_redirect_v{4,6} to avoid verifier complexity.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Switch to bpf_fib_lookup_padded version given some call-sites need this
for calling into fib_redirect(). The padded one helps on older kernels
when some of the fib_params are passed to helpers such as key for map
lookup on neighbor table.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Convert tail_nodeport_nat_egress_ipv{4,6} to use common fib_redirect() code.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@borkmann borkmann marked this pull request as ready for review February 15, 2023 21:57
@borkmann borkmann requested review from a team as code owners February 15, 2023 21:57
@borkmann
Copy link
Member Author

/test

1 similar comment
@borkmann
Copy link
Member Author

/test

@borkmann
Copy link
Member Author

borkmann commented Feb 15, 2023

/test

Job 'Cilium-PR-K8s-1.24-kernel-5.4' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Unable to download helm chart v1.12 from GitHub

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.24-kernel-5.4 so I can create one.

@borkmann
Copy link
Member Author

/test-1.16-4.9

@borkmann
Copy link
Member Author

/test-runtime

@borkmann
Copy link
Member Author

/test-1.26-net-next

@borkmann
Copy link
Member Author

/test-1.25-4.19

@borkmann
Copy link
Member Author

borkmann commented Feb 16, 2023

/test-1.24-5.4

Job 'Cilium-PR-K8s-1.16-kernel-4.9' has 4 failures but they might be new flakes since it also hit 1 known flakes: #22056 (92.42)

Job 'Cilium-PR-K8s-1.26-kernel-net-next' has 4 failures but they might be new flakes since it also hit 1 known flakes: #22056 (93.08)

@borkmann
Copy link
Member Author

/test-1.24-5.4

@borkmann
Copy link
Member Author

/test-1.26-net-next

@borkmann
Copy link
Member Author

/test-1.25-4.19

@pchaigno
Copy link
Member

@borkmann Please triage the CI failures instead of restarting until it passes. We have a bunch of recurring failures for which nobody opened an issue because everyone is just restarting.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@borkmann
Copy link
Member Author

/test

@pchaigno pchaigno marked this pull request as draft February 17, 2023 10:30
@borkmann
Copy link
Member Author

@borkmann Please triage the CI failures instead of restarting until it passes. We have a bunch of recurring failures for which nobody opened an issue because everyone is just restarting.

I did look at the sysdumps to see if there is some correlation with the patch series and whether the errors are persistent in the first place. Given L7 related, my thinking was that it might have been due to the passed fib_lookup params.

@borkmann
Copy link
Member Author

Superseeded by #23884 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
None yet
2 participants