-
Notifications
You must be signed in to change notification settings - Fork 5.5k
upstream: fix subset_lb crash when host configured with no metadata #15171
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
Changes from 1 commit
d4be14d
c5401fc
b46d0da
7d4f55e
01c5a75
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -216,4 +216,26 @@ TEST_P(HttpSubsetLbIntegrationTest, SubsetLoadBalancer) { | |
| runTest(type_b_request_headers_, "b"); | ||
| } | ||
|
|
||
| // Tests subset-compatible load balancer policy with empty metadata | ||
|
asraa marked this conversation as resolved.
Outdated
|
||
| TEST_P(HttpSubsetLbIntegrationTest, SubsetLoadBalancer_null_metadata) { | ||
|
asraa marked this conversation as resolved.
Outdated
|
||
| config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { | ||
| auto* static_resources = bootstrap.mutable_static_resources(); | ||
| auto* cluster = static_resources->mutable_clusters(0); | ||
|
|
||
| // Set single_host_per_subset to be true | ||
| auto* subset_selector = cluster->mutable_lb_subset_config()->mutable_subset_selectors(0); | ||
| subset_selector->set_single_host_per_subset(true); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like this crash is limited to when this option is enabled, could we be more explicit about this in the comments or test name? Maybe even add a test case where this is disabled for completeness sake.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this crash only happens when single_host_per_subset is set to be true. Without it subset_lb.cc:rebuildSingle() will bailout early hence no crash. I will add some comments here to cover this. |
||
|
|
||
| // Clear the metadata for each host | ||
| auto* load_assignment = cluster->mutable_load_assignment(); | ||
| auto* endpoints = load_assignment->mutable_endpoints(0); | ||
| for (uint32_t i = 0; i < num_hosts_; i++) { | ||
| auto* lb_endpoint = endpoints->mutable_lb_endpoints(i); | ||
| lb_endpoint->clear_metadata(); | ||
| } | ||
| }); | ||
|
|
||
| initialize(); | ||
| } | ||
|
asraa marked this conversation as resolved.
|
||
|
|
||
| } // namespace Envoy | ||
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.
Maybe do
to reduce the nesting?
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.
sure, will do.