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.

Sign up for GitHub

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

Jump to bottom

dns: remove dns.lookup and dnsPromises.lookup options type coercion #41431

Merged
merged 2 commits into from
Apr 12, 2022
+161 −122
Merged

dns: remove dns.lookup and dnsPromises.lookup options type coercion #41431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3a26db9
net: make `server.address()` return an integer for `family`
aduh95 Jan 7, 2022
08773e3
dns: remove `dns.lookup` and `dnsPromises.lookup` options type coercion
aduh95 Jan 7, 2022
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
8 changes: 6 additions & 2 deletions doc/api/deprecations.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2943,6 +2943,9 @@ deprecated and should no longer be used.

<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/41431
description: End-of-Life.
- version: v17.0.0
pr-url: https://github.com/nodejs/node/pull/39793
description: Runtime deprecation.
Expand All @@ -2951,12 +2954,13 @@ changes:
description: Documentation-only deprecation.
-->

Type: Runtime
Type: End-of-Life

Using a non-nullish non-integer value for `family` option, a non-nullish
non-number value for `hints` option, a non-nullish non-boolean value for `all`
option, or a non-nullish non-boolean value for `verbatim` option in
[`dns.lookup()`][] and [`dnsPromises.lookup()`][] is deprecated.
[`dns.lookup()`][] and [`dnsPromises.lookup()`][] throws an
`ERR_INVALID_ARG_TYPE` error.

### DEP0154: RSA-PSS generate key pair options

Expand Down
55 changes: 27 additions & 28 deletions lib/dns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ const {
Resolver,
validateHints,
emitInvalidHostnameWarning,
emitTypeCoercionDeprecationWarning,
getDefaultVerbatim,
setDefaultResultOrder,
} = require('internal/dns/utils');
Expand All @@ -52,10 +51,12 @@ const {
ERR_MISSING_ARGS,
} = errors.codes;
const {
validateBoolean,
validateFunction,
validateNumber,
validateOneOf,
validatePort,
validateString,
validateOneOf,
} = require('internal/validators');

