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

domain-specific upstream not tested #4517

Closed
3 tasks done
MkQtS opened this issue Apr 23, 2022 · 8 comments
Closed
3 tasks done

domain-specific upstream not tested #4517

MkQtS opened this issue Apr 23, 2022 · 8 comments
Assignees
Milestone

Comments

@MkQtS
Copy link
Contributor

MkQtS commented Apr 23, 2022

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

Issue Details

  • Version of AdGuard Home server:
    • v0.107.6
  • How did you install AdGuard Home:
    • GitHub releases
  • How did you setup DNS configuration:
    • Router
  • CPU architecture:
    • AMD64
  • Operating system and version:
    • OpenWrt 21.02

Expected Behavior

Set upstream DNS servers like this:

[/example.com/]server1
server2
server3

When server1 doesn't work and other servers works well, click Test upstreams, ADGH should report server1 has a problem.

Actual Behavior

ADGH reports all servers work correctly. The server which only applied to specific domains is not tested.

@fernvenue
Copy link
Contributor

Actually AdGuardHome will send a type A request of google-public-dns-a.google.com to the target upstream server to test if upstream servers working or not. So here's the problem, how to test specific upstream? What if the upstream only work for that specific domains?

Here's an example, we have an upstream server 10.0.0.1 only work for a.lan, b.lan, c.lan and so on, now we use the rule like [/lan/]10.0.0.1, and how AdGuardHome should do?

And the another problem is, sometimes we use upstream_dns_file to define a lot of rules in one list, if we already find a way to test specific servers and solved the problem above, how to make sure AdGuardHome can test all upstream servers in the list as quickly as possible, and make sure we are not gonna abuse any server?

So...I think it will be so hard to do that :(

@MkQtS
Copy link
Contributor Author

MkQtS commented Apr 24, 2022

Your explainations are convincing and it sounds hard.

It makes me confusing when a certain domain-specific upstream didn't work and I tried to use ADGH to test but ADGH didn't report anything. I think some explanation in the web UI would be helpful.

For the testing strategy, how about testing those public DNS servers as normal and testing private servers using a configured domain? Some rules may be too complex or too general to test, like a big list or [/lan/]10.0.0.1, those rules can be directly ignored and then give a notification to the user.

@fernvenue
Copy link
Contributor

@MkQtS I agree with you, telling user in the web UI that some upstream will not be tested might be a good way for now.

@ainar-g ainar-g added this to the v0.107.9 milestone Jul 25, 2022
adguard pushed a commit that referenced this issue Jul 29, 2022
Merge in DNS/adguard-home from 4517-domain-specific-test to master

Updates #4517.

Squashed commit of the following:

commit 03a803f
Merge: 8ea2417 f5959a0
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 19:17:28 2022 +0300

    Merge branch 'master' into 4517-domain-specific-test

commit 8ea2417
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 18:44:26 2022 +0300

    all: log changes, imp docs

commit aa74c8b
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 18:07:12 2022 +0300

    dnsforward: imp logging

commit 02dccca
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 17:24:08 2022 +0300

    all: imp code, docs

commit 3b21067
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 13:34:55 2022 +0300

    client: add warning toast

commit ea2272d
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Jul 28 20:11:55 2022 +0300

    dnsforward: imp err msg, docs

commit fd9ee82
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Jul 28 19:24:58 2022 +0300

    dnsforward: test doain specific upstreams

commit 9a83ebf
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Jul 28 18:22:49 2022 +0300

    dnsforward: merge some logic
@EugeneOne1
Copy link
Member

EugeneOne1 commented Jul 29, 2022

@MkQtS, @fernvenue, we've finally improved the tests. Each upstream (including domain-specific ones) now tested with test. TLD as per RFC 6761. Also, if domain-specific upstream fails the test, an appropriate warning will be displayed. Those errors aren't critical and still may be followed by a green message about success so only tests of non-specific upstreams are blocking.

Could you please check, if our implementation works properly?

@MkQtS
Copy link
Contributor Author

MkQtS commented Jul 30, 2022

@EugeneOne1, I've tried the edge build, it works totally fine. However, Server "[/domain/]server" looks weird, would it be better to use Server in "[/domain/]server"?

image

@EugeneOne1
Copy link
Member

@MkQtS, great, thanks for testing it.

If I got it correctly, you're trying to point out the difference between an upstream server's configuration line and an upstream server itself?

@MkQtS
Copy link
Contributor Author

MkQtS commented Jul 30, 2022

If I got it correctly, you're trying to point out the difference between an upstream server's configuration line and an upstream server itself?

@EugeneOne1, yes. I mean the phrase Server "[/domain/]server" in the error message, I don't think the specified domain name should be considered as a part of a server.

adguard pushed a commit that referenced this issue Aug 1, 2022
Merge in DNS/adguard-home from 4517-warning-label to master

Updates #4517.

Squashed commit of the following:

commit 4987f63
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Aug 1 13:59:28 2022 +0300

    client: imp wording
@EugeneOne1
Copy link
Member

@MkQtS, the wording should now be fixed in the latest edge. I think I can close the issue for now, the suggestions and notions are still welcome.

adguard pushed a commit that referenced this issue Aug 3, 2022
Merge in DNS/adguard-home from 4517-domain-specific-test to master

Updates #4517.

Squashed commit of the following:

commit 03a803f
Merge: 8ea2417 f5959a0
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 19:17:28 2022 +0300

    Merge branch 'master' into 4517-domain-specific-test

commit 8ea2417
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 18:44:26 2022 +0300

    all: log changes, imp docs

commit aa74c8b
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 18:07:12 2022 +0300

    dnsforward: imp logging

commit 02dccca
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 17:24:08 2022 +0300

    all: imp code, docs

commit 3b21067
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 13:34:55 2022 +0300

    client: add warning toast

commit ea2272d
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Jul 28 20:11:55 2022 +0300

    dnsforward: imp err msg, docs

commit fd9ee82
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Jul 28 19:24:58 2022 +0300

    dnsforward: test doain specific upstreams

commit 9a83ebf
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Jul 28 18:22:49 2022 +0300

    dnsforward: merge some logic
adguard pushed a commit that referenced this issue Aug 3, 2022
Merge in DNS/adguard-home from 4517-warning-label to master

Updates #4517.

Squashed commit of the following:

commit 4987f63
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Aug 1 13:59:28 2022 +0300

    client: imp wording
@EugeneOne1 EugeneOne1 mentioned this issue Aug 12, 2022
3 tasks
heyxkhoa pushed a commit to heyxkhoa/AdGuardHome that referenced this issue Mar 20, 2023
Merge in DNS/adguard-home from 4517-domain-specific-test to master

Updates AdguardTeam#4517.

Squashed commit of the following:

commit 03a803f
Merge: 8ea2417 f5959a0
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 19:17:28 2022 +0300

    Merge branch 'master' into 4517-domain-specific-test

commit 8ea2417
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 18:44:26 2022 +0300

    all: log changes, imp docs

commit aa74c8b
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 18:07:12 2022 +0300

    dnsforward: imp logging

commit 02dccca
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 17:24:08 2022 +0300

    all: imp code, docs

commit 3b21067
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 13:34:55 2022 +0300

    client: add warning toast

commit ea2272d
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Jul 28 20:11:55 2022 +0300

    dnsforward: imp err msg, docs

commit fd9ee82
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Jul 28 19:24:58 2022 +0300

    dnsforward: test doain specific upstreams

commit 9a83ebf
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Jul 28 18:22:49 2022 +0300

    dnsforward: merge some logic
heyxkhoa pushed a commit to heyxkhoa/AdGuardHome that referenced this issue Mar 20, 2023
Merge in DNS/adguard-home from 4517-warning-label to master

Updates AdguardTeam#4517.

Squashed commit of the following:

commit 4987f63
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Aug 1 13:59:28 2022 +0300

    client: imp wording
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants