all: imp script, code

.github/workflows/build.yml

@@ -1,7 +1,7 @@
 'name': 'build'
 
 'env':
-  'GO_VERSION': '1.20.10'
+  'GO_VERSION': '1.20.11'
   'NODE_VERSION': '16'
 
 'on':

.github/workflows/lint.yml

@@ -1,7 +1,7 @@
 'name': 'lint'
 
 'env':
-  'GO_VERSION': '1.20.10'
+  'GO_VERSION': '1.20.11'
 
 'on':
   'push':

bamboo-specs/release.yaml

@@ -7,7 +7,7 @@
 # Make sure to sync any changes with the branch overrides below.
 'variables':
     'channel': 'edge'
-    'dockerGo': 'adguard/golang-ubuntu:7.4'
+    'dockerGo': 'adguard/golang-ubuntu:7.5'
 
 'stages':
   - 'Build frontend':

bamboo-specs/snapcraft.yaml

@@ -10,7 +10,7 @@
 # Make sure to sync any changes with the branch overrides below.
 'variables':
     'channel': 'edge'
-    'dockerGo': 'adguard/golang-ubuntu:7.4'
+    'dockerGo': 'adguard/golang-ubuntu:7.5'
     'snapcraftChannel': 'edge'
 
 'stages':

bamboo-specs/test.yaml

@@ -5,7 +5,7 @@
     'key': 'AHBRTSPECS'
     'name': 'AdGuard Home - Build and run tests'
 'variables':
-    'dockerGo': 'adguard/golang-ubuntu:7.4'
+    'dockerGo': 'adguard/golang-ubuntu:7.5'
 
 'stages':
   - 'Tests':

internal/dnsforward/http.go

