Skip to content

Commit

Permalink
clusters: cleaning up some exceptions (envoyproxy#34248)
Browse files Browse the repository at this point in the history 
Risk Level: low
Testing: updated tests
Docs Changes: n/a
Release Notes: n/a
envoyproxy/envoy-mobile#176

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
  • Loading branch information
@alyssawilk
alyssawilk committed May 23, 2024
1 parent c7ce725 commit eda7d32
Show file tree
Hide file tree
Showing 5 changed files with 86 additions and 65 deletions.
117 changes: 66 additions & 51 deletions source/common/upstream/upstream_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ createProtocolOptionsConfig(const std::string& name, const ProtobufWkt::Any& typ
}

if (factory == nullptr) {
throwEnvoyExceptionOrPanic(
return absl::InvalidArgumentError(
fmt::format("Didn't find a registered network or http filter or protocol "
"options implementation for name: '{}'",
name));
Expand All @@ -106,7 +106,8 @@ createProtocolOptionsConfig(const std::string& name, const ProtobufWkt::Any& typ
ProtobufTypes::MessagePtr proto_config = factory->createEmptyProtocolOptionsProto();

if (proto_config == nullptr) {
throwEnvoyExceptionOrPanic(fmt::format("filter {} does not support protocol options", name));
return absl::InvalidArgumentError(
fmt::format("filter {} does not support protocol options", name));
}

Envoy::Config::Utility::translateOpaqueConfig(
Expand Down Expand Up @@ -219,7 +220,7 @@ buildClusterSocketOptions(const envoy::config::cluster::v3::Cluster& cluster_con
return cluster_options;
}

std::vector<::Envoy::Upstream::UpstreamLocalAddress>
absl::StatusOr<std::vector<::Envoy::Upstream::UpstreamLocalAddress>>
parseBindConfig(::Envoy::OptRef<const envoy::config::core::v3::BindConfig> bind_config,
const absl::optional<std::string>& cluster_name,
Network::ConnectionSocket::OptionsSharedPtr base_socket_options,
Expand All @@ -233,7 +234,7 @@ parseBindConfig(::Envoy::OptRef<const envoy::config::core::v3::BindConfig> bind_

auto address_or_error =
::Envoy::Network::Address::resolveProtoSocketAddress(bind_config->source_address());
THROW_IF_STATUS_NOT_OK(address_or_error, throw);
RETURN_IF_STATUS_NOT_OK(address_or_error);
upstream_local_address.address_ = address_or_error.value();
}
upstream_local_address.socket_options_ = std::make_shared<Network::ConnectionSocket::Options>();
Expand All @@ -249,7 +250,7 @@ parseBindConfig(::Envoy::OptRef<const envoy::config::core::v3::BindConfig> bind_
UpstreamLocalAddress extra_upstream_local_address;
auto address_or_error =
::Envoy::Network::Address::resolveProtoSocketAddress(extra_source_address.address());
THROW_IF_STATUS_NOT_OK(address_or_error, throw);
RETURN_IF_STATUS_NOT_OK(address_or_error);
extra_upstream_local_address.address_ = address_or_error.value();

extra_upstream_local_address.socket_options_ =
Expand All @@ -273,7 +274,7 @@ parseBindConfig(::Envoy::OptRef<const envoy::config::core::v3::BindConfig> bind_
UpstreamLocalAddress additional_upstream_local_address;
auto address_or_error =
::Envoy::Network::Address::resolveProtoSocketAddress(additional_source_address);
THROW_IF_STATUS_NOT_OK(address_or_error, throw);
RETURN_IF_STATUS_NOT_OK(address_or_error);
additional_upstream_local_address.address_ = address_or_error.value();
additional_upstream_local_address.socket_options_ =
std::make_shared<::Envoy::Network::ConnectionSocket::Options>();
Expand All @@ -297,7 +298,7 @@ parseBindConfig(::Envoy::OptRef<const envoy::config::core::v3::BindConfig> bind_
if (upstream_local_addresses.size() > 1) {
for (auto const& upstream_local_address : upstream_local_addresses) {
if (upstream_local_address.address_ == nullptr) {
throwEnvoyExceptionOrPanic(fmt::format(
return absl::InvalidArgumentError(fmt::format(
"{}'s upstream binding config has invalid IP addresses.",
!(cluster_name.has_value()) ? "Bootstrap"
: fmt::format("Cluster {}", cluster_name.value())));
Expand All @@ -308,7 +309,8 @@ parseBindConfig(::Envoy::OptRef<const envoy::config::core::v3::BindConfig> bind_
return upstream_local_addresses;
}

Envoy::Upstream::UpstreamLocalAddressSelectorConstSharedPtr createUpstreamLocalAddressSelector(
absl::StatusOr<Envoy::Upstream::UpstreamLocalAddressSelectorConstSharedPtr>
createUpstreamLocalAddressSelector(
const envoy::config::cluster::v3::Cluster& cluster_config,
const absl::optional<envoy::config::core::v3::BindConfig>& bootstrap_bind_config) {

Expand All @@ -327,7 +329,7 @@ Envoy::Upstream::UpstreamLocalAddressSelectorConstSharedPtr createUpstreamLocalA
if (bind_config.has_value()) {
if (bind_config->additional_source_addresses_size() > 0 &&
bind_config->extra_source_addresses_size() > 0) {
throwEnvoyExceptionOrPanic(fmt::format(
return absl::InvalidArgumentError(fmt::format(
"Can't specify both `extra_source_addresses` and `additional_source_addresses` "
"in the {}'s upstream binding config",
!(cluster_name.has_value()) ? "Bootstrap"
Expand All @@ -337,7 +339,7 @@ Envoy::Upstream::UpstreamLocalAddressSelectorConstSharedPtr createUpstreamLocalA
if (!bind_config->has_source_address() &&
(bind_config->extra_source_addresses_size() > 0 ||
bind_config->additional_source_addresses_size() > 0)) {
throwEnvoyExceptionOrPanic(fmt::format(
return absl::InvalidArgumentError(fmt::format(
"{}'s upstream binding config has extra/additional source addresses but no "
"source_address. Extra/additional addresses cannot be specified if "
"source_address is not set.",
Expand All @@ -363,15 +365,17 @@ Envoy::Upstream::UpstreamLocalAddressSelectorConstSharedPtr createUpstreamLocalA
Config::Utility::getAndCheckFactory<UpstreamLocalAddressSelectorFactory>(typed_extension,
false);
}
auto selector_or_error = local_address_selector_factory->createLocalAddressSelector(
absl::StatusOr<std::vector<::Envoy::Upstream::UpstreamLocalAddress>> config_or_error =
parseBindConfig(
bind_config, cluster_name,
buildBaseSocketOptions(cluster_config, bootstrap_bind_config.value_or(
envoy::config::core::v3::BindConfig{})),
buildClusterSocketOptions(cluster_config, bootstrap_bind_config.value_or(
envoy::config::core::v3::BindConfig{}))),
cluster_name);
THROW_IF_STATUS_NOT_OK(selector_or_error, throw);
envoy::config::core::v3::BindConfig{})));
RETURN_IF_STATUS_NOT_OK(config_or_error);
auto selector_or_error = local_address_selector_factory->createLocalAddressSelector(
config_or_error.value(), cluster_name);
RETURN_IF_STATUS_NOT_OK(selector_or_error);
return selector_or_error.value();
}

Expand Down Expand Up @@ -944,7 +948,7 @@ ClusterInfoImpl::generateTimeoutBudgetStats(Stats::Scope& scope,
return {stat_names, scope};
}

std::shared_ptr<const ClusterInfoImpl::HttpProtocolOptionsConfigImpl>
absl::StatusOr<std::shared_ptr<const ClusterInfoImpl::HttpProtocolOptionsConfigImpl>>
createOptions(const envoy::config::cluster::v3::Cluster& config,
std::shared_ptr<const ClusterInfoImpl::HttpProtocolOptionsConfigImpl>&& options,
ProtobufMessage::ValidationVisitor& validation_visitor) {
Expand All @@ -955,7 +959,7 @@ createOptions(const envoy::config::cluster::v3::Cluster& config,
if (config.protocol_selection() == envoy::config::cluster::v3::Cluster::USE_CONFIGURED_PROTOCOL) {
// Make sure multiple protocol configurations are not present
if (config.has_http_protocol_options() && config.has_http2_protocol_options()) {
throwEnvoyExceptionOrPanic(
return absl::InvalidArgumentError(
fmt::format("cluster: Both HTTP1 and HTTP2 options may only be "
"configured with non-default 'protocol_selection' values"));
}
Expand All @@ -972,7 +976,7 @@ createOptions(const envoy::config::cluster::v3::Cluster& config,
config.protocol_selection() ==
envoy::config::cluster::v3::Cluster::USE_DOWNSTREAM_PROTOCOL,
config.has_http2_protocol_options(), validation_visitor);
THROW_IF_STATUS_NOT_OK(options_or_error, throw);
RETURN_IF_STATUS_NOT_OK(options_or_error);
return options_or_error.value();
}

Expand Down Expand Up @@ -1060,11 +1064,12 @@ ClusterInfoImpl::ClusterInfoImpl(
? std::make_unique<std::string>(config.eds_cluster_config().service_name())
: nullptr),
extension_protocol_options_(parseExtensionProtocolOptions(config, factory_context)),
http_protocol_options_(
http_protocol_options_(THROW_OR_RETURN_VALUE(
createOptions(config,
extensionProtocolOptionsTyped<HttpProtocolOptionsConfigImpl>(
"envoy.extensions.upstreams.http.v3.HttpProtocolOptions"),
factory_context.messageValidationVisitor())),
factory_context.messageValidationVisitor()),
std::shared_ptr<const ClusterInfoImpl::HttpProtocolOptionsConfigImpl>)),
tcp_protocol_options_(extensionProtocolOptionsTyped<TcpProtocolOptionsConfigImpl>(
"envoy.extensions.upstreams.tcp.v3.TcpProtocolOptions")),
max_requests_per_connection_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(
Expand Down Expand Up @@ -1097,7 +1102,9 @@ ClusterInfoImpl::ClusterInfoImpl(
resource_managers_(config, runtime, name_, *stats_scope_,
factory_context.clusterManager().clusterCircuitBreakersStatNames()),
maintenance_mode_runtime_key_(absl::StrCat("upstream.maintenance_mode.", name_)),
upstream_local_address_selector_(createUpstreamLocalAddressSelector(config, bind_config)),
upstream_local_address_selector_(
THROW_OR_RETURN_VALUE(createUpstreamLocalAddressSelector(config, bind_config),
Envoy::Upstream::UpstreamLocalAddressSelectorConstSharedPtr)),
upstream_config_(config.has_upstream_config()
? std::make_unique<envoy::config::core::v3::TypedExtensionConfig>(
config.upstream_config())
Expand Down Expand Up @@ -1170,7 +1177,7 @@ ClusterInfoImpl::ClusterInfoImpl(
config.lb_policy() == envoy::config::cluster::v3::Cluster::LOAD_BALANCING_POLICY_CONFIG) {
// If load_balancing_policy is set we will use it directly, ignoring lb_policy.

configureLbPolicies(config, server_context);
THROW_IF_NOT_OK(configureLbPolicies(config, server_context));
} else {
// If load_balancing_policy is not set, we will try to convert legacy lb_policy
// to load_balancing_policy and use it.
Expand Down Expand Up @@ -1306,23 +1313,24 @@ ClusterInfoImpl::ClusterInfoImpl(
}

// Configures the load balancer based on config.load_balancing_policy
void ClusterInfoImpl::configureLbPolicies(const envoy::config::cluster::v3::Cluster& config,
Server::Configuration::ServerFactoryContext& context) {
absl::Status
ClusterInfoImpl::configureLbPolicies(const envoy::config::cluster::v3::Cluster& config,
Server::Configuration::ServerFactoryContext& context) {
// Check if load_balancing_policy is set first.
if (!config.has_load_balancing_policy()) {
throwEnvoyExceptionOrPanic("cluster: field load_balancing_policy need to be set");
return absl::InvalidArgumentError("cluster: field load_balancing_policy need to be set");
}

if (config.has_lb_subset_config()) {
throwEnvoyExceptionOrPanic(
return absl::InvalidArgumentError(
"cluster: load_balancing_policy cannot be combined with lb_subset_config");
}

if (config.has_common_lb_config()) {
const auto& lb_config = config.common_lb_config();
if (lb_config.has_zone_aware_lb_config() || lb_config.has_locality_weighted_lb_config() ||
lb_config.has_consistent_hashing_lb_config()) {
throwEnvoyExceptionOrPanic(
return absl::InvalidArgumentError(
"cluster: load_balancing_policy cannot be combined with partial fields "
"(zone_aware_lb_config, "
"locality_weighted_lb_config, consistent_hashing_lb_config) of common_lb_config");
Expand Down Expand Up @@ -1350,11 +1358,12 @@ void ClusterInfoImpl::configureLbPolicies(const envoy::config::cluster::v3::Clus
}

if (load_balancer_factory_ == nullptr) {
throwEnvoyExceptionOrPanic(
return absl::InvalidArgumentError(
fmt::format("cluster: didn't find a registered load balancer factory "
"implementation for cluster: '{}' with names from [{}]",
name_, absl::StrJoin(missing_policies, ", ")));
}
return absl::OkStatus();
}

ProtocolOptionsConfigConstSharedPtr
Expand Down Expand Up @@ -1426,7 +1435,7 @@ ClusterInfoImpl::upstreamHttpProtocol(absl::optional<Http::Protocol> downstream_
: Http::Protocol::Http11};
}

bool validateTransportSocketSupportsQuic(
absl::StatusOr<bool> validateTransportSocketSupportsQuic(
const envoy::config::core::v3::TransportSocket& transport_socket) {
// The transport socket is valid for QUIC if it is either a QUIC transport socket,
// or if it is a QUIC transport socket wrapped in an HTTP/1.1 proxy socket.
Expand All @@ -1438,7 +1447,7 @@ bool validateTransportSocketSupportsQuic(
}
envoy::extensions::transport_sockets::http_11_proxy::v3::Http11ProxyUpstreamTransport
http11_socket;
THROW_IF_NOT_OK(MessageUtil::unpackTo(transport_socket.typed_config(), http11_socket));
RETURN_IF_NOT_OK(MessageUtil::unpackTo(transport_socket.typed_config(), http11_socket));
return http11_socket.transport_socket().name() == "envoy.transport_sockets.quic";
}

Expand Down Expand Up @@ -1500,7 +1509,8 @@ ClusterImplBase::ClusterImplBase(const envoy::config::cluster::v3::Cluster& clus

if (info_->features() & ClusterInfoImpl::Features::HTTP3) {
#if defined(ENVOY_ENABLE_QUIC)
if (!validateTransportSocketSupportsQuic(cluster.transport_socket())) {
if (!THROW_OR_RETURN_VALUE(validateTransportSocketSupportsQuic(cluster.transport_socket()),
bool)) {
throwEnvoyExceptionOrPanic(
fmt::format("HTTP3 requires a QuicUpstreamTransport transport socket: {} {}",
cluster.name(), cluster.transport_socket().DebugString()));
Expand Down Expand Up @@ -1772,32 +1782,35 @@ void ClusterImplBase::reloadHealthyHostsHelper(const HostSharedPtr&) {
}
}

const Network::Address::InstanceConstSharedPtr
absl::StatusOr<const Network::Address::InstanceConstSharedPtr>
ClusterImplBase::resolveProtoAddress(const envoy::config::core::v3::Address& address) {
absl::Status resolve_status;
TRY_ASSERT_MAIN_THREAD {
auto address_or_error = Network::Address::resolveProtoAddress(address);
THROW_IF_STATUS_NOT_OK(address_or_error, throw);
return address_or_error.value();
if (address_or_error.value()) {
return address_or_error.value();
}
resolve_status = address_or_error.status();
}
END_TRY
CATCH(EnvoyException & e, {
if (info_->type() == envoy::config::cluster::v3::Cluster::STATIC ||
info_->type() == envoy::config::cluster::v3::Cluster::EDS) {
throwEnvoyExceptionOrPanic(
fmt::format("{}. Consider setting resolver_name or setting cluster type "
"to 'STRICT_DNS' or 'LOGICAL_DNS'",
e.what()));
}
throw e;
});
CATCH(EnvoyException & e, { resolve_status = absl::InvalidArgumentError(e.what()); });
if (info_->type() == envoy::config::cluster::v3::Cluster::STATIC ||
info_->type() == envoy::config::cluster::v3::Cluster::EDS) {
return absl::InvalidArgumentError(
fmt::format("{}. Consider setting resolver_name or setting cluster type "
"to 'STRICT_DNS' or 'LOGICAL_DNS'",
resolve_status.message()));
}
return resolve_status;
}

void ClusterImplBase::validateEndpointsForZoneAwareRouting(
absl::Status ClusterImplBase::validateEndpointsForZoneAwareRouting(
const envoy::config::endpoint::v3::LocalityLbEndpoints& endpoints) const {
if (local_cluster_ && endpoints.priority() > 0) {
throwEnvoyExceptionOrPanic(
return absl::InvalidArgumentError(
fmt::format("Unexpected non-zero priority for local cluster '{}'.", info()->name()));
}
return absl::OkStatus();
}

ClusterInfoImpl::OptionalClusterStats::OptionalClusterStats(
Expand All @@ -1819,10 +1832,12 @@ ClusterInfoImpl::ResourceManagers::ResourceManagers(
const std::string& cluster_name, Stats::Scope& stats_scope,
const ClusterCircuitBreakersStatNames& circuit_breakers_stat_names)
: circuit_breakers_stat_names_(circuit_breakers_stat_names) {
managers_[enumToInt(ResourcePriority::Default)] =
load(config, runtime, cluster_name, stats_scope, envoy::config::core::v3::DEFAULT);
managers_[enumToInt(ResourcePriority::High)] =
load(config, runtime, cluster_name, stats_scope, envoy::config::core::v3::HIGH);
managers_[enumToInt(ResourcePriority::Default)] = THROW_OR_RETURN_VALUE(
load(config, runtime, cluster_name, stats_scope, envoy::config::core::v3::DEFAULT),
ResourceManagerImplPtr);
managers_[enumToInt(ResourcePriority::High)] = THROW_OR_RETURN_VALUE(
load(config, runtime, cluster_name, stats_scope, envoy::config::core::v3::HIGH),
ResourceManagerImplPtr);
}

ClusterCircuitBreakersStats
Expand Down Expand Up @@ -1922,7 +1937,7 @@ ClusterInfoImpl::createSingletonUpstreamNetworkFilterConfigProviderManager(
[] { return std::make_shared<Filter::UpstreamNetworkFilterConfigProviderManagerImpl>(); });
}

ResourceManagerImplPtr
absl::StatusOr<ResourceManagerImplPtr>
ClusterInfoImpl::ResourceManagers::load(const envoy::config::cluster::v3::Cluster& config,
Runtime::Loader& runtime, const std::string& cluster_name,
Stats::Scope& stats_scope,
Expand Down Expand Up @@ -1983,7 +1998,7 @@ ClusterInfoImpl::ResourceManagers::load(const envoy::config::cluster::v3::Cluste
if (per_host_it->has_max_pending_requests() || per_host_it->has_max_requests() ||
per_host_it->has_max_retries() || per_host_it->has_max_connection_pools() ||
per_host_it->has_retry_budget()) {
throwEnvoyExceptionOrPanic("Unsupported field in per_host_thresholds");
return absl::InvalidArgumentError("Unsupported field in per_host_thresholds");
}
if (per_host_it->has_max_connections()) {
max_connections_per_host = per_host_it->max_connections().value();
Expand Down
Loading

0 comments on commit eda7d32

Please sign in to comment.