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

envoy: fix lb backend endpoint calculation #27923

Conversation

mhofstetter
Copy link
Member

Currently, mapping loadbalancing backends to Envoy endpoints contains a bug that the LbEndpoints are kept/appended over the
whole backendMap. Therefore, later endpoints will contain the LBEndpoints of all previous backends.

This commit fixes this by putting the variable lbEndpoints into the right scope (per port).

@mhofstetter mhofstetter added kind/bug This is a bug in the Cilium logic. area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. release-note/bug This PR fixes an issue in a previous release of Cilium. area/servicemesh GH issues or PRs regarding servicemesh labels Sep 4, 2023
@mhofstetter
Copy link
Member Author

/test

@mhofstetter mhofstetter marked this pull request as ready for review September 5, 2023 12:04
@mhofstetter mhofstetter requested a review from a team as a code owner September 5, 2023 12:04
@github-actions
Copy link

github-actions bot commented Oct 6, 2023

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Oct 6, 2023
@mhofstetter mhofstetter removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Oct 16, 2023
@tommyp1ckles
Copy link
Contributor

tommyp1ckles commented Oct 24, 2023

Looks good, sorry for the delay, was there an issue for this bug?

pkg/envoy/ciliumenvoyconfig_test.go Outdated Show resolved Hide resolved
@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Oct 24, 2023
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/envoy-endpoints-lbendpoints branch from eee6047 to 0ba766b Compare October 24, 2023 07:27
@mhofstetter mhofstetter removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 24, 2023
This commit extracts the logic that creates Envoy endpoints
(ClusterLoadAssignments) for LoadBalancing Backends into its
function.

In addition some unit tests were added.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Currently, mapping loadbalancing backends to Envoy endpoints
contains a bug that the LbEndpoints are kept/appended over the
whole backendMap. Therefore, later endpoints will contain the
LBEndpoints of all previous backends.

This commit fixes this by putting the variable `lbEndpoints`
into the right scope (per port).

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/envoy-endpoints-lbendpoints branch from 0ba766b to 044da91 Compare October 24, 2023 07:32
@mhofstetter
Copy link
Member Author

rebased to main without any further changes

@mhofstetter
Copy link
Member Author

mhofstetter commented Oct 24, 2023

Looks good, sorry for the delay, was there an issue for this bug?

no problem. no there isn't a reported issue.

@mhofstetter
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 Oct 24, 2023
@dylandreimerink dylandreimerink merged commit c4079a7 into cilium:main Oct 24, 2023
61 of 62 checks passed
@mhofstetter mhofstetter deleted the pr/mhofstetter/envoy-endpoints-lbendpoints branch October 24, 2023 12:21
@tommyp1ckles
Copy link
Contributor

@mhofstetter This probably needs backports right? (looks like the same issue might exist back to at least v1.13).

@mhofstetter mhofstetter added needs-backport/1.13 needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Oct 25, 2023
@pippolo84 pippolo84 mentioned this pull request Oct 30, 2023
9 tasks
@pippolo84 pippolo84 added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Oct 30, 2023
@pippolo84 pippolo84 mentioned this pull request Oct 30, 2023
6 tasks
@pippolo84 pippolo84 added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 labels Nov 2, 2023
@jibi jibi added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. area/servicemesh GH issues or PRs regarding servicemesh backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
Status: Released
Status: Released
Development

Successfully merging this pull request may close these issues.

5 participants