upstream: remove cluster.hosts

api/envoy/api/v2/cds.proto

@@ -176,34 +176,22 @@ message Cluster {
   // when picking a host in the cluster.
   LbPolicy lb_policy = 6 [(validate.rules).enum.defined_only = true];
 
-  // If the service discovery type is
-  // :ref:`STATIC<envoy_api_enum_value_Cluster.DiscoveryType.STATIC>`,
-  // :ref:`STRICT_DNS<envoy_api_enum_value_Cluster.DiscoveryType.STRICT_DNS>`
-  // or :ref:`LOGICAL_DNS<envoy_api_enum_value_Cluster.DiscoveryType.LOGICAL_DNS>`,
-  // then hosts is required.
-  //
-  // .. attention::
-  //
-  //   **This field is deprecated**. Set the
-  //   :ref:`load_assignment<envoy_api_field_Cluster.load_assignment>` field instead.
-  //
-  repeated core.Address hosts = 7 [deprecated = true];
+  reserved 7; // hosts is deprecated by :ref:`load_assignment
+              // <envoy_api_field_Cluster.load_assignment>`
+  reserved "hosts";
 
   // Setting this is required for specifying members of
   // :ref:`STATIC<envoy_api_enum_value_Cluster.DiscoveryType.STATIC>`,
   // :ref:`STRICT_DNS<envoy_api_enum_value_Cluster.DiscoveryType.STRICT_DNS>`
   // or :ref:`LOGICAL_DNS<envoy_api_enum_value_Cluster.DiscoveryType.LOGICAL_DNS>` clusters.
-  // This field supersedes :ref:`hosts<envoy_api_field_Cluster.hosts>` field.
-  // [#comment:TODO(dio): Deprecate the hosts field and add it to DEPRECATED.md
-  // once load_assignment is implemented.]
   //
-  // .. attention::
+  // .. note::
   //
   //   Setting this allows non-EDS cluster types to contain embedded EDS equivalent
   //   :ref:`endpoint assignments<envoy_api_msg_ClusterLoadAssignment>`.
   //   Setting this overrides :ref:`hosts<envoy_api_field_Cluster.hosts>` values.
   //
-  ClusterLoadAssignment load_assignment = 33;
+  ClusterLoadAssignment load_assignment = 33 [(gogoproto.nullable) = false];
 
   // Optional :ref:`active health checking <arch_overview_health_checking>`
   // configuration for the cluster. If no

docs/root/intro/version_history.rst

@@ -72,6 +72,7 @@ Version history
   routing to certain hosts only when there are insufficient healthy hosts available.
 * upstream: add cluster factory to allow creating and registering :ref:`custom cluster type<arch_overview_service_discovery_types_custom>`.
 * upstream: added a :ref:`circuit breaker <arch_overview_circuit_break_cluster_maximum_connection_pools>` to limit the number of concurrent connection pools in use.
+* upstream: removed cluster.hosts.
 * tracing: added :ref:`verbose <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.tracing>` to support logging annotations on spans.
 * upstream: added support for host weighting and :ref:`locality weighting <arch_overview_load_balancing_locality_weighted_lb>` in the :ref:`ring hash load balancer <arch_overview_load_balancing_types_ring_hash>`, and added a :ref:`maximum_ring_size<envoy_api_field_Cluster.RingHashLbConfig.maximum_ring_size>` config parameter to strictly bound the ring size.
 * zookeeper: added a ZooKeeper proxy filter that parses ZooKeeper messages (requests/responses/events).

source/common/config/cds_json.cc

@@ -108,26 +108,14 @@ void CdsJson::translateCluster(const Json::Object& json_cluster,
   cluster.set_name(name);
 
   const std::string string_type = json_cluster.getString("type");
-  auto set_dns_hosts = [&json_cluster, &cluster] {
+  auto set_dns_hosts = [&json_cluster, &cluster, name] {
     const auto hosts = json_cluster.getObjectArray("hosts");
-    std::transform(hosts.cbegin(), hosts.cend(),
-                   Protobuf::RepeatedPtrFieldBackInserter(cluster.mutable_hosts()),
-                   [](const Json::ObjectSharedPtr& host) {
-                     envoy::api::v2::core::Address address;
-                     AddressJson::translateAddress(host->getString("url"), true, false, address);
-                     return address;
-                   });
+    translateClusterHosts(name, hosts, true, false, *cluster.mutable_load_assignment());
   };
   if (string_type == "static") {
     cluster.set_type(envoy::api::v2::Cluster::STATIC);
     const auto hosts = json_cluster.getObjectArray("hosts");
-    std::transform(hosts.cbegin(), hosts.cend(),
-                   Protobuf::RepeatedPtrFieldBackInserter(cluster.mutable_hosts()),
-                   [](const Json::ObjectSharedPtr& host) {
-                     envoy::api::v2::core::Address address;
-                     AddressJson::translateAddress(host->getString("url"), true, true, address);
-                     return address;
-                   });
+    translateClusterHosts(name, hosts, true, true, *cluster.mutable_load_assignment());
   } else if (string_type == "strict_dns") {
     cluster.set_type(envoy::api::v2::Cluster::STRICT_DNS);
     set_dns_hosts();
@@ -221,5 +209,23 @@ void CdsJson::translateCluster(const Json::Object& json_cluster,
   }
 }
 
+void CdsJson::translateClusterHosts(const std::string& cluster_name,
+                                    const std::vector<Json::ObjectSharedPtr>& json_cluster_hosts,
+                                    bool url, bool resolved,
+                                    envoy::api::v2::ClusterLoadAssignment& load_assignment) {
+  // TODO(dio): Early return when json_cluster_hosts is empty.
+  load_assignment.set_cluster_name(cluster_name);
+  auto* locality_lb_endpoints = load_assignment.add_endpoints();
+  // Since this LocalityLbEndpoints is built from hosts list, set the default weight to 1.
+  locality_lb_endpoints->mutable_load_balancing_weight()->set_value(1);
+  for (const Json::ObjectSharedPtr& json_host : json_cluster_hosts) {
+    envoy::api::v2::core::Address address;
+    AddressJson::translateAddress(json_host->getString("url"), url, resolved, address);
+    auto* lb_endpoint = locality_lb_endpoints->add_lb_endpoints();
+    lb_endpoint->mutable_endpoint()->mutable_address()->MergeFrom(address);
+    lb_endpoint->mutable_load_balancing_weight()->set_value(1);
+  }
+}
+
 } // namespace Config
 } // namespace Envoy

source/common/config/cds_json.h

@@ -67,6 +67,23 @@ class CdsJson {
                                const absl::optional<envoy::api::v2::core::ConfigSource>& eds_config,
                                envoy::api::v2::Cluster& cluster,
                                const Stats::StatsOptions& stats_options);
+
+  /**
+   * Translate a v1 JSON Cluster addresses (hosts) vector to v2
+   * envoy::api::v2::ClusterLoadAssignment. This is needed since there are many tests depend on
+   * CdsJson::translateCluster that previously require the deprecated envoy::api::v2::Cluster's
+   * hosts.
+   * @param cluster_name the cluster name.
+   * @param json_cluster_hosts source v1 JSON Cluster Hosts vector.
+   * @param url is json_address a URL? E.g. tcp://<ip>:<port>. If not, it is
+   *            treated as <ip>:<port>.
+   * @param resolved is the address of json_addresses a concrete IP/pipe or unresolved hostname?
+   * @param load assignment destination v2 envoy::api::v2::ClusterLoadAssignment.
+   */
+  static void translateClusterHosts(const std::string& cluster_name,
+                                    const std::vector<Json::ObjectSharedPtr>& json_addresses,
+                                    bool url, bool resolved,
+                                    envoy::api::v2::ClusterLoadAssignment& load_assignment);
 };
 
 } // namespace Config

