vhds: removing an exception

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alyssawilk · Jan 22, 2024 · fe3aaf5 · fe3aaf5
1 parent 644c718
commit fe3aaf5
Show file tree

Hide file tree

Showing 8 changed files with 53 additions and 25 deletions.
diff --git 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
@@ -72,8 +72,10 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks,
   void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason,
                             const EnvoyException*) override;
 
-  virtual void beforeProviderUpdate(std::unique_ptr<Init::ManagerImpl>&,
-                                    std::unique_ptr<Cleanup>&) {}
+  virtual absl::Status beforeProviderUpdate(std::unique_ptr<Init::ManagerImpl>&,
+                                            std::unique_ptr<Cleanup>&) {
+    return absl::OkStatus();
+  }
   virtual void afterProviderUpdate() {}
 
 protected:

diff --git 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<Init::ManagerImpl>& noop_init_manager, std::unique_ptr<Cleanup>& 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<VhdsSubscription>(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
@@ -113,8 +113,8 @@ class RdsRouteConfigSubscription : public Rds::RdsRouteConfigSubscription {
                               std::unique_ptr<Cleanup>& resume_rds);
 
 private:
-  void beforeProviderUpdate(std::unique_ptr<Init::ManagerImpl>& noop_init_manager,
-                            std::unique_ptr<Cleanup>& resume_rds) override;
+  absl::Status beforeProviderUpdate(std::unique_ptr<Init::ManagerImpl>& noop_init_manager,
+                                    std::unique_ptr<Cleanup>& resume_rds) override;
   void afterProviderUpdate() override;
 
   ABSL_MUST_USE_RESULT Common::CallbackHandlePtr addUpdateCallback(std::function<void()> callback) {

diff --git a/source/common/router/vhds.cc b/source/common/router/vhds.cc
@@ -19,6 +19,23 @@
 namespace Envoy {
 namespace Router {
 
+absl::StatusOr<VhdsSubscriptionPtr> 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<VhdsSubscription>(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
@@ -39,9 +39,11 @@ struct VhdsStats {
 class VhdsSubscription : Envoy::Config::SubscriptionBase<envoy::config::route::v3::VirtualHost>,
                          Logger::Loggable<Logger::Id::router> {
 public:
-  VhdsSubscription(RouteConfigUpdatePtr& config_update_info,
-                   Server::Configuration::ServerFactoryContext& factory_context,
-                   const std::string& stat_prefix, Rds::RouteConfigProvider* route_config_provider);
+  static absl::StatusOr<std::unique_ptr<VhdsSubscription>>
+  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<envoy::config::route::v
   }
 
 private:
+  VhdsSubscription(RouteConfigUpdatePtr& config_update_info,
+                   Server::Configuration::ServerFactoryContext& factory_context,
+                   const std::string& stat_prefix, Rds::RouteConfigProvider* route_config_provider);
+
   // Config::SubscriptionCallbacks
   absl::Status onConfigUpdate(const std::vector<Envoy::Config::DecodedResourceRef>&,
                               const std::string&) override {

diff --git 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<envoy::config::route::v3::RouteConfiguration>(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<envoy::config::route::v3::RouteConfiguration>(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
@@ -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