From bda352ef9000063a0944377840a1b9fc4b1ad169 Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Thu, 26 Jul 2018 23:38:26 -0700 Subject: [PATCH 1/5] rds: make RouteConfigProvider unique_ptr Signed-off-by: Lizan Zhou --- include/envoy/router/rds.h | 2 +- .../router/route_config_provider_manager.h | 30 +++++----- source/common/router/rds_impl.cc | 56 +++++++++---------- source/common/router/rds_impl.h | 27 +++++---- .../network/http_connection_manager/config.h | 2 +- test/common/router/rds_impl_test.cc | 28 ++++------ test/mocks/router/mocks.h | 15 +++-- 7 files changed, 77 insertions(+), 83 deletions(-) diff --git a/include/envoy/router/rds.h b/include/envoy/router/rds.h index 827740aee3fc..8ff43f213f4f 100644 --- a/include/envoy/router/rds.h +++ b/include/envoy/router/rds.h @@ -45,7 +45,7 @@ class RouteConfigProvider { virtual SystemTime lastUpdated() const PURE; }; -typedef std::shared_ptr RouteConfigProviderSharedPtr; +typedef std::unique_ptr RouteConfigProviderPtr; } // namespace Router } // namespace Envoy diff --git a/include/envoy/router/route_config_provider_manager.h b/include/envoy/router/route_config_provider_manager.h index 98b8db5a937e..b728af56698b 100644 --- a/include/envoy/router/route_config_provider_manager.h +++ b/include/envoy/router/route_config_provider_manager.h @@ -27,43 +27,41 @@ 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; + virtual RouteConfigProviderPtr + createStaticRouteConfigProvider(const envoy::api::v2::RouteConfiguration& route_config, + Server::Configuration::FactoryContext& factory_context) PURE; /** - * @return std::vector a list of all the + * @return std::vector a list of all the * dynamic (RDS) RouteConfigProviders currently loaded. */ - virtual std::vector getRdsRouteConfigProviders() PURE; + virtual std::vector getRdsRouteConfigProviders() PURE; /** - * @return std::vector a list of all the + * @return std::vector a list of all the * static RouteConfigProviders currently loaded. */ - virtual std::vector getStaticRouteConfigProviders() PURE; + virtual std::vector getStaticRouteConfigProviders() PURE; }; } // namespace Router diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index 4586845b2691..e92089386bad 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -21,7 +21,7 @@ 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, @@ -29,11 +29,11 @@ RouteConfigProviderSharedPtr RouteConfigProviderUtil::create( 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; } @@ -41,9 +41,17 @@ RouteConfigProviderSharedPtr RouteConfigProviderUtil::create( 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,9 +197,8 @@ RouteConfigProviderManagerImpl::RouteConfigProviderManagerImpl(Server::Admin& ad RELEASE_ASSERT(config_tracker_entry_, ""); } -std::vector -RouteConfigProviderManagerImpl::getRdsRouteConfigProviders() { - std::vector ret; +std::vector RouteConfigProviderManagerImpl::getRdsRouteConfigProviders() { + std::vector ret; ret.reserve(route_config_subscriptions_.size()); for (const auto& element : route_config_subscriptions_) { // Because the RouteConfigProviderManager's weak_ptrs only get cleaned up @@ -200,29 +207,20 @@ RouteConfigProviderManagerImpl::getRdsRouteConfigProviders() { 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()); + ret.push_back(*subscription->route_config_providers_.begin()); } return ret; }; -std::vector -RouteConfigProviderManagerImpl::getStaticRouteConfigProviders() { - std::vector 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); - } - } +std::vector RouteConfigProviderManagerImpl::getStaticRouteConfigProviders() { + std::vector ret; - // Replace our stored list of weak_ptrs with the filtered list. - static_route_config_providers_.assign(providers_strong.begin(), providers_strong.end()); + ret.assign(static_route_config_providers_.begin(), static_route_config_providers_.end()); - return providers_strong; + return ret; }; -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,17 +250,17 @@ 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(std::move(route_config), factory_context); - static_route_config_providers_.push_back(provider); + absl::make_unique(route_config, factory_context, *this); + static_route_config_providers_.insert(provider.get()); return provider; } diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index 261759248f9d..f5a9b114200c 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -35,20 +35,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 +65,7 @@ class StaticRouteConfigProviderImpl : public RouteConfigProvider { ConfigConstSharedPtr config_; envoy::api::v2::RouteConfiguration route_config_proto_; SystemTime last_updated_; + RouteConfigProviderManagerImpl& route_config_provider_manager_; }; /** @@ -80,7 +85,6 @@ struct RdsStats { ALL_RDS_STATS(GENERATE_COUNTER_STRUCT) }; -class RouteConfigProviderManagerImpl; class RdsRouteConfigProviderImpl; /** @@ -146,7 +150,6 @@ typedef std::shared_ptr RdsRouteConfigSubscriptionSh * the subscription. */ class RdsRouteConfigProviderImpl : public RouteConfigProvider, - public std::enable_shared_from_this, Logger::Loggable { public: ~RdsRouteConfigProviderImpl(); @@ -172,7 +175,6 @@ class RdsRouteConfigProviderImpl : public RouteConfigProvider, RdsRouteConfigSubscriptionSharedPtr subscription_; Server::Configuration::FactoryContext& factory_context_; ThreadLocal::SlotPtr tls_; - const std::string route_config_name_; friend class RouteConfigProviderManagerImpl; }; @@ -183,17 +185,17 @@ class RouteConfigProviderManagerImpl : public RouteConfigProviderManager, RouteConfigProviderManagerImpl(Server::Admin& admin); // RouteConfigProviderManager - std::vector getRdsRouteConfigProviders() override; - std::vector getStaticRouteConfigProviders() override; + std::vector getRdsRouteConfigProviders() override; + std::vector getStaticRouteConfigProviders() override; - RouteConfigProviderSharedPtr getRdsRouteConfigProvider( + 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(); @@ -204,10 +206,11 @@ class RouteConfigProviderManagerImpl : public RouteConfigProviderManager, // in getRdsRouteConfigProviders()/getStaticRouteConfigProviders() goes away. std::unordered_map> route_config_subscriptions_; - std::vector> static_route_config_providers_; + std::unordered_set static_route_config_providers_; Server::ConfigTracker::EntryOwnerPtr config_tracker_entry_; friend class RdsRouteConfigSubscription; + friend class StaticRouteConfigProviderImpl; }; } // namespace Router diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index f4d173059c37..c4cffd2dd458 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -139,7 +139,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, absl::optional user_agent_; absl::optional 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_; diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index a918767606e7..ab5bc943d333 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -116,7 +116,7 @@ class RdsImplTest : public RdsTestBase { NiceMock scope_; NiceMock server_; std::unique_ptr route_config_provider_manager_; - RouteConfigProviderSharedPtr rds_; + RouteConfigProviderPtr rds_; }; TEST_F(RdsImplTest, RdsAndStatic) { @@ -375,8 +375,8 @@ class RouteConfigProviderManagerImplTest : public RdsTestBase { EXPECT_CALL(*cluster.info_, addedViaApi()); EXPECT_CALL(*cluster.info_, type()); interval_timer_ = new Event::MockTimer(&factory_context_.dispatcher_); - provider_ = route_config_provider_manager_->getRdsRouteConfigProvider(rds_, factory_context_, - "foo_prefix."); + provider_ = route_config_provider_manager_->createRdsRouteConfigProvider(rds_, factory_context_, + "foo_prefix."); } RouteConfigProviderManagerImplTest() { @@ -390,7 +390,7 @@ class RouteConfigProviderManagerImplTest : public RdsTestBase { Stats::StatsOptionsImpl stats_options_; envoy::config::filter::network::http_connection_manager::v2::Rds rds_; std::unique_ptr route_config_provider_manager_; - RouteConfigProviderSharedPtr provider_; + RouteConfigProviderPtr provider_; }; envoy::api::v2::RouteConfiguration parseRouteConfigurationFromV2Yaml(const std::string& yaml) { @@ -429,8 +429,8 @@ name: foo EXPECT_CALL(factory_context_, systemTimeSource()).WillRepeatedly(ReturnRef(system_time_source_)); // Only static route. - RouteConfigProviderSharedPtr static_config = - route_config_provider_manager_->getStaticRouteConfigProvider( + RouteConfigProviderPtr static_config = + route_config_provider_manager_->createStaticRouteConfigProvider( parseRouteConfigurationFromV2Yaml(config_yaml), factory_context_); message_ptr = factory_context_.admin_.config_tracker_.config_tracker_callbacks_["routes"](); const auto& route_config_dump2 = @@ -510,9 +510,8 @@ TEST_F(RouteConfigProviderManagerImplTest, Basic) { // Because this get has the same cluster and route_config_name, the provider returned is just a // shared_ptr to the same provider as the one above. - RouteConfigProviderSharedPtr provider2 = - route_config_provider_manager_->getRdsRouteConfigProvider(rds_, factory_context_, - "foo_prefix"); + RouteConfigProviderPtr provider2 = route_config_provider_manager_->createRdsRouteConfigProvider( + rds_, factory_context_, "foo_prefix"); // So this means that both provider have same subscription. EXPECT_NE(provider_, provider2); EXPECT_EQ(&dynamic_cast(*provider_).subscription(), @@ -538,16 +537,13 @@ TEST_F(RouteConfigProviderManagerImplTest, Basic) { EXPECT_CALL(*cluster.info_, addedViaApi()); EXPECT_CALL(*cluster.info_, type()); new Event::MockTimer(&factory_context_.dispatcher_); - RouteConfigProviderSharedPtr provider3 = - route_config_provider_manager_->getRdsRouteConfigProvider(rds2, factory_context_, - "foo_prefix"); + RouteConfigProviderPtr provider3 = route_config_provider_manager_->createRdsRouteConfigProvider( + rds2, factory_context_, "foo_prefix"); EXPECT_NE(provider3, provider_); - EXPECT_EQ(1UL, provider3.use_count()); - std::vector configured_providers = + std::vector configured_providers = route_config_provider_manager_->getRdsRouteConfigProviders(); EXPECT_EQ(2UL, configured_providers.size()); - EXPECT_EQ(2UL, provider3.use_count()); provider_.reset(); provider2.reset(); @@ -557,7 +553,7 @@ TEST_F(RouteConfigProviderManagerImplTest, Basic) { // now we should only have the provider pointed at by provider3. configured_providers = route_config_provider_manager_->getRdsRouteConfigProviders(); EXPECT_EQ(1UL, configured_providers.size()); - EXPECT_EQ(provider3, configured_providers.front()); + EXPECT_EQ(provider3.get(), configured_providers.front()); provider3.reset(); configured_providers.clear(); diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 9040253dc63d..cf5726258385 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -310,17 +310,16 @@ class MockRouteConfigProviderManager : public RouteConfigProviderManager { MockRouteConfigProviderManager(); ~MockRouteConfigProviderManager(); - MOCK_METHOD3(getRdsRouteConfigProvider, - RouteConfigProviderSharedPtr( + MOCK_METHOD3(createRdsRouteConfigProvider, + RouteConfigProviderPtr( const envoy::config::filter::network::http_connection_manager::v2::Rds& rds, Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix)); - MOCK_METHOD2( - getStaticRouteConfigProvider, - RouteConfigProviderSharedPtr(const envoy::api::v2::RouteConfiguration& route_config, - Server::Configuration::FactoryContext& factory_context)); - MOCK_METHOD0(getRdsRouteConfigProviders, std::vector()); - MOCK_METHOD0(getStaticRouteConfigProviders, std::vector()); + MOCK_METHOD2(createStaticRouteConfigProvider, + RouteConfigProviderPtr(const envoy::api::v2::RouteConfiguration& route_config, + Server::Configuration::FactoryContext& factory_context)); + MOCK_METHOD0(getRdsRouteConfigProviders, std::vector()); + MOCK_METHOD0(getStaticRouteConfigProviders, std::vector()); }; } // namespace Router From 140f85f3e45619e75c69fc10ac24f0e91f369fff Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Mon, 30 Jul 2018 15:35:48 -0700 Subject: [PATCH 2/5] remove unused interface methods - add more tests - make dump config public Signed-off-by: Lizan Zhou --- .../router/route_config_provider_manager.h | 12 ---- source/common/router/rds_impl.cc | 56 +++++++------------ source/common/router/rds_impl.h | 8 +-- test/common/router/rds_impl_test.cc | 46 ++++++++++----- 4 files changed, 56 insertions(+), 66 deletions(-) diff --git a/include/envoy/router/route_config_provider_manager.h b/include/envoy/router/route_config_provider_manager.h index b728af56698b..daacf2e8e6de 100644 --- a/include/envoy/router/route_config_provider_manager.h +++ b/include/envoy/router/route_config_provider_manager.h @@ -50,18 +50,6 @@ class RouteConfigProviderManager { virtual RouteConfigProviderPtr createStaticRouteConfigProvider(const envoy::api::v2::RouteConfiguration& route_config, Server::Configuration::FactoryContext& factory_context) PURE; - - /** - * @return std::vector a list of all the - * dynamic (RDS) RouteConfigProviders currently loaded. - */ - virtual std::vector getRdsRouteConfigProviders() PURE; - - /** - * @return std::vector a list of all the - * static RouteConfigProviders currently loaded. - */ - virtual std::vector getStaticRouteConfigProviders() PURE; }; } // namespace Router diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index e92089386bad..ebe3de14808b 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -197,29 +197,6 @@ RouteConfigProviderManagerImpl::RouteConfigProviderManagerImpl(Server::Admin& ad RELEASE_ASSERT(config_tracker_entry_, ""); } -std::vector RouteConfigProviderManagerImpl::getRdsRouteConfigProviders() { - std::vector 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()); - } - return ret; -}; - -std::vector RouteConfigProviderManagerImpl::getStaticRouteConfigProviders() { - std::vector ret; - - ret.assign(static_route_config_providers_.begin(), static_route_config_providers_.end()); - - return ret; -}; - 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) { @@ -253,7 +230,7 @@ Router::RouteConfigProviderPtr RouteConfigProviderManagerImpl::createRdsRouteCon Router::RouteConfigProviderPtr new_provider{ new RdsRouteConfigProviderImpl(std::move(subscription), factory_context)}; return new_provider; -}; +} RouteConfigProviderPtr RouteConfigProviderManagerImpl::createStaticRouteConfigProvider( const envoy::api::v2::RouteConfiguration& route_config, @@ -264,29 +241,36 @@ RouteConfigProviderPtr RouteConfigProviderManagerImpl::createStaticRouteConfigPr return provider; } -ProtobufTypes::MessagePtr RouteConfigProviderManagerImpl::dumpRouteConfigs() { +std::unique_ptr +RouteConfigProviderManagerImpl::dumpRouteConfigs() const { auto config_dump = std::make_unique(); - for (const auto& provider : getRdsRouteConfigProviders()) { - auto config_info = provider->configInfo(); - if (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())); + 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); + + auto* dynamic_config = config_dump->mutable_dynamic_route_configs()->Add(); + if (subscription->config_info_) { + 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 diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index f5a9b114200c..60ae9b57bfc2 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -6,6 +6,7 @@ #include #include +#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" @@ -184,10 +185,9 @@ class RouteConfigProviderManagerImpl : public RouteConfigProviderManager, public: RouteConfigProviderManagerImpl(Server::Admin& admin); - // RouteConfigProviderManager - std::vector getRdsRouteConfigProviders() override; - std::vector getStaticRouteConfigProviders() override; + std::unique_ptr dumpRouteConfigs() const; + // RouteConfigProviderManager RouteConfigProviderPtr createRdsRouteConfigProvider( const envoy::config::filter::network::http_connection_manager::v2::Rds& rds, Server::Configuration::FactoryContext& factory_context, @@ -198,8 +198,6 @@ class RouteConfigProviderManagerImpl : public RouteConfigProviderManager, 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 diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index ab5bc943d333..9a5ad5d0d2bb 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -508,14 +508,34 @@ TEST_F(RouteConfigProviderManagerImplTest, Basic) { // Get a RouteConfigProvider. This one should create an entry in the RouteConfigProviderManager. setup(); - // Because this get has the same cluster and route_config_name, the provider returned is just a - // shared_ptr to the same provider as the one above. + EXPECT_FALSE(provider_->configInfo().has_value()); + + Protobuf::RepeatedPtrField route_configs; + route_configs.Add()->MergeFrom(parseRouteConfigurationFromV2Yaml(R"EOF( +name: foo_route_config +virtual_hosts: + - name: bar + domains: ["*"] + routes: + - match: { prefix: "/" } + route: { cluster: baz } +)EOF")); + + RdsRouteConfigSubscription& subscription = + dynamic_cast(*provider_).subscription(); + + subscription.onConfigUpdate(route_configs, "1"); + RouteConfigProviderPtr provider2 = route_config_provider_manager_->createRdsRouteConfigProvider( rds_, factory_context_, "foo_prefix"); + + // provider2 should have route config immediately after create + EXPECT_TRUE(provider2->configInfo().has_value()); + // So this means that both provider have same subscription. - EXPECT_NE(provider_, provider2); EXPECT_EQ(&dynamic_cast(*provider_).subscription(), &dynamic_cast(*provider2).subscription()); + EXPECT_EQ(&provider_->configInfo().value().config_, &provider2->configInfo().value().config_); std::string config_json2 = R"EOF( { @@ -541,25 +561,25 @@ TEST_F(RouteConfigProviderManagerImplTest, Basic) { rds2, factory_context_, "foo_prefix"); EXPECT_NE(provider3, provider_); - std::vector configured_providers = - route_config_provider_manager_->getRdsRouteConfigProviders(); - EXPECT_EQ(2UL, configured_providers.size()); + EXPECT_EQ(2UL, + route_config_provider_manager_->dumpRouteConfigs()->dynamic_route_configs().size()); provider_.reset(); provider2.reset(); - configured_providers.clear(); // All shared_ptrs to the provider pointed at by provider1, and provider2 have been deleted, so // now we should only have the provider pointed at by provider3. - configured_providers = route_config_provider_manager_->getRdsRouteConfigProviders(); - EXPECT_EQ(1UL, configured_providers.size()); - EXPECT_EQ(provider3.get(), configured_providers.front()); + auto dynamic_route_configs = + route_config_provider_manager_->dumpRouteConfigs()->dynamic_route_configs(); + EXPECT_EQ(1UL, dynamic_route_configs.size()); + + // Make sure the left one is provider3, which hasn't received route config yet. + EXPECT_FALSE(dynamic_route_configs[0].has_route_config()); provider3.reset(); - configured_providers.clear(); - configured_providers = route_config_provider_manager_->getRdsRouteConfigProviders(); - EXPECT_EQ(0UL, configured_providers.size()); + EXPECT_EQ(0UL, + route_config_provider_manager_->dumpRouteConfigs()->dynamic_route_configs().size()); } // Negative test for protoc-gen-validate constraints. From b7ef2e5da684fa2c69f6bbc5fafa0077d8147a93 Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Mon, 30 Jul 2018 16:57:48 -0700 Subject: [PATCH 3/5] clear unused getRdsRouteConfigProviders()/getStaticRouteConfigProviders() Signed-off-by: Lizan Zhou --- source/common/router/rds_impl.h | 3 +-- test/mocks/router/mocks.h | 2 -- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index 60ae9b57bfc2..b859b0bca41a 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -200,8 +200,7 @@ class RouteConfigProviderManagerImpl : public RouteConfigProviderManager, private: // 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> route_config_subscriptions_; std::unordered_set static_route_config_providers_; diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index cf5726258385..24b643370682 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -318,8 +318,6 @@ class MockRouteConfigProviderManager : public RouteConfigProviderManager { MOCK_METHOD2(createStaticRouteConfigProvider, RouteConfigProviderPtr(const envoy::api::v2::RouteConfiguration& route_config, Server::Configuration::FactoryContext& factory_context)); - MOCK_METHOD0(getRdsRouteConfigProviders, std::vector()); - MOCK_METHOD0(getStaticRouteConfigProviders, std::vector()); }; } // namespace Router From b3ca9333fa7b73e5728623646245ac2794b6c70c Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Mon, 30 Jul 2018 21:43:55 -0700 Subject: [PATCH 4/5] not dump empty config Signed-off-by: Lizan Zhou --- source/common/router/rds_impl.cc | 6 +++--- test/common/router/rds_impl_test.cc | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index ebe3de14808b..6a3e8c545cae 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -253,13 +253,13 @@ RouteConfigProviderManagerImpl::dumpRouteConfigs() const { ASSERT(subscription); ASSERT(subscription->route_config_providers_.size() > 0); - auto* dynamic_config = config_dump->mutable_dynamic_route_configs()->Add(); if (subscription->config_info_) { + auto* dynamic_config = config_dump->mutable_dynamic_route_configs()->Add(); 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()); } - TimestampUtil::systemClockToTimestamp(subscription->last_updated_, - *dynamic_config->mutable_last_updated()); } for (const auto& provider : static_route_config_providers_) { diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index 9a5ad5d0d2bb..f32674889fb5 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -560,6 +560,7 @@ name: foo_route_config RouteConfigProviderPtr provider3 = route_config_provider_manager_->createRdsRouteConfigProvider( rds2, factory_context_, "foo_prefix"); EXPECT_NE(provider3, provider_); + dynamic_cast(*provider3).subscription().onConfigUpdate(route_configs, "provider3"); EXPECT_EQ(2UL, route_config_provider_manager_->dumpRouteConfigs()->dynamic_route_configs().size()); @@ -573,8 +574,8 @@ name: foo_route_config route_config_provider_manager_->dumpRouteConfigs()->dynamic_route_configs(); EXPECT_EQ(1UL, dynamic_route_configs.size()); - // Make sure the left one is provider3, which hasn't received route config yet. - EXPECT_FALSE(dynamic_route_configs[0].has_route_config()); + // Make sure the left one is provider3 + EXPECT_EQ("provider3", dynamic_route_configs[0].version_info()); provider3.reset(); From 7e80ee0e377da22626a61eb12e3ba64c24cbae40 Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Tue, 31 Jul 2018 09:52:53 -0700 Subject: [PATCH 5/5] fix format Signed-off-by: Lizan Zhou --- test/common/router/rds_impl_test.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index f32674889fb5..c0ff21dd58b9 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -560,7 +560,9 @@ name: foo_route_config RouteConfigProviderPtr provider3 = route_config_provider_manager_->createRdsRouteConfigProvider( rds2, factory_context_, "foo_prefix"); EXPECT_NE(provider3, provider_); - dynamic_cast(*provider3).subscription().onConfigUpdate(route_configs, "provider3"); + dynamic_cast(*provider3) + .subscription() + .onConfigUpdate(route_configs, "provider3"); EXPECT_EQ(2UL, route_config_provider_manager_->dumpRouteConfigs()->dynamic_route_configs().size());