OCPBUGS-38078: Validate HAProxy health check interval time value - #618
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@grzpiotrowski: This pull request references Jira Issue OCPBUGS-38078, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (iamin@redhat.com), skipping review request. The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@grzpiotrowski: This pull request references Jira Issue OCPBUGS-38078, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (iamin@redhat.com), skipping review request. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@grzpiotrowski unless someone has advised you otherwise, I will request that you do not rename the function or constant. It will make this easier to review and backport, by simplifying the number of changes. |
0e507e4 to
0ecfb1b
Compare
Yes, I didn't consider the ease of backporting in this case. |
|
I was just wondering, should we keep the log messages unchanged or adjust them to say Example of a log for clipping the health check interval now: template_helper.go:356] "msg"="route annotation timeout exceeds maximum allowable by HAProxy, clipping to 2147483647ms" "input"="50000d" "logger"="template" |
Mark bug as verified |
openshift-ci
Bot
added
the
qe-approved
|
/retest |
|
/assign |
|
Unrelated failure in e2e-agnostic.
/test e2e-agnostic |
|
As far as gathering a list of other places we may need to clip values, I found some that are not annotations: router/images/router/haproxy/conf/haproxy-config.template Lines 158 to 167 in 0ecfb1b However, in https://github.com/openshift/cluster-ingress-operator/blob/8252ac492c04d161fbcf60ef82af2989c99f4a9d/pkg/operator/controller/ingress/deployment.go#L610-L634 we do take care of clipping them. |
|
@grzpiotrowski this looks good, but we will need some kind of test with this to make sure there are no regressions over time, and validate that it works as is. Maybe something like adding a few test cases to the tests in |
0ecfb1b to
e9081e6
Compare
|
I finally added the health check interval test cases in the |
e9081e6 to
a77cf40
Compare
|
Rebased after #627 has merged. |
|
/retest-required |
| {{- end }}{{/* end type specific options*/}} | ||
|
|
||
| {{- if and (not $endpoint.NoHealthCheck) (gt $cfg.ActiveEndpoints 1) }} check inter {{firstMatch $timeSpecPattern (index $cfg.Annotations "router.openshift.io/haproxy.health.check.interval") (env "ROUTER_BACKEND_CHECK_INTERVAL") "5000ms" }} | ||
| {{- if and (not $endpoint.NoHealthCheck) (gt $cfg.ActiveEndpoints 1) }} check inter {{ clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "router.openshift.io/haproxy.health.check.interval") (env "ROUTER_BACKEND_CHECK_INTERVAL") "5000ms") }} |
There was a problem hiding this comment.
Because the input "abc" (SyntaxError) should return an empty string, I think we want to remove the 5000ms default out of the parameter list for firstMatch. And remove the env variable check. I'll look into the other places we use clipHAProxyTimeoutValue.
| {{- if and (not $endpoint.NoHealthCheck) (gt $cfg.ActiveEndpoints 1) }} check inter {{ clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "router.openshift.io/haproxy.health.check.interval") (env "ROUTER_BACKEND_CHECK_INTERVAL") "5000ms") }} | |
| {{- if and (not $endpoint.NoHealthCheck) (gt $cfg.ActiveEndpoints 1) }} check inter {{ clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "router.openshift.io/haproxy.health.check.interval")) (env "ROUTER_BACKEND_CHECK_INTERVAL") "5000ms" }} |
There was a problem hiding this comment.
No, I think this is okay after all. Sorry for the noise.
| section: "backend", | ||
| sectionName: insecureBackendName(h.namespace, "r3"), | ||
| attribute: "server", | ||
| value: "inter 5000ms", |
There was a problem hiding this comment.
This is interesting. I would have thought it would return an empty string instead of the default because it is a SyntaxError: https://github.com/openshift/router/blob/master/pkg/router/template/template_helper.go#L345-L347
There was a problem hiding this comment.
No, it doesn't return the empty string because the template defaults it to 5000ms when the empty string is returned. We need to consider the logged message in https://github.com/openshift/router/blob/master/pkg/router/template/template_helper.go#L346
Use clipHAProxyTimeoutValue on router.openshift.io/haproxy.health.check.interval annotation to ensure it is within the range that HAProxy can parse. This commit fixes OCPBUGS-38078. https://issues.redhat.com/browse/OCPBUGS-38078 * images/router/haproxy/conf/haproxy-config.template: Use clipHAProxyTimeoutValue on `router.openshift.io/haproxy.health.check.interval` annotation. * pkg/router/template/template_helper.go (clipHAProxyTimeoutValue): Modify log messages and godoc comments to reflect the more generic use of this function for various HaProxy time annotations.
a77cf40 to
6cc5c3c
Compare
| mustCreateEndpointSlices: []mustCreateEndpointSlice{ | ||
| { | ||
| name: "servicer1", | ||
| serviceName: "servicer1", | ||
| addresses: []string{"1.1.1.1", "1.1.1.2"}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Do you need the mustCreateEndpointSlices in your tests because of the attribute: "server" in the MustMatchConfig?
There was a problem hiding this comment.
Yes, this is why it is needed.
* pkg/router/router_test.go: (TestConfigTemplate): Add test cases for route health check interval annotation. Verify that the correct value is added to the backend server line and that the values exceeding the maximum haproxy time value get clipped to the max limit. Verify that invalid annotation values result in the default health check interval value applied. Test clipping of the health check interval for a passthrough route. (MustCreateEndpointSlice): Add addresses field ((MustCreateEndpointSlice).Apply): Initialize the endpoint's Addresses field to the addresses array from the mustCreateEndpointSlice parameters if specified. This enables having at least two endpoints for the route in the test case and satisfy the conditions needed to configure the check inter in the backend server line. (passthroughBackendName): New helper function to construct a config's backend name for a passthrough route.
6cc5c3c to
bbbe88d
Compare
| } | ||
|
|
||
| // passthroughBackendName contructs the HAProxy config's backend name for a passthrough route. | ||
| func passthroughBackendName(ns, route string) string { |
|
Thanks @grzpiotrowski -- impressive work. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: candita The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
1 similar comment
|
@grzpiotrowski: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
@grzpiotrowski: Jira Issue OCPBUGS-38078: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-38078 has been moved to the MODIFIED state. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
[ART PR BUILD NOTIFIER] Distgit: ose-haproxy-router-base |
|
[ART PR BUILD NOTIFIER] Distgit: openshift-enterprise-haproxy-router |
6 participants
The maximum valid time representing value in HAProxy is 2147483647ms (max positive value for a 32-bit signed integer).
Setting time value to one exceeding the maximum handled by HAProxy for
router.openshift.io/haproxy.health.check.intervalbreaks the router-default pods.Prior to this PR
clipHAProxyTimeoutValuewas used to validate the timeout values set in annotations likehaproxy.router.openshift.io/timeoutorhaproxy.router.openshift.io/timeout-tunnel.By reusing the
clipHAProxyTimeoutValuefunction, this PR adds the same validation for the value set inrouter.openshift.io/haproxy.health.check.intervalannotation to ensure it is within the range that HAProxy can parse.