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.

Sign up for GitHub

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

Jump to bottom

Feature. Set DNS server port for DNS client #7903

Closed
amit777 opened this issue Jul 28, 2016 · 21 comments
Closed

Feature. Set DNS server port for DNS client #7903

amit777 opened this issue Jul 28, 2016 · 21 comments
Labels
cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. dns Issues and PRs related to the dns subsystem. feature request Issues that request new features to be added to Node.js.

Comments

@amit777
Copy link

amit777 commented Jul 28, 2016

the Docs on the DNS client say that the dns.setServers(servers) method will strip out any custom ports provided. "If a port specified on the address it will be removed."

I'm curious if there is a specific reason for not wanting to support it, or is it planned to be supported?
@mscdex mscdex added question Issues that look for answers. dns Issues and PRs related to the dns subsystem. labels Jul 28, 2016
@bnoordhuis
Copy link
Member

We could add support for port numbers. dns.setServers() is currently glue for ares_set_servers(), which ignores port numbers, but that could be changed to ares_set_servers_ports(), which respects them.

Can you explain your use case though? I've never seen a DNS server on a non-standard port, not even in testing setups.

@amit777
Copy link
Author

amit777 commented Jul 28, 2016

It's mainly for an e2e test suite to be run on a production server that already is serving DNS. I didn't want to have to spin up another server or shutdown the existing DNS server. Maybe to reduce impact, we could keep the existing behavior and have an options flag to use the ares_set_server_ports() function?

@bnoordhuis bnoordhuis added feature request Issues that request new features to be added to Node.js. and removed question Issues that look for answers. labels Jul 28, 2016
@bnoordhuis
Copy link
Member

I've added the 'feature request' label.

Pull requests welcome but I won't vouch they'll get accepted. It seems like a rather esoteric feature when you can't even configure /etc/resolv.conf to use a non-standard port.

@imyller
Copy link
Member

imyller commented Aug 1, 2016

System resolvers rarely allow non-standard ports - rightfully so - but application resolvers often do allow them for various reasons.

I wouldn't consider Node.js to be a system level resolver and thus supporting non-standard ports might not be so "esoteric feature".

In addition to network domains, DNS can be used for other purposes. For example as a P2P service discovery protocol, where use of custom ports is desired. In this case, port is not used to differentiate the technical protocol, but the type of content served.

Right now, Node.js can not be used to implement clients for these kinds of service discovery implementations; or at least not by using it's built-in resolver.

@silverwind
Copy link
Contributor

Not against it, but also not really thrilled about a likely required sync-style method like dns.setPort(if we wanna keep the API consistent with setServers), for cases where you want to use the system's servers on an alternative port.

I'd generally recommend dns-socket which can do pretty much everything tools like dig can do.

@jasnell
Copy link
Member

jasnell commented Aug 3, 2016

To be honest, the entire DNS module is a bit ripe for refactoring. There are challenges with setServers() when it's called at the wrong time and the APIs are a bit messy in general. I'm not against making this change, but perhaps we should give some thought to more extensive changes?

@silverwind
Copy link
Contributor

Yeah, dns is lacking both feature-wise and API-wise. Adding more API surface now is not going to make a future transition easier, so I think that should be kept in mind.

@imyller
Copy link
Member

imyller commented Aug 3, 2016

I remember seeing some discussion / PRs for transitioning to pure JS DNS resolver.

What is the status of that?

@jasnell
Copy link
Member

jasnell commented Aug 3, 2016

@mscdex can comment more specifically but it stalled out due to an inability to get the necessary performance.

@mscdex
Copy link
Contributor

mscdex commented Aug 3, 2016

@jasnell Yep.

@imyller
Copy link
Member

imyller commented Aug 3, 2016

@mscdex Just curious, what type of performance degradation? DNS isn't exactly a high frequency, high bandwidth protocol by it's nature.

I'd expect native c-ares to offer better latency and more queries per second, but does minor performance degradation even matter for a any real world DNS usage scenario?

@mscdex
Copy link
Contributor

mscdex commented Aug 3, 2016

@imyller I think it's pretty critical to have good DNS performance, since it's used so much by pretty much everyone. For more information and a few numbers from that last time I worked on it, see here.

@imyller
Copy link
Member

imyller commented Aug 3, 2016

@mscdex Benchmark results you linked are proving your point, 1000 queries in 44ms for c-ares and 130ms for JS resolver. However, 1000 queries/responses from single client to single DNS server within 44ms timeframe is very far from typical usage. But still slower - no arguing with that.

I looked briefly at other languages DNS libraries. Most seem to offer just a wrapper for accessing the system resolver.

Why Node.js has to ship with it's own application level resolver; and more importantly why not just leave this job for thin libuv abstraction?

@bnoordhuis
Copy link
Member

In a word: performance. System resolvers typically don't have an asynchronous or non-blocking interface so you're forced to start 1,000 threads if you want to do 1,000 concurrent lookups.

@mscdex
Copy link
Contributor

mscdex commented Aug 3, 2016 •
edited
Loading

