-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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: default to verbatim=true in dns.lookup() #37681
Changes from 12 commits
5bef0de
4be87a0
43d1492
beb6420
604366c
1bb0e55
16a1682
178683c
2b67c17
4b06383
6340f13
2f7edf0
75bd959
a19f8ec
0232a4a
2ef88c7
98b3ffb
7a0e447
5037efc
787dc69
26f7f45
269eda7
1928b44
5557b85
bccf676
0c8be88
7cae409
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is problematic because we don't know if
I think it's better to stick to pure IPv4 for now. |
||
port: port, | ||
headers: h | ||
}); | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -83,7 +83,7 @@ const net = require('net'); | |||||
} | ||||||
}, expectedConnections)); | ||||||
|
||||||
server.listen(0, 'localhost', common.mustCall(() => { | ||||||
server.listen(0, '127.0.0.1', common.mustCall(() => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should change anything in this test, it should work just fine with IPv6 as with IPv4 – at least it does on my machine.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, the problem is that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's interesting, I missed that information. It makes more sense now. It's a bit concerning that this change may break code using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good question and I am not sure how this will play out in production outside of the test suite. If everyone would have the See https://github.com/nodejs/node/blob/master/lib/net.js and look for On the other hand, not setting |
||||||
const port = server.address().port; | ||||||
|
||||||
// Total connections = 3 * 4(canConnect) * 6(doConnect) = 72 | ||||||
|
@@ -167,25 +167,25 @@ function canConnect(port) { | |||||
const noop = () => common.mustCall(); | ||||||
|
||||||
// connect(port, cb) and connect(port) | ||||||
const portArgFunctions = doConnect([port], noop); | ||||||
const portArgFunctions = doConnect([port, '127.0.0.1'], noop); | ||||||
for (const fn of portArgFunctions) { | ||||||
fn(); | ||||||
} | ||||||
|
||||||
// connect(port, host, cb) and connect(port, host) | ||||||
const portHostArgFunctions = doConnect([port, 'localhost'], noop); | ||||||
const portHostArgFunctions = doConnect([port, '127.0.0.1'], noop); | ||||||
for (const fn of portHostArgFunctions) { | ||||||
fn(); | ||||||
} | ||||||
|
||||||
// connect({port}, cb) and connect({port}) | ||||||
const portOptFunctions = doConnect([{ port }], noop); | ||||||
const portOptFunctions = doConnect([{ port, family: 4 }], noop); | ||||||
for (const fn of portOptFunctions) { | ||||||
fn(); | ||||||
} | ||||||
|
||||||
// connect({port, host}, cb) and connect({port, host}) | ||||||
const portHostOptFns = doConnect([{ port, host: 'localhost' }], noop); | ||||||
const portHostOptFns = doConnect([{ port, host: '127.0.0.1' }], noop); | ||||||
for (const fn of portHostOptFns) { | ||||||
fn(); | ||||||
} | ||||||
|
@@ -205,13 +205,13 @@ function asyncFailToConnect(port) { | |||||
} | ||||||
|
||||||
// connect({port}, cb) and connect({port}) | ||||||
const portOptFunctions = doConnect([{ port }], dont); | ||||||
const portOptFunctions = doConnect([{ port, family: 4 }], dont); | ||||||
for (const fn of portOptFunctions) { | ||||||
fn().on('error', onError()); | ||||||
} | ||||||
|
||||||
// connect({port, host}, cb) and connect({port, host}) | ||||||
const portHostOptFns = doConnect([{ port, host: 'localhost' }], dont); | ||||||
const portHostOptFns = doConnect([{ port, host: '127.0.0.1' }], dont); | ||||||
for (const fn of portHostOptFns) { | ||||||
fn().on('error', onError()); | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -81,11 +81,11 @@ function pingPongTest(port, host) { | |||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
server.listen(port, host, common.mustCall(function() { | ||||||||||||||||
server.listen(port, '127.0.0.1', common.mustCall(function() { | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
if (this.address().port) | ||||||||||||||||
port = this.address().port; | ||||||||||||||||
|
||||||||||||||||
const client = net.createConnection(port, host); | ||||||||||||||||
const client = net.createConnection(port, '127.0.0.1'); | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
|
||||||||||||||||
client.setEncoding('ascii'); | ||||||||||||||||
client.on('connect', common.mustCall(function() { | ||||||||||||||||
|
@@ -130,6 +130,4 @@ const tmpdir = require('../common/tmpdir'); | |||||||||||||||
tmpdir.refresh(); | ||||||||||||||||
pingPongTest(common.PIPE); | ||||||||||||||||
pingPongTest(0); | ||||||||||||||||
pingPongTest(0, 'localhost'); | ||||||||||||||||
if (common.hasIPv6) | ||||||||||||||||
pingPongTest(0, '::1'); | ||||||||||||||||
pingPongTest(0, '127.0.0.1'); | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's keep localhost.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will fail at least on SmartOS because listening on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PIng, can we add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, that doesn't make much sense to me as this test is clearly aimed at IPv4, because there's the second test that explicitly checks for IPv6. Furthermore, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, so with |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -41,6 +41,7 @@ function test(size, err, next) { | |||||
const client = tls.connect({ | ||||||
minDHSize: 2048, | ||||||
port: this.address().port, | ||||||
host: '127.0.0.1', | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
rejectUnauthorized: false | ||||||
}, function() { | ||||||
nsuccess++; | ||||||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -80,7 +80,7 @@ function reopenAfterClose(msg) { | |||
} | ||||
|
||||
function ping(port, callback) { | ||||
net.connect(port) | ||||
net.connect(port, '127.0.0.1') | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: Line 85 in f392ac0
Not that it has anything to do with this PR, I just wanted to point it out. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||
.on('connect', function() { close(this); }) | ||||
.on('error', function(err) { close(this, err); }); | ||||
|
||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Shouldn't you instead change the assertion line 13: assert.strictEqual(e.address, common.hasIPv6 ? '::1' : '127.0.0.1'); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, because we can't predict to which IP address
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And also remove |
||||||
|
||||||
c.on('connect', common.mustNotCall()); | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -10,14 +10,16 @@ const expectedErrorCodes = ['ECONNREFUSED', 'EADDRINUSE']; | |||||||||||||
const optionsIPv4 = { | ||||||||||||||
port: common.PORT, | ||||||||||||||
localPort: common.PORT + 1, | ||||||||||||||
localAddress: common.localhostIPv4 | ||||||||||||||
localAddress: common.localhostIPv4, | ||||||||||||||
family: 4 | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would still like to keep the trailing comma. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't seen the trailing comma being used in other files, but I'm okay with either way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed you added the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would assume it's because, being options for IPv4, this configuration is seeking to get a v4 connection. Previously it would have due to reordering to always put v4 first. Now, it has to explicitly request that it wants v4. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly. Sometimes it would also try to bind to the wrong address family, causing PS: I am not happy with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think we should need to specify it though, when using the default value both IPv4 and IPv6 should be allowed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should. But since we can't reliably predict whether There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or you can force the IP address, as per my suggestion
Suggested change
Previously, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, can you make this change please? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one passed well. |
||||||||||||||
}; | ||||||||||||||
|
||||||||||||||
const optionsIPv6 = { | ||||||||||||||
host: '::1', | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, I haven't seen trailing comma elsewhere in the test suite, but no objection from my side. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this was necessary here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, unfortunately I lost a bit track during the extensive test runs where this failed and if this specifically fixed it or not. I would prefer to have |
||||||||||||||
}; | ||||||||||||||
|
||||||||||||||
function onError(err, options) { | ||||||||||||||
|
There was a problem hiding this comment.
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
That way, we are still testing the default value.
There was a problem hiding this comment.
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 to127.0.0.1
and will fail withECONNREFUSED
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 uselocalhost
, which always resolves to127.0.0.1
withverbatim
set tofalse
(that's the current test). Withverbatim
set totrue
, it will resolve to::1
on IPv6 enabled system, and to127.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.
There was a problem hiding this comment.
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 to127.0.0.1
on my machine until I set that line in/etc/hosts
. This wasn't a problem because the Linux kernel always adds127.0.0.1
to thelo
-interface, so even on an otherwise IPv6-only system it would still have that127.0.0.1
address available and most software will not only listen on::1
but also127.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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One could argue that the safe thing to do is to keep
verbatim
tofalse
… 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.There was a problem hiding this comment.
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 (forlocalhost
at least) this could cause some tiny problems. And those will only affectlocalhost
.