Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

order of getaddrinfo results not respected #6307

Closed
benschulz opened this issue Apr 20, 2016 · 65 comments
Closed

order of getaddrinfo results not respected #6307

benschulz opened this issue Apr 20, 2016 · 65 comments
Labels
dns Issues and PRs related to the dns subsystem.

Comments

@benschulz
Copy link

  • Version: v5.10.1
  • Platform: Linux archer 4.4.5-1-ARCH deps: update openssl to 1.0.1j #1 SMP PREEMPT Thu Mar 10 07:38:19 CET 2016 x86_64 GNU/Linux
  • Subsystem: dns/cares_wrap

The results returned by getaddrinfo are reordered, giving preference to ipv4 addresses. The explanation given in #4693 seems unsatisfactory. Not implementing happy eyeballs is a reasonable design decision, of course, but why does that mean ipv4? It should mean taking the first entry. At the moment node is deliberately defying the system configuration.

@jasnell
Copy link
Member

jasnell commented Apr 20, 2016

As I understand it, without the reordering, ipv6 addresses could be the first entry, which would break anywhere that doesn't have an ipv6 stack. The current implementation is intentional in order to give preference to ipv4. /cc @bnoordhuis

@addaleax
Copy link
Member

Is this still a problem when using the latest v6 release candidate (aka is this fixed by #6021)?

@addaleax addaleax added the dns Issues and PRs related to the dns subsystem. label Apr 20, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Apr 20, 2016

The DNS hints aren't related to the reordering.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 20, 2016

@bnoordhuis would the dns.ADDRCONFIG flag mitigate the problem?

@benschulz
Copy link
Author

As I understand it, the proper place to configure preferences is via gai.conf.

Snippet from my gai.conf:

# precedence  <mask>   <value>
#
#    Defaults:
#
#precedence  ::1/128       50
#precedence  ::/0          40
#precedence  2002::/16     30
#precedence ::/96          20
#precedence ::ffff:0:0/96  10
#
#    For sites which prefer IPv4 connections change the last line to
#
#precedence ::ffff:0:0/96  100

@bnoordhuis
Copy link
Member

would the dns.ADDRCONFIG flag mitigate the problem?

Only partially. AI_ADDRCONFIG means 'return AAAA entries only when there is a configured IPv6 interface' but the presence of such an interface doesn't say anything about IPv6 traffic actually being routable. (That's true for IPv4 as well, of course; my point is that AI_ADDRCONFIG isn't a fix per se.)

We made the decision to prefer IPv4 over IPv6 three or four years ago when IPv6 support was much more spotty than it is today. It's something we can reconsider if the majority thinks it's a good idea.

@jasnell
Copy link
Member

jasnell commented Apr 22, 2016

There are likely two things that can happen here:

  1. We can improve the documentation to indicate why the results are ordered the way they are, and
  2. We can add an option that would tell the impl not to re-order the output. The current default behavior would remain.

@benschulz
Copy link
Author

benschulz commented Apr 25, 2016

This is clearly a violation of the principle of least astonishment and I don't see how adding documentation is going to help. Hardly anyone affected by this will be using the dns module directly. It took me half a day before realizing my issue was with the dns module. Since then I have put a workaround in place and am no longer affected. Still, I think you should slowly phase out this odd logic. IMHO, over time, keeping it will hurt more than it helps.

@igalic
Copy link

igalic commented Dec 23, 2016

from what i gather, this is possibly the source of my issue with npm install failing (a possible regression of npm/npm#6857)

This time i'm running alpine (hence, muslc) under docker, again, IPv6-only network.

~ # node -e "dns.lookup('registry.npmjs.org', (...args) => console.log(args))"
[ null, '151.101.112.162', 4 ]
~ # 

note that musl implements rfc 3484/6724.

this works fine in ruby, btw:

# pry -r 'open-uri'
[1] pry(main)> open("https://registry.npmjs.org/")
=> #<StringIO:0x0055ecc25d4fd0
 @base_uri=#<URI::HTTPS https://registry.npmjs.org/>,
 @meta=
  {"server"=>"CouchDB/1.5.0 (Erlang OTP/R16B03)",
   "content-type"=>"application/json",
   "cache-control"=>"max-age=300",
   "content-length"=>"194",
   "accept-ranges"=>"bytes",
…etc…

from what i gather, this can be fixed with:

diff --git a/lib/dns.js b/lib/dns.js
index cbb994b8f2..c161a89a66 100644
--- a/lib/dns.js
+++ b/lib/dns.js
@@ -140,7 +140,7 @@ exports.lookup = function lookup(hostname, options, callback) {
     if (all) {
       callback(null, []);
     } else {
-      callback(null, null, family === 6 ? 6 : 4);
+      callback(null, null, family === 4 ? 4 : 6);
     }
     return {};
   }

@benschulz
Copy link
Author

That fix would default the family to 6 iff the caller isn't asking for all families. However, most of the time the caller should be asking for all families. Even if that was not the case, changing defaults is a mean thing to do to clients.

Generally any caller should ask for all families and then try the results in-order. If the caller were to implement happy eyeballs they would ask for all and concurrently try the per-familiy subsequences in-order.

IMHO the proper way to fix this is to apply #4693. Whether to happy eyeball or not is orthogonal.

@sam-github
Copy link
Contributor

sam-github commented Dec 29, 2016

Happy Eyeballs - for those who haven't heard the term

@igalic
Copy link

igalic commented Feb 3, 2017

every month or two, i try a fresh major release of nodejs, only to find this issue is still there.

(@jasnell commented on 22 Apr 2016) There are likely two things that can happen here:

  • We can improve the documentation to indicate why the results are ordered the way they are, and
  • We can add an option that would tell the impl not to re-order the output. The current default behavior would remain.

i highly agree with @benschulz here:

(@benschulz commented on 25 Apr 2016 • edited) This is clearly a violation of the principle of least astonishment and I don't see how adding documentation is going to help. Hardly anyone affected by this will be using the dns module directly. It took me half a day before realizing my issue was with the dns module. Since then I have put a workaround in place and am no longer affected. Still, I think you should slowly phase out this odd logic. IMHO, over time, keeping it will hurt more than it helps.

and believe that #4693 is the correct way forward — in 7.x+

@Trott
Copy link
Member

Trott commented Jul 16, 2017

Should this remain open? (I'm guessing the answer is "yes" but would be happy to find out I'm wrong.)

@benschulz
Copy link
Author

benschulz commented Jul 16, 2017

This is your project and you are free to take it in any direction you please. If you think the status quo is good enough or even right, you should close the ticket. All I can tell you is that I had a really rough time with this. But that's just one data point, I imagine there are several more people affected.
Fermi estimate: Two people have raised the issue; if we conservatively posit that .1‒1% of those affected manage to figure out the root cause and find this report, it'd be around 200‒2000 people affected in total.

Hope that helps. :P

Edit: Added missing "affected" qualifier. =)

@gibfahn
Copy link
Member

gibfahn commented Jul 17, 2017

@bnoordhuis looking at your comments:

#6307 (comment)

We made the decision to prefer IPv4 over IPv6 three or four years ago when IPv6 support was much more spotty than it is today. It's something we can reconsider if the majority thinks it's a good idea.

#4693 (comment)

We don't do happy eyeballs. If you change the order, that would have to change. That's a big can of worms.

I understand that you're against implementing happy eyeballs, but not necessarily against just letting the results come back in their original order. Is your second comment saying that ceasing to modify the order will force us to implement happy eyeballs?

I understand that this may be a massive breaking change, and thus a non-starter, but if it's possible to make that change we can at least consider it (even if it's just an option for now, and doesn't become the default till Node 18.x).

@bnoordhuis
Copy link
Member

Is your second comment saying that ceasing to modify the order will force us to implement happy eyeballs?

Not exactly. Happy eyeballs is a transition mechanism. It's not needed in a world where IPv6 connectivity is good enough. Do we live in that world yet? Maybe, maybe not; not enough data.

@benschulz
Copy link
Author

Happy eyeballs is a transition mechanism. It's not needed in a world where IPv6 connectivity is good enough.

That's the part of the argument I don't understand. I set my system up to resolve names to IPv6 addresses and, if that yields no results, to IPv4 addresses. What's the issue? (I consider names with invalid AAAA entries an issue with the name, not my system configuration.)

@bnoordhuis
Copy link
Member

A valid AAAA entry does not mean there is a network path to that machine.

That was historically a big barrier to IPv6 adoption and is what happy eyeballs tries to address: try IPv6 and IPv4 in parallel and use whatever manages to establish a connection.

@benschulz
Copy link
Author

A valid AAAA entry does not mean there is a network path to that machine.

So anyone affected has an IPv4 and an IPv6 address and their ISP is unable to route all IPv6 packets? Couldn't they do the reverse of what I do, i.e. prefer IPv4 over IPv6 at the OS rather than application level?

I appreciate the time taken for the various comments, thank you.

@bnoordhuis
Copy link
Member

Yes, but that takes time and effort to set up. We want to avoid a situation where a node.js release breaks the out-of-the-box experience for a large group of users, especially when it might be difficult to debug for them.

@igalic
Copy link

igalic commented Jul 17, 2017

then, the best way forward, as mentioned numerous times in this issue, would be to implement happy eyes in a future version of node.js

@gibfahn
Copy link
Member

gibfahn commented Jul 18, 2017

then, the best way forward, as mentioned numerous times in this issue, would be to implement happy eyes in a future version of node.js

I don't see anyone in this thread suggesting implementing happy eyeballs in Node.js

@telmich
Copy link

telmich commented Aug 8, 2017

Not sure if this is the right bug, but IPv6 support seems to be seriously broken since 0.10... until and including v6.11.2. No version (also not 4.8.2 or 4.8.4) are capable of working in IPv6 only networks (as originally described in node-fetch/node-fetch#66). It seems that npm is only trying IPv4, where everything else works.

This breaks various software building on top of node and will cause a variety of problems when bigger vendors like Apple will soon require IPv6 networks to function.

Is there any chance that there will be a proper IPv6 support in nodejs in the near future?

@telmich
Copy link

telmich commented Aug 8, 2017

I am sorry, I actually meant to refer to npm/npm#6857

@refack
Copy link
Contributor

refack commented Aug 9, 2017

@telmich just for clarity, there is a separation of responsibility between node and npm (although there is obviously strong coupling), and node does support other package managers.
My question is, can you make it clear whether node isn't working for you in an IPv6 only environment, or is it npm that fails?
If it is the former, can you open a new issue? Optimally with a clear description of the scenario, and a minimal reproducing code?
If it is the later, a better recourse would be to open an issue in the npm repository, or evaluate an alternative package manager.

@refack
Copy link
Contributor

refack commented Aug 9, 2017

@benschulz and @igalic will a configuration mechanism that will reorder/restore-the-order of getaddrinfo result fulfill the requirements of your use case?
Such a mechanism could be an environment variable (like NODE_OPTIONS), a global variable (like require.extensions) or a cli flag (like --zero-fill-buffers)

@refack refack self-assigned this Aug 9, 2017
@igalic
Copy link

igalic commented Aug 9, 2017

env options or global config vars would work.
cli options would not, cuz you don't know how many layers of software you're working under…

the (often unknown (to an end-user)) layers of indirection are also why it takes so long for people to pup up here after having exhausted all other venues.
when you read thru people's debugging history of this bug it's always,

  • blame the setup
  • verify other software
  • suspect npm (or equivalent)
  • npm devs tell you it's a dns (core module) issue
  • validate issue (wow, it's so obvious now!)

this behaviour of the software is really unintuitive.

but then people finally get here (at least those that persisted thru all these steps) and find a bug where we're arguing whether in a world where we've run out of IPv4 addresses, we should support an IPv6 only setup. for over a year now.

treysis pushed a commit to treysis/io.js that referenced this issue Sep 3, 2021
Switch the default from false (reorder the result so that IPv4 addresses
come before IPv6 addresses) to true (return them exactly as the resolver
sent them to us.)

Fixes: nodejs#31566
Refs: nodejs#6307
Refs: nodejs#20710
Refs: nodejs#38099
Reissue of nodejs#31567
Reissue of nodejs#37681
Reissue of nodejs#37931
nodejs-github-bot pushed a commit that referenced this issue Sep 12, 2021
Switch the default from false (reorder the result so that IPv4 addresses
come before IPv6 addresses) to true (return them exactly as the resolver
sent them to us.)

Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Reissue of #31567
Reissue of #37681
Reissue of #37931

PR-URL: #39987
Refs: #6307
Refs: #20710
Refs: #38099
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
nodejs-github-bot pushed a commit that referenced this issue Sep 12, 2021
PR-URL: #39987
Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
nodejs-github-bot pushed a commit that referenced this issue Sep 12, 2021
PR-URL: #39987
Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
nodejs-github-bot pushed a commit that referenced this issue Sep 12, 2021
PR-URL: #39987
Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
nodejs-github-bot pushed a commit that referenced this issue Sep 12, 2021
PR-URL: #39987
Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
nodejs-github-bot pushed a commit that referenced this issue Sep 12, 2021
PR-URL: #39987
Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
nodejs-github-bot pushed a commit that referenced this issue Sep 12, 2021
PR-URL: #39987
Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
nodejs-github-bot pushed a commit that referenced this issue Sep 12, 2021
PR-URL: #39987
Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
nodejs-github-bot pushed a commit that referenced this issue Sep 12, 2021
PR-URL: #39987
Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
nodejs-github-bot pushed a commit that referenced this issue Sep 12, 2021
PR-URL: #39987
Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
nodejs-github-bot pushed a commit that referenced this issue Sep 12, 2021
PR-URL: #39987
Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
nodejs-github-bot pushed a commit that referenced this issue Sep 12, 2021
PR-URL: #39987
Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
nodejs-github-bot pushed a commit that referenced this issue Sep 12, 2021
PR-URL: #39987
Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
nodejs-github-bot pushed a commit that referenced this issue Sep 12, 2021
PR-URL: #39987
Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
nodejs-github-bot pushed a commit that referenced this issue Sep 12, 2021
PR-URL: #39987
Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
nodejs-github-bot pushed a commit that referenced this issue Sep 12, 2021
PR-URL: #39987
Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
nodejs-github-bot pushed a commit that referenced this issue Sep 12, 2021
PR-URL: #39987
Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
nodejs-github-bot pushed a commit that referenced this issue Sep 12, 2021
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: #39987
Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
aduh95 pushed a commit that referenced this issue Sep 12, 2021
Switch the default from `ipv4first` to `verbatim` (return them exactly
as the resolver sent them to us).

PR-URL: #39987
Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
aduh95 pushed a commit that referenced this issue Sep 12, 2021
Switch the default from `ipv4first` to `verbatim` (return them exactly
as the resolver sent them to us).

PR-URL: #39987
Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Co-authored-by: treysis <treysis@gmx.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@zwhitchcox
Copy link

For all interested parties, I have implemented happy eyeballs for my company, Balena, here:

https://github.com/balena-io-modules/fetch

The code is open source, so if anyone wants to adapt it to their needs, please feel free.

If anyone would like me to make a PR to node with the code adapted for nodejs core, please let me know and how exactly it should be integrated.

As IPv4 addresses are exhausted, this will be more and more of a problem, especially on mobile networks, and especially in developing countries with large populations.

It's probably best to go ahead and prepare for it now if we can!

@zwhitchcox
Copy link

Ok, I have created another repo which implements "happy eyeballs" and will patch the http(s).Agents to default to the happy eyeballs algorithm:

https://www.npmjs.com/package/happy-eyeballs

With this package you can just add

import 'happy-eyeballs/eye-patch'

to the top of your file to default all createConnection calls to use happy eyeballs.

I will be making a PR to NodeJS core as well.

@telmich
Copy link

telmich commented Oct 1, 2021

That is amazing to see, thanks a lot, @zwhitchcox!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns Issues and PRs related to the dns subsystem.
Projects
None yet
Development

No branches or pull requests