Skip to content

OCPBUGS-61858: Add HTTPKeepAliveTimeout to IngressController API #2547

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

OCPBUGS-61858: Add HTTPKeepAliveTimeout to IngressController API #2547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions openapi/generated_openapi/zz_generated.openapi.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

85 changes: 85 additions & 0 deletions operator/v1/tests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,6 +563,91 @@ tests:
tuningOptions:
connectTimeout: "4 s"
expectedError: "IngressController.operator.openshift.io \"default\" is invalid: spec.tuningOptions.connectTimeout: Invalid value: \"4 s\": spec.tuningOptions.connectTimeout in body should match '^(0|([0-9]+(\\.[0-9]+)?(ns|us|µs|μs|ms|s|m|h))+)$'"
- name: Should be able to create an IngressController with valid nominal httpKeepAlive timeout
initial: |
apiVersion: operator.openshift.io/v1
kind: IngressController
metadata:
name: default
namespace: openshift-ingress-operator
spec:
tuningOptions:
httpKeepAliveTimeout: 10s
expected: |
apiVersion: operator.openshift.io/v1
kind: IngressController
metadata:
name: default
namespace: openshift-ingress-operator
spec:
httpEmptyRequestsPolicy: Respond
idleConnectionTerminationPolicy: Immediate
tuningOptions:
httpKeepAliveTimeout: 10s
- name: Should be able to create an IngressController with valid composite httpKeepAlive timeout
initial: |
apiVersion: operator.openshift.io/v1
kind: IngressController
metadata:
name: default
namespace: openshift-ingress-operator
spec:
tuningOptions:
httpKeepAliveTimeout: 100ms300μs
expected: |
apiVersion: operator.openshift.io/v1
kind: IngressController
metadata:
name: default
namespace: openshift-ingress-operator
spec:
httpEmptyRequestsPolicy: Respond
idleConnectionTerminationPolicy: Immediate
tuningOptions:
httpKeepAliveTimeout: 100ms300μs
- name: Should be able to create an IngressController with valid fraction httpKeepAlive timeout
initial: |
apiVersion: operator.openshift.io/v1
kind: IngressController
metadata:
name: default
namespace: openshift-ingress-operator
spec:
tuningOptions:
httpKeepAliveTimeout: 1.5m
expected: |
apiVersion: operator.openshift.io/v1
kind: IngressController
metadata:
name: default
namespace: openshift-ingress-operator
spec:
httpEmptyRequestsPolicy: Respond
idleConnectionTerminationPolicy: Immediate
tuningOptions:
httpKeepAliveTimeout: 1.5m
- name: Should not be able to create an IngressController with invalid unit httpKeepAlive timeout
initial: |
apiVersion: operator.openshift.io/v1
kind: IngressController
metadata:
name: default
namespace: openshift-ingress-operator
spec:
tuningOptions:
httpKeepAliveTimeout: 3d
expectedError: "IngressController.operator.openshift.io \"default\" is invalid: spec.tuningOptions.httpKeepAliveTimeout: Invalid value: \"3d\": spec.tuningOptions.httpKeepAliveTimeout in body should match '^(0|([0-9]+(\\.[0-9]+)?(ns|us|µs|μs|ms|s|m|h))+)$'"
- name: Should not be able to create an IngressController with invalid space httpKeepAlive timeout
initial: |
apiVersion: operator.openshift.io/v1
kind: IngressController
metadata:
name: default
namespace: openshift-ingress-operator
spec:
tuningOptions:
httpKeepAliveTimeout: "4 s"
expectedError: "IngressController.operator.openshift.io \"default\" is invalid: spec.tuningOptions.httpKeepAliveTimeout: Invalid value: \"4 s\": spec.tuningOptions.httpKeepAliveTimeout in body should match '^(0|([0-9]+(\\.[0-9]+)?(ns|us|µs|μs|ms|s|m|h))+)$'"
- name: Should be able to create an IngressController with valid domain
initial: |
apiVersion: operator.openshift.io/v1
Expand Down
25 changes: 25 additions & 0 deletions operator/v1/types_ingress.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1884,6 +1884,31 @@ type IngressControllerTuningOptions struct {
// +optional
ConnectTimeout *metav1.Duration `json:"connectTimeout,omitempty"`

// httpKeepAliveTimeout defines the maximum allowed time to wait for
// a new HTTP request to appear on a connection from the client to the router.
//
// This field expects an unsigned duration string of decimal numbers, each with optional
// fraction and a unit suffix, e.g. "300ms", "1.5h" or "2h45m".
// Valid time units are "ns", "us" (or "µs" U+00B5 or "μs" U+03BC), "ms", "s", "m", "h".
//
// When omitted, this means the user has no opinion and the platform is left
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding a section starting with "// Setting this field is generally not recommended..." is always helpful. We have it on most of the other tuning options to help people understand the consequence of changing the default value, with explanation for what happens if you set it too high (idle connections remain open longer and use unnecessary resources?), and what happens if you set it too low (idle connection could be closed sooner than wanted and interrupt traffic?).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"// Setting this field is generally not recommended..."

I cannot say that it's "not recommended". It's a prerogative of a customer, that's why we had an RFE fo it. I can elaborate on corner cases though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added 2 "paragraphs" about the potential impact of setting low and high values.

// to choose a reasonable default. This default is subject to change over time.
// The current default is 300s.
//
// Low values (milliseconds or less) can cause clients to close and reopen connections
// for each request, leading to reduced connection sharing.
// For HTTP/2, special care should be taken with low values.
// A few seconds is a reasonable starting point to avoid holding idle connections open
// while still allowing subsequent requests to reuse the connection.
//
// High values (minutes or more) favor connection reuse but may cause idle
// connections to linger longer.
//
// +kubebuilder:validation:Pattern=^(0|([0-9]+(\.[0-9]+)?(ns|us|µs|μs|ms|s|m|h))+)$
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// +kubebuilder:validation:Format=duration

does this works instead? spotted it on other field above

Copy link
Contributor Author

@alebedev87 alebedev87 Oct 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the duration validation is not aligned in the tuning options. I used the explicit regex for a reason that some kubebuilder's duration != golang's duration. We have a bug which showcases this for the client timeout.
However this made me think of the fact that I forgot to add tests for the new field, will do them.

// +kubebuilder:validation:Type:=string
// +optional
HTTPKeepAliveTimeout *metav1.Duration `json:"httpKeepAliveTimeout,omitempty"`
Copy link
Member

@saschagrunert saschagrunert Oct 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

We prefer not to use duration values anymore. Instead, we would create a int32 type, with units in the name. For example, this should be httpKeepAliveTimeoutSeconds.

Referring linter: kubernetes-sigs/kube-api-linter#24

We have a bunch of other *metav1.Duration types as part of this structure and I think we should keep them consistent with the new field.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a bunch of other *metav1.Duration types as part of this structure and I think we should keep them consistent with the new field.

I think this makes sense for new APIs (or new fields of existing APIs). But here I'm thinking of the consistency in the scope of the same API field. All other timeouts we have in IngressController.Spec.TuningOptions are metav1.Duration. Using httpKeepAliveTimeoutSeconds will break the existing pattern and harm the user experience. I acknowledge the new rule but I would like to stay consistent with other timeouts. Unless it's a hard requirement without which we won't get an approval from the API team.


// tlsInspectDelay defines how long the router can hold data to find a
// matching route.
//
Expand Down
23 changes: 23 additions & 0 deletions operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2250,6 +2250,29 @@ spec:
2147483647ms (24.85 days). Both are subject to change over time.
pattern: ^(0|([0-9]+(\.[0-9]+)?(ns|us|µs|μs|ms|s|m|h))+)$
type: string
httpKeepAliveTimeout:
description: |-
httpKeepAliveTimeout defines the maximum allowed time to wait for
a new HTTP request to appear on a connection from the client to the router.

This field expects an unsigned duration string of decimal numbers, each with optional
fraction and a unit suffix, e.g. "300ms", "1.5h" or "2h45m".
Valid time units are "ns", "us" (or "µs" U+00B5 or "μs" U+03BC), "ms", "s", "m", "h".

When omitted, this means the user has no opinion and the platform is left
to choose a reasonable default. This default is subject to change over time.
The current default is 300s.

Low values (milliseconds or less) can cause clients to close and reopen connections
for each request, leading to reduced connection sharing.
For HTTP/2, special care should be taken with low values.
A few seconds is a reasonable starting point to avoid holding idle connections open
while still allowing subsequent requests to reuse the connection.

High values (minutes or more) favor connection reuse but may cause idle
connections to linger longer.
pattern: ^(0|([0-9]+(\.[0-9]+)?(ns|us|µs|μs|ms|s|m|h))+)$
type: string
maxConnections:
description: |-
maxConnections defines the maximum number of simultaneous
Expand Down
5 changes: 5 additions & 0 deletions operator/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 23 additions & 0 deletions ...ated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2233,6 +2233,29 @@ spec:
2147483647ms (24.85 days). Both are subject to change over time.
pattern: ^(0|([0-9]+(\.[0-9]+)?(ns|us|µs|μs|ms|s|m|h))+)$
type: string
httpKeepAliveTimeout:
description: |-
httpKeepAliveTimeout defines the maximum allowed time to wait for
a new HTTP request to appear on a connection from the client to the router.

This field expects an unsigned duration string of decimal numbers, each with optional
fraction and a unit suffix, e.g. "300ms", "1.5h" or "2h45m".
Valid time units are "ns", "us" (or "µs" U+00B5 or "μs" U+03BC), "ms", "s", "m", "h".

When omitted, this means the user has no opinion and the platform is left
to choose a reasonable default. This default is subject to change over time.
The current default is 300s.

Low values (milliseconds or less) can cause clients to close and reopen connections
for each request, leading to reduced connection sharing.
For HTTP/2, special care should be taken with low values.
A few seconds is a reasonable starting point to avoid holding idle connections open
while still allowing subsequent requests to reuse the connection.

High values (minutes or more) favor connection reuse but may cause idle
connections to linger longer.
pattern: ^(0|([0-9]+(\.[0-9]+)?(ns|us|µs|μs|ms|s|m|h))+)$
type: string
maxConnections:
description: |-
maxConnections defines the maximum number of simultaneous
Expand Down
1 change: 1 addition & 0 deletions operator/v1/zz_generated.swagger_doc_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.