dns: add resolveAny support by XadillaX · Pull Request #13137 · nodejs/node

XadillaX · 2017-05-21T10:11:47Z

dns.resolveAny and dns.resolve with "ANY" has the similar behavior like $ dig <domain> any and returns an array with several types of records.

dns.resolveAny parses the result packet by several rules in turn.

Refs: #2848

Supported Types

Checklist

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

Affected core subsystem(s)

dns

silverwind · 2017-05-21T11:27:22Z

Thanks for tackling this. It also needs a doc update in the table here and a new section for resolveAny below it.

XadillaX · 2017-05-21T11:34:30Z

@silverwind thanks for mentioning it. I'm still working on implementing this feature.

XadillaX · 2017-05-22T06:47:39Z

@silverwind I've already added the document now.

cjihrig

A few comments on the test. Haven't reviewed the whole thing yet.

cjihrig · 2017-05-22T14:40:51Z

This should come right after the 'use strict'; directive.

cjihrig · 2017-05-22T14:42:21Z

Can you split these complex assertions into smaller individual assertions.

cjihrig · 2017-05-22T14:44:21Z

Why can't you call r.forEach() directly?

Because r is an array-liked object, not a real array instance.
{'0': 'foo','1': 'bar','length': 2,'type': 'TXT'}

cjihrig · 2017-05-22T14:44:50Z

Can you drop the console.log().

Ah, I forgot to delete it. sorry.

jasnell · 2017-05-22T14:50:39Z

Using the table to illustrate the structure is a bit unclear.

How about using list? or something else?

Not sure... the table may be ok if the sentence leading into it is changed up a bit. Not having any specific ideas on how to improve it tho. It just reads a bit awkward.

Hmmm... So I gave an example following.

jasnell · 2017-05-22T14:51:16Z

Following is a example of the `ret` object passed to the callback:

`dns.resolveAny` and `dns.resolve` with `"ANY"` has the similar behavior like `$ dig <domain> any` and returns an array with several types of records. `dns.resolveAny` parses the result packet by several rules in turn. Supported types: * A * AAAA * CNAME * MX * NAPTR * NS * PTR * SOA * SRV * TXT Refs: nodejs#2848

Trott · 2017-05-23T03:37:21Z

Based on git shortlog results: /cc @bnoordhuis @trevnorris @indutny

XadillaX · 2017-05-25T08:05:51Z

anyone has time to review this PR? thx.

/cc @bnoordhuis @trevnorris @indutny @addaleax

silverwind · 2017-05-25T15:44:36Z

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

addaleax · 2017-05-25T16:37:57Z

+ (((unsigned int)((unsigned char)(p)[0]) << 24U) | \
+ ((unsigned int)((unsigned char)(p)[1]) << 16U) | \
+ ((unsigned int)((unsigned char)(p)[2]) << 8U) | \
+ ((unsigned int)((unsigned char)(p)[3]))))


Can you use inline functions rather than macros, and static casts rather than C-style casts?

addaleax · 2017-05-25T16:39:16Z

 };

