Use node address instead of relying on loopback reported by redis

[dim]

Not sure if this is a redis bug or a feature, but we have recently noticed CLUSTER SLOTS suddenly reporting:

1) 1) (integer) 12290
   2) (integer) 12697
   3) 1) "10.0.0.120"
      2) (integer) 6379
      3) "906ad7f61c9173cc86a793e7bdfc9d204153476f"
   4) 1) "127.0.0.1"
      2) (integer) 6379
      3) "e8e136bf028b88f4c7d5d64e73f134ed9b89dab6"

A similar issue was reported here

[vmihailenco]

This breaks the build since we run multiple Redis Servers on 127.0.0.1 using different ports. And I don't see how we can fix that.

[dim]

I didn't want to overcomplicate this, but it seems I'll have to (a little). Will push an update shortly.

[vmihailenco]

Thanks. AFAIR I saw at least one similar report before.

[stonelgh]

@vmihailenco It seems that your later commit 4237a34 "cluster: fix origin addr check" breaks this fix. Due to the port check you added in this commit, it will only correct only one of the node's address in a cluster. Hence, you'll see interleaved successful and failing accesses in the test if different keys used. By the way, what does commit 4237a34 aim for?

[vmihailenco]

See #771. That commit fixed the case when some/all cluster nodes are on the same host, e.g. localhost:7000 and localhost:7001. Without that commit port was ignored and same address was used for different ports.

I don't understand the part about "only correct only one of the node's address in a cluster". I believe that the only change is that now code also compares nodes ports.

[stonelgh]

I think I get the point now. Assuming we have 3 nodes on one host, with port assigned as 7001-7003. They're reported as 127.0.0.1:700x in the cluster slots info. Now we're using ip 9.9.9.9 to access them from another host. The previous fix will set all nodes's address to 9.9.9.9:7001, while your fix will set them to 9.9.9.9:7001, 127.0.0.1:7002 and 127.0.0.1:7003.

[vmihailenco]

Yeah, you are right. I guess you'd like to get 9.9.9.9:7001, 9.9.9.9:7002 and 9.9.9.9:7003.

Is this still a problem with latest Redis Cluster? Are there any other fixes?..

[vmihailenco]

@stonelgh ptal #839

[stonelgh]

@vmihailenco Good job! This is really what I want to address. At least it is still a problem with Redis (v4.0.11) and go-redis (e3b56f7). And I don't see any other fix.

I'm not sure if it is worth mentioning here. But taken the case mentioned by @dim , let's assume that node e8e1 (reported as 127.0.0.1:6379 in dim's case, now let it be 127.0.0.1:7000) is running inside a docker on host 10.0.0.120, and is port-mapped to 37000. Hence we can access it via the address 10.0.0.120:37000. When we connect to the cluster via address 10.0.0.120:6379 or 10.0.0.120:37000, node e8e1's address will be revised to 10.0.0.120:7000, which is still not correct. To be still correct in this special case, we probably need to introduce some bigger changes.

One idea is to issue the cluster nodes command multiple times to identify valid addresses for nodes with unreachable reported IPs from the client. To be specific, if an unreachable address is reported, we can iterate through the addresses user provided to try to connect to it, issue a cluster nodes command and check the result. If the node ID marked with 'myself' in the returned result is the same as the one of current node, then the address used in this iteration is the correct address of current node. Thus, at least we can ensure that nodes with an valid address specified when calling NewClusterClient(...) will be available. This idea is also valid for clusters created using internal IPs but accessed via public IPs as long as the client user provides each node's public address when creating a ClusterClient.

What's your guys' opinion?

[vmihailenco]

I think the official recommendation for that case is to use 10.0.0.120:37000 everywhere in Redis config so Redis Cluster does not know about other addresses. Are there any considerable drawbacks?

I am reluctant to add anything more complex to go-redis because

1. it is complex and fixes mis-configuration on the user side
2. considerably slows cluster state creation

[stonelgh]

I don't see any considerable drawbacks other than that it will take more time to create the cluster state.

I understand your considerations. I think we can wait until more people think it is necessary or helpful even at the cost of complexity and longer startup time.