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

router: deprecating total_weight #23170

Merged
merged 5 commits into from
Sep 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 8 additions & 5 deletions api/envoy/config/route/v3/route_components.proto
Original file line number Diff line number Diff line change
Expand Up @@ -386,10 +386,10 @@ message WeightedCluster {
(udpa.annotations.field_migrate).oneof_promotion = "cluster_specifier"
];

// An integer between 0 and :ref:`total_weight
// <envoy_v3_api_field_config.route.v3.WeightedCluster.total_weight>`. When a request matches the route,
// the choice of an upstream cluster is determined by its weight. The sum of weights across all
// entries in the clusters array must add up to the total_weight, if total_weight is greater than 0.
// The weight of the cluster. This value is relative to the other clusters'
// weights. When a request matches the route, the choice of an upstream cluster
// is determined by its weight. The sum of weights across all
// entries in the clusters array must be greater than 0.
google.protobuf.UInt32Value weight = 2;

// Optional endpoint metadata match criteria used by the subset load balancer. Only endpoints in
Expand Down Expand Up @@ -459,7 +459,10 @@ message WeightedCluster {

// Specifies the total weight across all clusters. The sum of all cluster weights must equal this
// value, if this is greater than 0.
google.protobuf.UInt32Value total_weight = 3;
// This field is now deprecated, and the client will use the sum of all
// cluster weights. It is up to the management server to supply the correct weights.
google.protobuf.UInt32Value total_weight = 3
[deprecated = true, (envoy.annotations.deprecated_at_minor_version) = "3.0"];

// Specifies the runtime key prefix that should be used to construct the
// runtime keys associated with each cluster. When the ``runtime_key_prefix`` is
Expand Down
5 changes: 5 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -233,3 +233,8 @@ deprecated:
change: |
deprecated :ref:`append <envoy_v3_api_field_config.core.v3.HeaderValueOption.append>` and please use
:ref:`append_action <envoy_v3_api_field_config.core.v3.HeaderValueOption.append_action>` first.
- area: router
change: |
deprecated :ref:`total weight <envoy_v3_api_field_config.route.v3.WeightedCluster.total_weight>` for weighted
clusters. The sum of the :ref:`clusters' weights <envoy_v3_api_field_config.route.v3.WeightedCluster.ClusterWeight.weight>`
will be used as the total weight.
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,13 @@ to each upstream cluster.
weight: 34


By default, the weights must sum to exactly 100. In the V2 API, the
The sum of the weights needs to be greater than 0. In the V2 API, the
:ref:`total weight <envoy_v3_api_field_config.route.v3.WeightedCluster.total_weight>` defaults to 100, but can
be modified to allow finer granularity.
be modified to allow finer granularity. The
:ref:`total weight <envoy_v3_api_field_config.route.v3.WeightedCluster.total_weight>` is now deprecated,
and the relative value of each
:ref:`cluster weight <envoy_v3_api_field_config.route.v3.WeightedCluster.ClusterWeight.weight>` compared to
the sum of all cluster weights will be used.

The weights assigned to each cluster can be dynamically adjusted using the
following runtime variables: ``routing.traffic_split.helloworld.helloworld_v1``,
Expand Down
8 changes: 3 additions & 5 deletions source/common/router/config_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -534,8 +534,6 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost,
rate_limit_policy_(route.route().rate_limits(), validator),
priority_(ConfigUtility::parsePriority(route.route().priority())),
config_headers_(Http::HeaderUtility::buildHeaderDataVector(route.match().headers())),
total_cluster_weight_(
PROTOBUF_GET_WRAPPED_OR_DEFAULT(route.route().weighted_clusters(), total_weight, 0UL)),
request_headers_parser_(HeaderParser::configure(route.request_headers_to_add(),
route.request_headers_to_remove())),
response_headers_parser_(HeaderParser::configure(route.response_headers_to_add(),
Expand Down Expand Up @@ -593,9 +591,9 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost,
total_weight += weighted_clusters_.back()->clusterWeight();
}

if (total_cluster_weight_ > 0 && total_weight != total_cluster_weight_) {
throw EnvoyException(fmt::format("Sum of weights in the weighted_cluster should add up to {}",
total_cluster_weight_));
// Reject the config if the total_weight of all clusters is 0.
if (total_weight == 0) {
throw EnvoyException("Sum of weights in the weighted_cluster must be greater than 0.");
}

total_cluster_weight_ = total_weight;
Expand Down
36 changes: 21 additions & 15 deletions test/common/router/config_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3270,7 +3270,6 @@ TEST_F(RouteMatcherTest, WeightedClusterHeader) {
- match: { prefix: "/" }
route:
weighted_clusters:
total_weight: 100
clusters:
- cluster_header: some_header
weight: 30
Expand Down Expand Up @@ -3303,7 +3302,6 @@ TEST_F(RouteMatcherTest, WeightedClusterWithProvidedRandomValue) {
- match: { prefix: "/" }
route:
weighted_clusters:
total_weight: 80
header_name: "x_random_value"
clusters:
- name: cluster1
Expand Down Expand Up @@ -5848,7 +5846,6 @@ TEST_F(RouteMatcherTest, WeightedClusters) {
weight: 3000
- name: cluster3
weight: 5000
total_weight: 10000
- name: www3
domains: ["www3.lyft.com"]
routes:
Expand Down Expand Up @@ -5877,7 +5874,6 @@ TEST_F(RouteMatcherTest, WeightedClusters) {
weight: 3000
- name: cluster3
weight: 5000
total_weight: 10000
)EOF";

BazFactory baz_factory;
Expand Down Expand Up @@ -6145,7 +6141,7 @@ TEST_F(RouteMatcherTest, WeightedClustersEmptyClustersList) {
EnvoyException);
}

TEST_F(RouteMatcherTest, WeightedClustersSumOFWeightsNotEqualToMax) {
TEST_F(RouteMatcherTest, WeightedClustersZeroSumOfWeights) {
const std::string yaml = R"EOF(
virtual_hosts:
- name: www2
Expand All @@ -6154,19 +6150,33 @@ TEST_F(RouteMatcherTest, WeightedClustersSumOFWeightsNotEqualToMax) {
- match: { prefix: "/" }
route:
weighted_clusters:
total_weight: 99
clusters:
- name: cluster1
weight: 3
- name: cluster2
weight: 3
- name: cluster3
weight: 3
)EOF";

EXPECT_THROW_WITH_MESSAGE(
TestConfigImpl(parseRouteConfigurationFromYaml(yaml), factory_context_, true), EnvoyException,
"Sum of weights in the weighted_cluster should add up to 99");
"Field 'weight' is missing in: name: \"cluster1\"\n");

const std::string yaml2 = R"EOF(
virtual_hosts:
- name: www2
domains: ["www.lyft.com"]
routes:
- match: { prefix: "/" }
route:
weighted_clusters:
clusters:
- name: cluster1
weight: 0
- name: cluster2
weight: 0
)EOF";

EXPECT_THROW_WITH_MESSAGE(
TestConfigImpl(parseRouteConfigurationFromYaml(yaml2), factory_context_, true),
EnvoyException, "Sum of weights in the weighted_cluster must be greater than 0.");
}

TEST_F(RouteMatcherTest, TestWeightedClusterWithMissingWeights) {
Expand Down Expand Up @@ -6294,7 +6304,6 @@ TEST_F(RouteMatcherTest, TestWeightedClusterClusterHeaderHeaderManipulation) {
- match: { prefix: "/" }
route:
weighted_clusters:
total_weight: 50
clusters:
- cluster_header: x-route-to-this-cluster
weight: 50
Expand Down Expand Up @@ -6341,7 +6350,6 @@ TEST_F(RouteMatcherTest, WeightedClusterInvalidConfigWithBothNameAndClusterHeade
- match: { prefix: "/" }
route:
weighted_clusters:
total_weight: 100
clusters:
- cluster_header: some_header
name: some_name
Expand All @@ -6366,7 +6374,6 @@ TEST_F(RouteMatcherTest, WeightedClusterInvalidConfigWithNoClusterSpecifier) {
- match: { prefix: "/" }
route:
weighted_clusters:
total_weight: 30
clusters:
- weight:
30
Expand All @@ -6386,7 +6393,6 @@ TEST_F(RouteMatcherTest, WeightedClusterInvalidConfigWithInvalidHttpHeader) {
- match: { prefix: "/" }
route:
weighted_clusters:
total_weight: 30
clusters:
- cluster_header: "test\r"
weight: 30
Expand Down
3 changes: 0 additions & 3 deletions test/integration/weighted_cluster_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,6 @@ class WeightedClusterIntegrationTest : public testing::TestWithParam<Network::Ad
cluster = weighted_clusters->add_clusters();
cluster->set_cluster_header(std::string(Envoy::RepickClusterFilter::ClusterHeaderName));
cluster->mutable_weight()->set_value(weights[1]);

weighted_clusters->mutable_total_weight()->set_value(
std::accumulate(weights.begin(), weights.end(), 0UL));
});

HttpIntegrationTest::initialize();
Expand Down
1 change: 0 additions & 1 deletion test/tools/router_check/test/config/Redirect3.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ virtual_hosts:
clusters:
- name: www2
weight: 100
total_weight: 100
- name: redirect
domains:
- redirect.lyft.com
Expand Down