plugin/forward: make per-upstream read timeout configurable (#8205)

plugin/forward/README.md

@@ -47,6 +47,7 @@ forward FROM TO... {
     prefer_udp
     expire DURATION
     max_idle_conns INTEGER
+    read_timeout DURATION
     max_fails INTEGER
     max_connect_attempts INTEGER
     doh_method GET|POST
@@ -79,6 +80,11 @@ forward FROM TO... {
 * `doh_method` **GET|POST**, whether to use GET or POST http method for DoH requests (defaults to POST).
 * `max_idle_conns` **INTEGER**, maximum number of idle connections to cache per upstream for reuse.
   Default is 0, which means unlimited.
+* `read_timeout` **DURATION**, the per-query read timeout applied to each upstream when waiting for a
+  response. The default is 2s. Increase this if upstreams legitimately take longer than 2s to answer
+  (for example slow recursive resolutions that would otherwise surface as `SERVFAIL`/timeouts). Note
+  that this timeout applies to each upstream individually, so large values reduce the time available
+  to retry other upstreams within a single query.
 * `tls` **CERT** **KEY** **CA** define the TLS properties for TLS connection. From 0 to 3 arguments can be
   provided with the meaning as described below
 

plugin/forward/forward.go

@@ -29,8 +29,9 @@ import (
 var log = clog.NewWithPlugin("forward")
 
 const (
-	defaultExpire = 10 * time.Second
-	hcInterval    = 500 * time.Millisecond
+	defaultExpire      = 10 * time.Second
+	defaultReadTimeout = 2 * time.Second
+	hcInterval         = 500 * time.Millisecond
 )
 
 // Forward represents a plugin instance that can proxy requests to another (DNS) server. It has a list
@@ -53,6 +54,7 @@ type Forward struct {
 	maxfails                   uint32
 	expire                     time.Duration
 	maxAge                     time.Duration
+	readTimeout                time.Duration
 	maxIdleConns               int
 	dohMethod                  string
 	maxConcurrent              int64
@@ -77,7 +79,7 @@ type Forward struct {
 
 // New returns a new Forward.
 func New() *Forward {
-	f := &Forward{maxfails: 2, tlsConfig: new(tls.Config), expire: defaultExpire, p: new(random), from: ".", hcInterval: hcInterval, dohMethod: http.MethodPost, opts: proxyPkg.Options{ForceTCP: false, PreferUDP: false, HCRecursionDesired: true, HCDomain: "."}}
+	f := &Forward{maxfails: 2, tlsConfig: new(tls.Config), expire: defaultExpire, readTimeout: defaultReadTimeout, p: new(random), from: ".", hcInterval: hcInterval, dohMethod: http.MethodPost, opts: proxyPkg.Options{ForceTCP: false, PreferUDP: false, HCRecursionDesired: true, HCDomain: "."}}
 	return f
 }
 

plugin/forward/setup.go

@@ -250,6 +250,7 @@ func parseStanza(c *caddy.Controller) (*Forward, error) {
 		f.proxies[i].SetExpire(f.expire)
 		f.proxies[i].SetMaxAge(f.maxAge)
 		f.proxies[i].SetMaxIdleConns(f.maxIdleConns)
+		f.proxies[i].SetReadTimeout(f.readTimeout)
 		f.proxies[i].GetHealthchecker().SetRecursionDesired(f.opts.HCRecursionDesired)
 		// when TLS is used, checks are set to tcp-tls
 		if f.opts.ForceTCP && transports[i] != transport.TLS {
@@ -389,6 +390,18 @@ func parseBlock(c *caddy.Controller, f *Forward) error {
 			return fmt.Errorf("max_idle_conns can't be negative: %d", n)
 		}
 		f.maxIdleConns = n
+	case "read_timeout":
+		if !c.NextArg() {
+			return c.ArgErr()
+		}
+		dur, err := time.ParseDuration(c.Val())
+		if err != nil {
+			return err
+		}
+		if dur <= 0 {
+			return fmt.Errorf("read_timeout must be positive: %s", dur)
+		}
+		f.readTimeout = dur
 	case "doh_method":
 		if !c.NextArg() {
 			return c.ArgErr()

plugin/forward/setup_test.go

@@ -794,3 +794,75 @@ func TestSetupMaxAge(t *testing.T) {
 		})
 	}
 }
+
+func TestSetupReadTimeout(t *testing.T) {
+	tests := []struct {
+		name        string
+		input       string
+		shouldErr   bool
+		expectedVal time.Duration
+		expectedErr string
+	}{
+		{
+			name:        "default (no read_timeout)",
+			input:       "forward . 127.0.0.1\n",
+			expectedVal: defaultReadTimeout,
+		},
+		{
+			name:        "valid read_timeout",
+			input:       "forward . 127.0.0.1 {\nread_timeout 5s\n}\n",
+			expectedVal: 5 * time.Second,
+		},
+		{
+			name:        "sub-second read_timeout",
+			input:       "forward . 127.0.0.1 {\nread_timeout 500ms\n}\n",
+			expectedVal: 500 * time.Millisecond,
+		},
+		{
+			name:        "zero read_timeout",
+			input:       "forward . 127.0.0.1 {\nread_timeout 0s\n}\n",
+			shouldErr:   true,
+			expectedErr: "positive",
+		},
+		{
+			name:        "negative read_timeout",
+			input:       "forward . 127.0.0.1 {\nread_timeout -1s\n}\n",
+			shouldErr:   true,
+			expectedErr: "positive",
+		},
+		{
+			name:        "invalid read_timeout value",
+			input:       "forward . 127.0.0.1 {\nread_timeout invalid\n}\n",
+			shouldErr:   true,
+			expectedErr: "invalid",
+		},
+		{
+			name:      "missing read_timeout value",
+			input:     "forward . 127.0.0.1 {\nread_timeout\n}\n",
+			shouldErr: true,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			c := caddy.NewTestController("dns", test.input)
+			fs, err := parseForward(c)
+			if test.shouldErr {
+				if err == nil {
+					t.Errorf("expected error but found none for input %s", test.input)
+					return
+				}
+				if test.expectedErr != "" && !strings.Contains(err.Error(), test.expectedErr) {
+					t.Errorf("expected error to contain %q, got: %v", test.expectedErr, err)
+				}
+				return
+			}
+			if err != nil {
+				t.Errorf("expected no error but found: %v", err)
+				return
+			}
+			if fs[0].readTimeout != test.expectedVal {
+				t.Errorf("expected readTimeout %v, got %v", test.expectedVal, fs[0].readTimeout)
+			}
+		})
+	}
+}