dns: default to verbatim=true in dns.lookup() by treysis · Pull Request #37681 · nodejs/node

treysis · 2021-03-09T22:11:24Z

Switch the default from false (reorder the result so that IPv4 addresses
come before IPv6 addresses) to true (return them exactly as the resolver
sent them to us.)

Fixes: #31566
Refs: #6307
Refs: #20710

Reissue of #31567

jasnell · 2021-03-09T23:43:04Z

@nodejs/tsc ... do we need to to take the current default through a deprecation cycle first?

jasnell · 2021-03-09T23:44:14Z

This could use a test

mcollina

lgtm

richfelker · 2021-03-10T00:13:05Z

The failing tests look like they're just wrong assertions that localhost resolves to 127.0.0.1.

treysis · 2021-03-10T00:19:34Z

Yeah, now the question is...should localhost resolve to 127.0.0.1, or should assert test for ::1? I prefer the latter, but I see good reasons for the first one as well.

richfelker · 2021-03-10T00:23:58Z

The test should simply assert that it's one or the other, allowing either, since the result you get depends on whether the environment is ipv6-capable.

treysis · 2021-03-10T00:25:29Z

Is there an OR statement for assert.strictEqual?

aduh95 · 2021-03-10T00:39:24Z

Is there an OR statement for assert.strictEqual?

You can use assert.match and a RegEx.

treysis · 2021-03-10T09:40:22Z

I suspect it's failing now because the debugger only listens on 127.0.0.1:
https://nodejs.org/en/docs/guides/debugging-getting-started/

Now, either we assume that every IPv6 system will always be reachable via IPv4 at least on localhost, we could change localhost to hardcoded 127.0.0.1. Or is it possible that the debugger listens on dualstack localhost? I am not sure that is supported.

treysis · 2021-03-11T11:17:38Z

I feel lilke one problem here is that instances are listening on 127.0.0.1 by default and the documentation doesn't really give clear instructions on how to solve this and about the notation of IPv6 addresses (e.g. with or without braces); or if :: means IPv6-only or INADDR_ANY. Or is 0.0.0.0 euqal to INADDR_ANY?

Another obstacle will be to make the code really dualstack. Because localAddress can't hold a hostname. Exchanging 127.0.0.1 by ::1 is probably causing issues on legacy IPv4-only systems? So I see lots of if/then/else coming. I'll definitely will need some help there. So suggestions very welcome.

nodejs-github-bot · 2021-03-25T13:26:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/36963/

treysis · 2021-03-25T14:16:51Z

It passed. Somehow the connection to this PR is broken? Here it still shows as if it's running.
Also, I can't seem to find the logs where it shows which tests passed and which didn't.

nodejs-github-bot · 2021-03-25T14:20:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/36970/

treysis · 2021-03-25T14:57:57Z

Ok so it passed. Yet there are still some tests shown here on github that link to older runs and don't show as finished, e.g.:
node-test-commit-aix Pending — running testshttps://ci.nodejs.org/job/node-test-commit-smartos/nodes=smartos18-64/38097/

nodejs-github-bot · 2021-03-25T15:56:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/36973/

treysis · 2021-03-25T16:20:07Z

test-net-pingpong fails (as expected):

17:13:39 AssertionError [ERR_ASSERTION]: function should not have been called at /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos18-64/test/parallel/test-net-pingpong.js:124
17:13:39 called with arguments: Error: connect ECONNREFUSED 127.0.0.1:42600
17:13:39 at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1143:16) {
17:13:39 errno: -146,
17:13:39 code: 'ECONNREFUSED',
17:13:39 syscall: 'connect',
17:13:39 address: '127.0.0.1',
17:13:39 port: 42600
17:13:39 }
17:13:39 at Socket.mustNotCall (/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos18-64/test/common/index.js:448:12)
17:13:39 at Socket.emit (node:events:369:20)
17:13:39 at emitErrorNT (node:internal/streams/destroy:195:8)
17:13:39 at emitErrorCloseNT (node:internal/streams/destroy:160:3)
17:13:39 at processTicksAndRejections (node:internal/process/task_queues:81:21) {
17:13:39 generatedMessage: false,
17:13:39 code: 'ERR_ASSERTION',
17:13:39 actual: undefined,
17:13:39 expected: undefined,
17:13:39 operator: 'fail'
17:13:39 }

The other error is just 'flakiness' (nothing to do with networking).

nodejs-github-bot · 2021-03-25T16:41:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/36975/

treysis · 2021-03-25T17:12:46Z

Yeah, the other error disappeared. But test-net-pingpong is essentially still broken and test just passes due to flaky-setting.

aduh95 · 2021-03-25T17:15:45Z

Yeah, the other error disappeared. But test-net-pingpong is essentially still broken and test just passes due to flaky-setting.

Which is something that you are confident that it can be solved at the CI machine config level, correct? If so, that shouldn't be a problem for landing this PR.

treysis · 2021-03-25T17:21:42Z

Well, I am not very well versed in Unix, neither Solaris nor SmartOS in particular. It seems as if SmartOS only resolves localhost to '::1' when connecting if there's more IPv6 configured than the lo-interface. I don't know how this is handled on other Unix OS'. So, for now, I don't know how this can be fixed on SmartOS. Though the bigger question is why it is listening on ::1 is there is no IPv6 configured. And honestly, I have no idea where to look that up.

