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..daacf2e8e6de 100644 --- a/include/envoy/router/route_config_provider_manager.h +++ b/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 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; + virtual RouteConfigProviderPtr + createStaticRouteConfigProvider(const envoy::api::v2::RouteConfiguration& route_config, + Server::Configuration::FactoryContext& factory_context) PURE; }; } // namespace Router diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index 4586845b2691..6a3e8c545cae 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,40 +197,7 @@ 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())->shared_from_this()); - } - 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); - } - } - - // 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(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; } -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) { + 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 diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index 261759248f9d..b859b0bca41a 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" @@ -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 RdsRouteConfigSubscriptionSh * the subscription. */ class RdsRouteConfigProviderImpl : public RouteConfigProvider, - public std::enable_shared_from_this, Logger::Loggable { 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 getRdsRouteConfigProviders() override; - std::vector getStaticRouteConfigProviders() override; + std::unique_ptr 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> 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..c0ff21dd58b9 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 = @@ -508,15 +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. - RouteConfigProviderSharedPtr provider2 = - route_config_provider_manager_->getRdsRouteConfigProvider(rds_, factory_context_, - "foo_prefix"); + 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( { @@ -538,32 +557,32 @@ 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()); + dynamic_cast(*provider3) + .subscription() + .onConfigUpdate(route_configs, "provider3"); - std::vector configured_providers = - route_config_provider_manager_->getRdsRouteConfigProviders(); - EXPECT_EQ(2UL, configured_providers.size()); - EXPECT_EQ(2UL, provider3.use_count()); + 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, 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 + EXPECT_EQ("provider3", dynamic_route_configs[0].version_info()); 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. diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 9040253dc63d..24b643370682 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -310,17 +310,14 @@ 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)); }; } // namespace Router