Skip to content

Commit

Permalink
OCPBUGS-6958: Fix clipHAProxyTimeoutValue (#483)
Browse files Browse the repository at this point in the history
* 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 <amcdermo@redhat.com>
  • Loading branch information
candita and frobware authored Oct 30, 2023
1 parent a97e096 commit fb70ac4
Show file tree
Hide file tree
Showing 5 changed files with 348 additions and 33 deletions.
58 changes: 28 additions & 30 deletions pkg/router/template/template_helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}

Expand Down
100 changes: 98 additions & 2 deletions pkg/router/template/template_helper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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",
Expand All @@ -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 {
Expand Down
107 changes: 107 additions & 0 deletions pkg/router/template/util/haproxytime/duration_parser.go
Original file line number Diff line number Diff line change
@@ -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
}
Loading

0 comments on commit fb70ac4

Please sign in to comment.