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

Make AsyncResolver take hosts file into account #2149

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

hch12907
Copy link
Contributor

@hch12907 hch12907 commented Feb 19, 2024

Closes #2148.

I'm creating the PR for feedback.

This does not actually work. Given a /etc/hosts file containing 201.11.11.11 example.com, looking up the A record for example.com. using AsyncResolver will still return 93.184.216.34, so FQDN seems to trip Hosts::lookup_static_host() up. This is fixed by 9564566.

@hch12907
Copy link
Contributor Author

hch12907 commented Feb 19, 2024

Pushed a new commit that resolves the FQDN issue, although I am not sure whether it is the correct fix or not.

@djc
Copy link
Collaborator

djc commented Mar 1, 2024

It seems quite surprising that the lookup_ip() handling can differ quite a bit from basic lookup(), especially if more generic callers want to rely on lookup() for the wider applicability. Perhaps we should change generally change lookup_ip() to rely as completely as possible on lookup()? @bluejekyll thoughts?

crates/resolver/src/hosts.rs Outdated Show resolved Hide resolved
@bluejekyll
Copy link
Member

I generally agree @djc. For some background here, the Hosts file lookup is only valuable in the context of IP lookups, which is why there's different code paths here. I agree with unifying them. There's a challenge here that as I remember was around the generic interface that lookup supports vs. the specific interface that lookup_ip offers. I'm not sure how easy it is to merge those two, and I think that's the issue you're running into @hch12907?

I need to look at this in more detail when I have time to offer some better guidance here.

@djc
Copy link
Collaborator

djc commented Mar 6, 2024

The hosts file part seems kind of straightforward at least, since you could just handle it only for A/AAAA lookups. On the other hand, the IP lookup strategy seems harder to unify with the lookup_inner() code path, but maybe that's okay?

@hch12907
Copy link
Contributor Author

hch12907 commented Mar 9, 2024

The main problem with merging lookup and lookup_ip mainly has to do with that one is dualstacked and one is not, yeah.

@djc
Copy link
Collaborator

djc commented Mar 9, 2024

Maybe for now it would be good enough to just move the hosts file lookup from lookup_ip() to lookup() or lookup_inner(), only for A and AAAA queries?

@hch12907
Copy link
Contributor Author

Duplicating hosts lookup in both lookup and lookup_ip sounds more reasonable to me because the two functions follow very different code paths. The duplication is minimal, just a few lines long.

crates/resolver/src/lookup.rs Show resolved Hide resolved

// Ignore FQDN when we forward DNS queries. Without this we can't look
// up addresses from system hosts file.
let mut name: Name = name.clone().into();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a non-trivial change -- why do you think this is the right thing to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The query sent to upstream DNS is the same no matter FQDN is enabled or not. It seems that FQDN only affects hosts lookup.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want the Forwarder to use the local resolve config at all? Do other DNS servers do that on forwarded requests? I don't think so. I think if we want a Hosts file to be used, we should create a separate Authority type for that...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think dnsmasq does that, not too sure though... But maybe we can disable use_hosts_file by default in ForwardConfig?

Copy link
Collaborator

@djc djc Mar 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good to me.

@bluejekyll
Copy link
Member

Overall, I think I'm ok with this change, on the Forwarder, I think we actually don't want the resolve config file to be referenced, if I enabled that in the past, it probably was a poor decision.

@bluejekyll
Copy link
Member

To be clear, I think this is ready to land if we could just clean up the setup for the Forwarded Authority.

@hch12907
Copy link
Contributor Author

hch12907 commented Mar 19, 2024

I made a branch which makes use_hosts_file disabled for ForwardAuthority by default but not AsyncResolver: here.

If this is okay I can merge the branch back into this PR, but as an alternative I can drop 190a43a.

@djc
Copy link
Collaborator

djc commented Mar 20, 2024

I think your branch looks okay, but I think it would be helpful to use an explicit enum rather than Option<bool> -- I think the semantics for the latter would be kind of hard to explain and not obvious in code review.

Would propose something like:

enum ResolveHosts {
    Always,
    Never,
    Default, // maybe not the best name? `Depends` or `Contextual` or ...
}

@hch12907
Copy link
Contributor Author

I opted to name the third variant Auto (so that it's mostly consistent with other Linux configs that go [true, false, auto])

crates/resolver/src/config.rs Outdated Show resolved Hide resolved
crates/resolver/src/lookup.rs Show resolved Hide resolved
The Auto option is meant to allow downstream users to intercept the
config and replace it with what they deem to be suitable.
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.

Forwarder seems to not use /etc/hosts despite use_hosts_file == true
3 participants