Bug 1861383: Clip haproxy.router.openshift.io/timeout annotation values to prevent bricking on upgrade #196

sgreene570 · 2020-10-07T14:46:46Z

This PR adds the function clipHAProxyTimeoutValue(...) to pkg/router/template/template_helper.go to prevent invalid Route timeout annotations from breaking router reloads. In version 2.x, HAProxy supports a maximum timeout of 2147483647ms, which is ~24.8 days. clipHAProxyTimeoutValue will replace any timeout above the maximum amount with the maximum allowable amount to prevent HAProxy from crashing after upgrades that bump HAProxy 1.8 to 2.x.

This PR also adds unit tests cases for clipHAProxyTimeoutValue to verify that it works as intended.

openshift-ci-robot · 2020-10-07T14:46:55Z

@sgreene570: This pull request references Bugzilla bug 1861383, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

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

In response to this:

Bug 1861383: Validate haproxy.router.openshift.io/timeout annotation values

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.

sgreene570 · 2020-10-08T13:10:30Z

/retest

sgreene570 · 2020-10-08T16:27:02Z

Doesnt appear to be working as intended when I verified manually
/hold for now

sgreene570 · 2020-10-08T21:20:43Z

I now realize there's a better approach. Pushing a new fix soon.

openshift-ci-robot · 2020-10-09T13:23:03Z

@sgreene570: This pull request references Bugzilla bug 1861383, which is valid.

3 validation(s) were run on this bug

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

In response to this:

Bug 1861383: Clip haproxy.router.openshift.io/timeout annotation values to prevent bricking on upgrade

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-ci-robot · 2020-10-09T13:24:01Z

@sgreene570: This pull request references Bugzilla bug 1861383, which is valid.

3 validation(s) were run on this bug

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

In response to this:

Bug 1861383: Clip haproxy.router.openshift.io/timeout annotation values to prevent bricking on upgrade

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.

sgreene570 · 2020-10-09T13:24:12Z

/hold cancel

sgreene570 · 2020-10-09T15:57:21Z

/retest

Miciah · 2020-10-10T04:39:17Z

images/router/haproxy/conf/haproxy-config.template

@@ -463,7 +463,7 @@ backend {{genBackendNamePrefix $cfg.TLSTermination}}:{{$cfgIdx}}
      {{- end }}
  tcp-request content reject if !whitelist
    {{- end }}
