Repository navigation
order of getaddrinfo results not respected #6307
Description
Activity
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
Reacted by Ray Bellis, Christian, Tristan, Maurice Walker, treysis, Fernando van Loenhout, Miyuru, Paul-Louis Ageneau, Lukáš Kvídera, ronnyaa and 1 moreIs this still a problem when using the latest v6 release candidate (aka is this fixed by #6021)?
The DNS hints aren't related to the reordering.
@bnoordhuis would the
dns.ADDRCONFIGflag mitigate the problem?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 100would the dns.ADDRCONFIG flag mitigate the problem?
Only partially.
AI_ADDRCONFIGmeans '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 thatAI_ADDRCONFIGisn'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.
Reacted by Marco Davids and treysisThere 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.
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.
Reacted by treysisfrom what i gather, this is possibly the source of my issue with
npm installfailing (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 {}; }
Reacted by MiyuruThat 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.
Reacted by treysisHappy Eyeballs - for those who haven't heard the term
Reacted by Refael Ackermann, Anton Rudeshko, marka63, Christian, Xavier Ruiz and Dan Gevery 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+
Reacted by Marco DavidsShould this remain open? (I'm guessing the answer is "yes" but would be happy to find out I'm wrong.)
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. =)
Reacted by Refael Ackermann, Anton Rudeshko, treysis and ronnyaa77 remaining items
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!
Reacted by Émilien (perso) and Marco DavidsOk, 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
createConnectioncalls to use happy eyeballs.I will be making a PR to NodeJS core as well.
Reacted by Émilien (perso), Marco Davids and Jay StewartReacted by treysis and Arber XReacted by treysis and Dan GThat is amazing to see, thanks a lot, @zwhitchcox!
Reacted by Zane Hitchcox, Émilien (perso), Marco Davids and Suraj
The results returned by
getaddrinfoare 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.