Do not assume that all non-IP hosts are loopbacks #3085
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Failing tests are unrelated to this change and are due to the changes in 00d9848 which moved |
suever
force-pushed
the
suever/fix-loopback-origin-check
branch
2 times, most recently
from
98da050 to
e059f9f
Compare
ndyakov
reviewed
Feb 20, 2025
| @@ -720,7 +720,7 @@ func replaceLoopbackHost(nodeAddr, originHost string) string { | |||
| func isLoopback(host string) bool { | |||
| ip := net.ParseIP(host) | |||
| if ip == nil { | |||
| return true | |||
| return false | |||
There was a problem hiding this comment.
Shoudn't localhost return true here?
There was a problem hiding this comment.
Actually, based on the network setup, many hostnames can be considered loopbacks... we should definitely handle localhost. host.docker.internal or even anything *.docker.internal can be considered loopback
@suever WDYT?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
ClusterClientis 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 using127.0.0.1:7000 127.0.0.1:7001 127.0.0.1:7002.