diff --git a/api/v1alpha/httpproxy_types.go b/api/v1alpha/httpproxy_types.go index dac3aceb..5e4c45c0 100644 --- a/api/v1alpha/httpproxy_types.go +++ b/api/v1alpha/httpproxy_types.go @@ -266,8 +266,23 @@ type HTTPProxyRule struct { // +kubebuilder:validation:MinItems=0 // +kubebuilder:validation:MaxItems=16 Backends []HTTPProxyRuleBackend `json:"backends,omitempty"` + + // Timeouts bound how long a request matching this rule may take. + // + // The request timeout runs from the request reaching the edge until the + // last byte of the response is sent, so a streaming response or a large + // download must finish within it. It defaults to one hour, and the operator + // caps it at a configured maximum. It cannot be disabled. + // + // See documentation for the `timeouts` field in the `HTTPRouteRule` type at + // https://gateway-api.sigs.k8s.io/reference/spec/#httprouterule + // + // +kubebuilder:default={request: "1h"} + Timeouts *gatewayv1.HTTPRouteTimeouts `json:"timeouts,omitempty"` } +const HTTPProxyDefaultRequestTimeout = "1h" + // +kubebuilder:validation:XValidation:message="endpoint is required unless instance or networkService is set; instance and networkService are mutually exclusive with each other and with endpoint and connector",rule="has(self.instance) ? (!has(self.endpoint) && !has(self.connector) && !has(self.networkService)) : (has(self.networkService) ? (!has(self.endpoint) && !has(self.connector)) : has(self.endpoint))" // +kubebuilder:validation:XValidation:message="backend TLS is not supported for networkService backends",rule="has(self.networkService) ? !has(self.tls) : true" type HTTPProxyRuleBackend struct { diff --git a/api/v1alpha/zz_generated.deepcopy.go b/api/v1alpha/zz_generated.deepcopy.go index 7da540a5..77bca37a 100644 --- a/api/v1alpha/zz_generated.deepcopy.go +++ b/api/v1alpha/zz_generated.deepcopy.go @@ -561,6 +561,11 @@ func (in *HTTPProxyRule) DeepCopyInto(out *HTTPProxyRule) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.Timeouts != nil { + in, out := &in.Timeouts, &out.Timeouts + *out = new(apisv1.HTTPRouteTimeouts) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new HTTPProxyRule. diff --git a/claude-plugin/skills/alb/SKILL.md b/claude-plugin/skills/alb/SKILL.md index 8fc0864c..71d10f85 100644 --- a/claude-plugin/skills/alb/SKILL.md +++ b/claude-plugin/skills/alb/SKILL.md @@ -20,7 +20,7 @@ An Application Load Balancer is a **product built from several objects**, not a - Add routes and the origins behind them, by URL or by network service - Split a route's traffic across several origins by weight, choose the load balancing algorithm, and turn on passive health checks - Configure traffic protection (WAF), request headers and basic authentication -- Raise the 15-second response limit for streams and large downloads +- Set how long a response may run, up to an hour, per route - Read access logs for one load balancer - Preview every change before applying it - Attach advanced Envoy Gateway policies the plugin does not cover @@ -233,43 +233,36 @@ Datum attaches a `BackendTrafficPolicy` of its own to the load balancer's routin A Gateway takes one `BackendTrafficPolicy`. Put rate limiting, circuit breaking and the response limit below in the same policy. -### Responses that run longer than 15 seconds +### Responses that run longer than an hour -A response must finish within about 15 seconds of the request reaching the edge. After that the edge ends it: +Every route rule carries `timeouts.request`: the time from the request reaching the edge to the last byte of the response. It defaults to `1h`, so streams, large downloads and slow APIs run for up to an hour before the edge ends them: - If the origin has not started answering, the client gets a `504`. -- If the origin has started, the response is cut off mid-transfer with whatever status it began with. Large downloads stop partway, and Server-Sent Events, LLM token streams, log tails and long polls close. +- If the origin has started, the response is cut off mid-transfer with whatever status it began with. -WebSockets are not cut. In `alb logs`, a cut response shows a `DURATION` of about 15000 ms. +WebSockets are not cut. In `alb logs`, a cut response shows a `DURATION` equal to the rule's timeout. -`HTTPProxy` has no timeout field. Raise the limit, up to `1h`, with `timeout.http.requestTimeout` on the load balancer's Gateway: +The plugin does not set the timeout. Set it per rule in the `HTTPProxy` manifest, between `1ns` and `1h`. A lower value makes a hung origin fail fast; `0s` is refused, since it would also switch off the idle timeout and leave abandoned streams open: ```yaml -apiVersion: gateway.envoyproxy.io/v1alpha1 -kind: BackendTrafficPolicy +apiVersion: networking.datumapis.com/v1alpha +kind: HTTPProxy metadata: name: my-app namespace: default spec: - targetRefs: - - group: gateway.networking.k8s.io - kind: Gateway - name: my-app - timeout: - http: - requestTimeout: 1h + rules: + - matches: + - path: + type: PathPrefix + value: / + backends: + - endpoint: https://origin.example.com + timeouts: + request: 10m ``` -**Target the Gateway, never the HTTPRoute.** An HTTPRoute takes one `BackendTrafficPolicy`, and Datum attaches its own to the HTTPRoute when the load balancer has a network service origin, an algorithm or health checks. The older of the two wins and the other is rejected as `Conflicted`: - -- If Datum's is older, yours does nothing and responses are still cut at 15 seconds. -- If yours is older, Datum's is rejected and the load balancer silently loses its algorithm and health checks, while still reporting `Programmed`. - -A Gateway-level policy merges with Datum's, so both apply. Editing `timeouts` on the generated HTTPRoute does not work either: Datum rewrites that route from the `HTTPProxy` and the edit is reverted. - -`requestTimeout` covers everything from the request arriving to the last byte sent, so set it longer than the longest stream or download. A value above `1h` is rejected with `duration must be less than or equal to 1h0m0s`. - -To confirm it worked, check that `datumctl describe backendtrafficpolicy my-app` shows `Accepted` and not `Conflicted`, then make a request that runs past 15 seconds and check that it completes. To restore the default, delete the policy, or remove the `timeout` block if the policy carries other settings. +A value above `1h` is rejected with `duration must be less than or equal to 1h0m0s`. A `BackendTrafficPolicy` `requestTimeout` does not change a load balancer's limit any more: the rule's timeout takes precedence. Remove one you attached for that purpose. ### Authentication beyond basic auth @@ -302,7 +295,7 @@ Failures print a named exit code — `exit status 6 # ALB_INVALID`. The names ar - The portal manages one route: the pool of origins on `/`, with weights, the algorithm and health checks. Those are the same fields `datumctl alb` writes, so either can change them. Once a load balancer has a second route, a path match or a per-origin filter, the portal locks its origins, weights, algorithm, health checks, TLS, redirect and Host override and points to `datumctl`; hostnames, protection and auth stay editable there. Tell the user to expect that lock after you add a route - Traffic protection takes paranoia 1 to 4 here; the portal offers only 1 and 2 - Attach a `BackendTrafficPolicy` to the load balancer's Gateway, never its HTTPRoute. A route-level policy collides with the one Datum attaches for health checks and load balancing -- Responses end after about 15 seconds unless a `BackendTrafficPolicy` raises the limit, up to 1 hour +- Responses end after 1 hour by default. Lower it per rule with `timeouts.request` in the manifest; it cannot go above 1 hour or be switched off - Run `datumctl diff -f` before `apply`, and validate with `--dry-run=server` - `delete` has no confirmation prompt — verify the name first. Deleting a load balancer also removes its traffic protection policy and basic auth - Start traffic protection in `mode: Observe` on a live site, watch real traffic, then move to `Enforce`. Raise paranoia one level at a time; above 2 the false-positive rate climbs sharply diff --git a/claude-plugin/skills/httproute/SKILL.md b/claude-plugin/skills/httproute/SKILL.md index 34472b20..35a559e7 100644 --- a/claude-plugin/skills/httproute/SKILL.md +++ b/claude-plugin/skills/httproute/SKILL.md @@ -334,7 +334,7 @@ datumctl apply -f header-injection.yaml --project my-project - Use `sectionName` in `spec.parentRefs[]` to bind a route to a specific listener when a Gateway has both HTTP and HTTPS listeners; binding only to `http` is the standard pattern for redirect routes - `gateway.networking.k8s.io/v1` is stable GA — unlike the `gateway.envoyproxy.io` policy CRDs it is not subject to breaking alpha changes - Policy attachments from the alb skill (`SecurityPolicy`, `BackendTrafficPolicy`, `TrafficProtectionPolicy`) reference `HTTPRoute` resources by name via `targetRefs` — create the HTTPRoute first before attaching policies -- This skill is for HTTPRoutes you write yourself. An Application Load Balancer's HTTPRoute is generated from its `HTTPProxy` and rewritten on every change, so edits to it, `timeouts` included, are reverted. To let a load balancer's responses run longer than 15 seconds, attach a `BackendTrafficPolicy` to its Gateway as the [alb skill](../alb/SKILL.md) describes +- This skill is for HTTPRoutes you write yourself. An Application Load Balancer's HTTPRoute is generated from its `HTTPProxy` and rewritten on every change, so edits to it, `timeouts` included, are reverted. Set a load balancer's response limit with `timeouts.request` on the `HTTPProxy` rule instead, as the [alb skill](../alb/SKILL.md) describes - Run `datumctl diff -f` before `apply` for any changes - `--dry-run=server` validates the manifest against the API before committing - `delete` has no confirmation prompt — always verify the resource name first diff --git a/config/crd/bases/networking.datumapis.com_httpproxies.yaml b/config/crd/bases/networking.datumapis.com_httpproxies.yaml index 0ae6b601..9708a34e 100644 --- a/config/crd/bases/networking.datumapis.com_httpproxies.yaml +++ b/config/crd/bases/networking.datumapis.com_httpproxies.yaml @@ -3458,6 +3458,76 @@ spec: minLength: 1 pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string + timeouts: + default: + request: 1h + description: |- + Timeouts bound how long a request matching this rule may take. + + The request timeout runs from the request reaching the edge until the + last byte of the response is sent, so a streaming response or a large + download must finish within it. It defaults to one hour, and the operator + caps it at a configured maximum. It cannot be disabled. + + See documentation for the `timeouts` field in the `HTTPRouteRule` type at + https://gateway-api.sigs.k8s.io/reference/spec/#httprouterule + properties: + backendRequest: + description: |- + BackendRequest specifies a timeout for an individual request from the gateway + to a backend. This covers the time from when the request first starts being + sent from the gateway to when the full response has been received from the backend. + + Setting a timeout to the zero duration (e.g. "0s") SHOULD disable the timeout + completely. Implementations that cannot completely disable the timeout MUST + instead interpret the zero duration as the longest possible value to which + the timeout can be set. + + An entire client HTTP transaction with a gateway, covered by the Request timeout, + may result in more than one call from the gateway to the destination backend, + for example, if automatic retries are supported. + + The value of BackendRequest must be a Gateway API Duration string as defined by + GEP-2257. When this field is unspecified, its behavior is implementation-specific; + when specified, the value of BackendRequest must be no more than the value of the + Request timeout (since the Request timeout encompasses the BackendRequest timeout). + + Support: Extended + pattern: ^([0-9]{1,5}(h|m|s|ms)){1,4}$ + type: string + request: + description: |- + Request specifies the maximum duration for a gateway to respond to an HTTP request. + If the gateway has not been able to respond before this deadline is met, the gateway + MUST return a timeout error. + + For example, setting the `rules.timeouts.request` field to the value `10s` in an + `HTTPRoute` will cause a timeout if a client request is taking longer than 10 seconds + to complete. + + Setting a timeout to the zero duration (e.g. "0s") SHOULD disable the timeout + completely. Implementations that cannot completely disable the timeout MUST + instead interpret the zero duration as the longest possible value to which + the timeout can be set. + + This timeout is intended to cover as close to the whole request-response transaction + as possible although an implementation MAY choose to start the timeout after the entire + request stream has been received instead of immediately after the transaction is + initiated by the client. + + The value of Request is a Gateway API Duration string as defined by GEP-2257. When this + field is unspecified, request timeout behavior is implementation-specific. + + Support: Extended + pattern: ^([0-9]{1,5}(h|m|s|ms)){1,4}$ + type: string + type: object + x-kubernetes-validations: + - message: backendRequest timeout cannot be longer than request + timeout + rule: '!(has(self.request) && has(self.backendRequest) && + duration(self.request) != duration(''0s'') && duration(self.backendRequest) + > duration(self.request))' type: object x-kubernetes-validations: - message: RequestRedirect filter must not be used together with diff --git a/docs/agent/skills/access-log-triage.md b/docs/agent/skills/access-log-triage.md index ac011886..456abc6d 100644 --- a/docs/agent/skills/access-log-triage.md +++ b/docs/agent/skills/access-log-triage.md @@ -56,11 +56,12 @@ Then widen the window, or send a request yourself and look again. is down, refusing connections, or too slow. - `429`: rate limited. -**Durations near 15 seconds** mean the response limit, not a slow origin. A -response must finish within about 15 seconds of the request reaching the edge. -A request whose origin had not started answering by then comes back `504`. One -whose origin had started keeps its status, `200` included, and is cut off -mid-transfer. So a `200` at about 15000 ms is a large download or a stream that +**Durations at the route's timeout** mean the response limit, not a slow +origin. A response must finish within the rule's `timeouts.request`, one hour +by default, of the request reaching the edge. A request whose origin had not +started answering by then comes back `504`. One whose origin had started keeps +its status, `200` included, and is cut off mid-transfer. So a `200` at about +3600000 ms, or at whatever the rule sets, is a large download or a stream that was cut short, not a success. WebSockets are not cut. The limit can be raised to at most one hour. `HTTPProxy` has no field for it. diff --git a/docs/api/httpproxies.md b/docs/api/httpproxies.md index c765aaad..be6ce4bb 100644 --- a/docs/api/httpproxies.md +++ b/docs/api/httpproxies.md @@ -235,6 +235,24 @@ https://gateway-api.sigs.k8s.io/reference/spec/#httprouterule
if it is set.
false + + timeouts + object + + Timeouts bound how long a request matching this rule may take. + +The request timeout runs from the request reaching the edge until the +last byte of the response is sent, so a streaming response or a large +download must finish within it. It defaults to one hour, and the operator +caps it at a configured maximum. It cannot be disabled. + +See documentation for the `timeouts` field in the `HTTPRouteRule` type at +https://gateway-api.sigs.k8s.io/reference/spec/#httprouterule
+
+ Validations:
  • !(has(self.request) && has(self.backendRequest) && duration(self.request) != duration('0s') && duration(self.backendRequest) > duration(self.request)): backendRequest timeout cannot be longer than request timeout
    • + Default: map[request:1h]
    + + false @@ -4324,6 +4342,87 @@ documentation to determine the supported dialect.
    +### HTTPProxy.spec.rules[index].timeouts +[↩ Parent](#httpproxyspecrulesindex) + + + +Timeouts bound how long a request matching this rule may take. + +The request timeout runs from the request reaching the edge until the +last byte of the response is sent, so a streaming response or a large +download must finish within it. It defaults to one hour, and the operator +caps it at a configured maximum. It cannot be disabled. + +See documentation for the `timeouts` field in the `HTTPRouteRule` type at +https://gateway-api.sigs.k8s.io/reference/spec/#httprouterule + + + + + + + + + + + + + + + + + + + + + +
    NameTypeDescriptionRequired
    backendRequeststring + BackendRequest specifies a timeout for an individual request from the gateway +to a backend. This covers the time from when the request first starts being +sent from the gateway to when the full response has been received from the backend. + +Setting a timeout to the zero duration (e.g. "0s") SHOULD disable the timeout +completely. Implementations that cannot completely disable the timeout MUST +instead interpret the zero duration as the longest possible value to which +the timeout can be set. + +An entire client HTTP transaction with a gateway, covered by the Request timeout, +may result in more than one call from the gateway to the destination backend, +for example, if automatic retries are supported. + +The value of BackendRequest must be a Gateway API Duration string as defined by +GEP-2257. When this field is unspecified, its behavior is implementation-specific; +when specified, the value of BackendRequest must be no more than the value of the +Request timeout (since the Request timeout encompasses the BackendRequest timeout). + +Support: Extended
    +     		false
    requeststring + Request specifies the maximum duration for a gateway to respond to an HTTP request. +If the gateway has not been able to respond before this deadline is met, the gateway +MUST return a timeout error. + +For example, setting the `rules.timeouts.request` field to the value `10s` in an +`HTTPRoute` will cause a timeout if a client request is taking longer than 10 seconds +to complete. + +Setting a timeout to the zero duration (e.g. "0s") SHOULD disable the timeout +completely. Implementations that cannot completely disable the timeout MUST +instead interpret the zero duration as the longest possible value to which +the timeout can be set. + +This timeout is intended to cover as close to the whole request-response transaction +as possible although an implementation MAY choose to start the timeout after the entire +request stream has been received instead of immediately after the transaction is +initiated by the client. + +The value of Request is a Gateway API Duration string as defined by GEP-2257. When this +field is unspecified, request timeout behavior is implementation-specific. + +Support: Extended
    +     		false
    + + ### HTTPProxy.spec.healthCheck [↩ Parent](#httpproxyspec) diff --git a/internal/config/config.go b/internal/config/config.go index c761ef17..c0b947be 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -2145,9 +2145,25 @@ func (c *GatewayConfig) validate() error { } errs = append(errs, c.validateAuthorityLossGracePeriod()) errs = append(errs, c.DownstreamClientTraffic.validate()) + errs = append(errs, c.validateHTTPMaxRequestTimeout()) return errors.Join(errs...) } +func (c *GatewayConfig) validateHTTPMaxRequestTimeout() error { + max := c.ExtensionAPIValidationOptions.BackendTrafficPolicies.ClusterSettings.HTTPMaxRequestTimeout + if max == nil { + return nil + } + floor, err := time.ParseDuration(networkingv1alpha.HTTPProxyDefaultRequestTimeout) + if err != nil { + return err + } + if max.Duration < floor { + return fmt.Errorf("extensionAPIValidationOptions.backendTrafficPolicies.clusterSettings.httpMaxRequestTimeout %s is below the HTTPProxy request timeout default of %s, which every proxy without a timeout of its own carries", max.Duration, networkingv1alpha.HTTPProxyDefaultRequestTimeout) + } + return nil +} + // validateAuthorityLossGracePeriod refuses a negative period, which would turn the hold off without saying so; // zero is the documented way to turn it off. func (c *GatewayConfig) validateAuthorityLossGracePeriod() error { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index e5f1cf51..67ce84e7 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -717,6 +717,31 @@ func TestGatewayConfig_ValidateALPNProtocols(t *testing.T) { // An HTTP/2 window outside Envoy's range is refused at start: the edge would // keep its defaults, and nothing the operator reads would say so. +func TestGatewayConfig_ValidateHTTPMaxRequestTimeout(t *testing.T) { + for _, tt := range []struct { + name string + max *metav1.Duration + wantSub string + }{ + {name: "unset"}, + {name: "at the proxy default", max: &metav1.Duration{Duration: time.Hour}}, + {name: "above the proxy default", max: &metav1.Duration{Duration: 2 * time.Hour}}, + {name: "below the proxy default", max: &metav1.Duration{Duration: 10 * time.Minute}, wantSub: "httpMaxRequestTimeout 10m0s is below"}, + } { + t.Run(tt.name, func(t *testing.T) { + cfg := &NetworkServicesOperator{} + cfg.Gateway.ExtensionAPIValidationOptions.BackendTrafficPolicies.ClusterSettings.HTTPMaxRequestTimeout = tt.max + err := cfg.Validate() + if tt.wantSub == "" && err != nil { + t.Fatalf("expected nil, got %v", err) + } + if tt.wantSub != "" && (err == nil || !strings.Contains(err.Error(), tt.wantSub)) { + t.Fatalf("expected an error containing %q, got %v", tt.wantSub, err) + } + }) + } +} + func TestGatewayConfig_ValidateHTTP2Windows(t *testing.T) { tests := []struct { name, stream, connection, wantSub string diff --git a/internal/controller/httpproxy_controller.go b/internal/controller/httpproxy_controller.go index a3746ab5..dd098720 100644 --- a/internal/controller/httpproxy_controller.go +++ b/internal/controller/httpproxy_controller.go @@ -275,7 +275,7 @@ func (r *HTTPProxyReconciler) Reconcile(ctx context.Context, req mcreconcile.Req return ctrl.Result{}, addErr } - if errs := validation.ValidateHTTPProxy(&httpProxy, validation.HTTPProxyValidationOptions{Hostnames: validation.CustomHostnameOptions(r.Config.Gateway)}); len(errs) > 0 { + if errs := validation.ValidateHTTPProxy(&httpProxy, validation.HTTPProxyOptions(r.Config.Gateway)); len(errs) > 0 { acceptedCondition.Status = metav1.ConditionFalse acceptedCondition.Reason = networkingv1alpha.HTTPProxyReasonInvalid acceptedCondition.Message = fmt.Sprintf("The HTTPProxy is invalid and cannot be programmed: %s", errs.ToAggregate()) @@ -1370,6 +1370,7 @@ func (r *HTTPProxyReconciler) collectDesiredResources( Matches: rule.Matches, Filters: ruleFilters, BackendRefs: backendRefs, + Timeouts: rule.Timeouts, } } diff --git a/internal/controller/httpproxy_controller_test.go b/internal/controller/httpproxy_controller_test.go index 964bd309..7012677e 100644 --- a/internal/controller/httpproxy_controller_test.go +++ b/internal/controller/httpproxy_controller_test.go @@ -92,6 +92,18 @@ func TestHTTPProxyCollectDesiredResources(t *testing.T) { } }, }, + { + name: "rule timeouts reach the route", + httpProxy: newHTTPProxy(func(h *networkingv1alpha.HTTPProxy) { + h.Spec.Rules[0].Timeouts = &gatewayv1.HTTPRouteTimeouts{ + Request: ptr.To(gatewayv1.Duration("30m")), + BackendRequest: ptr.To(gatewayv1.Duration("5m")), + } + }), + assert: func(t *testing.T, httpProxy *networkingv1alpha.HTTPProxy, desiredResources *desiredHTTPProxyResources) { + assert.Equal(t, httpProxy.Spec.Rules[0].Timeouts, desiredResources.httpRoute.Spec.Rules[0].Timeouts) + }, + }, { name: "no URLRewrite filter in rule with hostname in endpoint", httpProxy: newHTTPProxy(func(h *networkingv1alpha.HTTPProxy) { diff --git a/internal/validation/httpproxy_validation.go b/internal/validation/httpproxy_validation.go index 6d29c3fd..db1117fb 100644 --- a/internal/validation/httpproxy_validation.go +++ b/internal/validation/httpproxy_validation.go @@ -14,6 +14,7 @@ import ( gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" + "go.datum.net/network-services-operator/internal/config" gatewayutil "go.datum.net/network-services-operator/internal/util/gateway" ) @@ -26,6 +27,30 @@ const ( // HTTPProxy may declare. type HTTPProxyValidationOptions struct { Hostnames HostnameOptions + + // MaxRequestTimeout caps every rule's request and backendRequest timeouts. + // Zero leaves them uncapped. + MaxRequestTimeout time.Duration +} + +func HTTPProxyOptions(gatewayConfig config.GatewayConfig) HTTPProxyValidationOptions { + opts := HTTPProxyValidationOptions{Hostnames: CustomHostnameOptions(gatewayConfig)} + if max := gatewayConfig.ExtensionAPIValidationOptions.BackendTrafficPolicies.ClusterSettings.HTTPMaxRequestTimeout; max != nil { + opts.MaxRequestTimeout = max.Duration + } + return opts +} + +func DefaultHTTPProxyRuleTimeouts(httpProxy *networkingv1alpha.HTTPProxy) { + for i := range httpProxy.Spec.Rules { + rule := &httpProxy.Spec.Rules[i] + if rule.Timeouts == nil { + rule.Timeouts = &gatewayv1.HTTPRouteTimeouts{} + } + if rule.Timeouts.Request == nil { + rule.Timeouts.Request = ptr.To(gatewayv1.Duration(networkingv1alpha.HTTPProxyDefaultRequestTimeout)) + } + } } func ValidateHTTPProxy(httpProxy *networkingv1alpha.HTTPProxy, opts HTTPProxyValidationOptions) field.ErrorList { @@ -48,7 +73,7 @@ func ValidateHTTPProxy(httpProxy *networkingv1alpha.HTTPProxy, opts HTTPProxyVal allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "name"), httpProxy.Name, msg)) } - allErrs = append(allErrs, validateHTTPProxyRules(httpProxy, field.NewPath("spec", "rules"))...) + allErrs = append(allErrs, validateHTTPProxyRules(httpProxy, field.NewPath("spec", "rules"), opts)...) allErrs = append(allErrs, validateHTTPProxyHealthCheck(httpProxy.Spec.HealthCheck, field.NewPath("spec", "healthCheck"))...) return allErrs @@ -115,22 +140,45 @@ func validateHostHeaderOverride(filters []gatewayv1.HTTPRouteFilter, fldPath *fi return validateProgrammableHostname(override.Value, hostHeaderPath) } -func validateHTTPProxyRules(httpProxy *networkingv1alpha.HTTPProxy, fldPath *field.Path) field.ErrorList { +func validateHTTPProxyRules(httpProxy *networkingv1alpha.HTTPProxy, fldPath *field.Path, opts HTTPProxyValidationOptions) field.ErrorList { allErrs := field.ErrorList{} for i, rule := range httpProxy.Spec.Rules { - allErrs = append(allErrs, validateHTTPProxyRule(rule, fldPath.Index(i))...) + allErrs = append(allErrs, validateHTTPProxyRule(rule, fldPath.Index(i), opts)...) } return allErrs } -func validateHTTPProxyRule(rule networkingv1alpha.HTTPProxyRule, fldPath *field.Path) field.ErrorList { +func validateHTTPProxyRule(rule networkingv1alpha.HTTPProxyRule, fldPath *field.Path, opts HTTPProxyValidationOptions) field.ErrorList { allErrs := field.ErrorList{} allErrs = append(allErrs, validateFilters(rule.Filters, supportedHTTPRouteRuleFilters, fldPath.Child("filters"))...) allErrs = append(allErrs, validateHostHeaderOverride(rule.Filters, fldPath.Child("filters"))...) allErrs = append(allErrs, validateHTTPProxyRuleBackends(rule, fldPath.Child("backends"))...) + allErrs = append(allErrs, validateHTTPProxyRuleTimeouts(rule.Timeouts, fldPath.Child("timeouts"), opts.MaxRequestTimeout)...) + + return allErrs +} + +func validateHTTPProxyRuleTimeouts(timeouts *gatewayv1.HTTPRouteTimeouts, fldPath *field.Path, maxRequestTimeout time.Duration) field.ErrorList { + if timeouts == nil { + return nil + } + + var upperLimit *time.Duration + if maxRequestTimeout > 0 { + upperLimit = &maxRequestTimeout + } + lowerLimit := ptr.To(time.Nanosecond) + + allErrs := field.ErrorList{} + if timeouts.Request != nil { + allErrs = append(allErrs, validateGatewayDuration(fldPath.Child("request"), timeouts.Request, lowerLimit, upperLimit)...) + } + if timeouts.BackendRequest != nil { + allErrs = append(allErrs, validateGatewayDuration(fldPath.Child("backendRequest"), timeouts.BackendRequest, lowerLimit, upperLimit)...) + } return allErrs } diff --git a/internal/validation/httpproxy_validation_test.go b/internal/validation/httpproxy_validation_test.go index ad640abf..d2c2a543 100644 --- a/internal/validation/httpproxy_validation_test.go +++ b/internal/validation/httpproxy_validation_test.go @@ -3,6 +3,7 @@ package validation import ( "strings" "testing" + "time" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" @@ -757,6 +758,108 @@ func TestValidateHTTPProxy(t *testing.T) { } } +func TestValidateHTTPProxyTimeouts(t *testing.T) { + timeoutsPath := rulePath().Child("timeouts") + scenarios := map[string]struct { + timeouts *gatewayv1.HTTPRouteTimeouts + expectedErrors field.ErrorList + }{ + "unset": { + expectedErrors: field.ErrorList{}, + }, + "within the maximum": { + timeouts: &gatewayv1.HTTPRouteTimeouts{ + Request: ptr.To(gatewayv1.Duration("30m")), + BackendRequest: ptr.To(gatewayv1.Duration("5m")), + }, + expectedErrors: field.ErrorList{}, + }, + "at the maximum": { + timeouts: &gatewayv1.HTTPRouteTimeouts{Request: ptr.To(gatewayv1.Duration("1h"))}, + expectedErrors: field.ErrorList{}, + }, + "request above the maximum": { + timeouts: &gatewayv1.HTTPRouteTimeouts{Request: ptr.To(gatewayv1.Duration("2h"))}, + expectedErrors: field.ErrorList{ + field.Invalid(timeoutsPath.Child("request"), "", ""), + }, + }, + "backendRequest above the maximum": { + timeouts: &gatewayv1.HTTPRouteTimeouts{BackendRequest: ptr.To(gatewayv1.Duration("90m"))}, + expectedErrors: field.ErrorList{ + field.Invalid(timeoutsPath.Child("backendRequest"), "", ""), + }, + }, + "zero disables the timeout and is refused": { + timeouts: &gatewayv1.HTTPRouteTimeouts{ + Request: ptr.To(gatewayv1.Duration("0s")), + BackendRequest: ptr.To(gatewayv1.Duration("0s")), + }, + expectedErrors: field.ErrorList{ + field.Invalid(timeoutsPath.Child("request"), "", ""), + field.Invalid(timeoutsPath.Child("backendRequest"), "", ""), + }, + }, + } + + for name, scenario := range scenarios { + t.Run(name, func(t *testing.T) { + proxy := &networkingv1alpha.HTTPProxy{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + Spec: networkingv1alpha.HTTPProxySpec{ + Rules: []networkingv1alpha.HTTPProxyRule{ + { + Backends: []networkingv1alpha.HTTPProxyRuleBackend{{Endpoint: "http://example.com"}}, + Timeouts: scenario.timeouts, + }, + }, + }, + } + errs := ValidateHTTPProxy(proxy, HTTPProxyValidationOptions{MaxRequestTimeout: time.Hour}) + delta := cmp.Diff(scenario.expectedErrors, errs, cmpopts.IgnoreFields(field.Error{}, "BadValue", "Detail")) + if delta != "" { + t.Errorf("expected errors '%v', got '%v', diff: '%v'", scenario.expectedErrors, errs, delta) + } + }) + } +} + +func TestDefaultHTTPProxyRuleTimeouts(t *testing.T) { + scenarios := map[string]struct { + timeouts *gatewayv1.HTTPRouteTimeouts + expected *gatewayv1.HTTPRouteTimeouts + }{ + "unset gets the default": { + expected: &gatewayv1.HTTPRouteTimeouts{Request: ptr.To(gatewayv1.Duration("1h"))}, + }, + "only backendRequest set gets the default request": { + timeouts: &gatewayv1.HTTPRouteTimeouts{BackendRequest: ptr.To(gatewayv1.Duration("5m"))}, + expected: &gatewayv1.HTTPRouteTimeouts{ + Request: ptr.To(gatewayv1.Duration("1h")), + BackendRequest: ptr.To(gatewayv1.Duration("5m")), + }, + }, + "request set is kept": { + timeouts: &gatewayv1.HTTPRouteTimeouts{Request: ptr.To(gatewayv1.Duration("30s"))}, + expected: &gatewayv1.HTTPRouteTimeouts{Request: ptr.To(gatewayv1.Duration("30s"))}, + }, + } + + for name, scenario := range scenarios { + t.Run(name, func(t *testing.T) { + proxy := &networkingv1alpha.HTTPProxy{ + Spec: networkingv1alpha.HTTPProxySpec{ + Rules: []networkingv1alpha.HTTPProxyRule{{Timeouts: scenario.timeouts}}, + }, + } + DefaultHTTPProxyRuleTimeouts(proxy) + if delta := cmp.Diff(scenario.expected, proxy.Spec.Rules[0].Timeouts); delta != "" { + t.Errorf("unexpected timeouts, diff: %s", delta) + } + }) + } +} + func rulePath() *field.Path { return field.NewPath("spec", "rules").Index(0) } diff --git a/internal/webhook/v1alpha/httpproxy_mutating_test.go b/internal/webhook/v1alpha/httpproxy_mutating_test.go index 0a74e3a9..d48a1e9b 100644 --- a/internal/webhook/v1alpha/httpproxy_mutating_test.go +++ b/internal/webhook/v1alpha/httpproxy_mutating_test.go @@ -45,3 +45,27 @@ func TestHTTPProxyCustomDefaulter_Default(t *testing.T) { require.NoError(t, (&HTTPProxyCustomDefaulter{}).Default(context.Background(), proxy)) assert.Equal(t, "Test ALB", proxy.Annotations[display.AnnotationDisplayName]) } + +func TestHTTPProxyCustomDefaulter_DefaultsRuleRequestTimeout(t *testing.T) { + t.Parallel() + + proxy := &networkingv1alpha.HTTPProxy{ + ObjectMeta: metav1.ObjectMeta{Name: "alb", Namespace: "proj"}, + Spec: networkingv1alpha.HTTPProxySpec{ + Rules: []networkingv1alpha.HTTPProxyRule{ + { + Backends: []networkingv1alpha.HTTPProxyRuleBackend{{Endpoint: "https://origin.example.com"}}, + Timeouts: &gatewayv1.HTTPRouteTimeouts{BackendRequest: ptr.To(gatewayv1.Duration("5m"))}, + }, + { + Backends: []networkingv1alpha.HTTPProxyRuleBackend{{Endpoint: "https://origin.example.com"}}, + }, + }, + }, + } + + require.NoError(t, (&HTTPProxyCustomDefaulter{}).Default(context.Background(), proxy)) + assert.Equal(t, gatewayv1.Duration("1h"), ptr.Deref(proxy.Spec.Rules[0].Timeouts.Request, "")) + assert.Equal(t, gatewayv1.Duration("5m"), ptr.Deref(proxy.Spec.Rules[0].Timeouts.BackendRequest, "")) + assert.Equal(t, gatewayv1.Duration("1h"), ptr.Deref(proxy.Spec.Rules[1].Timeouts.Request, "")) +} diff --git a/internal/webhook/v1alpha/httpproxy_webhook.go b/internal/webhook/v1alpha/httpproxy_webhook.go index 13db962c..12b5e5fc 100644 --- a/internal/webhook/v1alpha/httpproxy_webhook.go +++ b/internal/webhook/v1alpha/httpproxy_webhook.go @@ -24,7 +24,7 @@ import ( // SetupHTTPProxyWebhookWithManager registers the webhook for HTTPProxy in the manager. func SetupHTTPProxyWebhookWithManager(mgr mcmanager.Manager, gatewayConfig config.GatewayConfig) error { return ctrl.NewWebhookManagedBy(mgr.GetLocalManager(), &networkingv1alpha.HTTPProxy{}). - WithValidator(&HTTPProxyCustomValidator{mgr: mgr, opts: validation.HTTPProxyValidationOptions{Hostnames: validation.CustomHostnameOptions(gatewayConfig)}}). + WithValidator(&HTTPProxyCustomValidator{mgr: mgr, opts: validation.HTTPProxyOptions(gatewayConfig)}). WithDefaulter(&HTTPProxyCustomDefaulter{}). Complete() } @@ -37,6 +37,7 @@ var _ admission.Defaulter[*networkingv1alpha.HTTPProxy] = &HTTPProxyCustomDefaul func (d *HTTPProxyCustomDefaulter) Default(ctx context.Context, httpProxy *networkingv1alpha.HTTPProxy) error { _ = display.EnsureHTTPProxyAnnotations(httpProxy, oldHTTPProxy(ctx)) + validation.DefaultHTTPProxyRuleTimeouts(httpProxy) return nil } diff --git a/test/e2e/httpproxy-response-timeout/chainsaw-test.yaml b/test/e2e/httpproxy-response-timeout/chainsaw-test.yaml new file mode 100644 index 00000000..a5284bec --- /dev/null +++ b/test/e2e/httpproxy-response-timeout/chainsaw-test.yaml @@ -0,0 +1,347 @@ +# yaml-language-server: $schema=https://raw.githubusercontent.com/kyverno/chainsaw/main/.schemas/json/test-chainsaw-v1alpha1.json +# +# A response through an HTTPProxy may run longer than Envoy's 15-second default +# route timeout (issue #459). +# +# The generated HTTPRoute carried no timeouts, so Envoy applied its own default +# and cut every response still in flight at 15 seconds: large downloads, +# server-sent events, long polls. The rule now carries a request timeout that +# defaults to one hour and that the user can set. +# +# The origin is go-httpbin on the downstream cluster. Its /drip endpoint sends +# one byte at a time over the requested duration, so a 20-second drip through +# the edge is the exact request the default used to cut. +# +# The scenario proves the field is in the path from both sides: a proxy with +# the default completes the 20-second drip, and a proxy whose rule sets a +# 5-second request timeout has the same drip cut. Without the second case a +# timeout that Envoy ignored would pass the first. +apiVersion: chainsaw.kyverno.io/v1alpha1 +kind: Test +metadata: + name: httpproxy-response-timeout +spec: + cluster: nso-standard + timeouts: + cleanup: 120s + steps: + - name: Deploy a slow origin on the downstream cluster + try: + - create: + cluster: nso-infra + resource: + apiVersion: apps/v1 + kind: Deployment + metadata: + name: drip-origin + namespace: default + spec: + replicas: 1 + selector: + matchLabels: + app: drip-origin + template: + metadata: + labels: + app: drip-origin + spec: + containers: + - name: go-httpbin + image: ghcr.io/mccutchen/go-httpbin:2.18.1 + command: ["/bin/go-httpbin"] + args: + - -host + - "0.0.0.0" + - -port + - "8080" + - -max-duration + - 60s + ports: + - containerPort: 8080 + resources: + requests: + cpu: 100m + memory: 128Mi + limits: + cpu: 100m + memory: 128Mi + - create: + cluster: nso-infra + resource: + apiVersion: v1 + kind: Service + metadata: + name: drip-origin + namespace: default + spec: + selector: + app: drip-origin + ports: + - port: 8080 + targetPort: 8080 + - assert: + cluster: nso-infra + timeout: 120s + resource: + apiVersion: apps/v1 + kind: Deployment + metadata: + name: drip-origin + namespace: default + status: + availableReplicas: 1 + + - name: Create a proxy with the default timeout and one with a short timeout + try: + - script: + cluster: nso-standard + skipCommandOutput: true + skipLogOutput: true + content: | + kubectl get ns $NAMESPACE -o json + outputs: + - name: downstreamNamespaceName + value: (join('-', ['ns', json_parse($stdout).metadata.uid])) + - create: + cluster: nso-standard + resource: + apiVersion: networking.datumapis.com/v1alpha + kind: HTTPProxy + metadata: + name: default-timeout + spec: + rules: + - backends: + - endpoint: http://drip-origin.default.svc.cluster.local:8080 + - create: + cluster: nso-standard + resource: + apiVersion: networking.datumapis.com/v1alpha + kind: HTTPProxy + metadata: + name: short-timeout + spec: + rules: + - backends: + - endpoint: http://drip-origin.default.svc.cluster.local:8080 + timeouts: + request: 5s + - create: + cluster: nso-standard + resource: + apiVersion: networking.datumapis.com/v1alpha + kind: HTTPProxy + metadata: + name: partial-timeout + spec: + rules: + - backends: + - endpoint: http://drip-origin.default.svc.cluster.local:8080 + timeouts: + backendRequest: 5m + - assert: + cluster: nso-standard + resource: + apiVersion: networking.datumapis.com/v1alpha + kind: HTTPProxy + metadata: + name: default-timeout + spec: + rules: + - timeouts: + request: 1h + - assert: + cluster: nso-standard + resource: + apiVersion: networking.datumapis.com/v1alpha + kind: HTTPProxy + metadata: + name: partial-timeout + spec: + rules: + - timeouts: + request: 1h + backendRequest: 5m + - assert: + cluster: nso-standard + timeout: 120s + resource: + apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + name: default-timeout + spec: + rules: + - timeouts: + request: 1h + - assert: + cluster: nso-infra + timeout: 180s + resource: + apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + name: default-timeout + namespace: ($downstreamNamespaceName) + spec: + rules: + - timeouts: + request: 1h + - assert: + cluster: nso-infra + timeout: 180s + resource: + apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + name: short-timeout + namespace: ($downstreamNamespaceName) + spec: + rules: + - timeouts: + request: 5s + - assert: + cluster: nso-standard + timeout: 300s + resource: + apiVersion: networking.datumapis.com/v1alpha + kind: HTTPProxy + metadata: + name: default-timeout + status: + (conditions[?type == 'Programmed']): + - status: "True" + - assert: + cluster: nso-standard + timeout: 300s + resource: + apiVersion: networking.datumapis.com/v1alpha + kind: HTTPProxy + metadata: + name: short-timeout + status: + (conditions[?type == 'Programmed']): + - status: "True" + catch: + - script: + cluster: nso-standard + content: | + kubectl -n $NAMESPACE get httpproxy,httproute,gateway -o yaml + kubectl -n network-services-operator-system logs -l app.kubernetes.io/name=network-services-operator --tail=100 + - script: + cluster: nso-infra + env: + - name: DOWNSTREAM_NAMESPACE + value: ($downstreamNamespaceName) + content: | + kubectl -n $DOWNSTREAM_NAMESPACE get httproute,gateway,backendtrafficpolicy -o yaml + + - name: A 20-second response completes with the default and is cut by a 5-second timeout + description: | + Both proxies front the same origin through the shared downstream Envoy + on the kind hostPort. The request is served once a GET returns 200, and + then one 20-second drip is sent through each proxy. The default proxy + delivers every byte; the short-timeout proxy loses the stream before + the last byte, which shows the timeout on the rule is what Envoy + enforces. + try: + - script: + cluster: nso-standard + skipCommandOutput: true + skipLogOutput: true + content: | + kubectl -n $NAMESPACE get httpproxy default-timeout -o jsonpath='{.status.canonicalHostname}' + outputs: + - name: defaultHostname + value: ($stdout) + - script: + cluster: nso-standard + skipCommandOutput: true + skipLogOutput: true + content: | + kubectl -n $NAMESPACE get httpproxy short-timeout -o jsonpath='{.status.canonicalHostname}' + outputs: + - name: shortHostname + value: ($stdout) + - script: + timeout: 300s + env: + - name: DEFAULT_HOSTNAME + value: ($defaultHostname) + - name: SHORT_HOSTNAME + value: ($shortHostname) + content: | + set -u + for host in "$DEFAULT_HOSTNAME" "$SHORT_HOSTNAME"; do + for i in $(seq 1 60); do + code=$(curl -s -o /dev/null -w '%{http_code}' --max-time 5 \ + -H "Host: ${host}" http://localhost:30080/get) || code=000 + echo "${host} attempt ${i}: HTTP ${code}" + [ "${code}" = "200" ] && break + sleep 3 + done + [ "${code}" = "200" ] || { echo "${host} never answered 200"; exit 1; } + done + + drip() { + curl -s -o /dev/null --max-time 40 \ + -w 'http=%{http_code} bytes=%{size_download} seconds=%{time_total}\n' \ + -H "Host: $1" 'http://localhost:30080/drip?duration=20s&numbytes=20&code=200' + } + + echo "default timeout:" + out=$(drip "$DEFAULT_HOSTNAME"); rc=$? + echo " ${out} exit=${rc}" + case "${out}" in + *"http=200 bytes=20 "*) ;; + *) echo "the 20-second drip did not complete through the default proxy"; exit 1 ;; + esac + + echo "5-second timeout:" + out=$(drip "$SHORT_HOSTNAME"); rc=$? + echo " ${out} exit=${rc}" + case "${out}" in + *"bytes=20 "*) echo "the 20-second drip completed despite a 5-second request timeout"; exit 1 ;; + esac + echo "ok" + catch: + - script: + cluster: nso-infra + content: | + kubectl -n datum-downstream-gateway get pods -o wide + kubectl -n datum-downstream-gateway logs -l gateway.envoyproxy.io/owning-gateway-namespace --all-containers --tail=100 || true + + - name: A timeout above the maximum or of zero is refused at admission + try: + - apply: + cluster: nso-standard + expect: + - check: + ($error != null): true + resource: + apiVersion: networking.datumapis.com/v1alpha + kind: HTTPProxy + metadata: + name: timeout-too-long + spec: + rules: + - backends: + - endpoint: http://drip-origin.default.svc.cluster.local:8080 + timeouts: + request: 2h + - apply: + cluster: nso-standard + expect: + - check: + ($error != null): true + resource: + apiVersion: networking.datumapis.com/v1alpha + kind: HTTPProxy + metadata: + name: timeout-disabled + spec: + rules: + - backends: + - endpoint: http://drip-origin.default.svc.cluster.local:8080 + timeouts: + request: 0s