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

datapath: Remove 2005 route table for IPv4 #24807

Merged
merged 2 commits into from
Apr 24, 2023
Merged

datapath: Remove 2005 route table for IPv4 #24807

merged 2 commits into from
Apr 24, 2023

Conversation

jschwinger233
Copy link
Member

@jschwinger233 jschwinger233 commented Apr 11, 2023 •
edited by margamanterola

This reverts 3ed62d5 for IPv4 part only, as issue #21954 has been resolved by #24208.

Another PR #24882 removes 2005 route table for IPv6.

Signed-off-by: Zhichuan Liang <gray.isovalent.com>

Fix broken IPv4 connectivity from outside to NodePort service when using L7 ingress policy, by removing PROXY_RT route table.

@maintainer-s-little-helper
Copy link

Commit 03075f6cebd3913a65cd9acd96dda181969b6bc9 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 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-label The author needs to describe the release impact of these changes. labels Apr 11, 2023
@jschwinger233 jschwinger233 mentioned this pull request Apr 11, 2023
Merged
@jschwinger233 jschwinger233 force-pushed the gray/test-removing-2005-with-ipv6 branch from 03075f6 to 043e09f Compare April 11, 2023 10:58
@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 Apr 11, 2023
@jschwinger233 jschwinger233 force-pushed the gray/test-removing-2005-with-ipv6 branch from 043e09f to 09e9639 Compare April 11, 2023 11:30
@julianwiedmann
Copy link
Member

/test

@jschwinger233 jschwinger233 force-pushed the gray/test-removing-2005-with-ipv6 branch from 09e9639 to 0fb1260 Compare April 14, 2023 03:34
@jschwinger233 jschwinger233 changed the title Test removing 2005 with new cilium_host ipv6 Remove 2005 route table for IPv4 Apr 14, 2023
@jschwinger233 jschwinger233 added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Apr 14, 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 Apr 14, 2023
@jschwinger233 jschwinger233 marked this pull request as ready for review April 14, 2023 03:37
@jschwinger233 jschwinger233 requested a review from a team as a code owner April 14, 2023 03:37
@jschwinger233 jschwinger233 force-pushed the gray/test-removing-2005-with-ipv6 branch from 0fb1260 to 92e52a6 Compare April 14, 2023 03:38
@jschwinger233 jschwinger233 mentioned this pull request Apr 14, 2023
Merged
@jschwinger233 jschwinger233 changed the title Remove 2005 route table for IPv4 datapath: Remove 2005 route table for IPv4 Apr 14, 2023
pchaigno
pchaigno approved these changes Apr 16, 2023
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

LGTM.

This PR removes 2005 route table for IPv6, and changes datapath for return IPv6 traffic from L7 proxy.

Same comment as for the IPv6 PR:
Our release notes usually don't have a subject; it is understood to be "this release". Also nit: the comma before "and" is superfluous since both clauses share the same subject.

ti-mo
ti-mo reviewed Apr 17, 2023
View reviewed changes
bpf/init.sh Outdated Show resolved Hide resolved
@jschwinger233 jschwinger233 marked this pull request as draft April 19, 2023 04:14
@julianwiedmann julianwiedmann mentioned this pull request Apr 19, 2023
Merged
6 tasks
@julianwiedmann
Copy link
Member

This reverts 3ed62d5 for IPv4 part only, as issue #21954 has been resolved by #24208.

Just for clarity - for IPv4 we don't require #24208, all that's needed to fix #21954 is the removal of the 2005 route table (ie. this PR).

@julianwiedmann julianwiedmann added needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Apr 19, 2023
@jschwinger233 jschwinger233 force-pushed the gray/test-removing-2005-with-ipv6 branch from 92e52a6 to 897a434 Compare April 19, 2023 11:04
@jschwinger233 jschwinger233 marked this pull request as ready for review April 19, 2023 11:04
@julianwiedmann julianwiedmann added the backport/author The backport will be carried out by the author of the PR. label Apr 24, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.13.3 Apr 24, 2023
@julianwiedmann julianwiedmann mentioned this pull request Apr 24, 2023
Merged
1 task
@michi-covalent
Copy link
Contributor

why does this still have needs-backport/1.13 label? shouldn't it be backport-pending/1.13 instead since #25086 is open? 👀

@michi-covalent michi-covalent moved this from Needs backport from main to Backport pending to v1.13 in 1.13.3 Apr 25, 2023
@michi-covalent michi-covalent added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Apr 25, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.13 to Backport done to v1.13 in 1.13.3 Apr 25, 2023
@jschwinger233 jschwinger233 deleted the gray/test-removing-2005-with-ipv6 branch April 25, 2023 03:50
@margamanterola
Copy link
Member

Regarding the release note, what is the "2005 route table"? Given that this is release-note/bug, we want to give our users a better indication of when they would be affected by this bug and when they wouldn't. With the current description it's very hard.

@jschwinger233
Copy link
Member Author

Regarding the release note, what is the "2005 route table"?

It's the PROXY_RT_TABLE whose id is 2005, we created this route table to let all return traffic from proxy go to cilium_host, and this table caused #21954.

