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.

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: implement {ttl: true} for dns.resolve4() and dns.resolve6() #9296

Merged
merged 2 commits into from
Nov 18, 2016

Conversation

bnoordhuis
Copy link
Member

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 } ]

@nodejs-github-bot 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
@jasnell
Copy link
Member

jasnell commented Oct 26, 2016

SGTM

@cjihrig
Copy link
Contributor

cjihrig commented Oct 26, 2016

SGTM too

@silverwind
Copy link
Contributor

silverwind commented Oct 27, 2016

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

@bnoordhuis
Copy link
Member Author

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
Copy link
Contributor

silverwind commented Oct 31, 2016

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
Copy link
Member Author

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
Copy link
Member

jasnell commented Nov 2, 2016

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
Copy link
Contributor

silverwind commented Nov 2, 2016

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
Copy link
Contributor

@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.

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

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

@silverwind
Copy link
Contributor

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

@bnoordhuis
Copy link
Member Author

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 <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
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 <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@bnoordhuis bnoordhuis closed this Nov 18, 2016
@bnoordhuis bnoordhuis deleted the dns-ttl branch November 18, 2016 21:03
@bnoordhuis bnoordhuis merged commit 6b1a8d0 into nodejs:master Nov 18, 2016
Fishrock123 pushed a commit that referenced this pull request Nov 22, 2016
Add an option to retrieve the Time-To-Live of the A record.

PR-URL: #9296
Refs: #5893
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Nov 22, 2016
Add an option to retrieve the Time-To-Live of the AAAA record.

PR-URL: #9296
Refs: #5893
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Nov 22, 2016
2 tasks
Fishrock123 added a commit that referenced this pull request Nov 22, 2016
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
Fishrock123 added a commit that referenced this pull request Nov 22, 2016
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
imyller added a commit to imyller/meta-nodejs that referenced this pull request Nov 28, 2016
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 <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 16, 2017
Add an option to retrieve the Time-To-Live of the A record.

PR-URL: #9296
Refs: #5893
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 16, 2017
Add an option to retrieve the Time-To-Live of the AAAA record.

PR-URL: #9296
Refs: #5893
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 18, 2017
Add an option to retrieve the Time-To-Live of the A record.

PR-URL: #9296
Refs: #5893
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 18, 2017
Add an option to retrieve the Time-To-Live of the AAAA record.

PR-URL: #9296
Refs: #5893
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@tommarien
Copy link

Is this ever going to be backported to node 6 ?

@addaleax
Copy link
Member

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

@tommarien
Copy link

@addaleax That would be marvellous ;)

@MylesBorins MylesBorins mentioned this pull request May 23, 2017
MylesBorins added a commit that referenced this pull request Jun 6, 2017
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
MylesBorins added a commit that referenced this pull request Jun 6, 2017
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
Copy link
Member

indutny commented Jun 8, 2017

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

@indutny
Copy link
Member

indutny commented Jun 8, 2017

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

@gibfahn
Copy link
Member

gibfahn commented Jun 8, 2017

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
Copy link
Contributor

@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
Copy link
Member

indutny commented Jun 9, 2017

@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
Copy link
Member

indutny commented Jun 9, 2017

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
Copy link
Contributor

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

andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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 <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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 <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. dns Issues and PRs related to the dns subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants