From c1b2a019197b50493f0b55d672a438823e508a38 Mon Sep 17 00:00:00 2001 From: Andrey Lebedev Date: Fri, 21 Jun 2024 11:34:07 +0200 Subject: [PATCH] tmp: review - default timeouts retrieved once - max timeout computed once - maxTimeout function benchmarked: values separated from routes (x2) - unit test updated: no routes, no default values, no pattern match --- Makefile | 9 ++ .../haproxy/conf/haproxy-config.template | 29 ++--- pkg/router/template/template_helper.go | 11 +- pkg/router/template/template_helper_test.go | 118 +++++++++++++++++- 4 files changed, 147 insertions(+), 20 deletions(-) diff --git a/Makefile b/Makefile index 466487a64..9beb58f5b 100644 --- a/Makefile +++ b/Makefile @@ -25,6 +25,11 @@ GO_LDFLAGS ?=-ldflags "-s -w $(call version-ldflags,$(PACKAGE)/pkg/version) $(GO GO=GO111MODULE=on GOFLAGS=-mod=vendor go GO_BUILD_RECIPE=CGO_ENABLED=1 $(GO) build -o $(BIN) $(GO_GCFLAGS) $(GO_LDFLAGS) $(MAIN_PACKAGE) +BENCH_PKGS ?= $(shell \grep -lR '^func Benchmark' | xargs -I {} dirname {} | sed 's|^|./|' | paste -sd ' ') +BENCH_TEST ?= . +BENCH_TIME ?= 2s +BENCH_COUNT ?= 5 + all: build build: @@ -39,6 +44,10 @@ images/router/*/Dockerfile.rhel: images/router/base/Dockerfile.rhel check: CGO_ENABLED=1 $(GO) test -race ./... +.PHONY: bench +bench: + CGO_ENABLED=1 $(GO) test -bench=$(BENCH_TEST) -run='^#' -benchtime=$(BENCH_TIME) -count=$(BENCH_COUNT) -benchmem $(BENCH_PKGS) + .PHONY: verify verify: hack/verify-gofmt.sh diff --git a/images/router/haproxy/conf/haproxy-config.template b/images/router/haproxy/conf/haproxy-config.template index 8e20c0753..94df3bf6a 100644 --- a/images/router/haproxy/conf/haproxy-config.template +++ b/images/router/haproxy/conf/haproxy-config.template @@ -9,6 +9,8 @@ {{- $dynamicConfigManager := .DynamicConfigManager }} {{- $router_ip_v4_v6_mode := env "ROUTER_IP_V4_V6_MODE" "v4" }} {{- $router_disable_http2 := env "ROUTER_DISABLE_HTTP2" "false" }} +{{- $routerDefaultServerTimeout := env "ROUTER_DEFAULT_SERVER_TIMEOUT" "30s" }} +{{- $routerDefaultTunnelTimeout := env "ROUTER_DEFAULT_TUNNEL_TIMEOUT" "1h" }} {{- $haveClientCA := .HaveClientCA }} {{- $haveCRLs := .HaveCRLs }} @@ -42,6 +44,9 @@ {{- /* pathRewriteTargetPattern: Match path rewrite-Target */}} {{- $pathRewriteTargetPattern := `^/.*$` -}} +{{- /* Maximum timeout among all the routes, required to be set on the middle backends to avoid warning message about missing server timeout. */}} +{{- $routerMaxServerTimeout := maxTimeoutFirstMatchedAndClipped .State "haproxy.router.openshift.io/timeout" $timeSpecPattern $routerDefaultServerTimeout }} + global # Drop resource limit checks to mitigate https://issues.redhat.com/browse/OCPBUGS-21803 in HAProxy 2.6. no strict-limits @@ -314,9 +319,7 @@ frontend public_ssl # traffic ########################################################################## backend be_sni - {{- with $value := maxTimeoutFirstMatchedAndClipped .State "haproxy.router.openshift.io/timeout" $timeSpecPattern (env "ROUTER_DEFAULT_SERVER_TIMEOUT") "30s" }} - timeout server {{ $value }} - {{- end }} + timeout server {{ $routerMaxServerTimeout }} server fe_sni unix@/var/lib/haproxy/run/haproxy-sni.sock weight 1 send-proxy frontend fe_sni @@ -433,9 +436,7 @@ frontend fe_sni ########################################################################## # backend for when sni does not exist, or ssl term needs to happen on the edge backend be_no_sni - {{- with $value := maxTimeoutFirstMatchedAndClipped .State "haproxy.router.openshift.io/timeout" $timeSpecPattern (env "ROUTER_DEFAULT_SERVER_TIMEOUT") "30s" }} - timeout server {{ $value }} - {{- end }} + timeout server {{ $routerMaxServerTimeout }} server fe_no_sni unix@/var/lib/haproxy/run/haproxy-no-sni.sock weight 1 send-proxy frontend fe_no_sni @@ -595,11 +596,11 @@ backend {{ genBackendNamePrefix $cfg.TLSTermination }}:{{ $cfgIdx }} {{- end }} tcp-request content reject if !whitelist {{- end }} - {{- with $value := clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout") (env "ROUTER_DEFAULT_SERVER_TIMEOUT") "30s") }} - timeout server {{ $value }} + {{- with $value := clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout") $routerDefaultServerTimeout) }} + timeout server {{ $value }} {{- end }} - {{- with $value := clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout-tunnel") (env "ROUTER_DEFAULT_TUNNEL_TIMEOUT") "1h") }} - timeout tunnel {{ $value }} + {{- with $value := clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout-tunnel") $routerDefaultTunnelTimeout) }} + timeout tunnel {{ $value }} {{- end }} {{- if isTrue (index $cfg.Annotations "haproxy.router.openshift.io/rate-limit-connections") }} @@ -799,11 +800,11 @@ backend {{ genBackendNamePrefix $cfg.TLSTermination }}:{{ $cfgIdx }} {{- end }} tcp-request content reject if !whitelist {{- end }} - {{- with $value := clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout") (env "ROUTER_DEFAULT_SERVER_TIMEOUT") "30s") }} - timeout server {{ $value }} + {{- with $value := clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout") $routerDefaultServerTimeout) }} + timeout server {{ $value }} {{- end }} - {{- with $value := clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout-tunnel") (index $cfg.Annotations "haproxy.router.openshift.io/timeout") (env "ROUTER_DEFAULT_TUNNEL_TIMEOUT") "1h") }} - timeout tunnel {{ $value }} + {{- with $value := clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout-tunnel") (index $cfg.Annotations "haproxy.router.openshift.io/timeout") $routerDefaultTunnelTimeout) }} + timeout tunnel {{ $value }} {{- end }} {{- if isTrue (index $cfg.Annotations "haproxy.router.openshift.io/rate-limit-connections") }} diff --git a/pkg/router/template/template_helper.go b/pkg/router/template/template_helper.go index d4724f5b7..5511ac692 100644 --- a/pkg/router/template/template_helper.go +++ b/pkg/router/template/template_helper.go @@ -396,8 +396,9 @@ func maxTimeoutFirstMatchedAndClipped(aliases map[ServiceAliasConfigKey]ServiceA max string maxDuration time.Duration ) + // find max timeout in route annotations for _, cfg := range aliases { - timeout := clipHAProxyTimeoutValue(firstMatch(pattern, append([]string{cfg.Annotations[annotation]}, values...)...)) + timeout := clipHAProxyTimeoutValue(firstMatch(pattern, cfg.Annotations[annotation])) if timeout != "" { // No error handling because clipHAProxyTimeoutValue returns // a valid timeout or an empty string. The latter is already handled. @@ -408,6 +409,14 @@ func maxTimeoutFirstMatchedAndClipped(aliases map[ServiceAliasConfigKey]ServiceA } } } + // use values if no max was found in routes + if max == "" { + max = clipHAProxyTimeoutValue(firstMatch(pattern, values...)) + } + // use max haproxy timeout if no max was found + if max == "" { + max = templateutil.HaproxyMaxTimeout + } return max } diff --git a/pkg/router/template/template_helper_test.go b/pkg/router/template/template_helper_test.go index 300ee3eb9..0847670be 100644 --- a/pkg/router/template/template_helper_test.go +++ b/pkg/router/template/template_helper_test.go @@ -1068,7 +1068,7 @@ func TestParseIPList(t *testing.T) { } } -func TestMaxTimeoutFirstMatchedAndClipped(t *testing.T) { +func Test_maxTimeoutFirstMatchedAndClipped(t *testing.T) { testCases := []struct { name string state map[ServiceAliasConfigKey]ServiceAliasConfig @@ -1115,7 +1115,7 @@ func TestMaxTimeoutFirstMatchedAndClipped(t *testing.T) { expected: "30s", }, { - name: "Maximum default timeout is choosen", + name: "First default timeout is choosen", state: map[ServiceAliasConfigKey]ServiceAliasConfig{ "test:route1": {}, "test:route2": {}, @@ -1123,11 +1123,11 @@ func TestMaxTimeoutFirstMatchedAndClipped(t *testing.T) { }, annotation: "haproxy.router.openshift.io/timeout", pattern: `[1-9][0-9]*(us|ms|s|m|h|d)?`, - values: []string{"40s", "30s"}, - expected: "40s", + values: []string{"30s", "40s"}, + expected: "30s", }, { - name: "Route timeout doesn't match", + name: "One route timeout doesn't match pattern", state: map[ServiceAliasConfigKey]ServiceAliasConfig{ "test:route1": { Annotations: map[string]string{ @@ -1145,6 +1145,25 @@ func TestMaxTimeoutFirstMatchedAndClipped(t *testing.T) { values: []string{"30s"}, expected: "35s", }, + { + name: "No route timeout matches pattern", + state: map[ServiceAliasConfigKey]ServiceAliasConfig{ + "test:route1": { + Annotations: map[string]string{ + "haproxy.router.openshift.io/timeout": "5minutes", + }, + }, + "test:route2": { + Annotations: map[string]string{ + "haproxy.router.openshift.io/timeout": "35seconds", + }, + }, + }, + annotation: "haproxy.router.openshift.io/timeout", + pattern: `[1-9][0-9]*(us|ms|s|m|h|d)?`, + values: []string{"30s"}, + expected: "30s", + }, { name: "Route timeout clipped", state: map[ServiceAliasConfigKey]ServiceAliasConfig{ @@ -1159,6 +1178,33 @@ func TestMaxTimeoutFirstMatchedAndClipped(t *testing.T) { values: []string{"30s"}, expected: "2147483647ms", }, + { + name: "Default timeout overflows but route takes precedence", + state: map[ServiceAliasConfigKey]ServiceAliasConfig{ + "test:route1": { + Annotations: map[string]string{ + "haproxy.router.openshift.io/timeout": "30s", + }, + }, + }, + annotation: "haproxy.router.openshift.io/timeout", + pattern: `[1-9][0-9]*(us|ms|s|m|h|d)?`, + values: []string{"999999999s"}, + expected: "30s", + }, + { + name: "Empty state", + annotation: "haproxy.router.openshift.io/timeout", + pattern: `[1-9][0-9]*(us|ms|s|m|h|d)?`, + values: []string{"30s"}, + expected: "30s", + }, + { + name: "Empty state and values", + annotation: "haproxy.router.openshift.io/timeout", + pattern: `[1-9][0-9]*(us|ms|s|m|h|d)?`, + expected: "2147483647ms", + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { @@ -1169,3 +1215,65 @@ func TestMaxTimeoutFirstMatchedAndClipped(t *testing.T) { }) } } + +func Benchmark_maxTimeoutFirstMatchedAndClipped(b *testing.B) { + testCases := []struct { + name string + state map[ServiceAliasConfigKey]ServiceAliasConfig + annotation string + pattern string + values []string + }{ + { + name: "Input1", + state: map[ServiceAliasConfigKey]ServiceAliasConfig{ + "test:route1": { + Annotations: map[string]string{ + "haproxy.router.openshift.io/timeout": "5m", + }, + }, + }, + annotation: "haproxy.router.openshift.io/timeout", + pattern: `[1-9][0-9]*(us|ms|s|m|h|d)?`, + values: []string{"30s"}, + }, + { + name: "Input5", + state: map[ServiceAliasConfigKey]ServiceAliasConfig{ + "test:route1": { + Annotations: map[string]string{ + "haproxy.router.openshift.io/timeout": "5m", + }, + }, + "test:route2": { + Annotations: map[string]string{ + "haproxy.router.openshift.io/timeout": "35s", + }, + }, + "test:route3": { + Annotations: map[string]string{ + "haproxy.router.openshift.io/timeout-tunnel": "35s", + }, + }, + "test:route4": { + Annotations: map[string]string{ + "haproxy.router.openshift.io/timeout-tunnel": "10m", + }, + }, + "test:route5": {}, + }, + annotation: "haproxy.router.openshift.io/timeout", + pattern: `[1-9][0-9]*(us|ms|s|m|h|d)?`, + // valid value is the last one to force all the iterations + values: []string{"", "", "", "", "30s"}, + }, + } + for _, tc := range testCases { + b.ResetTimer() + b.Run(tc.name, func(b *testing.B) { + for n := 0; n < b.N; n++ { + maxTimeoutFirstMatchedAndClipped(tc.state, tc.annotation, tc.pattern, tc.values...) + } + }) + } +}