@@ -8,6 +8,7 @@ import (
 	"net/netip"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"time"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghhttp"
@@ -483,7 +484,7 @@ func validateUpstreamConfig(conf []string) (err error) {
 		}
 
 		for _, addr := range ups {
-			_, err = validateUpstream(addr, domains)
+			_, err = validateUpstream(addr, len(domains) > 0)
 			if err != nil {
 				return fmt.Errorf("validating upstream %q: %w", addr, err)
 			}
@@ -552,14 +553,14 @@ var protocols = []string{
 }
 
 // validateUpstream returns an error if u alongside with domains is not a valid
-// upstream configuration.  useDefault is true if the upstream is
+// upstream configuration.  usesDefault is true if the upstream is
 // domain-specific and is configured to point at the default upstream server
-// which is validated separately.  The upstream is considered domain-specific
-// only if domains is at least not nil.
-func validateUpstream(u string, domains []string) (useDefault bool, err error) {
+// which is validated separately.  specific reflects if the upstream is
+// domain-specific.
+func validateUpstream(u string, specific bool) (usesDefault bool, err error) {
 	// The special server address '#' means that default server must be used.
-	if useDefault = u == "#" && domains != nil; useDefault {
-		return useDefault, nil
+	if u == "#" && specific {
+		return true, nil
 	}
 
 	// Check if the upstream has a valid protocol prefix.
@@ -625,8 +626,9 @@ func separateUpstream(upstreamStr string) (upstreams, domains []string, err erro
 // properly.
 type healthCheckFunc func(u upstream.Upstream) (err error)
 
-// checkDNSUpstreamExc checks if the DNS upstream exchanges correctly.
-func checkDNSUpstreamExc(u upstream.Upstream) (err error) {
+// checkExchange is a [healthCheckFunc] that checks if the DNS upstream
+// exchanges correctly.
+func checkExchange(u upstream.Upstream) (err error) {
 	// testTLD is the special-use fully-qualified domain name for testing the
 	// DNS server reachability.
 	//
@@ -656,11 +658,11 @@ func checkDNSUpstreamExc(u upstream.Upstream) (err error) {
 	return nil
 }
 
-// checkPrivateUpstreamExc checks if the upstream for resolving private
-// addresses exchanges correctly.
+// checkPrivateExchange is a [healthCheckFunc] that 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) {
+func checkPrivateExchange(u upstream.Upstream) (err error) {
 	// inAddrArpaTLD is the special-use fully-qualified domain name for PTR IP
 	// address resolution.
 	//
@@ -701,73 +703,42 @@ func (err domainSpecificTestError) Error() (msg string) {
 	return fmt.Sprintf("WARNING: %s", err.error)
 }
 
-// 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,
-	check healthCheckFunc,
-) (result map[string]string) {
-	result = map[string]string{}
-	upstreams, domains, err := separateUpstream(line)
-	if err != nil {
-		return nil
-	}
-
-	specific := len(domains) > 0
-
-	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"
-		}
-	}
-
-	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
+// checkUpstreamAddr creates the upstream using opts and, possibly, information
+// from system hosts files, then 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.
+//
+// TODO(e.burkov):  Remove the receiver.
 func (s *Server) checkUpstreamAddr(
 	addr string,
 	specific bool,
-	opts *upstream.Options,
+	basicOpts *upstream.Options,
 	check healthCheckFunc,
 ) (err error) {
+	usesDefault, err := validateUpstream(addr, specific)
+	if err != nil {
+		return fmt.Errorf("wrong upstream format: %w", err)
+	} else if usesDefault {
+		return nil
+	}
+
+	log.Debug("dnsforward: checking if upstream %q works", addr)
+
 	defer func() {
 		if err != nil && specific {
 			err = domainSpecificTestError{error: err}
 		}
 	}()
 
-	opts = &upstream.Options{
-		Bootstrap:  opts.Bootstrap,
-		Timeout:    opts.Timeout,
-		PreferIPv6: opts.PreferIPv6,
+	opts := &upstream.Options{
+		Bootstrap:  basicOpts.Bootstrap,
+		Timeout:    basicOpts.Timeout,
+		PreferIPv6: basicOpts.PreferIPv6,
 	}
 
 	// dnsFilter can be nil during application update.
+	//
+	// TODO(e.burkov):  Remove when update dnsproxy.
 	if s.dnsFilter != nil {
 		recs := s.dnsFilter.EtcHostsRecords(extractUpstreamHost(addr))
 		for _, rec := range recs {
@@ -780,12 +751,103 @@ func (s *Server) checkUpstreamAddr(
 	if err != nil {
 		return fmt.Errorf("creating upstream for %q: %w", addr, err)
 	}
-
 	defer func() { err = errors.WithDeferred(err, u.Close()) }()
 
 	return check(u)
 }
 
+// checkResult is a result of checking an upstream server.
+type checkResult = struct {
+	// status is an error message if the upstream server is not working.  It's
+	// nil for working upstreams.
+	status error
+
+	// address is the upstream server address as given in the request.  It may
+	// appear to be a whole line if it's incorrect itself.
+	address string
+}
+
+// checkDNS parses an upstream configuration line using opts and checks if the
+// specified upstreams are working using check.  countWG is decremented when the
+// expected number of results added to resNum, then results are sent to resCh.
+//
+// TODO(e.burkov):  Remove the receiver.
+func (s *Server) checkDNS(
+	line string,
+	opts *upstream.Options,
+	check healthCheckFunc,
+	countWG *sync.WaitGroup,
+	resNum *atomic.Int32,
+	resCh chan<- checkResult,
+) {
+	defer log.OnPanic("dnsforward: checking upstreams")
+
+	addrs, domains, err := separateUpstream(line)
+	if err != nil {
+		resNum.Add(1)
+		countWG.Done()
+
+		resCh <- checkResult{
+			address: line,
+			status:  fmt.Errorf("wrong upstream format: %w", err),
+		}
+
+		return
+	}
+
+	resNum.Add(int32(len(addrs)))
+	countWG.Done()
+
+	specific := len(domains) > 0
+	for _, addr := range addrs {
+		resCh <- checkResult{
+			address: addr,
+			status:  s.checkUpstreamAddr(addr, specific, opts, check),
+		}
+	}
+}
+
+// check returns the mapping of upstream addresses to their check results.
+func (s *Server) check(req *upstreamJSON, opts *upstream.Options) (result map[string]string) {
+	req.Upstreams = stringutil.FilterOut(req.Upstreams, IsCommentOrEmpty)
+	req.FallbackDNS = stringutil.FilterOut(req.FallbackDNS, IsCommentOrEmpty)
+	req.PrivateUpstreams = stringutil.FilterOut(req.PrivateUpstreams, IsCommentOrEmpty)
+
+	countWG := &sync.WaitGroup{}
+	countWG.Add(len(req.Upstreams) + len(req.FallbackDNS) + len(req.PrivateUpstreams))
+
+	resNum := &atomic.Int32{}
+	resCh := make(chan checkResult)
+
+	for _, addr := range req.Upstreams {
+		go s.checkDNS(addr, opts, checkExchange, countWG, resNum, resCh)
+	}
+	for _, addr := range req.FallbackDNS {
+		go s.checkDNS(addr, opts, checkExchange, countWG, resNum, resCh)
+	}
+	for _, addr := range req.PrivateUpstreams {
+		go s.checkDNS(addr, opts, checkPrivateExchange, countWG, resNum, resCh)
+	}
+
+	// Wait until all the servers are counted and enqueued.
+	countWG.Wait()
+	n := resNum.Load()
+
+	result = make(map[string]string, n)
+	for i := int32(0); i < n; i++ {
+		// TODO(e.burkov):  Upstreams intended for different purposes should
+		// be distinguished in the result, even if specified equally.
+		res := <-resCh
+		if res.status != nil {
+			result[res.address] = res.status.Error()
+		} else {
+			result[res.address] = "OK"
+		}
+	}
+
+	return result
+}
+
 // handleTestUpstreamDNS handles requests to the POST /control/test_upstream_dns
 // endpoint.
 func (s *Server) handleTestUpstreamDNS(w http.ResponseWriter, r *http.Request) {
@@ -797,59 +859,18 @@ 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)
+	bootstrapAddrs := stringutil.FilterOut(req.BootstrapDNS, IsCommentOrEmpty)
+	if len(bootstrapAddrs) == 0 {
+		bootstrapAddrs = defaultBootstrap
+	}
 
 	opts := &upstream.Options{
-		Bootstrap:  req.BootstrapDNS,
+		Bootstrap:  bootstrapAddrs,
 		Timeout:    s.conf.UpstreamTimeout,
 		PreferIPv6: s.conf.BootstrapPreferIPv6,
 	}
-	if len(opts.Bootstrap) == 0 {
-		opts.Bootstrap = defaultBootstrap
-	}
 
-	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()
-	}
-
-	wg.Add(len(req.Upstreams) + len(req.FallbackDNS) + len(req.PrivateUpstreams))
-
-	for _, ups := range req.Upstreams {
-		go worker(ups, checkDNSUpstreamExc)
-	}
-	for _, ups := range req.FallbackDNS {
-		go worker(ups, checkDNSUpstreamExc)
-	}
-	for _, ups := range req.PrivateUpstreams {
-		go worker(ups, checkPrivateUpstreamExc)
-	}
-
-	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)
+	aghhttp.WriteJSONResponseOK(w, r, s.check(req, opts))
 }
 
 // handleCacheClear is the handler for the POST /control/cache_clear HTTP API.

internal/dnsforward/http_test.go

@@ -479,6 +479,8 @@ func TestServer_HandleTestUpstreamDNS(t *testing.T) {
 		Host:   newLocalUpstreamListener(t, 0, badHandler).String(),
 	}).String()
 
+	goodAndBadUps := strings.Join([]string{goodUps, badUps}, " ")
+
 	const (
 		upsTimeout = 100 * time.Millisecond
 
@@ -605,6 +607,29 @@ func TestServer_HandleTestUpstreamDNS(t *testing.T) {
 				`dns: id mismatch`,
 		},
 		name: "multiple_domain_specific_upstreams",
+	}, {
+		body: map[string]any{
+			"upstream_dns": []string{"[/domain.example/]/]1.2.3.4"},
+		},
+		wantResp: map[string]any{
+			"[/domain.example/]/]1.2.3.4": `wrong upstream format: ` +
+				`bad upstream for domain "[/domain.example/]/]1.2.3.4": ` +
+				`duplicated separator`,
+		},
+		name: "bad_specification",
+	}, {
+		body: map[string]any{
+			"upstream_dns":     []string{"[/domain.example/]" + goodAndBadUps},
+			"fallback_dns":     []string{"[/domain.example/]" + goodAndBadUps},
+			"private_upstream": []string{"[/domain.example/]" + goodAndBadUps},
+		},
+		wantResp: map[string]any{
+			goodUps: "OK",
+			badUps: `WARNING: couldn't communicate ` +
+				`with upstream: exchanging with ` + badUps + ` over tcp: ` +
+				`dns: id mismatch`,
+		},
+		name: "all_different",
 	}}
 
 	for _, tc := range testCases {

scripts/make/go-lint.sh

@@ -35,7 +35,7 @@ set -f -u
 go_version="$( "${GO:-go}" version )"
 readonly go_version
 
-go_min_version='go1.20.10'
+go_min_version='go1.20.11'
 go_version_msg="
 warning: your go version (${go_version}) is different from the recommended minimal one (${go_min_version}).
 if you have the version installed, please set the GO environment variable.