WIP: Follow up to https://github.com/openshift/router/pull/483

pkg/router/template/template_helper.go

@@ -16,18 +16,33 @@ import (
 	"time"
 
 	routev1 "github.com/openshift/api/route/v1"
-
 	"github.com/openshift/router/pkg/router/routeapihelpers"
 	templateutil "github.com/openshift/router/pkg/router/template/util"
 	haproxyutil "github.com/openshift/router/pkg/router/template/util/haproxy"
+	"github.com/openshift/router/pkg/router/template/util/haproxytime"
 )
 
 const (
 	certConfigMap = "cert_config.map"
-	// max timeout allowable by HAProxy
-	haproxyMaxTimeout = "2147483647ms"
 )
 
+// haproxyMaxTimeout stores the maximum timeout value parsed from the
+// HAProxy configuration. It is initialised in the init() function to
+// ensure that the value is a valid HAProxy duration.
+var haproxyMaxTimeout time.Duration
+
+// init initializes the haproxyMaxTimeout variable by parsing the
+// value of templateutil.HaproxyMaxTimeout. It panics if the value is
+// not a valid HAProxy time duration, serving as a safeguard against
+// invalid configuration changes.
+func init() {
+	duration, err := haproxytime.ParseDuration(templateutil.HaproxyMaxTimeout)
+	if err != nil {
+		panic(err)
+	}
+	haproxyMaxTimeout = duration
+}
+
 func isTrue(s string) bool {
 	v, _ := strconv.ParseBool(s)
 	return v
@@ -321,41 +336,39 @@ func generateHAProxyMap(name string, td templateData) []string {
 
 // clipHAProxyTimeoutValue prevents the HAProxy config file
 // from using timeout values specified via the haproxy.router.openshift.io/timeout
-// annotation that exceed the maximum value allowed by HAProxy.
-// Return the parameter string instead of an err in the event that a
+// annotation that exceed the maximum value allowed by HAProxy, or by time.ParseDuration.
+// Return the empty string instead of an err in the event that a
 // timeout string value is not parsable as a valid time duration.
 func clipHAProxyTimeoutValue(val string) string {
 	// If the empty string is passed in,
 	// simply return the empty string.
 	if len(val) == 0 {
 		return val
 	}
-	endIndex := len(val) - 1
-	maxTimeout, err := time.ParseDuration(haproxyMaxTimeout)
+
+	// First check to see if the timeout will fit into a time.Duration
+	duration, err := haproxytime.ParseDuration(val)
 	if err != nil {
-		return val
-	}
-	// time.ParseDuration doesn't work with days
-	// despite HAProxy accepting timeouts that specify day units
-	if val[endIndex] == 'd' {
-		days, err := strconv.Atoi(val[:endIndex])
-		if err != nil {
-			return val
-		}
-		if maxTimeout.Hours() < float64(days*24) {
-			log.V(7).Info("Route annotation timeout exceeds maximum allowable by HAProxy, clipping to max")
-			return haproxyMaxTimeout
-		}
-	} else {
-		duration, err := time.ParseDuration(val)
-		if err != nil {
-			return val
-		}
-		if maxTimeout.Milliseconds() < duration.Milliseconds() {
-			log.V(7).Info("Route annotation timeout exceeds maximum allowable by HAProxy, clipping to max")
-			return haproxyMaxTimeout
+		switch err {
+		case haproxytime.OverflowError:
+			log.Info("route annotation timeout exceeds maximum allowable by HAProxy, clipping to max", "max", templateutil.HaproxyMaxTimeout)
+			return templateutil.HaproxyMaxTimeout
+		case haproxytime.SyntaxError:
+			log.Error(err, "route annotation timeout removed because input is invalid")
+			return ""
+		default:
+			// This is not used at the moment
+			log.Info("invalid route annotation timeout, setting to", "default", templateutil.HaproxyDefaultTimeout)
+			return templateutil.HaproxyDefaultTimeout
 		}
 	}
+
+	// Then check to see if the timeout is larger than what HAProxy allows.
+	if duration > haproxyMaxTimeout {
+		log.Info("Route annotation timeout exceeds maximum allowable by HAProxy, clipping to max", "max", templateutil.HaproxyMaxTimeout)
+		return templateutil.HaproxyMaxTimeout
+	}
+
 	return val
 }
 

pkg/router/template/template_helper_test.go

@@ -12,6 +12,7 @@ import (
 	"testing"
 
 	routev1 "github.com/openshift/api/route/v1"
+	templateutil "github.com/openshift/router/pkg/router/template/util"
 )
 
 func buildServiceAliasConfig(name, namespace, host, path string, termination routev1.TLSTerminationType, policy routev1.InsecureEdgeTerminationPolicyType, wildcard bool) ServiceAliasConfig {
@@ -790,6 +791,15 @@ func TestClipHAProxyTimeoutValue(t *testing.T) {
 			value:    "",
 			expected: "",
 		},
+		{
+			value:    "0",
+			expected: "0",
+		},
+		{
+			value:    "s",
+			expected: "",
+			// Invalid input produces blank output
+		},
 		{
 			value:    "10",
 			expected: "10",
@@ -804,11 +814,79 @@ func TestClipHAProxyTimeoutValue(t *testing.T) {
 		},
 		{
 			value:    "100d",
-			expected: haproxyMaxTimeout,
+			expected: templateutil.HaproxyMaxTimeout,
 		},
 		{
 			value:    "1000h",
-			expected: haproxyMaxTimeout,
+			expected: templateutil.HaproxyMaxTimeout,
+		},
+		{
+			value:    "+-+",
+			expected: "",
+			// Invalid input produces blank output
+		},
+		{
+			value:    "1.5.8.9",
+			expected: "",
+			// Invalid input produces blank output
+		},
+		{
+			value:    "1.5s",
+			expected: "",
+			// Invalid input produces blank output
+		},
+		{
+			value:    "9223372036855ms",
+			expected: templateutil.HaproxyMaxTimeout,
+			// Exceeds the time.ParseDuration maximum
+		},
+		{
+			value:    "2147483647ms",
+			expected: "2147483647ms",
+		},
+		{
+			value:    "922337203685477581ms",
+			expected: templateutil.HaproxyMaxTimeout,
+			// Overflow error > 1<<63/10 + 1
+		},
+		{
+			value:    "2562047.99h",
+			expected: "",
+			// Invalid input produces blank output
+		},
+		{
+			value:    "100000000000s",
+			expected: templateutil.HaproxyMaxTimeout,
+			// Exceeds the time.ParseDuration maximum
+		},
+		{
+			value:    "9999999999999999",
+			expected: templateutil.HaproxyMaxTimeout,
+			// Exceeds the time.ParseDuration maximum and has no unit
+		},
+		{
+			value:    "25d",
+			expected: templateutil.HaproxyMaxTimeout,
+			// Exceeds the HAProxy maximum
+		},
+		{
+			value:    "24d",
+			expected: "24d",
+		},
+		{
+			value:    "24d1",
+			expected: "",
+			// Invalid input produces blank output
+		},
+		{
+			value:    "1d12h",
+			expected: "",
+			// Invalid input produces blank output
+		},
+		{
+			value:    "foo",
+			expected: "",
+			// Invalid input produces blank output
 		},
 	}
 	for _, tc := range testCases {

pkg/router/template/util/haproxytime/benchmark_test.go

@@ -0,0 +1,42 @@
+package haproxytime_test
+
+import (
+	"testing"
+
+	"github.com/openshift/router/pkg/router/template/util/haproxytime"
+)
+
+// Run as: go test -bench=. -test.run=BenchmarkParseDuration -benchmem -count=1 -benchtime=1s
+//
+//	% go test -bench=. -test.run=BenchmarkParseDuration -benchmem -count=1 -benchtime=1s
+//	goos: linux
+//	goarch: amd64
+//	pkg: github.com/openshift/router/pkg/router/template/util/haproxytime
+//	cpu: 11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz
+//	BenchmarkParseDuration-8   	 4414666	       275.6 ns/op	     112 B/op	       2 allocs/op
+//	PASS
+//	ok  	github.com/openshift/router/pkg/router/template/util/haproxytime	1.495s
+//
+// If you modify ParseDuration() and fictitiously remove the regexp
+// and the associated handling of numericPart and unitPart and just
+// assume the numericPart=input then the following results show the
+// cost of parsing with regular expressions.
+//
+//	% go test -bench=. -test.run=BenchmarkParseDuration -benchmem -count=1 -benchtime=1s
+//	goos: linux
+//	goarch: amd64
+//	pkg: github.com/openshift/router/pkg/router/template/util/haproxytime
+//	cpu: 11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz
+//	BenchmarkParseDuration-8   	72849655	        16.44 ns/op	       0 B/op	       0 allocs/op
+//	PASS
+//	ok  	github.com/openshift/router/pkg/router/template/util/haproxytime	1.217s
+func BenchmarkParseDuration(b *testing.B) {
+	b.ResetTimer()
+
+	for i := 0; i < b.N; i++ {
+		_, err := haproxytime.ParseDuration("2147483647")
+		if err != nil {
+			b.Fatal(err)
+		}
+	}
+}

pkg/router/template/util/haproxytime/duration_parser.go

@@ -0,0 +1,71 @@
+package haproxytime
+
+import (
+	"errors"
+	"math"
+	"regexp"
+	"strconv"
+	"time"
+)
+
+var (
+	// OverflowError is returned when the parsed value exceeds the
+	// maximum allowed.
+	OverflowError = errors.New("overflow")
+
+	// SyntaxError is returned when the input string doesn't match
+	// HAProxy's duration format.
+	SyntaxError = errors.New("invalid duration")
+
+	durationRE = regexp.MustCompile(`^([0-9]+)(us|ms|s|m|h|d)?$`)
+)
+
+// ParseDuration takes a string representing a duration in HAProxy's
+// specific format, which permits days ("d"), and converts it into a
+// time.Duration value. The input string can include an optional unit
+// suffix, such as "us", "ms", "s", "m", "h", or "d". If no suffix is
+// provided, milliseconds are assumed. The function returns an
+// OverflowError if the value would result in a 64-bit integer
+// overflow, or a SyntaxError if the input string doesn't match the
+// expected format.
+func ParseDuration(input string) (time.Duration, error) {
+	matches := durationRE.FindStringSubmatch(input)
+	if matches == nil {
+		return 0, SyntaxError
+	}
+
+	// Default unit is milliseconds, unless specified.
+	unit := time.Millisecond
+
+	numericPart := matches[1]
+	unitPart := ""
+	if len(matches) > 2 {
+		unitPart = matches[2]
+	}
+
+	switch unitPart {
+	case "us":
+		unit = time.Microsecond
+	case "ms":
+		unit = time.Millisecond
+	case "s":
+		unit = time.Second
+	case "m":
+		unit = time.Minute
+	case "h":
+		unit = time.Hour
+	case "d":
+		unit = 24 * time.Hour
+	}
+
+	value, err := strconv.ParseInt(numericPart, 10, 64)
+	if err != nil {
+		return 0, OverflowError
+	}
+
+	if value > math.MaxInt64/int64(unit) {
+		return 0, OverflowError
+	}
+
+	return time.Duration(value) * unit, nil
+}