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

Add version of dns.lookup that returns all results #736

Closed
jsha opened this issue Feb 6, 2015 · 12 comments
Closed

Add version of dns.lookup that returns all results #736

jsha opened this issue Feb 6, 2015 · 12 comments
Labels
dns Issues and PRs related to the dns subsystem.

Comments

@jsha
Copy link
Contributor

jsha commented Feb 6, 2015

The dns module provides two sets of methods: the resolve family, which directly resolves using the DNS protocol, and the lookup function, which uses the system resolver (through getaddrinfo) and returns at most one address.

It's often preferable to use the system resolver, since it can use things like mDNS. However, using the dns.lookup API makes it impossible to receive multiple IP addresses in response to a DNS query (See #708 for motivating example).

Proposal: Add a new function dns.lookupAll that behaves like dns.lookup, but returns all addresses given by getaddrinfo.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 6, 2015

This seems like a pretty simple addition. I'd be fine with working on it, if others find it useful. I think the window is currently open for a minor version bump too.

@brendanashworth brendanashworth added the dns Issues and PRs related to the dns subsystem. label Feb 6, 2015
@bnoordhuis
Copy link
Member

This seems like a perfectly reasonable addition to me. It doesn't even need a new function; dns.lookup() already looks up all the addresses but it's intentionally crippled to only report the first one. We can simply extend it like this:

dns.lookup('google.com', { all: true }, function(err, addrs) { /* ... */ });  // strawman

The callback gets the address family as its third argument now. For the { all: true } case, we probably want to drop that and report the family in the addrs array. It's a list of strings now, that would have to change to a list of objects.

@silverwind
Copy link
Contributor

I'll start working on this according to @bnoordhuis's proposal, if that's ok with you, @cjihrig.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 6, 2015

@silverwind sure. I was thinking you might just want to change this line to point to another function. And the callback would change from function(err, address, family) to function(err, addresses), where each element in addresses would be something like {ip: '127.0.0.1', family: 4}.

@silverwind
Copy link
Contributor

My first approach would've been to modify onlookup with additional logic and add a req.all property. You think that a second function would be cleaner?

@cjihrig
Copy link
Contributor

cjihrig commented Feb 6, 2015

I figured it would be simpler, with less chance of breaking existing code, to just point to a new function. The two functions will have very little overlap except checking for an error.

@silverwind
Copy link
Contributor

True, I'll add an second function then, definitely safer.

@silverwind
Copy link
Contributor

Any opinions if the address field in the object should be called address or ip? I'm leaning more towards address for correctness, but @cjihrig suggested ip. Also, the docs usually call it address.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 6, 2015

FWIW, I don't have a strong opinion. I just thought addresses.address seemed a little odd.

@bnoordhuis
Copy link
Member

We call it address in most other places, e.g. socket.address().address.

@silverwind
Copy link
Contributor

I'll put this up shortly for a PR, just have to test on OS X, as apparently my Windows machine isn't able to dns.lookup for an IPv6 (getting ENOENT), but dns.resolve6 strangely works. This is on v1.x too. Might be a separate issue.

@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 as completed Feb 6, 2015
cjihrig pushed a commit that referenced this issue 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns Issues and PRs related to the dns subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants