diff --git a/pkg/router/template/template_helper.go b/pkg/router/template/template_helper.go index cd2ee2d03..41869d675 100644 --- a/pkg/router/template/template_helper.go +++ b/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,8 +336,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 +345,30 @@ 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 := 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 } 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/haproxytime/benchmark_test.go b/pkg/router/template/util/haproxytime/benchmark_test.go new file mode 100644 index 000000000..0e8ffc3b3 --- /dev/null +++ b/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) + } + } +} 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..7981d57ec --- /dev/null +++ b/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 +} 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..09afe56d5 --- /dev/null +++ b/pkg/router/template/util/haproxytime/duration_parser_test.go @@ -0,0 +1,99 @@ +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 + }{ + // Syntax error test cases. + {" spaces are invalid", 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. + {"0", 0 * 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 + // 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. + // + // Therefore, the maximum durations for various units + // are calculated as follows: + // + // - Nanoseconds: 9223372036854775807 (since the base unit is a nanosecond) + // - 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. + {"9223372036854775807ns", 0, haproxytime.SyntaxError}, + {"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. + {"9223372036854775808ns", 0, haproxytime.SyntaxError}, + {"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..87751be89 100644 --- a/pkg/router/template/util/util.go +++ b/pkg/router/template/util/util.go @@ -12,6 +12,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.