Pull request: 5193-long-ups-check

Merge in DNS/adguard-home from 5193-long-ups-check to master

Closes #5193.

Squashed commit of the following:

commit 787e6b9950e85b79fe50e16cda5110fe53ec3e80
Merge: f62330bd 01652e6a
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Dec 5 17:07:40 2022 +0300

    Merge branch 'master' into 5193-long-ups-check

commit f62330bd17ec260bc8c7475c8f5ae236059cde35
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Dec 5 16:28:47 2022 +0300

    dnsforward: try to fix linux

commit 64bfacc58d2a4c2929d9c3cf80bc31bfca404d54
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Dec 5 15:55:48 2022 +0300

    all: log changes finally

commit 4331d1c2497a94a95e4eba0ebcb5a813260c188a
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Dec 5 15:26:45 2022 +0300

    all: imp log of chagnes

commit 62ed3c123eda100813a2de2ed95c1446c7a100f0
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Dec 5 15:23:17 2022 +0300

    all: log changes

commit 73f4d59796dc5de51cf9c9953a6a22342e1ce31a
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Dec 5 14:37:18 2022 +0300

    dnsforward: add defer

commit a15072f1ea3845ba135ddd61aa8d67d9c0dcd7ea
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Dec 5 14:30:16 2022 +0300

    dnsforward: imp tests

commit e74219f594094f1e3d0001664ed3f79050747a4d
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Sat Dec 3 16:29:31 2022 +0300

    dnsforward: get rid of wg

commit 165da7dc186285d6ff8b949e12d95e0da5e828eb
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Dec 2 15:42:55 2022 +0300

    dnsforward: add ups check test

commit 3045273997e45e952ba58e9c7fa5bba2d21ad286
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Dec 1 20:28:56 2022 +0300

    dnsforward: imp ups check perf

CHANGELOG.md

@@ -36,6 +36,7 @@ See also the [v0.107.20 GitHub milestone][ms-v0.107.20].
 
 ### Fixed
 
