Pull request: 2280 dns timeout

Updates #2280. Squashed commit of the following: commit d8c6aac Merge: 84df492 12f1e4e Author: Eugene Burkov <e.burkov@adguard.com> Date: Tue Jun 15 17:21:41 2021 +0300 Merge branch 'master' into 2280-dns-timeout commit 84df492 Author: Eugene Burkov <e.burkov@adguard.com> Date: Tue Jun 15 16:49:41 2021 +0300 home: fix docs & naming commit af44a86 Author: Eugene Burkov <e.burkov@adguard.com> Date: Tue Jun 15 15:55:12 2021 +0300 all: imp docs & tests commit 6ed6599 Author: Eugene Burkov <e.burkov@adguard.com> Date: Tue Jun 15 15:26:22 2021 +0300 home: imp duration tests commit 8fe7cb0 Author: Eugene Burkov <e.burkov@adguard.com> Date: Tue Jun 15 15:04:16 2021 +0300 all: imp code, docs & tests commit a989e8a Author: Eugene Burkov <e.burkov@adguard.com> Date: Sat Jun 12 19:02:23 2021 +0300 WIP commit b0362e2 Author: Eugene Burkov <e.burkov@adguard.com> Date: Sat Jun 12 18:58:09 2021 +0300 all: imp docs & tests commit 64b00fd Author: Eugene Burkov <e.burkov@adguard.com> Date: Sat Jun 12 03:44:29 2021 +0300 home: introduce marshalable duration commit bfb1a57 Author: Eugene Burkov <e.burkov@adguard.com> Date: Sat Jun 12 01:56:10 2021 +0300 all: add upstream timeout setting
AdguardTeam · Jun 15, 2021 · 4fd7fad · 4fd7fad
1 parent 12f1e4e
commit 4fd7fad
Show file tree

Hide file tree

Showing 12 changed files with 293 additions and 11 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,7 @@ and this project adheres to
 
 ### Added
 
