dns: add setLocalAddress to Resolver #34824

daguej · 2020-08-18T06:50:46Z

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

This allows users to specify the (local source) IP address used when a dns.Resolver instance makes DNS requests.

Closes #34818

nodejs-github-bot · 2020-08-18T06:50:51Z

Review requested:

@nodejs/net

doc/api/dns.md

src/cares_wrap.cc

test/parallel/test-dns-setlocaladdress.js

lib/internal/dns/utils.js

test/parallel/test-dns-setlocaladdress.js

daguej · 2020-08-18T23:32:33Z

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.

node/deps/cares/src/ares_process.c

Lines 1018 to 1037 in 4cfcaae

    
           if (family == AF_INET) { 
        
             if (channel->local_ip4) { 
        
               memset(&local.sa4, 0, sizeof(local.sa4)); 
        
               local.sa4.sin_family = AF_INET; 
        
               local.sa4.sin_addr.s_addr = htonl(channel->local_ip4); 
        
               if (bind(s, &local.sa, sizeof(local.sa4)) < 0) 
        
                 return -1; 
        
             } 
        
           } 
        
           else if (family == AF_INET6) { 
        
             if (memcmp(channel->local_ip6, &ares_in6addr_any, 
        
                        sizeof(channel->local_ip6)) != 0) { 
        
               memset(&local.sa6, 0, sizeof(local.sa6)); 
        
               local.sa6.sin6_family = AF_INET6; 
        
               memcpy(&local.sa6.sin6_addr, channel->local_ip6, 
        
                      sizeof(channel->local_ip6)); 
        
               if (bind(s, &local.sa, sizeof(local.sa6)) < 0) 
        
                 return -1; 
        
             } 
        
           }

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?

bnoordhuis · 2020-08-19T07:23:37Z

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

silverwind · 2020-08-21T06:12:10Z

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.

jasnell · 2020-08-21T15:28:31Z

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')

daguej · 2020-08-22T02:34:10Z

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.

silverwind · 2020-08-23T14:49:27Z

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.

daguej · 2020-08-25T23:41:50Z

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.

silverwind · 2020-08-26T06:08:45Z

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

daguej · 2020-08-27T02:28:04Z

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.

daguej · 2020-08-28T02:04:41Z

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

addaleax

LGTM with 2 comments (we always use error codes for new errors)

src/cares_wrap.cc

doc/api/dns.md

cjihrig · 2020-08-28T13:08:42Z

src/cares_wrap.cc

