redis / rueidis

A fast Golang Redis client that supports Client Side Caching, Auto Pipelining, Generics OM, RedisJSON, RedisBloom, RediSearch, etc.
Apache License 2.0
2.43k stars 154 forks source link

Rueidis making `CLIENT SETINFO` commands that are not supported by ElastiCache Redis #620

Closed rahulgkatre closed 2 months ago

rahulgkatre commented 2 months ago

We have observed that our application is making CLIENT SETINFO commands to the Redis server hosted on AWS ElastiCache. However, these commands are not supported by ElastiCache Redis.

The CLIENT SETINFO command is used to set arbitrary information for the current connection. While this command is available in open-source Redis version 7.2 and up, it is not supported by ElastiCache Redis (which is on "7.1" or 7.0).

Steps to Reproduce:

  1. Create an ElastiCache Redis instance (e.g. serverless)
  2. Connect to the ElastiCache Redis instance using a Rueidis client
  3. The ElastiCache metric CommandAuthorizationFailures should increase

Potential fix: The Rueidis client has an option for ClientSetInfo but there is no option to disable it entirely for support with older versions of Redis. An option can be added to the client options type to disable the execution of this command.

rueian commented 2 months ago

Instead of another option, I would prefer this:

var DisableClientSetInfo = make([]string)
...
rueidis.NewClient(rueidis.ClientOption{
    ClientSetInfo: rueidis.DisableClientSetInfo,
})
rahulgkatre commented 2 months ago

I've been trying your suggested fix out, diff here: https://github.com/rahulgkatre/rueidis/pull/1/files

The test I added for this change passes locally but in the workflow there are failures related to data races: https://github.com/rahulgkatre/rueidis/actions/runs/10605550928/job/29394567245#step:4:3962

rueian commented 2 months ago

Hi @rahulgkatre,

Thank you for trying to add the DisableClientSetInfo. There are a couple of lines that need to be changed in your PR:

  1. DisableClientSetInfo = make([]string) instead of DisableClientSetInfo = make([]string, 2)
  2. slices.Equal may not be necessary. We can just use the elseif option.ClientSetInfo == nil condition for the default behavior and then else for not setting the client name.
  3. There are at least 4 sections related to the option.ClientSetInfo and all of them need to be changed: https://github.com/redis/rueidis/blob/9a347d752902b16afe2fda0c327fdbee732b3919/pipe.go#L171-L175 https://github.com/redis/rueidis/blob/9a347d752902b16afe2fda0c327fdbee732b3919/pipe.go#L189 https://github.com/redis/rueidis/blob/9a347d752902b16afe2fda0c327fdbee732b3919/pipe.go#L252-L256 https://github.com/redis/rueidis/blob/9a347d752902b16afe2fda0c327fdbee732b3919/pipe.go#L261

You can sent the PR directly to this repository.

rahulgkatre commented 2 months ago

Thanks for the help, workflows passed and I've opened the PR to this repo.