dns: implement {ttl: true} for dns.resolve4() and dns.resolve6() by bnoordhuis · Pull Request #9296 · nodejs/node

bnoordhuis · 2016-10-26T16:48:39Z

See discussion in #5893 and other issues. We can retrieve the TTL for A and AAAA records looked up through the dns.resolve() function family.

I'll write up documentation and nice commit logs if we decide this is something we want to do. Example output:

$ out/Release/node -e 'dns.resolve4("google.com", {ttl:true}, console.log)'
null [ { address: '82.94.228.160', ttl: 299 },
{ address: '82.94.228.173', ttl: 299 },
{ address: '82.94.228.166', ttl: 299 },
{ address: '82.94.228.180', ttl: 299 },
{ address: '82.94.228.159', ttl: 299 },
{ address: '82.94.228.167', ttl: 299 },
{ address: '82.94.228.181', ttl: 299 },
{ address: '82.94.228.187', ttl: 299 },
{ address: '82.94.228.153', ttl: 299 },
{ address: '82.94.228.146', ttl: 299 },
{ address: '82.94.228.152', ttl: 299 },
{ address: '82.94.228.174', ttl: 299 } ]

jasnell · 2016-10-26T16:58:01Z

SGTM

cjihrig · 2016-10-26T17:01:30Z

SGTM too

silverwind · 2016-10-27T23:07:10Z

How about also supporting a 4-argument signature of dns.resolve?

bnoordhuis · 2016-10-31T10:12:59Z

How about also supporting a 4-argument signature of dns.resolve?

I thought about that but I decided to punt on what to do when someone requests the TTL for a non-A/AAAA record.

silverwind · 2016-10-31T16:06:05Z

I could also see the second argument to it being made a object, so we support two signatures:

dns.resolve(hostname[, rrtype], callback)
dns.resolve(hostname[, options], callback)

options could then include rrtype and ttl, and possibly more future additions like class. We could deprecate the first signature in v8.

bnoordhuis · 2016-11-01T09:31:44Z

Sure, but we'd still need to come up with a policy for dealing with bad inputs: throw, ignore, log a warning, something else. I figured it's easier to just sidestep the issue and leave dns.resolve() alone for now.

jasnell · 2016-11-02T14:20:08Z

I'm a bit -1 on the two signature approach. Perhaps an rrtype option can be added alongside the ttl option? e.g. dns.resolve(hostname, {ttl: true, rrtype: 'A'}, console.log). That said, I'm not entirely convinced we should add the rrtype option for the same reasons @bnoordhuis outlines.

silverwind · 2016-11-02T15:23:13Z

rrtype option can be added alongside the ttl

Yes, that's my plan. Currently the second argument is only accepted as a string. We can simply typecheck if it's string or object, and throw otherwise. Having object support in all resolve* methods will certainly pave the way for more additions in the future. I'm thinking about servers and class options for example.

silverwind · 2016-11-03T18:43:07Z

@bnoordhuis I think it'd be better if we just go all the way and add TTL support to all methods. Having only the A/AAAA shorthands support it would be quite confusing. What are you exactly worried about regarding input validation? Looks to be a simple case of string/object check and throw otherwise.

bnoordhuis · 2016-11-03T19:27:02Z

We can't add TTL support for all methods because c-ares only supports it for A and AAAA records.

silverwind · 2016-11-03T19:33:06Z

Oh dang, didn't realize that stupid limitation. In that case I guess you found a good compromise.

bnoordhuis · 2016-11-15T18:35:22Z

Updated with documentation and pretty commit logs, PTAL.

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

Add an option to retrieve the Time-To-Live of the A record. PR-URL: nodejs#9296 Refs: nodejs#5893 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Add an option to retrieve the Time-To-Live of the AAAA record. PR-URL: nodejs#9296 Refs: nodejs#5893 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Add an option to retrieve the Time-To-Live of the A record. PR-URL: #9296 Refs: #5893 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Add an option to retrieve the Time-To-Live of the AAAA record. PR-URL: #9296 Refs: #5893 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Notable changes: * crypto: The `Decipher` methods `setAuthTag()` and `setAAD` now return `this`. (Kirill Fomichev) #9398 * dns: Implemented `{ttl: true}` for `resolve4()` and `resolve6()`. (Ben Noordhuis) #9296 & #9296 * libuv: Upgrade to v1.10.1 (cjihrig) #9647 * process: Added a new `external` property to the data returned by `memoryUsage()`. (Fedor Indutny) #9587 * V8 (dep): Upgrade to v5.4.500.43 (Michaël Zasso) #9697 * v8: The data returned by `getHeapStatistics()` now includes three new fields: `malloced_memory`, `peak_malloced_memory`, and `does_zap_garbage`. (Gareth Ellis) #8610 PR-URL: #9745

This is a security release impacting Windows 10 users. Notable changes: * crypto: The `Decipher` methods `setAuthTag()` and `setAAD` now return `this`. (Kirill Fomichev) #9398 * dns: Implemented `{ttl: true}` for `resolve4()` and `resolve6()`. (Ben Noordhuis) #9296 & #9296 * libuv: Upgrade to v1.10.1 (cjihrig) #9647 - Fixed a potential buffer overflow when writing data to console on Windows 10. (CVE-2016-9551) * process: Added a new `external` property to the data returned by `memoryUsage()`. (Fedor Indutny) #9587 * tls: Fixed a memory leak when writes were queued on TLS connection that was destroyed during handshake. (Fedor Indutny) #9626 * V8 (dep): Upgrade to v5.4.500.43 (Michaël Zasso) #9697 * v8: The data returned by `getHeapStatistics()` now includes three new fields: `malloced_memory`, `peak_malloced_memory`, and `does_zap_garbage`. (Gareth Ellis) #8610 PR-URL: #9745

This is a security release impacting Windows 10 users. Notable changes: * crypto: The `Decipher` methods `setAuthTag()` and `setAAD` now return `this`. (Kirill Fomichev) nodejs/node#9398 * dns: Implemented `{ttl: true}` for `resolve4()` and `resolve6()`. (Ben Noordhuis) nodejs/node#9296 & nodejs/node#9296 * libuv: Upgrade to v1.10.1 (cjihrig) nodejs/node#9647 - Fixed a potential buffer overflow when writing data to console on Windows 10. (CVE-2016-9551) * process: Added a new `external` property to the data returned by `memoryUsage()`. (Fedor Indutny) nodejs/node#9587 * tls: Fixed a memory leak when writes were queued on TLS connection that was destroyed during handshake. (Fedor Indutny) nodejs/node#9626 * V8 (dep): Upgrade to v5.4.500.43 (Michaël Zasso) nodejs/node#9697 * v8: The data returned by `getHeapStatistics()` now includes three new fields: `malloced_memory`, `peak_malloced_memory`, and `does_zap_garbage`. (Gareth Ellis) nodejs/node#8610 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Add an option to retrieve the Time-To-Live of the A record. PR-URL: #9296 Refs: #5893 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Add an option to retrieve the Time-To-Live of the AAAA record. PR-URL: #9296 Refs: #5893 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Add an option to retrieve the Time-To-Live of the A record. PR-URL: #9296 Refs: #5893 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Add an option to retrieve the Time-To-Live of the AAAA record. PR-URL: #9296 Refs: #5893 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

tommarien · 2017-05-18T11:49:54Z

Is this ever going to be backported to node 6 ?

addaleax · 2017-05-18T11:51:29Z

@tommarien That this has gotten the land-on-v6.x label means it’s scheduled to come with the next v6.x release. :)

tommarien · 2017-05-18T11:54:42Z

@addaleax That would be marvellous ;)

