dns: add `verbatim` option to dns.lookup() by bnoordhuis · Pull Request #14731 · nodejs/node

bnoordhuis · 2017-08-10T09:40:44Z

When true, results from the DNS resolver are passed on as-is, without
the reshuffling that Node.js otherwise does that puts IPv4 addresses
before IPv6 addresses.

Refs: #6307

Suggestions on how to test this properly welcome.

When true, results from the DNS resolver are passed on as-is, without the reshuffling that Node.js otherwise does that puts IPv4 addresses before IPv6 addresses. Refs: nodejs#6307

refack · 2017-08-10T10:25:39Z

IMHO we should also add an environment flag that changes the default, for upstream users of 3rd party packages.

tniessen · 2017-08-10T10:39:42Z

+ IPv4 addresses are placed before IPv6 addresses.
+ Default: currently `false` (addresses are reordered) but this is expected
+ to change in the not too distant future.
+ New code should use `{ verbatim: true }`.


I don't have a strong opinion on preferring verbatim: true, but shouldn't we add a runtime deprecation of the old default?

That seems like a separate question to "should this be an option". This is semver-minor (I think), and could be backported to v6.x. A runtime deprecation would be major.

Documentation-only deprecation would be better, I think. We're not likely to get rid of the default, just change it. A runtime deprecation would be too noisy.

gibfahn · 2017-08-10T12:42:29Z

IMHO we should also add an environment flag that changes the default, for upstream users of 3rd party packages.

Or a command line flag that works with NODE_OPTIONS.

cjihrig

LGTM

Suggestions on how to test this properly welcome.

Could the new DNS functionality in test/common/dns.js be used?

bnoordhuis · 2017-08-10T14:54:58Z

Could the new DNS functionality in test/common/dns.js be used?

I don't think so. Node doesn't have control over what getaddrinfo(3) calls out to. I did look into it but I couldn't see a way to make that work that didn't involve root access and stomping all over files in /etc. :-)

gibfahn · 2017-08-10T16:50:26Z

I don't think so. Node doesn't have control over what getaddrinfo(3) calls out to. I did look into it but I couldn't see a way to make that work that didn't involve root access and stomping all over files in /etc. :-)

I guess we could monkey-patch the getaddrinfo(3) call sites to use a mocked up version, but that'd require recompiling node right?

bnoordhuis · 2017-08-10T17:46:44Z

Yes. The call site is in libuv so yeah.

refack · 2017-08-15T12:12:52Z

 const cares = process.binding('cares_wrap');
 const req = new cares.GetAddrInfoReqWrap();
-cares.getaddrinfo(req, 'nodejs.org', 4);
+cares.getaddrinfo(req, 'nodejs.org', 4, /* hints */ 0, /* verbatim */ true);


You could compare results to python -c "import socket ; a = socket.getaddrinfo('nodejs.org', None); print(a)"

That assumes python's implementation is exactly the same as node's, though. If it's not, or worse, not always, it's going to be a right pain in the neck to debug.
If you still think it's a good idea, can you file a separate issue to hash out the details?

bnoordhuis · 2017-08-17T12:19:08Z

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

jasnell · 2017-08-23T20:27:39Z

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

When true, results from the DNS resolver are passed on as-is, without the reshuffling that Node.js otherwise does that puts IPv4 addresses before IPv6 addresses. PR-URL: #14731 Ref: #6307 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2017-08-23T21:55:54Z

Landed in e007f66

When true, results from the DNS resolver are passed on as-is, without the reshuffling that Node.js otherwise does that puts IPv4 addresses before IPv6 addresses. PR-URL: nodejs/node#14731 Ref: nodejs/node#6307 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

When true, results from the DNS resolver are passed on as-is, without the reshuffling that Node.js otherwise does that puts IPv4 addresses before IPv6 addresses. PR-URL: #14731 Ref: #6307 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2017-09-12T12:31:55Z

Is this semver minor?

bnoordhuis · 2017-09-12T13:19:08Z

@MylesBorins Yes, semver-minor.

