search

redis / go-redis

Redis Go client
https://redis.uptrace.dev
BSD 2-Clause "Simplified" License
20.2k stars 2.38k forks source link

Do not assume that all non-IP hosts are loopbacks #3085

Open suever opened 3 months ago

suever commented 3 months ago

The ClusterClient is configured such that if the cluster nodes that are discovered have a loopback IP provided as their address (e.g. 127.0.0.1), that the address of the origin that was used to connect to the cluster should be used instead (if it is not also a loopback). The logic for this is captured here.

The latest change to this code was in this PR which was a fix for some errors in the behavior of #589. However, in fixing #589, the logic behind determining if the origin's address was a loopback address was incorrectly changed.

In #589, if the origin host was not an IP address, then it was not considered to be a loopback address. Since hostname resolution doesn't happen, there is no way as it is currently written to determine if it is a loopback address so I think returning that it is not a loopback address is a valid option.

In the fix implemented in #839, if a hostname is used to connect to the cluster rather than an IP address, it is assumed to be a loopback address based on this change and this therefore prevents the substitution of the origin for a loopback address of a node as should happen.

This PR changes the behavior back to that of #589, where a hostname is not considered to be a loopback address and this allows a cluster client to substitute that hostname for a loopback address of a discovered cluster node that announces its own IP as a loopback address.

Another possible solution would be to always change loopback addresses of nodes to the origin address regardless of whether the origin is a loopback or not, but I opted for the minimal touch here to preserve the logic.

This was discovered when using a redis cluster in a docker-compose file where one service referenced the redis-cluster service by name (e.g. redis-cluster:7000), but the redis cluster was constructed using 127.0.0.1:7000 127.0.0.1:7001 127.0.0.1:7002.

suever commented 3 months ago

Failing tests are unrelated to this change and are due to the changes in 00d98485f8357d016139ece133bd3272e7ff2526 which moved redis-stack-server to the latest (master branch build is currently failing as well but pinning to 7.4.0-v0 allowed tests to pass in this PR)