dns: make `dns.setServers` support customized port #13723

XadillaX · 2017-06-16T17:31:18Z

Make dns.setServers parameter may be strings contain IP and port which
split by :.

eg.

dns.setServers([ "103.238.225.181:666" ]);

And dns.getServers will return IP with port if that server is not use
default port.

Refs: #7903

Checklist

make -j4 test (UNIX)
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

dns

Make `dns.setServers` parameter may be strings contain IP and port which split by `:`. eg. ``` dns.setServers([ "103.238.225.181:666" ]); ``` And `dns.getServers` will return IP with port if that server is not use default port. Refs: nodejs#7903

XadillaX · 2017-06-16T17:53:44Z

@addaleax I think we can remove in progress label now.

silverwind · 2017-06-16T18:24:33Z

Maybe it's better to have the port as a separate argument so the user doesn't have to fiddle with the [host]:port syntax of IPv6. Also, it'd be more in line with other APIs that take ports and hosts, which also separate them.

silverwind · 2017-06-16T18:30:48Z

E.g.:

dns.setServers([
  {address: '1.2.3.4', port: 5353},
  {address: '::1234:5678', port: 5353},
  '2.3.4.5'
]);

So the method could both take a object or a string. It also has the benefit that we could extend these objects with future per-server options if necessary.

XadillaX · 2017-06-16T18:31:05Z

@silverwind [host]:port is valid in the previous versions of Node.js, but the port be ignored before. I'm only making it's valid and available now.

You mean let the function support host / [host]:port and { host: ..., port: ... } all these three situations?

refack

Got a little carried away with style suggestion... 🙄

refack · 2017-06-16T18:13:27Z

doc/api/dns.md

@@ -59,8 +59,20 @@ the [Implementation considerations section][] for more information.
 added: v0.11.3
 -->

-Returns an array of IP address strings that are being used for name
-resolution.
+Returns an array of IP address and port strings that are being used for name


🚲 🎪 Suggestion:

Returns an array of IP address strings, formatted according to [rfc3986][], that are currently configured for DNS resolution. A string will include a port section if a custom port is used. ... [rfc3986]: https://tools.ietf.org/html/rfc3986#section-3.2.2

refack · 2017-06-16T18:14:59Z

doc/api/dns.md

@@ -484,10 +496,20 @@ added: v0.11.3
 -->
 - `servers` {string[]}


- `servers` {string[]} array of [rfc3986][] formatted addresses

refack · 2017-06-16T18:18:48Z

doc/api/dns.md

@@ -484,10 +496,20 @@ added: v0.11.3
 -->
 - `servers` {string[]}

-Sets the IP addresses of the servers to be used when resolving. The `servers`
-argument is an array of IPv4 or IPv6 addresses.
+Sets the IP addresses and port of the servers to be used when resolving. The


Sets the IP address and port of servers to be used when performing DNS resolution. If the port is the IANA default DNS port (53) it can be omitted.

refack · 2017-06-16T18:22:56Z

lib/dns.js

+    if (!val[1] || val[1] === 53) return val[0];
+
+    const ipVersion = isIP(val[0]);
+    return ipVersion === 6 ? `[${val[0]}]:${val[1]}` : `${val[0]}:${val[1]}`;


Personal preference nit:

const host= 6 ? `[${val[0]}]` : val[0]; return ${host}:${val[1]};

refack · 2017-06-16T18:31:23Z

lib/dns.js

