Skip to content

v1.6 backport 2020-04-17 #11044

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
Closed
wants to merge 2 commits into from
Closed

v1.6 backport 2020-04-17 #11044

wants to merge 2 commits into from

Conversation

jrfastab
Copy link
Contributor

Note: Only IPv4 support added from #9649 IPv6 is not included here, we can debate if we want fib IPv6 support backported somewhere else but its strictly speaking not a fix. This was really an error on my side in original PR to mix a fix with a feature.

Note: Scripts broke on this set of backports so did this by hand comparing manual and v1.6

Once this PR is merged, you can update the PR labels via:

$ for pr in 9649 10218 10268 10551; do contrib/backporting/set-labels.py $pr done 1.6;

jrfastab added 2 commits April 17, 2020 11:13
@jrfastab
cilium: encryption, delete in xfrm rules if encryption toggled
40887a8 
[ upstream commit 4d4fed5 ]

We do not currently remove the 'dir in' encryption rules when encryption
is disabled. The original thinking is we could simply leave these xfrm
policy/state around because they would only ever be triggered if the
datapath marked packets for encryption which it wouldn't because encryption
was disabled. Then if encryption was ever (re)enabled the rules would
already be there. But, subtle detail if the cilium_host IP changes
after encryption is disabled then it (re)enabled with a new IP the
'dir in' state/policy in the xfrm table will no longer be valid. And
if the new cilium host IP is in the same CIDR we can end up with
a collision in the table and possible use old out of date rules.

Fix by removing any 'dir in' rules when encryption is disabled.

Signed-off-by: John Fastabend <john.fastabend@gmail.com>
@jrfastab
cilium: set mtu on tunnel device
2ce504a 
[ upstream commit 6ce5562 ]

Currently the tunnel device MTU will not be set. If the underlying device
is running some other mtu size this creates a case where mtu on tunnel
device is not in-sync. For example on system with 9000 mtu,

2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 9001 qdisc mq state UP mode DEFAULT group default qlen 1000
    link/ether 00:01:02:03:04:05 brd ff:ff:ff:ff:ff:ff
7: cilium_net@cilium_host: <BROADCAST,MULTICAST,NOARP,UP,LOWER_UP> mtu 9001 qdisc noqueue state UP mode DEFAULT group default qlen 1000
    link/ether 56:92:81:65:04:b6 brd ff:ff:ff:ff:ff:ff
8: cilium_host@cilium_net: <BROADCAST,MULTICAST,NOARP,UP,LOWER_UP> mtu 9001 qdisc noqueue state UP mode DEFAULT group default qlen 1000
    link/ether f6:9f:7e:fd:be:4a brd ff:ff:ff:ff:ff:ff
9: cilium_vxlan: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
    link/ether de:d3:72:2a:69:8a brd ff:ff:ff:ff:ff:ff

Or going the other way with network facing interface <1500 my GKE setup
is the following,

2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1460 qdisc fq_codel state UP mode DEFAULT group default qlen 1000
    link/ether 42:01:0a:8a:00:19 brd ff:ff:ff:ff:ff:ff
13: cilium_net@cilium_host: <BROADCAST,MULTICAST,NOARP,UP,LOWER_UP> mtu 1460 qdisc noqueue state UP mode DEFAULT group default qlen 1000
    link/ether 82:80:8c:7a:36:4a brd ff:ff:ff:ff:ff:ff
14: cilium_host@cilium_net: <BROADCAST,MULTICAST,NOARP,UP,LOWER_UP> mtu 1460 qdisc noqueue state UP mode DEFAULT group default qlen 1000
    link/ether ca:98:07:56:a1:2f brd ff:ff:ff:ff:ff:ff
15: cilium_vxlan: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
    link/ether 52:a7:67:e4:53:e4 brd ff:ff:ff:ff:ff:ff

Sending 1500 MTU on a 1460 interface doesn't make sense. But the routes
configured in the main table will help,

$ kubectl exec -ti -n cilium cilium-cwcbc -- ip r s
default via 10.138.0.1 dev eth0 proto dhcp src 10.138.0.25 metric 100
10.36.0.0/24 via 10.36.1.3 dev cilium_host src 10.36.1.3 mtu 1333
10.36.1.0/24 via 10.36.1.3 dev cilium_host src 10.36.1.3 mtu 1333
10.36.1.3 dev cilium_host scope link
10.138.0.1 dev eth0 proto dhcp scope link src 10.138.0.25 metric 100
169.254.123.0/24 dev docker0 proto kernel scope link src 169.254.123.1 linkdown

Still it would be best if the tunnel interface uses the same mtu size as
cilium_{host|net}.

Signed-off-by: John Fastabend <john.fastabend@gmail.com>
@jrfastab jrfastab requested a review from a team April 17, 2020 19:06
@jrfastab jrfastab requested a review from a team as a code owner April 17, 2020 19:06
@jrfastab jrfastab requested a review from a team April 17, 2020 19:06
@jrfastab jrfastab requested review from a team as code owners April 17, 2020 19:06
@jrfastab jrfastab requested a review from a team April 17, 2020 19:06
@jrfastab jrfastab requested review from a team as code owners April 17, 2020 19:06
@jrfastab jrfastab requested a review from a team April 17, 2020 19:06
@jrfastab jrfastab requested a review from a team as a code owner April 17, 2020 19:06
@jrfastab jrfastab requested a review from a team April 17, 2020 19:06
@jrfastab jrfastab requested review from a team as code owners April 17, 2020 19:06
@jrfastab jrfastab requested a review from a team April 17, 2020 19:06
@jrfastab jrfastab requested review from a team as code owners April 17, 2020 19:06
@maintainer-s-little-helper
Copy link

Commits e623325, 26e53d5, 1a5753d, 60d3051 do 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

@jrfastab jrfastab changed the base branch from master to v1.6 April 17, 2020 19:06
@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note labels Apr 17, 2020
@jrfastab
Copy link
Contributor Author

test-me-please

1 similar comment
@jrfastab
Copy link
Contributor Author

test-me-please

@joestringer
Copy link
Member

FYI gke/ with kernel runs are not supported on v1.6 branch so those should be fine to ignore.

@jrfastab jrfastab force-pushed the pr/v1.6-backport-2020-04-17 branch from 5fee678 to 39e7f0f Compare April 18, 2020 04:41
@jrfastab
Copy link
Contributor Author

bisecting...

@jrfastab
Copy link
Contributor Author

test-me-please

@jrfastab jrfastab added the wip label Apr 18, 2020
@jrfastab jrfastab force-pushed the pr/v1.6-backport-2020-04-17 branch from 39e7f0f to afec4d5 Compare April 20, 2020 14:27
@jrfastab
Copy link
Contributor Author

test-me-please

@jrfastab jrfastab force-pushed the pr/v1.6-backport-2020-04-17 branch from afec4d5 to 48e8543 Compare April 20, 2020 17:14
@jrfastab
Copy link
Contributor Author

test-me-please

@jrfastab jrfastab force-pushed the pr/v1.6-backport-2020-04-17 branch from 48e8543 to 2ce504a Compare April 21, 2020 21:29
@jrfastab
Copy link
Contributor Author

test-me-please

@aanm
Copy link
Member

aanm commented Apr 23, 2020

this PR has been marked as a draft PR since it had a WIP label. Please click in "Ready for review" [below vvv ] once the PR is ready to be reviewed. CI will still run for draft PRs.

@aanm aanm marked this pull request as draft April 23, 2020 10:08
@aanm aanm removed the wip label Apr 23, 2020
This was referenced May 12, 2020
Closed
Merged
Merged
@stale
Copy link

stale bot commented May 23, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label May 23, 2020
@gandro gandro added pinned These issues are not marked stale by our issue bot. and removed stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. labels May 26, 2020
@gandro
Copy link
Member

gandro commented May 26, 2020

I don't think we should close this PR as stale, since the backports are still marked as pending

@joestringer
Copy link
Member

@gandro keeping it open doesn't necessarily solve the problem at hand ;-)

@jrfastab have you heard any reports/concerns on this recently? Is v1.7 better on this front? Given that v1.8 is right around the corner I wonder whether we should simply drop these backports assuming that users will get better encryption behaviour by upgrading to a newer release.

@jrfastab
Copy link
Contributor Author

The only user I originally started this for was able to move to v1.7. I don't see any reason to keep them open at this time and its probably not worth it to pull in more patches.

@joestringer joestringer removed the pinned These issues are not marked stale by our issue bot. label Jun 3, 2020
@joestringer joestringer closed this Jun 3, 2020
@joestringer
Copy link
Member

In the off chance anyone wants to track this, #10741 might be another fix to pull in as well.

@tklauser tklauser deleted the pr/v1.6-backport-2020-04-17 branch April 21, 2021 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
kind/backports This PR provides functionality previously merged into master.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jrfastab @joestringer @aanm @gandro