Skip to content

Revert and fix ip rules #25350

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
Merged
merged 17 commits into from
May 23, 2023
Merged

Revert and fix ip rules #25350

merged 17 commits into from
May 23, 2023

Conversation

NikAleksandrov
Copy link

@NikAleksandrov NikAleksandrov commented May 10, 2023 •
edited
Loading

This set is a second attempt at reverting the revert PR #24756, which was reverting #24288 and #24577

The upgrade problem mentioned in PR #24756 was due to wrongfully assuming that DeleteRule()
was handling both IPv4 and IPv6. Turns out there was a specific DeleteRuleIPv6() function
which seems unnecessary, so there is 1 additional patch prior to the last revert which changes DeleteRule() and adds
a new family argument, then removes DeleteRuleIPv6(). The problem manifested only when both IPv4 and IPv6 were
enabled due to the IPv6 rule deletion which was actually removing the IPv4 rule.

Note I have removed the "Revert revert" in the patch titles to make checkpatch happy.

The upgrade was tested as:

$ helm install cilium install/cilium/cilium --version 1.13.2 --namespace kube-system --set ipv6.enabled=true

<make sure ip -d rule show has "proto unspec">

$ helm upgrade cilium install/kubernetes/cilium  --set image.override=192.168.122.1:5000/cilium/cilium-dev:latest --set ipv6.enabled=true --namespace kube-system
# upgrade to a local registry with patched cilium image

<make sure ip -d rule show now has "proto kernel" and there is connectivity>

@maintainer-s-little-helper 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 May 10, 2023
@NikAleksandrov NikAleksandrov self-assigned this May 10, 2023
@NikAleksandrov NikAleksandrov force-pushed the revert-and-fix-ip-rules branch 5 times, most recently from f2739c2 to 348bba8 Compare May 10, 2023 10:09
pengbinbin1 and others added 17 commits May 10, 2023 13:09
@pengbinbin1 @NikAleksandrov
fix the comments about Delete
b1517dc 
The endpoint's IP is not released int the function Delete, it is release
in the function EndpointDeleted. This comments will confused the code
reader.

Signed-off-by: pengbinbin1 <pengbiny@163.com>
@NikAleksandrov
vendor: update vishvananda/netlink/
1da125c 
Update vishvananda/netlink/ so we can make use of fib rule
RTA_PROTOCOL attribute.

Signed-off-by: Nikolay Aleksandrov <nikolay@isovalent.com>
@NikAleksandrov
datapath/linux/route: fix CI expectations for rule string format
13dd701 
This reverts commit 53fef54.

Signed-off-by: Nikolay Aleksandrov <nikolay@isovalent.com>
@NikAleksandrov
linux_defaults: add default rt protocol to use for fib rules and routes
279da4f 
This reverts commit 368ec8e.

Signed-off-by: Nikolay Aleksandrov <nikolay@isovalent.com>
@NikAleksandrov
linux_defaults: add local lookup priority to use for fib rules
4024d85 
This reverts commit ed5114d.

Signed-off-by: Nikolay Aleksandrov <nikolay@isovalent.com>
@NikAleksandrov
init.sh: install ip rules and routes with proto kernel
1618f63 
This reverts commit dbce5f1.

Signed-off-by: Nikolay Aleksandrov <nikolay@isovalent.com>
@NikAleksandrov
init.sh: take local lookup rule priority as an argument
24dc4e4 
This reverts commit 9e62a84.

Signed-off-by: Nikolay Aleksandrov <nikolay@isovalent.com>
@NikAleksandrov
datapath/linux/route: add support for rule protocol
21a44da 
This reverts commit 5fb791d.

Signed-off-by: Nikolay Aleksandrov <nikolay@isovalent.com>
@NikAleksandrov
datapath/linux/route: use proto kernel when installing routes
96380d3 
This reverts commit 0f3e989.

Signed-off-by: Nikolay Aleksandrov <nikolay@isovalent.com>
@NikAleksandrov
egressgw: use proto kernel for fib routes and rules
6ceb86d 
This reverts commit 3271cb2.

Signed-off-by: Nikolay Aleksandrov <nikolay@isovalent.com>
@NikAleksandrov
datapath/loader: use proto kernel for ENI fib rules and routes
b47af02 
This reverts commit afdc51f.

Signed-off-by: Nikolay Aleksandrov <nikolay@isovalent.com>
@NikAleksandrov
datapath/linux/routing: use proto kernel for fib routes and rules
76cd5db 
This reverts commit 9b5e74b.

Signed-off-by: Nikolay Aleksandrov <nikolay@isovalent.com>
@NikAleksandrov
datapath/linux/node: use proto kernel for fib rules and routes
5febf4a 
This reverts commit 2b6d5c4.

