OCPBUGS-6958: Fix clipHAProxyTimeoutValue #483

candita · 2023-05-19T21:22:30Z

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.

openshift-ci-robot · 2023-05-19T21:22:34Z

@candita: This pull request references Jira Issue OCPBUGS-6958, which is invalid:

expected the bug to target the "4.14.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

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 5s instead of being silently allowed

To check that time.ParseDuration is experiencing overflow, add a new ParseDurationWithErrors to the util package so we can evaluate the errors returned by time.ParseDuration without sacrificing its authority in parsing time strings.

Add more unit tests.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

candita · 2023-05-19T21:32:29Z

/jira refresh

openshift-ci-robot · 2023-05-19T21:32:37Z

@candita: This pull request references Jira Issue OCPBUGS-6958, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Miciah · 2023-05-20T04:40:14Z

pkg/router/template/util/util.go

+	// Consume [-+]?
+	if s != "" {
+		c := s[0]
+		if c == '-' || c == '+' {
+			neg = c == '-'
+			s = s[1:]
+		}
+	}


Does HAProxy allow a leading +/-? Doesn't look like it does: https://git.haproxy.org/?p=haproxy-2.2.git;a=blob;f=src/tools.c#l2066

Hm, if people used a +/- in the past, and it had no noticeable effect, would it be ok for me to make these changes now?

I'll just go ahead and disallow it.

On further thought, it makes sense to disallow it irrespective of what HAProxy allows as $timeSpecPattern already disallows it. There's no need to have logic to parse something that would be rejected by the regexp anyway.

Miciah · 2023-05-20T04:40:19Z

pkg/router/template/util/util.go

+		// Consume (\.[0-9]*)?
+		post := false
+		if s != "" && s[0] == '.' {
+			s = s[1:]
+			pl := len(s)
+			f, scale, s = leadingFraction(s)
+			post = pl != len(s)
+		}


Does HAProxy allow a decimal point?

durationToHAProxyTimespec should remove the decimal points.

Hm, but that's not for annotations, which is what this bug deals with.

it does not allow a decimal point, removed the code supporting this and updated unit tests.

The code to support decimal points is still present:

router/pkg/router/template/util/util.go

Lines 164 to 165 in a4b3f60

v, f uint64 // integers before, after decimal point

scale float64 = 1 // value = v + f/scale

router/pkg/router/template/util/util.go

Line 175 in a4b3f60

pl := len(s)

router/pkg/router/template/util/util.go

Lines 181 to 194 in a4b3f60

pre := pl != len(s) // whether we consumed anything before a period

// Consume (\.[0-9]*)?

post := false

if s != "" && s[0] == '.' {

s = s[1:]

pl := len(s)

f, scale, s = leadingFraction(s)

post = pl != len(s)

}

if !pre && !post {

// no digits (e.g. ".s" or "-.s")

return 0, InvalidInputError{errors.New("invalid duration, not a number " + orig)}

}

router/pkg/router/template/util/util.go

Lines 200 to 202 in a4b3f60

if c == '.' || '0' <= c && c <= '9' {

break

}

router/pkg/router/template/util/util.go

Lines 219 to 227 in a4b3f60

if f > 0 {

// float64 is needed to be nanosecond accurate for fractions of hours.

// v >= 0 && (f*unit/scale) <= 3.6e+12 (ns/h, h is the largest unit)

v += uint64(float64(f) * (float64(unit) / scale))

if v > 1<<63 {

// overflow

return HaproxyMaxTimeoutDuration, OverflowError{errors.New("invalid duration, overflow " + orig)}

}

}

As I mentioned in other comments, the regexp disallows decimal points anyway, so there's no need to have logic to parse them.

pkg/router/template/util/util.go

pkg/router/template/template_helper.go

Miciah · 2023-05-20T05:21:53Z

pkg/router/template/util/util.go

