redis filter: support CLUSTER SHARDS - #46480
Conversation
Signed-off-by: Aurimas Ragulis <aurimas.ragulis@vinted.com>
|
Hi @s7eamy, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
adisuissa
left a comment
There was a problem hiding this comment.
Assigning codeowners to review:
/assign @mattklein123 @dinesh-murugiah
|
@mattklein123 thanks for the review. is there anything else I need to do for CI checks to pass? it seems they're waiting on |
|
I'm not sure why they are not running. Try pushing an empty commit? |
|
retrying CI didn't help 🤔 can it be that you have to approve the workflow run somewhere here? https://github.com/envoyproxy/envoy/actions/runs/32003945969 |
previous run failed with: ``` WARNING: Download from https://github.com/zlib-ng/zlib-ng/archive/2.3.2.tar.gz failed: java.io.IOException GET returned 429 Too Many Requests ERROR: no such package '@@zlib-ng//' ... ERROR: Analysis of target '@@envoy//tools/code_format:check_format' failed; build aborted INFO: Elapsed time: 177.240s ERROR: Build failed. Not running target ``` Signed-off-by: Aurimas Ragulis <aurimas.ragulis@vinted.com>
|
last run failed with: I pushed an empty commit again to retry workflow before realizing there's |
|
@mattklein123 first time contributors need to have workflows approved its done via the environment api here
|
**Commit Message**: redis filter: support `CLUSTER SHARDS` Adds ``shards`` to the CLUSTER subcommand allowlist so cluster-aware clients can issue CLUSTER SHARDS (Redis 7.0+, the successor to the deprecated CLUSTER SLOTS) through the proxy. Like the other supported CLUSTER introspection subcommands (INFO, SLOTS, KEYSLOT, NODES), the command is forwarded to a single random upstream shard and the reply is returned to the client unmodified. See [here](https://redis.io/docs/latest/commands/cluster-shards/) for `CLUSTER SHARDS` docs. **Additional Description**: This builds on the random-shard command framework introduced in envoyproxy#41381: subcommand validation is data-driven via `commandSubcommandValidationMap()`, and routing/response handling reuse the existing `RandomShardRequest` passthrough (works identically for RESP2 and RESP3 connections since the reply is relayed verbatim). On upstreams older than Redis 7.0, the upstream's `ERR unknown subcommand` error is passed through to the client unchanged. I guess I can also add a small comment on why this is needed - we have a case where we want to use Envoy to fetch Redis cluster state before initializing a client, e.g. ruby `redis-cluster-client`. This library, however, first calls `CLUSTER SHARDS` and only then falls back to `CLUSTER NODES`. See [here](https://github.com/redis-rb/redis-cluster-client/blob/25603bb42d15e3c719a3e505891ce3e425a4d225/lib/redis_client/cluster/node.rb#L357-L365). Having Envoy support `CLUSTER SHARDS` would help us save some network roundtrips and potentially avoid thundering herd problem on our network links during large scale app rollouts. **Related issues, not fixed by this PR**: envoyproxy#38492 tracks broader redis_proxy command coverage; envoyproxy#36975 asks for Envoy's own cluster topology discovery to use CLUSTER SHARDS, which is out of scope here (discovery still uses CLUSTER SLOTS). **Per the generative AI policy**: this change was developed with the assistance of an AI tool (Claude Code). I have reviewed the change, understand it fully, and take ownership of it. **Risk Level**: Low — additive one-line allowlist change enabling passthrough of a single read-only introspection subcommand; no behavior change for any existing command. **Testing**: New unit test `RandomShardRequestTest.ClusterShards`, mirroring the existing `ClusterNodes` test. The full `//test/extensions/filters/network/redis_proxy:command_splitter_impl_test` target passes (249/249 tests), including the existing unsupported-subcommand rejection tests. `ci/do_ci.sh format` passes. **Docs Changes**: Added a ``CLUSTER SHARDS, Generic`` row to the supported commands table in `docs/root/intro/arch_overview/other_protocols/redis.rst`. **Release Notes**: `changelogs/current/new_features/redis_proxy__cluster-shards-command.rst` **Platform Specific Features**: N/A Signed-off-by: Aurimas Ragulis <aurimas.ragulis@vinted.com>
**Commit Message**: redis filter: support `CLUSTER SHARDS` Adds ``shards`` to the CLUSTER subcommand allowlist so cluster-aware clients can issue CLUSTER SHARDS (Redis 7.0+, the successor to the deprecated CLUSTER SLOTS) through the proxy. Like the other supported CLUSTER introspection subcommands (INFO, SLOTS, KEYSLOT, NODES), the command is forwarded to a single random upstream shard and the reply is returned to the client unmodified. See [here](https://redis.io/docs/latest/commands/cluster-shards/) for `CLUSTER SHARDS` docs. **Additional Description**: This builds on the random-shard command framework introduced in envoyproxy#41381: subcommand validation is data-driven via `commandSubcommandValidationMap()`, and routing/response handling reuse the existing `RandomShardRequest` passthrough (works identically for RESP2 and RESP3 connections since the reply is relayed verbatim). On upstreams older than Redis 7.0, the upstream's `ERR unknown subcommand` error is passed through to the client unchanged. I guess I can also add a small comment on why this is needed - we have a case where we want to use Envoy to fetch Redis cluster state before initializing a client, e.g. ruby `redis-cluster-client`. This library, however, first calls `CLUSTER SHARDS` and only then falls back to `CLUSTER NODES`. See [here](https://github.com/redis-rb/redis-cluster-client/blob/25603bb42d15e3c719a3e505891ce3e425a4d225/lib/redis_client/cluster/node.rb#L357-L365). Having Envoy support `CLUSTER SHARDS` would help us save some network roundtrips and potentially avoid thundering herd problem on our network links during large scale app rollouts. **Related issues, not fixed by this PR**: envoyproxy#38492 tracks broader redis_proxy command coverage; envoyproxy#36975 asks for Envoy's own cluster topology discovery to use CLUSTER SHARDS, which is out of scope here (discovery still uses CLUSTER SLOTS). **Per the generative AI policy**: this change was developed with the assistance of an AI tool (Claude Code). I have reviewed the change, understand it fully, and take ownership of it. **Risk Level**: Low — additive one-line allowlist change enabling passthrough of a single read-only introspection subcommand; no behavior change for any existing command. **Testing**: New unit test `RandomShardRequestTest.ClusterShards`, mirroring the existing `ClusterNodes` test. The full `//test/extensions/filters/network/redis_proxy:command_splitter_impl_test` target passes (249/249 tests), including the existing unsupported-subcommand rejection tests. `ci/do_ci.sh format` passes. **Docs Changes**: Added a ``CLUSTER SHARDS, Generic`` row to the supported commands table in `docs/root/intro/arch_overview/other_protocols/redis.rst`. **Release Notes**: `changelogs/current/new_features/redis_proxy__cluster-shards-command.rst` **Platform Specific Features**: N/A Signed-off-by: Aurimas Ragulis <aurimas.ragulis@vinted.com> Signed-off-by: Wenwei Zhao <wenweizhao@google.com>

Commit Message: redis filter: support
CLUSTER SHARDSAdds
shardsto the CLUSTER subcommand allowlist so cluster-aware clients can issue CLUSTER SHARDS (Redis 7.0+, the successor to the deprecated CLUSTER SLOTS) through the proxy. Like the other supported CLUSTER introspection subcommands (INFO, SLOTS, KEYSLOT, NODES), the command is forwarded to a single random upstream shard and the reply is returned to the client unmodified.See here for
CLUSTER SHARDSdocs.Additional Description:
This builds on the random-shard command framework introduced in #41381: subcommand validation is data-driven via
commandSubcommandValidationMap(), and routing/response handling reuse the existingRandomShardRequestpassthrough (works identically for RESP2 and RESP3 connections since the reply is relayed verbatim). On upstreams older than Redis 7.0, the upstream'sERR unknown subcommanderror is passed through to the client unchanged.I guess I can also add a small comment on why this is needed - we have a case where we want to use Envoy to fetch Redis cluster state before initializing a client, e.g. ruby
redis-cluster-client. This library, however, first callsCLUSTER SHARDSand only then falls back toCLUSTER NODES. See here.Having Envoy support
CLUSTER SHARDSwould help us save some network roundtrips and potentially avoid thundering herd problem on our network links during large scale app rollouts.Related issues, not fixed by this PR: #38492 tracks broader redis_proxy command coverage; #36975 asks for Envoy's own cluster topology discovery to use CLUSTER SHARDS, which is out of scope here (discovery still uses CLUSTER SLOTS).
Per the generative AI policy: this change was developed with the assistance of an AI tool (Claude Code). I have reviewed the change, understand it fully, and take ownership of it.
Risk Level: Low — additive one-line allowlist change enabling passthrough of a single read-only introspection subcommand; no behavior change for any existing command.
Testing: New unit test
RandomShardRequestTest.ClusterShards, mirroring the existingClusterNodestest. The full//test/extensions/filters/network/redis_proxy:command_splitter_impl_testtarget passes (249/249 tests), including the existing unsupported-subcommand rejection tests.ci/do_ci.sh formatpasses.Docs Changes: Added a
CLUSTER SHARDS, Genericrow to the supported commands table indocs/root/intro/arch_overview/other_protocols/redis.rst.Release Notes:
changelogs/current/new_features/redis_proxy__cluster-shards-command.rstPlatform Specific Features: N/A