wireguard: Set wireguard and route MTU to detected MTU #16020

joamaki · 2021-05-05T09:38:53Z

For more optimal packet sizes in large MTU setups, set the
cilium_wg0 MTU based on the detected MTU minus overhead.

Additionally set the route MTU on container side to detected
MTU minus wireguard overhead when wireguard is enabled.

Tested manually with "--mtu 1400", causing cilium_wg0 and
route MTU to be set to 1320.

brb · 2021-05-05T10:19:06Z

pkg/wireguard/agent/agent.go

+	// wireguardOverhead is an approximation for the overhead of wireguard
+	// encapsulation.
+	//
+	// https://github.com/torvalds/linux/blob/master/drivers/net/wireguard/device.c#L262:


Nit: maybe you could pin the URL to a specific tag, e.g. 5.12.

brb · 2021-05-05T10:22:16Z

pkg/wireguard/agent/agent.go

@@ -134,6 +146,12 @@ func (a *Agent) Init() error {
 		return err
 	}

+	linkMTU := mtuConfig.GetDeviceMTU() - wireguardOverhead
+	if err := netlink.LinkSetMTU(link, linkMTU); err != nil {


I think this is not enough. We need to make sure that route MTU in pod netns is set accordingly (MTU minus the WG overhead), grep for GetRouteMTU. Maybe this is exactly the same what you went in:

Should pkg/mtu be refactored so that the overhead computation works for both IPsec and Wireguard?

Yep you're right. The route MTU was set too high. Modified pkg/mtu to handle the wireguard case. That package needs a rewrite soon, but as discussed with @brb this will be addressed in v1.11 anyway, so not worth addressing that now.

joamaki · 2021-05-05T14:41:11Z

test-me-please

joestringer

I briefly glanced through, looks right to me - adjust route MTU by wireguard if that mode is configured, and set the wireguard interface MTU based on native device MTU minus wireguard overhead.

I assume we don't support wireguard + tunneling today (otherwise there'd be some extra changes lower in GetRouteMTU() where the encrypt+ipsec cases are handled).

joamaki · 2021-05-06T07:22:07Z

test-runtime

joamaki · 2021-05-06T07:22:16Z

test-net-next

brb

Looks good, thanks.

One question: have you tested that ping -c1 -s 1412 $REMOTE_POD_IP doesn't get fragmented, while ping -c1 -s 1413 $REMOTE_POD_IP does? -s 1412 because +8 for ICMP hdr.

brb · 2021-05-06T08:05:24Z

pkg/mtu/mtu.go

-	encapEnabled   bool
-	encryptEnabled bool
+	encapEnabled     bool
+	encryptEnabled   bool


As a follow up we should probably s/encryptEnabled/ipsecEnabled.

brb · 2021-05-06T08:10:05Z

assume we don't support wireguard + tunneling today (otherwise there'd be some extra changes lower in GetRouteMTU() where the encrypt+ipsec cases are handled).

@joestringer We do, just that in the case of tunneling pod2pod traffic goes only via the wg tunnel, so it's encapsulated only once (#15716 (comment)).

joamaki · 2021-05-06T12:18:18Z

Looks good, thanks.

One question: have you tested that ping -c1 -s 1412 $REMOTE_POD_IP doesn't get fragmented, while ping -c1 -s 1413 $REMOTE_POD_IP does? -s 1412 because +8 for ICMP hdr.

Yep, fragments as expected. Though there's odd issue with small fragments getting dropped. Not related to this issue though so I'll debug and fix that separately if I can.

EDIT: Figured out the cause. Documented here: #16036.

joamaki · 2021-05-06T12:19:50Z

test-1.21-4.9

joamaki · 2021-05-06T13:33:00Z

test-net-next

For more optimal packet sizes in large MTU setups, set the cilium_wg0 MTU based on the detected MTU minus overhead. Additionally set the route MTU on container side to detected MTU minus wireguard overhead when wireguard is enabled. Tested manually with "--mtu 1400", causing cilium_wg0 and route MTU to be set to 1320. Signed-off-by: Jussi Maki <jussi@isovalent.com>

