From bd100c7620ab0d4d8d7a0cfbb9fb1474d3f31294 Mon Sep 17 00:00:00 2001 From: Mitch Sukalski Date: Wed, 10 Apr 2019 10:38:27 -0700 Subject: [PATCH 1/5] redis: fix asking output for ask redirection support - issue separate, preceding "asking" command instead of prefixing "asking" to the redirected command. - combined all derived requests' onChildRedirection() methods into a single method. - fixed affected unit and integration tests. Signed-off-by: Mitch Sukalski --- .../redis_proxy/command_splitter_impl.cc | 138 +++++++----------- .../redis_proxy/command_splitter_impl.h | 31 ++-- .../redis_proxy/command_splitter_impl_test.cc | 82 ++++++++--- .../redis_proxy_integration_test.cc | 37 +++-- 4 files changed, 157 insertions(+), 131 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 136ad69169cc8..b56ce13d47b82 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc @@ -27,6 +27,19 @@ Common::Redis::RespValuePtr Utility::makeError(const std::string& error) { namespace { +// nullPoolCallbacks is used for requests that must be filtered and not redirected such as "asking". +DoNothingPoolCallbacks nullPoolCallbacks; + +// Create an asking command request. +Common::Redis::RespValue askingRequest() { + Common::Redis::RespValue asking_cmd, request; + asking_cmd.type(Common::Redis::RespType::BulkString); + asking_cmd.asString() = "asking"; + request.type(Common::Redis::RespType::Array); + request.asArray().push_back(asking_cmd); + return request; +} + /** * Validate the received moved/ask redirection error and the original redis request. * @param[in] original_request supplies the incoming request associated with the command splitter @@ -91,22 +104,6 @@ void SingleServerRequest::onFailure() { callbacks_.onResponse(Utility::makeError(Response::get().UpstreamFailure)); } -void SingleServerRequest::recreate(Common::Redis::RespValue& request, bool prepend_asking) { - if (!prepend_asking) { - request = *incoming_request_; - return; - } - - Common::Redis::RespValue asking_cmd; - asking_cmd.type(Common::Redis::RespType::BulkString); - asking_cmd.asString() = "asking"; - - request.type(Common::Redis::RespType::Array); - request.asArray().push_back(asking_cmd); - request.asArray().insert(request.asArray().end(), incoming_request_->asArray().begin(), - incoming_request_->asArray().end()); -} - bool SingleServerRequest::onRedirection(const Common::Redis::RespValue& value) { std::vector err; bool ask_redirection = false; @@ -114,11 +111,13 @@ bool SingleServerRequest::onRedirection(const Common::Redis::RespValue& value) { return false; } - Common::Redis::RespValue request; - recreate(request, ask_redirection); - const std::string host_address = std::string(err[2]); // ip:port - handle_ = conn_pool_->makeRequestToHost(host_address, request, *this); + // Prepend request with asking command if redirected via ASK error. + if (ask_redirection && + !conn_pool_->makeRequestToHost(host_address, askingRequest(), nullPoolCallbacks)) { + return false; + } + handle_ = conn_pool_->makeRequestToHost(host_address, *incoming_request_, *this); return (handle_ != nullptr); } @@ -240,6 +239,35 @@ SplitRequestPtr MGETRequest::create(ConnPool::Instance& conn_pool, return nullptr; } +bool FragmentedRequest::onChildRedirection(const Common::Redis::RespValue& value, uint32_t index, + ConnPool::Instance* conn_pool) { + std::vector err; + bool ask_redirection = false; + if (redirectionArgsInvalid(incoming_request_.get(), value, err, ask_redirection) || !conn_pool) { + return false; + } + + // MOVED and ASK redirection errors have the following substrings: MOVED or ASK (err[0]), hash key + // slot (err[1]), and IP address and TCP port separated by a colon (err[2]). + std::string host_address = std::string(err[2]); + Common::Redis::RespValue request; + recreate(request, index); + + // Prepend request with an asking command if redirected via an ASK error. The returned handle is + // not important since there is no point in being able to cancel the request. The use of + // nullPoolCallbacks ensures the transparent filtering of the Redis server's response to the + // "asking" command; this is fine since the server either responds with an OK or an error message + // if cluster support is not enabled (in which case, we should not get an ASK redirection error). + if (ask_redirection && + !conn_pool->makeRequestToHost(host_address, askingRequest(), nullPoolCallbacks)) { + return false; + } + + this->pending_requests_[index].handle_ = + conn_pool->makeRequestToHost(host_address, request, this->pending_requests_[index]); + return (this->pending_requests_[index].handle_ != nullptr); +} + void MGETRequest::onChildResponse(Common::Redis::RespValuePtr&& value, uint32_t index) { pending_requests_[index].handle_ = nullptr; @@ -273,9 +301,9 @@ void MGETRequest::onChildResponse(Common::Redis::RespValuePtr&& value, uint32_t } } -void MGETRequest::recreate(Common::Redis::RespValue& request, uint32_t index, bool prepend_asking) { +void MGETRequest::recreate(Common::Redis::RespValue& request, uint32_t index) { static const uint32_t GET_COMMAND_SUBSTRINGS = 2; - uint32_t num_values = prepend_asking ? (GET_COMMAND_SUBSTRINGS + 1) : GET_COMMAND_SUBSTRINGS; + uint32_t num_values = GET_COMMAND_SUBSTRINGS; std::vector values(num_values); for (uint32_t i = 0; i < num_values; i++) { @@ -283,30 +311,11 @@ void MGETRequest::recreate(Common::Redis::RespValue& request, uint32_t index, bo } values[--num_values].asString() = incoming_request_->asArray()[index + 1].asString(); values[--num_values].asString() = "get"; - if (prepend_asking) { - values[--num_values].asString() = "asking"; - } request.type(Common::Redis::RespType::Array); request.asArray().swap(values); } -bool MGETRequest::onChildRedirection(const Common::Redis::RespValue& value, uint32_t index, - ConnPool::Instance* conn_pool) { - std::vector err; - bool ask_redirection = false; - if (redirectionArgsInvalid(incoming_request_.get(), value, err, ask_redirection) || !conn_pool) { - return false; - } - - Common::Redis::RespValue request; - recreate(request, index, ask_redirection); - - this->pending_requests_[index].handle_ = - conn_pool->makeRequestToHost(std::string(err[2]), request, this->pending_requests_[index]); - return (this->pending_requests_[index].handle_ != nullptr); -} - SplitRequestPtr MSETRequest::create(ConnPool::Instance& conn_pool, Common::Redis::RespValuePtr&& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, @@ -388,9 +397,9 @@ void MSETRequest::onChildResponse(Common::Redis::RespValuePtr&& value, uint32_t } } -void MSETRequest::recreate(Common::Redis::RespValue& request, uint32_t index, bool prepend_asking) { +void MSETRequest::recreate(Common::Redis::RespValue& request, uint32_t index) { static const uint32_t SET_COMMAND_SUBSTRINGS = 3; - uint32_t num_values = prepend_asking ? (SET_COMMAND_SUBSTRINGS + 1) : SET_COMMAND_SUBSTRINGS; + uint32_t num_values = SET_COMMAND_SUBSTRINGS; std::vector values(num_values); for (uint32_t i = 0; i < num_values; i++) { @@ -399,30 +408,11 @@ void MSETRequest::recreate(Common::Redis::RespValue& request, uint32_t index, bo values[--num_values].asString() = incoming_request_->asArray()[(index * 2) + 2].asString(); values[--num_values].asString() = incoming_request_->asArray()[(index * 2) + 1].asString(); values[--num_values].asString() = "set"; - if (prepend_asking) { - values[--num_values].asString() = "asking"; - } request.type(Common::Redis::RespType::Array); request.asArray().swap(values); } -bool MSETRequest::onChildRedirection(const Common::Redis::RespValue& value, uint32_t index, - ConnPool::Instance* conn_pool) { - std::vector err; - bool ask_redirection = false; - if (redirectionArgsInvalid(incoming_request_.get(), value, err, ask_redirection) || !conn_pool) { - return false; - } - - Common::Redis::RespValue request; - recreate(request, index, ask_redirection); - - this->pending_requests_[index].handle_ = - conn_pool->makeRequestToHost(std::string(err[2]), request, this->pending_requests_[index]); - return (this->pending_requests_[index].handle_ != nullptr); -} - SplitRequestPtr SplitKeysSumResultRequest::create(ConnPool::Instance& conn_pool, Common::Redis::RespValuePtr&& incoming_request, SplitCallbacks& callbacks, @@ -496,10 +486,9 @@ void SplitKeysSumResultRequest::onChildResponse(Common::Redis::RespValuePtr&& va } } -void SplitKeysSumResultRequest::recreate(Common::Redis::RespValue& request, uint32_t index, - bool prepend_asking) { +void SplitKeysSumResultRequest::recreate(Common::Redis::RespValue& request, uint32_t index) { static const uint32_t BASE_COMMAND_SUBSTRINGS = 2; - uint32_t num_values = prepend_asking ? (BASE_COMMAND_SUBSTRINGS + 1) : BASE_COMMAND_SUBSTRINGS; + uint32_t num_values = BASE_COMMAND_SUBSTRINGS; std::vector values(num_values); for (uint32_t i = 0; i < num_values; i++) { @@ -507,30 +496,11 @@ void SplitKeysSumResultRequest::recreate(Common::Redis::RespValue& request, uint } values[--num_values].asString() = incoming_request_->asArray()[index + 1].asString(); values[--num_values].asString() = incoming_request_->asArray()[0].asString(); - if (prepend_asking) { - values[--num_values].asString() = "asking"; - } request.type(Common::Redis::RespType::Array); request.asArray().swap(values); } -bool SplitKeysSumResultRequest::onChildRedirection(const Common::Redis::RespValue& value, - uint32_t index, ConnPool::Instance* conn_pool) { - std::vector err; - bool ask_redirection = false; - if (redirectionArgsInvalid(incoming_request_.get(), value, err, ask_redirection) || !conn_pool) { - return false; - } - - Common::Redis::RespValue request; - recreate(request, index, ask_redirection); - - this->pending_requests_[index].handle_ = - conn_pool->makeRequestToHost(std::string(err[2]), request, this->pending_requests_[index]); - return (this->pending_requests_[index].handle_ != nullptr); -} - InstanceImpl::InstanceImpl(ConnPool::InstancePtr&& conn_pool, Stats::Scope& scope, const std::string& stat_prefix, TimeSource& time_source, bool latency_in_micros) 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 e6b1d475464e3..857451a55a64a 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.h +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.h @@ -113,8 +113,6 @@ class SingleServerRequest : public SplitRequestBase, public Common::Redis::Clien TimeSource& time_source, bool latency_in_micros) : SplitRequestBase(command_stats, time_source, latency_in_micros), callbacks_(callbacks) {} - void recreate(Common::Redis::RespValue& request, bool prepend_asking); - SplitCallbacks& callbacks_; ConnPool::Instance* conn_pool_{}; Common::Redis::Client::PoolRequest* handle_{}; @@ -191,8 +189,9 @@ 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::Instance* conn_pool) PURE; + bool onChildRedirection(const Common::Redis::RespValue& value, uint32_t index, + ConnPool::Instance* conn_pool); + virtual void recreate(Common::Redis::RespValue& request, uint32_t index) PURE; SplitCallbacks& callbacks_; @@ -221,9 +220,7 @@ class MGETRequest : public FragmentedRequest, Logger::Loggable { TimeSource& time_source_; }; +/** + * DoNothingPoolCallbacks is used for internally generated commands whose response is + * transparently filtered, and redirection never occurs (e.g., "asking", etc.). + */ +class DoNothingPoolCallbacks : public Common::Redis::Client::PoolCallbacks { +public: + // Common::Redis::Client::PoolCallbacks + void onResponse(Common::Redis::RespValuePtr&&) override{}; + void onFailure() override{}; + bool onRedirection(const Common::Redis::RespValue&) override { return false; }; +}; + } // namespace CommandSplitter } // namespace RedisProxy } // namespace NetworkFilters 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 b4f2c8fb70110..a50186c6d7bb4 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 @@ -412,7 +412,7 @@ TEST_F(RedisSingleServerRequestTest, RedirectionFailure) { TEST_F(RedisSingleServerRequestTest, AskRedirectionSuccess) { InSequence s; - Common::Redis::Client::MockPoolRequest pool_request2; + Common::Redis::Client::MockPoolRequest pool_request2, pool_request3; Common::Redis::RespValuePtr request{new Common::Redis::RespValue()}; makeBulkStringArray(*request, {"get", "foo"}); makeRequest("foo", std::move(request)); @@ -421,12 +421,12 @@ TEST_F(RedisSingleServerRequestTest, AskRedirectionSuccess) { Common::Redis::RespValue ask_response; ask_response.type(Common::Redis::RespType::Error); ask_response.asString() = "ASK 1111 10.1.2.3:4000"; - EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, Ref(*pool_callbacks_))) + EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, _)) .WillOnce( Invoke([&](const std::string& host_address, const Common::Redis::RespValue& request, Common::Redis::Client::PoolCallbacks&) -> Common::Redis::Client::PoolRequest* { - // Verify that the request has been properly modified in place with an "asking" prefix. - std::vector commands = {"asking", "get", "foo"}; + // Verify that the request has been properly prepended with an "asking" command. + std::vector commands = {"asking"}; EXPECT_EQ(host_address, "10.1.2.3:4000"); EXPECT_TRUE(request.type() == Common::Redis::RespType::Array); EXPECT_EQ(request.asArray().size(), commands.size()); @@ -436,6 +436,20 @@ TEST_F(RedisSingleServerRequestTest, AskRedirectionSuccess) { } return &pool_request2; })); + EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, Ref(*pool_callbacks_))) + .WillOnce( + Invoke([&](const std::string& host_address, const Common::Redis::RespValue& request, + Common::Redis::Client::PoolCallbacks&) -> Common::Redis::Client::PoolRequest* { + std::vector commands = {"get", "foo"}; + EXPECT_EQ(host_address, "10.1.2.3:4000"); + EXPECT_TRUE(request.type() == Common::Redis::RespType::Array); + EXPECT_EQ(request.asArray().size(), commands.size()); + for (unsigned int i = 0; i < commands.size(); i++) { + EXPECT_TRUE(request.asArray()[i].type() == Common::Redis::RespType::BulkString); + EXPECT_EQ(request.asArray()[i].asString(), commands[i]); + } + return &pool_request3; + })); EXPECT_TRUE(pool_callbacks_->onRedirection(ask_response)); respond(); }; @@ -738,20 +752,30 @@ TEST_F(RedisMGETCommandHandlerTest, NormalWithAskRedirection) { Common::Redis::RespValue ask_response; ask_response.type(Common::Redis::RespType::Error); ask_response.asString() = "ASK 1234 192.168.0.1:5000"; // Exact values are not important. + Common::Redis::Client::MockPoolRequest dummy_poolrequest; for (unsigned int i = 0; i < 2; i++) { - EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, Ref(*pool_callbacks_[i]))) + EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, _)) .WillOnce(Invoke( [&](const std::string& host_address, const Common::Redis::RespValue& request, Common::Redis::Client::PoolCallbacks&) -> Common::Redis::Client::PoolRequest* { EXPECT_EQ(host_address, "192.168.0.1:5000"); EXPECT_TRUE(request.type() == Common::Redis::RespType::Array); - EXPECT_EQ(request.asArray().size(), 3); + EXPECT_EQ(request.asArray().size(), 1); EXPECT_TRUE(request.asArray()[0].type() == Common::Redis::RespType::BulkString); EXPECT_EQ(request.asArray()[0].asString(), "asking"); + return &dummy_poolrequest; + })); + EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, Ref(*pool_callbacks_[i]))) + .WillOnce(Invoke( + [&](const std::string& host_address, const Common::Redis::RespValue& request, + Common::Redis::Client::PoolCallbacks&) -> Common::Redis::Client::PoolRequest* { + EXPECT_EQ(host_address, "192.168.0.1:5000"); + EXPECT_TRUE(request.type() == Common::Redis::RespType::Array); + EXPECT_EQ(request.asArray().size(), 2); + EXPECT_TRUE(request.asArray()[0].type() == Common::Redis::RespType::BulkString); + EXPECT_EQ(request.asArray()[0].asString(), "get"); EXPECT_TRUE(request.asArray()[1].type() == Common::Redis::RespType::BulkString); - EXPECT_EQ(request.asArray()[1].asString(), "get"); - EXPECT_TRUE(request.asArray()[2].type() == Common::Redis::RespType::BulkString); - EXPECT_EQ(request.asArray()[2].asString(), std::to_string(i)); + EXPECT_EQ(request.asArray()[1].asString(), std::to_string(i)); EXPECT_NE(&pool_requests_[i], nullptr); return &pool_requests_[i]; })); @@ -985,22 +1009,32 @@ TEST_F(RedisMSETCommandHandlerTest, NormalWithAskRedirection) { Common::Redis::RespValue ask_response; ask_response.type(Common::Redis::RespType::Error); ask_response.asString() = "ASK 1234 192.168.0.1:5000"; // Exact values are not important. + Common::Redis::Client::MockPoolRequest dummy_poolrequest; for (unsigned int i = 0; i < 2; i++) { - EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, Ref(*pool_callbacks_[i]))) + EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, _)) .WillOnce(Invoke( [&](const std::string& host_address, const Common::Redis::RespValue& request, Common::Redis::Client::PoolCallbacks&) -> Common::Redis::Client::PoolRequest* { EXPECT_EQ(host_address, "192.168.0.1:5000"); EXPECT_TRUE(request.type() == Common::Redis::RespType::Array); - EXPECT_EQ(request.asArray().size(), 4); + EXPECT_EQ(request.asArray().size(), 1); EXPECT_TRUE(request.asArray()[0].type() == Common::Redis::RespType::BulkString); EXPECT_EQ(request.asArray()[0].asString(), "asking"); + return &dummy_poolrequest; + })); + EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, Ref(*pool_callbacks_[i]))) + .WillOnce(Invoke( + [&](const std::string& host_address, const Common::Redis::RespValue& request, + Common::Redis::Client::PoolCallbacks&) -> Common::Redis::Client::PoolRequest* { + EXPECT_EQ(host_address, "192.168.0.1:5000"); + EXPECT_TRUE(request.type() == Common::Redis::RespType::Array); + EXPECT_EQ(request.asArray().size(), 3); + EXPECT_TRUE(request.asArray()[0].type() == Common::Redis::RespType::BulkString); + EXPECT_EQ(request.asArray()[0].asString(), "set"); EXPECT_TRUE(request.asArray()[1].type() == Common::Redis::RespType::BulkString); - EXPECT_EQ(request.asArray()[1].asString(), "set"); + EXPECT_EQ(request.asArray()[1].asString(), std::to_string(i)); EXPECT_TRUE(request.asArray()[2].type() == Common::Redis::RespType::BulkString); EXPECT_EQ(request.asArray()[2].asString(), std::to_string(i)); - EXPECT_TRUE(request.asArray()[3].type() == Common::Redis::RespType::BulkString); - EXPECT_EQ(request.asArray()[3].asString(), std::to_string(i)); EXPECT_NE(&pool_requests_[i], nullptr); return &pool_requests_[i]; })); @@ -1209,20 +1243,30 @@ TEST_P(RedisSplitKeysSumResultHandlerTest, NormalWithAskRedirection) { Common::Redis::RespValue ask_response; ask_response.type(Common::Redis::RespType::Error); ask_response.asString() = "ASK 1234 192.168.0.1:5000"; // Exact values are not important. + Common::Redis::Client::MockPoolRequest dummy_poolrequest; for (unsigned int i = 0; i < 2; i++) { - EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, Ref(*pool_callbacks_[i]))) + EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, _)) .WillOnce(Invoke( [&](const std::string& host_address, const Common::Redis::RespValue& request, Common::Redis::Client::PoolCallbacks&) -> Common::Redis::Client::PoolRequest* { EXPECT_EQ(host_address, "192.168.0.1:5000"); EXPECT_TRUE(request.type() == Common::Redis::RespType::Array); - EXPECT_EQ(request.asArray().size(), 3); + EXPECT_EQ(request.asArray().size(), 1); EXPECT_TRUE(request.asArray()[0].type() == Common::Redis::RespType::BulkString); EXPECT_EQ(request.asArray()[0].asString(), "asking"); + return &dummy_poolrequest; + })); + EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, Ref(*pool_callbacks_[i]))) + .WillOnce(Invoke( + [&](const std::string& host_address, const Common::Redis::RespValue& request, + Common::Redis::Client::PoolCallbacks&) -> Common::Redis::Client::PoolRequest* { + EXPECT_EQ(host_address, "192.168.0.1:5000"); + EXPECT_TRUE(request.type() == Common::Redis::RespType::Array); + EXPECT_EQ(request.asArray().size(), 2); + EXPECT_TRUE(request.asArray()[0].type() == Common::Redis::RespType::BulkString); + EXPECT_EQ(request.asArray()[0].asString(), GetParam()); EXPECT_TRUE(request.asArray()[1].type() == Common::Redis::RespType::BulkString); - EXPECT_EQ(request.asArray()[1].asString(), GetParam()); - EXPECT_TRUE(request.asArray()[2].type() == Common::Redis::RespType::BulkString); - EXPECT_EQ(request.asArray()[2].asString(), std::to_string(i)); + EXPECT_EQ(request.asArray()[1].asString(), std::to_string(i)); EXPECT_NE(&pool_requests_[i], nullptr); return &pool_requests_[i]; })); 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 ab727c0f846c6..f942fc302875b 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 @@ -139,12 +139,10 @@ class RedisProxyWithRedirectionIntegrationTest : public RedisProxyIntegrationTes * @param target_server a handle to the second server that will respond to the request. * @param request supplies client data to transmit to the first upstream server. * @param redirection_response supplies the moved or ask redirection error from the first server. - * @param received_request suplies data received by the second server from the proxy. * @param response supplies data sent by the second server back to the fake Redis client. */ void simpleRedirection(FakeUpstreamPtr& target_server, const std::string& request, - const std::string& redirection_response, - const std::string& received_request, const std::string& response); + const std::string& redirection_response, const std::string& response); }; INSTANTIATE_TEST_SUITE_P(IpVersions, RedisProxyIntegrationTest, @@ -201,8 +199,9 @@ void RedisProxyIntegrationTest::simpleProxyResponse(const std::string& request, void RedisProxyWithRedirectionIntegrationTest::simpleRedirection( FakeUpstreamPtr& target_server, const std::string& request, - const std::string& redirection_response, const std::string& received_request, - const std::string& response) { + const std::string& redirection_response, const std::string& response) { + + bool asking = (redirection_response.find("-ASK") != std::string::npos); std::string proxy_to_server; IntegrationTcpClientPtr redis_client = makeTcpConnection(lookupPort("redis_proxy")); redis_client->write(request); @@ -221,10 +220,20 @@ void RedisProxyWithRedirectionIntegrationTest::simpleRedirection( // The proxy should initiate a new connection to the fake redis server, target_server, in // response. EXPECT_TRUE(target_server->waitForRawConnection(fake_upstream_connection_2)); - // The server, target_server, should receive received_request which may or may not be the same as - // the original request. - EXPECT_TRUE(fake_upstream_connection_2->waitForData(received_request.size(), &proxy_to_server)); - EXPECT_EQ(received_request, proxy_to_server); + + if (asking) { + // The server, target_server, should receive an "asking" command before the original request. + std::string asking_request = makeBulkStringArray({"asking"}); + EXPECT_TRUE(fake_upstream_connection_2->waitForData(asking_request.size() + request.size(), + &proxy_to_server)); + EXPECT_EQ(asking_request + request, proxy_to_server); + // Respond with an "OK" to the "asking" command. + EXPECT_TRUE(fake_upstream_connection_2->write("+OK\r\n")); + } else { + // The server, target_server, should receive request unchanged. + EXPECT_TRUE(fake_upstream_connection_2->waitForData(request.size(), &proxy_to_server)); + EXPECT_EQ(request, proxy_to_server); + } // Send response from the second fake Redis server, target_server, to the client. EXPECT_TRUE(fake_upstream_connection_2->write(response)); @@ -287,12 +296,11 @@ TEST_P(RedisProxyWithRedirectionIntegrationTest, RedirectToKnownServer) { initialize(); std::stringstream redirection_error; redirection_error << "-MOVED 1111 " << redisAddressAndPort(fake_upstreams_[1]) << "\r\n"; - simpleRedirection(fake_upstreams_[1], request, redirection_error.str(), request, "$3\r\nbar\r\n"); + simpleRedirection(fake_upstreams_[1], request, redirection_error.str(), "$3\r\nbar\r\n"); redirection_error.str(""); redirection_error << "-ASK 1111 " << redisAddressAndPort(fake_upstreams_[1]) << "\r\n"; - simpleRedirection(fake_upstreams_[1], request, redirection_error.str(), - makeBulkStringArray({"asking", "get", "foo"}), "$3\r\nbar\r\n"); + simpleRedirection(fake_upstreams_[1], request, redirection_error.str(), "$3\r\nbar\r\n"); } // This test sends a simple Redis commands to a sequence of fake upstream @@ -312,12 +320,11 @@ TEST_P(RedisProxyWithRedirectionIntegrationTest, RedirectToUnknownServer) { std::stringstream redirection_error; redirection_error << "-MOVED 1111 " << redisAddressAndPort(target_server) << "\r\n"; - simpleRedirection(target_server, request, redirection_error.str(), request, "$3\r\nbar\r\n"); + simpleRedirection(target_server, request, redirection_error.str(), "$3\r\nbar\r\n"); redirection_error.str(""); redirection_error << "-ASK 1111 " << redisAddressAndPort(target_server) << "\r\n"; - simpleRedirection(target_server, request, redirection_error.str(), - makeBulkStringArray({"asking", "get", "foo"}), "$3\r\nbar\r\n"); + simpleRedirection(target_server, request, redirection_error.str(), "$3\r\nbar\r\n"); } // This test verifies that various forms of bad MOVED/ASK redirection errors From b2bb0693b65ca110a1a0ea9dafe86f48eec6da86 Mon Sep 17 00:00:00 2001 From: Mitch Sukalski Date: Wed, 10 Apr 2019 17:03:55 -0700 Subject: [PATCH 2/5] redis: fix asking output for ask redirection support (#6543) - adding additional unit and integration negative tests for coverage Signed-off-by: Mitch Sukalski --- .../redis_proxy/command_splitter_impl_test.cc | 62 +++++++++++++++ .../redis_proxy_integration_test.cc | 76 ++++++++++++++++++- 2 files changed, 134 insertions(+), 4 deletions(-) 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 a50186c6d7bb4..10fa4eb04a6b7 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 @@ -385,6 +385,14 @@ TEST_F(RedisSingleServerRequestTest, MovedRedirectionFailure) { moved_response.asInteger() = 1; EXPECT_FALSE(pool_callbacks_->onRedirection(moved_response)); + // Test an upstream error preventing the request from being sent. + moved_response.type(Common::Redis::RespType::Error); + moved_response.asString() = "MOVED 1111 10.1.2.3:4000"; + std::string host_address; + Common::Redis::RespValue request_copy; + EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, _)).WillOnce(Return(nullptr)); + EXPECT_FALSE(pool_callbacks_->onRedirection(moved_response)); + respond(); }; @@ -472,6 +480,60 @@ TEST_F(RedisSingleServerRequestTest, AskRedirectionFailure) { ask_response.asInteger() = 1; EXPECT_FALSE(pool_callbacks_->onRedirection(ask_response)); + // Test an upstream error from trying to send an "asking" command upstream. + ask_response.type(Common::Redis::RespType::Error); + ask_response.asString() = "ASK 1111 10.1.2.3:4000"; + EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, _)) + .WillOnce( + Invoke([&](const std::string& host_address, const Common::Redis::RespValue& request, + Common::Redis::Client::PoolCallbacks&) -> Common::Redis::Client::PoolRequest* { + // Verify that the request has been properly prepended with an "asking" command. + std::vector commands = {"asking"}; + EXPECT_EQ(host_address, "10.1.2.3:4000"); + EXPECT_TRUE(request.type() == Common::Redis::RespType::Array); + EXPECT_EQ(request.asArray().size(), commands.size()); + for (unsigned int i = 0; i < commands.size(); i++) { + EXPECT_TRUE(request.asArray()[i].type() == Common::Redis::RespType::BulkString); + EXPECT_EQ(request.asArray()[i].asString(), commands[i]); + } + return nullptr; + })); + EXPECT_FALSE(pool_callbacks_->onRedirection(ask_response)); + + // Test an upstream error from trying to send the original request after the "asking" command is + // sent successfully. + Common::Redis::Client::MockPoolRequest pool_request; + EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, _)) + .WillOnce( + Invoke([&](const std::string& host_address, const Common::Redis::RespValue& request, + Common::Redis::Client::PoolCallbacks&) -> Common::Redis::Client::PoolRequest* { + // Verify that the request has been properly prepended with an "asking" command. + std::vector commands = {"asking"}; + EXPECT_EQ(host_address, "10.1.2.3:4000"); + EXPECT_TRUE(request.type() == Common::Redis::RespType::Array); + EXPECT_EQ(request.asArray().size(), commands.size()); + for (unsigned int i = 0; i < commands.size(); i++) { + EXPECT_TRUE(request.asArray()[i].type() == Common::Redis::RespType::BulkString); + EXPECT_EQ(request.asArray()[i].asString(), commands[i]); + } + return &pool_request; + })); + EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, Ref(*pool_callbacks_))) + .WillOnce( + Invoke([&](const std::string& host_address, const Common::Redis::RespValue& request, + Common::Redis::Client::PoolCallbacks&) -> Common::Redis::Client::PoolRequest* { + std::vector commands = {"get", "foo"}; + EXPECT_EQ(host_address, "10.1.2.3:4000"); + EXPECT_TRUE(request.type() == Common::Redis::RespType::Array); + EXPECT_EQ(request.asArray().size(), commands.size()); + for (unsigned int i = 0; i < commands.size(); i++) { + EXPECT_TRUE(request.asArray()[i].type() == Common::Redis::RespType::BulkString); + EXPECT_EQ(request.asArray()[i].asString(), commands[i]); + } + return nullptr; + })); + EXPECT_FALSE(pool_callbacks_->onRedirection(ask_response)); + respond(); }; 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 f942fc302875b..6e4a63fc764b9 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 @@ -139,10 +139,13 @@ class RedisProxyWithRedirectionIntegrationTest : public RedisProxyIntegrationTes * @param target_server a handle to the second server that will respond to the request. * @param request supplies client data to transmit to the first upstream server. * @param redirection_response supplies the moved or ask redirection error from the first server. + * @param asking_response supplies the target_server's response to an "asking" command, if it is + * received. * @param response supplies data sent by the second server back to the fake Redis client. */ void simpleRedirection(FakeUpstreamPtr& target_server, const std::string& request, - const std::string& redirection_response, const std::string& response); + const std::string& redirection_response, const std::string& response, + const std::string& asking_response = "+OK\r\n"); }; INSTANTIATE_TEST_SUITE_P(IpVersions, RedisProxyIntegrationTest, @@ -199,7 +202,8 @@ void RedisProxyIntegrationTest::simpleProxyResponse(const std::string& request, void RedisProxyWithRedirectionIntegrationTest::simpleRedirection( FakeUpstreamPtr& target_server, const std::string& request, - const std::string& redirection_response, const std::string& response) { + const std::string& redirection_response, const std::string& response, + const std::string& asking_response) { bool asking = (redirection_response.find("-ASK") != std::string::npos); std::string proxy_to_server; @@ -227,8 +231,8 @@ void RedisProxyWithRedirectionIntegrationTest::simpleRedirection( EXPECT_TRUE(fake_upstream_connection_2->waitForData(asking_request.size() + request.size(), &proxy_to_server)); EXPECT_EQ(asking_request + request, proxy_to_server); - // Respond with an "OK" to the "asking" command. - EXPECT_TRUE(fake_upstream_connection_2->write("+OK\r\n")); + // Respond to the "asking" command. + EXPECT_TRUE(fake_upstream_connection_2->write(asking_response)); } else { // The server, target_server, should receive request unchanged. EXPECT_TRUE(fake_upstream_connection_2->waitForData(request.size(), &proxy_to_server)); @@ -378,5 +382,69 @@ TEST_P(RedisProxyWithRedirectionIntegrationTest, BadRedirectStrings) { } } +// This test verifies that an upstream connection failure during ask redirection processing is +// handled correctly. In this case the "asking" command and original client request have been sent +// to the target server, and then the connection is closed. The fake Redis client should receive an +// upstream failure error in response to its request. + +TEST_P(RedisProxyWithRedirectionIntegrationTest, ConnectionFailureBeforeAskingResponse) { + initialize(); + + std::string request = makeBulkStringArray({"get", "foo"}); + std::stringstream redirection_error; + redirection_error << "-ASK 1111 " << redisAddressAndPort(fake_upstreams_[1]) << "\r\n"; + + std::string proxy_to_server; + IntegrationTcpClientPtr redis_client = makeTcpConnection(lookupPort("redis_proxy")); + redis_client->write(request); + + FakeRawConnectionPtr fake_upstream_connection_1, fake_upstream_connection_2; + + // Data from the client should always be routed to fake_upstreams_[0] by the load balancer. + EXPECT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection_1)); + EXPECT_TRUE(fake_upstream_connection_1->waitForData(request.size(), &proxy_to_server)); + // The data in request should be received by the first server, fake_upstreams_[0]. + EXPECT_EQ(request, proxy_to_server); + proxy_to_server.clear(); + + // Send the redirection_response from the first fake Redis server back to the proxy. + EXPECT_TRUE(fake_upstream_connection_1->write(redirection_error.str())); + // The proxy should initiate a new connection to the fake redis server, target_server, in + // response. + EXPECT_TRUE(fake_upstreams_[1]->waitForRawConnection(fake_upstream_connection_2)); + + // The server, fake_upstreams_[1], should receive an "asking" command before the original request. + std::string asking_request = makeBulkStringArray({"asking"}); + EXPECT_TRUE(fake_upstream_connection_2->waitForData(asking_request.size() + request.size(), + &proxy_to_server)); + EXPECT_EQ(asking_request + request, proxy_to_server); + // Close the upstream connection before responding to the "asking" command. + EXPECT_TRUE(fake_upstream_connection_2->close()); + + // The fake Redis client should receive an upstream failure error from the proxy. + std::stringstream error_response; + error_response << "-" << RedisCmdSplitter::Response::get().UpstreamFailure << "\r\n"; + redis_client->waitForData(error_response.str()); + EXPECT_EQ(error_response.str(), redis_client->data()); + + redis_client->close(); + EXPECT_TRUE(fake_upstream_connection_1->close()); +} + +// This test verifies that a ASK redirection error as a response to an "asking" command is ignored. +// This is a negative test scenario that should never happen since a Redis server will reply to an +// "asking" command with either a "cluster support not enabled" error or "OK". + +TEST_P(RedisProxyWithRedirectionIntegrationTest, IgnoreRedirectionForAsking) { + initialize(); + std::string request = makeBulkStringArray({"get", "foo"}); + std::stringstream redirection_error, asking_response; + redirection_error << "-ASK 1111 " << redisAddressAndPort(fake_upstreams_[1]) << "\r\n"; + asking_response << "-ASK 1111 " << redisAddressAndPort(fake_upstreams_[0]) << "\r\n"; + simpleRedirection(fake_upstreams_[1], request, redirection_error.str(), "$3\r\nbar\r\n", + asking_response.str()); +} + } // namespace + } // namespace Envoy From ad4276fac1c73c8a4ebf6d7e2018609771b002b6 Mon Sep 17 00:00:00 2001 From: Mitch Sukalski Date: Wed, 10 Apr 2019 20:04:46 -0700 Subject: [PATCH 3/5] redis: fix asking output for ask redirection support (#6543) - extended tests for coverage gap Signed-off-by: Mitch Sukalski --- .../redis_proxy/command_splitter_impl_test.cc | 57 +++++++++++++++++++ .../redis_proxy_integration_test.cc | 4 +- 2 files changed, 59 insertions(+), 2 deletions(-) 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 10fa4eb04a6b7..ae7a9839147b6 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 @@ -753,6 +753,28 @@ TEST_F(RedisMGETCommandHandlerTest, NormalWithMovedRedirection) { Common::Redis::RespValue moved_response; moved_response.type(Common::Redis::RespType::Error); moved_response.asString() = "MOVED 1234 192.168.0.1:5000"; // Exact values are not important. + + // Test with simulated upstream failures. This exercises code in + // FragmentedRequest::onChildRedirection() common to MGET, MSET, and SplitKeysSumResult commands. + for (unsigned int i = 0; i < 2; i++) { + EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, Ref(*pool_callbacks_[i]))) + .WillOnce(Invoke( + [&](const std::string& host_address, const Common::Redis::RespValue& request, + Common::Redis::Client::PoolCallbacks&) -> Common::Redis::Client::PoolRequest* { + EXPECT_EQ(host_address, "192.168.0.1:5000"); + EXPECT_TRUE(request.type() == Common::Redis::RespType::Array); + EXPECT_EQ(request.asArray().size(), 2); + EXPECT_TRUE(request.asArray()[0].type() == Common::Redis::RespType::BulkString); + EXPECT_EQ(request.asArray()[0].asString(), "get"); + EXPECT_TRUE(request.asArray()[1].type() == Common::Redis::RespType::BulkString); + EXPECT_EQ(request.asArray()[1].asString(), std::to_string(i)); + EXPECT_NE(&pool_requests_[i], nullptr); + return nullptr; + })); + EXPECT_FALSE(pool_callbacks_[i]->onRedirection(moved_response)); + } + + // Test "successful" redirection. for (unsigned int i = 0; i < 2; i++) { EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, Ref(*pool_callbacks_[i]))) .WillOnce(Invoke( @@ -815,6 +837,41 @@ TEST_F(RedisMGETCommandHandlerTest, NormalWithAskRedirection) { ask_response.type(Common::Redis::RespType::Error); ask_response.asString() = "ASK 1234 192.168.0.1:5000"; // Exact values are not important. Common::Redis::Client::MockPoolRequest dummy_poolrequest; + + // Test redirection with simulated upstream failures. This exercises code in + // FragmentedRequest::onChildRedirection() common to MGET, MSET, and SplitKeysSumResult commands. + for (unsigned int i = 0; i < 2; i++) { + EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, _)) + .WillOnce(Invoke( + [&](const std::string& host_address, const Common::Redis::RespValue& request, + Common::Redis::Client::PoolCallbacks&) -> Common::Redis::Client::PoolRequest* { + EXPECT_EQ(host_address, "192.168.0.1:5000"); + EXPECT_TRUE(request.type() == Common::Redis::RespType::Array); + EXPECT_EQ(request.asArray().size(), 1); + EXPECT_TRUE(request.asArray()[0].type() == Common::Redis::RespType::BulkString); + EXPECT_EQ(request.asArray()[0].asString(), "asking"); + return (i == 0 ? nullptr : &dummy_poolrequest); + })); + if (i == 1) { + EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, Ref(*pool_callbacks_[i]))) + .WillOnce(Invoke( + [&](const std::string& host_address, const Common::Redis::RespValue& request, + Common::Redis::Client::PoolCallbacks&) -> Common::Redis::Client::PoolRequest* { + EXPECT_EQ(host_address, "192.168.0.1:5000"); + EXPECT_TRUE(request.type() == Common::Redis::RespType::Array); + EXPECT_EQ(request.asArray().size(), 2); + EXPECT_TRUE(request.asArray()[0].type() == Common::Redis::RespType::BulkString); + EXPECT_EQ(request.asArray()[0].asString(), "get"); + EXPECT_TRUE(request.asArray()[1].type() == Common::Redis::RespType::BulkString); + EXPECT_EQ(request.asArray()[1].asString(), std::to_string(i)); + EXPECT_NE(&pool_requests_[i], nullptr); + return (i == 1 ? nullptr : &pool_requests_[i]); + })); + } + EXPECT_FALSE(pool_callbacks_[i]->onRedirection(ask_response)); + } + + // Test "successful" redirection. for (unsigned int i = 0; i < 2; i++) { EXPECT_CALL(*conn_pool_, makeRequestToHost(_, _, _)) .WillOnce(Invoke( 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 6e4a63fc764b9..bde21a4655e71 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 @@ -139,9 +139,9 @@ class RedisProxyWithRedirectionIntegrationTest : public RedisProxyIntegrationTes * @param target_server a handle to the second server that will respond to the request. * @param request supplies client data to transmit to the first upstream server. * @param redirection_response supplies the moved or ask redirection error from the first server. - * @param asking_response supplies the target_server's response to an "asking" command, if it is - * received. * @param response supplies data sent by the second server back to the fake Redis client. + * @param asking_response supplies the target_server's response to an "asking" command, if + * appropriate. */ void simpleRedirection(FakeUpstreamPtr& target_server, const std::string& request, const std::string& redirection_response, const std::string& response, From 9a659db089839014c7d74c07e8b302f52edcec80 Mon Sep 17 00:00:00 2001 From: Mitch Sukalski Date: Fri, 12 Apr 2019 14:35:19 -0700 Subject: [PATCH 4/5] redis: fix asking output for ask redirection support (#6543) Signed-off-by: Mitch Sukalski --- .../network/redis_proxy/command_splitter_impl.cc | 13 ++++++++++--- .../redis_proxy/redis_proxy_integration_test.cc | 1 - 2 files changed, 10 insertions(+), 4 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 b56ce13d47b82..16efd0fb2ee5d 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc @@ -111,8 +111,15 @@ bool SingleServerRequest::onRedirection(const Common::Redis::RespValue& value) { return false; } - const std::string host_address = std::string(err[2]); // ip:port - // Prepend request with asking command if redirected via ASK error. + // MOVED and ASK redirection errors have the following substrings: MOVED or ASK (err[0]), hash key + // slot (err[1]), and IP address and TCP port separated by a colon (err[2]). + const std::string host_address = std::string(err[2]); + + // Prepend request with an asking command if redirected via an ASK error. The returned handle is + // not important since there is no point in being able to cancel the request. The use of + // nullPoolCallbacks ensures the transparent filtering of the Redis server's response to the + // "asking" command; this is fine since the server either responds with an OK or an error message + // if cluster support is not enabled (in which case we should not get an ASK redirection error). if (ask_redirection && !conn_pool_->makeRequestToHost(host_address, askingRequest(), nullPoolCallbacks)) { return false; @@ -257,7 +264,7 @@ bool FragmentedRequest::onChildRedirection(const Common::Redis::RespValue& value // not important since there is no point in being able to cancel the request. The use of // nullPoolCallbacks ensures the transparent filtering of the Redis server's response to the // "asking" command; this is fine since the server either responds with an OK or an error message - // if cluster support is not enabled (in which case, we should not get an ASK redirection error). + // if cluster support is not enabled (in which case we should not get an ASK redirection error). if (ask_redirection && !conn_pool->makeRequestToHost(host_address, askingRequest(), nullPoolCallbacks)) { return false; 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 bde21a4655e71..d23c2b2649514 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 @@ -446,5 +446,4 @@ TEST_P(RedisProxyWithRedirectionIntegrationTest, IgnoreRedirectionForAsking) { } } // namespace - } // namespace Envoy From ae201f2b2ea3d64ef015ba50e2071b976013a48f Mon Sep 17 00:00:00 2001 From: Mitch Sukalski Date: Fri, 12 Apr 2019 15:03:19 -0700 Subject: [PATCH 5/5] redis: fix asking output for ask redirection support (#6543) Signed-off-by: Mitch Sukalski --- .../redis_proxy/command_splitter_impl.cc | 31 ++++++++++++------- .../redis_proxy/command_splitter_impl.h | 6 ++-- 2 files changed, 22 insertions(+), 15 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 16efd0fb2ee5d..9fc6189a393e7 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc @@ -27,16 +27,23 @@ Common::Redis::RespValuePtr Utility::makeError(const std::string& error) { namespace { -// nullPoolCallbacks is used for requests that must be filtered and not redirected such as "asking". -DoNothingPoolCallbacks nullPoolCallbacks; +// null_pool_callbacks is used for requests that must be filtered and not redirected such as +// "asking". +DoNothingPoolCallbacks null_pool_callbacks; // Create an asking command request. -Common::Redis::RespValue askingRequest() { - Common::Redis::RespValue asking_cmd, request; - asking_cmd.type(Common::Redis::RespType::BulkString); - asking_cmd.asString() = "asking"; - request.type(Common::Redis::RespType::Array); - request.asArray().push_back(asking_cmd); +const Common::Redis::RespValue& askingRequest() { + static Common::Redis::RespValue request; + static bool initialized = false; + + if (!initialized) { + Common::Redis::RespValue asking_cmd; + asking_cmd.type(Common::Redis::RespType::BulkString); + asking_cmd.asString() = "asking"; + request.type(Common::Redis::RespType::Array); + request.asArray().push_back(asking_cmd); + initialized = true; + } return request; } @@ -117,11 +124,11 @@ bool SingleServerRequest::onRedirection(const Common::Redis::RespValue& value) { // Prepend request with an asking command if redirected via an ASK error. The returned handle is // not important since there is no point in being able to cancel the request. The use of - // nullPoolCallbacks ensures the transparent filtering of the Redis server's response to the + // null_pool_callbacks ensures the transparent filtering of the Redis server's response to the // "asking" command; this is fine since the server either responds with an OK or an error message // if cluster support is not enabled (in which case we should not get an ASK redirection error). if (ask_redirection && - !conn_pool_->makeRequestToHost(host_address, askingRequest(), nullPoolCallbacks)) { + !conn_pool_->makeRequestToHost(host_address, askingRequest(), null_pool_callbacks)) { return false; } handle_ = conn_pool_->makeRequestToHost(host_address, *incoming_request_, *this); @@ -262,11 +269,11 @@ bool FragmentedRequest::onChildRedirection(const Common::Redis::RespValue& value // Prepend request with an asking command if redirected via an ASK error. The returned handle is // not important since there is no point in being able to cancel the request. The use of - // nullPoolCallbacks ensures the transparent filtering of the Redis server's response to the + // null_pool_callbacks ensures the transparent filtering of the Redis server's response to the // "asking" command; this is fine since the server either responds with an OK or an error message // if cluster support is not enabled (in which case we should not get an ASK redirection error). if (ask_redirection && - !conn_pool->makeRequestToHost(host_address, askingRequest(), nullPoolCallbacks)) { + !conn_pool->makeRequestToHost(host_address, askingRequest(), null_pool_callbacks)) { return false; } 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 857451a55a64a..21eb847c73cf0 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.h +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.h @@ -343,9 +343,9 @@ class InstanceImpl : public Instance, Logger::Loggable { class DoNothingPoolCallbacks : public Common::Redis::Client::PoolCallbacks { public: // Common::Redis::Client::PoolCallbacks - void onResponse(Common::Redis::RespValuePtr&&) override{}; - void onFailure() override{}; - bool onRedirection(const Common::Redis::RespValue&) override { return false; }; + void onResponse(Common::Redis::RespValuePtr&&) override {} + void onFailure() override {} + bool onRedirection(const Common::Redis::RespValue&) override { return false; } }; } // namespace CommandSplitter