Prefer to use Endpoint.for so that we properly persist the original seed endpoint scheme.

[travisbell]

I am migrating our Elasticache Redis Cluster over to Valkey and decided to update our config to use TLS and IPv6. During this migration, I noticed two separate issues.

First, I got an error when URI.parse gets an IPv6 address back from the shard list:

client = Async::Redis::ClusterClient.new([Async::Redis::Endpoint.parse("rediss://#{CLUSTER_CONFIG_ADDRESS}:6379")])
client.clients_for("test")

/usr/local/lib/ruby/3.4.0/uri/rfc3986_parser.rb:130:in 'URI::RFC3986_Parser#split': bad URI (is not URI?): "redis://2600:1f28:372:c404:5c2d:ce68:3620:cc4b:" (URI::InvalidURIError)
	from /usr/local/lib/ruby/3.4.0/uri/rfc3986_parser.rb:135:in 'URI::RFC3986_Parser#parse'
	from /usr/local/lib/ruby/3.4.0/uri/common.rb:212:in 'URI.parse'
	from /usr/local/bundle/gems/async-redis-0.11.1/lib/async/redis/endpoint.rb:29:in 'Async::Redis::Endpoint.remote'
	from /usr/local/bundle/gems/async-redis-0.11.1/lib/async/redis/cluster_client.rb:119:in 'block (3 levels) in Async::Redis::ClusterClient#reload_cluster!'
	from /usr/local/bundle/gems/async-redis-0.11.1/lib/async/redis/cluster_client.rb:117:in 'Array#map'
	from /usr/local/bundle/gems/async-redis-0.11.1/lib/async/redis/cluster_client.rb:117:in 'block (2 levels) in Async::Redis::ClusterClient#reload_cluster!'
	from /usr/local/bundle/gems/async-redis-0.11.1/lib/async/redis/cluster_client.rb:111:in 'Array#each'
	from /usr/local/bundle/gems/async-redis-0.11.1/lib/async/redis/cluster_client.rb:111:in 'block in Async::Redis::ClusterClient#reload_cluster!'
	from /usr/local/bundle/gems/async-redis-0.11.1/lib/async/redis/cluster_client.rb:105:in 'Array#each'
	from /usr/local/bundle/gems/async-redis-0.11.1/lib/async/redis/cluster_client.rb:105:in 'Async::Redis::ClusterClient#reload_cluster!'
	from /usr/local/bundle/gems/async-redis-0.11.1/lib/async/redis/cluster_client.rb:88:in 'Async::Redis::ClusterClient#client_for'
	from /usr/local/bundle/gems/async-redis-0.11.1/lib/async/redis/cluster_client.rb:68:in 'block in Async::Redis::ClusterClient#clients_for'
	from /usr/local/bundle/gems/async-redis-0.11.1/lib/async/redis/cluster_client.rb:67:in 'Hash#each'
	from /usr/local/bundle/gems/async-redis-0.11.1/lib/async/redis/cluster_client.rb:67:in 'Async::Redis::ClusterClient#clients_for'
	from (irb):8:in 'block in <top (required)>'
	from /usr/local/bundle/gems/async-2.25.0/lib/async/task.rb:201:in 'block in Async::Task#run'

But then second, notice the scheme it's trying to connect to redis://2600:1f28:372:c404:5c2d:ce68:3620:cc4b:. So there's a few things wrong here.

Upon trying to decide the best way to fix this, I came across the Redis Cluster docs which say the following about how we should connect to the cluster nodes:

The 'nodes' field contains a list of all nodes within the shard. Each individual node is a map of attributes that describe the node. Some attributes are optional and more attributes may be added in the future. The current list of attributes:

...
endpoint: The preferred endpoint to reach the node, see below for more information about the possible values of this field.
ip: The IP address to send requests to for this node.
...

It specifically says to prefer using the endpoint attribute, instead of the ip attribute which is what we were trying to connect to originally. By switching Endpoint.remote out for Endpoint.for, we can actually kill two birds with one stone here. We are able to resolve the IPv6 address from the endpoint value without any trouble, and we're also able to pass the original scheme.

This report does highlight an unresolved issue for anyone trying to use Endpoint.remote with an IPv6 address but I figure that can be solved with a separate fix.

FWIW, the redis-rb clustering library gets around this because it is not using URI.parse at all. They're simply splitting the address string by the last :.

Types of Changes

• Bug fix

Contribution

• I tested my changes locally.
• I agree to the Developer's Certificate of Origin 1.1.

[ioquatix]

Thanks for this.

I'm okay with this change.

It should be noted that this generates an invalid URL but for our purposes it's good enough:

URI.parse URI::Generic.new("redis", nil, "2600:1f28:372:c404:5c2d:ce68:3620:cc4b", nil, nil, "/", nil, nil, nil)

[travisbell]

Yup, agreed that is not a perfect fix. It's a band-aid.

One thing I should have made sure to clarify is that (at least in AWS' case) the endpoint value is a hostname. I don't think it inherently needs to be though, as I can see in my own local test cluster, the endpoint value is actually an IP. So if someone's cluster returns an IPv6 as the endpoint address, we're back to the same root issue of Endpoint not supporting IPv6. But that was the case already.

Thanks for merging so quickly. 👍🏼