From 32e56daabe14e56384ea1a01e88ade0b7908246b Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 10 Aug 2017 21:50:35 -0600 Subject: [PATCH 01/12] move route config provider manager to singleton manager registry --- include/envoy/server/BUILD | 2 -- include/envoy/server/filter_config.h | 6 ------ include/envoy/server/instance.h | 6 ------ source/common/router/BUILD | 1 + source/common/router/rds_impl.h | 3 ++- source/server/config/network/BUILD | 1 + .../config/network/http_connection_manager.cc | 18 +++++++++++++----- .../config/network/http_connection_manager.h | 4 +++- source/server/config_validation/server.cc | 2 -- source/server/config_validation/server.h | 4 ---- source/server/listener_manager_impl.h | 3 --- source/server/server.cc | 3 --- source/server/server.h | 3 --- test/server/config/network/BUILD | 1 + .../network/http_connection_manager_test.cc | 8 +++++--- 15 files changed, 26 insertions(+), 39 deletions(-) diff --git a/include/envoy/server/BUILD b/include/envoy/server/BUILD index a42f300c36039..618eacc630a6a 100644 --- a/include/envoy/server/BUILD +++ b/include/envoy/server/BUILD @@ -63,7 +63,6 @@ envoy_cc_library( "//include/envoy/init:init_interface", "//include/envoy/local_info:local_info_interface", "//include/envoy/ratelimit:ratelimit_interface", - "//include/envoy/router:route_config_provider_manager_interface", "//include/envoy/runtime:runtime_interface", "//include/envoy/ssl:context_manager_interface", "//include/envoy/thread_local:thread_local_interface", @@ -107,7 +106,6 @@ envoy_cc_library( "//include/envoy/local_info:local_info_interface", "//include/envoy/network:drain_decision_interface", "//include/envoy/ratelimit:ratelimit_interface", - "//include/envoy/router:route_config_provider_manager_interface", "//include/envoy/runtime:runtime_interface", "//include/envoy/singleton:manager_interface", "//include/envoy/thread_local:thread_local_interface", diff --git a/include/envoy/server/filter_config.h b/include/envoy/server/filter_config.h index 3cf0f9ff117ea..5a56629570faa 100644 --- a/include/envoy/server/filter_config.h +++ b/include/envoy/server/filter_config.h @@ -9,7 +9,6 @@ #include "envoy/network/drain_decision.h" #include "envoy/network/filter.h" #include "envoy/ratelimit/ratelimit.h" -#include "envoy/router/route_config_provider_manager.h" #include "envoy/runtime/runtime.h" #include "envoy/singleton/manager.h" #include "envoy/thread_local/thread_local.h" @@ -117,11 +116,6 @@ class FactoryContext { * used to allow runtime lockless updates to configuration, etc. across multiple threads. */ virtual ThreadLocal::SlotAllocator& threadLocal() PURE; - - /** - * @return RouteConfigProviderManager& singleton for use by the entire server. - */ - virtual Router::RouteConfigProviderManager& routeConfigProviderManager() PURE; }; enum class NetworkFilterType { Read, Write, Both }; diff --git a/include/envoy/server/instance.h b/include/envoy/server/instance.h index 872261e24d81f..b6b1299da420a 100644 --- a/include/envoy/server/instance.h +++ b/include/envoy/server/instance.h @@ -10,7 +10,6 @@ #include "envoy/local_info/local_info.h" #include "envoy/network/listen_socket.h" #include "envoy/ratelimit/ratelimit.h" -#include "envoy/router/route_config_provider_manager.h" #include "envoy/runtime/runtime.h" #include "envoy/server/admin.h" #include "envoy/server/drain_manager.h" @@ -180,11 +179,6 @@ class Instance { * @return information about the local environment the server is running in. */ virtual const LocalInfo::LocalInfo& localInfo() PURE; - - /** - * @return the server's http route manager. - */ - virtual Router::ServerRouteConfigProviderManager& routeConfigProviderManager() PURE; }; } // namespace Server diff --git a/source/common/router/BUILD b/source/common/router/BUILD index 7cfc977848b7e..3fca20f62173f 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -59,6 +59,7 @@ envoy_cc_library( "//include/envoy/local_info:local_info_interface", "//include/envoy/router:rds_interface", "//include/envoy/router:route_config_provider_manager_interface", + "//include/envoy/singleton:instance_interface", "//include/envoy/thread_local:thread_local_interface", "//source/common/common:assert_lib", "//source/common/common:logger_lib", diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index 5da2ba9b6d660..18dcec9eddd68 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -10,6 +10,7 @@ #include "envoy/local_info/local_info.h" #include "envoy/router/rds.h" #include "envoy/router/route_config_provider_manager.h" +#include "envoy/singleton/instance.h" #include "envoy/thread_local/thread_local.h" #include "common/common/logger.h" @@ -124,7 +125,7 @@ class RdsRouteConfigProviderImpl : public RouteConfigProvider, }; // TODO(junr03): use then singleton manager in #1410. -class RouteConfigProviderManagerImpl : public ServerRouteConfigProviderManager { +class RouteConfigProviderManagerImpl : public ServerRouteConfigProviderManager, public Singleton::Instance { public: RouteConfigProviderManagerImpl(Runtime::Loader& runtime, Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, diff --git a/source/server/config/network/BUILD b/source/server/config/network/BUILD index d8ca02abaebd6..121414255c238 100644 --- a/source/server/config/network/BUILD +++ b/source/server/config/network/BUILD @@ -37,6 +37,7 @@ envoy_cc_library( "//include/envoy/filesystem:filesystem_interface", "//include/envoy/http:filter_interface", "//include/envoy/registry", + "//include/envoy/router:route_config_provider_manager_interface", "//include/envoy/server:filter_config_interface", "//include/envoy/server:options_interface", "//include/envoy/stats:stats_interface", diff --git a/source/server/config/network/http_connection_manager.cc b/source/server/config/network/http_connection_manager.cc index 714c2a6c3a7b9..d53c6863112d5 100644 --- a/source/server/config/network/http_connection_manager.cc +++ b/source/server/config/network/http_connection_manager.cc @@ -33,6 +33,9 @@ static Registry::RegisterFactory date_provider_singleton_registered_; +static constexpr char route_config_provider_manager_singleton_name[] = "route_config_provider_manager_singleton_name"; +static Registry::RegisterFactory, Singleton::Registration> route_config_provider_manager_singleton_registered_; + NetworkFilterFactoryCb HttpConnectionManagerFilterConfigFactory::createFilterFactory(const Json::Object& config, FactoryContext& context) { @@ -43,9 +46,14 @@ HttpConnectionManagerFilterConfigFactory::createFilterFactory(const Json::Object context.threadLocal()); }); + std::shared_ptr route_config_provider_manager = + context.singletonManager().getTyped(route_config_provider_manager_singleton_name, [&context] { + return std::make_shared(context.runtime(), context.dispatcher(), context.random(), context.localInfo(), context.threadLocal()); + }); + std::shared_ptr http_config( - new HttpConnectionManagerConfig(config, context, *date_provider)); - return [http_config, &context, date_provider](Network::FilterManager& filter_manager) -> void { + new HttpConnectionManagerConfig(config, context, *date_provider, *route_config_provider_manager)); + return [http_config, &context, date_provider, route_config_provider_manager](Network::FilterManager& filter_manager) -> void { filter_manager.addReadFilter(Network::ReadFilterSharedPtr{new Http::ConnectionManagerImpl( *http_config, context.drainDecision(), context.random(), context.httpTracer(), context.runtime(), context.localInfo(), context.clusterManager())}); @@ -79,7 +87,7 @@ HttpConnectionManagerConfigUtility::determineNextProtocol(Network::Connection& c HttpConnectionManagerConfig::HttpConnectionManagerConfig(const Json::Object& config, FactoryContext& context, - Http::DateProvider& date_provider) + Http::DateProvider& date_provider, Router::RouteConfigProviderManager& route_config_provider_manager) : Json::Validator(config, Json::Schema::HTTP_CONN_NETWORK_FILTER_SCHEMA), context_(context), stats_prefix_(fmt::format("http.{}.", config.getString("stat_prefix"))), stats_(Http::ConnectionManagerImpl::generateStats(stats_prefix_, context_.scope())), @@ -95,11 +103,11 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(const Json::Object& con http1_settings_(Http::Utility::parseHttp1Settings(config)), drain_timeout_(config.getInteger("drain_timeout_ms", 5000)), generate_request_id_(config.getBoolean("generate_request_id", true)), - date_provider_(date_provider) { + date_provider_(date_provider), route_config_provider_manager_(route_config_provider_manager) { route_config_provider_ = Router::RouteConfigProviderUtil::create( config, context_.runtime(), context_.clusterManager(), context_.scope(), stats_prefix_, - context_.initManager(), context_.routeConfigProviderManager()); + context_.initManager(), route_config_provider_manager_); if (config.hasObject("use_remote_address")) { use_remote_address_ = config.getBoolean("use_remote_address"); diff --git a/source/server/config/network/http_connection_manager.h b/source/server/config/network/http_connection_manager.h index 9179061134d0f..260a6b95a9f01 100644 --- a/source/server/config/network/http_connection_manager.h +++ b/source/server/config/network/http_connection_manager.h @@ -7,6 +7,7 @@ #include #include "envoy/http/filter.h" +#include "envoy/router/route_config_provider_manager.h" #include "envoy/server/filter_config.h" #include "common/common/logger.h" @@ -69,7 +70,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, Json::Validator { public: HttpConnectionManagerConfig(const Json::Object& config, FactoryContext& context, - Http::DateProvider& date_provider); + Http::DateProvider& date_provider, Router::RouteConfigProviderManager& route_config_provider_manager); // Http::FilterChainFactory void createFilterChain(Http::FilterChainFactoryCallbacks& callbacks) override; @@ -147,6 +148,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, std::chrono::milliseconds drain_timeout_; bool generate_request_id_; Http::DateProvider& date_provider_; + Router::RouteConfigProviderManager& route_config_provider_manager_; }; /** diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index ea28a70b0e474..bb1c3086ebffa 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -73,8 +73,6 @@ void ValidationInstance::initialize(Options& options, thread_local_.registerThread(*dispatcher_, true); runtime_loader_ = component_factory.createRuntime(*this, initial_config); ssl_context_manager_.reset(new Ssl::ContextManagerImpl(*runtime_loader_)); - route_config_provider_manager_.reset(new Router::RouteConfigProviderManagerImpl( - runtime(), dispatcher(), random(), localInfo(), threadLocal())); cluster_manager_factory_.reset(new Upstream::ValidationClusterManagerFactory( runtime(), stats(), threadLocal(), random(), dnsResolver(), sslContextManager(), dispatcher(), localInfo())); diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index 45690caedd891..a71be006e2f2b 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -86,9 +86,6 @@ class ValidationInstance : Logger::Loggable, Tracing::HttpTracer& httpTracer() override { return config_->httpTracer(); } ThreadLocal::Instance& threadLocal() override { return thread_local_; } const LocalInfo::LocalInfo& localInfo() override { return *local_info_; } - Router::ServerRouteConfigProviderManager& routeConfigProviderManager() override { - return *route_config_provider_manager_; - } // Server::ListenerComponentFactory std::vector @@ -131,7 +128,6 @@ class ValidationInstance : Logger::Loggable, AccessLog::AccessLogManagerImpl access_log_manager_; std::unique_ptr cluster_manager_factory_; InitManagerImpl init_manager_; - std::unique_ptr route_config_provider_manager_; ListenerManagerImpl listener_manager_; }; diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index c6539c64aaa9e..2083a66fd404f 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -222,9 +222,6 @@ class ListenerImpl : public Listener, Stats::Scope& scope() override { return *global_scope_; } Singleton::Manager& singletonManager() override { return parent_.server_.singletonManager(); } ThreadLocal::Instance& threadLocal() override { return parent_.server_.threadLocal(); } - Router::RouteConfigProviderManager& routeConfigProviderManager() override { - return parent_.server_.routeConfigProviderManager(); - } // Network::DrainDecision bool drainClose() const override; diff --git a/source/server/server.cc b/source/server/server.cc index 9886dcc6287f9..3fde57ea4046d 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -201,9 +201,6 @@ void InstanceImpl::initialize(Options& options, runtime(), stats(), threadLocal(), random(), dnsResolver(), sslContextManager(), dispatcher(), localInfo())); - route_config_provider_manager_.reset(new Router::RouteConfigProviderManagerImpl( - runtime(), dispatcher(), random(), localInfo(), threadLocal())); - // Now the configuration gets parsed. The configuration may start setting thread local data // per above. See MainImpl::initialize() for why we do this pointer dance. Configuration::MainImpl* main_config = new Configuration::MainImpl(); diff --git a/source/server/server.h b/source/server/server.h index e027fa3c084d5..f15e3536765e8 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -133,9 +133,6 @@ class InstanceImpl : Logger::Loggable, public Instance { Tracing::HttpTracer& httpTracer() override; ThreadLocal::Instance& threadLocal() override { return thread_local_; } const LocalInfo::LocalInfo& localInfo() override { return *local_info_; } - Router::ServerRouteConfigProviderManager& routeConfigProviderManager() override { - return *route_config_provider_manager_; - } private: void flushStats(); diff --git a/test/server/config/network/BUILD b/test/server/config/network/BUILD index b3dd60b7f0c32..04c0e385735d4 100644 --- a/test/server/config/network/BUILD +++ b/test/server/config/network/BUILD @@ -30,6 +30,7 @@ envoy_cc_test( deps = [ "//source/common/buffer:buffer_lib", "//source/common/event:dispatcher_lib", + "//source/common/router:rds_lib", "//source/server/config/http:dynamo_lib", "//source/server/config/network:http_connection_manager_lib", "//test/mocks/network:network_mocks", diff --git a/test/server/config/network/http_connection_manager_test.cc b/test/server/config/network/http_connection_manager_test.cc index c5d1113cbb041..cca8040fdb13b 100644 --- a/test/server/config/network/http_connection_manager_test.cc +++ b/test/server/config/network/http_connection_manager_test.cc @@ -1,5 +1,6 @@ #include "common/buffer/buffer_impl.h" #include "common/http/date_provider_impl.h" +#include "common/router/rds_impl.h" #include "server/config/network/http_connection_manager.h" @@ -22,6 +23,7 @@ class HttpConnectionManagerConfigTest : public testing::Test { public: NiceMock context_; Http::SlowDateProviderImpl date_provider_; + Router::RouteConfigProviderManagerImpl route_config_provider_manager_{context_.runtime(), context_.dispatcher(), context_.random(), context_.localInfo(), context_.threadLocal()}; }; TEST_F(HttpConnectionManagerConfigTest, InvalidFilterName) { @@ -52,7 +54,7 @@ TEST_F(HttpConnectionManagerConfigTest, InvalidFilterName) { )EOF"; Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); - EXPECT_THROW_WITH_MESSAGE(HttpConnectionManagerConfig(*json_config, context_, date_provider_), + EXPECT_THROW_WITH_MESSAGE(HttpConnectionManagerConfig(*json_config, context_, date_provider_, route_config_provider_manager_), EnvoyException, "unable to create http filter factory for 'foo'/'encoder'"); } @@ -85,7 +87,7 @@ TEST_F(HttpConnectionManagerConfigTest, InvalidFilterType) { )EOF"; Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); - EXPECT_THROW_WITH_MESSAGE(HttpConnectionManagerConfig(*json_config, context_, date_provider_), + EXPECT_THROW_WITH_MESSAGE(HttpConnectionManagerConfig(*json_config, context_, date_provider_, route_config_provider_manager_), EnvoyException, "unable to create http filter factory for 'router'/'encoder'"); } @@ -121,7 +123,7 @@ TEST_F(HttpConnectionManagerConfigTest, MiscConfig) { )EOF"; Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); - HttpConnectionManagerConfig config(*json_config, context_, date_provider_); + HttpConnectionManagerConfig config(*json_config, context_, date_provider_, route_config_provider_manager_); EXPECT_THAT(std::vector({Http::LowerCaseString("foo")}), ContainerEq(config.tracingConfig()->request_headers_for_tags_)); EXPECT_EQ(*context_.local_info_.address_, config.localAddress()); From f51cee1d22a9a07aa5d6b8df35e25cf9839b9c63 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 10 Aug 2017 23:05:50 -0600 Subject: [PATCH 02/12] comment --- source/common/router/rds_impl.h | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index 18dcec9eddd68..08d015d2a95a4 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -124,7 +124,6 @@ class RdsRouteConfigProviderImpl : public RouteConfigProvider, friend class RouteConfigProviderManagerImpl; }; -// TODO(junr03): use then singleton manager in #1410. class RouteConfigProviderManagerImpl : public ServerRouteConfigProviderManager, public Singleton::Instance { public: RouteConfigProviderManagerImpl(Runtime::Loader& runtime, Event::Dispatcher& dispatcher, From 67fe135cfd04d95141abb50e0eba2f29cb114a18 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 10 Aug 2017 23:09:32 -0600 Subject: [PATCH 03/12] format --- source/common/router/rds_impl.h | 3 +- .../config/network/http_connection_manager.cc | 30 ++++++++++++------- .../config/network/http_connection_manager.h | 3 +- .../network/http_connection_manager_test.cc | 13 +++++--- 4 files changed, 32 insertions(+), 17 deletions(-) diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index 08d015d2a95a4..68f01db6fbfb2 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -124,7 +124,8 @@ class RdsRouteConfigProviderImpl : public RouteConfigProvider, friend class RouteConfigProviderManagerImpl; }; -class RouteConfigProviderManagerImpl : public ServerRouteConfigProviderManager, public Singleton::Instance { +class RouteConfigProviderManagerImpl : public ServerRouteConfigProviderManager, + public Singleton::Instance { public: RouteConfigProviderManagerImpl(Runtime::Loader& runtime, Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, diff --git a/source/server/config/network/http_connection_manager.cc b/source/server/config/network/http_connection_manager.cc index d53c6863112d5..d68c6969ecf62 100644 --- a/source/server/config/network/http_connection_manager.cc +++ b/source/server/config/network/http_connection_manager.cc @@ -33,8 +33,12 @@ static Registry::RegisterFactory date_provider_singleton_registered_; -static constexpr char route_config_provider_manager_singleton_name[] = "route_config_provider_manager_singleton_name"; -static Registry::RegisterFactory, Singleton::Registration> route_config_provider_manager_singleton_registered_; +static constexpr char route_config_provider_manager_singleton_name[] = + "route_config_provider_manager_singleton_name"; +static Registry::RegisterFactory< + Singleton::RegistrationImpl, + Singleton::Registration> + route_config_provider_manager_singleton_registered_; NetworkFilterFactoryCb HttpConnectionManagerFilterConfigFactory::createFilterFactory(const Json::Object& config, @@ -47,13 +51,17 @@ HttpConnectionManagerFilterConfigFactory::createFilterFactory(const Json::Object }); std::shared_ptr route_config_provider_manager = - context.singletonManager().getTyped(route_config_provider_manager_singleton_name, [&context] { - return std::make_shared(context.runtime(), context.dispatcher(), context.random(), context.localInfo(), context.threadLocal()); - }); + context.singletonManager().getTyped( + route_config_provider_manager_singleton_name, [&context] { + return std::make_shared( + context.runtime(), context.dispatcher(), context.random(), context.localInfo(), + context.threadLocal()); + }); - std::shared_ptr http_config( - new HttpConnectionManagerConfig(config, context, *date_provider, *route_config_provider_manager)); - return [http_config, &context, date_provider, route_config_provider_manager](Network::FilterManager& filter_manager) -> void { + std::shared_ptr http_config(new HttpConnectionManagerConfig( + config, context, *date_provider, *route_config_provider_manager)); + return [http_config, &context, date_provider, + route_config_provider_manager](Network::FilterManager& filter_manager) -> void { filter_manager.addReadFilter(Network::ReadFilterSharedPtr{new Http::ConnectionManagerImpl( *http_config, context.drainDecision(), context.random(), context.httpTracer(), context.runtime(), context.localInfo(), context.clusterManager())}); @@ -85,9 +93,9 @@ HttpConnectionManagerConfigUtility::determineNextProtocol(Network::Connection& c return ""; } -HttpConnectionManagerConfig::HttpConnectionManagerConfig(const Json::Object& config, - FactoryContext& context, - Http::DateProvider& date_provider, Router::RouteConfigProviderManager& route_config_provider_manager) +HttpConnectionManagerConfig::HttpConnectionManagerConfig( + const Json::Object& config, FactoryContext& context, Http::DateProvider& date_provider, + Router::RouteConfigProviderManager& route_config_provider_manager) : Json::Validator(config, Json::Schema::HTTP_CONN_NETWORK_FILTER_SCHEMA), context_(context), stats_prefix_(fmt::format("http.{}.", config.getString("stat_prefix"))), stats_(Http::ConnectionManagerImpl::generateStats(stats_prefix_, context_.scope())), diff --git a/source/server/config/network/http_connection_manager.h b/source/server/config/network/http_connection_manager.h index 260a6b95a9f01..19c2e81358726 100644 --- a/source/server/config/network/http_connection_manager.h +++ b/source/server/config/network/http_connection_manager.h @@ -70,7 +70,8 @@ class HttpConnectionManagerConfig : Logger::Loggable, Json::Validator { public: HttpConnectionManagerConfig(const Json::Object& config, FactoryContext& context, - Http::DateProvider& date_provider, Router::RouteConfigProviderManager& route_config_provider_manager); + Http::DateProvider& date_provider, + Router::RouteConfigProviderManager& route_config_provider_manager); // Http::FilterChainFactory void createFilterChain(Http::FilterChainFactoryCallbacks& callbacks) override; diff --git a/test/server/config/network/http_connection_manager_test.cc b/test/server/config/network/http_connection_manager_test.cc index cca8040fdb13b..f2d7612f928e8 100644 --- a/test/server/config/network/http_connection_manager_test.cc +++ b/test/server/config/network/http_connection_manager_test.cc @@ -23,7 +23,9 @@ class HttpConnectionManagerConfigTest : public testing::Test { public: NiceMock context_; Http::SlowDateProviderImpl date_provider_; - Router::RouteConfigProviderManagerImpl route_config_provider_manager_{context_.runtime(), context_.dispatcher(), context_.random(), context_.localInfo(), context_.threadLocal()}; + Router::RouteConfigProviderManagerImpl route_config_provider_manager_{ + context_.runtime(), context_.dispatcher(), context_.random(), context_.localInfo(), + context_.threadLocal()}; }; TEST_F(HttpConnectionManagerConfigTest, InvalidFilterName) { @@ -54,7 +56,8 @@ TEST_F(HttpConnectionManagerConfigTest, InvalidFilterName) { )EOF"; Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); - EXPECT_THROW_WITH_MESSAGE(HttpConnectionManagerConfig(*json_config, context_, date_provider_, route_config_provider_manager_), + EXPECT_THROW_WITH_MESSAGE(HttpConnectionManagerConfig(*json_config, context_, date_provider_, + route_config_provider_manager_), EnvoyException, "unable to create http filter factory for 'foo'/'encoder'"); } @@ -87,7 +90,8 @@ TEST_F(HttpConnectionManagerConfigTest, InvalidFilterType) { )EOF"; Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); - EXPECT_THROW_WITH_MESSAGE(HttpConnectionManagerConfig(*json_config, context_, date_provider_, route_config_provider_manager_), + EXPECT_THROW_WITH_MESSAGE(HttpConnectionManagerConfig(*json_config, context_, date_provider_, + route_config_provider_manager_), EnvoyException, "unable to create http filter factory for 'router'/'encoder'"); } @@ -123,7 +127,8 @@ TEST_F(HttpConnectionManagerConfigTest, MiscConfig) { )EOF"; Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); - HttpConnectionManagerConfig config(*json_config, context_, date_provider_, route_config_provider_manager_); + HttpConnectionManagerConfig config(*json_config, context_, date_provider_, + route_config_provider_manager_); EXPECT_THAT(std::vector({Http::LowerCaseString("foo")}), ContainerEq(config.tracingConfig()->request_headers_for_tags_)); EXPECT_EQ(*context_.local_info_.address_, config.localAddress()); From 288ffaed8708f3ff54c6134526d79dfffb3e4913 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 10 Aug 2017 23:43:16 -0600 Subject: [PATCH 04/12] fix type name --- include/envoy/singleton/instance.h | 2 +- include/envoy/singleton/manager.h | 4 ++-- source/common/singleton/manager_impl.cc | 4 ++-- source/common/singleton/manager_impl.h | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/include/envoy/singleton/instance.h b/include/envoy/singleton/instance.h index 59d0f7b1ed8e2..8c1c92f2d0b22 100644 --- a/include/envoy/singleton/instance.h +++ b/include/envoy/singleton/instance.h @@ -11,7 +11,7 @@ class Instance { virtual ~Instance() {} }; -typedef std::shared_ptr InstancePtr; +typedef std::shared_ptr InstanceSharedPtr; } // namespace Singleton } // namespace Envoy diff --git a/include/envoy/singleton/manager.h b/include/envoy/singleton/manager.h index 78bfb1e0ebf8b..276af3d9fde30 100644 --- a/include/envoy/singleton/manager.h +++ b/include/envoy/singleton/manager.h @@ -39,7 +39,7 @@ template class RegistrationImpl : public Registration { /** * Callback function used to create a singleton. */ -typedef std::function SingletonFactoryCb; +typedef std::function SingletonFactoryCb; /** * A manager for all server-side singletons. @@ -66,7 +66,7 @@ class Manager { * singletons must store the shared_ptr for as long as the singleton is needed. * @return InstancePtr the singleton. */ - virtual InstancePtr get(const std::string& name, SingletonFactoryCb) PURE; + virtual InstanceSharedPtr get(const std::string& name, SingletonFactoryCb) PURE; }; typedef std::unique_ptr ManagerPtr; diff --git a/source/common/singleton/manager_impl.cc b/source/common/singleton/manager_impl.cc index 97fffb5ef3e83..8a8e87acbb2b1 100644 --- a/source/common/singleton/manager_impl.cc +++ b/source/common/singleton/manager_impl.cc @@ -7,14 +7,14 @@ namespace Envoy { namespace Singleton { -InstancePtr ManagerImpl::get(const std::string& name, SingletonFactoryCb cb) { +InstanceSharedPtr ManagerImpl::get(const std::string& name, SingletonFactoryCb cb) { ASSERT(run_tid_ == Thread::Thread::currentThreadId()); if (nullptr == Registry::FactoryRegistry::getFactory(name)) { PANIC(fmt::format("invalid singleton name '{}'. Make sure it is registered.", name)); } if (nullptr == singletons_[name].lock()) { - InstancePtr singleton = cb(); + InstanceSharedPtr singleton = cb(); singletons_[name] = singleton; return singleton; } else { diff --git a/source/common/singleton/manager_impl.h b/source/common/singleton/manager_impl.h index ff7c272d53989..066b4a3b838c6 100644 --- a/source/common/singleton/manager_impl.h +++ b/source/common/singleton/manager_impl.h @@ -19,7 +19,7 @@ class ManagerImpl : public Manager { ManagerImpl() : run_tid_(Thread::Thread::currentThreadId()) {} // Singleton::Manager - InstancePtr get(const std::string& name, SingletonFactoryCb cb) override; + InstanceSharedPtr get(const std::string& name, SingletonFactoryCb cb) override; private: std::unordered_map> singletons_; From d81b10987848b55420f664dcdfbc02dc9ddbc1f4 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 11 Aug 2017 22:10:11 -0600 Subject: [PATCH 05/12] update --- source/server/config/network/http_connection_manager.cc | 3 ++- source/server/config/network/http_connection_manager.h | 2 +- test/mocks/server/mocks.cc | 4 ---- test/mocks/server/mocks.h | 4 ---- 4 files changed, 3 insertions(+), 10 deletions(-) diff --git a/source/server/config/network/http_connection_manager.cc b/source/server/config/network/http_connection_manager.cc index d68c6969ecf62..1b3350aa17ad5 100644 --- a/source/server/config/network/http_connection_manager.cc +++ b/source/server/config/network/http_connection_manager.cc @@ -109,9 +109,10 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( return Http::Utility::parseHttp2Settings(http2_protocol_options); }()), http1_settings_(Http::Utility::parseHttp1Settings(config)), + route_config_provider_manager_(route_config_provider_manager), drain_timeout_(config.getInteger("drain_timeout_ms", 5000)), generate_request_id_(config.getBoolean("generate_request_id", true)), - date_provider_(date_provider), route_config_provider_manager_(route_config_provider_manager) { + date_provider_(date_provider) { route_config_provider_ = Router::RouteConfigProviderUtil::create( config, context_.runtime(), context_.clusterManager(), context_.scope(), stats_prefix_, diff --git a/source/server/config/network/http_connection_manager.h b/source/server/config/network/http_connection_manager.h index 19c2e81358726..fe701ec8fdaf7 100644 --- a/source/server/config/network/http_connection_manager.h +++ b/source/server/config/network/http_connection_manager.h @@ -145,11 +145,11 @@ class HttpConnectionManagerConfig : Logger::Loggable, Http::TracingConnectionManagerConfigPtr tracing_config_; Optional user_agent_; Optional idle_timeout_; + Router::RouteConfigProviderManager& route_config_provider_manager_; Router::RouteConfigProviderSharedPtr route_config_provider_; std::chrono::milliseconds drain_timeout_; bool generate_request_id_; Http::DateProvider& date_provider_; - Router::RouteConfigProviderManager& route_config_provider_manager_; }; /** diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc index af3b0bd4061d2..ad0016a9262c2 100644 --- a/test/mocks/server/mocks.cc +++ b/test/mocks/server/mocks.cc @@ -102,8 +102,6 @@ MockInstance::MockInstance() ON_CALL(*this, drainManager()).WillByDefault(ReturnRef(drain_manager_)); ON_CALL(*this, initManager()).WillByDefault(ReturnRef(init_manager_)); ON_CALL(*this, listenerManager()).WillByDefault(ReturnRef(listener_manager_)); - ON_CALL(*this, routeConfigProviderManager()) - .WillByDefault(ReturnRef(route_config_provider_manager_)); ON_CALL(*this, singletonManager()).WillByDefault(ReturnRef(*singleton_manager_)); } @@ -133,8 +131,6 @@ MockFactoryContext::MockFactoryContext() : singleton_manager_(new Singleton::Man ON_CALL(*this, server()).WillByDefault(ReturnRef(server_)); ON_CALL(*this, singletonManager()).WillByDefault(ReturnRef(*singleton_manager_)); ON_CALL(*this, threadLocal()).WillByDefault(ReturnRef(thread_local_)); - ON_CALL(*this, routeConfigProviderManager()) - .WillByDefault(ReturnRef(route_config_provider_manager_)); } MockFactoryContext::~MockFactoryContext() {} diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index ce731178a3881..9eee167bdef9c 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -266,7 +266,6 @@ class MockInstance : public Instance { MOCK_METHOD0(httpTracer, Tracing::HttpTracer&()); MOCK_METHOD0(threadLocal, ThreadLocal::Instance&()); MOCK_METHOD0(localInfo, const LocalInfo::LocalInfo&()); - MOCK_METHOD0(routeConfigProviderManager, Router::ServerRouteConfigProviderManager&()); testing::NiceMock thread_local_; Stats::IsolatedStoreImpl stats_store_; @@ -287,7 +286,6 @@ class MockInstance : public Instance { testing::NiceMock random_; testing::NiceMock local_info_; testing::NiceMock init_manager_; - testing::NiceMock route_config_provider_manager_; testing::NiceMock listener_manager_; Singleton::ManagerPtr singleton_manager_; }; @@ -341,11 +339,9 @@ class MockFactoryContext : public FactoryContext { MOCK_METHOD0(server, Instance&()); MOCK_METHOD0(singletonManager, Singleton::Manager&()); MOCK_METHOD0(threadLocal, ThreadLocal::Instance&()); - MOCK_METHOD0(routeConfigProviderManager, Router::RouteConfigProviderManager&()); testing::NiceMock access_log_manager_; testing::NiceMock cluster_manager_; - testing::NiceMock route_config_provider_manager_; testing::NiceMock dispatcher_; testing::NiceMock drain_manager_; testing::NiceMock http_tracer_; From 4de823685d5a010646230435fa29265c2db9954d Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Sat, 12 Aug 2017 07:16:40 -0600 Subject: [PATCH 06/12] comment --- source/server/config/network/http_connection_manager.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/source/server/config/network/http_connection_manager.cc b/source/server/config/network/http_connection_manager.cc index 1b3350aa17ad5..859eba45a4f59 100644 --- a/source/server/config/network/http_connection_manager.cc +++ b/source/server/config/network/http_connection_manager.cc @@ -60,6 +60,9 @@ HttpConnectionManagerFilterConfigFactory::createFilterFactory(const Json::Object std::shared_ptr http_config(new HttpConnectionManagerConfig( config, context, *date_provider, *route_config_provider_manager)); + + // This lambda captures the shared_ptrs created above, thus preserving the + // reference count. return [http_config, &context, date_provider, route_config_provider_manager](Network::FilterManager& filter_manager) -> void { filter_manager.addReadFilter(Network::ReadFilterSharedPtr{new Http::ConnectionManagerImpl( From bd6f5ce92667981941e725020a7cccb3ef0fd98e Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 15 Aug 2017 18:07:58 -0700 Subject: [PATCH 07/12] capture order --- source/server/config/network/http_connection_manager.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/server/config/network/http_connection_manager.cc b/source/server/config/network/http_connection_manager.cc index 9bd79e7c4c7ca..9fa6fa427b55e 100644 --- a/source/server/config/network/http_connection_manager.cc +++ b/source/server/config/network/http_connection_manager.cc @@ -66,8 +66,8 @@ NetworkFilterFactoryCb HttpConnectionManagerFilterConfigFactory::createFilterFac // This lambda captures the shared_ptrs created above, thus preserving the // reference count. - return [http_config, &context, date_provider, - route_config_provider_manager](Network::FilterManager& filter_manager) -> void { + return [route_config_provider_manager, http_config, &context, + date_provider](Network::FilterManager& filter_manager) -> void { filter_manager.addReadFilter(Network::ReadFilterSharedPtr{new Http::ConnectionManagerImpl( *http_config, context.drainDecision(), context.random(), context.httpTracer(), context.runtime(), context.localInfo(), context.clusterManager())}); From 5cb1981cb166c06367a9d15aec4be158a604e028 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 15 Aug 2017 19:54:03 -0700 Subject: [PATCH 08/12] test snuck in in merge --- .../network/http_connection_manager_test.cc | 33 ------------------- 1 file changed, 33 deletions(-) diff --git a/test/server/config/network/http_connection_manager_test.cc b/test/server/config/network/http_connection_manager_test.cc index 512cc6806475b..6482aa60f3590 100644 --- a/test/server/config/network/http_connection_manager_test.cc +++ b/test/server/config/network/http_connection_manager_test.cc @@ -71,39 +71,6 @@ TEST_F(HttpConnectionManagerConfigTest, InvalidFilterName) { EnvoyException, "unable to create http filter factory for 'foo'"); } -TEST_F(HttpConnectionManagerConfigTest, InvalidFilterType) { - const std::string json_string = R"EOF( - { - "codec_type": "http1", - "stat_prefix": "router", - "route_config": - { - "virtual_hosts": [ - { - "name": "service", - "domains": [ "*" ], - "routes": [ - { - "prefix": "/", - "cluster": "cluster" - } - ] - } - ] - }, - "filters": [ - { "type": "encoder", "name": "router", "config": {} - } - ] - } - )EOF"; - - EXPECT_THROW_WITH_MESSAGE( - HttpConnectionManagerConfig(parseHttpConnectionManagerFromJson(json_string), context_, - date_provider_, route_config_provider_manager_), - EnvoyException, "unable to create http filter factory for 'router'"); -} - TEST_F(HttpConnectionManagerConfigTest, MiscConfig) { const std::string json_string = R"EOF( { From f5c4e2eb95805b158c4ee24bd3bcbe08a33c4b74 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 16 Aug 2017 13:14:17 -0700 Subject: [PATCH 09/12] macro --- include/envoy/registry/registry.h | 10 ++++++++++ .../config/network/http_connection_manager.cc | 17 +++++------------ 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/include/envoy/registry/registry.h b/include/envoy/registry/registry.h index 4839f12e94bdf..53497196d0c97 100644 --- a/include/envoy/registry/registry.h +++ b/include/envoy/registry/registry.h @@ -10,6 +10,16 @@ namespace Envoy { namespace Registry { +/** + * Macro used to statically register singletons managed by the singleton manager + * defined in envoy/singleton/manager.h. + */ +#define SINGLETON_MANAGER_REGISTRATION(NAME) \ + static constexpr char NAME##_singleton_name[] = #NAME "_singleton"; \ + static Registry::RegisterFactory, \ + Singleton::Registration> \ + NAME##_singleton_registered_; + /** * General registry for implementation factories. The registry is templated by the Base class that a * set of factories conforms to. diff --git a/source/server/config/network/http_connection_manager.cc b/source/server/config/network/http_connection_manager.cc index 9fa6fa427b55e..0b4461894854b 100644 --- a/source/server/config/network/http_connection_manager.cc +++ b/source/server/config/network/http_connection_manager.cc @@ -29,17 +29,9 @@ namespace Configuration { const std::string HttpConnectionManagerConfig::DEFAULT_SERVER_STRING = "envoy"; -static constexpr char date_provider_singleton_name[] = "date_provider_singleton"; -static Registry::RegisterFactory, - Singleton::Registration> - date_provider_singleton_registered_; - -static constexpr char route_config_provider_manager_singleton_name[] = - "route_config_provider_manager_singleton_name"; -static Registry::RegisterFactory< - Singleton::RegistrationImpl, - Singleton::Registration> - route_config_provider_manager_singleton_registered_; +// Singleton registration via macro defined in envoy/registry/registry.h +SINGLETON_MANAGER_REGISTRATION(date_provider); +SINGLETON_MANAGER_REGISTRATION(route_config_provider_manager); NetworkFilterFactoryCb HttpConnectionManagerFilterConfigFactory::createFilterFactory( const Json::Object& json_http_connection_manager, FactoryContext& context) { @@ -65,7 +57,8 @@ NetworkFilterFactoryCb HttpConnectionManagerFilterConfigFactory::createFilterFac http_connection_manager, context, *date_provider, *route_config_provider_manager)); // This lambda captures the shared_ptrs created above, thus preserving the - // reference count. + // reference count. Moreover, keep in mind the capture list determines + // destruction order. return [route_config_provider_manager, http_config, &context, date_provider](Network::FilterManager& filter_manager) -> void { filter_manager.addReadFilter(Network::ReadFilterSharedPtr{new Http::ConnectionManagerImpl( From c6a65a68a976b36409059a9058c4b8efab9f8cbc Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 16 Aug 2017 15:16:40 -0700 Subject: [PATCH 10/12] move macro --- include/envoy/registry/registry.h | 10 ---------- include/envoy/singleton/instance.h | 14 ++++++++++++++ .../config/network/http_connection_manager.cc | 4 ++-- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/include/envoy/registry/registry.h b/include/envoy/registry/registry.h index 53497196d0c97..4839f12e94bdf 100644 --- a/include/envoy/registry/registry.h +++ b/include/envoy/registry/registry.h @@ -10,16 +10,6 @@ namespace Envoy { namespace Registry { -/** - * Macro used to statically register singletons managed by the singleton manager - * defined in envoy/singleton/manager.h. - */ -#define SINGLETON_MANAGER_REGISTRATION(NAME) \ - static constexpr char NAME##_singleton_name[] = #NAME "_singleton"; \ - static Registry::RegisterFactory, \ - Singleton::Registration> \ - NAME##_singleton_registered_; - /** * General registry for implementation factories. The registry is templated by the Base class that a * set of factories conforms to. diff --git a/include/envoy/singleton/instance.h b/include/envoy/singleton/instance.h index 8c1c92f2d0b22..893d8bdb032e4 100644 --- a/include/envoy/singleton/instance.h +++ b/include/envoy/singleton/instance.h @@ -13,5 +13,19 @@ class Instance { typedef std::shared_ptr InstanceSharedPtr; +/** + * Macro used to statically register singletons managed by the singleton manager + * defined in envoy/singleton/manager.h. After the NAME has been registered use the + * SINGLETON_MANAGER_REGISTERED_NAME macro to access the name registered with the + * singleton manager. + */ +#define SINGLETON_MANAGER_REGISTRATION(NAME) \ + static constexpr char NAME##_singleton_name[] = #NAME "_singleton"; \ + static Registry::RegisterFactory, \ + Singleton::Registration> \ + NAME##_singleton_registered_; + +#define SINGLETON_MANAGER_REGISTERED_NAME(NAME) NAME##_singleton_name + } // namespace Singleton } // namespace Envoy diff --git a/source/server/config/network/http_connection_manager.cc b/source/server/config/network/http_connection_manager.cc index 973e3ac1f360e..8dafea7f773ec 100644 --- a/source/server/config/network/http_connection_manager.cc +++ b/source/server/config/network/http_connection_manager.cc @@ -37,14 +37,14 @@ NetworkFilterFactoryCb HttpConnectionManagerFilterConfigFactory::createFilterFac const Json::Object& json_http_connection_manager, FactoryContext& context) { std::shared_ptr date_provider = context.singletonManager().getTyped( - date_provider_singleton_name, [&context] { + SINGLETON_MANAGER_REGISTERED_NAME(date_provider), [&context] { return std::make_shared(context.dispatcher(), context.threadLocal()); }); std::shared_ptr route_config_provider_manager = context.singletonManager().getTyped( - route_config_provider_manager_singleton_name, [&context] { + SINGLETON_MANAGER_REGISTERED_NAME(route_config_provider_manager), [&context] { return std::make_shared( context.runtime(), context.dispatcher(), context.random(), context.localInfo(), context.threadLocal()); From 85addbbccbcb03430490bea3502334f0433aa038 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 16 Aug 2017 15:18:24 -0700 Subject: [PATCH 11/12] comment --- source/server/config/network/http_connection_manager.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/server/config/network/http_connection_manager.cc b/source/server/config/network/http_connection_manager.cc index 8dafea7f773ec..14b849d29eda9 100644 --- a/source/server/config/network/http_connection_manager.cc +++ b/source/server/config/network/http_connection_manager.cc @@ -29,7 +29,7 @@ namespace Configuration { const std::string HttpConnectionManagerConfig::DEFAULT_SERVER_STRING = "envoy"; -// Singleton registration via macro defined in envoy/registry/registry.h +// Singleton registration via macro defined in envoy/singleton/instance.h SINGLETON_MANAGER_REGISTRATION(date_provider); SINGLETON_MANAGER_REGISTRATION(route_config_provider_manager); From 295cc0b7a3a4c3e6ca35cbe59861a23b8eb0d2fe Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 16 Aug 2017 16:18:15 -0700 Subject: [PATCH 12/12] move macro --- include/envoy/singleton/instance.h | 14 -------------- include/envoy/singleton/manager.h | 14 ++++++++++++++ .../config/network/http_connection_manager.cc | 2 +- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/include/envoy/singleton/instance.h b/include/envoy/singleton/instance.h index 893d8bdb032e4..8c1c92f2d0b22 100644 --- a/include/envoy/singleton/instance.h +++ b/include/envoy/singleton/instance.h @@ -13,19 +13,5 @@ class Instance { typedef std::shared_ptr InstanceSharedPtr; -/** - * Macro used to statically register singletons managed by the singleton manager - * defined in envoy/singleton/manager.h. After the NAME has been registered use the - * SINGLETON_MANAGER_REGISTERED_NAME macro to access the name registered with the - * singleton manager. - */ -#define SINGLETON_MANAGER_REGISTRATION(NAME) \ - static constexpr char NAME##_singleton_name[] = #NAME "_singleton"; \ - static Registry::RegisterFactory, \ - Singleton::Registration> \ - NAME##_singleton_registered_; - -#define SINGLETON_MANAGER_REGISTERED_NAME(NAME) NAME##_singleton_name - } // namespace Singleton } // namespace Envoy diff --git a/include/envoy/singleton/manager.h b/include/envoy/singleton/manager.h index 276af3d9fde30..f9bd23a3f2658 100644 --- a/include/envoy/singleton/manager.h +++ b/include/envoy/singleton/manager.h @@ -36,6 +36,20 @@ template class RegistrationImpl : public Registration { std::string name() override { return name_param; } }; +/** + * Macro used to statically register singletons managed by the singleton manager + * defined in envoy/singleton/manager.h. After the NAME has been registered use the + * SINGLETON_MANAGER_REGISTERED_NAME macro to access the name registered with the + * singleton manager. + */ +#define SINGLETON_MANAGER_REGISTRATION(NAME) \ + static constexpr char NAME##_singleton_name[] = #NAME "_singleton"; \ + static Registry::RegisterFactory, \ + Singleton::Registration> \ + NAME##_singleton_registered_; + +#define SINGLETON_MANAGER_REGISTERED_NAME(NAME) NAME##_singleton_name + /** * Callback function used to create a singleton. */ diff --git a/source/server/config/network/http_connection_manager.cc b/source/server/config/network/http_connection_manager.cc index 14b849d29eda9..a08d4640d23ee 100644 --- a/source/server/config/network/http_connection_manager.cc +++ b/source/server/config/network/http_connection_manager.cc @@ -29,7 +29,7 @@ namespace Configuration { const std::string HttpConnectionManagerConfig::DEFAULT_SERVER_STRING = "envoy"; -// Singleton registration via macro defined in envoy/singleton/instance.h +// Singleton registration via macro defined in envoy/singleton/manager.h SINGLETON_MANAGER_REGISTRATION(date_provider); SINGLETON_MANAGER_REGISTRATION(route_config_provider_manager);