api: deprecate hosts in Cluster. (envoyproxy#9663)

Now that we have stable Envoy API versioning, deprecate in v2, with the understanding that we won't ever make this a runtime controlled field. Risk level: Low Testing: CI Signed-off-by: Harvey Tuch <[email protected]>
hango-io · Jan 15, 2020 · ab96519 · ab96519
1 parent 7bd0c07
commit ab96519
Show file tree

Hide file tree

Showing 39 changed files with 333 additions and 197 deletions.
diff --git a/api/envoy/api/v2/cluster.proto b/api/envoy/api/v2/cluster.proto
@@ -551,7 +551,7 @@ message Cluster {
   //   **This field is deprecated**. Set the
   //   :ref:`load_assignment<envoy_api_field_Cluster.load_assignment>` field instead.
   //
-  repeated core.Address hosts = 7;
+  repeated core.Address hosts = 7 [deprecated = true];
 
   // Setting this is required for specifying members of
   // :ref:`STATIC<envoy_api_enum_value_Cluster.DiscoveryType.STATIC>`,

diff --git a/api/envoy/config/cluster/v3alpha/cluster.proto b/api/envoy/config/cluster/v3alpha/cluster.proto
@@ -491,9 +491,9 @@ message Cluster {
     google.protobuf.Duration max_interval = 2 [(validate.rules).duration = {gt {nanos: 1000000}}];
   }
 
-  reserved 12, 15, 11, 35;
+  reserved 12, 15, 7, 11, 35;
 
-  reserved "tls_context", "extension_protocol_options";
+  reserved "hosts", "tls_context", "extension_protocol_options";
 
   // Configuration to use different transport sockets for different endpoints.
   // The entry of *envoy.transport_socket* in the
@@ -578,21 +578,6 @@ 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_config.cluster.v3alpha.Cluster.DiscoveryType.STATIC>`,
-  // :ref:`STRICT_DNS<envoy_api_enum_value_config.cluster.v3alpha.Cluster.DiscoveryType.STRICT_DNS>`
-  // or
-  // :ref:`LOGICAL_DNS<envoy_api_enum_value_config.cluster.v3alpha.Cluster.DiscoveryType.LOGICAL_DNS>`,
-  // then hosts is required.
-  //
-  // .. attention::
-  //
-  //   **This field is deprecated**. Set the
-  //   :ref:`load_assignment<envoy_api_field_config.cluster.v3alpha.Cluster.load_assignment>` field
-  //   instead.
-  //
-  repeated core.v3alpha.Address hosts = 7;
-
   // Setting this is required for specifying members of
   // :ref:`STATIC<envoy_api_enum_value_config.cluster.v3alpha.Cluster.DiscoveryType.STATIC>`,
   // :ref:`STRICT_DNS<envoy_api_enum_value_config.cluster.v3alpha.Cluster.DiscoveryType.STRICT_DNS>`

diff --git a/examples/grpc-bridge/server/envoy-proxy.yaml b/examples/grpc-bridge/server/envoy-proxy.yaml
@@ -37,10 +37,15 @@ static_resources:
     type: strict_dns
     lb_policy: round_robin
     http2_protocol_options: {}
-    hosts:
-    - socket_address:
-        address: kv-backend-service
-        port_value: 8081
+    load_assignment:
+      cluster_name: backend_grpc_service
+      endpoints:
+      - lb_endpoints:
+        - endpoint:
+            address:
+              socket_address:
+                address: kv-backend-service
+                port_value: 8081
 
 admin:
   access_log_path: "/tmp/admin_access.log"

diff --git a/generated_api_shadow/envoy/api/v2/cluster.proto b/generated_api_shadow/envoy/api/v2/cluster.proto
diff --git a/generated_api_shadow/envoy/config/cluster/v3alpha/cluster.proto b/generated_api_shadow/envoy/config/cluster/v3alpha/cluster.proto
diff --git a/source/common/config/version_converter.cc b/source/common/config/version_converter.cc
@@ -115,7 +115,7 @@ void annotateWithOriginalType(const Protobuf::Descriptor& prev_descriptor,
       // ClusterManagerImpl with type erasure, but protobuf doesn't free up memory
       // as expected, we probably need some arena level trick to address this.
       if (prev_descriptor.full_name() == "envoy.api.v2.Cluster" &&
-          (field.name() == "hosts" || field.name() == "load_assignment")) {
+          (field.name() == "hidden_envoy_deprecated_hosts" || field.name() == "load_assignment")) {
         // This will cause the sub-message visit to abort early.
         return field.message_type();
       }

diff --git a/source/common/upstream/health_discovery_service.cc b/source/common/upstream/health_discovery_service.cc
@@ -152,9 +152,11 @@ void HdsDelegate::processMessage(
         ClusterConnectionBufferLimitBytes);
 
     // Add endpoints to cluster
+    auto* endpoints = cluster_config.mutable_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());
+        endpoints->add_lb_endpoints()->mutable_endpoint()->mutable_address()->MergeFrom(
+            endpoint.address());
       }
     }
 
@@ -231,9 +233,9 @@ HdsCluster::HdsCluster(Server::Admin& admin, Runtime::Loader& runtime,
       {admin, runtime_, cluster_, bind_config_, stats_, ssl_context_manager_, added_via_api_, cm,
        local_info, dispatcher, random, singleton_manager, tls, validation_visitor, api});
 
-  for (const auto& host : cluster.hosts()) {
+  for (const auto& host : cluster.load_assignment().endpoints(0).lb_endpoints()) {
     initial_hosts_->emplace_back(new HostImpl(
-        info_, "", Network::Address::resolveProtoAddress(host),
+        info_, "", Network::Address::resolveProtoAddress(host.endpoint().address()),
         envoy::config::core::v3alpha::Metadata::default_instance(), 1,
         envoy::config::core::v3alpha::Locality().default_instance(),
         envoy::config::endpoint::v3alpha::Endpoint::HealthCheckConfig().default_instance(), 0,

diff --git a/source/common/upstream/logical_dns_cluster.cc b/source/common/upstream/logical_dns_cluster.cc
@@ -55,9 +55,10 @@ LogicalDnsCluster::LogicalDnsCluster(
       resolve_timer_(
           factory_context.dispatcher().createTimer([this]() -> void { startResolve(); })),
       local_info_(factory_context.localInfo()),
-      load_assignment_(cluster.has_load_assignment()
-                           ? convertPriority(cluster.load_assignment())
-                           : Config::Utility::translateClusterHosts(cluster.hosts())) {
+      load_assignment_(
+          cluster.has_load_assignment()
+              ? convertPriority(cluster.load_assignment())
+              : Config::Utility::translateClusterHosts(cluster.hidden_envoy_deprecated_hosts())) {
   failure_backoff_strategy_ = Config::Utility::prepareDnsRefreshStrategy(
       cluster, dns_refresh_rate_ms_.count(), factory_context.random());
 

diff --git a/source/common/upstream/original_dst_cluster.cc b/source/common/upstream/original_dst_cluster.cc
@@ -116,7 +116,7 @@ OriginalDstCluster::OriginalDstCluster(
                            : false),
       host_map_(std::make_shared<HostMap>()) {
   // TODO(dio): Remove hosts check once the hosts field is removed.
-  if (config.has_load_assignment() || !config.hosts().empty()) {
+  if (config.has_load_assignment() || !config.hidden_envoy_deprecated_hosts().empty()) {
     throw EnvoyException("ORIGINAL_DST clusters must have no load assignment or hosts configured");
   }
   cleanup_timer_->enableTimer(cleanup_interval_ms_);

diff --git a/source/common/upstream/static_cluster.cc b/source/common/upstream/static_cluster.cc
@@ -16,8 +16,9 @@ StaticClusterImpl::StaticClusterImpl(
           new PriorityStateManager(*this, factory_context.localInfo(), nullptr)) {
   // TODO(dio): Use by-reference when cluster.hosts() is removed.
   const envoy::config::endpoint::v3alpha::ClusterLoadAssignment cluster_load_assignment(
-      cluster.has_load_assignment() ? cluster.load_assignment()
-                                    : Config::Utility::translateClusterHosts(cluster.hosts()));
+      cluster.has_load_assignment()
+          ? cluster.load_assignment()
+          : Config::Utility::translateClusterHosts(cluster.hidden_envoy_deprecated_hosts()));
 
   overprovisioning_factor_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(
       cluster_load_assignment.policy(), overprovisioning_factor, kDefaultOverProvisioningFactor);

diff --git a/source/common/upstream/strict_dns_cluster.cc b/source/common/upstream/strict_dns_cluster.cc
@@ -24,8 +24,9 @@ StrictDnsClusterImpl::StrictDnsClusterImpl(
 
   std::list<ResolveTargetPtr> resolve_targets;
   const envoy::config::endpoint::v3alpha::ClusterLoadAssignment load_assignment(
-      cluster.has_load_assignment() ? cluster.load_assignment()
-                                    : Config::Utility::translateClusterHosts(cluster.hosts()));
+      cluster.has_load_assignment()
+          ? cluster.load_assignment()
+          : Config::Utility::translateClusterHosts(cluster.hidden_envoy_deprecated_hosts()));
   const auto& locality_lb_endpoints = load_assignment.endpoints();
   for (const auto& locality_lb_endpoint : locality_lb_endpoints) {
     validateEndpointsForZoneAwareRouting(locality_lb_endpoint);

diff --git a/source/extensions/clusters/redis/redis_cluster.cc b/source/extensions/clusters/redis/redis_cluster.cc
@@ -41,9 +41,10 @@ RedisCluster::RedisCluster(
       host_degraded_refresh_threshold_(redis_cluster.host_degraded_refresh_threshold()),
       dispatcher_(factory_context.dispatcher()), dns_resolver_(std::move(dns_resolver)),
       dns_lookup_family_(Upstream::getDnsLookupFamilyFromCluster(cluster)),
-      load_assignment_(cluster.has_load_assignment()
-                           ? cluster.load_assignment()
-                           : Config::Utility::translateClusterHosts(cluster.hosts())),
+      load_assignment_(
+          cluster.has_load_assignment()
+              ? cluster.load_assignment()
+              : Config::Utility::translateClusterHosts(cluster.hidden_envoy_deprecated_hosts())),
       local_info_(factory_context.localInfo()), random_(factory_context.random()),
       redis_discovery_session_(*this, redis_client_factory), lb_factory_(std::move(lb_factory)),
       auth_password_(

diff --git a/test/common/config/version_converter_test.cc b/test/common/config/version_converter_test.cc
@@ -41,8 +41,8 @@ TEST(VersionConverterTest, Upgrade) {
   VersionConverter::upgrade(source, dst);
   // Verify fields in v3 Cluster.
   EXPECT_TRUE(hasOriginalTypeInformation(dst));
-  EXPECT_FALSE(dst.hosts().empty());
-  EXPECT_FALSE(hasOriginalTypeInformation(dst.hosts(0)));
+  EXPECT_FALSE(dst.hidden_envoy_deprecated_hosts().empty());
+  EXPECT_FALSE(hasOriginalTypeInformation(dst.hidden_envoy_deprecated_hosts(0)));
   EXPECT_EQ("bar", dst.load_assignment().cluster_name());
   EXPECT_FALSE(hasOriginalTypeInformation(dst.load_assignment()));
   EXPECT_EQ("foo", dst.eds_cluster_config().service_name());

diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc
@@ -1566,9 +1566,14 @@ TEST_F(StaticClusterImplTest, UnsupportedLBType) {
     connect_timeout: 0.25s
     type: static
     lb_policy: fakelbtype
-    hosts:
-    - { socket_address: { address: 192.168.1.1, port_value: 22 }}
-    - { socket_address: { address: 192.168.1.2, port_value: 44 }}
+    load_assignment:
+      cluster_name: addressportconfig
+      endpoints:
+      - lb_endpoints:
+        - endpoint:
+            address:
+              socket_address: { address: 192.168.1.1, port_value: 22 }
+              socket_address: { address: 192.168.1.2, port_value: 44 }
   )EOF";
 
   EXPECT_THROW_WITH_MESSAGE(

diff --git a/test/config/integration/google_com_proxy_port_0.v2.yaml b/test/config/integration/google_com_proxy_port_0.v2.yaml
@@ -34,4 +34,9 @@ static_resources:
     # Comment out the following line to test on v6 networks
     dns_lookup_family: {{ dns_lookup_family }}
     lb_policy: ROUND_ROBIN
-    hosts: [{ socket_address: { address: google.com, port_value: 443 }}]
+    load_assignment:
+      cluster_name: service_google
+      endpoints:
+      - lb_endpoints:
+        - endpoint:
+            address: { socket_address: { address: google.com, port_value: 443 }}
diff --git a/test/config/integration/server.yaml b/test/config/integration/server.yaml
@@ -87,43 +87,68 @@ static_resources:
   clusters:
   - name: cluster_1
     connect_timeout: 5s
-    hosts:
-    - socket_address:
-        address: {{ ip_loopback_address }}
-        port_value: {{ upstream_0 }}
+    load_assignment:
+      cluster_name: cluster_1
+      endpoints:
+      - lb_endpoints:
+        - endpoint:
+            address:
+              socket_address:
+                address: {{ ip_loopback_address }}
+                port_value: {{ upstream_0 }}
     dns_lookup_family: "{{ dns_lookup_family }}"
   - name: cluster_2
     type: STRICT_DNS
     connect_timeout: 5s
-    hosts:
-    - socket_address:
-        address: localhost
-        port_value: {{ upstream_1 }}
+    load_assignment:
+      cluster_name: cluster_2
+      endpoints:
+      - lb_endpoints:
+        - endpoint:
+            address:
+              socket_address:
+                address: localhost
+                port_value: {{ upstream_1 }}
     dns_lookup_family: "{{ dns_lookup_family }}"
   - name: cluster_3
     connect_timeout: 5s
     per_connection_buffer_limit_bytes: 1024
-    hosts:
-    - socket_address:
-        address: {{ ip_loopback_address }}
-        port_value: {{ upstream_0 }}
+    load_assignment:
+      cluster_name: cluster_3
+      endpoints:
+      - lb_endpoints:
+        - endpoint:
+            address:
+              socket_address:
+                address: {{ ip_loopback_address }}
+                port_value: {{ upstream_0 }}
     dns_lookup_family: "{{ dns_lookup_family }}"
   - name: statsd
     type: STRICT_DNS
     connect_timeout: 5s
-    hosts:
-    - socket_address:
-        address: localhost
-        port_value: 4
+    load_assignment:
+      cluster_name: statsd
+      endpoints:
+      - lb_endpoints:
+        - endpoint:
+            address:
+              socket_address:
+                address: localhost
+                port_value: 4
     dns_lookup_family: "{{ dns_lookup_family }}"
   - name: redis
     type: STRICT_DNS
     connect_timeout: 5s
     lb_policy: RING_HASH
-    hosts:
-    - socket_address:
-        address: localhost
-        port_value: 4
+    load_assignment:
+      cluster_name: redis
+      endpoints:
+      - lb_endpoints:
+        - endpoint:
+            address:
+              socket_address:
+                address: localhost
+                port_value: 4
     dns_lookup_family: "{{ dns_lookup_family }}"
     outlier_detection: {}
 dynamic_resources: {}

diff --git a/test/config/integration/server_unix_listener.yaml b/test/config/integration/server_unix_listener.yaml
@@ -26,10 +26,15 @@ static_resources:
   clusters:
   - name: cluster_0
     connect_timeout: 5s
-    hosts:
-    - socket_address:
-        address: "{{ ip_loopback_address }}"
-        port_value: 0
+    load_assignment:
+      cluster_name: cluster_0
+      endpoints:
+      - lb_endpoints:
+        - endpoint:
+            address:
+              socket_address:
+                address: "{{ ip_loopback_address }}"
+                port_value: 0
     dns_lookup_family: V4_ONLY
 cluster_manager: {}
 watchdog: {}