From 76e36c722f5f07c7a23d93036e98671f78ac42e7 Mon Sep 17 00:00:00 2001 From: Grzegorz Piotrowski Date: Fri, 16 Aug 2024 12:59:16 +0100 Subject: [PATCH 1/2] Use ClipHAProxyTimeoutValue for HAProxy health check interval 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. --- .../haproxy/conf/haproxy-config.template | 6 +++--- pkg/router/template/template_helper.go | 20 +++++++++---------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/images/router/haproxy/conf/haproxy-config.template b/images/router/haproxy/conf/haproxy-config.template index 0f6063e67..1e9f29cb8 100644 --- a/images/router/haproxy/conf/haproxy-config.template +++ b/images/router/haproxy/conf/haproxy-config.template @@ -749,7 +749,7 @@ backend {{ genBackendNamePrefix $cfg.TLSTermination }}:{{ $cfgIdx }} {{- end }} {{- 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") }} {{- end }}{{/* end else no health check */}} {{- with $podMaxConn := index $cfg.Annotations "haproxy.router.openshift.io/pod-concurrent-connections" }} {{- if (isInteger (index $cfg.Annotations "haproxy.router.openshift.io/pod-concurrent-connections")) }} maxconn {{$podMaxConn }} {{- end }} @@ -764,7 +764,7 @@ backend {{ genBackendNamePrefix $cfg.TLSTermination }}:{{ $cfgIdx }} {{- if (eq $cfg.TLSTermination "reencrypt") }} dynamic-cookie-key {{ $cfg.RoutingKeyName }} {{- range $idx, $serverName := $dynamicConfigManager.GenerateDynamicServerNames $cfgIdx }} - server {{ $serverName }} 172.4.0.4:8765 weight 0 ssl disabled check inter {{ firstMatch $timeSpecPattern (index $cfg.Annotations "router.openshift.io/haproxy.health.check.interval") (env "ROUTER_BACKEND_CHECK_INTERVAL") "5000ms" }} + server {{ $serverName }} 172.4.0.4:8765 weight 0 ssl disabled check inter {{ clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "router.openshift.io/haproxy.health.check.interval") (env "ROUTER_BACKEND_CHECK_INTERVAL") "5000ms") }} {{- if gt (len (index $cfg.Certificates (printf "%s_pod" $cfg.Host)).Contents) 0 }} verify required ca-file {{ $workingDir }}/router/cacerts/{{$cfgIdx }}.pem {{- else }} {{- if not (isTrue $router_disable_http2) }} alpn h2,http/1.1 @@ -845,7 +845,7 @@ backend {{ genBackendNamePrefix $cfg.TLSTermination }}:{{ $cfgIdx }} {{- with $serviceUnit := index $.ServiceUnits $serviceUnitName }} {{- range $idx, $endpoint := processEndpointsForAlias $cfg $serviceUnit (env "ROUTER_BACKEND_PROCESS_ENDPOINTS" "") }} server {{ $endpoint.ID }} {{ $endpoint.IP }}:{{ $endpoint.Port }} weight {{ $weight }} - {{- 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") }} {{- end }}{{/* end else no health check */}} {{- with $podMaxConn := index $cfg.Annotations "haproxy.router.openshift.io/pod-concurrent-connections" }} {{- if (isInteger (index $cfg.Annotations "haproxy.router.openshift.io/pod-concurrent-connections")) }} maxconn {{$podMaxConn }} {{- end }} diff --git a/pkg/router/template/template_helper.go b/pkg/router/template/template_helper.go index 153e2b82c..df0024b91 100644 --- a/pkg/router/template/template_helper.go +++ b/pkg/router/template/template_helper.go @@ -320,14 +320,14 @@ 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, or by +// from using time values specified via the annotations +// 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. +// time string value is not parsable as a valid time duration. +// Return the largest HAProxy time if the input value exceeds it. +// Return the default time (5s) if there is another error. func clipHAProxyTimeoutValue(val string) string { // If the empty string is passed in, // simply return the empty string. @@ -340,20 +340,20 @@ func clipHAProxyTimeoutValue(val string) string { switch err { case nil: case haproxytime.OverflowError: - log.Info("route annotation timeout exceeds maximum allowable format, clipping to "+templateutil.HaproxyMaxTimeout, "input", val) + log.Info("route annotation time value 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) + log.Error(err, "route annotation time value ignored or defaulted 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) + log.Info("invalid route annotation time value, setting to "+templateutil.HaproxyDefaultTimeout, "input", val) return templateutil.HaproxyDefaultTimeout } - // Then check to see if the timeout is larger than what HAProxy allows + // Then check to see if the time 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) + log.Info("route annotation time value exceeds maximum allowable by HAProxy, clipping to "+templateutil.HaproxyMaxTimeout, "input", val) return templateutil.HaproxyMaxTimeout } From bbbe88d53ff1948b931df5c9a0ba24c26b6a9652 Mon Sep 17 00:00:00 2001 From: Grzegorz Piotrowski Date: Wed, 16 Oct 2024 14:57:30 +0100 Subject: [PATCH 2/2] Add test cases for health check interval value * 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. --- pkg/router/router_test.go | 122 +++++++++++++++++++++++++++++++++++++- 1 file changed, 121 insertions(+), 1 deletion(-) diff --git a/pkg/router/router_test.go b/pkg/router/router_test.go index 889a1289e..5821d75eb 100644 --- a/pkg/router/router_test.go +++ b/pkg/router/router_test.go @@ -802,6 +802,115 @@ func TestConfigTemplate(t *testing.T) { }, }, }, + "valid route health check interval annotation": { + mustCreateWithConfig{ + mustCreateEndpointSlices: []mustCreateEndpointSlice{ + { + name: "servicer1", + serviceName: "servicer1", + addresses: []string{"1.1.1.1", "1.1.1.2"}, + }, + }, + mustCreateRoute: mustCreateRoute{ + name: "r1", + host: "r1example.com", + targetServiceName: "servicer1", + weight: 1, + time: start, + annotations: map[string]string{ + "router.openshift.io/haproxy.health.check.interval": "10s", + }, + }, + mustMatchConfig: mustMatchConfig{ + section: "backend", + sectionName: insecureBackendName(h.namespace, "r1"), + attribute: "server", + value: "inter 10s", + }, + }, + }, + "route health check interval annotation exceeds the haproxy maximum": { + mustCreateWithConfig{ + mustCreateEndpointSlices: []mustCreateEndpointSlice{ + { + name: "servicer2", + serviceName: "servicer2", + addresses: []string{"1.1.1.1", "1.1.1.2"}, + }, + }, + mustCreateRoute: mustCreateRoute{ + name: "r2", + host: "r2example.com", + targetServiceName: "servicer2", + weight: 1, + time: start, + annotations: map[string]string{ + "router.openshift.io/haproxy.health.check.interval": "5000d", + }, + }, + mustMatchConfig: mustMatchConfig{ + section: "backend", + sectionName: insecureBackendName(h.namespace, "r2"), + attribute: "server", + value: "inter 2147483647ms", + }, + }, + }, + "invalid route health check interval annotation": { + mustCreateWithConfig{ + mustCreateEndpointSlices: []mustCreateEndpointSlice{ + { + name: "servicer3", + serviceName: "servicer3", + addresses: []string{"1.1.1.1", "1.1.1.2"}, + }, + }, + mustCreateRoute: mustCreateRoute{ + name: "r3", + host: "r3example.com", + targetServiceName: "servicer3", + weight: 1, + time: start, + annotations: map[string]string{ + "router.openshift.io/haproxy.health.check.interval": "abc", + }, + }, + mustMatchConfig: mustMatchConfig{ + section: "backend", + sectionName: insecureBackendName(h.namespace, "r3"), + attribute: "server", + value: "inter 5000ms", + }, + }, + }, + "passthrough route health check interval annotation exceeds the haproxy maximum": { + mustCreateWithConfig{ + mustCreateEndpointSlices: []mustCreateEndpointSlice{ + { + name: "servicer4", + serviceName: "servicer4", + addresses: []string{"1.1.1.1", "1.1.1.2"}, + }, + }, + mustCreateRoute: mustCreateRoute{ + name: "r4", + host: "r4example.com", + targetServiceName: "servicer4", + weight: 1, + time: start, + annotations: map[string]string{ + "router.openshift.io/haproxy.health.check.interval": "9000d", + }, + tlsTermination: routev1.TLSTerminationPassthrough, + }, + mustMatchConfig: mustMatchConfig{ + section: "backend", + sectionName: passthroughBackendName(h.namespace, "r4"), + attribute: "server", + value: "inter 2147483647ms", + }, + }, + }, } defer cleanUpRoutes(t) @@ -963,6 +1072,8 @@ type mustCreateEndpointSlice struct { serviceName string // appProtocol is the appProtocol of the endpointslice. appProtocol string + // addresses is the addresses field of the endpoint + addresses []string } func (e mustCreateEndpointSlice) Apply(h *harness) error { @@ -973,6 +1084,10 @@ func (e mustCreateEndpointSlice) Apply(h *harness) error { if e.appProtocol != "" { appProtocol = &e.appProtocol } + addresses := []string{"1.1.1.1"} + if len(e.addresses) > 0 { + addresses = e.addresses + } ep := &discoveryv1.EndpointSlice{ ObjectMeta: metav1.ObjectMeta{ Namespace: h.namespace, @@ -983,7 +1098,7 @@ func (e mustCreateEndpointSlice) Apply(h *harness) error { UID: h.nextUID(), }, Endpoints: []discoveryv1.Endpoint{{ - Addresses: []string{"1.1.1.1"}, + Addresses: addresses, }}, Ports: []discoveryv1.EndpointPort{{ AppProtocol: appProtocol, @@ -1246,3 +1361,8 @@ func insecureBackendName(ns, route string) string { func reencryptBackendName(ns, route string) string { return "be_secure:" + ns + ":" + route } + +// passthroughBackendName contructs the HAProxy config's backend name for a passthrough route. +func passthroughBackendName(ns, route string) string { + return "be_tcp:" + ns + ":" + route +}