From 49e74fd4936e5400765647a4740e2d38492a5c17 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 20 Mar 2024 14:58:06 -0400 Subject: [PATCH] subscription: making exception free Signed-off-by: Alyssa Wilk --- envoy/config/subscription_factory.h | 10 +-- envoy/runtime/runtime.h | 3 +- .../config/subscription_factory_impl.cc | 69 +++++++++++-------- .../common/config/subscription_factory_impl.h | 11 ++- source/common/filter/config_discovery_impl.cc | 7 +- source/common/listener_manager/lds_api.cc | 12 ++-- .../rds/rds_route_config_subscription.cc | 5 +- source/common/router/scoped_rds.cc | 10 +-- source/common/router/vhds.cc | 5 +- source/common/runtime/runtime_impl.cc | 12 ++-- source/common/runtime/runtime_impl.h | 4 +- source/common/secret/sds_api.cc | 6 +- source/common/upstream/cds_api_impl.cc | 12 ++-- source/common/upstream/od_cds_api_impl.cc | 12 ++-- source/extensions/clusters/eds/eds.cc | 5 +- source/extensions/clusters/eds/leds.cc | 5 +- source/server/config_validation/server.cc | 2 +- source/server/server.cc | 2 +- test/common/runtime/runtime_impl_test.cc | 5 +- test/common/secret/sds_api_test.cc | 4 +- .../common/subscription_factory_impl_test.cc | 37 ++++++---- test/mocks/config/mocks.h | 4 +- test/mocks/runtime/mocks.h | 2 +- tools/code_format/config.yaml | 6 +- 24 files changed, 150 insertions(+), 100 deletions(-) diff --git a/envoy/config/subscription_factory.h b/envoy/config/subscription_factory.h index de9d02100ef3..36193cdc4d82 100644 --- a/envoy/config/subscription_factory.h +++ b/envoy/config/subscription_factory.h @@ -54,9 +54,10 @@ class SubscriptionFactory { * @param resource_decoder how incoming opaque resource objects are to be decoded. * @param options subscription options. * - * @return SubscriptionPtr subscription object corresponding for config and type_url. + * @return SubscriptionPtr subscription object corresponding for config and type_url or error + * status. */ - virtual SubscriptionPtr subscriptionFromConfigSource( + virtual absl::StatusOr subscriptionFromConfigSource( const envoy::config::core::v3::ConfigSource& config, absl::string_view type_url, Stats::Scope& scope, SubscriptionCallbacks& callbacks, OpaqueResourceDecoderSharedPtr resource_decoder, const SubscriptionOptions& options) PURE; @@ -73,9 +74,10 @@ class SubscriptionFactory { * CollectionSubscription object. * @param resource_decoder how incoming opaque resource objects are to be decoded. * - * @return SubscriptionPtr subscription object corresponding for collection_locator. + * @return SubscriptionPtr subscription object corresponding for collection_locator or error + * status. */ - virtual SubscriptionPtr + virtual absl::StatusOr collectionSubscriptionFromUrl(const xds::core::v3::ResourceLocator& collection_locator, const envoy::config::core::v3::ConfigSource& config, absl::string_view type_url, Stats::Scope& scope, diff --git a/envoy/runtime/runtime.h b/envoy/runtime/runtime.h index a3c1f8bd1353..bd9d06a50a2b 100644 --- a/envoy/runtime/runtime.h +++ b/envoy/runtime/runtime.h @@ -227,8 +227,9 @@ class Loader { * the constructor is finished, with the exception of dynamic RTDS layers, * which require ClusterManager. * @param cm cluster manager reference. + * @return a status indicating if initialization was successful. */ - virtual void initialize(Upstream::ClusterManager& cm) PURE; + virtual absl::Status initialize(Upstream::ClusterManager& cm) PURE; /** * @return const Snapshot& the current snapshot. This reference is safe to use for the duration of diff --git a/source/common/config/subscription_factory_impl.cc b/source/common/config/subscription_factory_impl.cc index 4e11b27da8b5..c0f9ce344767 100644 --- a/source/common/config/subscription_factory_impl.cc +++ b/source/common/config/subscription_factory_impl.cc @@ -23,11 +23,11 @@ SubscriptionFactoryImpl::SubscriptionFactoryImpl( validation_visitor_(validation_visitor), api_(api), server_(server), xds_resources_delegate_(xds_resources_delegate), xds_config_tracker_(xds_config_tracker) {} -SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( +absl::StatusOr SubscriptionFactoryImpl::subscriptionFromConfigSource( const envoy::config::core::v3::ConfigSource& config, absl::string_view type_url, Stats::Scope& scope, SubscriptionCallbacks& callbacks, OpaqueResourceDecoderSharedPtr resource_decoder, const SubscriptionOptions& options) { - THROW_IF_NOT_OK(Config::Utility::checkLocalInfo(type_url, local_info_)); + RETURN_IF_NOT_OK(Config::Utility::checkLocalInfo(type_url, local_info_)); SubscriptionStats stats = Utility::generateStats(scope); std::string subscription_type = ""; @@ -50,29 +50,29 @@ SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( switch (config.config_source_specifier_case()) { case envoy::config::core::v3::ConfigSource::ConfigSourceSpecifierCase::kPath: { - THROW_IF_NOT_OK(Utility::checkFilesystemSubscriptionBackingPath(config.path(), api_)); + RETURN_IF_NOT_OK(Utility::checkFilesystemSubscriptionBackingPath(config.path(), api_)); subscription_type = "envoy.config_subscription.filesystem"; break; } case envoy::config::core::v3::ConfigSource::ConfigSourceSpecifierCase::kPathConfigSource: { - THROW_IF_NOT_OK( + RETURN_IF_NOT_OK( Utility::checkFilesystemSubscriptionBackingPath(config.path_config_source().path(), api_)); subscription_type = "envoy.config_subscription.filesystem"; break; } case envoy::config::core::v3::ConfigSource::ConfigSourceSpecifierCase::kApiConfigSource: { const envoy::config::core::v3::ApiConfigSource& api_config_source = config.api_config_source(); - THROW_IF_NOT_OK(Utility::checkApiConfigSourceSubscriptionBackingCluster(cm_.primaryClusters(), - api_config_source)); - THROW_IF_NOT_OK(Utility::checkTransportVersion(api_config_source)); + RETURN_IF_NOT_OK(Utility::checkApiConfigSourceSubscriptionBackingCluster(cm_.primaryClusters(), + api_config_source)); + RETURN_IF_NOT_OK(Utility::checkTransportVersion(api_config_source)); switch (api_config_source.api_type()) { PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; case envoy::config::core::v3::ApiConfigSource::AGGREGATED_GRPC: - throwEnvoyExceptionOrPanic("Unsupported config source AGGREGATED_GRPC"); + return absl::InvalidArgumentError("Unsupported config source AGGREGATED_GRPC"); case envoy::config::core::v3::ApiConfigSource::AGGREGATED_DELTA_GRPC: - throwEnvoyExceptionOrPanic("Unsupported config source AGGREGATED_DELTA_GRPC"); + return absl::InvalidArgumentError("Unsupported config source AGGREGATED_DELTA_GRPC"); case envoy::config::core::v3::ApiConfigSource::DEPRECATED_AND_UNAVAILABLE_DO_NOT_USE: - throwEnvoyExceptionOrPanic( + return absl::InvalidArgumentError( "REST_LEGACY no longer a supported ApiConfigSource. " "Please specify an explicit supported api_type in the following config:\n" + config.DebugString()); @@ -87,7 +87,7 @@ SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( break; } if (subscription_type.empty()) { - throwEnvoyExceptionOrPanic("Invalid API config source API type"); + return absl::InvalidArgumentError("Invalid API config source API type"); } break; } @@ -96,32 +96,32 @@ SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( break; } default: - throwEnvoyExceptionOrPanic( + return absl::InvalidArgumentError( "Missing config source specifier in envoy::config::core::v3::ConfigSource"); } ConfigSubscriptionFactory* factory = Registry::FactoryRegistry::getFactory(subscription_type); if (factory == nullptr) { - throwEnvoyExceptionOrPanic(fmt::format( + return absl::InvalidArgumentError(fmt::format( "Didn't find a registered config subscription factory implementation for name: '{}'", subscription_type)); } return factory->create(data); } -SubscriptionPtr createFromFactoryOrThrow(ConfigSubscriptionFactory::SubscriptionData& data, - absl::string_view subscription_type) { +absl::StatusOr createFromFactory(ConfigSubscriptionFactory::SubscriptionData& data, + absl::string_view subscription_type) { ConfigSubscriptionFactory* factory = Registry::FactoryRegistry::getFactory(subscription_type); if (factory == nullptr) { - throwEnvoyExceptionOrPanic(fmt::format( + return absl::InvalidArgumentError(fmt::format( "Didn't find a registered config subscription factory implementation for name: '{}'", subscription_type)); } return factory->create(data); } -SubscriptionPtr SubscriptionFactoryImpl::collectionSubscriptionFromUrl( +absl::StatusOr SubscriptionFactoryImpl::collectionSubscriptionFromUrl( const xds::core::v3::ResourceLocator& collection_locator, const envoy::config::core::v3::ConfigSource& config, absl::string_view resource_type, Stats::Scope& scope, SubscriptionCallbacks& callbacks, @@ -148,13 +148,15 @@ SubscriptionPtr SubscriptionFactoryImpl::collectionSubscriptionFromUrl( switch (collection_locator.scheme()) { case xds::core::v3::ResourceLocator::FILE: { const std::string path = Http::Utility::localPathFromFilePath(collection_locator.id()); - THROW_IF_NOT_OK(Utility::checkFilesystemSubscriptionBackingPath(path, api_)); + RETURN_IF_NOT_OK(Utility::checkFilesystemSubscriptionBackingPath(path, api_)); factory_config.set_path(path); - return createFromFactoryOrThrow(data, "envoy.config_subscription.filesystem_collection"); + auto ptr_or_error = createFromFactory(data, "envoy.config_subscription.filesystem_collection"); + RETURN_IF_NOT_OK(ptr_or_error.status()); + return std::move(ptr_or_error.value()); } case xds::core::v3::ResourceLocator::XDSTP: { if (resource_type != collection_locator.resource_type()) { - throwEnvoyExceptionOrPanic( + return absl::InvalidArgumentError( fmt::format("xdstp:// type does not match {} in {}", resource_type, Config::XdsResourceIdentifier::encodeUrl(collection_locator))); } @@ -162,8 +164,8 @@ SubscriptionPtr SubscriptionFactoryImpl::collectionSubscriptionFromUrl( case envoy::config::core::v3::ConfigSource::ConfigSourceSpecifierCase::kApiConfigSource: { const envoy::config::core::v3::ApiConfigSource& api_config_source = config.api_config_source(); - THROW_IF_NOT_OK(Utility::checkApiConfigSourceSubscriptionBackingCluster(cm_.primaryClusters(), - api_config_source)); + RETURN_IF_NOT_OK(Utility::checkApiConfigSourceSubscriptionBackingCluster( + cm_.primaryClusters(), api_config_source)); // All Envoy collections currently are xDS resource graph roots and require node context // parameters. options.add_xdstp_node_context_params_ = true; @@ -171,17 +173,22 @@ SubscriptionPtr SubscriptionFactoryImpl::collectionSubscriptionFromUrl( case envoy::config::core::v3::ApiConfigSource::DELTA_GRPC: { std::string type_url = TypeUtil::descriptorFullNameToTypeUrl(resource_type); data.type_url_ = type_url; - return createFromFactoryOrThrow(data, "envoy.config_subscription.delta_grpc_collection"); + auto ptr_or_error = + createFromFactory(data, "envoy.config_subscription.delta_grpc_collection"); + RETURN_IF_NOT_OK(ptr_or_error.status()); + return std::move(ptr_or_error.value()); } case envoy::config::core::v3::ApiConfigSource::AGGREGATED_GRPC: FALLTHRU; case envoy::config::core::v3::ApiConfigSource::AGGREGATED_DELTA_GRPC: { - return createFromFactoryOrThrow(data, - "envoy.config_subscription.aggregated_grpc_collection"); + auto ptr_or_error = + createFromFactory(data, "envoy.config_subscription.aggregated_grpc_collection"); + RETURN_IF_NOT_OK(ptr_or_error.status()); + return std::move(ptr_or_error.value()); } default: - throwEnvoyExceptionOrPanic(fmt::format("Unknown xdstp:// transport API type in {}", - api_config_source.DebugString())); + return absl::InvalidArgumentError(fmt::format("Unknown xdstp:// transport API type in {}", + api_config_source.DebugString())); } } case envoy::config::core::v3::ConfigSource::ConfigSourceSpecifierCase::kAds: { @@ -189,10 +196,12 @@ SubscriptionPtr SubscriptionFactoryImpl::collectionSubscriptionFromUrl( // All Envoy collections currently are xDS resource graph roots and require node context // parameters. options.add_xdstp_node_context_params_ = true; - return createFromFactoryOrThrow(data, "envoy.config_subscription.ads_collection"); + auto ptr_or_error = createFromFactory(data, "envoy.config_subscription.ads_collection"); + RETURN_IF_NOT_OK(ptr_or_error.status()); + return std::move(ptr_or_error.value()); } default: - throwEnvoyExceptionOrPanic( + return absl::InvalidArgumentError( "Missing or not supported config source specifier in " "envoy::config::core::v3::ConfigSource for a collection. Only ADS and " "gRPC in delta-xDS mode are supported."); @@ -200,7 +209,7 @@ SubscriptionPtr SubscriptionFactoryImpl::collectionSubscriptionFromUrl( } default: // TODO(htuch): Implement HTTP semantics for collection ResourceLocators. - throwEnvoyExceptionOrPanic("Unsupported code path"); + return absl::InvalidArgumentError("Unsupported code path"); } } diff --git a/source/common/config/subscription_factory_impl.h b/source/common/config/subscription_factory_impl.h index 2f37e08408e7..aff5a088340a 100644 --- a/source/common/config/subscription_factory_impl.h +++ b/source/common/config/subscription_factory_impl.h @@ -26,12 +26,11 @@ class SubscriptionFactoryImpl : public SubscriptionFactory, Logger::Loggable subscriptionFromConfigSource( + const envoy::config::core::v3::ConfigSource& config, absl::string_view type_url, + Stats::Scope& scope, SubscriptionCallbacks& callbacks, + OpaqueResourceDecoderSharedPtr resource_decoder, const SubscriptionOptions& options) override; + absl::StatusOr collectionSubscriptionFromUrl(const xds::core::v3::ResourceLocator& collection_locator, const envoy::config::core::v3::ConfigSource& config, absl::string_view resource_type, Stats::Scope& scope, diff --git a/source/common/filter/config_discovery_impl.cc b/source/common/filter/config_discovery_impl.cc index d3cb3e26b70d..5962df8e2345 100644 --- a/source/common/filter/config_discovery_impl.cc +++ b/source/common/filter/config_discovery_impl.cc @@ -78,8 +78,11 @@ FilterConfigSubscription::FilterConfigSubscription( filter_config_provider_manager_(filter_config_provider_manager), subscription_id_(subscription_id) { const auto resource_name = getResourceName(); - subscription_ = cluster_manager.subscriptionFactory().subscriptionFromConfigSource( - config_source, Grpc::Common::typeUrl(resource_name), *scope_, *this, resource_decoder_, {}); + subscription_ = + THROW_OR_RETURN_VALUE(cluster_manager.subscriptionFactory().subscriptionFromConfigSource( + config_source, Grpc::Common::typeUrl(resource_name), *scope_, *this, + resource_decoder_, {}), + Config::SubscriptionPtr); } void FilterConfigSubscription::start() { diff --git a/source/common/listener_manager/lds_api.cc b/source/common/listener_manager/lds_api.cc index b3b48a10c916..f77333ea0cce 100644 --- a/source/common/listener_manager/lds_api.cc +++ b/source/common/listener_manager/lds_api.cc @@ -32,11 +32,15 @@ LdsApiImpl::LdsApiImpl(const envoy::config::core::v3::ConfigSource& lds_config, init_target_("LDS", [this]() { subscription_->start({}); }) { const auto resource_name = getResourceName(); if (lds_resources_locator == nullptr) { - subscription_ = cm.subscriptionFactory().subscriptionFromConfigSource( - lds_config, Grpc::Common::typeUrl(resource_name), *scope_, *this, resource_decoder_, {}); + subscription_ = THROW_OR_RETURN_VALUE(cm.subscriptionFactory().subscriptionFromConfigSource( + lds_config, Grpc::Common::typeUrl(resource_name), + *scope_, *this, resource_decoder_, {}), + Config::SubscriptionPtr); } else { - subscription_ = cm.subscriptionFactory().collectionSubscriptionFromUrl( - *lds_resources_locator, lds_config, resource_name, *scope_, *this, resource_decoder_); + subscription_ = THROW_OR_RETURN_VALUE( + cm.subscriptionFactory().collectionSubscriptionFromUrl( + *lds_resources_locator, lds_config, resource_name, *scope_, *this, resource_decoder_), + Config::SubscriptionPtr); } init_manager.add(init_target_); } diff --git a/source/common/rds/rds_route_config_subscription.cc b/source/common/rds/rds_route_config_subscription.cc index 4bda9ae928c5..2f8eea06f982 100644 --- a/source/common/rds/rds_route_config_subscription.cc +++ b/source/common/rds/rds_route_config_subscription.cc @@ -31,10 +31,11 @@ RdsRouteConfigSubscription::RdsRouteConfigSubscription( manager_identifier_(manager_identifier), config_update_info_(std::move(config_update)), resource_decoder_(std::move(resource_decoder)) { const auto resource_type = route_config_provider_manager_.protoTraits().resourceType(); - subscription_ = + subscription_ = THROW_OR_RETURN_VALUE( factory_context.clusterManager().subscriptionFactory().subscriptionFromConfigSource( config_source, Envoy::Grpc::Common::typeUrl(resource_type), *scope_, *this, - resource_decoder_, {}); + resource_decoder_, {}), + Envoy::Config::SubscriptionPtr); local_init_manager_.add(local_init_target_); } diff --git a/source/common/router/scoped_rds.cc b/source/common/router/scoped_rds.cc index 1ad77bd8aecd..e459d0dc6be0 100644 --- a/source/common/router/scoped_rds.cc +++ b/source/common/router/scoped_rds.cc @@ -144,18 +144,20 @@ ScopedRdsConfigSubscription::ScopedRdsConfigSubscription( route_config_provider_manager_(route_config_provider_manager) { const auto resource_name = getResourceName(); if (scoped_rds.srds_resources_locator().empty()) { - subscription_ = + subscription_ = THROW_OR_RETURN_VALUE( factory_context.clusterManager().subscriptionFactory().subscriptionFromConfigSource( scoped_rds.scoped_rds_config_source(), Grpc::Common::typeUrl(resource_name), *scope_, - *this, resource_decoder_, {}); + *this, resource_decoder_, {}), + Envoy::Config::SubscriptionPtr); } else { const auto srds_resources_locator = THROW_OR_RETURN_VALUE( Envoy::Config::XdsResourceIdentifier::decodeUrl(scoped_rds.srds_resources_locator()), xds::core::v3::ResourceLocator); - subscription_ = + subscription_ = THROW_OR_RETURN_VALUE( factory_context.clusterManager().subscriptionFactory().collectionSubscriptionFromUrl( srds_resources_locator, scoped_rds.scoped_rds_config_source(), resource_name, *scope_, - *this, resource_decoder_); + *this, resource_decoder_), + Envoy::Config::SubscriptionPtr); } // TODO(tony612): consider not using the callback here. diff --git a/source/common/router/vhds.cc b/source/common/router/vhds.cc index 823ee0b0f86d..b182c5b1f96c 100644 --- a/source/common/router/vhds.cc +++ b/source/common/router/vhds.cc @@ -58,10 +58,11 @@ VhdsSubscription::VhdsSubscription(RouteConfigUpdatePtr& config_update_info, const auto resource_name = getResourceName(); Envoy::Config::SubscriptionOptions options; options.use_namespace_matching_ = true; - subscription_ = + subscription_ = THROW_OR_RETURN_VALUE( factory_context.clusterManager().subscriptionFactory().subscriptionFromConfigSource( config_update_info_->protobufConfigurationCast().vhds().config_source(), - Grpc::Common::typeUrl(resource_name), *scope_, *this, resource_decoder_, options); + Grpc::Common::typeUrl(resource_name), *scope_, *this, resource_decoder_, options), + Envoy::Config::SubscriptionPtr); } void VhdsSubscription::updateOnDemand(const std::string& with_route_config_name_prefix) { diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index 253ea9710471..2d625947ccee 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -605,12 +605,13 @@ absl::Status LoaderImpl::initLayers(Event::Dispatcher& dispatcher, return loadNewSnapshot(); } -void LoaderImpl::initialize(Upstream::ClusterManager& cm) { +absl::Status LoaderImpl::initialize(Upstream::ClusterManager& cm) { cm_ = &cm; for (const auto& s : subscriptions_) { - s->createSubscription(); + RETURN_IF_NOT_OK(s->createSubscription()); } + return absl::OkStatus(); } void LoaderImpl::startRtdsSubscriptions(ReadyCallback on_done) { @@ -632,11 +633,14 @@ RtdsSubscription::RtdsSubscription( stats_scope_(store_.createScope("runtime")), resource_name_(rtds_layer.name()), init_target_("RTDS " + resource_name_, [this]() { start(); }) {} -void RtdsSubscription::createSubscription() { +absl::Status RtdsSubscription::createSubscription() { const auto resource_name = getResourceName(); - subscription_ = parent_.cm_->subscriptionFactory().subscriptionFromConfigSource( + auto subscription_or_error = parent_.cm_->subscriptionFactory().subscriptionFromConfigSource( config_source_, Grpc::Common::typeUrl(resource_name), *stats_scope_, *this, resource_decoder_, {}); + RETURN_IF_NOT_OK(subscription_or_error.status()); + subscription_ = std::move(*subscription_or_error); + return absl::OkStatus(); } absl::Status diff --git a/source/common/runtime/runtime_impl.h b/source/common/runtime/runtime_impl.h index 9c960a58cbd3..cf47d6f435d9 100644 --- a/source/common/runtime/runtime_impl.h +++ b/source/common/runtime/runtime_impl.h @@ -193,7 +193,7 @@ struct RtdsSubscription : Envoy::Config::SubscriptionBase& removed_resources); - void createSubscription(); + absl::Status createSubscription(); LoaderImpl& parent_; const envoy::config::core::v3::ConfigSource config_source_; @@ -223,7 +223,7 @@ class LoaderImpl : public Loader, Logger::Loggable { Api::Api& api); // Runtime::Loader - void initialize(Upstream::ClusterManager& cm) override; + absl::Status initialize(Upstream::ClusterManager& cm) override; const Snapshot& snapshot() override; SnapshotConstSharedPtr threadsafeSnapshot() override; absl::Status mergeValues(const absl::node_hash_map& values) override; diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 1cce6941e2f9..a8ab37af3ee6 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -32,8 +32,10 @@ SdsApi::SdsApi(envoy::config::core::v3::ConfigSource sds_config, absl::string_vi time_source_.systemTime()} { const auto resource_name = getResourceName(); // This has to happen here (rather than in initialize()) as it can throw exceptions. - subscription_ = subscription_factory_.subscriptionFromConfigSource( - sds_config_, Grpc::Common::typeUrl(resource_name), *scope_, *this, resource_decoder_, {}); + subscription_ = THROW_OR_RETURN_VALUE( + subscription_factory_.subscriptionFromConfigSource( + sds_config_, Grpc::Common::typeUrl(resource_name), *scope_, *this, resource_decoder_, {}), + Config::SubscriptionPtr); } void SdsApi::resolveDataSource(const FileContentMap& files, diff --git a/source/common/upstream/cds_api_impl.cc b/source/common/upstream/cds_api_impl.cc index cfb0ee3e8b21..caa8c5de399a 100644 --- a/source/common/upstream/cds_api_impl.cc +++ b/source/common/upstream/cds_api_impl.cc @@ -25,11 +25,15 @@ CdsApiImpl::CdsApiImpl(const envoy::config::core::v3::ConfigSource& cds_config, helper_(cm, "cds"), cm_(cm), scope_(scope.createScope("cluster_manager.cds.")) { const auto resource_name = getResourceName(); if (cds_resources_locator == nullptr) { - subscription_ = cm_.subscriptionFactory().subscriptionFromConfigSource( - cds_config, Grpc::Common::typeUrl(resource_name), *scope_, *this, resource_decoder_, {}); + subscription_ = THROW_OR_RETURN_VALUE(cm_.subscriptionFactory().subscriptionFromConfigSource( + cds_config, Grpc::Common::typeUrl(resource_name), + *scope_, *this, resource_decoder_, {}), + Config::SubscriptionPtr); } else { - subscription_ = cm.subscriptionFactory().collectionSubscriptionFromUrl( - *cds_resources_locator, cds_config, resource_name, *scope_, *this, resource_decoder_); + subscription_ = THROW_OR_RETURN_VALUE( + cm.subscriptionFactory().collectionSubscriptionFromUrl( + *cds_resources_locator, cds_config, resource_name, *scope_, *this, resource_decoder_), + Config::SubscriptionPtr); } } diff --git a/source/common/upstream/od_cds_api_impl.cc b/source/common/upstream/od_cds_api_impl.cc index a9ec3c349693..f52fee9f193a 100644 --- a/source/common/upstream/od_cds_api_impl.cc +++ b/source/common/upstream/od_cds_api_impl.cc @@ -30,11 +30,15 @@ OdCdsApiImpl::OdCdsApiImpl(const envoy::config::core::v3::ConfigSource& odcds_co // class for CDS and ODCDS. const auto resource_name = getResourceName(); if (!odcds_resources_locator.has_value()) { - subscription_ = cm_.subscriptionFactory().subscriptionFromConfigSource( - odcds_config, Grpc::Common::typeUrl(resource_name), *scope_, *this, resource_decoder_, {}); + subscription_ = THROW_OR_RETURN_VALUE(cm_.subscriptionFactory().subscriptionFromConfigSource( + odcds_config, Grpc::Common::typeUrl(resource_name), + *scope_, *this, resource_decoder_, {}), + Config::SubscriptionPtr); } else { - subscription_ = cm.subscriptionFactory().collectionSubscriptionFromUrl( - *odcds_resources_locator, odcds_config, resource_name, *scope_, *this, resource_decoder_); + subscription_ = THROW_OR_RETURN_VALUE(cm.subscriptionFactory().collectionSubscriptionFromUrl( + *odcds_resources_locator, odcds_config, resource_name, + *scope_, *this, resource_decoder_), + Config::SubscriptionPtr); } } diff --git a/source/extensions/clusters/eds/eds.cc b/source/extensions/clusters/eds/eds.cc index 5ea525cd7b20..02a2b748298d 100644 --- a/source/extensions/clusters/eds/eds.cc +++ b/source/extensions/clusters/eds/eds.cc @@ -34,10 +34,11 @@ EdsClusterImpl::EdsClusterImpl(const envoy::config::cluster::v3::Cluster& cluste initialize_phase_ = InitializePhase::Secondary; } const auto resource_name = getResourceName(); - subscription_ = + subscription_ = THROW_OR_RETURN_VALUE( cluster_context.clusterManager().subscriptionFactory().subscriptionFromConfigSource( eds_config, Grpc::Common::typeUrl(resource_name), info_->statsScope(), *this, - resource_decoder_, {}); + resource_decoder_, {}), + Config::SubscriptionPtr); } EdsClusterImpl::~EdsClusterImpl() { diff --git a/source/extensions/clusters/eds/leds.cc b/source/extensions/clusters/eds/leds.cc index 5cad9d6c8b40..a3e5310b3e5b 100644 --- a/source/extensions/clusters/eds/leds.cc +++ b/source/extensions/clusters/eds/leds.cc @@ -24,10 +24,11 @@ LedsSubscription::LedsSubscription( Config::XdsResourceIdentifier::decodeUrl(leds_config.leds_collection_name()), xds::core::v3::ResourceLocator); const auto resource_name = getResourceName(); - subscription_ = + subscription_ = THROW_OR_RETURN_VALUE( factory_context.clusterManager().subscriptionFactory().collectionSubscriptionFromUrl( leds_resource_locator, leds_config.leds_config(), resource_name, *stats_scope_, *this, - resource_decoder_); + resource_decoder_), + Config::SubscriptionPtr); subscription_->start({}); } diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index 7aeb4e1d41a1..344bc8b5cd5a 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -144,7 +144,7 @@ void ValidationInstance::initialize(const Options& options, [this]() -> Network::DnsResolverSharedPtr { return this->dnsResolver(); }, sslContextManager(), *secret_manager_, quic_stat_names_, *this); THROW_IF_NOT_OK(config_.initialize(bootstrap_, *this, *cluster_manager_factory_)); - runtime().initialize(clusterManager()); + THROW_IF_NOT_OK(runtime().initialize(clusterManager())); clusterManager().setInitializedCb([this]() -> void { init_manager_.initialize(init_watcher_); }); } diff --git a/source/server/server.cc b/source/server/server.cc index ee3ab436b23e..0c02614b833e 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -768,7 +768,7 @@ absl::Status InstanceBase::initializeOrThrow(Network::Address::InstanceConstShar // We have to defer RTDS initialization until after the cluster manager is // instantiated (which in turn relies on runtime...). - runtime().initialize(clusterManager()); + RETURN_IF_NOT_OK(runtime().initialize(clusterManager())); clusterManager().setPrimaryClustersInitializedCb( [this]() { onClusterManagerPrimaryInitializationComplete(); }); diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc index fa6415809cde..9d90992b54ab 100644 --- a/test/common/runtime/runtime_impl_test.cc +++ b/test/common/runtime/runtime_impl_test.cc @@ -962,7 +962,7 @@ class RtdsLoaderImplTest : public LoaderImplTest { dispatcher_, tls_, config, local_info_, store_, generator_, validation_visitor_, *api_); THROW_IF_NOT_OK(loader.status()); loader_ = std::move(loader.value()); - loader_->initialize(cm_); + THROW_IF_NOT_OK(loader_->initialize(cm_)); for (auto* sub : rtds_subscriptions_) { EXPECT_CALL(*sub, start(_)); } @@ -1298,7 +1298,8 @@ TEST_F(RtdsLoaderImplTest, BadConfigSource) { absl::StatusOr> loader = Runtime::LoaderImpl::create( dispatcher_, tls_, config, local_info_, store_, generator_, validation_visitor_, *api_); - EXPECT_THROW_WITH_MESSAGE(loader.value()->initialize(cm_), EnvoyException, "bad config"); + EXPECT_THROW_WITH_MESSAGE(loader.value()->initialize(cm_).IgnoreError(), EnvoyException, + "bad config"); } } // namespace diff --git a/test/common/secret/sds_api_test.cc b/test/common/secret/sds_api_test.cc index 4e1fab3c1042..1f73cacb7e39 100644 --- a/test/common/secret/sds_api_test.cc +++ b/test/common/secret/sds_api_test.cc @@ -112,7 +112,7 @@ TEST_F(SdsApiTest, InitManagerInitialised) { &stats](const envoy::config::core::v3::ConfigSource&, absl::string_view, Stats::Scope&, Config::SubscriptionCallbacks& cbs, Config::OpaqueResourceDecoderSharedPtr, - const Config::SubscriptionOptions&) -> Config::SubscriptionPtr { + const Config::SubscriptionOptions&) { return std::make_unique( *dispatcher_, Config::makePathConfigSource(sds_config_path), cbs, resource_decoder, stats, validation_visitor_, *api_); @@ -138,7 +138,7 @@ TEST_F(SdsApiTest, BadConfigSource) { ::testing::InSequence s; envoy::config::core::v3::ConfigSource config_source; EXPECT_CALL(subscription_factory_, subscriptionFromConfigSource(_, _, _, _, _, _)) - .WillOnce(InvokeWithoutArgs([]() -> Config::SubscriptionPtr { + .WillOnce(InvokeWithoutArgs([]() { throw EnvoyException("bad config"); return nullptr; })); diff --git a/test/extensions/config_subscription/common/subscription_factory_impl_test.cc b/test/extensions/config_subscription/common/subscription_factory_impl_test.cc index 04ff5951273b..ed4470656532 100644 --- a/test/extensions/config_subscription/common/subscription_factory_impl_test.cc +++ b/test/extensions/config_subscription/common/subscription_factory_impl_test.cc @@ -53,18 +53,21 @@ class SubscriptionFactoryTest : public testing::Test { SubscriptionPtr subscriptionFromConfigSource(const envoy::config::core::v3::ConfigSource& config) { - return subscription_factory_.subscriptionFromConfigSource( - config, Config::TypeUrl::get().ClusterLoadAssignment, *stats_store_.rootScope(), callbacks_, - resource_decoder_, {}); + return THROW_OR_RETURN_VALUE(subscription_factory_.subscriptionFromConfigSource( + config, Config::TypeUrl::get().ClusterLoadAssignment, + *stats_store_.rootScope(), callbacks_, resource_decoder_, {}), + SubscriptionPtr); } SubscriptionPtr collectionSubscriptionFromUrl(const std::string& xds_url, const envoy::config::core::v3::ConfigSource& config) { const auto resource_locator = XdsResourceIdentifier::decodeUrl(xds_url).value(); - return subscription_factory_.collectionSubscriptionFromUrl( - resource_locator, config, "envoy.config.endpoint.v3.ClusterLoadAssignment", - *stats_store_.rootScope(), callbacks_, resource_decoder_); + return THROW_OR_RETURN_VALUE(subscription_factory_.collectionSubscriptionFromUrl( + resource_locator, config, + "envoy.config.endpoint.v3.ClusterLoadAssignment", + *stats_store_.rootScope(), callbacks_, resource_decoder_), + SubscriptionPtr); } Upstream::MockClusterManager cm_; @@ -114,7 +117,7 @@ TEST_F(SubscriptionFactoryTest, RestClusterEmpty) { config.mutable_api_config_source()->set_api_type(envoy::config::core::v3::ApiConfigSource::REST); - EXPECT_CALL(cm_, primaryClusters()).WillOnce(ReturnRef(primary_clusters)); + EXPECT_CALL(cm_, primaryClusters()).WillRepeatedly(ReturnRef(primary_clusters)); EXPECT_THROW_WITH_REGEX(subscriptionFromConfigSource(config), EnvoyException, "API configs must have either a gRPC service or a cluster name defined:"); } @@ -125,7 +128,7 @@ TEST_P(SubscriptionFactoryTestUnifiedOrLegacyMux, GrpcClusterEmpty) { config.mutable_api_config_source()->set_api_type(envoy::config::core::v3::ApiConfigSource::GRPC); - EXPECT_CALL(cm_, primaryClusters()).WillOnce(ReturnRef(primary_clusters)); + EXPECT_CALL(cm_, primaryClusters()).WillRepeatedly(ReturnRef(primary_clusters)); EXPECT_THROW_WITH_REGEX(subscriptionFromConfigSource(config), EnvoyException, "API configs must have either a gRPC service or a cluster name defined:"); } @@ -187,7 +190,7 @@ TEST_F(SubscriptionFactoryTest, RestClusterMultiton) { config.mutable_api_config_source()->add_cluster_names("static_cluster_bar"); primary_clusters.insert("static_cluster_bar"); - EXPECT_CALL(cm_, primaryClusters()).WillOnce(ReturnRef(primary_clusters)); + EXPECT_CALL(cm_, primaryClusters()).WillRepeatedly(ReturnRef(primary_clusters)); EXPECT_THROW_WITH_REGEX(subscriptionFromConfigSource(config), EnvoyException, fmt::format("{} must have a singleton cluster name specified:", config.mutable_api_config_source()->GetTypeName())); @@ -207,7 +210,7 @@ TEST_P(SubscriptionFactoryTestUnifiedOrLegacyMux, GrpcClusterMultiton) { primary_clusters.insert("static_cluster_bar"); EXPECT_CALL(cm_, grpcAsyncClientManager()).WillRepeatedly(ReturnRef(cm_.async_client_manager_)); - EXPECT_CALL(cm_, primaryClusters()).WillOnce(ReturnRef(primary_clusters)); + EXPECT_CALL(cm_, primaryClusters()).WillRepeatedly(ReturnRef(primary_clusters)); EXPECT_THROW_WITH_REGEX(subscriptionFromConfigSource(config), EnvoyException, fmt::format("{}::.DELTA_.GRPC must have a " @@ -486,10 +489,13 @@ TEST_F(SubscriptionFactoryTest, LogWarningOnDeprecatedV2Transport) { primary_clusters.insert("static_cluster"); EXPECT_CALL(cm_, primaryClusters()).WillOnce(ReturnRef(primary_clusters)); - EXPECT_THROW_WITH_REGEX(subscription_factory_.subscriptionFromConfigSource( - config, Config::TypeUrl::get().ClusterLoadAssignment, - *stats_store_.rootScope(), callbacks_, resource_decoder_, {}), - EnvoyException, "V2 xDS transport protocol version is deprecated in"); + EXPECT_THAT(subscription_factory_ + .subscriptionFromConfigSource( + config, Config::TypeUrl::get().ClusterLoadAssignment, + *stats_store_.rootScope(), callbacks_, resource_decoder_, {}) + .status() + .message(), + testing::HasSubstr("V2 xDS transport protocol version is deprecated in")); } // Use of AUTO transport fails by default. This will encourage folks to upgrade to explicit V3. @@ -506,6 +512,7 @@ TEST_F(SubscriptionFactoryTest, AutoTransportIsAllowed) { Upstream::ClusterManager::ClusterSet primary_clusters; primary_clusters.insert("static_cluster"); EXPECT_CALL(cm_, primaryClusters()).WillOnce(ReturnRef(primary_clusters)); + EXPECT_CALL(cm_, grpcAsyncClientManager()).WillOnce(ReturnRef(cm_.async_client_manager_)); EXPECT_CALL(cm_.async_client_manager_, factoryForGrpcService(ProtoEq(expected_grpc_service), _, _)) @@ -539,7 +546,7 @@ TEST_P(SubscriptionFactoryTestApiConfigSource, NonExistentCluster) { api_config_source->add_cluster_names("static_cluster"); } Upstream::ClusterManager::ClusterSet primary_clusters; - EXPECT_CALL(cm_, primaryClusters()).WillOnce(ReturnRef(primary_clusters)); + EXPECT_CALL(cm_, primaryClusters()).WillRepeatedly(ReturnRef(primary_clusters)); EXPECT_THROW_WITH_MESSAGE(subscriptionFromConfigSource(config)->start({"static_cluster"}), EnvoyException, fmt::format("{} must have a statically defined " diff --git a/test/mocks/config/mocks.h b/test/mocks/config/mocks.h index ef9897f9cb2b..556ce0f7c0ff 100644 --- a/test/mocks/config/mocks.h +++ b/test/mocks/config/mocks.h @@ -79,12 +79,12 @@ class MockSubscriptionFactory : public SubscriptionFactory { MockSubscriptionFactory(); ~MockSubscriptionFactory() override; - MOCK_METHOD(SubscriptionPtr, subscriptionFromConfigSource, + MOCK_METHOD(absl::StatusOr, subscriptionFromConfigSource, (const envoy::config::core::v3::ConfigSource& config, absl::string_view type_url, Stats::Scope& scope, SubscriptionCallbacks& callbacks, OpaqueResourceDecoderSharedPtr resource_decoder, const SubscriptionOptions& options)); - MOCK_METHOD(SubscriptionPtr, collectionSubscriptionFromUrl, + MOCK_METHOD(absl::StatusOr, collectionSubscriptionFromUrl, (const xds::core::v3::ResourceLocator& collection_locator, const envoy::config::core::v3::ConfigSource& config, absl::string_view type_url, Stats::Scope& scope, SubscriptionCallbacks& callbacks, diff --git a/test/mocks/runtime/mocks.h b/test/mocks/runtime/mocks.h index 0329bf8154ca..9bb7e6c09c89 100644 --- a/test/mocks/runtime/mocks.h +++ b/test/mocks/runtime/mocks.h @@ -61,7 +61,7 @@ class MockLoader : public Loader { MockLoader(); ~MockLoader() override; - MOCK_METHOD(void, initialize, (Upstream::ClusterManager & cm)); + MOCK_METHOD(absl::Status, initialize, (Upstream::ClusterManager & cm)); MOCK_METHOD(const Snapshot&, snapshot, ()); MOCK_METHOD(SnapshotConstSharedPtr, threadsafeSnapshot, ()); MOCK_METHOD(absl::Status, mergeValues, ((const absl::node_hash_map&))); diff --git a/tools/code_format/config.yaml b/tools/code_format/config.yaml index ffa3c7582a2a..fa8766048cad 100644 --- a/tools/code_format/config.yaml +++ b/tools/code_format/config.yaml @@ -117,7 +117,11 @@ paths: - source/common/quic/quic_server_transport_socket_factory.cc - source/common/grpc/google_grpc_utils.cc - source/common/tcp_proxy/tcp_proxy.cc - - source/common/config/subscription_factory_impl.cc + - source/common/listener_manager/lds_api.cc + - source/common/upstream/od_cds_api_impl.cc + - source/common/upstream/cds_api_impl.cc + - source/common/router/vhds.cc + - source/common/rds/rds_route_config_subscription.cc - source/common/filter/config_discovery_impl.cc - source/common/json/json_internal.cc - source/common/router/scoped_rds.cc