From a36017e93022d4a25fae6ddb79560ed66f9969fb Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Mon, 14 Jan 2019 22:52:57 -0500 Subject: [PATCH 01/18] redis: prefixed routing Signed-off-by: Maxime Bedard --- DEPRECATED.md | 1 + .../network/redis_proxy/v2/redis_proxy.proto | 44 ++++- docs/root/intro/arch_overview/redis.rst | 5 +- docs/root/intro/version_history.rst | 1 + source/common/common/utility.h | 26 +++ .../filters/network/redis_proxy/BUILD | 29 ++- .../redis_proxy/command_splitter_impl.cc | 45 +++-- .../redis_proxy/command_splitter_impl.h | 35 ++-- .../filters/network/redis_proxy/config.cc | 42 +++- .../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 | 41 ++++ .../network/redis_proxy/router_impl.cc | 54 ++++++ .../filters/network/redis_proxy/router_impl.h | 58 ++++++ test/common/common/utility_test.cc | 17 ++ .../filters/network/redis_proxy/BUILD | 12 ++ .../redis_proxy/command_lookup_speed_test.cc | 12 +- .../redis_proxy/command_splitter_impl_test.cc | 24 ++- .../network/redis_proxy/config_test.cc | 22 +++ .../redis_proxy/conn_pool_impl_test.cc | 7 +- .../filters/network/redis_proxy/mocks.cc | 3 + .../filters/network/redis_proxy/mocks.h | 11 ++ .../network/redis_proxy/proxy_filter_test.cc | 2 +- .../network/redis_proxy/router_impl_test.cc | 181 ++++++++++++++++++ 26 files changed, 591 insertions(+), 87 deletions(-) create mode 100644 source/extensions/filters/network/redis_proxy/router.h create mode 100644 source/extensions/filters/network/redis_proxy/router_impl.cc create mode 100644 source/extensions/filters/network/redis_proxy/router_impl.h create mode 100644 test/extensions/filters/network/redis_proxy/router_impl_test.cc diff --git a/DEPRECATED.md b/DEPRECATED.md index 199e6bc6c01d4..2b847550101ab 100644 --- a/DEPRECATED.md +++ b/DEPRECATED.md @@ -13,6 +13,7 @@ A logged warning is expected for each deprecated item that is in deprecation win Set the `filter_enabled` field instead. * Use of google.protobuf.Struct for extension opaque configs is deprecated. Use google.protobuf.Any instead or pack google.protobuf.Struct in google.protobuf.Any. +* Use of `cluster` and `settings`, found in [redis-proxy.proto](https://github.com/envoyproxy/envoy/blob/master/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto). Set a `catch_all` route 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 cd8c18b128755..5519d153ee419 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,7 +22,13 @@ message RedisProxy { // Name of cluster from cluster manager. See the :ref:`configuration section // ` of the architecture overview for recommendations on // configuring the backing cluster. - string cluster = 2 [(validate.rules).string.min_bytes = 1]; + // + // .. attention:: + // + // This field is deprecated. Use a :ref:`catch-all + // route` + // instead. + string cluster = 2 [deprecated = true]; // Redis connection pool settings. message ConnPoolSettings { @@ -48,10 +54,44 @@ message RedisProxy { bool enable_hashtagging = 2; } - // Network settings for the connection pool to the upstream cluster. + // Network settings for the connection pool to the upstream clusters. 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 PrefixRoute { + // String prefix that must match the beginning of the keys. Envoy will always favor the longest + // matches. + string prefix = 1 [(validate.rules).string.min_bytes = 1]; + + // Indicates if the prefix needs to be removed from the key when forwarded. + bool preserve_prefix = 2; + + // Upstream cluster to forward the command to. + string cluster = 3; + } + + message PrefixRoutes { + // List of prefix routes. + repeated PrefixRoute 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 prefixes used to separate keys from different workloads to different clusters. Any + // overlapping prefixes will be ignored, and Envoy will always favor the longest matches first. A + // catch-all cluster can be used to forward commands to when there is no match. Time complexity of + // the lookups are in O(min(longest key prefix, key length)). + // + // 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 044ea66553726..b3aa16565ad78 100644 --- a/docs/root/intro/arch_overview/redis.rst +++ b/docs/root/intro/arch_overview/redis.rst @@ -8,7 +8,9 @@ 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. +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. The Redis project offers a thorough reference on partitioning as it relates to Redis. See "`Partitioning: how to split data among multiple Redis instances @@ -22,6 +24,7 @@ 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 a8e0813cd129c..800183f28e48b 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -48,6 +48,7 @@ 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 785df6d8aa404..564aea0b35697 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -599,6 +599,32 @@ 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 findPrefix(const char* key) const { + const TrieEntry* current = &root_; + const TrieEntry* result = nullptr; + while (uint8_t c = *key) { + if (current->value_) { + result = current; + } + + // TODO(maximebedard): this could be optimized + // https://github.com/facebook/mcrouter/blob/master/mcrouter/lib/fbi/cpp/Trie-inl.h#L126-L143 + current = current->entries_[c].get(); + if (current == nullptr) { + break; + } + + key++; + } + return result ? result->value_ : nullptr; + } + TrieEntry root_; }; diff --git a/source/extensions/filters/network/redis_proxy/BUILD b/source/extensions/filters/network/redis_proxy/BUILD index 8cd0a234462e0..911edafb83684 100644 --- a/source/extensions/filters/network/redis_proxy/BUILD +++ b/source/extensions/filters/network/redis_proxy/BUILD @@ -30,13 +30,22 @@ 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", - ":conn_pool_interface", + ":router_interface", "//include/envoy/stats:stats_macros", "//include/envoy/stats:timespan", "//source/common/common:assert_lib", @@ -54,7 +63,6 @@ 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", @@ -73,6 +81,7 @@ 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", @@ -95,7 +104,21 @@ 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:conn_pool_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", ], ) 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 beea0fbaa32ee..17c0c68be46e6 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(ConnPool::Instance& conn_pool, +SplitRequestPtr SimpleRequest::create(Router& router, 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_ = conn_pool.makeRequest(incoming_request.asArray()[1].asString(), - incoming_request, *request_ptr); + request_ptr->handle_ = + router.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(ConnPool::Instance& conn_pool, return std::move(request_ptr); } -SplitRequestPtr EvalRequest::create(ConnPool::Instance& conn_pool, +SplitRequestPtr EvalRequest::create(Router& router, const Common::Redis::RespValue& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros) { @@ -89,10 +89,9 @@ SplitRequestPtr EvalRequest::create(ConnPool::Instance& conn_pool, return nullptr; } - std::unique_ptr request_ptr{ - new EvalRequest(callbacks, command_stats, time_source, latency_in_micros)}; - request_ptr->handle_ = conn_pool.makeRequest(incoming_request.asArray()[3].asString(), - incoming_request, *request_ptr); + 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); if (!request_ptr->handle_) { command_stats.error_.inc(); request_ptr->callbacks_.onResponse(Utility::makeError(Response::get().NoUpstreamHost)); @@ -123,7 +122,7 @@ void FragmentedRequest::onChildFailure(uint32_t index) { onChildResponse(Utility::makeError(Response::get().UpstreamFailure), index); } -SplitRequestPtr MGETRequest::create(ConnPool::Instance& conn_pool, +SplitRequestPtr MGETRequest::create(Router& router, const Common::Redis::RespValue& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros) { @@ -152,8 +151,8 @@ SplitRequestPtr MGETRequest::create(ConnPool::Instance& conn_pool, single_mget.asArray()[1].asString() = incoming_request.asArray()[i].asString(); ENVOY_LOG(debug, "redis: parallel get: '{}'", single_mget.toString()); - pending_request.handle_ = conn_pool.makeRequest(incoming_request.asArray()[i].asString(), - single_mget, pending_request); + pending_request.handle_ = + router.makeRequest(incoming_request.asArray()[i].asString(), single_mget, pending_request); if (!pending_request.handle_) { pending_request.onResponse(Utility::makeError(Response::get().NoUpstreamHost)); } @@ -195,7 +194,7 @@ void MGETRequest::onChildResponse(Common::Redis::RespValuePtr&& value, uint32_t } } -SplitRequestPtr MSETRequest::create(ConnPool::Instance& conn_pool, +SplitRequestPtr MSETRequest::create(Router& router, const Common::Redis::RespValue& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros) { @@ -231,8 +230,8 @@ SplitRequestPtr MSETRequest::create(ConnPool::Instance& conn_pool, single_mset.asArray()[2].asString() = incoming_request.asArray()[i + 1].asString(); ENVOY_LOG(debug, "redis: parallel set: '{}'", single_mset.toString()); - pending_request.handle_ = conn_pool.makeRequest(incoming_request.asArray()[i].asString(), - single_mset, pending_request); + pending_request.handle_ = + router.makeRequest(incoming_request.asArray()[i].asString(), single_mset, pending_request); if (!pending_request.handle_) { pending_request.onResponse(Utility::makeError(Response::get().NoUpstreamHost)); } @@ -270,7 +269,7 @@ void MSETRequest::onChildResponse(Common::Redis::RespValuePtr&& value, uint32_t } } -SplitRequestPtr SplitKeysSumResultRequest::create(ConnPool::Instance& conn_pool, +SplitRequestPtr SplitKeysSumResultRequest::create(Router& router, const Common::Redis::RespValue& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, @@ -299,8 +298,8 @@ SplitRequestPtr SplitKeysSumResultRequest::create(ConnPool::Instance& conn_pool, 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_ = conn_pool.makeRequest(incoming_request.asArray()[i].asString(), - single_fragment, pending_request); + pending_request.handle_ = router.makeRequest(incoming_request.asArray()[i].asString(), + single_fragment, pending_request); if (!pending_request.handle_) { pending_request.onResponse(Utility::makeError(Response::get().NoUpstreamHost)); } @@ -337,12 +336,12 @@ void SplitKeysSumResultRequest::onChildResponse(Common::Redis::RespValuePtr&& va } } -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_), + +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_), 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 b7ac2b90f409b..0175edc9285fb 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.h +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.h @@ -17,6 +17,7 @@ #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 { @@ -68,9 +69,9 @@ class CommandHandler { class CommandHandlerBase { protected: - CommandHandlerBase(ConnPool::Instance& conn_pool) : conn_pool_(conn_pool) {} + CommandHandlerBase(Router& router) : router_(router) {} - ConnPool::Instance& conn_pool_; + Router& router_; }; class SplitRequestBase : public SplitRequest { @@ -121,8 +122,7 @@ class SingleServerRequest : public SplitRequestBase, public Common::Redis::Clien */ class SimpleRequest : public SingleServerRequest { public: - static SplitRequestPtr create(ConnPool::Instance& conn_pool, - const Common::Redis::RespValue& incoming_request, + static SplitRequestPtr create(Router& router, const Common::Redis::RespValue& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros); @@ -137,8 +137,7 @@ class SimpleRequest : public SingleServerRequest { */ class EvalRequest : public SingleServerRequest { public: - static SplitRequestPtr create(ConnPool::Instance& conn_pool, - const Common::Redis::RespValue& incoming_request, + static SplitRequestPtr create(Router& router, const Common::Redis::RespValue& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros); @@ -195,8 +194,7 @@ class FragmentedRequest : public SplitRequestBase { */ class MGETRequest : public FragmentedRequest, Logger::Loggable { public: - static SplitRequestPtr create(ConnPool::Instance& conn_pool, - const Common::Redis::RespValue& incoming_request, + static SplitRequestPtr create(Router& router, const Common::Redis::RespValue& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros); @@ -217,8 +215,7 @@ class MGETRequest : public FragmentedRequest, Logger::Loggable { public: - static SplitRequestPtr create(ConnPool::Instance& conn_pool, - const Common::Redis::RespValue& incoming_request, + static SplitRequestPtr create(Router& router, const Common::Redis::RespValue& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros); @@ -240,11 +237,9 @@ class SplitKeysSumResultRequest : public FragmentedRequest, Logger::Loggable { public: - static SplitRequestPtr create(ConnPool::Instance& conn_pool, - const Common::Redis::RespValue& incoming_request, + static SplitRequestPtr create(Router& router, const Common::Redis::RespValue& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros); - private: MSETRequest(SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros) @@ -261,12 +256,10 @@ class MSETRequest : public FragmentedRequest, Logger::Loggable class CommandHandlerFactory : public CommandHandler, CommandHandlerBase { public: - CommandHandlerFactory(ConnPool::Instance& conn_pool) : CommandHandlerBase(conn_pool) {} + CommandHandlerFactory(Router& router) : CommandHandlerBase(router) {} SplitRequestPtr startRequest(const Common::Redis::RespValue& request, SplitCallbacks& callbacks, - CommandStats& command_stats, TimeSource& time_source, - bool latency_in_micros) { - return RequestClass::create(conn_pool_, request, callbacks, command_stats, time_source, - latency_in_micros); + CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros) { + return RequestClass::create(router_, request, callbacks, command_stats, time_source, latency_in_micros); } }; @@ -288,8 +281,8 @@ struct InstanceStats { class InstanceImpl : public Instance, Logger::Loggable { public: - InstanceImpl(ConnPool::InstancePtr&& conn_pool, Stats::Scope& scope, - const std::string& stat_prefix, TimeSource& time_source, bool latency_in_micros); + InstanceImpl(RouterPtr&& router, 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, @@ -307,7 +300,7 @@ class InstanceImpl : public Instance, Logger::Loggable { CommandHandler& handler); void onInvalidRequest(SplitCallbacks& callbacks); - ConnPool::InstancePtr conn_pool_; + RouterPtr router_; 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 bae74e8633713..0a274e91c1948 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,18 +24,42 @@ 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())); - 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())); + + 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()); 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 442219e79b547..713e4f7310cc5 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::unique_ptr InstancePtr; +typedef std::shared_ptr InstanceSharedPtr; } // 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 1dfb363573ab2..17facb93afbc4 100644 --- a/source/extensions/filters/network/redis_proxy/conn_pool_impl.h +++ b/source/extensions/filters/network/redis_proxy/conn_pool_impl.h @@ -37,7 +37,6 @@ 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 d5fc143e9be09..f36d692ea9cae 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), cluster_name_(config.cluster()), + : drain_decision_(drain_decision), runtime_(runtime), 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 3f8dc62d6eecd..ae2141a322d94 100644 --- a/source/extensions/filters/network/redis_proxy/proxy_filter.h +++ b/source/extensions/filters/network/redis_proxy/proxy_filter.h @@ -56,7 +56,6 @@ 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 new file mode 100644 index 0000000000000..5dd09e2ce70b5 --- /dev/null +++ b/source/extensions/filters/network/redis_proxy/router.h @@ -0,0 +1,41 @@ +#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 new file mode 100644 index 0000000000000..ea082359e60ae --- /dev/null +++ b/source/extensions/filters/network/redis_proxy/router_impl.cc @@ -0,0 +1,54 @@ +#include "extensions/filters/network/redis_proxy/router_impl.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); + } + + prefix_lookup_table_.add(copy.c_str(), std::make_shared(Prefix{ + route.prefix(), + route.preserve_prefix(), + upstreams_.at(route.cluster()), + })); + } +} + +Common::Redis::Client::PoolRequest* PrefixRoutes::makeRequest(const std::string& key, const Common::Redis::RespValue& request, + Common::Redis::Client::PoolCallbacks& callbacks) { + + std::string copy(key); + if (case_insensitive_) { + to_lower_table_.toLowerCase(copy); + } + + auto value = prefix_lookup_table_.findPrefix(copy.c_str()); + + if (value != nullptr) { + // TODO: preserve_prefix + value->upstream->makeRequest(key, 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 new file mode 100644 index 0000000000000..a0e1c8beba4a1 --- /dev/null +++ b/source/extensions/filters/network/redis_proxy/router_impl.h @@ -0,0 +1,58 @@ +#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 preserve_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 6434cd140280b..e404d5ee2d7f8 100644 --- a/test/common/common/utility_test.cc +++ b/test/common/common/utility_test.cc @@ -828,4 +828,21 @@ TEST(DateFormatter, FromTimeSameWildcard) { DateFormatter("%Y-%m-%dT%H:%M:%S.000Z%1f%2f").fromTime(time1)); } +TEST(TrieLookupTable, LongestPrefix) { + TrieLookupTable trie; + trie.add("foo", "a"); + trie.add("bar", "b"); + trie.add("baro", "c"); + + EXPECT_EQ("a", trie.find("foo")); + EXPECT_EQ("a", trie.findPrefix("foosball")); + + EXPECT_EQ("b", trie.find("bar")); + EXPECT_EQ("b", trie.findPrefix("baritone")); + EXPECT_EQ("c", trie.findPrefix("barometer")); + + EXPECT_EQ(nullptr, trie.find("toto")); + EXPECT_EQ(nullptr, trie.findPrefix("toto")); +} + } // namespace Envoy diff --git a/test/extensions/filters/network/redis_proxy/BUILD b/test/extensions/filters/network/redis_proxy/BUILD index 492404c41547e..8c469d90008e3 100644 --- a/test/extensions/filters/network/redis_proxy/BUILD +++ b/test/extensions/filters/network/redis_proxy/BUILD @@ -75,6 +75,7 @@ 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", ], ) @@ -104,3 +105,14 @@ 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", + "//test/extensions/filters/network/common/redis:redis_mocks", + "//source/extensions/filters/network/redis_proxy:router_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 2f4d8e30e1b0b..0f49cdde5fa2a 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,10 +30,9 @@ class NoOpSplitCallbacks : public CommandSplitter::SplitCallbacks { void onResponse(Common::Redis::RespValuePtr&&) override {} }; -class NullInstanceImpl : public ConnPool::Instance { - Common::Redis::Client::PoolRequest* makeRequest(const std::string&, - const Common::Redis::RespValue&, - Common::Redis::Client::PoolCallbacks&) override { +class NullRouterImpl : public Router { + Common::Redis::Client::PoolRequest* makeRequest(const std::string&, const Common::Redis::RespValue&, + Common::Redis::Client::PoolCallbacks&) override { return nullptr; } }; @@ -65,11 +64,10 @@ class CommandLookUpSpeedTest { } } - ConnPool::Instance* conn_pool_{new NullInstanceImpl()}; + Router* router_{new NullRouterImpl()}; Stats::IsolatedStoreImpl store_; Event::SimulatedTimeSystem time_system_; - CommandSplitter::InstanceImpl splitter_{ConnPool::InstancePtr{conn_pool_}, store_, "redis.foo.", - time_system_, false}; + CommandSplitter::InstanceImpl splitter_{RouterPtr{router_}, 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 252078432334a..ff52c8013496d 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,11 +50,10 @@ class RedisCommandSplitterImplTest : public testing::Test { value.asArray().swap(values); } - ConnPool::MockInstance* conn_pool_{new ConnPool::MockInstance()}; + MockRouter* router_{new MockRouter()}; NiceMock store_; Event::SimulatedTimeSystem time_system_; - InstanceImpl splitter_{ConnPool::InstancePtr{conn_pool_}, store_, "redis.foo.", time_system_, - false}; + InstanceImpl splitter_{RouterPtr{router_}, store_, "redis.foo.", time_system_, false}; MockSplitCallbacks callbacks_; SplitRequestPtr handle_; }; @@ -111,7 +110,7 @@ class RedisSingleServerRequestTest : public RedisCommandSplitterImplTest, public testing::WithParamInterface { public: void makeRequest(const std::string& hash_key, const Common::Redis::RespValue& request) { - EXPECT_CALL(*conn_pool_, makeRequest(hash_key, Ref(request), _)) + EXPECT_CALL(*router_, makeRequest(hash_key, Ref(request), _)) .WillOnce(DoAll(WithArg<2>(SaveArgAddress(&pool_callbacks_)), Return(&pool_request_))); handle_ = splitter_.makeRequest(request, callbacks_); } @@ -223,7 +222,7 @@ TEST_P(RedisSingleServerRequestTest, NoUpstream) { Common::Redis::RespValue request; makeBulkStringArray(request, {GetParam(), "hello"}); - EXPECT_CALL(*conn_pool_, makeRequest("hello", Ref(request), _)).WillOnce(Return(nullptr)); + EXPECT_CALL(*router_, makeRequest("hello", Ref(request), _)).WillOnce(Return(nullptr)); Common::Redis::RespValue response; response.type(Common::Redis::RespType::Error); response.asString() = Response::get().NoUpstreamHost; @@ -324,7 +323,7 @@ TEST_F(RedisSingleServerRequestTest, EvalNoUpstream) { Common::Redis::RespValue request; makeBulkStringArray(request, {"eval", "return {ARGV[1]}", "1", "key", "arg"}); - EXPECT_CALL(*conn_pool_, makeRequest("key", Ref(request), _)).WillOnce(Return(nullptr)); + EXPECT_CALL(*router_, makeRequest("key", Ref(request), _)).WillOnce(Return(nullptr)); Common::Redis::RespValue response; response.type(Common::Redis::RespType::Error); response.asString() = Response::get().NoUpstreamHost; @@ -359,7 +358,7 @@ class RedisMGETCommandHandlerTest : public RedisCommandSplitterImplTest { null_handle_indexes.end()) { request_to_use = &pool_requests_[i]; } - EXPECT_CALL(*conn_pool_, makeRequest(std::to_string(i), Eq(ByRef(expected_requests_[i])), _)) + EXPECT_CALL(*router_, makeRequest(std::to_string(i), Eq(ByRef(expected_requests_[i])), _)) .WillOnce(DoAll(WithArg<2>(SaveArgAddress(&pool_callbacks_[i])), Return(request_to_use))); } @@ -562,7 +561,7 @@ class RedisMSETCommandHandlerTest : public RedisCommandSplitterImplTest { null_handle_indexes.end()) { request_to_use = &pool_requests_[i]; } - EXPECT_CALL(*conn_pool_, makeRequest(std::to_string(i), Eq(ByRef(expected_requests_[i])), _)) + EXPECT_CALL(*router_, makeRequest(std::to_string(i), Eq(ByRef(expected_requests_[i])), _)) .WillOnce(DoAll(WithArg<2>(SaveArgAddress(&pool_callbacks_[i])), Return(request_to_use))); } @@ -685,7 +684,7 @@ class RedisSplitKeysSumResultHandlerTest : public RedisCommandSplitterImplTest, null_handle_indexes.end()) { request_to_use = &pool_requests_[i]; } - EXPECT_CALL(*conn_pool_, makeRequest(std::to_string(i), Eq(ByRef(expected_requests_[i])), _)) + EXPECT_CALL(*router_, makeRequest(std::to_string(i), Eq(ByRef(expected_requests_[i])), _)) .WillOnce(DoAll(WithArg<2>(SaveArgAddress(&pool_callbacks_[i])), Return(request_to_use))); } @@ -773,14 +772,13 @@ INSTANTIATE_TEST_SUITE_P( class RedisSingleServerRequestWithLatencyMicrosTest : public RedisSingleServerRequestTest { public: void makeRequest(const std::string& hash_key, const Common::Redis::RespValue& request) { - EXPECT_CALL(*conn_pool_, makeRequest(hash_key, Ref(request), _)) + EXPECT_CALL(*router_, makeRequest(hash_key, Ref(request), _)) .WillOnce(DoAll(WithArg<2>(SaveArgAddress(&pool_callbacks_)), Return(&pool_request_))); handle_ = splitter_.makeRequest(request, callbacks_); } - ConnPool::MockInstance* conn_pool_{new ConnPool::MockInstance()}; - InstanceImpl splitter_{ConnPool::InstancePtr{conn_pool_}, store_, "redis.foo.", time_system_, - true}; + MockRouter* router_{new MockRouter()}; + InstanceImpl splitter_{RouterPtr{router_}, 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 074862e5718c8..1613ee4efbfb5 100644 --- a/test/extensions/filters/network/redis_proxy/config_test.cc +++ b/test/extensions/filters/network/redis_proxy/config_test.cc @@ -23,6 +23,28 @@ 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( + { + try { + RedisProxyFilterConfigFactory().createFilterFactoryFromProto(config, context); + } catch (const EnvoyException& ex) { + EXPECT_STREQ("cannot configure a redis-proxy without any upstream", ex.what()); + throw; + } + }, + EnvoyException); +} + 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 bd267cd1670d2..f364b03d49ea9 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,9 @@ 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_unique(cluster_name_, cm_, *this, tls_, - Common::Redis::Client::createConnPoolSettings()); + + conn_pool_ = + std::make_shared(cluster_name_, cm_, *this, tls_, Common::Redis::Client::createConnPoolSettings()); } void makeSimpleRequest(bool create_client) { @@ -74,7 +75,7 @@ class RedisConnPoolImplTest : public testing::Test, public Common::Redis::Client const std::string cluster_name_{"fake_cluster"}; NiceMock cm_; NiceMock tls_; - InstancePtr conn_pool_; + InstanceSharedPtr 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 7e0ce1eff0bde..3bbb28baba804 100644 --- a/test/extensions/filters/network/redis_proxy/mocks.cc +++ b/test/extensions/filters/network/redis_proxy/mocks.cc @@ -15,6 +15,9 @@ 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 19c724ac74478..59dec19f1cc5d 100644 --- a/test/extensions/filters/network/redis_proxy/mocks.h +++ b/test/extensions/filters/network/redis_proxy/mocks.h @@ -8,6 +8,7 @@ #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" @@ -18,6 +19,16 @@ 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 333a9687dc501..4cb73b89186b2 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("fake_cluster", config.cluster_name_); + EXPECT_EQ("redis.foo.", config.stat_prefix_); } 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 new file mode 100644 index 0000000000000..2c0172bb911e0 --- /dev/null +++ b/test/extensions/filters/network/redis_proxy/router_impl_test.cc @@ -0,0 +1,181 @@ +#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 "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)); +} + +// TODO(maximebedard) +// TEST(PrefixRoutesTest, StrippedPrefix) { +// 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_preserve_prefix(false); +// } + +// 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)); +} + +} // namespace RedisProxy +} // namespace NetworkFilters +} // namespace Extensions +} // namespace Envoy From de8847096426501b7a93fc00f802467258b8cc99 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Tue, 5 Mar 2019 15:24:48 -0500 Subject: [PATCH 02/18] Formatting Signed-off-by: Maxime Bedard --- source/extensions/filters/network/redis_proxy/router_impl.h | 1 - 1 file changed, 1 deletion(-) diff --git a/source/extensions/filters/network/redis_proxy/router_impl.h b/source/extensions/filters/network/redis_proxy/router_impl.h index a0e1c8beba4a1..9bf101818fd9c 100644 --- a/source/extensions/filters/network/redis_proxy/router_impl.h +++ b/source/extensions/filters/network/redis_proxy/router_impl.h @@ -31,7 +31,6 @@ class PrefixRoutes : public Router { 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; From da0e9582c5534466875e76609259ec21e3964298 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Mon, 11 Mar 2019 10:13:02 -0400 Subject: [PATCH 03/18] Address comments Signed-off-by: Maxime Bedard --- DEPRECATED.md | 2 +- .../network/redis_proxy/v2/redis_proxy.proto | 35 +++++++++++++------ .../network/redis_proxy/router_impl.cc | 4 +-- .../filters/network/redis_proxy/router_impl.h | 2 +- 4 files changed, 28 insertions(+), 15 deletions(-) diff --git a/DEPRECATED.md b/DEPRECATED.md index 2b847550101ab..24ae94a18daa4 100644 --- a/DEPRECATED.md +++ b/DEPRECATED.md @@ -13,7 +13,7 @@ A logged warning is expected for each deprecated item that is in deprecation win Set the `filter_enabled` field instead. * Use of google.protobuf.Struct for extension opaque configs is deprecated. Use google.protobuf.Any instead or pack google.protobuf.Struct in google.protobuf.Any. -* Use of `cluster` and `settings`, found in [redis-proxy.proto](https://github.com/envoyproxy/envoy/blob/master/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto). Set a `catch_all` route instead. +* Use of `cluster` and `settings`, found in [redis-proxy.proto](https://github.com/envoyproxy/envoy/blob/master/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto). Set a `catch_all_cluster` route 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 5519d153ee419..0bb0c72230005 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 @@ -61,21 +61,21 @@ message RedisProxy { // milliseconds. bool latency_in_micros = 4; - message PrefixRoute { - // String prefix that must match the beginning of the keys. Envoy will always favor the longest - // matches. - string prefix = 1 [(validate.rules).string.min_bytes = 1]; + message PrefixRoutes { + message Route { + // String prefix that must match the beginning of the keys. Envoy will always favor the longest + // matches. + string prefix = 1 [(validate.rules).string.min_bytes = 1]; - // Indicates if the prefix needs to be removed from the key when forwarded. - bool preserve_prefix = 2; + // 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; - } + // Upstream cluster to forward the command to. + string cluster = 3; + } - message PrefixRoutes { // List of prefix routes. - repeated PrefixRoute routes = 1 [(gogoproto.nullable) = false]; + repeated Route routes = 1 [(gogoproto.nullable) = false]; // Indicates that prefix matching should be case insensitive. bool case_insensitive = 2; @@ -90,6 +90,19 @@ message RedisProxy { // catch-all cluster can be used to forward commands to 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: "abc" + // cluster: "cluster_a" + // # the following route will be ignored + // - prefix: "abc" + // cluster: "cluster_b" + // - prefix: "a" + // cluster: "cluster_c" + // // See the :ref:`configuration section // ` of the architecture overview for recommendations on // configuring the backing clusters. diff --git a/source/extensions/filters/network/redis_proxy/router_impl.cc b/source/extensions/filters/network/redis_proxy/router_impl.cc index ea082359e60ae..8155a00c8c36d 100644 --- a/source/extensions/filters/network/redis_proxy/router_impl.cc +++ b/source/extensions/filters/network/redis_proxy/router_impl.cc @@ -22,7 +22,7 @@ PrefixRoutes::PrefixRoutes( prefix_lookup_table_.add(copy.c_str(), std::make_shared(Prefix{ route.prefix(), - route.preserve_prefix(), + route.remove_prefix(), upstreams_.at(route.cluster()), })); } @@ -39,7 +39,7 @@ Common::Redis::Client::PoolRequest* PrefixRoutes::makeRequest(const std::string& auto value = prefix_lookup_table_.findPrefix(copy.c_str()); if (value != nullptr) { - // TODO: preserve_prefix + // TODO: remove_prefix value->upstream->makeRequest(key, request, callbacks); } else if (catch_all_upstream_ != nullptr) { catch_all_upstream_.value()->makeRequest(key, request, callbacks); diff --git a/source/extensions/filters/network/redis_proxy/router_impl.h b/source/extensions/filters/network/redis_proxy/router_impl.h index 9bf101818fd9c..b1dedfac00de4 100644 --- a/source/extensions/filters/network/redis_proxy/router_impl.h +++ b/source/extensions/filters/network/redis_proxy/router_impl.h @@ -34,7 +34,7 @@ class PrefixRoutes : public Router { private: struct Prefix { const std::string prefix; - const bool preserve_prefix; + const bool remove_prefix; ConnPool::InstanceSharedPtr upstream; }; From 3b12bcba5b99a2b903993f95ddd393632bcf5805 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Mon, 11 Mar 2019 10:14:37 -0400 Subject: [PATCH 04/18] formatting Signed-off-by: Maxime Bedard --- .../config/filter/network/redis_proxy/v2/redis_proxy.proto | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 0bb0c72230005..0cb9f3647018d 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 @@ -63,8 +63,8 @@ message RedisProxy { message PrefixRoutes { message Route { - // String prefix that must match the beginning of the keys. Envoy will always favor the longest - // matches. + // String prefix that must match the beginning of the keys. Envoy will always favor the + // longest matches. string prefix = 1 [(validate.rules).string.min_bytes = 1]; // Indicates if the prefix needs to be removed from the key when forwarded. From 0fde3aa604d36de942c97fe8b8912a4df2ec38a0 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Mon, 11 Mar 2019 10:46:45 -0400 Subject: [PATCH 05/18] Formatting Signed-off-by: Maxime Bedard --- source/extensions/filters/network/redis_proxy/config.cc | 8 ++++---- source/extensions/filters/network/redis_proxy/router.h | 5 +++-- .../extensions/filters/network/redis_proxy/router_impl.cc | 5 +++-- .../extensions/filters/network/redis_proxy/router_impl.h | 6 ++++-- test/extensions/filters/network/redis_proxy/BUILD | 2 +- .../network/redis_proxy/command_lookup_speed_test.cc | 5 +++-- .../filters/network/redis_proxy/conn_pool_impl_test.cc | 4 ++-- test/extensions/filters/network/redis_proxy/mocks.h | 5 +++-- 8 files changed, 23 insertions(+), 17 deletions(-) diff --git a/source/extensions/filters/network/redis_proxy/config.cc b/source/extensions/filters/network/redis_proxy/config.cc index 0a274e91c1948..c150b2bae08f3 100644 --- a/source/extensions/filters/network/redis_proxy/config.cc +++ b/source/extensions/filters/network/redis_proxy/config.cc @@ -49,10 +49,10 @@ Network::FilterFactoryCb RedisProxyFilterConfigFactory::createFilterFactoryFromP 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())); + 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)); diff --git a/source/extensions/filters/network/redis_proxy/router.h b/source/extensions/filters/network/redis_proxy/router.h index 5dd09e2ce70b5..1317b170aca4c 100644 --- a/source/extensions/filters/network/redis_proxy/router.h +++ b/source/extensions/filters/network/redis_proxy/router.h @@ -29,8 +29,9 @@ class Router { * @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; + 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; diff --git a/source/extensions/filters/network/redis_proxy/router_impl.cc b/source/extensions/filters/network/redis_proxy/router_impl.cc index 8155a00c8c36d..90e95295cdb80 100644 --- a/source/extensions/filters/network/redis_proxy/router_impl.cc +++ b/source/extensions/filters/network/redis_proxy/router_impl.cc @@ -28,8 +28,9 @@ PrefixRoutes::PrefixRoutes( } } -Common::Redis::Client::PoolRequest* PrefixRoutes::makeRequest(const std::string& key, const Common::Redis::RespValue& request, - Common::Redis::Client::PoolCallbacks& callbacks) { +Common::Redis::Client::PoolRequest* +PrefixRoutes::makeRequest(const std::string& key, const Common::Redis::RespValue& request, + Common::Redis::Client::PoolCallbacks& callbacks) { std::string copy(key); if (case_insensitive_) { diff --git a/source/extensions/filters/network/redis_proxy/router_impl.h b/source/extensions/filters/network/redis_proxy/router_impl.h index b1dedfac00de4..535db37b9f846 100644 --- a/source/extensions/filters/network/redis_proxy/router_impl.h +++ b/source/extensions/filters/network/redis_proxy/router_impl.h @@ -29,8 +29,10 @@ class PrefixRoutes : public Router { 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; + 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; diff --git a/test/extensions/filters/network/redis_proxy/BUILD b/test/extensions/filters/network/redis_proxy/BUILD index 8c469d90008e3..861336d56675b 100644 --- a/test/extensions/filters/network/redis_proxy/BUILD +++ b/test/extensions/filters/network/redis_proxy/BUILD @@ -112,7 +112,7 @@ envoy_extension_cc_test( extension_name = "envoy.filters.network.redis_proxy", deps = [ ":redis_mocks", - "//test/extensions/filters/network/common/redis:redis_mocks", "//source/extensions/filters/network/redis_proxy:router_lib", + "//test/extensions/filters/network/common/redis:redis_mocks", ], ) 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 0f49cdde5fa2a..03beee6a88314 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 @@ -31,8 +31,9 @@ class NoOpSplitCallbacks : public CommandSplitter::SplitCallbacks { }; class NullRouterImpl : public Router { - Common::Redis::Client::PoolRequest* makeRequest(const std::string&, const Common::Redis::RespValue&, - Common::Redis::Client::PoolCallbacks&) override { + Common::Redis::Client::PoolRequest* makeRequest(const std::string&, + const Common::Redis::RespValue&, + Common::Redis::Client::PoolCallbacks&) override { return nullptr; } }; 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 f364b03d49ea9..464b1eff494f1 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 @@ -44,8 +44,8 @@ class RedisConnPoolImplTest : public testing::Test, public Common::Redis::Client EXPECT_CALL(cm_, get("fake_cluster")).WillOnce(Return(nullptr)); } - conn_pool_ = - std::make_shared(cluster_name_, cm_, *this, tls_, Common::Redis::Client::createConnPoolSettings()); + conn_pool_ = std::make_shared(cluster_name_, cm_, *this, tls_, + Common::Redis::Client::createConnPoolSettings()); } void makeSimpleRequest(bool create_client) { diff --git a/test/extensions/filters/network/redis_proxy/mocks.h b/test/extensions/filters/network/redis_proxy/mocks.h index 59dec19f1cc5d..e959475542654 100644 --- a/test/extensions/filters/network/redis_proxy/mocks.h +++ b/test/extensions/filters/network/redis_proxy/mocks.h @@ -25,8 +25,9 @@ class MockRouter : public Router { ~MockRouter(); MOCK_METHOD3(makeRequest, - Common::Redis::Client::PoolRequest*(const std::string& hash_key, const Common::Redis::RespValue& request, - Common::Redis::Client::PoolCallbacks& callbacks)); + Common::Redis::Client::PoolRequest*( + const std::string& hash_key, const Common::Redis::RespValue& request, + Common::Redis::Client::PoolCallbacks& callbacks)); }; namespace ConnPool { From 164159fda68befed31721600acdc801435020e7a Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Mon, 11 Mar 2019 11:48:55 -0400 Subject: [PATCH 06/18] indenting Signed-off-by: Maxime Bedard --- .../network/redis_proxy/v2/redis_proxy.proto | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) 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 0cb9f3647018d..9d43c0abc50f9 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 @@ -93,15 +93,15 @@ message RedisProxy { // Example: // .. code-block:: yaml // - // prefix_routes: - // routes: - // - prefix: "abc" - // cluster: "cluster_a" - // # the following route will be ignored - // - prefix: "abc" - // cluster: "cluster_b" - // - prefix: "a" - // cluster: "cluster_c" + // prefix_routes: + // routes: + // - prefix: "abc" + // cluster: "cluster_a" + // # the following route will be ignored + // - prefix: "abc" + // cluster: "cluster_b" + // - prefix: "a" + // cluster: "cluster_c" // // See the :ref:`configuration section // ` of the architecture overview for recommendations on From 36b5ee261d473f3d53076621111ae77f615f7814 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Tue, 12 Mar 2019 09:41:12 -0400 Subject: [PATCH 07/18] Try to fix doc warning Signed-off-by: Maxime Bedard --- .../config/filter/network/redis_proxy/v2/redis_proxy.proto | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 9d43c0abc50f9..8192402b2b954 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 @@ -91,14 +91,14 @@ message RedisProxy { // the lookups are in O(min(longest key prefix, key length)). // // Example: + // // .. code-block:: yaml // // prefix_routes: // routes: // - prefix: "abc" // cluster: "cluster_a" - // # the following route will be ignored - // - prefix: "abc" + // - prefix: "abc" # this prefix will be ignored. // cluster: "cluster_b" // - prefix: "a" // cluster: "cluster_c" From 74c22483c3ba3b72270f36ed3e6d35acff0e34a7 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Tue, 19 Mar 2019 09:26:11 -0400 Subject: [PATCH 08/18] more formatting Signed-off-by: Maxime Bedard --- .../filters/network/redis_proxy/command_splitter_impl.cc | 4 ++-- .../filters/network/redis_proxy/command_splitter_impl.h | 7 +++++-- source/extensions/filters/network/redis_proxy/config.cc | 3 ++- .../network/redis_proxy/command_lookup_speed_test.cc | 3 ++- 4 files changed, 11 insertions(+), 6 deletions(-) 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 17c0c68be46e6..415a754e0ac6a 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc @@ -89,7 +89,8 @@ SplitRequestPtr EvalRequest::create(Router& router, return nullptr; } - std::unique_ptr request_ptr{new EvalRequest(callbacks, command_stats, time_source, latency_in_micros)}; + 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); if (!request_ptr->handle_) { @@ -336,7 +337,6 @@ 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_), 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 0175edc9285fb..45ac46b71cd37 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.h +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.h @@ -240,6 +240,7 @@ class MSETRequest : public FragmentedRequest, Logger::Loggable splitter = std::make_shared( - std::move(router), context.scope(), filter_config->stat_prefix_, context.timeSource(), proto_config.latency_in_micros()); + std::move(router), 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/test/extensions/filters/network/redis_proxy/command_lookup_speed_test.cc b/test/extensions/filters/network/redis_proxy/command_lookup_speed_test.cc index 03beee6a88314..d70fdb02a5e02 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 @@ -68,7 +68,8 @@ class CommandLookUpSpeedTest { Router* router_{new NullRouterImpl()}; Stats::IsolatedStoreImpl store_; Event::SimulatedTimeSystem time_system_; - CommandSplitter::InstanceImpl splitter_{RouterPtr{router_}, store_, "redis.foo.", time_system_, false}; + CommandSplitter::InstanceImpl splitter_{RouterPtr{router_}, store_, "redis.foo.", time_system_, + false}; NoOpSplitCallbacks callbacks_; CommandSplitter::SplitRequestPtr handle_; }; From 8dd1e5fd135f007eb1684ced1dac2e130de21569 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Tue, 19 Mar 2019 15:20:55 -0400 Subject: [PATCH 09/18] update doc and add missing tests to cover prefix lookup edge cases Signed-off-by: Maxime Bedard --- DEPRECATED.md | 2 +- .../filter/network/redis_proxy/v2/redis_proxy.proto | 12 ++++++------ source/common/common/utility.h | 5 ++--- test/common/common/utility_test.cc | 5 +++++ 4 files changed, 14 insertions(+), 10 deletions(-) diff --git a/DEPRECATED.md b/DEPRECATED.md index 24ae94a18daa4..0e331d82556af 100644 --- a/DEPRECATED.md +++ b/DEPRECATED.md @@ -13,7 +13,7 @@ A logged warning is expected for each deprecated item that is in deprecation win Set the `filter_enabled` field instead. * Use of google.protobuf.Struct for extension opaque configs is deprecated. Use google.protobuf.Any instead or pack google.protobuf.Struct in google.protobuf.Any. -* Use of `cluster` and `settings`, found in [redis-proxy.proto](https://github.com/envoyproxy/envoy/blob/master/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto). Set a `catch_all_cluster` route instead. +* 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 8192402b2b954..3c127ed98848e 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 @@ -85,10 +85,10 @@ message RedisProxy { string catch_all_cluster = 3; } - // List of prefixes used to separate keys from different workloads to different clusters. Any - // overlapping prefixes will be ignored, and Envoy will always favor the longest matches first. A - // catch-all cluster can be used to forward commands to when there is no match. Time complexity of - // the lookups are in O(min(longest key prefix, key length)). + // List of prefixes used to separate keys from different workloads to different clusters. Envoy will always + // favor the longest matches first. Prefixes are stored in a Trie and are accessed using a LIFO, thus identical + // prefixes will always resolve to the last entry inserted. A catch-all cluster can be used to forward commands + // to when there is no match. Time complexity of the lookups are in O(min(longest key prefix, key length)). // // Example: // @@ -96,9 +96,9 @@ message RedisProxy { // // prefix_routes: // routes: - // - prefix: "abc" + // - prefix: "abc" # this prefix will be overwritten by the next entry // cluster: "cluster_a" - // - prefix: "abc" # this prefix will be ignored. + // - prefix: "abc" // cluster: "cluster_b" // - prefix: "a" // cluster: "cluster_c" diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 564aea0b35697..66e739473e11f 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -613,16 +613,15 @@ template struct TrieLookupTable { result = current; } - // TODO(maximebedard): this could be optimized // https://github.com/facebook/mcrouter/blob/master/mcrouter/lib/fbi/cpp/Trie-inl.h#L126-L143 current = current->entries_[c].get(); if (current == nullptr) { - break; + return result ? result->value_ : nullptr; } key++; } - return result ? result->value_ : nullptr; + return current ? current->value_ : result->value_; } TrieEntry root_; diff --git a/test/common/common/utility_test.cc b/test/common/common/utility_test.cc index e404d5ee2d7f8..b19e5c34663f0 100644 --- a/test/common/common/utility_test.cc +++ b/test/common/common/utility_test.cc @@ -831,18 +831,23 @@ TEST(DateFormatter, FromTimeSameWildcard) { TEST(TrieLookupTable, LongestPrefix) { TrieLookupTable trie; trie.add("foo", "a"); + trie.add("bar", "ignored by the following element with the same key"); trie.add("bar", "b"); trie.add("baro", "c"); EXPECT_EQ("a", trie.find("foo")); + EXPECT_EQ("a", trie.findPrefix("foo")); EXPECT_EQ("a", trie.findPrefix("foosball")); EXPECT_EQ("b", trie.find("bar")); + EXPECT_EQ("b", trie.findPrefix("bar")); EXPECT_EQ("b", trie.findPrefix("baritone")); EXPECT_EQ("c", trie.findPrefix("barometer")); EXPECT_EQ(nullptr, trie.find("toto")); EXPECT_EQ(nullptr, trie.findPrefix("toto")); + EXPECT_EQ(nullptr, trie.find(" ")); + EXPECT_EQ(nullptr, trie.findPrefix(" ")); } } // namespace Envoy From 0b8e8615d5b635f0ffd6692373aaf7c05253542b Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Tue, 19 Mar 2019 15:23:28 -0400 Subject: [PATCH 10/18] formatting Signed-off-by: Maxime Bedard --- .../filter/network/redis_proxy/v2/redis_proxy.proto | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) 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 3c127ed98848e..168d98b3662e2 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 @@ -85,10 +85,11 @@ message RedisProxy { string catch_all_cluster = 3; } - // List of prefixes used to separate keys from different workloads to different clusters. Envoy will always - // favor the longest matches first. Prefixes are stored in a Trie and are accessed using a LIFO, thus identical - // prefixes will always resolve to the last entry inserted. A catch-all cluster can be used to forward commands - // to when there is no match. Time complexity of the lookups are in O(min(longest key prefix, key length)). + // List of prefixes used to separate keys from different workloads to different clusters. Envoy + // will always favor the longest matches first. Prefixes are stored in a Trie and are accessed + // using a LIFO, thus identical prefixes will always resolve to the last entry inserted. A + // catch-all cluster can be used to forward commands to when there is no match. Time complexity of + // the lookups are in O(min(longest key prefix, key length)). // // Example: // From ce5b01154337216a0b85e4a8361a3ba30ed7c0da Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Thu, 21 Mar 2019 10:01:00 -0400 Subject: [PATCH 11/18] Valide uniqueness in trie, update typos in doc Signed-off-by: Maxime Bedard --- .../network/redis_proxy/v2/redis_proxy.proto | 17 ++++++------ source/common/common/utility.h | 7 ++++- .../network/redis_proxy/router_impl.cc | 6 ++++- test/common/common/utility_test.cc | 8 +++--- .../network/redis_proxy/router_impl_test.cc | 26 +++++++++++++++++++ 5 files changed, 50 insertions(+), 14 deletions(-) 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 168d98b3662e2..6aa57f9e1253f 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 @@ -64,7 +64,7 @@ message RedisProxy { message PrefixRoutes { message Route { // String prefix that must match the beginning of the keys. Envoy will always favor the - // longest matches. + // longest match. string prefix = 1 [(validate.rules).string.min_bytes = 1]; // Indicates if the prefix needs to be removed from the key when forwarded. @@ -85,11 +85,9 @@ message RedisProxy { string catch_all_cluster = 3; } - // List of prefixes used to separate keys from different workloads to different clusters. Envoy - // will always favor the longest matches first. Prefixes are stored in a Trie and are accessed - // using a LIFO, thus identical prefixes will always resolve to the last entry inserted. A - // catch-all cluster can be used to forward commands to when there is no match. Time complexity of - // the lookups are in O(min(longest key prefix, key length)). + // 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 to when there is no match. Time complexity of the lookups are in O(min(longest key prefix, key length)). // // Example: // @@ -97,12 +95,15 @@ message RedisProxy { // // prefix_routes: // routes: - // - prefix: "abc" # this prefix will be overwritten by the next entry + // - prefix: "ab" // cluster: "cluster_a" // - prefix: "abc" // cluster: "cluster_b" + // # uniqueness constraint is not respected if the next 2 lines are uncommented. + // # - prefix: "abc" + // # cluster: "cluster_c" // - prefix: "a" - // cluster: "cluster_c" + // cluster: "cluster_d" // // See the :ref:`configuration section // ` of the architecture overview for recommendations on diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 66e739473e11f..b65ecf7a85470 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -568,8 +568,9 @@ 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. + * @return false when a value already exists for the given key. */ - void add(const char* key, Value value) { + bool add(const char* key, Value value) { TrieEntry* current = &root_; while (uint8_t c = *key) { if (!current->entries_[c]) { @@ -578,7 +579,11 @@ template struct TrieLookupTable { current = current->entries_[c].get(); key++; } + if (current->value_) { + return false; + } current->value_ = value; + return true; } /** diff --git a/source/extensions/filters/network/redis_proxy/router_impl.cc b/source/extensions/filters/network/redis_proxy/router_impl.cc index 90e95295cdb80..7f71c6a7f2bc8 100644 --- a/source/extensions/filters/network/redis_proxy/router_impl.cc +++ b/source/extensions/filters/network/redis_proxy/router_impl.cc @@ -1,4 +1,5 @@ #include "extensions/filters/network/redis_proxy/router_impl.h" +#include "common/common/fmt.h" namespace Envoy { namespace Extensions { @@ -20,11 +21,14 @@ PrefixRoutes::PrefixRoutes( to_lower_table_.toLowerCase(copy); } - prefix_lookup_table_.add(copy.c_str(), std::make_shared(Prefix{ + auto success = prefix_lookup_table_.add(copy.c_str(), std::make_shared(Prefix{ route.prefix(), route.remove_prefix(), upstreams_.at(route.cluster()), })); + if (!success) { + throw EnvoyException(fmt::format("prefix `{}` already exists.", route.prefix())); + } } } diff --git a/test/common/common/utility_test.cc b/test/common/common/utility_test.cc index b19e5c34663f0..31c3145a06f9c 100644 --- a/test/common/common/utility_test.cc +++ b/test/common/common/utility_test.cc @@ -830,10 +830,10 @@ TEST(DateFormatter, FromTimeSameWildcard) { TEST(TrieLookupTable, LongestPrefix) { TrieLookupTable trie; - trie.add("foo", "a"); - trie.add("bar", "ignored by the following element with the same key"); - trie.add("bar", "b"); - trie.add("baro", "c"); + EXPECT_TRUE(trie.add("foo", "a")); + EXPECT_TRUE(trie.add("bar", "b")); + EXPECT_FALSE(trie.add("bar", "this key is already used")); + EXPECT_TRUE(trie.add("baro", "c")); EXPECT_EQ("a", trie.find("foo")); EXPECT_EQ("a", trie.findPrefix("foo")); diff --git a/test/extensions/filters/network/redis_proxy/router_impl_test.cc b/test/extensions/filters/network/redis_proxy/router_impl_test.cc index 2c0172bb911e0..ee749671fca0e 100644 --- a/test/extensions/filters/network/redis_proxy/router_impl_test.cc +++ b/test/extensions/filters/network/redis_proxy/router_impl_test.cc @@ -175,6 +175,32 @@ TEST(PrefixRoutesTest, DifferentPrefixesSameUpstream) { 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( + { + try { + PrefixRoutes router(prefix_routes, std::move(upstreams)); + } catch (const EnvoyException& ex) { + EXPECT_STREQ("prefix `ab` already exists.", ex.what()); + throw; + } + }, + EnvoyException); +} + } // namespace RedisProxy } // namespace NetworkFilters } // namespace Extensions From b34640e284facc26adaee4c3f4076a547f041298 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Thu, 21 Mar 2019 10:04:35 -0400 Subject: [PATCH 12/18] formatting Signed-off-by: Maxime Bedard --- .../network/redis_proxy/v2/redis_proxy.proto | 7 ++++--- .../filters/network/redis_proxy/router_impl.cc | 9 +++++---- .../network/redis_proxy/router_impl_test.cc | 18 +++++++++--------- 3 files changed, 18 insertions(+), 16 deletions(-) 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 6aa57f9e1253f..5ff2e9f07134c 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 @@ -85,9 +85,10 @@ message RedisProxy { 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 to when there is no match. Time complexity of the lookups are in O(min(longest key prefix, key length)). + // 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 to when there is no match. Time complexity of the + // lookups are in O(min(longest key prefix, key length)). // // Example: // diff --git a/source/extensions/filters/network/redis_proxy/router_impl.cc b/source/extensions/filters/network/redis_proxy/router_impl.cc index 7f71c6a7f2bc8..19721e422f7d9 100644 --- a/source/extensions/filters/network/redis_proxy/router_impl.cc +++ b/source/extensions/filters/network/redis_proxy/router_impl.cc @@ -1,4 +1,5 @@ #include "extensions/filters/network/redis_proxy/router_impl.h" + #include "common/common/fmt.h" namespace Envoy { @@ -22,10 +23,10 @@ PrefixRoutes::PrefixRoutes( } auto success = prefix_lookup_table_.add(copy.c_str(), std::make_shared(Prefix{ - route.prefix(), - route.remove_prefix(), - upstreams_.at(route.cluster()), - })); + route.prefix(), + route.remove_prefix(), + upstreams_.at(route.cluster()), + })); if (!success) { throw EnvoyException(fmt::format("prefix `{}` already exists.", route.prefix())); } diff --git a/test/extensions/filters/network/redis_proxy/router_impl_test.cc b/test/extensions/filters/network/redis_proxy/router_impl_test.cc index ee749671fca0e..16723d18335c0 100644 --- a/test/extensions/filters/network/redis_proxy/router_impl_test.cc +++ b/test/extensions/filters/network/redis_proxy/router_impl_test.cc @@ -190,15 +190,15 @@ TEST(PrefixRoutesTest, DuplicatePrefix) { } EXPECT_THROW( - { - try { - PrefixRoutes router(prefix_routes, std::move(upstreams)); - } catch (const EnvoyException& ex) { - EXPECT_STREQ("prefix `ab` already exists.", ex.what()); - throw; - } - }, - EnvoyException); + { + try { + PrefixRoutes router(prefix_routes, std::move(upstreams)); + } catch (const EnvoyException& ex) { + EXPECT_STREQ("prefix `ab` already exists.", ex.what()); + throw; + } + }, + EnvoyException); } } // namespace RedisProxy From 2168949401659a56f801706d457a092d9f050624 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Fri, 22 Mar 2019 10:08:15 -0400 Subject: [PATCH 13/18] Finnish remove_prefix part Signed-off-by: Maxime Bedard --- .../network/redis_proxy/router_impl.cc | 15 ++++++-- .../network/redis_proxy/router_impl_test.cc | 37 +++++++++---------- 2 files changed, 29 insertions(+), 23 deletions(-) diff --git a/source/extensions/filters/network/redis_proxy/router_impl.cc b/source/extensions/filters/network/redis_proxy/router_impl.cc index 19721e422f7d9..c9d7dc9da40d6 100644 --- a/source/extensions/filters/network/redis_proxy/router_impl.cc +++ b/source/extensions/filters/network/redis_proxy/router_impl.cc @@ -37,16 +37,23 @@ Common::Redis::Client::PoolRequest* PrefixRoutes::makeRequest(const std::string& key, const Common::Redis::RespValue& request, Common::Redis::Client::PoolCallbacks& callbacks) { - std::string copy(key); + PrefixPtr value = nullptr; if (case_insensitive_) { + std::string copy(key); to_lower_table_.toLowerCase(copy); + value = prefix_lookup_table_.findPrefix(copy.c_str()); + } else { + value = prefix_lookup_table_.findPrefix(key.c_str()); } - auto value = prefix_lookup_table_.findPrefix(copy.c_str()); - if (value != nullptr) { // TODO: remove_prefix - value->upstream->makeRequest(key, request, callbacks); + 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); } diff --git a/test/extensions/filters/network/redis_proxy/router_impl_test.cc b/test/extensions/filters/network/redis_proxy/router_impl_test.cc index 16723d18335c0..0638fcf8d8ea3 100644 --- a/test/extensions/filters/network/redis_proxy/router_impl_test.cc +++ b/test/extensions/filters/network/redis_proxy/router_impl_test.cc @@ -107,31 +107,30 @@ TEST(PrefixRoutesTest, CaseUnsensitivePrefix) { EXPECT_EQ(nullptr, router.makeRequest("AB:bar", value, callbacks)); } -// TODO(maximebedard) -// TEST(PrefixRoutesTest, StrippedPrefix) { -// auto upstream_a = std::make_shared(); +TEST(PrefixRoutesTest, RemovePrefix) { + auto upstream_a = std::make_shared(); -// Upstreams upstreams; -// upstreams.emplace("fake_clusterA", upstream_a); -// upstreams.emplace("fake_clusterB", std::make_shared()); + Upstreams upstreams; + upstreams.emplace("fake_clusterA", upstream_a); + upstreams.emplace("fake_clusterB", std::make_shared()); -// auto prefix_routes = createPrefixRoutes(); + auto prefix_routes = createPrefixRoutes(); -// { -// auto* route = prefix_routes.mutable_routes()->Add(); -// route->set_prefix("abc"); -// route->set_cluster("fake_clusterA"); -// route->set_preserve_prefix(false); -// } + { + 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"), _, _)); + EXPECT_CALL(*upstream_a, makeRequest(Eq(":bar"), _, _)); -// PrefixRoutes router(prefix_routes, std::move(upstreams)); -// Common::Redis::RespValue value; -// Common::Redis::Client::MockPoolCallbacks callbacks; + 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)); -// } + EXPECT_EQ(nullptr, router.makeRequest("abc:bar", value, callbacks)); +} TEST(PrefixRoutesTest, RoutedToShortestPrefix) { auto upstream_b = std::make_shared(); From 71e619ecb9e9bf4180158fd109b0f21aa6e9f0c6 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Fri, 22 Mar 2019 10:14:46 -0400 Subject: [PATCH 14/18] fix typo Signed-off-by: Maxime Bedard --- .../config/filter/network/redis_proxy/v2/redis_proxy.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 5ff2e9f07134c..23f9c9728d712 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 @@ -100,7 +100,7 @@ message RedisProxy { // cluster: "cluster_a" // - prefix: "abc" // cluster: "cluster_b" - // # uniqueness constraint is not respected if the next 2 lines are uncommented. + // # next 2 lines will break uniqueness constraint // # - prefix: "abc" // # cluster: "cluster_c" // - prefix: "a" From 26256805089f5561ba2b54e1423ace2351a78080 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Fri, 22 Mar 2019 14:28:14 -0400 Subject: [PATCH 15/18] improve docs & tests. Cleanup old comments Signed-off-by: Maxime Bedard --- .../network/redis_proxy/v2/redis_proxy.proto | 19 +++++++++++-------- source/common/common/utility.h | 2 +- .../network/redis_proxy/router_impl.cc | 5 ++--- .../filters/network/redis_proxy/router_impl.h | 4 ---- .../filters/network/redis_proxy/BUILD | 1 + .../network/redis_proxy/config_test.cc | 13 +++---------- .../network/redis_proxy/router_impl_test.cc | 14 ++++---------- 7 files changed, 22 insertions(+), 36 deletions(-) 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 23f9c9728d712..2a04ae6474a1b 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 @@ -26,7 +26,7 @@ message RedisProxy { // .. attention:: // // This field is deprecated. Use a :ref:`catch-all - // route` + // cluster` // instead. string cluster = 2 [deprecated = true]; @@ -71,7 +71,7 @@ message RedisProxy { bool remove_prefix = 2; // Upstream cluster to forward the command to. - string cluster = 3; + string cluster = 3 [(validate.rules).string.min_bytes = 1]; } // List of prefix routes. @@ -87,7 +87,7 @@ message RedisProxy { // 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 to when there is no match. Time complexity of the + // 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: @@ -100,11 +100,14 @@ message RedisProxy { // cluster: "cluster_a" // - prefix: "abc" // cluster: "cluster_b" - // # next 2 lines will break uniqueness constraint - // # - prefix: "abc" - // # cluster: "cluster_c" - // - prefix: "a" - // cluster: "cluster_d" + // + // 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 retrived the key from that cluster instead. // // See the :ref:`configuration section // ` of the architecture overview for recommendations on diff --git a/source/common/common/utility.h b/source/common/common/utility.h index b65ecf7a85470..99a9fce6dd188 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -610,7 +610,7 @@ template struct TrieLookupTable { * @param key the key used to find. * @return the value matching the longest prefix based on the key. */ - Value findPrefix(const char* key) const { + Value findLongestPrefix(const char* key) const { const TrieEntry* current = &root_; const TrieEntry* result = nullptr; while (uint8_t c = *key) { diff --git a/source/extensions/filters/network/redis_proxy/router_impl.cc b/source/extensions/filters/network/redis_proxy/router_impl.cc index c9d7dc9da40d6..41380cf205aa4 100644 --- a/source/extensions/filters/network/redis_proxy/router_impl.cc +++ b/source/extensions/filters/network/redis_proxy/router_impl.cc @@ -41,13 +41,12 @@ PrefixRoutes::makeRequest(const std::string& key, const Common::Redis::RespValue if (case_insensitive_) { std::string copy(key); to_lower_table_.toLowerCase(copy); - value = prefix_lookup_table_.findPrefix(copy.c_str()); + value = prefix_lookup_table_.findLongestPrefix(copy.c_str()); } else { - value = prefix_lookup_table_.findPrefix(key.c_str()); + value = prefix_lookup_table_.findLongestPrefix(key.c_str()); } if (value != nullptr) { - // TODO: remove_prefix absl::string_view view(key); if (value->remove_prefix) { view.remove_prefix(value->prefix.length()); diff --git a/source/extensions/filters/network/redis_proxy/router_impl.h b/source/extensions/filters/network/redis_proxy/router_impl.h index 535db37b9f846..0c3d50356c02d 100644 --- a/source/extensions/filters/network/redis_proxy/router_impl.h +++ b/source/extensions/filters/network/redis_proxy/router_impl.h @@ -43,13 +43,9 @@ class PrefixRoutes : public Router { 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_; }; diff --git a/test/extensions/filters/network/redis_proxy/BUILD b/test/extensions/filters/network/redis_proxy/BUILD index 861336d56675b..7b6629b6e4917 100644 --- a/test/extensions/filters/network/redis_proxy/BUILD +++ b/test/extensions/filters/network/redis_proxy/BUILD @@ -114,5 +114,6 @@ envoy_extension_cc_test( ":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/config_test.cc b/test/extensions/filters/network/redis_proxy/config_test.cc index 1613ee4efbfb5..07f61c8f6232f 100644 --- a/test/extensions/filters/network/redis_proxy/config_test.cc +++ b/test/extensions/filters/network/redis_proxy/config_test.cc @@ -33,16 +33,9 @@ TEST(RedisProxyFilterConfigFactoryTest, NoUpstreamDefined) { NiceMock context; - EXPECT_THROW( - { - try { - RedisProxyFilterConfigFactory().createFilterFactoryFromProto(config, context); - } catch (const EnvoyException& ex) { - EXPECT_STREQ("cannot configure a redis-proxy without any upstream", ex.what()); - throw; - } - }, - EnvoyException); + EXPECT_THROW_WITH_MESSAGE( + RedisProxyFilterConfigFactory().createFilterFactoryFromProto(config, context), + EnvoyException, "cannot configure a redis-proxy without any upstream"); } TEST(RedisProxyFilterConfigFactoryTest, RedisProxyCorrectJson) { diff --git a/test/extensions/filters/network/redis_proxy/router_impl_test.cc b/test/extensions/filters/network/redis_proxy/router_impl_test.cc index 0638fcf8d8ea3..6983d1cd04189 100644 --- a/test/extensions/filters/network/redis_proxy/router_impl_test.cc +++ b/test/extensions/filters/network/redis_proxy/router_impl_test.cc @@ -8,6 +8,7 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" +#include "test/test_common/utility.h" using testing::_; using testing::Eq; @@ -188,16 +189,9 @@ TEST(PrefixRoutesTest, DuplicatePrefix) { route->set_cluster("this_will_throw"); } - EXPECT_THROW( - { - try { - PrefixRoutes router(prefix_routes, std::move(upstreams)); - } catch (const EnvoyException& ex) { - EXPECT_STREQ("prefix `ab` already exists.", ex.what()); - throw; - } - }, - EnvoyException); + EXPECT_THROW_WITH_MESSAGE( + PrefixRoutes router(prefix_routes, std::move(upstreams)), + EnvoyException, "prefix `ab` already exists.") } } // namespace RedisProxy From 5f9cbc94143bff7dda01f98a18d88d23c41d6982 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Fri, 22 Mar 2019 15:15:14 -0400 Subject: [PATCH 16/18] Add overwrite_existing Signed-off-by: Maxime Bedard --- source/common/common/utility.h | 5 +-- .../network/redis_proxy/router_impl.cc | 2 +- test/common/common/utility_test.cc | 31 ++++++++++++++----- 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 99a9fce6dd188..6bd7accc617e5 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -568,9 +568,10 @@ 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 add(const char* key, Value value, bool overwrite_existing = true) { TrieEntry* current = &root_; while (uint8_t c = *key) { if (!current->entries_[c]) { @@ -579,7 +580,7 @@ template struct TrieLookupTable { current = current->entries_[c].get(); key++; } - if (current->value_) { + if (current->value_ && !overwrite_existing) { return false; } current->value_ = value; diff --git a/source/extensions/filters/network/redis_proxy/router_impl.cc b/source/extensions/filters/network/redis_proxy/router_impl.cc index 41380cf205aa4..ed01936564b84 100644 --- a/source/extensions/filters/network/redis_proxy/router_impl.cc +++ b/source/extensions/filters/network/redis_proxy/router_impl.cc @@ -26,7 +26,7 @@ PrefixRoutes::PrefixRoutes( route.prefix(), route.remove_prefix(), upstreams_.at(route.cluster()), - })); + }), false); if (!success) { throw EnvoyException(fmt::format("prefix `{}` already exists.", route.prefix())); } diff --git a/test/common/common/utility_test.cc b/test/common/common/utility_test.cc index 31c3145a06f9c..e2a084651065a 100644 --- a/test/common/common/utility_test.cc +++ b/test/common/common/utility_test.cc @@ -828,26 +828,41 @@ 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_FALSE(trie.add("bar", "this key is already used")); EXPECT_TRUE(trie.add("baro", "c")); EXPECT_EQ("a", trie.find("foo")); - EXPECT_EQ("a", trie.findPrefix("foo")); - EXPECT_EQ("a", trie.findPrefix("foosball")); + EXPECT_EQ("a", trie.findLongestPrefix("foo")); + EXPECT_EQ("a", trie.findLongestPrefix("foosball")); EXPECT_EQ("b", trie.find("bar")); - EXPECT_EQ("b", trie.findPrefix("bar")); - EXPECT_EQ("b", trie.findPrefix("baritone")); - EXPECT_EQ("c", trie.findPrefix("barometer")); + 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.findPrefix("toto")); + EXPECT_EQ(nullptr, trie.findLongestPrefix("toto")); EXPECT_EQ(nullptr, trie.find(" ")); - EXPECT_EQ(nullptr, trie.findPrefix(" ")); + EXPECT_EQ(nullptr, trie.findLongestPrefix(" ")); } } // namespace Envoy From 9abb8f98285571bd9e7e267cc3a0d6744c08ba90 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Fri, 22 Mar 2019 15:16:49 -0400 Subject: [PATCH 17/18] formatting Signed-off-by: Maxime Bedard --- .../filter/network/redis_proxy/v2/redis_proxy.proto | 4 ++-- source/common/common/utility.h | 3 ++- .../filters/network/redis_proxy/router_impl.cc | 12 +++++++----- .../filters/network/redis_proxy/config_test.cc | 4 ++-- .../filters/network/redis_proxy/router_impl_test.cc | 7 +++---- 5 files changed, 16 insertions(+), 14 deletions(-) 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 2a04ae6474a1b..40658c218c88b 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 @@ -106,8 +106,8 @@ message RedisProxy { // * '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 retrived the key from that cluster instead. + // cluster` + // would have retrived the key from that cluster instead. // // See the :ref:`configuration section // ` of the architecture overview for recommendations on diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 6bd7accc617e5..9eaddb7f64da1 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -568,7 +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. + * @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) { diff --git a/source/extensions/filters/network/redis_proxy/router_impl.cc b/source/extensions/filters/network/redis_proxy/router_impl.cc index ed01936564b84..009cc345b3844 100644 --- a/source/extensions/filters/network/redis_proxy/router_impl.cc +++ b/source/extensions/filters/network/redis_proxy/router_impl.cc @@ -22,11 +22,13 @@ PrefixRoutes::PrefixRoutes( 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); + 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())); } diff --git a/test/extensions/filters/network/redis_proxy/config_test.cc b/test/extensions/filters/network/redis_proxy/config_test.cc index 07f61c8f6232f..be23782420b43 100644 --- a/test/extensions/filters/network/redis_proxy/config_test.cc +++ b/test/extensions/filters/network/redis_proxy/config_test.cc @@ -34,8 +34,8 @@ TEST(RedisProxyFilterConfigFactoryTest, NoUpstreamDefined) { NiceMock context; EXPECT_THROW_WITH_MESSAGE( - RedisProxyFilterConfigFactory().createFilterFactoryFromProto(config, context), - EnvoyException, "cannot configure a redis-proxy without any upstream"); + RedisProxyFilterConfigFactory().createFilterFactoryFromProto(config, context), EnvoyException, + "cannot configure a redis-proxy without any upstream"); } TEST(RedisProxyFilterConfigFactoryTest, RedisProxyCorrectJson) { diff --git a/test/extensions/filters/network/redis_proxy/router_impl_test.cc b/test/extensions/filters/network/redis_proxy/router_impl_test.cc index 6983d1cd04189..62b012e4abc27 100644 --- a/test/extensions/filters/network/redis_proxy/router_impl_test.cc +++ b/test/extensions/filters/network/redis_proxy/router_impl_test.cc @@ -5,10 +5,10 @@ #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" -#include "test/test_common/utility.h" using testing::_; using testing::Eq; @@ -189,9 +189,8 @@ TEST(PrefixRoutesTest, DuplicatePrefix) { route->set_cluster("this_will_throw"); } - EXPECT_THROW_WITH_MESSAGE( - PrefixRoutes router(prefix_routes, std::move(upstreams)), - EnvoyException, "prefix `ab` already exists.") + EXPECT_THROW_WITH_MESSAGE(PrefixRoutes router(prefix_routes, std::move(upstreams)), + EnvoyException, "prefix `ab` already exists.") } } // namespace RedisProxy From c6d6cc8fcfa1895d1186dc6f9028f674421e8286 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Mon, 25 Mar 2019 15:33:44 -0400 Subject: [PATCH 18/18] fix typo Signed-off-by: Maxime Bedard --- .../config/filter/network/redis_proxy/v2/redis_proxy.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 40658c218c88b..696bf26b8b5c9 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 @@ -107,7 +107,7 @@ message RedisProxy { // * '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 retrived the key from that cluster instead. + // would have retrieved the key from that cluster instead. // // See the :ref:`configuration section // ` of the architecture overview for recommendations on