Review requested:

• @nodejs/net

Thanks for the reviews. My schedule got a little busy; I'll address them later this week.

One thing that has occurred to me: I think I misunderstood the exact behavior of ares_set_local_ip4 and ares_set_local_ip6. I thought they set a single source address option, but it looks like they maintain separate settings for IPv4 and IPv6 and the source address used depends on the IP version of the server used at resolve time.

If you wanted to set both IPv4 and v6 source addresses with the current implementation of this PR, one could do something like

resolver.setLocalAddress('127.0.0.1');
resolver.setLocalAddress('::1');

…but that seems a little goofy. Any suggestions for a good API? I'm thinking:

resolver.setLocalAddress('127.0.0.1'); // sets IPv4, clears IPv6
resolver.setLocalAddress('::1'); // sets IPv6, clears IPv4
resolver.setLocalAddress('127.0.0.1', '::1'); // sets both
resolver.setLocalAddress(null); // clears both?  or just '0.0.0.0' instead of null?
// or should we require both v4 and v6 args to avoid ambiguity when calling with a single arg?

That's a good point. Borrowing from python, explicit > implicit, so maybe this?

resolver.setLocalAddressIPv4('127.0.0.1')
resolver.setLocalAddressIPv6('::1')

resolver.setLocalAddressIPv4('::1')  // throws
resolver.setLocalAddressIPv6('127.0.0.1') // ditto

Should probably add this as dns.setLocalAddress and make Resolver inherit it so it works on both cases, like dns.setServers. Also for parity with dig, it would be nice to also have the ability to set the source port, so a host:port syntax may be appropriate. Also, I think we can call it setSource(ip, port).

       -b address[#port]
          Set the source IP address of the query. The address must be a valid address on one of the host's
          network interfaces, or "0.0.0.0" or "::". An optional port may be specified by appending "#<port>"

set both IPv4 and v6 source addresses

Why would you want to do that? a packet can only have one source address.

Our handling of the differences between ipv4 and ipv6 are way too consistent across the codebase. Sometimes we use argument/option values like IPv4 and IPv6, other places we use things like udp4 and udp6. For the net.BlockList, I opted to go for an argument 'ipv4' and 'ipv6' and will be opening a PR that makes those case-insensitive. Let's standardize on that approach.

resolver.setLocalAddress('127.0.0.1') // defaults to ipv4
resolver.setLocalAddress('127.0.0.1', 'ipv4')
resolver.setLocalAddress('::1', 'ipv6')

I've pushed commits addressing the review comments above.

On the topic of what exactly the JS API should look like, I'm not in love with the idea of calling the same method multiple times since it is ambiguous to the caller how exactly the internals handle those calls. Does the IPv6 call replace the IPv4 setting or are they managed separately? If a user only sets an IPv4 address, will they be surprised when a v6 request goes over the default interface?

This could be solved in the documentation, but I think it's preferable to have an interface works without ambiguity in the first place.

That's why I proposed having a call that always sets IP 4 and 6, perhaps making it mandatory to specify both so there are no surprises.

@silverwind:

• I'm ultimately indifferent as to whether or not this should be available on the global resolver, but am a little concerned that it might be abused eg by a module that unexpectedly sets this globally. Thoughts?

• It looks like the source port can only be specified in ares_init_options, and is independent of source IP address (ie, it must be the same for v4 and v6). At best, local port would have to be an option passed in to Resolver(opts).

• set both IPv4 and v6 source addresses

  Why would you want to do that? a packet can only have one source address.

  A Resolver can have both IPv4 and IPv6 servers configured (resolver.setServers(['127.0.0.1', '::1'])), and ares may send packets to either. Thus, you need to set both the v4 and v6 source IP to ensure that all packets go over the desired interface.

it might be abused eg by a module that unexpectedly sets this globally

There's already hundrets of such possibilities in various Node.js API, having one more probably does not hurt. Users concerned about it can use Resolver but for simple use cases, global API should be compatible.

It looks like the source port can only be specified

If it's not too much work, I would suggest having it available.

As for API, how about

.setSourceAddress('1.2.3.4')
.setSourceAddress('1.2.3.4:1234')
.setSourceAddress('1.2.3.4', '1:2:3:4::')
.setSourceAddress('1.2.3.4', '[1:2:3:4::]:1234')
.setSourceAddress('1.2.3.4:1234', '[1:2:3:4::]:1234')

First argument is IPv4, second IPv6 in host:port syntax like in URLs. If any of them is falsy, ignore and don't change the source address/port. If argument is truthy and not an IP, e.g. net.isIP(ip) is 0, throw an error.

I have to disagree with suggested ('127.0.0.1', 'ipv4') API, we have the ability to detect IP version (and in my example above we actually only need to validate) and not doing so puts unnecessary burden on the consumer.

c-ares does not provide an API for changing the source port option after initialization, so we can't change it from an instance method — ie, we can't do .setSourceAddress('ip:port'). If source port is to be an option, it must be passed in to the Resolver constructor.

Happy to omit a port option in that case as it's a rather obscure option anyways.

987294a2264e9cf750ebf226cbd6c2a85c2c57e5 clarifies the API's handling of IPv4 and IPv6 source addresses. The signature is now setLocalAddress([ipv4][, ipv6]) and I've updated the docs:

resolver.setLocalAddress([ipv4][, ipv6])

• ipv4 {string} A string representation of an IPv4 address.
  Default: '0.0.0.0'
• ipv6 {string} A string representation of an IPv6 address.
  Default: '::0'

The resolver instance will send its requests from the specified IP address.
This allows programs to specify outbound interfaces when used on multi-homed
systems.

If a v4 or v6 address is not specified, it is set to 0.0.0.0 or ::0
respectively, which is the default and means "choose automatically from
available IP addresses".

The resolver will use the v4 local address when making requests to IPv4 DNS
servers, and the v6 local address when making requests to IPv6 DNS servers.
The rrtype of resolution requests has no impact on the local address used.

Regarding making this available on the default resolver (ie dns.setLocalAddress(…)):

After some investigation, it looks like doing so is not trivial. This is because dns.setServers(…) actually replaces the entire Resolver instance used as the default resolver:

https://github.com/nodejs/node/blob/9ac125bd378660181529e7f1099c318345d8ab21/lib/dns.js#L268-L277

ares does not provide a way to get the current local IP configuration, so we'd have to maintain additional state regarding the local address and then make sure it is re-applied whenever the global Resolver is swapped out.

It's doable, but I'd personally prefer to avoid the extra complexity.

I think everything outstanding has been resolved (unless anyone still thinks the API needs some work).

The merge commit was breaking our tools, I've rebased it against master to allow it to land cleanly.

Codecov Report

Attention: Patch coverage is 79.59184% with 10 lines in your changes missing coverage. Please review.

Project coverage is 87.90%. Comparing base (06f0d78) to head (4b1c360).

@@            Coverage Diff             @@
##           master   #34824      +/-   ##
==========================================
- Coverage   88.21%   87.90%   -0.31%     
==========================================
  Files         479      477       -2     
  Lines      113942   113132     -810     
  Branches    25649    24648    -1001     
==========================================
- Hits       100509    99453    -1056     
- Misses       7676     7961     +285     
+ Partials     5757     5718      -39     

... and 153 files with indirect coverage changes

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

Landed in 3e10ce3