Signed-off-by: Nikolay Aleksandrov <nikolay@isovalent.com>
@NikAleksandrov
datapath: remove RouteProtocolIPSec and use proto kernel
0b3ad81 
This reverts commit 05593ee.

Signed-off-by: Nikolay Aleksandrov <nikolay@isovalent.com>
@NikAleksandrov
bugtool: dump fib rule protocol
4fea039 
This reverts commit a9cad19.

Signed-off-by: Nikolay Aleksandrov <nikolay@isovalent.com>
@NikAleksandrov
datapath/linux/route: DeleteRule takes family as an argument
dfeb732 
Remove the IPv6-specific DeleteRuleIPv6() and add a family argument to
DeleteRule so we can use the same function for both families.

Signed-off-by: Nikolay Aleksandrov <nikolay@isovalent.com>
@NikAleksandrov
datapath/linux: make sure we have a local rule with proto kernel
5ae2df5 
This reverts commit 9d60341.

Signed-off-by: Nikolay Aleksandrov <nikolay@isovalent.com>
@NikAleksandrov NikAleksandrov force-pushed the revert-and-fix-ip-rules branch from 348bba8 to 5ae2df5 Compare May 10, 2023 10:09
@NikAleksandrov NikAleksandrov marked this pull request as ready for review May 10, 2023 10:09
@NikAleksandrov NikAleksandrov requested a review from a team as a code owner May 10, 2023 10:09
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 15, 2023
@borkmann borkmann added area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels May 15, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 15, 2023
@kamikaze
Copy link

got the same problem with systemd and recommended networkd options don't help :/

@borkmann
Copy link
Member

/test-1.16-4.19

@borkmann
Copy link
Member

/test-1.24-5.4

@borkmann
Copy link
Member

/test-1.25-4.19

@borkmann
Copy link
Member

/test-1.26-net-next

@borkmann
Copy link
Member

/test-runtime

@borkmann
Copy link
Member

(net-next one hit #25605)

@aanm aanm merged commit a63aabc into cilium:main May 23, 2023
@sayboras sayboras mentioned this pull request May 28, 2023
Merged
10 tasks
@sayboras sayboras added backport-pending/1.13 backport/author The backport will be carried out by the author of the PR. and removed backport-pending/1.13 labels May 28, 2023
@julianwiedmann
Copy link
Member

As note for a backport to v1.13

  • we need to check for any additional usage of routing rules in v1.13, and fix those up as well. For instance I did stumble over wireguard.
  • we need to check for upgrade scenarios, so that a v1.14 agent also considers new style of rules/routes. Again see wireguard.

@borkmann borkmann mentioned this pull request May 30, 2023
Closed
2 tasks
@mate4st mate4st mentioned this pull request Aug 3, 2023
Merged
@jrajahalme
Copy link
Member

Removing release blocker 1.14 as this is already in v1.14: a63aabc

@julianwiedmann julianwiedmann added affects/v1.13 This issue affects v1.13 branch and removed needs-backport/1.13 labels Feb 10, 2024
@julianwiedmann
Copy link
Member

I dropped the PR from the 1.13 backport queue for now, let's not set wrong expectations.

@mfoliveira
Copy link

Hi @nebril

nebril added this to Needs backport from main in 1.13.17 last week
nebril removed this from Needs backport from main in 1.13.16 last week
nebril removed this from Needs backport from main in 1.13.17 last week

Apparently, this indicates that attempts to queue this for backporting to 1.13.x are over; is that correct?

If at all possible, could you please describe the concerns? (e.g., CI regressions, regression potential, community help with the backport is needed)

Thank you, and have a great weekend!

skmatti pushed a commit to skmatti/cilium that referenced this pull request Jul 24, 2024
@yasz24
[Backport] Install fib rules and routes with proto kernel to avoid sy…
e40a942 
…stemd messing with them.

Can be dropped on v1.14+

Upstream-PR: cilium#25350

Bug: b/339297320

Change-Id: I340da52623eb5cbc6e571c2df7a48ced80eb0a42
Reviewed-on: https://gke-internal-review.googlesource.com/c/third_party/cilium/+/1016152
Unit-Verified: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com>
Reviewed-by: Sugang Li <sugangli@google.com>
Lint: Lint 🤖 <android-build-ayeaye@system.gserviceaccount.com>
Reviewed-by: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com>
Tested-by: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com>
Reviewed-by: Mark St. John <markstjohn@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brb brb brb approved these changes

@borkmann borkmann borkmann approved these changes

@asauber asauber asauber approved these changes

@aanm aanm aanm approved these changes

@YutaroHayakawa YutaroHayakawa YutaroHayakawa approved these changes

@pchaigno pchaigno Awaiting requested review from pchaigno

Assignees

@NikAleksandrov NikAleksandrov

Labels
affects/v1.13 This issue affects v1.13 branch area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. backport/author The backport will be carried out by the author of the PR. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.