source/common/config/utility.cc

@@ -268,21 +268,6 @@ Grpc::AsyncClientFactoryPtr Utility::factoryForGrpcApiConfigSource(
   return async_client_manager.factoryForGrpcService(grpc_service, scope, false);
 }
 
-envoy::api::v2::ClusterLoadAssignment Utility::translateClusterHosts(
-    const Protobuf::RepeatedPtrField<envoy::api::v2::core::Address>& hosts) {
-  envoy::api::v2::ClusterLoadAssignment load_assignment;
-  envoy::api::v2::endpoint::LocalityLbEndpoints* locality_lb_endpoints =
-      load_assignment.add_endpoints();
-  // Since this LocalityLbEndpoints is built from hosts list, set the default weight to 1.
-  locality_lb_endpoints->mutable_load_balancing_weight()->set_value(1);
-  for (const envoy::api::v2::core::Address& host : hosts) {
-    envoy::api::v2::endpoint::LbEndpoint* lb_endpoint = locality_lb_endpoints->add_lb_endpoints();
-    lb_endpoint->mutable_endpoint()->mutable_address()->MergeFrom(host);
-    lb_endpoint->mutable_load_balancing_weight()->set_value(1);
-  }
-  return load_assignment;
-}
-
 void Utility::translateOpaqueConfig(const ProtobufWkt::Any& typed_config,
                                     const ProtobufWkt::Struct& config,
                                     Protobuf::Message& out_proto) {

source/common/config/utility.h

@@ -311,14 +311,6 @@ class Utility {
                                 const envoy::api::v2::core::ApiConfigSource& api_config_source,
                                 Stats::Scope& scope);
 
-  /**
-   * Translate a set of cluster's hosts into a load assignment configuration.
-   * @param hosts cluster's list of hosts.
-   * @return envoy::api::v2::ClusterLoadAssignment a load assignment configuration.
-   */
-  static envoy::api::v2::ClusterLoadAssignment
-  translateClusterHosts(const Protobuf::RepeatedPtrField<envoy::api::v2::core::Address>& hosts);
-
   /**
    * Translate opaque config from google.protobuf.Any or google.protobuf.Struct to defined proto
    * message.

source/common/upstream/cluster_manager_impl.cc

@@ -205,6 +205,10 @@ ClusterManagerImpl::ClusterManagerImpl(
     }
   }
 
+  // TODO(dio): make sure cluster's load_assignment cluster name to be the same with its cluster
+  // name, even for static configuration.
+  // https://github.com/envoyproxy/envoy/pull/4123#issuecomment-412693859.
+
   // Now setup ADS if needed, this might rely on a primary cluster.
   if (bootstrap.dynamic_resources().has_ads_config()) {
     ads_mux_ = std::make_unique<Config::GrpcMuxImpl>(

source/common/upstream/health_discovery_service.cc

@@ -130,10 +130,13 @@ void HdsDelegate::processMessage(
     cluster_config.mutable_per_connection_buffer_limit_bytes()->set_value(
         ClusterConnectionBufferLimitBytes);
 
-    // Add endpoints to cluster
+    // Add endpoints to the cluster.
+    auto* load_assignment = cluster_config.mutable_load_assignment();
+    auto* locality_lb_endpoints = load_assignment->add_endpoints();
     for (const auto& locality_endpoints : cluster_health_check.locality_endpoints()) {
       for (const auto& endpoint : locality_endpoints.endpoints()) {
-        cluster_config.add_hosts()->MergeFrom(endpoint.address());
+        auto* lb_endpoint = locality_lb_endpoints->add_lb_endpoints();
+        lb_endpoint->mutable_endpoint()->MergeFrom(endpoint);
       }
     }
 
@@ -207,13 +210,16 @@ HdsCluster::HdsCluster(Server::Admin& admin, Runtime::Loader& runtime,
                                           ssl_context_manager_, added_via_api_, cm, local_info,
                                           dispatcher, random, singleton_manager, tls, api});
 
-  for (const auto& host : cluster.hosts()) {
-    initial_hosts_->emplace_back(
-        new HostImpl(info_, "", Network::Address::resolveProtoAddress(host),
-                     envoy::api::v2::core::Metadata::default_instance(), 1,
-                     envoy::api::v2::core::Locality().default_instance(),
-                     envoy::api::v2::endpoint::Endpoint::HealthCheckConfig().default_instance(), 0,
-                     envoy::api::v2::core::HealthStatus::UNKNOWN));
+  const auto& locality_lb_endpoints = cluster.load_assignment().endpoints();
+  for (const auto& locality_lb_endpoint : locality_lb_endpoints) {
+    for (const auto& lb_endpoint : locality_lb_endpoint.lb_endpoints()) {
+      const auto& host = lb_endpoint.endpoint().address();
+      // TODO(dio): Check if initial weight and priority need to be inferred from the config.
+      initial_hosts_->emplace_back(new HostImpl(
+          info_, "", Network::Address::resolveProtoAddress(host), lb_endpoint.metadata(), 1,
+          locality_lb_endpoint.locality(), lb_endpoint.endpoint().health_check_config(), 0,
+          envoy::api::v2::core::HealthStatus::UNKNOWN));
+    }
   }
   initialize([] {});
 }

source/common/upstream/logical_dns_cluster.cc

@@ -28,18 +28,11 @@ LogicalDnsCluster::LogicalDnsCluster(
           std::chrono::milliseconds(PROTOBUF_GET_MS_OR_DEFAULT(cluster, dns_refresh_rate, 5000))),
       tls_(tls.allocateSlot()), resolve_timer_(factory_context.dispatcher().createTimer(
                                     [this]() -> void { startResolve(); })),
-      local_info_(factory_context.localInfo()),
-      load_assignment_(cluster.has_load_assignment()
-                           ? cluster.load_assignment()
-                           : Config::Utility::translateClusterHosts(cluster.hosts())) {
+      local_info_(factory_context.localInfo()), load_assignment_(cluster.load_assignment()) {
   const auto& locality_lb_endpoints = load_assignment_.endpoints();
   if (locality_lb_endpoints.size() != 1 || locality_lb_endpoints[0].lb_endpoints().size() != 1) {
-    if (cluster.has_load_assignment()) {
-      throw EnvoyException(
-          "LOGICAL_DNS clusters must have a single locality_lb_endpoint and a single lb_endpoint");
-    } else {
-      throw EnvoyException("LOGICAL_DNS clusters must have a single host");
-    }
+    throw EnvoyException(
+        "LOGICAL_DNS clusters must have a single locality_lb_endpoint and a single lb_endpoint");
   }
 
   switch (cluster.dns_lookup_family()) {

source/common/upstream/upstream_impl.cc

@@ -1019,11 +1019,8 @@ StaticClusterImpl::StaticClusterImpl(
     : ClusterImplBase(cluster, runtime, factory_context, std::move(stats_scope), added_via_api),
       priority_state_manager_(
           new PriorityStateManager(*this, factory_context.localInfo(), nullptr)) {
-  // TODO(dio): Use by-reference when cluster.hosts() is removed.
-  const envoy::api::v2::ClusterLoadAssignment cluster_load_assignment(
-      cluster.has_load_assignment() ? cluster.load_assignment()
-                                    : Config::Utility::translateClusterHosts(cluster.hosts()));
 
+  const auto& cluster_load_assignment = cluster.load_assignment();
   overprovisioning_factor_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(
       cluster_load_assignment.policy(), overprovisioning_factor, kDefaultOverProvisioningFactor);
 
@@ -1261,11 +1258,8 @@ StrictDnsClusterImpl::StrictDnsClusterImpl(
     NOT_REACHED_GCOVR_EXCL_LINE;
   }
 
-  const envoy::api::v2::ClusterLoadAssignment load_assignment(
-      cluster.has_load_assignment() ? cluster.load_assignment()
-                                    : Config::Utility::translateClusterHosts(cluster.hosts()));
-  const auto& locality_lb_endpoints = load_assignment.endpoints();
-  for (const auto& locality_lb_endpoint : locality_lb_endpoints) {
+  const auto& load_assignment = cluster.load_assignment();
+  for (const auto& locality_lb_endpoint : load_assignment.endpoints()) {
     for (const auto& lb_endpoint : locality_lb_endpoint.lb_endpoints()) {
       const auto& host = lb_endpoint.endpoint().address();
       const std::string& url = fmt::format("tcp://{}:{}", host.socket_address().address(),

test/common/config/utility_test.cc

@@ -216,7 +216,9 @@ TEST(UtilityTest, UnixClusterStatic) {
   envoy::api::v2::core::ConfigSource eds_config;
   Stats::StatsOptionsImpl stats_options;
   Config::CdsJson::translateCluster(*json_object_ptr, eds_config, cluster, stats_options);
-  EXPECT_EQ("/test.sock", cluster.hosts(0).pipe().path());
+  EXPECT_EQ(
+      "/test.sock",
+      cluster.load_assignment().endpoints(0).lb_endpoints(0).endpoint().address().pipe().path());
 }
 
 TEST(UtilityTest, CheckFilesystemSubscriptionBackingPath) {

test/common/upstream/cluster_manager_impl_test.cc

@@ -209,13 +209,20 @@ class ClusterManagerImplTest : public testing::Test {
       connect_timeout: 0.250s
       type: STATIC
       lb_policy: ROUND_ROBIN
-      hosts:
-      - socket_address:
-          address: "127.0.0.1"
-          port_value: 11001
-      - socket_address:
-          address: "127.0.0.1"
-          port_value: 11002
+      load_assignment:
+        cluster_name: cluster_1
+        endpoints:
+          - lb_endpoints:
+              - endpoint:
+                  address:
+                    socket_address:
+                      address: 127.0.0.1
+                      port_value: 11001
+              - endpoint:
+                  address:
+                    socket_address:
+                      address: 127.0.0.1
+                      port_value: 11002
   )EOF";
     const std::string merge_window_enabled = R"EOF(
       common_lb_config:

test/common/upstream/logical_dns_cluster_test.cc

@@ -300,8 +300,11 @@ TEST_F(LogicalDnsClusterTest, BadConfig) {
   }
   )EOF";
 
-  EXPECT_THROW_WITH_MESSAGE(setupFromV1Json(multiple_hosts_json), EnvoyException,
-                            "LOGICAL_DNS clusters must have a single host");
+  EXPECT_THROW_WITH_MESSAGE(
+      setupFromV1Json(multiple_hosts_json), EnvoyException,
+      // Since Envoy::Config::CdsJson now translates hosts to load_assignment, hence the message is
+      // changed.
+      "LOGICAL_DNS clusters must have a single locality_lb_endpoint and a single lb_endpoint");
 
   const std::string multiple_lb_endpoints_yaml = R"EOF(
   name: name
@@ -389,10 +392,15 @@ TEST_F(LogicalDnsClusterTest, Basic) {
   # Since the following expectResolve() requires Network::DnsLookupFamily::V4Only we need to set
   # dns_lookup_family to V4_ONLY explicitly for v2 .yaml config.
   dns_lookup_family: V4_ONLY
-  hosts:
-  - socket_address:
-      address: foo.bar.com
-      port_value: 443
+  load_assignment:
+    cluster_name: name
+    endpoints:
+      - lb_endpoints:
+          - endpoint:
+              address:
+                socket_address:
+                  address: foo.bar.com
+                  port_value: 443
   )EOF";
 
   const std::string basic_yaml_load_assignment = R"EOF(