I'd change the pingpong-test to use 127.0.0.1 by default, and if there is IPv6 it will also do the IPv6 test. The time when localhost won't resolve to 127.0.0.1 anymore in general is long in the future.

aduh95

Can we try those suggestions before calling for more reviews?

aduh95 · 2021-03-26T11:24:29Z

 const assert = require('assert');

-const c = net.createConnection(common.PORT);
+const c = net.createConnection(common.PORT, common.localhostIPv4);


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

aduh95 · 2021-03-26T11:30:18Z

I you are able to implement the last two request above, I can spawn another CI job (if it doesn't pass we'll revert and call it done).

Once my suggestions addressed, I think considering the size of this PR and the fact there is already ≥200 comments on it, it would be worth opening a new one, and explain in the OP:

why we have to use 127.0.0.1 instead of locahost for some tests
why some tests are flaky on SmartOS
add a note regarding the fact that the inspector default IP address is 127.0.0.1 which is too bad but has nothing to do with this PR.

I don't think we would fine anyone with enough motivation to go through reading all comments to understand how we arrived at this result.

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

treysis · 2021-03-26T15:19:15Z

I you are able to implement the last two request above, I can spawn another CI job (if it doesn't pass we'll revert and call it done).

Done.

Once my suggestions addressed, I think considering the size of this PR and the fact there is already ≥200 comments on it, it would be worth opening a new one, and explain in the OP:
why we have to use 127.0.0.1 instead of locahost for some tests
why some tests are flaky on SmartOS
add a note regarding the fact that the inspector default IP address is 127.0.0.1 which is too bad but has nothing to do with this PR.

Totally agree!

I don't think we would fine anyone with enough motivation to go through reading all comments to understand how we arrived at this result.

Actually, I feel like starting over from zero with all that we have learned so far. If just the CI jobs were faster or I could set it up at my PC to test it locally :/ it would be nice being able to just run the tests that fail, and not the whole building procedure and all of the tests.

nodejs-github-bot · 2021-03-26T15:22:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/36996/

treysis · 2021-03-26T16:04:37Z

Failed now on test-net-remote-address-port (y can't I comment on the code review anymore?).

16:52:20 not ok 1604 parallel/test-net-remote-address-port
16:52:20 ---
16:52:20 duration_ms: 0.585
16:52:20 severity: fail
16:52:20 exitcode: 1
16:52:20 stack: |-
16:52:20 node:events:346
16:52:20 throw er; // Unhandled 'error' event
16:52:20 ^
16:52:20 16:52:20 Error: connect ECONNREFUSED 127.0.0.1:46978
16:52:20 at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1143:16)
16:52:20 Emitted 'error' event on Socket instance at:
16:52:20 at emitErrorNT (node:internal/streams/destroy:195:8)
16:52:20 at emitErrorCloseNT (node:internal/streams/destroy:160:3)
16:52:20 at processTicksAndRejections (node:internal/process/task_queues:81:21) {
16:52:20 errno: -146,
16:52:20 code: 'ECONNREFUSED',
16:52:20 syscall: 'connect',
16:52:20 address: '127.0.0.1',
16:52:20 port: 46978
16:52:20 }

I think I'll craft a new PR and we continue from there.

aduh95 · 2021-03-26T16:06:40Z

test-net-remote-address-port should be added to the list of FLAKY tests. +1 on creating a new PR.

Switch the default from false (reorder the result so that IPv4 addresses come before IPv6 addresses) to true (return them exactly as the resolver sent them to us.) Fixes: nodejs#31566 Refs: nodejs#6307 Refs: nodejs#20710 Reissue of nodejs#31567 Reissue of nodejs#37681

treysis · 2021-03-26T16:18:20Z

Continues in #37931

Switch the default from false (reorder the result so that IPv4 addresses come before IPv6 addresses) to true (return them exactly as the resolver sent them to us.) Fixes: nodejs#31566 Refs: nodejs#6307 Refs: nodejs#20710 Refs: nodejs#38099 Reissue of nodejs#31567 Reissue of nodejs#37681 Reissue of nodejs#37931

nodejs-github-bot added dns Issues and PRs related to the dns subsystem. needs-ci PRs that need a full CI run. labels Mar 9, 2021

treysis force-pushed the verbatim-true branch 2 times, most recently from 42e0a69 to 0e73564Compare March 9, 2021 22:31

aduh95 reviewed Mar 9, 2021
View reviewed changes

Comment threaddoc/api/dns.md Outdated

aduh95 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 9, 2021

mcollina approved these changes Mar 9, 2021
View reviewed changes

treysis force-pushed the verbatim-true branch from 0e73564 to 050cf02Compare March 10, 2021 00:15

treysis force-pushed the verbatim-true branch 3 times, most recently from e6d93bb to 6aca9afCompare March 10, 2021 14:21

aduh95 reviewed Mar 10, 2021
View reviewed changes

Comment threadtest/parallel/test-http-localaddress.js Outdated
Comment threadtest/parallel/test-https-localaddress.js Outdated
Comment threadtest/sequential/test-net-better-error-messages-port.js Outdated

treysis force-pushed the verbatim-true branch from 170ceac to 5df9014Compare March 11, 2021 07:08

aduh95 reviewed Mar 11, 2021
View reviewed changes

Comment threadtest/sequential/test-net-better-error-messages-port.js Outdated
Comment threadtest/parallel/test-http-localaddress.js Outdated
Comment threadtest/parallel/test-http2-connect-options.js Outdated
Comment threadtest/parallel/test-https-localaddress.js Outdated