IPv6 support for org.springframework.data.redis.connection.convert.Converters.ClusterNodesConverter#convert

[TheTweak]

Hello, when IPv6 network stack is used, org.springframework.data.redis.connection.convert.Converters.ClusterNodesConverter#convert can not convert the response from Redis CLUSTER NODES command and throws NumberFormatException.

Input to convert() in this case is:
67adfe3df1058896e3cb49d2863e0f70e7e159fa 2a02:6b8:c67:9c:0:6d8b:33da:5a2c:6380@16380,redis-master master,nofailover - 0 1692108412315 1 connected 0-5460
and then Integer.parseInt("6b8:c67:9c:0:6d8b:33da:5a2c:6380") fails https://github.com/spring-projects/spring-data-redis/blob/main/src/main/java/org/springframework/data/redis/connection/convert/Converters.java#L577

Discovered when using RedisHealthIndicator from Spring Data Redis module, and when connection to cluster is done via IPv6 network.

Expected behaviour:

• org.springframework.data.redis.connection.convert.Converters.ClusterNodesConverter#convert returns RedisClusterNode object

Actual behaviour:

• NumberFormatException is thrown

Spring Data Redis version: 3.1.1

Possible fix could be to split on the last occurrence of : here https://github.com/spring-projects/spring-data-redis/blob/main/src/main/java/org/springframework/data/redis/connection/convert/Converters.java#L563

[mp911de]

We indeed have a problem here, however, I wonder why Redis doesn't wrap the address in square brackets to disambiguate the port ([2a02:6b8:c67:9c:0:6d8b:33da:5a2c]:6380). In any case, we should leniently use the lastIndexOf approach as you suggested.

Do you want to submit a pull request or do you want us to implement the fix?

[TheTweak]

I will submit a PR, thanks!

[jxblum]

@TheTweak - You should also have a look at Issue #2360.

Also note, RedisHealthIndicator is part of Spring Boot, NOT part of Spring Data Redis!

I am also not entirely convinced that using lastIndexOf(':') is necessarily the correct approach here. It would seem the CLUSTER NODES command (doc) should always return a value of ip:port@cport, even for nodes in the cluster using the default port.

However, ip is the more ambiguous and variable part, so 1 thing to watch out for is that IPv6 addresses can omit 0s. See here.

[TheTweak]

@jxblum Thanks for the info!

So RedisNode.fromString() supports IPv6 format with ip in square brackets, which is a documented format for Spring Redis nodes configuration, and in this case RedisHealthIndicator passes raw response from CLUSTER NODES to converter, where ip is not in square brackets.

On one hand, support for IPv6 address w/o square brackets can be added to RedisNode.fromString(), but it doesn't seem right to me.. On the other hand, ClusterNodesConverter (or even RedisHealthIndicator) can check if square brackets are present and add them if not, then use RedisNode.fromString(), although it seems like a dirty hack.

Another option could be a regexp that can parse both inputs, with and w/o square brackets, implemented in ClusterNodesConverter, but that would partially duplicate the functionality of RedisNode.fromString()..

What do you think?

[mp911de]

CLUSTER NODE is somewhat specific as outlined in my first comment. It doesn't make also sense to implement host and port parsing twice, however, it would be neat to pre-process the endpoint information from CLUSTER NODES and pass it to RedisNode.from(…) for host and port parsing.

I'll take your pull request and amend it accordingly.

[mp911de]

Looking through the history how CLUSTER NODES evolved, we have patterns %s:%i,%s:%i@%i and %s:%i@%i,%s (IP + port, IP + port and bus port, IP + port, bus port and hostname).

It think that a regex is the best here, otherwise parsing can easily become a nightmare.