From bfa3b282512ab5222c73c1d5fa1287aa5d736d26 Mon Sep 17 00:00:00 2001 From: candita Date: Fri, 19 May 2023 17:13:55 -0400 Subject: [PATCH 1/7] OCPBUGS-6958: Fix clipHAProxyTimeoutValue so that: * a value larger than time.ParseDuration can handle is clipped to the HAProxy max timeout * a value that cannot be properly parsed for other reasons is set to empty instead of being silently allowed To check that time.ParseDuration is experiencing overflow, add a new ParseHAProxyDuration to the util package so we can evaluate the errors returned by time.ParseDuration without sacrificing its authority in parsing time strings. --- pkg/router/template/template_helper.go | 51 ++++---- pkg/router/template/template_helper_test.go | 82 ++++++++++++- pkg/router/template/util/util.go | 123 ++++++++++++++++++++ 3 files changed, 225 insertions(+), 31 deletions(-) diff --git a/pkg/router/template/template_helper.go b/pkg/router/template/template_helper.go index cd2ee2d03..297d01c4e 100644 --- a/pkg/router/template/template_helper.go +++ b/pkg/router/template/template_helper.go @@ -13,10 +13,8 @@ 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" @@ -24,8 +22,6 @@ import ( const ( certConfigMap = "cert_config.map" - // max timeout allowable by HAProxy - haproxyMaxTimeout = "2147483647ms" ) func isTrue(s string) bool { @@ -321,8 +317,8 @@ 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, @@ -330,32 +326,29 @@ func clipHAProxyTimeoutValue(val string) 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 := templateutil.ParseHAProxyDuration(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.(type) { + case templateutil.OverflowError: + log.Info("route annotation timeout exceeds maximum allowable by HAProxy, clipping to max", "max", templateutil.HaproxyMaxTimeout) + return templateutil.HaproxyMaxTimeout + case templateutil.InvalidInputError: + 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 templateutil.HaproxyMaxTimeoutDuration < duration { + log.Info("Route annotation timeout exceeds maximum allowable by HAProxy, clipping to max", "max", templateutil.HaproxyMaxTimeout) + return templateutil.HaproxyMaxTimeout + } return val } diff --git a/pkg/router/template/template_helper_test.go b/pkg/router/template/template_helper_test.go index 9e0a95c5c..22b9d24cf 100644 --- a/pkg/router/template/template_helper_test.go +++ b/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 { diff --git a/pkg/router/template/util/util.go b/pkg/router/template/util/util.go index 411238a60..0560e514e 100644 --- a/pkg/router/template/util/util.go +++ b/pkg/router/template/util/util.go @@ -1,9 +1,11 @@ package util import ( + "errors" "fmt" "regexp" "strings" + "time" routev1 "github.com/openshift/api/route/v1" @@ -12,6 +14,14 @@ import ( logf "github.com/openshift/router/log" ) +const ( + // HaproxyMaxTimeout is the max timeout allowable by HAProxy. + HaproxyMaxTimeout = "2147483647ms" + // HaproxyDefaultTimeout is the default timeout to use when the + // timeout value is not parseable for reasons other than it is too large. + HaproxyDefaultTimeout = "5s" +) + var log = logf.Logger.WithName("util") // generateRouteHostRegexp generates a regular expression to match route hosts. @@ -94,3 +104,116 @@ func GenerateBackendNamePrefix(termination routev1.TLSTerminationType) string { return prefix } + +var haproxyDurationRE = regexp.MustCompile(`^([0-9]+)(us|ms|s|m|h|d)?$`) + +// HaproxyMaxTimeoutDuration is HaproxyMaxTimeout as a time.Duration value. +var HaproxyMaxTimeoutDuration = func() time.Duration { + d, err := time.ParseDuration(HaproxyMaxTimeout) + if err != nil { + panic(err) + } + return d +}() + +// OverflowError represents an overflow error from ParseHAProxyDuration. +// OverflowError is returned if the value is greater than what time.ParseDuration +// allows (value must be representable as uint64, e.g. approximately 2562047.79 hours +// or 9223372036854775807 nanoseconds). +type OverflowError struct { + error +} + +// InvalidInputError represents an error based on invalid input to ParseHAProxyDuration +type InvalidInputError struct { + error +} + +// ParseHAProxyDuration is similar to time.ParseDuration, but modified with meaningful +// error messages for invalid input, with support for decimal points removed, with +// support for the "d" unit for days, and with "ms" as the default unit. +// It parses a duration string and returns a suitable duration. A duration string is a +// sequence of decimal numbers followed by a unit suffix, +// such as "300ms", "1h" or "45m". If the unit suffix is omitted, "ms" is used. +// Valid time units are "us", "ms", "s", "m", "h", and "d". +func ParseHAProxyDuration(s string) (time.Duration, error) { + orig := s + var d uint64 + + // Compare the input to the HAProxy duration regex. + if !haproxyDurationRE.MatchString(s) { + return 0, InvalidInputError{errors.New("invalid duration, no pattern match " + s)} + } + + if s == "0" { + return 0, nil + } + for s != "" { + // The next character must be [0-9]. + if !('0' <= s[0] && s[0] <= '9') { + return 0, InvalidInputError{errors.New("invalid duration, not a number " + orig)} + } + + // Consume [0-9]*. + var v uint64 + var err error + v, s, err = leadingInt(s) + if err != nil { + // overflow. + return 0, OverflowError{errors.New(err.Error() + orig)} + } + + // Set the default HAProxy unit of "ms", in case a unit wasn't provided. + u := "ms" + // Consume unit. + i := 0 + for ; i < len(s); i++ { + c := s[i] + if '0' <= c && c <= '9' { + break + } + } + if i > 0 { + u = s[:i] + } + s = s[i:] + unit, _ := unitMap[u] + + // Calculate the current value we've parsed, and check for overflow. + if v > 1<<63/unit { + // overflow. + return HaproxyMaxTimeoutDuration, OverflowError{errors.New("invalid duration, overflow " + orig)} + } + v *= unit + d += v + } + if d > 1<<63-1 { + return HaproxyMaxTimeoutDuration, OverflowError{errors.New("invalid duration, overflow " + orig)} + } + return time.Duration(d), nil +} + +// leadingInt consumes the leading [0-9]* from s, and comes from the time package, +// a private function used by time.ParseDuration, and needed by ParseHAProxyDuration above. +func leadingInt[bytes []byte | string](s bytes) (x uint64, rem bytes, err error) { + i := 0 + for ; i < len(s); i++ { + c := s[i] + if c < '0' || c > '9' { + break + } + x = x*10 + uint64(c) - '0' + } + return x, s[i:], nil +} + +// unitMap is used by ParseHAProxyDuration for mapping a string-based unit +// to a time-based uint64. +var unitMap = map[string]uint64{ + "us": uint64(time.Microsecond), + "ms": uint64(time.Millisecond), + "s": uint64(time.Second), + "m": uint64(time.Minute), + "h": uint64(time.Hour), + "d": uint64(time.Hour * 24), // HAProxy does accept d, convert to h for parsing +} From cf596f8df054e108807e0c9ac0e2c538b27d456f Mon Sep 17 00:00:00 2001 From: Andrew McDermott Date: Thu, 24 Aug 2023 09:30:59 +0100 Subject: [PATCH 2/7] add haproxytime --- .../util/haproxytime/duration_parser.go | 78 +++++++++++++++++++ .../util/haproxytime/duration_parser_test.go | 59 ++++++++++++++ 2 files changed, 137 insertions(+) create mode 100644 pkg/router/template/util/haproxytime/duration_parser.go create mode 100644 pkg/router/template/util/haproxytime/duration_parser_test.go diff --git a/pkg/router/template/util/haproxytime/duration_parser.go b/pkg/router/template/util/haproxytime/duration_parser.go new file mode 100644 index 000000000..a12d569c1 --- /dev/null +++ b/pkg/router/template/util/haproxytime/duration_parser.go @@ -0,0 +1,78 @@ +package haproxytime + +import ( + "errors" + "regexp" + "strconv" + "time" +) + +// MaxTimeout defines the maximum duration that can be represented in +// HAProxy's configuration. +const MaxTimeout = 2147483647 * time.Millisecond + +var ( + // OverflowError is returned when the parsed value exceeds the + // maximum allowed. + OverflowError = errors.New("value out of range") + + // 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 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 an OverflowError if the value exceeds +// MaxTimeout, 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, 32) + 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 + } + + duration := time.Duration(value) * unit + if duration > MaxTimeout { + return 0, OverflowError + } + + return duration, nil +} diff --git a/pkg/router/template/util/haproxytime/duration_parser_test.go b/pkg/router/template/util/haproxytime/duration_parser_test.go new file mode 100644 index 000000000..0fa4ff723 --- /dev/null +++ b/pkg/router/template/util/haproxytime/duration_parser_test.go @@ -0,0 +1,59 @@ +package haproxytime_test + +import ( + "testing" + "time" + + "github.com/openshift/router/pkg/router/template/util/haproxytime" +) + +func TestParseHAProxyDuration(t *testing.T) { + tests := []struct { + input string + expectedDuration time.Duration + expectedErr error + }{ + // Success Cases + {"0", 0, nil}, + {"123us", 123 * time.Microsecond, nil}, + {"456ms", 456 * time.Millisecond, nil}, + {"789s", 789 * time.Second, nil}, + {"5m", 5 * time.Minute, nil}, + {"2h", 2 * time.Hour, nil}, + {"1d", 24 * time.Hour, nil}, + {"24d", 24 * 24 * time.Hour, nil}, + {"2147483646", haproxytime.MaxTimeout - time.Millisecond, nil}, + {"2147483647", haproxytime.MaxTimeout, nil}, + + // Syntax Error Cases + {"", 0, haproxytime.SyntaxError}, + {"invalid", 0, haproxytime.SyntaxError}, + {"+100", 0, haproxytime.SyntaxError}, + {"-100", 0, haproxytime.SyntaxError}, + {"/", 0, haproxytime.SyntaxError}, + {" spaces are invalid", 0, haproxytime.SyntaxError}, + + // Overflow Error Cases + {"25d", 0, haproxytime.OverflowError}, + {"9999999999", 0, haproxytime.OverflowError}, + {"1000000000000000000000000000", 0, haproxytime.OverflowError}, + {"1000000000000000000000000000h", 0, haproxytime.OverflowError}, + {"2147483648", 0, haproxytime.OverflowError}, + } + + for _, tc := range tests { + t.Run(tc.input, func(t *testing.T) { + duration, err := haproxytime.ParseDuration(tc.input) + if duration != tc.expectedDuration { + t.Errorf("expected duration %v, got %v", tc.expectedDuration, duration) + } + if err != nil && tc.expectedErr == nil { + t.Errorf("expected no error, got %v", err) + } else if err == nil && tc.expectedErr != nil { + t.Errorf("expected error %v, got none", tc.expectedErr) + } else if err != nil && tc.expectedErr != nil && err.Error() != tc.expectedErr.Error() { + t.Errorf("expected error %v, got %v", tc.expectedErr, err) + } + }) + } +} From d434ca602493a409da78bbc28ba371dd8abc42a8 Mon Sep 17 00:00:00 2001 From: Andrew McDermott Date: Thu, 24 Aug 2023 09:34:18 +0100 Subject: [PATCH 3/7] use pkg/router/template/util/haproxytime --- pkg/router/template/template_helper.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/pkg/router/template/template_helper.go b/pkg/router/template/template_helper.go index 297d01c4e..bc574b4be 100644 --- a/pkg/router/template/template_helper.go +++ b/pkg/router/template/template_helper.go @@ -18,6 +18,7 @@ import ( "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 ( @@ -328,13 +329,13 @@ func clipHAProxyTimeoutValue(val string) string { } // First check to see if the timeout will fit into a time.Duration - duration, err := templateutil.ParseHAProxyDuration(val) + _, err := haproxytime.ParseDuration(val) if err != nil { - switch err.(type) { - case templateutil.OverflowError: + 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 templateutil.InvalidInputError: + case haproxytime.SyntaxError: log.Error(err, "route annotation timeout removed because input is invalid") return "" default: @@ -344,11 +345,6 @@ func clipHAProxyTimeoutValue(val string) string { } } - // Then check to see if the timeout is larger than what HAProxy allows - if templateutil.HaproxyMaxTimeoutDuration < duration { - log.Info("Route annotation timeout exceeds maximum allowable by HAProxy, clipping to max", "max", templateutil.HaproxyMaxTimeout) - return templateutil.HaproxyMaxTimeout - } return val } From 01038ad8defc3936583e242a66cc75dd95663716 Mon Sep 17 00:00:00 2001 From: Andrew McDermott Date: Thu, 24 Aug 2023 09:34:30 +0100 Subject: [PATCH 4/7] drop existing ParseHAProxyDuration --- pkg/router/template/util/util.go | 115 ------------------------------- 1 file changed, 115 deletions(-) diff --git a/pkg/router/template/util/util.go b/pkg/router/template/util/util.go index 0560e514e..87751be89 100644 --- a/pkg/router/template/util/util.go +++ b/pkg/router/template/util/util.go @@ -1,11 +1,9 @@ package util import ( - "errors" "fmt" "regexp" "strings" - "time" routev1 "github.com/openshift/api/route/v1" @@ -104,116 +102,3 @@ func GenerateBackendNamePrefix(termination routev1.TLSTerminationType) string { return prefix } - -var haproxyDurationRE = regexp.MustCompile(`^([0-9]+)(us|ms|s|m|h|d)?$`) - -// HaproxyMaxTimeoutDuration is HaproxyMaxTimeout as a time.Duration value. -var HaproxyMaxTimeoutDuration = func() time.Duration { - d, err := time.ParseDuration(HaproxyMaxTimeout) - if err != nil { - panic(err) - } - return d -}() - -// OverflowError represents an overflow error from ParseHAProxyDuration. -// OverflowError is returned if the value is greater than what time.ParseDuration -// allows (value must be representable as uint64, e.g. approximately 2562047.79 hours -// or 9223372036854775807 nanoseconds). -type OverflowError struct { - error -} - -// InvalidInputError represents an error based on invalid input to ParseHAProxyDuration -type InvalidInputError struct { - error -} - -// ParseHAProxyDuration is similar to time.ParseDuration, but modified with meaningful -// error messages for invalid input, with support for decimal points removed, with -// support for the "d" unit for days, and with "ms" as the default unit. -// It parses a duration string and returns a suitable duration. A duration string is a -// sequence of decimal numbers followed by a unit suffix, -// such as "300ms", "1h" or "45m". If the unit suffix is omitted, "ms" is used. -// Valid time units are "us", "ms", "s", "m", "h", and "d". -func ParseHAProxyDuration(s string) (time.Duration, error) { - orig := s - var d uint64 - - // Compare the input to the HAProxy duration regex. - if !haproxyDurationRE.MatchString(s) { - return 0, InvalidInputError{errors.New("invalid duration, no pattern match " + s)} - } - - if s == "0" { - return 0, nil - } - for s != "" { - // The next character must be [0-9]. - if !('0' <= s[0] && s[0] <= '9') { - return 0, InvalidInputError{errors.New("invalid duration, not a number " + orig)} - } - - // Consume [0-9]*. - var v uint64 - var err error - v, s, err = leadingInt(s) - if err != nil { - // overflow. - return 0, OverflowError{errors.New(err.Error() + orig)} - } - - // Set the default HAProxy unit of "ms", in case a unit wasn't provided. - u := "ms" - // Consume unit. - i := 0 - for ; i < len(s); i++ { - c := s[i] - if '0' <= c && c <= '9' { - break - } - } - if i > 0 { - u = s[:i] - } - s = s[i:] - unit, _ := unitMap[u] - - // Calculate the current value we've parsed, and check for overflow. - if v > 1<<63/unit { - // overflow. - return HaproxyMaxTimeoutDuration, OverflowError{errors.New("invalid duration, overflow " + orig)} - } - v *= unit - d += v - } - if d > 1<<63-1 { - return HaproxyMaxTimeoutDuration, OverflowError{errors.New("invalid duration, overflow " + orig)} - } - return time.Duration(d), nil -} - -// leadingInt consumes the leading [0-9]* from s, and comes from the time package, -// a private function used by time.ParseDuration, and needed by ParseHAProxyDuration above. -func leadingInt[bytes []byte | string](s bytes) (x uint64, rem bytes, err error) { - i := 0 - for ; i < len(s); i++ { - c := s[i] - if c < '0' || c > '9' { - break - } - x = x*10 + uint64(c) - '0' - } - return x, s[i:], nil -} - -// unitMap is used by ParseHAProxyDuration for mapping a string-based unit -// to a time-based uint64. -var unitMap = map[string]uint64{ - "us": uint64(time.Microsecond), - "ms": uint64(time.Millisecond), - "s": uint64(time.Second), - "m": uint64(time.Minute), - "h": uint64(time.Hour), - "d": uint64(time.Hour * 24), // HAProxy does accept d, convert to h for parsing -} From e52afb2fdc237e0b1d1f53ef33b756e9a1676ffd Mon Sep 17 00:00:00 2001 From: candita Date: Fri, 19 May 2023 17:13:55 -0400 Subject: [PATCH 5/7] OCPBUGS-6958: Fix clipHAProxyTimeoutValue so that: * a value larger than time.ParseDuration can handle is clipped to the HAProxy max timeout * a value that cannot be properly parsed for other reasons is set to empty instead of being silently allowed To check that time.ParseDuration is experiencing overflow, add a new ParseHAProxyDuration to the util package so we can evaluate the errors returned by time.ParseDuration without sacrificing its authority in parsing time strings. --- pkg/router/template/template_helper.go | 51 ++++---- pkg/router/template/template_helper_test.go | 87 ++++++++++++- pkg/router/template/util/util.go | 134 ++++++++++++++++++++ pkg/router/template/util/util_test.go | 98 ++++++++++++++ 4 files changed, 339 insertions(+), 31 deletions(-) diff --git a/pkg/router/template/template_helper.go b/pkg/router/template/template_helper.go index cd2ee2d03..297d01c4e 100644 --- a/pkg/router/template/template_helper.go +++ b/pkg/router/template/template_helper.go @@ -13,10 +13,8 @@ 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" @@ -24,8 +22,6 @@ import ( const ( certConfigMap = "cert_config.map" - // max timeout allowable by HAProxy - haproxyMaxTimeout = "2147483647ms" ) func isTrue(s string) bool { @@ -321,8 +317,8 @@ 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, @@ -330,32 +326,29 @@ func clipHAProxyTimeoutValue(val string) 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 := templateutil.ParseHAProxyDuration(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.(type) { + case templateutil.OverflowError: + log.Info("route annotation timeout exceeds maximum allowable by HAProxy, clipping to max", "max", templateutil.HaproxyMaxTimeout) + return templateutil.HaproxyMaxTimeout + case templateutil.InvalidInputError: + 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 templateutil.HaproxyMaxTimeoutDuration < duration { + log.Info("Route annotation timeout exceeds maximum allowable by HAProxy, clipping to max", "max", templateutil.HaproxyMaxTimeout) + return templateutil.HaproxyMaxTimeout + } return val } diff --git a/pkg/router/template/template_helper_test.go b/pkg/router/template/template_helper_test.go index 9e0a95c5c..d83216919 100644 --- a/pkg/router/template/template_helper_test.go +++ b/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,12 +814,85 @@ 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: "9223372036854775807", // max int64 without unit + expected: templateutil.HaproxyMaxTimeout, }, + { + 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 { actual := clipHAProxyTimeoutValue(tc.value) diff --git a/pkg/router/template/util/util.go b/pkg/router/template/util/util.go index 411238a60..0b866464b 100644 --- a/pkg/router/template/util/util.go +++ b/pkg/router/template/util/util.go @@ -1,9 +1,11 @@ package util import ( + "errors" "fmt" "regexp" "strings" + "time" routev1 "github.com/openshift/api/route/v1" @@ -12,6 +14,14 @@ import ( logf "github.com/openshift/router/log" ) +const ( + // HaproxyMaxTimeout is the max timeout allowable by HAProxy. + HaproxyMaxTimeout = "2147483647ms" + // HaproxyDefaultTimeout is the default timeout to use when the + // timeout value is not parseable for reasons other than it is too large. + HaproxyDefaultTimeout = "5s" +) + var log = logf.Logger.WithName("util") // generateRouteHostRegexp generates a regular expression to match route hosts. @@ -94,3 +104,127 @@ func GenerateBackendNamePrefix(termination routev1.TLSTerminationType) string { return prefix } + +var haproxyDurationRE = regexp.MustCompile(`^([0-9]+)(us|ms|s|m|h|d)?$`) + +// HaproxyMaxTimeoutDuration is HaproxyMaxTimeout as a time.Duration value. +var HaproxyMaxTimeoutDuration = func() time.Duration { + d, err := time.ParseDuration(HaproxyMaxTimeout) + if err != nil { + panic(err) + } + return d +}() + +// OverflowError represents an overflow error from ParseHAProxyDuration. +// OverflowError is returned if the value is greater than what time.ParseDuration +// allows (value must be representable as uint64, e.g. approximately 2562047.79 hours +// or 9223372036854775807 nanoseconds). +type OverflowError struct { + error +} + +// InvalidInputError represents an error based on invalid input to ParseHAProxyDuration. +type InvalidInputError struct { + error +} + +// ParseHAProxyDuration is similar to time.ParseDuration, but modified with meaningful +// error messages for invalid input, with support for decimal points removed, with +// support for the "d" unit for days, and with "ms" as the default unit. +// It parses a duration string and returns a suitable duration. A duration string is a +// sequence of decimal numbers followed by a unit suffix, +// such as "300ms", "1h" or "45m". If the unit suffix is omitted, "ms" is used. +// Valid time units are "us", "ms", "s", "m", "h", and "d". +func ParseHAProxyDuration(s string) (time.Duration, error) { + orig := s + var d uint64 + + // Compare the input to the HAProxy duration regex. + if !haproxyDurationRE.MatchString(s) { + return 0, InvalidInputError{errors.New("invalid duration, no pattern match " + s)} + } + + if s == "0" { + return 0, nil + } + for s != "" { + // The next character must be [0-9]. + if !('0' <= s[0] && s[0] <= '9') { + return 0, InvalidInputError{errors.New("invalid duration, not a number " + orig)} + } + + // Consume [0-9]*. + var v uint64 + var err error + v, s, err = leadingInt(s) + if err != nil { + // overflow. + return 0, OverflowError{errors.New(err.Error() + orig)} + } + + // Set the default HAProxy unit of "ms", in case a unit wasn't provided. + u := "ms" + // Consume unit. + i := 0 + for ; i < len(s); i++ { + c := s[i] + if '0' <= c && c <= '9' { + break + } + } + if i > 0 { + u = s[:i] + } + s = s[i:] + unit, _ := unitMap[u] + + // Calculate the current value we've parsed, and check for overflow. + if v > 1<<63/unit { + // overflow. + return HaproxyMaxTimeoutDuration, OverflowError{errors.New("invalid duration, overflow " + orig)} + } + v *= unit + d += v + } + if d > 1<<63-1 { + return HaproxyMaxTimeoutDuration, OverflowError{errors.New("invalid duration, overflow " + orig)} + } + return time.Duration(d), nil +} + +// leadingInt consumes the leading [0-9]* from s, and comes from the time package, +// a private function used by time.ParseDuration, and needed by ParseHAProxyDuration above. +func leadingInt(s string) (x uint64, rem string, err error) { + i := 0 + for ; i < len(s); i++ { + c := s[i] + if c < '0' || c > '9' { + break + } + if x >= 1<<63/10 { + // Potential for overflow on next iteration + nextVal := x*10 + uint64(c) - '0' + if nextVal > 1<<63-1 { + overflowErr := OverflowError{errors.New("value too large to be represented as duration")} + if i == len(s)-1 { + return 0, "", overflowErr + } + return 0, s[i+1:], overflowErr + } + } + x = x*10 + uint64(c) - '0' + } + return x, s[i:], nil +} + +// unitMap is used by ParseHAProxyDuration for mapping a string-based unit +// to a time-based uint64. +var unitMap = map[string]uint64{ + "us": uint64(time.Microsecond), + "ms": uint64(time.Millisecond), + "s": uint64(time.Second), + "m": uint64(time.Minute), + "h": uint64(time.Hour), + "d": uint64(time.Hour * 24), // HAProxy does accept d, convert to h for parsing +} diff --git a/pkg/router/template/util/util_test.go b/pkg/router/template/util/util_test.go index ca3d6d0e8..6eb885015 100644 --- a/pkg/router/template/util/util_test.go +++ b/pkg/router/template/util/util_test.go @@ -1,6 +1,7 @@ package util import ( + "errors" "regexp" "testing" @@ -256,3 +257,100 @@ func TestGenerateBackendNamePrefix(t *testing.T) { } } } + +func TestLeadingInt(t *testing.T) { + tests := []struct { + name string + input string + expected uint64 + rem string + err error + }{{ + name: "BasicNumber", + input: "123", + expected: 123, + rem: "", + err: nil, + }, { + name: "EmptyString", + input: "", + expected: 0, + rem: "", + err: nil, + }, { + name: "MaxInt64", + input: "9223372036854775807", + expected: 9223372036854775807, + rem: "", + err: nil, + }, { + name: "NearOverflowWith6", + input: "9223372036854775806", + expected: 9223372036854775806, + rem: "", + err: nil, + }, { + name: "NearOverflowWith7", + input: "9223372036854775807", + expected: 9223372036854775807, + rem: "", + err: nil, + }, { + name: "NearOverflowWithNonDigit", + input: "922337203685477580a", + expected: 922337203685477580, + rem: "a", + err: nil, + }, { + name: "NearOverflowWithSpace", + input: "922337203685477580 ", + expected: 922337203685477580, + rem: " ", + err: nil, + }, { + name: "NegativeNumber", + input: "-12345", + expected: 0, + rem: "-12345", + err: nil, + }, { + name: "NumberWithNonNumericSuffix", + input: "123abc", + expected: 123, + rem: "abc", + err: nil, + }, { + name: "Overflow1", + input: "9223372036854775808", + expected: 0, + rem: "", + err: OverflowError{errors.New("value too large to be represented as duration")}, + }, { + name: "OverflowWithMultipleTrailingText", + input: "9223372036854775808abc", + expected: 0, + rem: "abc", + err: OverflowError{errors.New("value too large to be represented as duration")}, + }, { + name: "OverflowWithSingleTrailingText", + input: "9223372036854775808a", + expected: 0, + rem: "a", + err: OverflowError{errors.New("value too large to be represented as duration")}, + }} + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, rem, err := leadingInt(tc.input) + if got != tc.expected { + t.Errorf("For input %q got: %v, expected: %v", tc.input, got, tc.expected) + } + if rem != tc.rem { + t.Errorf("For input %q remainder got: %q, expected: %q", tc.input, rem, tc.rem) + } + if (err != nil && tc.err == nil) || (err == nil && tc.err != nil) || (err != nil && err.Error() != tc.err.Error()) { + t.Errorf("For input %q error got: %v, expected: %v", tc.input, err, tc.err) + } + }) + } +} From 267fb6c528a842c5ad105f95549995788dfe9a17 Mon Sep 17 00:00:00 2001 From: candita Date: Fri, 6 Oct 2023 16:47:37 -0400 Subject: [PATCH 6/7] OCPBUGS-6958: Fix clipHAProxyTimeoutValue so that: - a value larger than time.ParseDuration can handle is clipped to the HAProxy max timeout - a value that cannot be properly parsed for other reasons is set to empty instead of being silently allowed To check that time.ParseDuration is experiencing overflow, add a new ParseDuration so we can evaluate the errors that wouldn't have been explicitly returned by time.ParseDuration, e.g. invalid HAProxy time format syntax and integer overflows. Add more unit tests. --- pkg/router/template/template_helper.go | 31 +++-- pkg/router/template/template_helper_test.go | 111 ++++++++++-------- .../util/haproxytime/duration_parser.go | 46 ++++++-- .../util/haproxytime/duration_parser_test.go | 15 +-- pkg/router/template/util/util.go | 19 +-- 5 files changed, 132 insertions(+), 90 deletions(-) diff --git a/pkg/router/template/template_helper.go b/pkg/router/template/template_helper.go index 7d3ed6eba..7ce99c22c 100644 --- a/pkg/router/template/template_helper.go +++ b/pkg/router/template/template_helper.go @@ -332,26 +332,25 @@ func clipHAProxyTimeoutValue(val string) string { return val } - // First check to see if the timeout will fit into a time.Duration + // First check to see if the value is syntactically correct and fits into a time.Duration. duration, err := haproxytime.ParseDuration(val) - if err != nil { - switch err { - 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 removed because input 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 - } + 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 } // Then check to see if the timeout is larger than what HAProxy allows - if templateutil.HaproxyMaxTimeoutDuration < duration { - log.Info("route annotation timeout exceeds maximum allowable by HAProxy, clipping to " + templateutil.HaproxyMaxTimeout, "input", val) + if duration > templateutil.HaproxyMaxTimeoutDuration { + log.Info("route annotation timeout exceeds maximum allowable by HAProxy, clipping to "+templateutil.HaproxyMaxTimeout, "input", val) return templateutil.HaproxyMaxTimeout } diff --git a/pkg/router/template/template_helper_test.go b/pkg/router/template/template_helper_test.go index d83216919..03b1177bc 100644 --- a/pkg/router/template/template_helper_test.go +++ b/pkg/router/template/template_helper_test.go @@ -792,107 +792,120 @@ func TestClipHAProxyTimeoutValue(t *testing.T) { expected: "", }, { - value: "0", - expected: "0", + value: "s", + expected: "", + // Invalid input produces blank output. }, { - value: "s", + value: "0", expected: "", - // Invalid input produces blank output + // Invalid input produces blank output. }, { - value: "10", - expected: "10", + value: "01s", + expected: "", + // Invalid input produces blank output. }, { - value: "10s", - expected: "10s", + value: "1.5.8.9", + expected: "", + // Invalid input produces blank output. }, { - value: "10d", - expected: "10d", + value: "1.5s", + expected: "", + // Invalid input produces blank output. }, { - value: "100d", - expected: templateutil.HaproxyMaxTimeout, + value: "+-+", + expected: "", + // Invalid input produces blank output. }, { - value: "1000h", - expected: templateutil.HaproxyMaxTimeout, + value: "24d1", + expected: "", + // Invalid input produces blank output. }, { - value: "+-+", + value: "1d12h", expected: "", - // Invalid input produces blank output + // Invalid input produces blank output. }, { - value: "1.5.8.9", + value: "foo", expected: "", - // Invalid input produces blank output + // Invalid input produces blank output. }, { - value: "1.5s", + value: "2562047.99h", expected: "", - // Invalid input produces blank output + // Invalid input produces blank output. }, { - value: "9223372036855ms", - expected: templateutil.HaproxyMaxTimeout, - // Exceeds the time.ParseDuration maximum + value: "10", + expected: "10", + }, + { + value: "10s", + expected: "10s", + }, + { + value: "10d", + expected: "10d", + }, + { + value: "24d", + expected: "24d", }, { value: "2147483647ms", expected: "2147483647ms", }, { - value: "922337203685477581ms", + value: "100d", expected: templateutil.HaproxyMaxTimeout, - // Overflow error > 1<<63/10 + 1 + // Exceeds the HAProxy maximum. }, { - value: "9223372036854775807", // max int64 without unit + value: "1000h", expected: templateutil.HaproxyMaxTimeout, + // Exceeds the HAProxy maximum. }, { - value: "2562047.99h", - expected: "", - // Invalid input produces blank output - }, - { - value: "100000000000s", + value: "2147483648", expected: templateutil.HaproxyMaxTimeout, - // Exceeds the time.ParseDuration maximum + // Exceeds the HAProxy maximum and has no unit. }, { - value: "9999999999999999", + value: "9223372036855ms", expected: templateutil.HaproxyMaxTimeout, - // Exceeds the time.ParseDuration maximum and has no unit + // Exceeds the haproxytime.ParseDuration maximum. }, { - value: "25d", + value: "9223372036854776us", expected: templateutil.HaproxyMaxTimeout, - // Exceeds the HAProxy maximum + // Exceeds the haproxytime.ParseDuration maximum. }, { - value: "24d", - expected: "24d", + value: "100000000000s", + expected: templateutil.HaproxyMaxTimeout, + // Exceeds the haproxytime.ParseDuration maximum. }, { - value: "24d1", - expected: "", - // Invalid input produces blank output + value: "922337203685477581ms", + expected: templateutil.HaproxyMaxTimeout, + // Exceeds the haproxytime.ParseDuration maximum. }, { - value: "1d12h", - expected: "", - // Invalid input produces blank output + value: "9223372036854775807", + expected: templateutil.HaproxyMaxTimeout, + // Exceeds the int64 maximum and has no unit. }, { - value: "foo", - expected: "", - // Invalid input produces blank output + value: "9999999999999999", + expected: templateutil.HaproxyMaxTimeout, + // Exceeds the haproxytime.ParseDuration maximum and has no unit. }, - } for _, tc := range testCases { actual := clipHAProxyTimeoutValue(tc.value) diff --git a/pkg/router/template/util/haproxytime/duration_parser.go b/pkg/router/template/util/haproxytime/duration_parser.go index e4842f8e7..108bd1ab8 100644 --- a/pkg/router/template/util/haproxytime/duration_parser.go +++ b/pkg/router/template/util/haproxytime/duration_parser.go @@ -10,30 +10,33 @@ import ( var ( // OverflowError represents an overflow error from ParseDuration. - // OverflowError is returned if the value is greater than what time.ParseDuration - // allows (value must be representable as uint64, e.g. 9223372036854775807 nanoseconds). + // 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.MustCompile(`^([0-9]+)(us|ms|s|m|h|d)?$`) + // 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 an OverflowError if the value exceeds -// MaxTimeout, or a SyntaxError if the input string doesn't match the -// expected format. +// 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 } - // Default unit is milliseconds, unless specified. + // Unit is milliseconds when left unspecified. unit := time.Millisecond numericPart := matches[1] @@ -66,6 +69,35 @@ func ParseDuration(input string) (time.Duration, error) { 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 } diff --git a/pkg/router/template/util/haproxytime/duration_parser_test.go b/pkg/router/template/util/haproxytime/duration_parser_test.go index 122c9aa9c..c666976a2 100644 --- a/pkg/router/template/util/haproxytime/duration_parser_test.go +++ b/pkg/router/template/util/haproxytime/duration_parser_test.go @@ -7,14 +7,17 @@ import ( "github.com/openshift/router/pkg/router/template/util/haproxytime" ) -func TestParseDuration(t *testing.T) { +func Test_ParseDuration(t *testing.T) { tests := []struct { input string expectedDuration time.Duration expectedErr error }{ // Syntax error test cases. - {" spaces are invalid", 0, haproxytime.SyntaxError}, + {" 1m", 0, haproxytime.SyntaxError}, + {"1m ", 0, haproxytime.SyntaxError}, + {"1 m", 0, haproxytime.SyntaxError}, + {"m", 0, haproxytime.SyntaxError}, {"", 0, haproxytime.SyntaxError}, {"+100", 0, haproxytime.SyntaxError}, {"-100", 0, haproxytime.SyntaxError}, @@ -24,20 +27,14 @@ func TestParseDuration(t *testing.T) { {"invalid", 0, haproxytime.SyntaxError}, // Validate default unit. - {"0", 0 * time.Millisecond, nil}, + {"1", 1 * time.Millisecond, nil}, // Small values for each unit. - {"0us", 0 * time.Microsecond, nil}, {"1us", 1 * time.Microsecond, nil}, - {"0ms", 0 * time.Millisecond, nil}, {"1ms", 1 * time.Millisecond, nil}, - {"0s", 0 * time.Second, nil}, {"1s", 1 * time.Second, nil}, - {"0m", 0 * time.Minute, nil}, {"1m", 1 * time.Minute, nil}, - {"0h", 0 * time.Hour, nil}, {"1h", 1 * time.Hour, nil}, - {"0d", 0 * time.Hour, nil}, {"1d", 24 * time.Hour, nil}, // The maximum duration that can be represented in a diff --git a/pkg/router/template/util/util.go b/pkg/router/template/util/util.go index 6b9401531..88c6558b4 100644 --- a/pkg/router/template/util/util.go +++ b/pkg/router/template/util/util.go @@ -8,6 +8,7 @@ import ( routev1 "github.com/openshift/api/route/v1" "github.com/openshift/router/pkg/router/routeapihelpers" + "github.com/openshift/router/pkg/router/template/util/haproxytime" logf "github.com/openshift/router/log" ) @@ -21,6 +22,15 @@ const ( HaproxyDefaultTimeout = "5s" ) +// HaproxyMaxTimeoutDuration is HaproxyMaxTimeout as a time.Duration value. +var HaproxyMaxTimeoutDuration = func() time.Duration { + d, err := haproxytime.ParseDuration(HaproxyMaxTimeout) + if err != nil { + panic(err) + } + return d +}() + var log = logf.Logger.WithName("util") // generateRouteHostRegexp generates a regular expression to match route hosts. @@ -103,12 +113,3 @@ func GenerateBackendNamePrefix(termination routev1.TLSTerminationType) string { return prefix } - -// HaproxyMaxTimeoutDuration is HaproxyMaxTimeout as a time.Duration value. -var HaproxyMaxTimeoutDuration = func() time.Duration { - d, err := time.ParseDuration(HaproxyMaxTimeout) - if err != nil { - panic(err) - } - return d -}() \ No newline at end of file From 6f75c3558be295d545bf30db337690f58a5b62dd Mon Sep 17 00:00:00 2001 From: candita Date: Fri, 6 Oct 2023 16:47:37 -0400 Subject: [PATCH 7/7] OCPBUGS-6958: Fix clipHAProxyTimeoutValue so that: - a value larger than time.ParseDuration can handle is clipped to the HAProxy max timeout - a value that cannot be properly parsed for other reasons is set to empty instead of being silently allowed To check that time.ParseDuration is experiencing overflow, add a new ParseDuration so we can evaluate the errors that wouldn't have been explicitly returned by time.ParseDuration, e.g. invalid HAProxy time format syntax and integer overflows. Add more unit tests. --- pkg/router/template/util/haproxytime/duration_parser_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/router/template/util/haproxytime/duration_parser_test.go b/pkg/router/template/util/haproxytime/duration_parser_test.go index c666976a2..b185f3b79 100644 --- a/pkg/router/template/util/haproxytime/duration_parser_test.go +++ b/pkg/router/template/util/haproxytime/duration_parser_test.go @@ -17,6 +17,7 @@ func Test_ParseDuration(t *testing.T) { {" 1m", 0, haproxytime.SyntaxError}, {"1m ", 0, haproxytime.SyntaxError}, {"1 m", 0, haproxytime.SyntaxError}, + {"0", 0, haproxytime.SyntaxError}, {"m", 0, haproxytime.SyntaxError}, {"", 0, haproxytime.SyntaxError}, {"+100", 0, haproxytime.SyntaxError},