Support DSR with Geneve dispatch #23890

ysksuzuki · 2023-02-20T17:08:36Z

For reviewers,

Currently, this implementation doesn't support IPv6 with XDP. It works with IPv4 either in XDP or TC, relying on XDP to TC punt. To support IPv6 in XDP, we need to implement the XDP native encap, which planning to do as a follow-up.
This implementation is compatible with the Geneve tunneling mode, which means that it works with either the direct routing mode or the Geneve tunneling mode. but unfortunately, it doesn't work with VXlan tunnel because there's no space to store service IP/port in the VXlan header.
~~Picked up an unassigned Geneve option class and type, referencing~~ We have been allocated 0x014B https://www.iana.org/assignments/nvo3/nvo3.xhtml#geneve-option-class.
~~Trying to mitigate the MTU overhead by encapsulating only SYN packets as the IP option mode does~~ Encapsulate the whole connection, but only add the GENEVE option to the TCP-SYN packets

Please ensure your pull request adheres to the following guidelines:

For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
If your commit description contains a Fixes: <commit-id> tag, then
please add the commit author[s] as reviewer[s] to this issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Thanks for contributing!

Support DSR with Geneve dispatch in CNI mode

ysksuzuki · 2023-02-20T17:10:28Z

Hi @julianwiedmann! Here is the draft PR on top of #22978. I will update docs tomorrow.

julianwiedmann · 2023-02-20T17:13:13Z

Hi @julianwiedmann! Here is the draft PR on top of #22978. I will update docs tomorrow.

awesome, thanks @ysksuzuki ! Will try to take a first look tomorrow :)

julianwiedmann · 2023-02-20T17:20:02Z

/test

julianwiedmann

Hi @ysksuzuki - just a few comments from a quick look. Haven't poked at the XDP-to_TC logic yet. Looking really good so far! Please consider if you can split the patches up somewhat - maybe one for the DSR insertion, one for the DSR extraction, one for the agent ...?

My initial expectation was that we would encapsulate the whole DSR connection in Geneve, not just the TCP-SYN. Seeing this mixed pattern in network traces will be fun when debugging :).

bpf/lib/nodeport.h

bpf/lib/common.h

bpf/lib/nodeport.h

bpf/bpf_overlay.c

ysksuzuki · 2023-02-22T00:51:03Z

@julianwiedmann Thank you for the review!

Please consider if you can split the patches up somewhat - maybe one for the DSR insertion, one for the DSR extraction, and one for the agent ...?

Sure, I will reorganize my commits.

My initial expectation was that we would encapsulate the whole DSR connection in Geneve, not just the TCP-SYN. Seeing this mixed pattern in network traces will be fun when debugging :).

I'm trying to mitigate the MTU overhead, but it might be a premature optimization. What do you think? Should we encapsulate the whole DSR connection and keep it simple?

I implemented this because

The current IP option-based DSR is doing the same optimization
There's a need to mitigate tunneling overhead. e.g. CFP: Implement hybrid tunnel mode #22898

bpf/bpf_host.c

julianwiedmann · 2023-02-23T08:43:01Z

My initial expectation was that we would encapsulate the whole DSR connection in Geneve, not just the TCP-SYN. Seeing this mixed pattern in network traces will be fun when debugging :).

I'm trying to mitigate the MTU overhead, but it might be a premature optimization. What do you think? Should we encapsulate the whole DSR connection and keep it simple?

I implemented this because
* The current IP option-based DSR is doing the same optimization

* There's a need to mitigate tunneling overhead. e.g. [CFP: Implement hybrid tunnel mode  #22898](https://github.com/cilium/cilium/issues/22898)

Avoiding the MTU overhead is definitely a good motivation for this approach, agreed.

Besides the general "packet flow won't look homogenous" thought, I'm a bit concerned about encountering connection-tracking in middle boxes, and them not seeing our TCP-SYN (as it's encapsulated).

ysksuzuki · 2023-02-23T10:00:19Z

I'm a bit concerned about encountering connection-tracking in middle boxes, and them not seeing our TCP-SYN (as it's encapsulated).

Yeah, I'm worried about it too. So how about this, we encapsulate the whole connection and keep it simple for now, and save this approach until we get feedback from users that the MTU overhead is pretty high and want to avoid it.

julianwiedmann · 2023-02-23T12:16:34Z

I'm a bit concerned about encountering connection-tracking in middle boxes, and them not seeing our TCP-SYN (as it's encapsulated).

Yeah, I'm worried about it too. So how about this, we encapsulate the whole connection and keep it simple for now, and save this approach until we get feedback from users that the MTU overhead is pretty high and want to avoid it.

Yep, sounds good!

julianwiedmann

Thanks @ysksuzuki ! Just a few smaller things, still need to digest the rest. So maybe hold off for a bit ...

Do we need to reduce the MTU accordingly to the max GENEVE option-size? 🤔

bpf/lib/common.h

bpf/lib/nodeport.h

bpf/lib/encap.h

ysksuzuki · 2023-04-01T13:00:30Z

we're trying to get away from ginkgo-based testing

