plugin/timeouts: add maxtcpqueries option to bound queries per TCP/TLS connection (#8376)

plugin/timeouts/README.md

@@ -2,7 +2,7 @@
 
 ## Name
 
-*timeouts* - allows you to configure the supported server read, write and idle timeouts for the TCP, TLS, DoH and DoQ servers.
+*timeouts* - allows you to configure the supported server read, write and idle timeouts for the TCP, TLS, DoH and DoQ servers, and the maximum number of queries served on a single TCP or TLS connection.
 
 ## Description
 
@@ -15,7 +15,8 @@ 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.
+idle timeouts, and the maximum number of queries CoreDNS will serve on a single
+TCP or TLS connection before closing it.
 
 ## Syntax
 
@@ -24,13 +25,24 @@ timeouts {
 	read DURATION
 	write DURATION
 	idle DURATION
+	maxtcpqueries MAXIMUM
 }
 ~~~
 
 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
+depending on the server type. At least one option must be specified otherwise
 the entire timeouts block should be omitted.
 
+* `maxtcpqueries` sets the maximum number of queries served on a single TCP or
+  TLS connection before CoreDNS closes it. **MAXIMUM** must be a positive
+  integer, or `-1` to allow an unlimited number of queries per connection
+  (the default). Long-lived connections that serve an unlimited number of
+  queries can cause uneven load distribution across CoreDNS replicas that sit
+  behind a connection-based load balancer, since new queries keep reusing the
+  same connection instead of establishing a new one. Setting a bound, e.g.
+  `maxtcpqueries 128`, forces clients to periodically reconnect, which allows
+  the load balancer to redistribute load.
+
 The configured timeouts apply where the selected server transport supports
 them. TCP, TLS and DoH servers use the read, write and idle timeouts. DoQ
 servers use the read timeout to bound receiving a query on an opened QUIC
@@ -94,3 +106,16 @@ configured. The timeouts are only applied to the TCP side of the server.
 	forward . /etc/resolv.conf
 }
 ~~~
+
+Start a standard TCP/UDP server that closes a TCP connection after it has
+served 128 queries, to help spread load evenly across replicas sitting behind
+a connection-based load balancer.
+
+~~~
+. {
+	timeouts {
+		maxtcpqueries 128
+	}
+	forward . /etc/resolv.conf
+}
+~~~

plugin/timeouts/timeouts.go

@@ -1,6 +1,7 @@
 package timeouts
 
 import (
+	"strconv"
 	"time"
 
 	"github.com/coredns/caddy"
@@ -36,6 +37,19 @@ func parseTimeouts(c *caddy.Controller) error {
 				return c.ArgErr()
 			}
 
+			if block == "maxtcpqueries" {
+				n, err := strconv.Atoi(timeoutArgs[0])
+				if err != nil {
+					return c.Errf("invalid value for maxtcpqueries '%s': %v", timeoutArgs[0], err)
+				}
+				if n == 0 || n < -1 {
+					return c.Errf("maxtcpqueries provided '%d' needs to be -1 (unlimited) or a positive integer", n)
+				}
+				config.MaxTCPQueries = &n
+				b++
+				continue
+			}
+
 			timeout, err := durations.NewDurationFromArg(timeoutArgs[0])
 			if err != nil {
 				return c.Err(err.Error())

plugin/timeouts/timeouts_test.go

@@ -5,58 +5,81 @@ import (
 	"testing"
 
 	"github.com/coredns/caddy"
+	"github.com/coredns/coredns/core/dnsserver"
 )
 
 func TestTimeouts(t *testing.T) {
+	n128, nUnlimited := 128, -1
+
 	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.
+		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.
+		expectedMaxTCPQueries *int   // expected Config.MaxTCPQueries after setup. nil means left unset.
 	}{
 		// positive
 		{`timeouts {
 			read 30s
-		}`, false, "", ""},
+		}`, false, "", "", nil},
 		{`timeouts {
 			read 1m
 			write 2m
-		}`, false, "", ""},
+		}`, false, "", "", nil},
 		{` timeouts {
 			idle 1h
-		}`, false, "", ""},
+		}`, false, "", "", nil},
 		{`timeouts {
 			read 10
 			write 20
 			idle 60
-		}`, false, "", ""},
-		// negative
-		{`timeouts`, true, "", "block with no timeouts specified"},
+		}`, false, "", "", nil},
 		{`timeouts {
-		}`, true, "", "block with no timeouts specified"},
+			maxtcpqueries 128
+		}`, false, "", "", &n128},
+		{`timeouts {
+			maxtcpqueries -1
+		}`, false, "", "", &nUnlimited},
+		{`timeouts {
+			read 10s
+			maxtcpqueries 128
+		}`, false, "", "", &n128},
+		// negative
+		{`timeouts`, true, "", "block with no timeouts specified", nil},
+		{`timeouts {
+		}`, true, "", "block with no timeouts specified", nil},
 		{`timeouts {
 			read 10s
 			giraffe 30s
-		}`, true, "", "unknown option"},
+		}`, true, "", "unknown option", nil},
 		{`timeouts {
 			read 10s 20s
 			write 30s
-		}`, true, "", "Wrong argument"},
+		}`, true, "", "Wrong argument", nil},
 		{`timeouts {
 			write snake
-		}`, true, "", "failed to parse duration"},
+		}`, true, "", "failed to parse duration", nil},
 		{`timeouts {
 			idle 0s
-		}`, true, "", "needs to be between"},
+		}`, true, "", "needs to be between", nil},
 		{`timeouts {
 			read 48h
-		}`, true, "", "needs to be between"},
+		}`, true, "", "needs to be between", nil},
+		{`timeouts {
+			maxtcpqueries 0
+		}`, true, "", "needs to be -1", nil},
+		{`timeouts {
+			maxtcpqueries -2
+		}`, true, "", "needs to be -1", nil},
+		{`timeouts {
+			maxtcpqueries snake
+		}`, true, "", "invalid value for maxtcpqueries", nil},
 	}
 
 	for i, test := range tests {
 		c := caddy.NewTestController("dns", test.input)
 		err := setup(c)
-		//cfg := dnsserver.GetConfig(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)
@@ -70,6 +93,18 @@ func TestTimeouts(t *testing.T) {
 			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)
 			}
+			continue
+		}
+
+		switch {
+		case test.expectedMaxTCPQueries == nil && cfg.MaxTCPQueries != nil:
+			t.Errorf("Test %d: Expected Config.MaxTCPQueries to remain unset for input %s, got %d", i, test.input, *cfg.MaxTCPQueries)
+		case test.expectedMaxTCPQueries != nil:
+			if cfg.MaxTCPQueries == nil {
+				t.Errorf("Test %d: Expected Config.MaxTCPQueries to be %d for input %s, got unset", i, *test.expectedMaxTCPQueries, test.input)
+			} else if *cfg.MaxTCPQueries != *test.expectedMaxTCPQueries {
+				t.Errorf("Test %d: Expected Config.MaxTCPQueries to be %d for input %s, got %d", i, *test.expectedMaxTCPQueries, test.input, *cfg.MaxTCPQueries)
+			}
 		}
 	}
 }