(wip) use resolv_conf to parse resolv.conf files #305

little-dude · 2017-12-01T05:49:19Z

This is still work in progress but I'm opening early to have feedback about whether this is the right direction.
Here is what I did:

get rid of the lalrpop parser
move unix specific stuff to a unix.rs module
derived a couple more trait on the config types (Eq is convenient to have for tests, and Copy is convenient for NameServerConfig)

Note that for the moment, this depends on tailhook/resolv-conf#5

Also, for the moment resolv-conf does not make a distinction between domain and search: https://github.com/tailhook/resolv-conf/blob/d7ab0ce0a80f6433dbb6ba32468ad3e6b5d272c2/src/grammar.rs#L141-L149

I think this is an issue.

little-dude · 2017-12-01T05:58:05Z

resolver/Cargo.toml

 [dependencies]
+# resolv-conf = { git = "https://github.com/tailhook/resolv-conf" }
+resolv-conf = { path = "../../resolv-conf/" }


that won't compile due to this. I'll uncomment the line above one when tailhook/resolv-conf#5 is merged.

bluejekyll · 2017-12-01T19:22:44Z

Also, for the moment resolv-conf does not make a distinction between domain and search: https://github.com/tailhook/resolv-conf/blob/d7ab0ce0a80f6433dbb6ba32468ad3e6b5d272c2/src/grammar.rs#L141-L149

I think this is an issue.

