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.

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

Revert and fix ip rules #25350

Merged
merged 17 commits into from
May 23, 2023
Merged

Conversation

NikAleksandrov
Copy link

@NikAleksandrov NikAleksandrov commented May 10, 2023

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
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 <[email protected]>
Update vishvananda/netlink/ so we can make use of fib rule
RTA_PROTOCOL attribute.

Signed-off-by: Nikolay Aleksandrov <[email protected]>
This reverts commit 53fef54.

Signed-off-by: Nikolay Aleksandrov <[email protected]>
This reverts commit ed5114d.

Signed-off-by: Nikolay Aleksandrov <[email protected]>
This reverts commit dbce5f1.

Signed-off-by: Nikolay Aleksandrov <[email protected]>
This reverts commit 9e62a84.

Signed-off-by: Nikolay Aleksandrov <[email protected]>
This reverts commit 5fb791d.

Signed-off-by: Nikolay Aleksandrov <[email protected]>
This reverts commit 0f3e989.

Signed-off-by: Nikolay Aleksandrov <[email protected]>
This reverts commit 3271cb2.

Signed-off-by: Nikolay Aleksandrov <[email protected]>
This reverts commit afdc51f.

Signed-off-by: Nikolay Aleksandrov <[email protected]>
This reverts commit 9b5e74b.

Signed-off-by: Nikolay Aleksandrov <[email protected]>
This reverts commit 2b6d5c4.

Signed-off-by: Nikolay Aleksandrov <[email protected]>
This reverts commit 05593ee.

Signed-off-by: Nikolay Aleksandrov <[email protected]>
This reverts commit a9cad19.

Signed-off-by: Nikolay Aleksandrov <[email protected]>
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 <[email protected]>
This reverts commit 9d60341.

Signed-off-by: Nikolay Aleksandrov <[email protected]>
@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
@jrajahalme
Copy link
Member

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

@jrajahalme jrajahalme removed the release-blocker/1.14 This issue will prevent the release of the next version of Cilium. label Oct 17, 2023
@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
…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 <[email protected]>
Reviewed-by: Sugang Li <[email protected]>
Lint: Lint 🤖 <[email protected]>
Reviewed-by: Prow_Bot_V2 <[email protected]>
Tested-by: Prow_Bot_V2 <[email protected]>
Reviewed-by: Mark St. John <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects/v1.13 This issue affects v1.13 branch 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. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.