From 485f5d348f3cad4a1727e8333bb330ab1b01179b Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Wed, 17 May 2017 13:15:55 -0400 Subject: [PATCH 1/8] Added static registration for http tracers. (#967) --- source/exe/BUILD | 2 + source/server/config/http/BUILD | 22 +++++++ .../config/http/lightstep_http_tracer.cc | 43 ++++++++++++++ .../config/http/lightstep_http_tracer.h | 27 +++++++++ .../server/config/http/zipkin_http_tracer.cc | 37 ++++++++++++ .../server/config/http/zipkin_http_tracer.h | 27 +++++++++ source/server/configuration_impl.cc | 38 ++++-------- source/server/configuration_impl.h | 34 +++++++++++ test/integration/BUILD | 2 + test/server/config/http/BUILD | 2 + test/server/config/http/config_test.cc | 59 +++++++++++++++++++ 11 files changed, 268 insertions(+), 25 deletions(-) create mode 100644 source/server/config/http/lightstep_http_tracer.cc create mode 100644 source/server/config/http/lightstep_http_tracer.h create mode 100644 source/server/config/http/zipkin_http_tracer.cc create mode 100644 source/server/config/http/zipkin_http_tracer.h diff --git a/source/exe/BUILD b/source/exe/BUILD index 192c7ca8fd581..ed61583b45f29 100644 --- a/source/exe/BUILD +++ b/source/exe/BUILD @@ -35,8 +35,10 @@ envoy_cc_library( "//source/server/config/http:dynamo_lib", "//source/server/config/http:fault_lib", "//source/server/config/http:grpc_http1_bridge_lib", + "//source/server/config/http:lightstep_lib", "//source/server/config/http:ratelimit_lib", "//source/server/config/http:router_lib", + "//source/server/config/http:zipkin_lib", "//source/server/config/network:client_ssl_auth_lib", "//source/server/config/network:echo_lib", "//source/server/config/network:http_connection_manager_lib", diff --git a/source/server/config/http/BUILD b/source/server/config/http/BUILD index 49800b12c3492..2aad7bb20b21b 100644 --- a/source/server/config/http/BUILD +++ b/source/server/config/http/BUILD @@ -53,6 +53,17 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "lightstep_lib", + srcs = ["lightstep_http_tracer.cc"], + hdrs = ["lightstep_http_tracer.h"], + deps = [ + "//include/envoy/server:instance_interface", + "//source/common/tracing:http_tracer_lib", + "//source/server:configuration_lib", + ], +) + envoy_cc_library( name = "ratelimit_lib", srcs = ["ratelimit.cc"], @@ -77,3 +88,14 @@ envoy_cc_library( "//source/server/config/network:http_connection_manager_lib", ], ) + +envoy_cc_library( + name = "zipkin_lib", + srcs = ["zipkin_http_tracer.cc"], + hdrs = ["zipkin_http_tracer.h"], + deps = [ + "//include/envoy/server:instance_interface", + "//source/common/tracing/zipkin:zipkin_lib", + "//source/server:configuration_lib", + ], +) diff --git a/source/server/config/http/lightstep_http_tracer.cc b/source/server/config/http/lightstep_http_tracer.cc new file mode 100644 index 0000000000000..0641f1e6f2eb2 --- /dev/null +++ b/source/server/config/http/lightstep_http_tracer.cc @@ -0,0 +1,43 @@ +#include "server/config/http/lightstep_http_tracer.h" + +#include + +#include "common/common/utility.h" +#include "common/tracing/http_tracer_impl.h" +#include "common/tracing/lightstep_tracer_impl.h" + +namespace Envoy { +namespace Server { +namespace Configuration { + +Tracing::HttpTracerPtr LightstepHttpTracerFactory::tryCreateHttpTracer( + const std::string& type, const Json::Object& json_config, Server::Instance& server, + Upstream::ClusterManager& cluster_manager) { + if (type != "lightstep") { + return nullptr; + } + + Envoy::Runtime::RandomGenerator& rand = server.random(); + + std::unique_ptr opts(new lightstep::TracerOptions()); + opts->access_token = server.api().fileReadToEnd(json_config.getString("access_token_file")); + StringUtil::rtrim(opts->access_token); + + opts->tracer_attributes["lightstep.component_name"] = server.localInfo().clusterName(); + opts->guid_generator = [&rand]() { return rand.random(); }; + + Tracing::DriverPtr lightstep_driver( + new Tracing::LightStepDriver(json_config, cluster_manager, server.stats(), + server.threadLocal(), server.runtime(), std::move(opts))); + return Tracing::HttpTracerPtr( + new Tracing::HttpTracerImpl(std::move(lightstep_driver), server.localInfo())); +} + +/** + * Static registration for the lightstep http tracer. @see RegisterHttpTracerFactory. + */ +static RegisterHttpTracerFactory register_; + +} // Configuration +} // Server +} // Envoy diff --git a/source/server/config/http/lightstep_http_tracer.h b/source/server/config/http/lightstep_http_tracer.h new file mode 100644 index 0000000000000..de1445db94939 --- /dev/null +++ b/source/server/config/http/lightstep_http_tracer.h @@ -0,0 +1,27 @@ +#pragma once + +#include + +#include "envoy/server/instance.h" + +#include "server/configuration_impl.h" + +namespace Envoy { +namespace Server { +namespace Configuration { + +/** + * Config registration for the lightstep tracer. @see HttpTracerFactory. + */ +class LightstepHttpTracerFactory : public HttpTracerFactory { +public: + // HttpTracerFactory + Tracing::HttpTracerPtr tryCreateHttpTracer(const std::string& type, + const Json::Object& json_config, + Server::Instance& server, + Upstream::ClusterManager& cluster_manager); +}; + +} // Configuration +} // Server +} // Envoy diff --git a/source/server/config/http/zipkin_http_tracer.cc b/source/server/config/http/zipkin_http_tracer.cc new file mode 100644 index 0000000000000..933ddec897fcf --- /dev/null +++ b/source/server/config/http/zipkin_http_tracer.cc @@ -0,0 +1,37 @@ +#include "server/config/http/zipkin_http_tracer.h" + +#include + +#include "common/common/utility.h" +#include "common/tracing/http_tracer_impl.h" +#include "common/tracing/zipkin/zipkin_tracer_impl.h" + +namespace Envoy { +namespace Server { +namespace Configuration { + +Tracing::HttpTracerPtr ZipkinHttpTracerFactory::tryCreateHttpTracer( + const std::string& type, const Json::Object& json_config, Server::Instance& server, + Upstream::ClusterManager& cluster_manager) { + if (type != "zipkin") { + return nullptr; + } + + Envoy::Runtime::RandomGenerator& rand = server.random(); + + Tracing::DriverPtr zipkin_driver(new Zipkin::Driver(json_config, cluster_manager, server.stats(), + server.threadLocal(), server.runtime(), + server.localInfo(), rand)); + + return Tracing::HttpTracerPtr( + new Tracing::HttpTracerImpl(std::move(zipkin_driver), server.localInfo())); +} + +/** + * Static registration for the lightstep http tracer. @see RegisterHttpTracerFactory. + */ +static RegisterHttpTracerFactory register_; + +} // Configuration +} // Server +} // Envoy diff --git a/source/server/config/http/zipkin_http_tracer.h b/source/server/config/http/zipkin_http_tracer.h new file mode 100644 index 0000000000000..ff7b2a80fd0b6 --- /dev/null +++ b/source/server/config/http/zipkin_http_tracer.h @@ -0,0 +1,27 @@ +#pragma once + +#include + +#include "envoy/server/instance.h" + +#include "server/configuration_impl.h" + +namespace Envoy { +namespace Server { +namespace Configuration { + +/** + * Config registration for the zipkin http tracer. @see HttpTracerFactory. + */ +class ZipkinHttpTracerFactory : public HttpTracerFactory { +public: + // HttpTracerFactory + Tracing::HttpTracerPtr tryCreateHttpTracer(const std::string& type, + const Json::Object& json_config, + Server::Instance& server, + Upstream::ClusterManager& cluster_manager); +}; + +} // Configuration +} // Server +} // Envoy \ No newline at end of file diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index d98f8e605734d..51e68fffcc9ce 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -116,32 +116,20 @@ void MainImpl::initializeTracers(const Json::Object& configuration) { std::string type = driver->getString("type"); log().info(fmt::format(" loading tracing driver: {}", type)); - Envoy::Runtime::RandomGenerator& rand = server_.random(); - - if (type == "lightstep") { - Json::ObjectSharedPtr lightstep_config = driver->getObject("config"); - - std::unique_ptr opts(new lightstep::TracerOptions()); - opts->access_token = - server_.api().fileReadToEnd(lightstep_config->getString("access_token_file")); - StringUtil::rtrim(opts->access_token); - - opts->tracer_attributes["lightstep.component_name"] = server_.localInfo().clusterName(); - opts->guid_generator = [&rand]() { return rand.random(); }; - - Tracing::DriverPtr lightstep_driver( - new Tracing::LightStepDriver(*lightstep_config, *cluster_manager_, server_.stats(), - server_.threadLocal(), server_.runtime(), std::move(opts))); - http_tracer_.reset( - new Tracing::HttpTracerImpl(std::move(lightstep_driver), server_.localInfo())); - } else { - ASSERT(type == "zipkin"); - - Tracing::DriverPtr zipkin_driver( - new Zipkin::Driver(*driver->getObject("config"), *cluster_manager_, server_.stats(), - server_.threadLocal(), server_.runtime(), server_.localInfo(), rand)); + Json::ObjectSharedPtr driver_config = driver->getObject("config"); + + bool found_tracer = false; + for (HttpTracerFactory* http_tracer_factory : httpTracerFactories()) { + Tracing::HttpTracerPtr http_tracer = + http_tracer_factory->tryCreateHttpTracer(type, *driver_config, server_, *cluster_manager_); + if (http_tracer) { + http_tracer_ = std::move(http_tracer); + found_tracer = true; + } + } - http_tracer_.reset(new Tracing::HttpTracerImpl(std::move(zipkin_driver), server_.localInfo())); + if (!found_tracer) { + throw EnvoyException(fmt::format("unable to create tracer for type {}", type)); } } diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index b339dda5f2895..3da0b5c57f930 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -11,6 +11,7 @@ #include "envoy/network/filter.h" #include "envoy/server/configuration.h" #include "envoy/server/instance.h" +#include "envoy/tracing/http_tracer.h" #include "common/common/logger.h" #include "common/json/json_loader.h" @@ -43,6 +44,19 @@ class NetworkFilterConfigFactory { Server::Instance& server) PURE; }; +/** + * Implemented by each network filter and registered via registerHttpTracerFactory() or + * the convenience class RegisterHttpTracerFactory. + */ +class HttpTracerFactory { +public: + virtual ~HttpTracerFactory() {} + + virtual Tracing::HttpTracerPtr + tryCreateHttpTracer(const std::string& type, const Json::Object& json_config, + Server::Instance& server, Upstream::ClusterManager& cluster_manager) PURE; +}; + /** * Utilities for creating a filter chain for a network connection. */ @@ -67,6 +81,10 @@ class MainImpl : Logger::Loggable, public Main { filterConfigFactories().push_back(&factory); } + static void registerHttpTracerFactory(HttpTracerFactory& factory) { + httpTracerFactories().push_back(&factory); + } + /** * Initialize the configuration. This happens here vs. the constructor because the initialization * will call through the server to get the cluster manager so the server variable must be @@ -135,6 +153,11 @@ class MainImpl : Logger::Loggable, public Main { return filter_config_factories; } + static std::list& httpTracerFactories() { + static std::list http_tracer_factories; + return http_tracer_factories; + } + Server::Instance& server_; std::unique_ptr cluster_manager_factory_; std::unique_ptr cluster_manager_; @@ -161,6 +184,17 @@ template class RegisterNetworkFilterConfigFactory { } }; +/** + * @see HttpTracerFactory. + */ +template class RegisterHttpTracerFactory { +public: + RegisterHttpTracerFactory() { + static T instance; + MainImpl::registerHttpTracerFactory(instance); + } +}; + /** * Initial configuration that reads from JSON. */ diff --git a/test/integration/BUILD b/test/integration/BUILD index d33c5e2510d86..9ba65c1280352 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -128,8 +128,10 @@ envoy_cc_test_library( "//source/server/config/http:dynamo_lib", "//source/server/config/http:fault_lib", "//source/server/config/http:grpc_http1_bridge_lib", + "//source/server/config/http:lightstep_lib", "//source/server/config/http:ratelimit_lib", "//source/server/config/http:router_lib", + "//source/server/config/http:zipkin_lib", "//source/server/config/network:client_ssl_auth_lib", "//source/server/config/network:echo_lib", "//source/server/config/network:http_connection_manager_lib", diff --git a/test/server/config/http/BUILD b/test/server/config/http/BUILD index 373c8530f4379..c2d31325abb9a 100644 --- a/test/server/config/http/BUILD +++ b/test/server/config/http/BUILD @@ -16,8 +16,10 @@ envoy_cc_test( "//source/server/config/http:dynamo_lib", "//source/server/config/http:fault_lib", "//source/server/config/http:grpc_http1_bridge_lib", + "//source/server/config/http:lightstep_lib", "//source/server/config/http:ratelimit_lib", "//source/server/config/http:router_lib", + "//source/server/config/http:zipkin_lib", "//source/server/http:health_check_lib", "//test/mocks/server:server_mocks", ], diff --git a/test/server/config/http/config_test.cc b/test/server/config/http/config_test.cc index 42f8e3bb39b04..29c5a736afd54 100644 --- a/test/server/config/http/config_test.cc +++ b/test/server/config/http/config_test.cc @@ -4,8 +4,10 @@ #include "server/config/http/dynamo.h" #include "server/config/http/fault.h" #include "server/config/http/grpc_http1_bridge.h" +#include "server/config/http/lightstep_http_tracer.h" #include "server/config/http/ratelimit.h" #include "server/config/http/router.h" +#include "server/config/http/zipkin_http_tracer.h" #include "server/http/health_check.h" #include "test/mocks/server/mocks.h" @@ -16,6 +18,7 @@ namespace Envoy { using testing::_; using testing::NiceMock; +using testing::Return; namespace Server { namespace Configuration { @@ -175,6 +178,62 @@ TEST(HttpFilterConfigTest, BadRouterFilterConfig) { Json::Exception); } +TEST(HttpTracerConfigTest, LightstepHttpTracer) { + NiceMock cm; + EXPECT_CALL(cm, get("fake_cluster")).WillRepeatedly(Return(&cm.thread_local_cluster_)); + ON_CALL(*cm.thread_local_cluster_.cluster_.info_, features()) + .WillByDefault(Return(Upstream::ClusterInfo::Features::HTTP2)); + + std::string valid_config = R"EOF( + { + "collector_cluster": "fake_cluster", + "access_token_file": "fake_file" + } + )EOF"; + Json::ObjectSharedPtr valid_json = Json::Factory::loadFromString(valid_config); + NiceMock server; + LightstepHttpTracerFactory factory; + Tracing::HttpTracerPtr lightstep_tracer = + factory.tryCreateHttpTracer("lightstep", *valid_json, server, cm); + EXPECT_NE(nullptr, lightstep_tracer); +} + +TEST(HttpTracerConfigTest, ZipkinHttpTracer) { + NiceMock cm; + EXPECT_CALL(cm, get("fake_cluster")).WillRepeatedly(Return(&cm.thread_local_cluster_)); + + std::string valid_config = R"EOF( + { + "collector_cluster": "fake_cluster", + "collector_endpoint": "/api/v1/spans" + } + )EOF"; + Json::ObjectSharedPtr valid_json = Json::Factory::loadFromString(valid_config); + NiceMock server; + ZipkinHttpTracerFactory factory; + Tracing::HttpTracerPtr zipkin_tracer = + factory.tryCreateHttpTracer("zipkin", *valid_json, server, cm); + EXPECT_NE(nullptr, zipkin_tracer); +} + +TEST(HttpTracerConfigTest, InvalidHttpTracer) { + NiceMock cm; + + std::string valid_config = R"EOF( + {"collector_cluster": "fake_cluster"} + )EOF"; + Json::ObjectSharedPtr valid_json = Json::Factory::loadFromString(valid_config); + NiceMock server; + LightstepHttpTracerFactory lightstep_factory; + ZipkinHttpTracerFactory zipkin_factory; + std::string tracer_type = "invalid"; + Tracing::HttpTracerPtr tracer = + lightstep_factory.tryCreateHttpTracer(tracer_type, *valid_json, server, cm); + EXPECT_EQ(nullptr, tracer); + tracer = zipkin_factory.tryCreateHttpTracer(tracer_type, *valid_json, server, cm); + EXPECT_EQ(nullptr, tracer); +} + } // Configuration } // Server } // Envoy From 597a76f6ddaae9fea4cc6b407d5227a91fb7741c Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Thu, 18 May 2017 17:25:20 -0400 Subject: [PATCH 2/8] Addresses review comments, PTAL. --- source/server/config/http/lightstep_http_tracer.cc | 3 +++ source/server/configuration_impl.cc | 1 + source/server/configuration_impl.h | 8 +++++++- 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/source/server/config/http/lightstep_http_tracer.cc b/source/server/config/http/lightstep_http_tracer.cc index 0641f1e6f2eb2..a1215c41f244c 100644 --- a/source/server/config/http/lightstep_http_tracer.cc +++ b/source/server/config/http/lightstep_http_tracer.cc @@ -6,6 +6,9 @@ #include "common/tracing/http_tracer_impl.h" #include "common/tracing/lightstep_tracer_impl.h" +#include "lightstep/options.h" +#include "lightstep/tracer.h" + namespace Envoy { namespace Server { namespace Configuration { diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index 51e68fffcc9ce..ef2fcd74365a3 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -125,6 +125,7 @@ void MainImpl::initializeTracers(const Json::Object& configuration) { if (http_tracer) { http_tracer_ = std::move(http_tracer); found_tracer = true; + break; } } diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index 3da0b5c57f930..e5cc7cadef265 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -45,13 +45,19 @@ class NetworkFilterConfigFactory { }; /** - * Implemented by each network filter and registered via registerHttpTracerFactory() or + * Implemented by each HttpTracer and registered via registerHttpTracerFactory() or * the convenience class RegisterHttpTracerFactory. */ class HttpTracerFactory { public: virtual ~HttpTracerFactory() {} + /** + * Attempts to create a particular HttpTracer implementation corresponding to the type of factory. + * If the type argument doesn't match the expected value for the factory, a nullptr will be + * returned. However, if the type matches and the HttpTracer instantiation throws an exception, + * that exception will be propagated. + */ virtual Tracing::HttpTracerPtr tryCreateHttpTracer(const std::string& type, const Json::Object& json_config, Server::Instance& server, Upstream::ClusterManager& cluster_manager) PURE; From 4963472ea5b812dffa9880d63edd8ed10f07d603 Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Fri, 19 May 2017 10:26:26 -0400 Subject: [PATCH 3/8] Corrected static initialization. --- source/server/configuration_impl.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index e5cc7cadef265..9da786d38172c 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -155,13 +155,13 @@ class MainImpl : Logger::Loggable, public Main { }; static std::list& filterConfigFactories() { - static std::list filter_config_factories; - return filter_config_factories; + static std::list *filter_config_factories = new std::list; + return *filter_config_factories; } static std::list& httpTracerFactories() { - static std::list http_tracer_factories; - return http_tracer_factories; + static std::list *http_tracer_factories = new std::list; + return *http_tracer_factories; } Server::Instance& server_; @@ -185,8 +185,8 @@ class MainImpl : Logger::Loggable, public Main { template class RegisterNetworkFilterConfigFactory { public: RegisterNetworkFilterConfigFactory() { - static T instance; - MainImpl::registerNetworkFilterConfigFactory(instance); + static T *instance = new T; + MainImpl::registerNetworkFilterConfigFactory(*instance); } }; @@ -196,8 +196,8 @@ template class RegisterNetworkFilterConfigFactory { template class RegisterHttpTracerFactory { public: RegisterHttpTracerFactory() { - static T instance; - MainImpl::registerHttpTracerFactory(instance); + static T *instance = new T; + MainImpl::registerHttpTracerFactory(*instance); } }; From 402a4e9d0ffc4bcc0becc330fa181e80739312c5 Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Fri, 19 May 2017 11:10:50 -0400 Subject: [PATCH 4/8] Corrected formatting errors. --- source/server/configuration_impl.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index 9da786d38172c..87eeeae749c13 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -155,12 +155,13 @@ class MainImpl : Logger::Loggable, public Main { }; static std::list& filterConfigFactories() { - static std::list *filter_config_factories = new std::list; + static std::list* filter_config_factories = + new std::list; return *filter_config_factories; } static std::list& httpTracerFactories() { - static std::list *http_tracer_factories = new std::list; + static std::list* http_tracer_factories = new std::list; return *http_tracer_factories; } @@ -185,7 +186,7 @@ class MainImpl : Logger::Loggable, public Main { template class RegisterNetworkFilterConfigFactory { public: RegisterNetworkFilterConfigFactory() { - static T *instance = new T; + static T* instance = new T; MainImpl::registerNetworkFilterConfigFactory(*instance); } }; @@ -196,7 +197,7 @@ template class RegisterNetworkFilterConfigFactory { template class RegisterHttpTracerFactory { public: RegisterHttpTracerFactory() { - static T *instance = new T; + static T* instance = new T; MainImpl::registerHttpTracerFactory(*instance); } }; From 7f59e9b66baa24f4edeebccc65c4e883ec03eb3b Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Fri, 19 May 2017 17:30:31 -0400 Subject: [PATCH 5/8] Modified static registration to use unordered_map --- source/server/config/http/buffer.cc | 16 +++-- source/server/config/http/buffer.h | 8 +-- source/server/config/http/dynamo.cc | 16 +++-- source/server/config/http/dynamo.h | 7 +- source/server/config/http/fault.cc | 16 +++-- source/server/config/http/fault.h | 8 +-- .../server/config/http/grpc_http1_bridge.cc | 16 +++-- source/server/config/http/grpc_http1_bridge.h | 6 +- .../config/http/lightstep_http_tracer.cc | 12 ++-- .../config/http/lightstep_http_tracer.h | 8 +-- source/server/config/http/ratelimit.cc | 16 +++-- source/server/config/http/ratelimit.h | 6 +- source/server/config/http/router.cc | 16 +++-- source/server/config/http/router.h | 8 +-- .../server/config/http/zipkin_http_tracer.cc | 11 ++- .../server/config/http/zipkin_http_tracer.h | 7 +- .../server/config/network/client_ssl_auth.cc | 13 ++-- .../server/config/network/client_ssl_auth.h | 7 +- source/server/config/network/echo.cc | 11 +-- .../config/network/http_connection_manager.cc | 30 +++----- .../config/network/http_connection_manager.h | 45 ++++++++---- source/server/config/network/mongo_proxy.cc | 12 ++-- source/server/config/network/mongo_proxy.h | 7 +- source/server/config/network/ratelimit.cc | 14 ++-- source/server/config/network/ratelimit.h | 8 ++- source/server/config/network/redis_proxy.cc | 12 ++-- source/server/config/network/redis_proxy.h | 6 +- source/server/config/network/tcp_proxy.cc | 14 ++-- source/server/config/network/tcp_proxy.h | 6 +- source/server/configuration_impl.cc | 37 +++------- source/server/configuration_impl.h | 68 ++++++++++++++----- source/server/http/health_check.cc | 16 +++-- source/server/http/health_check.h | 6 +- test/server/config/http/config_test.cc | 61 +++++++---------- test/server/config/network/config_test.cc | 37 +++++----- test/server/http/health_check_test.cc | 13 ++-- 36 files changed, 317 insertions(+), 283 deletions(-) diff --git a/source/server/config/http/buffer.cc b/source/server/config/http/buffer.cc index 9c777e46f40db..4429b40082e6c 100644 --- a/source/server/config/http/buffer.cc +++ b/source/server/config/http/buffer.cc @@ -11,13 +11,13 @@ namespace Envoy { namespace Server { namespace Configuration { -HttpFilterFactoryCb BufferFilterConfig::tryCreateFilterFactory(HttpFilterType type, - const std::string& name, - const Json::Object& json_config, - const std::string& stats_prefix, - Server::Instance& server) { - if (type != HttpFilterType::Decoder || name != "buffer") { - return nullptr; +HttpFilterFactoryCb BufferFilterConfig::createFilterFactory(HttpFilterType type, + const Json::Object& json_config, + const std::string& stats_prefix, + Server::Instance& server) { + if (type != HttpFilterType::Decoder) { + throw EnvoyException( + fmt::format("{} http filter must be configured as a decoder filter.", name())); } json_config.validateSchema(Json::Schema::BUFFER_HTTP_FILTER_SCHEMA); @@ -32,6 +32,8 @@ HttpFilterFactoryCb BufferFilterConfig::tryCreateFilterFactory(HttpFilterType ty }; } +std::string BufferFilterConfig::name() { return "buffer"; } + /** * Static registration for the buffer filter. @see RegisterHttpFilterConfigFactory. */ diff --git a/source/server/config/http/buffer.h b/source/server/config/http/buffer.h index bbfa2ee2dc33e..590de549b8e19 100644 --- a/source/server/config/http/buffer.h +++ b/source/server/config/http/buffer.h @@ -15,10 +15,10 @@ namespace Configuration { */ class BufferFilterConfig : public HttpFilterConfigFactory { public: - HttpFilterFactoryCb tryCreateFilterFactory(HttpFilterType type, const std::string& name, - const Json::Object& json_config, - const std::string& stats_prefix, - Server::Instance& server) override; + HttpFilterFactoryCb createFilterFactory(HttpFilterType type, const Json::Object& json_config, + const std::string& stats_prefix, + Server::Instance& server) override; + std::string name() override; }; } // Configuration diff --git a/source/server/config/http/dynamo.cc b/source/server/config/http/dynamo.cc index 843fffcf4bd97..6d16bce3add75 100644 --- a/source/server/config/http/dynamo.cc +++ b/source/server/config/http/dynamo.cc @@ -8,13 +8,13 @@ namespace Envoy { namespace Server { namespace Configuration { -HttpFilterFactoryCb DynamoFilterConfig::tryCreateFilterFactory(HttpFilterType type, - const std::string& name, - const Json::Object&, - const std::string& stat_prefix, - Server::Instance& server) { - if (type != HttpFilterType::Both || name != "http_dynamo_filter") { - return nullptr; +HttpFilterFactoryCb DynamoFilterConfig::createFilterFactory(HttpFilterType type, + const Json::Object&, + const std::string& stat_prefix, + Server::Instance& server) { + if (type != HttpFilterType::Both) { + throw EnvoyException(fmt::format( + "{} http filter must be configured as both a decoder and encoder filter.", name())); } return [&server, stat_prefix](Http::FilterChainFactoryCallbacks& callbacks) -> void { @@ -23,6 +23,8 @@ HttpFilterFactoryCb DynamoFilterConfig::tryCreateFilterFactory(HttpFilterType ty }; } +std::string DynamoFilterConfig::name() { return "http_dynamo_filter"; } + /** * Static registration for the http dynamodb filter. @see RegisterHttpFilterConfigFactory. */ diff --git a/source/server/config/http/dynamo.h b/source/server/config/http/dynamo.h index cfaa9f36360b5..50efda21affcc 100644 --- a/source/server/config/http/dynamo.h +++ b/source/server/config/http/dynamo.h @@ -13,9 +13,10 @@ namespace Configuration { */ class DynamoFilterConfig : public HttpFilterConfigFactory { public: - HttpFilterFactoryCb tryCreateFilterFactory(HttpFilterType type, const std::string& name, - const Json::Object&, const std::string& stat_prefix, - Server::Instance& server) override; + HttpFilterFactoryCb createFilterFactory(HttpFilterType type, const Json::Object&, + const std::string& stat_prefix, + Server::Instance& server) override; + std::string name() override; }; } // Configuration diff --git a/source/server/config/http/fault.cc b/source/server/config/http/fault.cc index 69c7402faf29a..c2ed13b0f61d0 100644 --- a/source/server/config/http/fault.cc +++ b/source/server/config/http/fault.cc @@ -9,13 +9,13 @@ namespace Envoy { namespace Server { namespace Configuration { -HttpFilterFactoryCb FaultFilterConfig::tryCreateFilterFactory(HttpFilterType type, - const std::string& name, - const Json::Object& json_config, - const std::string& stats_prefix, - Server::Instance& server) { - if (type != HttpFilterType::Decoder || name != "fault") { - return nullptr; +HttpFilterFactoryCb FaultFilterConfig::createFilterFactory(HttpFilterType type, + const Json::Object& json_config, + const std::string& stats_prefix, + Server::Instance& server) { + if (type != HttpFilterType::Decoder) { + throw EnvoyException( + fmt::format("{} http filter must be configured as a decoder filter.", name())); } Http::FaultFilterConfigSharedPtr config( @@ -26,6 +26,8 @@ HttpFilterFactoryCb FaultFilterConfig::tryCreateFilterFactory(HttpFilterType typ }; } +std::string FaultFilterConfig::name() { return "fault"; } + /** * Static registration for the fault filter. @see RegisterHttpFilterConfigFactory. */ diff --git a/source/server/config/http/fault.h b/source/server/config/http/fault.h index 530e13aafd88f..2c16d3ee7b8f8 100644 --- a/source/server/config/http/fault.h +++ b/source/server/config/http/fault.h @@ -15,10 +15,10 @@ namespace Configuration { */ class FaultFilterConfig : public HttpFilterConfigFactory { public: - HttpFilterFactoryCb tryCreateFilterFactory(HttpFilterType type, const std::string& name, - const Json::Object& json_config, - const std::string& stats_prefix, - Server::Instance& server) override; + HttpFilterFactoryCb createFilterFactory(HttpFilterType type, const Json::Object& json_config, + const std::string& stats_prefix, + Server::Instance& server) override; + std::string name() override; }; } // Configuration diff --git a/source/server/config/http/grpc_http1_bridge.cc b/source/server/config/http/grpc_http1_bridge.cc index 079c4f79c8360..0e1831502dd31 100644 --- a/source/server/config/http/grpc_http1_bridge.cc +++ b/source/server/config/http/grpc_http1_bridge.cc @@ -8,13 +8,13 @@ namespace Envoy { namespace Server { namespace Configuration { -HttpFilterFactoryCb GrpcHttp1BridgeFilterConfig::tryCreateFilterFactory(HttpFilterType type, - const std::string& name, - const Json::Object&, - const std::string&, - Server::Instance& server) { - if (type != HttpFilterType::Both || name != "grpc_http1_bridge") { - return nullptr; +HttpFilterFactoryCb GrpcHttp1BridgeFilterConfig::createFilterFactory(HttpFilterType type, + const Json::Object&, + const std::string&, + Server::Instance& server) { + if (type != HttpFilterType::Both) { + throw EnvoyException(fmt::format( + "{} http filter must be configured as both a decoder and encoder filter.", name())); } return [&server](Http::FilterChainFactoryCallbacks& callbacks) -> void { @@ -23,6 +23,8 @@ HttpFilterFactoryCb GrpcHttp1BridgeFilterConfig::tryCreateFilterFactory(HttpFilt }; } +std::string GrpcHttp1BridgeFilterConfig::name() { return "grpc_http1_bridge"; } + /** * Static registration for the grpc HTTP1 bridge filter. @see RegisterHttpFilterConfigFactory. */ diff --git a/source/server/config/http/grpc_http1_bridge.h b/source/server/config/http/grpc_http1_bridge.h index fef25d1e57b19..31219d2a419e1 100644 --- a/source/server/config/http/grpc_http1_bridge.h +++ b/source/server/config/http/grpc_http1_bridge.h @@ -15,9 +15,9 @@ namespace Configuration { */ class GrpcHttp1BridgeFilterConfig : public HttpFilterConfigFactory { public: - HttpFilterFactoryCb tryCreateFilterFactory(HttpFilterType type, const std::string& name, - const Json::Object&, const std::string&, - Server::Instance& server) override; + HttpFilterFactoryCb createFilterFactory(HttpFilterType type, const Json::Object&, + const std::string&, Server::Instance& server) override; + std::string name() override; }; } // Configuration diff --git a/source/server/config/http/lightstep_http_tracer.cc b/source/server/config/http/lightstep_http_tracer.cc index a1215c41f244c..980bdeaa20563 100644 --- a/source/server/config/http/lightstep_http_tracer.cc +++ b/source/server/config/http/lightstep_http_tracer.cc @@ -13,12 +13,10 @@ namespace Envoy { namespace Server { namespace Configuration { -Tracing::HttpTracerPtr LightstepHttpTracerFactory::tryCreateHttpTracer( - const std::string& type, const Json::Object& json_config, Server::Instance& server, - Upstream::ClusterManager& cluster_manager) { - if (type != "lightstep") { - return nullptr; - } +Tracing::HttpTracerPtr +LightstepHttpTracerFactory::createHttpTracer(const Json::Object& json_config, + Server::Instance& server, + Upstream::ClusterManager& cluster_manager) { Envoy::Runtime::RandomGenerator& rand = server.random(); @@ -36,6 +34,8 @@ Tracing::HttpTracerPtr LightstepHttpTracerFactory::tryCreateHttpTracer( new Tracing::HttpTracerImpl(std::move(lightstep_driver), server.localInfo())); } +std::string LightstepHttpTracerFactory::name() { return "lightstep"; } + /** * Static registration for the lightstep http tracer. @see RegisterHttpTracerFactory. */ diff --git a/source/server/config/http/lightstep_http_tracer.h b/source/server/config/http/lightstep_http_tracer.h index de1445db94939..721ea74580256 100644 --- a/source/server/config/http/lightstep_http_tracer.h +++ b/source/server/config/http/lightstep_http_tracer.h @@ -16,10 +16,10 @@ namespace Configuration { class LightstepHttpTracerFactory : public HttpTracerFactory { public: // HttpTracerFactory - Tracing::HttpTracerPtr tryCreateHttpTracer(const std::string& type, - const Json::Object& json_config, - Server::Instance& server, - Upstream::ClusterManager& cluster_manager); + Tracing::HttpTracerPtr createHttpTracer(const Json::Object& json_config, Server::Instance& server, + Upstream::ClusterManager& cluster_manager) override; + + std::string name() override; }; } // Configuration diff --git a/source/server/config/http/ratelimit.cc b/source/server/config/http/ratelimit.cc index f2b5ad9541ba4..383ce6f7582a7 100644 --- a/source/server/config/http/ratelimit.cc +++ b/source/server/config/http/ratelimit.cc @@ -9,13 +9,13 @@ namespace Envoy { namespace Server { namespace Configuration { -HttpFilterFactoryCb RateLimitFilterConfig::tryCreateFilterFactory(HttpFilterType type, - const std::string& name, - const Json::Object& config, - const std::string&, - Server::Instance& server) { - if (type != HttpFilterType::Decoder || name != "rate_limit") { - return nullptr; +HttpFilterFactoryCb RateLimitFilterConfig::createFilterFactory(HttpFilterType type, + const Json::Object& config, + const std::string&, + Server::Instance& server) { + if (type != HttpFilterType::Decoder) { + throw EnvoyException( + fmt::format("{} http filter must be configured as a decoder filter.", name())); } Http::RateLimit::FilterConfigSharedPtr filter_config(new Http::RateLimit::FilterConfig( @@ -26,6 +26,8 @@ HttpFilterFactoryCb RateLimitFilterConfig::tryCreateFilterFactory(HttpFilterType }; } +std::string RateLimitFilterConfig::name() { return "rate_limit"; } + /** * Static registration for the rate limit filter. @see RegisterHttpFilterConfigFactory. */ diff --git a/source/server/config/http/ratelimit.h b/source/server/config/http/ratelimit.h index 8988e0d82ef71..9bb89e79c6609 100644 --- a/source/server/config/http/ratelimit.h +++ b/source/server/config/http/ratelimit.h @@ -15,9 +15,9 @@ namespace Configuration { */ class RateLimitFilterConfig : public HttpFilterConfigFactory { public: - HttpFilterFactoryCb tryCreateFilterFactory(HttpFilterType type, const std::string& name, - const Json::Object& config, const std::string&, - Server::Instance& server) override; + HttpFilterFactoryCb createFilterFactory(HttpFilterType type, const Json::Object& config, + const std::string&, Server::Instance& server) override; + std::string name() override; }; } // Configuration diff --git a/source/server/config/http/router.cc b/source/server/config/http/router.cc index 8e7b8332524f3..b97bf9e4c03f8 100644 --- a/source/server/config/http/router.cc +++ b/source/server/config/http/router.cc @@ -10,13 +10,13 @@ namespace Envoy { namespace Server { namespace Configuration { -HttpFilterFactoryCb RouterFilterConfig::tryCreateFilterFactory(HttpFilterType type, - const std::string& name, - const Json::Object& json_config, - const std::string& stat_prefix, - Server::Instance& server) { - if (type != HttpFilterType::Decoder || name != "router") { - return nullptr; +HttpFilterFactoryCb RouterFilterConfig::createFilterFactory(HttpFilterType type, + const Json::Object& json_config, + const std::string& stat_prefix, + Server::Instance& server) { + if (type != HttpFilterType::Decoder) { + throw EnvoyException( + fmt::format("{} http filter must be configured as a decoder filter.", name())); } json_config.validateSchema(Json::Schema::ROUTER_HTTP_FILTER_SCHEMA); @@ -31,6 +31,8 @@ HttpFilterFactoryCb RouterFilterConfig::tryCreateFilterFactory(HttpFilterType ty -> void { callbacks.addStreamDecoderFilter(std::make_shared(*config)); }; } +std::string RouterFilterConfig::name() { return "router"; } + /** * Static registration for the router filter. @see RegisterHttpFilterConfigFactory. */ diff --git a/source/server/config/http/router.h b/source/server/config/http/router.h index b8136c6f8234e..4eb85daf21097 100644 --- a/source/server/config/http/router.h +++ b/source/server/config/http/router.h @@ -15,10 +15,10 @@ namespace Configuration { */ class RouterFilterConfig : public HttpFilterConfigFactory { public: - HttpFilterFactoryCb tryCreateFilterFactory(HttpFilterType type, const std::string& name, - const Json::Object& json_config, - const std::string& stat_prefix, - Server::Instance& server) override; + HttpFilterFactoryCb createFilterFactory(HttpFilterType type, const Json::Object& json_config, + const std::string& stat_prefix, + Server::Instance& server) override; + std::string name() override; }; } // Configuration diff --git a/source/server/config/http/zipkin_http_tracer.cc b/source/server/config/http/zipkin_http_tracer.cc index 933ddec897fcf..ae002740aaae5 100644 --- a/source/server/config/http/zipkin_http_tracer.cc +++ b/source/server/config/http/zipkin_http_tracer.cc @@ -10,12 +10,9 @@ namespace Envoy { namespace Server { namespace Configuration { -Tracing::HttpTracerPtr ZipkinHttpTracerFactory::tryCreateHttpTracer( - const std::string& type, const Json::Object& json_config, Server::Instance& server, - Upstream::ClusterManager& cluster_manager) { - if (type != "zipkin") { - return nullptr; - } +Tracing::HttpTracerPtr +ZipkinHttpTracerFactory::createHttpTracer(const Json::Object& json_config, Server::Instance& server, + Upstream::ClusterManager& cluster_manager) { Envoy::Runtime::RandomGenerator& rand = server.random(); @@ -27,6 +24,8 @@ Tracing::HttpTracerPtr ZipkinHttpTracerFactory::tryCreateHttpTracer( new Tracing::HttpTracerImpl(std::move(zipkin_driver), server.localInfo())); } +std::string ZipkinHttpTracerFactory::name() { return "zipkin"; } + /** * Static registration for the lightstep http tracer. @see RegisterHttpTracerFactory. */ diff --git a/source/server/config/http/zipkin_http_tracer.h b/source/server/config/http/zipkin_http_tracer.h index ff7b2a80fd0b6..fc464f6c99f0d 100644 --- a/source/server/config/http/zipkin_http_tracer.h +++ b/source/server/config/http/zipkin_http_tracer.h @@ -16,10 +16,9 @@ namespace Configuration { class ZipkinHttpTracerFactory : public HttpTracerFactory { public: // HttpTracerFactory - Tracing::HttpTracerPtr tryCreateHttpTracer(const std::string& type, - const Json::Object& json_config, - Server::Instance& server, - Upstream::ClusterManager& cluster_manager); + Tracing::HttpTracerPtr createHttpTracer(const Json::Object& json_config, Server::Instance& server, + Upstream::ClusterManager& cluster_manager) override; + std::string name() override; }; } // Configuration diff --git a/source/server/config/network/client_ssl_auth.cc b/source/server/config/network/client_ssl_auth.cc index 7ead762cdbc6e..f5f1678689d3c 100644 --- a/source/server/config/network/client_ssl_auth.cc +++ b/source/server/config/network/client_ssl_auth.cc @@ -11,12 +11,11 @@ namespace Envoy { namespace Server { namespace Configuration { -NetworkFilterFactoryCb -ClientSslAuthConfigFactory::tryCreateFilterFactory(NetworkFilterType type, const std::string& name, - const Json::Object& json_config, - Server::Instance& server) { - if (type != NetworkFilterType::Read || name != "client_ssl_auth") { - return nullptr; +NetworkFilterFactoryCb ClientSslAuthConfigFactory::createFilterFactory( + NetworkFilterType type, const Json::Object& json_config, Server::Instance& server) { + if (type != NetworkFilterType::Read) { + throw EnvoyException( + fmt::format("{} network filter must be configured as a read filter.", name())); } Filter::Auth::ClientSsl::ConfigSharedPtr config(Filter::Auth::ClientSsl::Config::create( @@ -28,6 +27,8 @@ ClientSslAuthConfigFactory::tryCreateFilterFactory(NetworkFilterType type, const }; } +std::string ClientSslAuthConfigFactory::name() { return "client_ssl_auth"; } + /** * Static registration for the client SSL auth filter. @see RegisterNetworkFilterConfigFactory. */ diff --git a/source/server/config/network/client_ssl_auth.h b/source/server/config/network/client_ssl_auth.h index 56bb6ecf9696b..cc7a8116a0d5f 100644 --- a/source/server/config/network/client_ssl_auth.h +++ b/source/server/config/network/client_ssl_auth.h @@ -14,9 +14,10 @@ namespace Configuration { class ClientSslAuthConfigFactory : public NetworkFilterConfigFactory { public: // NetworkFilterConfigFactory - NetworkFilterFactoryCb tryCreateFilterFactory(NetworkFilterType type, const std::string& name, - const Json::Object& json_config, - Server::Instance& server); + NetworkFilterFactoryCb createFilterFactory(NetworkFilterType type, + const Json::Object& json_config, + Server::Instance& server) override; + std::string name() override; }; } // Configuration diff --git a/source/server/config/network/echo.cc b/source/server/config/network/echo.cc index 63e14de6f9e5c..043263c1c1d15 100644 --- a/source/server/config/network/echo.cc +++ b/source/server/config/network/echo.cc @@ -16,15 +16,18 @@ namespace Configuration { class EchoConfigFactory : public NetworkFilterConfigFactory { public: // NetworkFilterConfigFactory - NetworkFilterFactoryCb tryCreateFilterFactory(NetworkFilterType type, const std::string& name, - const Json::Object&, Server::Instance&) { - if (type != NetworkFilterType::Read || name != "echo") { - return nullptr; + NetworkFilterFactoryCb createFilterFactory(NetworkFilterType type, const Json::Object&, + Server::Instance&) override { + if (type != NetworkFilterType::Read) { + throw EnvoyException( + fmt::format("{} network filter must be configured as a read filter.", name())); } return [](Network::FilterManager& filter_manager) -> void { filter_manager.addReadFilter(Network::ReadFilterSharedPtr{new Filter::Echo()}); }; } + + std::string name() override { return "echo"; } }; /** diff --git a/source/server/config/network/http_connection_manager.cc b/source/server/config/network/http_connection_manager.cc index 252b41b94571c..2add8eb812ebc 100644 --- a/source/server/config/network/http_connection_manager.cc +++ b/source/server/config/network/http_connection_manager.cc @@ -26,11 +26,11 @@ namespace Configuration { const std::string HttpConnectionManagerConfig::DEFAULT_SERVER_STRING = "envoy"; -NetworkFilterFactoryCb HttpConnectionManagerFilterConfigFactory::tryCreateFilterFactory( - NetworkFilterType type, const std::string& name, const Json::Object& config, - Server::Instance& server) { - if (type != NetworkFilterType::Read || name != "http_connection_manager") { - return nullptr; +NetworkFilterFactoryCb HttpConnectionManagerFilterConfigFactory::createFilterFactory( + NetworkFilterType type, const Json::Object& config, Server::Instance& server) { + if (type != NetworkFilterType::Read) { + throw EnvoyException( + fmt::format("{} network filter must be configured as a read filter.", name())); } std::shared_ptr http_config( @@ -42,6 +42,8 @@ NetworkFilterFactoryCb HttpConnectionManagerFilterConfigFactory::tryCreateFilter }; } +std::string HttpConnectionManagerFilterConfigFactory::name() { return "http_connection_manager"; } + /** * Static registration for the HTTP connection manager filter. @see * RegisterNetworkFilterConfigFactory. @@ -152,21 +154,11 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(const Json::Object& con log().info(" name: {}", string_name); HttpFilterType type = stringToType(string_type); - bool found_filter = false; - for (HttpFilterConfigFactory* factory : filterConfigFactories()) { - HttpFilterFactoryCb callback = - factory->tryCreateFilterFactory(type, string_name, *config_object, stats_prefix_, server); - if (callback) { - filter_factories_.push_back(callback); - found_filter = true; - break; - } - } - if (!found_filter) { - throw EnvoyException(fmt::format("unable to create http filter factory for '{}'/'{}'", - string_name, string_type)); - } + HttpFilterConfigFactory* factory = filterConfigFactories().at(string_name); + HttpFilterFactoryCb callback = + factory->createFilterFactory(type, *config_object, stats_prefix_, server); + filter_factories_.push_back(callback); } } diff --git a/source/server/config/network/http_connection_manager.h b/source/server/config/network/http_connection_manager.h index b040329d4d21e..704d76e9fd201 100644 --- a/source/server/config/network/http_connection_manager.h +++ b/source/server/config/network/http_connection_manager.h @@ -30,9 +30,10 @@ class HttpConnectionManagerFilterConfigFactory : Logger::Loggable, const Optional& userAgent() override { return user_agent_; } static void registerHttpFilterConfigFactory(HttpFilterConfigFactory& factory) { - filterConfigFactories().push_back(&factory); + auto result = filterConfigFactories().emplace(std::make_pair(factory.name(), &factory)); + + // result is a pair whose second member is a boolean indicating, if false, that the key exists + // and that the value was not inserted. + if (!result.second) { + throw EnvoyException(fmt::format( + "Attempted to register multiple HttpFilterConfigFactory objects with name: '{}'", + factory.name())); + } } static const std::string DEFAULT_SERVER_STRING; @@ -113,9 +131,10 @@ class HttpConnectionManagerConfig : Logger::Loggable, private: enum class CodecType { HTTP1, HTTP2, AUTO }; - static std::list& filterConfigFactories() { - static std::list filter_config_factories; - return filter_config_factories; + static std::unordered_map& filterConfigFactories() { + static std::unordered_map* filter_config_factories = + new std::unordered_map; + return *filter_config_factories; } HttpFilterType stringToType(const std::string& type); @@ -145,8 +164,8 @@ class HttpConnectionManagerConfig : Logger::Loggable, template class RegisterHttpFilterConfigFactory { public: RegisterHttpFilterConfigFactory() { - static T instance; - HttpConnectionManagerConfig::registerHttpFilterConfigFactory(instance); + static T* instance = new T; + HttpConnectionManagerConfig::registerHttpFilterConfigFactory(*instance); } }; diff --git a/source/server/config/network/mongo_proxy.cc b/source/server/config/network/mongo_proxy.cc index db8462252c035..c592edd3145a8 100644 --- a/source/server/config/network/mongo_proxy.cc +++ b/source/server/config/network/mongo_proxy.cc @@ -12,11 +12,11 @@ namespace Envoy { namespace Server { namespace Configuration { -NetworkFilterFactoryCb MongoProxyFilterConfigFactory::tryCreateFilterFactory( - NetworkFilterType type, const std::string& name, const Json::Object& config, - Server::Instance& server) { - if (type != NetworkFilterType::Both || name != "mongo_proxy") { - return nullptr; +NetworkFilterFactoryCb MongoProxyFilterConfigFactory::createFilterFactory( + NetworkFilterType type, const Json::Object& config, Server::Instance& server) { + if (type != NetworkFilterType::Both) { + throw EnvoyException(fmt::format( + "{} network filter must be configured as both a read and write filter.", name())); } config.validateSchema(Json::Schema::MONGO_PROXY_NETWORK_FILTER_SCHEMA); @@ -34,6 +34,8 @@ NetworkFilterFactoryCb MongoProxyFilterConfigFactory::tryCreateFilterFactory( }; } +std::string MongoProxyFilterConfigFactory::name() { return "mongo_proxy"; } + /** * Static registration for the mongo filter. @see RegisterNetworkFilterConfigFactory. */ diff --git a/source/server/config/network/mongo_proxy.h b/source/server/config/network/mongo_proxy.h index 678ad40e2a82d..4ca42b10e72e9 100644 --- a/source/server/config/network/mongo_proxy.h +++ b/source/server/config/network/mongo_proxy.h @@ -14,9 +14,10 @@ namespace Configuration { class MongoProxyFilterConfigFactory : public NetworkFilterConfigFactory { public: // NetworkFilterConfigFactory - NetworkFilterFactoryCb tryCreateFilterFactory(NetworkFilterType type, const std::string& name, - const Json::Object& config, - Server::Instance& server); + NetworkFilterFactoryCb createFilterFactory(NetworkFilterType type, const Json::Object& config, + Server::Instance& server) override; + + std::string name() override; }; } // Configuration diff --git a/source/server/config/network/ratelimit.cc b/source/server/config/network/ratelimit.cc index 0f46b20ceddef..da8b3c6f10b59 100644 --- a/source/server/config/network/ratelimit.cc +++ b/source/server/config/network/ratelimit.cc @@ -11,12 +11,12 @@ namespace Envoy { namespace Server { namespace Configuration { -NetworkFilterFactoryCb -RateLimitConfigFactory::tryCreateFilterFactory(NetworkFilterType type, const std::string& name, - const Json::Object& json_config, - Server::Instance& server) { - if (type != NetworkFilterType::Read || name != "ratelimit") { - return nullptr; +NetworkFilterFactoryCb RateLimitConfigFactory::createFilterFactory(NetworkFilterType type, + const Json::Object& json_config, + Server::Instance& server) { + if (type != NetworkFilterType::Read) { + throw EnvoyException( + fmt::format("{} network filter must be configured as a read filter.", name())); } RateLimit::TcpFilter::ConfigSharedPtr config( @@ -27,6 +27,8 @@ RateLimitConfigFactory::tryCreateFilterFactory(NetworkFilterType type, const std }; } +std::string RateLimitConfigFactory::name() { return "ratelimit"; } + /** * Static registration for the rate limit filter. @see RegisterNetworkFilterConfigFactory. */ diff --git a/source/server/config/network/ratelimit.h b/source/server/config/network/ratelimit.h index 972b4f346edb4..af19e862a4476 100644 --- a/source/server/config/network/ratelimit.h +++ b/source/server/config/network/ratelimit.h @@ -14,9 +14,11 @@ namespace Configuration { class RateLimitConfigFactory : public NetworkFilterConfigFactory { public: // NetworkFilterConfigFactory - NetworkFilterFactoryCb tryCreateFilterFactory(NetworkFilterType type, const std::string& name, - const Json::Object& json_config, - Server::Instance& server); + NetworkFilterFactoryCb createFilterFactory(NetworkFilterType type, + const Json::Object& json_config, + Server::Instance& server) override; + + std::string name() override; }; } // Configuration diff --git a/source/server/config/network/redis_proxy.cc b/source/server/config/network/redis_proxy.cc index c614298318bbd..5ad671dcd2425 100644 --- a/source/server/config/network/redis_proxy.cc +++ b/source/server/config/network/redis_proxy.cc @@ -12,11 +12,11 @@ namespace Envoy { namespace Server { namespace Configuration { -NetworkFilterFactoryCb RedisProxyFilterConfigFactory::tryCreateFilterFactory( - NetworkFilterType type, const std::string& name, const Json::Object& config, - Server::Instance& server) { - if (type != NetworkFilterType::Read || name != "redis_proxy") { - return nullptr; +NetworkFilterFactoryCb RedisProxyFilterConfigFactory::createFilterFactory( + NetworkFilterType type, const Json::Object& config, Server::Instance& server) { + if (type != NetworkFilterType::Read) { + throw EnvoyException( + fmt::format("{} network filter must be configured as a read filter.", name())); } Redis::ProxyFilterConfigSharedPtr filter_config( @@ -35,6 +35,8 @@ NetworkFilterFactoryCb RedisProxyFilterConfigFactory::tryCreateFilterFactory( }; } +std::string RedisProxyFilterConfigFactory::name() { return "redis_proxy"; } + /** * Static registration for the redis filter. @see RegisterNetworkFilterConfigFactory. */ diff --git a/source/server/config/network/redis_proxy.h b/source/server/config/network/redis_proxy.h index 058da13ec6ad1..d666eed861ebb 100644 --- a/source/server/config/network/redis_proxy.h +++ b/source/server/config/network/redis_proxy.h @@ -14,9 +14,9 @@ namespace Configuration { class RedisProxyFilterConfigFactory : public NetworkFilterConfigFactory { public: // NetworkFilterConfigFactory - NetworkFilterFactoryCb tryCreateFilterFactory(NetworkFilterType type, const std::string& name, - const Json::Object& config, - Server::Instance& server); + NetworkFilterFactoryCb createFilterFactory(NetworkFilterType type, const Json::Object& config, + Server::Instance& server) override; + std::string name() override; }; } // Configuration diff --git a/source/server/config/network/tcp_proxy.cc b/source/server/config/network/tcp_proxy.cc index f1f4afad05132..74e5d1a925acd 100644 --- a/source/server/config/network/tcp_proxy.cc +++ b/source/server/config/network/tcp_proxy.cc @@ -11,12 +11,12 @@ namespace Envoy { namespace Server { namespace Configuration { -NetworkFilterFactoryCb TcpProxyConfigFactory::tryCreateFilterFactory(NetworkFilterType type, - const std::string& name, - const Json::Object& config, - Server::Instance& server) { - if (type != NetworkFilterType::Read || name != "tcp_proxy") { - return nullptr; +NetworkFilterFactoryCb TcpProxyConfigFactory::createFilterFactory(NetworkFilterType type, + const Json::Object& config, + Server::Instance& server) { + if (type != NetworkFilterType::Read) { + throw EnvoyException( + fmt::format("{} network filter must be configured as a read filter.", name())); } Filter::TcpProxyConfigSharedPtr filter_config( @@ -27,6 +27,8 @@ NetworkFilterFactoryCb TcpProxyConfigFactory::tryCreateFilterFactory(NetworkFilt }; } +std::string TcpProxyConfigFactory::name() { return "tcp_proxy"; } + /** * Static registration for the tcp_proxy filter. @see RegisterNetworkFilterConfigFactory. */ diff --git a/source/server/config/network/tcp_proxy.h b/source/server/config/network/tcp_proxy.h index 94a817a517c42..2c5f7f7816ce1 100644 --- a/source/server/config/network/tcp_proxy.h +++ b/source/server/config/network/tcp_proxy.h @@ -14,9 +14,9 @@ namespace Configuration { class TcpProxyConfigFactory : public NetworkFilterConfigFactory { public: // NetworkFilterConfigFactory - NetworkFilterFactoryCb tryCreateFilterFactory(NetworkFilterType type, const std::string& name, - const Json::Object& config, - Server::Instance& server); + NetworkFilterFactoryCb createFilterFactory(NetworkFilterType type, const Json::Object& config, + Server::Instance& server) override; + std::string name() override; }; } // Configuration diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index ef2fcd74365a3..c577815b65948 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -118,20 +118,10 @@ void MainImpl::initializeTracers(const Json::Object& configuration) { Json::ObjectSharedPtr driver_config = driver->getObject("config"); - bool found_tracer = false; - for (HttpTracerFactory* http_tracer_factory : httpTracerFactories()) { - Tracing::HttpTracerPtr http_tracer = - http_tracer_factory->tryCreateHttpTracer(type, *driver_config, server_, *cluster_manager_); - if (http_tracer) { - http_tracer_ = std::move(http_tracer); - found_tracer = true; - break; - } - } - - if (!found_tracer) { - throw EnvoyException(fmt::format("unable to create tracer for type {}", type)); - } + HttpTracerFactory* http_tracer_factory = httpTracerFactories().at(type); + Tracing::HttpTracerPtr http_tracer = + http_tracer_factory->createHttpTracer(*driver_config, server_, *cluster_manager_); + http_tracer_ = std::move(http_tracer); } const std::list& MainImpl::listeners() { return listeners_; } @@ -182,21 +172,10 @@ MainImpl::ListenerConfig::ListenerConfig(MainImpl& parent, Json::Object& json) : } // Now see if there is a factory that will accept the config. - bool found_filter = false; - for (NetworkFilterConfigFactory* config_factory : filterConfigFactories()) { - NetworkFilterFactoryCb callback = - config_factory->tryCreateFilterFactory(type, string_name, *config, parent_.server_); - if (callback) { - filter_factories_.push_back(callback); - found_filter = true; - break; - } - } - - if (!found_filter) { - throw EnvoyException( - fmt::format("unable to create filter factory for '{}'/'{}'", string_name, string_type)); - } + NetworkFilterConfigFactory* config_factory = filterConfigFactories().at(string_name); + NetworkFilterFactoryCb callback = + config_factory->createFilterFactory(type, *config, parent_.server_); + filter_factories_.push_back(callback); } } diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index 87eeeae749c13..2fa7e5ae79fa0 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -6,6 +6,8 @@ #include #include #include +#include +#include #include "envoy/http/filter.h" #include "envoy/network/filter.h" @@ -38,10 +40,19 @@ class NetworkFilterConfigFactory { public: virtual ~NetworkFilterConfigFactory() {} - virtual NetworkFilterFactoryCb tryCreateFilterFactory(NetworkFilterType type, - const std::string& name, - const Json::Object& config, - Server::Instance& server) PURE; + /** + * Create a particular network filter factory implementation. If the creation fails, an error will + * be thrown. The returned callback should always be valid. + */ + virtual NetworkFilterFactoryCb createFilterFactory(NetworkFilterType type, + const Json::Object& config, + Server::Instance& server) PURE; + + /** + * Provides the identifying name for a particular implementation of a network filter produced by + * the factory. + */ + virtual std::string name() PURE; }; /** @@ -53,14 +64,18 @@ class HttpTracerFactory { virtual ~HttpTracerFactory() {} /** - * Attempts to create a particular HttpTracer implementation corresponding to the type of factory. - * If the type argument doesn't match the expected value for the factory, a nullptr will be - * returned. However, if the type matches and the HttpTracer instantiation throws an exception, - * that exception will be propagated. + * Create a particular HttpTracer implementation. If the creation fails, an error will be thrown. + * The returned pointer should always be valid. + */ + virtual Tracing::HttpTracerPtr createHttpTracer(const Json::Object& json_config, + Server::Instance& server, + Upstream::ClusterManager& cluster_manager) PURE; + + /** + * Provides the identifying name for a particular implementation of HttpTracer produced by the + * factory. */ - virtual Tracing::HttpTracerPtr - tryCreateHttpTracer(const std::string& type, const Json::Object& json_config, - Server::Instance& server, Upstream::ClusterManager& cluster_manager) PURE; + virtual std::string name() PURE; }; /** @@ -84,11 +99,27 @@ class MainImpl : Logger::Loggable, public Main { MainImpl(Server::Instance& server); static void registerNetworkFilterConfigFactory(NetworkFilterConfigFactory& factory) { - filterConfigFactories().push_back(&factory); + auto result = filterConfigFactories().emplace(std::make_pair(factory.name(), &factory)); + + // result is a pair whose second member is a boolean indicating, if false, that the key exists + // and that the value was not inserted. + if (!result.second) { + throw EnvoyException(fmt::format( + "attempted to register multiple NetworkFilterConfigFactory objects with name: '{}'", + factory.name())); + } } static void registerHttpTracerFactory(HttpTracerFactory& factory) { - httpTracerFactories().push_back(&factory); + auto result = httpTracerFactories().emplace(std::make_pair(factory.name(), &factory)); + + // result is a pair whose second member is a boolean indicating, if false, that the key exists + // and that the value was not inserted. + if (!result.second) { + throw EnvoyException( + fmt::format("attempted to register multiple HttpTracerFactory objects with name: '{}'", + factory.name())); + } } /** @@ -154,14 +185,15 @@ class MainImpl : Logger::Loggable, public Main { std::list filter_factories_; }; - static std::list& filterConfigFactories() { - static std::list* filter_config_factories = - new std::list; + static std::unordered_map& filterConfigFactories() { + static std::unordered_map* filter_config_factories = + new std::unordered_map; return *filter_config_factories; } - static std::list& httpTracerFactories() { - static std::list* http_tracer_factories = new std::list; + static std::unordered_map& httpTracerFactories() { + static std::unordered_map* http_tracer_factories = + new std::unordered_map; return *http_tracer_factories; } diff --git a/source/server/http/health_check.cc b/source/server/http/health_check.cc index 96df845baebb2..022b2e8b2a5a9 100644 --- a/source/server/http/health_check.cc +++ b/source/server/http/health_check.cc @@ -24,13 +24,13 @@ namespace Configuration { /** * Config registration for the health check filter. @see HttpFilterConfigFactory. */ -HttpFilterFactoryCb HealthCheckFilterConfig::tryCreateFilterFactory(HttpFilterType type, - const std::string& name, - const Json::Object& config, - const std::string&, - Server::Instance& server) { - if (type != HttpFilterType::Both || name != "health_check") { - return nullptr; +HttpFilterFactoryCb HealthCheckFilterConfig::createFilterFactory(HttpFilterType type, + const Json::Object& config, + const std::string&, + Server::Instance& server) { + if (type != HttpFilterType::Both) { + throw EnvoyException(fmt::format( + "{} network filter must be configured as both a read and write filter.", name())); } config.validateSchema(Json::Schema::HEALTH_CHECK_HTTP_FILTER_SCHEMA); @@ -56,6 +56,8 @@ HttpFilterFactoryCb HealthCheckFilterConfig::tryCreateFilterFactory(HttpFilterTy }; } +std::string HealthCheckFilterConfig::name() { return "health_check"; } + /** * Static registration for the health check filter. @see RegisterHttpFilterConfigFactory. */ diff --git a/source/server/http/health_check.h b/source/server/http/health_check.h index 6237573ec1c76..d930bb7b9e0c9 100644 --- a/source/server/http/health_check.h +++ b/source/server/http/health_check.h @@ -16,9 +16,9 @@ namespace Configuration { class HealthCheckFilterConfig : public HttpFilterConfigFactory { public: - HttpFilterFactoryCb tryCreateFilterFactory(HttpFilterType type, const std::string& name, - const Json::Object& config, const std::string&, - Server::Instance& server) override; + HttpFilterFactoryCb createFilterFactory(HttpFilterType type, const Json::Object& config, + const std::string&, Server::Instance& server) override; + std::string name() override; }; } // Configuration diff --git a/test/server/config/http/config_test.cc b/test/server/config/http/config_test.cc index 29c5a736afd54..633989956308e 100644 --- a/test/server/config/http/config_test.cc +++ b/test/server/config/http/config_test.cc @@ -34,8 +34,8 @@ TEST(HttpFilterConfigTest, BufferFilter) { Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); NiceMock server; BufferFilterConfig factory; - HttpFilterFactoryCb cb = factory.tryCreateFilterFactory(HttpFilterType::Decoder, "buffer", - *json_config, "stats", server); + HttpFilterFactoryCb cb = + factory.createFilterFactory(HttpFilterType::Decoder, *json_config, "stats", server); Http::MockFilterChainFactoryCallbacks filter_callback; EXPECT_CALL(filter_callback, addStreamDecoderFilter(_)); cb(filter_callback); @@ -52,8 +52,7 @@ TEST(HttpFilterConfigTest, BadBufferFilterConfig) { Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); NiceMock server; BufferFilterConfig factory; - EXPECT_THROW(factory.tryCreateFilterFactory(HttpFilterType::Decoder, "buffer", *json_config, - "stats", server), + EXPECT_THROW(factory.createFilterFactory(HttpFilterType::Decoder, *json_config, "stats", server), Json::Exception); } @@ -66,8 +65,8 @@ TEST(HttpFilterConfigTest, DynamoFilter) { Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); NiceMock server; DynamoFilterConfig factory; - HttpFilterFactoryCb cb = factory.tryCreateFilterFactory( - HttpFilterType::Both, "http_dynamo_filter", *json_config, "stats", server); + HttpFilterFactoryCb cb = + factory.createFilterFactory(HttpFilterType::Both, *json_config, "stats", server); Http::MockFilterChainFactoryCallbacks filter_callback; EXPECT_CALL(filter_callback, addStreamFilter(_)); cb(filter_callback); @@ -87,8 +86,8 @@ TEST(HttpFilterConfigTest, FaultFilter) { Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); NiceMock server; FaultFilterConfig factory; - HttpFilterFactoryCb cb = factory.tryCreateFilterFactory(HttpFilterType::Decoder, "fault", - *json_config, "stats", server); + HttpFilterFactoryCb cb = + factory.createFilterFactory(HttpFilterType::Decoder, *json_config, "stats", server); Http::MockFilterChainFactoryCallbacks filter_callback; EXPECT_CALL(filter_callback, addStreamDecoderFilter(_)); cb(filter_callback); @@ -103,8 +102,8 @@ TEST(HttpFilterConfigTest, GrpcHttp1BridgeFilter) { Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); NiceMock server; GrpcHttp1BridgeFilterConfig factory; - HttpFilterFactoryCb cb = factory.tryCreateFilterFactory(HttpFilterType::Both, "grpc_http1_bridge", - *json_config, "stats", server); + HttpFilterFactoryCb cb = + factory.createFilterFactory(HttpFilterType::Both, *json_config, "stats", server); Http::MockFilterChainFactoryCallbacks filter_callback; EXPECT_CALL(filter_callback, addStreamFilter(_)); cb(filter_callback); @@ -121,8 +120,8 @@ TEST(HttpFilterConfigTest, HealthCheckFilter) { Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); NiceMock server; HealthCheckFilterConfig factory; - HttpFilterFactoryCb cb = factory.tryCreateFilterFactory(HttpFilterType::Both, "health_check", - *json_config, "stats", server); + HttpFilterFactoryCb cb = + factory.createFilterFactory(HttpFilterType::Both, *json_config, "stats", server); Http::MockFilterChainFactoryCallbacks filter_callback; EXPECT_CALL(filter_callback, addStreamFilter(_)); cb(filter_callback); @@ -140,8 +139,7 @@ TEST(HttpFilterConfigTest, BadHealthCheckFilterConfig) { Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); NiceMock server; HealthCheckFilterConfig factory; - EXPECT_THROW(factory.tryCreateFilterFactory(HttpFilterType::Both, "health_check", *json_config, - "stats", server), + EXPECT_THROW(factory.createFilterFactory(HttpFilterType::Both, *json_config, "stats", server), Json::Exception); } @@ -155,8 +153,8 @@ TEST(HttpFilterConfigTest, RouterFilter) { Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); NiceMock server; RouterFilterConfig factory; - HttpFilterFactoryCb cb = factory.tryCreateFilterFactory(HttpFilterType::Decoder, "router", - *json_config, "stats", server); + HttpFilterFactoryCb cb = + factory.createFilterFactory(HttpFilterType::Decoder, *json_config, "stats", server); Http::MockFilterChainFactoryCallbacks filter_callback; EXPECT_CALL(filter_callback, addStreamDecoderFilter(_)); cb(filter_callback); @@ -173,11 +171,14 @@ TEST(HttpFilterConfigTest, BadRouterFilterConfig) { Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); NiceMock server; RouterFilterConfig factory; - EXPECT_THROW(factory.tryCreateFilterFactory(HttpFilterType::Decoder, "router", *json_config, - "stats", server), + EXPECT_THROW(factory.createFilterFactory(HttpFilterType::Decoder, *json_config, "stats", server), Json::Exception); } +TEST(HttpFilterConfigTest, DoubleRegistrationTest) { + EXPECT_THROW(RegisterHttpFilterConfigFactory(), EnvoyException); +} + TEST(HttpTracerConfigTest, LightstepHttpTracer) { NiceMock cm; EXPECT_CALL(cm, get("fake_cluster")).WillRepeatedly(Return(&cm.thread_local_cluster_)); @@ -193,8 +194,7 @@ TEST(HttpTracerConfigTest, LightstepHttpTracer) { Json::ObjectSharedPtr valid_json = Json::Factory::loadFromString(valid_config); NiceMock server; LightstepHttpTracerFactory factory; - Tracing::HttpTracerPtr lightstep_tracer = - factory.tryCreateHttpTracer("lightstep", *valid_json, server, cm); + Tracing::HttpTracerPtr lightstep_tracer = factory.createHttpTracer(*valid_json, server, cm); EXPECT_NE(nullptr, lightstep_tracer); } @@ -211,27 +211,12 @@ TEST(HttpTracerConfigTest, ZipkinHttpTracer) { Json::ObjectSharedPtr valid_json = Json::Factory::loadFromString(valid_config); NiceMock server; ZipkinHttpTracerFactory factory; - Tracing::HttpTracerPtr zipkin_tracer = - factory.tryCreateHttpTracer("zipkin", *valid_json, server, cm); + Tracing::HttpTracerPtr zipkin_tracer = factory.createHttpTracer(*valid_json, server, cm); EXPECT_NE(nullptr, zipkin_tracer); } -TEST(HttpTracerConfigTest, InvalidHttpTracer) { - NiceMock cm; - - std::string valid_config = R"EOF( - {"collector_cluster": "fake_cluster"} - )EOF"; - Json::ObjectSharedPtr valid_json = Json::Factory::loadFromString(valid_config); - NiceMock server; - LightstepHttpTracerFactory lightstep_factory; - ZipkinHttpTracerFactory zipkin_factory; - std::string tracer_type = "invalid"; - Tracing::HttpTracerPtr tracer = - lightstep_factory.tryCreateHttpTracer(tracer_type, *valid_json, server, cm); - EXPECT_EQ(nullptr, tracer); - tracer = zipkin_factory.tryCreateHttpTracer(tracer_type, *valid_json, server, cm); - EXPECT_EQ(nullptr, tracer); +TEST(HttpTracerConfigTest, DoubleRegistrationTest) { + EXPECT_THROW(RegisterHttpTracerFactory(), EnvoyException); } } // Configuration diff --git a/test/server/config/network/config_test.cc b/test/server/config/network/config_test.cc index fcae5c40ff43f..f27e55a633a16 100644 --- a/test/server/config/network/config_test.cc +++ b/test/server/config/network/config_test.cc @@ -34,7 +34,7 @@ TEST(NetworkFilterConfigTest, RedisProxy) { NiceMock server; RedisProxyFilterConfigFactory factory; NetworkFilterFactoryCb cb = - factory.tryCreateFilterFactory(NetworkFilterType::Read, "redis_proxy", *json_config, server); + factory.createFilterFactory(NetworkFilterType::Read, *json_config, server); Network::MockConnection connection; EXPECT_CALL(connection, addReadFilter(_)); cb(connection); @@ -52,7 +52,7 @@ TEST(NetworkFilterConfigTest, MongoProxy) { NiceMock server; MongoProxyFilterConfigFactory factory; NetworkFilterFactoryCb cb = - factory.tryCreateFilterFactory(NetworkFilterType::Both, "mongo_proxy", *json_config, server); + factory.createFilterFactory(NetworkFilterType::Both, *json_config, server); Network::MockConnection connection; EXPECT_CALL(connection, addFilter(_)); cb(connection); @@ -70,9 +70,8 @@ TEST(NetworkFilterConfigTest, BadMongoProxyConfig) { Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); NiceMock server; MongoProxyFilterConfigFactory factory; - EXPECT_THROW( - factory.tryCreateFilterFactory(NetworkFilterType::Both, "mongo_proxy", *json_config, server), - Json::Exception); + EXPECT_THROW(factory.createFilterFactory(NetworkFilterType::Both, *json_config, server), + Json::Exception); } TEST(NetworkFilterConfigTest, TcpProxy) { @@ -107,13 +106,13 @@ TEST(NetworkFilterConfigTest, TcpProxy) { NiceMock server; TcpProxyConfigFactory factory; NetworkFilterFactoryCb cb = - factory.tryCreateFilterFactory(NetworkFilterType::Read, "tcp_proxy", *json_config, server); + factory.createFilterFactory(NetworkFilterType::Read, *json_config, server); Network::MockConnection connection; EXPECT_CALL(connection, addReadFilter(_)); cb(connection); - EXPECT_EQ(nullptr, factory.tryCreateFilterFactory(NetworkFilterType::Both, "tcp_proxy", - *json_config, server)); + EXPECT_THROW(factory.createFilterFactory(NetworkFilterType::Both, *json_config, server), + EnvoyException); } TEST(NetworkFilterConfigTest, ClientSslAuth) { @@ -128,8 +127,8 @@ TEST(NetworkFilterConfigTest, ClientSslAuth) { Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); NiceMock server; ClientSslAuthConfigFactory factory; - NetworkFilterFactoryCb cb = factory.tryCreateFilterFactory( - NetworkFilterType::Read, "client_ssl_auth", *json_config, server); + NetworkFilterFactoryCb cb = + factory.createFilterFactory(NetworkFilterType::Read, *json_config, server); Network::MockConnection connection; EXPECT_CALL(connection, addReadFilter(_)); cb(connection); @@ -148,7 +147,7 @@ TEST(NetworkFilterConfigTest, Ratelimit) { NiceMock server; RateLimitConfigFactory factory; NetworkFilterFactoryCb cb = - factory.tryCreateFilterFactory(NetworkFilterType::Read, "ratelimit", *json_config, server); + factory.createFilterFactory(NetworkFilterType::Read, *json_config, server); Network::MockConnection connection; EXPECT_CALL(connection, addReadFilter(_)); cb(connection); @@ -180,8 +179,7 @@ TEST(NetworkFilterConfigTest, BadHttpConnectionMangerConfig) { Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); HttpConnectionManagerFilterConfigFactory factory; NiceMock server; - EXPECT_THROW(factory.tryCreateFilterFactory(NetworkFilterType::Read, "http_connection_manager", - *json_config, server), + EXPECT_THROW(factory.createFilterFactory(NetworkFilterType::Read, *json_config, server), Json::Exception); } @@ -223,8 +221,7 @@ TEST(NetworkFilterConfigTest, BadAccessLogConfig) { Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); HttpConnectionManagerFilterConfigFactory factory; NiceMock server; - EXPECT_THROW(factory.tryCreateFilterFactory(NetworkFilterType::Read, "http_connection_manager", - *json_config, server), + EXPECT_THROW(factory.createFilterFactory(NetworkFilterType::Read, *json_config, server), Json::Exception); } @@ -268,8 +265,7 @@ TEST(NetworkFilterConfigTest, BadAccessLogType) { Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); HttpConnectionManagerFilterConfigFactory factory; NiceMock server; - EXPECT_THROW(factory.tryCreateFilterFactory(NetworkFilterType::Read, "http_connection_manager", - *json_config, server), + EXPECT_THROW(factory.createFilterFactory(NetworkFilterType::Read, *json_config, server), Json::Exception); } @@ -323,11 +319,14 @@ TEST(NetworkFilterConfigTest, BadAccessLogNestedTypes) { Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); HttpConnectionManagerFilterConfigFactory factory; NiceMock server; - EXPECT_THROW(factory.tryCreateFilterFactory(NetworkFilterType::Read, "http_connection_manager", - *json_config, server), + EXPECT_THROW(factory.createFilterFactory(NetworkFilterType::Read, *json_config, server), Json::Exception); } +TEST(NetworkFilterConfigTest, DoubleRegistrationTest) { + EXPECT_THROW(RegisterNetworkFilterConfigFactory(), EnvoyException); +} + } // Configuration } // Server } // Envoy diff --git a/test/server/http/health_check_test.cc b/test/server/http/health_check_test.cc index 49755cd248647..977342ffe2cec 100644 --- a/test/server/http/health_check_test.cc +++ b/test/server/http/health_check_test.cc @@ -208,10 +208,10 @@ TEST(HealthCheckFilterConfig, failsWhenNotPassThroughButTimeoutSet) { "{\"pass_through_mode\":false, \"cache_time_ms\":234, \"endpoint\":\"foo\"}"); NiceMock serverMock; - EXPECT_THROW(healthCheckFilterConfig.tryCreateFilterFactory( - Server::Configuration::HttpFilterType::Both, "health_check", *config, - "dummy_stats_prefix", serverMock), - EnvoyException); + EXPECT_THROW( + healthCheckFilterConfig.createFilterFactory(Server::Configuration::HttpFilterType::Both, + *config, "dummy_stats_prefix", serverMock), + EnvoyException); } TEST(HealthCheckFilterConfig, notFailingWhenNotPassThroughAndTimeoutNotSet) { @@ -220,8 +220,7 @@ TEST(HealthCheckFilterConfig, notFailingWhenNotPassThroughAndTimeoutNotSet) { Json::Factory::loadFromString("{\"pass_through_mode\":false, \"endpoint\":\"foo\"}"); NiceMock serverMock; - healthCheckFilterConfig.tryCreateFilterFactory(Server::Configuration::HttpFilterType::Both, - "health_check", *config, "dummy_stats_prefix", - serverMock); + healthCheckFilterConfig.createFilterFactory(Server::Configuration::HttpFilterType::Both, *config, + "dummy_stats_prefix", serverMock); } } // Envoy From 050c7d0ac46e838a6e612e1b740ad5c4796c91f3 Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Mon, 22 May 2017 14:11:29 -0400 Subject: [PATCH 6/8] Deprecated and added back list registration behavior for filters and responded to comments --- source/server/config/http/buffer.cc | 4 +- source/server/config/http/buffer.h | 4 +- source/server/config/http/dynamo.cc | 4 +- source/server/config/http/dynamo.h | 2 +- source/server/config/http/fault.cc | 4 +- source/server/config/http/fault.h | 4 +- .../server/config/http/grpc_http1_bridge.cc | 4 +- source/server/config/http/grpc_http1_bridge.h | 4 +- source/server/config/http/ratelimit.cc | 4 +- source/server/config/http/ratelimit.h | 4 +- source/server/config/http/router.cc | 4 +- source/server/config/http/router.h | 4 +- .../server/config/network/client_ssl_auth.cc | 4 +- .../server/config/network/client_ssl_auth.h | 6 +- source/server/config/network/echo.cc | 10 +- .../config/network/http_connection_manager.cc | 34 ++++- .../config/network/http_connection_manager.h | 100 ++++++++++++--- source/server/config/network/mongo_proxy.cc | 4 +- source/server/config/network/mongo_proxy.h | 6 +- source/server/config/network/ratelimit.cc | 4 +- source/server/config/network/ratelimit.h | 6 +- source/server/config/network/redis_proxy.cc | 4 +- source/server/config/network/redis_proxy.h | 6 +- source/server/config/network/tcp_proxy.cc | 4 +- source/server/config/network/tcp_proxy.h | 6 +- source/server/configuration_impl.cc | 39 ++++-- source/server/configuration_impl.h | 118 +++++++++++++++--- source/server/http/health_check.cc | 6 +- source/server/http/health_check.h | 2 +- test/server/BUILD | 1 + test/server/config/http/BUILD | 1 + test/server/config/http/config_test.cc | 9 +- test/server/config/network/BUILD | 1 + test/server/config/network/config_test.cc | 6 +- test/server/configuration_impl_test.cc | 63 ++++++++++ 35 files changed, 380 insertions(+), 106 deletions(-) diff --git a/source/server/config/http/buffer.cc b/source/server/config/http/buffer.cc index 4429b40082e6c..d5ced667dd44c 100644 --- a/source/server/config/http/buffer.cc +++ b/source/server/config/http/buffer.cc @@ -35,9 +35,9 @@ HttpFilterFactoryCb BufferFilterConfig::createFilterFactory(HttpFilterType type, std::string BufferFilterConfig::name() { return "buffer"; } /** - * Static registration for the buffer filter. @see RegisterHttpFilterConfigFactory. + * Static registration for the buffer filter. @see RegisterNamedHttpFilterConfigFactory. */ -static RegisterHttpFilterConfigFactory register_; +static RegisterNamedHttpFilterConfigFactory register_; } // Configuration } // Server diff --git a/source/server/config/http/buffer.h b/source/server/config/http/buffer.h index 590de549b8e19..c72eb35a6f7dd 100644 --- a/source/server/config/http/buffer.h +++ b/source/server/config/http/buffer.h @@ -11,9 +11,9 @@ namespace Server { namespace Configuration { /** - * Config registration for the buffer filter. @see HttpFilterConfigFactory. + * Config registration for the buffer filter. @see NamedHttpFilterConfigFactory. */ -class BufferFilterConfig : public HttpFilterConfigFactory { +class BufferFilterConfig : public NamedHttpFilterConfigFactory { public: HttpFilterFactoryCb createFilterFactory(HttpFilterType type, const Json::Object& json_config, const std::string& stats_prefix, diff --git a/source/server/config/http/dynamo.cc b/source/server/config/http/dynamo.cc index 6d16bce3add75..1184e4de07950 100644 --- a/source/server/config/http/dynamo.cc +++ b/source/server/config/http/dynamo.cc @@ -26,9 +26,9 @@ HttpFilterFactoryCb DynamoFilterConfig::createFilterFactory(HttpFilterType type, std::string DynamoFilterConfig::name() { return "http_dynamo_filter"; } /** - * Static registration for the http dynamodb filter. @see RegisterHttpFilterConfigFactory. + * Static registration for the http dynamodb filter. @see RegisterNamedHttpFilterConfigFactory. */ -static RegisterHttpFilterConfigFactory register_; +static RegisterNamedHttpFilterConfigFactory register_; } // Configuration } // Server diff --git a/source/server/config/http/dynamo.h b/source/server/config/http/dynamo.h index 50efda21affcc..dae2852e217c9 100644 --- a/source/server/config/http/dynamo.h +++ b/source/server/config/http/dynamo.h @@ -11,7 +11,7 @@ namespace Configuration { /** * Config registration for http dynamodb filter. */ -class DynamoFilterConfig : public HttpFilterConfigFactory { +class DynamoFilterConfig : public NamedHttpFilterConfigFactory { public: HttpFilterFactoryCb createFilterFactory(HttpFilterType type, const Json::Object&, const std::string& stat_prefix, diff --git a/source/server/config/http/fault.cc b/source/server/config/http/fault.cc index c2ed13b0f61d0..d4002d54bbd55 100644 --- a/source/server/config/http/fault.cc +++ b/source/server/config/http/fault.cc @@ -29,9 +29,9 @@ HttpFilterFactoryCb FaultFilterConfig::createFilterFactory(HttpFilterType type, std::string FaultFilterConfig::name() { return "fault"; } /** - * Static registration for the fault filter. @see RegisterHttpFilterConfigFactory. + * Static registration for the fault filter. @see RegisterNamedHttpFilterConfigFactory. */ -static RegisterHttpFilterConfigFactory register_; +static RegisterNamedHttpFilterConfigFactory register_; } // Configuration } // Server diff --git a/source/server/config/http/fault.h b/source/server/config/http/fault.h index 2c16d3ee7b8f8..dfe810f9867d3 100644 --- a/source/server/config/http/fault.h +++ b/source/server/config/http/fault.h @@ -11,9 +11,9 @@ namespace Server { namespace Configuration { /** - * Config registration for the fault injection filter. @see HttpFilterConfigFactory. + * Config registration for the fault injection filter. @see NamedHttpFilterConfigFactory. */ -class FaultFilterConfig : public HttpFilterConfigFactory { +class FaultFilterConfig : public NamedHttpFilterConfigFactory { public: HttpFilterFactoryCb createFilterFactory(HttpFilterType type, const Json::Object& json_config, const std::string& stats_prefix, diff --git a/source/server/config/http/grpc_http1_bridge.cc b/source/server/config/http/grpc_http1_bridge.cc index 0e1831502dd31..5eac2197dbbcf 100644 --- a/source/server/config/http/grpc_http1_bridge.cc +++ b/source/server/config/http/grpc_http1_bridge.cc @@ -26,9 +26,9 @@ HttpFilterFactoryCb GrpcHttp1BridgeFilterConfig::createFilterFactory(HttpFilterT std::string GrpcHttp1BridgeFilterConfig::name() { return "grpc_http1_bridge"; } /** - * Static registration for the grpc HTTP1 bridge filter. @see RegisterHttpFilterConfigFactory. + * Static registration for the grpc HTTP1 bridge filter. @see RegisterNamedHttpFilterConfigFactory. */ -static RegisterHttpFilterConfigFactory register_; +static RegisterNamedHttpFilterConfigFactory register_; } // Configuration } // Server diff --git a/source/server/config/http/grpc_http1_bridge.h b/source/server/config/http/grpc_http1_bridge.h index 31219d2a419e1..8d7aa767592ce 100644 --- a/source/server/config/http/grpc_http1_bridge.h +++ b/source/server/config/http/grpc_http1_bridge.h @@ -11,9 +11,9 @@ namespace Server { namespace Configuration { /** - * Config registration for the grpc HTTP1 bridge filter. @see HttpFilterConfigFactory. + * Config registration for the grpc HTTP1 bridge filter. @see NamedHttpFilterConfigFactory. */ -class GrpcHttp1BridgeFilterConfig : public HttpFilterConfigFactory { +class GrpcHttp1BridgeFilterConfig : public NamedHttpFilterConfigFactory { public: HttpFilterFactoryCb createFilterFactory(HttpFilterType type, const Json::Object&, const std::string&, Server::Instance& server) override; diff --git a/source/server/config/http/ratelimit.cc b/source/server/config/http/ratelimit.cc index 383ce6f7582a7..a08dab171e80e 100644 --- a/source/server/config/http/ratelimit.cc +++ b/source/server/config/http/ratelimit.cc @@ -29,9 +29,9 @@ HttpFilterFactoryCb RateLimitFilterConfig::createFilterFactory(HttpFilterType ty std::string RateLimitFilterConfig::name() { return "rate_limit"; } /** - * Static registration for the rate limit filter. @see RegisterHttpFilterConfigFactory. + * Static registration for the rate limit filter. @see RegisterNamedHttpFilterConfigFactory. */ -static RegisterHttpFilterConfigFactory register_; +static RegisterNamedHttpFilterConfigFactory register_; } // Configuration } // Server diff --git a/source/server/config/http/ratelimit.h b/source/server/config/http/ratelimit.h index 9bb89e79c6609..b26faee251e60 100644 --- a/source/server/config/http/ratelimit.h +++ b/source/server/config/http/ratelimit.h @@ -11,9 +11,9 @@ namespace Server { namespace Configuration { /** - * Config registration for the rate limit filter. @see HttpFilterConfigFactory. + * Config registration for the rate limit filter. @see NamedHttpFilterConfigFactory. */ -class RateLimitFilterConfig : public HttpFilterConfigFactory { +class RateLimitFilterConfig : public NamedHttpFilterConfigFactory { public: HttpFilterFactoryCb createFilterFactory(HttpFilterType type, const Json::Object& config, const std::string&, Server::Instance& server) override; diff --git a/source/server/config/http/router.cc b/source/server/config/http/router.cc index b97bf9e4c03f8..eea5977f1c407 100644 --- a/source/server/config/http/router.cc +++ b/source/server/config/http/router.cc @@ -34,9 +34,9 @@ HttpFilterFactoryCb RouterFilterConfig::createFilterFactory(HttpFilterType type, std::string RouterFilterConfig::name() { return "router"; } /** - * Static registration for the router filter. @see RegisterHttpFilterConfigFactory. + * Static registration for the router filter. @see RegisterNamedHttpFilterConfigFactory. */ -static RegisterHttpFilterConfigFactory register_; +static RegisterNamedHttpFilterConfigFactory register_; } // Configuration } // Server diff --git a/source/server/config/http/router.h b/source/server/config/http/router.h index 4eb85daf21097..41434beacb74e 100644 --- a/source/server/config/http/router.h +++ b/source/server/config/http/router.h @@ -11,9 +11,9 @@ namespace Server { namespace Configuration { /** - * Config registration for the router filter. @see HttpFilterConfigFactory. + * Config registration for the router filter. @see NamedHttpFilterConfigFactory. */ -class RouterFilterConfig : public HttpFilterConfigFactory { +class RouterFilterConfig : public NamedHttpFilterConfigFactory { public: HttpFilterFactoryCb createFilterFactory(HttpFilterType type, const Json::Object& json_config, const std::string& stat_prefix, diff --git a/source/server/config/network/client_ssl_auth.cc b/source/server/config/network/client_ssl_auth.cc index f5f1678689d3c..6362648ba28ea 100644 --- a/source/server/config/network/client_ssl_auth.cc +++ b/source/server/config/network/client_ssl_auth.cc @@ -30,9 +30,9 @@ NetworkFilterFactoryCb ClientSslAuthConfigFactory::createFilterFactory( std::string ClientSslAuthConfigFactory::name() { return "client_ssl_auth"; } /** - * Static registration for the client SSL auth filter. @see RegisterNetworkFilterConfigFactory. + * Static registration for the client SSL auth filter. @see RegisterNamedNetworkFilterConfigFactory. */ -static RegisterNetworkFilterConfigFactory registered_; +static RegisterNamedNetworkFilterConfigFactory registered_; } // Configuration } // Server diff --git a/source/server/config/network/client_ssl_auth.h b/source/server/config/network/client_ssl_auth.h index cc7a8116a0d5f..5818d4b1e870d 100644 --- a/source/server/config/network/client_ssl_auth.h +++ b/source/server/config/network/client_ssl_auth.h @@ -9,11 +9,11 @@ namespace Server { namespace Configuration { /** - * Config registration for the client SSL auth filter. @see NetworkFilterConfigFactory. + * Config registration for the client SSL auth filter. @see NamedNetworkFilterConfigFactory. */ -class ClientSslAuthConfigFactory : public NetworkFilterConfigFactory { +class ClientSslAuthConfigFactory : public NamedNetworkFilterConfigFactory { public: - // NetworkFilterConfigFactory + // NamedNetworkFilterConfigFactory NetworkFilterFactoryCb createFilterFactory(NetworkFilterType type, const Json::Object& json_config, Server::Instance& server) override; diff --git a/source/server/config/network/echo.cc b/source/server/config/network/echo.cc index 043263c1c1d15..136d0202e7425 100644 --- a/source/server/config/network/echo.cc +++ b/source/server/config/network/echo.cc @@ -11,11 +11,11 @@ namespace Server { namespace Configuration { /** - * Config registration for the echo filter. @see NetworkFilterConfigFactory. + * Config registration for the echo filter. @see NamedNetworkFilterConfigFactory. */ -class EchoConfigFactory : public NetworkFilterConfigFactory { +class EchoConfigFactory : public NamedNetworkFilterConfigFactory { public: - // NetworkFilterConfigFactory + // NamedNetworkFilterConfigFactory NetworkFilterFactoryCb createFilterFactory(NetworkFilterType type, const Json::Object&, Server::Instance&) override { if (type != NetworkFilterType::Read) { @@ -31,9 +31,9 @@ class EchoConfigFactory : public NetworkFilterConfigFactory { }; /** - * Static registration for the echo filter. @see RegisterNetworkFilterConfigFactory. + * Static registration for the echo filter. @see RegisterNamedNetworkFilterConfigFactory. */ -static RegisterNetworkFilterConfigFactory registered_; +static RegisterNamedNetworkFilterConfigFactory registered_; } // Configuration } // Server diff --git a/source/server/config/network/http_connection_manager.cc b/source/server/config/network/http_connection_manager.cc index 2add8eb812ebc..a78ee2c82a7ca 100644 --- a/source/server/config/network/http_connection_manager.cc +++ b/source/server/config/network/http_connection_manager.cc @@ -46,9 +46,10 @@ std::string HttpConnectionManagerFilterConfigFactory::name() { return "http_conn /** * Static registration for the HTTP connection manager filter. @see - * RegisterNetworkFilterConfigFactory. + * RegisterNamedNetworkFilterConfigFactory. */ -static RegisterNetworkFilterConfigFactory registered_; +static RegisterNamedNetworkFilterConfigFactory + registered_; std::string HttpConnectionManagerConfigUtility::determineNextProtocol(Network::Connection& connection, @@ -155,10 +156,31 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(const Json::Object& con HttpFilterType type = stringToType(string_type); - HttpFilterConfigFactory* factory = filterConfigFactories().at(string_name); - HttpFilterFactoryCb callback = - factory->createFilterFactory(type, *config_object, stats_prefix_, server); - filter_factories_.push_back(callback); + // Now see if there is a factory that will accept the config. + auto search_it = namedFilterConfigFactories().find(string_name); + if (search_it != namedFilterConfigFactories().end()) { + HttpFilterFactoryCb callback = + search_it->second->createFilterFactory(type, *config_object, stats_prefix_, server); + filter_factories_.push_back(callback); + } else { + // DEPRECATED + // This name wasn't found in the named map, so search in the deprecated list registry. + bool found_filter = false; + for (HttpFilterConfigFactory* config_factory : filterConfigFactories()) { + HttpFilterFactoryCb callback = config_factory->tryCreateFilterFactory( + type, string_name, *config_object, stats_prefix_, server); + if (callback) { + filter_factories_.push_back(callback); + found_filter = true; + break; + } + } + + if (!found_filter) { + throw EnvoyException(fmt::format("unable to create http filter factory for '{}'/'{}'", + string_name, string_type)); + } + } } } diff --git a/source/server/config/network/http_connection_manager.h b/source/server/config/network/http_connection_manager.h index 704d76e9fd201..8e048a398042b 100644 --- a/source/server/config/network/http_connection_manager.h +++ b/source/server/config/network/http_connection_manager.h @@ -24,12 +24,12 @@ namespace Configuration { enum class HttpFilterType { Decoder, Encoder, Both }; /** - * Config registration for the HTTP connection manager filter. @see NetworkFilterConfigFactory. + * Config registration for the HTTP connection manager filter. @see NamedNetworkFilterConfigFactory. */ class HttpConnectionManagerFilterConfigFactory : Logger::Loggable, - public NetworkFilterConfigFactory { + public NamedNetworkFilterConfigFactory { public: - // NetworkFilterConfigFactory + // NamedNetworkFilterConfigFactory NetworkFilterFactoryCb createFilterFactory(NetworkFilterType type, const Json::Object& config, Server::Instance& server) override; @@ -42,23 +42,43 @@ class HttpConnectionManagerFilterConfigFactory : Logger::Loggable HttpFilterFactoryCb; /** - * Implemented by each HTTP filter and registered via registerHttpFilterConfigFactory() or the - * convenience class RegisterHttpFilterConfigFactory. + * DEPRECATED - Implemented by each HTTP filter and registered via registerHttpFilterConfigFactory() + * or the convenience class RegisterHttpFilterConfigFactory. */ class HttpFilterConfigFactory { public: virtual ~HttpFilterConfigFactory() {} + virtual HttpFilterFactoryCb tryCreateFilterFactory(HttpFilterType type, const std::string& name, + const Json::Object& config, + const std::string& stat_prefix, + Server::Instance& server) PURE; +}; + +/** + * Implemented by each HTTP filter and registered via registerNamedHttpFilterConfigFactory() or the + * convenience class RegisterNamedHttpFilterConfigFactory. + */ +class NamedHttpFilterConfigFactory { +public: + virtual ~NamedHttpFilterConfigFactory() {} + /** - * Create a particular http filter factory implementation. If the creation fails, an error will be - * thrown. The returned callback should always be valid. + * Create a particular http filter factory implementation. If the implementation is unable to + * produce a factory with the provided parameters, it should throw an EnvoyException in the case of + * general error or a Json::Exception if the json configuration is erroneous. The returned + * callback should always be initialized. + * @param type supplies type of filter to initialize (decoder, encoder, or both) + * @param config supplies the general json configuration for the filter + * @param stat_prefix prefix for stat logging + * @param server supplies the server instance */ virtual HttpFilterFactoryCb createFilterFactory(HttpFilterType type, const Json::Object& config, const std::string& stat_prefix, Server::Instance& server) PURE; /** - * Provides the identifying name for a particular implementation of an http filter produced by the + * Returns the identifying name for a particular implementation of an http filter produced by the * factory. */ virtual std::string name() PURE; @@ -114,14 +134,28 @@ class HttpConnectionManagerConfig : Logger::Loggable, const Network::Address::Instance& localAddress() override; const Optional& userAgent() override { return user_agent_; } + /** + * DEPRECATED - Register an HttpFilterConfigFactory implementation as an option to create + * instances of HttpFilterFactoryCb. + * @param factory the HttpFilterConfigFactory implementation + */ static void registerHttpFilterConfigFactory(HttpFilterConfigFactory& factory) { - auto result = filterConfigFactories().emplace(std::make_pair(factory.name(), &factory)); + filterConfigFactories().push_back(&factory); + } + + /** + * Register a NamedHttpFilterConfigFactory implementation as an option to create instances of + * HttpFilterFactoryCb. + * @param factory the NamedHttpFilterConfigFactory implementation + */ + static void registerNamedHttpFilterConfigFactory(NamedHttpFilterConfigFactory& factory) { + auto result = namedFilterConfigFactories().emplace(std::make_pair(factory.name(), &factory)); // result is a pair whose second member is a boolean indicating, if false, that the key exists // and that the value was not inserted. if (!result.second) { throw EnvoyException(fmt::format( - "Attempted to register multiple HttpFilterConfigFactory objects with name: '{}'", + "Attempted to register multiple NamedHttpFilterConfigFactory objects with name: '{}'", factory.name())); } } @@ -131,12 +165,27 @@ class HttpConnectionManagerConfig : Logger::Loggable, private: enum class CodecType { HTTP1, HTTP2, AUTO }; - static std::unordered_map& filterConfigFactories() { - static std::unordered_map* filter_config_factories = - new std::unordered_map; + /** + * DEPRECATED - Returns a list of the currently registered HttpFilterConfigFactory + * implementations. + */ + static std::list& filterConfigFactories() { + static std::list* filter_config_factories = + new std::list; return *filter_config_factories; } + /** + * Returns a map of the currently registered NamedHttpFilterConfigFactory implementations. + */ + static std::unordered_map& + namedFilterConfigFactories() { + static std::unordered_map* + named_filter_config_factories = + new std::unordered_map; + return *named_filter_config_factories; + } + HttpFilterType stringToType(const std::string& type); Server::Instance& server_; @@ -159,13 +208,34 @@ class HttpConnectionManagerConfig : Logger::Loggable, }; /** - * @see HttpFilterConfigFactory. + * @see NamedHttpFilterConfigFactory. An instantiation of this class registers a + * NamedHttpFilterConfigFactory implementation (T) so it can be used to create instances of + * HttpFilterFactoryCb. + */ +template class RegisterNamedHttpFilterConfigFactory { +public: + /** + * Registers the implementation. + */ + RegisterNamedHttpFilterConfigFactory() { + static T* instance = new T; + HttpConnectionManagerConfig::registerNamedHttpFilterConfigFactory(*instance); + } +}; + +/** + * DEPRECATED @see HttpFilterConfigFactory. An instantiation of this class registers a + * HttpFilterConfigFactory implementation (T) so it can be used to create instances of + * HttpFilterFactoryCb. */ template class RegisterHttpFilterConfigFactory { public: + /** + * Registers the implementation. + */ RegisterHttpFilterConfigFactory() { static T* instance = new T; - HttpConnectionManagerConfig::registerHttpFilterConfigFactory(*instance); + HttpConnectionManagerConfig::registerHttpFilterConfigFactory(instance); } }; diff --git a/source/server/config/network/mongo_proxy.cc b/source/server/config/network/mongo_proxy.cc index c592edd3145a8..9a05b496b31a5 100644 --- a/source/server/config/network/mongo_proxy.cc +++ b/source/server/config/network/mongo_proxy.cc @@ -37,9 +37,9 @@ NetworkFilterFactoryCb MongoProxyFilterConfigFactory::createFilterFactory( std::string MongoProxyFilterConfigFactory::name() { return "mongo_proxy"; } /** - * Static registration for the mongo filter. @see RegisterNetworkFilterConfigFactory. + * Static registration for the mongo filter. @see RegisterNamedNetworkFilterConfigFactory. */ -static RegisterNetworkFilterConfigFactory registered_; +static RegisterNamedNetworkFilterConfigFactory registered_; } // Configuration } // Server diff --git a/source/server/config/network/mongo_proxy.h b/source/server/config/network/mongo_proxy.h index 4ca42b10e72e9..901b0297c28c8 100644 --- a/source/server/config/network/mongo_proxy.h +++ b/source/server/config/network/mongo_proxy.h @@ -9,11 +9,11 @@ namespace Server { namespace Configuration { /** - * Config registration for the mongo proxy filter. @see NetworkFilterConfigFactory. + * Config registration for the mongo proxy filter. @see NamedNetworkFilterConfigFactory. */ -class MongoProxyFilterConfigFactory : public NetworkFilterConfigFactory { +class MongoProxyFilterConfigFactory : public NamedNetworkFilterConfigFactory { public: - // NetworkFilterConfigFactory + // NamedNetworkFilterConfigFactory NetworkFilterFactoryCb createFilterFactory(NetworkFilterType type, const Json::Object& config, Server::Instance& server) override; diff --git a/source/server/config/network/ratelimit.cc b/source/server/config/network/ratelimit.cc index da8b3c6f10b59..23c7a781afee2 100644 --- a/source/server/config/network/ratelimit.cc +++ b/source/server/config/network/ratelimit.cc @@ -30,9 +30,9 @@ NetworkFilterFactoryCb RateLimitConfigFactory::createFilterFactory(NetworkFilter std::string RateLimitConfigFactory::name() { return "ratelimit"; } /** - * Static registration for the rate limit filter. @see RegisterNetworkFilterConfigFactory. + * Static registration for the rate limit filter. @see RegisterNamedNetworkFilterConfigFactory. */ -static RegisterNetworkFilterConfigFactory registered_; +static RegisterNamedNetworkFilterConfigFactory registered_; } // Configuration } // Server diff --git a/source/server/config/network/ratelimit.h b/source/server/config/network/ratelimit.h index af19e862a4476..d6bb11672c3a1 100644 --- a/source/server/config/network/ratelimit.h +++ b/source/server/config/network/ratelimit.h @@ -9,11 +9,11 @@ namespace Server { namespace Configuration { /** - * Config registration for the rate limit filter. @see NetworkFilterConfigFactory. + * Config registration for the rate limit filter. @see NamedNetworkFilterConfigFactory. */ -class RateLimitConfigFactory : public NetworkFilterConfigFactory { +class RateLimitConfigFactory : public NamedNetworkFilterConfigFactory { public: - // NetworkFilterConfigFactory + // NamedNetworkFilterConfigFactory NetworkFilterFactoryCb createFilterFactory(NetworkFilterType type, const Json::Object& json_config, Server::Instance& server) override; diff --git a/source/server/config/network/redis_proxy.cc b/source/server/config/network/redis_proxy.cc index 5ad671dcd2425..c950d8b71b976 100644 --- a/source/server/config/network/redis_proxy.cc +++ b/source/server/config/network/redis_proxy.cc @@ -38,9 +38,9 @@ NetworkFilterFactoryCb RedisProxyFilterConfigFactory::createFilterFactory( std::string RedisProxyFilterConfigFactory::name() { return "redis_proxy"; } /** - * Static registration for the redis filter. @see RegisterNetworkFilterConfigFactory. + * Static registration for the redis filter. @see RegisterNamedNetworkFilterConfigFactory. */ -static RegisterNetworkFilterConfigFactory registered_; +static RegisterNamedNetworkFilterConfigFactory registered_; } // Configuration } // Server diff --git a/source/server/config/network/redis_proxy.h b/source/server/config/network/redis_proxy.h index d666eed861ebb..a162a2a26633f 100644 --- a/source/server/config/network/redis_proxy.h +++ b/source/server/config/network/redis_proxy.h @@ -9,11 +9,11 @@ namespace Server { namespace Configuration { /** - * Config registration for the redis proxy filter. @see NetworkFilterConfigFactory. + * Config registration for the redis proxy filter. @see NamedNetworkFilterConfigFactory. */ -class RedisProxyFilterConfigFactory : public NetworkFilterConfigFactory { +class RedisProxyFilterConfigFactory : public NamedNetworkFilterConfigFactory { public: - // NetworkFilterConfigFactory + // NamedNetworkFilterConfigFactory NetworkFilterFactoryCb createFilterFactory(NetworkFilterType type, const Json::Object& config, Server::Instance& server) override; std::string name() override; diff --git a/source/server/config/network/tcp_proxy.cc b/source/server/config/network/tcp_proxy.cc index 74e5d1a925acd..7365a3de727ec 100644 --- a/source/server/config/network/tcp_proxy.cc +++ b/source/server/config/network/tcp_proxy.cc @@ -30,9 +30,9 @@ NetworkFilterFactoryCb TcpProxyConfigFactory::createFilterFactory(NetworkFilterT std::string TcpProxyConfigFactory::name() { return "tcp_proxy"; } /** - * Static registration for the tcp_proxy filter. @see RegisterNetworkFilterConfigFactory. + * Static registration for the tcp_proxy filter. @see RegisterNamedNetworkFilterConfigFactory. */ -static RegisterNetworkFilterConfigFactory registered_; +static RegisterNamedNetworkFilterConfigFactory registered_; } // Configuration } // Server diff --git a/source/server/config/network/tcp_proxy.h b/source/server/config/network/tcp_proxy.h index 2c5f7f7816ce1..6ce37826ba855 100644 --- a/source/server/config/network/tcp_proxy.h +++ b/source/server/config/network/tcp_proxy.h @@ -9,11 +9,11 @@ namespace Server { namespace Configuration { /** - * Config registration for the tcp proxy filter. @see NetworkFilterConfigFactory. + * Config registration for the tcp proxy filter. @see NamedNetworkFilterConfigFactory. */ -class TcpProxyConfigFactory : public NetworkFilterConfigFactory { +class TcpProxyConfigFactory : public NamedNetworkFilterConfigFactory { public: - // NetworkFilterConfigFactory + // NamedNetworkFilterConfigFactory NetworkFilterFactoryCb createFilterFactory(NetworkFilterType type, const Json::Object& config, Server::Instance& server) override; std::string name() override; diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index c577815b65948..ecce1aa5424fe 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -118,10 +118,13 @@ void MainImpl::initializeTracers(const Json::Object& configuration) { Json::ObjectSharedPtr driver_config = driver->getObject("config"); - HttpTracerFactory* http_tracer_factory = httpTracerFactories().at(type); - Tracing::HttpTracerPtr http_tracer = - http_tracer_factory->createHttpTracer(*driver_config, server_, *cluster_manager_); - http_tracer_ = std::move(http_tracer); + // Now see if there is a factory that will accept the config. + auto search_it = httpTracerFactories().find(type); + if (search_it != httpTracerFactories().end()) { + http_tracer_ = search_it->second->createHttpTracer(*driver_config, server_, *cluster_manager_); + } else { + throw EnvoyException(fmt::format("No HttpTracerFactory found for type: {}", type)); + } } const std::list& MainImpl::listeners() { return listeners_; } @@ -172,10 +175,30 @@ MainImpl::ListenerConfig::ListenerConfig(MainImpl& parent, Json::Object& json) : } // Now see if there is a factory that will accept the config. - NetworkFilterConfigFactory* config_factory = filterConfigFactories().at(string_name); - NetworkFilterFactoryCb callback = - config_factory->createFilterFactory(type, *config, parent_.server_); - filter_factories_.push_back(callback); + auto search_it = namedFilterConfigFactories().find(string_name); + if (search_it != namedFilterConfigFactories().end()) { + NetworkFilterFactoryCb callback = + search_it->second->createFilterFactory(type, *config, parent_.server_); + filter_factories_.push_back(callback); + } else { + // DEPRECATED + // This name wasn't found in the named map, so search in the deprecated list registry. + bool found_filter = false; + for (NetworkFilterConfigFactory* config_factory : filterConfigFactories()) { + NetworkFilterFactoryCb callback = + config_factory->tryCreateFilterFactory(type, string_name, *config, parent_.server_); + if (callback) { + filter_factories_.push_back(callback); + found_filter = true; + break; + } + } + + if (!found_filter) { + throw EnvoyException( + fmt::format("unable to create filter factory for '{}'/'{}'", string_name, string_type)); + } + } } } diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index 2fa7e5ae79fa0..9ae0bf38444cc 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -33,23 +33,42 @@ enum class NetworkFilterType { Read, Write, Both }; typedef std::function NetworkFilterFactoryCb; /** - * Implemented by each network filter and registered via registerNetworkFilterConfigFactory() or - * the convenience class RegisterNetworkFilterConfigFactory. + * DEPRECATED - Implemented by each network filter and registered via + * registerNetworkFilterConfigFactory() or the convenience class RegisterNetworkFilterConfigFactory. */ class NetworkFilterConfigFactory { public: virtual ~NetworkFilterConfigFactory() {} + virtual NetworkFilterFactoryCb tryCreateFilterFactory(NetworkFilterType type, + const std::string& name, + const Json::Object& config, + Server::Instance& server) PURE; +}; + +/** + * Implemented by each network filter and registered via registerNamedNetworkFilterConfigFactory() + * or the convenience class RegisterNamedNetworkFilterConfigFactory. + */ +class NamedNetworkFilterConfigFactory { +public: + virtual ~NamedNetworkFilterConfigFactory() {} + /** - * Create a particular network filter factory implementation. If the creation fails, an error will - * be thrown. The returned callback should always be valid. - */ + * Create a particular network filter factory implementation. If the implementation is unable to + * produce a factory with the provided parameters, it should throw an EnvoyException in the case + * of general error or a Json::Exception if the json configuration is erroneous. The returned + * callback should always be initialized. + * @param type supplies type of filter to initialize (read, write, or both) + * @param config supplies the general json configuration for the filter + * @param server supplies the server instance + */ virtual NetworkFilterFactoryCb createFilterFactory(NetworkFilterType type, const Json::Object& config, Server::Instance& server) PURE; /** - * Provides the identifying name for a particular implementation of a network filter produced by + * Returns the identifying name for a particular implementation of a network filter produced by * the factory. */ virtual std::string name() PURE; @@ -64,15 +83,20 @@ class HttpTracerFactory { virtual ~HttpTracerFactory() {} /** - * Create a particular HttpTracer implementation. If the creation fails, an error will be thrown. - * The returned pointer should always be valid. + * Create a particular HttpTracer implementation. If the implementation is unable to produce an + * HttpTracer with the provided parameters, it should throw an EnvoyException in the case of + * general error or a Json::Exception if the json configuration is erroneous. The returned pointer + * should always be valid. + * @param json_config supplies the general json configuration for the HttpTracer + * @param server supplies the server instance + * @param cluster_manager supplies the cluster_manager instance */ virtual Tracing::HttpTracerPtr createHttpTracer(const Json::Object& json_config, Server::Instance& server, Upstream::ClusterManager& cluster_manager) PURE; /** - * Provides the identifying name for a particular implementation of HttpTracer produced by the + * Returns the identifying name for a particular implementation of HttpTracer produced by the * factory. */ virtual std::string name() PURE; @@ -98,18 +122,36 @@ class MainImpl : Logger::Loggable, public Main { public: MainImpl(Server::Instance& server); + /** + * DEPRECATED - Register an NetworkFilterConfigFactory implementation as an option to create + * instances of NetworkFilterFactoryCb. + * @param factory the NetworkFilterConfigFactory implementation + */ static void registerNetworkFilterConfigFactory(NetworkFilterConfigFactory& factory) { - auto result = filterConfigFactories().emplace(std::make_pair(factory.name(), &factory)); + filterConfigFactories().push_back(&factory); + } + + /** + * Register an NamedNetworkFilterConfigFactory implementation as an option to create instances of + * NetworkFilterFactoryCb. + * @param factory the NamedNetworkFilterConfigFactory implementation + */ + static void registerNamedNetworkFilterConfigFactory(NamedNetworkFilterConfigFactory& factory) { + auto result = namedFilterConfigFactories().emplace(std::make_pair(factory.name(), &factory)); // result is a pair whose second member is a boolean indicating, if false, that the key exists // and that the value was not inserted. if (!result.second) { throw EnvoyException(fmt::format( - "attempted to register multiple NetworkFilterConfigFactory objects with name: '{}'", + "Attempted to register multiple NamedNetworkFilterConfigFactory objects with name: '{}'", factory.name())); } } + /** + * Register an HttpTracerFactory as an option to create instances of HttpTracers. + * @param factory the HttpTracerFactory implementation + */ static void registerHttpTracerFactory(HttpTracerFactory& factory) { auto result = httpTracerFactories().emplace(std::make_pair(factory.name(), &factory)); @@ -117,7 +159,7 @@ class MainImpl : Logger::Loggable, public Main { // and that the value was not inserted. if (!result.second) { throw EnvoyException( - fmt::format("attempted to register multiple HttpTracerFactory objects with name: '{}'", + fmt::format("Attempted to register multiple HttpTracerFactory objects with name: '{}'", factory.name())); } } @@ -185,12 +227,29 @@ class MainImpl : Logger::Loggable, public Main { std::list filter_factories_; }; - static std::unordered_map& filterConfigFactories() { - static std::unordered_map* filter_config_factories = - new std::unordered_map; + /** + * DEPRECATED - Returns a list of the currently registered NetworkConfigFactories. + */ + static std::list& filterConfigFactories() { + static std::list* filter_config_factories = + new std::list; return *filter_config_factories; } + /** + * Returns a map of the currently registered NamedNetworkConfigFactories. + */ + static std::unordered_map& + namedFilterConfigFactories() { + static std::unordered_map* + named_filter_config_factories = + new std::unordered_map; + return *named_filter_config_factories; + } + + /** + * Returns a map of the currently registered HttpTracerFactories. + */ static std::unordered_map& httpTracerFactories() { static std::unordered_map* http_tracer_factories = new std::unordered_map; @@ -213,10 +272,15 @@ class MainImpl : Logger::Loggable, public Main { }; /** - * @see NetworkFilterConfigFactory. + * DEPRECATED - @see NetworkFilterConfigFactory. An instantiation of this class registers a + * NetworkFilterConfigFactory implementation (T) so it can be used to create instances of + * NetworkFilterFactoryCb. */ template class RegisterNetworkFilterConfigFactory { public: + /** + * Registers the implementation. + */ RegisterNetworkFilterConfigFactory() { static T* instance = new T; MainImpl::registerNetworkFilterConfigFactory(*instance); @@ -224,10 +288,30 @@ template class RegisterNetworkFilterConfigFactory { }; /** - * @see HttpTracerFactory. + * @see NamedNetworkFilterConfigFactory. An instantiation of this class registers a + * NamedNetworkFilterConfigFactory implementation (T) so it can be used to create instances of + * NetworkFilterFactoryCb. + */ +template class RegisterNamedNetworkFilterConfigFactory { +public: + /** + * Registers the implementation. + */ + RegisterNamedNetworkFilterConfigFactory() { + static T* instance = new T; + MainImpl::registerNamedNetworkFilterConfigFactory(*instance); + } +}; + +/** + * @see HttpTracerFactory. An instantiation of this class registers an HttpTracerFactory + * implementation (T) so it can be used to create instances of HttpTracer. */ template class RegisterHttpTracerFactory { public: + /** + * Registers the implementation. + */ RegisterHttpTracerFactory() { static T* instance = new T; MainImpl::registerHttpTracerFactory(*instance); diff --git a/source/server/http/health_check.cc b/source/server/http/health_check.cc index 022b2e8b2a5a9..00b722341eab9 100644 --- a/source/server/http/health_check.cc +++ b/source/server/http/health_check.cc @@ -22,7 +22,7 @@ namespace Server { namespace Configuration { /** - * Config registration for the health check filter. @see HttpFilterConfigFactory. + * Config registration for the health check filter. @see NamedHttpFilterConfigFactory. */ HttpFilterFactoryCb HealthCheckFilterConfig::createFilterFactory(HttpFilterType type, const Json::Object& config, @@ -59,9 +59,9 @@ HttpFilterFactoryCb HealthCheckFilterConfig::createFilterFactory(HttpFilterType std::string HealthCheckFilterConfig::name() { return "health_check"; } /** - * Static registration for the health check filter. @see RegisterHttpFilterConfigFactory. + * Static registration for the health check filter. @see RegisterNamedHttpFilterConfigFactory. */ -static RegisterHttpFilterConfigFactory register_; +static RegisterNamedHttpFilterConfigFactory register_; } // Configuration } // Server diff --git a/source/server/http/health_check.h b/source/server/http/health_check.h index d930bb7b9e0c9..7430cbb97ae3d 100644 --- a/source/server/http/health_check.h +++ b/source/server/http/health_check.h @@ -14,7 +14,7 @@ namespace Envoy { namespace Server { namespace Configuration { -class HealthCheckFilterConfig : public HttpFilterConfigFactory { +class HealthCheckFilterConfig : public NamedHttpFilterConfigFactory { public: HttpFilterFactoryCb createFilterFactory(HttpFilterType type, const Json::Object& config, const std::string&, Server::Instance& server) override; diff --git a/test/server/BUILD b/test/server/BUILD index 77ea8fc38a61d..3c673ae71c503 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -26,6 +26,7 @@ envoy_cc_test( "//test/mocks/network:network_mocks", "//test/mocks/server:server_mocks", "//test/test_common:environment_lib", + "//test/test_common:utility_lib", ], ) diff --git a/test/server/config/http/BUILD b/test/server/config/http/BUILD index c2d31325abb9a..1de16b1b075f2 100644 --- a/test/server/config/http/BUILD +++ b/test/server/config/http/BUILD @@ -22,5 +22,6 @@ envoy_cc_test( "//source/server/config/http:zipkin_lib", "//source/server/http:health_check_lib", "//test/mocks/server:server_mocks", + "//test/test_common:utility_lib", ], ) diff --git a/test/server/config/http/config_test.cc b/test/server/config/http/config_test.cc index 633989956308e..2469caafe0496 100644 --- a/test/server/config/http/config_test.cc +++ b/test/server/config/http/config_test.cc @@ -11,6 +11,7 @@ #include "server/http/health_check.h" #include "test/mocks/server/mocks.h" +#include "test/test_common/utility.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -176,7 +177,9 @@ TEST(HttpFilterConfigTest, BadRouterFilterConfig) { } TEST(HttpFilterConfigTest, DoubleRegistrationTest) { - EXPECT_THROW(RegisterHttpFilterConfigFactory(), EnvoyException); + EXPECT_THROW_WITH_MESSAGE( + RegisterNamedHttpFilterConfigFactory(), EnvoyException, + "Attempted to register multiple NamedHttpFilterConfigFactory objects with name: 'router'"); } TEST(HttpTracerConfigTest, LightstepHttpTracer) { @@ -216,7 +219,9 @@ TEST(HttpTracerConfigTest, ZipkinHttpTracer) { } TEST(HttpTracerConfigTest, DoubleRegistrationTest) { - EXPECT_THROW(RegisterHttpTracerFactory(), EnvoyException); + EXPECT_THROW_WITH_MESSAGE( + RegisterHttpTracerFactory(), EnvoyException, + "Attempted to register multiple HttpTracerFactory objects with name: 'zipkin'"); } } // Configuration diff --git a/test/server/config/network/BUILD b/test/server/config/network/BUILD index d825cfc00d67c..bdaec75e23b87 100644 --- a/test/server/config/network/BUILD +++ b/test/server/config/network/BUILD @@ -19,6 +19,7 @@ envoy_cc_test( "//source/server/config/network:redis_proxy_lib", "//source/server/config/network:tcp_proxy_lib", "//test/mocks/server:server_mocks", + "//test/test_common:utility_lib", ], ) diff --git a/test/server/config/network/config_test.cc b/test/server/config/network/config_test.cc index f27e55a633a16..459235cb9034b 100644 --- a/test/server/config/network/config_test.cc +++ b/test/server/config/network/config_test.cc @@ -8,6 +8,7 @@ #include "server/config/network/tcp_proxy.h" #include "test/mocks/server/mocks.h" +#include "test/test_common/utility.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -324,7 +325,10 @@ TEST(NetworkFilterConfigTest, BadAccessLogNestedTypes) { } TEST(NetworkFilterConfigTest, DoubleRegistrationTest) { - EXPECT_THROW(RegisterNetworkFilterConfigFactory(), EnvoyException); + EXPECT_THROW_WITH_MESSAGE(RegisterNamedNetworkFilterConfigFactory(), + EnvoyException, "Attempted to register multiple " + "NamedNetworkFilterConfigFactory objects with name: " + "'client_ssl_auth'"); } } // Configuration diff --git a/test/server/configuration_impl_test.cc b/test/server/configuration_impl_test.cc index 7bcb89e8ed626..e1165aa4771f4 100644 --- a/test/server/configuration_impl_test.cc +++ b/test/server/configuration_impl_test.cc @@ -8,6 +8,7 @@ #include "test/mocks/network/mocks.h" #include "test/mocks/server/mocks.h" #include "test/test_common/environment.h" +#include "test/test_common/utility.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -275,6 +276,35 @@ TEST(ConfigurationImplTest, BadFilterConfig) { EXPECT_THROW(config.initialize(*loader), Json::Exception); } +TEST(ConfigurationImplTest, BadFilterName) { + std::string json = R"EOF( + { + "listeners" : [ + { + "address": "tcp://127.0.0.1:1234", + "filters": [ + { + "type" : "read", + "name" : "invalid", + "config" : {} + } + ] + } + ], + "cluster_manager": { + "clusters": [] + } + } + )EOF"; + + Json::ObjectSharedPtr loader = Json::Factory::loadFromString(json); + + NiceMock server; + MainImpl config(server); + EXPECT_THROW_WITH_MESSAGE(config.initialize(*loader), EnvoyException, + "unable to create filter factory for 'invalid'/'read'"); +} + TEST(ConfigurationImplTest, ServiceClusterNotSetWhenLSTracing) { std::string json = R"EOF( { @@ -368,6 +398,39 @@ TEST(ConfigurationImplTest, NullTracerSetWhenHttpKeyAbsentFromTracerConfiguratio EXPECT_NE(nullptr, dynamic_cast(&config.httpTracer())); } +TEST(ConfigurationImplTest, ConfigurationFailsWhenInvalidTracerSpecified) { + std::string json = R"EOF( + { + "listeners" : [ + { + "address": "tcp://127.0.0.1:1234", + "filters": [] + } + ], + "cluster_manager": { + "clusters": [] + }, + "tracing": { + "http": { + "driver": { + "type": "invalid", + "config": { + "access_token_file": "/etc/envoy/envoy.cfg" + } + } + } + } + } + )EOF"; + + Json::ObjectSharedPtr loader = Json::Factory::loadFromString(json); + + NiceMock server; + MainImpl config(server); + EXPECT_THROW_WITH_MESSAGE(config.initialize(*loader), EnvoyException, + "No HttpTracerFactory found for type: invalid"); +} + } // Configuration } // Server } // Envoy From 1090023d80ff1744c128f072b4a60baaae351d38 Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Tue, 23 May 2017 10:22:11 -0400 Subject: [PATCH 7/8] fixed formatting --- test/server/configuration_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/server/configuration_impl_test.cc b/test/server/configuration_impl_test.cc index e1165aa4771f4..2087cfef51f9d 100644 --- a/test/server/configuration_impl_test.cc +++ b/test/server/configuration_impl_test.cc @@ -302,7 +302,7 @@ TEST(ConfigurationImplTest, BadFilterName) { NiceMock server; MainImpl config(server); EXPECT_THROW_WITH_MESSAGE(config.initialize(*loader), EnvoyException, - "unable to create filter factory for 'invalid'/'read'"); + "unable to create filter factory for 'invalid'/'read'"); } TEST(ConfigurationImplTest, ServiceClusterNotSetWhenLSTracing) { From cb977801d27c99b07e4d38c4c92aab991eb3aa7e Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Tue, 23 May 2017 11:51:16 -0400 Subject: [PATCH 8/8] Added tests to ensure deprecated registration style continues to work. --- .../config/network/http_connection_manager.h | 2 +- test/server/BUILD | 1 + test/server/config/network/BUILD | 1 + test/server/config/network/config_test.cc | 62 +++++++++++++++++++ test/server/configuration_impl_test.cc | 51 +++++++++++++++ 5 files changed, 116 insertions(+), 1 deletion(-) diff --git a/source/server/config/network/http_connection_manager.h b/source/server/config/network/http_connection_manager.h index 8e048a398042b..304874056d268 100644 --- a/source/server/config/network/http_connection_manager.h +++ b/source/server/config/network/http_connection_manager.h @@ -235,7 +235,7 @@ template class RegisterHttpFilterConfigFactory { */ RegisterHttpFilterConfigFactory() { static T* instance = new T; - HttpConnectionManagerConfig::registerHttpFilterConfigFactory(instance); + HttpConnectionManagerConfig::registerHttpFilterConfigFactory(*instance); } }; diff --git a/test/server/BUILD b/test/server/BUILD index 3c673ae71c503..3339c39e3b055 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -21,6 +21,7 @@ envoy_cc_test( data = ["//test/common/ssl/test_data:certs"], deps = [ "//source/common/event:dispatcher_lib", + "//source/common/filter:echo_lib", "//source/server:configuration_lib", "//test/mocks:common_lib", "//test/mocks/network:network_mocks", diff --git a/test/server/config/network/BUILD b/test/server/config/network/BUILD index bdaec75e23b87..75e17b1c21a94 100644 --- a/test/server/config/network/BUILD +++ b/test/server/config/network/BUILD @@ -12,6 +12,7 @@ envoy_cc_test( name = "config_test", srcs = ["config_test.cc"], deps = [ + "//source/common/dynamo:dynamo_filter_lib", "//source/server/config/network:client_ssl_auth_lib", "//source/server/config/network:http_connection_manager_lib", "//source/server/config/network:mongo_proxy_lib", diff --git a/test/server/config/network/config_test.cc b/test/server/config/network/config_test.cc index 459235cb9034b..3319714f730e0 100644 --- a/test/server/config/network/config_test.cc +++ b/test/server/config/network/config_test.cc @@ -1,5 +1,7 @@ #include +#include "common/dynamo/dynamo_filter.h" + #include "server/config/network/client_ssl_auth.h" #include "server/config/network/http_connection_manager.h" #include "server/config/network/mongo_proxy.h" @@ -324,6 +326,66 @@ TEST(NetworkFilterConfigTest, BadAccessLogNestedTypes) { Json::Exception); } +/** + * Deprecated version of config registration for http dynamodb filter. + */ +class TestDeprecatedDynamoFilterConfig : public HttpFilterConfigFactory { +public: + HttpFilterFactoryCb tryCreateFilterFactory(HttpFilterType type, const std::string& name, + const Json::Object&, const std::string& stat_prefix, + Server::Instance& server) override { + if (type != HttpFilterType::Both || name != "http_dynamo_filter_deprecated") { + return nullptr; + } + + return [&server, stat_prefix](Http::FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamFilter(Http::StreamFilterSharedPtr{ + new Dynamo::DynamoFilter(server.runtime(), stat_prefix, server.stats())}); + }; + } +}; + +TEST(NetworkFilterConfigTest, DeprecatedHttpFilterConfigFactoryTest) { + // Test just ensures that the deprecated http filter registration still works without error. + + // Register the config factory + RegisterHttpFilterConfigFactory registered; + + std::string json = R"EOF( + { + "codec_type" : "http1", + "stat_prefix" : "my_stat_prefix", + "route_config" : { + "virtual_hosts" : [ + { + "name" : "default", + "domains" : ["*"], + "routes" : [ + { + "prefix" : "/", + "cluster": "fake_cluster" + } + ] + } + ] + }, + "filters" : [ + { + "type" : "both", + "name" : "http_dynamo_filter_deprecated", + "config" : {} + } + ] + } + )EOF"; + + Json::ObjectSharedPtr loader = Json::Factory::loadFromString(json); + + HttpConnectionManagerFilterConfigFactory factory; + NiceMock server; + factory.createFilterFactory(NetworkFilterType::Read, *loader, server); +} + TEST(NetworkFilterConfigTest, DoubleRegistrationTest) { EXPECT_THROW_WITH_MESSAGE(RegisterNamedNetworkFilterConfigFactory(), EnvoyException, "Attempted to register multiple " diff --git a/test/server/configuration_impl_test.cc b/test/server/configuration_impl_test.cc index 2087cfef51f9d..62a168bb638b0 100644 --- a/test/server/configuration_impl_test.cc +++ b/test/server/configuration_impl_test.cc @@ -2,6 +2,8 @@ #include #include +#include "common/filter/echo.h" + #include "server/configuration_impl.h" #include "test/mocks/common.h" @@ -431,6 +433,55 @@ TEST(ConfigurationImplTest, ConfigurationFailsWhenInvalidTracerSpecified) { "No HttpTracerFactory found for type: invalid"); } +/** + * Config registration for the echo filter using the deprecated registration class. + */ +class TestDeprecatedEchoConfigFactory : public NetworkFilterConfigFactory { +public: + // NetworkFilterConfigFactory + NetworkFilterFactoryCb tryCreateFilterFactory(NetworkFilterType type, const std::string& name, + const Json::Object&, Server::Instance&) override { + if (type != NetworkFilterType::Read || name != "echo_deprecated") { + return nullptr; + } + + return [](Network::FilterManager& filter_manager) + -> void { filter_manager.addReadFilter(Network::ReadFilterSharedPtr{new Filter::Echo()}); }; + } +}; + +TEST(NetworkFilterConfigTest, DeprecatedFilterConfigFactoryRegistrationTest) { + // Test ensures that the deprecated network filter registration still works without error. + + // Register the config factory + RegisterNetworkFilterConfigFactory registered; + + std::string json = R"EOF( + { + "listeners" : [ + { + "address": "tcp://127.0.0.1:1234", + "filters": [ + { + "type" : "read", + "name" : "echo_deprecated", + "config" : {} + } + ] + } + ], + "cluster_manager": { + "clusters": [] + } + } + )EOF"; + + Json::ObjectSharedPtr loader = Json::Factory::loadFromString(json); + + NiceMock server; + MainImpl config(server); + config.initialize(*loader); +} } // Configuration } // Server } // Envoy