cilium: encryption, additional mtu fix for non-default 1500B MTU #10551
Conversation
|
Commit 44035d8f13e2616dc5d2b699ba9392ac307dcc69 does not contain "Signed-off-by". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
|
Release note label not set, please set the appropriate release note. |
|
Release note label not set, please set the appropriate release note. |
|
test-me-please |
|
test-me-please |
test/k8sT/DatapathConfiguration.go
Outdated
| @@ -444,13 +446,23 @@ var _ = Describe("K8sDatapathConfig", func() { | |||
| }) | |||
| }) | |||
|
|
|||
| func testPodConnectivityAcrossNodesMTU(kubectl *helpers.Kubectl) bool { | |||
| result, _ := testPodConnectivityAndReturnIP(kubectl, true, 1, 1422) | |||
There was a problem hiding this comment.
I suspect that depending on where we run these tests (eg GKE), these MTU numbers may vary because of the base MTU in the environment? Were you trying to take that into account here?
(Also, the numbers seem magic to me; it'd be nice to have constant declaration with a brief summary of how the number was calculated so it can be amended if necessary in future)
There was a problem hiding this comment.
Its the magic number of bytes in the overhead. We should probably calculate it from mtu on the device correctly though.
|
tested fix on gke which uses non-standard mtu 1460 and verified route MTUs are set as expected. Verified before patch route MTUs are broken. |
In the fix, "cilium: encryption route table need to account for tunnel headers" we tried to account for tunnel overhead in the encryption routing table (ip r s t 200). But we only fixed the case where mtu is default 1500 if the mtu is anything else we calculate incorrectly. The initial reporter had a MTU 1500B so it resolved their issue but didn't fix the general issue. After this patch we will account for the configured MTU as well as handle the direct routing case correctly by setting MTU to the default route interface MTU. Fixes: 25a890c ("cilium: encryption route table need to account for tunnel headers") Signed-off-by: John Fastabend <john.fastabend@gmail.com>
|
test-me-please |
|
Unrelated etcd flake in unit tests |
We tried to fix the encryption route table in the tunnel mode case here, #10218 but we used the default MTU variable,
EthernetMTUto calculate the newest route MTU. This only works when network facing MTU is the same as EthernetMTU, 1500B in current code base. I must have believed EthernetMTU would be changed to match current MTU, its not.To fix use configured MTU instead of hard coded default MTU. After this we will use the correct MTU even if network facing MTU is 9k (apparently fairly common) or some other value.
This change is