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 - add 'all' option to dns.lookup #744

Closed
wants to merge 1 commit into from

Conversation

silverwind
Copy link
Contributor

Issue: #736

Open questions:

  • Should dns.lookup(null, {all: true}) return an empty array (returns null right now)?
  • I assume c-ares can't return mixed 4/6 results, as the callback just gets addresses. Correct?
  • Test: Is it possible to reliably test the !this.family code path?

/cc @bnoordhuis

if (err) {
return this.callback(errnoException(err, 'getaddrinfo', this.hostname));
}
this.callback(null, addresses.map(function(addr) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you do this with a for loop instead of map() for performance reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, if the difference is worthwile.

@bnoordhuis
Copy link
Member

Should dns.lookup(null, {all: true}) return an empty array (returns null right now) ?

That seems reasonable.

I assume C-Ares can't return mixed 4/6 results, as the callback just gets addresses. Correct?

Wrong. :-)

The logic is in AfterGetAddrInfo() in src/cares_wrap.cc and probably needs to be adjusted in this PR. In a nutshell, you can tell c-ares to query for IPv4 and IPv6 by setting the address family to AF_UNSPEC (family=0 in JS.)

Test: Is it possible to reliably test the !this.family code path?

Not easily, I think.

if (self.family) {
return { address: addr, family: self.family };
} else {
return { address: addr, family: addr.indexOf(':') >= 0 ? 6 : 4 };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addr.includes(':')?

EDIT: Never mind, just noticed that onlookup() does the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 6, 2015

@bnoordhuis why does the C++ logic need to change? It's returning the correct array based on the desired family. The JS logic is just giving all the results back instead of the first result.

@bnoordhuis
Copy link
Member

@cjihrig Please disregard that remark. I wrote that before I realized that lib/dns.js already does an ersatz IPv6 check.

@@ -471,6 +471,42 @@ TEST(function test_lookup_localhost_ipv4(done) {
});


TEST(function test_lookup_all_ipv4(done) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test that doesn't specify the family and check for mixed results.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean fail the test if mixed results are obtained?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. If you don't specify the family, you should be able to get all IPv4 and IPv6 addresses.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what you mean:

> require("dns").lookup("www.google.com", {all: true}, console.log)
null [ { address: '188.21.9.123', family: 4 },
  { address: '188.21.9.117', family: 4 },
  { address: '188.21.9.121', family: 4 },
  { address: '188.21.9.122', family: 4 },
  { address: '188.21.9.118', family: 4 },
  { address: '188.21.9.119', family: 4 },
  { address: '188.21.9.116', family: 4 },
  { address: '188.21.9.120', family: 4 },
  { address: '2a00:1450:4014:80a::1010', family: 6 } ]

@silverwind
Copy link
Contributor Author

So, no change need there? I see that family = 0 can reach cares.getaddrinfo.

`getaddrinfo` flags. If `hints` is not provided, then no flags are passed to
`getaddrinfo`. Multiple flags can be passed through `hints` by logically
`OR`ing their values.
* `all`: {Boolean} - Instead of returning a single address in the callback,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add Defaults to false.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This describes how the callback arguments are changed, but doesn't really describe the purpose of all. Can you add something like "Return all matching addresses when true"

@bnoordhuis
Copy link
Member

So, no change need there? I see that family = 0 can reach cares.getaddrinfo.

Yep, it's fine.

@silverwind
Copy link
Contributor Author

Mixed test done. Had to fight a few other test failing, but both appear to be because of my specific dns setup. Just to note, the failing tests on OS X were:

  • test_resolve_failure: returned ENODATA instead of ENOTFOUND / ESERVFAIL
  • test_reverse_failure: my router did reverse-resolve 0.0.0.0 because I have intercepting adblocking through DNS in place.

the arguments change to `(err, addresses)`, with `addresses` being an array of
objects with the properties `address` and `family`.

`address` is a string representation of a IP v4 or v6 address. `family` is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go before the With the all option set,... sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole paragraph?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I think it's helpful to define the default behavior before going into what it means for all to be set.

@silverwind
Copy link
Contributor Author

All done. I restructured the docs to make more sense in the last commit.

Let me know when to squash, as I'd like to fix the commit message.

return this.callback(errnoException(err, 'getaddrinfo', this.hostname));
}

for (i = 0; i < addresses.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny style nit, but can you put the var here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Generally, I try to put var at the start of the scope, but i guess I'll follow the style of that file.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 6, 2015

There is one case that hasn't been covered - https://github.com/iojs/io.js/blob/v1.x/lib/dns.js#L130. Other than that, and the style nit, LGTM. You can squash, the commits and follow the instructions in https://github.com/iojs/io.js/blob/v1.x/CONTRIBUTING.md#step-3-commit

@silverwind
Copy link
Contributor Author

@cjihrig honored your last suggestions and also added two more tests. do these look good to you so I can squash?

var req = dns.lookup(null, {all: true}, function(err, ips, family) {
if (err) throw err;
assert.strictEqual(ips.length, 0);
assert.ok(Array.isArray(ips));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't matter a ton since this is a test, but I think you'd want to check for Array.isArray(ips) before checking the ips.length.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I'll reorder the type checks to be on top.

The 'all' option allows to retrieve all addresses returned by
`getaddrinfo` using the system resolver. While `dns.resolve`
already does return multiple results, it's often preferable to
use the system resolver, like for mDNS.
@silverwind
Copy link
Contributor Author

And it's squashed.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 6, 2015

Let's see what the CI thinks. https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/154/

I'll do some testing locally too. I don't think the CI runs the internet tests.

@cjihrig cjihrig added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 6, 2015
@cjihrig
Copy link
Contributor

cjihrig commented Feb 6, 2015

CI seems happy - the FreeBSD and Arm failures are pretty standard. I also ran the Internet tests locally, and they passed. This is a semver minor bump, but that window is already open due to #697.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 6, 2015

Thanks! Landed in eb30009de65a9432b59ae77a425b623df28dbd27 with a tweaked commit message.

EDIT: Someone performed a force push after I landed this. So the mentioned commit was overwritten. The actual commit is now 633a990

@cjihrig cjihrig closed this Feb 6, 2015
cjihrig pushed a commit that referenced this pull request Feb 6, 2015
This commit adds the 'all' option to dns.lookup(), allowing
all lookup results to be returned.

Semver: Minor
Fixes: #736
PR-URL: #744
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@micnic
Copy link
Contributor

micnic commented Feb 6, 2015

@cjihrig, oops, it was me, sorry for that, I edited a wrong commit message

@cjihrig
Copy link
Contributor

cjihrig commented Feb 6, 2015

@micnic no big deal. It's taken care of.

@rvagg rvagg mentioned this pull request Feb 9, 2015
rvagg added a commit that referenced this pull request Feb 11, 2015
Notable changes:

* stream:
  - Simpler stream construction, see
    nodejs/readable-stream#102 for details.
    This extends the streams base objects to make their constructors
    accept default implementation methods, reducing the boilerplate
    required to implement custom streams. An updated version of
    readable-stream will eventually be released to match this change
    in core. (@sonewman)
* dns:
  - `lookup()` now supports an `'all'` boolean option, default to
    `false` but when turned on will cause the method to return an
    array of *all* resolved names for an address, see,
    #744 (@silverwind)
* assert:
  - Remove `prototype` property comparison in `deepEqual()`,
    considered a bugfix, see #636
    (@vkurchatkin)
  - Introduce a `deepStrictEqual()` method to mirror `deepEqual()`
    but performs strict equality checks on primitives, see
    #639 (@vkurchatkin)
* **tracing**:
  - Add LTTng (Linux Trace Toolkit Next Generation) when compiled
    with the  `--with-lttng` option. Trace points match those
    available for DTrace and ETW.
    #702 (@thekemkid)
* npm upgrade to 2.5.1
* **libuv** upgrade to 1.4.0
* Add new collaborators:
  - Aleksey Smolenchuk (@lxe)
  - Shigeki Ohtsu (@shigeki)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants