bpf: minor LB cleanups #25061
Merged
bpf: minor LB cleanups #25061
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
julianwiedmann
added
sig/datapath
|
/test Job 'Cilium-PR-K8s-1.25-kernel-5.4' failed: Click to show.Test NameFailure OutputJenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.25-kernel-5.4/89/ If it is a flake and a GitHub issue doesn't already exist to track it, comment |
ct_create*() can result in CT-related DROP reasons. Return those to the caller, instead of a generic DROP_NO_SERVICE. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
CT_RELATED is only relevant for CT tuples that have TUPLE_F_RELATED set. The LB code doesn't build such tuples - it uses ct_lb_lookup(), and hence never passes through the relevant ICMP code in ct_lookup(). So treat this as unexpected condition, and drop the packet. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
A CT lookup with CT_SERVICE scope will only look up in the reverse direction, and thus __ct_lookup*() never returns CT_REOPENED or CT_ESTABLISHED (they would always get translated to CT_REPLY). So treat these two as invalid result. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Hide the details of managing the csum_offset struct. It's just something we need when actually re-writing the packet. For lb4_rev_nat(), we now consistently apply the micro-optimization to only initialize the struct for packets with a L4 header. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
julianwiedmann
force-pushed
the
1.14-bpf-lb-cleanups-v2
branch
from
c5f8587
to
63e3520
Compare
|
/test |
gentoo-root
approved these changes
Apr 24, 2023
| __be32 old_sip, new_sip, sum = 0; | ||
| int ret; | ||
|
|
||
| cilium_dbg_lb(ctx, DBG_LB4_REVERSE_NAT, nat->address, nat->port); | ||
|
|
||
| if (has_l4_header) | ||
| csum_l4_offset_and_flags(tuple->nexthdr, &csum_off); |
There was a problem hiding this comment.
Offtopic question: why don't we set off->offset for IPPROTO_ICMP in csum_l4_offset_and_flags?
There was a problem hiding this comment.
I've wondered about that myself recently while poking around the NAT code :) (as it requires us to set the offset manually ...). Best I could come up with was that we didn't need it in the pure LB paths.
So one option would be to declare the existing helpers as LB-specific, and introduce generic variants that do handle ICMP.
maintainer-s-little-helper
bot
added
the
ready-to-merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A few small LB cleanups that have accumulated. Ideally the first
twothree will make review of a pending fix a bit easier.