Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

KevinLussier-Netskope
Copy link
Contributor

@KevinLussier-Netskope KevinLussier-Netskope commented Jan 4, 2024

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'".

… 'cluster nodes' result. 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.
@bjosv
Copy link
Collaborator

bjosv commented Jan 4, 2024

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
Copy link
Collaborator

bjosv commented Jan 4, 2024

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
Copy link
Contributor Author

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.

hircluster.c Outdated Show resolved Hide resolved
@zuiderkwast zuiderkwast changed the title Fix the check for the 'ip:port@cport' part when parsing a line in the 'cluster nodes' result Fix the check for ":0@0" in CLUSTER NODES result Feb 9, 2024
Copy link
Collaborator

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge this and include in the release. It's small enough to merge without a test case.

@zuiderkwast zuiderkwast merged commit 2c1da82 into Nordix:master Feb 9, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants