search

Nordix / hiredis-cluster

C client library for Valkey/Redis Cluster. This project is used and sponsored by Ericsson. It is a fork of the now unmaintained hiredis-vip.
BSD 3-Clause "New" or "Revised" License
86 stars 42 forks source link

Fix the check for ":0@0" in CLUSTER NODES result #199

Closed KevinLussier-Netskope closed 6 months ago

KevinLussier-Netskope commented 8 months ago

The current implementation assumes that the entire part is only 2 characters when the IP and port are not set (IP is empty and port is 0). However, this is incorrect because the '@cport' is also present so the current check fails. Additionally, there may be an optional ',hostname' as well which is not accounted for.

The fix here simply changes the logic from "if the part is ':0'" to "if the part starts with ':0'".

bjosv commented 8 months ago

Good finding. I wonder which scenarios this check is expected to help in, any idea? It shouldn't have been triggered since @ was introduced. After some digging in legacy it seems that this check was added 2015, must have been Redis 4 at that time.

bjosv commented 8 months ago

I found an example in an issue where :0@ is received from Redis. (I also searched for CLUSTER_NODE_NOADDR in the Redis source code)

If I understand it correctly it's in scenarios where node-ips are reused for new cluster nodes, like when running a Redis cluster in K8s. The check is probably still needed then, but with your correction. It would be great with an additional testcase for this though.

KevinLussier-Netskope commented 8 months ago

I found an example in an issue where :0@ is received from Redis. (I also searched for CLUSTER_NODE_NOADDR in the Redis source code)

If I understand it correctly it's in scenarios where node-ips are reused for new cluster nodes, like when running a Redis cluster in K8s. The check is probably still needed then, but with your correction. It would be great with an additional testcase for this though.

Yes. that example that you linked is precisely what was seen in our Redis Cluster. Hence the need for the fix :-). I'll try to add a test case to cover this.