Skip to content

Throwing an exception in case the sum of weights is over uint32_t max value #10578

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

Merged
mattklein123 merged 10 commits into from
Apr 8, 2020
Merged

Throwing an exception in case the sum of weights is over uint32_t max value #10578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
94737cc
Throwing an exception in case the sum of weights is over uint32_t max…
adisuissa Mar 30, 2020
ef56589
Fixing exception text, and adding tests
adisuissa Apr 1, 2020
358920b
Fixing format
adisuissa Apr 2, 2020
2fa1d19
Fixing error messages
adisuissa Apr 3, 2020
0abf123
Adding the fixed tests
adisuissa Apr 3, 2020
64f85a8
Updating the proto description to describe the sum limit
adisuissa Apr 4, 2020
bf98761
Updating the proto description to describe the sum limit (generated_a…
adisuissa Apr 4, 2020
e9bba69
merging upstream/master
adisuissa Apr 8, 2020
6cca6ec
Merge remote-tracking branch 'upstream/master' into fix_oss_fuzz_bug_…
adisuissa Apr 8, 2020
79beb32
Merge remote-tracking branch 'upstream/master' into fix_oss_fuzz_bug_…
adisuissa Apr 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions api/envoy/api/v2/endpoint/endpoint_components.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ message LbEndpoint {
// percentage of traffic for the endpoint. This percentage is then further
// weighted by the endpoint's locality's load balancing weight from
// LocalityLbEndpoints. If unspecified, each host is presumed to have equal
// weight in a locality.
// weight in a locality. The sum of the weights of all endpoints in the
// endpoint's locality must not exceed uint32_t maximal value (4294967295).
google.protobuf.UInt32Value load_balancing_weight = 4 [(validate.rules).uint32 = {gte: 1}];
}

Expand All @@ -116,7 +117,8 @@ message LocalityLbEndpoints {
// Optional: Per priority/region/zone/sub_zone weight; at least 1. The load
// balancing weight for a locality is divided by the sum of the weights of all
// localities at the same priority level to produce the effective percentage
// of traffic for the locality.
// of traffic for the locality. The sum of the weights of all localities at
// the same priority level must not exceed uint32_t maximal value (4294967295).
//
// Locality weights are only considered when :ref:`locality weighted load
// balancing <arch_overview_load_balancing_locality_weighted_lb>` is
Expand Down
6 changes: 4 additions & 2 deletions api/envoy/config/endpoint/v3/endpoint_components.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,8 @@ message LbEndpoint {
// percentage of traffic for the endpoint. This percentage is then further
// weighted by the endpoint's locality's load balancing weight from
// LocalityLbEndpoints. If unspecified, each host is presumed to have equal
// weight in a locality.
// weight in a locality. The sum of the weights of all endpoints in the
// endpoint's locality must not exceed uint32_t maximal value (4294967295).
google.protobuf.UInt32Value load_balancing_weight = 4 [(validate.rules).uint32 = {gte: 1}];
}

Expand All @@ -125,7 +126,8 @@ message LocalityLbEndpoints {
// Optional: Per priority/region/zone/sub_zone weight; at least 1. The load
// balancing weight for a locality is divided by the sum of the weights of all
// localities at the same priority level to produce the effective percentage
// of traffic for the locality.
// of traffic for the locality. The sum of the weights of all localities at
// the same priority level must not exceed uint32_t maximal value (4294967295).
//
// Locality weights are only considered when :ref:`locality weighted load
// balancing <arch_overview_load_balancing_locality_weighted_lb>` is
Expand Down
6 changes: 4 additions & 2 deletions generated_api_shadow/envoy/api/v2/endpoint/endpoint_components.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions generated_api_shadow/envoy/config/endpoint/v3/endpoint_components.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 16 additions & 2 deletions source/common/upstream/thread_aware_lb_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,16 @@ namespace {
void normalizeHostWeights(const HostVector& hosts, double normalized_locality_weight,
NormalizedHostWeightVector& normalized_host_weights,
double& min_normalized_weight, double& max_normalized_weight) {
uint32_t sum = 0;
// sum should be at most uint32_t max value, so we can validate it by accumulating into unit64_t
// and making sure there was no overflow
uint64_t sum = 0;
for (const auto& host : hosts) {
sum += host->weight();
if (sum > std::numeric_limits<uint32_t>::max()) {
Copy link
Contributor

@snowp snowp Mar 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a fine way to check for overflow (the other one I could think of would be if (sum + host->weight() >= sum) with sum being uint32_t).

Can you make sure both this and the other exception has coverage and if not add a unit test?

Copy link
Contributor Author

@adisuissa adisuissa Apr 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @snowp !
I agree that we can check if sum is increasing, but I think the current option clarifies that we check for an overflow.

throw EnvoyException(
fmt::format("The sum of weights of all upstream hosts in a locality exceeds {}",
std::numeric_limits<uint32_t>::max()));
}
}

for (const auto& host : hosts) {
Expand All @@ -31,9 +38,16 @@ void normalizeLocalityWeights(const HostsPerLocality& hosts_per_locality,
double& min_normalized_weight, double& max_normalized_weight) {
ASSERT(locality_weights.size() == hosts_per_locality.get().size());

uint32_t sum = 0;
// sum should be at most uint32_t max value, so we can validate it by accumulating into unit64_t
// and making sure there was no overflow
uint64_t sum = 0;
for (const auto weight : locality_weights) {
sum += weight;
if (sum > std::numeric_limits<uint32_t>::max()) {
throw EnvoyException(
fmt::format("The sum of weights of all localities at the same priority exceeds {}",
std::numeric_limits<uint32_t>::max()));
}
}

// Locality weights (unlike host weights) may be 0. If _all_ locality weights were 0, bail out.
Expand Down
185 changes: 185 additions & 0 deletions test/server/configuration_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,191 @@ TEST_F(ConfigurationImplTest, AdminSocketOptions) {
EXPECT_EQ(detail->name_, Envoy::Network::SocketOptionName(4, 5, "4/5"));
}

TEST_F(ConfigurationImplTest, ExceedLoadBalancerHostWeightsLimit) {
const std::string json = R"EOF(
{
"static_resources": {
"listeners" : [],
"clusters": [
{
"name": "test_cluster",
"type": "static",
"connect_timeout": "0.01s",
"per_connection_buffer_limit_bytes": 8192,
"lb_policy": "RING_HASH",
"load_assignment": {
"cluster_name": "load_test_cluster",
"endpoints": [
{
"priority": 93
},
{
"locality": {
"zone": "zone1"
},
"lb_endpoints": [
{
"endpoint": {
"address": {
"pipe": {
"path": "path/to/pipe"
}
}
},
"health_status": "TIMEOUT",
"load_balancing_weight": {
"value": 4294967295
}
},
{
"endpoint": {
"address": {
"pipe": {
"path": "path/to/pipe2"
}
}
},
"health_status": "TIMEOUT",
"load_balancing_weight": {
"value": 1
}
}
],
"load_balancing_weight": {
"value": 122
}
}
]
}
}
]
},
"admin": {
"access_log_path": "/dev/null",
"address": {
"socket_address": {
"address": "1.2.3.4",
"port_value": 5678
}
}
}
}
)EOF";

auto bootstrap = Upstream::parseBootstrapFromV2Json(json);

MainImpl config;
EXPECT_THROW_WITH_MESSAGE(
config.initialize(bootstrap, server_, cluster_manager_factory_), EnvoyException,
"The sum of weights of all upstream hosts in a locality exceeds 4294967295");
}

TEST_F(ConfigurationImplTest, ExceedLoadBalancerLocalityWeightsLimit) {
const std::string json = R"EOF(
{
"static_resources": {
"listeners" : [],
"clusters": [
{
"name": "test_cluster",
"type": "static",
"connect_timeout": "0.01s",
"per_connection_buffer_limit_bytes": 8192,
"lb_policy": "RING_HASH",
"load_assignment": {
"cluster_name": "load_test_cluster",
"endpoints": [
{
"priority": 93
},
{
"locality": {
"zone": "zone1"
},
"lb_endpoints": [
{
"endpoint": {
"address": {
"pipe": {
"path": "path/to/pipe"
}
}
},
"health_status": "TIMEOUT",
"load_balancing_weight": {
"value": 7
}
}
],
"load_balancing_weight": {
"value": 4294967295
}
},
{
"locality": {
"region": "domains",
"sub_zone": "sub_zone1"
},
"lb_endpoints": [
{
"endpoint": {
"address": {
"pipe": {
"path": "path/to/pipe"
}
}
},
"health_status": "TIMEOUT",
"load_balancing_weight": {
"value": 8
}
}
],
"load_balancing_weight": {
"value": 2
}
}
]
},
"lb_subset_config": {
"fallback_policy": "ANY_ENDPOINT",
"subset_selectors": {
"keys": [
"x"
]
},
"locality_weight_aware": "true"
},
"common_lb_config": {
"healthy_panic_threshold": {
"value": 0.8
},
"locality_weighted_lb_config": {
}
}
}
]
},
"admin": {
"access_log_path": "/dev/null",
"address": {
"socket_address": {
"address": "1.2.3.4",
"port_value": 5678
}
}
}
}
)EOF";

auto bootstrap = Upstream::parseBootstrapFromV2Json(json);

MainImpl config;
EXPECT_THROW_WITH_MESSAGE(
config.initialize(bootstrap, server_, cluster_manager_factory_), EnvoyException,
"The sum of weights of all localities at the same priority exceeds 4294967295");
}

} // namespace
} // namespace Configuration
} // namespace Server
Expand Down
Loading