dns: default to verbatim=true in dns.lookup() by bnoordhuis · Pull Request #31567 · nodejs/node

bnoordhuis · 2020-01-29T12:16:57Z

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

Passes make test on my machine but something tells me the CI results won't be so pretty...

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

nodejs-github-bot · 2020-01-29T12:18:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28759/

addaleax · 2020-01-31T21:13:12Z

Looks like this causes a significant number of failures in CI...

bnoordhuis · 2020-02-01T10:09:42Z

Yep, I didn't expect anything less. I'll be working on fixing up the tests in the next few days.

benschulz · 2020-02-01T11:21:03Z

@bnoordhuis, I'm the original reporter of #6307. I saw the recent activity there and wanted to say that I appreciate all the work that went into this and is still going into it. Thank you.

jasnell · 2020-06-25T23:22:50Z

Ping @bnoordhuis

github-actions · 2020-10-19T18:09:28Z

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

telmich · 2020-10-20T08:31:05Z

Ping. What is blocking this to be merged?

treysis · 2021-01-21T13:54:12Z

@telmich Maybe the CI does not support IPv6. but running the code with verbatim=true will make it try to connect to an IPv6 address, which of course fails?

richfelker · 2021-01-25T15:18:39Z

The fix to keep it verbatim might not even be enough for MUSL libc based systems, see libuv/libuv#2225.

I don't see any reason to believe that. The musl getaddrinfo (and any getaddrinfo implementation aiming to conform to RFC 3484) sorts results by routability preference, so the first result should always be routable if any of them are. There are still good reasons to perform the proper fallback sequence through the results (first server of multihomed site may be down) but basic functionality should be fixed as soon as node stops undoing the correct RFC 3484 sorting.

telmich · 2021-01-25T21:03:41Z

@richfelker Interesting. As far as I understood the musl getaddrinfo implementation orientates itself on https://pubs.opengroup.org/onlinepubs/9699919799/functions/getaddrinfo.html.

So practically speaking, there are probably 2 fixes necessary:

Don't reorder by default -> let the resolver library do their job
For connect()-alike methods, try all results, not just the first one

Do I see this correctly?

richfelker · 2021-01-25T22:34:06Z

@telmich: Either (1) or (2) should solve the immediate problem. Both (1) and (2) should be done for independent reasons. (1) ensures you try the best results first, and (2) ensures that you don't spuriously fail from the first result being momentarily down or unreachable.

telmich · 2021-01-28T15:56:43Z

Is there anything that I can do to help merging this patch, @richfelker / @jasnell ?

aduh95 · 2021-03-09T18:50:18Z

Is there anything that I can do to help merging this patch?

I think the way forward is to open a new PR picking up the changes in this one, and try to solve the CI failures.

richfelker · 2021-03-09T19:46:15Z

What is wrong with the current PR? Why can't it just be merged?

ljharb · 2021-03-09T19:49:43Z

@richfelker tests are failing; merging it would cause tests to fail in master.

richfelker · 2021-03-09T20:07:27Z

Well is something wrong with this change, or are the failing tests just wrong/invalid? I don't know how to look at what the tests in question actually are, but from their names it sounds like maybe the problem is that they're being run in a misconfigured environment where getaddrinfo produces v6 results first but v6 is blocked or not functional.

aduh95 · 2021-03-09T21:17:57Z

is something wrong with this change, or are the failing tests just wrong/invalid?

Either way, for the tests to land on master we need to have a green CI. Help is needed to investigate why the tests are failing, and how to make them pass.

it sounds like maybe the problem is that they're being run in a misconfigured environment where getaddrinfo produces v6 results first but v6 is blocked or not functional.

If someone knows how to make this work, please send a PR. I believe our CI machine configuration files are located at nodejs/build.

treysis · 2021-03-09T21:22:14Z

And where are the log files?

aduh95 · 2021-03-09T21:34:20Z

Last CI run was back in January 2020, unfortunately the log files are gone. We would need to first resolve the git conflict to spawn a new CI job for this PR – and it seems the original author is not interested in working on it anymore. If someone is interested to open a new PR with these changes to solve the conflict, we could spawn a CI job and move forward with this.

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 Reissue of nodejs#31567

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 Reissue of nodejs#31567 Reissue of nodejs#37681

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

aduh95 · 2021-09-12T19:18:02Z

Superseded by 1b2749e.

dns: default to verbatim=true in dns.lookup()
7ad4989
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

nodejs-github-bot added the dns Issues and PRs related to the dns subsystem. label Jan 29, 2020

fixup! update pr-url
7d81200

bnoordhuis mentioned this pull request Jan 29, 2020
order of getaddrinfo results not respected #6307
Closed

addaleax approved these changes Jan 29, 2020
View reviewed changes

bnoordhuis mentioned this pull request Jan 29, 2020
dns: default to verbatim=true in dns.lookup() #20710
Closed

bnoordhuis added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 29, 2020

devnexen approved these changes Jan 29, 2020
View reviewed changes

santigimeno approved these changes Jan 29, 2020
View reviewed changes

cjihrig approved these changes Jan 29, 2020
View reviewed changes

lpinca approved these changes Jan 29, 2020
View reviewed changes

jasnell approved these changes Jan 29, 2020
View reviewed changes

Trott approved these changes Jan 31, 2020
View reviewed changes

kenpowers-signal mentioned this pull request Apr 2, 2020
Signal desktop app does not work in IPv6-only environment. signalapp/Signal-Desktop#4121
Closed

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72Compare May 31, 2020 12:19

jasnell added the stalled Issues and PRs that are stalled. label Jun 25, 2020

devsnek approved these changes Jun 25, 2020
View reviewed changes

bnoordhuis requested a review from a team as a code owner August 10, 2020 16:07

aduh95 added stalled Issues and PRs that are stalled. and removed stalled Issues and PRs that are stalled. labels Oct 19, 2020

NeverBehave mentioned this pull request Dec 24, 2020
npm fails to install on IPv6 only networks npm/cli#348
Closed

shawaj mentioned this pull request Feb 1, 2021
Metadata issue? NebraLtd/Helium-Guides#20
Closed

AllanChain mentioned this pull request Feb 1, 2021
NPM Registry: What If I Cannot Reach It? AllanChain/blog#160
Open

treysis mentioned this pull request Mar 9, 2021
dns: default to verbatim=true in dns.lookup() #37681
Closed

treysis mentioned this pull request Mar 26, 2021
dns: default to verbatim=true in dns.lookup() #37931
Closed

guybedford force-pushed the master branch from dc5a5da to 8e46568Compare March 29, 2021 21:33

treysis mentioned this pull request Sep 3, 2021
dns: default to verbatim=true in dns.lookup() #39987
Closed

aduh95 closed this Sep 12, 2021

Uh oh!

Uh oh!

Conversation

bnoordhuis commented Jan 29, 2020

Uh oh!

nodejs-github-bot commented Jan 29, 2020

Uh oh!

addaleax commented Jan 31, 2020

Uh oh!

bnoordhuis commented Feb 1, 2020

Uh oh!

benschulz commented Feb 1, 2020

Uh oh!

jasnell commented Jun 25, 2020

Uh oh!

github-actionsBot commented Oct 19, 2020

Uh oh!

telmich commented Oct 20, 2020

Uh oh!

treysis commented Jan 21, 2021

Uh oh!

richfelker commented Jan 25, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

telmich commented Jan 25, 2021

Uh oh!

richfelker commented Jan 25, 2021

Uh oh!

telmich commented Jan 28, 2021

Uh oh!

aduh95 commented Mar 9, 2021

Uh oh!

richfelker commented Mar 9, 2021

Uh oh!

ljharb commented Mar 9, 2021

Uh oh!

richfelker commented Mar 9, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Mar 9, 2021

Uh oh!

treysis commented Mar 9, 2021

Uh oh!

aduh95 commented Mar 9, 2021

Uh oh!

aduh95 commented Sep 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

16 participants

richfelker commented Jan 25, 2021•
edited
Loading

richfelker commented Mar 9, 2021•
edited
Loading