router: deprecating total_weight (#23170)

Signed-off-by: Adi Suissa-Peleg <[email protected]>

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
-    // <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
@@ -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

changelogs/current.yaml

@@ -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.

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 <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``,

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;

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

test/integration/weighted_cluster_integration_test.cc

@@ -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();

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