SentinelsClient: Use the given protocol to connect to sentinels

[jlledom]

I was trying to connect to some sentinels that require password authentication. For that, I created a new protocol as in the example: https://github.com/socketry/async-redis/blob/main/examples/auth/protocol.rb

class AuthenticatedRESP2
  def initialize(password)
    @password = password
  end

  def client(stream)
    client = Async::Redis::Protocol::RESP2.client(stream)

    client.write_request(["AUTH", *@password])
    client.read_response # Ignore response.

    client
  end
end

When SentinelsClient is used, it ignores the provided protocol when it tries to resolve address for master and slaves. As a result, password protected sentinels are not supported by async-redis.

This PR includes a small change to fix that.

Types of Changes

• Bug fix.

Contribution

• [ ] I added tests for my changes.
• [x] I tested my changes locally.
• [x] I agree to the Developer's Certificate of Origin 1.1.

[ioquatix]

I am planning to introduce Async::Redis::Endpoint: https://github.com/socketry/async-redis/pull/50.

It may provide a better foundation for this functionality.

We may wish to introduce Async::Redis::SentinelEndpoint which handles the indirection about how to connect.

[ioquatix]

Thanks for your contribution.

I'm sorry I did not attend to this PR sooner. However, now it's merged and I'll release it soon.

[jlledom]

Thanks @ioquatix.

Note how this PR assumes the password for the sentinels and master is the same, it's reusing the same protocol for both. In order to support different credentials for sentinels and master, I ended up using @protocol only for master and extracting the sentinels credentials from the config (code). It's probably useful doing the same thing here.

[ioquatix]

@jlledom Let me make a first pass over it, and then if you can give feedback on the design, that would be great.

[ioquatix]

See https://github.com/socketry/async-redis/pull/51 for the updated sentinels code.

[ioquatix]

https://github.com/socketry/async-redis/blob/main/changes.md FYI

[ioquatix]

A few thoughts.

Now it's possible to supply endpoints for each sentinel which allows them to have different credentials.

However, I'm not sure how TLS would or should work for the actual instances. GET-MASTER-ADDR-BY-NAME only returns a host and port, not a URL e.g. rediss:// which would allow us to deduce the connection parameters.

I suppose what we could do is make the address construction a method, and allow sub-classes to override it, because I'm not sure if there is any general approach here.

[jlledom]

Now it's possible to supply endpoints for each sentinel which allows them to have different credentials.

I think that's not allowed. The docs say:

Note that Sentinel's authentication configuration should be applied to each of
the instances in your deployment, and all instances should use the same configuration.

I understood this as: it's mandatory that all instances use the same credentials. Also the example sentinels.conf file in the repo is more clear:

# You can configure Sentinel itself to require a password, however when doing
# so Sentinel will try to authenticate with the same password to all the
# other Sentinels. So you need to configure all your Sentinels in a given
# group with the same "requirepass" password. Check the following documentation
# for more info: https://redis.io/topics/sentinel

However, I'm not sure how TLS would or should work for the actual instances. GET-MASTER-ADDR-BY-NAME only returns a host and port, not a URL e.g. rediss:// which would allow us to deduce the connection parameters.

If I understood your code correctly, Redis::Endpoint doesn't currently accept SSL params, and the only param it sets is verify_mode. IMO, Redis::Endpoint should accept two more options: :ssl and :ssl_params. :ssl should determine whether a secure connection is used or not, rather than :scheme, and if :ssl is true, then :ssl_params should be used to build the ssl context. My implementation might be useful.

I don't think there's any other way to connect to TLS protected instances than sending :ssl and :ssl_params to the SentinelClient initializer and pass them along to the endpoint initializer.

[ioquatix]

Understood, so in any case, the sentinels should all have the same credentials.

Regarding Async::Redis::Endpoint, it does accept ssl_context which is the most general ssl configuration mechanism (ssl_params is more specific, but we could also add support for it).

I added the documentation here: https://github.com/socketry/async-redis/commit/6a75f890e67c57ff4d6417dc835379c13bf097d3

In this way, the sentinels could be using TLS but it may also need to be identical, or perhaps it's simply not supported?

[jlledom]

Regarding Async::Redis::Endpoint, it does accept ssl_context which is the most general ssl configuration mechanism

I added the documentation here: 6a75f89

I understand.

In this way, the sentinels could be using TLS but it may also need to be identical, or perhaps it's simply not supported?

Sentinels can be TLS protected. According to the docs, a sentinel will listen in the TLS port only when tls-replication is enabled. Which means TLS must be enabled for all sentinels or none. However, I don't know whether all sentinels must share the same SSL context. I'd say it's not really needed, any sentinel can have its own TLS cert and key as long as all of them accept connections from each other and from a client.

About how to connect to master from SentinelClient, the initializer can accept a new option :client_ssl_context to be used when acting as a client, i.e. when connecting to instances.

[ioquatix]

I think we should provide some more generic mechanism, like client_endpoint_options or something. It's used to construct the client endpoint from the host/port etc.