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

dns: allow --dns-verbatim to change default dns verbatim #38099

Merged
merged 1 commit into from
Jun 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,19 @@ Make built-in language features like `eval` and `new Function` that generate
code from strings throw an exception instead. This does not affect the Node.js
`vm` module.

### `--dns-result-order=order`
<!-- YAML
added: REPLACEME
-->

Set the default value of `verbatim` in [`dns.lookup()`][] and
[`dnsPromises.lookup()`][]. The value could be:
* `ipv4first`: sets default `verbatim` `false`.
* `verbatim`: sets default `verbatim` `true`.

The default is `ipv4first` and [`dns.setDefaultResultOrder()`][] have higher
priority than `--dns-result-order`.

### `--enable-fips`
<!-- YAML
added: v6.0.0
Expand Down Expand Up @@ -1379,6 +1392,7 @@ Node.js options that are allowed are:
* `--conditions`, `-C`
* `--diagnostic-dir`
* `--disable-proto`
* `--dns-result-order`
* `--enable-fips`
* `--enable-source-maps`
* `--experimental-abortcontroller`
Expand Down Expand Up @@ -1767,6 +1781,9 @@ $ node --max-old-space-size=1536 index.js
[`NODE_OPTIONS`]: #cli_node_options_options
[`NO_COLOR`]: https://no-color.org
[`SlowBuffer`]: buffer.md#buffer_class_slowbuffer
[`dns.lookup()`]: dns.md#dns_dns_lookup_hostname_options_callback
[`dns.setDefaultResultOrder()`]: dns.md#dns_dns_setdefaultresultorder_order
[`dnsPromises.lookup()`]: dns.md#dns_dnspromises_lookup_hostname_options
[`process.setUncaughtExceptionCaptureCallback()`]: process.md#process_process_setuncaughtexceptioncapturecallback_fn
[`tls.DEFAULT_MAX_VERSION`]: tls.md#tls_tls_default_max_version
[`tls.DEFAULT_MIN_VERSION`]: tls.md#tls_tls_default_min_version
Expand Down
48 changes: 44 additions & 4 deletions doc/api/dns.md
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,9 @@ changes:
addresses in the order the DNS resolver returned them. When `false`,
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 }`.
expected to change in the not too distant future. Default value is
configurable using [`dns.setDefaultResultOrder()`][] or
[`--dns-result-order`][]. New code should use `{ verbatim: true }`.
* `callback` {Function}
* `err` {Error}
* `address` {string} A string representation of an IPv4 or IPv6 address.
Expand Down Expand Up @@ -633,6 +634,23 @@ array of host names.
On error, `err` is an [`Error`][] object, where `err.code` is
one of the [DNS error codes][].

## `dns.setDefaultResultOrder(order)`
<!-- YAML
added: REPLACEME
-->

* `order` {string} must be `'ipv4first'` or `'verbatim'`.

Set the default value of `verbatim` in [`dns.lookup()`][] and
[`dnsPromises.lookup()`][]. The value could be:
* `ipv4first`: sets default `verbatim` `false`.
* `verbatim`: sets default `verbatim` `true`.

The default is `ipv4first` and [`dns.setDefaultResultOrder()`][] have higher
priority than [`--dns-result-order`][]. When using [worker threads][],
[`dns.setDefaultResultOrder()`][] from the main thread won't affect the default
dns orders in workers.

## `dns.setServers(servers)`
<!-- YAML
added: v0.11.3
Expand Down Expand Up @@ -783,8 +801,9 @@ added: v10.6.0
IPv6 addresses in the order the DNS resolver returned them. When `false`,
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 }`.
expected to change in the not too distant future. Default value is
configurable using [`dns.setDefaultResultOrder()`][] or
[`--dns-result-order`][]. New code should use `{ verbatim: true }`.

Resolves a host name (e.g. `'nodejs.org'`) into the first found A (IPv4) or
AAAA (IPv6) record. All `option` properties are optional. If `options` is an
Expand Down Expand Up @@ -1140,6 +1159,23 @@ array of host names.
On error, the `Promise` is rejected with an [`Error`][] object, where `err.code`
is one of the [DNS error codes](#dns_error_codes).

### `dnsPromises.setDefaultResultOrder(order)`
<!-- YAML
added: REPLACEME
-->

* `order` {string} must be `'ipv4first'` or `'verbatim'`.

Set the default value of `verbatim` in [`dns.lookup()`][] and
[`dnsPromises.lookup()`][]. The value could be:
* `ipv4first`: sets default `verbatim` `false`.
* `verbatim`: sets default `verbatim` `true`.

The default is `ipv4first` and [`dnsPromises.setDefaultResultOrder()`][] have
higher priority than [`--dns-result-order`][]. When using [worker threads][],
[`dnsPromises.setDefaultResultOrder()`][] from the main thread won't affect the
default dns orders in workers.

### `dnsPromises.setServers(servers)`
<!-- YAML
added: v10.6.0
Expand Down Expand Up @@ -1249,6 +1285,7 @@ uses. For instance, _they do not use the configuration from `/etc/hosts`_.
[Implementation considerations section]: #dns_implementation_considerations
[RFC 5952]: https://tools.ietf.org/html/rfc5952#section-6
[RFC 8482]: https://tools.ietf.org/html/rfc8482
[`--dns-result-order`]: cli.md#cli_dns_result_order_order
[`Error`]: errors.md#errors_class_error
[`UV_THREADPOOL_SIZE`]: cli.md#cli_uv_threadpool_size_size
[`dgram.createSocket()`]: dgram.md#dgram_dgram_createsocket_options_callback
Expand All @@ -1268,6 +1305,7 @@ uses. For instance, _they do not use the configuration from `/etc/hosts`_.
[`dns.resolveSrv()`]: #dns_dns_resolvesrv_hostname_callback
[`dns.resolveTxt()`]: #dns_dns_resolvetxt_hostname_callback
[`dns.reverse()`]: #dns_dns_reverse_ip_callback
[`dns.setDefaultResultOrder()`]: #dns_dns_setdefaultresultorder_order
[`dns.setServers()`]: #dns_dns_setservers_servers
[`dnsPromises.getServers()`]: #dns_dnspromises_getservers
[`dnsPromises.lookup()`]: #dns_dnspromises_lookup_hostname_options
Expand All @@ -1285,7 +1323,9 @@ uses. For instance, _they do not use the configuration from `/etc/hosts`_.
[`dnsPromises.resolveSrv()`]: #dns_dnspromises_resolvesrv_hostname
[`dnsPromises.resolveTxt()`]: #dns_dnspromises_resolvetxt_hostname
[`dnsPromises.reverse()`]: #dns_dnspromises_reverse_ip
[`dnsPromises.setDefaultResultOrder()`]: #dns_dnspromises_setdefaultresultorder_order
[`dnsPromises.setServers()`]: #dns_dnspromises_setservers_servers
[`socket.connect()`]: net.md#net_socket_connect_options_connectlistener
[`util.promisify()`]: util.md#util_util_promisify_original
[supported `getaddrinfo` flags]: #dns_supported_getaddrinfo_flags
[worker threads]: worker_threads.md
10 changes: 8 additions & 2 deletions lib/dns.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ const {
Resolver,
validateHints,
emitInvalidHostnameWarning,
getDefaultVerbatim,
setDefaultResultOrder,
} = require('internal/dns/utils');
const {
ERR_INVALID_ARG_TYPE,
Expand Down Expand Up @@ -96,7 +98,7 @@ function lookup(hostname, options, callback) {
let hints = 0;
let family = -1;
let all = false;
let verbatim = false;
let verbatim = getDefaultVerbatim();
aduh95 marked this conversation as resolved.
Show resolved Hide resolved

// Parse arguments
if (hostname) {
Expand All @@ -113,7 +115,9 @@ function lookup(hostname, options, callback) {
hints = options.hints >>> 0;
family = options.family >>> 0;
all = options.all === true;
verbatim = options.verbatim === true;
if (typeof options.verbatim === 'boolean') {
verbatim = options.verbatim === true;
}

validateHints(hints);
} else {
Expand Down Expand Up @@ -286,6 +290,7 @@ module.exports = {
lookupService,

Resolver,
setDefaultResultOrder,
setServers: defaultResolverSetServers,

// uv_getaddrinfo flags
Expand Down Expand Up @@ -330,6 +335,7 @@ ObjectDefineProperties(module.exports, {
if (promises === null) {
promises = require('internal/dns/promises');
promises.setServers = defaultResolverSetServers;
promises.setDefaultResultOrder = setDefaultResultOrder;
}
return promises;
}
Expand Down
8 changes: 5 additions & 3 deletions lib/internal/dns/promises.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
'use strict';

const {
ArrayPrototypeMap,
ObjectCreate,
Expand All @@ -14,6 +13,7 @@ const {
validateHints,
validateTimeout,
emitInvalidHostnameWarning,
getDefaultVerbatim,
} = require('internal/dns/utils');
const { codes, dnsException } = require('internal/errors');
const { toASCII } = require('internal/idna');
Expand Down Expand Up @@ -103,7 +103,7 @@ function lookup(hostname, options) {
var hints = 0;
var family = -1;
var all = false;
var verbatim = false;
var verbatim = getDefaultVerbatim();
aduh95 marked this conversation as resolved.
Show resolved Hide resolved

// Parse arguments
if (hostname && typeof hostname !== 'string') {
Expand All @@ -112,7 +112,9 @@ function lookup(hostname, options) {
hints = options.hints >>> 0;
family = options.family >>> 0;
all = options.all === true;
verbatim = options.verbatim === true;
if (typeof options.verbatim === 'boolean') {
verbatim = options.verbatim === true;
}
Comment on lines +115 to +117
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be more useful to throw if the value is not a boolean rather than ignoring it? It the value is not a boolean, it's probably a mistake that users would want to spot.

Suggested change
if (typeof options.verbatim === 'boolean') {
verbatim = options.verbatim === true;
}
if (options.verbatim !== undefined) {
validateBoolean(options.verbatim, 'options.verbatim');
verbatim = options.verbatim;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's better. But I'm afraid of introducing a breaking change here. Let's reconsider this in the future.

Copy link
Contributor

@aduh95 aduh95 Apr 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's OK to do it in a possible breaking way as long as we don't land it in v15.x. Once it has landed in v16.0.0, we can backport it to v14.x (and v12.x?) without the breaking part.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcollina wdyt? Should this land with or without argument validation (possibly breaking change)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to say I prefer the argument validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still want to keep the possibility to backport this to v12 and v14.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could still backport it without the argument validation. I volunteer to take care of the backport PR if that if that helps.


validateHints(hints);
} else {
Expand Down
21 changes: 21 additions & 0 deletions lib/internal/dns/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@ const {

const errors = require('internal/errors');
const { isIP } = require('internal/net');
const { getOptionValue } = require('internal/options');
const {
validateArray,
validateInt32,
validateOneOf,
validateString,
} = require('internal/validators');
const {
Expand Down Expand Up @@ -184,6 +186,23 @@ function emitInvalidHostnameWarning(hostname) {
);
}

let dnsOrder = getOptionValue('--dns-result-order') || 'ipv4first';

function getDefaultVerbatim() {
switch (dnsOrder) {
case 'verbatim':
return true;
case 'ipv4first':
default:
return false;
}
}

function setDefaultResultOrder(value) {
validateOneOf(value, 'dnsOrder', ['verbatim', 'ipv4first']);
dnsOrder = value;
}

module.exports = {
bindDefaultResolver,
getDefaultResolver,
Expand All @@ -192,4 +211,6 @@ module.exports = {
validateTimeout,
Resolver,
emitInvalidHostnameWarning,
getDefaultVerbatim,
setDefaultResultOrder,
};
7 changes: 7 additions & 0 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,13 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
" (default: current working directory)",
&EnvironmentOptions::diagnostic_dir,
kAllowedInEnvironment);
AddOption("--dns-result-order",
"set default value of verbatim in dns.lookup. Options are "
"'ipv4first' (IPv4 addresses are placed before IPv6 addresses) "
"'verbatim' (addresses are in the order the DNS resolver "
"returned)",
oyyd marked this conversation as resolved.
Show resolved Hide resolved
&EnvironmentOptions::dns_result_order,
kAllowedInEnvironment);
AddOption("--enable-source-maps",
"Source Map V3 support for stack traces",
&EnvironmentOptions::enable_source_maps,
Expand Down
1 change: 1 addition & 0 deletions src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ class EnvironmentOptions : public Options {
public:
bool abort_on_uncaught_exception = false;
std::vector<std::string> conditions;
std::string dns_result_order;
bool enable_source_maps = false;
bool experimental_json_modules = false;
bool experimental_modules = false;
Expand Down
51 changes: 51 additions & 0 deletions test/parallel/test-dns-default-verbatim-false.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Flags: --expose-internals --dns-result-order=ipv4first
'use strict';
const common = require('../common');
const assert = require('assert');
const { internalBinding } = require('internal/test/binding');
const cares = internalBinding('cares_wrap');
const { promisify } = require('util');

// Test that --dns-result-order=ipv4first works as expected.

const originalGetaddrinfo = cares.getaddrinfo;
const calls = [];
cares.getaddrinfo = common.mustCallAtLeast((...args) => {
calls.push(args);
originalGetaddrinfo(...args);
}, 1);

const dns = require('dns');
const dnsPromises = dns.promises;

oyyd marked this conversation as resolved.
Show resolved Hide resolved
let verbatim;

// We want to test the parameter of verbatim only so that we
// ignore possible errors here.
function allowFailed(fn) {
return fn.catch((_err) => {
//
});
}

(async () => {
let callsLength = 0;
const checkParameter = (expected) => {
assert.strictEqual(calls.length, callsLength + 1);
verbatim = calls[callsLength][4];
assert.strictEqual(verbatim, expected);
callsLength += 1;
};

await allowFailed(promisify(dns.lookup)('example.org'));
checkParameter(false);

await allowFailed(dnsPromises.lookup('example.org'));
checkParameter(false);

await allowFailed(promisify(dns.lookup)('example.org', {}));
checkParameter(false);

await allowFailed(dnsPromises.lookup('example.org', {}));
checkParameter(false);
})().then(common.mustCall());
51 changes: 51 additions & 0 deletions test/parallel/test-dns-default-verbatim-true.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Flags: --expose-internals --dns-result-order=verbatim
'use strict';
const common = require('../common');
const assert = require('assert');
const { internalBinding } = require('internal/test/binding');
const cares = internalBinding('cares_wrap');
const { promisify } = require('util');

// Test that --dns-result-order=verbatim works as expected.

const originalGetaddrinfo = cares.getaddrinfo;
const calls = [];
cares.getaddrinfo = common.mustCallAtLeast((...args) => {
calls.push(args);
originalGetaddrinfo(...args);
}, 1);

const dns = require('dns');
const dnsPromises = dns.promises;

let verbatim;

// We want to test the parameter of verbatim only so that we
// ignore possible errors here.
function allowFailed(fn) {
return fn.catch((_err) => {
//
});
}

(async () => {
let callsLength = 0;
const checkParameter = (expected) => {
assert.strictEqual(calls.length, callsLength + 1);
verbatim = calls[callsLength][4];
assert.strictEqual(verbatim, expected);
callsLength += 1;
};

await allowFailed(promisify(dns.lookup)('example.org'));
checkParameter(true);

await allowFailed(dnsPromises.lookup('example.org'));
checkParameter(true);

await allowFailed(promisify(dns.lookup)('example.org', {}));
checkParameter(true);

await allowFailed(dnsPromises.lookup('example.org', {}));
checkParameter(true);
})().then(common.mustCall());
Loading