Handle connecting to ipv6 addresses correctly

[jszwedko]

Previously, it would attempt to use the host from the url, which
includes the [] which to_socket_addrs() used directly when
resolving which would cause it to fail to resolve. This change
change strips the []s.

Fixes: #322

Signed-off-by: Jesse Szwedko jesse@szwedko.me

[Jarema]

@jszwedko thanks for taking the time and making this PR!

We will look into it this week.
Sorry for the delay!

[gaffneyc]

@Jarema I'm sorry for the ping but any chance you could look into this soon?

[Jarema]

@gaffneyc of course, I will look into that tomorrow, sorry for the delay!

[Jarema]

@jszwedko I'm a little surprised by this, as docs clearly state:

An IPv6 address. Url::host_str returns the serialization of that address between [ and ] brackets, in the format per RFC 5952 A Recommendation for IPv6 Address Text Representation: lowercase hexadecimal with maximal :: compression.

https://docs.rs/url/latest/url/enum.Host.html

so stripping should not be necessary.

Could you please provide failing test or some more details?

[gaffneyc]

It's not a failing test case (as I don't know enough Rust to contribute one) but the simple program below from #322 shows the issue.

// docker run --rm -ti -p 4223:4223 nats -p 4223 --user user --pass pass
fn main() {
    let nc = nats::connect("nats://user:pass@[::1]:4223");
    println!("{:#?}", nc);
}

[jszwedko]

HI @Jarema !

Thanks for taking a look.

It looks like host_str on Url contains the []. You can see in this in the url crate's test cases:

https://github.com/servo/rust-url/blob/cbc6a6d9a1e78e5504dc5502d29b67dd6ae3e87f/url/tests/unit.rs#L282-L297

Looking at RFC 5952 A Recommendation for IPv6 Address Text Representation this line jumped out to me:

The [] style as expressed in [RFC3986] SHOULD be employed, and is the default unless otherwise specified.

Which makes me think that wrapping it in [] is preferred.

However, then looking at https://url.spec.whatwg.org/#host-parsing (linked from https://docs.rs/url/latest/url/enum.Host.html#method.parse) it specifically says:

Return the result of IPv6 parsing input with its leading U+005B ([) and trailing U+005D (]) removed.

So now I'm not sure 😄

Probably worth opening up an issue on the url crate confirm expected behavior. I'll do that.

[jszwedko]

I guess the docs for host_str do explicitly state:

IPv6 addresses are given between [ and ] brackets.

I still think it's a bit confusing though so I opened servo/rust-url#770 to get some clarity from the url maintainers.

[Jarema]

I'll get on this review just after implementing Pull Consumers (on it now). Saw your issue, but no response yet, so I guess we have to work around it.

[jbergstroem]

Can I help testing this somehow? Would solve a few ugly workarounds I have in place.

[Jarema]

After investigation, seems that docs are off and we have to remove the [] ourselves, so that validates this PR.

Some small changes are suggested, but one missing thing is:

Please add tests :). It is especially important in the context that it is a bug on rust-url.
I would like to have this test failed when behaviour changes instead of getting unhappy users with broken clients.
If you need help with this let me know.

[Jarema]

I think if with else would be cleaner here.

[jszwedko]

Thanks for taking a look @Jarema ! I pushed up some unit tests. Let me know if you were thinking of something else for testing.

[Jarema]

LGTM!

thank you for your contribution and patience!