Prefer the load_balancing_policy cluster field over lb_policy (#18419)

This change updates Envoy to consider the load balancing configuration set in load_balancing_policy, regardless of what is set in lb_policy. Previously the load_balancing_policy field was only considered if lb_policy was set to LOAD_BALANCING_POLICY_CONFIG. Going forward the approach is to only use the load_balancing_policy field and the extensible configuration mechanism it provides to configure all load balancing policies, which makes lb_policy deprecated. This change still preserves backward compatibility, allowing old clients to continue using the lb_policy field and new ones to switch to just considering load_balancing_policy. Commit Message: Prefer the load_balancing_policy cluster field over lb_policy Additional Description: This change updates Envoy to consider the load balancing configuration set in load_balancing_policy, regardless of what is set in lb_policy. Risk Level: Low Testing: New unit tests for upstream_impl.cc Docs Changes: Documentation to follow once consensus on this change is reached. Release Notes: Not currently user impacting as backward compatibility is maintained. Platform Specific Features: None. Signed-off-by: Terry Wilson <[email protected]>
envoyproxy · Oct 27, 2021 · bc641db · bc641db
1 parent b9939c6
commit bc641db
Show file tree

Hide file tree

Showing 8 changed files with 257 additions and 69 deletions.
diff --git a/api/envoy/config/cluster/v3/cluster.proto b/api/envoy/config/cluster/v3/cluster.proto
@@ -112,9 +112,9 @@ message Cluster {
 
     // Use the new :ref:`load_balancing_policy
     // <envoy_v3_api_field_config.cluster.v3.Cluster.load_balancing_policy>` field to determine the LB policy.
-    // [#next-major-version: In the v3 API, we should consider deprecating the lb_policy field
-    // and instead using the new load_balancing_policy field as the one and only mechanism for
-    // configuring this.]
+    // This has been deprecated in favor of using the :ref:`load_balancing_policy
+    // <envoy_v3_api_field_config.cluster.v3.Cluster.load_balancing_policy>` field without
+    // setting any value in :ref:`lb_policy<envoy_v3_api_field_config.cluster.v3.Cluster.lb_policy>`.
     LOAD_BALANCING_POLICY_CONFIG = 7;
   }
 
@@ -1044,9 +1044,8 @@ message Cluster {
   // servers of this cluster.
   repeated Filter filters = 40;
 
-  // New mechanism for LB policy configuration. Used only if the
-  // :ref:`lb_policy<envoy_v3_api_field_config.cluster.v3.Cluster.lb_policy>` field has the value
-  // :ref:`LOAD_BALANCING_POLICY_CONFIG<envoy_v3_api_enum_value_config.cluster.v3.Cluster.LbPolicy.LOAD_BALANCING_POLICY_CONFIG>`.
+  // If this field is set and is supported by the client, it will supersede the value of
+  // :ref:`lb_policy<envoy_v3_api_field_config.cluster.v3.Cluster.lb_policy>`.
   LoadBalancingPolicy load_balancing_policy = 41;
 
   // [#not-implemented-hide:]

diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc
@@ -836,72 +836,42 @@ ClusterInfoImpl::ClusterInfoImpl(
                          "HttpProtocolOptions can be specified");
   }
 
-  switch (config.lb_policy()) {
-  case envoy::config::cluster::v3::Cluster::ROUND_ROBIN:
-    lb_type_ = LoadBalancerType::RoundRobin;
-    break;
-  case envoy::config::cluster::v3::Cluster::LEAST_REQUEST:
-    lb_type_ = LoadBalancerType::LeastRequest;
-    break;
-  case envoy::config::cluster::v3::Cluster::RANDOM:
-    lb_type_ = LoadBalancerType::Random;
-    break;
-  case envoy::config::cluster::v3::Cluster::RING_HASH:
-    lb_type_ = LoadBalancerType::RingHash;
-    break;
-  case envoy::config::cluster::v3::Cluster::MAGLEV:
-    lb_type_ = LoadBalancerType::Maglev;
-    break;
-  case envoy::config::cluster::v3::Cluster::CLUSTER_PROVIDED:
-    if (config.has_lb_subset_config()) {
-      throw EnvoyException(
-          fmt::format("cluster: LB policy {} cannot be combined with lb_subset_config",
-                      envoy::config::cluster::v3::Cluster::LbPolicy_Name(config.lb_policy())));
-    }
-
-    lb_type_ = LoadBalancerType::ClusterProvided;
-    break;
-  case envoy::config::cluster::v3::Cluster::LOAD_BALANCING_POLICY_CONFIG: {
-    if (config.has_lb_subset_config()) {
-      throw EnvoyException(
-          fmt::format("cluster: LB policy {} cannot be combined with lb_subset_config",
-                      envoy::config::cluster::v3::Cluster::LbPolicy_Name(config.lb_policy())));
-    }
-
-    if (config.has_common_lb_config()) {
-      throw EnvoyException(
-          fmt::format("cluster: LB policy {} cannot be combined with common_lb_config",
-                      envoy::config::cluster::v3::Cluster::LbPolicy_Name(config.lb_policy())));
-    }
-
-    if (!config.has_load_balancing_policy()) {
-      throw EnvoyException(
-          fmt::format("cluster: LB policy {} requires load_balancing_policy to be set",
-                      envoy::config::cluster::v3::Cluster::LbPolicy_Name(config.lb_policy())));
-    }
-
-    for (const auto& policy : config.load_balancing_policy().policies()) {
-      TypedLoadBalancerFactory* factory =
-          Config::Utility::getAndCheckFactory<TypedLoadBalancerFactory>(
-              policy.typed_extension_config(), /*is_optional=*/true);
-      if (factory != nullptr) {
-        load_balancing_policy_ = policy;
-        load_balancer_factory_ = factory;
-        break;
+  // If load_balancing_policy is set we will use it directly, ignoring lb_policy.
+  if (config.has_load_balancing_policy()) {
+    configureLbPolicies(config);
+  } else {
+    switch (config.lb_policy()) {
+    case envoy::config::cluster::v3::Cluster::ROUND_ROBIN:
+      lb_type_ = LoadBalancerType::RoundRobin;
+      break;
+    case envoy::config::cluster::v3::Cluster::LEAST_REQUEST:
+      lb_type_ = LoadBalancerType::LeastRequest;
+      break;
+    case envoy::config::cluster::v3::Cluster::RANDOM:
+      lb_type_ = LoadBalancerType::Random;
+      break;
+    case envoy::config::cluster::v3::Cluster::RING_HASH:
+      lb_type_ = LoadBalancerType::RingHash;
+      break;
+    case envoy::config::cluster::v3::Cluster::MAGLEV:
+      lb_type_ = LoadBalancerType::Maglev;
+      break;
+    case envoy::config::cluster::v3::Cluster::CLUSTER_PROVIDED:
+      if (config.has_lb_subset_config()) {
+        throw EnvoyException(
+            fmt::format("cluster: LB policy {} cannot be combined with lb_subset_config",
+                        envoy::config::cluster::v3::Cluster::LbPolicy_Name(config.lb_policy())));
       }
-    }
 
-    if (load_balancer_factory_ == nullptr) {
-      throw EnvoyException(fmt::format(
-          "Didn't find a registered load balancer factory implementation for cluster: '{}'",
-          name_));
+      lb_type_ = LoadBalancerType::ClusterProvided;
+      break;
+    case envoy::config::cluster::v3::Cluster::LOAD_BALANCING_POLICY_CONFIG: {
+      configureLbPolicies(config);
+      break;
+    }
+    default:
+      NOT_REACHED_GCOVR_EXCL_LINE;
     }
-
-    lb_type_ = LoadBalancerType::LoadBalancingPolicyConfig;
-    break;
-  }
-  default:
-    NOT_REACHED_GCOVR_EXCL_LINE;
   }
 
   if (config.lb_subset_config().locality_weight_aware() &&
@@ -960,6 +930,45 @@ ClusterInfoImpl::ClusterInfoImpl(
   }
 }
 
+// Configures the load balancer based on config.load_balancing_policy
+void ClusterInfoImpl::configureLbPolicies(const envoy::config::cluster::v3::Cluster& config) {
+  if (config.has_lb_subset_config()) {
+    throw EnvoyException(
+        fmt::format("cluster: LB policy {} cannot be combined with lb_subset_config",
+                    envoy::config::cluster::v3::Cluster::LbPolicy_Name(config.lb_policy())));
+  }
+
+  if (config.has_common_lb_config()) {
+    throw EnvoyException(
+        fmt::format("cluster: LB policy {} cannot be combined with common_lb_config",
+                    envoy::config::cluster::v3::Cluster::LbPolicy_Name(config.lb_policy())));
+  }
+
+  if (!config.has_load_balancing_policy()) {
+    throw EnvoyException(
+        fmt::format("cluster: LB policy {} requires load_balancing_policy to be set",
+                    envoy::config::cluster::v3::Cluster::LbPolicy_Name(config.lb_policy())));
+  }
+
+  for (const auto& policy : config.load_balancing_policy().policies()) {
+    TypedLoadBalancerFactory* factory =
+        Config::Utility::getAndCheckFactory<TypedLoadBalancerFactory>(
+            policy.typed_extension_config(), /*is_optional=*/true);
+    if (factory != nullptr) {
+      load_balancing_policy_ = policy;
+      load_balancer_factory_ = factory;
+      break;
+    }
+  }
+
+  if (load_balancer_factory_ == nullptr) {
+    throw EnvoyException(fmt::format(
+        "Didn't find a registered load balancer factory implementation for cluster: '{}'", name_));
+  }
+
+  lb_type_ = LoadBalancerType::LoadBalancingPolicyConfig;
+}
+
 ProtocolOptionsConfigConstSharedPtr
 ClusterInfoImpl::extensionProtocolOptions(const std::string& name) const {
   auto i = extension_protocol_options_.find(name);

diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h
@@ -634,6 +634,7 @@ class ClusterInfoImpl : public ClusterInfo,
   const envoy::config::core::v3::HttpProtocolOptions& commonHttpProtocolOptions() const override {
     return http_protocol_options_->common_http_protocol_options_;
   }
+  void configureLbPolicies(const envoy::config::cluster::v3::Cluster& config);
   ProtocolOptionsConfigConstSharedPtr
   extensionProtocolOptions(const std::string& name) const override;
   LoadBalancerType lbType() const override { return lb_type_; }

diff --git a/test/common/upstream/BUILD b/test/common/upstream/BUILD
@@ -672,6 +672,8 @@ envoy_cc_test(
         "//test/mocks/upstream:cluster_manager_mocks",
         "//test/mocks/upstream:health_checker_mocks",
         "//test/mocks/upstream:priority_set_mocks",
+        "//test/mocks/upstream:thread_aware_load_balancer_mocks",
+        "//test/mocks/upstream:typed_load_balancer_factory_mocks",
         "//test/test_common:registry_lib",
         "//test/test_common:test_runtime_lib",
         "//test/test_common:utility_lib",

diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc
@@ -39,6 +39,8 @@
 #include "test/mocks/upstream/cluster_manager.h"
 #include "test/mocks/upstream/health_checker.h"
 #include "test/mocks/upstream/priority_set.h"
+#include "test/mocks/upstream/thread_aware_load_balancer.h"
+#include "test/mocks/upstream/typed_load_balancer_factory.h"
 #include "test/test_common/environment.h"
 #include "test/test_common/registry.h"
 #include "test/test_common/test_runtime.h"
@@ -2079,6 +2081,138 @@ TEST_F(StaticClusterImplTest, UnsupportedLBType) {
       EnvoyException, "invalid value \"fakelbtype\"");
 }
 
+// load_balancing_policy should be used when lb_policy is set to LOAD_BALANCING_POLICY_CONFIG.
+TEST_F(StaticClusterImplTest, LoadBalancingPolicyWithLbPolicy) {
+  const std::string yaml = R"EOF(
+    name: staticcluster
+    connect_timeout: 0.25s
+    type: static
+    lb_policy: LOAD_BALANCING_POLICY_CONFIG
+    load_balancing_policy:
+      policies:
+        - typed_extension_config:
+            name: custom_lb
+            typed_config:
+              "@type": type.googleapis.com/google.protobuf.Struct
+              value:
+                foo: "bar"
+    load_assignment:
+        endpoints:
+          - lb_endpoints:
+            - endpoint:
+                address:
+                  socket_address:
+                    address: 10.0.0.1
+                    port_value: 11001
+  )EOF";
+
+  NiceMock<MockTypedLoadBalancerFactory> factory;
+  EXPECT_CALL(factory, name()).WillRepeatedly(Return("custom_lb"));
+  Registry::InjectFactory<TypedLoadBalancerFactory> registered_factory(factory);
+
+  envoy::config::cluster::v3::Cluster cluster_config = parseClusterFromV3Yaml(yaml);
+  Envoy::Stats::ScopePtr scope = stats_.createScope(fmt::format(
+      "cluster.{}.", cluster_config.alt_stat_name().empty() ? cluster_config.name()
+                                                            : cluster_config.alt_stat_name()));
+  Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context(
+      admin_, ssl_context_manager_, *scope, cm_, local_info_, dispatcher_, stats_,
+      singleton_manager_, tls_, validation_visitor_, *api_, options_);
+  StaticClusterImpl cluster(cluster_config, runtime_, factory_context, std::move(scope), true);
+  cluster.initialize([] {});
+
+  EXPECT_EQ(1UL, cluster.prioritySet().hostSetsPerPriority()[0]->healthyHosts().size());
+  EXPECT_EQ(LoadBalancerType::LoadBalancingPolicyConfig, cluster.info()->lbType());
+  EXPECT_TRUE(cluster.info()->addedViaApi());
+}
+
+// load_balancing_policy should also be used when lb_policy is set to something else besides
+// LOAD_BALANCING_POLICY_CONFIG.
+TEST_F(StaticClusterImplTest, LoadBalancingPolicyWithOtherLbPolicy) {
+  const std::string yaml = R"EOF(
+    name: staticcluster
+    connect_timeout: 0.25s
+    type: static
+    lb_policy: ROUND_ROBIN
+    load_balancing_policy:
+      policies:
+        - typed_extension_config:
+            name: custom_lb
+            typed_config:
+              "@type": type.googleapis.com/google.protobuf.Struct
+              value:
+                foo: "bar"
+    load_assignment:
+        endpoints:
+          - lb_endpoints:
+            - endpoint:
+                address:
+                  socket_address:
+                    address: 10.0.0.1
+                    port_value: 11001
+  )EOF";
+
+  NiceMock<MockTypedLoadBalancerFactory> factory;
+  EXPECT_CALL(factory, name()).WillRepeatedly(Return("custom_lb"));
+  Registry::InjectFactory<TypedLoadBalancerFactory> registered_factory(factory);
+
+  envoy::config::cluster::v3::Cluster cluster_config = parseClusterFromV3Yaml(yaml);
+  Envoy::Stats::ScopePtr scope = stats_.createScope(fmt::format(
+      "cluster.{}.", cluster_config.alt_stat_name().empty() ? cluster_config.name()
+                                                            : cluster_config.alt_stat_name()));
+  Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context(
+      admin_, ssl_context_manager_, *scope, cm_, local_info_, dispatcher_, stats_,
+      singleton_manager_, tls_, validation_visitor_, *api_, options_);
+  StaticClusterImpl cluster(cluster_config, runtime_, factory_context, std::move(scope), true);
+  cluster.initialize([] {});
+
+  EXPECT_EQ(1UL, cluster.prioritySet().hostSetsPerPriority()[0]->healthyHosts().size());
+  EXPECT_EQ(LoadBalancerType::LoadBalancingPolicyConfig, cluster.info()->lbType());
+  EXPECT_TRUE(cluster.info()->addedViaApi());
+}
+
+// load_balancing_policy should also be used when lb_policy is omitted.
+TEST_F(StaticClusterImplTest, LoadBalancingPolicyWithoutLbPolicy) {
+  const std::string yaml = R"EOF(
+    name: staticcluster
+    connect_timeout: 0.25s
+    type: static
+    load_balancing_policy:
+      policies:
+        - typed_extension_config:
+            name: custom_lb
+            typed_config:
+              "@type": type.googleapis.com/google.protobuf.Struct
+              value:
+                foo: "bar"
+    load_assignment:
+        endpoints:
+          - lb_endpoints:
+            - endpoint:
+                address:
+                  socket_address:
+                    address: 10.0.0.1
+                    port_value: 11001
+  )EOF";
+
+  NiceMock<MockTypedLoadBalancerFactory> factory;
+  EXPECT_CALL(factory, name()).WillRepeatedly(Return("custom_lb"));
+  Registry::InjectFactory<TypedLoadBalancerFactory> registered_factory(factory);
+
+  envoy::config::cluster::v3::Cluster cluster_config = parseClusterFromV3Yaml(yaml);
+  Envoy::Stats::ScopePtr scope = stats_.createScope(fmt::format(
+      "cluster.{}.", cluster_config.alt_stat_name().empty() ? cluster_config.name()
+                                                            : cluster_config.alt_stat_name()));
+  Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context(
+      admin_, ssl_context_manager_, *scope, cm_, local_info_, dispatcher_, stats_,
+      singleton_manager_, tls_, validation_visitor_, *api_, options_);
+  StaticClusterImpl cluster(cluster_config, runtime_, factory_context, std::move(scope), true);
+  cluster.initialize([] {});
+
+  EXPECT_EQ(1UL, cluster.prioritySet().hostSetsPerPriority()[0]->healthyHosts().size());
+  EXPECT_EQ(LoadBalancerType::LoadBalancingPolicyConfig, cluster.info()->lbType());
+  EXPECT_TRUE(cluster.info()->addedViaApi());
+}
+
 TEST_F(StaticClusterImplTest, MalformedHostIP) {
   const std::string yaml = R"EOF(
     name: name

diff --git a/test/mocks/upstream/BUILD b/test/mocks/upstream/BUILD
@@ -93,6 +93,7 @@ envoy_cc_mock(
         ":thread_aware_load_balancer_mocks",
         ":thread_local_cluster_mocks",
         ":transport_socket_match_mocks",
+        ":typed_load_balancer_factory_mocks",
         "//envoy/http:async_client_interface",
         "//envoy/upstream:cluster_factory_interface",
         "//envoy/upstream:cluster_manager_interface",
@@ -204,6 +205,15 @@ envoy_cc_mock(
     ],
 )
 
+envoy_cc_mock(
+    name = "typed_load_balancer_factory_mocks",
+    srcs = ["typed_load_balancer_factory.cc"],
+    hdrs = ["typed_load_balancer_factory.h"],
+    deps = [
+        "//envoy/upstream:load_balancer_interface",
+    ],
+)
+
 envoy_cc_mock(
     name = "thread_local_cluster_mocks",
     srcs = ["thread_local_cluster.cc"],

diff --git a/test/mocks/upstream/typed_load_balancer_factory.cc b/test/mocks/upstream/typed_load_balancer_factory.cc
@@ -0,0 +1,10 @@
+#include "typed_load_balancer_factory.h"
+
+namespace Envoy {
+namespace Upstream {
+MockTypedLoadBalancerFactory::MockTypedLoadBalancerFactory() = default;
+
+MockTypedLoadBalancerFactory::~MockTypedLoadBalancerFactory() = default;
+
+} // namespace Upstream
+} // namespace Envoy