brb · 2021-05-07T13:03:44Z

test-net-next

joamaki added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label May 5, 2021

joamaki requested a review from brb May 5, 2021 09:38

maintainer-s-little-helper bot assigned brb May 5, 2021

joamaki mentioned this pull request May 5, 2021

Wireguard integration follow-up #15462

Closed

24 tasks

brb reviewed May 5, 2021

View reviewed changes

joamaki force-pushed the jussi/pr/set-wireguard-mtu branch from e57aa86 to 21680e5 Compare May 5, 2021 12:59

joamaki marked this pull request as ready for review May 5, 2021 14:41

joamaki requested review from a team May 5, 2021 14:41

joamaki requested a review from a team as a code owner May 5, 2021 14:41

joamaki requested a review from aanm May 5, 2021 14:41

maintainer-s-little-helper bot assigned aanm May 5, 2021

gandro added the needs-backport/1.10 label May 5, 2021

maintainer-s-little-helper bot added this to Needs backport from master in 1.10.0-rc2 May 5, 2021

joamaki force-pushed the jussi/pr/set-wireguard-mtu branch from 21680e5 to 957f95f Compare May 5, 2021 17:07

joamaki changed the title ~~wireguard: Set wireguard device MTU to detected route MTU~~ wireguard: Set wireguard and route MTU to detected MTU May 5, 2021

aanm approved these changes May 5, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned aanm May 5, 2021

joestringer approved these changes May 5, 2021

View reviewed changes

brb approved these changes May 6, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned brb May 6, 2021

joamaki force-pushed the jussi/pr/set-wireguard-mtu branch from 957f95f to 5951163 Compare May 6, 2021 15:30

brb added the release-blocker/1.10 label May 7, 2021

joamaki added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 10, 2021

ti-mo merged commit 6ae08fd into cilium:master May 10, 2021

glibsm added backport-pending/1.10 and removed needs-backport/1.10 labels May 12, 2021

maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.0-rc2 May 12, 2021

glibsm mentioned this pull request May 12, 2021

v1.10 backports 2021-05-12 #16103

Merged

aanm added backport-done/1.10 and removed backport-pending/1.10 labels May 13, 2021

maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.0-rc2 May 13, 2021

aanm mentioned this pull request May 17, 2021

Prepare for release v1.10.0-rc2 #16167

Merged

aojea mentioned this pull request Jun 4, 2023

CFP: Patch route MTU when other CNI is handling the container veth setup #25872

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wireguard: Set wireguard and route MTU to detected MTU #16020

wireguard: Set wireguard and route MTU to detected MTU #16020

joamaki commented May 5, 2021 •

edited

brb May 5, 2021

joamaki May 5, 2021

brb May 5, 2021

joamaki May 5, 2021

joamaki commented May 5, 2021

joestringer left a comment

joamaki commented May 6, 2021

joamaki commented May 6, 2021

brb left a comment •

edited

brb May 6, 2021

brb commented May 6, 2021

joamaki commented May 6, 2021 •

edited

joamaki commented May 6, 2021

joamaki commented May 6, 2021

brb commented May 7, 2021

wireguard: Set wireguard and route MTU to detected MTU #16020

wireguard: Set wireguard and route MTU to detected MTU #16020

Conversation

joamaki commented May 5, 2021 • edited

brb May 5, 2021

Choose a reason for hiding this comment

joamaki May 5, 2021

Choose a reason for hiding this comment

brb May 5, 2021

Choose a reason for hiding this comment

joamaki May 5, 2021

Choose a reason for hiding this comment

joamaki commented May 5, 2021

joestringer left a comment

Choose a reason for hiding this comment

joamaki commented May 6, 2021

joamaki commented May 6, 2021

brb left a comment • edited

Choose a reason for hiding this comment

brb May 6, 2021

Choose a reason for hiding this comment

brb commented May 6, 2021

joamaki commented May 6, 2021 • edited

joamaki commented May 6, 2021

joamaki commented May 6, 2021

brb commented May 7, 2021

joamaki commented May 5, 2021 •

edited

brb left a comment •

edited

joamaki commented May 6, 2021 •

edited