Make sure we handle IPV6 ips properly when extracting host and port. #5104

Sylvain-Royer · 2025-12-26T08:04:34Z

Issue link

This Pull Request is linked to issue 5103

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one issue.
Commit message has a detailed description of what changed and why.
Tests are added or updated.
CHANGELOG.md and documentation files are updated.
Destination branch is correct - main or release
Create merge commit if merging release branch into main, squash otherwise.

currantw

Just a couple comments from me. Let me know if you have the capacity to make these changes, otherwise I'm happy to take this over the finish line as well.

(Also seems to be some linting issues that need to be resolved.)

currantw · 2026-01-03T16:47:14Z

glide-core/redis-rs/redis/src/cluster_async/connections_logic.rs

 /// Splits a string address into host and port. If the passed address cannot be parsed, None is returned.
-/// [addr] should be in the following format: "<host>:<port>".
+/// [addr] should be in one of the following formats:
+/// - IPv4/hostname: "<host>:<port>" (e.g., "127.0.0.1:6379")
+/// - IPv6 bracketed: "[<ipv6>]:<port>" (e.g., "[2001:db8::1]:6379")
+/// - IPv6 unbracketed with port: "<ipv6>:<port>" - last segment treated as port (e.g., "2001:db8::1:6379" → host "2001:db8::1", port 6379)
 pub(crate) fn get_host_and_port_from_addr(addr: &str) -> Option<(&str, u16)> {


Do you have any reason to believe that we need to support unbracketed IPv6 format? My understanding is that RFC 3986 requires brackets, and that this should be the format returned by Valkey/Redis servers.

That would also allow the algorithm to be simplified significantly:

Split off the last :

Remove [] from left string if present

So, if an end user passes in unbracketed values into the initial hosts. this would allow it to work. Now, if we don't want it to work and we want it to fail, then fair enough, I could simplify this logic and only allow for bracketed ipv6.

that said, we can still simplify the algo to be what you described. it would work for both bracketed and unbracketed. Let me do that.

So, if an end user passes in unbracketed values into the initial hosts. this would allow it to work.

If I understand correctly, you are describing the following:

When the user creates the Valkey cluster servers, they specify IPv6 addresses without brackets.

When trying to refresh topology, get_host_and_port_from_addr gets called (internally) with an address string that does not include the brackets.

Am I understanding this correctly?

Yes, I believe so. Though I could be wrong, it is possible that the ipv6 host gets added brackets later on.

glide-core/redis-rs/redis/src/cluster_async/connections_logic.rs

jduo · 2026-01-05T16:29:11Z

glide-core/redis-rs/redis/src/cluster_async/connections_logic.rs

+/// - IPv6 bracketed: "[<ipv6>]:<port>" (e.g., "[2001:db8::1]:6379")
+/// - IPv6 unbracketed with port: "<ipv6>:<port>" - last segment treated as port (e.g., "2001:db8::1:6379" → host "2001:db8::1", port 6379)
+pub fn get_host_and_port_from_addr(addr: &str) -> Option<(&str, u16)> {
+    let (host, port_str) = addr.rsplit_once(':')?;


This seems reasonable, but might be better to use https://doc.rust-lang.org/std/net/struct.Ipv6Addr.html ?

Sorry, I'm not sure I fully understand how we would use this structure to help out here. This seems to be helpful for IP address which contain both IPv4 and IPv6 in them. Is your thought to use the to_ipv6_compatible function to convert IPv4 to IPv6 compatible IPv4 addresses?

It'd be to use the parse() and ToString() functions instead of manually parsing the format.

Unfortunately, I think we would still need to manually parse out the brackets and remove the port from the original. When I have either [2001:db8:85a3:8d3:1319:8a2e:370:7348]:8080 or [2001:db8:85a3:8d3:1319:8a2e:370:7348] as the host, this host.parse::<Ipv6Addr>() does not return OK. It is only when I have the host be 2001:db8:85a3:8d3:1319:8a2e:370:7348 that it works correctly.

I do think that having that parse will still be useful in terms of validating that it is a valid IPv6 address after doing all the parsing, but I don't believe this can help with parsing. I'll go ahead and add that validation step.

currantw · 2026-01-06T23:27:38Z

glide-core/redis-rs/redis/src/cluster_async/connections_logic.rs

+    let host = host
+        .strip_prefix('[')
+        .and_then(|h| h.strip_suffix(']'))
+        .map(|inner| inner.parse::<Ipv6Addr>().ok().map(|_| inner))
+        .unwrap_or(Some(host))?;


Nit. I think we could make this a bit simpler/more readable (though I am certainly no Rust expert!) 🤷

Using filter:

let host = host .strip_prefix('[') .and_then(|h| h.strip_suffix(']')) .filter(|inner| inner.parse::<Ipv6Addr>().is_ok()) .unwrap_or(host)

Or with an if block:

let host = if host.starts_with('[') && host.ends_with(']') { let inner = &host[1..host.len() - 1]; inner.parse::<Ipv6Addr>().ok()?; inner } else { host };

~~I think filter makes the most sense. I'm also no rust expert, first time using this language. Updated.~~

Nevermind, if statement I think is a little better and is more correct/allows for the ipv6 validation to filter out bad ones better.

currantw · 2026-01-06T23:40:28Z

glide-core/redis-rs/redis/tests/test_async_cluster_connections_logic.rs

+    #[tokio::test]
+    async fn test_get_host_and_port_from_addr_ipv4() {
+        assert_eq!(
+            get_host_and_port_from_addr("127.0.0.1:6379"),
+            Some(("127.0.0.1", 6379))
+        );
+        assert_eq!(
+            get_host_and_port_from_addr("192.168.1.1:8080"),
+            Some(("192.168.1.1", 8080))
+        );
+    }


I don't think it is necessary for these to be async:

Suggested change

#[tokio::test]

async fn test_get_host_and_port_from_addr_ipv4() {

assert_eq!(

get_host_and_port_from_addr("127.0.0.1:6379"),

Some(("127.0.0.1", 6379))

);

assert_eq!(

get_host_and_port_from_addr("192.168.1.1:8080"),

Some(("192.168.1.1", 8080))

);

}

#[test]

fn test_get_host_and_port_from_addr_ipv4() {

assert_eq!(

get_host_and_port_from_addr("127.0.0.1:6379"),

Some(("127.0.0.1", 6379))

);

assert_eq!(

get_host_and_port_from_addr("192.168.1.1:8080"),

Some(("192.168.1.1", 8080))

);

}

Makes sense, updated

Make sure we handle IPV6 ips properly when extracting host and port.

1ff2e97

Sylvain-Royer requested a review from a team as a code owner December 26, 2025 08:04

currantw self-requested a review January 3, 2026 03:10

currantw reviewed Jan 3, 2026

View reviewed changes

Sylvain-Royer added 2 commits January 5, 2026 10:25

Move tests to test_async_cluster_connections_logic file.

466db35

Simplify get_host_and_port_from_addr

445aa3a

jduo reviewed Jan 5, 2026

View reviewed changes

Validate IPv6 address after parsing.

44efb79

xShinnRyuu linked an issue Jan 6, 2026 that may be closed by this pull request

Core: Hostnames which resolve to IPv6 only do not work #5103

Open

currantw reviewed Jan 6, 2026

View reviewed changes

Sylvain-Royer added 2 commits January 7, 2026 11:37

Fix nit, change tests to be sync.

c551d24

Swap to if statement, a little more readable.

01deca6

Make sure we handle IPV6 ips properly when extracting host and port. #5104

Are you sure you want to change the base?

Make sure we handle IPV6 ips properly when extracting host and port. #5104

Uh oh!

Conversation

Sylvain-Royer commented Dec 26, 2025

Issue link

Checklist

Uh oh!

currantw left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sylvain-Royer Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

currantw left a comment •

edited

Loading

Sylvain-Royer Jan 7, 2026 •

edited

Loading