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 IPv6 #24882

Merged
merged 2 commits into from Apr 28, 2023
Merged

datapath: Remove 2005 route table for IPv6 #24882

merged 2 commits into from Apr 28, 2023

Conversation

jschwinger233
Copy link
Member

@jschwinger233 jschwinger233 commented Apr 14, 2023 •
edited

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

Another PR #24807 removes 2005 route table for IPv4.

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

Fix broken IPv6 connectivity from outside to NodePort service when L7 ingress policy applied by removing PROXY_RT route table.

@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 Apr 14, 2023
@jschwinger233 jschwinger233 mentioned this pull request Apr 14, 2023
Merged
@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:55
@jschwinger233 jschwinger233 requested review from a team as code owners April 14, 2023 03:55
christarazi
christarazi reviewed Apr 14, 2023
View reviewed changes
test/k8s/services.go Outdated Show resolved Hide resolved
@jschwinger233 jschwinger233 changed the title Remove 2005 for IPv6 datapath: Remove 2005 for IPv6 Apr 14, 2023
@jschwinger233 jschwinger233 changed the title datapath: Remove 2005 for IPv6 datapath: Remove 2005 route table for IPv6 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.

Out of curiosity, why did you separate the PRs for IPv4 and IPv6?

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

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.

@pchaigno
Copy link
Member

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

This also doesn't describe the user-visible impact. Didn't we initially remove this routing table to fix a bug?

@jschwinger233
Copy link
Member Author

jschwinger233 commented Apr 17, 2023 •
edited

Out of curiosity, why did you separate the PRs for IPv4 and IPv6?

@julianwiedmann and me suspect that we will ship the IPv4-only removal of the 2005 table to 1.13.

And thanks for pointing out!

@jschwinger233 jschwinger233 marked this pull request as draft April 19, 2023 11:11
jschwinger233 added a commit to jschwinger233/cilium that referenced this pull request Apr 24, 2023
@jschwinger233
test: add e2e test for reaching nodeport from outside with L7 policy
56bdbff 
This commit adds e2e test to cover issue cilium#21954.

Test cases for IPv6 are deleted and PR cilium#24882 will take care of them.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
ti-mo pushed a commit that referenced this pull request Apr 24, 2023
@jschwinger233 @ti-mo
test: add e2e test for reaching nodeport from outside with L7 policy
17df079 
This commit adds e2e test to cover issue #21954.

Test cases for IPv6 are deleted and PR #24882 will take care of them.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
julianwiedmann pushed a commit to julianwiedmann/cilium that referenced this pull request Apr 24, 2023
@jschwinger233 @julianwiedmann
test: add e2e test for reaching nodeport from outside with L7 policy
08618e8 
[ upstream commit 17df079 ]

This commit adds e2e test to cover issue cilium#21954.

Test cases for IPv6 are deleted and PR cilium#24882 will take care of them.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@jschwinger233 jschwinger233 marked this pull request as ready for review April 24, 2023 13:47
michi-covalent pushed a commit that referenced this pull request Apr 25, 2023
@jschwinger233 @michi-covalent
test: add e2e test for reaching nodeport from outside with L7 policy
8230cdb 
[ upstream commit 17df079 ]

This commit adds e2e test to cover issue #21954.

Test cases for IPv6 are deleted and PR #24882 will take care of them.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@jschwinger233
Copy link
Member Author

jschwinger233 commented Apr 26, 2023 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.19' failed:

Click to show.

Test Name

K8sAgentPolicyTest Multi-node policy test with L7 policy using connectivity-check to check datapath

Failure Output

FAIL: cannot install connectivity-check

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.16-kernel-4.19/1126/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.19 so I can create one.

Then please upload the Jenkins artifacts to that issue.

@jschwinger233
Copy link
Member Author

/test

@jschwinger233
datapath: remove 2005 route table for ipv6 only
c584b58 
This reverts 3ed62d5 partially and only removes ipv6 2005 route table.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
@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>
@jschwinger233
Copy link
Member Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 27, 2023
@jschwinger233 jschwinger233 mentioned this pull request Apr 28, 2023
Merged
@pchaigno pchaigno merged commit c35ac1c into cilium:main Apr 28, 2023
56 checks passed
jrajahalme pushed a commit to jrajahalme/cilium that referenced this pull request May 22, 2023
@jschwinger233
test: add e2e test for reaching nodeport from outside with L7 policy
7e9fd08 
This commit adds e2e test to cover issue cilium#21954.

Test cases for IPv6 are deleted and PR cilium#24882 will take care of them.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christarazi christarazi christarazi left review comments

@pchaigno pchaigno pchaigno approved these changes

@ti-mo ti-mo ti-mo approved these changes

@aspsk aspsk aspsk approved these changes

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jschwinger233 @pchaigno @ti-mo @christarazi @aspsk