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

dns.js not properly iterating through DNS array #21391

Closed
MatthewPinheiro opened this issue Jun 18, 2018 · 6 comments
Closed

dns.js not properly iterating through DNS array #21391

MatthewPinheiro opened this issue Jun 18, 2018 · 6 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. doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors.

Comments

@MatthewPinheiro
Copy link

  • Version: 8.11.3
  • Platform: Windows 10 Pro v1709
  • Subsystem: dns

I'll preface this by saying that there's a significant chance I'm simply misunderstanding dns.setServers, but that being said, I believe I'm experiencing undesired functionality.

Given var dnsServer1 and var dnsServer2 are two valid IP Address strings of two valid DNS servers, and given that var host is a valid string which resolves to an IP on dnsServer1 but not on dnsServer2, consider the following code:

dns.setServers([dnsServer2, dnsServer1]);
dns.resolve(host, (err, records) => {
    if (err) {
        console.log(err);
    } else {
        console.log(records);
});

If the above is run, an ENOTFOUND error will be produced and records will be undefined. However, if the order of the array passed into dns.setServers is reversed, i.e.

dns.setServers([dnsServer1, dnsServer2])

and the same call to dns.resolve is made, no error will be produced and records will have resolved to a valid IP.

Isn't dns.resolve supposed to check through each DNS IP set by dns.setServers until either one is able to resolve host or none of them are able to do so? Here it instead seems the entire resolve operation fails as soon as the first DNS IP fails, and succeeds as soon as the first DNS IP succeeds.

I've tried to find more thorough documentation but couldn't find anything that said one way or the other.

@addaleax addaleax added dns Issues and PRs related to the dns subsystem. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. labels Jun 18, 2018
@bnoordhuis
Copy link
Member

Isn't dns.resolve supposed to check through each DNS IP set by dns.setServers until either one is able to resolve host or none of them are able to do so?

No, the second one is the fallback for when the first one times out or errors; not when it reports "no such host exists."

I've tried to find more thorough documentation but couldn't find anything that said one way or the other.

It's basically how /etc/resolv.conf works. I suppose a note could be added to the docs. PR welcome.

@bnoordhuis bnoordhuis added doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. labels Jun 18, 2018
@BeniCheni
Copy link
Contributor

BeniCheni commented Jun 18, 2018

Hello, just minor friendly message that I'm happy to give this "good first issue" issue an attempt. Hopefully I could make progress to open a PR soon.

P.S.: Updated below to strike through my outdated comments from previous edits:
Noticed that this #21273 back-filled a contributor to the README. If I open a PR for this issue, I would also update the "collaborators" field to include self, since I've landed 2 "good first issue" PRs before. (#20092 & #20460).

I'd hope it's okay to add myself as a "contributor". But if my assumption is incorrect about the definition of the "contributor", please kindly let me know. (So I won't update the "contributors" field of README mistakenly.)

P.S.: Question - What does Collaborator Emeriti imply for the context of contributors?

(Sorry that my question is not related to this issue; just got curious about the definition of the keyword "Emeriti"; I can also do a search of the keyword in the docs in the repo to try to learn more about that)

@richardlau
Copy link
Member

@BeniCheni Collaborators are not the same as contributors. Please see https://github.com/nodejs/node/blob/master/GOVERNANCE.md for more information about collaborators.

All contributors to Node.js are listed in https://github.com/nodejs/node/blob/master/AUTHORS (I believe

node/AUTHORS

Line 2190 in 4970e2b

BeniCheni <benjaminlchen@gmail.com>
is you).

@Shivang44
Copy link
Contributor

I didn't see any PR for 4 days from the user above so I made one: #21469

Let me know if everyone likes the note. Perhaps we could clarify further on what errors it will continue checking other servers? I couldn't find this information in the documentation so I'm not sure.

@MatthewPinheiro
Copy link
Author

Thanks for making the PR @Shivang44. Didn't mean to completely abandon my own issue, but just completely forgot to make a PR once I was home. Thanks!

vsemozhetbyt pushed a commit that referenced this issue Jun 25, 2018
Added a note that that clarifies the fact that setServers() does not
check subsequent servers when the first one produces a NOTFOUND error.

PR-URL: #21469
Refs: #21391
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Jun 26, 2018
Added a note that that clarifies the fact that setServers() does not
check subsequent servers when the first one produces a NOTFOUND error.

PR-URL: #21469
Refs: #21391
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Shivang44
Copy link
Contributor

Landed in 95205a6. I believe this issue can be closed as long as @MatthewPinheiro is okay with it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Development

No branches or pull requests

7 participants