Pull request: 4517 domain specific test

Merge in DNS/adguard-home from 4517-domain-specific-test to master

Updates #4517.

Squashed commit of the following:

commit 03a803f831749a060923ec966592696f99591786
Merge: 8ea24170 f5959a0d
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 8ea2417036547996bb2d39b75b0ff31de4fe9b21
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 18:44:26 2022 +0300

    all: log changes, imp docs

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

    dnsforward: imp logging

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

    all: imp code, docs

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

    client: add warning toast

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

    dnsforward: imp err msg, docs

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

    dnsforward: test doain specific upstreams

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

    dnsforward: merge some logic

CHANGELOG.md

@@ -22,6 +22,8 @@ and this project adheres to
 
 ### Added
 
+- Domain-specific upstream servers test.  Such test fails with an appropriate
+  warning message ([#4517]).
 - Support for Discovery of Designated Resolvers (DDR) according to the [RFC
   draft][ddr-draft] ([#4463]).
 - `windows/arm64` support ([#3057]).
@@ -37,6 +39,7 @@ and this project adheres to
 
 [#2993]: https://github.com/AdguardTeam/AdGuardHome/issues/2993
 [#3057]: https://github.com/AdguardTeam/AdGuardHome/issues/3057
+[#4517]: https://github.com/AdguardTeam/AdGuardHome/issues/4517
 
 [ddr-draft]: https://datatracker.ietf.org/doc/html/draft-ietf-add-ddr-08
 

client/src/__locales/en.json

@@ -222,6 +222,7 @@
     "updated_upstream_dns_toast": "Upstream servers successfully saved",
     "dns_test_ok_toast": "Specified DNS servers are working correctly",
     "dns_test_not_ok_toast": "Server \"{{key}}\": could not be used, please check that you've written it correctly",
+    "dns_test_warning_toast": "Server \"{{key}}\" does not respond to test requests and may not work properly",
     "unblock": "Unblock",
     "block": "Block",
     "disallow_this_client": "Disallow this client",

client/src/actions/index.js

@@ -314,13 +314,15 @@ export const testUpstream = (
         const testMessages = Object.keys(upstreamResponse)
             .map((key) => {
                 const message = upstreamResponse[key];
-                if (message !== 'OK') {
+                if (message.startsWith('WARNING:')) {
+                    dispatch(addErrorToast({ error: i18next.t('dns_test_warning_toast', { key }) }));
+                } else if (message !== 'OK') {
                     dispatch(addErrorToast({ error: i18next.t('dns_test_not_ok_toast', { key }) }));
                 }
                 return message;
             });
 
-        if (testMessages.every((message) => message === 'OK')) {
+        if (testMessages.every((message) => message === 'OK' || message.startsWith('WARNING:'))) {
             dispatch(addSuccessToast('dns_test_ok_toast'));
         }
 

internal/dnsforward/http.go

@@ -363,6 +363,21 @@ func newUpstreamConfig(upstreams []string) (conf *proxy.UpstreamConfig, err erro
 		return nil, nil
 	}
 
+	for _, u := range upstreams {
+		var ups string
+		var domains []string
+		ups, domains, err = separateUpstream(u)
+		if err != nil {
+			// Don't wrap the error since it's informative enough as is.
+			return nil, err
+		}
+
+		_, err = validateUpstream(ups, domains)
+		if err != nil {
+			return nil, fmt.Errorf("validating upstream %q: %w", u, err)
+		}
+	}
+
 	conf, err = proxy.ParseUpstreamsConfig(
 		upstreams,
 		&upstream.Options{Bootstrap: []string{}, Timeout: DefaultTimeout},
@@ -373,13 +388,6 @@ func newUpstreamConfig(upstreams []string) (conf *proxy.UpstreamConfig, err erro
 		return nil, errors.Error("no default upstreams specified")
 	}
 
-	for _, u := range upstreams {
-		_, err = validateUpstream(u)
-		if err != nil {
-			return nil, err
-		}
-	}
-
 	return conf, nil
 }
 
@@ -449,16 +457,14 @@ func ValidateUpstreamsPrivate(upstreams []string, privateNets netutil.SubnetSet)
 
 var protocols = []string{"udp://", "tcp://", "tls://", "https://", "sdns://", "quic://"}
 
-func validateUpstream(u string) (useDefault bool, err error) {
+// validateUpstream returns an error if u alongside with domains is not a valid
-	// Check if the user tries to specify upstream for domain.
+// upstream configuration.  useDefault is true if the upstream is
-	var isDomainSpec bool
+// domain-specific and is configured to point at the default upstream server
-	u, isDomainSpec, err = separateUpstream(u)
+// which is validated separately.  The upstream is considered domain-specific
-	if err != nil {
+// only if domains is at least not nil.
-		return !isDomainSpec, err
+func validateUpstream(u string, domains []string) (useDefault bool, err error) {
-	}
-
 	// The special server address '#' means that default server must be used.
-	if useDefault = !isDomainSpec; u == "#" && isDomainSpec {
+	if useDefault = u == "#" && domains != nil; useDefault {
 		return useDefault, nil
 	}
 
@@ -485,12 +491,14 @@ func validateUpstream(u string) (useDefault bool, err error) {
 	return useDefault, nil
 }
 
-// separateUpstream returns the upstream without the specified domains.
+// separateUpstream returns the upstream and the specified domains.  domains is
-// isDomainSpec is true when the upstream is domains-specific.
+// nil when the upstream is not domains-specific.  Otherwise it may also be
-func separateUpstream(upstreamStr string) (upstream string, isDomainSpec bool, err error) {
+// empty.
+func separateUpstream(upstreamStr string) (ups string, domains []string, err error) {
 	if !strings.HasPrefix(upstreamStr, "[/") {
-		return upstreamStr, false, nil
+		return upstreamStr, nil, nil
 	}
+
 	defer func() { err = errors.Annotate(err, "bad upstream for domain %q: %w", upstreamStr) }()
 
 	parts := strings.Split(upstreamStr[2:], "/]")
@@ -498,40 +506,46 @@ func separateUpstream(upstreamStr string) (upstream string, isDomainSpec bool, e
 	case 2:
 		// Go on.
 	case 1:
-		return "", false, errors.Error("missing separator")
+		return "", nil, errors.Error("missing separator")
 	default:
-		return "", true, errors.Error("duplicated separator")
+		return "", []string{}, errors.Error("duplicated separator")
 	}
 
-	var domains string
+	for i, host := range strings.Split(parts[0], "/") {
-	domains, upstream = parts[0], parts[1]
-	for i, host := range strings.Split(domains, "/") {
 		if host == "" {
 			continue
 		}
 
-		host = strings.TrimPrefix(host, "*.")
+		err = netutil.ValidateDomainName(strings.TrimPrefix(host, "*."))
-		err = netutil.ValidateDomainName(host)
 		if err != nil {
-			return "", true, fmt.Errorf("domain at index %d: %w", i, err)
+			return "", domains, fmt.Errorf("domain at index %d: %w", i, err)
-		}
 		}
 
-	return upstream, true, nil
+		domains = append(domains, host)
+	}
+
+	return parts[1], domains, nil
 }
 
-// excFunc is a signature of function to check if upstream exchanges correctly.
+// healthCheckFunc is a signature of function to check if upstream exchanges
-type excFunc func(u upstream.Upstream) (err error)
+// properly.
+type healthCheckFunc func(u upstream.Upstream) (err error)
 
 // checkDNSUpstreamExc checks if the DNS upstream exchanges correctly.
 func checkDNSUpstreamExc(u upstream.Upstream) (err error) {
+	// testTLD is the special-use fully-qualified domain name for testing the
+	// DNS server reachability.
+	//
+	// See https://datatracker.ietf.org/doc/html/rfc6761#section-6.2.
+	const testTLD = "test."
+
 	req := &dns.Msg{
 		MsgHdr: dns.MsgHdr{
 			Id:               dns.Id(),
 			RecursionDesired: true,
 		},
 		Question: []dns.Question{{
-			Name:   "google-public-dns-a.google.com.",
+			Name:   testTLD,
 			Qtype:  dns.TypeA,
 			Qclass: dns.ClassINET,
 		}},
@@ -541,12 +555,8 @@ func checkDNSUpstreamExc(u upstream.Upstream) (err error) {
 	reply, err = u.Exchange(req)
 	if err != nil {
 		return fmt.Errorf("couldn't communicate with upstream: %w", err)
-	}
+	} else if len(reply.Answer) != 0 {
-
+		return errors.Error("wrong response")
-	if len(reply.Answer) != 1 {
-		return fmt.Errorf("wrong response")
-	} else if a, ok := reply.Answer[0].(*dns.A); !ok || !a.A.Equal(net.IP{8, 8, 8, 8}) {
-		return fmt.Errorf("wrong response")
 	}
 
 	return nil
@@ -554,14 +564,22 @@ func checkDNSUpstreamExc(u upstream.Upstream) (err error) {
 
 // checkPrivateUpstreamExc checks if the upstream for resolving private
 // addresses exchanges correctly.
+//
+// TODO(e.burkov):  Think about testing the ip6.arpa. as well.
 func checkPrivateUpstreamExc(u upstream.Upstream) (err error) {
+	// inAddrArpaTLD is the special-use fully-qualified domain name for PTR IP
+	// address resolution.
+	//
+	// See https://datatracker.ietf.org/doc/html/rfc1035#section-3.5.
+	const inAddrArpaTLD = "in-addr.arpa."
+
 	req := &dns.Msg{
 		MsgHdr: dns.MsgHdr{
 			Id:               dns.Id(),
 			RecursionDesired: true,
 		},
 		Question: []dns.Question{{
-			Name:   "1.0.0.127.in-addr.arpa.",
+			Name:   inAddrArpaTLD,
 			Qtype:  dns.TypePTR,
 			Qclass: dns.ClassINET,
 		}},
@@ -574,46 +592,66 @@ func checkPrivateUpstreamExc(u upstream.Upstream) (err error) {
 	return nil
 }
 
-func checkDNS(input string, bootstrap []string, timeout time.Duration, ef excFunc) (err error) {
+// domainSpecificTestError is a wrapper for errors returned by checkDNS to mark
-	if IsCommentOrEmpty(input) {
+// the tested upstream domain-specific and therefore consider its errors
+// non-critical.
+//
+// TODO(a.garipov):  Some common mechanism of distinguishing between errors and
+// warnings (non-critical errors) is desired.
+type domainSpecificTestError struct {
+	error
+}
+
+// checkDNS checks the upstream server defined by upstreamConfigStr using
+// healthCheck for actually exchange messages.  It uses bootstrap to resolve the
+// upstream's address.
+func checkDNS(
+	upstreamConfigStr string,
+	bootstrap []string,
+	timeout time.Duration,
+	healthCheck healthCheckFunc,
+) (err error) {
+	if IsCommentOrEmpty(upstreamConfigStr) {
 		return nil
 	}
 
 	// Separate upstream from domains list.
-	var useDefault bool
+	upstreamAddr, domains, err := separateUpstream(upstreamConfigStr)
-	if useDefault, err = validateUpstream(input); err != nil {
+	if err != nil {
 		return fmt.Errorf("wrong upstream format: %w", err)
 	}
 
-	// No need to check this DNS server.
+	useDefault, err := validateUpstream(upstreamAddr, domains)
-	if !useDefault {
+	if err != nil {
+		return fmt.Errorf("wrong upstream format: %w", err)
+	} else if useDefault {
 		return nil
 	}
 
-	if input, _, err = separateUpstream(input); err != nil {
-		return fmt.Errorf("wrong upstream format: %w", err)
-	}
-
 	if len(bootstrap) == 0 {
 		bootstrap = defaultBootstrap
 	}
 
-	log.Debug("checking if upstream %s works", input)
+	log.Debug("dnsforward: checking if upstream %q works", upstreamAddr)
 
-	var u upstream.Upstream
+	u, err := upstream.AddressToUpstream(upstreamAddr, &upstream.Options{
-	u, err = upstream.AddressToUpstream(input, &upstream.Options{
 		Bootstrap: bootstrap,
 		Timeout:   timeout,
 	})
 	if err != nil {
-		return fmt.Errorf("failed to choose upstream for %q: %w", input, err)
+		return fmt.Errorf("failed to choose upstream for %q: %w", upstreamAddr, err)
 	}
 
-	if err = ef(u); err != nil {
+	if err = healthCheck(u); err != nil {
-		return fmt.Errorf("upstream %q fails to exchange: %w", input, err)
+		err = fmt.Errorf("upstream %q fails to exchange: %w", upstreamAddr, err)
+		if domains != nil {
+			return domainSpecificTestError{error: err}
 		}
 
-	log.Debug("upstream %s is ok", input)
+		return err
+	}
+
+	log.Debug("dnsforward: upstream %q is ok", upstreamAddr)
 
 	return nil
 }
@@ -636,6 +674,9 @@ func (s *Server) handleTestUpstreamDNS(w http.ResponseWriter, r *http.Request) {
 		if err != nil {
 			log.Info("%v", err)
 			result[host] = err.Error()
+			if _, ok := err.(domainSpecificTestError); ok {
+				result[host] = fmt.Sprintf("WARNING: %s", result[host])
+			}
 
 			continue
 		}
@@ -651,6 +692,9 @@ func (s *Server) handleTestUpstreamDNS(w http.ResponseWriter, r *http.Request) {
 			// above, we rewriting the error for it.  These cases should be
 			// handled properly instead.
 			result[host] = err.Error()
+			if _, ok := err.(domainSpecificTestError); ok {
+				result[host] = fmt.Sprintf("WARNING: %s", result[host])
+			}
 
 			continue
 		}

internal/dnsforward/http_test.go

@@ -185,7 +185,8 @@ func TestDNSForwardHTTP_handleSetConfig(t *testing.T) {
 		wantSet: "",
 	}, {
 		name: "upstream_dns_bad",
-		wantSet: `validating upstream servers: bad ipport address "!!!": ` +
+		wantSet: `validating upstream servers: ` +
+			`validating upstream "!!!": bad ipport address "!!!": ` +
 			`address !!!: missing port in address`,
 	}, {
 		name: "bootstraps_bad",
@@ -256,112 +257,6 @@ func TestIsCommentOrEmpty(t *testing.T) {
 	}
 }
 
-func TestValidateUpstream(t *testing.T) {
-	testCases := []struct {
-		wantDef  assert.BoolAssertionFunc
-		name     string
-		upstream string
-		wantErr  string
-	}{{
-		wantDef:  assert.True,
-		name:     "invalid",
-		upstream: "1.2.3.4.5",
-		wantErr:  `bad ipport address "1.2.3.4.5": address 1.2.3.4.5: missing port in address`,
-	}, {
-		wantDef:  assert.True,
-		name:     "invalid",
-		upstream: "123.3.7m",
-		wantErr:  `bad ipport address "123.3.7m": address 123.3.7m: missing port in address`,
-	}, {
-		wantDef:  assert.True,
-		name:     "invalid",
-		upstream: "htttps://google.com/dns-query",
-		wantErr:  `wrong protocol`,
-	}, {
-		wantDef:  assert.True,
-		name:     "invalid",
-		upstream: "[/host.com]tls://dns.adguard.com",
-		wantErr:  `bad upstream for domain "[/host.com]tls://dns.adguard.com": missing separator`,
-	}, {
-		wantDef:  assert.True,
-		name:     "invalid",
-		upstream: "[host.ru]#",
-		wantErr:  `bad ipport address "[host.ru]#": address [host.ru]#: missing port in address`,
-	}, {
-		wantDef:  assert.True,
-		name:     "valid_default",
-		upstream: "1.1.1.1",
-		wantErr:  ``,
-	}, {
-		wantDef:  assert.True,
-		name:     "valid_default",
-		upstream: "tls://1.1.1.1",
-		wantErr:  ``,
-	}, {
-		wantDef:  assert.True,
-		name:     "valid_default",
-		upstream: "https://dns.adguard.com/dns-query",
-		wantErr:  ``,
-	}, {
-		wantDef:  assert.True,
-		name:     "valid_default",
-		upstream: "sdns://AQMAAAAAAAAAFDE3Ni4xMDMuMTMwLjEzMDo1NDQzINErR_JS3PLCu_iZEIbq95zkSV2LFsigxDIuUso_OQhzIjIuZG5zY3J5cHQuZGVmYXVsdC5uczEuYWRndWFyZC5jb20",
-		wantErr:  ``,
-	}, {
-		wantDef:  assert.True,
-		name:     "default_udp_host",
-		upstream: "udp://dns.google",
-	}, {
-		wantDef:  assert.True,
-		name:     "default_udp_ip",
-		upstream: "udp://8.8.8.8",
-	}, {
-		wantDef:  assert.False,
-		name:     "valid",
-		upstream: "[/host.com/]1.1.1.1",
-		wantErr:  ``,
-	}, {
-		wantDef:  assert.False,
-		name:     "valid",
-		upstream: "[//]tls://1.1.1.1",
-		wantErr:  ``,
-	}, {
-		wantDef:  assert.False,
-		name:     "valid",
-		upstream: "[/www.host.com/]#",
-		wantErr:  ``,
-	}, {
-		wantDef:  assert.False,
-		name:     "valid",
-		upstream: "[/host.com/google.com/]8.8.8.8",
-		wantErr:  ``,
-	}, {
-		wantDef:  assert.False,
-		name:     "valid",
-		upstream: "[/host/]sdns://AQMAAAAAAAAAFDE3Ni4xMDMuMTMwLjEzMDo1NDQzINErR_JS3PLCu_iZEIbq95zkSV2LFsigxDIuUso_OQhzIjIuZG5zY3J5cHQuZGVmYXVsdC5uczEuYWRndWFyZC5jb20",
-		wantErr:  ``,
-	}, {
-		wantDef:  assert.False,
-		name:     "idna",
-		upstream: "[/пример.рф/]8.8.8.8",
-		wantErr:  ``,
-	}, {
-		wantDef:  assert.False,
-		name:     "bad_domain",
-		upstream: "[/!/]8.8.8.8",
-		wantErr: `bad upstream for domain "[/!/]8.8.8.8": domain at index 0: ` +
-			`bad domain name "!": bad domain name label "!": bad domain name label rune '!'`,
-	}}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			defaultUpstream, err := validateUpstream(tc.upstream)
-			testutil.AssertErrorMsg(t, tc.wantErr, err)
-			tc.wantDef(t, defaultUpstream)
-		})
-	}
-}
-
 func TestValidateUpstreams(t *testing.T) {
 	testCases := []struct {
 		name    string
@@ -376,7 +271,7 @@ func TestValidateUpstreams(t *testing.T) {
 		wantErr: ``,
 		set:     []string{"# comment"},
 	}, {
-		name:    "valid_no_default",
+		name:    "no_default",
 		wantErr: `no default upstreams specified`,
 		set: []string{
 			"[/host.com/]1.1.1.1",
@@ -386,7 +281,7 @@ func TestValidateUpstreams(t *testing.T) {
 			"[/host/]sdns://AQMAAAAAAAAAFDE3Ni4xMDMuMTMwLjEzMDo1NDQzINErR_JS3PLCu_iZEIbq95zkSV2LFsigxDIuUso_OQhzIjIuZG5zY3J5cHQuZGVmYXVsdC5uczEuYWRndWFyZC5jb20",
 		},
 	}, {
-		name:    "valid_with_default",
+		name:    "with_default",
 		wantErr: ``,
 		set: []string{
 			"[/host.com/]1.1.1.1",
@@ -398,8 +293,46 @@ func TestValidateUpstreams(t *testing.T) {
 		},
 	}, {
 		name:    "invalid",
-		wantErr: `cannot prepare the upstream dhcp://fake.dns ([]): unsupported url scheme: dhcp`,
+		wantErr: `validating upstream "dhcp://fake.dns": wrong protocol`,
 		set:     []string{"dhcp://fake.dns"},
+	}, {
+		name:    "invalid",
+		wantErr: `validating upstream "1.2.3.4.5": bad ipport address "1.2.3.4.5": address 1.2.3.4.5: missing port in address`,
+		set:     []string{"1.2.3.4.5"},
+	}, {
+		name:    "invalid",
+		wantErr: `validating upstream "123.3.7m": bad ipport address "123.3.7m": address 123.3.7m: missing port in address`,
+		set:     []string{"123.3.7m"},
+	}, {
+		name:    "invalid",
+		wantErr: `bad upstream for domain "[/host.com]tls://dns.adguard.com": missing separator`,
+		set:     []string{"[/host.com]tls://dns.adguard.com"},
+	}, {
+		name:    "invalid",
+		wantErr: `validating upstream "[host.ru]#": bad ipport address "[host.ru]#": address [host.ru]#: missing port in address`,
+		set:     []string{"[host.ru]#"},
+	}, {
+		name:    "valid_default",
+		wantErr: ``,
+		set: []string{
+			"1.1.1.1",
+			"tls://1.1.1.1",
+			"https://dns.adguard.com/dns-query",
+			"sdns://AQMAAAAAAAAAFDE3Ni4xMDMuMTMwLjEzMDo1NDQzINErR_JS3PLCu_iZEIbq95zkSV2LFsigxDIuUso_OQhzIjIuZG5zY3J5cHQuZGVmYXVsdC5uczEuYWRndWFyZC5jb20",
+			"udp://dns.google",
+			"udp://8.8.8.8",
+			"[/host.com/]1.1.1.1",
+			"[//]tls://1.1.1.1",
+			"[/www.host.com/]#",
+			"[/host.com/google.com/]8.8.8.8",
+			"[/host/]sdns://AQMAAAAAAAAAFDE3Ni4xMDMuMTMwLjEzMDo1NDQzINErR_JS3PLCu_iZEIbq95zkSV2LFsigxDIuUso_OQhzIjIuZG5zY3J5cHQuZGVmYXVsdC5uczEuYWRndWFyZC5jb20",
+			"[/пример.рф/]8.8.8.8",
+		},
+	}, {
+		name: "bad_domain",
+		wantErr: `bad upstream for domain "[/!/]8.8.8.8": domain at index 0: ` +
+			`bad domain name "!": bad domain name label "!": bad domain name label rune '!'`,
+		set: []string{"[/!/]8.8.8.8"},
 	}}
 
 	for _, tc := range testCases {