-      if (ipVersion !== 0)
-        return newSet.push([ipVersion, match[1]]);
+      if (ipVersion !== 0) {
+        const portMatch = serv.match(addrSplitRE) || [];


personal style nit:

const addrSplitRE = /(^.+?)(?::(\d+))?$/; const port = parseInt(serv.replace(addrSplitRE, '$2')) || 53; return newSet.push([ipVersion, match[1], port);

refack · 2017-06-16T18:35:39Z

lib/dns.js

+      if (ipVersion !== 0) {
+        const portMatch = serv.match(addrSplitRE) || [];
+        return newSet.push([ipVersion, match[1], parseInt(portMatch[1] || 53)]);
+      }
    }

    const s = serv.split(addrSplitRE)[0];


const [, s, p] = serv.match(addrSplitRE);

refack · 2017-06-16T18:40:57Z

🤔 If I understand correctly dns.setServers([ "103.238.225.181:666" ]); used to work but ignored the port? Now it will use the port?
Need to consider semver level, and run CITGM when PR converges.

XadillaX · 2017-06-16T18:46:00Z

@refack Yes you understood right. Refer here https://nodejs.org/docs/v8.0.0/api/dns.html#dns_dns_setservers_servers.

If a port is specified on the address, it will be removed.

And the addrSplitRE in the source code used to ignore the port.

silverwind · 2017-06-16T18:49:48Z

[host]:port is valid in the previous versions of Node.js

Hmm, right the docs even mention "If a port is specified on the address, it will be removed.", so I guess we're stuck with the syntax. And no, I wouldn't support three cases. Strictly speaking, this should be semver-major, but if CITGM turns out okay (which I expect), than minor is okay.

refack

Code 💯
LGTM % semver discussion

refack · 2017-06-16T19:34:43Z

CI: https://ci.nodejs.org/job/node-test-commit/10614/

XadillaX · 2017-06-18T14:50:01Z

I think after this PR is landed, we can start the test for dns.resolveAny.

refack · 2017-06-19T02:30:15Z

🤔 If I understand correctly dns.setServers([ "103.238.225.181:666" ]) used to work but ignored the port? Now it will use the port?
Need to consider semver level, and run CITGM when PR converges.

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/880/

@addaleax @nodejs/ctc PTAL at semver level

refack · 2017-06-19T02:42:50Z

lib/dns.js

-  return cares.getServers();
+  const ret = cares.getServers();
+  return ret.map((val) => {
+    if (!val[1] || val[1] === 53) return val[0];


I just noticed this, IMHO we should format IPv6 with [], so need to move the const host up:
e.g.

return ret.map(([host, port]) => { if (isIP(host) === 6) host = `[${host}]`; return (!port || port === 53) host : `${host}:${port}`; }

But that will make this semver-major (since till now IPv6 would be represented without [])

Fix this in a future PR.
Ref: #13795

Need to think about semver-level, and IPv6 string format

XadillaX · 2017-06-19T03:15:36Z

@refack how about adding a parameter to indicate whether using customized port or not

mcollina

LGTM as semver-minor

refack · 2017-06-19T12:11:54Z

@refack how about adding a parameter to indicate whether using customized port or not

CITGM is clean.
Let's land this as is. And open a semver-major PR to fix the formatting.

refack · 2017-06-19T12:21:56Z

doc/api/dns.md

+```js
+[
+  '4.4.4.4',
+  '[2001:4860:4860::8888]',


Need to remove [] for this line.
Ref: #13795

This still needs to be changed, since currently the output will be without the []

> dns.setServers(['192.168.1.1', '2001:4860:4860::8888']) undefined > dns.getServers() [ '192.168.1.1', '2001:4860:4860::8888' ]

refack · 2017-06-19T12:22:19Z

lib/dns.js

-  return cares.getServers();
+  const ret = cares.getServers();
+  return ret.map((val) => {
+    if (!val[1] || val[1] === 53) return val[0];


Fix this in a future PR.
Ref: #13795

XadillaX · 2017-06-19T14:42:41Z

Let's land this as is. And open a semver-major PR to fix the formatting.

Land this as SEMVER-MINOR? And what formatting for SEMVER-MAJOR?

refack · 2017-06-20T22:52:08Z

Landed in 330349f
(fixed a nit in the doc /s/eg./For example:/, and the language on the commit message body)

refack · 2017-06-20T22:53:46Z

Extra sanity on master: https://ci.nodejs.org/job/node-test-commit-linuxone/6777/