Refactor cluster manager init sequence. (envoyproxy#33221)

This is to address the issue: envoyproxy#33218

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

envoy/upstream/cluster_manager.h

@@ -214,6 +214,21 @@ class ClusterManager {
 
   virtual ~ClusterManager() = default;
 
+  // API to initialize the ClusterManagerImpl instance based on the given Bootstrap config.
+  //
+  // This method *must* be called prior to invoking any other methods on the class and *must* only
+  // be called once. This method should be called immediately after ClusterManagerImpl construction
+  // and from the same thread in which the ClusterManagerImpl was constructed.
+  //
+  // The initialization is separated from the constructor because lots of work, including ADS
+  // initialization, is done in this method. If the contents of this method are invoked during
+  // construction, a derived class cannot override any of the virtual methods and have them invoked
+  // instead, since the base class's methods are used when in a base class constructor.
+  virtual absl::Status initialize(const envoy::config::bootstrap::v3::Bootstrap& bootstrap) PURE;
+
+  // API to return whether the ClusterManagerImpl instance is initialized.
+  virtual bool initialized() PURE;
+
   /**
    * Add or update a cluster via API. The semantics of this API are:
    * 1) The hash of the config is used to determine if an already existing cluster has changed.
@@ -491,6 +506,8 @@ class ClusterManagerFactory {
 
   /**
    * Allocate a cluster manager from configuration proto.
+   * The cluster manager init() method needs to be called right after this method.
+   * Please check https://github.com/envoyproxy/envoy/issues/33218 for details.
    */
   virtual ClusterManagerPtr
   clusterManagerFromProto(const envoy::config::bootstrap::v3::Bootstrap& bootstrap) PURE;

source/common/upstream/cluster_manager_impl.cc

@@ -370,7 +370,8 @@ ClusterManagerImpl::ClusterManagerImpl(
       makeOptRefFromPtr(xds_config_tracker_.get()));
 }
 
-absl::Status ClusterManagerImpl::init(const envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
+absl::Status
+ClusterManagerImpl::initialize(const envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
   ASSERT(!initialized_);
   initialized_ = true;
 
@@ -2224,7 +2225,6 @@ ClusterManagerPtr ProdClusterManagerFactory::clusterManagerFromProto(
       context_.accessLogManager(), context_.mainThreadDispatcher(), context_.admin(),
       context_.messageValidationContext(), context_.api(), http_context_, context_.grpcContext(),
       context_.routerContext(), server_)};
-  THROW_IF_NOT_OK(cluster_manager_impl->init(bootstrap));
   return cluster_manager_impl;
 }
 

source/common/upstream/cluster_manager_impl.h

@@ -246,17 +246,9 @@ class ClusterManagerImpl : public ClusterManager,
                            public MissingClusterNotifier,
                            Logger::Loggable<Logger::Id::upstream> {
 public:
-  // Initializes the ClusterManagerImpl instance based on the given Bootstrap config.
-  //
-  // This method *must* be called prior to invoking any other methods on the class and *must* only
-  // be called once. This method should be called immediately after ClusterManagerImpl construction
-  // and from the same thread in which the ClusterManagerImpl was constructed.
-  //
-  // The initialization is separated from the constructor because lots of work, including ADS
-  // initialization, is done in this method. If the contents of this method are invoked during
-  // construction, a derived class cannot override any of the virtual methods and have them invoked
-  // instead, since the base class's methods are used when in a base class constructor.
-  absl::Status init(const envoy::config::bootstrap::v3::Bootstrap& bootstrap);
+  absl::Status initialize(const envoy::config::bootstrap::v3::Bootstrap& bootstrap) override;
+
+  bool initialized() override { return initialized_; }
 
   std::size_t warmingClusterCount() const { return warming_clusters_.size(); }
 

source/server/config_validation/cluster_manager.cc

@@ -15,7 +15,6 @@ ClusterManagerPtr ValidationClusterManagerFactory::clusterManagerFromProto(
       context_.accessLogManager(), context_.mainThreadDispatcher(), context_.admin(),
       context_.messageValidationContext(), context_.api(), http_context_, context_.grpcContext(),
       context_.routerContext(), server_)};
-  THROW_IF_NOT_OK(cluster_manager->init(bootstrap));
   return cluster_manager;
 }
 

source/server/configuration_impl.cc

@@ -128,7 +128,11 @@ absl::Status MainImpl::initialize(const envoy::config::bootstrap::v3::Bootstrap&
   }
 
   ENVOY_LOG(info, "loading {} cluster(s)", bootstrap.static_resources().clusters().size());
+
+  // clusterManagerFromProto() and init() have to be called consecutively.
   cluster_manager_ = cluster_manager_factory.clusterManagerFromProto(bootstrap);
+  status = cluster_manager_->initialize(bootstrap);
+  RETURN_IF_NOT_OK(status);
 
   const auto& listeners = bootstrap.static_resources().listeners();
   ENVOY_LOG(info, "loading {} listener(s)", listeners.size());

test/common/upstream/cluster_manager_impl_test.cc

@@ -279,7 +279,7 @@ class ClusterManagerImplTest : public testing::Test {
         factory_.runtime_, factory_.local_info_, log_manager_, factory_.dispatcher_, admin_,
         validation_context_, *factory_.api_, local_cluster_update_, local_hosts_removed_,
         http_context_, grpc_context_, router_context_, server_);
-    THROW_IF_NOT_OK(cluster_manager_->init(bootstrap));
+    THROW_IF_NOT_OK(cluster_manager_->initialize(bootstrap));
   }
 
   void createWithUpdateOverrideClusterManager(const Bootstrap& bootstrap) {
@@ -288,7 +288,7 @@ class ClusterManagerImplTest : public testing::Test {
         factory_.runtime_, factory_.local_info_, log_manager_, factory_.dispatcher_, admin_,
         validation_context_, *factory_.api_, http_context_, grpc_context_, router_context_,
         server_);
-    THROW_IF_NOT_OK(cluster_manager_->init(bootstrap));
+    THROW_IF_NOT_OK(cluster_manager_->initialize(bootstrap));
   }
 
   void checkStats(uint64_t added, uint64_t modified, uint64_t removed, uint64_t active,

test/common/upstream/test_cluster_manager.h

@@ -174,7 +174,7 @@ class TestClusterManagerImpl : public ClusterManagerImpl {
         new TestClusterManagerImpl(bootstrap, factory, context, stats, tls, runtime, local_info,
                                    log_manager, main_thread_dispatcher, admin, validation_context,
                                    api, http_context, grpc_context, router_context, server)};
-    THROW_IF_NOT_OK(cluster_manager->init(bootstrap));
+    THROW_IF_NOT_OK(cluster_manager->initialize(bootstrap));
     return cluster_manager;
   }
 

test/mocks/upstream/cluster_manager.h

@@ -35,6 +35,8 @@ class MockClusterManager : public ClusterManager {
   void initializeThreadLocalClusters(const std::vector<std::string>& cluster_names);
 
   // Upstream::ClusterManager
+  MOCK_METHOD(absl::Status, initialize, (const envoy::config::bootstrap::v3::Bootstrap& bootstrap));
+  MOCK_METHOD(bool, initialized, ());
   MOCK_METHOD(bool, addOrUpdateCluster,
               (const envoy::config::cluster::v3::Cluster& cluster,
                const std::string& version_info));