plugin/timeouts - Allow ability to configure listening server timeouts (#5784)

.gitignore

@@ -1,6 +1,7 @@
 # only add build artifacts concerning coredns - no editor related files
 coredns
 coredns.exe
+Corefile
 build/
 release/
 vendor/

core/dnsserver/config.go

@@ -5,6 +5,7 @@ import (
 	"crypto/tls"
 	"fmt"
 	"net/http"
+	"time"
 
 	"github.com/coredns/caddy"
 	"github.com/coredns/coredns/plugin"
@@ -53,6 +54,11 @@ type Config struct {
 	// TLSConfig when listening for encrypted connections (gRPC, DNS-over-TLS).
 	TLSConfig *tls.Config
 
+	// Timeouts for TCP, TLS and HTTPS servers.
+	ReadTimeout  time.Duration
+	WriteTimeout time.Duration
+	IdleTimeout  time.Duration
+
 	// TSIG secrets, [name]key.
 	TsigSecret map[string]string
 

core/dnsserver/register.go

@@ -150,6 +150,9 @@ func (h *dnsContext) MakeServers() ([]caddy.Server, error) {
 
 		// Fork TLSConfig for each encrypted connection
 		c.TLSConfig = c.firstConfigInBlock.TLSConfig.Clone()
+		c.ReadTimeout = c.firstConfigInBlock.ReadTimeout
+		c.WriteTimeout = c.firstConfigInBlock.WriteTimeout
+		c.IdleTimeout = c.firstConfigInBlock.IdleTimeout
 		c.TsigSecret = c.firstConfigInBlock.TsigSecret
 	}
 
@@ -223,7 +226,8 @@ func (c *Config) AddPlugin(m plugin.Plugin) {
 }
 
 // registerHandler adds a handler to a site's handler registration. Handlers
-//  use this to announce that they exist to other plugin.
+//
+//	use this to announce that they exist to other plugin.
 func (c *Config) registerHandler(h plugin.Handler) {
 	if c.registry == nil {
 		c.registry = make(map[string]plugin.Handler)

core/dnsserver/server.go

@@ -44,6 +44,9 @@ type Server struct {
 	debug        bool                 // disable recover()
 	stacktrace   bool                 // enable stacktrace in recover error log
 	classChaos   bool                 // allow non-INET class queries
+	idleTimeout  time.Duration        // Idle timeout for TCP
+	readTimeout  time.Duration        // Read timeout for TCP
+	writeTimeout time.Duration        // Write timeout for TCP
 
 	tsigSecret map[string]string
 }
@@ -60,6 +63,9 @@ func NewServer(addr string, group []*Config) (*Server, error) {
 		Addr:         addr,
 		zones:        make(map[string][]*Config),
 		graceTimeout: 5 * time.Second,
+		idleTimeout:  10 * time.Second,
+		readTimeout:  3 * time.Second,
+		writeTimeout: 5 * time.Second,
 		tsigSecret:   make(map[string]string),
 	}
 
@@ -81,6 +87,17 @@ func NewServer(addr string, group []*Config) (*Server, error) {
 		// append the config to the zone's configs
 		s.zones[site.Zone] = append(s.zones[site.Zone], site)
 
+		// set timeouts
+		if site.ReadTimeout != 0 {
+			s.readTimeout = site.ReadTimeout
+		}
+		if site.WriteTimeout != 0 {
+			s.writeTimeout = site.WriteTimeout
+		}
+		if site.IdleTimeout != 0 {
+			s.idleTimeout = site.IdleTimeout
+		}
+
 		// copy tsig secrets
 		for key, secret := range site.TsigSecret {
 			s.tsigSecret[key] = secret
@@ -130,11 +147,22 @@ var _ caddy.GracefulServer = &Server{}
 // This implements caddy.TCPServer interface.
 func (s *Server) Serve(l net.Listener) error {
 	s.m.Lock()
-	s.server[tcp] = &dns.Server{Listener: l, Net: "tcp", Handler: dns.HandlerFunc(func(w dns.ResponseWriter, r *dns.Msg) {
+
-		ctx := context.WithValue(context.Background(), Key{}, s)
+	s.server[tcp] = &dns.Server{Listener: l,
-		ctx = context.WithValue(ctx, LoopKey{}, 0)
+		Net:           "tcp",
-		s.ServeDNS(ctx, w, r)
+		TsigSecret:    s.tsigSecret,
-	}), TsigSecret: s.tsigSecret}
+		MaxTCPQueries: tcpMaxQueries,
+		ReadTimeout:   s.readTimeout,
+		WriteTimeout:  s.writeTimeout,
+		IdleTimeout: func() time.Duration {
+			return s.idleTimeout
+		},
+		Handler: dns.HandlerFunc(func(w dns.ResponseWriter, r *dns.Msg) {
+			ctx := context.WithValue(context.Background(), Key{}, s)
+			ctx = context.WithValue(ctx, LoopKey{}, 0)
+			s.ServeDNS(ctx, w, r)
+		})}
+
 	s.m.Unlock()
 
 	return s.server[tcp].ActivateAndServe()
@@ -404,6 +432,8 @@ func errorAndMetricsFunc(server string, w dns.ResponseWriter, r *dns.Msg, rc int
 const (
 	tcp = 0
 	udp = 1
+
+	tcpMaxQueries = -1
 )
 
 type (

core/dnsserver/server_https.go

@@ -75,9 +75,9 @@ func NewServerHTTPS(addr string, group []*Config) (*ServerHTTPS, error) {
 	}
 
 	srv := &http.Server{
-		ReadTimeout:  5 * time.Second,
+		ReadTimeout:  s.readTimeout,
-		WriteTimeout: 10 * time.Second,
+		WriteTimeout: s.writeTimeout,
-		IdleTimeout:  120 * time.Second,
+		IdleTimeout:  s.idleTimeout,
 		ErrorLog:     stdlog.New(&loggerAdapter{}, "", 0),
 	}
 	sh := &ServerHTTPS{

core/dnsserver/server_tls.go

@@ -5,6 +5,7 @@ import (
 	"crypto/tls"
 	"fmt"
 	"net"
+	"time"
 
 	"github.com/coredns/caddy"
 	"github.com/coredns/coredns/plugin/pkg/reuseport"
@@ -50,11 +51,20 @@ func (s *ServerTLS) Serve(l net.Listener) error {
 	}
 
 	// Only fill out the TCP server for this one.
-	s.server[tcp] = &dns.Server{Listener: l, Net: "tcp-tls", Handler: dns.HandlerFunc(func(w dns.ResponseWriter, r *dns.Msg) {
+	s.server[tcp] = &dns.Server{Listener: l,
-		ctx := context.WithValue(context.Background(), Key{}, s.Server)
+		Net:           "tcp-tls",
-		ctx = context.WithValue(ctx, LoopKey{}, 0)
+		MaxTCPQueries: tlsMaxQueries,
-		s.ServeDNS(ctx, w, r)
+		ReadTimeout:   s.readTimeout,
-	})}
+		WriteTimeout:  s.writeTimeout,
+		IdleTimeout: func() time.Duration {
+			return s.idleTimeout
+		},
+		Handler: dns.HandlerFunc(func(w dns.ResponseWriter, r *dns.Msg) {
+			ctx := context.WithValue(context.Background(), Key{}, s.Server)
+			ctx = context.WithValue(ctx, LoopKey{}, 0)
+			s.ServeDNS(ctx, w, r)
+		})}
+
 	s.m.Unlock()
 
 	return s.server[tcp].ActivateAndServe()
@@ -87,3 +97,7 @@ func (s *ServerTLS) OnStartupComplete() {
 		fmt.Print(out)
 	}
 }
+
+const (
+	tlsMaxQueries = -1
+)

core/dnsserver/zdirectives.go

@@ -14,6 +14,7 @@ var Directives = []string{
 	"geoip",
 	"cancel",
 	"tls",
+	"timeouts",
 	"reload",
 	"nsid",
 	"bufsize",

core/plugin/zplugin.go

@@ -49,6 +49,7 @@ import (
 	_ "github.com/coredns/coredns/plugin/secondary"
 	_ "github.com/coredns/coredns/plugin/sign"
 	_ "github.com/coredns/coredns/plugin/template"
+	_ "github.com/coredns/coredns/plugin/timeouts"
 	_ "github.com/coredns/coredns/plugin/tls"
 	_ "github.com/coredns/coredns/plugin/trace"
 	_ "github.com/coredns/coredns/plugin/transfer"

plugin.cfg

@@ -23,6 +23,7 @@ metadata:metadata
 geoip:geoip
 cancel:cancel
 tls:tls
+timeouts:timeouts
 reload:reload
 nsid:nsid
 bufsize:bufsize

plugin/pkg/durations/durations.go

@@ -0,0 +1,26 @@
+package durations
+
+import (
+	"fmt"
+	"strconv"
+	"time"
+)
+
+// NewDurationFromArg returns a time.Duration from a configuration argument
+// (string) which has come from the Corefile. The argument has some basic
+// validation applied before returning a time.Duration. If the argument has no
+// time unit specified and is numeric the argument will be treated as seconds
+// rather than GO's default of nanoseconds.
+func NewDurationFromArg(arg string) (time.Duration, error) {
+	_, err := strconv.Atoi(arg)
+	if err == nil {
+		arg = arg + "s"
+	}
+
+	d, err := time.ParseDuration(arg)
+	if err != nil {
+		return 0, fmt.Errorf("failed to parse duration '%s'", arg)
+	}
+
+	return d, nil
+}

plugin/pkg/durations/durations_test.go

@@ -0,0 +1,51 @@
+package durations
+
+import (
+	"testing"
+	"time"
+)
+
+func TestNewDurationFromArg(t *testing.T) {
+	tests := []struct {
+		name    string
+		arg     string
+		wantErr bool
+		want    time.Duration
+	}{
+		{
+			name: "valid GO duration - seconds",
+			arg:  "30s",
+			want: 30 * time.Second,
+		},
+		{
+			name: "valid GO duration - minutes",
+			arg:  "2m",
+			want: 2 * time.Minute,
+		},
+		{
+			name: "number - fallback to seconds",
+			arg:  "30",
+			want: 30 * time.Second,
+		},
+		{
+			name:    "invalid duration",
+			arg:     "twenty seconds",
+			wantErr: true,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			actual, err := NewDurationFromArg(test.arg)
+			if test.wantErr && err == nil {
+				t.Error("error was expected")
+			}
+			if !test.wantErr && err != nil {
+				t.Error("error was not expected")
+			}
+
+			if test.want != actual {
+				t.Errorf("expected '%v' got '%v'", test.want, actual)
+			}
+		})
+	}
+}

plugin/timeouts/README.md

@@ -0,0 +1,76 @@
+# timeouts
+
+## Name
+
+*timeouts* - allows you to configure the server read, write and idle timeouts for the TCP, TLS and DoH servers.
+
+## Description
+
+CoreDNS is configured with sensible timeouts for server connections by default.
+However in some cases for example where CoreDNS is serving over a slow mobile
+data connection the default timeouts are not optimal.
+
+Additionally some routers hold open connections when using DNS over TLS or DNS
+over HTTPS. Allowing a longer idle timeout helps performance and reduces issues
+with such routers.
+
+The *timeouts* "plugin" allows you to configure CoreDNS server read, write and
+idle timeouts.
+
+## Syntax
+
+~~~ txt
+timeouts {
+	read DURATION
+	write DURATION
+	idle DURATION
+}
+~~~
+
+For any timeouts that are not provided, default values are used which may vary
+depending on the server type. At least one timeout must be specified otherwise
+the entire timeouts block should be omitted.
+
+## Examples
+
+Start a DNS-over-TLS server that picks up incoming DNS-over-TLS queries on port
+5553 and uses the nameservers defined in `/etc/resolv.conf` to resolve the
+query. This proxy path uses plain old DNS. A 10 second read timeout, 20
+second write timeout and a 60 second idle timeout have been configured.
+
+~~~
+tls://.:5553 {
+	tls cert.pem key.pem ca.pem
+	timeouts {
+		read 10s
+		write 20s
+		idle 60s
+	}
+	forward . /etc/resolv.conf
+}
+~~~
+
+Start a DNS-over-HTTPS server that is similar to the previous example. Only the
+read timeout has been configured for 1 minute.
+
+~~~
+https://. {
+	tls cert.pem key.pem ca.pem
+	timeouts {
+		read 1m
+	}
+	forward . /etc/resolv.conf
+}
+~~~
+
+Start a standard TCP/UDP server on port 1053. A read and write timeout has been
+configured. The timeouts are only applied to the TCP side of the server.
+~~~
+.:1053 {
+	timeouts {
+		read 15s
+                write 30s
+	}
+	forward . /etc/resolv.conf
+}
+~~~

plugin/timeouts/timeouts.go

@@ -0,0 +1,69 @@
+package timeouts
+
+import (
+	"time"
+
+	"github.com/coredns/caddy"
+	"github.com/coredns/coredns/core/dnsserver"
+	"github.com/coredns/coredns/plugin"
+	"github.com/coredns/coredns/plugin/pkg/durations"
+)
+
+func init() { plugin.Register("timeouts", setup) }
+
+func setup(c *caddy.Controller) error {
+	err := parseTimeouts(c)
+	if err != nil {
+		return plugin.Error("timeouts", err)
+	}
+	return nil
+}
+
+func parseTimeouts(c *caddy.Controller) error {
+	config := dnsserver.GetConfig(c)
+
+	for c.Next() {
+		args := c.RemainingArgs()
+		if len(args) > 0 {
+			return plugin.Error("timeouts", c.ArgErr())
+		}
+
+		b := 0
+		for c.NextBlock() {
+			block := c.Val()
+			timeoutArgs := c.RemainingArgs()
+			if len(timeoutArgs) != 1 {
+				return c.ArgErr()
+			}
+
+			timeout, err := durations.NewDurationFromArg(timeoutArgs[0])
+			if err != nil {
+				return c.Err(err.Error())
+			}
+
+			if timeout < (1*time.Second) || timeout > (24*time.Hour) {
+				return c.Errf("timeout provided '%s' needs to be between 1 second and 24 hours", timeout)
+			}
+
+			switch block {
+			case "read":
+				config.ReadTimeout = timeout
+
+			case "write":
+				config.WriteTimeout = timeout
+
+			case "idle":
+				config.IdleTimeout = timeout
+
+			default:
+				return c.Errf("unknown option: '%s'", block)
+			}
+			b++
+		}
+
+		if b == 0 {
+			return plugin.Error("timeouts", c.Err("timeouts block with no timeouts specified"))
+		}
+	}
+	return nil
+}

plugin/timeouts/timeouts_test.go

@@ -0,0 +1,75 @@
+package timeouts
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/coredns/caddy"
+)
+
+func TestTimeouts(t *testing.T) {
+	tests := []struct {
+		input              string
+		shouldErr          bool
+		expectedRoot       string // expected root, set to the controller. Empty for negative cases.
+		expectedErrContent string // substring from the expected error. Empty for positive cases.
+	}{
+		// positive
+		{`timeouts {
+			read 30s
+		}`, false, "", ""},
+		{`timeouts {
+			read 1m
+			write 2m
+		}`, false, "", ""},
+		{` timeouts {
+			idle 1h
+		}`, false, "", ""},
+		{`timeouts {
+			read 10
+			write 20
+			idle 60
+		}`, false, "", ""},
+		// negative
+		{`timeouts`, true, "", "block with no timeouts specified"},
+		{`timeouts {
+		}`, true, "", "block with no timeouts specified"},
+		{`timeouts {
+			read 10s
+			giraffe 30s
+		}`, true, "", "unknown option"},
+		{`timeouts {
+			read 10s 20s
+			write 30s
+		}`, true, "", "Wrong argument"},
+		{`timeouts {
+			write snake
+		}`, true, "", "failed to parse duration"},
+		{`timeouts {
+			idle 0s
+		}`, true, "", "needs to be between"},
+		{`timeouts {
+			read 48h
+		}`, true, "", "needs to be between"},
+	}
+
+	for i, test := range tests {
+		c := caddy.NewTestController("dns", test.input)
+		err := setup(c)
+		//cfg := dnsserver.GetConfig(c)
+
+		if test.shouldErr && err == nil {
+			t.Errorf("Test %d: Expected error but found %s for input %s", i, err, test.input)
+		}
+
+		if err != nil {
+			if !test.shouldErr {
+				t.Errorf("Test %d: Expected no error but found one for input %s. Error was: %v", i, test.input, err)
+			}
+
+			if !strings.Contains(err.Error(), test.expectedErrContent) {
+				t.Errorf("Test %d: Expected error to contain: %v, found error: %v, input: %s", i, test.expectedErrContent, err, test.input)
+			}
+		}
+	}
+}