+Local<Array> AddrTTLToArray(Environment* env,
+ ares_addrttl* addrttls,


const ares_addrttl*?

addaleax · 2017-05-25T16:39:22Z


+Local<Array> AddrTTLToArray(Environment* env,
+ ares_addrttl* addrttls,
+ int naddrttls) {


naddrttls should be int because ares_parse_a_reply needs naddrttls be an integer but not an unsigned integer.

addaleax · 2017-05-25T16:40:03Z

+
+ Local<Array> ttls = Array::New(isolate, naddrttls);
+ auto context = env->context();
+ for (int i = 0; i < naddrttls; i += 1) {


size_t (and btw, we tend to use i++ here)

addaleax · 2017-05-25T16:40:53Z

+
+Local<Array> AddrTTL6ToArray(Environment* env,
+ ares_addr6ttl* addrttls,
+ int naddrttls) {


Ditto – I think it’s safe to merge these two methods into a template, by the way.

addaleax · 2017-05-25T16:41:48Z

+ status = ares_parse_a_reply(buf,
+ len,
+ &host,
+ reinterpret_cast<ares_addrttl*>(addrttls),


There’s no need to reinterpret_cast here, static_cast should be enough

addaleax · 2017-05-25T16:43:18Z

+
+
+int ParseGeneralReply(Environment* env,
+ unsigned char* buf,


buf can be const unsigned char*, right?

addaleax · 2017-05-25T16:44:38Z

+
+ /* If it's `CNAME`, return the CNAME value */
+ /* And if it's `CNAME_OR_A` and it has value in `h_name` and `h_aliases[0]` */
+ /* We consider it's a CNAME record, otherwise we consider it's an A record */


This continues the sentence from the line before, right? Could you start it with a lowercase w then? And maybe consider folding the comments into a single multi-line comment, i.e.
/* foo * bar * baz */

addaleax · 2017-05-25T16:45:20Z

+
+
+int ParseMxReply(Environment* env,
+ unsigned char* buf,


addaleax · 2017-05-25T16:48:49Z

+ mx_record->Set(exchange_symbol,
+ OneByteString(env->isolate(), current->host));
+ mx_record->Set(priority_symbol,
+ Integer::New(env->isolate(), current->priority));


Regarding all uses of Set(): Can you use the overload that returns a Maybe<bool> and check the result?

By using FromJust()?

silverwind

Left some docs comments

silverwind · 2017-05-26T17:36:05Z

"to resolve all records (also known as ANY or * query)"
(This is taken out of the rfc - 255 A request for all records)

silverwind · 2017-05-26T17:37:12Z

Maybe replace the last sentence with: "Depending on the type, additional properties will be present on the object:

silverwind · 2017-05-26T17:42:05Z

Why not return a regular array (e.g. where Array.isArray(array) returns true)?

Because we need the extra column type.
You mean create an array with extra type directly?
eg.
constarr=['foo','bar'];arr.type='TXT';

Right, seems reasonable to keep it that way.

typo: array-like
I think this should be an array; if necessary, you can make it an entries: […] property on the object itself.

silverwind · 2017-05-26T17:42:55Z

"Here is ..." might be better

silverwind · 2017-05-26T17:44:01Z

Maybe remove this comment, while correct, it seems unrelated here.

silverwind · 2017-05-26T17:44:26Z

Can you structure this example to type is sorted first in each object?

XadillaX · 2017-05-27T03:34:52Z

Should this PR backport to 6.x and 4.x?

`dns.resolveAny` and `dns.resolve` with `"ANY"` has the similar behavior like `$ dig <domain> any` and returns an array with several types of records. `dns.resolveAny` parses the result packet by several rules in turn. Supported types: * A * AAAA * CNAME * MX * NAPTR * NS * PTR * SOA * SRV * TXT Fixes: #2848 PR-URL: #13137 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Notable changes: * **Async Hooks** * Multiple improvements to Promise support in `async_hooks` have been made. * **Build** * The compiler version requirement to build Node with GCC has been raised to GCC 4.9.4. [[`23d41f3118`](2abaa86ba8)] [#13466](#13466) * **DNS** * The server used for DNS queries can now use a custom port. [[`2bb6614904`](8506acc1b5)] [#13723](#13723) * Support for `dns.resolveAny()` has been added. [[`30bc9dc20f`](30bc9dc20f)] [#13137](#13137) * **V8** * The V8 engine has been upgraded to version 5.9, which has a significantly changed performance profile. [#13515](#13515) PR-URL: #13744

`dns.resolveAny` and `dns.resolve` with `"ANY"` has the similar behavior like `$ dig <domain> any` and returns an array with several types of records. `dns.resolveAny` parses the result packet by several rules in turn. Supported types: * A * AAAA * CNAME * MX * NAPTR * NS * PTR * SOA * SRV * TXT Fixes: #2848 PR-URL: #13137 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Notable changes: * **Async Hooks** * Multiple improvements to Promise support in `async_hooks` have been made. * **Build** * The compiler version requirement to build Node with GCC has been raised to GCC 4.9.4. [[`23d41f3118`](2abaa86ba8)] [#13466](#13466) * **DNS** * The server used for DNS queries can now use a custom port. [[`2bb6614904`](8506acc1b5)] [#13723](#13723) * Support for `dns.resolveAny()` has been added. [[`30bc9dc20f`](30bc9dc20f)] [#13137](#13137) * **V8** * The V8 engine has been upgraded to version 5.9, which has a significantly changed performance profile. [#13515](#13515) PR-URL: #13744

`dns.resolveAny` and `dns.resolve` with `"ANY"` has the similar behavior like `$ dig <domain> any` and returns an array with several types of records. `dns.resolveAny` parses the result packet by several rules in turn. Supported types: * A * AAAA * CNAME * MX * NAPTR * NS * PTR * SOA * SRV * TXT Fixes: #2848 PR-URL: #13137 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Notable changes: * **Async Hooks** * Multiple improvements to Promise support in `async_hooks` have been made. * **Build** * The compiler version requirement to build Node with GCC has been raised to GCC 4.9.4. [[`23d41f3118`](2abaa86ba8)] [#13466](#13466) * **DNS** * The server used for DNS queries can now use a custom port. [[`2bb6614904`](8506acc1b5)] [#13723](#13723) * Support for `dns.resolveAny()` has been added. [[`30bc9dc20f`](30bc9dc20f)] [#13137](#13137) * **V8** * The V8 engine has been upgraded to version 5.9, which has a significantly changed performance profile. [#13515](#13515) PR-URL: #13744

This is a bit of a check to see how people feel about having this kind of test. Ref: nodejs#13137 PR-URL: nodejs#13883 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

`dns.resolveAny` and `dns.resolve` with `"ANY"` has the similar behavior like `$ dig <domain> any` and returns an array with several types of records. `dns.resolveAny` parses the result packet by several rules in turn. Supported types: * A * AAAA * CNAME * MX * NAPTR * NS * PTR * SOA * SRV * TXT Fixes: #2848 PR-URL: #13137 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Notable changes: * **Async Hooks** * Multiple improvements to Promise support in `async_hooks` have been made. * **Build** * The compiler version requirement to build Node with GCC has been raised to GCC 4.9.4. [[`820b011ed6`](820b011ed6)] [#13466](#13466) * **Cluster** * Users now have more fine-grained control over the inspector port used by individual cluster workers. Previously, cluster workers would simply increment from the master's debug port. [[`dfc46e262a`](dfc46e262a)] [#14140](#14140) * **DNS** * The server used for DNS queries can now use a custom port. [[`ebe7bb29aa`](ebe7bb29aa)] [#13723](#13723) * Support for `dns.resolveAny()` has been added. [[`6e30e2558e`](6e30e2558e)] [#13137](#13137) * **npm** * The `npm` CLI has been updated to version 5.3.0. In particular, it now comes with the `npx` binary, which is also shipped with Node. [[`dc3f6b9ac1`](dc3f6b9ac1)] [#14235](#14235) * `npm` Changelogs: - [v5.0.4](https://github.com/npm/npm/releases/tag/v5.0.4) - [v5.1.0](https://github.com/npm/npm/releases/tag/v5.1.0) - [v5.2.0](https://github.com/npm/npm/releases/tag/v5.2.0) - [v5.3.0](https://github.com/npm/npm/releases/tag/v5.3.0) PR-URL: #13744

@addaleax

Big thanks to @addaleax who prepared the vast majority of this release. Notable changes: * **Async Hooks** * Multiple improvements to Promise support in `async_hooks` have been made. * **Build** * The compiler version requirement to build Node with GCC has been raised to GCC 4.9.4. [[`820b011ed6`](nodejs@820b011ed6)] [nodejs#13466](nodejs#13466) * **Cluster** * Users now have more fine-grained control over the inspector port used by individual cluster workers. Previously, cluster workers would simply increment from the master's debug port. [[`dfc46e262a`](nodejs@dfc46e262a)] [nodejs#14140](nodejs#14140) * **DNS** * The server used for DNS queries can now use a custom port. [[`ebe7bb29aa`](nodejs@ebe7bb29aa)] [nodejs#13723](nodejs#13723) * Support for `dns.resolveAny()` has been added. [[`6e30e2558e`](nodejs@6e30e2558e)] [nodejs#13137](nodejs#13137) * **npm** * The `npm` CLI has been updated to version 5.3.0. In particular, it now comes with the `npx` binary, which is also shipped with Node. [[`dc3f6b9ac1`](nodejs@dc3f6b9ac1)] [nodejs#14235](nodejs#14235) * `npm` Changelogs: - [v5.0.4](https://github.com/npm/npm/releases/tag/v5.0.4) - [v5.1.0](https://github.com/npm/npm/releases/tag/v5.1.0) - [v5.2.0](https://github.com/npm/npm/releases/tag/v5.2.0) - [v5.3.0](https://github.com/npm/npm/releases/tag/v5.3.0) PR-URL: nodejs#13744

@addaleax

Big thanks to @addaleax who prepared the vast majority of this release. Notable changes: * **Async Hooks** * Multiple improvements to Promise support in `async_hooks` have been made. * **Build** * The compiler version requirement to build Node with GCC has been raised to GCC 4.9.4. [[`820b011ed6`](820b011ed6)] [#13466](#13466) * **Cluster** * Users now have more fine-grained control over the inspector port used by individual cluster workers. Previously, cluster workers would simply increment from the master's debug port. [[`dfc46e262a`](dfc46e262a)] [#14140](#14140) * **DNS** * The server used for DNS queries can now use a custom port. [[`ebe7bb29aa`](ebe7bb29aa)] [#13723](#13723) * Support for `dns.resolveAny()` has been added. [[`6e30e2558e`](6e30e2558e)] [#13137](#13137) * **npm** * The `npm` CLI has been updated to version 5.3.0. In particular, it now comes with the `npx` binary, which is also shipped with Node. [[`dc3f6b9ac1`](dc3f6b9ac1)] [#14235](#14235) * `npm` Changelogs: - [v5.0.4](https://github.com/npm/npm/releases/tag/v5.0.4) - [v5.1.0](https://github.com/npm/npm/releases/tag/v5.1.0) - [v5.2.0](https://github.com/npm/npm/releases/tag/v5.2.0) - [v5.3.0](https://github.com/npm/npm/releases/tag/v5.3.0) PR-URL: #13744

@addaleax

Big thanks to @addaleax who prepared the vast majority of this release. Notable changes: * **Async Hooks** * Multiple improvements to Promise support in `async_hooks` have been made. * **Build** * The compiler version requirement to build Node with GCC has been raised to GCC 4.9.4. [[`820b011ed6`](820b011ed6)] [#13466](#13466) * **Cluster** * Users now have more fine-grained control over the inspector port used by individual cluster workers. Previously, cluster workers would simply increment from the master's debug port. [[`dfc46e262a`](dfc46e262a)] [#14140](#14140) * **DNS** * The server used for DNS queries can now use a custom port. [[`ebe7bb29aa`](ebe7bb29aa)] [#13723](#13723) * Support for `dns.resolveAny()` has been added. [[`6e30e2558e`](6e30e2558e)] [#13137](#13137) * **npm** * The `npm` CLI has been updated to version 5.3.0. In particular, it now comes with the `npx` binary, which is also shipped with Node. [[`dc3f6b9ac1`](dc3f6b9ac1)] [#14235](#14235) * `npm` Changelogs: - [v5.0.4](https://github.com/npm/npm/releases/tag/v5.0.4) - [v5.1.0](https://github.com/npm/npm/releases/tag/v5.1.0) - [v5.2.0](https://github.com/npm/npm/releases/tag/v5.2.0) - [v5.3.0](https://github.com/npm/npm/releases/tag/v5.3.0) PR-URL: #13744

This is a bit of a check to see how people feel about having this kind of test. Ref: #13137 PR-URL: #13883 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

gibfahn · 2018-01-15T21:51:54Z

Release team were -1 on landing this in 6.x

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. dns Issues and PRs related to the dns subsystem. labels May 21, 2017

XadillaX force-pushed the feature/dns-resolve-any branch from 85adf13 to c4cb648Compare May 21, 2017 10:29

XadillaX mentioned this pull request May 21, 2017
Add dns.resolve that returns all results #2848
Closed

silverwind added the semver-minor PRs that contain new features and should be released in the next minor version. label May 21, 2017

XadillaX force-pushed the feature/dns-resolve-any branch from c4cb648 to a161474Compare May 21, 2017 13:25

mscdex added the wip Issues and PRs that are still a work in progress. label May 21, 2017

XadillaX force-pushed the feature/dns-resolve-any branch 3 times, most recently from 1c4fddb to acc9637Compare May 22, 2017 06:46

XadillaX changed the title ~~[WIP] dns: add resolveAny support~~dns: add resolveAny supportMay 22, 2017

XadillaX force-pushed the feature/dns-resolve-any branch from acc9637 to 3643ebeCompare May 22, 2017 09:07

cjihrig reviewed May 22, 2017
View reviewed changes

jasnell reviewed May 22, 2017
View reviewed changes

Comment threaddoc/api/dns.md Outdated
jasnellMay 22, 2017
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following is a example of the `ret` object passed to the callback:

XadillaX force-pushed the feature/dns-resolve-any branch from 3643ebe to e44d750Compare May 22, 2017 15:12

jasnell approved these changes May 25, 2017
View reviewed changes

addaleax removed the wip Issues and PRs that are still a work in progress. label May 25, 2017

addaleax reviewed May 25, 2017
View reviewed changes

silverwind reviewed May 26, 2017
View reviewed changes

XadillaX force-pushed the feature/dns-resolve-any branch from f95255d to f11a55fCompare May 27, 2017 02:02

addaleax mentioned this pull request Jun 22, 2017
test: add non-internet resolveAny tests #13883
Closed
3 tasks

addaleax mentioned this pull request Jun 24, 2017
v8.2.0 proposal #13744
Merged

silverwind mentioned this pull request Aug 6, 2017
dns: support AXFR queries #14648
Closed

gibfahn added the dont-land-on-v6.x label Jan 15, 2018

Uh oh!

Uh oh!

Conversation

XadillaX commented May 21, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Supported Types

Checklist

Affected core subsystem(s)

Uh oh!

silverwind commented May 21, 2017

Uh oh!

XadillaX commented May 21, 2017

Uh oh!

XadillaX commented May 22, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented May 23, 2017

Uh oh!

XadillaX commented May 25, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverwind commented May 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

XadillaX commented May 21, 2017•
edited
Loading

XadillaX commented May 22, 2017•
edited
Loading

XadillaX commented May 25, 2017•
edited
Loading