subset lb: allow ring hash/maglev LB to work with subsets

Skip initializing the thread aware LB for a cluster when the subset
load balancer is enabled. Also adds some extra checks for LB policies
that are incompatible with the subset load balancer.

Risk Level: low
Testing: test additional checks
Docs Changes: updated docs w.r.t subset lb compatibility
Release Notes: n/a
Fixes: envoyproxy#7651

Signed-off-by: Stephan Zuercher <zuercher@gmail.com>

docs/root/intro/arch_overview/upstream/load_balancing/subsets.rst

@@ -35,12 +35,20 @@ therefore, contain a definition that has the same keys as a given route in order
 balancing to occur.
 
 This feature can only be enabled using the V2 configuration API. Furthermore, host metadata is only
-supported when using the EDS discovery type for clusters. Host metadata for subset load balancing
-must be placed under the filter name ``"envoy.lb"``. Similarly, route metadata match criteria use
-the ``"envoy.lb"`` filter name. Host metadata may be hierarchical (e.g., the value for a top-level
-key may be a structured value or list), but the subset load balancer only compares top-level keys
-and values. Therefore when using structured values, a route's match criteria will only match if an
-identical structured value appears in the host's metadata.
+supported when hosts are defined using
+:ref:`ClusterLoadAssignments <envoy_api_msg_ClusterLoadAssignment>`. ClusterLoadAssignments are
+available via EDS or the Cluster :ref:`load_assignment <envoy_api_field_Cluster.load_assignment>`
+field. Host metadata for subset load balancing must be placed under the filter name ``"envoy.lb"``.
+Similarly, route metadata match criteria use ``"envoy.lb"`` filter name. Host metadata may be
+hierarchical (e.g., the value for a top-level key may be a structured value or list), but the
+subset load balancer only compares top-level keys and values. Therefore when using structured
+values, a route's match criteria will only match if an identical structured value appears in the
+host's metadata.
+
+Finally, note that subset load balancing is not available for the
+:ref:`ORIGINAL_DST_LB <envoy_api_enum_value_Cluster.LbPolicy.ORIGINAL_DST_LB>` or
+:ref:`CLUSTER_PROVIDED <envoy_api_enum_value_Cluster.LbPolicy.CLUSTER_PROVIDED>` load balancer
+policies.
 
 Examples
 ^^^^^^^^

source/common/upstream/cluster_manager_impl.cc

@@ -640,17 +640,21 @@ void ClusterManagerImpl::loadCluster(const envoy::api::v2::Cluster& cluster,
   const auto cluster_entry_it = cluster_map.find(cluster_reference.info()->name());
 
   // If an LB is thread aware, create it here. The LB is not initialized until cluster pre-init
-  // finishes.
+  // finishes. For RingHash/Maglev don't create the LB here if subset balancing is enabled.
   if (cluster_reference.info()->lbType() == LoadBalancerType::RingHash) {
-    cluster_entry_it->second->thread_aware_lb_ = std::make_unique<RingHashLoadBalancer>(
-        cluster_reference.prioritySet(), cluster_reference.info()->stats(),
-        cluster_reference.info()->statsScope(), runtime_, random_,
-        cluster_reference.info()->lbRingHashConfig(), cluster_reference.info()->lbConfig());
+    if (!cluster_reference.info()->lbSubsetInfo().isEnabled()) {
+      cluster_entry_it->second->thread_aware_lb_ = std::make_unique<RingHashLoadBalancer>(
+          cluster_reference.prioritySet(), cluster_reference.info()->stats(),
+          cluster_reference.info()->statsScope(), runtime_, random_,
+          cluster_reference.info()->lbRingHashConfig(), cluster_reference.info()->lbConfig());
+    }
   } else if (cluster_reference.info()->lbType() == LoadBalancerType::Maglev) {
-    cluster_entry_it->second->thread_aware_lb_ = std::make_unique<MaglevLoadBalancer>(
-        cluster_reference.prioritySet(), cluster_reference.info()->stats(),
-        cluster_reference.info()->statsScope(), runtime_, random_,
-        cluster_reference.info()->lbConfig());
+    if (!cluster_reference.info()->lbSubsetInfo().isEnabled()) {
+      cluster_entry_it->second->thread_aware_lb_ = std::make_unique<MaglevLoadBalancer>(
+          cluster_reference.prioritySet(), cluster_reference.info()->stats(),
+          cluster_reference.info()->statsScope(), runtime_, random_,
+          cluster_reference.info()->lbConfig());
+    }
   } else if (cluster_reference.info()->lbType() == LoadBalancerType::ClusterProvided) {
     cluster_entry_it->second->thread_aware_lb_ = std::move(new_cluster_pair.second);
   }

source/common/upstream/upstream_impl.cc

@@ -643,12 +643,24 @@ ClusterInfoImpl::ClusterInfoImpl(
                       envoy::api::v2::Cluster_LbPolicy_Name(config.lb_policy()),
                       envoy::api::v2::Cluster_DiscoveryType_Name(config.type())));
     }
+    if (config.has_lb_subset_config()) {
+      throw EnvoyException(
+          fmt::format("cluster: LB policy {} cannot be combined with lb_subset_config",
+                      envoy::api::v2::Cluster_LbPolicy_Name(config.lb_policy())));
+    }
+
     lb_type_ = LoadBalancerType::ClusterProvided;
     break;
   case envoy::api::v2::Cluster::MAGLEV:
     lb_type_ = LoadBalancerType::Maglev;
     break;
   case envoy::api::v2::Cluster::CLUSTER_PROVIDED:
+    if (config.has_lb_subset_config()) {
+      throw EnvoyException(
+          fmt::format("cluster: LB policy {} cannot be combined with lb_subset_config",
+                      envoy::api::v2::Cluster_LbPolicy_Name(config.lb_policy())));
+    }
+
     lb_type_ = LoadBalancerType::ClusterProvided;
     break;
   default:

test/common/upstream/cluster_manager_impl_test.cc

@@ -610,7 +610,7 @@ TEST_F(ClusterManagerImplTest, SubsetLoadBalancerInitialization) {
   factory_.tls_.shutdownThread();
 }
 
-TEST_F(ClusterManagerImplTest, SubsetLoadBalancerRestriction) {
+TEST_F(ClusterManagerImplTest, SubsetLoadBalancerOriginalDstRestriction) {
   const std::string yaml = R"EOF(
 static_resources:
   clusters:
@@ -631,6 +631,27 @@ TEST_F(ClusterManagerImplTest, SubsetLoadBalancerRestriction) {
       "cluster: cluster type 'original_dst' may not be used with lb_subset_config");
 }
 
+TEST_F(ClusterManagerImplTest, SubsetLoadBalancerClusterProvidedLbRestriction) {
+  const std::string yaml = R"EOF(
+static_resources:
+  clusters:
+  - name: cluster_1
+    connect_timeout: 0.250s
+    type: static
+    lb_policy: cluster_provided
+  )EOF";
+
+  envoy::config::bootstrap::v2::Bootstrap bootstrap = parseBootstrapFromV2Yaml(yaml);
+  envoy::api::v2::Cluster::LbSubsetConfig* subset_config =
+      bootstrap.mutable_static_resources()->mutable_clusters(0)->mutable_lb_subset_config();
+  subset_config->set_fallback_policy(envoy::api::v2::Cluster::LbSubsetConfig::ANY_ENDPOINT);
+  subset_config->add_subset_selectors()->add_keys("x");
+
+  EXPECT_THROW_WITH_MESSAGE(
+      create(bootstrap), EnvoyException,
+      "cluster: LB policy CLUSTER_PROVIDED cannot be combined with lb_subset_config");
+}
+
 TEST_F(ClusterManagerImplTest, SubsetLoadBalancerLocalityAware) {
   const std::string yaml = R"EOF(
 static_resources: