AdguardTeam / AdGuardHome

Network-wide ads & trackers blocking DNS server
https://adguard.com/adguard-home.html
GNU General Public License v3.0
24.67k stars 1.78k forks source link

Optimistic DNS #2145

Closed lincolnguang closed 3 years ago

lincolnguang commented 3 years ago

Prerequisites

Please answer the following questions for yourself before submitting an issue. YOU MAY DELETE THE PREREQUISITES SECTION.

Problem Description

Optimistic DNS can reduce latency caused by DNS resolution when a client reconnects after a record has expired, which idea from apple in page 13.

Proposed Solution

When the local DNS cache expires, Adguard Home can continue answer with the IP in the local cache results with 1 TTL, while a new DNS query is made to update the cache. If client can still connect to the server by using the old results, then that's great and reduces the time waiting for DNS query. And if not, after a short TTL, the new DNS result can be sent to client and reconnect again.

Additional Information

Also, there is another Go project named Clash support this feature, you can check this pr.

ameshkov commented 3 years ago

Makes perfect sense, thanks for filing the feature request!

lancelot-moon commented 3 years ago

This was done in AG for Android... https://github.com/AdguardTeam/AdguardForAndroid/issues/2539

I'm astonished that it's not provided in AG Home. Does AG for iOS support Optimistic DNS?

ameshkov commented 3 years ago

I am not so sure DnsLibs (the new implementation that replaced dnsproxy) actually supports optimistic DNS, so I copied this FR there as well: https://github.com/AdguardTeam/DnsLibs/issues/83

IrineSistiana commented 3 years ago

Just in curiosity.

Optimistic DNS can reduce latency caused by DNS resolution when a client reconnects after a record has expired, which idea from apple in page 13.

App developers might know that their servers' IPs will not be changed easily. So it's safe for them to implement Optimistic DNS.

DNS is a system with an eventual consistency model. Is it safe to modify TTL arbitrarily in this way for all clients?

ameshkov commented 3 years ago

Tbh, I doubt it will cause any issues. DNS records TTL does not guarantee an exact precision due to its nature (it's always TTL + latency).

ainar-g commented 3 years ago

This feature should be added to the edge channel as of revision b9e85695db80bef00172f95dde6b7a68572513a3. Can you please check if our implementation works for you?

lincolnguang commented 3 years ago

It works very well. I've noticed that Optimistic DNS gives a default ttl of 60 for expired DNS records, which seems a bit long, and 10 might be a bit better. image

Maybe there needs to be an additional flag for records that trigger Optimistic DNS. btw, if I find anything else, I'll reply under this issue.

ainar-g commented 3 years ago

We'll consider lowering the cache TTL, thanks for the suggestion. As for showing it in the query log, we plan to introduce a cache indicator in the new design, so we're not sure if optimistic cache requires a special treatment there, but we'll think about it.

Thanks for testing the feature! I'll close this issue for now. If you have any new suggestions or find bugs, please fill a new issue.

demifiend9 commented 2 years ago

The TTL of 60 is too high for expired records and I've personally encountered problems when trying to access home servers using ddns.

Most websites have static ip so serving expired records with high TTL isn't a big deal but ISPs give dynamic ip to homes which change frequently on router reboots.

The purpose of simultaneously refreshing cache when serving expired record as described by the OP is that "if something does break, a simple refresh would fix it". The TTL of 60 defeats the purpose. It should be something very small like 1 or at max 5.

JsBergbau commented 2 years ago

@ameshkov Thanks for implementing this. I agree with previous messages that ttl of 60 is way too long for optimistic caching. When blocking a URL via Filters --> Blocked Services, then a TTL of 10 is set. Would be great if could reduce TTL for optimistic caching to 10 seconds, maybe 5 would be even better. Thank you very much a lot.

ameshkov commented 2 years ago

@ainar-g can we maybe simply make it 10 seconds by default? Making it configurable seems too much to me.

EugeneOne1 commented 2 years ago

@JsBergbau, it's now 10 second in the latest edge builds.

JsBergbau commented 2 years ago

Thank you very much.

ghost commented 2 years ago

Does this feature make DoT/DoH/DoQ caching more susceptible to poisoning?