Skip to content

Commit

Permalink
Merge pull request #618 from grzpiotrowski/OCPBUGS-38078-haproxy-heal…
Browse files Browse the repository at this point in the history 
…thcheck-interval

OCPBUGS-38078: Validate HAProxy health check interval time value
  • Loading branch information
@openshift-merge-bot
openshift-merge-bot[bot] authored Nov 27, 2024
2 parents be319d0 + bbbe88d commit be831fa
Show file tree
Hide file tree
Showing 3 changed files with 134 additions and 14 deletions.
6 changes: 3 additions & 3 deletions images/router/haproxy/conf/haproxy-config.template
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Expand All @@ -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
Expand Down Expand Up @@ -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 }}
Expand Down
122 changes: 121 additions & 1 deletion pkg/router/router_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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
}
20 changes: 10 additions & 10 deletions pkg/router/template/template_helper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
}

Expand Down

0 comments on commit be831fa

Please sign in to comment.