I didn't know about it. Do you have an issue or design docs about it? I'm happy to work on testing DSR with Geneve functionality using the new test framework.

julianwiedmann · 2023-04-03T13:09:46Z

/test

ysksuzuki · 2023-04-03T15:46:27Z

k8s-1.26-kernel-net-next failed. Flake: #24687
https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/1614/testReport/junit/Suite-k8s-1/26/K8sUpdates_Tests_upgrade_and_downgrade_from_a_Cilium_stable_image_to_master/

runtime failed. Flake: #24326
https://jenkins.cilium.io/job/Cilium-PR-Runtime-net-next/5851/testReport/junit/(root)/Suite-runtime/RuntimeDatapathPrivilegedUnitTests_Run_Tests/

	 [13:40:21] FAIL: node_linux_test.go:2247: linuxPrivilegedIPv4OnlyTestSuite.TestArpPingHandling
	 [13:40:21] 
	 [13:40:21] node_linux_test.go:2451:
	 [13:40:21]     // deleteNeighbor is invoked async too
	 [13:40:21]     wait(nodev1.Identity(), "veth0", nil, true)
	 [13:40:21] node_linux_test.go:2368:
	 [13:40:21]     c.Assert(err, check.IsNil)
	 [13:40:21] ... value *errors.errorString = &errors.errorString{s:"timeout reached while waiting for condition"} ("timeout reached while waiting for condition")

ysksuzuki · 2023-04-03T16:19:25Z

Need to rebase or just rerun?

This commit encapsulates the DSR flow with Geneve, inserts DSR specific option, the service IP/port, into the Geneve option and redirects to the selected backend. Currently, it doesn't support IPv6 with XDP. Signed-off-by: Yusuke Suzuki <yusuke-suzuki@cybozu.co.jp>

This commit optimizes the tunneling overhead by only adding the GENEVE option to the TCP-SYN packets, sending the whole connection over the tunnel Signed-off-by: Yusuke Suzuki <yusuke-suzuki@cybozu.co.jp>

This commit checks the expanded packet length when encapsulating the dsr flow with GENEVE header and if it exceeds the device MTU, then returns ICMP Destination Unreachable/Fragmentation needed. Signed-off-by: Yusuke Suzuki <yusuke-suzuki@cybozu.co.jp>

Let the dsr code extract the DSR specific option, the service IP/port, from the Geneve header and integrate with the DSR implementation on the receiver side. Signed-off-by: Yusuke Suzuki <yusuke-suzuki@cybozu.co.jp>

Signed-off-by: Yusuke Suzuki <yusuke-suzuki@cybozu.co.jp>

This commit adds DSR with Geneve dispatch mode. Geneve dispatch mode is compatible with the Geneve tunneling mode, which means that it works with either the direct routing mode or the Geneve tunneling mode. but unfortunately, it doesn't work with vxlan tunnel because there's no space to store service IP/port in the vxlan header. Signed-off-by: Yusuke Suzuki <yusuke-suzuki@cybozu.co.jp>

Signed-off-by: Yusuke Suzuki <yusuke-suzuki@cybozu.co.jp>

ldelossa

LGTM, nice work.

joestringer · 2023-04-04T02:24:49Z

/test

ysksuzuki · 2023-04-04T04:59:14Z

All Green!

julianwiedmann · 2023-04-04T05:58:13Z

@ldelossa was requested for cilium/cli, but for some reason GH didn't resolve that codeowner on his review 🤨.

All reviews in & tests pass, off we go.

joestringer · 2023-04-04T19:02:28Z

@ldelossa was requested for cilium/cli, but for some reason GH didn't resolve that codeowner on his review

Maybe he removed himself from cilium/cli? I don't see him in that github team currently.

ysksuzuki requested review from a team as code owners February 20, 2023 17:08

ysksuzuki requested review from ti-mo and julianwiedmann February 20, 2023 17:08

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 20, 2023

ysksuzuki requested review from nebril and ldelossa February 20, 2023 17:08

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Feb 20, 2023

ysksuzuki marked this pull request as draft February 20, 2023 17:09

julianwiedmann added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Feb 20, 2023

maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Feb 20, 2023

ysksuzuki force-pushed the dsr-geneve-on-22978 branch from 4572ca8 to b9b73b2 Compare February 21, 2023 04:47

julianwiedmann reviewed Feb 21, 2023

View reviewed changes

bpf/lib/nodeport.h Outdated Show resolved Hide resolved

bpf/lib/common.h Outdated Show resolved Hide resolved

bpf/lib/nodeport.h Outdated Show resolved Hide resolved

bpf/bpf_overlay.c Outdated Show resolved Hide resolved

ysksuzuki commented Feb 22, 2023

View reviewed changes

bpf/bpf_host.c Outdated Show resolved Hide resolved

ysksuzuki force-pushed the dsr-geneve-on-22978 branch from 63097a0 to b91913c Compare February 24, 2023 14:58

ysksuzuki force-pushed the dsr-geneve-on-22978 branch from b91913c to c4cbacd Compare March 8, 2023 09:37

julianwiedmann requested changes Mar 8, 2023

View reviewed changes

bpf/lib/common.h Outdated Show resolved Hide resolved

bpf/lib/nodeport.h Outdated Show resolved Hide resolved

bpf/lib/nodeport.h Outdated Show resolved Hide resolved

bpf/lib/encap.h Outdated Show resolved Hide resolved

ysksuzuki force-pushed the dsr-geneve-on-22978 branch 2 times, most recently from 345dd9b to e951684 Compare April 1, 2023 15:19

ysksuzuki force-pushed the dsr-geneve-on-22978 branch from e951684 to a73fb7a Compare April 4, 2023 00:44

ysksuzuki added 8 commits April 4, 2023 09:49

bpf: nodeport: Optimize the GENEVE encapsulation for the TCP flow

ce25a91

This commit optimizes the tunneling overhead by only adding the GENEVE option to the TCP-SYN packets, sending the whole connection over the tunnel Signed-off-by: Yusuke Suzuki <yusuke-suzuki@cybozu.co.jp>

bpf: nodeport: Extract DSR specific option from the Geneve header

297838e

Let the dsr code extract the DSR specific option, the service IP/port, from the Geneve header and integrate with the DSR implementation on the receiver side. Signed-off-by: Yusuke Suzuki <yusuke-suzuki@cybozu.co.jp>

bpf,test: Add complexity-test configs for DSR with Geneve

3d0b091

Signed-off-by: Yusuke Suzuki <yusuke-suzuki@cybozu.co.jp>

test: add test for DSR with Geneve

de1c5ac

Signed-off-by: Yusuke Suzuki <yusuke-suzuki@cybozu.co.jp>

docs: Add description for DSR with Geneve

a73fb7a

Signed-off-by: Yusuke Suzuki <yusuke-suzuki@cybozu.co.jp>

ldelossa approved these changes Apr 4, 2023

View reviewed changes

julianwiedmann added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 4, 2023

squeed merged commit 40d9787 into cilium:master Apr 4, 2023
57 checks passed

julianwiedmann mentioned this pull request Apr 4, 2023

Split --tunnel flag to allow setting a tunneling protocol for a subset of traffic #24561

Merged

gandro mentioned this pull request Apr 4, 2023

chore(deps): update dependency cilium/hubble to v0.11.3 (master) #24703

Merged

1 task

ysksuzuki deleted the dsr-geneve-on-22978 branch April 18, 2023 23:29

julianwiedmann mentioned this pull request Apr 26, 2023

datapath: improve config checks for DSR-GENEVE #25129

Closed

This was referenced May 17, 2023

support DSR in VXLAN tunneling mode #10114

Open

add native tunnel encapsulation support for the XDP Loadbalancer #24422

Merged

julianwiedmann mentioned this pull request Jun 1, 2023

datapath: support Hybrid-DSR mode for Geneve dispatch #25553

Merged

ysksuzuki mentioned this pull request Jun 9, 2023

Request to add ysksuzuki as sig-lb reviewer cilium/community#36

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support DSR with Geneve dispatch #23890

Support DSR with Geneve dispatch #23890

ysksuzuki commented Feb 20, 2023 •

edited

ysksuzuki commented Feb 20, 2023

julianwiedmann commented Feb 20, 2023

julianwiedmann commented Feb 20, 2023

julianwiedmann left a comment

ysksuzuki commented Feb 22, 2023 •

edited

julianwiedmann commented Feb 23, 2023

ysksuzuki commented Feb 23, 2023

julianwiedmann commented Feb 23, 2023

julianwiedmann left a comment

ysksuzuki commented Apr 1, 2023

julianwiedmann commented Apr 3, 2023

ysksuzuki commented Apr 3, 2023

ysksuzuki commented Apr 3, 2023

ldelossa left a comment

joestringer commented Apr 4, 2023

ysksuzuki commented Apr 4, 2023

julianwiedmann commented Apr 4, 2023

joestringer commented Apr 4, 2023

Support DSR with Geneve dispatch #23890

Support DSR with Geneve dispatch #23890

Conversation

ysksuzuki commented Feb 20, 2023 • edited

ysksuzuki commented Feb 20, 2023

julianwiedmann commented Feb 20, 2023

julianwiedmann commented Feb 20, 2023

julianwiedmann left a comment

Choose a reason for hiding this comment

ysksuzuki commented Feb 22, 2023 • edited

julianwiedmann commented Feb 23, 2023

ysksuzuki commented Feb 23, 2023

julianwiedmann commented Feb 23, 2023

julianwiedmann left a comment

Choose a reason for hiding this comment

ysksuzuki commented Apr 1, 2023

julianwiedmann commented Apr 3, 2023

ysksuzuki commented Apr 3, 2023

ysksuzuki commented Apr 3, 2023

ldelossa left a comment

Choose a reason for hiding this comment

joestringer commented Apr 4, 2023

ysksuzuki commented Apr 4, 2023

julianwiedmann commented Apr 4, 2023

joestringer commented Apr 4, 2023

ysksuzuki commented Feb 20, 2023 •

edited

ysksuzuki commented Feb 22, 2023 •

edited