Notable Changes * build: * Snapshots are now re-enabled in V8 #14875 * console: * Implement minimal `console.group()`. #14910 * deps: * upgrade libuv to 1.14.1 #14866 * update nghttp2 to v1.25.0 #14955 * dns: * Add `verbatim` option to dns.lookup(). When true, results from the DNS resolver are passed on as-is, without the reshuffling that Node.js otherwise does that puts IPv4 addresses before IPv6 addresses. #14731 * fs: * add fs.copyFile and fs.copyFileSync which allows for more efficient copying of files. #15034 * inspector: * Enable async stack traces #13870 * module: * Add support for ESM. This is currently behind the `--experimental-modules` flag and requires the .mjs extension. `node --experimental-modules index.mjs` #14369 * napi: * implement promise #14365 * os: * Add support for CIDR notation to the output of the networkInterfaces() method. #14307 * perf_hooks: * An initial implementation of the Performance Timing API for Node.js. This is the same Performance Timing API implemented by modern browsers with a number of Node.js specific properties. The User Timing mark() and measure() APIs are implemented, as is a Node.js specific flavor of the Frame Timing for measuring event loop duration. #14680 * tls: * multiple PFX in createSecureContext [#14793](#14793) * Added new collaborators: * BridgeAR – Ruben Bridgewater PR-URL: #15308

Notable Changes * build: * Snapshots are now re-enabled in V8 nodejs#14875 * console: * Implement minimal `console.group()`. nodejs#14910 * deps: * upgrade libuv to 1.14.1 nodejs#14866 * update nghttp2 to v1.25.0 nodejs#14955 * dns: * Add `verbatim` option to dns.lookup(). When true, results from the DNS resolver are passed on as-is, without the reshuffling that Node.js otherwise does that puts IPv4 addresses before IPv6 addresses. nodejs#14731 * fs: * add fs.copyFile and fs.copyFileSync which allows for more efficient copying of files. nodejs#15034 * inspector: * Enable async stack traces nodejs#13870 * module: * Add support for ESM. This is currently behind the `--experimental-modules` flag and requires the .mjs extension. `node --experimental-modules index.mjs` nodejs#14369 * napi: * implement promise nodejs#14365 * os: * Add support for CIDR notation to the output of the networkInterfaces() method. nodejs#14307 * perf_hooks: * An initial implementation of the Performance Timing API for Node.js. This is the same Performance Timing API implemented by modern browsers with a number of Node.js specific properties. The User Timing mark() and measure() APIs are implemented, as is a Node.js specific flavor of the Frame Timing for measuring event loop duration. nodejs#14680 * tls: * multiple PFX in createSecureContext [nodejs#14793](nodejs#14793) * Added new collaborators: * BridgeAR – Ruben Bridgewater PR-URL: nodejs#15308

Added flag with NODE_OPTIONS whitelist to allow preserving lookup order as retrieved from resolver. Still allows individual lookup requests with verbatim flag assigned explicitly to override the config flag. Open for suggestions on how to craft a test for this. Refs: nodejs#14731 (comment)

gibfahn · 2018-01-15T21:47:31Z

Release team decided not to land on v6.x, if you disagree let us know.

bnoordhuis · 2018-01-17T11:10:02Z

@gibfahn I think it would be useful to have this in v6.x. The plan is to make {verbatim: true} the default in the future. Having this in v6.x allows for easier forward-portability of user code.

gibfahn · 2018-01-18T16:58:56Z

cc/ @nodejs/lts , what do you think? If we're going to include this we should decide before next Tuesday.

MylesBorins · 2018-01-22T17:46:29Z

If you think it is useful and can open a backport I don't object to it landing

…

On Jan 18, 2018 11:59 AM, "Gibson Fahnestock" ***@***.***> wrote: cc/ @nodejs/lts <https://github.com/orgs/nodejs/teams/lts> , what do you think? If we're going to include this we should decide before next Tuesday. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14731 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV07zy_g29SK_hJX6MG_VX-Fnme3Fks5tL3hZgaJpZM4OzLuW> .

