Support IPv6 addresses in the :net_http adapter #621
Conversation
Fixes lostisland#589. The problem comes from Net::HTTP not being able to understand brackets when passed a literal IPv6 address: Net::HTTP.start("[::1]") #=> :( Net::HTTP.start("::1") #=> :) URI provides two different methods to extract the network host from the URI: #host, and #hostname. The only difference being that `#hostname` will unwrap brackets for IPv6 addresses, which is what we want here.
|
Hi @foca and thanks for your PR. |
|
Thanks for the fix, @foca! Do you think it would be possible to add an automated test for all adapters that makes a rudimentary IPv6 request to our test server on localhost? |
|
Added an IPv6 test in a different branch. It fails (see test run at the end) for:
This isn't a problem with Faraday per-se, but with individual adapters (at least in the case of Net::HTTP, just the wrapper, not the library itself.) I would like to argue in favor of fixing them separately, as I don't really have time right now to delve into EventMachine and figure out how EM::HTTP works, and I'm not sure when I'll be able to get around to fix it. Would that be an acceptable path moving forward? (edit: That branch doesn't have the commit in this one, which is why it fails for Net::HTTP there)
|
|
|
👌 I'll do this today or tomorrow. Thanks ❤️ |
|
Ok, I opened issues upstream with both …and now it turns out that Travis's cloud providers don't support IPv6 on the loopback interface, so we can't really test against 😢 |
|
Given there's no guarantee Travis will support IPv6 anytime soon, is there any way we can get the original patch (using |
|
Hi @foca, I understand your frustration with all these and that the fix you're proposing for Net::HTTP adapter is extremely easy (using However, I hope you'll understand that we can't merge this branch adding IPv6 support for Net::HTTP adapter only if we don't even know if it's supported by the other adapters. I know this all sounds exaggerated, but it's our duty to guarantee that a feature is working (and will keep doing so) once we decide to support it. Maybe there's a way to mock these IPv6 calls? As far as I understood IPv6 support is disabled on containers used by Travis-CI, but at least mocking the calls will allow us to test if adapters are parsing them as expected... |
|
I would also argue that we probably want the PR/Issues on net-http-persistent and em-http-request to be merged before merging this one as that will grant compatibility with those 2 adapters (and we should enable IPv6 tests for them as well) |
No, it does not. There, hard question answered 😛 If you read through the original ruby-core issue where they end up adding hostname, you'll see that the only reason they did this was to preserve backwards compatibility, but that moving forward people are supposed to be using Also, most of Let's forget about IPv6 for a second here, and just consider "should Faraday use the recommended URI APIs, or the old ones"? Waiting for every single adapter to support IPv6 shouldn't hold that patch. Waiting on Travis to support IPv6 (which atm is, at best, a… "wellllllll, maybe someday?" scenario) should not hold that patch. I'd send a patch that just changes If you disagree this is a reasonable / pragmatic course of action, then feel free to close this PR 😄 |
|
I would like to see this PR merged to move this forward. IMO, Faraday can state they "support IPv6 if the underlying adapter supports it". Then, raise a NotImplementedError for adapters that don't support IPv6 (not sure on the how the actual implementation would be done, offhand, but that would solidify the question) |
|
@Fryguy we would all like to move towards IPv6, but apparently there's not much support on it (yet!). |
|
@foca @Fryguy I've reluctantly merged #714 with the motivations expressed in that PR, so that should be a good news for you. |
Fixes #589.
The problem comes from Net::HTTP not being able to understand brackets when passed a literal IPv6 address:
URI provides two different methods to extract the network host from the URI: #host, and #hostname.
The only difference being that
#hostnamewill unwrap brackets for IPv6 addresses, which is what we want here.There's a bit more background on why there's a different method in this ruby-core discussion/bug report: https://bugs.ruby-lang.org/issues/3788