rds: make RouteConfigProvider unique_ptr

include/envoy/router/rds.h

@@ -45,7 +45,7 @@ class RouteConfigProvider {
   virtual SystemTime lastUpdated() const PURE;
 };
 
-typedef std::shared_ptr<RouteConfigProvider> RouteConfigProviderSharedPtr;
+typedef std::unique_ptr<RouteConfigProvider> RouteConfigProviderPtr;
 
 } // namespace Router
 } // namespace Envoy

include/envoy/router/route_config_provider_manager.h

@@ -27,43 +27,29 @@ class RouteConfigProviderManager {
   virtual ~RouteConfigProviderManager() {}
 
   /**
-   * Get a RouteConfigProviderSharedPtr for a route from RDS. Ownership of the RouteConfigProvider
-   * is shared by all the HttpConnectionManagers who own a RouteConfigProviderSharedPtr. The
-   * RouteConfigProviderManager holds weak_ptrs to the RouteConfigProviders. Clean up of the weak
-   * ptrs happen from the destructor of the RouteConfigProvider. This function creates a
-   * RouteConfigProvider if there isn't one with the same (route_config_name, cluster) already.
-   * Otherwise, it returns a RouteConfigProviderSharedPtr created from the manager held weak_ptr.
+   * Get a RouteConfigProviderPtr for a route from RDS. Ownership of the RouteConfigProvider is the
+   * HttpConnectionManagers who calls this function. The RouteConfigProviderManager holds raw
+   * pointers to the RouteConfigProviders. Clean up of the pointers happen from the destructor of
+   * the RouteConfigProvider. This method creates a RouteConfigProvider which may share the
+   * underlying RDS subscription with the same (route_config_name, cluster).
    * @param rds supplies the proto configuration of an RDS-configured RouteConfigProvider.
    * @param factory_context is the context to use for the route config provider.
    * @param stat_prefix supplies the stat_prefix to use for the provider stats.
    */
-  virtual RouteConfigProviderSharedPtr getRdsRouteConfigProvider(
+  virtual RouteConfigProviderPtr createRdsRouteConfigProvider(
       const envoy::config::filter::network::http_connection_manager::v2::Rds& rds,
       Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix) PURE;
 
   /**
    * Get a RouteConfigSharedPtr for a statically defined route. Ownership is as described for
-   * getRdsRouteConfigProvider above. Unlike getRdsRouteConfigProvider(), this method always creates
-   * a new RouteConfigProvider.
+   * getRdsRouteConfigProvider above. This method always create a new RouteConfigProvider.
    * @param route_config supplies the RouteConfiguration for this route
    * @param runtime supplies the runtime loader.
    * @param cm supplies the ClusterManager.
    */
-  virtual RouteConfigProviderSharedPtr
-  getStaticRouteConfigProvider(const envoy::api::v2::RouteConfiguration& route_config,
-                               Server::Configuration::FactoryContext& factory_context) PURE;
-
-  /**
-   * @return std::vector<Router::RouteConfigProviderSharedPtr> a list of all the
-   * dynamic (RDS) RouteConfigProviders currently loaded.
-   */
-  virtual std::vector<RouteConfigProviderSharedPtr> getRdsRouteConfigProviders() PURE;
-
-  /**
-   * @return std::vector<Router::RouteConfigProviderSharedPtr> a list of all the
-   * static RouteConfigProviders currently loaded.
-   */
-  virtual std::vector<RouteConfigProviderSharedPtr> getStaticRouteConfigProviders() PURE;
+  virtual RouteConfigProviderPtr
+  createStaticRouteConfigProvider(const envoy::api::v2::RouteConfiguration& route_config,
+                                  Server::Configuration::FactoryContext& factory_context) PURE;
 };
 
 } // namespace Router

source/common/router/rds_impl.cc

@@ -21,29 +21,37 @@
 namespace Envoy {
 namespace Router {
 
-RouteConfigProviderSharedPtr RouteConfigProviderUtil::create(
+RouteConfigProviderPtr RouteConfigProviderUtil::create(
     const envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager&
         config,
     Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix,
     RouteConfigProviderManager& route_config_provider_manager) {
   switch (config.route_specifier_case()) {
   case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::
       kRouteConfig:
-    return route_config_provider_manager.getStaticRouteConfigProvider(config.route_config(),
-                                                                      factory_context);
+    return route_config_provider_manager.createStaticRouteConfigProvider(config.route_config(),
+                                                                         factory_context);
   case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::kRds:
-    return route_config_provider_manager.getRdsRouteConfigProvider(config.rds(), factory_context,
-                                                                   stat_prefix);
+    return route_config_provider_manager.createRdsRouteConfigProvider(config.rds(), factory_context,
+                                                                      stat_prefix);
   default:
     NOT_REACHED_GCOVR_EXCL_LINE;
   }
 }
 
 StaticRouteConfigProviderImpl::StaticRouteConfigProviderImpl(
     const envoy::api::v2::RouteConfiguration& config,
-    Server::Configuration::FactoryContext& factory_context)
+    Server::Configuration::FactoryContext& factory_context,
+    RouteConfigProviderManagerImpl& route_config_provider_manager)
     : config_(new ConfigImpl(config, factory_context, true)), route_config_proto_{config},
-      last_updated_(factory_context.systemTimeSource().currentTime()) {}
+      last_updated_(factory_context.systemTimeSource().currentTime()),
+      route_config_provider_manager_(route_config_provider_manager) {
+  route_config_provider_manager_.static_route_config_providers_.insert(this);
+}
+
+StaticRouteConfigProviderImpl::~StaticRouteConfigProviderImpl() {
+  route_config_provider_manager_.static_route_config_providers_.erase(this);
+}
 
 // TODO(htuch): If support for multiple clusters is added per #1170 cluster_name_
 // initialization needs to be fixed.
@@ -189,40 +197,7 @@ RouteConfigProviderManagerImpl::RouteConfigProviderManagerImpl(Server::Admin& ad
   RELEASE_ASSERT(config_tracker_entry_, "");
 }
 
-std::vector<RouteConfigProviderSharedPtr>
-RouteConfigProviderManagerImpl::getRdsRouteConfigProviders() {
-  std::vector<RouteConfigProviderSharedPtr> ret;
-  ret.reserve(route_config_subscriptions_.size());
-  for (const auto& element : route_config_subscriptions_) {
-    // 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_.size() > 0);
-    ret.push_back((*subscription->route_config_providers_.begin())->shared_from_this());
-  }
-  return ret;
-};
-
-std::vector<RouteConfigProviderSharedPtr>
-RouteConfigProviderManagerImpl::getStaticRouteConfigProviders() {
-  std::vector<RouteConfigProviderSharedPtr> providers_strong;
-  // Collect non-expired providers.
-  for (const auto& weak_provider : static_route_config_providers_) {
-    const auto strong_provider = weak_provider.lock();
-    if (strong_provider != nullptr) {
-      providers_strong.push_back(strong_provider);
-    }
-  }
-
-  // Replace our stored list of weak_ptrs with the filtered list.
-  static_route_config_providers_.assign(providers_strong.begin(), providers_strong.end());
-
-  return providers_strong;
-};
-
-Router::RouteConfigProviderSharedPtr RouteConfigProviderManagerImpl::getRdsRouteConfigProvider(
+Router::RouteConfigProviderPtr RouteConfigProviderManagerImpl::createRdsRouteConfigProvider(
     const envoy::config::filter::network::http_connection_manager::v2::Rds& rds,
     Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix) {
 
@@ -252,43 +227,50 @@ Router::RouteConfigProviderSharedPtr RouteConfigProviderManagerImpl::getRdsRoute
   }
   ASSERT(subscription);
 
-  Router::RouteConfigProviderSharedPtr new_provider{
+  Router::RouteConfigProviderPtr new_provider{
       new RdsRouteConfigProviderImpl(std::move(subscription), factory_context)};
   return new_provider;
-};
+}
 
-RouteConfigProviderSharedPtr RouteConfigProviderManagerImpl::getStaticRouteConfigProvider(
+RouteConfigProviderPtr RouteConfigProviderManagerImpl::createStaticRouteConfigProvider(
     const envoy::api::v2::RouteConfiguration& route_config,
     Server::Configuration::FactoryContext& factory_context) {
   auto provider =
-      std::make_shared<StaticRouteConfigProviderImpl>(std::move(route_config), factory_context);
-  static_route_config_providers_.push_back(provider);
+      absl::make_unique<StaticRouteConfigProviderImpl>(route_config, factory_context, *this);
+  static_route_config_providers_.insert(provider.get());
   return provider;
 }
 
-ProtobufTypes::MessagePtr RouteConfigProviderManagerImpl::dumpRouteConfigs() {
+std::unique_ptr<envoy::admin::v2alpha::RoutesConfigDump>
+RouteConfigProviderManagerImpl::dumpRouteConfigs() const {
   auto config_dump = std::make_unique<envoy::admin::v2alpha::RoutesConfigDump>();
 
-  for (const auto& provider : getRdsRouteConfigProviders()) {
-    auto config_info = provider->configInfo();
-    if (config_info) {
+  for (const auto& element : route_config_subscriptions_) {
+    // 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_.size() > 0);
+
+    if (subscription->config_info_) {
       auto* dynamic_config = config_dump->mutable_dynamic_route_configs()->Add();
-      dynamic_config->set_version_info(config_info.value().version_);
-      dynamic_config->mutable_route_config()->MergeFrom(config_info.value().config_);
-      TimestampUtil::systemClockToTimestamp(provider->lastUpdated(),
-                                            *(dynamic_config->mutable_last_updated()));
+      dynamic_config->set_version_info(subscription->config_info_.value().last_config_version_);
+      dynamic_config->mutable_route_config()->MergeFrom(subscription->route_config_proto_);
+      TimestampUtil::systemClockToTimestamp(subscription->last_updated_,
+                                            *dynamic_config->mutable_last_updated());
     }
   }
 
-  for (const auto& provider : getStaticRouteConfigProviders()) {
+  for (const auto& provider : static_route_config_providers_) {
     ASSERT(provider->configInfo());
     auto* static_config = config_dump->mutable_static_route_configs()->Add();
     static_config->mutable_route_config()->MergeFrom(provider->configInfo().value().config_);
     TimestampUtil::systemClockToTimestamp(provider->lastUpdated(),
-                                          *(static_config->mutable_last_updated()));
+                                          *static_config->mutable_last_updated());
   }
 
-  return ProtobufTypes::MessagePtr{std::move(config_dump)};
+  return config_dump;
 }
 
 } // namespace Router

source/common/router/rds_impl.h

@@ -6,6 +6,7 @@
 #include <unordered_map>
 #include <unordered_set>
 
+#include "envoy/admin/v2alpha/config_dump.pb.h"
 #include "envoy/api/v2/rds.pb.h"
 #include "envoy/api/v2/route/route.pb.h"
 #include "envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.pb.h"
@@ -35,20 +36,24 @@ class RouteConfigProviderUtil {
    * @return RouteConfigProviderPtr a new route configuration provider based on the supplied proto
    *         configuration.
    */
-  static RouteConfigProviderSharedPtr
+  static RouteConfigProviderPtr
   create(const envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager&
              config,
          Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix,
          RouteConfigProviderManager& route_config_provider_manager);
 };
 
+class RouteConfigProviderManagerImpl;
+
 /**
  * Implementation of RouteConfigProvider that holds a static route configuration.
  */
 class StaticRouteConfigProviderImpl : public RouteConfigProvider {
 public:
   StaticRouteConfigProviderImpl(const envoy::api::v2::RouteConfiguration& config,
-                                Server::Configuration::FactoryContext& factory_context);
+                                Server::Configuration::FactoryContext& factory_context,
+                                RouteConfigProviderManagerImpl& route_config_provider_manager);
+  ~StaticRouteConfigProviderImpl();
 
   // Router::RouteConfigProvider
   Router::ConfigConstSharedPtr config() override { return config_; }
@@ -61,6 +66,7 @@ class StaticRouteConfigProviderImpl : public RouteConfigProvider {
   ConfigConstSharedPtr config_;
   envoy::api::v2::RouteConfiguration route_config_proto_;
   SystemTime last_updated_;
+  RouteConfigProviderManagerImpl& route_config_provider_manager_;
 };
 
 /**
@@ -80,7 +86,6 @@ struct RdsStats {
   ALL_RDS_STATS(GENERATE_COUNTER_STRUCT)
 };
 
-class RouteConfigProviderManagerImpl;
 class RdsRouteConfigProviderImpl;
 
 /**
@@ -146,7 +151,6 @@ typedef std::shared_ptr<RdsRouteConfigSubscription> RdsRouteConfigSubscriptionSh
  * the subscription.
  */
 class RdsRouteConfigProviderImpl : public RouteConfigProvider,
-                                   public std::enable_shared_from_this<RdsRouteConfigProviderImpl>,
                                    Logger::Loggable<Logger::Id::router> {
 public:
   ~RdsRouteConfigProviderImpl();
@@ -172,7 +176,6 @@ class RdsRouteConfigProviderImpl : public RouteConfigProvider,
   RdsRouteConfigSubscriptionSharedPtr subscription_;
   Server::Configuration::FactoryContext& factory_context_;
   ThreadLocal::SlotPtr tls_;
-  const std::string route_config_name_;
 
   friend class RouteConfigProviderManagerImpl;
 };
@@ -182,32 +185,29 @@ class RouteConfigProviderManagerImpl : public RouteConfigProviderManager,
 public:
   RouteConfigProviderManagerImpl(Server::Admin& admin);
 
-  // RouteConfigProviderManager
-  std::vector<RouteConfigProviderSharedPtr> getRdsRouteConfigProviders() override;
-  std::vector<RouteConfigProviderSharedPtr> getStaticRouteConfigProviders() override;
+  std::unique_ptr<envoy::admin::v2alpha::RoutesConfigDump> dumpRouteConfigs() const;
 
-  RouteConfigProviderSharedPtr getRdsRouteConfigProvider(
+  // RouteConfigProviderManager
+  RouteConfigProviderPtr createRdsRouteConfigProvider(
       const envoy::config::filter::network::http_connection_manager::v2::Rds& rds,
       Server::Configuration::FactoryContext& factory_context,
       const std::string& stat_prefix) override;
 
-  RouteConfigProviderSharedPtr
-  getStaticRouteConfigProvider(const envoy::api::v2::RouteConfiguration& route_config,
-                               Server::Configuration::FactoryContext& factory_context) override;
+  RouteConfigProviderPtr
+  createStaticRouteConfigProvider(const envoy::api::v2::RouteConfiguration& route_config,
+                                  Server::Configuration::FactoryContext& factory_context) override;
 
 private:
-  ProtobufTypes::MessagePtr dumpRouteConfigs();
-
   // 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. Plus the copypasta
-  // in getRdsRouteConfigProviders()/getStaticRouteConfigProviders() goes away.
+  // Then the lifetime management stuff is centralized and opaque.
   std::unordered_map<std::string, std::weak_ptr<RdsRouteConfigSubscription>>
       route_config_subscriptions_;
-  std::vector<std::weak_ptr<RouteConfigProvider>> static_route_config_providers_;
+  std::unordered_set<RouteConfigProvider*> static_route_config_providers_;
   Server::ConfigTracker::EntryOwnerPtr config_tracker_entry_;
 
   friend class RdsRouteConfigSubscription;
+  friend class StaticRouteConfigProviderImpl;
 };
 
 } // namespace Router

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

@@ -139,7 +139,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   absl::optional<std::string> user_agent_;
   absl::optional<std::chrono::milliseconds> idle_timeout_;
   std::chrono::milliseconds stream_idle_timeout_;
-  Router::RouteConfigProviderSharedPtr route_config_provider_;
+  Router::RouteConfigProviderPtr route_config_provider_;
   std::chrono::milliseconds drain_timeout_;
   bool generate_request_id_;
   Http::DateProvider& date_provider_;