Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cilium: encryption, additional mtu fix for non-default 1500B MTU #10551

Merged
merged 1 commit into from
Mar 17, 2020
Merged

cilium: encryption, additional mtu fix for non-default 1500B MTU #10551

merged 1 commit into from
Mar 17, 2020

Conversation

jrfastab
Copy link
Contributor

@jrfastab jrfastab commented Mar 11, 2020 •
edited by tgraf

We tried to fix the encryption route table in the tunnel mode case here, #10218 but we used the default MTU variable, EthernetMTU to 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 Reviewable

@jrfastab jrfastab requested a review from a team March 11, 2020 21:44
@jrfastab jrfastab requested a review from a team as a code owner March 11, 2020 21:44
@maintainer-s-little-helper
Copy link

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

@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 11, 2020
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Mar 11, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 11, 2020
@jrfastab jrfastab added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. needs-backport/1.7 labels Mar 11, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.7.2 Mar 11, 2020
@jrfastab jrfastab added kind/bug This is a bug in the Cilium logic. area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. and removed dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. labels Mar 11, 2020
@jrfastab
Copy link
Contributor Author

test-me-please

@jrfastab jrfastab added the wip label Mar 11, 2020
@jrfastab jrfastab requested a review from lbernail March 11, 2020 21:50
@jrfastab
Copy link
Contributor Author

test-me-please

@coveralls
Copy link

coveralls commented Mar 11, 2020 •
edited

Coverage Status

Coverage decreased (-0.01%) to 45.628% when pulling 8a71989 on mtu-fixes-fix into 970a259 on master.

joestringer
joestringer reviewed Mar 11, 2020
View reviewed changes
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its the magic number of bytes in the overhead. We should probably calculate it from mtu on the device correctly though.

pkg/mtu/mtu.go Outdated Show resolved Hide resolved
@joestringer joestringer added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Mar 12, 2020
@jrfastab
Copy link
Contributor Author

jrfastab commented Mar 16, 2020 •
edited

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.

@jrfastab
cilium: encryption routing table using invalid mtu when mtu != 1500B
8a71989 
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>
@jrfastab jrfastab removed the wip label Mar 16, 2020
@joestringer
Copy link
Member

test-me-please

@tgraf
Copy link
Member

tgraf commented Mar 17, 2020

Unrelated etcd flake in unit tests

@tgraf tgraf merged commit f03b69c into master Mar 17, 2020
1.8.0 automation moved this from In progress to Merged Mar 17, 2020
@tgraf tgraf deleted the mtu-fixes-fix branch March 17, 2020 10:28
@rolinh rolinh mentioned this pull request Mar 19, 2020
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.2 Mar 19, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.2 Mar 19, 2020
@jrfastab jrfastab mentioned this pull request Mar 19, 2020
Closed
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Backport pending to v1.6 in 1.6.8 Mar 19, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.2 Mar 23, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.2 Mar 23, 2020
@joestringer joestringer removed this from Backport pending to v1.6 in 1.6.8 Mar 25, 2020
@joestringer joestringer added this to Backport pending to v1.6 in 1.6.9 Mar 25, 2020
@jrfastab jrfastab mentioned this pull request Apr 17, 2020
Closed
@joestringer joestringer removed this from Backport pending to v1.6 in 1.6.9 Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

@tgraf tgraf tgraf approved these changes

@lbernail lbernail Awaiting requested review from lbernail

Assignees
No one assigned
Labels
area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.7.2
Backport done to v1.7
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jrfastab @coveralls @joestringer @tgraf @rolinh