datapath: Fix redirect from from L3 netdev to tunnel #33421
Merged
+269
−0
Conversation
This file contains hidden or 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
This was referenced Jun 27, 2024
e9951ca to
9e852ad
Compare
9e852ad to
1c35824
Compare
Previously, the redirect from L3 netdev (in nodeport.h) to Cilium's
tunnel device failed due to the bpf_redirect returning -ERANGE [1].
Further investigation revealed the following values for the troublesome
skb:
skb->len = 60
skb->data = 0x000000009566a77e
skb->head = 0x000000001a70ff88
skb->network_header = 64
"skb->data" was set to a weird location when compared to "skb->head",
and thus made the check to fail.
Adding the L2 hdr before redirecting to the tunnel device resolved the
issue.
P.S., I wanted to use "if (__ctx_is == __ctx_skb)" instead of the #if.
Unfortunately, then the bpf_xdp.c fails to compile with:
error: use of undeclared identifier 'ENCAP_IFINDEX'
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/net/core/filter.c?h=v6.7.4#
n2147
Signed-off-by: Martynas Pumputis <m@lambda.lt>
1c35824 to
a4b5ee7
Compare
To test the fix in the previous commit. Ideally, we would want to test against running L3 netdev and vxlan netdev with Cilium's BPF progs loaded. Unfortunately, we don't have an infrastructure for such a test. Thus, only check that the L2 hdr is appended before the redirect. Signed-off-by: Martynas Pumputis <m@lambda.lt>
a4b5ee7 to
4159ad9
Compare
|
/test |
markpash
approved these changes
Jun 27, 2024
|
@brb do you have any smart ideas how to wire up L3 device testing into CI ? I feel we're playing whack-a-mole here :/. Surely the other users of |
|
For this PR I experimented with integration tests in which we attach BPF progs (to L3 netdev and tunnel netdev) from the partly mocked agent, and then inject with libpcap a packet. Unfortunately, the former turned to be not that straight forward task, so I abandoned it due to time constraints. Nevertheless, I'd like to chat with folks (foundations / loader / agent) about adding such tests (maybe at the hive time?). Another approach would be to extend Kind setup in ci-e2e by adding L3 netdev. This feels like doable in a reasonable amount of time. Let me create an issue for it. |
This was referenced Jul 10, 2024
julianwiedmann
added a commit
to julianwiedmann/cilium
that referenced
this pull request
Oct 2, 2024
When redirecting from a L3 device to the overlay interface, we need to manually add a L2 header to the (inner) packet. cilium#33421 fixed this for the case of Nodeport NAT traffic from the LB node to a backend. Generalize it so that it helps all users of the nodeport_add_tunnel_encap() helper - for example DSR-Geneve or EgressGW reply traffic. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
julianwiedmann
added a commit
to julianwiedmann/cilium
that referenced
this pull request
Oct 2, 2024
When redirecting from a L3 device to the overlay interface, we need to manually add a L2 header to the (inner) packet. cilium#33421 fixed this for the case of Nodeport NAT traffic from the LB node to a backend. Generalize it so that it helps all users of the nodeport_add_tunnel_encap() helper - for example DSR-Geneve or EgressGW reply traffic. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
1 task
1 task
julianwiedmann
added a commit
to julianwiedmann/cilium
that referenced
this pull request
Oct 8, 2024
When redirecting from a L3 device to the overlay interface, we need to manually add a L2 header to the (inner) packet. cilium#33421 fixed this for the case of Nodeport NAT traffic from the LB node to a backend. Generalize it so that it helps all users of the nodeport_add_tunnel_encap() helper - for example DSR-Geneve or EgressGW reply traffic. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Oct 13, 2024
When redirecting from a L3 device to the overlay interface, we need to manually add a L2 header to the (inner) packet. #33421 fixed this for the case of Nodeport NAT traffic from the LB node to a backend. Generalize it so that it helps all users of the nodeport_add_tunnel_encap() helper - for example DSR-Geneve or EgressGW reply traffic. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
tklauser
pushed a commit
that referenced
this pull request
Oct 22, 2024
[ upstream commit abdaddc ] When redirecting from a L3 device to the overlay interface, we need to manually add a L2 header to the (inner) packet. #33421 fixed this for the case of Nodeport NAT traffic from the LB node to a backend. Generalize it so that it helps all users of the nodeport_add_tunnel_encap() helper - for example DSR-Geneve or EgressGW reply traffic. Signed-off-by: Julian Wiedmann <jwi@isovalent.com> Signed-off-by: Tobias Klauser <tobias@cilium.io>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Oct 22, 2024
[ upstream commit abdaddc ] When redirecting from a L3 device to the overlay interface, we need to manually add a L2 header to the (inner) packet. #33421 fixed this for the case of Nodeport NAT traffic from the LB node to a backend. Generalize it so that it helps all users of the nodeport_add_tunnel_encap() helper - for example DSR-Geneve or EgressGW reply traffic. Signed-off-by: Julian Wiedmann <jwi@isovalent.com> Signed-off-by: Tobias Klauser <tobias@cilium.io>
rastislavs
pushed a commit
that referenced
this pull request
Oct 28, 2024
[ upstream commit abdaddc ] When redirecting from a L3 device to the overlay interface, we need to manually add a L2 header to the (inner) packet. #33421 fixed this for the case of Nodeport NAT traffic from the LB node to a backend. Generalize it so that it helps all users of the nodeport_add_tunnel_encap() helper - for example DSR-Geneve or EgressGW reply traffic. Signed-off-by: Julian Wiedmann <jwi@isovalent.com> Signed-off-by: Rastislav Szabo <rastislav.szabo@isovalent.com>
julianwiedmann
added a commit
that referenced
this pull request
Oct 29, 2024
[ upstream commit abdaddc ] When redirecting from a L3 device to the overlay interface, we need to manually add a L2 header to the (inner) packet. #33421 fixed this for the case of Nodeport NAT traffic from the LB node to a backend. Generalize it so that it helps all users of the nodeport_add_tunnel_encap() helper - for example DSR-Geneve or EgressGW reply traffic. Signed-off-by: Julian Wiedmann <jwi@isovalent.com> Signed-off-by: Rastislav Szabo <rastislav.szabo@isovalent.com>
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.
Previously, the redirect from L3 netdev (in nodeport.h) to Cilium's tunnel device failed due to the bpf_redirect returning -ERANGE [1].
Further investigation revealed the following values for the troublesome skb:
"skb->data" was set to a weird location when compared to "skb->head", and thus made the check to fail.
Adding the L2 hdr before redirecting to the tunnel device resolved the issue.
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/net/core/filter.c?h=v6.7.4#n2147
Fix #26847
cc @jspaleta