diff --git a/include/envoy/config/config_provider.h b/include/envoy/config/config_provider.h index 8a8fbecb2efd..dbc5b579c626 100644 --- a/include/envoy/config/config_provider.h +++ b/include/envoy/config/config_provider.h @@ -121,7 +121,7 @@ class ConfigProvider { for (const auto* elem : config_protos) { ret_protos.push_back(static_cast(elem)); } - return ConfigProtoInfoVector

{ret_protos, getConfigVersion()}; + return ConfigProtoInfoVector

{std::move(ret_protos), getConfigVersion()}; } /** diff --git a/include/envoy/router/rds.h b/include/envoy/router/rds.h index 456e44922043..978ffa6fe9da 100644 --- a/include/envoy/router/rds.h +++ b/include/envoy/router/rds.h @@ -56,6 +56,7 @@ class RouteConfigProvider { }; using RouteConfigProviderPtr = std::unique_ptr; +using RouteConfigProviderSharedPtr = std::shared_ptr; } // 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 9912aa460356..387c84fcb9b7 100644 --- a/include/envoy/router/route_config_provider_manager.h +++ b/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; diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index 6164b39a7d17..7e9317191521 100644 --- a/source/common/router/rds_impl.cc +++ b/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(factory_context.timeSource(), validator_); + std::make_unique(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(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 new_provider{ + new RdsRouteConfigProviderImpl(std::move(subscription), factory_context)}; + dynamic_route_config_providers_.insert( + {manager_identifier, std::weak_ptr(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 RouteConfigProviderManagerImpl::dumpRouteConfigs() const { auto config_dump = std::make_unique(); - 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()); diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index f9d0164eb61a..1cc07d685993 100644 --- a/source/common/router/rds_impl.h +++ b/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& 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 route_config_providers_; VhdsSubscriptionPtr vhds_subscription_; RouteConfigUpdatePtr config_update_info_; @@ -217,7 +219,7 @@ class RouteConfigProviderManagerImpl : public RouteConfigProviderManager, std::unique_ptr 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> - route_config_subscriptions_; + std::unordered_map> + dynamic_route_config_providers_; std::unordered_set static_route_config_providers_; Server::ConfigTracker::EntryOwnerPtr config_tracker_entry_; diff --git a/source/common/router/scoped_rds.cc b/source/common/router/scoped_rds.cc index b97c5f3405f4..a1b79e98a82b 100644 --- a/source/common/router/scoped_rds.cc +++ b/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( - parent_.route_config_provider_manager_ - .createRdsRouteConfigProvider(rds, parent_.factory_context_, parent_.stat_prefix_, - init_manager) - .release())), + route_provider_(std::dynamic_pointer_cast( + 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()); diff --git a/source/common/router/scoped_rds.h b/source/common/router/scoped_rds.h index 422551449acd..d226f3dddc7e 100644 --- a/source/common/router/scoped_rds.h +++ b/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 route_provider_; + std::shared_ptr 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_; diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 4bce5086ad2b..63c47252e996 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -180,7 +180,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, absl::optional 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_; diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index 4e7be26a9a8e..a4b3300e1c6d 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -117,7 +117,7 @@ stat_prefix: foo NiceMock server_; std::unique_ptr 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 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(*provider_).subscription(), &dynamic_cast(*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 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 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(); diff --git a/test/common/router/scoped_rds_test.cc b/test/common/router/scoped_rds_test.cc index 140b800b1c79..cead6e004886 100644 --- a/test/common/router/scoped_rds_test.cc +++ b/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 diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 91c49470539b..ae2a92b4e195 100644 --- a/test/mocks/router/mocks.h +++ b/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));