From 9874abed67a462377bbdf1f7926500aabfc775fd Mon Sep 17 00:00:00 2001 From: "Adi (Suissa) Peleg" Date: Wed, 21 Sep 2022 11:04:16 -0400 Subject: [PATCH] router: deprecating total_weight (#23170) Signed-off-by: Adi Suissa-Peleg --- .../config/route/v3/route_components.proto | 13 ++++--- changelogs/current.yaml | 5 +++ .../http/http_conn_man/traffic_splitting.rst | 8 +++-- source/common/router/config_impl.cc | 8 ++--- test/common/router/config_impl_test.cc | 36 +++++++++++-------- .../weighted_cluster_integration_test.cc | 3 -- .../router_check/test/config/Redirect3.yaml | 1 - 7 files changed, 43 insertions(+), 31 deletions(-) diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index 86a14434ad9f..b82e653246ba 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -386,10 +386,10 @@ message WeightedCluster { (udpa.annotations.field_migrate).oneof_promotion = "cluster_specifier" ]; - // An integer between 0 and :ref:`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 @@ -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 diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 417617c92054..6651cf747cf2 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -233,3 +233,8 @@ deprecated: change: | deprecated :ref:`append ` and please use :ref:`append_action ` first. +- area: router + change: | + deprecated :ref:`total weight ` for weighted + clusters. The sum of the :ref:`clusters' weights ` + will be used as the total weight. diff --git a/docs/root/configuration/http/http_conn_man/traffic_splitting.rst b/docs/root/configuration/http/http_conn_man/traffic_splitting.rst index eab4577f5301..99be7c4b057f 100644 --- a/docs/root/configuration/http/http_conn_man/traffic_splitting.rst +++ b/docs/root/configuration/http/http_conn_man/traffic_splitting.rst @@ -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 ` defaults to 100, but can -be modified to allow finer granularity. +be modified to allow finer granularity. The +:ref:`total weight ` is now deprecated, +and the relative value of each +:ref:`cluster 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``, diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 00abecd05ba3..261bff0d748e 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -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(), @@ -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; diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 56174930ed72..50818f6a8f25 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -3270,7 +3270,6 @@ TEST_F(RouteMatcherTest, WeightedClusterHeader) { - match: { prefix: "/" } route: weighted_clusters: - total_weight: 100 clusters: - cluster_header: some_header weight: 30 @@ -3303,7 +3302,6 @@ TEST_F(RouteMatcherTest, WeightedClusterWithProvidedRandomValue) { - match: { prefix: "/" } route: weighted_clusters: - total_weight: 80 header_name: "x_random_value" clusters: - name: cluster1 @@ -5848,7 +5846,6 @@ TEST_F(RouteMatcherTest, WeightedClusters) { weight: 3000 - name: cluster3 weight: 5000 - total_weight: 10000 - name: www3 domains: ["www3.lyft.com"] routes: @@ -5877,7 +5874,6 @@ TEST_F(RouteMatcherTest, WeightedClusters) { weight: 3000 - name: cluster3 weight: 5000 - total_weight: 10000 )EOF"; BazFactory baz_factory; @@ -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 @@ -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) { @@ -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 @@ -6341,7 +6350,6 @@ TEST_F(RouteMatcherTest, WeightedClusterInvalidConfigWithBothNameAndClusterHeade - match: { prefix: "/" } route: weighted_clusters: - total_weight: 100 clusters: - cluster_header: some_header name: some_name @@ -6366,7 +6374,6 @@ TEST_F(RouteMatcherTest, WeightedClusterInvalidConfigWithNoClusterSpecifier) { - match: { prefix: "/" } route: weighted_clusters: - total_weight: 30 clusters: - weight: 30 @@ -6386,7 +6393,6 @@ TEST_F(RouteMatcherTest, WeightedClusterInvalidConfigWithInvalidHttpHeader) { - match: { prefix: "/" } route: weighted_clusters: - total_weight: 30 clusters: - cluster_header: "test\r" weight: 30 diff --git a/test/integration/weighted_cluster_integration_test.cc b/test/integration/weighted_cluster_integration_test.cc index 268bf0a215c0..0902ea9e1add 100644 --- a/test/integration/weighted_cluster_integration_test.cc +++ b/test/integration/weighted_cluster_integration_test.cc @@ -60,9 +60,6 @@ class WeightedClusterIntegrationTest : public testing::TestWithParamadd_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(); diff --git a/test/tools/router_check/test/config/Redirect3.yaml b/test/tools/router_check/test/config/Redirect3.yaml index 7852fa4e1cb9..fb76ec9a7a69 100644 --- a/test/tools/router_check/test/config/Redirect3.yaml +++ b/test/tools/router_check/test/config/Redirect3.yaml @@ -10,7 +10,6 @@ virtual_hosts: clusters: - name: www2 weight: 100 - total_weight: 100 - name: redirect domains: - redirect.lyft.com