From fe3aaf5c0b0be37c8eb8a5b4a89528a234da48c8 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 22 Jan 2024 15:09:51 -0500 Subject: [PATCH] vhds: removing an exception Signed-off-by: Alyssa Wilk --- .../rds/rds_route_config_subscription.cc | 2 +- .../rds/rds_route_config_subscription.h | 6 +++-- source/common/router/rds_impl.cc | 9 ++++--- source/common/router/rds_impl.h | 4 +-- source/common/router/vhds.cc | 25 +++++++++++++------ source/common/router/vhds.h | 12 ++++++--- test/common/router/vhds_test.cc | 19 ++++++++++---- tools/code_format/config.yaml | 1 - 8 files changed, 53 insertions(+), 25 deletions(-) diff --git a/source/common/rds/rds_route_config_subscription.cc b/source/common/rds/rds_route_config_subscription.cc index 44d2e15d59a6..c5280d501727 100644 --- a/source/common/rds/rds_route_config_subscription.cc +++ b/source/common/rds/rds_route_config_subscription.cc @@ -85,7 +85,7 @@ absl::Status RdsRouteConfigSubscription::onConfigUpdate( stats_.config_reload_.inc(); stats_.config_reload_time_ms_.set(DateUtil::nowToMilliseconds(factory_context_.timeSource())); - beforeProviderUpdate(noop_init_manager, resume_rds); + THROW_IF_NOT_OK(beforeProviderUpdate(noop_init_manager, resume_rds)); ENVOY_LOG(debug, "rds: loading new configuration: config_name={} hash={}", route_config_name_, config_update_info_->configHash()); diff --git a/source/common/rds/rds_route_config_subscription.h b/source/common/rds/rds_route_config_subscription.h index 205375cef14f..525a22b3825b 100644 --- a/source/common/rds/rds_route_config_subscription.h +++ b/source/common/rds/rds_route_config_subscription.h @@ -72,8 +72,10 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks, void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, const EnvoyException*) override; - virtual void beforeProviderUpdate(std::unique_ptr&, - std::unique_ptr&) {} + virtual absl::Status beforeProviderUpdate(std::unique_ptr&, + std::unique_ptr&) { + return absl::OkStatus(); + } virtual void afterProviderUpdate() {} protected: diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index 18475ae48d06..98abfb3d046a 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -82,7 +82,7 @@ RdsRouteConfigSubscription::RdsRouteConfigSubscription( RdsRouteConfigSubscription::~RdsRouteConfigSubscription() { config_update_info_.release(); } -void RdsRouteConfigSubscription::beforeProviderUpdate( +absl::Status RdsRouteConfigSubscription::beforeProviderUpdate( std::unique_ptr& noop_init_manager, std::unique_ptr& resume_rds) { if (config_update_info_->protobufConfigurationCast().has_vhds() && config_update_info_->vhdsConfigurationChanged()) { @@ -92,11 +92,14 @@ void RdsRouteConfigSubscription::beforeProviderUpdate( ASSERT(config_update_info_->configInfo().has_value()); maybeCreateInitManager(routeConfigUpdate()->configInfo().value().version_, noop_init_manager, resume_rds); - vhds_subscription_ = std::make_unique(config_update_info_, factory_context_, - stat_prefix_, route_config_provider_); + auto subscription_or_error = VhdsSubscription::createVhdsSubscription( + config_update_info_, factory_context_, stat_prefix_, route_config_provider_); + RETURN_IF_STATUS_NOT_OK(subscription_or_error); + vhds_subscription_ = std::move(subscription_or_error.value()); vhds_subscription_->registerInitTargetWithInitManager( noop_init_manager == nullptr ? local_init_manager_ : *noop_init_manager); } + return absl::OkStatus(); } void RdsRouteConfigSubscription::afterProviderUpdate() { diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index 5d5eb826d5c7..fcd287d5451d 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -113,8 +113,8 @@ class RdsRouteConfigSubscription : public Rds::RdsRouteConfigSubscription { std::unique_ptr& resume_rds); private: - void beforeProviderUpdate(std::unique_ptr& noop_init_manager, - std::unique_ptr& resume_rds) override; + absl::Status beforeProviderUpdate(std::unique_ptr& noop_init_manager, + std::unique_ptr& resume_rds) override; void afterProviderUpdate() override; ABSL_MUST_USE_RESULT Common::CallbackHandlePtr addUpdateCallback(std::function callback) { diff --git a/source/common/router/vhds.cc b/source/common/router/vhds.cc index 7f0139907337..06d92b24ca75 100644 --- a/source/common/router/vhds.cc +++ b/source/common/router/vhds.cc @@ -19,6 +19,23 @@ namespace Envoy { namespace Router { +absl::StatusOr VhdsSubscription::createVhdsSubscription( + RouteConfigUpdatePtr& config_update_info, + Server::Configuration::ServerFactoryContext& factory_context, const std::string& stat_prefix, + Rds::RouteConfigProvider* route_config_provider) { + const auto& config_source = config_update_info->protobufConfigurationCast() + .vhds() + .config_source() + .api_config_source() + .api_type(); + if (config_source != envoy::config::core::v3::ApiConfigSource::DELTA_GRPC) { + return absl::InvalidArgumentError("vhds: only 'DELTA_GRPC' is supported as an api_type."); + } + + return std::unique_ptr(new VhdsSubscription( + config_update_info, factory_context, stat_prefix, route_config_provider)); +} + // Implements callbacks to handle DeltaDiscovery protocol for VirtualHostDiscoveryService VhdsSubscription::VhdsSubscription(RouteConfigUpdatePtr& config_update_info, Server::Configuration::ServerFactoryContext& factory_context, @@ -37,14 +54,6 @@ VhdsSubscription::VhdsSubscription(RouteConfigUpdatePtr& config_update_info, {config_update_info_->protobufConfigurationCast().name()}); }), route_config_provider_(route_config_provider) { - const auto& config_source = config_update_info_->protobufConfigurationCast() - .vhds() - .config_source() - .api_config_source() - .api_type(); - if (config_source != envoy::config::core::v3::ApiConfigSource::DELTA_GRPC) { - throwEnvoyExceptionOrPanic("vhds: only 'DELTA_GRPC' is supported as an api_type."); - } const auto resource_name = getResourceName(); Envoy::Config::SubscriptionOptions options; options.use_namespace_matching_ = true; diff --git a/source/common/router/vhds.h b/source/common/router/vhds.h index 6e3f397b76a8..8b20961c72bc 100644 --- a/source/common/router/vhds.h +++ b/source/common/router/vhds.h @@ -39,9 +39,11 @@ struct VhdsStats { class VhdsSubscription : Envoy::Config::SubscriptionBase, Logger::Loggable { public: - VhdsSubscription(RouteConfigUpdatePtr& config_update_info, - Server::Configuration::ServerFactoryContext& factory_context, - const std::string& stat_prefix, Rds::RouteConfigProvider* route_config_provider); + static absl::StatusOr> + createVhdsSubscription(RouteConfigUpdatePtr& config_update_info, + Server::Configuration::ServerFactoryContext& factory_context, + const std::string& stat_prefix, + Rds::RouteConfigProvider* route_config_provider); ~VhdsSubscription() override { init_target_.ready(); } @@ -57,6 +59,10 @@ class VhdsSubscription : Envoy::Config::SubscriptionBase&, const std::string&) override { diff --git a/test/common/router/vhds_test.cc b/test/common/router/vhds_test.cc index 4b2f65be16c5..7330f59f3134 100644 --- a/test/common/router/vhds_test.cc +++ b/test/common/router/vhds_test.cc @@ -100,7 +100,10 @@ TEST_F(VhdsTest, VhdsInstantiationShouldSucceedWithDELTA_GRPC) { TestUtility::parseYaml(default_vhds_config_); RouteConfigUpdatePtr config_update_info = makeRouteConfigUpdate(route_config); - EXPECT_NO_THROW(VhdsSubscription(config_update_info, factory_context_, context_, provider_)); + EXPECT_TRUE(VhdsSubscription::createVhdsSubscription(config_update_info, factory_context_, + context_, provider_) + .status() + .ok()); } // verify that api_type: GRPC fails validation @@ -118,8 +121,10 @@ name: my_route )EOF"); RouteConfigUpdatePtr config_update_info = makeRouteConfigUpdate(route_config); - EXPECT_THROW(VhdsSubscription(config_update_info, factory_context_, context_, provider_), - EnvoyException); + EXPECT_FALSE(VhdsSubscription::createVhdsSubscription(config_update_info, factory_context_, + context_, provider_) + .status() + .ok()); } // verify addition/updating of virtual hosts @@ -128,7 +133,9 @@ TEST_F(VhdsTest, VhdsAddsVirtualHosts) { TestUtility::parseYaml(default_vhds_config_); RouteConfigUpdatePtr config_update_info = makeRouteConfigUpdate(route_config); - VhdsSubscription subscription(config_update_info, factory_context_, context_, provider_); + VhdsSubscriptionPtr subscription = VhdsSubscription::createVhdsSubscription( + config_update_info, factory_context_, context_, provider_) + .value(); EXPECT_EQ(0UL, config_update_info->protobufConfigurationCast().virtual_hosts_size()); auto vhost = buildVirtualHost("vhost1", "vhost.first"); @@ -188,7 +195,9 @@ name: my_route )EOF"); RouteConfigUpdatePtr config_update_info = makeRouteConfigUpdate(route_config); - VhdsSubscription subscription(config_update_info, factory_context_, context_, provider_); + VhdsSubscriptionPtr subscription = VhdsSubscription::createVhdsSubscription( + config_update_info, factory_context_, context_, provider_) + .value(); EXPECT_EQ(1UL, config_update_info->protobufConfigurationCast().virtual_hosts_size()); EXPECT_EQ("vhost_rds1", config_update_info->protobufConfigurationCast().virtual_hosts(0).name()); diff --git a/tools/code_format/config.yaml b/tools/code_format/config.yaml index 71bd0803514c..39a3a307b839 100644 --- a/tools/code_format/config.yaml +++ b/tools/code_format/config.yaml @@ -143,7 +143,6 @@ paths: - source/common/router/config_impl.cc - source/common/router/scoped_config_impl.cc - source/common/router/router_ratelimit.cc - - source/common/router/vhds.cc - source/common/router/header_parser.cc - source/common/filesystem/inotify/watcher_impl.cc - source/common/filesystem/posix/directory_iterator_impl.cc