Skip to content
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.

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

feat: implement RequestTimeout in BackendTrafficPolicy #4329

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
5 changes: 5 additions & 0 deletions api/v1alpha1/timeout_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ type HTTPTimeout struct {
//
// +optional
MaxConnectionDuration *gwapiv1.Duration `json:"maxConnectionDuration,omitempty"`

// RequestTimeout is the time until which entire response is received from the upstream.
//
// +optional
RequestTimeout *gwapiv1.Duration `json:"requestTimeout,omitempty" yaml:"requestTimeout,omitempty"`
}

type ClientTimeout struct {
Expand Down
27 changes: 18 additions & 9 deletions internal/gatewayapi/clustersettings.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,11 @@
return ret, nil
}

func buildClusterSettingsTimeout(policy egv1a1.ClusterSettings, traffic *ir.TrafficFeatures) (*ir.Timeout, error) {
func buildClusterSettingsTimeout(policy egv1a1.ClusterSettings, routeTrafficFeatures *ir.TrafficFeatures) (*ir.Timeout, error) {
if policy.Timeout == nil {
if traffic != nil {
if routeTrafficFeatures != nil {
// Don't lose any existing timeout definitions.
return mergeTimeoutSettings(nil, traffic.Timeout), nil
return mergeTimeoutSettings(nil, routeTrafficFeatures.Timeout), nil
}
return nil, nil
}
Expand All @@ -109,6 +109,7 @@
if pto.HTTP != nil {
var cit *metav1.Duration
var mcd *metav1.Duration
var rt *metav1.Duration

if pto.HTTP.ConnectionIdleTimeout != nil {
d, err := time.ParseDuration(string(*pto.HTTP.ConnectionIdleTimeout))
Expand All @@ -128,19 +129,27 @@
}
}

if pto.HTTP.RequestTimeout != nil {
d, err := time.ParseDuration(string(*pto.HTTP.RequestTimeout))
if err != nil {
errs = errors.Join(errs, fmt.Errorf("invalid RequestTimeout value %s", *pto.HTTP.RequestTimeout))

Check warning on line 135 in internal/gatewayapi/clustersettings.go

View check run for this annotation

Codecov / codecov/patch

internal/gatewayapi/clustersettings.go#L135

Added line #L135 was not covered by tests
} else {
rt = ptr.To(metav1.Duration{Duration: d})
}
}

to.HTTP = &ir.HTTPTimeout{
ConnectionIdleTimeout: cit,
MaxConnectionDuration: mcd,
RequestTimeout: rt,
sanposhiho marked this conversation as resolved.
Show resolved Hide resolved
}
}

// http request timeout is translated during the gateway-api route resource translation
// merge route timeout setting with backendtrafficpolicy timeout settings.
// Merging is done after the clustersettings definitions are translated so that
// clustersettings will override previous settings.
if traffic != nil {
to = mergeTimeoutSettings(to, traffic.Timeout)
// The timeout from route's TrafficFeatures takes precedence over the timeout in BTP
if routeTrafficFeatures != nil {
to = mergeTimeoutSettings(routeTrafficFeatures.Timeout, to)
}

return to, errs
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ httpRoutes:
backendRefs:
- name: service-1
port: 8080
timeouts:
request: 1s
backendTrafficPolicies:
- apiVersion: gateway.envoyproxy.io/v1alpha1
kind: BackendTrafficPolicy
Expand All @@ -79,6 +81,7 @@ backendTrafficPolicies:
http:
connectionIdleTimeout: 16s
maxConnectionDuration: 17s
requestTimeout: 18s
- apiVersion: gateway.envoyproxy.io/v1alpha1
kind: BackendTrafficPolicy
metadata:
Expand All @@ -95,3 +98,4 @@ backendTrafficPolicies:
http:
connectionIdleTimeout: 21s
maxConnectionDuration: 22s
requestTimeout: 23s
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
http:
connectionIdleTimeout: 21s
maxConnectionDuration: 22s
requestTimeout: 23s
tcp:
connectTimeout: 20s
status:
Expand Down Expand Up @@ -46,6 +47,7 @@
http:
connectionIdleTimeout: 16s
maxConnectionDuration: 17s
requestTimeout: 18s
tcp:
connectTimeout: 15s
status:
Expand Down Expand Up @@ -195,6 +197,8 @@
- backendRefs:
- name: service-1
port: 8080
timeouts:
request: 1s
matches:
- path:
value: /
Expand Down Expand Up @@ -289,6 +293,7 @@
http:
connectionIdleTimeout: 16s
maxConnectionDuration: 17s
requestTimeout: 18s
tcp:
connectTimeout: 15s
envoy-gateway/gateway-2:
Expand Down Expand Up @@ -336,5 +341,6 @@
http:
connectionIdleTimeout: 21s
maxConnectionDuration: 22s
requestTimeout: 1s # Overwritten by the request timeout in HTTPRoute

Check warning on line 344 in internal/gatewayapi/testdata/backendtrafficpolicy-with-timeout.out.yaml

View workflow job for this annotation

GitHub Actions / lint

344:34 [comments] too few spaces before comment

Check warning on line 344 in internal/gatewayapi/testdata/backendtrafficpolicy-with-timeout.out.yaml

View workflow job for this annotation

GitHub Actions / lint

344:34 [comments] too few spaces before comment
tcp:
connectTimeout: 20s
Loading