Thanks for reminding me of the bad release note, I'll revise it for better understanding.

jschwinger233 added a commit to jschwinger233/cilium that referenced this pull request Apr 27, 2023
@jschwinger233
test: enable ipv6 test for reaching nodeport from outside
0e88bdc 
Those test cases were temporarily deleted by cilium#24807 to pass CI, and this
commit adds them back.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
pchaigno pushed a commit that referenced this pull request Apr 28, 2023
@jschwinger233 @pchaigno
test: enable ipv6 test for reaching nodeport from outside
c35ac1c 
Those test cases were temporarily deleted by #24807 to pass CI, and this
commit adds them back.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
@julianwiedmann julianwiedmann mentioned this pull request May 8, 2023
Merged
brb added a commit to cilium/cilium-cli that referenced this pull request May 15, 2023
@brb
connectivity: Skip L7 N/S test on < v1.13.3
3cb290c 
The required PR [1] is scheduled to be released with v1.13.2. Until
then, we need to skip the test.

[1]: cilium/cilium#24807

Signed-off-by: Martynas Pumputis <m@lambda.lt>
brb added a commit to cilium/cilium-cli that referenced this pull request May 15, 2023
@brb
connectivity: Skip L7 N/S test on < v1.13.3
d59a51f 
The required PR [1] is scheduled to be released with v1.13.2. Until
then, we need to skip the test.

[1]: cilium/cilium#24807

Signed-off-by: Martynas Pumputis <m@lambda.lt>
brb added a commit to cilium/cilium-cli that referenced this pull request May 15, 2023
@brb
connectivity: Skip L7 N/S test on < v1.13.3
907dfb0 
The required PR [1] is scheduled to be released with v1.13.2. Until
then, we need to skip the test.

[1]: cilium/cilium#24807

Signed-off-by: Martynas Pumputis <m@lambda.lt>
brb added a commit to cilium/cilium-cli that referenced this pull request May 17, 2023
@brb
connectivity: Skip L7 N/S test on < v1.13.3
74ea687 
The required PR [1] is scheduled to be released with v1.13.2. Until
then, we need to skip the test.

[1]: cilium/cilium#24807

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@thorn3r thorn3r mentioned this pull request May 17, 2023
Merged
brb added a commit to cilium/cilium-cli that referenced this pull request May 18, 2023
@brb
connectivity: Skip L7 N/S test on < v1.13.3
9b8fd4f 
The required PR [1] is scheduled to be released with v1.13.2. Until
then, we need to skip the test.

[1]: cilium/cilium#24807

Signed-off-by: Martynas Pumputis <m@lambda.lt>
brb added a commit to cilium/cilium-cli that referenced this pull request May 18, 2023
@brb
connectivity: Skip L7 N/S test on < v1.13.3
5483883 
The required PR [1] is scheduled to be released with v1.13.2. Until
then, we need to skip the test.

[1]: cilium/cilium#24807

Signed-off-by: Martynas Pumputis <m@lambda.lt>
brb added a commit to cilium/cilium-cli that referenced this pull request May 24, 2023
@brb
connectivity: Skip L7 N/S test on < v1.13.3
9c6a520 
The required PR [1] is scheduled to be released with v1.13.2. Until
then, we need to skip the test.

[1]: cilium/cilium#24807

Signed-off-by: Martynas Pumputis <m@lambda.lt>
brb added a commit to cilium/cilium-cli that referenced this pull request May 24, 2023
@brb
connectivity: Skip L7 N/S test on < v1.13.3
12f5799 
The required PR [1] is scheduled to be released with v1.13.2. Until
then, we need to skip the test.

[1]: cilium/cilium#24807

Signed-off-by: Martynas Pumputis <m@lambda.lt>
brb added a commit to cilium/cilium-cli that referenced this pull request May 25, 2023
@brb
connectivity: Skip L7 N/S test on < v1.13.3
fbc7585 
The required PR [1] is scheduled to be released with v1.13.2. Until
then, we need to skip the test.

[1]: cilium/cilium#24807

Signed-off-by: Martynas Pumputis <m@lambda.lt>
michi-covalent pushed a commit to michi-covalent/cilium that referenced this pull request May 30, 2023
@brb @michi-covalent
connectivity: Skip L7 N/S test on < v1.13.3
68bdfa1 
The required PR [1] is scheduled to be released with v1.13.2. Until
then, we need to skip the test.

[1]: cilium#24807

Signed-off-by: Martynas Pumputis <m@lambda.lt>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ti-mo ti-mo ti-mo left review comments

@pchaigno pchaigno pchaigno approved these changes

@christarazi christarazi christarazi approved these changes

@julianwiedmann julianwiedmann julianwiedmann approved these changes

@jibi jibi Awaiting requested review from jibi jibi is a code owner automatically assigned from cilium/sig-lb

Assignees
No one assigned
Labels
backport/author The backport will be carried out by the author of the PR. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.13.3
Backport done to v1.13
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jschwinger233 @julianwiedmann @michi-covalent @margamanterola @pchaigno @ti-mo @christarazi