From 00e51d8a5969bfb2348f958c5b25b749b0e26518 Mon Sep 17 00:00:00 2001 From: Nicolas Flacco Date: Wed, 27 Mar 2019 10:54:41 -0700 Subject: [PATCH] Revert "redis: prefixed routing (#5658)" The change breaks the existing Redis operation, for example redis-cli -p [WHATEVER] GET 1 crashes Envoy. This reverts commit 046e98904f6df60f0c548ffe77ffb5f5f980179d. Signed-off-by: Nicolas Flacco --- DEPRECATED.md | 1 - .../network/redis_proxy/v2/redis_proxy.proto | 63 +----- docs/root/intro/arch_overview/redis.rst | 5 +- docs/root/intro/version_history.rst | 1 - source/common/common/utility.h | 34 +-- .../filters/network/redis_proxy/BUILD | 29 +-- .../redis_proxy/command_splitter_impl.cc | 41 ++-- .../redis_proxy/command_splitter_impl.h | 30 +-- .../filters/network/redis_proxy/config.cc | 43 +--- .../filters/network/redis_proxy/conn_pool.h | 2 +- .../network/redis_proxy/conn_pool_impl.h | 1 + .../network/redis_proxy/proxy_filter.cc | 2 +- .../network/redis_proxy/proxy_filter.h | 1 + .../filters/network/redis_proxy/router.h | 42 ---- .../network/redis_proxy/router_impl.cc | 68 ------ .../filters/network/redis_proxy/router_impl.h | 55 ----- test/common/common/utility_test.cc | 37 ---- .../filters/network/redis_proxy/BUILD | 13 -- .../redis_proxy/command_lookup_speed_test.cc | 8 +- .../redis_proxy/command_splitter_impl_test.cc | 24 ++- .../network/redis_proxy/config_test.cc | 15 -- .../redis_proxy/conn_pool_impl_test.cc | 5 +- .../filters/network/redis_proxy/mocks.cc | 3 - .../filters/network/redis_proxy/mocks.h | 12 -- .../network/redis_proxy/proxy_filter_test.cc | 2 +- .../network/redis_proxy/router_impl_test.cc | 199 ------------------ 26 files changed, 78 insertions(+), 658 deletions(-) delete mode 100644 source/extensions/filters/network/redis_proxy/router.h delete mode 100644 source/extensions/filters/network/redis_proxy/router_impl.cc delete mode 100644 source/extensions/filters/network/redis_proxy/router_impl.h delete mode 100644 test/extensions/filters/network/redis_proxy/router_impl_test.cc diff --git a/DEPRECATED.md b/DEPRECATED.md index 502987c09035d..9743d8d3a910b 100644 --- a/DEPRECATED.md +++ b/DEPRECATED.md @@ -15,7 +15,6 @@ A logged warning is expected for each deprecated item that is in deprecation win [fault.proto](https://github.com/envoyproxy/envoy/blob/master/api/envoy/config/filter/fault/v2/fault.proto)) has been deprecated. It was never used and setting it has no effect. It will be removed in the following release. -* Use of `cluster`, found in [redis-proxy.proto](https://github.com/envoyproxy/envoy/blob/master/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto) is deprecated. Set a `PrefixRoutes.catch_all_cluster` instead. ## Version 1.9.0 (Dec 20, 2018) diff --git a/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto b/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto index 696bf26b8b5c9..cd8c18b128755 100644 --- a/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto +++ b/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto @@ -22,13 +22,7 @@ message RedisProxy { // Name of cluster from cluster manager. See the :ref:`configuration section // ` of the architecture overview for recommendations on // configuring the backing cluster. - // - // .. attention:: - // - // This field is deprecated. Use a :ref:`catch-all - // cluster` - // instead. - string cluster = 2 [deprecated = true]; + string cluster = 2 [(validate.rules).string.min_bytes = 1]; // Redis connection pool settings. message ConnPoolSettings { @@ -54,63 +48,10 @@ message RedisProxy { bool enable_hashtagging = 2; } - // Network settings for the connection pool to the upstream clusters. + // Network settings for the connection pool to the upstream cluster. ConnPoolSettings settings = 3 [(validate.rules).message.required = true]; // Indicates that latency stat should be computed in microseconds. By default it is computed in // milliseconds. bool latency_in_micros = 4; - - message PrefixRoutes { - message Route { - // String prefix that must match the beginning of the keys. Envoy will always favor the - // longest match. - string prefix = 1 [(validate.rules).string.min_bytes = 1]; - - // Indicates if the prefix needs to be removed from the key when forwarded. - bool remove_prefix = 2; - - // Upstream cluster to forward the command to. - string cluster = 3 [(validate.rules).string.min_bytes = 1]; - } - - // List of prefix routes. - repeated Route routes = 1 [(gogoproto.nullable) = false]; - - // Indicates that prefix matching should be case insensitive. - bool case_insensitive = 2; - - // Optional catch-all route to forward commands that doesn't match any of the routes. The - // catch-all route becomes required when no routes are specified. - string catch_all_cluster = 3; - } - - // List of **unique** prefixes used to separate keys from different workloads to different - // clusters. Envoy will always favor the longest match first in case of overlap. A catch-all - // cluster can be used to forward commands when there is no match. Time complexity of the - // lookups are in O(min(longest key prefix, key length)). - // - // Example: - // - // .. code-block:: yaml - // - // prefix_routes: - // routes: - // - prefix: "ab" - // cluster: "cluster_a" - // - prefix: "abc" - // cluster: "cluster_b" - // - // When using the above routes, the following prefixes would be sent to: - // - // * 'get abc:users' would retrive the key 'abc:users' from cluster_b. - // * 'get ab:users' would retrive the key 'ab:users' from cluster_a. - // * 'get z:users' would return a NoUpstreamHost error. A :ref:`catch-all - // cluster` - // would have retrieved the key from that cluster instead. - // - // See the :ref:`configuration section - // ` of the architecture overview for recommendations on - // configuring the backing clusters. - PrefixRoutes prefix_routes = 5 [(gogoproto.nullable) = false]; } diff --git a/docs/root/intro/arch_overview/redis.rst b/docs/root/intro/arch_overview/redis.rst index b3aa16565ad78..044ea66553726 100644 --- a/docs/root/intro/arch_overview/redis.rst +++ b/docs/root/intro/arch_overview/redis.rst @@ -8,9 +8,7 @@ In this mode, the goals of Envoy are to maintain availability and partition tole over consistency. This is the key point when comparing Envoy to `Redis Cluster `_. Envoy is designed as a best-effort cache, meaning that it will not try to reconcile inconsistent data or keep a globally consistent -view of cluster membership. It also supports routing commands from different workload to -different to different upstream clusters based on their access patterns, eviction, or isolation -requirements. +view of cluster membership. The Redis project offers a thorough reference on partitioning as it relates to Redis. See "`Partitioning: how to split data among multiple Redis instances @@ -24,7 +22,6 @@ The Redis project offers a thorough reference on partitioning as it relates to R * Detailed command statistics. * Active and passive healthchecking. * Hash tagging. -* Prefix routing. **Planned future enhancements**: diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 8032b072d2f00..b601e6f1dad62 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -53,7 +53,6 @@ Version history * ratelimit: removed deprecated rate limit configuration from bootstrap. * redis: added :ref:`hashtagging ` to guarantee a given key's upstream. * redis: added :ref:`latency stats ` for commands. -* redis: added :ref:`prefix routing ` to enable routing commands based on their key's prefix to different upstream. * redis: added :ref:`success and error stats ` for commands. * redis: migrate hash function for host selection to `MurmurHash2 `_ from std::hash. MurmurHash2 is compatible with std::hash in GNU libstdc++ 3.4.20 or above. This is typically the case when compiled on Linux and not macOS. * redis: added :ref:`latency_in_micros ` to specify the redis commands stats time unit in microseconds. diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 9eaddb7f64da1..785df6d8aa404 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -568,11 +568,8 @@ template struct TrieLookupTable { * Adds an entry to the Trie at the given Key. * @param key the key used to add the entry. * @param value the value to be associated with the key. - * @param overwrite_existing will overwrite the value when the value for a given key already - * exists. - * @return false when a value already exists for the given key. */ - bool add(const char* key, Value value, bool overwrite_existing = true) { + void add(const char* key, Value value) { TrieEntry* current = &root_; while (uint8_t c = *key) { if (!current->entries_[c]) { @@ -581,11 +578,7 @@ template struct TrieLookupTable { current = current->entries_[c].get(); key++; } - if (current->value_ && !overwrite_existing) { - return false; - } current->value_ = value; - return true; } /** @@ -606,31 +599,6 @@ template struct TrieLookupTable { return current->value_; } - /** - * Finds the entry associated with the longest prefix. Complexity is O(min(longest key prefix, key - * length)) - * @param key the key used to find. - * @return the value matching the longest prefix based on the key. - */ - Value findLongestPrefix(const char* key) const { - const TrieEntry* current = &root_; - const TrieEntry* result = nullptr; - while (uint8_t c = *key) { - if (current->value_) { - result = current; - } - - // https://github.com/facebook/mcrouter/blob/master/mcrouter/lib/fbi/cpp/Trie-inl.h#L126-L143 - current = current->entries_[c].get(); - if (current == nullptr) { - return result ? result->value_ : nullptr; - } - - key++; - } - return current ? current->value_ : result->value_; - } - TrieEntry root_; }; diff --git a/source/extensions/filters/network/redis_proxy/BUILD b/source/extensions/filters/network/redis_proxy/BUILD index 911edafb83684..8cd0a234462e0 100644 --- a/source/extensions/filters/network/redis_proxy/BUILD +++ b/source/extensions/filters/network/redis_proxy/BUILD @@ -30,22 +30,13 @@ envoy_cc_library( ], ) -envoy_cc_library( - name = "router_interface", - hdrs = ["router.h"], - deps = [ - ":conn_pool_interface", - "@envoy_api//envoy/config/filter/network/redis_proxy/v2:redis_proxy_cc", - ], -) - envoy_cc_library( name = "command_splitter_lib", srcs = ["command_splitter_impl.cc"], hdrs = ["command_splitter_impl.h"], deps = [ ":command_splitter_interface", - ":router_interface", + ":conn_pool_interface", "//include/envoy/stats:stats_macros", "//include/envoy/stats:timespan", "//source/common/common:assert_lib", @@ -63,6 +54,7 @@ envoy_cc_library( hdrs = ["conn_pool_impl.h"], deps = [ ":conn_pool_interface", + "//include/envoy/router:router_interface", "//include/envoy/thread_local:thread_local_interface", "//include/envoy/upstream:cluster_manager_interface", "//source/common/buffer:buffer_lib", @@ -81,7 +73,6 @@ envoy_cc_library( hdrs = ["proxy_filter.h"], deps = [ ":command_splitter_interface", - ":router_interface", "//include/envoy/network:drain_decision_interface", "//include/envoy/network:filter_interface", "//include/envoy/upstream:cluster_manager_interface", @@ -104,21 +95,7 @@ envoy_cc_library( "//source/extensions/filters/network/common:factory_base_lib", "//source/extensions/filters/network/common/redis:codec_lib", "//source/extensions/filters/network/redis_proxy:command_splitter_lib", - "//source/extensions/filters/network/redis_proxy:proxy_filter_lib", - "//source/extensions/filters/network/redis_proxy:router_lib", - ], -) - -envoy_cc_library( - name = "router_lib", - srcs = ["router_impl.cc"], - hdrs = ["router_impl.h"], - deps = [ - ":router_interface", - "//include/envoy/thread_local:thread_local_interface", - "//include/envoy/upstream:cluster_manager_interface", - "//source/common/common:to_lower_table_lib", "//source/extensions/filters/network/redis_proxy:conn_pool_lib", - "@envoy_api//envoy/config/filter/network/redis_proxy/v2:redis_proxy_cc", + "//source/extensions/filters/network/redis_proxy:proxy_filter_lib", ], ) diff --git a/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc b/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc index 415a754e0ac6a..beea0fbaa32ee 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc @@ -59,15 +59,15 @@ void SingleServerRequest::cancel() { handle_ = nullptr; } -SplitRequestPtr SimpleRequest::create(Router& router, +SplitRequestPtr SimpleRequest::create(ConnPool::Instance& conn_pool, const Common::Redis::RespValue& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros) { std::unique_ptr request_ptr{ new SimpleRequest(callbacks, command_stats, time_source, latency_in_micros)}; - request_ptr->handle_ = - router.makeRequest(incoming_request.asArray()[1].asString(), incoming_request, *request_ptr); + request_ptr->handle_ = conn_pool.makeRequest(incoming_request.asArray()[1].asString(), + incoming_request, *request_ptr); if (!request_ptr->handle_) { request_ptr->callbacks_.onResponse(Utility::makeError(Response::get().NoUpstreamHost)); return nullptr; @@ -76,7 +76,7 @@ SplitRequestPtr SimpleRequest::create(Router& router, return std::move(request_ptr); } -SplitRequestPtr EvalRequest::create(Router& router, +SplitRequestPtr EvalRequest::create(ConnPool::Instance& conn_pool, const Common::Redis::RespValue& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros) { @@ -91,8 +91,8 @@ SplitRequestPtr EvalRequest::create(Router& router, std::unique_ptr request_ptr{ new EvalRequest(callbacks, command_stats, time_source, latency_in_micros)}; - request_ptr->handle_ = - router.makeRequest(incoming_request.asArray()[3].asString(), incoming_request, *request_ptr); + request_ptr->handle_ = conn_pool.makeRequest(incoming_request.asArray()[3].asString(), + incoming_request, *request_ptr); if (!request_ptr->handle_) { command_stats.error_.inc(); request_ptr->callbacks_.onResponse(Utility::makeError(Response::get().NoUpstreamHost)); @@ -123,7 +123,7 @@ void FragmentedRequest::onChildFailure(uint32_t index) { onChildResponse(Utility::makeError(Response::get().UpstreamFailure), index); } -SplitRequestPtr MGETRequest::create(Router& router, +SplitRequestPtr MGETRequest::create(ConnPool::Instance& conn_pool, const Common::Redis::RespValue& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros) { @@ -152,8 +152,8 @@ SplitRequestPtr MGETRequest::create(Router& router, single_mget.asArray()[1].asString() = incoming_request.asArray()[i].asString(); ENVOY_LOG(debug, "redis: parallel get: '{}'", single_mget.toString()); - pending_request.handle_ = - router.makeRequest(incoming_request.asArray()[i].asString(), single_mget, pending_request); + pending_request.handle_ = conn_pool.makeRequest(incoming_request.asArray()[i].asString(), + single_mget, pending_request); if (!pending_request.handle_) { pending_request.onResponse(Utility::makeError(Response::get().NoUpstreamHost)); } @@ -195,7 +195,7 @@ void MGETRequest::onChildResponse(Common::Redis::RespValuePtr&& value, uint32_t } } -SplitRequestPtr MSETRequest::create(Router& router, +SplitRequestPtr MSETRequest::create(ConnPool::Instance& conn_pool, const Common::Redis::RespValue& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros) { @@ -231,8 +231,8 @@ SplitRequestPtr MSETRequest::create(Router& router, single_mset.asArray()[2].asString() = incoming_request.asArray()[i + 1].asString(); ENVOY_LOG(debug, "redis: parallel set: '{}'", single_mset.toString()); - pending_request.handle_ = - router.makeRequest(incoming_request.asArray()[i].asString(), single_mset, pending_request); + pending_request.handle_ = conn_pool.makeRequest(incoming_request.asArray()[i].asString(), + single_mset, pending_request); if (!pending_request.handle_) { pending_request.onResponse(Utility::makeError(Response::get().NoUpstreamHost)); } @@ -270,7 +270,7 @@ void MSETRequest::onChildResponse(Common::Redis::RespValuePtr&& value, uint32_t } } -SplitRequestPtr SplitKeysSumResultRequest::create(Router& router, +SplitRequestPtr SplitKeysSumResultRequest::create(ConnPool::Instance& conn_pool, const Common::Redis::RespValue& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, @@ -299,8 +299,8 @@ SplitRequestPtr SplitKeysSumResultRequest::create(Router& router, single_fragment.asArray()[1].asString() = incoming_request.asArray()[i].asString(); ENVOY_LOG(debug, "redis: parallel {}: '{}'", incoming_request.asArray()[0].asString(), single_fragment.toString()); - pending_request.handle_ = router.makeRequest(incoming_request.asArray()[i].asString(), - single_fragment, pending_request); + pending_request.handle_ = conn_pool.makeRequest(incoming_request.asArray()[i].asString(), + single_fragment, pending_request); if (!pending_request.handle_) { pending_request.onResponse(Utility::makeError(Response::get().NoUpstreamHost)); } @@ -337,11 +337,12 @@ void SplitKeysSumResultRequest::onChildResponse(Common::Redis::RespValuePtr&& va } } -InstanceImpl::InstanceImpl(RouterPtr&& router, Stats::Scope& scope, const std::string& stat_prefix, - TimeSource& time_source, bool latency_in_micros) - : router_(std::move(router)), simple_command_handler_(*router_), - eval_command_handler_(*router_), mget_handler_(*router_), mset_handler_(*router_), - split_keys_sum_result_handler_(*router_), +InstanceImpl::InstanceImpl(ConnPool::InstancePtr&& conn_pool, Stats::Scope& scope, + const std::string& stat_prefix, TimeSource& time_source, + bool latency_in_micros) + : conn_pool_(std::move(conn_pool)), simple_command_handler_(*conn_pool_), + eval_command_handler_(*conn_pool_), mget_handler_(*conn_pool_), mset_handler_(*conn_pool_), + split_keys_sum_result_handler_(*conn_pool_), stats_{ALL_COMMAND_SPLITTER_STATS(POOL_COUNTER_PREFIX(scope, stat_prefix + "splitter."))}, latency_in_micros_(latency_in_micros), time_source_(time_source) { for (const std::string& command : Common::Redis::SupportedCommands::simpleCommands()) { diff --git a/source/extensions/filters/network/redis_proxy/command_splitter_impl.h b/source/extensions/filters/network/redis_proxy/command_splitter_impl.h index 45ac46b71cd37..b7ac2b90f409b 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.h +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.h @@ -17,7 +17,6 @@ #include "extensions/filters/network/common/redis/client_impl.h" #include "extensions/filters/network/redis_proxy/command_splitter.h" #include "extensions/filters/network/redis_proxy/conn_pool.h" -#include "extensions/filters/network/redis_proxy/router.h" namespace Envoy { namespace Extensions { @@ -69,9 +68,9 @@ class CommandHandler { class CommandHandlerBase { protected: - CommandHandlerBase(Router& router) : router_(router) {} + CommandHandlerBase(ConnPool::Instance& conn_pool) : conn_pool_(conn_pool) {} - Router& router_; + ConnPool::Instance& conn_pool_; }; class SplitRequestBase : public SplitRequest { @@ -122,7 +121,8 @@ class SingleServerRequest : public SplitRequestBase, public Common::Redis::Clien */ class SimpleRequest : public SingleServerRequest { public: - static SplitRequestPtr create(Router& router, const Common::Redis::RespValue& incoming_request, + static SplitRequestPtr create(ConnPool::Instance& conn_pool, + const Common::Redis::RespValue& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros); @@ -137,7 +137,8 @@ class SimpleRequest : public SingleServerRequest { */ class EvalRequest : public SingleServerRequest { public: - static SplitRequestPtr create(Router& router, const Common::Redis::RespValue& incoming_request, + static SplitRequestPtr create(ConnPool::Instance& conn_pool, + const Common::Redis::RespValue& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros); @@ -194,7 +195,8 @@ class FragmentedRequest : public SplitRequestBase { */ class MGETRequest : public FragmentedRequest, Logger::Loggable { public: - static SplitRequestPtr create(Router& router, const Common::Redis::RespValue& incoming_request, + static SplitRequestPtr create(ConnPool::Instance& conn_pool, + const Common::Redis::RespValue& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros); @@ -215,7 +217,8 @@ class MGETRequest : public FragmentedRequest, Logger::Loggable { public: - static SplitRequestPtr create(Router& router, const Common::Redis::RespValue& incoming_request, + static SplitRequestPtr create(ConnPool::Instance& conn_pool, + const Common::Redis::RespValue& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros); @@ -237,7 +240,8 @@ class SplitKeysSumResultRequest : public FragmentedRequest, Logger::Loggable { public: - static SplitRequestPtr create(Router& router, const Common::Redis::RespValue& incoming_request, + static SplitRequestPtr create(ConnPool::Instance& conn_pool, + const Common::Redis::RespValue& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros); @@ -257,11 +261,11 @@ class MSETRequest : public FragmentedRequest, Logger::Loggable class CommandHandlerFactory : public CommandHandler, CommandHandlerBase { public: - CommandHandlerFactory(Router& router) : CommandHandlerBase(router) {} + CommandHandlerFactory(ConnPool::Instance& conn_pool) : CommandHandlerBase(conn_pool) {} SplitRequestPtr startRequest(const Common::Redis::RespValue& request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros) { - return RequestClass::create(router_, request, callbacks, command_stats, time_source, + return RequestClass::create(conn_pool_, request, callbacks, command_stats, time_source, latency_in_micros); } }; @@ -284,8 +288,8 @@ struct InstanceStats { class InstanceImpl : public Instance, Logger::Loggable { public: - InstanceImpl(RouterPtr&& router, Stats::Scope& scope, const std::string& stat_prefix, - TimeSource& time_source, bool latency_in_micros); + InstanceImpl(ConnPool::InstancePtr&& conn_pool, Stats::Scope& scope, + const std::string& stat_prefix, TimeSource& time_source, bool latency_in_micros); // RedisProxy::CommandSplitter::Instance SplitRequestPtr makeRequest(const Common::Redis::RespValue& request, @@ -303,7 +307,7 @@ class InstanceImpl : public Instance, Logger::Loggable { CommandHandler& handler); void onInvalidRequest(SplitCallbacks& callbacks); - RouterPtr router_; + ConnPool::InstancePtr conn_pool_; CommandHandlerFactory simple_command_handler_; CommandHandlerFactory eval_command_handler_; CommandHandlerFactory mget_handler_; diff --git a/source/extensions/filters/network/redis_proxy/config.cc b/source/extensions/filters/network/redis_proxy/config.cc index 9838c2cc5ebf4..bae74e8633713 100644 --- a/source/extensions/filters/network/redis_proxy/config.cc +++ b/source/extensions/filters/network/redis_proxy/config.cc @@ -11,8 +11,8 @@ #include "extensions/filters/network/common/redis/client_impl.h" #include "extensions/filters/network/common/redis/codec_impl.h" #include "extensions/filters/network/redis_proxy/command_splitter_impl.h" +#include "extensions/filters/network/redis_proxy/conn_pool_impl.h" #include "extensions/filters/network/redis_proxy/proxy_filter.h" -#include "extensions/filters/network/redis_proxy/router_impl.h" namespace Envoy { namespace Extensions { @@ -24,43 +24,18 @@ Network::FilterFactoryCb RedisProxyFilterConfigFactory::createFilterFactoryFromP Server::Configuration::FactoryContext& context) { ASSERT(!proto_config.stat_prefix().empty()); + ASSERT(!proto_config.cluster().empty()); ASSERT(proto_config.has_settings()); ProxyFilterConfigSharedPtr filter_config(std::make_shared( proto_config, context.scope(), context.drainDecision(), context.runtime())); - - envoy::config::filter::network::redis_proxy::v2::RedisProxy::PrefixRoutes prefix_routes( - proto_config.prefix_routes()); - - // set the catch-all route from the deprecated cluster and settings parameters. - if (prefix_routes.catch_all_cluster().empty() && prefix_routes.routes_size() == 0) { - if (proto_config.cluster().empty()) { - throw EnvoyException("cannot configure a redis-proxy without any upstream"); - } - - prefix_routes.set_catch_all_cluster(proto_config.cluster()); - } - - std::set unique_clusters; - for (auto& route : prefix_routes.routes()) { - unique_clusters.emplace(route.cluster()); - } - unique_clusters.emplace(prefix_routes.catch_all_cluster()); - - Upstreams upstreams; - for (auto& cluster : unique_clusters) { - upstreams.emplace(cluster, std::make_shared( - cluster, context.clusterManager(), - Common::Redis::Client::ClientFactoryImpl::instance_, - context.threadLocal(), proto_config.settings())); - } - - auto router = std::make_unique(prefix_routes, std::move(upstreams)); - - std::shared_ptr splitter = - std::make_shared( - std::move(router), context.scope(), filter_config->stat_prefix_, context.timeSource(), - proto_config.latency_in_micros()); + ConnPool::InstancePtr conn_pool( + new ConnPool::InstanceImpl(filter_config->cluster_name_, context.clusterManager(), + Common::Redis::Client::ClientFactoryImpl::instance_, + context.threadLocal(), proto_config.settings())); + std::shared_ptr splitter(new CommandSplitter::InstanceImpl( + std::move(conn_pool), context.scope(), filter_config->stat_prefix_, context.timeSource(), + proto_config.latency_in_micros())); return [splitter, filter_config](Network::FilterManager& filter_manager) -> void { Common::Redis::DecoderFactoryImpl factory; filter_manager.addReadFilter(std::make_shared( diff --git a/source/extensions/filters/network/redis_proxy/conn_pool.h b/source/extensions/filters/network/redis_proxy/conn_pool.h index 713e4f7310cc5..442219e79b547 100644 --- a/source/extensions/filters/network/redis_proxy/conn_pool.h +++ b/source/extensions/filters/network/redis_proxy/conn_pool.h @@ -36,7 +36,7 @@ class Instance { Common::Redis::Client::PoolCallbacks& callbacks) PURE; }; -typedef std::shared_ptr InstanceSharedPtr; +typedef std::unique_ptr InstancePtr; } // namespace ConnPool } // namespace RedisProxy diff --git a/source/extensions/filters/network/redis_proxy/conn_pool_impl.h b/source/extensions/filters/network/redis_proxy/conn_pool_impl.h index 17facb93afbc4..1dfb363573ab2 100644 --- a/source/extensions/filters/network/redis_proxy/conn_pool_impl.h +++ b/source/extensions/filters/network/redis_proxy/conn_pool_impl.h @@ -37,6 +37,7 @@ class InstanceImpl : public Instance { const std::string& cluster_name, Upstream::ClusterManager& cm, Common::Redis::Client::ClientFactory& client_factory, ThreadLocal::SlotAllocator& tls, const envoy::config::filter::network::redis_proxy::v2::RedisProxy::ConnPoolSettings& config); + // RedisProxy::ConnPool::Instance Common::Redis::Client::PoolRequest* makeRequest(const std::string& key, const Common::Redis::RespValue& request, diff --git a/source/extensions/filters/network/redis_proxy/proxy_filter.cc b/source/extensions/filters/network/redis_proxy/proxy_filter.cc index f36d692ea9cae..d5fc143e9be09 100644 --- a/source/extensions/filters/network/redis_proxy/proxy_filter.cc +++ b/source/extensions/filters/network/redis_proxy/proxy_filter.cc @@ -17,7 +17,7 @@ namespace RedisProxy { ProxyFilterConfig::ProxyFilterConfig( const envoy::config::filter::network::redis_proxy::v2::RedisProxy& config, Stats::Scope& scope, const Network::DrainDecision& drain_decision, Runtime::Loader& runtime) - : drain_decision_(drain_decision), runtime_(runtime), + : drain_decision_(drain_decision), runtime_(runtime), cluster_name_(config.cluster()), stat_prefix_(fmt::format("redis.{}.", config.stat_prefix())), stats_(generateStats(stat_prefix_, scope)) {} diff --git a/source/extensions/filters/network/redis_proxy/proxy_filter.h b/source/extensions/filters/network/redis_proxy/proxy_filter.h index ae2141a322d94..3f8dc62d6eecd 100644 --- a/source/extensions/filters/network/redis_proxy/proxy_filter.h +++ b/source/extensions/filters/network/redis_proxy/proxy_filter.h @@ -56,6 +56,7 @@ class ProxyFilterConfig { const Network::DrainDecision& drain_decision_; Runtime::Loader& runtime_; + const std::string cluster_name_; const std::string stat_prefix_; const std::string redis_drain_close_runtime_key_{"redis.drain_close_enabled"}; ProxyStats stats_; diff --git a/source/extensions/filters/network/redis_proxy/router.h b/source/extensions/filters/network/redis_proxy/router.h deleted file mode 100644 index 1317b170aca4c..0000000000000 --- a/source/extensions/filters/network/redis_proxy/router.h +++ /dev/null @@ -1,42 +0,0 @@ -#pragma once - -#include -#include - -#include "envoy/common/pure.h" -#include "envoy/config/filter/network/redis_proxy/v2/redis_proxy.pb.h" - -#include "extensions/filters/network/redis_proxy/conn_pool.h" - -namespace Envoy { -namespace Extensions { -namespace NetworkFilters { -namespace RedisProxy { - -/* - * Decorator of a connection pool in order to enable key based routing. - */ -class Router { -public: - virtual ~Router() = default; - - /** - * Forwards the request to the connection pool that matches a route or uses the wildcard route - * when no match is found. - * @param key supplies the key of the current command. - * @param request supplies the RESP request to make. - * @param callbacks supplies the request callbacks. - * @return PoolRequest* a handle to the active request or nullptr if the request could not be made - * for some reason. - */ - virtual Common::Redis::Client::PoolRequest* - makeRequest(const std::string& key, const Common::Redis::RespValue& request, - Common::Redis::Client::PoolCallbacks& callbacks) PURE; -}; - -typedef std::unique_ptr RouterPtr; - -} // namespace RedisProxy -} // namespace NetworkFilters -} // namespace Extensions -} // namespace Envoy diff --git a/source/extensions/filters/network/redis_proxy/router_impl.cc b/source/extensions/filters/network/redis_proxy/router_impl.cc deleted file mode 100644 index 009cc345b3844..0000000000000 --- a/source/extensions/filters/network/redis_proxy/router_impl.cc +++ /dev/null @@ -1,68 +0,0 @@ -#include "extensions/filters/network/redis_proxy/router_impl.h" - -#include "common/common/fmt.h" - -namespace Envoy { -namespace Extensions { -namespace NetworkFilters { -namespace RedisProxy { - -PrefixRoutes::PrefixRoutes( - const envoy::config::filter::network::redis_proxy::v2::RedisProxy::PrefixRoutes& config, - Upstreams&& upstreams) - : case_insensitive_(config.case_insensitive()), upstreams_(std::move(upstreams)), - catch_all_upstream_(config.catch_all_cluster().empty() - ? nullptr - : upstreams_.at(config.catch_all_cluster())) { - - for (auto const& route : config.routes()) { - std::string copy(route.prefix()); - - if (case_insensitive_) { - to_lower_table_.toLowerCase(copy); - } - - auto success = prefix_lookup_table_.add(copy.c_str(), - std::make_shared(Prefix{ - route.prefix(), - route.remove_prefix(), - upstreams_.at(route.cluster()), - }), - false); - if (!success) { - throw EnvoyException(fmt::format("prefix `{}` already exists.", route.prefix())); - } - } -} - -Common::Redis::Client::PoolRequest* -PrefixRoutes::makeRequest(const std::string& key, const Common::Redis::RespValue& request, - Common::Redis::Client::PoolCallbacks& callbacks) { - - PrefixPtr value = nullptr; - if (case_insensitive_) { - std::string copy(key); - to_lower_table_.toLowerCase(copy); - value = prefix_lookup_table_.findLongestPrefix(copy.c_str()); - } else { - value = prefix_lookup_table_.findLongestPrefix(key.c_str()); - } - - if (value != nullptr) { - absl::string_view view(key); - if (value->remove_prefix) { - view.remove_prefix(value->prefix.length()); - } - std::string str(view); - value->upstream->makeRequest(str, request, callbacks); - } else if (catch_all_upstream_ != nullptr) { - catch_all_upstream_.value()->makeRequest(key, request, callbacks); - } - - return nullptr; -} - -} // namespace RedisProxy -} // namespace NetworkFilters -} // namespace Extensions -} // namespace Envoy diff --git a/source/extensions/filters/network/redis_proxy/router_impl.h b/source/extensions/filters/network/redis_proxy/router_impl.h deleted file mode 100644 index 0c3d50356c02d..0000000000000 --- a/source/extensions/filters/network/redis_proxy/router_impl.h +++ /dev/null @@ -1,55 +0,0 @@ -#pragma once - -#include -#include -#include -#include -#include -#include - -#include "envoy/config/filter/network/redis_proxy/v2/redis_proxy.pb.h" -#include "envoy/thread_local/thread_local.h" -#include "envoy/upstream/cluster_manager.h" - -#include "common/common/to_lower_table.h" - -#include "extensions/filters/network/redis_proxy/conn_pool_impl.h" -#include "extensions/filters/network/redis_proxy/router.h" - -namespace Envoy { -namespace Extensions { -namespace NetworkFilters { -namespace RedisProxy { - -typedef std::map Upstreams; - -class PrefixRoutes : public Router { -public: - PrefixRoutes(const envoy::config::filter::network::redis_proxy::v2::RedisProxy::PrefixRoutes& - prefix_routes, - Upstreams&& upstreams); - - Common::Redis::Client::PoolRequest* - makeRequest(const std::string& hash_key, const Common::Redis::RespValue& request, - Common::Redis::Client::PoolCallbacks& callbacks) override; - -private: - struct Prefix { - const std::string prefix; - const bool remove_prefix; - ConnPool::InstanceSharedPtr upstream; - }; - - typedef std::shared_ptr PrefixPtr; - - TrieLookupTable prefix_lookup_table_; - const ToLowerTable to_lower_table_; - const bool case_insensitive_; - Upstreams upstreams_; - absl::optional catch_all_upstream_; -}; - -} // namespace RedisProxy -} // namespace NetworkFilters -} // namespace Extensions -} // namespace Envoy diff --git a/test/common/common/utility_test.cc b/test/common/common/utility_test.cc index e2a084651065a..6434cd140280b 100644 --- a/test/common/common/utility_test.cc +++ b/test/common/common/utility_test.cc @@ -828,41 +828,4 @@ TEST(DateFormatter, FromTimeSameWildcard) { DateFormatter("%Y-%m-%dT%H:%M:%S.000Z%1f%2f").fromTime(time1)); } -TEST(TrieLookupTable, AddItems) { - TrieLookupTable trie; - EXPECT_TRUE(trie.add("foo", "a")); - EXPECT_TRUE(trie.add("bar", "b")); - EXPECT_EQ("a", trie.find("foo")); - EXPECT_EQ("b", trie.find("bar")); - - // overwrite_existing = false - EXPECT_FALSE(trie.add("foo", "c", false)); - EXPECT_EQ("a", trie.find("foo")); - - // overwrite_existing = true - EXPECT_TRUE(trie.add("foo", "c")); - EXPECT_EQ("c", trie.find("foo")); -} - -TEST(TrieLookupTable, LongestPrefix) { - TrieLookupTable trie; - EXPECT_TRUE(trie.add("foo", "a")); - EXPECT_TRUE(trie.add("bar", "b")); - EXPECT_TRUE(trie.add("baro", "c")); - - EXPECT_EQ("a", trie.find("foo")); - EXPECT_EQ("a", trie.findLongestPrefix("foo")); - EXPECT_EQ("a", trie.findLongestPrefix("foosball")); - - EXPECT_EQ("b", trie.find("bar")); - EXPECT_EQ("b", trie.findLongestPrefix("bar")); - EXPECT_EQ("b", trie.findLongestPrefix("baritone")); - EXPECT_EQ("c", trie.findLongestPrefix("barometer")); - - EXPECT_EQ(nullptr, trie.find("toto")); - EXPECT_EQ(nullptr, trie.findLongestPrefix("toto")); - EXPECT_EQ(nullptr, trie.find(" ")); - EXPECT_EQ(nullptr, trie.findLongestPrefix(" ")); -} - } // namespace Envoy diff --git a/test/extensions/filters/network/redis_proxy/BUILD b/test/extensions/filters/network/redis_proxy/BUILD index 7b6629b6e4917..492404c41547e 100644 --- a/test/extensions/filters/network/redis_proxy/BUILD +++ b/test/extensions/filters/network/redis_proxy/BUILD @@ -75,7 +75,6 @@ envoy_cc_mock( "//source/extensions/filters/network/common/redis:codec_lib", "//source/extensions/filters/network/redis_proxy:command_splitter_interface", "//source/extensions/filters/network/redis_proxy:conn_pool_interface", - "//source/extensions/filters/network/redis_proxy:router_interface", ], ) @@ -105,15 +104,3 @@ envoy_extension_cc_test_binary( "//test/test_common:simulated_time_system_lib", ], ) - -envoy_extension_cc_test( - name = "router_impl_test", - srcs = ["router_impl_test.cc"], - extension_name = "envoy.filters.network.redis_proxy", - deps = [ - ":redis_mocks", - "//source/extensions/filters/network/redis_proxy:router_lib", - "//test/extensions/filters/network/common/redis:redis_mocks", - "//test/test_common:utility_lib", - ], -) diff --git a/test/extensions/filters/network/redis_proxy/command_lookup_speed_test.cc b/test/extensions/filters/network/redis_proxy/command_lookup_speed_test.cc index d70fdb02a5e02..2f4d8e30e1b0b 100644 --- a/test/extensions/filters/network/redis_proxy/command_lookup_speed_test.cc +++ b/test/extensions/filters/network/redis_proxy/command_lookup_speed_test.cc @@ -30,7 +30,7 @@ class NoOpSplitCallbacks : public CommandSplitter::SplitCallbacks { void onResponse(Common::Redis::RespValuePtr&&) override {} }; -class NullRouterImpl : public Router { +class NullInstanceImpl : public ConnPool::Instance { Common::Redis::Client::PoolRequest* makeRequest(const std::string&, const Common::Redis::RespValue&, Common::Redis::Client::PoolCallbacks&) override { @@ -65,11 +65,11 @@ class CommandLookUpSpeedTest { } } - Router* router_{new NullRouterImpl()}; + ConnPool::Instance* conn_pool_{new NullInstanceImpl()}; Stats::IsolatedStoreImpl store_; Event::SimulatedTimeSystem time_system_; - CommandSplitter::InstanceImpl splitter_{RouterPtr{router_}, store_, "redis.foo.", time_system_, - false}; + CommandSplitter::InstanceImpl splitter_{ConnPool::InstancePtr{conn_pool_}, store_, "redis.foo.", + time_system_, false}; NoOpSplitCallbacks callbacks_; CommandSplitter::SplitRequestPtr handle_; }; diff --git a/test/extensions/filters/network/redis_proxy/command_splitter_impl_test.cc b/test/extensions/filters/network/redis_proxy/command_splitter_impl_test.cc index ff52c8013496d..252078432334a 100644 --- a/test/extensions/filters/network/redis_proxy/command_splitter_impl_test.cc +++ b/test/extensions/filters/network/redis_proxy/command_splitter_impl_test.cc @@ -50,10 +50,11 @@ class RedisCommandSplitterImplTest : public testing::Test { value.asArray().swap(values); } - MockRouter* router_{new MockRouter()}; + ConnPool::MockInstance* conn_pool_{new ConnPool::MockInstance()}; NiceMock store_; Event::SimulatedTimeSystem time_system_; - InstanceImpl splitter_{RouterPtr{router_}, store_, "redis.foo.", time_system_, false}; + InstanceImpl splitter_{ConnPool::InstancePtr{conn_pool_}, store_, "redis.foo.", time_system_, + false}; MockSplitCallbacks callbacks_; SplitRequestPtr handle_; }; @@ -110,7 +111,7 @@ class RedisSingleServerRequestTest : public RedisCommandSplitterImplTest, public testing::WithParamInterface { public: void makeRequest(const std::string& hash_key, const Common::Redis::RespValue& request) { - EXPECT_CALL(*router_, makeRequest(hash_key, Ref(request), _)) + EXPECT_CALL(*conn_pool_, makeRequest(hash_key, Ref(request), _)) .WillOnce(DoAll(WithArg<2>(SaveArgAddress(&pool_callbacks_)), Return(&pool_request_))); handle_ = splitter_.makeRequest(request, callbacks_); } @@ -222,7 +223,7 @@ TEST_P(RedisSingleServerRequestTest, NoUpstream) { Common::Redis::RespValue request; makeBulkStringArray(request, {GetParam(), "hello"}); - EXPECT_CALL(*router_, makeRequest("hello", Ref(request), _)).WillOnce(Return(nullptr)); + EXPECT_CALL(*conn_pool_, makeRequest("hello", Ref(request), _)).WillOnce(Return(nullptr)); Common::Redis::RespValue response; response.type(Common::Redis::RespType::Error); response.asString() = Response::get().NoUpstreamHost; @@ -323,7 +324,7 @@ TEST_F(RedisSingleServerRequestTest, EvalNoUpstream) { Common::Redis::RespValue request; makeBulkStringArray(request, {"eval", "return {ARGV[1]}", "1", "key", "arg"}); - EXPECT_CALL(*router_, makeRequest("key", Ref(request), _)).WillOnce(Return(nullptr)); + EXPECT_CALL(*conn_pool_, makeRequest("key", Ref(request), _)).WillOnce(Return(nullptr)); Common::Redis::RespValue response; response.type(Common::Redis::RespType::Error); response.asString() = Response::get().NoUpstreamHost; @@ -358,7 +359,7 @@ class RedisMGETCommandHandlerTest : public RedisCommandSplitterImplTest { null_handle_indexes.end()) { request_to_use = &pool_requests_[i]; } - EXPECT_CALL(*router_, makeRequest(std::to_string(i), Eq(ByRef(expected_requests_[i])), _)) + EXPECT_CALL(*conn_pool_, makeRequest(std::to_string(i), Eq(ByRef(expected_requests_[i])), _)) .WillOnce(DoAll(WithArg<2>(SaveArgAddress(&pool_callbacks_[i])), Return(request_to_use))); } @@ -561,7 +562,7 @@ class RedisMSETCommandHandlerTest : public RedisCommandSplitterImplTest { null_handle_indexes.end()) { request_to_use = &pool_requests_[i]; } - EXPECT_CALL(*router_, makeRequest(std::to_string(i), Eq(ByRef(expected_requests_[i])), _)) + EXPECT_CALL(*conn_pool_, makeRequest(std::to_string(i), Eq(ByRef(expected_requests_[i])), _)) .WillOnce(DoAll(WithArg<2>(SaveArgAddress(&pool_callbacks_[i])), Return(request_to_use))); } @@ -684,7 +685,7 @@ class RedisSplitKeysSumResultHandlerTest : public RedisCommandSplitterImplTest, null_handle_indexes.end()) { request_to_use = &pool_requests_[i]; } - EXPECT_CALL(*router_, makeRequest(std::to_string(i), Eq(ByRef(expected_requests_[i])), _)) + EXPECT_CALL(*conn_pool_, makeRequest(std::to_string(i), Eq(ByRef(expected_requests_[i])), _)) .WillOnce(DoAll(WithArg<2>(SaveArgAddress(&pool_callbacks_[i])), Return(request_to_use))); } @@ -772,13 +773,14 @@ INSTANTIATE_TEST_SUITE_P( class RedisSingleServerRequestWithLatencyMicrosTest : public RedisSingleServerRequestTest { public: void makeRequest(const std::string& hash_key, const Common::Redis::RespValue& request) { - EXPECT_CALL(*router_, makeRequest(hash_key, Ref(request), _)) + EXPECT_CALL(*conn_pool_, makeRequest(hash_key, Ref(request), _)) .WillOnce(DoAll(WithArg<2>(SaveArgAddress(&pool_callbacks_)), Return(&pool_request_))); handle_ = splitter_.makeRequest(request, callbacks_); } - MockRouter* router_{new MockRouter()}; - InstanceImpl splitter_{RouterPtr{router_}, store_, "redis.foo.", time_system_, true}; + ConnPool::MockInstance* conn_pool_{new ConnPool::MockInstance()}; + InstanceImpl splitter_{ConnPool::InstancePtr{conn_pool_}, store_, "redis.foo.", time_system_, + true}; }; TEST_P(RedisSingleServerRequestWithLatencyMicrosTest, Success) { diff --git a/test/extensions/filters/network/redis_proxy/config_test.cc b/test/extensions/filters/network/redis_proxy/config_test.cc index be23782420b43..074862e5718c8 100644 --- a/test/extensions/filters/network/redis_proxy/config_test.cc +++ b/test/extensions/filters/network/redis_proxy/config_test.cc @@ -23,21 +23,6 @@ TEST(RedisProxyFilterConfigFactoryTest, ValidateFail) { ProtoValidationException); } -TEST(RedisProxyFilterConfigFactoryTest, NoUpstreamDefined) { - envoy::config::filter::network::redis_proxy::v2::RedisProxy::ConnPoolSettings settings; - settings.mutable_op_timeout()->CopyFrom(Protobuf::util::TimeUtil::MillisecondsToDuration(20)); - - envoy::config::filter::network::redis_proxy::v2::RedisProxy config; - config.set_stat_prefix("foo"); - config.mutable_settings()->CopyFrom(settings); - - NiceMock context; - - EXPECT_THROW_WITH_MESSAGE( - RedisProxyFilterConfigFactory().createFilterFactoryFromProto(config, context), EnvoyException, - "cannot configure a redis-proxy without any upstream"); -} - TEST(RedisProxyFilterConfigFactoryTest, RedisProxyCorrectJson) { std::string json_string = R"EOF( { diff --git a/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc b/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc index 464b1eff494f1..bd267cd1670d2 100644 --- a/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc +++ b/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc @@ -43,8 +43,7 @@ class RedisConnPoolImplTest : public testing::Test, public Common::Redis::Client if (!cluster_exists) { EXPECT_CALL(cm_, get("fake_cluster")).WillOnce(Return(nullptr)); } - - conn_pool_ = std::make_shared(cluster_name_, cm_, *this, tls_, + conn_pool_ = std::make_unique(cluster_name_, cm_, *this, tls_, Common::Redis::Client::createConnPoolSettings()); } @@ -75,7 +74,7 @@ class RedisConnPoolImplTest : public testing::Test, public Common::Redis::Client const std::string cluster_name_{"fake_cluster"}; NiceMock cm_; NiceMock tls_; - InstanceSharedPtr conn_pool_; + InstancePtr conn_pool_; Upstream::ClusterUpdateCallbacks* update_callbacks_{}; Common::Redis::Client::MockClient* client_{}; }; diff --git a/test/extensions/filters/network/redis_proxy/mocks.cc b/test/extensions/filters/network/redis_proxy/mocks.cc index 3bbb28baba804..7e0ce1eff0bde 100644 --- a/test/extensions/filters/network/redis_proxy/mocks.cc +++ b/test/extensions/filters/network/redis_proxy/mocks.cc @@ -15,9 +15,6 @@ namespace Extensions { namespace NetworkFilters { namespace RedisProxy { -MockRouter::MockRouter() {} -MockRouter::~MockRouter() {} - namespace ConnPool { MockInstance::MockInstance() {} diff --git a/test/extensions/filters/network/redis_proxy/mocks.h b/test/extensions/filters/network/redis_proxy/mocks.h index e959475542654..19c724ac74478 100644 --- a/test/extensions/filters/network/redis_proxy/mocks.h +++ b/test/extensions/filters/network/redis_proxy/mocks.h @@ -8,7 +8,6 @@ #include "extensions/filters/network/common/redis/codec_impl.h" #include "extensions/filters/network/redis_proxy/command_splitter.h" #include "extensions/filters/network/redis_proxy/conn_pool.h" -#include "extensions/filters/network/redis_proxy/router.h" #include "test/test_common/printers.h" @@ -19,17 +18,6 @@ namespace Extensions { namespace NetworkFilters { namespace RedisProxy { -class MockRouter : public Router { -public: - MockRouter(); - ~MockRouter(); - - MOCK_METHOD3(makeRequest, - Common::Redis::Client::PoolRequest*( - const std::string& hash_key, const Common::Redis::RespValue& request, - Common::Redis::Client::PoolCallbacks& callbacks)); -}; - namespace ConnPool { class MockInstance : public Instance { diff --git a/test/extensions/filters/network/redis_proxy/proxy_filter_test.cc b/test/extensions/filters/network/redis_proxy/proxy_filter_test.cc index 4cb73b89186b2..333a9687dc501 100644 --- a/test/extensions/filters/network/redis_proxy/proxy_filter_test.cc +++ b/test/extensions/filters/network/redis_proxy/proxy_filter_test.cc @@ -60,7 +60,7 @@ TEST_F(RedisProxyFilterConfigTest, Normal) { envoy::config::filter::network::redis_proxy::v2::RedisProxy proto_config = parseProtoFromJson(json_string); ProxyFilterConfig config(proto_config, store_, drain_decision_, runtime_); - EXPECT_EQ("redis.foo.", config.stat_prefix_); + EXPECT_EQ("fake_cluster", config.cluster_name_); } TEST_F(RedisProxyFilterConfigTest, BadRedisProxyConfig) { diff --git a/test/extensions/filters/network/redis_proxy/router_impl_test.cc b/test/extensions/filters/network/redis_proxy/router_impl_test.cc deleted file mode 100644 index 62b012e4abc27..0000000000000 --- a/test/extensions/filters/network/redis_proxy/router_impl_test.cc +++ /dev/null @@ -1,199 +0,0 @@ -#include - -#include "extensions/filters/network/redis_proxy/conn_pool_impl.h" -#include "extensions/filters/network/redis_proxy/router_impl.h" - -#include "test/extensions/filters/network/common/redis/mocks.h" -#include "test/extensions/filters/network/redis_proxy/mocks.h" -#include "test/test_common/utility.h" - -#include "gmock/gmock.h" -#include "gtest/gtest.h" - -using testing::_; -using testing::Eq; -using testing::InSequence; -using testing::Return; -using testing::StrEq; - -namespace Envoy { -namespace Extensions { -namespace NetworkFilters { -namespace RedisProxy { - -envoy::config::filter::network::redis_proxy::v2::RedisProxy::PrefixRoutes createPrefixRoutes() { - envoy::config::filter::network::redis_proxy::v2::RedisProxy::PrefixRoutes prefix_routes; - auto* routes = prefix_routes.mutable_routes(); - - { - auto* route = routes->Add(); - route->set_prefix("ab"); - route->set_cluster("fake_clusterA"); - } - - { - auto* route = routes->Add(); - route->set_prefix("a"); - route->set_cluster("fake_clusterB"); - } - - return prefix_routes; -} - -TEST(PrefixRoutesTest, MissingCatchAll) { - Upstreams upstreams; - upstreams.emplace("fake_clusterA", std::make_shared()); - upstreams.emplace("fake_clusterB", std::make_shared()); - - PrefixRoutes router(createPrefixRoutes(), std::move(upstreams)); - - Common::Redis::RespValue value; - Common::Redis::Client::MockPoolCallbacks callbacks; - - EXPECT_EQ(nullptr, router.makeRequest("c:bar", value, callbacks)); -} - -TEST(PrefixRoutesTest, RoutedToCatchAll) { - auto upstream_c = std::make_shared(); - - Upstreams upstreams; - upstreams.emplace("fake_clusterA", std::make_shared()); - upstreams.emplace("fake_clusterB", std::make_shared()); - upstreams.emplace("fake_clusterC", upstream_c); - - auto prefix_routes = createPrefixRoutes(); - prefix_routes.set_catch_all_cluster("fake_clusterC"); - - EXPECT_CALL(*upstream_c, makeRequest(Eq("c:bar"), _, _)); - - PrefixRoutes router(prefix_routes, std::move(upstreams)); - Common::Redis::RespValue value; - Common::Redis::Client::MockPoolCallbacks callbacks; - - EXPECT_EQ(nullptr, router.makeRequest("c:bar", value, callbacks)); -} - -TEST(PrefixRoutesTest, RoutedToLongestPrefix) { - auto upstream_a = std::make_shared(); - - Upstreams upstreams; - upstreams.emplace("fake_clusterA", upstream_a); - upstreams.emplace("fake_clusterB", std::make_shared()); - - EXPECT_CALL(*upstream_a, makeRequest(Eq("ab:bar"), _, _)); - - PrefixRoutes router(createPrefixRoutes(), std::move(upstreams)); - Common::Redis::RespValue value; - Common::Redis::Client::MockPoolCallbacks callbacks; - - EXPECT_EQ(nullptr, router.makeRequest("ab:bar", value, callbacks)); -} - -TEST(PrefixRoutesTest, CaseUnsensitivePrefix) { - auto upstream_a = std::make_shared(); - - Upstreams upstreams; - upstreams.emplace("fake_clusterA", upstream_a); - upstreams.emplace("fake_clusterB", std::make_shared()); - - auto prefix_routes = createPrefixRoutes(); - prefix_routes.set_case_insensitive(true); - - EXPECT_CALL(*upstream_a, makeRequest(Eq("AB:bar"), _, _)); - - PrefixRoutes router(prefix_routes, std::move(upstreams)); - Common::Redis::RespValue value; - Common::Redis::Client::MockPoolCallbacks callbacks; - - EXPECT_EQ(nullptr, router.makeRequest("AB:bar", value, callbacks)); -} - -TEST(PrefixRoutesTest, RemovePrefix) { - auto upstream_a = std::make_shared(); - - Upstreams upstreams; - upstreams.emplace("fake_clusterA", upstream_a); - upstreams.emplace("fake_clusterB", std::make_shared()); - - auto prefix_routes = createPrefixRoutes(); - - { - auto* route = prefix_routes.mutable_routes()->Add(); - route->set_prefix("abc"); - route->set_cluster("fake_clusterA"); - route->set_remove_prefix(true); - } - - EXPECT_CALL(*upstream_a, makeRequest(Eq(":bar"), _, _)); - - PrefixRoutes router(prefix_routes, std::move(upstreams)); - Common::Redis::RespValue value; - Common::Redis::Client::MockPoolCallbacks callbacks; - - EXPECT_EQ(nullptr, router.makeRequest("abc:bar", value, callbacks)); -} - -TEST(PrefixRoutesTest, RoutedToShortestPrefix) { - auto upstream_b = std::make_shared(); - - Upstreams upstreams; - upstreams.emplace("fake_clusterA", std::make_shared()); - upstreams.emplace("fake_clusterB", upstream_b); - - EXPECT_CALL(*upstream_b, makeRequest(Eq("a:bar"), _, _)); - - PrefixRoutes router(createPrefixRoutes(), std::move(upstreams)); - Common::Redis::RespValue value; - Common::Redis::Client::MockPoolCallbacks callbacks; - - EXPECT_EQ(nullptr, router.makeRequest("a:bar", value, callbacks)); -} - -TEST(PrefixRoutesTest, DifferentPrefixesSameUpstream) { - auto upstream_b = std::make_shared(); - - Upstreams upstreams; - upstreams.emplace("fake_clusterA", std::make_shared()); - upstreams.emplace("fake_clusterB", upstream_b); - - auto prefix_routes = createPrefixRoutes(); - - { - auto* route = prefix_routes.mutable_routes()->Add(); - route->set_prefix("also_route_to_b"); - route->set_cluster("fake_clusterB"); - } - - EXPECT_CALL(*upstream_b, makeRequest(Eq("a:bar"), _, _)); - EXPECT_CALL(*upstream_b, makeRequest(Eq("also_route_to_b:bar"), _, _)); - - PrefixRoutes router(prefix_routes, std::move(upstreams)); - Common::Redis::RespValue value; - Common::Redis::Client::MockPoolCallbacks callbacks; - - EXPECT_EQ(nullptr, router.makeRequest("a:bar", value, callbacks)); - EXPECT_EQ(nullptr, router.makeRequest("also_route_to_b:bar", value, callbacks)); -} - -TEST(PrefixRoutesTest, DuplicatePrefix) { - Upstreams upstreams; - upstreams.emplace("fake_clusterA", std::make_shared()); - upstreams.emplace("fake_clusterB", std::make_shared()); - upstreams.emplace("this_will_throw", std::make_shared()); - - auto prefix_routes = createPrefixRoutes(); - - { - auto* route = prefix_routes.mutable_routes()->Add(); - route->set_prefix("ab"); - route->set_cluster("this_will_throw"); - } - - EXPECT_THROW_WITH_MESSAGE(PrefixRoutes router(prefix_routes, std::move(upstreams)), - EnvoyException, "prefix `ab` already exists.") -} - -} // namespace RedisProxy -} // namespace NetworkFilters -} // namespace Extensions -} // namespace Envoy