+- The ability to set the timeout for querying the upstream servers ([#2280]).
 - The ability to change group and user ID on startup on Unix ([#2763]).
 - Experimental OpenBSD support for AMD64 and 64-bit ARM CPUs ([#2439]).
 - Support for custom port in DNS-over-HTTPS profiles for Apple's devices
@@ -60,6 +61,7 @@ released by then.
 
 - Go 1.15 support.
 
+[#2280]: https://github.com/AdguardTeam/AdGuardHome/issues/2280
 [#2439]: https://github.com/AdguardTeam/AdGuardHome/issues/2439
 [#2441]: https://github.com/AdguardTeam/AdGuardHome/issues/2441
 [#2443]: https://github.com/AdguardTeam/AdGuardHome/issues/2443

diff --git a/HACKING.md b/HACKING.md
@@ -197,13 +197,24 @@ attributes to make it work in Markdown renderers that strip "id". -->
 
  ###  <a href="#formatting" id="formatting" name="formatting">Formatting</a>
 
- *  Decorate `break`, `continue`, `fallthrough`, `return`, and other function
-    exit points with empty lines unless it's the only statement in that block.
+ *  Decorate `break`, `continue`, `fallthrough`, `return`, and other terminating
+    statements with empty lines unless it's the only statement in that block.
 
  *  Don't group type declarations together.  Unlike with blocks of `const`s,
     where a `iota` may be used or where all constants belong to a certain type,
     there is no reason to group `type`s.
 
+ *  Group `require.*` blocks together with the presceding related statements,
+    but separate from the following `assert.*` and unrelated requirements.
+
+    ```go
+    val, ok := valMap[key]
+    require.True(t, ok)
+    require.NotNil(t, val)
+
+    assert.Equal(t, expected, val)
+    ```
+
  *  Use `gofumpt --extra -s`.
 
  *  Write slices of struct like this:

diff --git a/internal/dnsforward/config.go b/internal/dnsforward/config.go
@@ -9,6 +9,7 @@ import (
 	"os"
 	"sort"
 	"strings"
+	"time"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghstrings"
@@ -142,6 +143,9 @@ type ServerConfig struct {
 	DNSCryptConfig
 	TLSAllowUnencryptedDOH bool
 
+	// UpstreamTimeout is the timeout for querying upstream servers.
+	UpstreamTimeout time.Duration
+
 	TLSv12Roots *x509.CertPool // list of root CAs for TLSv1.2
 	TLSCiphers  []uint16       // list of TLS ciphers to use
 
@@ -261,6 +265,10 @@ func (s *Server) initDefaultSettings() {
 	if len(s.conf.BlockedHosts) == 0 {
 		s.conf.BlockedHosts = defaultBlockedHosts
 	}
+
+	if s.conf.UpstreamTimeout == 0 {
+		s.conf.UpstreamTimeout = DefaultTimeout
+	}
 }
 
 // prepareUpstreamSettings - prepares upstream DNS server settings
@@ -299,7 +307,7 @@ func (s *Server) prepareUpstreamSettings() error {
 		upstreams,
 		upstream.Options{
 			Bootstrap: s.conf.BootstrapDNS,
-			Timeout:   DefaultTimeout,
+			Timeout:   s.conf.UpstreamTimeout,
 		},
 	)
 	if err != nil {
@@ -313,7 +321,7 @@ func (s *Server) prepareUpstreamSettings() error {
 			defaultDNS,
 			upstream.Options{
 				Bootstrap: s.conf.BootstrapDNS,
-				Timeout:   DefaultTimeout,
+				Timeout:   s.conf.UpstreamTimeout,
 			},
 		)
 		if err != nil {

diff --git a/internal/dnsforward/dnsforward_test.go b/internal/dnsforward/dnsforward_test.go
@@ -279,6 +279,34 @@ func TestServer(t *testing.T) {
 	}
 }
 
+func TestServer_timeout(t *testing.T) {
+	const timeout time.Duration = time.Second
+
+	t.Run("custom", func(t *testing.T) {
+		srvConf := &ServerConfig{
+			UpstreamTimeout: timeout,
+		}
+
+		s, err := NewServer(DNSCreateParams{})
+		require.NoError(t, err)
+
+		err = s.Prepare(srvConf)
+		require.NoError(t, err)
+
+		assert.Equal(t, timeout, s.conf.UpstreamTimeout)
+	})
+
+	t.Run("default", func(t *testing.T) {
+		s, err := NewServer(DNSCreateParams{})
+		require.NoError(t, err)
+
+		err = s.Prepare(nil)
+		require.NoError(t, err)
+
+		assert.Equal(t, DefaultTimeout, s.conf.UpstreamTimeout)
+	})
+}
+
 func TestServerWithProtectionDisabled(t *testing.T) {
 	s := createTestServer(t, &filtering.Config{}, ServerConfig{
 		UDPListenAddrs: []*net.UDPAddr{{}},

diff --git a/internal/dnsforward/http.go b/internal/dnsforward/http.go
@@ -7,6 +7,7 @@ import (
 	"net/http"
 	"strconv"
 	"strings"
+	"time"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghstrings"
@@ -529,7 +530,7 @@ func checkPrivateUpstreamExc(u upstream.Upstream) (err error) {
 	return nil
 }
 
-func checkDNS(input string, bootstrap []string, ef excFunc) (err error) {
+func checkDNS(input string, bootstrap []string, timeout time.Duration, ef excFunc) (err error) {
 	if aghstrings.IsCommentOrEmpty(input) {
 		return nil
 	}
@@ -557,7 +558,7 @@ func checkDNS(input string, bootstrap []string, ef excFunc) (err error) {
 	var u upstream.Upstream
 	u, err = upstream.AddressToUpstream(input, upstream.Options{
 		Bootstrap: bootstrap,
-		Timeout:   DefaultTimeout,
+		Timeout:   timeout,
 	})
 	if err != nil {
 		return fmt.Errorf("failed to choose upstream for %q: %w", input, err)
@@ -584,8 +585,9 @@ 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, checkDNSUpstreamExc)
+		err = checkDNS(host, bootstraps, timeout, checkDNSUpstreamExc)
 		if err != nil {
 			log.Info("%v", err)
 			result[host] = err.Error()
@@ -597,7 +599,7 @@ func (s *Server) handleTestUpstreamDNS(w http.ResponseWriter, r *http.Request) {
 	}
 
 	for _, host := range req.PrivateUpstreams {
-		err = checkDNS(host, bootstraps, checkPrivateUpstreamExc)
+		err = checkDNS(host, bootstraps, timeout, checkPrivateUpstreamExc)
 		if err != nil {
 			log.Info("%v", err)
 			// TODO(e.burkov): If passed upstream have already

diff --git a/internal/dnsforward/http_test.go b/internal/dnsforward/http_test.go
@@ -18,7 +18,8 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// fakeSystemResolvers is a mock aghnet.SystemResolvers implementation for tests.
+// fakeSystemResolvers is a mock aghnet.SystemResolvers implementation for
+// tests.
 type fakeSystemResolvers struct {
 	// SystemResolvers is embedded here simply to make *fakeSystemResolvers
 	// an aghnet.SystemResolvers without actually implementing all methods.

diff --git a/internal/home/authratelimiter.go b/internal/home/authratelimiter.go
@@ -72,7 +72,7 @@ func (ab *authRateLimiter) check(usrID string) (left time.Duration) {
 // incLocked increments the number of unsuccessful attempts for attempter with
 // ip and updates it's blocking moment if needed.  For internal use only.
 func (ab *authRateLimiter) incLocked(usrID string, now time.Time) {
-	var until time.Time = now.Add(failedAuthTTL)
+	until := now.Add(failedAuthTTL)
 	var attNum uint = 1
 
 	a, ok := ab.failedAuths[usrID]

diff --git a/internal/home/clients.go b/internal/home/clients.go
@@ -361,7 +361,7 @@ func (clients *clientsContainer) findUpstreams(
 		upstreams,
 		upstream.Options{
 			Bootstrap: config.DNS.BootstrapDNS,
-			Timeout:   dnsforward.DefaultTimeout,
+			Timeout:   config.DNS.UpstreamTimeout.Duration,
 		},
 	)
 	if err != nil {

diff --git a/internal/home/config.go b/internal/home/config.go
@@ -114,6 +114,9 @@ type dnsConfig struct {
 	FiltersUpdateIntervalHours uint32           `yaml:"filters_update_interval"` // time period to update filters (in hours)
 	DnsfilterConf              filtering.Config `yaml:",inline"`
 
+	// UpstreamTimeout is the timeout for querying upstream servers.
+	UpstreamTimeout Duration `yaml:"upstream_timeout"`
+
 	// LocalDomainName is the domain name used for known internal hosts.
 	// For example, a machine called "myhost" can be addressed as
 	// "myhost.lan" when LocalDomainName is "lan".
@@ -182,6 +185,7 @@ var config = configuration{
 		},
 		FilteringEnabled:           true, // whether or not use filter lists
 		FiltersUpdateIntervalHours: 24,
+		UpstreamTimeout:            Duration{dnsforward.DefaultTimeout},
 		LocalDomainName:            "lan",
 		ResolveClients:             true,
 		UsePrivateRDNS:             true,
@@ -276,6 +280,10 @@ func parseConfig() error {
 		config.DNS.FiltersUpdateIntervalHours = 24
 	}
 
+	if config.DNS.UpstreamTimeout.Duration == 0 {
+		config.DNS.UpstreamTimeout = Duration{dnsforward.DefaultTimeout}
+	}
+
 	return nil
 }
 

diff --git a/internal/home/dns.go b/internal/home/dns.go
@@ -202,6 +202,7 @@ func generateServerConfig() (newConf dnsforward.ServerConfig, err error) {
 	newConf.ResolveClients = dnsConf.ResolveClients
 	newConf.UsePrivateRDNS = dnsConf.UsePrivateRDNS
 	newConf.LocalPTRResolvers = dnsConf.LocalPTRResolvers
+	newConf.UpstreamTimeout = dnsConf.UpstreamTimeout.Duration
 
 	return newConf, nil
 }

diff --git a/internal/home/duration.go b/internal/home/duration.go
@@ -0,0 +1,28 @@
+package home
+
+import (
+	"time"
+
+	"github.com/AdguardTeam/golibs/errors"
+)
+
+// Duration is a wrapper for time.Duration providing functionality for encoding.
+type Duration struct {
+	// time.Duration is embedded here to avoid implementing all the methods.
+	time.Duration
+}
+
+// MarshalText implements the encoding.TextMarshaler interface for Duration.
+func (d Duration) MarshalText() (text []byte, err error) {
+	return []byte(d.String()), nil
+}
+
+// UnmarshalText implements the encoding.TextUnmarshaler interface for
+// *Duration.
+func (d *Duration) UnmarshalText(b []byte) (err error) {
+	defer func() { err = errors.Annotate(err, "unmarshalling duration: %w") }()
+
+	d.Duration, err = time.ParseDuration(string(b))
+
+	return err
+}