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 lookups are not working with multiple system DNS servers #280

Closed
DMarby opened this issue Mar 24, 2021 · 15 comments
Closed

DNS lookups are not working with multiple system DNS servers #280

DMarby opened this issue Mar 24, 2021 · 15 comments

Comments

@DMarby
Copy link

DMarby commented Mar 24, 2021

With the recent switch to the c-ares DNS resolver backend in version 2.3.0, it seems like DNS resolution is sometimes failing when using multiple DNS servers. A lot of our users have reported that they are unable to make requests due to this: Kong/insomnia#3234.

Do you think it would be reasonable to stop using c-ares for the prebuilt binaries, given that there are likely a lot of other issues like this compared to the built-in system resolvers, and it's not possible to disable it during runtime?

@JCMais
Copy link
Owner

JCMais commented Mar 24, 2021

Hi @DMarby,

If you believe this is a bug with c-ares could you also report this in their repository? If this is indeed a bug with c-ares the same issue should appear if you build libcurl from source using https://github.com/JCMais/curl-for-windows/tree/d6c10ce12588e723e7ffee52aae2df577391d99c and change the example.c in there: https://github.com/JCMais/curl-for-windows/blob/d6c10ce12588e723e7ffee52aae2df577391d99c/example.c to make a similar call to the one being made in Insomnia.

Could you check if using the option CURLOPT_DNS_SERVERS to explicitly set the DNS servers works (there are a few other options related to DNS)?

handle.setOpt('DNS_SERVERS', "ip1, ip2")

@DMarby
Copy link
Author

DMarby commented Mar 24, 2021

Hi @JCMais,

Thanks for the quick reply, will build from that repo and test the example and let you know what the results are.

As for using the CURLOPT_DNS_SERVERS, I can test that as well and report back, but it's a very bad experience if a user has to configure the DNS servers on an app level in addition to a system level in order for things to work, so would like to avoid that.

@nijikokun
Copy link

nijikokun commented Mar 24, 2021

👋 Hi @JCMais Product Owner for Insomnia here, we are getting quite a lot of reports of users being unable to make requests in Insomnia because of this change. Is it possible to revert?

We do not want to lose the ability to rely on the system DNS and we don't have the time or capability to build out the really arduous task of gathering system DNS to supplement with the setOpt function. We are looking into building node-libcurl ourselves but that will obviously take some time and effort to setup as well - any help is greatly appreciated 😓

@JCMais
Copy link
Owner

JCMais commented Mar 24, 2021

@DMarby @nijikokun Is the issue happening only on Windows?

What I can do is release a new pre-release version, like 2.3.1-electron-no-cares, that does not build libcurl with c-ares support when building the electron binaries.

We are looking into building node-libcurl ourselves but that will obviously take some time and effort to setup as well - any help is greatly appreciated 😓

All the build scripts are available here https://github.com/JCMais/node-libcurl/tree/develop/scripts/ci and are under the same license as the repository, in case the Insomnia team wants to replicate the build steps. Windows builds are a bit different, in this case, the dependency is given directly in the bindings.gyp using the scripts/retrieve-win-deps.js script to retrieve libcurl and their dependencies from a separate repository (curl-for-windows).

@nijikokun
Copy link

What I can do is release a new pre-release version, like 2.3.1-electron-no-cares, that does not build libcurl with c-ares support when building the electron binaries.

This would be extremely helpful for the time being until we get to the bottom of the issue and or get our build system up 🙏

All the build scripts are available here https://github.com/JCMais/node-libcurl/tree/develop/scripts/ci and are under the same license as the repository, in case the Insomnia team wants to replicate the build steps. Windows builds are a bit different, in this case, the dependency is given directly in the bindings.gyp using the scripts/retrieve-win-deps.js script to retrieve libcurl and their dependencies from a separate repository (curl-for-windows).

Are there any special environment variables defined within the CIs that we should know about?

@DMarby
Copy link
Author

DMarby commented Mar 24, 2021

@DMarby @nijikokun Is the issue happening only on Windows?

I have only reproduced it on Windows so far, unfortunately I have not been able to try it on macOS or Linux let, so I am not sure if it affects other platforms.

@DMarby
Copy link
Author

DMarby commented Mar 24, 2021

Hi @JCMais,

Still working on reproducing with just the curl-for-windows/example.c, but we were just able to confirm this issue on macOS too, so doesn't seem isolated to Windows unfortunately, I would guess that it's broken on Linux as well.

If possible, releasing a version without cares would be of great help. We use node-libcurl in our companion CLI insomnia-inso as well, so if possible having binaries for that build for the node targets as well as the electron targets too would be very appreciated.

@JCMais
Copy link
Owner

JCMais commented Mar 24, 2021

Are there any special environment variables defined within the CIs that we should know about?

There are a few, please see https://github.com/JCMais/node-libcurl/blob/develop/.github/workflows/build-and-release.yaml and https://github.com/JCMais/node-libcurl/blob/develop/.appveyor.yml.

If possible, releasing a version without cares would be of great help. We use node-libcurl in our companion CLI insomnia-inso as well, so if possible having binaries for that build for the node targets as well as the electron targets too would be very appreciated.

I will do that later today or tomorrow morning, I will update this issue when it is available.

@nijikokun
Copy link

@DMarby @nijikokun Is the issue happening only on Windows?

Tested on macOS and Linux, it impacts all platforms

I will do that later today or tomorrow morning, I will update this issue when it is available.

Thank you, this will help so many Insomnia users 🙏

@nijikokun
Copy link

nijikokun commented Mar 24, 2021

Side note and potentially related issue we uncovered during discovery, also during retro on how this was missed it was brought up that the changelog and release notes have a typo stating it's *not* built with c-ares. and was not marked as breaking so we didn't think too much about it. Hope this helps 🙏


Update: Yup, just got a report for it 😢

@JCMais
Copy link
Owner

JCMais commented Mar 24, 2021

Thanks @nijikokun , yep that was a typo, it was supposed to be it's now built with c-ares. It was not marked as a breaking change because I did not expect this to cause any major issues.

I will prob just remove c-ares altogether for now and wait until this is fixed.

@JCMais
Copy link
Owner

JCMais commented Mar 24, 2021

v2.3.2 has been released without c-ares.

@JCMais
Copy link
Owner

JCMais commented Nov 6, 2021

@nijikokun @DMarby FYI this has been fixed here: c-ares/c-ares#430

The node.js dependency has been updated here: nodejs/node@badd1fa

@DMarby
Copy link
Author

DMarby commented Nov 7, 2021

@JCMais Unless I'm misunderstanding that PR, I think that only resolves the issue of "localhost" failing to be resolved, not the second issue of local DNS servers not being used? But I could very well be wrong about that

@JCMais
Copy link
Owner

JCMais commented Nov 8, 2021

@DMarby yeah, it seems to fix only the localhost issue. Not sure the other one has been reported tho.

namcxn pushed a commit to namcxn/uptime-monitor that referenced this issue Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants