From fb70ac4d547954e7f96c12b4bc329917f1695d35 Mon Sep 17 00:00:00 2001 From: Candace Holman Date: Mon, 30 Oct 2023 13:12:48 -0400 Subject: [PATCH] OCPBUGS-6958: Fix clipHAProxyTimeoutValue (#483) * 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. * add haproxytime * use pkg/router/template/util/haproxytime * drop existing ParseHAProxyDuration * 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. * 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. * 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. --------- Co-authored-by: Andrew McDermott --- pkg/router/template/template_helper.go | 58 +++++----- pkg/router/template/template_helper_test.go | 100 +++++++++++++++- .../util/haproxytime/duration_parser.go | 107 ++++++++++++++++++ .../util/haproxytime/duration_parser_test.go | 95 ++++++++++++++++ pkg/router/template/util/util.go | 21 +++- 5 files changed, 348 insertions(+), 33 deletions(-) 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/template_helper.go b/pkg/router/template/template_helper.go index cd2ee2d03..7ce99c22c 100644 --- a/pkg/router/template/template_helper.go +++ b/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 } diff --git a/pkg/router/template/template_helper_test.go b/pkg/router/template/template_helper_test.go index 9e0a95c5c..03b1177bc 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,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 { 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..108bd1ab8 --- /dev/null +++ b/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 +} 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..b185f3b79 --- /dev/null +++ b/pkg/router/template/util/haproxytime/duration_parser_test.go @@ -0,0 +1,95 @@ +package haproxytime_test + +import ( + "testing" + "time" + + "github.com/openshift/router/pkg/router/template/util/haproxytime" +) + +func Test_ParseDuration(t *testing.T) { + tests := []struct { + input string + expectedDuration time.Duration + expectedErr error + }{ + // Syntax error test cases. + {" 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}, + {"-100", 0, haproxytime.SyntaxError}, + {"-1us", 0, haproxytime.SyntaxError}, + {"/", 0, haproxytime.SyntaxError}, + {"123ns", 0, haproxytime.SyntaxError}, + {"invalid", 0, haproxytime.SyntaxError}, + + // Validate default unit. + {"1", 1 * time.Millisecond, nil}, + + // Small values for each unit. + {"1us", 1 * time.Microsecond, nil}, + {"1ms", 1 * time.Millisecond, nil}, + {"1s", 1 * time.Second, nil}, + {"1m", 1 * time.Minute, nil}, + {"1h", 1 * time.Hour, nil}, + {"1d", 24 * time.Hour, nil}, + + // The maximum duration that can be represented in a + // time.Duration value is determined by the limits of + // int64, as time.Duration is just an alias for int64 + // where each unit represents a nanosecond. + // + // The maximum int64 value is 9223372036854775807, which in + // microseconds is 9223372036854775us + // + // Therefore, the maximum durations for various units + // are calculated as follows: + // + // - Microseconds: 9223372036854775 (9223372036854775807 / 1000) + // - Milliseconds: 9223372036854 (9223372036854775807 / 1000000) + // - Seconds: 9223372036 (9223372036854775807 / 1000000000) + // - Minutes: 153722867 (9223372036854775807 / 60000000000) + // - Hours: 2562047 (9223372036854775807 / 3600000000000) + // - Days: 106751 (9223372036854775807 / 86400000000000) + + // The largest representable value for each unit. + {"9223372036854775us", 9223372036854775 * time.Microsecond, nil}, + {"9223372036854ms", 9223372036854 * time.Millisecond, nil}, + {"9223372036s", 9223372036 * time.Second, nil}, + {"153722867m", 153722867 * time.Minute, nil}, + {"2562047h", 2562047 * time.Hour, nil}, + {"106751d", 106751 * 24 * time.Hour, nil}, + + // Overflow cases. + {"9223372036854776us", 0, haproxytime.OverflowError}, + {"9223372036855ms", 0, haproxytime.OverflowError}, + {"9223372037s", 0, haproxytime.OverflowError}, + {"153722868m", 0, haproxytime.OverflowError}, + {"2562048h", 0, haproxytime.OverflowError}, + {"106752d", 0, haproxytime.OverflowError}, + + // Test strconv.ParseInt errors as value is bigger + // than int64 max. + {"18446744073709551615us", 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) + } + }) + } +} diff --git a/pkg/router/template/util/util.go b/pkg/router/template/util/util.go index 411238a60..88c6558b4 100644 --- a/pkg/router/template/util/util.go +++ b/pkg/router/template/util/util.go @@ -4,14 +4,33 @@ import ( "fmt" "regexp" "strings" + "time" 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" ) +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" +) + +// 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.