From 6d1adf74b1fbb5f0a941d0b7b68c4469d189adf6 Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Mon, 5 Dec 2022 17:24:32 +0300 Subject: [PATCH] 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 Date: Mon Dec 5 17:07:40 2022 +0300 Merge branch 'master' into 5193-long-ups-check commit f62330bd17ec260bc8c7475c8f5ae236059cde35 Author: Eugene Burkov Date: Mon Dec 5 16:28:47 2022 +0300 dnsforward: try to fix linux commit 64bfacc58d2a4c2929d9c3cf80bc31bfca404d54 Author: Eugene Burkov Date: Mon Dec 5 15:55:48 2022 +0300 all: log changes finally commit 4331d1c2497a94a95e4eba0ebcb5a813260c188a Author: Eugene Burkov Date: Mon Dec 5 15:26:45 2022 +0300 all: imp log of chagnes commit 62ed3c123eda100813a2de2ed95c1446c7a100f0 Author: Eugene Burkov Date: Mon Dec 5 15:23:17 2022 +0300 all: log changes commit 73f4d59796dc5de51cf9c9953a6a22342e1ce31a Author: Eugene Burkov Date: Mon Dec 5 14:37:18 2022 +0300 dnsforward: add defer commit a15072f1ea3845ba135ddd61aa8d67d9c0dcd7ea Author: Eugene Burkov Date: Mon Dec 5 14:30:16 2022 +0300 dnsforward: imp tests commit e74219f594094f1e3d0001664ed3f79050747a4d Author: Eugene Burkov Date: Sat Dec 3 16:29:31 2022 +0300 dnsforward: get rid of wg commit 165da7dc186285d6ff8b949e12d95e0da5e828eb Author: Eugene Burkov Date: Fri Dec 2 15:42:55 2022 +0300 dnsforward: add ups check test commit 3045273997e45e952ba58e9c7fa5bba2d21ad286 Author: Eugene Burkov Date: Thu Dec 1 20:28:56 2022 +0300 dnsforward: imp ups check perf --- CHANGELOG.md | 2 + internal/dnsforward/http.go | 61 +++++++------ internal/dnsforward/http_test.go | 142 +++++++++++++++++++++++++++++++ 3 files changed, 179 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 95bfb899..abd261d8 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/internal/dnsforward/http.go b/internal/dnsforward/http.go index 1baf14b2..5668573b 100644 --- a/internal/dnsforward/http.go +++ b/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 - } - - result[host] = "OK" + type upsCheckResult = struct { + res string + host string } - for _, host := range req.PrivateUpstreams { - err = checkDNS(host, bootstraps, timeout, checkPrivateUpstreamExc) - 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]) - } + upsNum := len(req.Upstreams) + len(req.PrivateUpstreams) + resCh := make(chan upsCheckResult, upsNum) - 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) } diff --git a/internal/dnsforward/http_test.go b/internal/dnsforward/http_test.go index 64ba21c4..5e0b8018 100644 --- a/internal/dnsforward/http_test.go +++ b/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")) + }) +}