const {
Expand Down Expand Up @@ -107,9 +108,10 @@ function onlookupall(err, addresses) {

// Easy DNS A/AAAA look up
// lookup(hostname, [options,] callback)
const validFamilies = [0, 4, 6];
function lookup(hostname, options, callback) {
let hints = 0;
let family = -1;
let family = 0;
let all = false;
let verbatim = getDefaultVerbatim();

Expand All @@ -121,39 +123,36 @@ function lookup(hostname, options, callback) {
if (typeof options === 'function') {
callback = options;
family = 0;
} else if (typeof options === 'number') {
validateFunction(callback, 'callback');

validateOneOf(options, 'family', validFamilies, true);
family = options;
} else if (options !== undefined && typeof options !== 'object') {
validateFunction(arguments.length === 2 ? options : callback, 'callback');
throw new ERR_INVALID_ARG_TYPE('options', ['integer', 'object'], options);
} else {
validateFunction(callback, 'callback');

if (options !== null && typeof options === 'object') {
if (options.hints != null && typeof options.hints !== 'number') {
emitTypeCoercionDeprecationWarning();
}
if (options?.hints != null) {
validateNumber(options.hints, 'options.hints');
hints = options.hints >>> 0;
if (options.family != null && typeof options.family !== 'number') {
emitTypeCoercionDeprecationWarning();
}
family = options.family >>> 0;
if (options.all != null && typeof options.all !== 'boolean') {
emitTypeCoercionDeprecationWarning();
}
all = options.all === true;
if (typeof options.verbatim === 'boolean') {
verbatim = options.verbatim === true;
} else if (options.verbatim != null) {
emitTypeCoercionDeprecationWarning();
}

validateHints(hints);
} else {
if (options != null && typeof options !== 'number') {
emitTypeCoercionDeprecationWarning();
}
family = options >>> 0;
}
if (options?.family != null) {
validateOneOf(options.family, 'options.family', validFamilies, true);
family = options.family;
}
if (options?.all != null) {
validateBoolean(options.all, 'options.all');
all = options.all;
}
if (options?.verbatim != null) {
validateBoolean(options.verbatim, 'options.verbatim');
verbatim = options.verbatim;
}
}

validateOneOf(family, 'family', [0, 4, 6]);

if (!hostname) {
emitInvalidHostnameWarning(hostname);
if (all) {
Expand Down
51 changes: 25 additions & 26 deletions lib/internal/dns/promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ const {
validateTimeout,
validateTries,
emitInvalidHostnameWarning,
emitTypeCoercionDeprecationWarning,
getDefaultVerbatim,
} = require('internal/dns/utils');
const { codes, dnsException } = require('internal/errors');
Expand All @@ -30,13 +29,16 @@ const {
QueryReqWrap
} = internalBinding('cares_wrap');
const {
ERR_INVALID_ARG_TYPE,
ERR_INVALID_ARG_VALUE,
ERR_MISSING_ARGS,
} = codes;
const {
validateBoolean,
validateNumber,
validateOneOf,
validatePort,
validateString,
validateOneOf,
} = require('internal/validators');

const kPerfHooksDnsLookupContext = Symbol('kPerfHooksDnsLookupContext');
Expand Down Expand Up @@ -120,9 +122,10 @@ function createLookupPromise(family, hostname, all, hints, verbatim) {
});
}

const validFamilies = [0, 4, 6];
function lookup(hostname, options) {
let hints = 0;
let family = -1;
let family = 0;
let all = false;
let verbatim = getDefaultVerbatim();

Expand All @@ -131,35 +134,31 @@ function lookup(hostname, options) {
validateString(hostname, 'hostname');
}

if (options !== null && typeof options === 'object') {
if (options.hints != null && typeof options.hints !== 'number') {
emitTypeCoercionDeprecationWarning();
if (typeof options === 'number') {
validateOneOf(options, 'family', validFamilies, true);
family = options;
} else if (options !== undefined && typeof options !== 'object') {
throw new ERR_INVALID_ARG_TYPE('options', ['integer', 'object'], options);
} else {
if (options?.hints != null) {
validateNumber(options.hints, 'options.hints');
hints = options.hints >>> 0;
validateHints(hints);
}
hints = options.hints >>> 0;
if (options.family != null && typeof options.family !== 'number') {
emitTypeCoercionDeprecationWarning();
if (options?.family != null) {
validateOneOf(options.family, 'options.family', validFamilies, true);
family = options.family;
}
family = options.family >>> 0;
if (options.all != null && typeof options.all !== 'boolean') {
emitTypeCoercionDeprecationWarning();
if (options?.all != null) {
validateBoolean(options.all, 'options.all');
all = options.all;
}
all = options.all === true;
if (typeof options.verbatim === 'boolean') {
verbatim = options.verbatim === true;
} else if (options.verbatim != null) {
emitTypeCoercionDeprecationWarning();
if (options?.verbatim != null) {
validateBoolean(options.verbatim, 'options.verbatim');
verbatim = options.verbatim;
}

validateHints(hints);
} else {
if (options != null && typeof options !== 'number') {
emitTypeCoercionDeprecationWarning();
}
family = options >>> 0;
}

validateOneOf(family, 'family', [0, 4, 6], true);

return createLookupPromise(family, hostname, all, hints, verbatim);
}

Expand Down
13 changes: 0 additions & 13 deletions lib/internal/dns/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,18 +190,6 @@ function emitInvalidHostnameWarning(hostname) {
}
}

let typeCoercionWarningEmitted = false;
function emitTypeCoercionDeprecationWarning() {
if (!typeCoercionWarningEmitted) {
process.emitWarning(
'Type coercion of dns.lookup options is deprecated',
'DeprecationWarning',
'DEP0153'
);
typeCoercionWarningEmitted = true;
}
}

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

function getDefaultVerbatim() {
Expand All @@ -222,7 +210,6 @@ module.exports = {
validateTries,
Resolver,
emitInvalidHostnameWarning,
emitTypeCoercionDeprecationWarning,
getDefaultVerbatim,
setDefaultResultOrder,
};
2 changes: 1 addition & 1 deletion lib/net.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -760,7 +760,7 @@ protoGetter('remoteAddress', function remoteAddress() {
});

protoGetter('remoteFamily', function remoteFamily() {
return this._getpeername().family;
return `IPv${this._getpeername().family}`;
});

protoGetter('remotePort', function remotePort() {
Expand Down
4 changes: 2 additions & 2 deletions lib/os.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ function getCIDR(address, netmask, family) {
let groupLength = 8;
let hasZeros = false;

if (family === 'IPv6') {
if (family === 6) {
split = ':';
range = 16;
groupLength = 16;
Expand Down Expand Up @@ -248,7 +248,7 @@ function getCIDR(address, netmask, family) {
* @returns {Record<string, Array<{
* address: string
* netmask: string
* family: 'IPv4' | 'IPv6'
* family: 4 | 6
* mac: string
* internal: boolean
* scopeid: number
Expand Down
2 changes: 0 additions & 2 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,8 +289,6 @@ constexpr size_t kFsStatsBufferLength =
V(input_string, "input") \
V(internal_binding_string, "internalBinding") \
V(internal_string, "internal") \
V(ipv4_string, "IPv4") \
V(ipv6_string, "IPv6") \
V(isclosing_string, "isClosing") \
V(issuer_string, "issuer") \
V(issuercert_string, "issuerCertificate") \
Expand Down
9 changes: 5 additions & 4 deletions src/node_os.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,8 @@ static void GetInterfaceAddresses(const FunctionCallbackInfo<Value>& args) {
char ip[INET6_ADDRSTRLEN];
char netmask[INET6_ADDRSTRLEN];
std::array<char, 18> mac;
Local<String> name, family;
Local<String> name;
Local<Integer> family;

int err = uv_interface_addresses(&interfaces, &count);

Expand Down Expand Up @@ -214,14 +215,14 @@ static void GetInterfaceAddresses(const FunctionCallbackInfo<Value>& args) {
if (interfaces[i].address.address4.sin_family == AF_INET) {
uv_ip4_name(&interfaces[i].address.address4, ip, sizeof(ip));
uv_ip4_name(&interfaces[i].netmask.netmask4, netmask, sizeof(netmask));
family = env->ipv4_string();
family = Integer::New(env->isolate(), 4);
} else if (interfaces[i].address.address4.sin_family == AF_INET6) {
uv_ip6_name(&interfaces[i].address.address6, ip, sizeof(ip));
uv_ip6_name(&interfaces[i].netmask.netmask6, netmask, sizeof(netmask));
family = env->ipv6_string();
family = Integer::New(env->isolate(), 6);
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 concerned about these two specific changes, moving from IPv4 to 4 and IPv6 to 6. I'm 100% sure those will break some of my modules (Fastify v4).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for this change is that currently passing an address object to the DNS resolver, the 'IPv6' string is converted to 0.
Would it be reasonable to ask you to update the code that using that to expect one form or the other? Something like info.family === 6 || info.family === 'IPv6'? If not, we could make the DNS resolver accept a string and interpret 'IPv6' same as 6.

Copy link

@Apollon77 Apollon77 Apr 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I assume this change will hurt the community a lot.

I personally would see this:

If not, we could make the DNS resolver accept a string and interpret 'IPv6' same as 6.

as the better solution

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed here. I realize that Node v18 was a major version change, but changing the output of a widely used API (socket.address()) just so another core-Node API could match up with it instead of making the DNS resolver more flexible seems pretty overkill!

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that it's out in the wild, can confirm it caused some widespread breaking for us that's going to make node 18 a pain and very uncertain to roll out.

Copy link

Choose a reason for hiding this comment

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

I assume this change will hurt the community a lot.

Can confirm completely unexpected breakage

Copy link

Choose a reason for hiding this comment

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

@aduh95 @mcollina Any chance to think about that change again and consider alternatives?

Copy link

Choose a reason for hiding this comment

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

maybe a new issue is more visible to the nodejs team and allows a better discussion ... so see #43014 ... Please "thumbs up" it if you support to adjust this to be backward compatible. Thank you

} else {
strncpy(ip, "<unknown sa family>", INET6_ADDRSTRLEN);
family = env->unknown_string();
family = Integer::New(env->isolate(), 0);
}

result.emplace_back(name);
Expand Down
4 changes: 2 additions & 2 deletions src/tcp_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ MaybeLocal<Object> AddressToJS(Environment* env,
OneByteString(env->isolate(), ip)).Check();
info->Set(env->context(),
env->family_string(),
env->ipv6_string()).Check();
Integer::New(env->isolate(), 6)).Check();
info->Set(env->context(),
env->port_string(),
Integer::New(env->isolate(), port)).Check();
Expand All @@ -395,7 +395,7 @@ MaybeLocal<Object> AddressToJS(Environment* env,
OneByteString(env->isolate(), ip)).Check();
info->Set(env->context(),
env->family_string(),
env->ipv4_string()).Check();
Integer::New(env->isolate(), 4)).Check();
info->Set(env->context(),
env->port_string(),
Integer::New(env->isolate(), port)).Check();
Expand Down
2 changes: 1 addition & 1 deletion test/common/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -845,7 +845,7 @@ const common = {
const re = isWindows ? /Loopback Pseudo-Interface/ : /lo/;
return Object.keys(iFaces).some((name) => {
return re.test(name) &&
iFaces[name].some(({ family }) => family === 'IPv6');
iFaces[name].some(({ family }) => family === 6);
});
},

Expand Down
6 changes: 3 additions & 3 deletions test/common/udppair.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class FakeUDPWrap extends EventEmitter {
this._handle.onwrite =
(wrap, buffers, addr) => this._write(wrap, buffers, addr);
this._handle.getsockname = (obj) => {
Object.assign(obj, { address: '127.0.0.1', family: 'IPv4', port: 1337 });
Object.assign(obj, { address: '127.0.0.1', family: 4, port: 1337 });
return 0;
};

Expand Down Expand Up @@ -72,8 +72,8 @@ class FakeUDPWrap extends EventEmitter {

let familyInt;
switch (family) {
case 'IPv4': familyInt = 4; break;
case 'IPv6': familyInt = 6; break;
case 4: familyInt = 4; break;
case 6: familyInt = 6; break;
default: throw new Error('bad family');
}

Expand Down
4 changes: 2 additions & 2 deletions test/es-module/test-http-imports.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ const internalInterfaces = Object.values(os.networkInterfaces()).flat().filter(
);
for (const iface of internalInterfaces) {
testListeningOptions.push({
hostname: iface?.family === 'IPv6' ? `[${iface?.address}]` : iface?.address,
hostname: iface?.family === 6 ? `[${iface.address}]` : iface?.address,
listenOptions: {
host: iface?.address,
ipv6Only: iface?.family === 'IPv6'
ipv6Only: iface?.family === 6
}
});
}
Expand Down
2 changes: 1 addition & 1 deletion test/internet/test-dgram-broadcast-multi-process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ get_bindAddress: for (const name in networkInterfaces) {
const interfaces = networkInterfaces[name];
for (let i = 0; i < interfaces.length; i++) {
const localInterface = interfaces[i];
if (!localInterface.internal && localInterface.family === 'IPv4') {
if (!localInterface.internal && localInterface.family === 4) {
bindAddress = localInterface.address;
break get_bindAddress;
}
Expand Down
2 changes: 1 addition & 1 deletion test/internet/test-dgram-multicast-set-interface-lo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ const TMPL = (tail) => `${NOW} - ${tail}`;
const interfaceAddress = ((networkInterfaces) => {
for (const name in networkInterfaces) {
for (const localInterface of networkInterfaces[name]) {
if (!localInterface.internal && localInterface.family === FAM) {
if (!localInterface.internal && `IPv${localInterface.family}` === FAM) {
let interfaceAddress = localInterface.address;
// On Windows, IPv6 would need: `%${localInterface.scopeid}`
if (FAM === 'IPv6')
Expand Down
2 changes: 1 addition & 1 deletion test/internet/test-dgram-multicast-ssm-multi-process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ get_sourceAddress: for (const name in networkInterfaces) {
const interfaces = networkInterfaces[name];
for (let i = 0; i < interfaces.length; i++) {
const localInterface = interfaces[i];
if (!localInterface.internal && localInterface.family === 'IPv4') {
if (!localInterface.internal && localInterface.family === 4) {
sourceAddress = localInterface.address;
break get_sourceAddress;
}
Expand Down
Loading