dns, src: add --preserve-dns-order flag with NODE_OPTIONS whitelist

[jkantr]

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.
Need discussion for whether this should be added to --help output
I will follow up with commit for documentation changes for dns.lookup(),
but not sure where else might require updating.

Refs: #14731 (comment)

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

dns, src

[richardlau]

The documented whitelisted options for NODE_OPTIONS located in doc/api/cli.md should also be updated. Probably also test/parallel/test-cli-node-options.js.

[richardlau]

Most Node.js command line options only support the - variants (i.e. --preserve-dns-order). As this stands the other variant, --preserve_dns_order, would be allowed on the command line but not in NODE_OPTIONS.

[richardlau]

Need discussion for whether this should be added to --help output

Note the comments for the whitelist:

// Node options, sorted in `node --help` order for ease of comparison.

so if it's decided not to add this option to --help then this option should be moved into another commented block (e.g. // Node options, not listed in 'node --help') before the V8 options.

[bnoordhuis]

I strongly dislike adding flags that override default behavior of API methods. I would have added it in the original PR if I thought a flag was a good idea.

Strictly speaking it's not needed either because it can be monkey-patched with -r preload-script.js. The -r option is whitelisted for use in NODE_OPTIONS.

[jasnell]

I can see the rationale for this but I'm also leaning against it for the same reason as @bnoordhuis. Specifically, providing command line args that override core API behavior is a bit of an anti-pattern. We've been forced to do it in a few other cases but there was strong reluctance there also. Using a preload script is a very under-appreciated and not-well-understood approach to handling the same issue.

[asbachb]

I can understand that you'd argue that having such args is kind of a anti-pattern since it switches default node behaviour.

But on the other hand there should be a mechanism to switch dns resolution to an RFC3484 and host resolution compliant way without explicitily passing the family within all layers of libraries/application.

The default policy table gives IPv6 addresses higher precedence than
IPv4 addresses. This means that applications will use IPv6 in
preference to IPv4 when the two are equally suitable.
(see https://tools.ietf.org/html/rfc3484#section-10.3)

At least on linux glibc the dns reordering is already done by getaddrinfo based on ipv6 interface availability.

[bnoordhuis]

The plan is to make verbatim: true the default in the near future, maybe even in node 10.

[asbachb]

Even if it lands in node 10 which is planned for Q2/2018 (when I get it right) there's
a) No usable short term solution
b) No solution for LTS version at all

So couldn't be the short term solution:
a) Merge flag into v8/v9
b) Remove flag when switching to verbatim: default in v10

[jkantr]

May I ask... is the idea to conform to RFC3484 based on an enhancement, or just a behavior change to match the adoption of an emerging technology? In other words... might that behavior adjustment itself be backport-able?

[asbachb]

Is there any alternative for this patch?

@bnoordhuis mentions some monkey patching. I'm a little bit unsure if/how it's easily possible.

[bnoordhuis]

@asbachb Pretty easy:

// Run like this: `node -r ./preload.js app.js`
'use strict';
const dns = require('dns'), {lookup} = dns;
dns.lookup = function(name, opts, cb) {
  if (typeof cb !== 'function') return lookup(name, {verbatim:true}, opts);
  return lookup(name, Object.assign({verbatim:true}, opts), cb);
};

[BridgeAR]

Ping @jkantr

[jkantr]

@BridgeAR yep, still here. I can rewrite this PR but it seems that monkey patching dns for now until the behavior of verbatim is adjusted in a future version is what was being leaned toward by those with considerably greater experience than me :)

In fact we should probably close this unless there's more to be discussed? @bnoordhuis

[bnoordhuis]

I'll close it out. Thank you for taking the time to put together a pull request.