Skip to content

api: deprecate hosts in Cluster. #9663

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
htuch merged 9 commits into from
Jan 15, 2020
Merged

api: deprecate hosts in Cluster. #9663

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c6e4cc7
api: deprecate hosts in Cluster.
htuch Jan 12, 2020
f349baa
All tests pass with --define deprecated_features=disabled.
htuch Jan 14, 2020
e397703
Bump upper bound on host memory for Mac.
htuch Jan 14, 2020
22d5dfc
Merge remote-tracking branch 'upstream/master' into deprecate-hosts
htuch Jan 14, 2020
818522e
Review feedback.
htuch Jan 14, 2020
868253f
Fix.
htuch Jan 14, 2020
73c4d23
Review feedback.
htuch Jan 15, 2020
207a3f6
Fix spelling.
htuch Jan 15, 2020
2266a4e
Merge remote-tracking branch 'upstream/master' into deprecate-hosts
htuch Jan 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/envoy/api/v2/cluster.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>`,
Expand Down
19 changes: 2 additions & 17 deletions api/envoy/config/cluster/v3alpha/cluster.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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>`
Expand Down
13 changes: 9 additions & 4 deletions examples/grpc-bridge/server/envoy-proxy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion generated_api_shadow/envoy/api/v2/cluster.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion generated_api_shadow/envoy/config/cluster/v3alpha/cluster.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion source/common/config/version_converter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
4 changes: 2 additions & 2 deletions source/common/upstream/health_discovery_service.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ void HdsDelegate::processMessage(
// Add endpoints to cluster
for (const auto& locality_endpoints : cluster_health_check.locality_endpoints()) {
for (const auto& endpoint : locality_endpoints.endpoints()) {
cluster_config.add_hosts()->MergeFrom(endpoint.address());
cluster_config.add_hidden_envoy_deprecated_hosts()->MergeFrom(endpoint.address());
Comment thread
htuch marked this conversation as resolved.
Outdated
}
}

Expand Down Expand Up @@ -231,7 +231,7 @@ 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.hidden_envoy_deprecated_hosts()) {
initial_hosts_->emplace_back(new HostImpl(
info_, "", Network::Address::resolveProtoAddress(host),
envoy::config::core::v3alpha::Metadata::default_instance(), 1,
Expand Down
7 changes: 4 additions & 3 deletions source/common/upstream/logical_dns_cluster.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());

Expand Down
2 changes: 1 addition & 1 deletion source/common/upstream/original_dst_cluster.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {

@alyssawilk alyssawilk Jan 15, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that the point of doing conversion was to not have to deal with the old style things in Envoy
Is this due to the special case TODO above?

@htuch htuch Jan 15, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, in this case, we don't do any conversion from hosts --> load assignment, since neither of these are used by original dst clusters. So, here we just check that both are not set.

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_);
Expand Down
5 changes: 3 additions & 2 deletions source/common/upstream/static_cluster.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
5 changes: 3 additions & 2 deletions source/common/upstream/strict_dns_cluster.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
7 changes: 4 additions & 3 deletions source/extensions/clusters/redis/redis_cluster.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_(
Expand Down
4 changes: 2 additions & 2 deletions test/common/config/version_converter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
2 changes: 1 addition & 1 deletion test/common/upstream/upstream_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1566,7 +1566,7 @@ TEST_F(StaticClusterImplTest, UnsupportedLBType) {
connect_timeout: 0.25s
type: static
lb_policy: fakelbtype
hosts:
hidden_envoy_deprecated_hosts:

@mattklein123 mattklein123 Jan 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be testing this field like this? Either convert or use the deprecated field with a deprecated field test? (We should make sure we still have tests that cover the conversion path)

@htuch htuch Jan 14, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this one. There are actually a bunch of places where hosts: still appears in this file. These are never going to be deprecated tests as I don't think we plan to turn down v2 hosts until v2 disappears. So, I think we have sufficient coverage. The reason this specific test was problematic was due to this being upgraded to v3 due to an invalid LB type and then failing the config check for the wrong reason at v3.

@mattklein123 mattklein123 Jan 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused, doesn't marking it deprecated cause the fail on warn tests to fail? Or is the issue that those tests don't run through the validation path? I would expect to see a DEPRECATED_FEATURE_TEST here unless the config is not going through the validation path?

@htuch htuch Jan 15, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would fail due to this, except it fails even earlier, because lb_policy has an invalid value (which is what the test is trying to catch).

@mattklein123 mattklein123 Jan 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a follow up shouldn't we fix all these tests even though they fail for some other reason? Mainly just for reduced confusion in the future?

@htuch htuch Jan 15, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For hosts specifically, I think we are supporting this forever for v2, we're not going to disable-by-default, so it's fine to keep using this. We probably do need a bucket of work in moving the trailing example configs/docs/unit tests that still use v2 to v3, but I think that's out of scope for this PR.

@mattklein123 mattklein123 Jan 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are talking past each other here, even without disable-by-default, we still have DEPRECATED_FEATURE_TEST and a compile time options build that fails on any use of a deprecated feature, whether disabled by default or not. My point is that I would expect any use of host to fail in this build, but I think it's failing for other reasons. We should still clean up hosts in these tests otherwise it's debt that will be confusing later. cc @alyssawilk for further comment.

@alyssawilk alyssawilk Jan 15, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, basically I think this config would be fatal anyway, but as Matt points out, we hit the fatal condition we care about first so it's not a problem as far as gunit is concerned.
I agree with Matt that I'd prefer we clean configs up as much as possible (if for no other reason than for folks who copy-paste-edit) but I don't think it needs to be release blocking

I think in general once v3 lands, we need to do a sweep for v2 config and stop using it apart from coverage checks. We have to do the clean up eventually and doing it sooner rather than later avoids hacks like this. I think that'd resolve this concern since hosts isn't in v3 at all.

- { socket_address: { address: 192.168.1.1, port_value: 22 }}
- { socket_address: { address: 192.168.1.2, port_value: 44 }}
)EOF";
Expand Down
7 changes: 6 additions & 1 deletion test/config/integration/google_com_proxy_port_0.v2.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
65 changes: 45 additions & 20 deletions test/config/integration/server.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: {}
Expand Down
13 changes: 9 additions & 4 deletions test/config/integration/server_unix_listener.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: {}
Expand Down
Loading