From c272308f44e834661400d8f116af169de80f7647 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Wed, 27 Mar 2019 17:36:49 -0400 Subject: [PATCH 01/10] Revert "Revert "redis: prefixed routing (#5658)" (#6401)" This reverts commit bacd89e866b4d81dd316613ce11c0b9c678cc421. Signed-off-by: Maxime Bedard --- DEPRECATED.md | 5 + .../network/redis_proxy/v2/redis_proxy.proto | 63 +++++- docs/root/intro/arch_overview/redis.rst | 5 +- docs/root/intro/version_history.rst | 1 + source/common/common/utility.h | 34 ++- .../filters/network/redis_proxy/BUILD | 29 ++- .../redis_proxy/command_splitter_impl.cc | 41 ++-- .../redis_proxy/command_splitter_impl.h | 30 ++- .../filters/network/redis_proxy/config.cc | 43 +++- .../filters/network/redis_proxy/conn_pool.h | 2 +- .../network/redis_proxy/conn_pool_impl.h | 1 - .../network/redis_proxy/proxy_filter.cc | 2 +- .../network/redis_proxy/proxy_filter.h | 1 - .../filters/network/redis_proxy/router.h | 42 ++++ .../network/redis_proxy/router_impl.cc | 68 ++++++ .../filters/network/redis_proxy/router_impl.h | 55 +++++ test/common/common/utility_test.cc | 37 ++++ .../filters/network/redis_proxy/BUILD | 13 ++ .../redis_proxy/command_lookup_speed_test.cc | 8 +- .../redis_proxy/command_splitter_impl_test.cc | 24 +-- .../network/redis_proxy/config_test.cc | 15 ++ .../redis_proxy/conn_pool_impl_test.cc | 5 +- .../filters/network/redis_proxy/mocks.cc | 3 + .../filters/network/redis_proxy/mocks.h | 12 ++ .../network/redis_proxy/proxy_filter_test.cc | 2 +- .../network/redis_proxy/router_impl_test.cc | 199 ++++++++++++++++++ 26 files changed, 662 insertions(+), 78 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 f64cd7a3415b6..502987c09035d 100644 --- a/DEPRECATED.md +++ b/DEPRECATED.md @@ -11,6 +11,11 @@ A logged warning is expected for each deprecated item that is in deprecation win * Use of `enabled` in `CorsPolicy`, found in [route.proto](https://github.com/envoyproxy/envoy/blob/master/api/envoy/api/v2/route/route.proto). Set the `filter_enabled` field instead. +* Use of the `type` field in the `FaultDelay` message (found in + [fault.proto](https://github.com/envoyproxy/envoy/blob/master/api/envoy/config/filter/fault/v2/fault.proto)) + has been deprecated. It was never used and setting it has no effect. It will be removed in the + following release. +* Use of `cluster`, found in [redis-proxy.proto](https://github.com/envoyproxy/envoy/blob/master/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto) is deprecated. Set a `PrefixRoutes.catch_all_cluster` instead. ## Version 1.9.0 (Dec 20, 2018) diff --git a/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto b/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto index cd8c18b128755..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 @@ -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 + // cluster` + // instead. + string cluster = 2 [deprecated = true]; // Redis connection pool settings. message ConnPoolSettings { @@ -48,10 +54,63 @@ 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 PrefixRoutes { + message Route { + // String prefix that must match the beginning of the keys. Envoy will always favor the + // longest match. + string prefix = 1 [(validate.rules).string.min_bytes = 1]; + + // Indicates if the prefix needs to be removed from the key when forwarded. + bool remove_prefix = 2; + + // Upstream cluster to forward the command to. + string cluster = 3 [(validate.rules).string.min_bytes = 1]; + } + + // List of prefix routes. + repeated Route routes = 1 [(gogoproto.nullable) = false]; + + // Indicates that prefix matching should be case insensitive. + bool case_insensitive = 2; + + // Optional catch-all route to forward commands that doesn't match any of the routes. The + // catch-all route becomes required when no routes are specified. + string catch_all_cluster = 3; + } + + // List of **unique** prefixes used to separate keys from different workloads to different + // clusters. Envoy will always favor the longest match first in case of overlap. A catch-all + // cluster can be used to forward commands when there is no match. Time complexity of the + // lookups are in O(min(longest key prefix, key length)). + // + // Example: + // + // .. code-block:: yaml + // + // prefix_routes: + // routes: + // - prefix: "ab" + // cluster: "cluster_a" + // - prefix: "abc" + // cluster: "cluster_b" + // + // When using the above routes, the following prefixes would be sent to: + // + // * 'get abc:users' would retrive the key 'abc:users' from cluster_b. + // * 'get ab:users' would retrive the key 'ab:users' from cluster_a. + // * 'get z:users' would return a NoUpstreamHost error. A :ref:`catch-all + // cluster` + // would have retrieved the key from that cluster instead. + // + // See the :ref:`configuration section + // ` of the architecture overview for recommendations on + // configuring the backing clusters. + PrefixRoutes prefix_routes = 5 [(gogoproto.nullable) = false]; } diff --git a/docs/root/intro/arch_overview/redis.rst b/docs/root/intro/arch_overview/redis.rst index 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 b307ddc5d1bdb..18ce98def67c6 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -51,6 +51,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..9eaddb7f64da1 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -568,8 +568,11 @@ 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. */ - void 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]) { @@ -578,7 +581,11 @@ template struct TrieLookupTable { current = current->entries_[c].get(); key++; } + if (current->value_ && !overwrite_existing) { + return false; + } current->value_ = value; + return true; } /** @@ -599,6 +606,31 @@ template struct TrieLookupTable { return current->value_; } + /** + * Finds the entry associated with the longest prefix. Complexity is O(min(longest key prefix, key + * length)) + * @param key the key used to find. + * @return the value matching the longest prefix based on the key. + */ + Value findLongestPrefix(const char* key) const { + const TrieEntry* current = &root_; + const TrieEntry* result = nullptr; + while (uint8_t c = *key) { + if (current->value_) { + result = current; + } + + // https://github.com/facebook/mcrouter/blob/master/mcrouter/lib/fbi/cpp/Trie-inl.h#L126-L143 + current = current->entries_[c].get(); + if (current == nullptr) { + return result ? result->value_ : nullptr; + } + + key++; + } + return current ? current->value_ : result->value_; + } + TrieEntry root_; }; diff --git a/source/extensions/filters/network/redis_proxy/BUILD b/source/extensions/filters/network/redis_proxy/BUILD index 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..415a754e0ac6a 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) { @@ -91,8 +91,8 @@ SplitRequestPtr EvalRequest::create(ConnPool::Instance& conn_pool, 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); + 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 +123,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 +152,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 +195,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 +231,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 +270,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 +299,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 +337,11 @@ 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..45ac46b71cd37 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,8 +237,7 @@ 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); @@ -261,11 +257,11 @@ 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, + return RequestClass::create(router_, request, callbacks, command_stats, time_source, latency_in_micros); } }; @@ -288,8 +284,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 +303,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..9838c2cc5ebf4 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,43 @@ 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..1317b170aca4c --- /dev/null +++ b/source/extensions/filters/network/redis_proxy/router.h @@ -0,0 +1,42 @@ +#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..009cc345b3844 --- /dev/null +++ b/source/extensions/filters/network/redis_proxy/router_impl.cc @@ -0,0 +1,68 @@ +#include "extensions/filters/network/redis_proxy/router_impl.h" + +#include "common/common/fmt.h" + +namespace Envoy { +namespace Extensions { +namespace NetworkFilters { +namespace RedisProxy { + +PrefixRoutes::PrefixRoutes( + const envoy::config::filter::network::redis_proxy::v2::RedisProxy::PrefixRoutes& config, + Upstreams&& upstreams) + : case_insensitive_(config.case_insensitive()), upstreams_(std::move(upstreams)), + catch_all_upstream_(config.catch_all_cluster().empty() + ? nullptr + : upstreams_.at(config.catch_all_cluster())) { + + for (auto const& route : config.routes()) { + std::string copy(route.prefix()); + + if (case_insensitive_) { + to_lower_table_.toLowerCase(copy); + } + + auto success = prefix_lookup_table_.add(copy.c_str(), + std::make_shared(Prefix{ + route.prefix(), + route.remove_prefix(), + upstreams_.at(route.cluster()), + }), + false); + if (!success) { + throw EnvoyException(fmt::format("prefix `{}` already exists.", route.prefix())); + } + } +} + +Common::Redis::Client::PoolRequest* +PrefixRoutes::makeRequest(const std::string& key, const Common::Redis::RespValue& request, + Common::Redis::Client::PoolCallbacks& callbacks) { + + PrefixPtr value = nullptr; + if (case_insensitive_) { + std::string copy(key); + to_lower_table_.toLowerCase(copy); + value = prefix_lookup_table_.findLongestPrefix(copy.c_str()); + } else { + value = prefix_lookup_table_.findLongestPrefix(key.c_str()); + } + + if (value != nullptr) { + absl::string_view view(key); + if (value->remove_prefix) { + view.remove_prefix(value->prefix.length()); + } + std::string str(view); + value->upstream->makeRequest(str, request, callbacks); + } else if (catch_all_upstream_ != nullptr) { + catch_all_upstream_.value()->makeRequest(key, request, callbacks); + } + + return nullptr; +} + +} // namespace RedisProxy +} // namespace NetworkFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/network/redis_proxy/router_impl.h b/source/extensions/filters/network/redis_proxy/router_impl.h new file mode 100644 index 0000000000000..0c3d50356c02d --- /dev/null +++ b/source/extensions/filters/network/redis_proxy/router_impl.h @@ -0,0 +1,55 @@ +#pragma once + +#include +#include +#include +#include +#include +#include + +#include "envoy/config/filter/network/redis_proxy/v2/redis_proxy.pb.h" +#include "envoy/thread_local/thread_local.h" +#include "envoy/upstream/cluster_manager.h" + +#include "common/common/to_lower_table.h" + +#include "extensions/filters/network/redis_proxy/conn_pool_impl.h" +#include "extensions/filters/network/redis_proxy/router.h" + +namespace Envoy { +namespace Extensions { +namespace NetworkFilters { +namespace RedisProxy { + +typedef std::map Upstreams; + +class PrefixRoutes : public Router { +public: + PrefixRoutes(const envoy::config::filter::network::redis_proxy::v2::RedisProxy::PrefixRoutes& + prefix_routes, + Upstreams&& upstreams); + + Common::Redis::Client::PoolRequest* + makeRequest(const std::string& hash_key, const Common::Redis::RespValue& request, + Common::Redis::Client::PoolCallbacks& callbacks) override; + +private: + struct Prefix { + const std::string prefix; + const bool remove_prefix; + ConnPool::InstanceSharedPtr upstream; + }; + + typedef std::shared_ptr PrefixPtr; + + TrieLookupTable prefix_lookup_table_; + const ToLowerTable to_lower_table_; + const bool case_insensitive_; + Upstreams upstreams_; + absl::optional catch_all_upstream_; +}; + +} // namespace RedisProxy +} // namespace NetworkFilters +} // namespace Extensions +} // namespace Envoy diff --git a/test/common/common/utility_test.cc b/test/common/common/utility_test.cc index 6434cd140280b..e2a084651065a 100644 --- a/test/common/common/utility_test.cc +++ b/test/common/common/utility_test.cc @@ -828,4 +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_TRUE(trie.add("baro", "c")); + + EXPECT_EQ("a", trie.find("foo")); + EXPECT_EQ("a", trie.findLongestPrefix("foo")); + EXPECT_EQ("a", trie.findLongestPrefix("foosball")); + + EXPECT_EQ("b", trie.find("bar")); + EXPECT_EQ("b", trie.findLongestPrefix("bar")); + EXPECT_EQ("b", trie.findLongestPrefix("baritone")); + EXPECT_EQ("c", trie.findLongestPrefix("barometer")); + + EXPECT_EQ(nullptr, trie.find("toto")); + EXPECT_EQ(nullptr, trie.findLongestPrefix("toto")); + EXPECT_EQ(nullptr, trie.find(" ")); + EXPECT_EQ(nullptr, trie.findLongestPrefix(" ")); +} + } // namespace Envoy diff --git a/test/extensions/filters/network/redis_proxy/BUILD b/test/extensions/filters/network/redis_proxy/BUILD index 492404c41547e..7b6629b6e4917 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,15 @@ envoy_extension_cc_test_binary( "//test/test_common:simulated_time_system_lib", ], ) + +envoy_extension_cc_test( + name = "router_impl_test", + srcs = ["router_impl_test.cc"], + extension_name = "envoy.filters.network.redis_proxy", + deps = [ + ":redis_mocks", + "//source/extensions/filters/network/redis_proxy:router_lib", + "//test/extensions/filters/network/common/redis:redis_mocks", + "//test/test_common:utility_lib", + ], +) diff --git a/test/extensions/filters/network/redis_proxy/command_lookup_speed_test.cc b/test/extensions/filters/network/redis_proxy/command_lookup_speed_test.cc index 2f4d8e30e1b0b..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 @@ -30,7 +30,7 @@ class NoOpSplitCallbacks : public CommandSplitter::SplitCallbacks { void onResponse(Common::Redis::RespValuePtr&&) override {} }; -class NullInstanceImpl : public ConnPool::Instance { +class NullRouterImpl : public Router { Common::Redis::Client::PoolRequest* makeRequest(const std::string&, const Common::Redis::RespValue&, Common::Redis::Client::PoolCallbacks&) override { @@ -65,11 +65,11 @@ 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..be23782420b43 100644 --- a/test/extensions/filters/network/redis_proxy/config_test.cc +++ b/test/extensions/filters/network/redis_proxy/config_test.cc @@ -23,6 +23,21 @@ TEST(RedisProxyFilterConfigFactoryTest, ValidateFail) { ProtoValidationException); } +TEST(RedisProxyFilterConfigFactoryTest, NoUpstreamDefined) { + envoy::config::filter::network::redis_proxy::v2::RedisProxy::ConnPoolSettings settings; + settings.mutable_op_timeout()->CopyFrom(Protobuf::util::TimeUtil::MillisecondsToDuration(20)); + + envoy::config::filter::network::redis_proxy::v2::RedisProxy config; + config.set_stat_prefix("foo"); + config.mutable_settings()->CopyFrom(settings); + + NiceMock context; + + EXPECT_THROW_WITH_MESSAGE( + RedisProxyFilterConfigFactory().createFilterFactoryFromProto(config, context), EnvoyException, + "cannot configure a redis-proxy without any upstream"); +} + TEST(RedisProxyFilterConfigFactoryTest, RedisProxyCorrectJson) { std::string json_string = R"EOF( { diff --git a/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc b/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc index bd267cd1670d2..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 @@ -43,7 +43,8 @@ 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_, + + conn_pool_ = std::make_shared(cluster_name_, cm_, *this, tls_, Common::Redis::Client::createConnPoolSettings()); } @@ -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..e959475542654 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,17 @@ 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..62b012e4abc27 --- /dev/null +++ b/test/extensions/filters/network/redis_proxy/router_impl_test.cc @@ -0,0 +1,199 @@ +#include + +#include "extensions/filters/network/redis_proxy/conn_pool_impl.h" +#include "extensions/filters/network/redis_proxy/router_impl.h" + +#include "test/extensions/filters/network/common/redis/mocks.h" +#include "test/extensions/filters/network/redis_proxy/mocks.h" +#include "test/test_common/utility.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::_; +using testing::Eq; +using testing::InSequence; +using testing::Return; +using testing::StrEq; + +namespace Envoy { +namespace Extensions { +namespace NetworkFilters { +namespace RedisProxy { + +envoy::config::filter::network::redis_proxy::v2::RedisProxy::PrefixRoutes createPrefixRoutes() { + envoy::config::filter::network::redis_proxy::v2::RedisProxy::PrefixRoutes prefix_routes; + auto* routes = prefix_routes.mutable_routes(); + + { + auto* route = routes->Add(); + route->set_prefix("ab"); + route->set_cluster("fake_clusterA"); + } + + { + auto* route = routes->Add(); + route->set_prefix("a"); + route->set_cluster("fake_clusterB"); + } + + return prefix_routes; +} + +TEST(PrefixRoutesTest, MissingCatchAll) { + Upstreams upstreams; + upstreams.emplace("fake_clusterA", std::make_shared()); + upstreams.emplace("fake_clusterB", std::make_shared()); + + PrefixRoutes router(createPrefixRoutes(), std::move(upstreams)); + + Common::Redis::RespValue value; + Common::Redis::Client::MockPoolCallbacks callbacks; + + EXPECT_EQ(nullptr, router.makeRequest("c:bar", value, callbacks)); +} + +TEST(PrefixRoutesTest, RoutedToCatchAll) { + auto upstream_c = std::make_shared(); + + Upstreams upstreams; + upstreams.emplace("fake_clusterA", std::make_shared()); + upstreams.emplace("fake_clusterB", std::make_shared()); + upstreams.emplace("fake_clusterC", upstream_c); + + auto prefix_routes = createPrefixRoutes(); + prefix_routes.set_catch_all_cluster("fake_clusterC"); + + EXPECT_CALL(*upstream_c, makeRequest(Eq("c:bar"), _, _)); + + PrefixRoutes router(prefix_routes, std::move(upstreams)); + Common::Redis::RespValue value; + Common::Redis::Client::MockPoolCallbacks callbacks; + + EXPECT_EQ(nullptr, router.makeRequest("c:bar", value, callbacks)); +} + +TEST(PrefixRoutesTest, RoutedToLongestPrefix) { + auto upstream_a = std::make_shared(); + + Upstreams upstreams; + upstreams.emplace("fake_clusterA", upstream_a); + upstreams.emplace("fake_clusterB", std::make_shared()); + + EXPECT_CALL(*upstream_a, makeRequest(Eq("ab:bar"), _, _)); + + PrefixRoutes router(createPrefixRoutes(), std::move(upstreams)); + Common::Redis::RespValue value; + Common::Redis::Client::MockPoolCallbacks callbacks; + + EXPECT_EQ(nullptr, router.makeRequest("ab:bar", value, callbacks)); +} + +TEST(PrefixRoutesTest, CaseUnsensitivePrefix) { + auto upstream_a = std::make_shared(); + + Upstreams upstreams; + upstreams.emplace("fake_clusterA", upstream_a); + upstreams.emplace("fake_clusterB", std::make_shared()); + + auto prefix_routes = createPrefixRoutes(); + prefix_routes.set_case_insensitive(true); + + EXPECT_CALL(*upstream_a, makeRequest(Eq("AB:bar"), _, _)); + + PrefixRoutes router(prefix_routes, std::move(upstreams)); + Common::Redis::RespValue value; + Common::Redis::Client::MockPoolCallbacks callbacks; + + EXPECT_EQ(nullptr, router.makeRequest("AB:bar", value, callbacks)); +} + +TEST(PrefixRoutesTest, RemovePrefix) { + auto upstream_a = std::make_shared(); + + Upstreams upstreams; + upstreams.emplace("fake_clusterA", upstream_a); + upstreams.emplace("fake_clusterB", std::make_shared()); + + auto prefix_routes = createPrefixRoutes(); + + { + auto* route = prefix_routes.mutable_routes()->Add(); + route->set_prefix("abc"); + route->set_cluster("fake_clusterA"); + route->set_remove_prefix(true); + } + + EXPECT_CALL(*upstream_a, makeRequest(Eq(":bar"), _, _)); + + PrefixRoutes router(prefix_routes, std::move(upstreams)); + Common::Redis::RespValue value; + Common::Redis::Client::MockPoolCallbacks callbacks; + + EXPECT_EQ(nullptr, router.makeRequest("abc:bar", value, callbacks)); +} + +TEST(PrefixRoutesTest, RoutedToShortestPrefix) { + auto upstream_b = std::make_shared(); + + Upstreams upstreams; + upstreams.emplace("fake_clusterA", std::make_shared()); + upstreams.emplace("fake_clusterB", upstream_b); + + EXPECT_CALL(*upstream_b, makeRequest(Eq("a:bar"), _, _)); + + PrefixRoutes router(createPrefixRoutes(), std::move(upstreams)); + Common::Redis::RespValue value; + Common::Redis::Client::MockPoolCallbacks callbacks; + + EXPECT_EQ(nullptr, router.makeRequest("a:bar", value, callbacks)); +} + +TEST(PrefixRoutesTest, DifferentPrefixesSameUpstream) { + auto upstream_b = std::make_shared(); + + Upstreams upstreams; + upstreams.emplace("fake_clusterA", std::make_shared()); + upstreams.emplace("fake_clusterB", upstream_b); + + auto prefix_routes = createPrefixRoutes(); + + { + auto* route = prefix_routes.mutable_routes()->Add(); + route->set_prefix("also_route_to_b"); + route->set_cluster("fake_clusterB"); + } + + EXPECT_CALL(*upstream_b, makeRequest(Eq("a:bar"), _, _)); + EXPECT_CALL(*upstream_b, makeRequest(Eq("also_route_to_b:bar"), _, _)); + + PrefixRoutes router(prefix_routes, std::move(upstreams)); + Common::Redis::RespValue value; + Common::Redis::Client::MockPoolCallbacks callbacks; + + EXPECT_EQ(nullptr, router.makeRequest("a:bar", value, callbacks)); + EXPECT_EQ(nullptr, router.makeRequest("also_route_to_b:bar", value, callbacks)); +} + +TEST(PrefixRoutesTest, DuplicatePrefix) { + Upstreams upstreams; + upstreams.emplace("fake_clusterA", std::make_shared()); + upstreams.emplace("fake_clusterB", std::make_shared()); + upstreams.emplace("this_will_throw", std::make_shared()); + + auto prefix_routes = createPrefixRoutes(); + + { + auto* route = prefix_routes.mutable_routes()->Add(); + route->set_prefix("ab"); + route->set_cluster("this_will_throw"); + } + + EXPECT_THROW_WITH_MESSAGE(PrefixRoutes router(prefix_routes, std::move(upstreams)), + EnvoyException, "prefix `ab` already exists.") +} + +} // namespace RedisProxy +} // namespace NetworkFilters +} // namespace Extensions +} // namespace Envoy From 41d9b4dd0de4cf1c6d4d2cdf12da8998dbddad03 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Wed, 27 Mar 2019 17:33:10 -0400 Subject: [PATCH 02/10] Fix segfault, should have early returned Signed-off-by: Maxime Bedard --- .../filters/network/redis_proxy/router_impl.cc | 4 ++-- .../network/redis_proxy/router_impl_test.cc | 18 ++++++++++-------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/source/extensions/filters/network/redis_proxy/router_impl.cc b/source/extensions/filters/network/redis_proxy/router_impl.cc index 009cc345b3844..cfede3c6ae87e 100644 --- a/source/extensions/filters/network/redis_proxy/router_impl.cc +++ b/source/extensions/filters/network/redis_proxy/router_impl.cc @@ -54,9 +54,9 @@ PrefixRoutes::makeRequest(const std::string& key, const Common::Redis::RespValue view.remove_prefix(value->prefix.length()); } std::string str(view); - value->upstream->makeRequest(str, request, callbacks); + return value->upstream->makeRequest(str, request, callbacks); } else if (catch_all_upstream_ != nullptr) { - catch_all_upstream_.value()->makeRequest(key, request, callbacks); + return catch_all_upstream_.value()->makeRequest(key, request, callbacks); } return nullptr; 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 62b012e4abc27..554f9c3d9e6ab 100644 --- a/test/extensions/filters/network/redis_proxy/router_impl_test.cc +++ b/test/extensions/filters/network/redis_proxy/router_impl_test.cc @@ -13,6 +13,7 @@ using testing::_; using testing::Eq; using testing::InSequence; +using testing::Ref; using testing::Return; using testing::StrEq; @@ -64,13 +65,13 @@ TEST(PrefixRoutesTest, RoutedToCatchAll) { 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::Client::MockPoolRequest active_request; Common::Redis::RespValue value; Common::Redis::Client::MockPoolCallbacks callbacks; - EXPECT_EQ(nullptr, router.makeRequest("c:bar", value, callbacks)); + EXPECT_CALL(*upstream_c, makeRequest(Eq("c:bar"), Ref(value), Ref(callbacks))).WillOnce(Return(&active_request));; + EXPECT_EQ(&active_request, router.makeRequest("c:bar", value, callbacks)); } TEST(PrefixRoutesTest, RoutedToLongestPrefix) { @@ -80,13 +81,13 @@ TEST(PrefixRoutesTest, RoutedToLongestPrefix) { 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::Client::MockPoolRequest active_request; Common::Redis::RespValue value; Common::Redis::Client::MockPoolCallbacks callbacks; - EXPECT_EQ(nullptr, router.makeRequest("ab:bar", value, callbacks)); + EXPECT_CALL(*upstream_a, makeRequest(Eq("ab:bar"), Ref(value), Ref(callbacks))).WillOnce(Return(&active_request)); + EXPECT_EQ(&active_request, router.makeRequest("ab:bar", value, callbacks)); } TEST(PrefixRoutesTest, CaseUnsensitivePrefix) { @@ -140,13 +141,14 @@ TEST(PrefixRoutesTest, RoutedToShortestPrefix) { 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::Client::MockPoolRequest active_request; Common::Redis::RespValue value; Common::Redis::Client::MockPoolCallbacks callbacks; - EXPECT_EQ(nullptr, router.makeRequest("a:bar", value, callbacks)); + EXPECT_CALL(*upstream_b, makeRequest(Eq("a:bar"), Ref(value), Ref(callbacks))).WillOnce(Return(&active_request)); + EXPECT_EQ(&active_request, router.makeRequest("a:bar", value, callbacks)); } TEST(PrefixRoutesTest, DifferentPrefixesSameUpstream) { From 7b4e9939977efcbf6e6f61adad21930e740c60f3 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Thu, 28 Mar 2019 10:11:12 -0400 Subject: [PATCH 03/10] formatting Signed-off-by: Maxime Bedard --- .../filters/network/redis_proxy/router_impl_test.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) 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 554f9c3d9e6ab..72512f426c37e 100644 --- a/test/extensions/filters/network/redis_proxy/router_impl_test.cc +++ b/test/extensions/filters/network/redis_proxy/router_impl_test.cc @@ -70,7 +70,8 @@ TEST(PrefixRoutesTest, RoutedToCatchAll) { Common::Redis::RespValue value; Common::Redis::Client::MockPoolCallbacks callbacks; - EXPECT_CALL(*upstream_c, makeRequest(Eq("c:bar"), Ref(value), Ref(callbacks))).WillOnce(Return(&active_request));; + EXPECT_CALL(*upstream_c, makeRequest(Eq("c:bar"), Ref(value), Ref(callbacks))) + .WillOnce(Return(&active_request)); EXPECT_EQ(&active_request, router.makeRequest("c:bar", value, callbacks)); } @@ -86,7 +87,8 @@ TEST(PrefixRoutesTest, RoutedToLongestPrefix) { Common::Redis::RespValue value; Common::Redis::Client::MockPoolCallbacks callbacks; - EXPECT_CALL(*upstream_a, makeRequest(Eq("ab:bar"), Ref(value), Ref(callbacks))).WillOnce(Return(&active_request)); + EXPECT_CALL(*upstream_a, makeRequest(Eq("ab:bar"), Ref(value), Ref(callbacks))) + .WillOnce(Return(&active_request)); EXPECT_EQ(&active_request, router.makeRequest("ab:bar", value, callbacks)); } @@ -141,13 +143,13 @@ TEST(PrefixRoutesTest, RoutedToShortestPrefix) { upstreams.emplace("fake_clusterA", std::make_shared()); upstreams.emplace("fake_clusterB", upstream_b); - PrefixRoutes router(createPrefixRoutes(), std::move(upstreams)); Common::Redis::Client::MockPoolRequest active_request; Common::Redis::RespValue value; Common::Redis::Client::MockPoolCallbacks callbacks; - EXPECT_CALL(*upstream_b, makeRequest(Eq("a:bar"), Ref(value), Ref(callbacks))).WillOnce(Return(&active_request)); + EXPECT_CALL(*upstream_b, makeRequest(Eq("a:bar"), Ref(value), Ref(callbacks))) + .WillOnce(Return(&active_request)); EXPECT_EQ(&active_request, router.makeRequest("a:bar", value, callbacks)); } From 91cd111e587d6d3dc0acab87808a5d9ca5b023fa Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Wed, 10 Apr 2019 10:00:57 -0400 Subject: [PATCH 04/10] Formatting Signed-off-by: Maxime Bedard --- .../redis_proxy/command_splitter_impl.cc | 29 +++++++++---------- .../redis_proxy/command_splitter_impl.h | 19 +++++------- .../filters/network/redis_proxy/router.h | 4 +-- .../filters/network/redis_proxy/BUILD | 2 +- .../redis_proxy/command_lookup_speed_test.cc | 4 +-- .../redis_proxy/command_splitter_impl_test.cc | 8 +++-- 6 files changed, 30 insertions(+), 36 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 b6e0bbfcb0959..ba007b6b3b674 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc @@ -138,7 +138,7 @@ SplitRequestPtr SimpleRequest::create(Router& router, if (conn_pool) { request_ptr->conn_pool_ = conn_pool; request_ptr->handle_ = conn_pool->makeRequest(incoming_request->asArray()[1].asString(), - *incoming_request, *request_ptr); + *incoming_request, *request_ptr); } if (!request_ptr->handle_) { @@ -150,8 +150,7 @@ SplitRequestPtr SimpleRequest::create(Router& router, return std::move(request_ptr); } -SplitRequestPtr EvalRequest::create(Router& router, - Common::Redis::RespValuePtr&& incoming_request, +SplitRequestPtr EvalRequest::create(Router& router, Common::Redis::RespValuePtr&& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros) { // EVAL looks like: EVAL script numkeys key [key ...] arg [arg ...] @@ -168,7 +167,8 @@ SplitRequestPtr EvalRequest::create(Router& router, auto conn_pool = router.upstreamPool(incoming_request->asArray()[3].asString()); if (conn_pool) { request_ptr->conn_pool_ = conn_pool; - request_ptr->handle_ = conn_pool->makeRequest(incoming_request->asArray()[3].asString(), *incoming_request, *request_ptr); + request_ptr->handle_ = conn_pool->makeRequest(incoming_request->asArray()[3].asString(), + *incoming_request, *request_ptr); } if (!request_ptr->handle_) { @@ -202,8 +202,7 @@ void FragmentedRequest::onChildFailure(uint32_t index) { onChildResponse(Utility::makeError(Response::get().UpstreamFailure), index); } -SplitRequestPtr MGETRequest::create(Router& router, - Common::Redis::RespValuePtr&& incoming_request, +SplitRequestPtr MGETRequest::create(Router& router, Common::Redis::RespValuePtr&& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros) { std::unique_ptr request_ptr{ @@ -235,7 +234,7 @@ SplitRequestPtr MGETRequest::create(Router& router, if (conn_pool) { pending_request.conn_pool_ = conn_pool; pending_request.handle_ = conn_pool->makeRequest(incoming_request->asArray()[i].asString(), - single_mget, pending_request); + single_mget, pending_request); } if (!pending_request.handle_) { @@ -318,8 +317,7 @@ bool MGETRequest::onChildRedirection(const Common::Redis::RespValue& value, uint return (this->pending_requests_[index].handle_ != nullptr); } -SplitRequestPtr MSETRequest::create(Router& router, - Common::Redis::RespValuePtr&& incoming_request, +SplitRequestPtr MSETRequest::create(Router& router, Common::Redis::RespValuePtr&& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros) { if ((incoming_request->asArray().size() - 1) % 2 != 0) { @@ -358,7 +356,7 @@ SplitRequestPtr MSETRequest::create(Router& router, if (conn_pool) { pending_request.conn_pool_ = conn_pool; pending_request.handle_ = conn_pool->makeRequest(incoming_request->asArray()[i].asString(), - single_mset, pending_request); + single_mset, pending_request); } if (!pending_request.handle_) { @@ -470,7 +468,8 @@ SplitRequestPtr SplitKeysSumResultRequest::create(Router& router, auto conn_pool = router.upstreamPool(incoming_request->asArray()[i].asString()); if (conn_pool) { pending_request.conn_pool_ = conn_pool; - pending_request.handle_ = conn_pool->makeRequest(incoming_request->asArray()[i].asString(), single_fragment, pending_request); + pending_request.handle_ = conn_pool->makeRequest(incoming_request->asArray()[i].asString(), + single_fragment, pending_request); } if (!pending_request.handle_) { @@ -534,7 +533,8 @@ void SplitKeysSumResultRequest::recreate(Common::Redis::RespValue& request, uint } bool SplitKeysSumResultRequest::onChildRedirection(const Common::Redis::RespValue& value, - uint32_t index, ConnPool::InstanceSharedPtr conn_pool) { + uint32_t index, + ConnPool::InstanceSharedPtr conn_pool) { std::vector err; bool ask_redirection = false; if (redirectionArgsInvalid(incoming_request_.get(), value, err, ask_redirection) || !conn_pool) { @@ -549,9 +549,8 @@ bool SplitKeysSumResultRequest::onChildRedirection(const Common::Redis::RespValu return (this->pending_requests_[index].handle_ != nullptr); } -InstanceImpl::InstanceImpl(RouterPtr&& router, Stats::Scope& scope, - const std::string& stat_prefix, TimeSource& time_source, - bool latency_in_micros) +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_), 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 fb3b2c8abf781..4f63c3cdf3de6 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.h +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.h @@ -127,8 +127,7 @@ class SingleServerRequest : public SplitRequestBase, public Common::Redis::Clien */ class SimpleRequest : public SingleServerRequest { public: - static SplitRequestPtr create(Router& router, - Common::Redis::RespValuePtr&& incoming_request, + static SplitRequestPtr create(Router& router, Common::Redis::RespValuePtr&& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros); @@ -143,8 +142,7 @@ class SimpleRequest : public SingleServerRequest { */ class EvalRequest : public SingleServerRequest { public: - static SplitRequestPtr create(Router& router, - Common::Redis::RespValuePtr&& incoming_request, + static SplitRequestPtr create(Router& router, Common::Redis::RespValuePtr&& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros); @@ -210,8 +208,7 @@ class FragmentedRequest : public SplitRequestBase { */ class MGETRequest : public FragmentedRequest, Logger::Loggable { public: - static SplitRequestPtr create(Router& router, - Common::Redis::RespValuePtr&& incoming_request, + static SplitRequestPtr create(Router& router, Common::Redis::RespValuePtr&& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros); @@ -235,8 +232,7 @@ class MGETRequest : public FragmentedRequest, Logger::Loggable { public: - static SplitRequestPtr create(Router& router, - Common::Redis::RespValuePtr&& incoming_request, + static SplitRequestPtr create(Router& router, Common::Redis::RespValuePtr&& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros); @@ -261,8 +257,7 @@ class SplitKeysSumResultRequest : public FragmentedRequest, Logger::Loggable { public: - static SplitRequestPtr create(Router& router, - Common::Redis::RespValuePtr&& incoming_request, + static SplitRequestPtr create(Router& router, Common::Redis::RespValuePtr&& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros); @@ -289,8 +284,8 @@ class CommandHandlerFactory : public CommandHandler, CommandHandlerBase { SplitRequestPtr startRequest(Common::Redis::RespValuePtr&& request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros) { - return RequestClass::create(router_, std::move(request), callbacks, command_stats, - time_source, latency_in_micros); + return RequestClass::create(router_, std::move(request), callbacks, command_stats, time_source, + latency_in_micros); } }; diff --git a/source/extensions/filters/network/redis_proxy/router.h b/source/extensions/filters/network/redis_proxy/router.h index 76dfeece7ffdd..5312e34cea4be 100644 --- a/source/extensions/filters/network/redis_proxy/router.h +++ b/source/extensions/filters/network/redis_proxy/router.h @@ -21,8 +21,8 @@ class Router { virtual ~Router() = default; /** - * Returns a connection pool that matches a given route. When no match is found, the catch all pool is used. When - * remove prefix is set to true, the prefix will be removed from the key. + * Returns a connection pool that matches a given route. When no match is found, the catch all + * pool is used. When remove prefix is set to true, the prefix will be removed from the key. * @param key mutable reference to the key of the current command. * @return a handle to the connection pool. */ diff --git a/test/extensions/filters/network/redis_proxy/BUILD b/test/extensions/filters/network/redis_proxy/BUILD index cd0b408914a16..eb6d003e5927e 100644 --- a/test/extensions/filters/network/redis_proxy/BUILD +++ b/test/extensions/filters/network/redis_proxy/BUILD @@ -116,7 +116,7 @@ envoy_extension_cc_test( "//source/extensions/filters/network/redis_proxy:router_lib", "//test/extensions/filters/network/common/redis:redis_mocks", "//test/test_common:utility_lib", - ] + ], ) envoy_extension_cc_test( 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 6a9db0c15216c..555088f075731 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 @@ -33,9 +33,7 @@ class NoOpSplitCallbacks : public CommandSplitter::SplitCallbacks { }; class NullRouterImpl : public Router { - ConnPool::InstanceSharedPtr upstreamPool(std::string&) override { - return nullptr; - } + ConnPool::InstanceSharedPtr upstreamPool(std::string&) override { return nullptr; } }; class CommandLookUpSpeedTest { 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 b9dee80da4b9b..e87d17ee98e03 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 @@ -43,6 +43,7 @@ class PassthruRouter : public Router { PassthruRouter(ConnPool::InstanceSharedPtr conn_pool) : conn_pool_(conn_pool) {} ConnPool::InstanceSharedPtr upstreamPool(std::string&) override { return conn_pool_; } + private: ConnPool::InstanceSharedPtr conn_pool_; }; @@ -64,7 +65,8 @@ class RedisCommandSplitterImplTest : public testing::Test { ConnPool::MockInstance* conn_pool_{new ConnPool::MockInstance()}; NiceMock store_; Event::SimulatedTimeSystem time_system_; - InstanceImpl splitter_{std::make_unique(ConnPool::InstanceSharedPtr{conn_pool_}), store_, "redis.foo.", time_system_, false}; + InstanceImpl splitter_{std::make_unique(ConnPool::InstanceSharedPtr{conn_pool_}), + store_, "redis.foo.", time_system_, false}; MockSplitCallbacks callbacks_; SplitRequestPtr handle_; }; @@ -1275,9 +1277,9 @@ class RedisSingleServerRequestWithLatencyMicrosTest : public RedisSingleServerRe handle_ = splitter_.makeRequest(std::move(request), callbacks_); } - ConnPool::MockInstance* conn_pool_{new ConnPool::MockInstance()}; - InstanceImpl splitter_{std::make_unique(ConnPool::InstanceSharedPtr{conn_pool_}), store_, "redis.foo.", time_system_, true}; + InstanceImpl splitter_{std::make_unique(ConnPool::InstanceSharedPtr{conn_pool_}), + store_, "redis.foo.", time_system_, true}; }; TEST_P(RedisSingleServerRequestWithLatencyMicrosTest, Success) { From a4c86cf364285ad8e0b7c40db88363581340c2d8 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Fri, 12 Apr 2019 12:16:16 -0400 Subject: [PATCH 05/10] WIP Signed-off-by: Maxime Bedard --- .../redis_proxy/command_splitter_impl.cc | 6 ++-- .../redis_proxy/command_splitter_impl.h | 8 ++--- .../redis_proxy_integration_test.cc | 31 +++++++++++++++++-- 3 files changed, 35 insertions(+), 10 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 ba007b6b3b674..86006f509d179 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc @@ -302,7 +302,7 @@ void MGETRequest::recreate(Common::Redis::RespValue& request, uint32_t index, bo } bool MGETRequest::onChildRedirection(const Common::Redis::RespValue& value, uint32_t index, - ConnPool::InstanceSharedPtr conn_pool) { + const ConnPool::InstanceSharedPtr conn_pool) { std::vector err; bool ask_redirection = false; if (redirectionArgsInvalid(incoming_request_.get(), value, err, ask_redirection) || !conn_pool) { @@ -421,7 +421,7 @@ void MSETRequest::recreate(Common::Redis::RespValue& request, uint32_t index, bo } bool MSETRequest::onChildRedirection(const Common::Redis::RespValue& value, uint32_t index, - ConnPool::InstanceSharedPtr conn_pool) { + const ConnPool::InstanceSharedPtr conn_pool) { std::vector err; bool ask_redirection = false; if (redirectionArgsInvalid(incoming_request_.get(), value, err, ask_redirection) || !conn_pool) { @@ -534,7 +534,7 @@ void SplitKeysSumResultRequest::recreate(Common::Redis::RespValue& request, uint bool SplitKeysSumResultRequest::onChildRedirection(const Common::Redis::RespValue& value, uint32_t index, - ConnPool::InstanceSharedPtr conn_pool) { + const ConnPool::InstanceSharedPtr conn_pool) { std::vector err; bool ask_redirection = false; if (redirectionArgsInvalid(incoming_request_.get(), value, err, ask_redirection) || !conn_pool) { 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 4f63c3cdf3de6..bd72ca15db3f6 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.h +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.h @@ -191,7 +191,7 @@ class FragmentedRequest : public SplitRequestBase { virtual void onChildResponse(Common::Redis::RespValuePtr&& value, uint32_t index) PURE; void onChildFailure(uint32_t index); virtual bool onChildRedirection(const Common::Redis::RespValue& value, uint32_t index, - ConnPool::InstanceSharedPtr conn_pool) PURE; + const ConnPool::InstanceSharedPtr conn_pool) PURE; SplitCallbacks& callbacks_; @@ -220,7 +220,7 @@ class MGETRequest : public FragmentedRequest, Logger::Loggable&& command_strings) { @@ -147,6 +157,12 @@ class RedisProxyWithRedirectionIntegrationTest : public RedisProxyIntegrationTes const std::string& received_request, const std::string& response); }; +class RedisProxyWithRoutesIntegrationTest : public RedisProxyIntegrationTest { +public: + RedisProxyWithRoutesIntegrationTest() + : RedisProxyIntegrationTest(CONFIG_WITH_ROUTES, 2) {} +}; + INSTANTIATE_TEST_SUITE_P(IpVersions, RedisProxyIntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); @@ -237,6 +253,15 @@ void RedisProxyWithRedirectionIntegrationTest::simpleRedirection( EXPECT_TRUE(fake_upstream_connection_2->close()); } +// This test sends a simple "get foo" identical to SimpleRequestAndResponse +// but will hit the catch all upstream when routes are defined. +TEST_P(RedisProxyWithRoutesIntegrationTest, SimpleRequestAndResponse) { + initialize(); + simpleRoutedRequestAndResponse(makeBulkStringArray({"get", "foo"}), "$3\r\nbar\r\n"); +} + +// This test sends a simple + // This test sends a simple "get foo" command from a fake // downstream client through the proxy to a fake upstream // Redis server. The fake server sends a valid response From 45af715bad883553b59e52d4cebc518db6128215 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Mon, 15 Apr 2019 15:28:15 -0400 Subject: [PATCH 06/10] Add routing integration test Signed-off-by: Maxime Bedard --- .../redis_proxy_integration_test.cc | 123 +++++++++++++++--- 1 file changed, 108 insertions(+), 15 deletions(-) diff --git a/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc b/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc index 55de1f402e2eb..21cd037b51397 100644 --- a/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc +++ b/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc @@ -65,14 +65,86 @@ const std::string CONFIG_WITH_REDIRECTION = CONFIG + R"EOF( enable_redirection: true )EOF"; -const std::string CONFIG_WITH_ROUTES = CONFIG + R"EOF( +const std::string CONFIG_WITH_ROUTES = R"EOF( +admin: + access_log_path: /dev/null + address: + socket_address: + address: 127.0.0.1 + port_value: 0 +static_resources: + clusters: + - name: cluster_0 + type: STATIC + lb_policy: RANDOM + load_assignment: + cluster_name: cluster_0 + endpoints: + - lb_endpoints: + - endpoint: + address: + socket_address: + address: 127.0.0.1 + port_value: 0 + - endpoint: + address: + socket_address: + address: 127.0.0.1 + port_value: 0 + - name: cluster_1 + type: STATIC + lb_policy: RANDOM + load_assignment: + cluster_name: cluster_1 + endpoints: + - lb_endpoints: + - endpoint: + address: + socket_address: + address: 127.0.0.1 + port_value: 1 + - endpoint: + address: + socket_address: + address: 127.0.0.1 + port_value: 1 + - name: cluster_2 + type: STATIC + lb_policy: RANDOM + load_assignment: + cluster_name: cluster_2 + endpoints: + - lb_endpoints: + - endpoint: + address: + socket_address: + address: 127.0.0.1 + port_value: 2 + - endpoint: + address: + socket_address: + address: 127.0.0.1 + port_value: 2 + listeners: + name: listener_0 + address: + socket_address: + address: 127.0.0.1 + port_value: 0 + filter_chains: + filters: + name: envoy.redis_proxy + config: + stat_prefix: redis_stats + settings: + op_timeout: 5s prefix_routes: + catch_all_cluster: cluster_0 routes: - - prefix: "foo" + - prefix: "foo:" cluster: cluster_1 - - prefix: "bar" + - prefix: "baz:" cluster: cluster_2 - catch_all_cluster: cluster_0 )EOF"; // This function encodes commands as an array of bulkstrings as transmitted by Redis clients to @@ -125,7 +197,18 @@ class RedisProxyIntegrationTest : public testing::TestWithParamwrite(request); FakeRawConnectionPtr fake_upstream_connection; - EXPECT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + EXPECT_TRUE(upstream->waitForRawConnection(fake_upstream_connection)); EXPECT_TRUE(fake_upstream_connection->waitForData(request.size(), &proxy_to_server)); // The original request should be the same as the data received by the server. EXPECT_EQ(request, proxy_to_server); @@ -253,13 +340,6 @@ void RedisProxyWithRedirectionIntegrationTest::simpleRedirection( EXPECT_TRUE(fake_upstream_connection_2->close()); } -// This test sends a simple "get foo" identical to SimpleRequestAndResponse -// but will hit the catch all upstream when routes are defined. -TEST_P(RedisProxyWithRoutesIntegrationTest, SimpleRequestAndResponse) { - initialize(); - simpleRoutedRequestAndResponse(makeBulkStringArray({"get", "foo"}), "$3\r\nbar\r\n"); -} - // This test sends a simple // This test sends a simple "get foo" command from a fake @@ -396,5 +476,18 @@ TEST_P(RedisProxyWithRedirectionIntegrationTest, BadRedirectStrings) { } } +TEST_P(RedisProxyWithRoutesIntegrationTest, SimpleRequestAndResponseRoutedByPrefix) { + initialize(); + + // roundtrip to cluster_0 (catch_all route) + simpleRoundtripToUpstream(fake_upstreams_[0], makeBulkStringArray({"get", "toto"}), "$3\r\nbar\r\n"); + + // roundtrip to cluster_1 (prefix "foo:" route) + simpleRoundtripToUpstream(fake_upstreams_[2], makeBulkStringArray({"get", "foo:123"}), "$3\r\nbar\r\n"); + + // roundtrip to cluster_2 (prefix "baz:" route) + simpleRoundtripToUpstream(fake_upstreams_[4], makeBulkStringArray({"get", "baz:123"}), "$3\r\nbar\r\n"); +} + } // namespace } // namespace Envoy From eea7029dad61c7b0ed812ae98de752a5143542bb Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Mon, 15 Apr 2019 15:30:05 -0400 Subject: [PATCH 07/10] format Signed-off-by: Maxime Bedard --- .../redis_proxy_integration_test.cc | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc b/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc index 21cd037b51397..43b793e2e5dc6 100644 --- a/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc +++ b/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc @@ -207,7 +207,8 @@ class RedisProxyIntegrationTest : public testing::TestWithParamwrite(request); @@ -480,13 +481,16 @@ TEST_P(RedisProxyWithRoutesIntegrationTest, SimpleRequestAndResponseRoutedByPref initialize(); // roundtrip to cluster_0 (catch_all route) - simpleRoundtripToUpstream(fake_upstreams_[0], makeBulkStringArray({"get", "toto"}), "$3\r\nbar\r\n"); + simpleRoundtripToUpstream(fake_upstreams_[0], makeBulkStringArray({"get", "toto"}), + "$3\r\nbar\r\n"); // roundtrip to cluster_1 (prefix "foo:" route) - simpleRoundtripToUpstream(fake_upstreams_[2], makeBulkStringArray({"get", "foo:123"}), "$3\r\nbar\r\n"); + simpleRoundtripToUpstream(fake_upstreams_[2], makeBulkStringArray({"get", "foo:123"}), + "$3\r\nbar\r\n"); // roundtrip to cluster_2 (prefix "baz:" route) - simpleRoundtripToUpstream(fake_upstreams_[4], makeBulkStringArray({"get", "baz:123"}), "$3\r\nbar\r\n"); + simpleRoundtripToUpstream(fake_upstreams_[4], makeBulkStringArray({"get", "baz:123"}), + "$3\r\nbar\r\n"); } } // namespace From 8fdcf56f3903c892382dd559e727012862d36e69 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Mon, 15 Apr 2019 16:27:07 -0400 Subject: [PATCH 08/10] format once more Signed-off-by: Maxime Bedard --- .../filters/network/redis_proxy/command_splitter_impl.cc | 9 +++------ 1 file changed, 3 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 7ace5f87540d8..751583abc3dc6 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc @@ -340,8 +340,7 @@ void MGETRequest::recreate(Common::Redis::RespValue& request, uint32_t index) { request.asArray().swap(values); } -SplitRequestPtr MSETRequest::create(Router& router, - Common::Redis::RespValuePtr&& incoming_request, +SplitRequestPtr MSETRequest::create(Router& router, Common::Redis::RespValuePtr&& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros) { if ((incoming_request->asArray().size() - 1) % 2 != 0) { @@ -533,10 +532,8 @@ void SplitKeysSumResultRequest::recreate(Common::Redis::RespValue& request, uint request.asArray().swap(values); } - -InstanceImpl::InstanceImpl(RouterPtr&& router, Stats::Scope& scope, - const std::string& stat_prefix, TimeSource& time_source, - bool latency_in_micros) +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_), From b2fb0a85c2be62817007515d288b6f3b4fda19dd Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Mon, 15 Apr 2019 21:22:03 -0400 Subject: [PATCH 09/10] Fix comments on PR and remove bad merge conflict leftovers Signed-off-by: Maxime Bedard --- docs/root/intro/deprecated.rst | 2 +- docs/root/intro/version_history.rst | 2 +- .../filters/network/redis_proxy/command_splitter_impl.cc | 2 +- .../filters/network/redis_proxy/command_splitter_impl.h | 2 +- .../filters/network/redis_proxy/redis_proxy_integration_test.cc | 2 -- 5 files changed, 4 insertions(+), 6 deletions(-) diff --git a/docs/root/intro/deprecated.rst b/docs/root/intro/deprecated.rst index f8ac0ac251e1c..2381acf8208c6 100644 --- a/docs/root/intro/deprecated.rst +++ b/docs/root/intro/deprecated.rst @@ -13,7 +13,7 @@ Deprecated items below are listed in chronological order. Version 1.11.0 (Pending) ======================== -* Use of :ref:`cluster ` in :ref:`redis_proxy.proto ` +* Use of :ref:`cluster ` in :ref:`redis_proxy.proto ` is deprecated. Set a :ref:`catch_all_cluster ` instead. Version 1.10.0 (Apr 5, 2019) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 1b22fec0b34cb..348872c2f0a62 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -5,6 +5,7 @@ Version history ================ * dubbo_proxy: support the :ref:`Dubbo proxy filter `. * http: mitigated a race condition with the :ref:`delayed_close_timeout` where it could trigger while actively flushing a pending write buffer for a downstream connection. +* redis: added :ref:`prefix routing ` to enable routing commands based on their key's prefix to different upstream. * redis: add support for zpopmax and zpopmin commands. * upstream: added :ref:`upstream_cx_pool_overflow ` for the connection pool circuit breaker. @@ -65,7 +66,6 @@ Version history * ratelimit: removed deprecated rate limit configuration from bootstrap. * redis: added :ref:`hashtagging ` to guarantee a given key's upstream. * redis: added :ref:`latency stats ` for commands. -* redis: added :ref:`prefix routing ` to enable routing commands based on their key's prefix to different upstream. * redis: added :ref:`success and error stats ` for commands. * redis: migrate hash function for host selection to `MurmurHash2 `_ from std::hash. MurmurHash2 is compatible with std::hash in GNU libstdc++ 3.4.20 or above. This is typically the case when compiled on Linux and not macOS. * redis: added :ref:`latency_in_micros ` to specify the redis commands stats time unit in microseconds. diff --git a/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc b/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc index 751583abc3dc6..276c1ac15f867 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc @@ -264,7 +264,7 @@ SplitRequestPtr MGETRequest::create(Router& router, Common::Redis::RespValuePtr& } bool FragmentedRequest::onChildRedirection(const Common::Redis::RespValue& value, uint32_t index, - const ConnPool::InstanceSharedPtr conn_pool) { + const ConnPool::InstanceSharedPtr& conn_pool) { std::vector err; bool ask_redirection = false; if (redirectionArgsInvalid(incoming_request_.get(), value, err, ask_redirection) || !conn_pool) { 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 5b2bb66142c5f..5ca017ca8fdb9 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.h +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.h @@ -189,7 +189,7 @@ class FragmentedRequest : public SplitRequestBase { virtual void onChildResponse(Common::Redis::RespValuePtr&& value, uint32_t index) PURE; void onChildFailure(uint32_t index); bool onChildRedirection(const Common::Redis::RespValue& value, uint32_t index, - const ConnPool::InstanceSharedPtr conn_pool); + const ConnPool::InstanceSharedPtr& conn_pool); virtual void recreate(Common::Redis::RespValue& request, uint32_t index) PURE; SplitCallbacks& callbacks_; diff --git a/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc b/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc index bbbbb97d58dc1..07997b76c0f1d 100644 --- a/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc +++ b/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc @@ -354,8 +354,6 @@ void RedisProxyWithRedirectionIntegrationTest::simpleRedirection( EXPECT_TRUE(fake_upstream_connection_2->close()); } -// This test sends a simple - // This test sends a simple "get foo" command from a fake // downstream client through the proxy to a fake upstream // Redis server. The fake server sends a valid response From a72162923f5646235830837f9b1cb584b9207009 Mon Sep 17 00:00:00 2001 From: Maxime Bedard Date: Mon, 15 Apr 2019 21:31:50 -0400 Subject: [PATCH 10/10] fix docs CI error Signed-off-by: Maxime Bedard --- docs/root/intro/deprecated.rst | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/docs/root/intro/deprecated.rst b/docs/root/intro/deprecated.rst index 2381acf8208c6..b3a7b7d918a2d 100644 --- a/docs/root/intro/deprecated.rst +++ b/docs/root/intro/deprecated.rst @@ -12,9 +12,7 @@ Deprecated items below are listed in chronological order. Version 1.11.0 (Pending) ======================== - -* Use of :ref:`cluster ` in :ref:`redis_proxy.proto ` - is deprecated. Set a :ref:`catch_all_cluster ` instead. +* Use of :ref:`cluster ` in :ref:`redis_proxy.proto ` is deprecated. Set a :ref:`catch_all_cluster ` instead. Version 1.10.0 (Apr 5, 2019) ============================