Skip to content

Commit

Permalink
Upstream: removing exceptions from hostimp
Browse files Browse the repository at this point in the history 
Signed-off-by: Alyssa Wilk <[email protected]>
  • Loading branch information
@alyssawilk
alyssawilk committed Oct 8, 2024
1 parent a9ce686 commit 78eb3de
Show file tree
Hide file tree
Showing 29 changed files with 332 additions and 204 deletions.
5 changes: 3 additions & 2 deletions mobile/library/common/network/connectivity_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion mobile/library/common/network/connectivity_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ 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 {}
void onDnsHostRemove(const std::string& /*host*/) override {}
Expand Down
30 changes: 18 additions & 12 deletions mobile/test/common/network/connectivity_manager_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>(),
Network::DnsResolver::ResolutionStatus::Completed);
connectivity_manager_->onDnsResolutionComplete(
"not-cached.example.com",
std::make_shared<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>(),
Network::DnsResolver::ResolutionStatus::Completed);
connectivity_manager_->onDnsResolutionComplete(
"not-cached2.example.com",
std::make_shared<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>(),
Network::DnsResolver::ResolutionStatus::Completed);
connectivity_manager_
->onDnsResolutionComplete(
"cached.example.com",
std::make_shared<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>(),
Network::DnsResolver::ResolutionStatus::Completed)
.IgnoreError();
connectivity_manager_
->onDnsResolutionComplete(
"not-cached.example.com",
std::make_shared<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>(),
Network::DnsResolver::ResolutionStatus::Completed)
.IgnoreError();
connectivity_manager_
->onDnsResolutionComplete(
"not-cached2.example.com",
std::make_shared<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>(),
Network::DnsResolver::ResolutionStatus::Completed)
.IgnoreError();
}

TEST_F(ConnectivityManagerTest, WhenDrainPostDnsNotEnabledDoesntDrainPostDnsRefresh) {
Expand Down
18 changes: 10 additions & 8 deletions source/common/upstream/health_discovery_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<HostImpl>(
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<HostImpl>(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<HostImpl>));
// 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
Expand Down Expand Up @@ -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<HostImpl>(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<HostImpl>(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<HostImpl>));

// Set the initial health status as in HdsCluster::initialize.
host->healthFlagSet(Host::HealthFlag::FAILED_ACTIVE_HC);
Expand Down
59 changes: 46 additions & 13 deletions source/common/upstream/upstream_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -427,14 +427,29 @@ LoadMetricStats::StatMapPtr LoadMetricStatsImpl::latch() {
return latched;
}

HostDescriptionImpl::HostDescriptionImpl(
absl::StatusOr<std::unique_ptr<HostDescriptionImpl>> 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<HostDescriptionImpl>(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)) {}
Expand All @@ -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(),
Expand All @@ -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);
Expand Down Expand Up @@ -649,6 +664,23 @@ Host::CreateConnectionData HostImplBase::createConnection(

void HostImplBase::weight(uint32_t new_weight) { weight_ = std::max(1U, new_weight); }

absl::StatusOr<std::unique_ptr<HostImpl>> 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<HostImpl>(
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<HostsPerLocalityConstSharedPtr> HostsPerLocalityImpl::filter(
const std::vector<std::function<bool(const Host&)>>& predicates) const {
// We keep two lists: one for being able to mutate the clone and one for returning to the
Expand Down Expand Up @@ -2163,11 +2195,13 @@ void PriorityStateManager::registerHostForPriority(
locality_lb_endpoint.has_metadata()
? parent_.constMetadataSharedPool()->getObject(locality_lb_endpoint.metadata())
: nullptr;
const auto host = std::make_shared<HostImpl>(
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<HostImpl>(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<HostImpl>));
registerHostForPriority(host, locality_lb_endpoint);
}

Expand Down Expand Up @@ -2471,7 +2505,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;
}
Expand Down Expand Up @@ -2563,7 +2596,7 @@ Network::Address::InstanceConstSharedPtr resolveHealthCheckAddress(
auto address = address_or_error.value();
health_check_address =
port_value == 0 ? address : Network::Utility::getAddressWithPort(*address, port_value);
} else {
} else if (host_address && host_address->ip()) {
health_check_address = port_value == 0
? host_address
: Network::Utility::getAddressWithPort(*host_address, port_value);
Expand Down
58 changes: 39 additions & 19 deletions source/common/upstream/upstream_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,14 +159,6 @@ class DetectorHostMonitorNullImpl : public Outlier::DetectorHostMonitor {
class HostDescriptionImplBase : virtual public HostDescription,
protected Logger::Loggable<Logger::Id::upstream> {
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_;
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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<std::unique_ptr<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 = {});

// HostDescription
Network::Address::InstanceConstSharedPtr address() const override { return address_; }
Expand All @@ -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
Expand Down Expand Up @@ -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<std::unique_ptr<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 = {});

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 {
Expand Down
Loading

0 comments on commit 78eb3de

Please sign in to comment.