I agree. Domain and Search are generally treated the same, so I understand why they are combined in the library. Their usage is here (it's a stack, so the last thing pushed will be the first lookup attempted):

https://github.com/bluejekyll/trust-dns/blob/master/resolver/src/resolver_future.rs#L186-L208

As you can see, they are basically treated the same way in the resolver, so it's not horrible that resolv-conf does this. The only difference being that the domain portion should be preferred over search.

All the other changes look good.

codecov · 2017-12-01T20:31:35Z

Codecov Report

Merging #305 into master will decrease coverage by 0.11%.
The diff coverage is 90.62%.

@@            Coverage Diff             @@
##           master     #305      +/-   ##
==========================================
- Coverage    86.8%   86.69%   -0.12%     
==========================================
  Files         114      113       -1     
  Lines       11794    11679     -115     
==========================================
- Hits        10238    10125     -113     
+ Misses       1556     1554       -2

Impacted Files	Coverage Δ
resolver/src/lib.rs	`100% <ø> (ø)`	⬆️
resolver/src/config.rs	`89.88% <100%> (+5.88%)`	⬆️
resolver/src/system_conf/unix.rs	`89.88% <89.88%> (ø)`
proto/src/dns_handle.rs	`80.09% <0%> (ø)`	⬆️
server/src/authority/authority.rs	`89.37% <0%> (+0.04%)`	⬆️
server/src/config.rs	`87.62% <0%> (+0.12%)`	⬆️
server/src/server/server_future.rs	`70.99% <0%> (+0.22%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 164dbf1...b97607b. Read the comment docs.

little-dude · 2017-12-02T00:00:57Z

I'm not sure about the logic of putting the domain at the beginning of the search list.
In DNS & Bind, they say domain and search a mutually exclusive, and I think it makes sense, because it allows users to have a searchlist that does not contain their domain (why would one want this, I don't know, but at least it's possible). If we follow this logic, ResolverConfig.domain should be an Option<Name>.

By the way, I see that by default, domain is set to . in the resolver config. I think we should try to set it to the hostname instead. To quote the man for resolv.confg:

domain Local domain name.
    Most queries for names within this domain can use short names relative to
    the local domain.  If set to '.', the root domain is considered.  If no
    domain entry is present, the domain is deter‐ mined from the local hostname
    returned by gethostname(2); the domain part is taken to be everything after
    the first '.'.  Finally, if the hostname does not contain a  domain  part,
    the root domain is assumed.

bluejekyll · 2017-12-02T00:31:26Z

I'm not sure about the logic of putting the domain at the beginning of the search list.
In DNS & Bind, they say domain and search a mutually exclusive, and I think it makes sense, because it allows users to have a searchlist that does not contain their domain (why would one want this, I don't know, but at least it's possible). If we follow this logic, ResolverConfig.domain should be an Option.

Sorry, by mutually exclusive you mean you can't have both in the resolv.conf file? If that's accurate we can change it. I may have misunderstood the specs here.

By the way, I see that by default, domain is set to . in the resolver config. I think we should try to set it to the hostname instead.

I agree that we should "try", but if it's not successful it makes sense to me to default to '.', as there aren't a lot of other options at that point...

little-dude · 2017-12-02T01:17:59Z

Sorry, by mutually exclusive you mean you can't have both in the resolv.conf file? If that's accurate we can change it. I may have misunderstood the specs here.

We can have both but the last occurence is supposed to win. Quoting man 5 resolv.conf on my linux box:

The domain and search keywords are mutually exclusive.  If more than one instance of these keywords is present, the last instance wins.

I think this is a change to do resolv-conf though. I'll propose it.

I agree that we should "try", but if it's not successful it makes sense to me to default to '.', as there aren't a lot of other options at that point...

There is one more option in case neither search nor domain is specified: the LOCALDOMAIN environment variable. Quoting the same man page:

The search keyword of a system's resolv.conf file can be overridden on a per-process basis by setting the environment variable LOCALDOMAIN to a space-separated list of search domains

little-dude · 2017-12-02T01:34:57Z

To sum up I think that this is how we should build the search list:

if LOCALDOMAIN is set, use it, and ignore the domain and search
if neither search nor domain is specified
- if hostname is a dotted name, consider what's after the first dot as the domain, and set the search list to that.
- otherwise, set the search list to ["."]
if only domain is specified, the search list has only the domain
if only search is specified, the search list is whatever is specified
if both domain and search are specified, the last one win

EDIT: at least for Linux (and BSD probably). I'm not sure if it's the same for Mac.

bluejekyll · 2017-12-02T02:10:35Z

I want to clarify some things in your proposed algorithm.

ndots: We will still always add the fqdn variant as the first lookup based on the config option
domain: The big difference between the proposal and what exists, is that the domain will only be added if it comes after search or if it's the only one present. I'm not sure I completely agree with this, I understand that it deviates from bind/glibc potentially.

I'd personally like to see domain always included, regardless of search. domain should be set on this preferential order:

a) LOCALDOMAIN environment variable
b) from hostname
c) domain section of resolv.conf
d) ., i.e. Name::root

search: Then the search list in order as specified in resolv.conf

This feels more deterministic to me. Otherwise we end up with some awkward things where some are used or not used in different situations. I do like using adding hostname and LOCALDOMAIN for configuration though. I'm open to leaving the . off the lookup and only adding it in the case there were no domain or search params as the final option.

Thoughts?

edit: I keep reading this and it doesn't make sense to me why they both exist if only one can be used. for reference if others don't have the man page: http://man7.org/linux/man-pages/man5/resolv.conf.5.html

If we decide to only support domain or search, then I think we should just remove domain from ResolverConfig all together, and only use search. I think that would remove my concerns around this being clear and deterministic...

little-dude · 2017-12-02T02:35:57Z

ndots: We will still always add the fqdn variant as the first lookup based on the config option

Yes sorry, I forgot to mention the fqdn variant. It's always tried first based on the ndots value.

domain: The big difference between the proposal and what exists, is that the domain will only be added if it comes after search or if it's the only one present. I'm not sure I completely agree with this, I understand that it deviates from bind/glibc potentially.

Yes, my only two sources are the "DNS & Bind" book and the resolv.conf man page. And I agree that this algorithm makes domain much less useful. I also agree that always including the domain makes sense. I think we agree that in that case, the domain should be tried before the domain specified by search. The only part where I disagree is to set the domain to Name::root as last resort because the probability that our host is in the root domain is close to 0, and we're going to waste one request for the root domain each time, before we try the domains specified with search. Why not make domain an Option instead and set it to None if we can't find it?

Edit: Sorry I realize I missed this sentence in your reply:

I'm open to leaving the . off the lookup and only adding it in the case there were no domain or search params as the final option.

That's basically what I'm saying too, so I think we're on the same page!

cssivision · 2017-12-02T15:48:59Z

this change is awesome.

bluejekyll · 2017-12-02T22:09:37Z

I think we are on the same page at this point. Thanks for working on this!

Some resolvers need to distinguish the domain from the search list (see for example hickory-dns/hickory-dns#305 (comment)) to build a list of suffixes.

resolv_conf[0] is a parser for resolv.conf files that depends on a single crate. Using it instead of the LALRPOP parser makes builds faster, and adds better support for comments. [0] https://github.com/tailhook/resolv-conf

Add logic to try to determin the domain from: - the LOCALDOMAIN environment variable - the hostname - the config file (/etc/resolv.conf) This adds a dependency on the "hostname" crate[0] [0] https://docs.rs/hostname/0.1.3/hostname/

little-dude · 2017-12-03T03:35:17Z

I pushed new commits that make domain option in the resolver config, and that implements domain detection from LOCALDOMAIN and hostname. I still need to add tests.

Apart from this I see two things missing I'd like to address in this PR:

when building the list of domains to try, append "." if domain and search are missing, as discussed
when building the list of domains, make sure there are not duplicates. This can happen if we have:

domain example.com
search example.com sub.example.com

bluejekyll · 2017-12-03T21:04:13Z

when building the list of domains, make sure there are not duplicates. This can happen if we have:

push_name does this today when building the set of names:

https://github.com/bluejekyll/trust-dns/blob/master/resolver/src/resolver_future.rs#L176-L180

little-dude · 2017-12-03T21:05:33Z

oh nice, I didn't realize!

bluejekyll · 2017-12-03T21:06:34Z

Yeah, it's not efficient, but I think this list will always be small in general, so the search on the Vec seems fine to me...

little-dude · 2017-12-03T23:34:47Z

I'll wait for tailhook/resolv-conf#6 to continue working on this.

cssivision · 2017-12-09T10:28:52Z

tailhook/resolv-conf#6 is already merged.

little-dude · 2017-12-10T19:21:48Z

@cssivision I'm travelling right now. I'll try to finish this up tomorrow.

bluejekyll · 2018-01-26T06:01:53Z

ping, @little-dude do you want to finish this one up? If you don't have time, I can finish it off for you...

cssivision · 2018-02-01T07:41:53Z

how about this pr? @bluejekyll

bluejekyll · 2018-02-01T07:47:11Z

resolver/src/system_conf/unix.rs

+    }
+
+    // #[test]
+    // fn test_ip_addr() {


The rest of these tests seem to be all commented out. Do you think we should just remove them?

I'm open to this.

bluejekyll · 2018-02-01T07:49:35Z

I think if the tests that are all commented out are now irrelevant, let’s just remove them.

Otherwise, the PR looks good to me, and I’m ready to merge in once it’s updated and tests are passing. Thanks for picking this back up!

Edit: looks like a misidentified you as the owner of this @cssivision. I think it's worth getting this in for the faster builds it will produce. I'll see about trying to do it this weekend if @little-dude doesn't have time.

cssivision · 2018-02-04T14:52:26Z

I cut a new pr including this change, #335

bluejekyll · 2018-02-05T17:38:27Z

this was merged in with #335

little-dude commented Dec 1, 2017

View reviewed changes

little-dude force-pushed the resolvconf branch 2 times, most recently from 8d73fbb to dbbf126 Compare December 1, 2017 16:57

bluejekyll changed the title ~~use resolv_conf to parse resolv.conf files~~ (wip) use resolv_conf to parse resolv.conf files Dec 1, 2017

little-dude mentioned this pull request Dec 3, 2017

treat domain differently than search tailhook/resolv-conf#6

Merged

little-dude added 3 commits December 2, 2017 18:50

resolver: use resolv_conf to parse config files

0a05c84

resolv_conf[0] is a parser for resolv.conf files that depends on a single crate. Using it instead of the LALRPOP parser makes builds faster, and adds better support for comments. [0] https://github.com/tailhook/resolv-conf

resolver: make domain optional

7b54029

resolver: improve domain detection on unix

b97607b

Add logic to try to determin the domain from: - the LOCALDOMAIN environment variable - the hostname - the config file (/etc/resolv.conf) This adds a dependency on the "hostname" crate[0] [0] https://docs.rs/hostname/0.1.3/hostname/

little-dude force-pushed the resolvconf branch from dbbf126 to b97607b Compare December 3, 2017 03:32

little-dude mentioned this pull request Dec 11, 2017

Introduce a Label type instead of using Rc<String> #312

Closed

bluejekyll reviewed Feb 1, 2018

View reviewed changes

bluejekyll closed this Feb 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(wip) use resolv_conf to parse resolv.conf files #305

(wip) use resolv_conf to parse resolv.conf files #305

little-dude commented Dec 1, 2017

little-dude Dec 1, 2017

bluejekyll commented Dec 1, 2017 •

edited

codecov bot commented Dec 1, 2017 •

edited

little-dude commented Dec 2, 2017

bluejekyll commented Dec 2, 2017

little-dude commented Dec 2, 2017 •

edited

little-dude commented Dec 2, 2017 •

edited

bluejekyll commented Dec 2, 2017 •

edited

little-dude commented Dec 2, 2017 •

edited

cssivision commented Dec 2, 2017

bluejekyll commented Dec 2, 2017

little-dude commented Dec 3, 2017 •

edited

bluejekyll commented Dec 3, 2017

little-dude commented Dec 3, 2017

bluejekyll commented Dec 3, 2017

little-dude commented Dec 3, 2017

cssivision commented Dec 9, 2017

little-dude commented Dec 10, 2017

bluejekyll commented Jan 26, 2018

cssivision commented Feb 1, 2018 •

edited

bluejekyll Feb 1, 2018

cssivision Feb 1, 2018

bluejekyll commented Feb 1, 2018 •

edited

cssivision commented Feb 4, 2018

bluejekyll commented Feb 5, 2018

(wip) use resolv_conf to parse resolv.conf files #305

(wip) use resolv_conf to parse resolv.conf files #305

Conversation

little-dude commented Dec 1, 2017

little-dude Dec 1, 2017

Choose a reason for hiding this comment

bluejekyll commented Dec 1, 2017 • edited

codecov bot commented Dec 1, 2017 • edited

Codecov Report

little-dude commented Dec 2, 2017

bluejekyll commented Dec 2, 2017

little-dude commented Dec 2, 2017 • edited

little-dude commented Dec 2, 2017 • edited

bluejekyll commented Dec 2, 2017 • edited

little-dude commented Dec 2, 2017 • edited

cssivision commented Dec 2, 2017

bluejekyll commented Dec 2, 2017

little-dude commented Dec 3, 2017 • edited

bluejekyll commented Dec 3, 2017

little-dude commented Dec 3, 2017

bluejekyll commented Dec 3, 2017

little-dude commented Dec 3, 2017

cssivision commented Dec 9, 2017

little-dude commented Dec 10, 2017

bluejekyll commented Jan 26, 2018

cssivision commented Feb 1, 2018 • edited

bluejekyll Feb 1, 2018

Choose a reason for hiding this comment

cssivision Feb 1, 2018

Choose a reason for hiding this comment

bluejekyll commented Feb 1, 2018 • edited

cssivision commented Feb 4, 2018

bluejekyll commented Feb 5, 2018

bluejekyll commented Dec 1, 2017 •

edited

codecov bot commented Dec 1, 2017 •

edited

little-dude commented Dec 2, 2017 •

edited

little-dude commented Dec 2, 2017 •

edited

bluejekyll commented Dec 2, 2017 •

edited

little-dude commented Dec 2, 2017 •

edited

little-dude commented Dec 3, 2017 •

edited

cssivision commented Feb 1, 2018 •

edited

bluejekyll commented Feb 1, 2018 •

edited