From 5024ec7b304a8a305fe0f95473aeac6fd20ddaea Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Wed, 9 Oct 2024 16:10:17 -0400 Subject: [PATCH] upstream: reducing exceptions (#36497) Risk Level: low Testing: updated tests Docs Changes: n/a Release Notes: n/a https://github.com/envoyproxy/envoy-mobile/issues/176 Signed-off-by: Alyssa Wilk --- .../upstream/health_discovery_service.cc | 10 ++- source/common/upstream/upstream_impl.cc | 75 +++++++++++++------ source/common/upstream/upstream_impl.h | 21 ++++-- 3 files changed, 72 insertions(+), 34 deletions(-) diff --git a/source/common/upstream/health_discovery_service.cc b/source/common/upstream/health_discovery_service.cc index c12503b2169c..36fa65196d7e 100644 --- a/source/common/upstream/health_discovery_service.cc +++ b/source/common/upstream/health_discovery_service.cc @@ -550,10 +550,12 @@ ProdClusterInfoFactory::createClusterInfo(const CreateClusterInfoParams& params) factory_context, socket_factory, *scope), std::unique_ptr); - return std::make_unique( - 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); } void HdsCluster::initHealthchecks() { diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 49f7bdaa932e..1dafc23d65b8 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -1132,13 +1132,30 @@ LegacyLbPolicyConfigHelper::getTypedLbConfigFromLegacyProto( using ProtocolOptionsHashMap = absl::flat_hash_map; +absl::StatusOr> +ClusterInfoImpl::create(Init::Manager& info, + Server::Configuration::ServerFactoryContext& server_context, + const envoy::config::cluster::v3::Cluster& config, + const absl::optional& 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(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& 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(config.alt_stat_name()) @@ -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 @@ -1264,21 +1282,25 @@ 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. @@ -1286,10 +1308,7 @@ ClusterInfoImpl::ClusterInfoImpl( 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); @@ -1297,9 +1316,11 @@ ClusterInfoImpl::ClusterInfoImpl( 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 @@ -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; } } @@ -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()); @@ -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; } } @@ -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); } } @@ -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( - 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(self)); diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index b3c93e451bb7..6b537c75d9cd 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -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& bind_config, - Runtime::Loader& runtime, TransportSocketMatcherPtr&& socket_matcher, - Stats::ScopeSharedPtr&& stats_scope, bool added_via_api, - Server::Configuration::TransportSocketFactoryContext&); + static absl::StatusOr> + create(Init::Manager& info, Server::Configuration::ServerFactoryContext& server_context, + const envoy::config::cluster::v3::Cluster& config, + const absl::optional& 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, @@ -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& 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>