-
Notifications
You must be signed in to change notification settings - Fork 631
Backend traffic distribution fixes #13005
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
Conversation
Signed-off-by: Yossi Mesika <[email protected]>
Signed-off-by: Yossi Mesika <[email protected]>
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.
Pull request overview
This PR fixes a critical bug where the TrafficDistribution field was not being compared in the Equals() methods of BackendObjectIR and EndpointsForBackend. This caused backends with different traffic distribution policies (e.g., "Any" vs "PreferNetwork") to be incorrectly treated as equal, leading to stale configurations when traffic distribution changed. Additionally, the PR renames the traffic distribution value from PreferSameNetwork to PreferNetwork to align with Istio's upstream naming convention.
Key changes:
- Added
TrafficDistributioncomparison toBackendObjectIR.Equals()andEndpointsForBackend.Equals()methods - Added
TrafficDistributionfield toEndpointsForBackend.EmptyCopy()method - Renamed
TrafficDistributionPreferSameNetworkconstant toTrafficDistributionPreferNetwork - Updated all references throughout the codebase to use
PreferNetworkinstead ofPreferSameNetwork
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/pluginsdk/ir/model.go | Added TrafficDistribution to EmptyCopy() and Equals() methods for EndpointsForBackend; removed TODO comment |
| pkg/pluginsdk/ir/backend.go | Added TrafficDistribution comparison to BackendObjectIR.Equals() method; removed TODO comment |
| pkg/pluginsdk/ir/backend_test.go | Added comprehensive unit tests for BackendObjectIR.Equals() covering various traffic distribution scenarios |
| internal/kgateway/wellknown/traffic_dist.go | Renamed constant and updated parsing logic from PreferSameNetwork to PreferNetwork |
| internal/kgateway/setup/testdata/traffic_distribution/svc-prefer-same-network.yaml | Updated annotation value from PreferSameNetwork to PreferNetwork |
| internal/kgateway/setup/testdata/traffic_distribution/se-prefer-same-network.yaml | Updated annotation values and comments from PreferSameNetwork to PreferNetwork |
| internal/kgateway/krtcollections/endpoints_test.go | Updated test to use renamed TrafficDistributionPreferNetwork constant |
| internal/kgateway/endpoints/prioritize.go | Updated case statement to use renamed TrafficDistributionPreferNetwork constant |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
|
|
||
| func createTestBackendObjectIR(trafficDist wellknown.TrafficDistribution) BackendObjectIR { |
Copilot
AI
Dec 1, 2025
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.
The new test TestBackendObjectIREquals doesn't test scenarios where other fields differ (e.g., different Port, Namespace, or Name). While testing TrafficDistribution is good, consider adding test cases that verify the method correctly handles differences in other key fields to ensure comprehensive coverage of the equality logic.
|
|
||
| func (c EndpointsForBackend) Equals(in EndpointsForBackend) bool { | ||
| return c.UpstreamResourceName == in.UpstreamResourceName && c.ClusterName == in.ClusterName && c.Port == in.Port && c.LbEpsEqualityHash == in.LbEpsEqualityHash && c.Hostname == in.Hostname | ||
| return c.UpstreamResourceName == in.UpstreamResourceName && c.ClusterName == in.ClusterName && c.Port == in.Port && c.LbEpsEqualityHash == in.LbEpsEqualityHash && c.Hostname == in.Hostname && c.TrafficDistribution == in.TrafficDistribution |
Copilot
AI
Dec 1, 2025
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.
While EndpointsForBackend.Equals() now correctly compares TrafficDistribution, there are no unit tests that directly verify the Equals() method behavior with different TrafficDistribution values. The existing test TestEndpointsForUpstreamWithDifferentTrafficDistributionButSameEndpoints only verifies hash differences. Consider adding a test similar to TestBackendObjectIREquals that directly calls EndpointsForBackend.Equals() with different traffic distributions to ensure the equality logic is properly validated.
Signed-off-by: Yossi Mesika <[email protected]>
Signed-off-by: Yossi Mesika <[email protected]>
Signed-off-by: Yossi Mesika <[email protected]>
Signed-off-by: Yossi Mesika <[email protected]>
Description
The BackendObjectIR.Equals() method was missing comparison of the TrafficDistribution field, so backends with different traffic distribution values (like "Any" vs "PreferNetwork") were incorrectly considered equal. This could lead to stale configurations being reused when traffic distribution policies changed. The fix adds c.TrafficDistribution == in.TrafficDistribution to the equality check.
Also changed
PreferSameNetworktoPreferNetworkas this is the expected value from Istio upstream. There is no constant for that there.Fixes #13004
Change Type
/kind fix
Changelog
Additional Notes