-    {{- with $value := firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout")}}
+    {{- with $value := clipHAProxyTimeoutValue firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout")}}


Suggested change

{{- with $value := clipHAProxyTimeoutValue firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout")}}

{{- with $value := clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout")) }}

openshift-ci-robot · 2020-10-12T17:28:24Z

@sgreene570: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-metal-ipi	`211451e`	link	`/test e2e-metal-ipi`
ci/prow/e2e-metal-ipi-ovn-router	`211451e`	link	`/test e2e-metal-ipi-ovn-router`

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 · 2020-10-22T11:04:18Z

/retest

Should clear metal-ipi jobs that are not mandatory.

sgreene570 · 2020-10-22T13:28:18Z

Should clear metal-ipi jobs that are not mandatory.

hm, don't think that did the trick.
/refresh maybe?

frobware · 2020-10-22T17:32:29Z

hm, don't think that did the trick.
/refresh maybe?

Or close and reopen? I recall we had a similar issue with another PR where the release job criteria had changed.

openshift-ci-robot · 2020-10-22T17:35:23Z

@sgreene570: An error was encountered updating to the NEW state for bug 1861383 on the Bugzilla server at https://bugzilla.redhat.com:

code 32000: Red Hat Bugzilla's database reported a query serialization error. Most likely this occurred because another user or process attempted to change the same data that you were attempting to change. Please press Back and retry the transaction.
 UPDATE bugs SET bug_status = ? WHERE bug_id = ? at /var/www/html/bugzilla/Bugzilla/Object.pm line 544. 
Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

In response to this:

Bug 1861383: Clip haproxy.router.openshift.io/timeout annotation values to prevent bricking on upgrade

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-ci-robot · 2020-10-22T17:35:23Z

@sgreene570: This pull request references Bugzilla bug 1861383, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

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

In response to this:

Bug 1861383: Clip haproxy.router.openshift.io/timeout annotation values to prevent bricking on upgrade

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.

frobware · 2020-10-22T17:37:59Z

pkg/router/template/template_helper_test.go

+			expected: haproxyMaxTimeout,
+		},
+		{
+			value:    "1000h",


And a [test] case without any units defaults to milliseconds?

And a [test] case without any units defaults to milliseconds?

haproxy defaults to milliseconds:

timeout client <timeout> timeout clitimeout <timeout> (deprecated) Set the maximum inactivity time on the client side. May be used in sections : defaults | frontend | listen | backend yes | yes | yes | no Arguments : <timeout> is the timeout value specified in milliseconds by default, but can be in any other unit if the number is suffixed by the unit, as explained at the top of this document.

If no time unit is specific, HAProxy will assume milliseconds.

Added a test to highlight this.

Add a new function, clipHAProxyTimeoutValue(...) to pkg/router/template/template_helper.go that clips values passed in to the template via the haproxy.router.openshift.io/timeout annotation. This way, clusters updating from a cluster version with a prior version of HAProxy (1.8) won't break when an annotation timeout of > 25 days was previously set on a route.

Add a unit test for clipHAProxyTimeoutValue(...) to pkg/router/template/template_helper_test.go

sgreene570 · 2020-10-26T19:49:24Z

/retest

frobware · 2020-11-10T17:48:02Z

/lgtm

openshift-ci-robot · 2020-11-10T17:48:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frobware, sgreene570

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~~ [frobware,sgreene570]

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

sgreene570 · 2020-11-10T19:50:53Z

/skip

sgreene570 · 2020-11-10T20:00:04Z

/test e2e-upgrade

openshift-bot · 2020-11-10T20:43:06Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-merge-robot · 2020-11-10T21:56:04Z

@sgreene570: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-metal-ipi-ovn-router	`211451e`	link	`/test e2e-metal-ipi-ovn-router`

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.

openshift-bot · 2020-11-10T22:27:04Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-11-11T00:17:32Z

@sgreene570: All pull requests linked via external trackers have merged:

openshift/router#196

Bugzilla bug 1861383 has been moved to the MODIFIED state.

In response to this:

Bug 1861383: Clip haproxy.router.openshift.io/timeout annotation values to prevent bricking on upgrade

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.

sgreene570 · 2020-11-11T19:56:11Z

/cherry-pick release-4.6

openshift-cherrypick-robot · 2020-11-11T19:56:19Z

@sgreene570: new pull request created: #217

In response to this:

/cherry-pick release-4.6

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.

sgreene570 · 2020-11-11T20:04:04Z

/cherry-pick release-4.5

openshift-cherrypick-robot · 2020-11-11T20:04:06Z

@sgreene570: #196 failed to apply on top of branch "release-4.5":

Applying: template: Add timeout value helper function
Applying: template: Add unit tests for clipHAProxyTimeoutValue(...)
Using index info to reconstruct a base tree...
M	pkg/router/template/template_helper_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/router/template/template_helper_test.go
CONFLICT (content): Merge conflict in pkg/router/template/template_helper_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 template: Add unit tests for clipHAProxyTimeoutValue(...)
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.5

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-ci-robot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Oct 7, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Oct 7, 2020

openshift-ci-robot requested review from danehans and ironcladlou October 7, 2020 14:47

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 7, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 8, 2020

sgreene570 force-pushed the bz-1861383 branch from d4856eb to b3b6083 Compare October 8, 2020 16:30

sgreene570 changed the title ~~Bug 1861383: Validate haproxy.router.openshift.io/timeout annotation values~~ [WIP] Bug 1861383: Validate haproxy.router.openshift.io/timeout annotation values Oct 8, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 8, 2020

sgreene570 force-pushed the bz-1861383 branch from 54504bb to 2847d9f Compare October 9, 2020 13:20

sgreene570 changed the title ~~[WIP] Bug 1861383: Validate haproxy.router.openshift.io/timeout annotation values~~ Bug 1861383: Clip haproxy.router.openshift.io/timeout annotation values to prevent bricking on upgrade Oct 9, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 9, 2020

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

sgreene570 force-pushed the bz-1861383 branch from 2847d9f to 08ddd44 Compare October 9, 2020 13:26

Miciah reviewed Oct 10, 2020

View reviewed changes

sgreene570 force-pushed the bz-1861383 branch 3 times, most recently from 67b4600 to 211451e Compare October 12, 2020 14:49

sgreene570 closed this Oct 22, 2020

sgreene570 reopened this Oct 22, 2020

frobware reviewed Oct 22, 2020

View reviewed changes

sgreene570 force-pushed the bz-1861383 branch 2 times, most recently from e7ecfce to 0e871e0 Compare October 22, 2020 18:12

sgreene570 added 2 commits October 26, 2020 11:16

template: Add unit tests for clipHAProxyTimeoutValue(...)

6bde7a0

Add a unit test for clipHAProxyTimeoutValue(...) to pkg/router/template/template_helper_test.go

sgreene570 force-pushed the bz-1861383 branch from 0e871e0 to 6bde7a0 Compare October 26, 2020 15:16

openshift-ci-robot assigned frobware Nov 10, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 10, 2020

openshift-merge-robot merged commit def7ce9 into openshift:master Nov 11, 2020

openshift-cherrypick-robot mentioned this pull request Nov 11, 2020

[release-4.6] Bug 1896905: Clip haproxy.router.openshift.io/timeout annotation values to prevent bricking on upgrade #217

Merged

sgreene570 mentioned this pull request Nov 11, 2020

[release-4.5] Bug 1896914: Back port Clip haproxy.router.openshift.io/timeout annotation values to prevent bricking on upgrade #218

Merged

stbenjam mentioned this pull request Aug 30, 2022

Duplicate e2e tests with their corresponding apigroup labels openshift/release#31818

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1861383: Clip haproxy.router.openshift.io/timeout annotation values to prevent bricking on upgrade #196

Bug 1861383: Clip haproxy.router.openshift.io/timeout annotation values to prevent bricking on upgrade #196

sgreene570 commented Oct 7, 2020 •

edited

openshift-ci-robot commented Oct 7, 2020

sgreene570 commented Oct 8, 2020

sgreene570 commented Oct 8, 2020

sgreene570 commented Oct 8, 2020

openshift-ci-robot commented Oct 9, 2020

openshift-ci-robot commented Oct 9, 2020

sgreene570 commented Oct 9, 2020

sgreene570 commented Oct 9, 2020

Miciah Oct 10, 2020

sgreene570 Oct 12, 2020

openshift-ci-robot commented Oct 12, 2020

frobware commented Oct 22, 2020

sgreene570 commented Oct 22, 2020

frobware commented Oct 22, 2020

openshift-ci-robot commented Oct 22, 2020

openshift-ci-robot commented Oct 22, 2020

frobware Oct 22, 2020

frobware Oct 22, 2020

sgreene570 Oct 22, 2020

sgreene570 commented Oct 26, 2020

frobware commented Nov 10, 2020

openshift-ci-robot commented Nov 10, 2020

sgreene570 commented Nov 10, 2020

sgreene570 commented Nov 10, 2020

openshift-bot commented Nov 10, 2020

openshift-merge-robot commented Nov 10, 2020 •

edited

openshift-bot commented Nov 10, 2020

openshift-ci-robot commented Nov 11, 2020

sgreene570 commented Nov 11, 2020

openshift-cherrypick-robot commented Nov 11, 2020

sgreene570 commented Nov 11, 2020

openshift-cherrypick-robot commented Nov 11, 2020

	{{- with $value := clipHAProxyTimeoutValue firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout")}}
	{{- with $value := clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout")) }}

Bug 1861383: Clip haproxy.router.openshift.io/timeout annotation values to prevent bricking on upgrade #196

Bug 1861383: Clip haproxy.router.openshift.io/timeout annotation values to prevent bricking on upgrade #196

Conversation

sgreene570 commented Oct 7, 2020 • edited

openshift-ci-robot commented Oct 7, 2020

sgreene570 commented Oct 8, 2020

sgreene570 commented Oct 8, 2020

sgreene570 commented Oct 8, 2020

openshift-ci-robot commented Oct 9, 2020

openshift-ci-robot commented Oct 9, 2020

sgreene570 commented Oct 9, 2020

sgreene570 commented Oct 9, 2020

Miciah Oct 10, 2020

Choose a reason for hiding this comment

sgreene570 Oct 12, 2020

Choose a reason for hiding this comment

openshift-ci-robot commented Oct 12, 2020

frobware commented Oct 22, 2020

sgreene570 commented Oct 22, 2020

frobware commented Oct 22, 2020

openshift-ci-robot commented Oct 22, 2020

openshift-ci-robot commented Oct 22, 2020

frobware Oct 22, 2020

Choose a reason for hiding this comment

frobware Oct 22, 2020

Choose a reason for hiding this comment

sgreene570 Oct 22, 2020

Choose a reason for hiding this comment

sgreene570 commented Oct 26, 2020

frobware commented Nov 10, 2020

openshift-ci-robot commented Nov 10, 2020

sgreene570 commented Nov 10, 2020

sgreene570 commented Nov 10, 2020

openshift-bot commented Nov 10, 2020

openshift-merge-robot commented Nov 10, 2020 • edited

openshift-bot commented Nov 10, 2020

openshift-ci-robot commented Nov 11, 2020

sgreene570 commented Nov 11, 2020

openshift-cherrypick-robot commented Nov 11, 2020

sgreene570 commented Nov 11, 2020

openshift-cherrypick-robot commented Nov 11, 2020

sgreene570 commented Oct 7, 2020 •

edited

openshift-merge-robot commented Nov 10, 2020 •

edited