bpf: nodeport: minor DSR improvements #23326
Merged
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
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
|
/test |
julianwiedmann
force-pushed
the
1.14-nodeport-dsr-improvements
branch
from
cfb0ffc
to
ee0bda2
Compare
|
/test |
julianwiedmann
force-pushed
the
1.14-nodeport-dsr-improvements
branch
from
ee0bda2
to
7ad0b59
Compare
julianwiedmann
added
sig/datapath
maintainer-s-little-helper
bot
removed
dont-merge/needs-release-note-label
julianwiedmann
changed the title
julianwiedmann
changed the title
|
/test |
julianwiedmann
requested review from
dylandreimerink,
borkmann and
YutaroHayakawa
borkmann
approved these changes
Jan 25, 2023
pchaigno
approved these changes
Jan 25, 2023
There was a problem hiding this comment.
LGTM. One nit below but it's probably not worth repushing just for that.
Probably would be best to move new tests like the last commit into separate PRs in the future. We may want to backport changes here separately (e.g., don't backport refactoring but backport the test).
EDIT: Really nice cleanups and nice commit breakdown by the way ❤️
| #define DSR_IPV4_OPT_32 0x9a080000 | ||
| #define DSR_IPV4_OPT_MASK 0xffff0000 | ||
| #define DSR_IPV4_DPORT_MASK 0x0000ffff | ||
| #define DSR_IPV4_OPT_TYPE (IPOPT_COPY | 0x1a) |
There was a problem hiding this comment.
Nit: We probably need to remove the Len line in the comment above, so clarify this is now encoding only the option number as it should be.
There was a problem hiding this comment.
Yep, on my radar 👍 . Once the DSR code fully moves into nodeport.h, this stuff goes with it (right next to the dsr_opt_v4 struct).
I'll need to push once more either way, to adjust for #23281. |
julianwiedmann
force-pushed
the
1.14-nodeport-dsr-improvements
branch
from
7ad0b59
to
35d2935
Compare
|
/test |
Currently we first extract the ports from the packet in their actual order, and then store them into the CT tuple in reverse order. Just follow the established pattern and immediately store into the CT tuple in reverse order. While at it also switch to the l4_load_ports() helper. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
The preceding revalidate_data() always locates the L3 header at `data + ETH_HLEN`. So there's no need for a more generic approach when determining the L4 offset a few lines later. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Let bpf_lxc provide a validated L3 header to the low-level DSR code. Also pass this on to snat_*_create_dsr(). Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Introduce a proper struct to get rid of all the complicated masking, and clearly spell out the .type and .len parts of the option. Use this struct to fill & extract the DSR info. The on-wire format stays the same, and this patch doesn't try to solve the endianness confusion for the .port and .addr fields. When fetching the IPv4 option words, use a single ctx_load_bytes(). We've previously checked ip4->ihl, so there's no risk that the packet might be too short. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
The pre-defined ports are in network byte-order, do the same for the IP addresses. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Add tests for forwarding to a remote backend with DSR, where the LB needs to insert the IPv4 Option. We're not testing local-backend here (it's identical to non-DSR), and there's no reply path trough the LB node. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
julianwiedmann
force-pushed
the
1.14-nodeport-dsr-improvements
branch
from
35d2935
to
8323e33
Compare
|
/test |
YutaroHayakawa
approved these changes
Jan 26, 2023
julianwiedmann
added
the
ready-to-merge
6 tasks
julianwiedmann
added
the
backport-done/1.13
maintainer-s-little-helper
bot
removed
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.
Looks like #22978 needs some more cycles. So let's split off some non-controversial cleanups & tests to reduce the review later on. I'll tag this PR for backports once #22978 is ready and needs it.
Best reviewed patch-by-patch - #23262 has some more context on the IPv4 option endianness.