+    ares_set_local_ip6(channel->cares_channel(), addr0);
+    type0 = 6;
+  } else {
+    return env->ThrowError("Invalid IP address.");


Any reason not to do the input validation in JS?

It would be redundant since we need to parse the strings in C land anyway and call ares_set_local_ip4/ares_set_local_ip6 as appropriate.

aduh95 · 2020-10-24T14:05:39Z

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

codecov-io · 2020-10-24T15:16:39Z

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

Files with missing lines	Patch %	Lines
src/cares_wrap.cc	77.77%	2 Missing and 6 partials ⚠️
lib/internal/dns/utils.js	83.33%	1 Missing and 1 partial ⚠️

Additional details and impacted files

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

Files with missing lines	Coverage Δ
lib/internal/dns/promises.js	`100.00% <100.00%> (ø)`
lib/internal/dns/utils.js	`98.96% <83.33%> (+0.51%)`	⬆️
src/cares_wrap.cc	`63.42% <77.77%> (ø)`

... and 153 files with indirect coverage changes

nodejs-github-bot · 2020-10-24T17:42:22Z

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

aduh95 · 2020-10-25T10:18:59Z

Landed in 3e10ce3

Fixes: #34818 PR-URL: #34824 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

Notable changes: child_process: * (SEMVER-MINOR) add ChildProcess 'spawn' event (Matthew Francis Brunetti) #35369 dns: * (SEMVER-MINOR) add setLocalAddress to Resolver (Josh Dague) #34824 http: * (SEMVER-MINOR) report request start and end with diagnostics_channel (Stephen Belanger) #34895 http2: * (SEMVER-MINOR) add updateSettings to both http2 servers (Vincent Boivin) #35383 lib: * (SEMVER-MINOR) create diagnostics_channel module (Stephen Belanger) #34895 src: * (SEMVER-MINOR) add --heapsnapshot-near-heap-limit option (Joyee Cheung) #33010 v8: * (SEMVER-MINOR) implement v8.stopCoverage() (Joyee Cheung) #33807 * (SEMVER-MINOR) implement v8.takeCoverage() (Joyee Cheung) #33807 worker: * (SEMVER-MINOR) add eventLoopUtilization() (Trevor Norris) #35664 PR-URL: TODO

Notable changes: child_process: * (SEMVER-MINOR) add ChildProcess 'spawn' event (Matthew Francis Brunetti) #35369 dns: * (SEMVER-MINOR) add setLocalAddress to Resolver (Josh Dague) #34824 http: * (SEMVER-MINOR) report request start and end with diagnostics_channel (Stephen Belanger) #34895 http2: * (SEMVER-MINOR) add updateSettings to both http2 servers (Vincent Boivin) #35383 lib: * (SEMVER-MINOR) create diagnostics_channel module (Stephen Belanger) #34895 src: * (SEMVER-MINOR) add --heapsnapshot-near-heap-limit option (Joyee Cheung) #33010 v8: * (SEMVER-MINOR) implement v8.stopCoverage() (Joyee Cheung) #33807 * (SEMVER-MINOR) implement v8.takeCoverage() (Joyee Cheung) #33807 worker: * (SEMVER-MINOR) add eventLoopUtilization() (Trevor Norris) #35664 PR-URL: #35948

Fixes: #34818 PR-URL: #34824 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

daguej requested a review from a team as a code owner August 18, 2020 06:50

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 Aug 18, 2020

targos reviewed Aug 18, 2020

View reviewed changes

doc/api/dns.md Outdated Show resolved Hide resolved

daguej force-pushed the dns-localaddress branch 2 times, most recently from 7c36555 to 26704d0 Compare August 18, 2020 07:43

bnoordhuis reviewed Aug 18, 2020

View reviewed changes

src/cares_wrap.cc Outdated Show resolved Hide resolved

src/cares_wrap.cc Outdated Show resolved Hide resolved

mscdex reviewed Aug 18, 2020

View reviewed changes

test/parallel/test-dns-setlocaladdress.js Outdated Show resolved Hide resolved

cjihrig reviewed Aug 18, 2020

View reviewed changes

lib/internal/dns/utils.js Outdated Show resolved Hide resolved

test/parallel/test-dns-setlocaladdress.js Show resolved Hide resolved

daguej requested review from mscdex, targos, bnoordhuis and cjihrig and removed request for a team August 28, 2020 02:04

addaleax approved these changes Aug 28, 2020

View reviewed changes

src/cares_wrap.cc Outdated Show resolved Hide resolved

src/cares_wrap.cc Outdated Show resolved Hide resolved

cjihrig approved these changes Aug 28, 2020

View reviewed changes

addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Aug 29, 2020

daguej added 8 commits October 24, 2020 16:01

nits

1344714

add promise resolver tests

a8b30db

clarify IPv4/IPv6 API

de1e8c5

fix lint

d672863

throw errors with codes

ac0f189

tweak doc language

605d999

fix lint

cbc571a

cares_get_32bit -> ReadUint32BE

4b1c360

aduh95 force-pushed the dns-localaddress branch from 0c8b43f to 4b1c360 Compare October 24, 2020 14:04

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 24, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 24, 2020

This comment has been minimized.

Sign in to view

aduh95 closed this Oct 25, 2020

aduh95 pushed a commit that referenced this pull request Oct 25, 2020

dns: add setLocalAddress to Resolver

3e10ce3

Fixes: #34818 PR-URL: #34824 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

targos pushed a commit that referenced this pull request Nov 3, 2020

dns: add setLocalAddress to Resolver

548f91a

Fixes: #34818 PR-URL: #34824 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

targos mentioned this pull request Nov 3, 2020

v15.1.0 release proposal #35948

Merged

targos pushed a commit that referenced this pull request Mar 3, 2021

dns: add setLocalAddress to Resolver

1ca213c

Fixes: #34818 PR-URL: #34824 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

MylesBorins pushed a commit that referenced this pull request Apr 6, 2021

dns: add setLocalAddress to Resolver

ed79c98

Fixes: #34818 PR-URL: #34824 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns: add setLocalAddress to Resolver #34824

dns: add setLocalAddress to Resolver #34824

daguej commented Aug 18, 2020

nodejs-github-bot commented Aug 18, 2020

daguej commented Aug 18, 2020 •

edited

Loading

bnoordhuis commented Aug 19, 2020

silverwind commented Aug 21, 2020 •

edited

Loading

jasnell commented Aug 21, 2020

daguej commented Aug 22, 2020

silverwind commented Aug 23, 2020 •

edited

Loading

daguej commented Aug 25, 2020

silverwind commented Aug 26, 2020

daguej commented Aug 27, 2020

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

daguej commented Aug 28, 2020

addaleax left a comment

cjihrig Aug 28, 2020

daguej Aug 29, 2020

aduh95 commented Oct 24, 2020

This comment has been minimized.

codecov-io commented Oct 24, 2020 •

edited by codecov bot

Loading

nodejs-github-bot commented Oct 24, 2020

aduh95 commented Oct 25, 2020

dns: add setLocalAddress to Resolver #34824

dns: add setLocalAddress to Resolver #34824

Conversation

daguej commented Aug 18, 2020

Checklist

nodejs-github-bot commented Aug 18, 2020

daguej commented Aug 18, 2020 • edited Loading

bnoordhuis commented Aug 19, 2020

silverwind commented Aug 21, 2020 • edited Loading

jasnell commented Aug 21, 2020

daguej commented Aug 22, 2020

silverwind commented Aug 23, 2020 • edited Loading

daguej commented Aug 25, 2020

silverwind commented Aug 26, 2020

daguej commented Aug 27, 2020

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

daguej commented Aug 28, 2020

addaleax left a comment

Choose a reason for hiding this comment

cjihrig Aug 28, 2020

Choose a reason for hiding this comment

daguej Aug 29, 2020

Choose a reason for hiding this comment

aduh95 commented Oct 24, 2020

This comment has been minimized.

codecov-io commented Oct 24, 2020 • edited by codecov bot Loading

Codecov Report

nodejs-github-bot commented Oct 24, 2020

aduh95 commented Oct 25, 2020

daguej commented Aug 18, 2020 •

edited

Loading

silverwind commented Aug 21, 2020 •

edited

Loading

silverwind commented Aug 23, 2020 •

edited

Loading

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

codecov-io commented Oct 24, 2020 •

edited by codecov bot

Loading