CoreDNS server timeouts pass 1

Signed-off-by: Rich <git0@bitservices.io>
coredns · Tantalor93 · Dec 28, 2022 · Nov 28, 2022 · Dec 6, 2022 · Dec 6, 2022
commit 1fc3a69e48ce8d38e52ff9d1e9715d0b764fffba
@@ -1,6 +1,7 @@
 # only add build artifacts concerning coredns - no editor related files
 coredns
 coredns.exe
+Corefile
 build/
 release/
 vendor/
@@ -5,6 +5,7 @@ import (
 	"crypto/tls"
 	"fmt"
 	"net/http"
+	"time"
 
 	"github.com/coredns/caddy"
 	"github.com/coredns/coredns/plugin"
@@ -53,6 +54,9 @@ type Config struct {
 	// TLSConfig when listening for encrypted connections (gRPC, DNS-over-TLS).
 	TLSConfig *tls.Config
 
+	// Timeouts for TCP, TLS and HTTPS servers.
+	Timeouts map[string]time.Duration
+
 	// TSIG secrets, [name]key.
 	TsigSecret map[string]string
 

@@ -150,6 +150,7 @@ func (h *dnsContext) MakeServers() ([]caddy.Server, error) {
 
 		// Fork TLSConfig for each encrypted connection
 		c.TLSConfig = c.firstConfigInBlock.TLSConfig.Clone()
+		c.Timeouts = c.firstConfigInBlock.Timeouts
 		c.TsigSecret = c.firstConfigInBlock.TsigSecret
 	}
 
@@ -223,7 +224,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)

@@ -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,18 @@ 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
+		for key, timeout := range site.Timeouts {
+			switch key {
+			case "idle":
+				s.idleTimeout = timeout
+			case "read":
+				s.readTimeout = timeout
+			case "write":
+				s.writeTimeout = timeout
+			}
+		}
+
 		// copy tsig secrets
 		for key, secret := range site.TsigSecret {
 			s.tsigSecret[key] = secret
@@ -130,11 +148,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)
-		ctx = context.WithValue(ctx, LoopKey{}, 0)
-		s.ServeDNS(ctx, w, r)
-	}), TsigSecret: s.tsigSecret}
+
+	s.server[tcp] = &dns.Server{Listener: l,
+		Net:           "tcp",
+		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 +433,8 @@ func errorAndMetricsFunc(server string, w dns.ResponseWriter, r *dns.Msg, rc int
 const (
 	tcp = 0
 	udp = 1
+
+	tcpMaxQueries = -1
 )
 
 type (

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

@@ -5,6 +5,7 @@ import (
 	"crypto/tls"
 	"fmt"
 	"net"
+	"time"
 
 	"github.com/coredns/caddy"
 	"github.com/coredns/coredns/plugin/pkg/reuseport"
@@ -49,12 +50,43 @@ func (s *ServerTLS) Serve(l net.Listener) error {
 		l = tls.NewListener(l, s.tlsConfig)
 	}
 
+	var (
+		TLSIdleTimeout  time.Duration
+		TLSReadTimeout  time.Duration
+		TLSWriteTimeout time.Duration
+	)
+
+	if s.idleTimeout == time.Duration(0) {
+		TLSIdleTimeout = DefaultTLSIdleTimeout
+	} else {
+		TLSIdleTimeout = s.idleTimeout
+	}
+	if s.readTimeout == time.Duration(0) {
+		TLSReadTimeout = DefaultTLSReadTimeout
+	} else {
+		TLSReadTimeout = s.readTimeout
+	}
+	if s.readTimeout == time.Duration(0) {
+		TLSWriteTimeout = DefaultTLSWriteTimeout
+	} else {
+		TLSWriteTimeout = s.writeTimeout
+	}
+
 	// 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) {
-		ctx := context.WithValue(context.Background(), Key{}, s.Server)
-		ctx = context.WithValue(ctx, LoopKey{}, 0)
-		s.ServeDNS(ctx, w, r)
-	})}
+	s.server[tcp] = &dns.Server{Listener: l,
+		Net:           "tcp-tls",
+		MaxTCPQueries: TLSMaxQueries,
+		ReadTimeout:   TLSReadTimeout,
+		WriteTimeout:  TLSWriteTimeout,
+		IdleTimeout: func() time.Duration {
+			return TLSIdleTimeout
+		},
+		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 +119,7 @@ func (s *ServerTLS) OnStartupComplete() {
 		fmt.Print(out)
 	}
 }
+
+const (
+	tlsMaxQueries = -1
+)
@@ -14,6 +14,7 @@ var Directives = []string{
 	"geoip",
 	"cancel",
 	"tls",
+	"timeouts",
 	"reload",
 	"nsid",
 	"bufsize",

@@ -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"

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

diff --git a/plugin/pkg/timeouts/timeouts.go b/plugin/pkg/timeouts/timeouts.go
@@ -0,0 +1,45 @@
+package timeouts
+
+import (
+	"fmt"
+	"strconv"
+	"time"
+)
+
+func NewTimeoutsConfigFromArgs(args ...string) (map[string]time.Duration, error) {
+	c := make(map[string]time.Duration)
+
+	for i := 0; i < len(args); i++ {
+		t, err := validateTimeout(args[i])
+
+		if err != nil {
+			return c, err
+		}
+
+		switch i {
+		case 0:
+			c["read"] = t
+		case 1:
+			c["write"] = t
+		case 2:
+			c["idle"] = t
+		default:
+			return c, fmt.Errorf("maximum of three arguments allowed for timeouts config, found %d", len(args))
+		}
+	}
+
+	return c, nil
+}
+
+func validateTimeout(t string) (time.Duration, error) {
+	i, err := strconv.Atoi(t)
+	if err != nil {
+		return time.Duration(0), fmt.Errorf("timeout provided '%s' does not appear to be numeric", t)
+	}
+
+	if i < 1 || i > 86400 {
+		return time.Duration(0), fmt.Errorf("timeout provided '%d' needs to be between 1 and 86400 second(s)", i)
+	}
+
+	return time.Duration(i) * time.Second, nil
+}
diff --git a/plugin/pkg/timeouts/timeouts_test.go b/plugin/pkg/timeouts/timeouts_test.go
@@ -0,0 +1,101 @@
+package timeouts
+
+import (
+	"testing"
+)
+
+func TestNewTimeoutsConfigFromArgs(t *testing.T) {
+	var validIdleTimeoutArg = "300" // Args from configuration are always strings
+	var validReadTimeoutArg = "30"
+	var validWriteTimeoutArg = "60"
+
+	var invalidTimeoutString = "twenty seconds"
+	var invalidTimeoutLow = "0"
+	var invalidTimeoutHigh = "86401"
+
+	// No Arguments
+	to, err := NewTimeoutsConfigFromArgs()
+	if err != nil {
+		t.Errorf("Failed to create timeouts map when no arguments specified: %s", err)
+	}
+
+	if len(to) != 0 {
+		t.Error("Timeouts map with no arguments should be empty")
+	}
+
+	// Read Timeout only
+	to, err = NewTimeoutsConfigFromArgs(validReadTimeoutArg)
+	if err != nil {
+		t.Errorf("Failed to create timeouts map given just a read timeout: %s", err)
+	}
+
+	if _, ok := to["read"]; !ok {
+		t.Error("Timeouts map given just a read timeout did not return a read timeout")
+	}
+
+	if _, ok := to["write"]; ok {
+		t.Error("Timeouts map given just a read timeout also returned a write timeout")
+	}
+
+	if _, ok := to["idle"]; ok {
+		t.Error("Timeouts map given just a read timeout also returned an idle timeout")
+	}
+
+	// Read and Write Timeouts (no Idle)
+	to, err = NewTimeoutsConfigFromArgs(validReadTimeoutArg, validWriteTimeoutArg)
+	if err != nil {
+		t.Errorf("Failed to create timeouts map given a read and write timeout: %s", err)
+	}
+
+	if _, ok := to["read"]; !ok {
+		t.Error("Timeouts map given a read and write timeout did not return a read timeout")
+	}
+
+	if _, ok := to["write"]; !ok {
+		t.Error("Timeouts map given a read and write timeout did not return a write timeout")
+	}
+
+	if _, ok := to["idle"]; ok {
+		t.Error("Timeouts map given a read and write timeout also returned an idle timeout")
+	}
+
+	// All Timeouts
+	to, err = NewTimeoutsConfigFromArgs(validReadTimeoutArg, validWriteTimeoutArg, validIdleTimeoutArg)
+	if err != nil {
+		t.Errorf("Failed to create timeouts map given all timeouts: %s", err)
+	}
+
+	if _, ok := to["read"]; !ok {
+		t.Error("Timeouts map given all timeouts did not return a read timeout")
+	}
+
+	if _, ok := to["write"]; !ok {
+		t.Error("Timeouts map given all timeouts did not return a write timeout")
+	}
+
+	if _, ok := to["idle"]; !ok {
+		t.Error("Timeouts map given all timeouts did not return an idle idle timeout")
+	}
+
+	// Too Many Timeouts
+	to, err = NewTimeoutsConfigFromArgs(validReadTimeoutArg, validWriteTimeoutArg, validIdleTimeoutArg, "100")
+	if err == nil {
+		t.Error("Attempt to create timeouts with too many arguments was successful")
+	}
+
+	// Timeout Validation
+	to, err = NewTimeoutsConfigFromArgs(invalidTimeoutString)
+	if err == nil {
+		t.Error("Attempt to create timeouts with non-numeric value was successful")
+	}
+
+	to, err = NewTimeoutsConfigFromArgs(invalidTimeoutLow)
+	if err == nil {
+		t.Error("Attempt to create a timeout of less than 1 second was successful")
+	}
+
+	to, err = NewTimeoutsConfigFromArgs(invalidTimeoutHigh)
+	if err == nil {
+		t.Error("Attempt to create a timeout of more than 1 day was successful")
+	}
+}