xds: share rds configuration among envoy 2/2

include/envoy/config/config_provider.h

@@ -121,7 +121,7 @@ class ConfigProvider {
     for (const auto* elem : config_protos) {
       ret_protos.push_back(static_cast<const P*>(elem));
     }
-    return ConfigProtoInfoVector<P>{ret_protos, getConfigVersion()};
+    return ConfigProtoInfoVector<P>{std::move(ret_protos), getConfigVersion()};
   }
 
   /**

include/envoy/router/rds.h

@@ -56,6 +56,7 @@ class RouteConfigProvider {
 };
 
 using RouteConfigProviderPtr = std::unique_ptr<RouteConfigProvider>;
+using RouteConfigProviderSharedPtr = std::shared_ptr<RouteConfigProvider>;
 
 } // namespace Router
 } // namespace Envoy

include/envoy/router/route_config_provider_manager.h

@@ -36,7 +36,7 @@ class RouteConfigProviderManager {
    * @param init_manager the Init::Manager used to coordinate initialization of a the underlying RDS
    * subscription.
    */
-  virtual RouteConfigProviderPtr createRdsRouteConfigProvider(
+  virtual RouteConfigProviderSharedPtr createRdsRouteConfigProvider(
       const envoy::config::filter::network::http_connection_manager::v2::Rds& rds,
       Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix,
       Init::Manager& init_manager) PURE;

source/common/router/rds_impl.cc

@@ -18,7 +18,7 @@
 namespace Envoy {
 namespace Router {
 
-RouteConfigProviderPtr RouteConfigProviderUtil::create(
+RouteConfigProviderSharedPtr RouteConfigProviderUtil::create(
     const envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager&
         config,
     Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix,
@@ -74,7 +74,7 @@ RdsRouteConfigSubscription::RdsRouteConfigSubscription(
           Grpc::Common::typeUrl(envoy::api::v2::RouteConfiguration().GetDescriptor()->full_name()),
           *scope_, *this);
   config_update_info_ =
-      std::make_unique<RouteConfigUpdateReceiverImpl>(factory_context.timeSource(), validator_);
+      std::make_unique<RouteConfigUpdateReceiverImpl>(factory_context.timeSource(), validator);
 }
 
 RdsRouteConfigSubscription::~RdsRouteConfigSubscription() {
@@ -85,7 +85,7 @@ RdsRouteConfigSubscription::~RdsRouteConfigSubscription() {
   // hold a shared_ptr to it. The RouteConfigProviderManager holds weak_ptrs to the
   // RdsRouteConfigProviders. Therefore, the map entry for the RdsRouteConfigProvider has to get
   // cleaned by the RdsRouteConfigProvider's destructor.
-  route_config_provider_manager_.route_config_subscriptions_.erase(manager_identifier_);
+  route_config_provider_manager_.dynamic_route_config_providers_.erase(manager_identifier_);
 }
 
 void RdsRouteConfigSubscription::onConfigUpdate(
@@ -215,11 +215,15 @@ RdsRouteConfigProviderImpl::RdsRouteConfigProviderImpl(
   tls_->set([initial_config](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr {
     return std::make_shared<ThreadLocalConfig>(initial_config);
   });
+  // It should be 1:1 mapping due to shared rds config.
+  ASSERT(subscription_->routeConfigProviders().empty());
   subscription_->routeConfigProviders().insert(this);
 }
 
 RdsRouteConfigProviderImpl::~RdsRouteConfigProviderImpl() {
   subscription_->routeConfigProviders().erase(this);
+  // It should be 1:1 mapping due to shared rds config.
+  ASSERT(subscription_->routeConfigProviders().empty());
 }
 
 Router::ConfigConstSharedPtr RdsRouteConfigProviderImpl::config() {
@@ -251,37 +255,38 @@ RouteConfigProviderManagerImpl::RouteConfigProviderManagerImpl(Server::Admin& ad
   RELEASE_ASSERT(config_tracker_entry_, "");
 }
 
-Router::RouteConfigProviderPtr RouteConfigProviderManagerImpl::createRdsRouteConfigProvider(
+Router::RouteConfigProviderSharedPtr RouteConfigProviderManagerImpl::createRdsRouteConfigProvider(
     const envoy::config::filter::network::http_connection_manager::v2::Rds& rds,
     Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix,
     Init::Manager& init_manager) {
   // RdsRouteConfigSubscriptions are unique based on their serialized RDS config.
   const uint64_t manager_identifier = MessageUtil::hash(rds);
-  auto& server_factory_context = factory_context.getServerFactoryContext();
+  auto it = dynamic_route_config_providers_.find(manager_identifier);
 
-  RdsRouteConfigSubscriptionSharedPtr subscription;
-
-  auto it = route_config_subscriptions_.find(manager_identifier);
-  if (it == route_config_subscriptions_.end()) {
+  if (it == dynamic_route_config_providers_.end()) {
     // std::make_shared does not work for classes with private constructors. There are ways
     // around it. However, since this is not a performance critical path we err on the side
     // of simplicity.
-    subscription.reset(new RdsRouteConfigSubscription(
-        rds, manager_identifier, server_factory_context, factory_context.messageValidationVisitor(),
-        init_manager, stat_prefix, *this));
+    RdsRouteConfigSubscriptionSharedPtr subscription(new RdsRouteConfigSubscription(
+        rds, manager_identifier, factory_context.getServerFactoryContext(),
+        factory_context.messageValidationVisitor(), factory_context.initManager(), stat_prefix,
+        *this));
     init_manager.add(subscription->init_target_);
-    route_config_subscriptions_.insert({manager_identifier, subscription});
+    std::shared_ptr<RdsRouteConfigProviderImpl> new_provider{
+        new RdsRouteConfigProviderImpl(std::move(subscription), factory_context)};
+    dynamic_route_config_providers_.insert(
+        {manager_identifier, std::weak_ptr<RdsRouteConfigProviderImpl>(new_provider)});
+    return new_provider;
   } else {
     // Because the RouteConfigProviderManager's weak_ptrs only get cleaned up
     // in the RdsRouteConfigSubscription destructor, and the single threaded nature
     // of this code, locking the weak_ptr will not fail.
-    subscription = it->second.lock();
+    auto existing_provider = it->second.lock();
+    RELEASE_ASSERT(existing_provider != nullptr,
+                   absl::StrCat("cannot find subscribed rds resource ", rds.route_config_name()));
+    init_manager.add(existing_provider->subscription_->init_target_);
+    return existing_provider;
   }
-  ASSERT(subscription);
-
-  Router::RouteConfigProviderPtr new_provider{
-      new RdsRouteConfigProviderImpl(std::move(subscription), factory_context)};
-  return new_provider;
 }
 
 RouteConfigProviderPtr RouteConfigProviderManagerImpl::createStaticRouteConfigProvider(
@@ -297,11 +302,11 @@ std::unique_ptr<envoy::admin::v2alpha::RoutesConfigDump>
 RouteConfigProviderManagerImpl::dumpRouteConfigs() const {
   auto config_dump = std::make_unique<envoy::admin::v2alpha::RoutesConfigDump>();
 
-  for (const auto& element : route_config_subscriptions_) {
+  for (const auto& element : dynamic_route_config_providers_) {
+    const auto& subscription = element.second.lock()->subscription_;
     // Because the RouteConfigProviderManager's weak_ptrs only get cleaned up
     // in the RdsRouteConfigSubscription destructor, and the single threaded nature
     // of this code, locking the weak_ptr will not fail.
-    auto subscription = element.second.lock();
     ASSERT(subscription);
     ASSERT(!subscription->route_config_providers_.empty());
 

source/common/router/rds_impl.h

@@ -45,10 +45,10 @@ class ScopedRdsConfigSubscription;
 class RouteConfigProviderUtil {
 public:
   /**
-   * @return RouteConfigProviderPtr a new route configuration provider based on the supplied proto
-   *         configuration.
+   * @return RouteConfigProviderSharedPtr a new route configuration provider based on the supplied
+   * proto configuration. Notes the provider object could be shared among multiple listeners.
    */
-  static RouteConfigProviderPtr
+  static RouteConfigProviderSharedPtr
   create(const envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager&
              config,
          Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix,
@@ -112,6 +112,7 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks,
   ~RdsRouteConfigSubscription() override;
 
   std::unordered_set<RouteConfigProvider*>& routeConfigProviders() {
+    ASSERT(route_config_providers_.size() == 1 || route_config_providers_.empty());
     return route_config_providers_;
   }
   RouteConfigUpdatePtr& routeConfigUpdate() { return config_update_info_; }
@@ -153,12 +154,13 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks,
   Server::Configuration::ServerFactoryContext& factory_context_;
   ProtobufMessage::ValidationVisitor& validator_;
   Init::Manager& init_manager_;
-  Init::TargetImpl init_target_;
+  Init::SharedTargetImpl init_target_;
   Stats::ScopePtr scope_;
   std::string stat_prefix_;
   RdsStats stats_;
   RouteConfigProviderManagerImpl& route_config_provider_manager_;
   const uint64_t manager_identifier_;
+  // TODO(lambdai): Prove that a subscription has exactly one provider and remove the container.
   std::unordered_set<RouteConfigProvider*> route_config_providers_;
   VhdsSubscriptionPtr vhds_subscription_;
   RouteConfigUpdatePtr config_update_info_;
@@ -217,7 +219,7 @@ class RouteConfigProviderManagerImpl : public RouteConfigProviderManager,
   std::unique_ptr<envoy::admin::v2alpha::RoutesConfigDump> dumpRouteConfigs() const;
 
   // RouteConfigProviderManager
-  RouteConfigProviderPtr createRdsRouteConfigProvider(
+  RouteConfigProviderSharedPtr createRdsRouteConfigProvider(
       const envoy::config::filter::network::http_connection_manager::v2::Rds& rds,
       Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix,
       Init::Manager& init_manager) override;
@@ -230,8 +232,8 @@ class RouteConfigProviderManagerImpl : public RouteConfigProviderManager,
   // TODO(jsedgwick) These two members are prime candidates for the owned-entry list/map
   // as in ConfigTracker. I.e. the ProviderImpls would have an EntryOwner for these lists
   // Then the lifetime management stuff is centralized and opaque.
-  std::unordered_map<uint64_t, std::weak_ptr<RdsRouteConfigSubscription>>
-      route_config_subscriptions_;
+  std::unordered_map<uint64_t, std::weak_ptr<RdsRouteConfigProviderImpl>>
+      dynamic_route_config_providers_;
   std::unordered_set<RouteConfigProvider*> static_route_config_providers_;
   Server::ConfigTracker::EntryOwnerPtr config_tracker_entry_;
 

source/common/router/scoped_rds.cc

@@ -114,11 +114,10 @@ ScopedRdsConfigSubscription::RdsRouteConfigProviderHelper::RdsRouteConfigProvide
     envoy::config::filter::network::http_connection_manager::v2::Rds& rds,
     Init::Manager& init_manager)
     : parent_(parent), scope_name_(scope_name),
-      route_provider_(static_cast<RdsRouteConfigProviderImpl*>(
-          parent_.route_config_provider_manager_
-              .createRdsRouteConfigProvider(rds, parent_.factory_context_, parent_.stat_prefix_,
-                                            init_manager)
-              .release())),
+      route_provider_(std::dynamic_pointer_cast<RdsRouteConfigProviderImpl>(
+          parent_.route_config_provider_manager_.createRdsRouteConfigProvider(
+              rds, parent_.factory_context_, parent_.stat_prefix_, init_manager))),
+
       rds_update_callback_handle_(route_provider_->subscription().addUpdateCallback([this]() {
         // Subscribe to RDS update.
         parent_.onRdsConfigUpdate(scope_name_, route_provider_->subscription());

source/common/router/scoped_rds.h

@@ -117,7 +117,7 @@ class ScopedRdsConfigSubscription : public Envoy::Config::DeltaConfigSubscriptio
 
     ScopedRdsConfigSubscription& parent_;
     std::string scope_name_;
-    std::unique_ptr<RdsRouteConfigProviderImpl> route_provider_;
+    std::shared_ptr<RdsRouteConfigProviderImpl> route_provider_;
     // This handle_ is owned by the route config provider's RDS subscription, when the helper
     // destructs, the handle is deleted as well.
     Common::CallbackHandle* rds_update_callback_handle_;

source/extensions/filters/network/http_connection_manager/config.h

@@ -180,7 +180,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   absl::optional<std::chrono::milliseconds> max_connection_duration_;
   std::chrono::milliseconds stream_idle_timeout_;
   std::chrono::milliseconds request_timeout_;
-  Router::RouteConfigProviderPtr route_config_provider_;
+  Router::RouteConfigProviderSharedPtr route_config_provider_;
   Config::ConfigProviderPtr scoped_routes_config_provider_;
   std::chrono::milliseconds drain_timeout_;
   bool generate_request_id_;

test/common/router/rds_impl_test.cc

@@ -117,7 +117,7 @@ stat_prefix: foo
 
   NiceMock<Server::MockInstance> server_;
   std::unique_ptr<RouteConfigProviderManagerImpl> route_config_provider_manager_;
-  RouteConfigProviderPtr rds_;
+  RouteConfigProviderSharedPtr rds_;
 };
 
 TEST_F(RdsImplTest, RdsAndStatic) {
@@ -331,7 +331,7 @@ class RouteConfigProviderManagerImplTest : public RdsTestBase {
 
   envoy::config::filter::network::http_connection_manager::v2::Rds rds_;
   std::unique_ptr<RouteConfigProviderManagerImpl> route_config_provider_manager_;
-  RouteConfigProviderPtr provider_;
+  RouteConfigProviderSharedPtr provider_;
 };
 
 envoy::api::v2::RouteConfiguration parseRouteConfigurationFromV2Yaml(const std::string& yaml) {
@@ -470,12 +470,15 @@ name: foo_route_config
   server_factory_context_.cluster_manager_.subscription_factory_.callbacks_->onConfigUpdate(
       route_configs, "1");
 
-  RouteConfigProviderPtr provider2 = route_config_provider_manager_->createRdsRouteConfigProvider(
-      rds_, mock_factory_context_, "foo_prefix", outer_init_manager_);
+  RouteConfigProviderSharedPtr provider2 =
+      route_config_provider_manager_->createRdsRouteConfigProvider(
+          rds_, mock_factory_context_, "foo_prefix", outer_init_manager_);
 
   // provider2 should have route config immediately after create
   EXPECT_TRUE(provider2->configInfo().has_value());
 
+  EXPECT_EQ(provider_, provider2) << "fail to obtain the same rds config provider object";
+
   // So this means that both provider have same subscription.
   EXPECT_EQ(&dynamic_cast<RdsRouteConfigProviderImpl&>(*provider_).subscription(),
             &dynamic_cast<RdsRouteConfigProviderImpl&>(*provider2).subscription());
@@ -484,8 +487,9 @@ name: foo_route_config
   envoy::config::filter::network::http_connection_manager::v2::Rds rds2;
   rds2.set_route_config_name("foo_route_config");
   rds2.mutable_config_source()->set_path("bar_path");
-  RouteConfigProviderPtr provider3 = route_config_provider_manager_->createRdsRouteConfigProvider(
-      rds2, mock_factory_context_, "foo_prefix", mock_factory_context_.initManager());
+  RouteConfigProviderSharedPtr provider3 =
+      route_config_provider_manager_->createRdsRouteConfigProvider(
+          rds2, mock_factory_context_, "foo_prefix", mock_factory_context_.initManager());
   EXPECT_NE(provider3, provider_);
   server_factory_context_.cluster_manager_.subscription_factory_.callbacks_->onConfigUpdate(
       route_configs, "provider3");
@@ -510,6 +514,49 @@ name: foo_route_config
             route_config_provider_manager_->dumpRouteConfigs()->dynamic_route_configs().size());
 }
 
+TEST_F(RouteConfigProviderManagerImplTest, SameProviderOnTwoInitManager) {
+  Buffer::OwnedImpl data;
+  // Get a RouteConfigProvider. This one should create an entry in the RouteConfigProviderManager.
+  setup();
+
+  EXPECT_FALSE(provider_->configInfo().has_value());
+
+  NiceMock<Server::Configuration::MockFactoryContext> mock_factory_context2;
+
+  Init::WatcherImpl real_watcher("real", []() {});
+  Init::ManagerImpl real_init_manager("real");
+
+  RouteConfigProviderSharedPtr provider2 =
+      route_config_provider_manager_->createRdsRouteConfigProvider(rds_, mock_factory_context2,
+                                                                   "foo_prefix", real_init_manager);
+
+  EXPECT_FALSE(provider2->configInfo().has_value());
+
+  EXPECT_EQ(provider_, provider2) << "fail to obtain the same rds config provider object";
+  real_init_manager.initialize(real_watcher);
+  EXPECT_EQ(Init::Manager::State::Initializing, real_init_manager.state());
+
+  {
+    Protobuf::RepeatedPtrField<ProtobufWkt::Any> route_configs;
+    route_configs.Add()->PackFrom(parseRouteConfigurationFromV2Yaml(R"EOF(
+name: foo_route_config
+virtual_hosts:
+  - name: bar
+    domains: ["*"]
+    routes:
+      - match: { prefix: "/" }
+        route: { cluster: baz }
+)EOF"));
+
+    server_factory_context_.cluster_manager_.subscription_factory_.callbacks_->onConfigUpdate(
+        route_configs, "1");
+
+    EXPECT_TRUE(provider_->configInfo().has_value());
+    EXPECT_TRUE(provider2->configInfo().has_value());
+    EXPECT_EQ(Init::Manager::State::Initialized, real_init_manager.state());
+  }
+}
+
 // Negative test for protoc-gen-validate constraints.
 TEST_F(RouteConfigProviderManagerImplTest, ValidateFail) {
   setup();

test/common/router/scoped_rds_test.cc

@@ -279,7 +279,7 @@ route_configuration_name: foo_routes
   parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml2);
   EXPECT_NO_THROW(srds_subscription_->onConfigUpdate(resources, "1"));
   context_init_manager_.initialize(init_watcher_);
-  init_watcher_.expectReady().Times(2); // SRDS and RDS "foo_routes"
+  init_watcher_.expectReady().Times(3); // SRDS x2 and RDS "foo_routes"
   EXPECT_EQ(
       1UL,
       factory_context_.scope_.counter("foo.scoped_rds.foo_scoped_routes.config_reload").value());
@@ -333,7 +333,7 @@ route_configuration_name: foo_routes
 // Tests that multiple uniquely named non-conflict resources are allowed in config updates.
 TEST_F(ScopedRdsTest, MultipleResourcesDelta) {
   setup();
-  init_watcher_.expectReady().Times(2); // SRDS and RDS "foo_routes"
+  init_watcher_.expectReady().Times(3); // SRDS x2 and RDS "foo_routes"
 
   const std::string config_yaml = R"EOF(
 name: foo_scope

test/mocks/router/mocks.h

@@ -430,7 +430,7 @@ class MockRouteConfigProviderManager : public RouteConfigProviderManager {
   ~MockRouteConfigProviderManager() override;
 
   MOCK_METHOD4(createRdsRouteConfigProvider,
-               RouteConfigProviderPtr(
+               RouteConfigProviderSharedPtr(
                    const envoy::config::filter::network::http_connection_manager::v2::Rds& rds,
                    Server::Configuration::FactoryContext& factory_context,
                    const std::string& stat_prefix, Init::Manager& init_manager));