+- Slow upstream checks making the API unresponsive ([#5193]).
 - The TLS initialization errors preventing AdGuard Home from starting ([#5189]).
   Instead, AdGuard Home disables encryption and shows an error message on the
   encryption settings page in the UI, which was the intended previous behavior.
@@ -44,6 +45,7 @@ See also the [v0.107.20 GitHub milestone][ms-v0.107.20].
 [#4944]: https://github.com/AdguardTeam/AdGuardHome/issues/4944
 [#5189]: https://github.com/AdguardTeam/AdGuardHome/issues/5189
 [#5190]: https://github.com/AdguardTeam/AdGuardHome/issues/5190
+[#5193]: https://github.com/AdguardTeam/AdGuardHome/issues/5193
 
 
 

internal/dnsforward/http.go

@@ -566,6 +566,11 @@ type domainSpecificTestError struct {
 	error
 }
 
+// Error implements the [error] interface for domainSpecificTestError.
+func (err domainSpecificTestError) Error() (msg string) {
+	return fmt.Sprintf("WARNING: %s", err.error)
+}
+
 // checkDNS checks the upstream server defined by upstreamConfigStr using
 // healthCheck for actually exchange messages.  It uses bootstrap to resolve the
 // upstream's address.
@@ -632,41 +637,45 @@ func (s *Server) handleTestUpstreamDNS(w http.ResponseWriter, r *http.Request) {
 
 	result := map[string]string{}
 	bootstraps := req.BootstrapDNS
-
 	timeout := s.conf.UpstreamTimeout
-	for _, host := range req.Upstreams {
-		err = checkDNS(host, bootstraps, timeout, checkDNSUpstreamExc)
-		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
+	type upsCheckResult = struct {
-		}
+		res  string
-
+		host string
-		result[host] = "OK"
 	}
 
-	for _, host := range req.PrivateUpstreams {
+	upsNum := len(req.Upstreams) + len(req.PrivateUpstreams)
-		err = checkDNS(host, bootstraps, timeout, checkPrivateUpstreamExc)
+	resCh := make(chan upsCheckResult, upsNum)
-		if err != nil {
-			log.Info("%v", err)
-			// TODO(e.burkov): If passed upstream have already written an error
-			// 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
+	checkUps := func(ups string, healthCheck healthCheckFunc) {
+		res := upsCheckResult{
+			host: ups,
 		}
+		defer func() { resCh <- res }()
 
-		result[host] = "OK"
+		checkErr := checkDNS(ups, bootstraps, timeout, healthCheck)
+		if checkErr != nil {
+			res.res = checkErr.Error()
+		} else {
+			res.res = "OK"
+		}
 	}
 
+	for _, ups := range req.Upstreams {
+		go checkUps(ups, checkDNSUpstreamExc)
+	}
+	for _, ups := range req.PrivateUpstreams {
+		go checkUps(ups, checkPrivateUpstreamExc)
+	}
+
+	for i := 0; i < upsNum; i++ {
+		pair := <-resCh
+		// TODO(e.burkov):  The upstreams used for both common and private
+		// resolving should be reported separately.
+		result[pair.host] = pair.res
+	}
+	close(resCh)
+
 	_ = aghhttp.WriteJSONResponse(w, r, result)
 }
 

internal/dnsforward/http_test.go

@@ -7,16 +7,20 @@ import (
 	"net"
 	"net/http"
 	"net/http/httptest"
+	"net/netip"
+	"net/url"
 	"os"
 	"path/filepath"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghhttp"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
 	"github.com/AdguardTeam/AdGuardHome/internal/filtering"
 	"github.com/AdguardTeam/golibs/netutil"
 	"github.com/AdguardTeam/golibs/testutil"
+	"github.com/miekg/dns"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -392,3 +396,141 @@ func TestValidateUpstreamsPrivate(t *testing.T) {
 		})
 	}
 }
+
+func newLocalUpstreamListener(t *testing.T, port int, handler dns.Handler) (real net.Addr) {
+	startCh := make(chan struct{})
+	upsSrv := &dns.Server{
+		Addr:              netip.AddrPortFrom(netutil.IPv4Localhost(), uint16(port)).String(),
+		Net:               "tcp",
+		Handler:           handler,
+		NotifyStartedFunc: func() { close(startCh) },
+	}
+	go func() {
+		t := testutil.PanicT{}
+
+		err := upsSrv.ListenAndServe()
+		require.NoError(t, err)
+	}()
+	<-startCh
+	testutil.CleanupAndRequireSuccess(t, upsSrv.Shutdown)
+
+	return upsSrv.Listener.Addr()
+}
+
+func TestServer_handleTestUpstreaDNS(t *testing.T) {
+	goodHandler := dns.HandlerFunc(func(w dns.ResponseWriter, m *dns.Msg) {
+		err := w.WriteMsg(new(dns.Msg).SetReply(m))
+		require.NoError(testutil.PanicT{}, err)
+	})
+	badHandler := dns.HandlerFunc(func(w dns.ResponseWriter, _ *dns.Msg) {
+		err := w.WriteMsg(new(dns.Msg))
+		require.NoError(testutil.PanicT{}, err)
+	})
+
+	goodUps := (&url.URL{
+		Scheme: "tcp",
+		Host:   newLocalUpstreamListener(t, 0, goodHandler).String(),
+	}).String()
+	badUps := (&url.URL{
+		Scheme: "tcp",
+		Host:   newLocalUpstreamListener(t, 0, badHandler).String(),
+	}).String()
+
+	const upsTimeout = 100 * time.Millisecond
+
+	srv := createTestServer(t, &filtering.Config{}, ServerConfig{
+		UDPListenAddrs:  []*net.UDPAddr{{}},
+		TCPListenAddrs:  []*net.TCPAddr{{}},
+		UpstreamTimeout: upsTimeout,
+	}, nil)
+	startDeferStop(t, srv)
+
+	testCases := []struct {
+		body     map[string]any
+		wantResp map[string]any
+		name     string
+	}{{
+		body: map[string]any{
+			"upstream_dns": []string{goodUps},
+		},
+		wantResp: map[string]any{
+			goodUps: "OK",
+		},
+		name: "success",
+	}, {
+		body: map[string]any{
+			"upstream_dns": []string{badUps},
+		},
+		wantResp: map[string]any{
+			badUps: `upstream "` + badUps + `" fails to exchange: ` +
+				`couldn't communicate with upstream: dns: id mismatch`,
+		},
+		name: "broken",
+	}, {
+		body: map[string]any{
+			"upstream_dns": []string{goodUps, badUps},
+		},
+		wantResp: map[string]any{
+			goodUps: "OK",
+			badUps: `upstream "` + badUps + `" fails to exchange: ` +
+				`couldn't communicate with upstream: dns: id mismatch`,
+		},
+		name: "both",
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			reqBody, err := json.Marshal(tc.body)
+			require.NoError(t, err)
+
+			w := httptest.NewRecorder()
+			r, err := http.NewRequest(http.MethodPost, "", bytes.NewReader(reqBody))
+			require.NoError(t, err)
+
+			srv.handleTestUpstreamDNS(w, r)
+			require.Equal(t, http.StatusOK, w.Code)
+
+			resp := map[string]any{}
+			err = json.NewDecoder(w.Body).Decode(&resp)
+			require.NoError(t, err)
+
+			assert.Equal(t, tc.wantResp, resp)
+		})
+	}
+
+	t.Run("timeout", func(t *testing.T) {
+		slowHandler := dns.HandlerFunc(func(w dns.ResponseWriter, m *dns.Msg) {
+			time.Sleep(upsTimeout * 2)
+			writeErr := w.WriteMsg(new(dns.Msg).SetReply(m))
+			require.NoError(testutil.PanicT{}, writeErr)
+		})
+		sleepyUps := (&url.URL{
+			Scheme: "tcp",
+			Host:   newLocalUpstreamListener(t, 0, slowHandler).String(),
+		}).String()
+
+		req := map[string]any{
+			"upstream_dns": []string{sleepyUps},
+		}
+		reqBody, err := json.Marshal(req)
+		require.NoError(t, err)
+
+		w := httptest.NewRecorder()
+		r, err := http.NewRequest(http.MethodPost, "", bytes.NewReader(reqBody))
+		require.NoError(t, err)
+
+		srv.handleTestUpstreamDNS(w, r)
+		require.Equal(t, http.StatusOK, w.Code)
+
+		resp := map[string]any{}
+		err = json.NewDecoder(w.Body).Decode(&resp)
+		require.NoError(t, err)
+
+		require.Contains(t, resp, sleepyUps)
+		require.IsType(t, "", resp[sleepyUps])
+		sleepyRes, _ := resp[sleepyUps].(string)
+
+		// TODO(e.burkov):  Improve the format of an error in dnsproxy.
+		assert.True(t, strings.HasSuffix(sleepyRes, "i/o timeout"))
+	})
+}