From e31bc1edec3bb80760a1442869cc442d123e9adc Mon Sep 17 00:00:00 2001 From: pianiststickman <34144687+pianiststickman@users.noreply.github.com> Date: Fri, 13 Aug 2021 22:42:24 -0400 Subject: [PATCH] Enable load balancing policy extensions (#17400) Enables `LOAD_BALANCING_POLICY_CONFIG` enum value in `LbPolicy` and supports typed load balancers specified in `load_balancing_policy`. Continues work done by Charlie Getzen in #15827. Custom load balancers specified by `load_balancing_policy` are created as implementations of `ThreadAwareLoadBalancer`. Thread-local load balancers can be implemented as thread-aware load balancers that contain no logic at the thread-aware level, i.e. the purpose of the thread-aware LB is solely to contain the factory used to instantiate the thread-local LBs. (In the future it might be appropriate to provide a construct that abstracts away thread-aware aspects of `ThreadAwareLoadBalancer` for LBs that don't need to be thread-aware.) A cluster that uses `LOAD_BALANCING_POLICY_CONFIG` may not also set a subset LB configuration. If the load balancer type makes use of subsetting, it should include a subset configuration in its own configuration message. Future work on load balancing extensions should include moving the subset LB to use load balancing extensions. Similarly, a cluster that uses `LOAD_BALANCING_POLICY_CONFIG` may not set the `CommonLbConfig`, and it is not passed into load balancer creation (mostly owing to its dubious applicability as a top level configuration message to hierarchical load balancing policy). If the load balancer type makes use of the `CommonLbConfig`, it should include a `CommonLbConfig` in the configuration message for the load balancing policy. Considerations for migration of existing load balancers: - pieces of the `ThreadAwareLoadBalancerBase` implementation are specific to the built-in hashing load balancers and should be moved into a base class specifically for hashing load balancers. As it stands, custom load balancing policies are required to implement a `createLoadBalancer()` method even if the architecture of the LB policy does not require a hashing load balancer. I think we would also benefit from disentangling `ThreadAwareLoadBalancerBase` from `LoadBalancerBase`, as the former never actually does any host picking. - as we convert existing thread-local load balancers to thread-aware load balancers, new local LBs will be re-created upon membership changes. We should provide a mechanism allowing load balancers to control whether this rebuild should occur, e.g. a callback that calls `create()` for thread-aware LBs by default, which can be overridden to do nothing for thread-local LBs. Risk Level: low Testing: brought up a cluster with a custom load balancer specified by `load_balancing_policy`; new unit tests included Docs Changes: n/a Release Notes: Enable load balancing policy extensions Platform Specific Features: n/a Fixes #5598 Signed-off-by: Eugene Chan --- api/envoy/config/cluster/v3/cluster.proto | 18 +- .../config/cluster/v4alpha/cluster.proto | 18 +- envoy/upstream/load_balancer.h | 21 ++ envoy/upstream/load_balancer_type.h | 3 +- envoy/upstream/upstream.h | 15 ++ .../envoy/config/cluster/v3/cluster.proto | 16 +- .../config/cluster/v4alpha/cluster.proto | 18 +- source/common/upstream/BUILD | 8 + .../common/upstream/cluster_manager_impl.cc | 8 + .../upstream/load_balancer_factory_base.h | 29 +++ source/common/upstream/subset_lb.cc | 4 +- source/common/upstream/upstream_impl.cc | 39 ++++ source/common/upstream/upstream_impl.h | 7 + test/common/upstream/BUILD | 1 + .../upstream/cluster_manager_impl_test.cc | 197 +++++++++++++++++- .../http_subset_lb_integration_test.cc | 3 +- test/mocks/upstream/cluster_info.h | 3 + 17 files changed, 360 insertions(+), 48 deletions(-) create mode 100644 source/common/upstream/load_balancer_factory_base.h diff --git a/api/envoy/config/cluster/v3/cluster.proto b/api/envoy/config/cluster/v3/cluster.proto index 35bfa93ea352..85662e8f89d2 100644 --- a/api/envoy/config/cluster/v3/cluster.proto +++ b/api/envoy/config/cluster/v3/cluster.proto @@ -110,7 +110,7 @@ message Cluster { // this option or not. CLUSTER_PROVIDED = 6; - // [#not-implemented-hide:] Use the new :ref:`load_balancing_policy + // Use the new :ref:`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 @@ -720,8 +720,7 @@ message Cluster { // The :ref:`load balancer type ` to use // when picking a host in the cluster. - // [#comment:TODO: Remove enum constraint :ref:`LOAD_BALANCING_POLICY_CONFIG` when implemented.] - LbPolicy lb_policy = 6 [(validate.rules).enum = {defined_only: true not_in: 7}]; + LbPolicy lb_policy = 6 [(validate.rules).enum = {defined_only: true}]; // Setting this is required for specifying members of // :ref:`STATIC`, @@ -1008,7 +1007,7 @@ message Cluster { // servers of this cluster. repeated Filter filters = 40; - // [#not-implemented-hide:] New mechanism for LB policy configuration. Used only if the + // New mechanism for LB policy configuration. Used only if the // :ref:`lb_policy` field has the value // :ref:`LOAD_BALANCING_POLICY_CONFIG`. LoadBalancingPolicy load_balancing_policy = 41; @@ -1073,7 +1072,7 @@ message Cluster { bool connection_pool_per_downstream_connection = 51; } -// [#not-implemented-hide:] Extensible load balancing policy configuration. +// Extensible load balancing policy configuration. // // Every LB policy defined via this mechanism will be identified via a unique name using reverse // DNS notation. If the policy needs configuration parameters, it must define a message for its @@ -1099,14 +1098,11 @@ message LoadBalancingPolicy { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.LoadBalancingPolicy.Policy"; - reserved 2; - - reserved "config"; + reserved 2, 1, 3; - // Required. The name of the LB policy. - string name = 1; + reserved "config", "name", "typed_config"; - google.protobuf.Any typed_config = 3; + core.v3.TypedExtensionConfig typed_extension_config = 4; } // Each client will iterate over the list in order and stop at the first policy that it diff --git a/api/envoy/config/cluster/v4alpha/cluster.proto b/api/envoy/config/cluster/v4alpha/cluster.proto index 9b367fe22067..1fabdbe707cd 100644 --- a/api/envoy/config/cluster/v4alpha/cluster.proto +++ b/api/envoy/config/cluster/v4alpha/cluster.proto @@ -110,7 +110,7 @@ message Cluster { // this option or not. CLUSTER_PROVIDED = 6; - // [#not-implemented-hide:] Use the new :ref:`load_balancing_policy + // Use the new :ref:`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 @@ -730,8 +730,7 @@ message Cluster { // The :ref:`load balancer type ` to use // when picking a host in the cluster. - // [#comment:TODO: Remove enum constraint :ref:`LOAD_BALANCING_POLICY_CONFIG` when implemented.] - LbPolicy lb_policy = 6 [(validate.rules).enum = {defined_only: true not_in: 7}]; + LbPolicy lb_policy = 6 [(validate.rules).enum = {defined_only: true}]; // Setting this is required for specifying members of // :ref:`STATIC`, @@ -916,7 +915,7 @@ message Cluster { // servers of this cluster. repeated Filter filters = 40; - // [#not-implemented-hide:] New mechanism for LB policy configuration. Used only if the + // New mechanism for LB policy configuration. Used only if the // :ref:`lb_policy` field has the value // :ref:`LOAD_BALANCING_POLICY_CONFIG`. LoadBalancingPolicy load_balancing_policy = 41; @@ -968,7 +967,7 @@ message Cluster { bool connection_pool_per_downstream_connection = 51; } -// [#not-implemented-hide:] Extensible load balancing policy configuration. +// Extensible load balancing policy configuration. // // Every LB policy defined via this mechanism will be identified via a unique name using reverse // DNS notation. If the policy needs configuration parameters, it must define a message for its @@ -995,14 +994,11 @@ message LoadBalancingPolicy { option (udpa.annotations.versioning).previous_message_type = "envoy.config.cluster.v3.LoadBalancingPolicy.Policy"; - reserved 2; - - reserved "config"; + reserved 2, 1, 3; - // Required. The name of the LB policy. - string name = 1; + reserved "config", "name", "typed_config"; - google.protobuf.Any typed_config = 3; + core.v4alpha.TypedExtensionConfig typed_extension_config = 4; } // Each client will iterate over the list in order and stop at the first policy that it diff --git a/envoy/upstream/load_balancer.h b/envoy/upstream/load_balancer.h index b1b20324b84e..af85933b63d3 100644 --- a/envoy/upstream/load_balancer.h +++ b/envoy/upstream/load_balancer.h @@ -173,5 +173,26 @@ class ThreadAwareLoadBalancer { using ThreadAwareLoadBalancerPtr = std::unique_ptr; +/** + * Factory for (thread-aware) load balancers. To support a load balancing policy of + * LOAD_BALANCING_POLICY_CONFIG, at least one load balancer factory corresponding to a policy in + * load_balancing_policy must be registered with Envoy. Envoy will use the first policy for which + * it has a registered factory. + */ +class TypedLoadBalancerFactory : public Config::UntypedFactory { +public: + ~TypedLoadBalancerFactory() override = default; + + /** + * @return ThreadAwareLoadBalancerPtr a new thread-aware load balancer. + */ + virtual ThreadAwareLoadBalancerPtr + create(const PrioritySet& priority_set, ClusterStats& stats, Stats::Scope& stats_scope, + Runtime::Loader& runtime, Random::RandomGenerator& random, + const ::envoy::config::cluster::v3::LoadBalancingPolicy_Policy& lb_policy) PURE; + + std::string category() const override { return "envoy.load_balancers"; } +}; + } // namespace Upstream } // namespace Envoy diff --git a/envoy/upstream/load_balancer_type.h b/envoy/upstream/load_balancer_type.h index 280c317cdcce..b8423a1d703c 100644 --- a/envoy/upstream/load_balancer_type.h +++ b/envoy/upstream/load_balancer_type.h @@ -22,7 +22,8 @@ enum class LoadBalancerType { RingHash, OriginalDst, Maglev, - ClusterProvided + ClusterProvided, + LoadBalancingPolicyConfig }; /** diff --git a/envoy/upstream/upstream.h b/envoy/upstream/upstream.h index d8b73f39de23..e9cf44b9f3c9 100644 --- a/envoy/upstream/upstream.h +++ b/envoy/upstream/upstream.h @@ -692,6 +692,8 @@ using ProtocolOptionsConfigConstSharedPtr = std::shared_ptr(extensionProtocolOptions(name)); } + /** + * @return const envoy::config::cluster::v3::LoadBalancingPolicy_Policy& the load balancing policy + * to use for this cluster. + */ + virtual const envoy::config::cluster::v3::LoadBalancingPolicy_Policy& + loadBalancingPolicy() const PURE; + + /** + * @return the load balancer factory for this cluster if the load balancing type is + * LOAD_BALANCING_POLICY_CONFIG. + */ + virtual TypedLoadBalancerFactory* loadBalancerFactory() const PURE; + /** * @return const envoy::config::cluster::v3::Cluster::CommonLbConfig& the common configuration for * all load balancers for this cluster. diff --git a/generated_api_shadow/envoy/config/cluster/v3/cluster.proto b/generated_api_shadow/envoy/config/cluster/v3/cluster.proto index 137300708e37..6a87b55a825d 100644 --- a/generated_api_shadow/envoy/config/cluster/v3/cluster.proto +++ b/generated_api_shadow/envoy/config/cluster/v3/cluster.proto @@ -107,7 +107,7 @@ message Cluster { // this option or not. CLUSTER_PROVIDED = 6; - // [#not-implemented-hide:] Use the new :ref:`load_balancing_policy + // Use the new :ref:`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 @@ -721,8 +721,7 @@ message Cluster { // The :ref:`load balancer type ` to use // when picking a host in the cluster. - // [#comment:TODO: Remove enum constraint :ref:`LOAD_BALANCING_POLICY_CONFIG` when implemented.] - LbPolicy lb_policy = 6 [(validate.rules).enum = {defined_only: true not_in: 7}]; + LbPolicy lb_policy = 6 [(validate.rules).enum = {defined_only: true}]; // Setting this is required for specifying members of // :ref:`STATIC`, @@ -1009,7 +1008,7 @@ message Cluster { // servers of this cluster. repeated Filter filters = 40; - // [#not-implemented-hide:] New mechanism for LB policy configuration. Used only if the + // New mechanism for LB policy configuration. Used only if the // :ref:`lb_policy` field has the value // :ref:`LOAD_BALANCING_POLICY_CONFIG`. LoadBalancingPolicy load_balancing_policy = 41; @@ -1090,7 +1089,7 @@ message Cluster { ]; } -// [#not-implemented-hide:] Extensible load balancing policy configuration. +// Extensible load balancing policy configuration. // // Every LB policy defined via this mechanism will be identified via a unique name using reverse // DNS notation. If the policy needs configuration parameters, it must define a message for its @@ -1116,10 +1115,11 @@ message LoadBalancingPolicy { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.LoadBalancingPolicy.Policy"; - // Required. The name of the LB policy. - string name = 1; + reserved 1, 3; - google.protobuf.Any typed_config = 3; + reserved "name", "typed_config"; + + core.v3.TypedExtensionConfig typed_extension_config = 4; google.protobuf.Struct hidden_envoy_deprecated_config = 2 [deprecated = true, (envoy.annotations.deprecated_at_minor_version) = "3.0"]; diff --git a/generated_api_shadow/envoy/config/cluster/v4alpha/cluster.proto b/generated_api_shadow/envoy/config/cluster/v4alpha/cluster.proto index 5f8eeac2f67f..5217313a1f34 100644 --- a/generated_api_shadow/envoy/config/cluster/v4alpha/cluster.proto +++ b/generated_api_shadow/envoy/config/cluster/v4alpha/cluster.proto @@ -112,7 +112,7 @@ message Cluster { // this option or not. CLUSTER_PROVIDED = 6; - // [#not-implemented-hide:] Use the new :ref:`load_balancing_policy + // Use the new :ref:`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 @@ -729,8 +729,7 @@ message Cluster { // The :ref:`load balancer type ` to use // when picking a host in the cluster. - // [#comment:TODO: Remove enum constraint :ref:`LOAD_BALANCING_POLICY_CONFIG` when implemented.] - LbPolicy lb_policy = 6 [(validate.rules).enum = {defined_only: true not_in: 7}]; + LbPolicy lb_policy = 6 [(validate.rules).enum = {defined_only: true}]; // Setting this is required for specifying members of // :ref:`STATIC`, @@ -1017,7 +1016,7 @@ message Cluster { // servers of this cluster. repeated Filter filters = 40; - // [#not-implemented-hide:] New mechanism for LB policy configuration. Used only if the + // New mechanism for LB policy configuration. Used only if the // :ref:`lb_policy` field has the value // :ref:`LOAD_BALANCING_POLICY_CONFIG`. LoadBalancingPolicy load_balancing_policy = 41; @@ -1082,7 +1081,7 @@ message Cluster { bool connection_pool_per_downstream_connection = 51; } -// [#not-implemented-hide:] Extensible load balancing policy configuration. +// Extensible load balancing policy configuration. // // Every LB policy defined via this mechanism will be identified via a unique name using reverse // DNS notation. If the policy needs configuration parameters, it must define a message for its @@ -1109,14 +1108,11 @@ message LoadBalancingPolicy { option (udpa.annotations.versioning).previous_message_type = "envoy.config.cluster.v3.LoadBalancingPolicy.Policy"; - reserved 2; - - reserved "config"; + reserved 2, 1, 3; - // Required. The name of the LB policy. - string name = 1; + reserved "config", "name", "typed_config"; - google.protobuf.Any typed_config = 3; + core.v4alpha.TypedExtensionConfig typed_extension_config = 4; } // Each client will iterate over the list in order and stop at the first policy that it diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD index aeda29f2e18f..7d6858495114 100644 --- a/source/common/upstream/BUILD +++ b/source/common/upstream/BUILD @@ -219,6 +219,14 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "load_balancer_factory_base_lib", + hdrs = ["load_balancer_factory_base.h"], + deps = [ + ":load_balancer_lib", + ], +) + envoy_cc_library( name = "load_stats_reporter_lib", srcs = ["load_stats_reporter.cc"], diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 21461da3bbdb..f7e77808bc9e 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -831,6 +831,13 @@ ClusterManagerImpl::loadCluster(const envoy::config::cluster::v3::Cluster& clust } } else if (cluster_reference.info()->lbType() == LoadBalancerType::ClusterProvided) { cluster_entry_it->second->thread_aware_lb_ = std::move(new_cluster_pair.second); + } else if (cluster_reference.info()->lbType() == LoadBalancerType::LoadBalancingPolicyConfig) { + const auto& policy = cluster_reference.info()->loadBalancingPolicy(); + TypedLoadBalancerFactory* typed_lb_factory = cluster_reference.info()->loadBalancerFactory(); + RELEASE_ASSERT(typed_lb_factory != nullptr, "ClusterInfo should contain a valid factory"); + cluster_entry_it->second->thread_aware_lb_ = + typed_lb_factory->create(cluster_reference.prioritySet(), cluster_reference.info()->stats(), + cluster_reference.info()->statsScope(), runtime_, random_, policy); } updateClusterCounts(); @@ -1363,6 +1370,7 @@ ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::ClusterEntry( break; } case LoadBalancerType::ClusterProvided: + case LoadBalancerType::LoadBalancingPolicyConfig: case LoadBalancerType::RingHash: case LoadBalancerType::Maglev: case LoadBalancerType::OriginalDst: { diff --git a/source/common/upstream/load_balancer_factory_base.h b/source/common/upstream/load_balancer_factory_base.h new file mode 100644 index 000000000000..ce3fa56eabac --- /dev/null +++ b/source/common/upstream/load_balancer_factory_base.h @@ -0,0 +1,29 @@ +#pragma once + +#include "envoy/upstream/load_balancer.h" + +namespace Envoy { +namespace Upstream { + +/** + * Base class for cluster provided load balancers and load balancers specified by load balancing + * policy config. This class should be extended directly if the load balancing policy specifies a + * thread-aware load balancer. + * + * TODO: provide a ThreadLocalLoadBalancer construct to abstract away thread-awareness from load + * balancing extensions that don't require it. + */ +class TypedLoadBalancerFactoryBase : public TypedLoadBalancerFactory { +public: + // Upstream::TypedLoadBalancerFactory + std::string name() const override { return name_; } + +protected: + TypedLoadBalancerFactoryBase(const std::string& name) : name_(name) {} + +private: + const std::string name_; +}; + +} // namespace Upstream +} // namespace Envoy diff --git a/source/common/upstream/subset_lb.cc b/source/common/upstream/subset_lb.cc index 95cbf16af8cb..2a080c32ee60 100644 --- a/source/common/upstream/subset_lb.cc +++ b/source/common/upstream/subset_lb.cc @@ -781,8 +781,8 @@ SubsetLoadBalancer::PrioritySubsetImpl::PrioritySubsetImpl(const SubsetLoadBalan case LoadBalancerType::OriginalDst: case LoadBalancerType::ClusterProvided: - // LoadBalancerType::OriginalDst is blocked in the factory. LoadBalancerType::ClusterProvided - // is impossible because the subset LB returns a null load balancer from its factory. + case LoadBalancerType::LoadBalancingPolicyConfig: + // These load balancer types can only be created when there is no subset configuration. NOT_REACHED_GCOVR_EXCL_LINE; } diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 44097ffe65de..62fe07a69e83 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -807,6 +807,45 @@ ClusterInfoImpl::ClusterInfoImpl( 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( + 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; + break; + } default: NOT_REACHED_GCOVR_EXCL_LINE; } diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 6674082cd8d4..b233f2bd839d 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -573,6 +573,11 @@ class ClusterInfoImpl : public ClusterInfo, // Upstream::ClusterInfo bool addedViaApi() const override { return added_via_api_; } + const envoy::config::cluster::v3::LoadBalancingPolicy_Policy& + loadBalancingPolicy() const override { + return load_balancing_policy_; + } + TypedLoadBalancerFactory* loadBalancerFactory() const override { return load_balancer_factory_; } const envoy::config::cluster::v3::Cluster::CommonLbConfig& lbConfig() const override { return common_lb_config_; } @@ -756,6 +761,8 @@ class ClusterInfoImpl : public ClusterInfo, LoadBalancerSubsetInfoImpl lb_subset_; const envoy::config::core::v3::Metadata metadata_; Envoy::Config::TypedMetadataImpl typed_metadata_; + envoy::config::cluster::v3::LoadBalancingPolicy_Policy load_balancing_policy_; + TypedLoadBalancerFactory* load_balancer_factory_ = nullptr; const envoy::config::cluster::v3::Cluster::CommonLbConfig common_lb_config_; const Network::ConnectionSocket::OptionsSharedPtr cluster_socket_options_; const bool drain_connections_on_host_removal_; diff --git a/test/common/upstream/BUILD b/test/common/upstream/BUILD index 45bcb56efddd..06264193ba2a 100644 --- a/test/common/upstream/BUILD +++ b/test/common/upstream/BUILD @@ -41,6 +41,7 @@ envoy_cc_test( deps = [ ":test_cluster_manager", "//source/common/router:context_lib", + "//source/common/upstream:load_balancer_factory_base_lib", "//source/extensions/transport_sockets/tls:config", "//test/config:v2_link_hacks", "//test/mocks/matcher:matcher_mocks", diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 5c1cbe1727e3..a25a721219ea 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -7,6 +7,7 @@ #include "source/common/network/raw_buffer_socket.h" #include "source/common/network/resolver_impl.h" #include "source/common/router/context_impl.h" +#include "source/common/upstream/load_balancer_factory_base.h" #include "source/extensions/transport_sockets/raw_buffer/config.h" #include "test/common/upstream/test_cluster_manager.h" @@ -525,8 +526,7 @@ class ClusterManagerSubsetInitializationTest if (envoy::config::cluster::v3::Cluster::LbPolicy_IsValid(i)) { auto policy = static_cast(i); if (policy != - envoy::config::cluster::v3::Cluster::hidden_envoy_deprecated_ORIGINAL_DST_LB && - policy != envoy::config::cluster::v3::Cluster::LOAD_BALANCING_POLICY_CONFIG) { + envoy::config::cluster::v3::Cluster::hidden_envoy_deprecated_ORIGINAL_DST_LB) { policies.push_back(policy); } } @@ -579,7 +579,8 @@ TEST_P(ClusterManagerSubsetInitializationTest, SubsetLoadBalancerInitialization) } const std::string yaml = fmt::format(yamlPattern, cluster_type, policy_name); - if (GetParam() == envoy::config::cluster::v3::Cluster::CLUSTER_PROVIDED) { + if (GetParam() == envoy::config::cluster::v3::Cluster::CLUSTER_PROVIDED || + GetParam() == envoy::config::cluster::v3::Cluster::LOAD_BALANCING_POLICY_CONFIG) { EXPECT_THROW_WITH_MESSAGE( create(parseBootstrapFromV3Yaml(yaml)), EnvoyException, fmt::format("cluster: LB policy {} cannot be combined with lb_subset_config", @@ -757,6 +758,196 @@ TEST_F(ClusterManagerImplTest, ClusterProvidedLbNotConfigured) { "'cluster_0' provided one. Check cluster documentation."); } +class CustomLbFactory : public TypedLoadBalancerFactoryBase { +public: + CustomLbFactory() : TypedLoadBalancerFactoryBase("envoy.load_balancers.custom_lb") {} + + ThreadAwareLoadBalancerPtr + create(const PrioritySet&, ClusterStats&, Stats::Scope&, Runtime::Loader&, + Random::RandomGenerator&, + const ::envoy::config::cluster::v3::LoadBalancingPolicy_Policy&) override { + return std::make_unique(); + } + +private: + class LbImpl : public LoadBalancer { + public: + LbImpl() = default; + + Upstream::HostConstSharedPtr chooseHost(Upstream::LoadBalancerContext*) override { + return nullptr; + } + Upstream::HostConstSharedPtr peekAnotherHost(Upstream::LoadBalancerContext*) override { + return nullptr; + } + }; + + class LbFactory : public LoadBalancerFactory { + public: + LbFactory() = default; + + Upstream::LoadBalancerPtr create() override { return std::make_unique(); } + }; + + class ThreadAwareLbImpl : public Upstream::ThreadAwareLoadBalancer { + public: + ThreadAwareLbImpl() = default; + + Upstream::LoadBalancerFactorySharedPtr factory() override { + return std::make_shared(); + } + void initialize() override {} + }; +}; + +// Verify that specifying LOAD_BALANCING_POLICY_CONFIG with CommonLbConfig is an error. +TEST_F(ClusterManagerImplTest, LbPolicyConfigCannotSpecifyCommonLbConfig) { + CustomLbFactory factory; + Registry::InjectFactory registration(factory); + + const std::string yaml = fmt::format(R"EOF( + static_resources: + clusters: + - name: cluster_1 + connect_timeout: 0.250s + type: STATIC + lb_policy: LOAD_BALANCING_POLICY_CONFIG + load_balancing_policy: + policies: + - typed_extension_config: + name: envoy.load_balancers.custom_lb + common_lb_config: + update_merge_window: 3s + load_assignment: + cluster_name: cluster_1 + endpoints: + - lb_endpoints: + - endpoint: + address: + socket_address: + address: 127.0.0.1 + port_value: 8000 + - endpoint: + address: + socket_address: + address: 127.0.0.1 + port_value: 8001 + )EOF"); + + EXPECT_THROW_WITH_MESSAGE( + create(parseBootstrapFromV3Yaml(yaml)), EnvoyException, + "cluster: LB policy LOAD_BALANCING_POLICY_CONFIG cannot be combined with common_lb_config"); +} + +// Verify that LOAD_BALANCING_POLICY_CONFIG without specifying load balancing policy is an error. +TEST_F(ClusterManagerImplTest, LbPolicyConfigMustSpecifyLbPolicy) { + const std::string yaml = fmt::format(R"EOF( + static_resources: + clusters: + - name: cluster_1 + connect_timeout: 0.250s + type: STATIC + lb_policy: LOAD_BALANCING_POLICY_CONFIG + load_assignment: + cluster_name: cluster_1 + endpoints: + - lb_endpoints: + - endpoint: + address: + socket_address: + address: 127.0.0.1 + port_value: 8000 + - endpoint: + address: + socket_address: + address: 127.0.0.1 + port_value: 8001 + )EOF"); + + EXPECT_THROW_WITH_MESSAGE( + create(parseBootstrapFromV3Yaml(yaml)), EnvoyException, + "cluster: LB policy LOAD_BALANCING_POLICY_CONFIG requires load_balancing_policy to be set"); +} + +// Verify that multiple load balancing policies can be specified, and Envoy selects the first +// policy that it has a factory for. +TEST_F(ClusterManagerImplTest, LbPolicyConfig) { + CustomLbFactory factory; + Registry::InjectFactory registration(factory); + + const std::string yaml = fmt::format(R"EOF( + static_resources: + clusters: + - name: cluster_1 + connect_timeout: 0.250s + type: STATIC + lb_policy: LOAD_BALANCING_POLICY_CONFIG + load_balancing_policy: + policies: + - typed_extension_config: + name: envoy.load_balancers.unknown_lb + - typed_extension_config: + name: envoy.load_balancers.custom_lb + load_assignment: + cluster_name: cluster_1 + endpoints: + - lb_endpoints: + - endpoint: + address: + socket_address: + address: 127.0.0.1 + port_value: 8000 + - endpoint: + address: + socket_address: + address: 127.0.0.1 + port_value: 8001 + )EOF"); + + create(parseBootstrapFromV3Yaml(yaml)); + const auto& cluster = cluster_manager_->clusters().getCluster("cluster_1"); + EXPECT_NE(cluster, absl::nullopt); + EXPECT_EQ(cluster->get().info()->loadBalancingPolicy().typed_extension_config().name(), + "envoy.load_balancers.custom_lb"); +} + +// Verify that if Envoy does not have a factory for any of the load balancing policies specified in +// the load balancing policy config, it is an error. +TEST_F(ClusterManagerImplTest, LbPolicyConfigThrowsExceptionIfNoLbPoliciesFound) { + const std::string yaml = fmt::format(R"EOF( + static_resources: + clusters: + - name: cluster_1 + connect_timeout: 0.250s + type: STATIC + lb_policy: LOAD_BALANCING_POLICY_CONFIG + load_balancing_policy: + policies: + - typed_extension_config: + name: envoy.load_balancers.unknown_lb_1 + - typed_extension_config: + name: envoy.load_balancers.unknown_lb_2 + load_assignment: + cluster_name: cluster_1 + endpoints: + - lb_endpoints: + - endpoint: + address: + socket_address: + address: 127.0.0.1 + port_value: 8000 + - endpoint: + address: + socket_address: + address: 127.0.0.1 + port_value: 8001 + )EOF"); + + EXPECT_THROW_WITH_MESSAGE( + create(parseBootstrapFromV3Yaml(yaml)), EnvoyException, + "Didn't find a registered load balancer factory implementation for cluster: 'cluster_1'"); +} + class ClusterManagerImplThreadAwareLbTest : public ClusterManagerImplTest { public: void doTest(LoadBalancerType lb_type) { diff --git a/test/integration/http_subset_lb_integration_test.cc b/test/integration/http_subset_lb_integration_test.cc index 6d9b1d96e363..d32ec0b1a94f 100644 --- a/test/integration/http_subset_lb_integration_test.cc +++ b/test/integration/http_subset_lb_integration_test.cc @@ -15,7 +15,8 @@ class HttpSubsetLbIntegrationTest : public testing::TestWithParam, public HttpIntegrationTest { public: - // Returns all load balancer types except ORIGINAL_DST_LB and CLUSTER_PROVIDED. + // Returns all load balancer types except ORIGINAL_DST_LB, CLUSTER_PROVIDED + // and LOAD_BALANCING_POLICY_CONFIG. static std::vector getSubsetLbTestParams() { int first = static_cast(envoy::config::cluster::v3::Cluster::LbPolicy_MIN); int last = static_cast(envoy::config::cluster::v3::Cluster::LbPolicy_MAX); diff --git a/test/mocks/upstream/cluster_info.h b/test/mocks/upstream/cluster_info.h index 060806a37636..5e5415f88472 100644 --- a/test/mocks/upstream/cluster_info.h +++ b/test/mocks/upstream/cluster_info.h @@ -105,6 +105,9 @@ class MockClusterInfo : public ClusterInfo { (const)); MOCK_METHOD(ProtocolOptionsConfigConstSharedPtr, extensionProtocolOptions, (const std::string&), (const)); + MOCK_METHOD(const envoy::config::cluster::v3::LoadBalancingPolicy_Policy&, loadBalancingPolicy, + (), (const)); + MOCK_METHOD(TypedLoadBalancerFactory*, loadBalancerFactory, (), (const)); MOCK_METHOD(const envoy::config::cluster::v3::Cluster::CommonLbConfig&, lbConfig, (), (const)); MOCK_METHOD(LoadBalancerType, lbType, (), (const)); MOCK_METHOD(envoy::config::cluster::v3::Cluster::DiscoveryType, type, (), (const));