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

dns: default to verbatim=true in dns.lookup() #37681

Closed
wants to merge 27 commits into from
Closed

dns: default to verbatim=true in dns.lookup() #37681

Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
5bef0de
dns: default to verbatim=true in dns.lookup()
bnoordhuis Jan 29, 2020
4be87a0
Fixes for IPv6 compatibility; new const 'common.localhostIP'
treysis Mar 10, 2021
43d1492
Update test-net-dns-lookup.js
treysis Mar 13, 2021
beb6420
Update test-net-dns-lookup.js
treysis Mar 13, 2021
604366c
Fix test-net-dns-lookup.js
treysis Mar 13, 2021
1bb0e55
Apply suggestions from code review
treysis Mar 13, 2021
16a1682
IPv6 fixes
treysis Mar 14, 2021
178683c
Update inspector-helper.js
treysis Mar 14, 2021
2b67c17
Revert "Fixes for IPv6 compatibility; new const 'common.localhostIP'"
treysis Mar 16, 2021
4b06383
Update test-net-better-error-messages-port.js
treysis Mar 16, 2021
6340f13
Test suite fixes. Hardcode localhost IPv4.
treysis Mar 16, 2021
2f7edf0
More localhost/127.0.0.1 fixes.
treysis Mar 16, 2021
75bd959
Update test-net-pingpong.js
treysis Mar 16, 2021
a19f8ec
Update test/parallel/test-net-writable.js
treysis Mar 20, 2021
0232a4a
Update test-net-writable.js
treysis Mar 24, 2021
2ef88c7
Update test-net-writable.js
treysis Mar 24, 2021
98b3ffb
Update test-net-writable.js
treysis Mar 24, 2021
7a0e447
Update test-net-writable.js
treysis Mar 24, 2021
5037efc
Update parallel.status
treysis Mar 24, 2021
787dc69
Update test-net-connect-options-port.js
treysis Mar 24, 2021
26f7f45
Update parallel.status
treysis Mar 24, 2021
269eda7
Update test-net-connect-options-port.js
treysis Mar 24, 2021
1928b44
Update parallel.status
treysis Mar 24, 2021
5557b85
Updated from review
treysis Mar 25, 2021
bccf676
Update test-net-remote-address-port.js
treysis Mar 26, 2021
0c8be88
Update test/parallel/test-net-remote-address-port.js
treysis Mar 26, 2021
7cae409
Update test-net-better-error-messages-port.js
treysis Mar 26, 2021
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
7 changes: 4 additions & 3 deletions doc/api/dns.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,9 @@ section if a custom port is used.
<!-- YAML
added: v0.1.90
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/37681
description: The `verbatim` options defaults to `true` now.
- version: v8.5.0
pr-url: https://github.com/nodejs/node/pull/14731
description: The `verbatim` option is supported now.
Expand All @@ -183,9 +186,7 @@ changes:
* `verbatim` {boolean} When `true`, the callback receives IPv4 and 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 }`.
**Default:** `true`.
* `callback` {Function}
* `err` {Error}
* `address` {string} A string representation of an IPv4 or IPv6 address.
Expand Down
4 changes: 2 additions & 2 deletions lib/dns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ function lookup(hostname, options, callback) {
let hints = 0;
let family = -1;
let all = false;
let verbatim = false;
let verbatim = true;

// Parse arguments
if (hostname) {
Expand All @@ -113,7 +113,7 @@ function lookup(hostname, options, callback) {
hints = options.hints >>> 0;
family = options.family >>> 0;
all = options.all === true;
verbatim = options.verbatim === true;
verbatim = options.verbatim !== false;

validateHints(hints);
} else {
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/dns/promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ function lookup(hostname, options) {
var hints = 0;
var family = -1;
var all = false;
var verbatim = false;
var verbatim = true;

// Parse arguments
if (hostname && typeof hostname !== 'string') {
Expand All @@ -112,7 +112,7 @@ function lookup(hostname, options) {
hints = options.hints >>> 0;
family = options.family >>> 0;
all = options.all === true;
verbatim = options.verbatim === true;
verbatim = options.verbatim !== false;

validateHints(hints);
} else {
Expand Down
3 changes: 2 additions & 1 deletion test/common/inspector-helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ class NodeInstance extends EventEmitter {
console.log('[test]', `Testing ${path}`);
const headers = hostHeaderValue ? { 'Host': hostHeaderValue } : null;
return this.portPromise.then((port) => new Promise((resolve, reject) => {
const req = http.get({ host, port, path, headers }, (res) => {
const req = http.get({ host, family: 4, port, path, headers }, (res) => {
let response = '';
res.setEncoding('utf8');
res
Expand All @@ -418,6 +418,7 @@ class NodeInstance extends EventEmitter {
const devtoolsUrl = response[0].webSocketDebuggerUrl;
const port = await this.portPromise;
return http.get({
family: 4,
port,
path: parseURL(devtoolsUrl).path,
headers: {
Expand Down
4 changes: 4 additions & 0 deletions test/parallel/parallel.status
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ test-crypto-dh-stateless: SKIP
test-crypto-keygen: SKIP

[$system==solaris] # Also applies to SmartOS
# https://github.com/nodejs/node/pull/37681
test-net-writable: PASS,FLAKY
test-net-connect-options-port: PASS,FLAKY
test-net-pingpong: PASS,FLAKY

[$system==freebsd]
# https://github.com/nodejs/node/issues/31727
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-cluster-message.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ if (cluster.isWorker) {
// When a TCP server is listening in the worker connect to it
worker.on('listening', function(address) {

client = net.connect(address.port, function() {
client = net.connect(address.port, '127.0.0.1', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing this, I suggest changing line 62 to

server.listen(0, common.hasIPv6 ? '::1' : '127.0.0.1');

That way, we are still testing the default value.

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 don't think that works. Or we have to change the connect-line as follows as well:
client = net.connect(address.port, common.hasIPv6 ? '::1' : '127.0.0.1', function() {

Otherwise it will listen on ::1 on an IPv6-enabled system, but will try to connect to 127.0.0.1 and will fail with ECONNREFUSED.

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise it will listen on ::1 on an IPv6-enabled system, but will try to connect to 127.0.0.1 and will fail with ECONNREFUSED.

Hum that's not what I see on my machine… It tries to connect to ::1 as expected.
If you don't specify a host in net.connect, it will use localhost, which always resolves to 127.0.0.1 with verbatim set to false (that's the current test). With verbatim set to true, it will resolve to ::1 on IPv6 enabled system, and to 127.0.0.1 otherwise.

Anyway, I don't feel strongly about this, if you think it's simpler let's keep IPv4 for this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It did almost everywhere. But on SmartOS it would still resolve to 127.0.0.1, probably because ::1 localhost is not set in /etc/hosts. I seem to remember that it also resolved to 127.0.0.1 on my machine until I set that line in /etc/hosts. This wasn't a problem because the Linux kernel always adds 127.0.0.1 to the lo-interface, so even on an otherwise IPv6-only system it would still have that 127.0.0.1 address available and most software will not only listen on ::1 but also 127.0.0.1. Until we can be sure that every system will also have ::1 localhost I think the safe solution is to hardcode the address here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Until we can be sure that every system will also have ::1 localhost I think the safe solution is to hardcode the address here.

One could argue that the safe thing to do is to keep verbatim to false… If it's not easy to fix our CI machines, I don't think we can expect our users to fix their machines when updating Node.js.

Copy link
Contributor Author

@treysis treysis Mar 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm sure if we add ::1 localhost to /etc/hosts/ will solve this. Unfortunately I don't have access to SmartOS to examine this. As mentioned already, IPv6 is here to stay and should be accounted for. Having systems with faulty IPv6 will fail eventually. This is a bit ideologic, though, I have to admit. But landing this in the next LTS would give people both the time and the incentive to fix some fundamental issues. Otherwise people that need IPv6 will suffer. Unless we rewrite a lot of the logic, to catch all all cases, and/or implement dualstack listeners (for localhost at least) this could cause some tiny problems. And those will only affect localhost.

// Send message to worker.
worker.send('message from primary');
});
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-http-localaddress.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ const server = http.createServer((req, res) => {

server.listen(0, '127.0.0.1', () => {
const options = { host: 'localhost',
family: 4,
port: server.address().port,
path: '/',
method: 'GET',
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-http-upgrade-client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ server.listen(0, '127.0.0.1', common.mustCall(function() {

headers.forEach(function(h) {
const req = http.get({
host: '127.0.0.1',
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing this, I suggest changing line 52 to

server.listen(0, common.hasIPv6 ? '::1' : '127.0.0.1', common.mustCall(function() {

That way, we are still testing the default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is problematic because we don't know if localhost resolves to ::1 or 127.0.0.1. And that will be the default as it seems if we don't specify the host for http.get. If we do as per your suggestion, we should also do

host: common.hasIPv6 ? '::1' :  '127.0.0.1'

I think it's better to stick to pure IPv4 for now.

port: port,
headers: h
});
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-http2-connect-options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ server.listen(0, '127.0.0.1', common.mustCall(() => {
const options = { localAddress: '127.0.0.2' };

const client = http2.connect(
'http://localhost:' + server.address().port,
'http://127.0.0.1:' + server.address().port,
options
);
const req = client.request({
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-https-localaddress.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ const server = https.createServer(options, function(req, res) {
server.listen(0, '127.0.0.1', function() {
const options = {
host: 'localhost',
family: 4,
port: this.address().port,
path: '/',
method: 'GET',
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-net-connect-options-port.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ const net = require('net');
}
}, expectedConnections));

server.listen(0, 'localhost', common.mustCall(() => {
server.listen(0, common.mustCall(() => {
const port = server.address().port;

// Total connections = 3 * 4(canConnect) * 6(doConnect) = 72
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-net-dns-lookup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const server = net.createServer(function(client) {
});

server.listen(0, '127.0.0.1', common.mustCall(function() {
net.connect(this.address().port, 'localhost')
net.connect({ port: this.address().port, host: 'localhost', family: 4 })
.on('lookup', common.mustCall(function(err, ip, type, host) {
assert.strictEqual(err, null);
assert.strictEqual(ip, '127.0.0.1');
Expand Down
6 changes: 3 additions & 3 deletions test/parallel/test-net-remote-address-port.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,9 @@ const server = net.createServer(common.mustCall(function(socket) {
socket.resume();
}, 2));

server.listen(0, 'localhost', function() {
const client = net.createConnection(this.address().port, 'localhost');
const client2 = net.createConnection(this.address().port);
server.listen(0, '127.0.0.1', function() {
const client = net.createConnection(this.address().port, '127.0.0.1');
const client2 = net.createConnection(this.address().port, '127.0.0.1');
treysis marked this conversation as resolved.
Show resolved Hide resolved
client.on('connect', function() {
assert.ok(remoteAddrCandidates.includes(client.remoteAddress));
assert.ok(remoteFamilyCandidates.includes(client.remoteFamily));
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-net-writable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ const net = require('net');
const server = net.createServer(common.mustCall(function(s) {
server.close();
s.end();
})).listen(0, 'localhost', common.mustCall(function() {
const socket = net.connect(this.address().port, 'localhost');
})).listen(0, common.mustCall(function() {
const socket = net.connect(this.address().port);
socket.on('end', common.mustCall(() => {
assert.strictEqual(socket.writable, true);
socket.write('hello world');
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-tcp-wrap-listen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ server.onconnection = (err, client) => {

const net = require('net');

const c = net.createConnection(port);
const c = net.createConnection(port, '127.0.0.1');

c.on('connect', common.mustCall(() => { c.end('hello world'); }));

Expand Down
6 changes: 4 additions & 2 deletions test/parallel/test-timers-socket-timeout-removes-other-socket-unref-timer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,14 @@ server.listen(0, common.localhostIPv4, common.mustCall(() => {
const countdown = new Countdown(2, () => server.close());

{
const client = net.connect({ port: server.address().port });
const client = net.connect({ port: server.address().port,
host: '127.0.0.1' });
client.on('end', () => countdown.dec());
}

{
const client = net.connect({ port: server.address().port });
const client = net.connect({ port: server.address().port,
host: '127.0.0.1' });
client.on('end', () => countdown.dec());
}
}));
1 change: 1 addition & 0 deletions test/parallel/test-tls-client-getephemeralkeyinfo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ function test(size, type, name, cipher) {
server.listen(0, '127.0.0.1', common.mustCall(() => {
const client = tls.connect({
port: server.address().port,
host: '127.0.0.1',
rejectUnauthorized: false
}, common.mustCall(function() {
const ekeyinfo = client.getEphemeralKeyInfo();
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-tls-client-mindhsize.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ function test(size, err, next) {
const client = tls.connect({
minDHSize: 2048,
port: this.address().port,
host: '127.0.0.1',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
host: '127.0.0.1',
family: 4,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can try this but I think it's safer to use hardcoded IPv4 because not specifying the host would mean it defaults to localhost anyways which was always expected to be 127.0.0.1 by the test suite.

rejectUnauthorized: false
}, function() {
nsuccess++;
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-tls-wrap-econnreset-localaddress.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const server = net.createServer((c) => {
let errored = false;
tls.connect({
port: port,
family: 4,
localAddress: common.localhostIPv4
}, common.localhostIPv4)
.once('error', common.mustCall((e) => {
Expand Down
2 changes: 1 addition & 1 deletion test/sequential/test-https-connect-localport.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const assert = require('assert');
res.end();
}));

server.listen(0, 'localhost', common.mustCall(() => {
server.listen(0, '127.0.0.1', common.mustCall(() => {
const port = server.address().port;
const req = https.get({
host: 'localhost',
Expand Down
2 changes: 1 addition & 1 deletion test/sequential/test-inspector-open.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ function reopenAfterClose(msg) {
}

function ping(port, callback) {
net.connect(port)
net.connect(port, '127.0.0.1')
Copy link
Contributor

Choose a reason for hiding this comment

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

The inspector should "just work" with the default IMHO, the fact that it defaults to IPv4 goes a bit against that. I think it comes from here:

node/src/node_options.h

Line 85 in f392ac0

HostPort host_port{"127.0.0.1", kDefaultInspectorPort};

Not that it has anything to do with this PR, I just wanted to point it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should, but it doesn't. On some systems it will listen on 127.0.0.1, and on others it will listen on ::1.

.on('connect', function() { close(this); })
.on('error', function(err) { close(this, err); });

Expand Down
2 changes: 1 addition & 1 deletion test/sequential/test-net-better-error-messages-port.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const common = require('../common');
const net = require('net');
const assert = require('assert');

const c = net.createConnection(common.PORT);
const c = net.createConnection(common.PORT, common.localhostIPv4);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const c = net.createConnection(common.PORT, common.localhostIPv4);
const c = net.createConnection(common.PORT);

Shouldn't you instead change the assertion line 13:

assert.strictEqual(e.address, common.hasIPv6 ? '::1' : '127.0.0.1');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because we can't predict to which IP address net.createConnection will try to connect to. This is independent of the system having IPv6 or not. Leaving out the address in net.createConnection just means it tries to connect to localhost, which is not reliably resolved on dualstack systems. So if we leave out the common.localhostIPv4 we have to use RegEx again:

assert.match(e.address, /^(127\.0\.0\.1|::1)$/);

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I think assert.match would make more sense for this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And also remove , common.localhostIPv4?


c.on('connect', common.mustNotCall());

Expand Down
5 changes: 4 additions & 1 deletion test/sequential/test-net-connect-local-error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,19 @@ const net = require('net');
const expectedErrorCodes = ['ECONNREFUSED', 'EADDRINUSE'];

const optionsIPv4 = {
host: common.localhostIPv4,
port: common.PORT,
localPort: common.PORT + 1,
localAddress: common.localhostIPv4
localAddress: common.localhostIPv4,
family: 4,
};

const optionsIPv6 = {
host: '::1',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
host: '::1',

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 wouldn't remove it. I feel this might break stuff.

port: common.PORT + 2,
localPort: common.PORT + 3,
localAddress: '::1',
family: 6,
};

function onError(err, options) {
Expand Down