@imyller The system resolver API isn't very flexible and is pretty limited. It's there to basically resolve hostnames to A or AAAA records (or vice versa) and nothing else. Also you're typically restricted to how the system resolver works, so for example you can't easily bypass system-configured DNS modules and just communicate with a DNS server from node. The glibc resolver has more flags nowadays but IIRC there is no (good) way to pass those at runtime.

Having a "userland" resolver like c-ares allows us to make more kinds of DNS queries and better control over TTL and other things. However, c-ares is far from perfect and lacks features that modern system resolvers have (e.g. DNSSEC for those that care about that).

@imyller
Copy link
Member

imyller commented Aug 3, 2016 •
edited
Loading

Good points.

I see that it's not trivial to improve Node.js DNS features while getting the performance required.

@imyller
Copy link
Member

imyller commented Aug 3, 2016

Interesting non-blocking resolver project:

https://github.com/wahern/dns

Seems to be MIT licensed. Good features. Pluggable cache interface etc.

@tamsky
Copy link

tamsky commented Sep 20, 2016 •
edited
Loading

@bnoordhuis

I've never seen a DNS server on a non-standard port, not even in testing setups.

https://www.consul.io/docs/agent/dns.html
by default, consul runs their DNS service on udp/8600.

@fvdm
Copy link

fvdm commented Nov 23, 2016 •
edited
Loading

+1 for custom port

I run two different DNS resolvers on my production box. One of them runs with normal settings and is for the system and generic apps, the other is for an app that requires custom settings and very short cache retention.

For now I'm going to use dns-socket native-dns to resolve this, but I believe the dns module should support custom ports.

Edit: switched to native-dns as I can't get dns-socket to work somehow.

@amit777
Copy link
Author

amit777 commented Jan 9, 2017

I also want to run the dnsmasq service locally on a non-standard port, and then query against it with my nodejs app (since my node app runs on port 53).. I want to leverage the dnsmasq caching etc..

XadillaX added a commit to XadillaX/node that referenced this issue Jun 16, 2017
@XadillaX
dns: make dns.setServers support customized port
b5b28e0 
Make `dns.setServers` parameter may be strings contain IP and port which
split by `:`.

eg.

```
dns.setServers([ "103.238.225.181:666" ]);
```

And `dns.getServers` will return IP with port if that server is not use
default port.

Refs: nodejs#7903
@XadillaX XadillaX mentioned this issue Jun 16, 2017
Closed
4 tasks
refack pushed a commit to refack/node that referenced this issue Jun 20, 2017
@XadillaX @refack
dns: make dns.setServers support customized port
330349f 
allow `dns.setServers` parameter to contain port

e.g.

```
dns.setServers([ '103.238.225.181:666' ]);
```

And `dns.getServers` will return IP with port if not the default port.

PR-URL: nodejs#13723
Refs: nodejs#7903
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
addaleax pushed a commit that referenced this issue Jun 21, 2017
@XadillaX @addaleax
dns: make dns.setServers support customized port
2bb6614 
allow `dns.setServers` parameter to contain port

e.g.

```
dns.setServers([ '103.238.225.181:666' ]);
```

And `dns.getServers` will return IP with port if not the default port.

PR-URL: #13723
Refs: #7903
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
addaleax pushed a commit that referenced this issue Jun 24, 2017
@XadillaX @addaleax
dns: make dns.setServers support customized port
b09b3e6 
allow `dns.setServers` parameter to contain port

e.g.

```
dns.setServers([ '103.238.225.181:666' ]);
```

And `dns.getServers` will return IP with port if not the default port.

PR-URL: #13723
Refs: #7903
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
rvagg pushed a commit that referenced this issue Jun 29, 2017
@XadillaX @rvagg
dns: make dns.setServers support customized port
132a58e 
allow `dns.setServers` parameter to contain port

e.g.

```
dns.setServers([ '103.238.225.181:666' ]);
```

And `dns.getServers` will return IP with port if not the default port.

PR-URL: #13723
Refs: #7903
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
addaleax pushed a commit that referenced this issue Jul 11, 2017
@XadillaX @addaleax
dns: make dns.setServers support customized port
4fc2059 
allow `dns.setServers` parameter to contain port

e.g.

```
dns.setServers([ '103.238.225.181:666' ]);
```

And `dns.getServers` will return IP with port if not the default port.

PR-URL: #13723
Refs: #7903
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@Trott Trott added the cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. label Jul 15, 2017
@refack
Copy link
Contributor

refack commented Jul 15, 2017

Fixed in #13723

@refack refack closed this as completed Jul 15, 2017
addaleax pushed a commit that referenced this issue Jul 18, 2017
@XadillaX @addaleax
dns: make dns.setServers support customized port
ebe7bb2 
allow `dns.setServers` parameter to contain port

e.g.

```
dns.setServers([ '103.238.225.181:666' ]);
```

And `dns.getServers` will return IP with port if not the default port.

PR-URL: #13723
Refs: #7903
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. dns Issues and PRs related to the dns subsystem. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
10 participants
@mscdex @fvdm @refack @tamsky @silverwind @bnoordhuis @jasnell @Trott @imyller @amit777