redis: add moved/ask redirection support

[msukalski]

• Redis requests are now redirected to a new upstream connection
  upon receipt of an MOVED or ASK error response. If the request
  cannot be redirected, then the error is passed downstream unmodified.
  The redirection IP (IPv4 or IPv6) address and TCP port specified
  in the Redis server error does not need to reference a known host of
  the cluster associated with the redis_proxy filter.

• added an enable_redirection boolean to the redis proxy connection
  pool settings to control whether or not server redirection errors are
  honored.

• RespValue copy constructor and copy assignment methods for
  easier manipulation of RespValues

• added cluster statistics, upstream_internal_redirect_succeeded_total
  and upstream_internal_redirect_failed_total in ClientImpl::onRespValue()
  callback

• extended unit tests for Redis connection pool, client, and command
  splitter

Signed-off-by: Mitch Sukalski mitch.sukalski@workday.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description:

Redis requests are now redirected to a new upstream connection
upon receipt of an MOVED or ASK error response. If the request
cannot be redirected, then the error is passed downstream unmodified.
The redirection IP (IPv4 or IPv6) address and TCP port specified
in the Redis server error does not need to reference a known host of
the cluster associated with the redis_proxy filter.

Risk Level: Medium

Testing:

unit testing at the redis command splitter, connection pool, and upstream client
levels, as well as integration testing for moved redirection with a 3 server Redis
cluster (tested with 2 static cluster Envoy configurations listing all 3 servers, and
listing only one).

Docs Changes:

As part of the architecture overview for the Redis proxy (intro/arch_overview/redis.rst),
these changes add "moved/ask redirection support" to the list of "Features of Envoy Redis".
Also 2 cluster statistics are updated to reflect the number of successful and failed redirected commands: upstream_internal_redirect_succeeded_total_, and upstream_internal_redirect_failed_total_, respectively.

Also there is a new redis proxy connection pool setting, enable_redirection (boolean). By
default it is false, and redirection errors from upstream redis servers are passed unchanged
downstream. If it is set to true, then moved/ask redirection support is enabled, and requests
are retried appropriately.

Release Notes:

redis: moved/ask redirection supported

[Optional Fixes #Issue]
[Optional Deprecated:]

[msukalski]

@HenryYYang, @FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg , and @mattklein123 -- here is the PR that we've been discussing. Matt -- this is code for issue #5697 (Support for Redis cluster protocol) phase 1.

[mattklein123]

@HenryYYang for first pass.

[msukalski]

As far as the failing mysql_integration_test in the circleci compile_time_options check is concerned, I'm able to reproduce the failure (timeout) for IPv6, but "bazel test --test_env=ENVOY_IP_TEST_VERSIONS=v4only --test_output=streamed --nocache_test_results //test/extensions/filters/network/mysql_proxy:mysql_integration_test" passes without error. In any case, I don't think that this failure has anything to do with any code modifications in this PR.

[venilnoronha]

The mysql_integration_test flake was fixed yesterday. You might want to merge master with your branch to get the update.

[msukalski]

thanks @venilnoronha ! I've rebased off master and pushed my branch back up again. Hopefully, it will pass all checks this time!

[msukalski]

I've just resolved upstream conflicts from a recent check-in by @rammantripragada, and pushed my branch up again. @HenryYYang and @mattklein123 -- I would appreciate some TLC for this PR to avoid more conflict resolution busy-work. Thanks in advance!

[HenryYYang]

Very excited about this work. I have some questions.

[msukalski]

I've just pushed new changes that get rid of all of the duplicate makeRequest, startRequest, and create methods at the command splitter implementation level. I've also implemented some of the feedback that you have given. Thanks @HenryYYang and @FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg !

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

Given that we use the following 8 times (but the piece above subtly changes), maybe just this should be pulled out?

  std::vector<absl::string_view> err = StringUtil::splitToken(value.asString(), " ", false);
  if (err.size() != 3) {
    return false;
  }

Other than that, I'll defer to Henry, as I have the least cpp knowledge of us three!

[HenryYYang]

looks good with some minor nits.

[HenryYYang]

+1

[mattklein123]

Thanks for this. Cool stuff. I have a bunch of small comments to get started with, and 2 high level comments:

1. I think we want this behavior to be configurable, at least for the current redis proxy. As such, I think you need to plumb through some enable boolean here? I could see users not wanting to support this behavior and maybe it should be off by default for the existing proxy.
2. I didn't look yet, but please make sure you have 100% code coverage for all the added code. You can look at coverage in CI.

Thanks!

/wait

[msukalski]

I'm implementing a new connection pool setting for the redis proxy, enable_redirection. It's false by default.

I've checked the CircleCI coverage report, and see that I'll need to beef up some negative testing to get the line coverage up to 100%. I'm working on that right now.

[mattklein123]

Thanks, a couple of more high level questions/comment.

/wait

[msukalski]

@mattklein123 I'm going to add the link to the redis docs as per your comment above. I'm also going to implement some changes in the command splitter requests' implementation that will make it easier to incorporate Maxime B.'s prefixed routing changes if this code goes in first. I'll also add at least a basic redis_proxy integration test that I've been working on -- so all of these changes will need to be reviewed. I think this will make everything a bit easier moving forward from here, however, despite the small churn.

I'll let you and @HenryYYang and @FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg know when I've got everything up again.

[msukalski]

ci/circleci:asan failed due to lack of disk space....

[mattklein123]

@msukalski friendly request to not force push your PR, it makes it hard to review. Also, this PR is now too long for a single PR, so if you are going to do an integration test, let's split that out into a separate PR and merge that first. Also, still need to merge the redirection methods into a single method. Thank you.

[msukalski]

@mattklein123, @HenryYYang, and @FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg sorry about the force push. I've pushed up a new version without the integration test (which is is now PR #6450), and refactored the moved and ask redirection callbacks into a single onRedirection() one.

[mattklein123]

@msukalski OK will take a look. Please don't force push this PR again.

[msukalski]

just pushed another unit test to get 100% coverage on modified code

[mattklein123]

Thanks this is really excellent work. Most nits and comment requests at this point. Let's get the other integration test PR merged and then I would also like to see an integration test in this PR specific to this change.

/wait

[msukalski]

I'll address the nits in the latest review and augment the new redis_proxy integration test to show redirection working when upstream hosts are all defined, and when they are not. I've had to fix the ConfigHelper constructor to properly work with multiple fake upstreams for IPv4 and IPv6 (as load_assignment endpoints or the older hosts configuration), and figure out how to steer traffic to the right fake upstream (setDeterministic() and then mock the random() calls to the MockRandomGenerator to return a value to choose the expected fake upstream).

[msukalski]

@mattklein123 I will need to rebase this branch (and thus force push later) to pick up the new redis_proxy integration test to build on. I know this will screw up the review, but I'll make sure I implement all of the review feedback to date.

[mattklein123]

@msukalski you do not ever need to rebase. Just merge origin/master into this branch. Please never rebase once you open a PR. It makes my life much more difficult. Thank you.

[msukalski]

ok

[mattklein123]

Thanks, this is really excellent and thorough work. I really appreciate the iteration here and all the tests. Thank you!