Pull request 2052: 4977-multiple-domain-specific-upstreams

Updates #4977. Squashed commit of the following: commit da28c1b508b1aa4838d753fbb5fcac64a5fcebb9 Author: Stanislav Chzhen <s.chzhen@adguard.com> Date: Fri Oct 27 17:24:38 2023 +0300 all: fix typo commit d6bca6b252c9bd264737c93072869499afa24864 Author: Stanislav Chzhen <s.chzhen@adguard.com> Date: Fri Oct 27 14:44:20 2023 +0300 all: add todo commit 30875515942c58881305aa963220d57d31e0e67d Author: Stanislav Chzhen <s.chzhen@adguard.com> Date: Wed Oct 25 20:00:17 2023 +0300 all: imp docs commit 04003c342fcf82aeb671938fb89592fd6baff16d Author: Stanislav Chzhen <s.chzhen@adguard.com> Date: Wed Oct 25 16:59:14 2023 +0300 all: multiple domain specific upstreams
2023-10-27 20:18:29 +03:00 · 2023-10-27 20:18:29 +03:00 · 62ec0d5adc
parent 2a56c78f26
commit 62ec0d5adc
9 changed files with 222 additions and 105 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -23,6 +23,11 @@ See also the [v0.107.41 GitHub milestone][ms-v0.107.41].
 NOTE: Add new changes BELOW THIS COMMENT.
 -->

+### Added
+
+- Ability to specify multiple domain specific upstreams per line, e.g.
+  `[/domain1/../domain2/]upstream1 upstream2 .. upstreamN` ([#4977]).
+
 ### Fixed

 - `$important,dnsrewrite` rules do not take precedence over allowlist rules
@ -30,6 +35,7 @@ NOTE: Add new changes BELOW THIS COMMENT.
 - Dark mode DNS rewrite background ([#6329]).
 - Issues with QUIC and HTTP/3 upstreams on Linux ([#6335]).

+[#4977]: https://github.com/AdguardTeam/AdGuardHome/issues/4977
 [#6204]: https://github.com/AdguardTeam/AdGuardHome/issues/6204
 [#6329]: https://github.com/AdguardTeam/AdGuardHome/issues/6329
 [#6335]: https://github.com/AdguardTeam/AdGuardHome/issues/6335
--- a/client/src/__locales/en.json
+++ b/client/src/__locales/en.json
@ -1,6 +1,7 @@
 {
    "client_settings": "Client settings",
    "example_upstream_reserved": "an upstream <0>for specific domains</0>;",
+    "example_multiple_upstreams_reserved": "multiple upstreams <0>for specific domains</0>;",
    "example_upstream_comment": "a comment.",
    "upstream_parallel": "Use parallel queries to speed up resolving by querying all upstream servers simultaneously.",
    "parallel_requests": "Parallel requests",
--- a/client/src/components/Settings/Dns/Upstream/Examples.js
+++ b/client/src/components/Settings/Dns/Upstream/Examples.js
@ -137,6 +137,22 @@ const Examples = (props) => (
                    example_upstream_reserved
                </Trans>
            </li>
+            <li>
+                <code>[/example.local/]94.140.14.140 2a10:50c0::1:ff</code>: <Trans
+                    components={[
+                        <a
+                            href="https://github.com/AdguardTeam/AdGuardHome/wiki/Configuration#upstreams-for-domains"
+                            target="_blank"
+                            rel="noopener noreferrer"
+                            key="0"
+                        >
+                            Link
+                        </a>,
+                    ]}
+                >
+                    example_multiple_upstreams_reserved
+                </Trans>
+            </li>
            <li>
                <code>{COMMENT_LINE_DEFAULT_TOKEN} comment</code>: <Trans>
                    example_upstream_comment
--- a/go.mod
+++ b/go.mod
@ -3,7 +3,7 @@ module github.com/AdguardTeam/AdGuardHome
 go 1.20

 require (
-	github.com/AdguardTeam/dnsproxy v0.56.2
+	github.com/AdguardTeam/dnsproxy v0.56.3
 	github.com/AdguardTeam/golibs v0.17.2
 	github.com/AdguardTeam/urlfilter v0.17.3
 	github.com/NYTimes/gziphandler v1.1.1
--- a/go.sum
+++ b/go.sum
@ -1,5 +1,5 @@
-github.com/AdguardTeam/dnsproxy v0.56.2 h1:+k1iUmp05QIqkgXWyPn70fki4FouHe6vHIyHguelKao=
-github.com/AdguardTeam/dnsproxy v0.56.2/go.mod h1:ZvkbM71HwpilgkCnTubDiR4Ba6x5Qvnhy2iasMWaTDM=
+github.com/AdguardTeam/dnsproxy v0.56.3 h1:WP1FooLfZQPHEH2SuwMtJsOurDt32rubGx0OddcsKT0=
+github.com/AdguardTeam/dnsproxy v0.56.3/go.mod h1:ZvkbM71HwpilgkCnTubDiR4Ba6x5Qvnhy2iasMWaTDM=
 github.com/AdguardTeam/golibs v0.17.2 h1:vg6wHMjUKscnyPGRvxS5kAt7Uw4YxcJiITZliZ476W8=
 github.com/AdguardTeam/golibs v0.17.2/go.mod h1:DKhCIXHcUYtBhU8ibTLKh1paUL96n5zhQBlx763sj+U=
 github.com/AdguardTeam/urlfilter v0.17.3 h1:fg/ObbnO0Cv6aw0tW6N/ETDMhhNvmcUUOZ7HlmKC3rw=
--- a/internal/dnsforward/access.go
+++ b/internal/dnsforward/access.go
@ -182,6 +182,7 @@ func (s *Server) accessListJSON() (j accessListJSON) {
 	}
 }

+// handleAccessList handles requests to the GET /control/access/list endpoint.
 func (s *Server) handleAccessList(w http.ResponseWriter, r *http.Request) {
 	aghhttp.WriteJSONResponseOK(w, r, s.accessListJSON())
 }
@ -224,6 +225,7 @@ func validateStrUniq(clients []string) (uc aghalg.UniqChecker[string], err error
 	return uc, uc.Validate()
 }

+// handleAccessSet handles requests to the POST /control/access/set endpoint.
 func (s *Server) handleAccessSet(w http.ResponseWriter, r *http.Request) {
 	list := &accessListJSON{}
 	err := json.NewDecoder(r.Body).Decode(&list)
--- a/internal/dnsforward/http.go
+++ b/internal/dnsforward/http.go
@ -7,6 +7,7 @@ import (
 	"net/http"
 	"net/netip"
 	"strings"
+	"sync"
 	"time"

 	"github.com/AdguardTeam/AdGuardHome/internal/aghhttp"
@ -444,19 +445,10 @@ 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)
-		}
+	err = validateUpstreamConfig(upstreams)
+	if err != nil {
+		// Don't wrap the error since it's informative enough as is.
+		return nil, err
 	}

 	conf, err = proxy.ParseUpstreamsConfig(
@ -467,6 +459,7 @@ func newUpstreamConfig(upstreams []string) (conf *proxy.UpstreamConfig, err erro
 		},
 	)
 	if err != nil {
+		// Don't wrap the error since it's informative enough as is.
 		return nil, err
 	} else if len(conf.Upstreams) == 0 {
 		return nil, errors.Error("no default upstreams specified")
@ -475,6 +468,31 @@ func newUpstreamConfig(upstreams []string) (conf *proxy.UpstreamConfig, err erro
 	return conf, nil
 }

+// validateUpstreamConfig validates each upstream from the upstream
+// configuration and returns an error if any upstream is invalid.
+//
+// TODO(e.burkov):  Move into aghnet or even into dnsproxy.
+func validateUpstreamConfig(conf []string) (err error) {
+	for _, u := range conf {
+		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 err
+		}
+
+		for _, addr := range ups {
+			_, err = validateUpstream(addr, domains)
+			if err != nil {
+				return fmt.Errorf("validating upstream %q: %w", addr, err)
+			}
+		}
+	}
+
+	return nil
+}
+
 // ValidateUpstreams validates each upstream and returns an error if any
 // upstream is invalid or if there are no default upstreams specified.
 //
@ -567,12 +585,12 @@ func validateUpstream(u string, domains []string) (useDefault bool, err error) {
 	return false, err
 }

-// separateUpstream returns the upstream and the specified domains.  domains is
-// nil when the upstream is not domains-specific.  Otherwise it may also be
+// separateUpstream returns the upstreams and the specified domains.  domains
+// is nil when the upstream is not domains-specific.  Otherwise it may also be
 // empty.
-func separateUpstream(upstreamStr string) (ups string, domains []string, err error) {
+func separateUpstream(upstreamStr string) (upstreams, domains []string, err error) {
 	if !strings.HasPrefix(upstreamStr, "[/") {
-		return upstreamStr, nil, nil
+		return []string{upstreamStr}, nil, nil
 	}

 	defer func() { err = errors.Annotate(err, "bad upstream for domain %q: %w", upstreamStr) }()
@ -582,9 +600,9 @@ func separateUpstream(upstreamStr string) (ups string, domains []string, err err
 	case 2:
 		// Go on.
 	case 1:
-		return "", nil, errors.Error("missing separator")
+		return nil, nil, errors.Error("missing separator")
 	default:
-		return "", []string{}, errors.Error("duplicated separator")
+		return nil, nil, errors.Error("duplicated separator")
 	}

 	for i, host := range strings.Split(parts[0], "/") {
@ -594,13 +612,13 @@ func separateUpstream(upstreamStr string) (ups string, domains []string, err err

 		err = netutil.ValidateDomainName(strings.TrimPrefix(host, "*."))
 		if err != nil {
-			return "", domains, fmt.Errorf("domain at index %d: %w", i, err)
+			return nil, nil, fmt.Errorf("domain at index %d: %w", i, err)
 		}

 		domains = append(domains, host)
 	}

-	return parts[1], domains, nil
+	return strings.Fields(parts[1]), domains, nil
 }

 // healthCheckFunc is a signature of function to check if upstream exchanges
@ -683,30 +701,65 @@ func (err domainSpecificTestError) Error() (msg string) {
 	return fmt.Sprintf("WARNING: %s", err.error)
 }

-// parseUpstreamLine parses line and creates the [upstream.Upstream] using opts
-// and information from [s.dnsFilter.EtcHosts].  It returns an error if the line
-// is not a valid upstream line, see [upstream.AddressToUpstream].  It's a
-// caller's responsibility to close u.
-func (s *Server) parseUpstreamLine(
+// checkDNS parses line, creates DNS upstreams using opts, and checks if the
+// upstreams are exchanging correctly.  It returns a map where key is an
+// upstream address and value is "OK", if the upstream exchanges correctly, or
+// text of the error.
+func (s *Server) checkDNS(
 	line string,
 	opts *upstream.Options,
-) (u upstream.Upstream, specific bool, err error) {
-	// Separate upstream from domains list.
-	upstreamAddr, domains, err := separateUpstream(line)
+	check healthCheckFunc,
+) (result map[string]string) {
+	result = map[string]string{}
+	upstreams, domains, err := separateUpstream(line)
 	if err != nil {
-		return nil, false, fmt.Errorf("wrong upstream format: %w", err)
+		return nil
 	}

-	specific = len(domains) > 0
+	specific := len(domains) > 0

-	useDefault, err := validateUpstream(upstreamAddr, domains)
-	if err != nil {
-		return nil, specific, fmt.Errorf("wrong upstream format: %w", err)
-	} else if useDefault {
-		return nil, specific, nil
+	for _, upstreamAddr := range upstreams {
+		var useDefault bool
+		useDefault, err = validateUpstream(upstreamAddr, domains)
+		if err != nil {
+			err = fmt.Errorf("wrong upstream format: %w", err)
+			result[upstreamAddr] = err.Error()
+
+			continue
+		}
+
+		if useDefault {
+			continue
+		}
+
+		log.Debug("dnsforward: checking if upstream %q works", upstreamAddr)
+
+		err = s.checkUpstreamAddr(upstreamAddr, specific, opts, check)
+		if err != nil {
+			result[upstreamAddr] = err.Error()
+		} else {
+			result[upstreamAddr] = "OK"
+		}
 	}

-	log.Debug("dnsforward: checking if upstream %q works", upstreamAddr)
+	return result
+}
+
+// checkUpstreamAddr creates the DNS upstream using opts and information from
+// [s.dnsFilter.EtcHosts].  Checks if the DNS upstream exchanges correctly.  It
+// returns an error if addr is not valid DNS upstream address or the upstream
+// is not exchanging correctly.
+func (s *Server) checkUpstreamAddr(
+	addr string,
+	specific bool,
+	opts *upstream.Options,
+	check healthCheckFunc,
+) (err error) {
+	defer func() {
+		if err != nil && specific {
+			err = domainSpecificTestError{error: err}
+		}
+	}()

 	opts = &upstream.Options{
 		Bootstrap:  opts.Bootstrap,
@ -716,42 +769,25 @@ func (s *Server) parseUpstreamLine(

 	// dnsFilter can be nil during application update.
 	if s.dnsFilter != nil {
-		recs := s.dnsFilter.EtcHostsRecords(extractUpstreamHost(upstreamAddr))
+		recs := s.dnsFilter.EtcHostsRecords(extractUpstreamHost(addr))
 		for _, rec := range recs {
 			opts.ServerIPAddrs = append(opts.ServerIPAddrs, rec.Addr.AsSlice())
 		}
 		sortNetIPAddrs(opts.ServerIPAddrs, opts.PreferIPv6)
 	}
-	u, err = upstream.AddressToUpstream(upstreamAddr, opts)
+
+	u, err := upstream.AddressToUpstream(addr, opts)
 	if err != nil {
-		return nil, specific, fmt.Errorf("creating upstream for %q: %w", upstreamAddr, err)
+		return fmt.Errorf("creating upstream for %q: %w", addr, err)
 	}

-	return u, specific, nil
-}
-
-func (s *Server) checkDNS(line string, opts *upstream.Options, check healthCheckFunc) (err error) {
-	if IsCommentOrEmpty(line) {
-		return nil
-	}
-
-	var u upstream.Upstream
-	var specific bool
-	defer func() {
-		if err != nil && specific {
-			err = domainSpecificTestError{error: err}
-		}
-	}()
-
-	u, specific, err = s.parseUpstreamLine(line, opts)
-	if err != nil || u == nil {
-		return err
-	}
 	defer func() { err = errors.WithDeferred(err, u.Close()) }()

 	return check(u)
 }

+// handleTestUpstreamDNS handles requests to the POST /control/test_upstream_dns
+// endpoint.
 func (s *Server) handleTestUpstreamDNS(w http.ResponseWriter, r *http.Request) {
 	req := &upstreamJSON{}
 	err := json.NewDecoder(r.Body).Decode(req)
@ -761,6 +797,10 @@ func (s *Server) handleTestUpstreamDNS(w http.ResponseWriter, r *http.Request) {
 		return
 	}

+	req.Upstreams = stringutil.FilterOut(req.Upstreams, IsCommentOrEmpty)
+	req.FallbackDNS = stringutil.FilterOut(req.FallbackDNS, IsCommentOrEmpty)
+	req.PrivateUpstreams = stringutil.FilterOut(req.PrivateUpstreams, IsCommentOrEmpty)
+
 	opts := &upstream.Options{
 		Bootstrap:  req.BootstrapDNS,
 		Timeout:    s.conf.UpstreamTimeout,
@ -770,54 +810,44 @@ func (s *Server) handleTestUpstreamDNS(w http.ResponseWriter, r *http.Request) {
 		opts.Bootstrap = defaultBootstrap
 	}

-	type upsCheckResult = struct {
-		err  error
-		host string
+	wg := &sync.WaitGroup{}
+	m := &sync.Map{}
+
+	// TODO(s.chzhen):  Separate to a different structure/file.
+	worker := func(upstreamLine string, check healthCheckFunc) {
+		defer log.OnPanic("dnsforward: checking upstreams")
+
+		res := s.checkDNS(upstreamLine, opts, check)
+		for ups, status := range res {
+			m.Store(ups, status)
+		}
+
+		wg.Done()
 	}

-	req.Upstreams = stringutil.FilterOut(req.Upstreams, IsCommentOrEmpty)
-	req.FallbackDNS = stringutil.FilterOut(req.FallbackDNS, IsCommentOrEmpty)
-	req.PrivateUpstreams = stringutil.FilterOut(req.PrivateUpstreams, IsCommentOrEmpty)
-
-	upsNum := len(req.Upstreams) + len(req.FallbackDNS) + len(req.PrivateUpstreams)
-	result := make(map[string]string, upsNum)
-	resCh := make(chan upsCheckResult, upsNum)
+	wg.Add(len(req.Upstreams) + len(req.FallbackDNS) + len(req.PrivateUpstreams))

 	for _, ups := range req.Upstreams {
-		go func(ups string) {
-			resCh <- upsCheckResult{
-				host: ups,
-				err:  s.checkDNS(ups, opts, checkDNSUpstreamExc),
-			}
-		}(ups)
+		go worker(ups, checkDNSUpstreamExc)
 	}
 	for _, ups := range req.FallbackDNS {
-		go func(ups string) {
-			resCh <- upsCheckResult{
-				host: ups,
-				err:  s.checkDNS(ups, opts, checkDNSUpstreamExc),
-			}
-		}(ups)
+		go worker(ups, checkDNSUpstreamExc)
 	}
 	for _, ups := range req.PrivateUpstreams {
-		go func(ups string) {
-			resCh <- upsCheckResult{
-				host: ups,
-				err:  s.checkDNS(ups, opts, checkPrivateUpstreamExc),
-			}
-		}(ups)
+		go worker(ups, checkPrivateUpstreamExc)
 	}

-	for i := 0; i < upsNum; i++ {
-		// TODO(e.burkov):  The upstreams used for both common and private
-		// resolving should be reported separately.
-		pair := <-resCh
-		if pair.err != nil {
-			result[pair.host] = pair.err.Error()
-		} else {
-			result[pair.host] = "OK"
-		}
-	}
+	wg.Wait()
+
+	result := map[string]string{}
+	m.Range(func(k, v any) bool {
+		ups := k.(string)
+		status := v.(string)
+
+		result[ups] = status
+
+		return true
+	})

 	aghhttp.WriteJSONResponseOK(w, r, result)
 }
--- a/internal/dnsforward/http_test.go
+++ b/internal/dnsforward/http_test.go
@ -49,13 +49,18 @@ func loadTestData(t *testing.T, casesFileName string, cases any) {
 	require.NoError(t, err)
 }

-const jsonExt = ".json"
+const (
+	jsonExt = ".json"
+
+	// testBlockedRespTTL is the TTL for blocked responses to use in tests.
+	testBlockedRespTTL = 10
+)

 func TestDNSForwardHTTP_handleGetConfig(t *testing.T) {
 	filterConf := &filtering.Config{
 		ProtectionEnabled:     true,
 		BlockingMode:          filtering.BlockingModeDefault,
-		BlockedResponseTTL:    10,
+		BlockedResponseTTL:    testBlockedRespTTL,
 		SafeBrowsingEnabled:   true,
 		SafeBrowsingCacheSize: 1000,
 		SafeSearchConf:        filtering.SafeSearchConfig{Enabled: true},
@ -133,7 +138,7 @@ func TestDNSForwardHTTP_handleSetConfig(t *testing.T) {
 	filterConf := &filtering.Config{
 		ProtectionEnabled:     true,
 		BlockingMode:          filtering.BlockingModeDefault,
-		BlockedResponseTTL:    10,
+		BlockedResponseTTL:    testBlockedRespTTL,
 		SafeBrowsingEnabled:   true,
 		SafeBrowsingCacheSize: 1000,
 		SafeSearchConf:        filtering.SafeSearchConfig{Enabled: true},
@ -229,6 +234,9 @@ func TestDNSForwardHTTP_handleSetConfig(t *testing.T) {
 	}, {
 		name:    "blocked_response_ttl",
 		wantSet: "",
+	}, {
+		name:    "multiple_domain_specific_upstreams",
+		wantSet: "",
 	}}

 	var data map[string]struct {
@ -250,6 +258,7 @@ func TestDNSForwardHTTP_handleSetConfig(t *testing.T) {
 				s.dnsFilter.SetBlockingMode(filtering.BlockingModeDefault, netip.Addr{}, netip.Addr{})
 				s.conf = defaultConf
 				s.conf.Config.EDNSClientSubnet = &EDNSClientSubnet{}
+				s.dnsFilter.SetBlockedResponseTTL(testBlockedRespTTL)
 			})

 			rBody := io.NopCloser(bytes.NewReader(caseData.Req))
@ -547,7 +556,7 @@ func TestServer_HandleTestUpstreamDNS(t *testing.T) {
 			"upstream_dns": []string{"[/domain.example/]" + badUps},
 		},
 		wantResp: map[string]any{
-			"[/domain.example/]" + badUps: `WARNING: couldn't communicate ` +
+			badUps: `WARNING: couldn't communicate ` +
 				`with upstream: exchanging with ` + badUps + ` over tcp: ` +
 				`dns: id mismatch`,
 		},
@ -585,6 +594,17 @@ func TestServer_HandleTestUpstreamDNS(t *testing.T) {
 			goodUps: "OK",
 		},
 		name: "fallback_comment_mix",
+	}, {
+		body: map[string]any{
+			"upstream_dns": []string{"[/domain.example/]" + goodUps + " " + badUps},
+		},
+		wantResp: map[string]any{
+			goodUps: "OK",
+			badUps: `WARNING: couldn't communicate ` +
+				`with upstream: exchanging with ` + badUps + ` over tcp: ` +
+				`dns: id mismatch`,
+		},
+		name: "multiple_domain_specific_upstreams",
 	}}

 	for _, tc := range testCases {
--- a/internal/dnsforward/testdata/TestDNSForwardHTTP_handleSetConfig.json
+++ b/internal/dnsforward/testdata/TestDNSForwardHTTP_handleSetConfig.json
@ -839,5 +839,47 @@
      "edns_cs_use_custom": false,
      "edns_cs_custom_ip": ""
    }
+  },
+  "multiple_domain_specific_upstreams": {
+    "req": {
+      "upstream_dns": [
+        "8.8.8.8:77",
+        "[/example.com/]8.8.4.4:77 9.9.9.10 https://1.1.1.1"
+      ]
+    },
+    "want": {
+      "upstream_dns": [
+        "8.8.8.8:77",
+        "[/example.com/]8.8.4.4:77 9.9.9.10 https://1.1.1.1"
+      ],
+      "upstream_dns_file": "",
+      "bootstrap_dns": [
+        "9.9.9.10",
+        "149.112.112.10",
+        "2620:fe::10",
+        "2620:fe::fe:10"
+      ],
+      "fallback_dns": [],
+      "protection_enabled": true,
+      "protection_disabled_until": null,
+      "ratelimit": 0,
+      "blocking_mode": "default",
+      "blocking_ipv4": "",
+      "blocking_ipv6": "",
+      "blocked_response_ttl": 10,
+      "edns_cs_enabled": false,
+      "dnssec_enabled": false,
+      "disable_ipv6": false,
+      "upstream_mode": "",
+      "cache_size": 0,
+      "cache_ttl_min": 0,
+      "cache_ttl_max": 0,
+      "cache_optimistic": false,
+      "resolve_clients": false,
+      "use_private_ptr_resolvers": false,
+      "local_ptr_upstreams": [],
+      "edns_cs_use_custom": false,
+      "edns_cs_custom_ip": ""
+    }
  }
 }