Skip to content

Commit 7ec6093

Browse files
Merge pull request #1333 from alebedev87/release-4.18-http-keep-alive
[release-4.18] OCPBUGS-70315: Implement HTTPKeepAliveTimeout tuning option
2 parents 80d4464 + c126df7 commit 7ec6093

16 files changed

Lines changed: 439 additions & 81 deletions

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,6 @@ require (
157157
// github.com/operator-framework/operator-sdk.
158158
replace (
159159
bitbucket.org/ww/goautoneg => github.com/munnerz/goautoneg v0.0.0-20120707110453-a547fc61f48d
160-
github.com/openshift/api => github.com/openshift/api v0.0.0-20251215163528-ca9ed80c43be
160+
github.com/openshift/api => github.com/openshift/api v0.0.0-20260105154426-319dc2e49203
161161
k8s.io/client-go => k8s.io/client-go v0.31.1
162162
)

go.sum

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -993,8 +993,8 @@ github.com/opencontainers/runtime-spec v0.1.2-0.20190507144316-5b71a03e2700/go.m
993993
github.com/opencontainers/runtime-spec v0.1.2-0.20190618234442-a950415649c7/go.mod h1:jwyrGlmzljRJv/Fgzds9SsS/C5hL+LL3ko9hs6T5lQ0=
994994
github.com/opencontainers/runtime-spec v1.0.0/go.mod h1:jwyrGlmzljRJv/Fgzds9SsS/C5hL+LL3ko9hs6T5lQ0=
995995
github.com/opencontainers/runtime-tools v0.0.0-20181011054405-1d69bd0f9c39/go.mod h1:r3f7wjNzSs2extwzU3Y+6pKfobzPh+kKFJ3ofN+3nfs=
996-
github.com/openshift/api v0.0.0-20251215163528-ca9ed80c43be h1:w8xI58kCoxA7BwYlMpjYxOi0tAx3uGO5XYbMmdSo6BA=
997-
github.com/openshift/api v0.0.0-20251215163528-ca9ed80c43be/go.mod h1:Shkl4HanLwDiiBzakv+con/aMGnVE2MAGvoKp5oyYUo=
996+
github.com/openshift/api v0.0.0-20260105154426-319dc2e49203 h1:7hlJcngwAd/hLtU+CkkgWs7liQ7lNjbb+DE1A1W5hOI=
997+
github.com/openshift/api v0.0.0-20260105154426-319dc2e49203/go.mod h1:Shkl4HanLwDiiBzakv+con/aMGnVE2MAGvoKp5oyYUo=
998998
github.com/openshift/build-machinery-go v0.0.0-20200211121458-5e3d6e570160/go.mod h1:1CkcsT3aVebzRBzVTSbiKSkJMsC/CASqxesfqEMfJEc=
999999
github.com/openshift/client-go v0.0.0-20200116152001-92a2713fa240/go.mod h1:4riOwdj99Hd/q+iAcJZfNCsQQQMwURnZV6RL4WHYS5w=
10001000
github.com/openshift/client-go v0.0.0-20240405120947-c67c8325cdd8 h1:HGfbllzRcrJBSiwzNjBCs7sExLUxC5/1evnvlNGB0Cg=

manifests/00-custom-resource-definition.yaml

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1322,9 +1322,9 @@ spec:
13221322
case HAProxy handles it in the old process and closes
13231323
the connection after sending the response.
13241324
1325-
- HAProxy's `timeout http-keep-alive` duration expires
1326-
(300 seconds in OpenShift's configuration, not
1327-
configurable).
1325+
- HAProxy's `timeout http-keep-alive` duration expires.
1326+
By default this is 300 seconds, but it can be changed
1327+
using httpKeepAliveTimeout tuning option.
13281328
13291329
- The client's keep-alive timeout expires, causing the
13301330
client to close the connection.
@@ -2272,6 +2272,44 @@ spec:
22722272
2147483647ms (24.85 days). Both are subject to change over time.
22732273
pattern: ^(0|([0-9]+(\.[0-9]+)?(ns|us|µs|μs|ms|s|m|h))+)$
22742274
type: string
2275+
httpKeepAliveTimeout:
2276+
description: |-
2277+
httpKeepAliveTimeout defines the maximum allowed time to wait for
2278+
a new HTTP request to appear on a connection from the client to the router.
2279+
2280+
This field expects an unsigned duration string of a decimal number, with optional
2281+
fraction and a unit suffix, e.g. "300ms", "1.5s" or "2m45s".
2282+
Valid time units are "ms", "s", "m".
2283+
The allowed range is from 1 millisecond to 15 minutes.
2284+
2285+
When omitted, this means the user has no opinion and the platform is left
2286+
to choose a reasonable default. This default is subject to change over time.
2287+
The current default is 300s.
2288+
2289+
Low values (tens of milliseconds or less) can cause clients to close and reopen connections
2290+
for each request, leading to reduced connection sharing.
2291+
For HTTP/2, special care should be taken with low values.
2292+
A few seconds is a reasonable starting point to avoid holding idle connections open
2293+
while still allowing subsequent requests to reuse the connection.
2294+
2295+
High values (minutes or more) favor connection reuse but may cause idle
2296+
connections to linger longer.
2297+
maxLength: 16
2298+
minLength: 1
2299+
type: string
2300+
x-kubernetes-validations:
2301+
- message: httpKeepAliveTimeout must be a valid duration string
2302+
composed of an unsigned integer value, optionally followed
2303+
by a decimal fraction and a unit suffix (ms, s, m)
2304+
rule: self.matches('^([0-9]+(\\.[0-9]+)?(ms|s|m))+$')
2305+
- message: httpKeepAliveTimeout must be less than or equal to
2306+
15 minutes
2307+
rule: '!self.matches(''^([0-9]+(\\.[0-9]+)?(ms|s|m))+$'') ||
2308+
duration(self) <= duration(''15m'')'
2309+
- message: httpKeepAliveTimeout must be greater than or equal
2310+
to 1 millisecond
2311+
rule: '!self.matches(''^([0-9]+(\\.[0-9]+)?(ms|s|m))+$'') ||
2312+
duration(self) >= duration(''1ms'')'
22752313
maxConnections:
22762314
description: |-
22772315
maxConnections defines the maximum number of simultaneous

pkg/operator/controller/ingress/deployment.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -663,6 +663,9 @@ func desiredRouterDeployment(ci *operatorv1.IngressController, ingressController
663663
if ci.Spec.TuningOptions.ConnectTimeout != nil && ci.Spec.TuningOptions.ConnectTimeout.Duration > 0*time.Second {
664664
env = append(env, corev1.EnvVar{Name: "ROUTER_DEFAULT_CONNECT_TIMEOUT", Value: durationToHAProxyTimespec(ci.Spec.TuningOptions.ConnectTimeout.Duration)})
665665
}
666+
if ci.Spec.TuningOptions.HTTPKeepAliveTimeout != nil && ci.Spec.TuningOptions.HTTPKeepAliveTimeout.Duration > 0*time.Second {
667+
env = append(env, corev1.EnvVar{Name: "ROUTER_SLOWLORIS_HTTP_KEEPALIVE", Value: durationToHAProxyTimespec(ci.Spec.TuningOptions.HTTPKeepAliveTimeout.Duration)})
668+
}
666669
if ci.Spec.TuningOptions.TLSInspectDelay != nil && ci.Spec.TuningOptions.TLSInspectDelay.Duration > 0*time.Second {
667670
env = append(env, corev1.EnvVar{Name: "ROUTER_INSPECT_DELAY", Value: durationToHAProxyTimespec(ci.Spec.TuningOptions.TLSInspectDelay.Duration)})
668671
}

pkg/operator/controller/ingress/deployment_test.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,7 @@ func TestTuningOptions(t *testing.T) {
173173
ic.Spec.TuningOptions.TLSInspectDelay = &metav1.Duration{Duration: 5 * time.Second}
174174
ic.Spec.TuningOptions.HealthCheckInterval = &metav1.Duration{Duration: 15 * time.Second}
175175
ic.Spec.TuningOptions.ReloadInterval = metav1.Duration{Duration: 30 * time.Second}
176+
ic.Spec.TuningOptions.HTTPKeepAliveTimeout = &metav1.Duration{Duration: 30 * time.Second}
176177

177178
deployment, err := desiredRouterDeployment(ic, ingressControllerImage, ingressConfig, infraConfig, apiConfig, networkConfig, false, false, nil, clusterProxyConfig, false, false)
178179
if err != nil {
@@ -187,6 +188,7 @@ func TestTuningOptions(t *testing.T) {
187188
{"ROUTER_DEFAULT_SERVER_FIN_TIMEOUT", true, "4s"},
188189
{"ROUTER_DEFAULT_TUNNEL_TIMEOUT", true, "30m"},
189190
{"ROUTER_DEFAULT_CONNECT_TIMEOUT", true, "30s"},
191+
{"ROUTER_SLOWLORIS_HTTP_KEEPALIVE", true, "30s"},
190192
{"ROUTER_INSPECT_DELAY", true, "5s"},
191193
{RouterBackendCheckInterval, true, "15s"},
192194
{RouterReloadIntervalEnvName, true, "30s"},
@@ -444,6 +446,7 @@ func Test_desiredRouterDeployment(t *testing.T) {
444446
{"ROUTER_DEFAULT_SERVER_TIMEOUT", false, ""},
445447
{"ROUTER_DEFAULT_TUNNEL_TIMEOUT", false, ""},
446448
{"ROUTER_DEFAULT_CONNECT_TIMEOUT", false, ""},
449+
{"ROUTER_SLOWLORIS_HTTP_KEEPALIVE", false, ""},
447450
{"ROUTER_ERRORFILE_503", false, ""},
448451
{"ROUTER_ERRORFILE_404", false, ""},
449452
{"ROUTER_HAPROXY_CONFIG_MANAGER", false, ""},

test/e2e/all_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,7 @@ func TestAll(t *testing.T) {
9191
t.Run("Test_IdleConnectionTerminationPolicyImmediate", Test_IdleConnectionTerminationPolicyImmediate)
9292
t.Run("Test_IdleConnectionTerminationPolicyDeferred", Test_IdleConnectionTerminationPolicyDeferred)
9393
t.Run("TestGatewayAPIUpgradeable", TestGatewayAPIUpgradeable)
94+
t.Run("Test_HTTPKeepAliveTimeout", Test_HTTPKeepAliveTimeout)
9495
})
9596

9697
t.Run("serial", func(t *testing.T) {

test/e2e/http_keep_alive_test.go

Lines changed: 198 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,198 @@
1+
//go:build e2e
2+
// +build e2e
3+
4+
package e2e
5+
6+
import (
7+
"context"
8+
"fmt"
9+
"net/http"
10+
"strings"
11+
"testing"
12+
"time"
13+
14+
corev1 "k8s.io/api/core/v1"
15+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
16+
"k8s.io/apimachinery/pkg/types"
17+
"k8s.io/apiserver/pkg/storage/names"
18+
19+
operatorv1 "github.com/openshift/api/operator/v1"
20+
routev1 "github.com/openshift/api/route/v1"
21+
)
22+
23+
// Test_HTTPKeepAliveTimeout verifies that the HTTPKeepAliveTimeout tuning option correctly closes
24+
// a persistent HTTP connection after the configured duration, forcing the client to establish a new one.
25+
func Test_HTTPKeepAliveTimeout(t *testing.T) {
26+
t.Parallel()
27+
28+
// Use client timeout value lesser than the http keep alive timeout
29+
// to make sure we avoid https://github.com/haproxy/haproxy/issues/2334.
30+
httpKeepAliveTimeoutRunTest(t, 7, 3)
31+
// Use the default client timeout.
32+
httpKeepAliveTimeoutRunTest(t, 7, 0)
33+
}
34+
35+
// httpKeepAliveTimeoutRunTest is a helper which runs a single test workflow.
36+
// The test workflow:
37+
// - Create an ingress controller with a set HTTPKeepAliveTimeout.
38+
// - Send multiple requests to confirm the connection reuse.
39+
// - Sleep for the duration of the timeout.
40+
// - Send a final request and verify a new connection is established.
41+
func httpKeepAliveTimeoutRunTest(t *testing.T, httpKATimeoutSeconds, clientTimeoutSeconds int64) {
42+
t.Helper()
43+
44+
const (
45+
webService = "web-service"
46+
// Number of requests to check the connection is reused.
47+
numConnReuseAttempts = 5
48+
)
49+
50+
createIngressController := func(t *testing.T, testNs *corev1.Namespace) (*operatorv1.IngressController, error) {
51+
t.Helper()
52+
53+
icName := types.NamespacedName{
54+
Namespace: operatorNamespace,
55+
Name: testNs.Name,
56+
}
57+
58+
t.Logf("Creating IngressController %q...", icName)
59+
60+
ic := newLoadBalancerController(icName, icName.Name+"."+dnsConfig.Spec.BaseDomain)
61+
ic.Spec.EndpointPublishingStrategy.LoadBalancer = &operatorv1.LoadBalancerStrategy{
62+
Scope: operatorv1.ExternalLoadBalancer,
63+
DNSManagementPolicy: operatorv1.ManagedLoadBalancerDNS,
64+
}
65+
ic.Spec.NamespaceSelector = &metav1.LabelSelector{
66+
MatchLabels: testNs.Labels,
67+
}
68+
ic.Spec.TuningOptions.HTTPKeepAliveTimeout = &metav1.Duration{Duration: time.Duration(httpKATimeoutSeconds) * time.Second}
69+
if clientTimeoutSeconds != 0 {
70+
ic.Spec.TuningOptions.ClientTimeout = &metav1.Duration{Duration: time.Duration(clientTimeoutSeconds) * time.Second}
71+
}
72+
73+
if err := kclient.Create(context.Background(), ic); err != nil {
74+
return nil, fmt.Errorf("failed to create IngressController: %w", err)
75+
}
76+
t.Cleanup(func() {
77+
t.Logf("Deleting IngressController %q...", icName)
78+
assertIngressControllerDeleted(t, kclient, ic)
79+
})
80+
81+
if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, icName, availableConditionsForIngressControllerWithLoadBalancer...); err != nil {
82+
return nil, fmt.Errorf("failed to observe expected conditions: %w", err)
83+
}
84+
85+
return ic, nil
86+
}
87+
88+
createTestServicesAndTestRoute := func(ns *corev1.Namespace, ic *operatorv1.IngressController) (string, error) {
89+
canaryImage, err := canaryImageReference(t)
90+
if err != nil {
91+
return "", fmt.Errorf("failed to get canary image reference: %w", err)
92+
}
93+
94+
t.Logf("Creating test workload in namespace %q...", ns.Name)
95+
96+
if err := idleConnectionCreateBackendService(context.Background(), t, ns.Name, webService, canaryImage); err != nil {
97+
return "", fmt.Errorf("failed to create service %s: %w", webService, err)
98+
}
99+
100+
routeName := types.NamespacedName{Namespace: ns.Name, Name: webService}
101+
route := buildRoute(routeName.Name, routeName.Namespace, webService)
102+
if err := kclient.Create(context.Background(), route); err != nil {
103+
return "", fmt.Errorf("failed to create route %s: %w", routeName, err)
104+
}
105+
106+
if err := waitForRouteIngressConditions(t, kclient, routeName, ic.Name, routev1.RouteIngressCondition{
107+
Type: routev1.RouteAdmitted,
108+
Status: corev1.ConditionTrue,
109+
}); err != nil {
110+
return "", fmt.Errorf("failed to observe route admitted condition: %w", err)
111+
}
112+
113+
if err := kclient.Get(context.Background(), routeName, route); err != nil {
114+
return "", fmt.Errorf("failed to get route %s: %w", routeName, err)
115+
}
116+
117+
routeHost := getRouteHost(route, ic.Name)
118+
if routeHost == "" {
119+
return "", fmt.Errorf("route %s has no host assigned by IngressController %s", routeName, ic.Name)
120+
}
121+
122+
return routeHost, nil
123+
}
124+
125+
testName := names.SimpleNameGenerator.GenerateName("http-keep-alive-")
126+
testNs := createNamespace(t, testName)
127+
128+
ic, err := createIngressController(t, testNs)
129+
if err != nil {
130+
t.Fatalf("Failed to create IngressController %s: %v", testName, err)
131+
}
132+
133+
routeHost, err := createTestServicesAndTestRoute(testNs, ic)
134+
if err != nil {
135+
t.Fatalf("Test setup failed: %v", err)
136+
}
137+
138+
elbAddress, err := resolveIngressControllerAddress(t, ic)
139+
if err != nil {
140+
t.Fatalf("Test setup failed: %v", err)
141+
}
142+
143+
// Create an HTTP client that reuses connections by default.
144+
httpClient := &http.Client{
145+
Timeout: time.Minute, // full request roundtrip (client-server-client)
146+
Transport: &addressCapturingRoundTripper{
147+
BaseTransport: &http.Transport{
148+
// Set client's idle timeout high to prevent it from closing the connection.
149+
IdleConnTimeout: 600 * time.Second,
150+
// Set connection pool sizes high to ensure the connection reuse.
151+
MaxConnsPerHost: 100, // active + idle per URL host
152+
MaxIdleConns: 100, // all idle
153+
MaxIdleConnsPerHost: 100, // idle per host
154+
},
155+
},
156+
}
157+
t.Logf("Checking connectivity to test route...")
158+
if err := checkRouteConnectivity(t, httpClient, elbAddress, routeHost); err != nil {
159+
t.Fatalf("Failed to check the connectivity to route %q: %v", routeHost, err)
160+
}
161+
162+
t.Logf("Checking connection reuse by sending %d requests...", numConnReuseAttempts)
163+
// Send N requests to make sure the connection keep alive is working.
164+
prevLocalAddr := ""
165+
for i := 1; i < (numConnReuseAttempts + 1); i++ {
166+
response, localAddr, err := idleConnectionFetchResponse(t, httpClient, elbAddress, routeHost)
167+
if err != nil {
168+
t.Errorf("Request %d failed: %v", i, err)
169+
}
170+
if !strings.HasPrefix(response, webService) {
171+
t.Errorf("Request %d failed: wrong response: %s", i, response)
172+
}
173+
if prevLocalAddr == "" {
174+
prevLocalAddr = localAddr
175+
} else if prevLocalAddr != localAddr {
176+
t.Errorf("Request %d failed: unexpected new connection", i)
177+
}
178+
}
179+
180+
// Don't send requests for longer than HTTPKeepAliveTimeout.
181+
sleepDuration := time.Duration(httpKATimeoutSeconds+1) * time.Second
182+
t.Logf("Going to sleep for %v...", sleepDuration)
183+
time.Sleep(sleepDuration)
184+
185+
// Send the last request to see that a new connection was created by the client.
186+
// Old connection is supposed to be reset by the router due to expired http-keep-alive.
187+
t.Logf("Checking connection reset...")
188+
response, localAddr, err := idleConnectionFetchResponse(t, httpClient, elbAddress, routeHost)
189+
if err != nil {
190+
t.Errorf("Last request failed: %v", err)
191+
}
192+
if !strings.HasPrefix(response, webService) {
193+
t.Errorf("Last request failed: wrong response: %s", response)
194+
}
195+
if prevLocalAddr == localAddr {
196+
t.Errorf("Last request failed: unexpected reused connection after keep alive timeout expired")
197+
}
198+
}

test/e2e/idle_connection_test.go

Lines changed: 0 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import (
1919
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2020
"k8s.io/apimachinery/pkg/types"
2121
"k8s.io/apimachinery/pkg/util/intstr"
22-
"k8s.io/apimachinery/pkg/util/wait"
2322
"k8s.io/apiserver/pkg/storage/names"
2423
"k8s.io/client-go/util/retry"
2524
"sigs.k8s.io/controller-runtime/pkg/client"
@@ -306,30 +305,6 @@ func idleConnectionTerminationPolicyRunTest(t *testing.T, policy operatorv1.Ingr
306305
webService2 = "web-service-2"
307306
)
308307

309-
canaryImageReference := func(t *testing.T) (string, error) {
310-
t.Helper()
311-
312-
ingressOperatorName := types.NamespacedName{
313-
Name: "ingress-operator",
314-
Namespace: operatorNamespace,
315-
}
316-
317-
deployment, err := getDeployment(t, kclient, ingressOperatorName, 1*time.Minute)
318-
if err != nil {
319-
return "", fmt.Errorf("failed to get deployment for IngressController %s: %w", ingressOperatorName, err)
320-
}
321-
322-
for _, container := range deployment.Spec.Template.Spec.Containers {
323-
for _, env := range container.Env {
324-
if env.Name == "CANARY_IMAGE" {
325-
return env.Value, nil
326-
}
327-
}
328-
}
329-
330-
return "", fmt.Errorf("CANARY_IMAGE environment variable not found in deployment %s/%s", ingressOperatorName.Namespace, ingressOperatorName.Name)
331-
}
332-
333308
createTestServicesAndTestRoute := func(ns *corev1.Namespace, ic *operatorv1.IngressController) (*idleConnectionTestConfig, error) {
334309
canaryImage, err := canaryImageReference(t)
335310
if err != nil {
@@ -410,27 +385,6 @@ func idleConnectionTerminationPolicyRunTest(t *testing.T, policy operatorv1.Ingr
410385
return ic, nil
411386
}
412387

413-
resolveIngressControllerAddress := func(t *testing.T, ic *operatorv1.IngressController) (string, error) {
414-
elbHostname := getIngressControllerLBAddress(t, ic)
415-
elbAddress := elbHostname
416-
417-
if net.ParseIP(elbHostname) == nil {
418-
if err := wait.PollUntilContextTimeout(context.Background(), 10*time.Second, 5*time.Minute, false, func(ctx context.Context) (bool, error) {
419-
addrs, err := net.LookupHost(elbHostname)
420-
if err != nil {
421-
t.Logf("%v error resolving %s: %v, retrying...", time.Now(), elbHostname, err)
422-
return false, nil
423-
}
424-
elbAddress = addrs[0]
425-
return true, nil
426-
}); err != nil {
427-
return "", fmt.Errorf("failed to resolve %s: %w", elbHostname, err)
428-
}
429-
}
430-
431-
return elbAddress, nil
432-
}
433-
434388
testName := names.SimpleNameGenerator.GenerateName(strings.ToLower(fmt.Sprintf("idle-connection-close-%s-", policy)))
435389
testNs := createNamespace(t, testName)
436390
testNs.Labels = map[string]string{testNs.Name + "-shard": testName}

0 commit comments

Comments
 (0)