+			return 0, errors.New("time: invalid duration, overflow " + orig)
+		}
+		v *= unit
+		if f > 0 {
+			// float64 is needed to be nanosecond accurate for fractions of hours.
+			// v >= 0 && (f*unit/scale) <= 3.6e+12 (ns/h, h is the largest unit)
+			v += uint64(float64(f) * (float64(unit) / scale))
+			if v > 1<<63 {
+				// overflow
+				return 0, errors.New("time: invalid duration, overflow " + orig)
+			}
+		}
+		d += v
+		if d > 1<<63 {
+			return 0, errors.New("time: invalid duration, overflow " + orig)
+		}
+	}
+	if neg {
+		return -time.Duration(d), nil
+	}
+	if d > 1<<63-1 {
+		return 0, errors.New("time: invalid duration, overflow " + orig)


Any reason not simply to return haproxyMaxTimeout in case of overflow?

If you prefer to return an error value here and handle overflow in clipHAProxyTimeoutValue, consider using a typed error value, something like this:

// OverflowError represents an overflow error from ParseDurationWithErrors. type OverflowError struct { error } func ParseDurationWithErrors(s string) (time.Duration, error) { // ... return 0, OverflowError{errors.New("time: invalid duration, overflow " + orig)} // ... } func clipHAProxyTimeoutValue(val string) string { // ... duration, err := templateutil.ParseDurationWithErrors(val) switch err.(type) { case nil: case templateutil.OverflowError: return haproxyMaxTimeout default: return haproxyDefaultTimeout } // ... }

It has its pros and cons. If a user mistakenly typed "h" instead of "s", this could really cause a problem. If they used a value that was only a small duration of time away from the max timeout, it would be fine.

I incorporated this suggestion, thanks.

It has its pros and cons. If a user mistakenly typed "h" instead of "s", this could really cause a problem. If they used a value that was only a small duration of time away from the max timeout, it would be fine.

I meant, rather than having ParseDurationWithErrors (which is now named ParseHAProxyDuration with your latest changes) return an overflow error and then having clipHAProxyTimeoutValue handle the overflow error by using the max value, why not simply have the parse function return the max value in case of overflows? If you prefer returning an overflow error and handling it in clipHAProxyTimeoutValue, that's fine, I was just thinking you could simplify the logic a little.

My intention was that it would be more easily reused if we left it up to the caller to interpret the error and choose what to do with it.

pkg/router/template/util/util.go

ShudiLi · 2023-05-22T02:59:19Z

From QE side, tested it with 4.13.0-0.ci.test-2023-05-22-014550-ci-ln-qsqqkkt-latest
`
1.
% oc get clusterversion
NAME VERSION AVAILABLE PROGRESSING SINCE STATUS
version 4.13.0-0.ci.test-2023-05-22-014550-ci-ln-qsqqkkt-latest True False 10m Cluster version is 4.13.0-0.ci.test-2023-05-22-014550-ci-ln-qsqqkkt-latest

% oc -n openshift-ingress get pods
NAME READY STATUS RESTARTS AGE
router-default-c9ccc9cc7-hcf2f 1/1 Running 0 32m
router-default-c9ccc9cc7-pj8b2 1/1 Running 0 32m
%

create pod, service and the route

% oc annotate route service-unsecure haproxy.router.openshift.io/timeout=100000000000s --overwrite
route.route.openshift.io/service-unsecure annotated
%

% oc get route service-unsecure -oyaml | grep timeout
haproxy.router.openshift.io/timeout: 100000000000s

% oc -n openshift-ingress rsh router-default-c9ccc9cc7-hcf2f cat haproxy.config | grep -A5 be_http:default:service-unsecure
backend be_http:default:service-unsecure
mode http
option redispatch
option forwardfor
balance random
timeout server 2147483647ms

% oc -n openshift-ingress get pods
NAME READY STATUS RESTARTS AGE
router-default-c9ccc9cc7-hcf2f 1/1 Running 0 52m
router-default-c9ccc9cc7-pj8b2 1/1 Running 0 52m
%
% oc -n openshift-ingress logs router-default-c9ccc9cc7-hcf2f | grep -i error
%

% curl http://service-unsecure-default.apps.ci-ln-qsqqkkt-72292.origin-ci-int-gce.dev.rhcloud.com
Hello-OpenShift-1 http-8080
%

`

/label qe-approved
thanks

candita · 2023-05-24T15:23:05Z

Assigning to @Miciah because he has provided feedback.
/assign @Miciah

candita · 2023-06-01T17:06:52Z

@frobware has an implementation as well: https://github.com/frobware/haproxytime/tree/main

candita · 2023-06-09T21:59:59Z

/test e2e-agnostic

candita · 2023-06-09T22:08:17Z

e2e-agnostic failure:

failed: (55.3s) 2023-06-09T17:20:52 "[sig-api-machinery] Aggregator Should be able to support the 1.17 Sample API Server using the current Aggregator [Conformance] [Suite:openshift/conformance/parallel/minimal]

fail [test/e2e/apimachinery/aggregator.go:474]: could not find group version resource for dynamic client and wardle/flunders (discovery error: unable to retrieve the complete list of server APIs: wardle.example.com/v1alpha1: stale GroupVersion discovery: wardle.example.com/v1alpha1, discovery results: map[schema.GroupVersionResource]struct {}{schema.GroupVersionResource{Group:"", Version:"v1", Resource:"bindings"}:.....

Recent changes for Kube bump:
https://github.com/openshift/origin/blob/master/vendor/k8s.io/kubernetes/test/e2e/apimachinery/aggregator.go#L92

candita · 2023-06-15T18:27:02Z

level=info msg=Cluster operator insights SCAAvailable is False with Forbidden: Failed to pull SCA certs from https://api.openshift.com/api/accounts_mgmt/v1/certificates: OCM API https://api.openshift.com/api/accounts_mgmt/v1/certificates returned HTTP 403: {"code":"ACCT-MGMT-11","href":"/api/accounts_mgmt/v1/errors/11","id":"11","kind":"Error","operation_id":"2cc86d1a-1ea9-4227-b5f2-0dd236eb4020","reason":"Account with ID 2DUeKzzTD9ngfsQ6YgkzdJn1jA4 denied access to perform create on Certificate with HTTP call POST /api/accounts_mgmt/v1/certificates"}
level=info msg=Cluster operator network ManagementStateDegraded is False with :

/test e2e-agnostic

candita · 2023-06-16T20:03:25Z

Seems like a cluster install issue, and it's repeated with different errors for days.

/test e2e-agnostic

pkg/router/template/util/util.go

Miciah · 2023-06-18T01:39:49Z

pkg/router/template/util/util.go

+	error
+}
+
+// InvalidInputError represents an error based on invalid input to ParseHAProxyDuration


Some comments are missing periods.

Suggested change

// InvalidInputError represents an error based on invalid input to ParseHAProxyDuration

// InvalidInputError represents an error based on invalid input to ParseHAProxyDuration.

I think I fixed all of them except this one, will get it in an upcoming update.

Miciah · 2023-06-18T01:56:43Z

pkg/router/template/util/util.go

+		// Consume (\.[0-9]*)?
+		post := false
+		if s != "" && s[0] == '.' {
+			s = s[1:]
+			pl := len(s)
+			f, scale, s = leadingFraction(s)
+			post = pl != len(s)
+		}


The code to support decimal points is still present:

router/pkg/router/template/util/util.go

Lines 164 to 165 in a4b3f60

v, f uint64 // integers before, after decimal point

scale float64 = 1 // value = v + f/scale

router/pkg/router/template/util/util.go

Line 175 in a4b3f60

pl := len(s)

router/pkg/router/template/util/util.go

Lines 181 to 194 in a4b3f60

pre := pl != len(s) // whether we consumed anything before a period

// Consume (\.[0-9]*)?

post := false

if s != "" && s[0] == '.' {

s = s[1:]

pl := len(s)

f, scale, s = leadingFraction(s)

post = pl != len(s)

}

if !pre && !post {

// no digits (e.g. ".s" or "-.s")

return 0, InvalidInputError{errors.New("invalid duration, not a number " + orig)}

}

router/pkg/router/template/util/util.go

Lines 200 to 202 in a4b3f60

if c == '.' || '0' <= c && c <= '9' {

break

}

router/pkg/router/template/util/util.go

Lines 219 to 227 in a4b3f60

if f > 0 {

// float64 is needed to be nanosecond accurate for fractions of hours.

// v >= 0 && (f*unit/scale) <= 3.6e+12 (ns/h, h is the largest unit)

v += uint64(float64(f) * (float64(unit) / scale))

if v > 1<<63 {

// overflow

return HaproxyMaxTimeoutDuration, OverflowError{errors.New("invalid duration, overflow " + orig)}

}

}

As I mentioned in other comments, the regexp disallows decimal points anyway, so there's no need to have logic to parse them.

pkg/router/template/util/util.go

Miciah · 2023-06-18T02:07:15Z

pkg/router/template/util/util.go

+	// Consume [-+]?
+	if s != "" {
+		c := s[0]
+		if c == '-' || c == '+' {
+			neg = c == '-'
+			s = s[1:]
+		}
+	}


On further thought, it makes sense to disallow it irrespective of what HAProxy allows as $timeSpecPattern already disallows it. There's no need to have logic to parse something that would be rejected by the regexp anyway.

Miciah

BTW, as a follow-up, it might make sense to replace clipHAProxyTimeoutValue (firstMatch $timeSpecPattern foo) with clipHAProxyTimeoutValue foo in haproxy-config.template as an optimization.

pkg/router/template/template_helper.go

pkg/router/template/util/util.go

frobware · 2023-06-19T11:30:24Z

pkg/router/template/util/util.go

+// ParseHAProxyDuration is similar to time.ParseDuration, but modified with meaningful
+// error messages for invalid input.
+// It parses a duration string and returns a suitable duration. A duration string is a
+// sequence of decimal numbers, each with optional fraction and a unit suffix,


What does an optional fraction look like for days, hours, minutes, seconds, milliseconds?

Do we have test cases for said fractional values?

What does an optional fraction look like for days, hours, minutes, seconds, milliseconds?

Do we have test cases for said fractional values?

There are a couple test cases for fractional values:

router/pkg/router/template/template_helper_test.go

Lines 824 to 828 in a4b3f60

{

value: "1.5s",

expected: "",

// Invalid input produces blank output

},

router/pkg/router/template/template_helper_test.go

Lines 838 to 842 in a4b3f60

{

value: "2562047.99h",

expected: "",

// Invalid input produces blank output

},

These are not allowed by HAProxy, as Miciah pointed out in #483 (comment) so I removed the tests and hopefully removed all the processing code for decimal points.

frobware · 2023-06-19T11:32:13Z

The PR description has "a value that cannot be properly parsed for other reasons is set to 5s instead of being silently allowed" - will we ever back port this change in behaviour?

Miciah · 2023-06-21T05:00:54Z

The PR description has "a value that cannot be properly parsed for other reasons is set to 5s instead of being silently allowed" - will we ever back port this change in behaviour?

Seems reasonable to me if requested. Silently allowing values that cannot be parsed breaks reloads.

BTW, @candita, can you make sure the information in the PR description makes it into the commit message?

- 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.

openshift-ci · 2023-10-23T21:24:28Z

@candita: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

frobware · 2023-10-24T06:56:10Z

/lgtm

frobware · 2023-10-24T09:40:19Z

I think it's totally reasonable that ParseDuration will parse 0 without reporting a syntax error. The semantic layering is wrong. Not accepting 0 is a property of the clip function; ParseDuration should be the generic case.

Proposal:

Revise the clip function to handle the corner case when the leading character in the input is '0'.
Additionally, revert the recent changes to the regular expression in ParseDuration and the test cases that asserted on leading 0.

/remove-lgtm
/lgtm cancel

frobware · 2023-10-24T09:41:29Z

/hold
I don't know the magic incantation to cancel an existing LGTM.

frobware · 2023-10-24T10:29:13Z

The "/lgtm cancel" turned up.

/hold cancel

candita · 2023-10-27T22:54:54Z

I think it's totally reasonable that ParseDuration will parse 0 without reporting a syntax error. The semantic layering is wrong. Not accepting 0 is a property of the clip function; ParseDuration should be the generic case.

Proposal:

Revise the clip function to handle the corner case when the leading character in the input is '0'.

Additionally, revert the recent changes to the regular expression in ParseDuration and the test cases that asserted on leading 0.

@frobware the HAProxy template does not accept 0 as the leading character. I thought we all agreed to make the regex pattern the same here as it is in HAProxy in #483 (comment). Are you referring to something different?

frobware · 2023-10-30T09:29:29Z

I previously gave this PR an /lgtm, but I've had reservations about the behaviour of haproxytime.ParseDuration—especially after we started removing test cases that involved parsing 0 as a duration. I'd like to present some counter-arguments for reconsideration.

Counter-Argument for Allowing `0` in `haproxytime.ParseDuration`

Principle of Least Astonishment: This principle suggests that our function should behave in a way that most users expect. Allowing 0 as a leading character aligns haproxytime.ParseDuration more closely with Go's native time.ParseDuration, which is likely familiar to many users.
Modularity and Single Responsibility Principle: A function should ideally do one thing well. haproxytime.ParseDuration should focus solely on parsing durations. HAProxy-specific constraints or project-specific requirements, like not allowing 0, should be managed externally. The clip function already alters behaviour by capping the maximum duration to a value much less than what haproxytime.ParseDuration supports (i.e., int64), making it the appropriate place to handle such edge cases.
Edge Cases in clip Function: Given that the constraint on 0 is specific to our use case, this makes it even more fitting to manage this edge case in the clip function, maintaining the modularity and focus of haproxytime.ParseDuration.
Consistency with Standard Libraries: Allowing 0 makes our function more in line with the behaviour of similar functions in Go's standard libraries, thereby making it more intuitive to use and easier to understand.
Ease of Future Maintenance: Making haproxytime.ParseDuration less use-case-specific and more generalised will simplify future maintenance and adaptations. Additionally, disallowing 0 within haproxytime.ParseDuration would create a ripple effect: if another caller is introduced later that does permit 0, this edge case would be perpetuated, complicating the codebase and hindering maintainability.
Leveraging Native Libraries: If Go's time.ParseDuration supported day-level granularity, we would have reused it and worked around the 0 edge case externally, which further argues for handling such adjustments outside of haproxytime.ParseDuration.

Based on these considerations, I propose that we allow 0 as a leading character in haproxytime.ParseDuration and manage any use-case-specific requirements, like the constraint on 0 when clipping, in the clip function.

Miciah · 2023-10-30T17:04:29Z

I understand the argument for consistency. However, I discussed this with @candita, and currently, all values that get parsed using haproxytime.ParseDuration are guarded by $timeSpecPattern; if a use-case arises in the future that requires haproxytime.ParseDuration to allow zero values for some duration value, we can update the function then. In the interest of avoiding continued churn, let's merge the PR in its current state.
/approve
/lgtm

openshift-ci · 2023-10-30T17:06:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Miciah]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2023-10-30T17:12:52Z

@candita: Jira Issue OCPBUGS-6958: All pull requests linked via external trackers have merged:

openshift/router#483

Jira Issue OCPBUGS-6958 has been moved to the MODIFIED state.

In response to this:

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.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-merge-robot · 2023-10-31T09:40:51Z

Fix included in accepted release 4.15.0-0.nightly-2023-10-31-054858

openshift-bot · 2023-11-21T01:31:13Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-haproxy-router-base-container-v4.15.0-202311202349.p0.gfb70ac4.assembly.stream for distgit ose-haproxy-router-base.
All builds following this will include this PR.

candita · 2024-03-11T14:03:39Z

/cherry-pick release-4.14

openshift-cherrypick-robot · 2024-03-11T14:04:21Z

@candita: #483 failed to apply on top of branch "release-4.14":

Applying: OCPBUGS-6958: Fix clipHAProxyTimeoutValue so that:
Applying: add haproxytime
Applying: use pkg/router/template/util/haproxytime
Applying: drop existing ParseHAProxyDuration
Applying: OCPBUGS-6958: Fix clipHAProxyTimeoutValue so that:
Using index info to reconstruct a base tree...
M	pkg/router/template/template_helper.go
M	pkg/router/template/template_helper_test.go
M	pkg/router/template/util/util.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/router/template/util/util.go
Auto-merging pkg/router/template/template_helper_test.go
CONFLICT (content): Merge conflict in pkg/router/template/template_helper_test.go
Auto-merging pkg/router/template/template_helper.go
CONFLICT (content): Merge conflict in pkg/router/template/template_helper.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0005 OCPBUGS-6958: Fix clipHAProxyTimeoutValue so that:
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

* 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 <[email protected]>

Miciah · 2024-05-04T15:00:05Z

/cherry-pick release-4.13

openshift-cherrypick-robot · 2024-05-04T15:00:47Z

@Miciah: #483 failed to apply on top of branch "release-4.13":

Applying: OCPBUGS-6958: Fix clipHAProxyTimeoutValue so that:
Applying: add haproxytime
Applying: use pkg/router/template/util/haproxytime
Applying: drop existing ParseHAProxyDuration
Applying: OCPBUGS-6958: Fix clipHAProxyTimeoutValue so that:
Using index info to reconstruct a base tree...
M	pkg/router/template/template_helper.go
M	pkg/router/template/template_helper_test.go
M	pkg/router/template/util/util.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/router/template/util/util.go
Auto-merging pkg/router/template/template_helper_test.go
CONFLICT (content): Merge conflict in pkg/router/template/template_helper_test.go
Auto-merging pkg/router/template/template_helper.go
CONFLICT (content): Merge conflict in pkg/router/template/template_helper.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0005 OCPBUGS-6958: Fix clipHAProxyTimeoutValue so that:
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.13

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

* 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 <[email protected]>

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels May 19, 2023

openshift-ci bot requested review from alebedev87 and Miciah May 19, 2023 21:22

candita force-pushed the OCPBUGS-6958-FixClipTimeout branch from 4a7018e to b21bc15 Compare May 19, 2023 21:26

openshift-ci bot requested a review from ShudiLi May 19, 2023 21:32

Miciah reviewed May 20, 2023

View reviewed changes

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label May 22, 2023

openshift-ci bot assigned Miciah May 24, 2023

candita force-pushed the OCPBUGS-6958-FixClipTimeout branch 3 times, most recently from 80441cb to a4b3f60 Compare June 9, 2023 16:20

Miciah reviewed Jun 18, 2023

View reviewed changes

frobware reviewed Jun 19, 2023

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2023

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2023

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed lgtm Indicates that a PR is ready to be merged. labels Oct 24, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 24, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 30, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2023

openshift-ci bot merged commit fb70ac4 into openshift:master Oct 30, 2023

Miciah mentioned this pull request Feb 13, 2024

OCPBUGS-14914: Set tunnel and server timeouts at backend level #536

Open

	v, f uint64 // integers before, after decimal point
	scale float64 = 1 // value = v + f/scale

	pre := pl != len(s) // whether we consumed anything before a period

	// Consume (\.[0-9]*)?
	post := false
	if s != "" && s[0] == '.' {
	s = s[1:]
	pl := len(s)
	f, scale, s = leadingFraction(s)
	post = pl != len(s)
	}
	if !pre && !post {
	// no digits (e.g. ".s" or "-.s")
	return 0, InvalidInputError{errors.New("invalid duration, not a number " + orig)}
	}

	if f > 0 {
	// float64 is needed to be nanosecond accurate for fractions of hours.
	// v >= 0 && (f*unit/scale) <= 3.6e+12 (ns/h, h is the largest unit)
	v += uint64(float64(f) * (float64(unit) / scale))
	if v > 1<<63 {
	// overflow
	return HaproxyMaxTimeoutDuration, OverflowError{errors.New("invalid duration, overflow " + orig)}
	}
	}

	// InvalidInputError represents an error based on invalid input to ParseHAProxyDuration
	// InvalidInputError represents an error based on invalid input to ParseHAProxyDuration.

	{
	value: "1.5s",
	expected: "",
	// Invalid input produces blank output
	},

	{
	value: "2562047.99h",
	expected: "",
	// Invalid input produces blank output
	},

OCPBUGS-6958: Fix clipHAProxyTimeoutValue #483

OCPBUGS-6958: Fix clipHAProxyTimeoutValue #483

Conversation

candita commented May 19, 2023 • edited Loading

openshift-ci-robot commented May 19, 2023

candita commented May 19, 2023

openshift-ci-robot commented May 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miciah Jun 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShudiLi commented May 22, 2023 • edited Loading

candita commented May 24, 2023

candita commented Jun 1, 2023

candita commented Jun 9, 2023

candita commented Jun 9, 2023 • edited Loading

candita commented Jun 15, 2023

candita commented Jun 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miciah Jun 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miciah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frobware commented Jun 19, 2023

Miciah commented Jun 21, 2023

openshift-ci bot commented Oct 23, 2023

frobware commented Oct 24, 2023

frobware commented Oct 24, 2023 • edited Loading

frobware commented Oct 24, 2023

frobware commented Oct 24, 2023

candita commented Oct 27, 2023

frobware commented Oct 30, 2023 • edited Loading

Counter-Argument for Allowing 0 in haproxytime.ParseDuration

Miciah commented Oct 30, 2023

openshift-ci bot commented Oct 30, 2023

openshift-ci-robot commented Oct 30, 2023

openshift-merge-robot commented Oct 31, 2023

openshift-bot commented Nov 21, 2023

candita commented Mar 11, 2024

openshift-cherrypick-robot commented Mar 11, 2024

Miciah commented May 4, 2024

openshift-cherrypick-robot commented May 4, 2024

candita commented May 19, 2023 •

edited

Loading

Miciah Jun 18, 2023 •

edited

Loading

ShudiLi commented May 22, 2023 •

edited

Loading

candita commented Jun 9, 2023 •

edited

Loading

Miciah Jun 18, 2023 •

edited

Loading

frobware commented Oct 24, 2023 •

edited

Loading

frobware commented Oct 30, 2023 •

edited

Loading

Counter-Argument for Allowing `0` in `haproxytime.ParseDuration`