Expose TTLs on lookups #444

hawkw · 2018-05-01T22:59:26Z

This PR changes the trust-dns-resolver API to expose the TTLs of DNS
lookups, which is needed downstream in Conduit (linkerd/linkerd2#711).

Reviewers should note the following:

In some cases, Lookups were constructed in places where the TTL is not
known or there is no TTL (e.g. for localhost in lookup_state.rs).
Therefore, I made the TTL field on Lookup optional, and added a new
Lookup::new_with_ttl constructor, to avoid breaking existing code.
When appending two Lookups, if both have known TTLs, the returned
Lookup has the lower of the two TTLs. Thus, the appended Lookup
should time out when the shorter-lived of either of its constituents
does. This seemed like the correct behaviour to me, so that append
doesn't become a way for short-lived Lookups to escape their TTLs,
but I wasn't sure about this --- please let me know if this intuition
is incorrect!
I've added a simple unit test for the added TTL function in lookup.rs.

Signed-off-by: Eliza Weisman eliza@buoyant.io

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

briansmith · 2018-05-01T23:08:16Z

It seems better for Trust-DNS to expose an expiration time (a deadline, an Instant) instead of the TTL (a Duration). In the case where the response is not from the internal cache then the time from which it should be measured is the time the query was sent. In the case where the response is from the internal cache then the time from which it should be measured is the time of the original query. How is the application supposed to know which of those two times to use?
In the case the Trust-DNS cache is active, how would the caller use this API to guarantee that its next request's response isn't served from the cache?

briansmith · 2018-05-01T23:09:40Z

resolver/src/lookup.rs

@@ -62,7 +81,14 @@ impl Lookup {
        rdatas.extend_from_slice(&*self.rdatas);
        rdatas.extend_from_slice(&*other.rdatas);

-        Self::new(Arc::new(rdatas))
+        // If both lookups have TTLs, choose the lower one.
+        match (self.ttl(), other.ttl()) {


It seems like we need to compare the expiration times, not the TTLs, here.

It depends on what you want to track here. The raw TTL, or the time in which the the record is no longer valid. The RFCs definitely recommend the latter.

bluejekyll

This all looks great. Thanks for the PR! I’ll merge this in once the tests pass.

Edit: I missed @briansmith comments. let's work that out.

briansmith · 2018-05-01T23:12:48Z

resolver/src/lookup.rs

+            ).wait()
+                .unwrap()
+                .ttl(),
+            Some(Duration::from_secs(86400))


It would be good to have a test for CNAME, in particular where the CNAME expires earlier than the A records.

It would also be good to have a test for NXDOMAIN that shows how this works for NXDOMAIN. In particular, "The remaining of the current meanings, of being the TTL to be used for negative responses, is the new defined meaning of the SOA minimum field." from https://tools.ietf.org/html/rfc2308; see https://tools.ietf.org/html/rfc2308#section-5.

There are some tests for TTLs as they relate to CNAME: https://github.com/bluejekyll/trust-dns/blob/master/resolver/src/lookup_state.rs#L946

We might have a gap on the NXDOMAIN/NoData responses, in regards to TTL.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

codecov · 2018-05-01T23:14:08Z

Codecov Report

Merging #444 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #444     +/-   ##
=========================================
+ Coverage   86.54%   86.64%   +0.1%     
=========================================
  Files         133      133             
  Lines       13391    13399      +8     
=========================================
+ Hits        11589    11610     +21     
+ Misses       1802     1789     -13

Impacted Files	Coverage Δ
resolver/src/lookup_ip.rs	`90.37% <ø> (ø)`	⬆️
resolver/src/hosts.rs	`95.53% <100%> (ø)`	⬆️
resolver/src/lookup.rs	`68.71% <100%> (+2.23%)`	⬆️
resolver/src/dns_lru.rs	`100% <100%> (ø)`	⬆️
resolver/src/resolver_future.rs	`72.05% <100%> (ø)`	⬆️
server/src/config.rs	`83.33% <0%> (+1.05%)`	⬆️
client/src/rr/dnssec/signer.rs	`98.95% <0%> (+1.36%)`	⬆️
resolver/src/name_server_pool.rs	`86.92% <0%> (+1.93%)`	⬆️

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 97afd93...1dfc697. Read the comment docs.

bluejekyll · 2018-05-01T23:15:56Z

resolver/src/dns_lru.rs

@@ -63,7 +63,7 @@ impl DnsLru {
        let ttl_until = now + ttl;

        // insert into the LRU
-        let lookup = Lookup::new(Arc::new(rdatas));
+        let lookup = Lookup::new_with_ttl(Arc::new(rdatas), ttl);


per @briansmith recommendation, it seems ttl_until is what is wanted here, no? I missed that before

hawkw · 2018-05-01T23:19:05Z

@briansmith

It seems better for Trust-DNS to expose an expiration time (a deadline, an Instant) instead of the TTL (a Duration). In the case where the response is not from the internal cache then the time from which it should be measured is the time the query was sent. In the case where the response is from the internal cache then the time from which it should be measured is the time of the original query. How is the application supposed to know which of those two times to use?

I agree, that seems much more correct. In 75cbbcd and ffc9cae, I've modified this PR so that the new field represents the expiration time for the lookup rather than the TTL.

briansmith · 2018-05-01T23:25:02Z

resolver/src/dns_lru.rs

@@ -63,7 +63,7 @@ impl DnsLru {
        let ttl_until = now + ttl;

        // insert into the LRU
-        let lookup = Lookup::new(Arc::new(rdatas));
+        let lookup = Lookup::new_with_ttl(Arc::new(rdatas), ttl_until);


I suggest renaming things to be consistent. Your new API functions is called valid_until. I suggest renaming the ttl_util variable here the same thing. I also suggest renaming Lookup::new_with_ttl to Lookup::new_valid_until. Then it becomes easier to understand the code holistically.

briansmith · 2018-05-01T23:25:57Z

resolver/src/lookup.rs

+    }
+
+    /// Return a new instance with the given rdatas and TTL.
+    pub fn new_with_deadline(rdatas: Arc<Vec<RData>>, ttl: Instant,) -> Self {


ttl should be renamed valid_until.

briansmith · 2018-05-01T23:34:01Z

resolver/src/lookup.rs

+            (Some(my_ttl), Some(other_ttl)) =>
+                Self::new_with_deadline(Arc::new(rdatas), min(my_ttl, other_ttl)),
+            (Some(ttl), None) | (None, Some(ttl)) =>
+                Self::new_with_deadline(Arc::new(rdatas), ttl),


It isn't clear that this logic is correct when mixing Some(ttl) and None. What's the rationale here? Is it even possible that this could happen for lookup_ip or is this something that could only happen for other kinds of queries?

OTOH maybe RFC 2308 means we should always have a TTL; if not from the record, then from the overall response? I'm not sure since I don't know all the DNS details.

Regardless, we should write down the reasoning, at least.

As far as I can tell, the only time Lookup::append is called on a Lookup that doesn't have a TTL is here:
https://github.com/bluejekyll/trust-dns/blob/97afd93b007e0ba4140be4f09ba7245f821f1fa9/resolver/src/hosts.rs#L63-L76

In this case, it seems that the empty lookups are only created as placeholders and should be immediately overwritten with the new lookup, so in this case, the logic seems correct. Everywhere else where Lookup::append is called, the lookups are actual responses to a DNS query and should always have a TTL.

I'd be happy to add a comment explaining this in the method, if you like.

hosts is an interesting case. It's driven by a file, as opposed to an actual lookup. I wonder if we could remove the optional, and just pick the max_ttl in this case. At the moment, we don't support live reloading of the hosts file. So if this is indeed the only case, then I say we get rid of it by giving it a better default.

I believe the other main case whereLookups are constructed without a TTL are the loopbacks. Do you think it's okay for localhost to appear to have MAX_TTL?

I just double checked the code, yes, I believe this is safe. My only concern was if this could have some effect on the LRU cache, but I think both of these code paths return before cache is either used. So we don't have to worry about what that would mean.

So yes, I think we can safely simplify this to default to MAX_TTL and thus get rid of the Option on TTL.

@bluejekyll okay, sounds good! I'll make that change shortly.

@bluejekyll: changed in 526b8d7

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

briansmith · 2018-05-02T18:47:32Z

resolver/src/lookup.rs

-    pub fn new(rdatas: Arc<Vec<RData>>) -> Self {
-        Lookup { rdatas }
+    /// Return new instance with given rdatas and the maximum TTL.
+    pub fn new(rdatas: Arc<Vec<RData>>,) -> Self {


I think we should switch every caller to use new_with_deadline and have them pass in valid_until. That way we don't accidentally default when we shouldn't. Is that practical or is there something that makes that awkward?

(NIT: Are we really going to do this trailing comma in arguments thing in rustfmt?)

I think we should switch every caller to use new_with_deadline and have them pass in valid_until. That way we don't accidentally default when we shouldn't. Is that practical or is there something that makes that awkward?

I don't think there's anything that makes that particularly difficult. I originally chose to leave the new constructor as-is and make it default because I wanted to keep the diff small, but I'm happy to change this so the caller is responsible for defaulting if you think that's better.

briansmith · 2018-05-02T19:03:15Z

resolver/src/lookup.rs

+    /// Return new instance with given rdatas and the maximum TTL.
+    pub fn new(rdatas: Arc<Vec<RData>>,) -> Self {
+        let valid_until =
+            Instant::now() + Duration::from_secs(MAX_TTL as u64);


Is there an Instant::MAX or something like it that can be used instead?
Instant::now() is relatively expensive (potentially a syscall). MAX_TTL is for parsing/validating input on the wire.

If there's no Instant::MAX or equivalent then maybe we should use something like enum Validity { ValidUntil(Instant), Forever }, isomorphic to Option, with comparison semantics such that ValidUntil(x) < Forever for all x.

@briansmith

If there's no Instant::MAX or equivalent then maybe we should use something like enum Validity { ValidUntil(Instant), Forever }, isomorphic to Option, with comparison semantics such that ValidUntil(x) < Forever for all x.

I like this approach (and you're correct that there's no Instant::MAX).

However, I would like to get @bluejekyll's opinion on whether we should do something like this rather than using MAX_TTL (which he suggested in https://github.com/bluejekyll/trust-dns/pull/444#discussion_r185392627) before I make more changes.

Personally I’m fine with MAX_TTL. I understand the concern @briansmith has, but I don’t feel like we need to over complicate this. As long as we guarantee that the correct TTL is used on insert.

Forcing the caller to pick a good value, rather than a decent default, seems potentially error prone as well. We could change ‘new’ to ‘with_max_ttl’ to make it more explicit.

hawkw · 2018-05-02T21:59:55Z

@bluejekyll It looks to me like the test failures on the most recent build of this branch are unrelated to my changes, do you agree that seems to be the case?

bluejekyll · 2018-05-02T23:10:54Z

I just kicked them off again. There are a couple of tests that have some races between server startup and the test running. I need to track those down and fix them.

Not your issue.

briansmith

LGTM.

bluejekyll · 2018-05-03T01:04:34Z

Thanks for the great work and the contribution!

hawkw · 2018-05-03T02:21:16Z

@bluejekyll thank you (and @briansmith) for all your guidance and feedback! :)

Closes #711. Depends on #967. This PR changes the proxy's `destination` module to honor the TTLs associated with DNS lookups, now that bluejekyll/trust-dns#444 has been merged and we can access this information from the Trust-DNS Resolver API. The `destination::background::DestinationSet` type has been modified so that, when a successful result is received for a DNS query, the DNS server will be polled again after the deadline associated with that query, rather than after a fixed deadline. The fixed deadline is still used to determine when to poll again for negative DNS responses or for errors. Furthermore, Conduit now accepts an optional CONDUIT_PROXY_DNS_MIN_TTL environment variable that will configure a minimum TTL for DNS results. If the deadline of a DNS response gives it a TTL shorter than the configured minimum, Conduit will not poll DNS again until after that minimum TTL is elapsed. By default, there is no minimum value set, as this feature is intended primarily for when Conduit is run locally for development purposes. Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw added 3 commits May 1, 2018 15:42

resolver: expose TTL on Lookup

7a39685

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

resolver: make lookup TTLs accessible from LookupIp

905c243

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

resolver: use the lower TTL when appending

e4e8886

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw mentioned this pull request May 1, 2018

TTLs are not honored when DNS is used for service discovery linkerd/linkerd2#711

Closed

briansmith reviewed May 1, 2018

View reviewed changes

bluejekyll approved these changes May 1, 2018

View reviewed changes

briansmith reviewed May 1, 2018

View reviewed changes

resolver: change lookup TTLs to deadlines rather than durations

75cbbcd

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

bluejekyll reviewed May 1, 2018

View reviewed changes

briansmith reviewed May 1, 2018

View reviewed changes

resolver: Change method names to better reflect that they're deadlines

4b61bad

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw force-pushed the eliza/resolver-ttls branch from ffc9cae to 4b61bad Compare May 1, 2018 23:42

hawkw added 4 commits May 1, 2018 16:44

resolver: rename LruValue.ttl_until for consistency

3d37bc3

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

resolver: fix test for Lookup deadlines

7ad65fc

Review feedback

fda17eb

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

resolver: use MAX_TTL rather than None when TTL is unknown

526b8d7

briansmith reviewed May 2, 2018

View reviewed changes

resolver: Review feedback

1dfc697

briansmith reviewed May 2, 2018

View reviewed changes

bluejekyll merged commit 1e43522 into hickory-dns:master May 3, 2018

hawkw mentioned this pull request May 21, 2018

proxy: Honor TTLs for DNS responses linkerd/linkerd2#974

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose TTLs on lookups #444

Expose TTLs on lookups #444

hawkw commented May 1, 2018

briansmith commented May 1, 2018

briansmith May 1, 2018

bluejekyll May 1, 2018

bluejekyll left a comment •

edited

Loading

briansmith May 1, 2018

bluejekyll May 1, 2018

codecov bot commented May 1, 2018 •

edited

Loading

bluejekyll May 1, 2018

hawkw commented May 1, 2018

briansmith May 1, 2018

briansmith May 1, 2018

briansmith May 1, 2018

hawkw May 1, 2018

bluejekyll May 2, 2018

hawkw May 2, 2018

bluejekyll May 2, 2018

hawkw May 2, 2018

hawkw May 2, 2018

briansmith May 2, 2018

hawkw May 2, 2018

briansmith May 2, 2018

hawkw May 2, 2018 •

edited

Loading

bluejekyll May 2, 2018

hawkw commented May 2, 2018

bluejekyll commented May 2, 2018 •

edited

Loading

briansmith left a comment

bluejekyll commented May 3, 2018

hawkw commented May 3, 2018

Expose TTLs on lookups #444

Expose TTLs on lookups #444

Conversation

hawkw commented May 1, 2018

briansmith commented May 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluejekyll left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 1, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

hawkw commented May 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw commented May 2, 2018

bluejekyll commented May 2, 2018 • edited Loading

briansmith left a comment

Choose a reason for hiding this comment

bluejekyll commented May 3, 2018

hawkw commented May 3, 2018

bluejekyll left a comment •

edited

Loading

codecov bot commented May 1, 2018 •

edited

Loading

hawkw May 2, 2018 •

edited

Loading

bluejekyll commented May 2, 2018 •

edited

Loading