Refs: nodejs#14731

PR-URL: #22949 Refs: #14731 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

dns: add verbatim option to dns.lookup()
320fb82
When true, results from the DNS resolver are passed on as-is, without the reshuffling that Node.js otherwise does that puts IPv4 addresses before IPv6 addresses. Refs: nodejs#6307

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. dns Issues and PRs related to the dns subsystem. labels Aug 10, 2017

bnoordhuis mentioned this pull request Aug 10, 2017
order of getaddrinfo results not respected #6307
Closed

refack approved these changes Aug 10, 2017
View reviewed changes

tniessen reviewed Aug 10, 2017
View reviewed changes

cjihrig approved these changes Aug 10, 2017
View reviewed changes

jasnell approved these changes Aug 10, 2017
View reviewed changes

refack reviewed Aug 15, 2017
View reviewed changes

jasnell added the notable-change PRs with changes that should be highlighted in changelogs. label Aug 23, 2017

jasnell closed this Aug 23, 2017

MylesBorins mentioned this pull request Sep 10, 2017
v8.5.0 proposal #15308
Merged

bnoordhuis deleted the dns-lookup-verbatim branch September 12, 2017 13:18

MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 12, 2017

This was referenced Oct 1, 2017
Error after Update node 8.6.0 #15711
Closed
Update to new cares_wrap.getaddrinfo() interface. agnat/node_mdns#200
Merged

igalic mentioned this pull request Oct 23, 2017
npm should use the new verbatim flag for dns queries npm/npm#18941
Open

asbachb mentioned this pull request Dec 17, 2017
ipv4 is shown in tracker also when ipv6 was specified during setup HorizenOfficial/nodetracker#25
Closed

jkantr mentioned this pull request Dec 20, 2017
dns, src: add --preserve-dns-order flag with NODE_OPTIONS whitelist #17793
Closed
4 tasks

asbachb mentioned this pull request Dec 21, 2017
verbatim flag for dns resolution does not change order #17802
Closed

gibfahn added dont-land-on-v6.x labels Jan 15, 2018

gibfahn added the lts-agenda label Jan 22, 2018

gibfahn added backport-requested-v6.x and removed dont-land-on-v6.x labels Jan 23, 2018

petrosagg mentioned this pull request Mar 5, 2018
IPv6 support balena-os/balena-supervisor#557
Closed

tniessen mentioned this pull request Sep 19, 2018
how to do feature detection #22585
Closed

tniessen added a commit to tniessen/node that referenced this pull request Sep 19, 2018
doc: add missing metadata for dns.lookup
a3dfdd3
Refs: nodejs#14731

tniessen mentioned this pull request Sep 19, 2018
doc: add missing metadata #22949
Closed
3 tasks

Uh oh!

Uh oh!

Conversation

bnoordhuis commented Aug 10, 2017

Uh oh!

refack commented Aug 10, 2017

Uh oh!

tniessenAug 10, 2017

Choose a reason for hiding this comment

Uh oh!

gibfahnAug 10, 2017

Choose a reason for hiding this comment

Uh oh!

jasnellAug 10, 2017

Choose a reason for hiding this comment

Uh oh!

gibfahn commented Aug 10, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Aug 10, 2017

Uh oh!

gibfahn commented Aug 10, 2017

Uh oh!

bnoordhuis commented Aug 10, 2017

Uh oh!

refackAug 15, 2017

Choose a reason for hiding this comment

Uh oh!

bnoordhuisAug 17, 2017

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Aug 17, 2017

Uh oh!

jasnell commented Aug 23, 2017

Uh oh!

jasnell commented Aug 23, 2017

Uh oh!

MylesBorins commented Sep 12, 2017

Uh oh!

bnoordhuis commented Sep 12, 2017

Uh oh!

gibfahn commented Jan 15, 2018

Uh oh!

bnoordhuis commented Jan 17, 2018

Uh oh!

gibfahn commented Jan 18, 2018

Uh oh!

MylesBorins commented Jan 22, 2018 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants