OCPBUGS-6958: Fix clipHAProxyTimeoutValue

pkg/router/template/template_helper.go

@@ -13,19 +13,16 @@ import (
 	"strings"
 	"sync"
 	"text/template"
-	"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"
 )
 
 func isTrue(s string) bool {
@@ -321,41 +318,42 @@ 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
+// haproxytime.ParseDuration.
+//
+// Return the empty string instead of an error in the event that a
 // timeout string value is not parsable as a valid time duration.
+// Return the largest HAProxy timeout if the input value exceeds it.
+// Return the default timeout (5s) if there is another error.
 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)
-	if err != nil {
-		return val
+
+	// First check to see if the value is syntactically correct and fits into a time.Duration.
+	duration, err := haproxytime.ParseDuration(val)
+	switch err {
+	case nil:
+	case haproxytime.OverflowError:
+		log.Info("route annotation timeout exceeds maximum allowable format, clipping to "+templateutil.HaproxyMaxTimeout, "input", val)
+		return templateutil.HaproxyMaxTimeout
+	case haproxytime.SyntaxError:
+		log.Error(err, "route annotation timeout ignored because value is invalid", "input", val)
+		return ""
+	default:
+		// This is not used at the moment
+		log.Info("invalid route annotation timeout, setting to "+templateutil.HaproxyDefaultTimeout, "input", val)
+		return templateutil.HaproxyDefaultTimeout
 	}
-	// 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
-		}
+
+	// Then check to see if the timeout is larger than what HAProxy allows
+	if duration > templateutil.HaproxyMaxTimeoutDuration {
+		log.Info("route annotation timeout exceeds maximum allowable by HAProxy, clipping to "+templateutil.HaproxyMaxTimeout, "input", val)
+		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,56 @@ func TestClipHAProxyTimeoutValue(t *testing.T) {
 			value:    "",
 			expected: "",
 		},
+		{
+			value:    "s",
+			expected: "",
+			// Invalid input produces blank output.
+		},
+		{
+			value:    "0",
+			expected: "",
+			// Invalid input produces blank output.
+		},
+		{
+			value:    "01s",
+			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:    "+-+",
+			expected: "",
+			// Invalid input produces blank output.
+		},
+		{
+			value:    "24d1",
+			expected: "",
+			// Invalid input produces blank output.
+		},
+		{
+			value:    "1d12h",
+			expected: "",
+			// Invalid input produces blank output.
+		},
+		{
+			value:    "foo",
+			expected: "",
+			// Invalid input produces blank output.
+		},
+		{
+			value:    "2562047.99h",
+			expected: "",
+			// Invalid input produces blank output.
+		},
 		{
 			value:    "10",
 			expected: "10",
@@ -802,13 +853,58 @@ func TestClipHAProxyTimeoutValue(t *testing.T) {
 			value:    "10d",
 			expected: "10d",
 		},
+		{
+			value:    "24d",
+			expected: "24d",
+		},
+		{
+			value:    "2147483647ms",
+			expected: "2147483647ms",
+		},
 		{
 			value:    "100d",
-			expected: haproxyMaxTimeout,
+			expected: templateutil.HaproxyMaxTimeout,
+			// Exceeds the HAProxy maximum.
 		},
 		{
 			value:    "1000h",
-			expected: haproxyMaxTimeout,
+			expected: templateutil.HaproxyMaxTimeout,
+			// Exceeds the HAProxy maximum.
+		},
+		{
+			value:    "2147483648",
+			expected: templateutil.HaproxyMaxTimeout,
+			// Exceeds the HAProxy maximum and has no unit.
+		},
+		{
+			value:    "9223372036855ms",
+			expected: templateutil.HaproxyMaxTimeout,
+			// Exceeds the haproxytime.ParseDuration maximum.
+		},
+		{
+			value:    "9223372036854776us",
+			expected: templateutil.HaproxyMaxTimeout,
+			// Exceeds the haproxytime.ParseDuration maximum.
+		},
+		{
+			value:    "100000000000s",
+			expected: templateutil.HaproxyMaxTimeout,
+			// Exceeds the haproxytime.ParseDuration maximum.
+		},
+		{
+			value:    "922337203685477581ms",
+			expected: templateutil.HaproxyMaxTimeout,
+			// Exceeds the haproxytime.ParseDuration maximum.
+		},
+		{
+			value:    "9223372036854775807",
+			expected: templateutil.HaproxyMaxTimeout,
+			// Exceeds the int64 maximum and has no unit.
+		},
+		{
+			value:    "9999999999999999",
+			expected: templateutil.HaproxyMaxTimeout,
+			// Exceeds the haproxytime.ParseDuration maximum and has no unit.
 		},
 	}
 	for _, tc := range testCases {

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

@@ -0,0 +1,107 @@
+package haproxytime
+
+import (
+	"errors"
+	"math"
+	"regexp"
+	"strconv"
+	"time"
+)
+
+var (
+	// OverflowError represents an overflow error from ParseDuration.
+	// OverflowError is returned if the input value is greater than what ParseDuration
+	// allows (value must be representable as int64, e.g. 9223372036854775807 nanoseconds).
+	OverflowError = errors.New("overflow")
+
+	// SyntaxError represents an error based on invalid input to ParseDuration.
+	SyntaxError = errors.New("invalid duration")
+
+	// durationRE regexp should match the one in $timeSpecPattern in the haproxy-config.template,
+	// except that we use ^$ anchors and a capture group around the numeric part to simplify the
+	// duration parsing.
+	durationRE = regexp.MustCompile(`^([1-9][0-9]*)(us|ms|s|m|h|d)?$`)
+)
+
+// ParseDuration takes a string representing a duration in HAProxy's
+// specific format and converts it into a time.Duration value. The
+// 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 OverflowError if the value exceeds
+// the maximum allowable input, or 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
+	}
+
+	// Unit is milliseconds when left unspecified.
+	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 {
+		// ParseInt is documented to return only ErrSyntax or
+		// ErrRange when an error occurs. As we've already
+		// covered the ErrSyntax case with the regex, we can
+		// assume this is ErrRange.
+		return 0, OverflowError
+	}
+
+	// Check for overflow conditions before multiplying 'value' by 'unit'.
+	// 'value' is guaranteed to be >= 0, as ensured by the preceding regular expression.
+	//
+	// The maximum allowable 'value' is determined by dividing math.MaxInt64
+	// by the nanosecond representation of 'unit'. This prevents overflow when
+	// 'value' is later multiplied by 'unit'.
+	//
+	// Examples:
+	//  1. If the 'unit' is time.Second (1e9 ns), then the maximum
+	//     'value' allowed is math.MaxInt64 / 1e9.
+	//  2. If the 'unit' is 24 * time.Hour (86400e9 ns), then the
+	//     maximum 'value' allowed is math.MaxInt64 / 86400e9.
+	//  3. If the 'unit' is time.Microsecond (1e3 ns), then the maximum
+	//     'value' allowed is math.MaxInt64 / 1e3.
+	//
+	// Concrete examples with actual values:
+	//   - No Overflow (days): "106751d" as input makes 'unit' 24 * time.Hour (86400e9 ns).
+	//     The check ensures 106751 <= math.MaxInt64 / 86400e9.
+	//     Specifically, 106751 <= 9223372036854775807 / 86400000000 (106751 <= 106751).
+	//     This is the maximum 'value' for days that won't cause an overflow.
+	//
+	//   - Overflow (days): Specifying "106752d" makes 'unit' 24 * time.Hour (86400e9 ns).
+	//     The check finds 106752 > math.MaxInt64 / 86400e9.
+	//     Specifically, 106752 > 9223372036854775807 / 86400000000 (106752 > 106751), causing an overflow.
+	//
+	//   - No Overflow (us): "9223372036854775us" makes 'unit' time.Microsecond (1e3 ns).
+	//     The check ensures 9223372036854775 <= math.MaxInt64 / 1e3.
+	//     Specifically, 9223372036854775 <= 9223372036854775807 / 1000 (9223372036854775 <= 9223372036854775).
+	//     This is the maximum 'value' for microseconds that won't cause an overflow.
+	if value > math.MaxInt64/int64(unit) {
+		return 0, OverflowError
+	}
+
+	duration := time.Duration(value) * unit
+	return duration, nil
+}