Skip to content

Commit

Permalink
tmp: review
Browse files Browse the repository at this point in the history 
- 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
  • Loading branch information
@alebedev87
alebedev87 committed Jul 2, 2024
1 parent 5cee412 commit c67ef49
Show file tree
Hide file tree
Showing 4 changed files with 149 additions and 20 deletions.
9 changes: 9 additions & 0 deletions Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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
Expand Down
29 changes: 15 additions & 14 deletions images/router/haproxy/conf/haproxy-config.template
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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") }}
Expand Down Expand Up @@ -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") }}
Expand Down
13 changes: 12 additions & 1 deletion pkg/router/template/template_helper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -391,13 +391,16 @@ func parseIPList(list string) string {
}

// maxTimeoutFirstMatchedAndClipped finds the maximum timeout managed by a given annotation among all the routes, matches it against a given pattern, and clips.
// The goal is to get the maximum timeout among the ones set on the lowest layer backends, rather than the maximum of all values provided to the function.
// For instance, if a route has a timeout annotation set, it will take precedence over the default timeout, even if the default timeout is greater.
func maxTimeoutFirstMatchedAndClipped(aliases map[ServiceAliasConfigKey]ServiceAliasConfig, annotation, pattern string, values ...string) string {
var (
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.
Expand All @@ -408,6 +411,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
}

Expand Down
118 changes: 113 additions & 5 deletions pkg/router/template/template_helper_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1115,19 +1115,19 @@ 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": {},
"test:route3": {},
},
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{
Expand All @@ -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{
Expand All @@ -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) {
Expand All @@ -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...)
}
})
}
}

0 comments on commit c67ef49

Please sign in to comment.