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.

Sign up for GitHub

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

Jump to bottom

Optimistic DNS #2145

Closed
lincolnguang opened this issue Oct 2, 2020 · 14 comments
Closed

Optimistic DNS #2145

lincolnguang opened this issue Oct 2, 2020 · 14 comments
Assignees
@EugeneOne1
Labels
enhancement external libs Issues that require changes in external libraries. P3: Medium UI
Milestone
v0.107.0

Comments

@lincolnguang
Copy link

Prerequisites

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

  • [-] I am running the latest version
  • [-] I checked the documentation and found no answer
  • [-] I checked to make sure that this issue has not already been filed

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 ameshkov added this to the v0.107 milestone Oct 2, 2020
@ameshkov
Copy link
Member

ameshkov commented Oct 2, 2020

Makes perfect sense, thanks for filing the feature request!

@lancelot-moon
Copy link

This was done in AG for Android...
AdguardTeam/AdguardForAndroid#2539

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

@ameshkov ameshkov mentioned this issue Oct 2, 2020
Closed
@ameshkov
Copy link
Member

ameshkov commented Oct 2, 2020

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

@ameshkov ameshkov mentioned this issue Dec 16, 2020
Closed
@ameshkov ameshkov mentioned this issue Jan 19, 2021
Open
@IrineSistiana
Copy link

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
Copy link
Member

ameshkov commented Feb 5, 2021

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 ainar-g added the external libs Issues that require changes in external libraries. label Jun 3, 2021
@ainar-g ainar-g changed the title [Feature Request] Optimistic DNS Optimistic DNS Jun 3, 2021
@ameshkov ameshkov mentioned this issue Jun 15, 2021
Closed
3 tasks
@ainar-g ainar-g added the UI label Jun 29, 2021
adguard pushed a commit that referenced this issue Jul 14, 2021
@ainar-g
Pull request: 2145 optimistic cache
b9e8569 
Updates #2145.

Squashed commit of the following:

commit 0c15347
Merge: 98bd3b8 ebade2b
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Wed Jul 14 20:44:58 2021 +0300

    Merge branch 'master' into 2145-optimistic-cache

commit 98bd3b8
Author: Ildar Kamalov <ik@adguard.com>
Date:   Wed Jul 14 13:12:56 2021 +0300

    client: handle optimistic cache

commit 0b469b7
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Thu Jul 1 19:01:01 2021 +0300

    openapi: fix log of changes

commit f1594e7
Merge: a034eb9 e113b27
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Thu Jul 1 18:53:01 2021 +0300

    Merge branch 'master' into 2145-optimistic-cache

commit a034eb9
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Tue Jun 29 18:45:28 2021 +0300

    dnsforward: fix tests

commit c72227f
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Tue Jun 29 18:33:12 2021 +0300

    openapi: imp docs

commit 35fe0d2
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Mon Jun 28 14:19:46 2021 +0300

    dnsforward: add optimistic cache
@ainar-g
Copy link
Contributor

ainar-g commented Jul 14, 2021

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

@lincolnguang
Copy link
Author

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
Copy link
Contributor

ainar-g commented Jul 15, 2021

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.

@ainar-g ainar-g closed this as completed Jul 15, 2021
@jumpsmm7 jumpsmm7 mentioned this issue Dec 23, 2021
Closed
@demifiend9
Copy link

demifiend9 commented Dec 28, 2021 •
edited
Loading

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
Copy link

@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
Copy link
Member

ameshkov commented Mar 8, 2022

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

adguard pushed a commit to AdguardTeam/dnsproxy that referenced this issue Mar 9, 2022
@EugeneOne1
Pull request: proxy: decrease optimistic ttl
e2fba4e 
Merge in DNS/dnsproxy from optimistic-ttl to master

Updates AdguardTeam/AdGuardHome#2145.

Squashed commit of the following:

commit 2b0a13c
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Mar 9 17:53:13 2022 +0500

    proxy: decr optimistic ttl
adguard pushed a commit that referenced this issue Mar 9, 2022
@EugeneOne1
Pull request: decr optimistic ttl
e7b3c99 
Merge in DNS/adguard-home from 2145-optimistic-ttl to master

Updates #2145.

Squashed commit of the following:

commit 81e5aba
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Mar 9 18:34:50 2022 +0500

    all: upd proxy
@EugeneOne1
Copy link
Member

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

@JsBergbau
Copy link

Thank you very much.

@ghost
Copy link

ghost commented Apr 16, 2022

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

@ignoramous ignoramous mentioned this issue Mar 19, 2023
Closed
heyxkhoa pushed a commit to heyxkhoa/AdGuardHome that referenced this issue Mar 20, 2023
@ainar-g @heyxkhoa
Pull request: 2145 optimistic cache
32990e8 
Updates AdguardTeam#2145.

Squashed commit of the following:

commit 0c15347
Merge: 98bd3b8 ebade2b
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Wed Jul 14 20:44:58 2021 +0300

    Merge branch 'master' into 2145-optimistic-cache

commit 98bd3b8
Author: Ildar Kamalov <ik@adguard.com>
Date:   Wed Jul 14 13:12:56 2021 +0300

    client: handle optimistic cache

commit 0b469b7
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Thu Jul 1 19:01:01 2021 +0300

    openapi: fix log of changes

commit f1594e7
Merge: a034eb9 e113b27
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Thu Jul 1 18:53:01 2021 +0300

    Merge branch 'master' into 2145-optimistic-cache

commit a034eb9
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Tue Jun 29 18:45:28 2021 +0300

    dnsforward: fix tests

commit c72227f
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Tue Jun 29 18:33:12 2021 +0300

    openapi: imp docs

commit 35fe0d2
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Mon Jun 28 14:19:46 2021 +0300

    dnsforward: add optimistic cache
heyxkhoa pushed a commit to heyxkhoa/AdGuardHome that referenced this issue Mar 20, 2023
@EugeneOne1 @heyxkhoa
Pull request: decr optimistic ttl
ee42915 
Merge in DNS/adguard-home from 2145-optimistic-ttl to master

Updates AdguardTeam#2145.

Squashed commit of the following:

commit 81e5aba
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Mar 9 18:34:50 2022 +0500

    all: upd proxy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@EugeneOne1 EugeneOne1

Labels
enhancement external libs Issues that require changes in external libraries. P3: Medium UI
Projects
None yet
Milestone
v0.107.0
Development

No branches or pull requests
8 participants
@ainar-g @ameshkov @demifiend9 @lancelot-moon @lincolnguang @JsBergbau @EugeneOne1 @IrineSistiana