This LTS release comes with 126 commits. This includes 40 which are test related, 32 which are doc related, 12 which are build / tool related and 4 commits which are updates to dependencies. Notable Changes: * build: - support for building mips64el (nanxiongchao) #10991 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) #10019 * crypto: - ability to select cert store at runtime (Adam Majer) #8334 - Use system CAs instead of using bundled ones (Adam Majer) #8334 - The `Decipher` methods `setAuthTag()` and `setAAD` now return `this`. (Kirill Fomichev) #9398 - adding support for OPENSSL_CONF again (Sam Roberts) #11006 - make LazyTransform compabile with Streams1 (Matteo Collina) #12380 * deps: - upgrade libuv to 1.11.0 (cjihrig) #11094 - upgrade libuv to 1.10.2 (cjihrig) #10717 - upgrade libuv to 1.10.1 (cjihrig) #9647 - upgrade libuv to 1.10.0 (cjihrig) #9267 * dns: - Implemented `{ttl: true}` for `resolve4()` and `resolve6()` (Ben Noordhuis) #9296 * process: - add NODE_NO_WARNINGS environment variable (cjihrig) #10842 * readline: - add option to stop duplicates in history (Danny Nemer) #2982 * src: - support "--" after "-e" as end-of-options (John Barboza) #10651 * tls: - new tls.TLSSocket() supports sec ctx options (Sam Roberts) #11005 - Allow obvious key/passphrase combinations. (Sam Roberts) #10294 PR-URL: #13059

indutny · 2017-06-08T19:14:27Z

Any reason why we decided to not land it on v4?

indutny · 2017-06-08T20:42:32Z

Another sincere question, how to use this code on both v6.11.0 and v6.10.0 😉?

gibfahn · 2017-06-08T23:37:40Z

Any reason why we decided to not land it on v4?

Mostly because Node 4 is already in maintenance mode, and no-one was pushing really strongly for it.

MylesBorins · 2017-06-09T19:55:00Z

@indutny we are not doing minors for 4 after maintenance mode.

There is no way to run it on 6.10.0 without manually landing the patch yourself

indutny · 2017-06-09T19:58:34Z

@MylesBorins yeah, it is too late to ask for v4 indeed. I was just curious why it didn't get there in November 2016, when the change landed in master.

As for v6.10.0, here is what I has to come up with: indutny/redns@4fee7b4 . Guess it was mostly a rant about adding new signature to the API method that throws on older node.js versions.

indutny · 2017-06-09T19:59:07Z

Erm, perhaps I phrased it wrong. I mean that dns.resolve4(host, options, callback) works on v6.11.0 and throws on v6.10.0. This was a reason for my little rant.

MylesBorins · 2017-06-09T20:11:55Z

It didn't get in earlier because no one asked for it... the minors are primarily done based on commits that people request backports of. We ended up including this in 6.11 due to this.

Not really sure a better way to handle this for 6.10.0... definitely a valid point regarding new apis throwing on older versions. This is something we should keep in mind for future minors

Add an option to retrieve the Time-To-Live of the A record. PR-URL: nodejs/node#9296 Refs: nodejs/node#5893 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Add an option to retrieve the Time-To-Live of the AAAA record. PR-URL: nodejs/node#9296 Refs: nodejs/node#5893 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

This LTS release comes with 126 commits. This includes 40 which are test related, 32 which are doc related, 12 which are build / tool related and 4 commits which are updates to dependencies. Notable Changes: * build: - support for building mips64el (nanxiongchao) nodejs/node#10991 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) nodejs/node#10019 * crypto: - ability to select cert store at runtime (Adam Majer) nodejs/node#8334 - Use system CAs instead of using bundled ones (Adam Majer) nodejs/node#8334 - The `Decipher` methods `setAuthTag()` and `setAAD` now return `this`. (Kirill Fomichev) nodejs/node#9398 - adding support for OPENSSL_CONF again (Sam Roberts) nodejs/node#11006 - make LazyTransform compabile with Streams1 (Matteo Collina) nodejs/node#12380 * deps: - upgrade libuv to 1.11.0 (cjihrig) nodejs/node#11094 - upgrade libuv to 1.10.2 (cjihrig) nodejs/node#10717 - upgrade libuv to 1.10.1 (cjihrig) nodejs/node#9647 - upgrade libuv to 1.10.0 (cjihrig) nodejs/node#9267 * dns: - Implemented `{ttl: true}` for `resolve4()` and `resolve6()` (Ben Noordhuis) nodejs/node#9296 * process: - add NODE_NO_WARNINGS environment variable (cjihrig) nodejs/node#10842 * readline: - add option to stop duplicates in history (Danny Nemer) nodejs/node#2982 * src: - support "--" after "-e" as end-of-options (John Barboza) nodejs/node#10651 * tls: - new tls.TLSSocket() supports sec ctx options (Sam Roberts) nodejs/node#11005 - Allow obvious key/passphrase combinations. (Sam Roberts) nodejs/node#10294 PR-URL: nodejs/node#13059

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dns Issues and PRs related to the dns subsystem. labels Oct 26, 2016

bnoordhuis mentioned this pull request Oct 26, 2016
Native DNS caching for lookups #5893
Closed

PaquitoSoft mentioned this pull request Oct 27, 2016
Ttl cache from DNS server yahoo/dnscache#14
Open

silverwind added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 3, 2016

bnoordhuis force-pushed the dns-ttl branch from ab539df to 2c63621Compare November 15, 2016 18:34

jasnell approved these changes Nov 18, 2016
View reviewed changes

silverwind approved these changes Nov 18, 2016
View reviewed changes

bnoordhuis added 2 commits November 18, 2016 22:01

dns: implement {ttl: true} for dns.resolve4()
c31b507
Add an option to retrieve the Time-To-Live of the A record. PR-URL: nodejs#9296 Refs: nodejs#5893 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

dns: implement {ttl: true} for dns.resolve6()
6b1a8d0
Add an option to retrieve the Time-To-Live of the AAAA record. PR-URL: nodejs#9296 Refs: nodejs#5893 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

bnoordhuis force-pushed the dns-ttl branch from 2c63621 to 6b1a8d0Compare November 18, 2016 21:03

bnoordhuis closed this Nov 18, 2016

bnoordhuis deleted the dns-ttl branch November 18, 2016 21:03

bnoordhuis merged commit 6b1a8d0 into nodejs:masterNov 18, 2016

Fishrock123 mentioned this pull request Nov 22, 2016
v7.2.0 proposal #9745
Merged
2 tasks

gibfahn mentioned this pull request Apr 24, 2017
Potential Semver Minor Backports nodejs/Release#188
Closed
31 tasks

MylesBorins added the land-on-v6.x label May 16, 2017

MylesBorins mentioned this pull request May 23, 2017
v6.11.0 proposal #13059
Merged

zllovesuki mentioned this pull request Nov 6, 2018
TCP transporter IP bug moleculerjs/moleculer#380
Closed

sindresorhus mentioned this pull request Nov 14, 2018
DNS cache sindresorhus/got#661
Closed

karolz-ms mentioned this pull request Oct 21, 2019
Consider using dnscache for caching DNS requests apocas/docker-modem#111
Open

This was referenced Aug 8, 2021
[dns] - resolveTxt ttl flag #39528
Closed
dns: TTL for TXT entries #39399
Closed

snyk-bot mentioned this pull request Jan 11, 2023
[Snyk] Fix for 1 vulnerabilities adamlaska/node#99
Open

Uh oh!

Uh oh!

Conversation

bnoordhuis commented Oct 26, 2016

Uh oh!

jasnell commented Oct 26, 2016

Uh oh!

cjihrig commented Oct 26, 2016

Uh oh!

silverwind commented Oct 27, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented Oct 31, 2016

Uh oh!

silverwind commented Oct 31, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented Nov 1, 2016

Uh oh!

jasnell commented Nov 2, 2016

Uh oh!

silverwind commented Nov 2, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverwind commented Nov 3, 2016

Uh oh!

bnoordhuis commented Nov 3, 2016

Uh oh!

silverwind commented Nov 3, 2016

Uh oh!

bnoordhuis commented Nov 15, 2016

Uh oh!

tommarien commented May 18, 2017

Uh oh!

addaleax commented May 18, 2017

Uh oh!

tommarien commented May 18, 2017

Uh oh!

indutny commented Jun 8, 2017

Uh oh!

indutny commented Jun 8, 2017

Uh oh!

gibfahn commented Jun 8, 2017

Uh oh!

MylesBorins commented Jun 9, 2017

Uh oh!

indutny commented Jun 9, 2017

Uh oh!

indutny commented Jun 9, 2017

Uh oh!

MylesBorins commented Jun 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

silverwind commented Oct 27, 2016•
edited
Loading

silverwind commented Oct 31, 2016•
edited
Loading

silverwind commented Nov 2, 2016•
edited
Loading