-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
service cache should handle duplicate endpoints addresses #24202
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely contained change. Thanks!
/test Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed: Click to show.Test Name
Failure Output
If it is a flake and a GitHub issue doesn't already exist to track it, comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes mostly look good! Thanks!
Kubernetes e2e test now passes
Can we add this test to the cilium k8s conformance test suite?
nit: Can you fix the release note?
Fix a bug where Cilium didn't handle correctly
*didn't correctly handle
Also, release notes are user facing, so they should highlight the issue from the user's perspective. I think your original issue title might be better as a release note "fail to forward traffic to custom Endpoint Slices with the same Address on different Slices".
I specifically added an e2e test in Kubernetes to verify this behavior, as this bug was reported internally to me and was only present in Cilium and I wanted to check nobody else has it. kubernetes/kubernetes#114155 I apologize but I don't have more time to add new conformance tests, however, I added a commit in #24174 to run this e2e kubernetes conformance tests, so it will be tested once that merges I spend a lot of time in Kubernetes creating, maintaining and ensuring the kubernetes SIG-network tests covers and test all the known issues and features so the ecosystem is consistent with the behaviors we define in Kubernetes, cilium should run these e2e tests, it should have caught issues like this one or #24174 |
In some corner cases, a group of endpointslices may have the same address duplicate in different slices. Since the Endpoints are cached by address, when aggregating the endpoints, we should merge the content instead of overwriting. Signed-off-by: Antonio Ojea <aojea@google.com>
/test Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed: Click to show.Test Name
Failure Output
If it is a flake and a GitHub issue doesn't already exist to track it, comment Hit #15455 |
/test-1.26-net-next |
Per the contributor ladder document https://github.com/cilium/community/blob/main/CONTRIBUTOR-LADDER.md#organization-member I'd like to request membership in the org for myself, since I plan to contribute regularly, mainly in the areas related to Kubernetes where I'm sig-network and sig-testing member and Kind maintainer. I've already contributed adding more test coverage for the Kubernetes integration to the project - cilium/cilium#25258 - cilium/cilium#24209 fixing some bugs: - cilium/cilium#24202 - cilium/cilium#24174 and helping to triage issues Signed-off-by: Antonio Ojea <antonio.ojea.garcia@gmail.com>
Per the contributor ladder document https://github.com/cilium/community/blob/main/CONTRIBUTOR-LADDER.md#organization-member I'd like to request membership in the org for myself, since I plan to contribute regularly, mainly in the areas related to Kubernetes where I'm sig-network and sig-testing member and Kind maintainer. I've already contributed adding more test coverage for the Kubernetes integration to the project - cilium/cilium#25258 - cilium/cilium#24209 fixing some bugs: - cilium/cilium#24202 - cilium/cilium#24174 and helping to triage issues Signed-off-by: Antonio Ojea <antonio.ojea.garcia@gmail.com>
In some corner cases, a group of endpointslices may have the same address duplicate in different slices.
Since the Endpoints are cached by address, when aggregating the endpoints, we should merge the content instead of overwriting.
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXX
line if the commit addresses a particularGitHub issue.
Fixes: <commit-id>
tag, thenplease add the commit author[s] as reviewer[s] to this issue.
Kubernetes e2e test now passes
Fixes: #22446