From 2f63ef73a461c8aec1c96c5850dfbd709b072674 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 14 Oct 2024 19:05:44 +0000 Subject: [PATCH] upstream: removing exceptions from hostimpl Signed-off-by: Alyssa Wilk --- .../common/network/connectivity_manager.cc | 5 +- .../common/network/connectivity_manager.h | 6 +- .../network/connectivity_manager_test.cc | 30 ++++---- .../upstream/health_discovery_service.cc | 18 ++--- source/common/upstream/upstream_impl.cc | 59 +++++++++++---- source/common/upstream/upstream_impl.h | 58 ++++++++++----- .../clusters/common/logical_host.cc | 19 ++++- .../extensions/clusters/common/logical_host.h | 23 ++++-- .../clusters/dynamic_forward_proxy/cluster.cc | 31 ++++---- .../clusters/dynamic_forward_proxy/cluster.h | 4 +- .../logical_dns/logical_dns_cluster.cc | 7 +- .../original_dst/original_dst_cluster.cc | 12 ++-- .../clusters/redis/redis_cluster.cc | 26 +++++-- .../extensions/clusters/redis/redis_cluster.h | 10 ++- .../clusters/strict_dns/strict_dns_cluster.cc | 22 +++--- .../common/dynamic_forward_proxy/dns_cache.h | 5 +- .../dynamic_forward_proxy/dns_cache_impl.cc | 9 +-- .../dynamic_forward_proxy/dns_cache_impl.h | 3 +- .../network/redis_proxy/conn_pool_impl.cc | 11 +-- test/common/http/conn_pool_grid_test.cc | 4 +- .../upstream/health_checker_impl_test.cc | 44 ++++++------ .../upstream/load_balancer_simulation_test.cc | 2 +- test/common/upstream/upstream_impl_test.cc | 71 +++++++++---------- test/common/upstream/utility.h | 41 +++++------ .../dynamic_forward_proxy/cluster_test.cc | 10 +-- .../common/dynamic_forward_proxy/mocks.h | 2 +- .../random/random_lb_simulation_test.cc | 2 +- .../clusters/custom_static_cluster.cc | 2 +- test/integration/utility.cc | 4 +- 29 files changed, 335 insertions(+), 205 deletions(-) diff --git a/mobile/library/common/network/connectivity_manager.cc b/mobile/library/common/network/connectivity_manager.cc index c6dd098c8553..e24374f436d0 100644 --- a/mobile/library/common/network/connectivity_manager.cc +++ b/mobile/library/common/network/connectivity_manager.cc @@ -199,14 +199,14 @@ void ConnectivityManagerImpl::reportNetworkUsage(envoy_netconf_t configuration_k } } -void ConnectivityManagerImpl::onDnsResolutionComplete( +absl::Status ConnectivityManagerImpl::onDnsResolutionComplete( const std::string& resolved_host, const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr&, Network::DnsResolver::ResolutionStatus) { if (enable_drain_post_dns_refresh_) { // Check if the set of hosts pending drain contains the current resolved host. if (hosts_to_drain_.erase(resolved_host) == 0) { - return; + return absl::OkStatus(); } // We ignore whether DNS resolution has succeeded here. If it failed, we may be offline and @@ -220,6 +220,7 @@ void ConnectivityManagerImpl::onDnsResolutionComplete( cluster_manager_.drainConnections( [resolved_host](const Upstream::Host& host) { return host.hostname() == resolved_host; }); } + return absl::OkStatus(); } void ConnectivityManagerImpl::setDrainPostDnsRefreshEnabled(bool enabled) { diff --git a/mobile/library/common/network/connectivity_manager.h b/mobile/library/common/network/connectivity_manager.h index b34f85abe79d..a699b81d2013 100644 --- a/mobile/library/common/network/connectivity_manager.h +++ b/mobile/library/common/network/connectivity_manager.h @@ -205,9 +205,11 @@ class ConnectivityManagerImpl : public ConnectivityManager, : cluster_manager_(cluster_manager), dns_cache_manager_(dns_cache_manager) {} // Extensions::Common::DynamicForwardProxy::DnsCache::UpdateCallbacks - void onDnsHostAddOrUpdate( + absl::Status onDnsHostAddOrUpdate( const std::string& /*host*/, - const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr&) override {} + const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr&) override { + return absl::OkStatus(); + } void onDnsHostRemove(const std::string& /*host*/) override {} void onDnsResolutionComplete(const std::string& /*host*/, const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr&, diff --git a/mobile/test/common/network/connectivity_manager_test.cc b/mobile/test/common/network/connectivity_manager_test.cc index 61e36c7e83e2..47d102f87925 100644 --- a/mobile/test/common/network/connectivity_manager_test.cc +++ b/mobile/test/common/network/connectivity_manager_test.cc @@ -78,18 +78,24 @@ TEST_F(ConnectivityManagerTest, WhenDrainPostDnsRefreshEnabledDrainsPostDnsRefre connectivity_manager_->refreshDns(configuration_key, true); EXPECT_CALL(cm_, drainConnections(_)); - connectivity_manager_->onDnsResolutionComplete( - "cached.example.com", - std::make_shared(), - Network::DnsResolver::ResolutionStatus::Completed); - connectivity_manager_->onDnsResolutionComplete( - "not-cached.example.com", - std::make_shared(), - Network::DnsResolver::ResolutionStatus::Completed); - connectivity_manager_->onDnsResolutionComplete( - "not-cached2.example.com", - std::make_shared(), - Network::DnsResolver::ResolutionStatus::Completed); + connectivity_manager_ + ->onDnsResolutionComplete( + "cached.example.com", + std::make_shared(), + Network::DnsResolver::ResolutionStatus::Completed) + .IgnoreError(); + connectivity_manager_ + ->onDnsResolutionComplete( + "not-cached.example.com", + std::make_shared(), + Network::DnsResolver::ResolutionStatus::Completed) + .IgnoreError(); + connectivity_manager_ + ->onDnsResolutionComplete( + "not-cached2.example.com", + std::make_shared(), + Network::DnsResolver::ResolutionStatus::Completed) + .IgnoreError(); } TEST_F(ConnectivityManagerTest, WhenDrainPostDnsNotEnabledDoesntDrainPostDnsRefresh) { diff --git a/source/common/upstream/health_discovery_service.cc b/source/common/upstream/health_discovery_service.cc index 36fa65196d7e..bb631e9c4e82 100644 --- a/source/common/upstream/health_discovery_service.cc +++ b/source/common/upstream/health_discovery_service.cc @@ -373,10 +373,11 @@ HdsCluster::HdsCluster(Server::Configuration::ServerFactoryContext& server_conte // Initialize an endpoint host object. auto address_or_error = Network::Address::resolveProtoAddress(host.endpoint().address()); THROW_IF_NOT_OK_REF(address_or_error.status()); - HostSharedPtr endpoint = std::make_shared( - info_, "", std::move(address_or_error.value()), nullptr, nullptr, 1, - locality_endpoints.locality(), host.endpoint().health_check_config(), 0, - envoy::config::core::v3::UNKNOWN, time_source_); + HostSharedPtr endpoint = std::shared_ptr(THROW_OR_RETURN_VALUE( + HostImpl::create(info_, "", std::move(address_or_error.value()), nullptr, nullptr, 1, + locality_endpoints.locality(), host.endpoint().health_check_config(), 0, + envoy::config::core::v3::UNKNOWN, time_source_), + std::unique_ptr)); // Add this host/endpoint pointer to our flat list of endpoints for health checking. hosts_->push_back(endpoint); // Add this host/endpoint pointer to our structured list by locality so results can be @@ -488,10 +489,11 @@ void HdsCluster::updateHosts( auto address_or_error = Network::Address::resolveProtoAddress(endpoint.endpoint().address()); THROW_IF_NOT_OK_REF(address_or_error.status()); - host = std::make_shared(info_, "", std::move(address_or_error.value()), nullptr, - nullptr, 1, endpoints.locality(), - endpoint.endpoint().health_check_config(), 0, - envoy::config::core::v3::UNKNOWN, time_source_); + host = std::shared_ptr(THROW_OR_RETURN_VALUE( + HostImpl::create(info_, "", std::move(address_or_error.value()), nullptr, nullptr, 1, + endpoints.locality(), endpoint.endpoint().health_check_config(), 0, + envoy::config::core::v3::UNKNOWN, time_source_), + std::unique_ptr)); // Set the initial health status as in HdsCluster::initialize. host->healthFlagSet(Host::HealthFlag::FAILED_ACTIVE_HC); diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 1dafc23d65b8..e85e2532c005 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -427,14 +427,29 @@ LoadMetricStats::StatMapPtr LoadMetricStatsImpl::latch() { return latched; } -HostDescriptionImpl::HostDescriptionImpl( +absl::StatusOr> HostDescriptionImpl::create( ClusterInfoConstSharedPtr cluster, const std::string& hostname, Network::Address::InstanceConstSharedPtr dest_address, MetadataConstSharedPtr endpoint_metadata, MetadataConstSharedPtr locality_metadata, const envoy::config::core::v3::Locality& locality, const envoy::config::endpoint::v3::Endpoint::HealthCheckConfig& health_check_config, + uint32_t priority, TimeSource& time_source, const AddressVector& address_list) { + absl::Status creation_status = absl::OkStatus(); + auto ret = std::unique_ptr(new HostDescriptionImpl( + creation_status, cluster, hostname, dest_address, endpoint_metadata, locality_metadata, + locality, health_check_config, priority, time_source, address_list)); + RETURN_IF_NOT_OK(creation_status); + return ret; +} + +HostDescriptionImpl::HostDescriptionImpl( + absl::Status& creation_status, ClusterInfoConstSharedPtr cluster, const std::string& hostname, + Network::Address::InstanceConstSharedPtr dest_address, MetadataConstSharedPtr endpoint_metadata, + MetadataConstSharedPtr locality_metadata, const envoy::config::core::v3::Locality& locality, + const envoy::config::endpoint::v3::Endpoint::HealthCheckConfig& health_check_config, uint32_t priority, TimeSource& time_source, const AddressVector& address_list) : HostDescriptionImplBase(cluster, hostname, dest_address, endpoint_metadata, locality_metadata, - locality, health_check_config, priority, time_source), + locality, health_check_config, priority, time_source, + creation_status), address_(dest_address), address_list_or_null_(makeAddressListOrNull(dest_address, address_list)), health_check_address_(resolveHealthCheckAddress(health_check_config, dest_address)) {} @@ -444,7 +459,7 @@ HostDescriptionImplBase::HostDescriptionImplBase( Network::Address::InstanceConstSharedPtr dest_address, MetadataConstSharedPtr endpoint_metadata, MetadataConstSharedPtr locality_metadata, const envoy::config::core::v3::Locality& locality, const envoy::config::endpoint::v3::Endpoint::HealthCheckConfig& health_check_config, - uint32_t priority, TimeSource& time_source) + uint32_t priority, TimeSource& time_source, absl::Status& creation_status) : cluster_(cluster), hostname_(hostname), health_checks_hostname_(health_check_config.hostname()), canary_(Config::Metadata::metadataValue(endpoint_metadata.get(), @@ -459,15 +474,15 @@ HostDescriptionImplBase::HostDescriptionImplBase( creation_time_(time_source.monotonicTime()) { if (health_check_config.port_value() != 0 && dest_address->type() != Network::Address::Type::Ip) { // Setting the health check port to non-0 only works for IP-type addresses. Setting the port - // for a pipe address is a misconfiguration. Throw an exception. - throwEnvoyExceptionOrPanic( + // for a pipe address is a misconfiguration. + creation_status = absl::InvalidArgumentError( fmt::format("Invalid host configuration: non-zero port for non-IP address")); } } HostDescription::SharedConstAddressVector HostDescriptionImplBase::makeAddressListOrNull( const Network::Address::InstanceConstSharedPtr& address, const AddressVector& address_list) { - if (address_list.empty()) { + if (!address || address_list.empty()) { return {}; } ASSERT(*address_list.front() == *address); @@ -649,6 +664,23 @@ Host::CreateConnectionData HostImplBase::createConnection( void HostImplBase::weight(uint32_t new_weight) { weight_ = std::max(1U, new_weight); } +absl::StatusOr> HostImpl::create( + ClusterInfoConstSharedPtr cluster, const std::string& hostname, + Network::Address::InstanceConstSharedPtr address, MetadataConstSharedPtr endpoint_metadata, + MetadataConstSharedPtr locality_metadata, uint32_t initial_weight, + const envoy::config::core::v3::Locality& locality, + const envoy::config::endpoint::v3::Endpoint::HealthCheckConfig& health_check_config, + uint32_t priority, const envoy::config::core::v3::HealthStatus health_status, + TimeSource& time_source, const AddressVector& address_list) { + absl::Status creation_status = absl::OkStatus(); + auto ret = std::unique_ptr( + new HostImpl(creation_status, cluster, hostname, address, endpoint_metadata, + locality_metadata, initial_weight, locality, health_check_config, priority, + health_status, time_source, address_list)); + RETURN_IF_NOT_OK(creation_status); + return ret; +} + std::vector HostsPerLocalityImpl::filter( const std::vector>& predicates) const { // We keep two lists: one for being able to mutate the clone and one for returning to the @@ -2190,11 +2222,13 @@ void PriorityStateManager::registerHostForPriority( locality_lb_endpoint.has_metadata() ? parent_.constMetadataSharedPool()->getObject(locality_lb_endpoint.metadata()) : nullptr; - const auto host = std::make_shared( - parent_.info(), hostname, address, endpoint_metadata, locality_metadata, - lb_endpoint.load_balancing_weight().value(), locality_lb_endpoint.locality(), - lb_endpoint.endpoint().health_check_config(), locality_lb_endpoint.priority(), - lb_endpoint.health_status(), time_source, address_list); + const auto host = std::shared_ptr(THROW_OR_RETURN_VALUE( + HostImpl::create(parent_.info(), hostname, address, endpoint_metadata, locality_metadata, + lb_endpoint.load_balancing_weight().value(), locality_lb_endpoint.locality(), + lb_endpoint.endpoint().health_check_config(), + locality_lb_endpoint.priority(), lb_endpoint.health_status(), time_source, + address_list), + std::unique_ptr)); registerHostForPriority(host, locality_lb_endpoint); } @@ -2498,7 +2532,6 @@ bool BaseDynamicClusterImpl::updateDynamicHostList( if (hosts_with_updated_locality_for_current_priority.contains(p->address()->asString())) { return false; } - if (hosts_with_active_health_check_flag_changed.contains(p->address()->asString())) { return false; } @@ -2591,7 +2624,7 @@ Network::Address::InstanceConstSharedPtr resolveHealthCheckAddress( health_check_address = port_value == 0 ? address : Network::Utility::getAddressWithPort(*address, port_value); } else { - health_check_address = port_value == 0 + health_check_address = (port_value == 0 || !host_address->ip()) ? host_address : Network::Utility::getAddressWithPort(*host_address, port_value); } diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 6b537c75d9cd..63fc2c200080 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -159,14 +159,6 @@ class DetectorHostMonitorNullImpl : public Outlier::DetectorHostMonitor { class HostDescriptionImplBase : virtual public HostDescription, protected Logger::Loggable { public: - HostDescriptionImplBase( - ClusterInfoConstSharedPtr cluster, const std::string& hostname, - Network::Address::InstanceConstSharedPtr dest_address, - MetadataConstSharedPtr endpoint_metadata, MetadataConstSharedPtr locality_metadata, - const envoy::config::core::v3::Locality& locality, - const envoy::config::endpoint::v3::Endpoint::HealthCheckConfig& health_check_config, - uint32_t priority, TimeSource& time_source); - Network::UpstreamTransportSocketFactory& transportSocketFactory() const override { absl::ReaderMutexLock lock(&metadata_mutex_); return socket_factory_; @@ -250,6 +242,14 @@ class HostDescriptionImplBase : virtual public HostDescription, } protected: + HostDescriptionImplBase( + ClusterInfoConstSharedPtr cluster, const std::string& hostname, + Network::Address::InstanceConstSharedPtr dest_address, + MetadataConstSharedPtr endpoint_metadata, MetadataConstSharedPtr locality_metadata, + const envoy::config::core::v3::Locality& locality, + const envoy::config::endpoint::v3::Endpoint::HealthCheckConfig& health_check_config, + uint32_t priority, TimeSource& time_source, absl::Status& creation_status); + /** * @return nullptr if address_list is empty, otherwise a shared_ptr copy of address_list. */ @@ -288,13 +288,13 @@ class HostDescriptionImplBase : virtual public HostDescription, */ class HostDescriptionImpl : public HostDescriptionImplBase { public: - HostDescriptionImpl( - ClusterInfoConstSharedPtr cluster, const std::string& hostname, - Network::Address::InstanceConstSharedPtr dest_address, - MetadataConstSharedPtr endpoint_metadata, MetadataConstSharedPtr locality_metadata, - const envoy::config::core::v3::Locality& locality, - const envoy::config::endpoint::v3::Endpoint::HealthCheckConfig& health_check_config, - uint32_t priority, TimeSource& time_source, const AddressVector& address_list = {}); + static absl::StatusOr> + create(ClusterInfoConstSharedPtr cluster, const std::string& hostname, + Network::Address::InstanceConstSharedPtr dest_address, + MetadataConstSharedPtr endpoint_metadata, MetadataConstSharedPtr locality_metadata, + const envoy::config::core::v3::Locality& locality, + const envoy::config::endpoint::v3::Endpoint::HealthCheckConfig& health_check_config, + uint32_t priority, TimeSource& time_source, const AddressVector& address_list = {}); // HostDescription Network::Address::InstanceConstSharedPtr address() const override { return address_; } @@ -303,6 +303,15 @@ class HostDescriptionImpl : public HostDescriptionImplBase { } SharedConstAddressVector addressListOrNull() const override { return address_list_or_null_; } +protected: + HostDescriptionImpl( + absl::Status& creation_status, ClusterInfoConstSharedPtr cluster, const std::string& hostname, + Network::Address::InstanceConstSharedPtr dest_address, + MetadataConstSharedPtr endpoint_metadata, MetadataConstSharedPtr locality_metadata, + const envoy::config::core::v3::Locality& locality, + const envoy::config::endpoint::v3::Endpoint::HealthCheckConfig& health_check_config, + uint32_t priority, TimeSource& time_source, const AddressVector& address_list = {}); + private: // No locks are required in this implementation: all address-related member // variables are set at construction and never change. See @@ -470,16 +479,27 @@ class HostImplBase : public Host, class HostImpl : public HostImplBase, public HostDescriptionImpl { public: - HostImpl(ClusterInfoConstSharedPtr cluster, const std::string& hostname, - Network::Address::InstanceConstSharedPtr address, + static absl::StatusOr> + create(ClusterInfoConstSharedPtr cluster, const std::string& hostname, + Network::Address::InstanceConstSharedPtr address, MetadataConstSharedPtr endpoint_metadata, + MetadataConstSharedPtr locality_metadata, uint32_t initial_weight, + const envoy::config::core::v3::Locality& locality, + const envoy::config::endpoint::v3::Endpoint::HealthCheckConfig& health_check_config, + uint32_t priority, const envoy::config::core::v3::HealthStatus health_status, + TimeSource& time_source, const AddressVector& address_list = {}); + +protected: + HostImpl(absl::Status& creation_status, ClusterInfoConstSharedPtr cluster, + const std::string& hostname, Network::Address::InstanceConstSharedPtr address, MetadataConstSharedPtr endpoint_metadata, MetadataConstSharedPtr locality_metadata, uint32_t initial_weight, const envoy::config::core::v3::Locality& locality, const envoy::config::endpoint::v3::Endpoint::HealthCheckConfig& health_check_config, uint32_t priority, const envoy::config::core::v3::HealthStatus health_status, TimeSource& time_source, const AddressVector& address_list = {}) : HostImplBase(initial_weight, health_check_config, health_status), - HostDescriptionImpl(cluster, hostname, address, endpoint_metadata, locality_metadata, - locality, health_check_config, priority, time_source, address_list) {} + HostDescriptionImpl(creation_status, cluster, hostname, address, endpoint_metadata, + locality_metadata, locality, health_check_config, priority, time_source, + address_list) {} }; class HostsPerLocalityImpl : public HostsPerLocality { diff --git a/source/extensions/clusters/common/logical_host.cc b/source/extensions/clusters/common/logical_host.cc index 71b42a834a79..2cbb898d00ca 100644 --- a/source/extensions/clusters/common/logical_host.cc +++ b/source/extensions/clusters/common/logical_host.cc @@ -3,13 +3,28 @@ namespace Envoy { namespace Upstream { +absl::StatusOr> LogicalHost::create( + const ClusterInfoConstSharedPtr& cluster, const std::string& hostname, + const Network::Address::InstanceConstSharedPtr& address, const AddressVector& address_list, + const envoy::config::endpoint::v3::LocalityLbEndpoints& locality_lb_endpoint, + const envoy::config::endpoint::v3::LbEndpoint& lb_endpoint, + const Network::TransportSocketOptionsConstSharedPtr& override_transport_socket_options, + TimeSource& time_source) { + absl::Status creation_status = absl::OkStatus(); + auto ret = std::unique_ptr( + new LogicalHost(cluster, hostname, address, address_list, locality_lb_endpoint, lb_endpoint, + override_transport_socket_options, time_source, creation_status)); + RETURN_IF_NOT_OK(creation_status); + return ret; +} + LogicalHost::LogicalHost( const ClusterInfoConstSharedPtr& cluster, const std::string& hostname, const Network::Address::InstanceConstSharedPtr& address, const AddressVector& address_list, const envoy::config::endpoint::v3::LocalityLbEndpoints& locality_lb_endpoint, const envoy::config::endpoint::v3::LbEndpoint& lb_endpoint, const Network::TransportSocketOptionsConstSharedPtr& override_transport_socket_options, - TimeSource& time_source) + TimeSource& time_source, absl::Status& creation_status) : HostImplBase(lb_endpoint.load_balancing_weight().value(), lb_endpoint.endpoint().health_check_config(), lb_endpoint.health_status()), HostDescriptionImplBase( @@ -19,7 +34,7 @@ LogicalHost::LogicalHost( std::make_shared( locality_lb_endpoint.metadata()), locality_lb_endpoint.locality(), lb_endpoint.endpoint().health_check_config(), - locality_lb_endpoint.priority(), time_source), + locality_lb_endpoint.priority(), time_source, creation_status), override_transport_socket_options_(override_transport_socket_options), address_(address), address_list_or_null_(makeAddressListOrNull(address, address_list)) { health_check_address_ = diff --git a/source/extensions/clusters/common/logical_host.h b/source/extensions/clusters/common/logical_host.h index 3c13c909904c..ca24428c15dd 100644 --- a/source/extensions/clusters/common/logical_host.h +++ b/source/extensions/clusters/common/logical_host.h @@ -17,13 +17,13 @@ namespace Upstream { */ class LogicalHost : public HostImplBase, public HostDescriptionImplBase { public: - LogicalHost( - const ClusterInfoConstSharedPtr& cluster, const std::string& hostname, - const Network::Address::InstanceConstSharedPtr& address, const AddressVector& address_list, - const envoy::config::endpoint::v3::LocalityLbEndpoints& locality_lb_endpoint, - const envoy::config::endpoint::v3::LbEndpoint& lb_endpoint, - const Network::TransportSocketOptionsConstSharedPtr& override_transport_socket_options, - TimeSource& time_source); + static absl::StatusOr> + create(const ClusterInfoConstSharedPtr& cluster, const std::string& hostname, + const Network::Address::InstanceConstSharedPtr& address, const AddressVector& address_list, + const envoy::config::endpoint::v3::LocalityLbEndpoints& locality_lb_endpoint, + const envoy::config::endpoint::v3::LbEndpoint& lb_endpoint, + const Network::TransportSocketOptionsConstSharedPtr& override_transport_socket_options, + TimeSource& time_source); /** * Sets new addresses. This can be called dynamically during operation, and @@ -53,6 +53,15 @@ class LogicalHost : public HostImplBase, public HostDescriptionImplBase { Network::Address::InstanceConstSharedPtr address() const override; Network::Address::InstanceConstSharedPtr healthCheckAddress() const override; +protected: + LogicalHost( + const ClusterInfoConstSharedPtr& cluster, const std::string& hostname, + const Network::Address::InstanceConstSharedPtr& address, const AddressVector& address_list, + const envoy::config::endpoint::v3::LocalityLbEndpoints& locality_lb_endpoint, + const envoy::config::endpoint::v3::LbEndpoint& lb_endpoint, + const Network::TransportSocketOptionsConstSharedPtr& override_transport_socket_options, + TimeSource& time_source, absl::Status& creation_status); + private: const Network::TransportSocketOptionsConstSharedPtr override_transport_socket_options_; diff --git a/source/extensions/clusters/dynamic_forward_proxy/cluster.cc b/source/extensions/clusters/dynamic_forward_proxy/cluster.cc index 4ec0a8298ac9..baa1242616e8 100644 --- a/source/extensions/clusters/dynamic_forward_proxy/cluster.cc +++ b/source/extensions/clusters/dynamic_forward_proxy/cluster.cc @@ -102,7 +102,10 @@ void Cluster::startPreInit() { std::unique_ptr hosts_added; dns_cache_->iterateHostMap( [&](absl::string_view host, const Common::DynamicForwardProxy::DnsHostInfoSharedPtr& info) { - addOrUpdateHost(host, info, hosts_added); + absl::Status status = addOrUpdateHost(host, info, hosts_added); + if (!status.ok()) { + ENVOY_LOG(warn, "Failed to add host from cache: {}", status.message()); + } }); if (hosts_added) { updatePriorityState(*hosts_added, {}); @@ -238,7 +241,7 @@ bool Cluster::ClusterInfo::checkIdle() { return false; } -void Cluster::addOrUpdateHost( +absl::Status Cluster::addOrUpdateHost( absl::string_view host, const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr& host_info, std::unique_ptr& hosts_added) { @@ -276,18 +279,20 @@ void Cluster::addOrUpdateHost( ENVOY_LOG(debug, "updating dfproxy cluster host address '{}'", host); host_map_it->second.logical_host_->setNewAddresses( host_info->address(), host_info->addressList(), dummy_lb_endpoint_); - return; + return absl::OkStatus(); } ENVOY_LOG(debug, "adding new dfproxy cluster host '{}'", host); + auto host_or_error = Upstream::LogicalHost::create( + info(), std::string{host}, host_info->address(), host_info->addressList(), + dummy_locality_lb_endpoint_, dummy_lb_endpoint_, nullptr, time_source_); + RETURN_IF_NOT_OK_REF(host_or_error.status()); - emplaced_host = host_map_ - .try_emplace(host, host_info, - std::make_shared( - info(), std::string{host}, host_info->address(), - host_info->addressList(), dummy_locality_lb_endpoint_, - dummy_lb_endpoint_, nullptr, time_source_)) - .first->second.logical_host_; + emplaced_host = + host_map_ + .try_emplace(host, host_info, + std::shared_ptr(host_or_error->release())) + .first->second.logical_host_; } ASSERT(emplaced_host); @@ -295,19 +300,21 @@ void Cluster::addOrUpdateHost( hosts_added = std::make_unique(); } hosts_added->emplace_back(emplaced_host); + return absl::OkStatus(); } -void Cluster::onDnsHostAddOrUpdate( +absl::Status Cluster::onDnsHostAddOrUpdate( const std::string& host, const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr& host_info) { ENVOY_LOG(debug, "Adding host info for {}", host); std::unique_ptr hosts_added; - addOrUpdateHost(host, host_info, hosts_added); + RETURN_IF_NOT_OK(addOrUpdateHost(host, host_info, hosts_added)); if (hosts_added != nullptr) { ASSERT(!hosts_added->empty()); updatePriorityState(*hosts_added, {}); } + return absl::OkStatus(); } void Cluster::updatePriorityState(const Upstream::HostVector& hosts_added, diff --git a/source/extensions/clusters/dynamic_forward_proxy/cluster.h b/source/extensions/clusters/dynamic_forward_proxy/cluster.h index edc87f9c357b..43192ab29bf7 100644 --- a/source/extensions/clusters/dynamic_forward_proxy/cluster.h +++ b/source/extensions/clusters/dynamic_forward_proxy/cluster.h @@ -34,7 +34,7 @@ class Cluster : public Upstream::BaseDynamicClusterImpl, void startPreInit() override; // Extensions::Common::DynamicForwardProxy::DnsCache::UpdateCallbacks - void onDnsHostAddOrUpdate( + absl::Status onDnsHostAddOrUpdate( const std::string& host, const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr& host_info) override; void onDnsHostRemove(const std::string& host) override; @@ -167,7 +167,7 @@ class Cluster : public Upstream::BaseDynamicClusterImpl, Cluster& cluster_; }; - void + absl::Status addOrUpdateHost(absl::string_view host, const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr& host_info, std::unique_ptr& hosts_added) diff --git a/source/extensions/clusters/logical_dns/logical_dns_cluster.cc b/source/extensions/clusters/logical_dns/logical_dns_cluster.cc index ef3d443af702..cfe15f73c8f9 100644 --- a/source/extensions/clusters/logical_dns/logical_dns_cluster.cc +++ b/source/extensions/clusters/logical_dns/logical_dns_cluster.cc @@ -118,9 +118,10 @@ void LogicalDnsCluster::startResolve() { auto address_list = DnsUtils::generateAddressList(response, dns_port_); if (!logical_host_) { - logical_host_ = std::make_shared(info_, hostname_, new_address, - address_list, localityLbEndpoint(), - lbEndpoint(), nullptr, time_source_); + logical_host_ = THROW_OR_RETURN_VALUE( + LogicalHost::create(info_, hostname_, new_address, address_list, + localityLbEndpoint(), lbEndpoint(), nullptr, time_source_), + std::unique_ptr); const auto& locality_lb_endpoint = localityLbEndpoint(); PriorityStateManager priority_state_manager(*this, local_info_, nullptr, random_); diff --git a/source/extensions/clusters/original_dst/original_dst_cluster.cc b/source/extensions/clusters/original_dst/original_dst_cluster.cc index ebc28716370f..4067962160cb 100644 --- a/source/extensions/clusters/original_dst/original_dst_cluster.cc +++ b/source/extensions/clusters/original_dst/original_dst_cluster.cc @@ -73,11 +73,13 @@ HostConstSharedPtr OriginalDstCluster::LoadBalancer::chooseHost(LoadBalancerCont Network::Utility::copyInternetAddressAndPort(*dst_ip)); // Create a host we can use immediately. auto info = parent_->cluster_->info(); - HostSharedPtr host(std::make_shared( - info, info->name() + dst_addr.asString(), std::move(host_ip_port), nullptr, nullptr, 1, - envoy::config::core::v3::Locality().default_instance(), - envoy::config::endpoint::v3::Endpoint::HealthCheckConfig().default_instance(), 0, - envoy::config::core::v3::UNKNOWN, parent_->cluster_->time_source_)); + HostSharedPtr host(std::shared_ptr(THROW_OR_RETURN_VALUE( + HostImpl::create( + info, info->name() + dst_addr.asString(), std::move(host_ip_port), nullptr, nullptr, + 1, envoy::config::core::v3::Locality().default_instance(), + envoy::config::endpoint::v3::Endpoint::HealthCheckConfig().default_instance(), 0, + envoy::config::core::v3::UNKNOWN, parent_->cluster_->time_source_), + std::unique_ptr))); ENVOY_LOG(debug, "Created host {} {}.", *host, host->address()->asString()); // Tell the cluster about the new host diff --git a/source/extensions/clusters/redis/redis_cluster.cc b/source/extensions/clusters/redis/redis_cluster.cc index ed96a0a49a10..5fbe0eadb2ac 100644 --- a/source/extensions/clusters/redis/redis_cluster.cc +++ b/source/extensions/clusters/redis/redis_cluster.cc @@ -14,6 +14,18 @@ namespace Extensions { namespace Clusters { namespace Redis { +absl::StatusOr> +RedisCluster::RedisHost::create(Upstream::ClusterInfoConstSharedPtr cluster, + const std::string& hostname, + Network::Address::InstanceConstSharedPtr address, + RedisCluster& parent, bool primary, TimeSource& time_source) { + absl::Status creation_status = absl::OkStatus(); + auto ret = std::unique_ptr(new RedisCluster::RedisHost( + cluster, hostname, address, parent, primary, time_source, creation_status)); + RETURN_IF_NOT_OK(creation_status); + return ret; +} + absl::StatusOr> RedisCluster::create( const envoy::config::cluster::v3::Cluster& cluster, const envoy::extensions::clusters::redis::v3::RedisClusterConfig& redis_cluster, @@ -112,13 +124,16 @@ void RedisCluster::onClusterSlotUpdate(ClusterSlotsSharedPtr&& slots) { for (const ClusterSlot& slot : *slots) { if (all_new_hosts.count(slot.primary()->asString()) == 0) { - new_hosts.emplace_back(new RedisHost(info(), "", slot.primary(), *this, true, time_source_)); + new_hosts.emplace_back(THROW_OR_RETURN_VALUE( + RedisHost::create(info(), "", slot.primary(), *this, true, time_source_), + std::unique_ptr)); all_new_hosts.emplace(slot.primary()->asString()); } for (auto const& replica : slot.replicas()) { if (all_new_hosts.count(replica.first) == 0) { - new_hosts.emplace_back( - new RedisHost(info(), "", replica.second, *this, false, time_source_)); + new_hosts.emplace_back(THROW_OR_RETURN_VALUE( + RedisHost::create(info(), "", replica.second, *this, false, time_source_), + std::unique_ptr)); all_new_hosts.emplace(replica.first); } } @@ -299,8 +314,9 @@ void RedisCluster::RedisDiscoverySession::startResolveRedis() { if (parent_.hosts_.empty()) { const int rand_idx = parent_.random_.random() % discovery_address_list_.size(); auto it = std::next(discovery_address_list_.begin(), rand_idx); - host = Upstream::HostSharedPtr{ - new RedisHost(parent_.info(), "", *it, parent_, true, parent_.timeSource())}; + host = Upstream::HostSharedPtr{THROW_OR_RETURN_VALUE( + RedisHost::create(parent_.info(), "", *it, parent_, true, parent_.timeSource()), + std::unique_ptr)}; } else { const int rand_idx = parent_.random_.random() % parent_.hosts_.size(); host = parent_.hosts_[rand_idx]; diff --git a/source/extensions/clusters/redis/redis_cluster.h b/source/extensions/clusters/redis/redis_cluster.h index c23aed876c37..bbefb6c876d9 100644 --- a/source/extensions/clusters/redis/redis_cluster.h +++ b/source/extensions/clusters/redis/redis_cluster.h @@ -153,11 +153,17 @@ class RedisCluster : public Upstream::BaseDynamicClusterImpl { // A redis node in the Redis cluster. class RedisHost : public Upstream::HostImpl { public: + static absl::StatusOr> + create(Upstream::ClusterInfoConstSharedPtr cluster, const std::string& hostname, + Network::Address::InstanceConstSharedPtr address, RedisCluster& parent, bool primary, + TimeSource& time_source); + + protected: RedisHost(Upstream::ClusterInfoConstSharedPtr cluster, const std::string& hostname, Network::Address::InstanceConstSharedPtr address, RedisCluster& parent, bool primary, - TimeSource& time_source) + TimeSource& time_source, absl::Status& creation_status) : Upstream::HostImpl( - cluster, hostname, address, + creation_status, cluster, hostname, address, // TODO(zyfjeff): Created through metadata shared pool std::make_shared(parent.lbEndpoint().metadata()), std::make_shared( diff --git a/source/extensions/clusters/strict_dns/strict_dns_cluster.cc b/source/extensions/clusters/strict_dns/strict_dns_cluster.cc index b379ef1e890e..187e6bef406b 100644 --- a/source/extensions/clusters/strict_dns/strict_dns_cluster.cc +++ b/source/extensions/clusters/strict_dns/strict_dns_cluster.cc @@ -148,15 +148,19 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() { continue; } - new_hosts.emplace_back(new HostImpl( - parent_.info_, hostname_, address, - // TODO(zyfjeff): Created through metadata shared pool - std::make_shared(lb_endpoint_.metadata()), - std::make_shared( - locality_lb_endpoints_.metadata()), - lb_endpoint_.load_balancing_weight().value(), locality_lb_endpoints_.locality(), - lb_endpoint_.endpoint().health_check_config(), locality_lb_endpoints_.priority(), - lb_endpoint_.health_status(), parent_.time_source_)); + new_hosts.emplace_back(THROW_OR_RETURN_VALUE( + HostImpl::create(parent_.info_, hostname_, address, + // TODO(zyfjeff): Created through metadata shared pool + std::make_shared( + lb_endpoint_.metadata()), + std::make_shared( + locality_lb_endpoints_.metadata()), + lb_endpoint_.load_balancing_weight().value(), + locality_lb_endpoints_.locality(), + lb_endpoint_.endpoint().health_check_config(), + locality_lb_endpoints_.priority(), lb_endpoint_.health_status(), + parent_.time_source_), + std::unique_ptr)); all_new_hosts.emplace(address->asString()); ttl_refresh_rate = min(ttl_refresh_rate, addrinfo.ttl_); } diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache.h b/source/extensions/common/dynamic_forward_proxy/dns_cache.h index 7afeb0fc4192..702a59cd8842 100644 --- a/source/extensions/common/dynamic_forward_proxy/dns_cache.h +++ b/source/extensions/common/dynamic_forward_proxy/dns_cache.h @@ -148,9 +148,10 @@ class DnsCache { * Called when a host has been added or has had its address updated. * @param host supplies the added/updated host. * @param host_info supplies the associated host info. + * @param return supplies if the host was successfully added */ - virtual void onDnsHostAddOrUpdate(const std::string& host, - const DnsHostInfoSharedPtr& host_info) PURE; + virtual absl::Status onDnsHostAddOrUpdate(const std::string& host, + const DnsHostInfoSharedPtr& host_info) PURE; /** * Called when a host has been removed. diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc index fffd784255a6..d136bdb51e58 100644 --- a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc +++ b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc @@ -495,7 +495,7 @@ void DnsCacheImpl::finishResolve(const std::string& host, primary_host_info->host_info_->setAddresses(new_address, std::move(address_list)); primary_host_info->host_info_->setDetails(details_with_maybe_trace); - runAddUpdateCallbacks(host, primary_host_info->host_info_); + THROW_IF_NOT_OK(runAddUpdateCallbacks(host, primary_host_info->host_info_)); primary_host_info->host_info_->setFirstResolveComplete(); address_changed = true; stats_.host_address_changed_.inc(); @@ -530,11 +530,12 @@ void DnsCacheImpl::finishResolve(const std::string& host, } } -void DnsCacheImpl::runAddUpdateCallbacks(const std::string& host, - const DnsHostInfoSharedPtr& host_info) { +absl::Status DnsCacheImpl::runAddUpdateCallbacks(const std::string& host, + const DnsHostInfoSharedPtr& host_info) { for (auto* callbacks : update_callbacks_) { - callbacks->callbacks_.onDnsHostAddOrUpdate(host, host_info); + RETURN_IF_NOT_OK(callbacks->callbacks_.onDnsHostAddOrUpdate(host, host_info)); } + return absl::OkStatus(); } void DnsCacheImpl::runResolutionCompleteCallbacks(const std::string& host, diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h index 15e0b41408d7..0423335d21df 100644 --- a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h +++ b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h @@ -224,7 +224,8 @@ class DnsCacheImpl : public DnsCache, Logger::Loggable&& response, absl::optional resolution_time = {}, bool is_proxy_lookup = false, bool is_timeout = false); - void runAddUpdateCallbacks(const std::string& host, const DnsHostInfoSharedPtr& host_info); + absl::Status runAddUpdateCallbacks(const std::string& host, + const DnsHostInfoSharedPtr& host_info); void runResolutionCompleteCallbacks(const std::string& host, const DnsHostInfoSharedPtr& host_info, Network::DnsResolver::ResolutionStatus status); diff --git a/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc b/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc index 574cfe251004..56814c669935 100644 --- a/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc +++ b/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc @@ -385,10 +385,13 @@ Common::Redis::Client::PoolRequest* InstanceImpl::ThreadLocalPool::makeRequestTo } END_TRY catch (const EnvoyException&) { return nullptr; } } - Upstream::HostSharedPtr new_host{new Upstream::HostImpl( - cluster_->info(), "", address_ptr, nullptr, nullptr, 1, envoy::config::core::v3::Locality(), - envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 0, - envoy::config::core::v3::UNKNOWN, dispatcher_.timeSource())}; + Upstream::HostSharedPtr new_host{THROW_OR_RETURN_VALUE( + Upstream::HostImpl::create( + cluster_->info(), "", address_ptr, nullptr, nullptr, 1, + envoy::config::core::v3::Locality(), + envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 0, + envoy::config::core::v3::UNKNOWN, dispatcher_.timeSource()), + std::unique_ptr)}; host_address_map_[host_address_map_key] = new_host; created_via_redirect_hosts_.push_back(new_host); it = host_address_map_.find(host_address_map_key); diff --git a/test/common/http/conn_pool_grid_test.cc b/test/common/http/conn_pool_grid_test.cc index 4ca040f8d41b..7af825a4eb60 100644 --- a/test/common/http/conn_pool_grid_test.cc +++ b/test/common/http/conn_pool_grid_test.cc @@ -179,11 +179,11 @@ class ConnectivityGridTest : public Event::TestUsingSimulatedTime, public testin #else std::make_unique(); #endif - host_ = std::make_shared( + host_ = std::shared_ptr(*Upstream::HostImpl::create( cluster_, host_impl_hostname_, *Network::Utility::resolveUrl("tcp://127.0.0.1:9000"), nullptr, nullptr, 1, envoy::config::core::v3::Locality(), envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 0, - envoy::config::core::v3::UNKNOWN, simTime(), address_list_); + envoy::config::core::v3::UNKNOWN, simTime(), address_list_)); grid_ = std::make_unique( dispatcher_, random_, host_, Upstream::ResourcePriority::Default, socket_options_, diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index 9eae91450eb1..ee0da3520176 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -1717,10 +1717,10 @@ TEST_F(HttpHealthCheckerImplTest, SuccessServiceCheckWithCustomHostValueOnTheHos const std::string host = "www.envoyproxy.io"; envoy::config::endpoint::v3::Endpoint::HealthCheckConfig health_check_config; health_check_config.set_hostname(host); - auto test_host = std::make_shared( - cluster_->info_, "", *Network::Utility::resolveUrl("tcp://127.0.0.1:80"), nullptr, nullptr, 1, - envoy::config::core::v3::Locality(), health_check_config, 0, envoy::config::core::v3::UNKNOWN, - simTime()); + auto test_host = std::shared_ptr( + *HostImpl::create(cluster_->info_, "", *Network::Utility::resolveUrl("tcp://127.0.0.1:80"), + nullptr, nullptr, 1, envoy::config::core::v3::Locality(), + health_check_config, 0, envoy::config::core::v3::UNKNOWN, simTime())); const std::string path = "/healthcheck"; setupServiceValidationHC(); // Requires non-empty `service_name` in config. @@ -1762,10 +1762,10 @@ TEST_F(HttpHealthCheckerImplTest, const std::string host = "www.envoyproxy.io"; envoy::config::endpoint::v3::Endpoint::HealthCheckConfig health_check_config; health_check_config.set_hostname(host); - auto test_host = std::make_shared( - cluster_->info_, "", *Network::Utility::resolveUrl("tcp://127.0.0.1:80"), nullptr, nullptr, 1, - envoy::config::core::v3::Locality(), health_check_config, 0, envoy::config::core::v3::UNKNOWN, - simTime()); + auto test_host = std::shared_ptr( + *HostImpl::create(cluster_->info_, "", *Network::Utility::resolveUrl("tcp://127.0.0.1:80"), + nullptr, nullptr, 1, envoy::config::core::v3::Locality(), + health_check_config, 0, envoy::config::core::v3::UNKNOWN, simTime())); const std::string path = "/healthcheck"; // Setup health check config with a different host, to check that we still get the host configured // on the endpoint. @@ -2599,19 +2599,19 @@ TEST_F(HttpHealthCheckerImplTest, DynamicRemoveDisableHC) { envoy::config::endpoint::v3::Endpoint::HealthCheckConfig health_check_config; health_check_config.set_disable_active_health_check(false); - auto enable_host = std::make_shared( + auto enable_host = std::shared_ptr(*HostImpl::create( cluster_->info_, "test_host", *Network::Utility::resolveUrl("tcp://127.0.0.1:80"), nullptr, nullptr, 1, envoy::config::core::v3::Locality(), health_check_config, 0, - envoy::config::core::v3::UNKNOWN, simTime()); + envoy::config::core::v3::UNKNOWN, simTime())); cluster_->prioritySet().getMockHostSet(0)->hosts_ = {enable_host}; health_checker_->start(); EXPECT_CALL(*this, onHostStatus(_, HealthTransition::Unchanged)); health_check_config.set_disable_active_health_check(true); - auto disable_host = std::make_shared( + auto disable_host = std::shared_ptr(*HostImpl::create( cluster_->info_, "test_host", *Network::Utility::resolveUrl("tcp://127.0.0.1:80"), nullptr, nullptr, 1, envoy::config::core::v3::Locality(), health_check_config, 0, - envoy::config::core::v3::UNKNOWN, simTime()); + envoy::config::core::v3::UNKNOWN, simTime())); cluster_->prioritySet().getMockHostSet(0)->hosts_ = {disable_host}; EXPECT_CALL(*test_sessions_[0]->client_connection_, close(Network::ConnectionCloseType::Abort)); cluster_->prioritySet().runUpdateCallbacks(0, {disable_host}, {enable_host}); @@ -2628,10 +2628,10 @@ TEST_F(HttpHealthCheckerImplTest, AddDisableHC) { envoy::config::endpoint::v3::Endpoint::HealthCheckConfig health_check_config; health_check_config.set_disable_active_health_check(true); - auto disable_host = std::make_shared( + auto disable_host = std::shared_ptr(*HostImpl::create( cluster_->info_, "test_host", *Network::Utility::resolveUrl("tcp://127.0.0.1:80"), nullptr, nullptr, 1, envoy::config::core::v3::Locality(), health_check_config, 0, - envoy::config::core::v3::UNKNOWN, simTime()); + envoy::config::core::v3::UNKNOWN, simTime())); cluster_->prioritySet().getMockHostSet(0)->hosts_ = {disable_host}; health_checker_->start(); EXPECT_CALL(*this, onHostStatus(_, HealthTransition::Unchanged)).Times(0); @@ -5238,10 +5238,10 @@ TEST_F(GrpcHealthCheckerImplTest, SuccessWithHostname) { envoy::config::endpoint::v3::Endpoint::HealthCheckConfig health_check_config; health_check_config.set_hostname(expected_host); - auto test_host = std::make_shared( - cluster_->info_, "", *Network::Utility::resolveUrl("tcp://127.0.0.1:80"), nullptr, nullptr, 1, - envoy::config::core::v3::Locality(), health_check_config, 0, envoy::config::core::v3::UNKNOWN, - simTime()); + auto test_host = std::shared_ptr( + *HostImpl::create(cluster_->info_, "", *Network::Utility::resolveUrl("tcp://127.0.0.1:80"), + nullptr, nullptr, 1, envoy::config::core::v3::Locality(), + health_check_config, 0, envoy::config::core::v3::UNKNOWN, simTime())); cluster_->prioritySet().getMockHostSet(0)->hosts_ = {test_host}; runHealthCheck(expected_host); } @@ -5253,10 +5253,10 @@ TEST_F(GrpcHealthCheckerImplTest, SuccessWithHostnameOverridesConfig) { envoy::config::endpoint::v3::Endpoint::HealthCheckConfig health_check_config; health_check_config.set_hostname(expected_host); - auto test_host = std::make_shared( - cluster_->info_, "", *Network::Utility::resolveUrl("tcp://127.0.0.1:80"), nullptr, nullptr, 1, - envoy::config::core::v3::Locality(), health_check_config, 0, envoy::config::core::v3::UNKNOWN, - simTime()); + auto test_host = std::shared_ptr( + *HostImpl::create(cluster_->info_, "", *Network::Utility::resolveUrl("tcp://127.0.0.1:80"), + nullptr, nullptr, 1, envoy::config::core::v3::Locality(), + health_check_config, 0, envoy::config::core::v3::UNKNOWN, simTime())); cluster_->prioritySet().getMockHostSet(0)->hosts_ = {test_host}; runHealthCheck(expected_host); } diff --git a/test/common/upstream/load_balancer_simulation_test.cc b/test/common/upstream/load_balancer_simulation_test.cc index ebb009931a97..288cc7f9d5ec 100644 --- a/test/common/upstream/load_balancer_simulation_test.cc +++ b/test/common/upstream/load_balancer_simulation_test.cc @@ -38,7 +38,7 @@ static HostSharedPtr newTestHost(Upstream::ClusterInfoConstSharedPtr cluster, uint32_t weight = 1, const std::string& zone = "") { envoy::config::core::v3::Locality locality; locality.set_zone(zone); - return HostSharedPtr{new HostImpl( + return HostSharedPtr{*HostImpl::create( cluster, "", *Network::Utility::resolveUrl(url), nullptr, nullptr, weight, locality, envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 0, envoy::config::core::v3::UNKNOWN, time_source)}; diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index fc78e4c7189d..b61496f1afd6 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -1653,18 +1653,18 @@ TEST_F(HostImplTest, HostnameCanaryAndLocality) { locality.set_region("oceania"); locality.set_zone("hello"); locality.set_sub_zone("world"); - HostImpl host(cluster.info_, "lyft.com", *Network::Utility::resolveUrl("tcp://10.0.0.1:1234"), - std::make_shared(metadata), nullptr, 1, - locality, - envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 1, - envoy::config::core::v3::UNKNOWN, simTime()); - EXPECT_EQ(cluster.info_.get(), &host.cluster()); - EXPECT_EQ("lyft.com", host.hostname()); - EXPECT_TRUE(host.canary()); - EXPECT_EQ("oceania", host.locality().region()); - EXPECT_EQ("hello", host.locality().zone()); - EXPECT_EQ("world", host.locality().sub_zone()); - EXPECT_EQ(1, host.priority()); + std::unique_ptr host = *HostImpl::create( + cluster.info_, "lyft.com", *Network::Utility::resolveUrl("tcp://10.0.0.1:1234"), + std::make_shared(metadata), nullptr, 1, locality, + envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 1, + envoy::config::core::v3::UNKNOWN, simTime()); + EXPECT_EQ(cluster.info_.get(), &host->cluster()); + EXPECT_EQ("lyft.com", host->hostname()); + EXPECT_TRUE(host->canary()); + EXPECT_EQ("oceania", host->locality().region()); + EXPECT_EQ("hello", host->locality().zone()); + EXPECT_EQ("world", host->locality().sub_zone()); + EXPECT_EQ(1, host->priority()); } TEST_F(HostImplTest, CreateConnection) { @@ -1679,11 +1679,11 @@ TEST_F(HostImplTest, CreateConnection) { locality.set_sub_zone("world"); Network::Address::InstanceConstSharedPtr address = *Network::Utility::resolveUrl("tcp://10.0.0.1:1234"); - auto host = std::make_shared( + auto host = std::shared_ptr(*HostImpl::create( cluster.info_, "lyft.com", address, std::make_shared(metadata), nullptr, 1, locality, envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 1, - envoy::config::core::v3::UNKNOWN, simTime()); + envoy::config::core::v3::UNKNOWN, simTime())); testing::StrictMock dispatcher; Network::TransportSocketOptionsConstSharedPtr transport_socket_options; @@ -1717,11 +1717,11 @@ TEST_F(HostImplTest, CreateConnectionHappyEyeballs) { address, *Network::Utility::resolveUrl("tcp://10.0.0.1:1235"), }; - auto host = std::make_shared( + auto host = std::shared_ptr(*HostImpl::create( cluster.info_, "lyft.com", address, std::make_shared(metadata), nullptr, 1, locality, envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 1, - envoy::config::core::v3::UNKNOWN, simTime(), address_list); + envoy::config::core::v3::UNKNOWN, simTime(), address_list)); testing::StrictMock dispatcher; Network::TransportSocketOptionsConstSharedPtr transport_socket_options; @@ -1764,11 +1764,11 @@ TEST_F(HostImplTest, ProxyOverridesHappyEyeballs) { address, *Network::Utility::resolveUrl("tcp://10.0.0.1:1235"), }; - auto host = std::make_shared( + auto host = std::shared_ptr(*HostImpl::create( cluster.info_, "lyft.com", address, std::make_shared(metadata), nullptr, 1, locality, envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 1, - envoy::config::core::v3::UNKNOWN, simTime(), address_list); + envoy::config::core::v3::UNKNOWN, simTime(), address_list)); testing::StrictMock dispatcher; auto proxy_info = std::make_unique( @@ -1822,11 +1822,11 @@ TEST_F(HostImplTest, CreateConnectionHappyEyeballsWithConfig) { address, *Network::Utility::resolveUrl("tcp://10.0.0.1:1235"), }; - auto host = std::make_shared( + auto host = std::shared_ptr(*HostImpl::create( cluster.info_, "lyft.com", address, std::make_shared(metadata), nullptr, 1, locality, envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 1, - envoy::config::core::v3::UNKNOWN, simTime(), address_list); + envoy::config::core::v3::UNKNOWN, simTime(), address_list)); testing::StrictMock dispatcher; Network::TransportSocketOptionsConstSharedPtr transport_socket_options; @@ -1875,11 +1875,11 @@ TEST_F(HostImplTest, CreateConnectionHappyEyeballsWithEmptyConfig) { address, *Network::Utility::resolveUrl("tcp://10.0.0.1:1235"), }; - auto host = std::make_shared( + auto host = std::shared_ptr(*HostImpl::create( cluster.info_, "lyft.com", address, std::make_shared(metadata), nullptr, 1, locality, envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 1, - envoy::config::core::v3::UNKNOWN, simTime(), address_list); + envoy::config::core::v3::UNKNOWN, simTime(), address_list)); testing::StrictMock dispatcher; Network::TransportSocketOptionsConstSharedPtr transport_socket_options; @@ -2018,16 +2018,15 @@ TEST_F(HostImplTest, SkipActiveHealthCheckFlag) { // This is a regression test for oss-fuzz issue // https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=11095 TEST_F(HostImplTest, HealthPipeAddress) { - EXPECT_THROW_WITH_MESSAGE( - { - std::shared_ptr info{new NiceMock()}; - envoy::config::endpoint::v3::Endpoint::HealthCheckConfig config; - config.set_port_value(8000); - HostDescriptionImpl descr(info, "", *Network::Utility::resolveUrl("unix://foo"), nullptr, - nullptr, envoy::config::core::v3::Locality().default_instance(), - config, 1, simTime()); - }, - EnvoyException, "Invalid host configuration: non-zero port for non-IP address"); + std::shared_ptr info{new NiceMock()}; + envoy::config::endpoint::v3::Endpoint::HealthCheckConfig config; + config.set_port_value(8000); + EXPECT_EQ(HostDescriptionImpl::create( + info, "", *Network::Utility::resolveUrl("unix://foo"), nullptr, nullptr, + envoy::config::core::v3::Locality().default_instance(), config, 1, simTime()) + .status() + .message(), + "Invalid host configuration: non-zero port for non-IP address"); } TEST_F(HostImplTest, HostAddressList) { @@ -2042,10 +2041,10 @@ TEST_F(HostImplTest, HealthcheckHostname) { std::shared_ptr info{new NiceMock()}; envoy::config::endpoint::v3::Endpoint::HealthCheckConfig config; config.set_hostname("foo"); - HostDescriptionImpl descr(info, "", *Network::Utility::resolveUrl("tcp://1.2.3.4:80"), nullptr, - nullptr, envoy::config::core::v3::Locality().default_instance(), config, - 1, simTime()); - EXPECT_EQ("foo", descr.hostnameForHealthChecks()); + std::unique_ptr descr = *HostDescriptionImpl::create( + info, "", *Network::Utility::resolveUrl("tcp://1.2.3.4:80"), nullptr, nullptr, + envoy::config::core::v3::Locality().default_instance(), config, 1, simTime()); + EXPECT_EQ("foo", descr->hostnameForHealthChecks()); } class StaticClusterImplTest : public testing::Test, public UpstreamImplTestBase {}; diff --git a/test/common/upstream/utility.h b/test/common/upstream/utility.h index b227995ded3c..c1f20ed2df0d 100644 --- a/test/common/upstream/utility.h +++ b/test/common/upstream/utility.h @@ -97,22 +97,22 @@ inline envoy::config::cluster::v3::Cluster defaultStaticCluster(const std::strin inline HostSharedPtr makeTestHost(ClusterInfoConstSharedPtr cluster, const std::string& hostname, const std::string& url, TimeSource& time_source, uint32_t weight = 1) { - return std::make_shared( + return std::shared_ptr(*HostImpl::create( cluster, hostname, *Network::Utility::resolveUrl(url), nullptr, nullptr, weight, envoy::config::core::v3::Locality(), envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 0, - envoy::config::core::v3::UNKNOWN, time_source); + envoy::config::core::v3::UNKNOWN, time_source)); } inline HostSharedPtr makeTestHost(ClusterInfoConstSharedPtr cluster, const std::string& url, TimeSource& time_source, uint32_t weight = 1, uint32_t priority = 0, Host::HealthStatus status = Host::HealthStatus::UNKNOWN) { - return std::make_shared( + return std::shared_ptr(*HostImpl::create( cluster, "", *Network::Utility::resolveUrl(url), nullptr, nullptr, weight, envoy::config::core::v3::Locality(), envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), priority, - status, time_source); + status, time_source)); } inline HostSharedPtr makeTestHost(ClusterInfoConstSharedPtr cluster, const std::string& url, @@ -120,42 +120,42 @@ inline HostSharedPtr makeTestHost(ClusterInfoConstSharedPtr cluster, const std:: envoy::config::core::v3::Locality locality, uint32_t weight = 1, uint32_t priority = 0, Host::HealthStatus status = Host::HealthStatus::UNKNOWN) { - return std::make_shared( + return std::shared_ptr(*HostImpl::create( cluster, "", *Network::Utility::resolveUrl(url), nullptr, nullptr, weight, locality, envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), priority, - status, time_source); + status, time_source)); } inline HostSharedPtr makeTestHost(ClusterInfoConstSharedPtr cluster, const std::string& url, const envoy::config::core::v3::Metadata& metadata, TimeSource& time_source, uint32_t weight = 1) { - return std::make_shared( + return std::shared_ptr(*HostImpl::create( cluster, "", *Network::Utility::resolveUrl(url), std::make_shared(metadata), nullptr, weight, envoy::config::core::v3::Locality(), envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 0, - envoy::config::core::v3::UNKNOWN, time_source); + envoy::config::core::v3::UNKNOWN, time_source)); } inline HostSharedPtr makeTestHost(ClusterInfoConstSharedPtr cluster, const std::string& url, const envoy::config::core::v3::Metadata& metadata, envoy::config::core::v3::Locality locality, TimeSource& time_source, uint32_t weight = 1) { - return std::make_shared( + return std::shared_ptr(*HostImpl::create( cluster, "", *Network::Utility::resolveUrl(url), std::make_shared(metadata), nullptr, weight, locality, envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 0, - envoy::config::core::v3::UNKNOWN, time_source); + envoy::config::core::v3::UNKNOWN, time_source)); } inline HostSharedPtr makeTestHost(ClusterInfoConstSharedPtr cluster, const std::string& url, const envoy::config::endpoint::v3::Endpoint::HealthCheckConfig& health_check_config, TimeSource& time_source, uint32_t weight = 1) { - return std::make_shared(cluster, "", *Network::Utility::resolveUrl(url), nullptr, - nullptr, weight, envoy::config::core::v3::Locality(), - health_check_config, 0, envoy::config::core::v3::UNKNOWN, - time_source); + return std::shared_ptr( + *HostImpl::create(cluster, "", *Network::Utility::resolveUrl(url), nullptr, nullptr, weight, + envoy::config::core::v3::Locality(), health_check_config, 0, + envoy::config::core::v3::UNKNOWN, time_source)); } inline HostSharedPtr makeTestHostWithHashKey(ClusterInfoConstSharedPtr cluster, @@ -165,32 +165,33 @@ inline HostSharedPtr makeTestHostWithHashKey(ClusterInfoConstSharedPtr cluster, Config::Metadata::mutableMetadataValue(metadata, Config::MetadataFilters::get().ENVOY_LB, Config::MetadataEnvoyLbKeys::get().HASH_KEY) .set_string_value(hash_key); - return std::make_shared( + return std::shared_ptr(*HostImpl::create( cluster, "", *Network::Utility::resolveUrl(url), std::make_shared(metadata), nullptr, weight, envoy::config::core::v3::Locality(), envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 0, - envoy::config::core::v3::UNKNOWN, time_source); + envoy::config::core::v3::UNKNOWN, time_source)); } inline HostSharedPtr makeTestHostWithMetadata(ClusterInfoConstSharedPtr cluster, MetadataConstSharedPtr metadata, const std::string& url, TimeSource& time_source, uint32_t weight = 1) { - return std::make_shared( + return std::shared_ptr(*HostImpl::create( cluster, "", *Network::Utility::resolveUrl(url), metadata, nullptr, weight, envoy::config::core::v3::Locality(), envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 0, - envoy::config::core::v3::UNKNOWN, time_source); + envoy::config::core::v3::UNKNOWN, time_source)); } inline HostDescriptionConstSharedPtr makeTestHostDescription(ClusterInfoConstSharedPtr cluster, const std::string& url, TimeSource& time_source) { - return std::make_shared( + return std::shared_ptr(*HostDescriptionImpl::create( cluster, "", *Network::Utility::resolveUrl(url), nullptr, nullptr, envoy::config::core::v3::Locality().default_instance(), - envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 0, time_source); + envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 0, + time_source)); } inline HostsPerLocalitySharedPtr makeHostsPerLocality(std::vector&& locality_hosts, diff --git a/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc b/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc index f5356450fd73..8e7c3cec365b 100644 --- a/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc +++ b/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc @@ -249,7 +249,7 @@ TEST_F(ClusterTest, BasicFlow) { // LB will immediately resolve host1. EXPECT_CALL(*this, onMemberUpdateCb(SizeIs(1), SizeIs(0))); - update_callbacks_->onDnsHostAddOrUpdate("host1:0", host_map_["host1:0"]); + EXPECT_TRUE(update_callbacks_->onDnsHostAddOrUpdate("host1:0", host_map_["host1:0"]).ok()); EXPECT_EQ(1UL, cluster_->prioritySet().hostSetsPerPriority()[0]->hosts().size()); EXPECT_EQ("1.2.3.4:0", cluster_->prioritySet().hostSetsPerPriority()[0]->hosts()[0]->address()->asString()); @@ -259,7 +259,7 @@ TEST_F(ClusterTest, BasicFlow) { // After changing the address, LB will immediately resolve the new address with a refresh. updateTestHostAddress("host1:0", "2.3.4.5"); - update_callbacks_->onDnsHostAddOrUpdate("host1:0", host_map_["host1:0"]); + EXPECT_TRUE(update_callbacks_->onDnsHostAddOrUpdate("host1:0", host_map_["host1:0"]).ok()); EXPECT_EQ(1UL, cluster_->prioritySet().hostSetsPerPriority()[0]->hosts().size()); EXPECT_EQ("2.3.4.5:0", cluster_->prioritySet().hostSetsPerPriority()[0]->hosts()[0]->address()->asString()); @@ -282,13 +282,13 @@ TEST_F(ClusterTest, OutlierDetection) { InSequence s; EXPECT_CALL(*this, onMemberUpdateCb(SizeIs(1), SizeIs(0))); - update_callbacks_->onDnsHostAddOrUpdate("host1:0", host_map_["host1:0"]); + EXPECT_TRUE(update_callbacks_->onDnsHostAddOrUpdate("host1:0", host_map_["host1:0"]).ok()); EXPECT_CALL(*host_map_["host1:0"], touch()); EXPECT_EQ("1.2.3.4:0", lb_->chooseHost(setHostAndReturnContext("host1:0"))->address()->asString()); EXPECT_CALL(*this, onMemberUpdateCb(SizeIs(1), SizeIs(0))); - update_callbacks_->onDnsHostAddOrUpdate("host2:0", host_map_["host2:0"]); + EXPECT_TRUE(update_callbacks_->onDnsHostAddOrUpdate("host2:0", host_map_["host2:0"]).ok()); EXPECT_CALL(*host_map_["host2:0"], touch()); EXPECT_EQ("5.6.7.8:0", lb_->chooseHost(setHostAndReturnContext("host2:0"))->address()->asString()); @@ -321,7 +321,7 @@ TEST_F(ClusterTest, FilterStateHostOverride) { makeTestHost("host1:0", "1.2.3.4"); EXPECT_CALL(*this, onMemberUpdateCb(SizeIs(1), SizeIs(0))); - update_callbacks_->onDnsHostAddOrUpdate("host1:0", host_map_["host1:0"]); + EXPECT_TRUE(update_callbacks_->onDnsHostAddOrUpdate("host1:0", host_map_["host1:0"]).ok()); EXPECT_CALL(*host_map_["host1:0"], touch()); EXPECT_EQ("1.2.3.4:0", lb_->chooseHost(setFilterStateHostAndReturnContext("host1:0"))->address()->asString()); diff --git a/test/extensions/common/dynamic_forward_proxy/mocks.h b/test/extensions/common/dynamic_forward_proxy/mocks.h index 03dc81476d29..fb59bc27600c 100644 --- a/test/extensions/common/dynamic_forward_proxy/mocks.h +++ b/test/extensions/common/dynamic_forward_proxy/mocks.h @@ -111,7 +111,7 @@ class MockUpdateCallbacks : public DnsCache::UpdateCallbacks { MockUpdateCallbacks(); ~MockUpdateCallbacks() override; - MOCK_METHOD(void, onDnsHostAddOrUpdate, + MOCK_METHOD(absl::Status, onDnsHostAddOrUpdate, (const std::string& host, const DnsHostInfoSharedPtr& address)); MOCK_METHOD(void, onDnsHostRemove, (const std::string& host)); MOCK_METHOD(void, onDnsResolutionComplete, diff --git a/test/extensions/load_balancing_policies/random/random_lb_simulation_test.cc b/test/extensions/load_balancing_policies/random/random_lb_simulation_test.cc index e4c08f8303da..b5675daffae1 100644 --- a/test/extensions/load_balancing_policies/random/random_lb_simulation_test.cc +++ b/test/extensions/load_balancing_policies/random/random_lb_simulation_test.cc @@ -23,7 +23,7 @@ static HostSharedPtr newTestHost(Upstream::ClusterInfoConstSharedPtr cluster, uint32_t weight = 1, const std::string& zone = "") { envoy::config::core::v3::Locality locality; locality.set_zone(zone); - return HostSharedPtr{new HostImpl( + return HostSharedPtr{*HostImpl::create( cluster, "", *Network::Utility::resolveUrl(url), nullptr, nullptr, weight, locality, envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 0, envoy::config::core::v3::UNKNOWN, time_source)}; diff --git a/test/integration/clusters/custom_static_cluster.cc b/test/integration/clusters/custom_static_cluster.cc index 944c11a0ff9a..0ad11eda1f26 100644 --- a/test/integration/clusters/custom_static_cluster.cc +++ b/test/integration/clusters/custom_static_cluster.cc @@ -24,7 +24,7 @@ void CustomStaticCluster::startPreInit() { Upstream::HostSharedPtr CustomStaticCluster::makeHost() { Network::Address::InstanceConstSharedPtr address = Network::Utility::parseInternetAddressNoThrow(address_, port_, true); - return Upstream::HostSharedPtr{new Upstream::HostImpl( + return Upstream::HostSharedPtr{*Upstream::HostImpl::create( info(), "", address, std::make_shared(info()->metadata()), nullptr, 1, envoy::config::core::v3::Locality::default_instance(), diff --git a/test/integration/utility.cc b/test/integration/utility.cc index 40d3a533639e..0cf478c4e883 100644 --- a/test/integration/utility.cc +++ b/test/integration/utility.cc @@ -221,13 +221,13 @@ IntegrationUtil::makeSingleRequest(const Network::Address::InstanceConstSharedPt std::shared_ptr cluster{new NiceMock()}; Upstream::HostDescriptionConstSharedPtr host_description = - std::make_shared( + std::shared_ptr(*Upstream::HostDescriptionImpl::create( cluster, "", *Network::Utility::resolveUrl( fmt::format("{}://127.0.0.1:80", (type == Http::CodecType::HTTP3 ? "udp" : "tcp"))), nullptr, nullptr, envoy::config::core::v3::Locality().default_instance(), envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 0, - time_system); + time_system)); if (type <= Http::CodecType::HTTP2) { Http::CodecClientProd client(type,