From 1fc3a69e48ce8d38e52ff9d1e9715d0b764fffba Mon Sep 17 00:00:00 2001 From: Rich Date: Mon, 28 Nov 2022 16:04:24 +0000 Subject: [PATCH 1/9] CoreDNS server timeouts pass 1 Signed-off-by: Rich --- .gitignore | 1 + core/dnsserver/config.go | 4 ++ core/dnsserver/register.go | 4 +- core/dnsserver/server.go | 41 +++++++++-- core/dnsserver/server_https.go | 6 +- core/dnsserver/server_tls.go | 46 ++++++++++-- core/dnsserver/zdirectives.go | 1 + core/plugin/zplugin.go | 1 + plugin.cfg | 1 + plugin/pkg/timeouts/timeouts.go | 45 ++++++++++++ plugin/pkg/timeouts/timeouts_test.go | 101 +++++++++++++++++++++++++++ plugin/timeouts/README.md | 58 +++++++++++++++ plugin/timeouts/timeouts.go | 41 +++++++++++ plugin/timeouts/timeouts_test.go | 48 +++++++++++++ 14 files changed, 384 insertions(+), 14 deletions(-) create mode 100644 plugin/pkg/timeouts/timeouts.go create mode 100644 plugin/pkg/timeouts/timeouts_test.go create mode 100644 plugin/timeouts/README.md create mode 100644 plugin/timeouts/timeouts.go create mode 100644 plugin/timeouts/timeouts_test.go diff --git a/.gitignore b/.gitignore index 67780a47152..5a6dd1245e6 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,7 @@ # only add build artifacts concerning coredns - no editor related files coredns coredns.exe +Corefile build/ release/ vendor/ diff --git a/core/dnsserver/config.go b/core/dnsserver/config.go index 3da86271e21..db986fac4b6 100644 --- a/core/dnsserver/config.go +++ b/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,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 diff --git a/core/dnsserver/register.go b/core/dnsserver/register.go index 176be49b8de..2141f69d497 100644 --- a/core/dnsserver/register.go +++ b/core/dnsserver/register.go @@ -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) diff --git a/core/dnsserver/server.go b/core/dnsserver/server.go index 478287bf8c1..4cd522a48ea 100644 --- a/core/dnsserver/server.go +++ b/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,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 ( diff --git a/core/dnsserver/server_https.go b/core/dnsserver/server_https.go index eda39c140af..cddf5989089 100644 --- a/core/dnsserver/server_https.go +++ b/core/dnsserver/server_https.go @@ -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{ diff --git a/core/dnsserver/server_tls.go b/core/dnsserver/server_tls.go index 6fff61d5c72..41b8395234d 100644 --- a/core/dnsserver/server_tls.go +++ b/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" @@ -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 +) diff --git a/core/dnsserver/zdirectives.go b/core/dnsserver/zdirectives.go index 38425fb0602..6d7137580c6 100644 --- a/core/dnsserver/zdirectives.go +++ b/core/dnsserver/zdirectives.go @@ -14,6 +14,7 @@ var Directives = []string{ "geoip", "cancel", "tls", + "timeouts", "reload", "nsid", "bufsize", diff --git a/core/plugin/zplugin.go b/core/plugin/zplugin.go index 08003f460c1..b97cd85c56d 100644 --- a/core/plugin/zplugin.go +++ b/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" diff --git a/plugin.cfg b/plugin.cfg index 5632a3b9703..407a668eb4e 100644 --- a/plugin.cfg +++ b/plugin.cfg @@ -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 new file mode 100644 index 00000000000..3b3a14408a3 --- /dev/null +++ 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 new file mode 100644 index 00000000000..974d5f2a063 --- /dev/null +++ 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") + } +} diff --git a/plugin/timeouts/README.md b/plugin/timeouts/README.md new file mode 100644 index 00000000000..a157db700b4 --- /dev/null +++ b/plugin/timeouts/README.md @@ -0,0 +1,58 @@ +# 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 [WRITE] [IDLE] +~~~ + +For any timeouts that are not provided, default values are used which vary depending on the server type. + +## 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 10 20 60 + forward . /etc/resolv.conf +} +~~~ + +Start a DNS-over-gRPC server that is similar to the previous example, but using DNS-over-HTTPS for +incoming queries. Only the read timeout has been configured. + +~~~ +https://. { + tls cert.pem key.pem ca.pem + timeouts 30 + 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 10 10 + forward . /etc/resolv.conf +} +~~~ diff --git a/plugin/timeouts/timeouts.go b/plugin/timeouts/timeouts.go new file mode 100644 index 00000000000..98e2e034b5c --- /dev/null +++ b/plugin/timeouts/timeouts.go @@ -0,0 +1,41 @@ +package timeouts + +import ( + "github.com/coredns/caddy" + "github.com/coredns/coredns/core/dnsserver" + "github.com/coredns/coredns/plugin" + "github.com/coredns/coredns/plugin/pkg/timeouts" +) + +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) + + if config.Timeouts != nil { + return plugin.Error("timeouts", c.Errf("Timeouts already configured for this server instance")) + } + + for c.Next() { + args := c.RemainingArgs() + if len(args) < 1 || len(args) > 3 { + return plugin.Error("timeouts", c.ArgErr()) + } + + timeoutsConfig, err := timeouts.NewTimeoutsConfigFromArgs(args...) + if err != nil { + return err + } + + config.Timeouts = timeoutsConfig + } + return nil +} diff --git a/plugin/timeouts/timeouts_test.go b/plugin/timeouts/timeouts_test.go new file mode 100644 index 00000000000..6c8b888fd4e --- /dev/null +++ b/plugin/timeouts/timeouts_test.go @@ -0,0 +1,48 @@ +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 30", false, "", ""}, + {"timeouts 30 60", false, "", ""}, + {"timeouts 30 60 300", false, "", ""}, + // negative + {"timeouts", true, "", "Wrong argument"}, + {"timeouts 30 60 300 600", true, "", "Wrong argument"}, + {"timeouts ten", true, "", "timeout provided 'ten' does not appear to be numeric"}, + {"timeouts 0", true, "", "timeout provided '0' needs to be between 1 and 86400 second(s)"}, + {"timeouts 86401", true, "", "timeout provided '86401' needs to be between 1 and 86400 second(s)"}, + } + + 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) + } + } + } +} From cce038dfacc3a1a0d791ed104c4e7a2d3441688e Mon Sep 17 00:00:00 2001 From: Rich Date: Tue, 6 Dec 2022 10:35:51 +0000 Subject: [PATCH 2/9] Fix error during rebase Signed-off-by: Rich --- core/dnsserver/server_tls.go | 30 ++++-------------------------- 1 file changed, 4 insertions(+), 26 deletions(-) diff --git a/core/dnsserver/server_tls.go b/core/dnsserver/server_tls.go index 41b8395234d..f2251efb49c 100644 --- a/core/dnsserver/server_tls.go +++ b/core/dnsserver/server_tls.go @@ -50,36 +50,14 @@ 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", - MaxTCPQueries: TLSMaxQueries, - ReadTimeout: TLSReadTimeout, - WriteTimeout: TLSWriteTimeout, + MaxTCPQueries: tlsMaxQueries, + ReadTimeout: s.readTimeout, + WriteTimeout: s.writeTimeout, IdleTimeout: func() time.Duration { - return TLSIdleTimeout + return s.idleTimeout }, Handler: dns.HandlerFunc(func(w dns.ResponseWriter, r *dns.Msg) { ctx := context.WithValue(context.Background(), Key{}, s.Server) From 71c80511b39126092c8377d85ed1bafaff51eb28 Mon Sep 17 00:00:00 2001 From: Rich Date: Tue, 6 Dec 2022 10:43:15 +0000 Subject: [PATCH 3/9] Resolve stickler comment Signed-off-by: Rich --- plugin/pkg/timeouts/timeouts.go | 11 +++++++++ plugin/pkg/tls/tls.go | 41 ++++++++++++++++++--------------- 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/plugin/pkg/timeouts/timeouts.go b/plugin/pkg/timeouts/timeouts.go index 3b3a14408a3..5be71579cc8 100644 --- a/plugin/pkg/timeouts/timeouts.go +++ b/plugin/pkg/timeouts/timeouts.go @@ -6,6 +6,17 @@ import ( "time" ) +// NewTimeoutsConfigFromArgs returns a map of durations keyed by strings based +// upon the passed in list of arguments. Typically these come straight from the +// Corefile. +// one arg: read timeout +// - only sets up the read timeout. Defaults will be used for write/idle timeouts. +// +// two args: read timeout, write timeout +// - sets up the read and write timeouts. Default will be used for idle timeout. +// +// three args: read timeout, write timeout, idle timeout +// - sets read write and idle timeouts. func NewTimeoutsConfigFromArgs(args ...string) (map[string]time.Duration, error) { c := make(map[string]time.Duration) diff --git a/plugin/pkg/tls/tls.go b/plugin/pkg/tls/tls.go index cba25503e00..41eff4bc0d7 100644 --- a/plugin/pkg/tls/tls.go +++ b/plugin/pkg/tls/tls.go @@ -31,28 +31,31 @@ func setTLSDefaults(ctls *tls.Config) { // in list of arguments. Typically these come straight from the // Corefile. // no args -// - creates a Config with no cert and using system CAs -// - use for a client that talks to a server with a public signed cert (CA installed in system) -// - the client will not be authenticated by the server since there is no cert +// - creates a Config with no cert and using system CAs +// - use for a client that talks to a server with a public signed cert (CA installed in system) +// - the client will not be authenticated by the server since there is no cert +// // one arg: the path to CA PEM file -// - creates a Config with no cert using a specific CA -// - use for a client that talks to a server with a private signed cert (CA not installed in system) -// - the client will not be authenticated by the server since there is no cert +// - creates a Config with no cert using a specific CA +// - use for a client that talks to a server with a private signed cert (CA not installed in system) +// - the client will not be authenticated by the server since there is no cert +// // two args: path to cert PEM file, the path to private key PEM file -// - creates a Config with a cert, using system CAs to validate the other end -// - use for: -// - a server; or, -// - a client that talks to a server with a public cert and needs certificate-based authentication -// - the other end will authenticate this end via the provided cert -// - the cert of the other end will be verified via system CAs +// - creates a Config with a cert, using system CAs to validate the other end +// - use for: +// - a server; or, +// - a client that talks to a server with a public cert and needs certificate-based authentication +// - the other end will authenticate this end via the provided cert +// - the cert of the other end will be verified via system CAs +// // three args: path to cert PEM file, path to client private key PEM file, path to CA PEM file -// - creates a Config with the cert, using specified CA to validate the other end -// - use for: -// - a server; or, -// - a client that talks to a server with a privately signed cert and needs certificate-based -// authentication -// - the other end will authenticate this end via the provided cert -// - this end will verify the other end's cert using the specified CA +// - creates a Config with the cert, using specified CA to validate the other end +// - use for: +// - a server; or, +// - a client that talks to a server with a privately signed cert and needs certificate-based +// authentication +// - the other end will authenticate this end via the provided cert +// - this end will verify the other end's cert using the specified CA func NewTLSConfigFromArgs(args ...string) (*tls.Config, error) { var err error var c *tls.Config From 80a4e7cc6495ee9cf1acd9540df8f742140e1aed Mon Sep 17 00:00:00 2001 From: Rich Date: Tue, 6 Dec 2022 10:44:35 +0000 Subject: [PATCH 4/9] Revert accidental FMT changes to TLS Signed-off-by: Rich --- plugin/pkg/tls/tls.go | 41 +++++++++++++++++++---------------------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/plugin/pkg/tls/tls.go b/plugin/pkg/tls/tls.go index 41eff4bc0d7..cba25503e00 100644 --- a/plugin/pkg/tls/tls.go +++ b/plugin/pkg/tls/tls.go @@ -31,31 +31,28 @@ func setTLSDefaults(ctls *tls.Config) { // in list of arguments. Typically these come straight from the // Corefile. // no args -// - creates a Config with no cert and using system CAs -// - use for a client that talks to a server with a public signed cert (CA installed in system) -// - the client will not be authenticated by the server since there is no cert -// +// - creates a Config with no cert and using system CAs +// - use for a client that talks to a server with a public signed cert (CA installed in system) +// - the client will not be authenticated by the server since there is no cert // one arg: the path to CA PEM file -// - creates a Config with no cert using a specific CA -// - use for a client that talks to a server with a private signed cert (CA not installed in system) -// - the client will not be authenticated by the server since there is no cert -// +// - creates a Config with no cert using a specific CA +// - use for a client that talks to a server with a private signed cert (CA not installed in system) +// - the client will not be authenticated by the server since there is no cert // two args: path to cert PEM file, the path to private key PEM file -// - creates a Config with a cert, using system CAs to validate the other end -// - use for: -// - a server; or, -// - a client that talks to a server with a public cert and needs certificate-based authentication -// - the other end will authenticate this end via the provided cert -// - the cert of the other end will be verified via system CAs -// +// - creates a Config with a cert, using system CAs to validate the other end +// - use for: +// - a server; or, +// - a client that talks to a server with a public cert and needs certificate-based authentication +// - the other end will authenticate this end via the provided cert +// - the cert of the other end will be verified via system CAs // three args: path to cert PEM file, path to client private key PEM file, path to CA PEM file -// - creates a Config with the cert, using specified CA to validate the other end -// - use for: -// - a server; or, -// - a client that talks to a server with a privately signed cert and needs certificate-based -// authentication -// - the other end will authenticate this end via the provided cert -// - this end will verify the other end's cert using the specified CA +// - creates a Config with the cert, using specified CA to validate the other end +// - use for: +// - a server; or, +// - a client that talks to a server with a privately signed cert and needs certificate-based +// authentication +// - the other end will authenticate this end via the provided cert +// - this end will verify the other end's cert using the specified CA func NewTLSConfigFromArgs(args ...string) (*tls.Config, error) { var err error var c *tls.Config From c0619725d8da46ae06121923bbfd586dbf47fbd1 Mon Sep 17 00:00:00 2001 From: Rich Date: Tue, 13 Dec 2022 13:08:38 +0000 Subject: [PATCH 5/9] Revise how plugin should be configured Signed-off-by: Rich --- core/dnsserver/config.go | 4 +- core/dnsserver/register.go | 4 +- core/dnsserver/server.go | 17 +++-- plugin/pkg/timeouts/timeouts.go | 54 ++++----------- plugin/pkg/timeouts/timeouts_test.go | 98 +++++++++------------------- plugin/timeouts/README.md | 54 ++++++++++----- plugin/timeouts/timeouts.go | 40 +++++++++--- plugin/timeouts/timeouts_test.go | 43 +++++++++--- 8 files changed, 161 insertions(+), 153 deletions(-) diff --git a/core/dnsserver/config.go b/core/dnsserver/config.go index db986fac4b6..9e11166503b 100644 --- a/core/dnsserver/config.go +++ b/core/dnsserver/config.go @@ -55,7 +55,9 @@ type Config struct { TLSConfig *tls.Config // Timeouts for TCP, TLS and HTTPS servers. - Timeouts map[string]time.Duration + ReadTimeout time.Duration + WriteTimeout time.Duration + IdleTimeout time.Duration // TSIG secrets, [name]key. TsigSecret map[string]string diff --git a/core/dnsserver/register.go b/core/dnsserver/register.go index 2141f69d497..2b673cab3f0 100644 --- a/core/dnsserver/register.go +++ b/core/dnsserver/register.go @@ -150,7 +150,9 @@ 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.ReadTimeout = c.firstConfigInBlock.ReadTimeout + c.WriteTimeout = c.firstConfigInBlock.WriteTimeout + c.IdleTimeout = c.firstConfigInBlock.IdleTimeout c.TsigSecret = c.firstConfigInBlock.TsigSecret } diff --git a/core/dnsserver/server.go b/core/dnsserver/server.go index 4cd522a48ea..b5177c3a387 100644 --- a/core/dnsserver/server.go +++ b/core/dnsserver/server.go @@ -88,15 +88,14 @@ func NewServer(addr string, group []*Config) (*Server, error) { 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 - } + if site.ReadTimeout != time.Duration(0) { + s.readTimeout = site.ReadTimeout + } + if site.WriteTimeout != time.Duration(0) { + s.writeTimeout = site.WriteTimeout + } + if site.IdleTimeout != time.Duration(0) { + s.idleTimeout = site.IdleTimeout } // copy tsig secrets diff --git a/plugin/pkg/timeouts/timeouts.go b/plugin/pkg/timeouts/timeouts.go index 5be71579cc8..f957bc07650 100644 --- a/plugin/pkg/timeouts/timeouts.go +++ b/plugin/pkg/timeouts/timeouts.go @@ -6,51 +6,25 @@ import ( "time" ) -// NewTimeoutsConfigFromArgs returns a map of durations keyed by strings based -// upon the passed in list of arguments. Typically these come straight from the -// Corefile. -// one arg: read timeout -// - only sets up the read timeout. Defaults will be used for write/idle timeouts. -// -// two args: read timeout, write timeout -// - sets up the read and write timeouts. Default will be used for idle timeout. -// -// three args: read timeout, write timeout, idle timeout -// - sets read write and idle timeouts. -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)) - } +// NewTimeoutFromArg 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. +func NewTimeoutFromArg(arg string) (time.Duration, error) { + _, err := strconv.Atoi(arg) + if err == nil { + // If no time unit is specified default to seconds rather than + // GO's default of nanoseconds. + arg = arg + "s" } - return c, nil -} - -func validateTimeout(t string) (time.Duration, error) { - i, err := strconv.Atoi(t) + d, err := time.ParseDuration(arg) if err != nil { - return time.Duration(0), fmt.Errorf("timeout provided '%s' does not appear to be numeric", t) + return time.Duration(0), fmt.Errorf("failed to parse timeout duration '%s'", arg) } - if i < 1 || i > 86400 { - return time.Duration(0), fmt.Errorf("timeout provided '%d' needs to be between 1 and 86400 second(s)", i) + if d < (1*time.Second) || d > (24*time.Hour) { + return time.Duration(0), fmt.Errorf("timeout provided '%s' needs to be between 1 second and 24 hours", arg) } - return time.Duration(i) * time.Second, nil + return d, nil } diff --git a/plugin/pkg/timeouts/timeouts_test.go b/plugin/pkg/timeouts/timeouts_test.go index 974d5f2a063..44a85efd5af 100644 --- a/plugin/pkg/timeouts/timeouts_test.go +++ b/plugin/pkg/timeouts/timeouts_test.go @@ -2,100 +2,64 @@ package timeouts import ( "testing" + "time" ) -func TestNewTimeoutsConfigFromArgs(t *testing.T) { - var validIdleTimeoutArg = "300" // Args from configuration are always strings - var validReadTimeoutArg = "30" - var validWriteTimeoutArg = "60" +func TestNewTimeoutFromArg(t *testing.T) { + var validSecondsTimeoutInput = "30s" + var validSecondsTimeoutOutput = time.Duration(30 * time.Second) + var validMinutesTimeoutInput = "2m" + var validMinutesTimeoutOutput = time.Duration(2 * time.Minute) + var validIntTimeoutInput = "30" + var validIntTimeoutOutput = time.Duration(30 * time.Second) var invalidTimeoutString = "twenty seconds" var invalidTimeoutLow = "0" - var invalidTimeoutHigh = "86401" + var invalidTimeoutHigh = "24h1s" - // No Arguments - to, err := NewTimeoutsConfigFromArgs() + // Valid timeout specified as Go duration (seconds) + to, err := NewTimeoutFromArg(validSecondsTimeoutInput) if err != nil { - t.Errorf("Failed to create timeouts map when no arguments specified: %s", err) + t.Errorf("Failed to create timeout duration given a valid Go duration in seconds: %s", err) } - if len(to) != 0 { - t.Error("Timeouts map with no arguments should be empty") + if to != time.Duration(validSecondsTimeoutOutput) { + t.Errorf("Timeout created given a valid Go duration in seconds appears to have unexpected value: %s", to) } - // Read Timeout only - to, err = NewTimeoutsConfigFromArgs(validReadTimeoutArg) + // Valid timeout specified as Go duration (minutes) + to, err = NewTimeoutFromArg(validMinutesTimeoutInput) if err != nil { - t.Errorf("Failed to create timeouts map given just a read timeout: %s", err) + t.Errorf("Failed to create timeout duration given a valid Go duration in minutes: %s", err) } - if _, ok := to["read"]; !ok { - t.Error("Timeouts map given just a read timeout did not return a read timeout") + if to != time.Duration(validMinutesTimeoutOutput) { + t.Errorf("Timeout created given a valid Go duration in minutes appears to have unexpected value: %s", to) } - 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) + // Valid timeout specified as int + to, err = NewTimeoutFromArg(validIntTimeoutInput) 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") + t.Errorf("Failed to create timeout duration given a valid int: %s", err) } - 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") + if to != time.Duration(validIntTimeoutOutput) { + t.Errorf("Timeout created given a valid int appears to have unexpected value: %s", to) } - // Timeout Validation - to, err = NewTimeoutsConfigFromArgs(invalidTimeoutString) + // Invalid timeouts + to, err = NewTimeoutFromArg(invalidTimeoutString) if err == nil { - t.Error("Attempt to create timeouts with non-numeric value was successful") + t.Error("Attempt to create timeout with non-numeric value was successful") } - to, err = NewTimeoutsConfigFromArgs(invalidTimeoutLow) + to, err = NewTimeoutFromArg(invalidTimeoutLow) if err == nil { - t.Error("Attempt to create a timeout of less than 1 second was successful") + t.Error("Attempt to create timeout of less than 1 second was successful") } - to, err = NewTimeoutsConfigFromArgs(invalidTimeoutHigh) + to, err = NewTimeoutFromArg(invalidTimeoutHigh) if err == nil { - t.Error("Attempt to create a timeout of more than 1 day was successful") + t.Error("Attempt to create timeout of more than 1 day was successful") } } diff --git a/plugin/timeouts/README.md b/plugin/timeouts/README.md index a157db700b4..098c9ccac7a 100644 --- a/plugin/timeouts/README.md +++ b/plugin/timeouts/README.md @@ -6,53 +6,71 @@ ## 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. +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. +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. +The *timeouts* "plugin" allows you to configure CoreDNS server read, write and +idle timeouts. ## Syntax ~~~ txt -timeouts READ [WRITE] [IDLE] +timeouts { + read DURATION + write DURATION + idle DURATION +} ~~~ -For any timeouts that are not provided, default values are used which vary depending on the server type. +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. +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 10 20 60 + timeouts { + read 10s + write 20s + idle 60s + } forward . /etc/resolv.conf } ~~~ -Start a DNS-over-gRPC server that is similar to the previous example, but using DNS-over-HTTPS for -incoming queries. Only the read timeout has been configured. +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 30 + 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. +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 10 10 + timeouts { + read 15s + write 30s + } forward . /etc/resolv.conf } ~~~ diff --git a/plugin/timeouts/timeouts.go b/plugin/timeouts/timeouts.go index 98e2e034b5c..baa514fc92f 100644 --- a/plugin/timeouts/timeouts.go +++ b/plugin/timeouts/timeouts.go @@ -20,22 +20,44 @@ func setup(c *caddy.Controller) error { func parseTimeouts(c *caddy.Controller) error { config := dnsserver.GetConfig(c) - if config.Timeouts != nil { - return plugin.Error("timeouts", c.Errf("Timeouts already configured for this server instance")) - } - for c.Next() { args := c.RemainingArgs() - if len(args) < 1 || len(args) > 3 { + if len(args) > 0 { return plugin.Error("timeouts", c.ArgErr()) } - timeoutsConfig, err := timeouts.NewTimeoutsConfigFromArgs(args...) - if err != nil { - return err + b := 0 + for c.NextBlock() { + block := c.Val() + timeoutArgs := c.RemainingArgs() + if len(timeoutArgs) != 1 { + return c.ArgErr() + } + + timeout, err := timeouts.NewTimeoutFromArg(timeoutArgs[0]) + if err != nil { + return err + } + + 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++ } - config.Timeouts = timeoutsConfig + if b == 0 { + return plugin.Error("timeouts", c.Err("timeouts block with no timeouts specified")) + } } return nil } diff --git a/plugin/timeouts/timeouts_test.go b/plugin/timeouts/timeouts_test.go index 6c8b888fd4e..a618968d2f4 100644 --- a/plugin/timeouts/timeouts_test.go +++ b/plugin/timeouts/timeouts_test.go @@ -15,15 +15,42 @@ func TestTimeouts(t *testing.T) { expectedErrContent string // substring from the expected error. Empty for positive cases. }{ // positive - {"timeouts 30", false, "", ""}, - {"timeouts 30 60", false, "", ""}, - {"timeouts 30 60 300", false, "", ""}, + {`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, "", "Wrong argument"}, - {"timeouts 30 60 300 600", true, "", "Wrong argument"}, - {"timeouts ten", true, "", "timeout provided 'ten' does not appear to be numeric"}, - {"timeouts 0", true, "", "timeout provided '0' needs to be between 1 and 86400 second(s)"}, - {"timeouts 86401", true, "", "timeout provided '86401' needs to be between 1 and 86400 second(s)"}, + {`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 timeout duration"}, + {`timeouts { + idle 0s + }`, true, "", "needs to be between"}, + {`timeouts { + read 48h + }`, true, "", "needs to be between"}, } for i, test := range tests { From 8bb15da5d32505fe60338f38c8e65d14894b2fb7 Mon Sep 17 00:00:00 2001 From: Rich Date: Thu, 15 Dec 2022 09:16:11 +0000 Subject: [PATCH 6/9] Fix lint errors Signed-off-by: Rich --- plugin/pkg/timeouts/timeouts_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plugin/pkg/timeouts/timeouts_test.go b/plugin/pkg/timeouts/timeouts_test.go index 44a85efd5af..d19dfe023af 100644 --- a/plugin/pkg/timeouts/timeouts_test.go +++ b/plugin/pkg/timeouts/timeouts_test.go @@ -48,17 +48,17 @@ func TestNewTimeoutFromArg(t *testing.T) { } // Invalid timeouts - to, err = NewTimeoutFromArg(invalidTimeoutString) + _, err = NewTimeoutFromArg(invalidTimeoutString) if err == nil { t.Error("Attempt to create timeout with non-numeric value was successful") } - to, err = NewTimeoutFromArg(invalidTimeoutLow) + _, err = NewTimeoutFromArg(invalidTimeoutLow) if err == nil { t.Error("Attempt to create timeout of less than 1 second was successful") } - to, err = NewTimeoutFromArg(invalidTimeoutHigh) + _, err = NewTimeoutFromArg(invalidTimeoutHigh) if err == nil { t.Error("Attempt to create timeout of more than 1 day was successful") } From ec7686aa5c5bbe0880b190c5fa6066667ba0c876 Mon Sep 17 00:00:00 2001 From: Rich Date: Fri, 16 Dec 2022 10:34:24 +0000 Subject: [PATCH 7/9] Remove boxing that is not required and fix error surfacing Signed-off-by: Rich --- core/dnsserver/server.go | 6 +++--- plugin/pkg/timeouts/timeouts.go | 4 ++-- plugin/pkg/timeouts/timeouts_test.go | 12 ++++++------ plugin/timeouts/timeouts.go | 2 +- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/core/dnsserver/server.go b/core/dnsserver/server.go index b5177c3a387..2107e8d01be 100644 --- a/core/dnsserver/server.go +++ b/core/dnsserver/server.go @@ -88,13 +88,13 @@ func NewServer(addr string, group []*Config) (*Server, error) { s.zones[site.Zone] = append(s.zones[site.Zone], site) // set timeouts - if site.ReadTimeout != time.Duration(0) { + if site.ReadTimeout != 0 { s.readTimeout = site.ReadTimeout } - if site.WriteTimeout != time.Duration(0) { + if site.WriteTimeout != 0 { s.writeTimeout = site.WriteTimeout } - if site.IdleTimeout != time.Duration(0) { + if site.IdleTimeout != 0 { s.idleTimeout = site.IdleTimeout } diff --git a/plugin/pkg/timeouts/timeouts.go b/plugin/pkg/timeouts/timeouts.go index f957bc07650..ffc88a7ecf2 100644 --- a/plugin/pkg/timeouts/timeouts.go +++ b/plugin/pkg/timeouts/timeouts.go @@ -19,11 +19,11 @@ func NewTimeoutFromArg(arg string) (time.Duration, error) { d, err := time.ParseDuration(arg) if err != nil { - return time.Duration(0), fmt.Errorf("failed to parse timeout duration '%s'", arg) + return 0, fmt.Errorf("failed to parse timeout duration '%s'", arg) } if d < (1*time.Second) || d > (24*time.Hour) { - return time.Duration(0), fmt.Errorf("timeout provided '%s' needs to be between 1 second and 24 hours", arg) + return 0, fmt.Errorf("timeout provided '%s' needs to be between 1 second and 24 hours", arg) } return d, nil diff --git a/plugin/pkg/timeouts/timeouts_test.go b/plugin/pkg/timeouts/timeouts_test.go index d19dfe023af..d00a7f31f00 100644 --- a/plugin/pkg/timeouts/timeouts_test.go +++ b/plugin/pkg/timeouts/timeouts_test.go @@ -7,11 +7,11 @@ import ( func TestNewTimeoutFromArg(t *testing.T) { var validSecondsTimeoutInput = "30s" - var validSecondsTimeoutOutput = time.Duration(30 * time.Second) + var validSecondsTimeoutOutput = 30 * time.Second var validMinutesTimeoutInput = "2m" - var validMinutesTimeoutOutput = time.Duration(2 * time.Minute) + var validMinutesTimeoutOutput = 2 * time.Minute var validIntTimeoutInput = "30" - var validIntTimeoutOutput = time.Duration(30 * time.Second) + var validIntTimeoutOutput = 30 * time.Second var invalidTimeoutString = "twenty seconds" var invalidTimeoutLow = "0" @@ -23,7 +23,7 @@ func TestNewTimeoutFromArg(t *testing.T) { t.Errorf("Failed to create timeout duration given a valid Go duration in seconds: %s", err) } - if to != time.Duration(validSecondsTimeoutOutput) { + if to != validSecondsTimeoutOutput { t.Errorf("Timeout created given a valid Go duration in seconds appears to have unexpected value: %s", to) } @@ -33,7 +33,7 @@ func TestNewTimeoutFromArg(t *testing.T) { t.Errorf("Failed to create timeout duration given a valid Go duration in minutes: %s", err) } - if to != time.Duration(validMinutesTimeoutOutput) { + if to != validMinutesTimeoutOutput { t.Errorf("Timeout created given a valid Go duration in minutes appears to have unexpected value: %s", to) } @@ -43,7 +43,7 @@ func TestNewTimeoutFromArg(t *testing.T) { t.Errorf("Failed to create timeout duration given a valid int: %s", err) } - if to != time.Duration(validIntTimeoutOutput) { + if to != validIntTimeoutOutput { t.Errorf("Timeout created given a valid int appears to have unexpected value: %s", to) } diff --git a/plugin/timeouts/timeouts.go b/plugin/timeouts/timeouts.go index baa514fc92f..ec1affffb8f 100644 --- a/plugin/timeouts/timeouts.go +++ b/plugin/timeouts/timeouts.go @@ -36,7 +36,7 @@ func parseTimeouts(c *caddy.Controller) error { timeout, err := timeouts.NewTimeoutFromArg(timeoutArgs[0]) if err != nil { - return err + return c.Err(err.Error()) } switch block { From d48f9fc969f95c8ebb8e908e9bc7166c96e7b49d Mon Sep 17 00:00:00 2001 From: Rich Date: Fri, 16 Dec 2022 15:00:42 +0000 Subject: [PATCH 8/9] Rename timeouts extra pkg to durations. Adopt table testing example Signed-off-by: Rich --- plugin/pkg/durations/durations.go | 26 +++++++++++ plugin/pkg/durations/durations_test.go | 51 ++++++++++++++++++++ plugin/pkg/timeouts/timeouts.go | 30 ------------ plugin/pkg/timeouts/timeouts_test.go | 65 -------------------------- plugin/timeouts/timeouts.go | 10 +++- plugin/timeouts/timeouts_test.go | 2 +- 6 files changed, 86 insertions(+), 98 deletions(-) create mode 100644 plugin/pkg/durations/durations.go create mode 100644 plugin/pkg/durations/durations_test.go delete mode 100644 plugin/pkg/timeouts/timeouts.go delete mode 100644 plugin/pkg/timeouts/timeouts_test.go diff --git a/plugin/pkg/durations/durations.go b/plugin/pkg/durations/durations.go new file mode 100644 index 00000000000..37771e79d3a --- /dev/null +++ b/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 +} diff --git a/plugin/pkg/durations/durations_test.go b/plugin/pkg/durations/durations_test.go new file mode 100644 index 00000000000..d1371b2dde1 --- /dev/null +++ b/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 timeout", + 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) + } + }) + } +} diff --git a/plugin/pkg/timeouts/timeouts.go b/plugin/pkg/timeouts/timeouts.go deleted file mode 100644 index ffc88a7ecf2..00000000000 --- a/plugin/pkg/timeouts/timeouts.go +++ /dev/null @@ -1,30 +0,0 @@ -package timeouts - -import ( - "fmt" - "strconv" - "time" -) - -// NewTimeoutFromArg 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. -func NewTimeoutFromArg(arg string) (time.Duration, error) { - _, err := strconv.Atoi(arg) - if err == nil { - // If no time unit is specified default to seconds rather than - // GO's default of nanoseconds. - arg = arg + "s" - } - - d, err := time.ParseDuration(arg) - if err != nil { - return 0, fmt.Errorf("failed to parse timeout duration '%s'", arg) - } - - if d < (1*time.Second) || d > (24*time.Hour) { - return 0, fmt.Errorf("timeout provided '%s' needs to be between 1 second and 24 hours", arg) - } - - return d, nil -} diff --git a/plugin/pkg/timeouts/timeouts_test.go b/plugin/pkg/timeouts/timeouts_test.go deleted file mode 100644 index d00a7f31f00..00000000000 --- a/plugin/pkg/timeouts/timeouts_test.go +++ /dev/null @@ -1,65 +0,0 @@ -package timeouts - -import ( - "testing" - "time" -) - -func TestNewTimeoutFromArg(t *testing.T) { - var validSecondsTimeoutInput = "30s" - var validSecondsTimeoutOutput = 30 * time.Second - var validMinutesTimeoutInput = "2m" - var validMinutesTimeoutOutput = 2 * time.Minute - var validIntTimeoutInput = "30" - var validIntTimeoutOutput = 30 * time.Second - - var invalidTimeoutString = "twenty seconds" - var invalidTimeoutLow = "0" - var invalidTimeoutHigh = "24h1s" - - // Valid timeout specified as Go duration (seconds) - to, err := NewTimeoutFromArg(validSecondsTimeoutInput) - if err != nil { - t.Errorf("Failed to create timeout duration given a valid Go duration in seconds: %s", err) - } - - if to != validSecondsTimeoutOutput { - t.Errorf("Timeout created given a valid Go duration in seconds appears to have unexpected value: %s", to) - } - - // Valid timeout specified as Go duration (minutes) - to, err = NewTimeoutFromArg(validMinutesTimeoutInput) - if err != nil { - t.Errorf("Failed to create timeout duration given a valid Go duration in minutes: %s", err) - } - - if to != validMinutesTimeoutOutput { - t.Errorf("Timeout created given a valid Go duration in minutes appears to have unexpected value: %s", to) - } - - // Valid timeout specified as int - to, err = NewTimeoutFromArg(validIntTimeoutInput) - if err != nil { - t.Errorf("Failed to create timeout duration given a valid int: %s", err) - } - - if to != validIntTimeoutOutput { - t.Errorf("Timeout created given a valid int appears to have unexpected value: %s", to) - } - - // Invalid timeouts - _, err = NewTimeoutFromArg(invalidTimeoutString) - if err == nil { - t.Error("Attempt to create timeout with non-numeric value was successful") - } - - _, err = NewTimeoutFromArg(invalidTimeoutLow) - if err == nil { - t.Error("Attempt to create timeout of less than 1 second was successful") - } - - _, err = NewTimeoutFromArg(invalidTimeoutHigh) - if err == nil { - t.Error("Attempt to create timeout of more than 1 day was successful") - } -} diff --git a/plugin/timeouts/timeouts.go b/plugin/timeouts/timeouts.go index ec1affffb8f..eea6a64885a 100644 --- a/plugin/timeouts/timeouts.go +++ b/plugin/timeouts/timeouts.go @@ -1,10 +1,12 @@ 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/timeouts" + "github.com/coredns/coredns/plugin/pkg/durations" ) func init() { plugin.Register("timeouts", setup) } @@ -34,11 +36,15 @@ func parseTimeouts(c *caddy.Controller) error { return c.ArgErr() } - timeout, err := timeouts.NewTimeoutFromArg(timeoutArgs[0]) + 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 diff --git a/plugin/timeouts/timeouts_test.go b/plugin/timeouts/timeouts_test.go index a618968d2f4..c01d3a0725d 100644 --- a/plugin/timeouts/timeouts_test.go +++ b/plugin/timeouts/timeouts_test.go @@ -44,7 +44,7 @@ func TestTimeouts(t *testing.T) { }`, true, "", "Wrong argument"}, {`timeouts { write snake - }`, true, "", "failed to parse timeout duration"}, + }`, true, "", "failed to parse duration"}, {`timeouts { idle 0s }`, true, "", "needs to be between"}, From fc2788ce3bf31c09b2e3ca87fd244258a1bd76d9 Mon Sep 17 00:00:00 2001 From: Rich Date: Fri, 16 Dec 2022 15:02:02 +0000 Subject: [PATCH 9/9] remove reference to timeouts from duration tests Signed-off-by: Rich --- plugin/pkg/durations/durations_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/pkg/durations/durations_test.go b/plugin/pkg/durations/durations_test.go index d1371b2dde1..12008a7134d 100644 --- a/plugin/pkg/durations/durations_test.go +++ b/plugin/pkg/durations/durations_test.go @@ -28,7 +28,7 @@ func TestNewDurationFromArg(t *testing.T) { want: 30 * time.Second, }, { - name: "invalid timeout", + name: "invalid duration", arg: "twenty seconds", wantErr: true, },