Skip to content

Conversation

Sylvain-Royer
Copy link
Contributor

@Sylvain-Royer Sylvain-Royer commented Jun 18, 2025

Previously, the code assumed that all hosts contained only 1 :, where the left side was the host and the right was the port. Unfortunately, this breaks for IPv6. Instead, this code will find the last : and have the host be the last side, port be right.

This is related to this issue.

I've tested this on my local machine using docker compose set up in ipv6 only, as well as on my company's ipv6 only Kubernetes cluster. This is confirmed working on my end.

@CLAassistant
Copy link

CLAassistant commented Jun 18, 2025

CLA assistant check
All committers have signed the CLA.

….XX:PPPP formated ips. For example, ipv6 ips.
@Sylvain-Royer Sylvain-Royer force-pushed the fix/cluster_info_handle_ipv6 branch from ccb3cb7 to b1c73de Compare June 18, 2025 02:53
@Sylvain-Royer Sylvain-Royer marked this pull request as draft June 18, 2025 02:54
@Sylvain-Royer Sylvain-Royer marked this pull request as ready for review June 18, 2025 05:13
@Sylvain-Royer
Copy link
Contributor Author

Hello @KIvanow @pawelangelow @KrumTy @ArtemHoruzhenko @pd-redis @dantovska , sorry for pinging directly, but I'm wondering if I could get someone to take a look at this PR. We're hoping to be able to get this fix into the main branch as we'd like to be able to migrate over from v1 to v2.

@pawelangelow
Copy link
Collaborator

Appreciate your work on this, @Sylvain-Royer - I’ll get to it soon.

Copy link
Collaborator

@pawelangelow pawelangelow left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! As a small improvement, it might be helpful to include a JSDoc example above the function to clarify IPv4 and IPv6 handling, for example:

/**
 * Parse and return all endpoints from the nodes list returned by "cluster info" command
 * @Input
 * ```
 * 08418e3514990489e48fa05d642efc33e205f5 172.31.100.211:6379@16379 myself,master - 0 1698694904000 1 connected 0-5460
 * d2dee846c715a917ec9a4963e8885b06130f9f 172.31.100.212:6379@16379 master - 0 1698694905285 2 connected 5461-10922
 * 3e92457ab813ad7a62dacf768ec7309210feaf [2001:db8::1]:7001@17001 master - 0 1698694906000 3 connected 10923-16383
 * ```
 * @Output
 * ```
 * [
 *   {
 *     host: "172.31.100.211",
 *     port: 6379
 *   },
 *   {
 *     host: "172.31.100.212",
 *     port: 6379
 *   },
 *   {
 *     host: "2001:db8::1",
 *     port: 7001
 *   }
 * ]
 * ```
 * @param info
 */

Copy link
Collaborator

@pawelangelow pawelangelow left a comment

Choose a reason for hiding this comment

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

Oh, my bad! Sorry, but can I ask you to change your branch name to be prefixed with bugfix/... instead of fix/... as we run the appropriate tests based on it?

@Sylvain-Royer
Copy link
Contributor Author

Updated branch name, created new PR here #4652

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