Skip to content

Commit

Permalink
upstream: reducing exceptions (#36497)
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 <[email protected]>
  • Loading branch information
@alyssawilk
alyssawilk authored Oct 9, 2024
1 parent 364e45c commit 5024ec7
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 34 deletions.
10 changes: 6 additions & 4 deletions source/common/upstream/health_discovery_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -550,10 +550,12 @@ ProdClusterInfoFactory::createClusterInfo(const CreateClusterInfoParams& params)
factory_context, socket_factory, *scope),
std::unique_ptr<TransportSocketMatcherImpl>);

return std::make_unique<ClusterInfoImpl>(
params.server_context_.initManager(), params.server_context_, params.cluster_,
params.bind_config_, params.server_context_.runtime(), std::move(socket_matcher),
std::move(scope), params.added_via_api_, factory_context);
return THROW_OR_RETURN_VALUE(
ClusterInfoImpl::create(params.server_context_.initManager(), params.server_context_,
params.cluster_, params.bind_config_,
params.server_context_.runtime(), std::move(socket_matcher),
std::move(scope), params.added_via_api_, factory_context),
std::unique_ptr<ClusterInfoImpl>);
}

void HdsCluster::initHealthchecks() {
Expand Down
75 changes: 51 additions & 24 deletions source/common/upstream/upstream_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1132,13 +1132,30 @@ LegacyLbPolicyConfigHelper::getTypedLbConfigFromLegacyProto(
using ProtocolOptionsHashMap =
absl::flat_hash_map<std::string, ProtocolOptionsConfigConstSharedPtr>;

absl::StatusOr<std::unique_ptr<ClusterInfoImpl>>
ClusterInfoImpl::create(Init::Manager& info,
Server::Configuration::ServerFactoryContext& server_context,
const envoy::config::cluster::v3::Cluster& config,
const absl::optional<envoy::config::core::v3::BindConfig>& bind_config,
Runtime::Loader& runtime, TransportSocketMatcherPtr&& socket_matcher,
Stats::ScopeSharedPtr&& stats_scope, bool added_via_api,
Server::Configuration::TransportSocketFactoryContext& ctx) {
absl::Status creation_status = absl::OkStatus();
auto ret = std::unique_ptr<ClusterInfoImpl>(new ClusterInfoImpl(
info, server_context, config, bind_config, runtime, std::move(socket_matcher),
std::move(stats_scope), added_via_api, ctx, creation_status));
RETURN_IF_NOT_OK(creation_status);
return ret;
}

ClusterInfoImpl::ClusterInfoImpl(
Init::Manager& init_manager, Server::Configuration::ServerFactoryContext& server_context,
const envoy::config::cluster::v3::Cluster& config,
const absl::optional<envoy::config::core::v3::BindConfig>& bind_config,
Runtime::Loader& runtime, TransportSocketMatcherPtr&& socket_matcher,
Stats::ScopeSharedPtr&& stats_scope, bool added_via_api,
Server::Configuration::TransportSocketFactoryContext& factory_context)
Server::Configuration::TransportSocketFactoryContext& factory_context,
absl::Status& creation_status)
: runtime_(runtime), name_(config.name()),
observability_name_(!config.alt_stat_name().empty()
? std::make_unique<std::string>(config.alt_stat_name())
Expand Down Expand Up @@ -1253,9 +1270,10 @@ ClusterInfoImpl::ClusterInfoImpl(
config.track_cluster_stats().per_endpoint_stats()) {
#ifdef WIN32
if (set_local_interface_name_on_upstream_connections_) {
throwEnvoyExceptionOrPanic(
creation_status = absl::InvalidArgumentError(
"set_local_interface_name_on_upstream_connections_ cannot be set to true "
"on Windows platforms");
return;
}
#endif

Expand All @@ -1264,42 +1282,45 @@ ClusterInfoImpl::ClusterInfoImpl(
// TODO(ggreenway): Verify that bypassing virtual dispatch here was intentional
if (ClusterInfoImpl::perEndpointStatsEnabled() &&
server_context.bootstrap().cluster_manager().has_load_stats_config()) {
throwEnvoyExceptionOrPanic("Only one of cluster per_endpoint_stats and cluster manager "
"load_stats_config can be specified");
creation_status =
absl::InvalidArgumentError("Only one of cluster per_endpoint_stats and cluster manager "
"load_stats_config can be specified");
return;
}

if (config.has_max_requests_per_connection() &&
http_protocol_options_->common_http_protocol_options_.has_max_requests_per_connection()) {
throwEnvoyExceptionOrPanic("Only one of max_requests_per_connection from Cluster or "
"HttpProtocolOptions can be specified");
creation_status =
absl::InvalidArgumentError("Only one of max_requests_per_connection from Cluster or "
"HttpProtocolOptions can be specified");
return;
}

if (config.has_load_balancing_policy() ||
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.

THROW_IF_NOT_OK(configureLbPolicies(config, server_context));
SET_AND_RETURN_IF_NOT_OK(configureLbPolicies(config, server_context), creation_status);
} else {
// If load_balancing_policy is not set, we will try to convert legacy lb_policy
// to load_balancing_policy and use it.
LoadBalancerFactoryContextImpl lb_factory_context(server_context);

auto lb_pair = LegacyLbPolicyConfigHelper::getTypedLbConfigFromLegacyProto(
lb_factory_context, config, server_context.messageValidationVisitor());

if (!lb_pair.ok()) {
throwEnvoyExceptionOrPanic(std::string(lb_pair.status().message()));
}
SET_AND_RETURN_IF_NOT_OK(lb_pair.status(), creation_status);
load_balancer_factory_ = lb_pair->factory;
ASSERT(load_balancer_factory_ != nullptr, "null load balancer factory");
load_balancer_config_ = std::move(lb_pair->config);
}

if (config.lb_subset_config().locality_weight_aware() &&
!config.common_lb_config().has_locality_weighted_lb_config()) {
throwEnvoyExceptionOrPanic(fmt::format("Locality weight aware subset LB requires that a "
"locality_weighted_lb_config be set in {}",
name_));
creation_status =
absl::InvalidArgumentError(fmt::format("Locality weight aware subset LB requires that a "
"locality_weighted_lb_config be set in {}",
name_));
return;
}

// Use default (1h) or configured `idle_timeout`, unless it's set to 0, indicating that no
Expand Down Expand Up @@ -1345,7 +1366,8 @@ ClusterInfoImpl::ClusterInfoImpl(

if (config.has_eds_cluster_config()) {
if (config.type() != envoy::config::cluster::v3::Cluster::EDS) {
throwEnvoyExceptionOrPanic("eds_cluster_config set in a non-EDS cluster");
creation_status = absl::InvalidArgumentError("eds_cluster_config set in a non-EDS cluster");
return;
}
}

Expand All @@ -1365,7 +1387,9 @@ ClusterInfoImpl::ClusterInfoImpl(

if (proto_config.has_config_discovery()) {
if (proto_config.has_typed_config()) {
throwEnvoyExceptionOrPanic("Only one of typed_config or config_discovery can be used");
creation_status =
absl::InvalidArgumentError("Only one of typed_config or config_discovery can be used");
return;
}

ENVOY_LOG(debug, " dynamic filter name: {}", proto_config.name());
Expand Down Expand Up @@ -1405,9 +1429,10 @@ ClusterInfoImpl::ClusterInfoImpl(
const auto last_type_url =
Config::Utility::getFactoryType(http_filters[http_filters.size() - 1].typed_config());
if (last_type_url != upstream_codec_type_url) {
throwEnvoyExceptionOrPanic(fmt::format(
creation_status = absl::InvalidArgumentError(fmt::format(
"The codec filter is the only valid terminal upstream HTTP filter, use '{}'",
upstream_codec_type_url));
return;
}
}

Expand All @@ -1416,8 +1441,9 @@ ClusterInfoImpl::ClusterInfoImpl(
Server::Configuration::UpstreamHttpFilterConfigFactory>
helper(*http_filter_config_provider_manager_, upstream_context_.serverFactoryContext(),
factory_context.clusterManager(), upstream_context_, prefix);
THROW_IF_NOT_OK(helper.processFilters(http_filters, "upstream http", "upstream http",
http_filter_factories_));
SET_AND_RETURN_IF_NOT_OK(helper.processFilters(http_filters, "upstream http", "upstream http",
http_filter_factories_),
creation_status);
}
}

Expand Down Expand Up @@ -1595,12 +1621,13 @@ ClusterImplBase::ClusterImplBase(const envoy::config::cluster::v3::Cluster& clus
auto socket_matcher = std::move(*socket_matcher_or_error);
const bool matcher_supports_alpn = socket_matcher->allMatchesSupportAlpn();
auto& dispatcher = server_context.mainThreadDispatcher();
auto info_or_error = ClusterInfoImpl::create(
init_manager_, server_context, cluster, cluster_context.clusterManager().bindConfig(),
runtime_, std::move(socket_matcher), std::move(stats_scope), cluster_context.addedViaApi(),
*transport_factory_context_);
SET_AND_RETURN_IF_NOT_OK(info_or_error.status(), creation_status);
info_ = std::shared_ptr<const ClusterInfoImpl>(
new ClusterInfoImpl(init_manager_, server_context, cluster,
cluster_context.clusterManager().bindConfig(), runtime_,
std::move(socket_matcher), std::move(stats_scope),
cluster_context.addedViaApi(), *transport_factory_context_),
[&dispatcher](const ClusterInfoImpl* self) {
(*info_or_error).release(), [&dispatcher](const ClusterInfoImpl* self) {
ENVOY_LOG(trace, "Schedule destroy cluster info {}", self->name());
dispatcher.deleteInDispatcherThread(
std::unique_ptr<const Event::DispatcherThreadDeletable>(self));
Expand Down
21 changes: 15 additions & 6 deletions source/common/upstream/upstream_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -812,12 +812,13 @@ class ClusterInfoImpl : public ClusterInfo,
using HttpProtocolOptionsConfigImpl =
Envoy::Extensions::Upstreams::Http::ProtocolOptionsConfigImpl;
using TcpProtocolOptionsConfigImpl = Envoy::Extensions::Upstreams::Tcp::ProtocolOptionsConfigImpl;
ClusterInfoImpl(Init::Manager& info, Server::Configuration::ServerFactoryContext& server_context,
const envoy::config::cluster::v3::Cluster& config,
const absl::optional<envoy::config::core::v3::BindConfig>& bind_config,
Runtime::Loader& runtime, TransportSocketMatcherPtr&& socket_matcher,
Stats::ScopeSharedPtr&& stats_scope, bool added_via_api,
Server::Configuration::TransportSocketFactoryContext&);
static absl::StatusOr<std::unique_ptr<ClusterInfoImpl>>
create(Init::Manager& info, Server::Configuration::ServerFactoryContext& server_context,
const envoy::config::cluster::v3::Cluster& config,
const absl::optional<envoy::config::core::v3::BindConfig>& bind_config,
Runtime::Loader& runtime, TransportSocketMatcherPtr&& socket_matcher,
Stats::ScopeSharedPtr&& stats_scope, bool added_via_api,
Server::Configuration::TransportSocketFactoryContext&);

static DeferredCreationCompatibleClusterTrafficStats
generateStats(Stats::ScopeSharedPtr scope, const ClusterTrafficStatNames& cluster_stat_names,
Expand Down Expand Up @@ -1038,6 +1039,14 @@ class ClusterInfoImpl : public ClusterInfo,
}

protected:
ClusterInfoImpl(Init::Manager& info, Server::Configuration::ServerFactoryContext& server_context,
const envoy::config::cluster::v3::Cluster& config,
const absl::optional<envoy::config::core::v3::BindConfig>& bind_config,
Runtime::Loader& runtime, TransportSocketMatcherPtr&& socket_matcher,
Stats::ScopeSharedPtr&& stats_scope, bool added_via_api,
Server::Configuration::TransportSocketFactoryContext& context,
absl::Status& creation_status);

// Gets the retry budget percent/concurrency from the circuit breaker thresholds. If the retry
// budget message is specified, defaults will be filled in if either params are unspecified.
static std::pair<absl::optional<double>, absl::optional<uint32_t>>
Expand Down

0 comments on commit 5024ec7

Please sign in to comment.