[server] Fix uncaught exceptions in callbacks that initialize seconda…

…ry clusters (envoyproxy#14963) Fixes uncaught exceptions in callbacks that can occur after server initialization. Signed-off-by: Asra Ali <asraa@google.com>
soulxu · Feb 9, 2021 · 7e2099a · 7e2099a
1 parent 9893386
commit 7e2099a
Show file tree

Hide file tree

Showing 6 changed files with 378 additions and 13 deletions.
diff --git a/source/server/server.cc b/source/server/server.cc
@@ -601,21 +601,33 @@ void InstanceImpl::onClusterManagerPrimaryInitializationComplete() {
 
 void InstanceImpl::onRuntimeReady() {
   // Begin initializing secondary clusters after RTDS configuration has been applied.
-  clusterManager().initializeSecondaryClusters(bootstrap_);
+  // Initializing can throw exceptions, so catch these.
+  try {
+    clusterManager().initializeSecondaryClusters(bootstrap_);
+  } catch (const EnvoyException& e) {
+    ENVOY_LOG(warn, "Skipping initialization of secondary cluster: {}", e.what());
+    shutdown();
+  }
 
   if (bootstrap_.has_hds_config()) {
     const auto& hds_config = bootstrap_.hds_config();
     async_client_manager_ = std::make_unique<Grpc::AsyncClientManagerImpl>(
         *config_.clusterManager(), thread_local_, time_source_, *api_, grpc_context_.statNames());
-    hds_delegate_ = std::make_unique<Upstream::HdsDelegate>(
-        stats_store_,
-        Config::Utility::factoryForGrpcApiConfigSource(*async_client_manager_, hds_config,
-                                                       stats_store_, false)
-            ->create(),
-        Config::Utility::getAndCheckTransportVersion(hds_config), *dispatcher_,
-        Runtime::LoaderSingleton::get(), stats_store_, *ssl_context_manager_, info_factory_,
-        access_log_manager_, *config_.clusterManager(), *local_info_, *admin_, *singleton_manager_,
-        thread_local_, messageValidationContext().dynamicValidationVisitor(), *api_);
+    try {
+      hds_delegate_ = std::make_unique<Upstream::HdsDelegate>(
+          stats_store_,
+          Config::Utility::factoryForGrpcApiConfigSource(*async_client_manager_, hds_config,
+                                                         stats_store_, false)
+              ->create(),
+          Config::Utility::getAndCheckTransportVersion(hds_config), *dispatcher_,
+          Runtime::LoaderSingleton::get(), stats_store_, *ssl_context_manager_, info_factory_,
+          access_log_manager_, *config_.clusterManager(), *local_info_, *admin_,
+          *singleton_manager_, thread_local_, messageValidationContext().dynamicValidationVisitor(),
+          *api_);
+    } catch (const EnvoyException& e) {
+      ENVOY_LOG(warn, "Skipping initialization of HDS cluster: {}", e.what());
+      shutdown();
+    }
   }
 
   // If there is no global limit to the number of active connections, warn on startup.

diff --git a/test/server/server_corpus/clusterfuzz-testcase-minimized-5146513187667968 b/test/server/server_corpus/clusterfuzz-testcase-minimized-5146513187667968
diff --git a/test/server/server_corpus/clusterfuzz-testcase-minimized-6612746286268416 b/test/server/server_corpus/clusterfuzz-testcase-minimized-6612746286268416
diff --git a/test/server/server_test.cc b/test/server/server_test.cc
@@ -391,6 +391,31 @@ TEST_P(ServerInstanceImplTest, EmptyShutdownLifecycleNotifications) {
   EXPECT_EQ(0L, TestUtility::findGauge(stats_store_, "server.state")->value());
 }
 
+// Catch exceptions in secondary cluster initialization callbacks. These are not caught in the main
+// initialization try/catch.
+TEST_P(ServerInstanceImplTest, SecondaryClusterExceptions) {
+  // Error in reading illegal file path for channel credentials in secondary cluster.
+  EXPECT_LOG_CONTAINS("warn",
+                      "Skipping initialization of secondary cluster: API configs must have either "
+                      "a gRPC service or a cluster name defined",
+                      {
+                        initialize("test/server/test_data/server/health_check_nullptr.yaml");
+                        server_->dispatcher().run(Event::Dispatcher::RunType::NonBlock);
+                      });
+}
+
+// Catch exceptions in HDS cluster initialization callbacks.
+TEST_P(ServerInstanceImplTest, HdsClusterException) {
+  // Error in reading illegal file path for channel credentials in secondary cluster.
+  EXPECT_LOG_CONTAINS("warn",
+                      "Skipping initialization of HDS cluster: API configs must have either a gRPC "
+                      "service or a cluster name defined",
+                      {
+                        initialize("test/server/test_data/server/hds_exception.yaml");
+                        server_->dispatcher().run(Event::Dispatcher::RunType::NonBlock);
+                      });
+}
+
 TEST_P(ServerInstanceImplTest, LifecycleNotifications) {
   bool startup = false, post_init = false, shutdown = false, shutdown_with_completion = false;
   absl::Notification started, post_init_fired, shutdown_begin, completion_block, completion_done;
@@ -812,9 +837,12 @@ TEST_P(ServerInstanceImplTest,
 // set.
 TEST_P(ServerInstanceImplTest,
        DEPRECATED_FEATURE_TEST(FailToLoadV2HdsTransportWithoutExplicitVersion)) {
-  EXPECT_THROW_WITH_REGEX(initialize("test/server/test_data/server/hds_v2.yaml"),
-                          DeprecatedMajorVersionException,
-                          "V2 .and AUTO. xDS transport protocol versions are deprecated in.*");
+  // HDS cluster initialization happens through callbacks after runtime initialization. Exceptions
+  // are caught and will result in server shutdown.
+  EXPECT_LOG_CONTAINS("warn",
+                      "Skipping initialization of HDS cluster: V2 (and AUTO) xDS transport "
+                      "protocol versions are deprecated",
+                      initialize("test/server/test_data/server/hds_v2.yaml"));
 }
 
 // Validate that bootstrap v2 is rejected when --bootstrap-version is not set.

diff --git a/test/server/test_data/server/hds_exception.yaml b/test/server/test_data/server/hds_exception.yaml
@@ -0,0 +1,27 @@
+staticResources:
+  clusters:
+    - name: ser
+      connectTimeout: 0.000000813s
+      healthChecks:
+        - timeout: 0.926363914s
+          interval: 165537s
+          unhealthyThreshold: 16
+          healthyThreshold: 0
+          httpHealthCheck:
+            host: h
+            path: '&'
+      loadAssignment:
+        clusterName: .
+        endpoints:
+          - lbEndpoints:
+              - endpoint:
+                  address:
+                    pipe:
+                      path: .
+                healthStatus: DRAINING
+hdsConfig:
+  apiType: GRPC
+  requestTimeout: 0.014024704s
+  transportApiVersion: V3
+
+enableDispatcherStats: true