dns: getServers returns IPv6 addresses without [] #13795

Activity

1. refack

   added

   caresIssues and PRs related to the c-ares dependency or the cares_wrap binding.Issues and PRs related to the c-ares dependency or the cares_wrap binding.

   dnsIssues and PRs related to the dns subsystem.Issues and PRs related to the dns subsystem.

   on Jun 19, 2017
2. refack commented on Jun 19, 2017

   

   refack

   on Jun 19, 2017

   ContributorAuthor

   More actions

   Ref: nodejs/node-v0.x-archive#5435
3. refack commented on Jun 19, 2017

   

   refack

   on Jun 19, 2017

   ContributorAuthor

   More actions

   I just noticed this, IMHO we should format IPv6 with []:
   e.g.

   return ret.map(([host, port]) => {
     if (isIP(host) === 6) host = `[${host}]`;
     return (!port || port === 53) host : `${host}:${port}`;
   }

   Fix will be semver-major
4. silverwind commented on Jun 19, 2017

   

   silverwind

   on Jun 19, 2017

   Contributor

   More actions

   I think a bare IPv6 without [] is fine as long as it's not in an contained in an URL. Also see rfc5952 which suggests to only use[] when a port is included.

   👍React with 👍2Reacted by Khaidi Chu and Benjamin Gruenbaum
5. refack commented on Jun 19, 2017

   

   refack

   on Jun 19, 2017

   ContributorAuthor

   More actions

   I was going by rfc3986, which is ratified while rfc5952 is still "proposed".
   IMHO this issue is not urgent and could stay open forever, or closed with known limitation.
6. silverwind commented on Jun 19, 2017

   

   silverwind

   on Jun 19, 2017

   Contributor

   More actions

   rfc3986 is about URIs, which I think host:port is not (IIRC, a valid URI always requires a protocol). Anyways, If you want to treat it like an URL, you can use the legacy url module to format it (not sure if the same is possible using WHATWG URL, as it doesn't allow calling the constructor without arguments):

   > u = new url.Url(); u.hostname = "::1"; u.format()
   '[::1]'
   > u = new url.Url(); u.hostname = "::1"; u.port = 53; u.format()
   '[::1]:53'
   > u = new url.Url(); u.hostname = "127.0.0.1"; u.format()
   '127.0.0.1'
   > u = new url.Url(); u.hostname = "127.0.0.1"; u.port = 53; u.format()
   '127.0.0.1:53'
7. refack commented on Jun 19, 2017

   

   refack

   on Jun 19, 2017

   ContributorAuthor

   More actions

   As suggested just now #13723 (comment) dns://[addr]:port...

   A semver-major fix could return a list of dns:// URLs (even raw not toStringed) 🤷‍♂️
8. refack commented on Jun 19, 2017

   

   refack

   on Jun 19, 2017

   ContributorAuthor

   More actions

    💡
   🤓
   Or add getServersURLs() as semver-minor (or getServers(true))
9. silverwind commented on Jun 19, 2017

   

   silverwind

   on Jun 19, 2017

   Contributor

   More actions

   dns://[addr]:port

   Declaring a DNS server like this is rather uncommon. Why not require and return [] style IPv6 when port is specified and not 53, e.g.:

   2001::db8
   [2001::db8]:5353
   127.0.0.1
   127.0.0.1:5353
   
10. refack commented on Jun 19, 2017

    

    refack

    on Jun 19, 2017

    ContributorAuthor

    More actions

    Declaring a DNS server like this is rather uncommon. Why not require and return [] style IPv6 when port is specified and not 53, e.g.:

    (again for context, I think this is of very low priority if at all, I just enjoy the discussion)

    Passing URLs (in and out) eliminates hand spun parsing code, by us in setServers and by the user in getServers
    We could as well "re-invent" a degenerated URL {addr, port} but even then we'll need to validate that addr is a string representing an IP, and port is a valid port number (0 < port < 2**16-1)
11. Trott commented on Jun 7, 2018

    

    Trott

    on Jun 7, 2018

    Member

    More actions

    Closing as known limitation. Feel absolutely free to re-open or comment if you feel it should stay open, especially if you think it might be addressed some point in the not-too-distant future.
12. Trott

    closed this as completedon Jun 7, 2018