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

allow path for resolv.conf file to be configurable #220

Closed
SirR4T opened this Issue Aug 31, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@SirR4T
Contributor

SirR4T commented Aug 31, 2018

Hi Team,

While working on this issue, I came to understand that the path to the resolv.conf file is hardcoded, via ./configure.

For testing purposes, we would like to launch a child process with a different path for the resolv.conf file (via an environment variable like CARES_RESOLV_CONF_PATH, say), where we would then be able to specify different resolv.conf files (with different search, nameserver, etc. stanzas, say).

I'd like to work on adding this support, but wanted to ask if such a PR would be welcome?

@SirR4T SirR4T referenced this issue Aug 31, 2018

Open

dns: support cares_search for resolution #22226

4 of 4 tasks complete
@bradh352

This comment has been minimized.

Member

bradh352 commented Sep 1, 2018

I'm personally not a fan of magic environment variables, especially for libraries that are embedded into other applications and frameworks. It could cause unintended consequences or possible security vulnerabilities.

I'd prefer a new ares_init_options() setting that could control the path.

@SirR4T

This comment has been minimized.

Contributor

SirR4T commented Sep 1, 2018

Comments in the code mention that instead of extending struct ares_options {...} (and modifying ares_init_options(), ares_save_options()), we should instead aim for adding ares_set_*() functions and modifying ares_dup(). Does this hold true now? Or is it ok to extend ares_options struct and ares_init_options()?

What implications does either approach have for backward and forward compatibility?

@SirR4T

This comment has been minimized.

Contributor

SirR4T commented Sep 1, 2018

Also, if I understand this correctly, the resolv.conf file is setup within the ares_init_options() (in init_by_resolv_conf()) itself.
So if we were to go with the ares_set_*() and ares_get_*() approach, I do not see how we could re-initialize the ares channel for the new resolv.conf path to take effect. This would either involve:

  • reordering of ares_init_options() call within the ares_dup() function, so that options are inited after copying over the channel constructs not supported by options struct, so the application using c-ares lib would have to do:
    • ares_init_options()
    • ares_set_resolvconfpath()
    • ares_dup() -> internally does ares_init_options() after first copying over the channel constructs.
      or
  • working on a new ares_reinit() function, as mentioned in the TODOs.

There's probably something else I'm missing, so would appreciate comments or suggestions on any other approach as well.

@bradh352

This comment has been minimized.

Member

bradh352 commented Sep 3, 2018

I think this is a case where it would be acceptable to simply extend ares_init_options() due to it really being part of the initialization process, unlike a lot of other options like timeouts, retries, etc.

SirR4T added a commit to SirR4T/c-ares that referenced this issue Sep 4, 2018

SirR4T added a commit to SirR4T/c-ares that referenced this issue Sep 4, 2018

SirR4T added a commit to SirR4T/c-ares that referenced this issue Sep 4, 2018

SirR4T added a commit to SirR4T/c-ares that referenced this issue Sep 4, 2018

bradh352 added a commit that referenced this issue Sep 16, 2018

Add ares_init_options() configurability for path to resolv.conf file
Add resolvconf_path to end of struct ares_options with ARES_OPT_RESOLVCONF option
so on Unix-like systems a custom path can be specified.  If no path is specified, 
/etc/resolv.conf is used like normal.

Fix By: Sarat Addepalli @SirR4T 
Fixes Bug: #220 
Review By: Brad House @bradh352
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment