Skip to content

Commit c1b2a01

Browse files
alebedev87alebedev87
committed
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
1 parent 5cee412 commit c1b2a01

File tree

4 files changed

+147
-20
lines changed

4 files changed

+147
-20
lines changed

‎Makefile‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,11 @@ GO_LDFLAGS ?=-ldflags "-s -w $(call version-ldflags,$(PACKAGE)/pkg/version) $(GO
2525
GO=GO111MODULE=on GOFLAGS=-mod=vendor go
2626
GO_BUILD_RECIPE=CGO_ENABLED=1 $(GO) build -o $(BIN) $(GO_GCFLAGS) $(GO_LDFLAGS) $(MAIN_PACKAGE)
2727

28+
BENCH_PKGS ?= $(shell \grep -lR '^func Benchmark' | xargs -I {} dirname {} | sed 's|^|./|' | paste -sd ' ')
29+
BENCH_TEST ?= .
30+
BENCH_TIME ?= 2s
31+
BENCH_COUNT ?= 5
32+
2833
all: build
2934

3035
build:
@@ -39,6 +44,10 @@ images/router/*/Dockerfile.rhel: images/router/base/Dockerfile.rhel
3944
check:
4045
CGO_ENABLED=1 $(GO) test -race ./...
4146

47+
.PHONY: bench
48+
bench:
49+
CGO_ENABLED=1 $(GO) test -bench=$(BENCH_TEST) -run='^#' -benchtime=$(BENCH_TIME) -count=$(BENCH_COUNT) -benchmem $(BENCH_PKGS)
50+
4251
.PHONY: verify
4352
verify:
4453
hack/verify-gofmt.sh

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

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99
{{- $dynamicConfigManager := .DynamicConfigManager }}
1010
{{- $router_ip_v4_v6_mode := env "ROUTER_IP_V4_V6_MODE" "v4" }}
1111
{{- $router_disable_http2 := env "ROUTER_DISABLE_HTTP2" "false" }}
12+
{{- $routerDefaultServerTimeout := env "ROUTER_DEFAULT_SERVER_TIMEOUT" "30s" }}
13+
{{- $routerDefaultTunnelTimeout := env "ROUTER_DEFAULT_TUNNEL_TIMEOUT" "1h" }}
1214
{{- $haveClientCA := .HaveClientCA }}
1315
{{- $haveCRLs := .HaveCRLs }}
1416

@@ -42,6 +44,9 @@
4244
{{- /* pathRewriteTargetPattern: Match path rewrite-Target */}}
4345
{{- $pathRewriteTargetPattern := `^/.*$` -}}
4446

47+
{{- /* Maximum timeout among all the routes, required to be set on the middle backends to avoid warning message about missing server timeout. */}}
48+
{{- $routerMaxServerTimeout := maxTimeoutFirstMatchedAndClipped .State "haproxy.router.openshift.io/timeout" $timeSpecPattern $routerDefaultServerTimeout }}
49+
4550
global
4651
# Drop resource limit checks to mitigate https://issues.redhat.com/browse/OCPBUGS-21803 in HAProxy 2.6.
4752
no strict-limits
@@ -314,9 +319,7 @@ frontend public_ssl
314319
# traffic
315320
##########################################################################
316321
backend be_sni
317-
{{- with $value := maxTimeoutFirstMatchedAndClipped .State "haproxy.router.openshift.io/timeout" $timeSpecPattern (env "ROUTER_DEFAULT_SERVER_TIMEOUT") "30s" }}
318-
timeout server {{ $value }}
319-
{{- end }}
322+
timeout server {{ $routerMaxServerTimeout }}
320323
server fe_sni unix@/var/lib/haproxy/run/haproxy-sni.sock weight 1 send-proxy
321324

322325
frontend fe_sni
@@ -433,9 +436,7 @@ frontend fe_sni
433436
##########################################################################
434437
# backend for when sni does not exist, or ssl term needs to happen on the edge
435438
backend be_no_sni
436-
{{- with $value := maxTimeoutFirstMatchedAndClipped .State "haproxy.router.openshift.io/timeout" $timeSpecPattern (env "ROUTER_DEFAULT_SERVER_TIMEOUT") "30s" }}
437-
timeout server {{ $value }}
438-
{{- end }}
439+
timeout server {{ $routerMaxServerTimeout }}
439440
server fe_no_sni unix@/var/lib/haproxy/run/haproxy-no-sni.sock weight 1 send-proxy
440441

441442
frontend fe_no_sni
@@ -595,11 +596,11 @@ backend {{ genBackendNamePrefix $cfg.TLSTermination }}:{{ $cfgIdx }}
595596
{{- end }}
596597
tcp-request content reject if !whitelist
597598
{{- end }}
598-
{{- with $value := clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout") (env "ROUTER_DEFAULT_SERVER_TIMEOUT") "30s") }}
599-
timeout server {{ $value }}
599+
{{- with $value := clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout") $routerDefaultServerTimeout) }}
600+
timeout server {{ $value }}
600601
{{- end }}
601-
{{- with $value := clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout-tunnel") (env "ROUTER_DEFAULT_TUNNEL_TIMEOUT") "1h") }}
602-
timeout tunnel {{ $value }}
602+
{{- with $value := clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout-tunnel") $routerDefaultTunnelTimeout) }}
603+
timeout tunnel {{ $value }}
603604
{{- end }}
604605

605606
{{- if isTrue (index $cfg.Annotations "haproxy.router.openshift.io/rate-limit-connections") }}
@@ -799,11 +800,11 @@ backend {{ genBackendNamePrefix $cfg.TLSTermination }}:{{ $cfgIdx }}
799800
{{- end }}
800801
tcp-request content reject if !whitelist
801802
{{- end }}
802-
{{- with $value := clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout") (env "ROUTER_DEFAULT_SERVER_TIMEOUT") "30s") }}
803-
timeout server {{ $value }}
803+
{{- with $value := clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout") $routerDefaultServerTimeout) }}
804+
timeout server {{ $value }}
804805
{{- end }}
805-
{{- 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") }}
806-
timeout tunnel {{ $value }}
806+
{{- with $value := clipHAProxyTimeoutValue (firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout-tunnel") (index $cfg.Annotations "haproxy.router.openshift.io/timeout") $routerDefaultTunnelTimeout) }}
807+
timeout tunnel {{ $value }}
807808
{{- end }}
808809

809810
{{- if isTrue (index $cfg.Annotations "haproxy.router.openshift.io/rate-limit-connections") }}

‎pkg/router/template/template_helper.go‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -396,8 +396,9 @@ func maxTimeoutFirstMatchedAndClipped(aliases map[ServiceAliasConfigKey]ServiceA
396396
max string
397397
maxDuration time.Duration
398398
)
399+
// find max timeout in route annotations
399400
for _, cfg := range aliases {
400-
timeout := clipHAProxyTimeoutValue(firstMatch(pattern, append([]string{cfg.Annotations[annotation]}, values...)...))
401+
timeout := clipHAProxyTimeoutValue(firstMatch(pattern, cfg.Annotations[annotation]))
401402
if timeout != "" {
402403
// No error handling because clipHAProxyTimeoutValue returns
403404
// a valid timeout or an empty string. The latter is already handled.
@@ -408,6 +409,14 @@ func maxTimeoutFirstMatchedAndClipped(aliases map[ServiceAliasConfigKey]ServiceA
408409
}
409410
}
410411
}
412+
// use values if no max was found in routes
413+
if max == "" {
414+
max = clipHAProxyTimeoutValue(firstMatch(pattern, values...))
415+
}
416+
// use max haproxy timeout if no max was found
417+
if max == "" {
418+
max = templateutil.HaproxyMaxTimeout
419+
}
411420
return max
412421
}
413422

‎pkg/router/template/template_helper_test.go‎

Lines changed: 113 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1068,7 +1068,7 @@ func TestParseIPList(t *testing.T) {
10681068
}
10691069
}
10701070

1071-
func TestMaxTimeoutFirstMatchedAndClipped(t *testing.T) {
1071+
func Test_maxTimeoutFirstMatchedAndClipped(t *testing.T) {
10721072
testCases := []struct {
10731073
name string
10741074
state map[ServiceAliasConfigKey]ServiceAliasConfig
@@ -1115,19 +1115,19 @@ func TestMaxTimeoutFirstMatchedAndClipped(t *testing.T) {
11151115
expected: "30s",
11161116
},
11171117
{
1118-
name: "Maximum default timeout is choosen",
1118+
name: "First default timeout is choosen",
11191119
state: map[ServiceAliasConfigKey]ServiceAliasConfig{
11201120
"test:route1": {},
11211121
"test:route2": {},
11221122
"test:route3": {},
11231123
},
11241124
annotation: "haproxy.router.openshift.io/timeout",
11251125
pattern: `[1-9][0-9]*(us|ms|s|m|h|d)?`,
1126-
values: []string{"40s", "30s"},
1127-
expected: "40s",
1126+
values: []string{"30s", "40s"},
1127+
expected: "30s",
11281128
},
11291129
{
1130-
name: "Route timeout doesn't match",
1130+
name: "One route timeout doesn't match pattern",
11311131
state: map[ServiceAliasConfigKey]ServiceAliasConfig{
11321132
"test:route1": {
11331133
Annotations: map[string]string{
@@ -1145,6 +1145,25 @@ func TestMaxTimeoutFirstMatchedAndClipped(t *testing.T) {
11451145
values: []string{"30s"},
11461146
expected: "35s",
11471147
},
1148+
{
1149+
name: "No route timeout matches pattern",
1150+
state: map[ServiceAliasConfigKey]ServiceAliasConfig{
1151+
"test:route1": {
1152+
Annotations: map[string]string{
1153+
"haproxy.router.openshift.io/timeout": "5minutes",
1154+
},
1155+
},
1156+
"test:route2": {
1157+
Annotations: map[string]string{
1158+
"haproxy.router.openshift.io/timeout": "35seconds",
1159+
},
1160+
},
1161+
},
1162+
annotation: "haproxy.router.openshift.io/timeout",
1163+
pattern: `[1-9][0-9]*(us|ms|s|m|h|d)?`,
1164+
values: []string{"30s"},
1165+
expected: "30s",
1166+
},
11481167
{
11491168
name: "Route timeout clipped",
11501169
state: map[ServiceAliasConfigKey]ServiceAliasConfig{
@@ -1159,6 +1178,33 @@ func TestMaxTimeoutFirstMatchedAndClipped(t *testing.T) {
11591178
values: []string{"30s"},
11601179
expected: "2147483647ms",
11611180
},
1181+
{
1182+
name: "Default timeout overflows but route takes precedence",
1183+
state: map[ServiceAliasConfigKey]ServiceAliasConfig{
1184+
"test:route1": {
1185+
Annotations: map[string]string{
1186+
"haproxy.router.openshift.io/timeout": "30s",
1187+
},
1188+
},
1189+
},
1190+
annotation: "haproxy.router.openshift.io/timeout",
1191+
pattern: `[1-9][0-9]*(us|ms|s|m|h|d)?`,
1192+
values: []string{"999999999s"},
1193+
expected: "30s",
1194+
},
1195+
{
1196+
name: "Empty state",
1197+
annotation: "haproxy.router.openshift.io/timeout",
1198+
pattern: `[1-9][0-9]*(us|ms|s|m|h|d)?`,
1199+
values: []string{"30s"},
1200+
expected: "30s",
1201+
},
1202+
{
1203+
name: "Empty state and values",
1204+
annotation: "haproxy.router.openshift.io/timeout",
1205+
pattern: `[1-9][0-9]*(us|ms|s|m|h|d)?`,
1206+
expected: "2147483647ms",
1207+
},
11621208
}
11631209
for _, tc := range testCases {
11641210
t.Run(tc.name, func(t *testing.T) {
@@ -1169,3 +1215,65 @@ func TestMaxTimeoutFirstMatchedAndClipped(t *testing.T) {
11691215
})
11701216
}
11711217
}
1218+
1219+
func Benchmark_maxTimeoutFirstMatchedAndClipped(b *testing.B) {
1220+
testCases := []struct {
1221+
name string
1222+
state map[ServiceAliasConfigKey]ServiceAliasConfig
1223+
annotation string
1224+
pattern string
1225+
values []string
1226+
}{
1227+
{
1228+
name: "Input1",
1229+
state: map[ServiceAliasConfigKey]ServiceAliasConfig{
1230+
"test:route1": {
1231+
Annotations: map[string]string{
1232+
"haproxy.router.openshift.io/timeout": "5m",
1233+
},
1234+
},
1235+
},
1236+
annotation: "haproxy.router.openshift.io/timeout",
1237+
pattern: `[1-9][0-9]*(us|ms|s|m|h|d)?`,
1238+
values: []string{"30s"},
1239+
},
1240+
{
1241+
name: "Input5",
1242+
state: map[ServiceAliasConfigKey]ServiceAliasConfig{
1243+
"test:route1": {
1244+
Annotations: map[string]string{
1245+
"haproxy.router.openshift.io/timeout": "5m",
1246+
},
1247+
},
1248+
"test:route2": {
1249+
Annotations: map[string]string{
1250+
"haproxy.router.openshift.io/timeout": "35s",
1251+
},
1252+
},
1253+
"test:route3": {
1254+
Annotations: map[string]string{
1255+
"haproxy.router.openshift.io/timeout-tunnel": "35s",
1256+
},
1257+
},
1258+
"test:route4": {
1259+
Annotations: map[string]string{
1260+
"haproxy.router.openshift.io/timeout-tunnel": "10m",
1261+
},
1262+
},
1263+
"test:route5": {},
1264+
},
1265+
annotation: "haproxy.router.openshift.io/timeout",
1266+
pattern: `[1-9][0-9]*(us|ms|s|m|h|d)?`,
1267+
// valid value is the last one to force all the iterations
1268+
values: []string{"", "", "", "", "30s"},
1269+
},
1270+
}
1271+
for _, tc := range testCases {
1272+
b.ResetTimer()
1273+
b.Run(tc.name, func(b *testing.B) {
1274+
for n := 0; n < b.N; n++ {
1275+
maxTimeoutFirstMatchedAndClipped(tc.state, tc.annotation, tc.pattern, tc.values...)
1276+
}
1277+
})
1278+
}
1279+
}

0 commit comments

Comments
 (0)