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

Add resolver fallback to the null resolver (getaddrinfo) #103

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

delthas
Copy link
Contributor

@delthas delthas commented Jul 25, 2023

This enables a best-effort, two-step resolution when a resolver is configured:

  • if the DNS resolver returns any domain, that record is used with its corresponding TTL
  • otherwise, the module tries resolving the name with the default, NULL resolver, which is just a wrapper over getaddrinfo
    • if the resolution succeeds, this was likely a domain in /etc/hosts or an IP literal, which was not known by the DNS server. The associated record has no TTL value and uses the default director TTL.
    • if the resolution fails, this was likely a bad domain. No records are stored and a new request is made after the default director TTL, as was done previously.

The patch is somewhat dumb, it just wraps the resolve logic in a loop that can iterate twice. The goal was to minimize the size of git diff -w.

This causes the module to log Lookup & Results & Error twice in case we do the fallback attempt, which could be a nice side-effect (we can investigate a DNS error in varnishlog but see that it still succeeded). I don't have a strong opinion about this one.

Feel free to amend and merge if you'd like. Otherwise I can make changes. 😄

This enables a best-effort, two-step resolution when a resolver is
configured:
- if the DNS resolver returns any domain, that record is used with its
  corresponding TTL
- otherwise, the module tries resolving the name with the default, NULL
  resolver, which is just a wrapper over getaddrinfo
  * if the resolution succeeds, this was likely a domain in /etc/hosts
    or an IP literal, which was not known by the DNS server. The
    associated record has no TTL value and uses the default director
    TTL.
  * if the resolution fails, this was likely a bad domain. No records
    are stored and a new request is made after the default director TTL,
    as was done previously.
@nigoroll
Copy link
Owner

nigoroll commented Jul 25, 2023

Thank you for your work on this!

Regarding the implementation, I would actually like to see if we can avoid the additional while block. As you already said you'd be fine with amending, I would just do that.

But, more importantly:

  • Shouldn't we make the fallback optional? I fear there might be cases where /etc/hosts in particular should not be used.
  • Or should we even go one step further and support a list of resolvers to try?

@nigoroll
Copy link
Owner

Hi @delthas, are you still interested in this patch? If yes, I would be interested in your opinion about my two questions:

  • Shouldn't we make the fallback optional? I fear there might be cases where /etc/hosts in particular should not be used.

  • Or should we even go one step further and support a list of resolvers to try?

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