Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure Redis Proxy can follow redirection when doing request mirroring #8606

Merged
merged 34 commits into from
Dec 6, 2019
Merged
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
293d852
Refactor the redirection logic into the RedisConnPool.
HenryYYang Oct 14, 2019
36ebc52
Fix format
HenryYYang Oct 14, 2019
882e42b
Fix tests
HenryYYang Oct 14, 2019
031ff92
Merge branch 'master' into redirect_mirror
HenryYYang Oct 14, 2019
d151ae2
Fix move constructor
HenryYYang Oct 15, 2019
7536789
Fix clang errors
HenryYYang Oct 15, 2019
a9cfd91
Fix clang errors
HenryYYang Oct 17, 2019
5793b6b
Format code
HenryYYang Oct 17, 2019
fac3ffb
Use absl::variant instead of shared_ptr to improve performance.
HenryYYang Oct 17, 2019
b940d04
Fix format
HenryYYang Oct 17, 2019
e63f38e
Fix clang
HenryYYang Oct 17, 2019
dd78fb0
Fix clang
HenryYYang Oct 18, 2019
a1b0741
Kick CI
HenryYYang Oct 18, 2019
6c02aeb
Merge branch 'master' into redirect_mirror
HenryYYang Oct 19, 2019
cf4c3f6
fix merge errors
HenryYYang Oct 19, 2019
deab4b6
Fix format
HenryYYang Oct 20, 2019
7a80d45
Fix tests
HenryYYang Oct 23, 2019
35142f2
Merge branch 'master' into redirect_mirror
HenryYYang Nov 6, 2019
aa89024
Fix merge errors
HenryYYang Nov 6, 2019
45e3cb1
Merge branch 'master' into redirect_mirror
HenryYYang Nov 8, 2019
f8f7041
Add speed test and make respvalue const
HenryYYang Nov 12, 2019
36d7190
fix format
HenryYYang Nov 12, 2019
2e293a5
Merge branch 'master' into redirect_mirror
HenryYYang Nov 12, 2019
ac9ba4e
update command splitter test
HenryYYang Nov 13, 2019
47fd4d9
Fix failing test
HenryYYang Nov 13, 2019
b6f57cc
Add failure test for ConnPoolImpl
HenryYYang Nov 14, 2019
089f74b
Merge branch 'master' into redirect_mirror
HenryYYang Nov 14, 2019
98ab0bc
sort version_history file
HenryYYang Nov 14, 2019
93311e5
Change the onDirection method signature.
HenryYYang Nov 27, 2019
2e301c3
Merge branch 'master' into redirect_mirror
HenryYYang Nov 27, 2019
4896554
fix unit test
HenryYYang Nov 27, 2019
d8384cb
fix PR feedback
HenryYYang Nov 27, 2019
2df5066
fix PR feedback
HenryYYang Dec 2, 2019
eb2e2e3
fix PR feedback
HenryYYang Dec 6, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/root/intro/version_history.rst
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Version history
* lb_subset_config: new fallback policy for selectors: :ref:`KEYS_SUBSET<envoy_api_enum_value_Cluster.LbSubsetConfig.LbSubsetSelector.LbSubsetSelectorFallbackPolicy.KEYS_SUBSET>`
* logger: added :ref:`--log-format-escaped <operations_cli>` command line option to escape newline characters in application logs.
* redis: performance improvement for larger split commands by avoiding string copies.
* redis: correctly follow MOVE/ASK redirection for mirrored clusters.
* router: added support for REQ(header-name) :ref:`header formatter <config_http_conn_man_headers_custom_request_headers>`.
* router: skip the Location header when the response code is not a 201 or a 3xx.
* server: fixed a bug in config validation for configs with runtime layers
Expand Down
9 changes: 6 additions & 3 deletions source/extensions/clusters/redis/redis_cluster.h
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ class RedisCluster : public Upstream::BaseDynamicClusterImpl {

struct RedisDiscoverySession
: public Extensions::NetworkFilters::Common::Redis::Client::Config,
public Extensions::NetworkFilters::Common::Redis::Client::PoolCallbacks {
public Extensions::NetworkFilters::Common::Redis::Client::ClientCallbacks {
RedisDiscoverySession(RedisCluster& parent,
NetworkFilters::Common::Redis::Client::ClientFactory& client_factory);

Expand Down Expand Up @@ -225,11 +225,14 @@ class RedisCluster : public Upstream::BaseDynamicClusterImpl {
return Extensions::NetworkFilters::Common::Redis::Client::ReadPolicy::Master;
}

// Extensions::NetworkFilters::Common::Redis::Client::PoolCallbacks
// Extensions::NetworkFilters::Common::Redis::Client::ClientCallbacks
void onResponse(NetworkFilters::Common::Redis::RespValuePtr&& value) override;
void onFailure() override;
// Note: Below callback isn't used in topology updates
bool onRedirection(const NetworkFilters::Common::Redis::RespValue&) override { return true; }
bool onRedirection(NetworkFilters::Common::Redis::RespValuePtr&&, const std::string&,
bool) override {
return true;
}
void onUnexpectedResponse(const NetworkFilters::Common::Redis::RespValuePtr&);

Network::Address::InstanceConstSharedPtr
Expand Down
19 changes: 12 additions & 7 deletions source/extensions/filters/network/common/redis/client.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ class PoolRequest {
/**
* Outbound request callbacks.
*/
class PoolCallbacks {
class ClientCallbacks {
public:
virtual ~PoolCallbacks() = default;
virtual ~ClientCallbacks() = default;

/**
* Called when a pipelined response is received.
Expand All @@ -48,21 +48,26 @@ class PoolCallbacks {
/**
* Called when a MOVED or ASK redirection error is received, and the request must be retried.
* @param value supplies the MOVED error response
* @param host_address supplies the redirection host address and port
* @param ask_redirection indicates if this is a ASK redirection
* @return bool true if the request is successfully redirected, false otherwise
*/
virtual bool onRedirection(const Common::Redis::RespValue& value) PURE;
virtual bool onRedirection(RespValuePtr&& value, const std::string& host_address,
bool ask_redirection) PURE;
};

/**
* DoNothingPoolCallbacks is used for internally generated commands whose response is
* transparently filtered, and redirection never occurs (e.g., "asking", "auth", etc.).
*/
class DoNothingPoolCallbacks : public PoolCallbacks {
class DoNothingPoolCallbacks : public ClientCallbacks {
public:
// PoolCallbacks
// ClientCallbacks
void onResponse(Common::Redis::RespValuePtr&&) override {}
void onFailure() override {}
bool onRedirection(const Common::Redis::RespValue&) override { return false; }
bool onRedirection(Common::Redis::RespValuePtr&&, const std::string&, bool) override {
return false;
}
};

/**
Expand Down Expand Up @@ -95,7 +100,7 @@ class Client : public Event::DeferredDeletable {
* @return PoolRequest* a handle to the active request or nullptr if the request could not be made
* for some reason.
*/
virtual PoolRequest* makeRequest(const RespValue& request, PoolCallbacks& callbacks) PURE;
virtual PoolRequest* makeRequest(const RespValue& request, ClientCallbacks& callbacks) PURE;

/**
* Initialize the connection. Issue the auth command and readonly command as needed.
Expand Down
16 changes: 11 additions & 5 deletions source/extensions/filters/network/common/redis/client_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ namespace Common {
namespace Redis {
namespace Client {
namespace {
// null_pool_callbacks is used for requests that must be filtered and not redirected such as
// "asking".
Common::Redis::Client::DoNothingPoolCallbacks null_pool_callbacks;
} // namespace

Expand Down Expand Up @@ -97,7 +99,7 @@ void ClientImpl::flushBufferAndResetTimer() {
connection_->write(encoder_buffer_, false);
}

PoolRequest* ClientImpl::makeRequest(const RespValue& request, PoolCallbacks& callbacks) {
PoolRequest* ClientImpl::makeRequest(const RespValue& request, ClientCallbacks& callbacks) {
ASSERT(connection_->state() == Network::Connection::State::Open);

const bool empty_buffer = encoder_buffer_.length() == 0;
Expand Down Expand Up @@ -212,7 +214,7 @@ void ClientImpl::onRespValue(RespValuePtr&& value) {
}
request.aggregate_request_timer_->complete();

PoolCallbacks& callbacks = request.callbacks_;
ClientCallbacks& callbacks = request.callbacks_;

// We need to ensure the request is popped before calling the callback, since the callback might
// result in closing the connection.
Expand All @@ -223,9 +225,13 @@ void ClientImpl::onRespValue(RespValuePtr&& value) {
std::vector<absl::string_view> err = StringUtil::splitToken(value->asString(), " ", false);
bool redirected = false;
if (err.size() == 3) {
// 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])
if (err[0] == RedirectionResponse::get().MOVED || err[0] == RedirectionResponse::get().ASK) {
redirected = callbacks.onRedirection(*value);
if (redirected) {
redirected = true;
bool redirect_succeeded = callbacks.onRedirection(std::move(value), std::string(err[2]),
err[0] == RedirectionResponse::get().ASK);
if (redirect_succeeded) {
host_->cluster().stats().upstream_internal_redirect_succeeded_total_.inc();
} else {
host_->cluster().stats().upstream_internal_redirect_failed_total_.inc();
Expand All @@ -251,7 +257,7 @@ void ClientImpl::onRespValue(RespValuePtr&& value) {
putOutlierEvent(Upstream::Outlier::Result::ExtOriginRequestSuccess);
}

ClientImpl::PendingRequest::PendingRequest(ClientImpl& parent, PoolCallbacks& callbacks,
ClientImpl::PendingRequest::PendingRequest(ClientImpl& parent, ClientCallbacks& callbacks,
Stats::StatName command)
: parent_(parent), callbacks_(callbacks), command_{command},
aggregate_request_timer_(parent_.redis_command_stats_->createAggregateTimer(
Expand Down
6 changes: 3 additions & 3 deletions source/extensions/filters/network/common/redis/client_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ class ClientImpl : public Client, public DecoderCallbacks, public Network::Conne
connection_->addConnectionCallbacks(callbacks);
}
void close() override;
PoolRequest* makeRequest(const RespValue& request, PoolCallbacks& callbacks) override;
PoolRequest* makeRequest(const RespValue& request, ClientCallbacks& callbacks) override;
bool active() override { return !pending_requests_.empty(); }
void flushBufferAndResetTimer();
void initialize(const std::string& auth_password) override;
Expand All @@ -104,14 +104,14 @@ class ClientImpl : public Client, public DecoderCallbacks, public Network::Conne
};

struct PendingRequest : public PoolRequest {
PendingRequest(ClientImpl& parent, PoolCallbacks& callbacks, Stats::StatName stat_name);
PendingRequest(ClientImpl& parent, ClientCallbacks& callbacks, Stats::StatName stat_name);
~PendingRequest() override;

// PoolRequest
void cancel() override;

ClientImpl& parent_;
PoolCallbacks& callbacks_;
ClientCallbacks& callbacks_;
Stats::StatName command_;
bool canceled_{};
Stats::TimespanPtr aggregate_request_timer_;
Expand Down
7 changes: 7 additions & 0 deletions source/extensions/filters/network/common/redis/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ AuthRequest::AuthRequest(const std::string& password) {
asArray().swap(values);
}

RespValuePtr makeError(const std::string& error) {
Common::Redis::RespValuePtr response(new RespValue());
response->type(Common::Redis::RespType::Error);
response->asString() = error;
return response;
}

ReadOnlyRequest::ReadOnlyRequest() {
std::vector<RespValue> values(1);
values[0].type(RespType::BulkString);
Expand Down
2 changes: 2 additions & 0 deletions source/extensions/filters/network/common/redis/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ class AuthRequest : public Redis::RespValue {
AuthRequest(const std::string& password);
};

RespValuePtr makeError(const std::string& error);

class ReadOnlyRequest : public Redis::RespValue {
public:
ReadOnlyRequest();
Expand Down
Loading