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

Cache DNS records based on the record requested #56

Closed
wants to merge 1 commit into from

Conversation

Peleg
Copy link
Contributor

@Peleg Peleg commented May 26, 2017

Currently amp/dns caches records returned from the dns server indexed by their returned type instead of the requested type. that means if we look up a DNAME record and instead receive a
CNAME record, it'll be cached under CNAME, leading to amp/dns making additional
requests for the DNAME record although it received a CNAME as an alternative after the first request.

This commit changes this behavior and instead caches the record with the
requested type as the key rather than the returned type.

Currently Amp/DNS will only cache (in-memory) for the record type returned from
the DNS server. that means if we look up a DNAME record and instead receive a
CNAME record, it'll be cached under CNAME, leading amp to make additional
requests for the DNAME record it technically already received.

This commit changes this behavior and instead caches the record with the
requested type as the key rather than the returned type.
@mention-bot
Copy link

@Peleg, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kelunik, @bwoebi and @trowski to be potential reviewers.

@kelunik
Copy link
Member

kelunik commented May 26, 2017

I guess it should store both?

@Peleg
Copy link
Contributor Author

Peleg commented May 26, 2017

@kelunik is it guaranteed that when you ask for DNAME and get a CNAME instead, that when you later ask for CNAME you'll get the same CNAME record?

if so, then sure thing. ill push an update

@kelunik
Copy link
Member

kelunik commented May 26, 2017

@Peleg I honestly don't know. Better ask @bwoebi or @DaveRandom.

@bwoebi
Copy link
Member

bwoebi commented May 26, 2017

I don't either, deferring to @DaveRandom ...

@DaveRandom
Copy link
Contributor

DaveRandom commented May 26, 2017

https://tools.ietf.org/html/rfc6672#section-3.1

Servers MUST be able to answer a query for a synthesized CNAME. Like other query types, this invokes the DNAME, and then the server synthesizes the CNAME and places it into the answer section. If the server in question is a cache, the synthesized CNAME's TTL SHOULD be equal to the decremented TTL of the cached DNAME.

As I read that, the short answer is yes, unless the DNAME record itself has changed.

The long answer, considering the wider implications, is a thing that I am going to need to think about a lot more and probably read that entire RFC through another 10+ times before I even have an opinion :-P

That said, it is still my view that we should not be explicitly querying for DNAME records, because servers are required to perform substitutions themselves. Thus (apart from the very specific use case of writing a DNS cache server application) we shouldn't actually be querying them at all in the normal name resolution flow - i.e. when we are attempting to resolve any explicit RR type other than DNAME itself.

@Peleg
Copy link
Contributor Author

Peleg commented May 26, 2017

thanks @DaveRandom. fwiw, this commit would also apply to other record types and not just DNAME => CNAME. ie when requesting an AAAA type record and getting a CNAME in return from the server, should it be cached under CNAME and AAAA, or just under AAAA as that was the type requested?

before this commit, AAAA resolving in CNAME would only be cached under CNAME. so i doubt caching under both keys would break anything. or at least it hasn't been brought up.

@DaveRandom
Copy link
Contributor

DaveRandom commented May 27, 2017

Sure I understand that 👍

I'm sorry but I've not really had time to properly review this patch and the implications of it, and I'm probably not going to be able to in the next week or so, but if others are happy with it I'll defer to them.

It may make sense to have a 2-layered approach to caching - caching entire responses, so that if a response is cached for a with an exact match for the query that can be used, if that isn't present then a "record"-level cache can be used to try and build a complete response. This is complicated somewhat by the fact that some servers will transparently dereference CNAMEs in some scenarios (e.g. most of the time if you send an A query for an fqn constructed from a CNAME, you will get the actual target A-record back).

That is beyond the scope of this PR though, which from a cursory look seems to stand on it's own and provide a useful change.

@bwoebi
Copy link
Member

bwoebi commented May 27, 2017

@DaveRandom given that it's really easy to miss small things in RFCs, I'd prefer you do it as you're probably most familiar with it of us all. It's not required that it's merged quickly, I'd rather be 100% sure it's done correctly.

@kelunik
Copy link
Member

kelunik commented Jun 16, 2017

I think we should just remove CNAME / DNAME requests, right?

@kelunik
Copy link
Member

kelunik commented Jun 23, 2017

The Amp v2 version will only query CNAME if there's no A or AAAA record and only query DNAME if there's no CNAME, I think that solves it. Additionally, negative results are cached for 300 seconds.

@kelunik kelunik closed this Jun 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants