diff --git a/plugin/forward/README.md b/plugin/forward/README.md index eb05fab3b..a7ca9a84b 100644 --- a/plugin/forward/README.md +++ b/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 diff --git a/plugin/forward/forward.go b/plugin/forward/forward.go index fdb3e71fc..760175d2b 100644 --- a/plugin/forward/forward.go +++ b/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 } diff --git a/plugin/forward/setup.go b/plugin/forward/setup.go index 2010131c2..86b078f91 100644 --- a/plugin/forward/setup.go +++ b/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() diff --git a/plugin/forward/setup_test.go b/plugin/forward/setup_test.go index 350194ef2..86dee82ca 100644 --- a/plugin/forward/setup_test.go +++ b/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) + } + }) + } +}