Skip to content

Add flag to not use a default local named server on channel initialization - #713

Merged
bradh352 merged 2 commits into
c-ares:mainfrom
oliverwelsh:oliverwelsh/no-dflt-srvr-opt
Feb 22, 2024
Merged

Add flag to not use a default local named server on channel initialization#713
bradh352 merged 2 commits into
c-ares:mainfrom
oliverwelsh:oliverwelsh/no-dflt-srvr-opt

Conversation

@oliverwelsh

@oliverwelsh oliverwelsh commented Feb 19, 2024

Copy link
Copy Markdown
Contributor

Hello, I work on an application for Microsoft which uses c-ares to perform DNS lookups. We have made some minor changes to the library over time, and would like to contribute these back to the project in case they are useful more widely. This PR adds a new channel init flag, described below.

Please let me know if I can include any more information to make this PR better/easier for you to review. Thanks!

Summary
When initializing a channel with ares_init_options(), if there are no nameservers available (because ARES_OPT_SERVERS is not used and /etc/resolv.conf is either empty or not available) then a default local named server will be added to the channel.

However in some applications a local named server will never be available. In this case, all subsequent queries on the channel will fail.

If we know this ahead of time, then it may be preferred to fail channel initialization directly rather than wait for the queries to fail. This gives better visibility, since we know that the failure is due to missing servers rather than something going wrong with the queries.

This PR adds a new flag ARES_FLAG_NO_DFLT_SVR, to indicate that a default local named server should not be added to a channel in this scenario. Instead, a new error ARES_EINITNOSERVER is returned and initialization fails.

Testing
I have added 2 new FV tests:

  • ContainerNoDfltSvrEmptyInit to test that initialization fails when no nameservers are available and the flag is set.
  • ContainerNoDfltSvrFullInit to test that initialization still succeeds when the flag is set but other nameservers are available.

Existing FVs are all passing.

Documentation
I have had a go at manually updating the docs to describe the new flag/error, but couldn't see any contributing guidance about testing this. Please let me know if you'd like anything more here.

a channel and no other servers are available. This can be used
in scenarios where an application will never have access to a local
named server, and it is preferred to fail initialization rather than
wait for a DNS query to later fail.
@bradh352

Copy link
Copy Markdown
Member

Makes sense, however I wonder if we shouldn't use this same error code if someone calls ares_set_servers*() with an empty server list then tries to perform a query. No need for it to be init-only. (Unfortunately, we have to support the ability to completely clear the server list due to some odd NodeJS requirement).

gets raised whenever a query is performed and fails due to no servers
being available.
@oliverwelsh

oliverwelsh commented Feb 21, 2024

Copy link
Copy Markdown
Contributor Author

Makes sense, however I wonder if we shouldn't use this same error code if someone calls ares_set_servers*() with an empty server list then tries to perform a query. No need for it to be init-only. (Unfortunately, we have to support the ability to completely clear the server list due to some odd NodeJS requirement).

Thank you, making the error a little more generic sounds good to me.

We check for no servers in 2 places during queries. I think updating these to use the new error would make sense.

  • ares_send_ex() before starting a query
  • ares__send_query() when selecting a server

I have pushed a markup to the PR which does this. It also renames the error to ARES_ENOSERVER and adds documentation. DefaultChannelTest.SetServers needed the error code updating (related to the NodeJS requirement you mentioned!).

An alternative could be to reuse ARES_ESERVFAIL on init. But I feel like that represents errors returned by the remote server, rather than a local config issue.

@bradh352
bradh352 merged commit 035c4c3 into c-ares:main Feb 22, 2024
@oliverwelsh
oliverwelsh deleted the oliverwelsh/no-dflt-srvr-opt branch February 23, 2024 16:43
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

Successfully merging this pull request may close these issues.

2 participants