diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d9bca4a..7f50e57f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **HTTPRoute `timeouts.backendRequest` and `timeouts.request`.** A rule's + timeout is carried through routing.json to ghost, which bridges it onto the + fetch (`connect_timeout`, `first_byte_timeout`, `between_bytes_timeout`); + exceeding it returns 504 instead of Varnish's 503. `request` is applied as an + alias for `backendRequest` — when both are set the tighter value wins. `0s` + ("disable") falls back to varnishd's global defaults rather than running + unbounded. Both conformance features are now declared. - **`GatewayPort8080` conformance support (#30).** The multi-listener architecture already mapped listener ports straight through to Service and container ports, so the feature is now declared in the conformance suite. @@ -16,6 +23,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`deploy/01-operator.yaml` now sets `GATEWAY_IMAGE`.** The manifest left it + unset, so the operator fell back to its built-in + `ghcr.io/varnish/gateway-chaperone:latest`. On a `:latest` tag the node pulls + from ghcr.io, so `make deploy`, `make kind-deploy` and + `make test-conformance-kind` ran the published data plane instead of a locally + built one, and no ghost or VCL change could be verified against them. + `make deploy VERSION=vX` now pins the operator and the chaperone to the same + version. - **Redirect port now derives from the Gateway listener, not the `Host` header.** A `RequestRedirect` filter with no `scheme` and no `port` was building the `Location` port from the client's `Host` header, so a request diff --git a/conformance/conformance_test.go b/conformance/conformance_test.go index ce2ebe53..0d955592 100644 --- a/conformance/conformance_test.go +++ b/conformance/conformance_test.go @@ -38,6 +38,8 @@ func TestConformance(t *testing.T) { features.SupportHTTPRoute308RedirectStatusCode, features.SupportHTTPRouteHostRewrite, features.SupportHTTPRoutePathRewrite, + features.SupportHTTPRouteBackendTimeout, + features.SupportHTTPRouteRequestTimeout, features.SupportGatewayHTTPListenerIsolation, features.SupportHTTPRouteParentRefPort, // BackendTLSPolicy: Varnish 9.0 does not expose a backend API field for diff --git a/deploy/01-operator.yaml b/deploy/01-operator.yaml index df7b7127..d4ee50da 100644 --- a/deploy/01-operator.yaml +++ b/deploy/01-operator.yaml @@ -143,6 +143,12 @@ spec: image: ghcr.io/varnish/gateway-operator:latest args: - --leader-elect=true + env: + # Without this the operator falls back to its built-in default, which + # is a :latest tag the node pulls from ghcr.io — a locally built and + # kind-loaded data plane is never used. deploy-update rewrites the tag. + - name: GATEWAY_IMAGE + value: ghcr.io/varnish/gateway-chaperone:latest ports: - name: metrics containerPort: 8080 diff --git a/docs/reference/httproute-timeouts.md b/docs/reference/httproute-timeouts.md new file mode 100644 index 00000000..4f0644ae --- /dev/null +++ b/docs/reference/httproute-timeouts.md @@ -0,0 +1,106 @@ +# HTTPRoute Timeouts + +`HTTPRouteRule.timeouts` bounds how long the gateway waits on a backend. + +```yaml +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: api-route +spec: + parentRefs: + - name: my-gateway + rules: + - matches: + - path: + type: PathPrefix + value: /slow-api + backendRefs: + - name: api-service + port: 8080 + timeouts: + backendRequest: 5s +``` + +A route whose backend exceeds the timeout returns **504 Gateway Timeout**. + +## `request` and `backendRequest` + +| Field | Gateway API scope | Varnish behaviour | +|---|---|---| +| `backendRequest` | Gateway sends request headers → complete response received | `connect_timeout` + `first_byte_timeout` + `between_bytes_timeout` | +| `request` | Client request received → response fully sent | Same as `backendRequest` | + +`request` is applied as an alias for `backendRequest`. When both are set the +tighter value wins — normally `backendRequest`, since the spec requires it to be +no larger than `request`. + +## How it maps to Varnish + +Varnish backends are pooled by `address:port`, so they cannot carry per-route +timeouts. Ghost bridges the value on the matched route to the fetch instead: + +``` +routing.json → ghost.json → ghost sets X-Ghost-Timeout on the request + → vcl_backend_fetch sets bereq.connect_timeout + bereq.first_byte_timeout + + bereq.between_bytes_timeout + → vcl_backend_error reports 504 instead of 503 +``` + +All three are set to the same value, so the route's budget bounds establishing the +connection as well as waiting for bytes. Gateway API scopes `backendRequest` to +after request headers are sent, but the bound users actually want is their own +wall-clock wait. A route with a tight timeout to an off-cluster backend +(ExternalName, or one using TLS) must complete the TCP and TLS handshake inside +that budget. + +## Caveats + +**Any fetch failure on a timeout route reports 504.** Varnish exposes no failure +reason in `vcl_backend_error`, so a refused connection on a route with +`backendRequest` set also returns 504 rather than 503. Routes without a timeout +are unaffected and keep 503. + +**There is no total-request cap.** Gateway API scopes `request` to the whole +client request-response cycle and `backendRequest` to the complete response, but +Varnish has neither bound — the clock necessarily starts at the backend fetch, and +time spent reading the client request body or streaming back to a slow client is +not counted. + +**Streaming responses are cut mid-body.** `between_bytes_timeout` bounds every +gap between response body bytes, not the total elapsed time, so a long-lived SSE +or streaming response on a route with a short `backendRequest` is terminated — +and once headers are delivered the 504 flip no longer applies, so the client sees +a truncated 200. Do not set `backendRequest` on streaming routes. + +**`0s` falls back to varnishd defaults.** Gateway API defines `0s` as "disable +the timeout". Varnish always applies its fetch timeouts, so a disabled route +inherits the global varnishd values (`connect_timeout` 3.5s, `first_byte_timeout` +and `between_bytes_timeout` 60s each) rather than running unbounded. Setting only +one of the two fields to `0s` leaves the other in force. + +**No retries.** A timed-out fetch fails immediately; Varnish only retries when +VCL calls `return (retry)`. + +## Interaction with user VCL + +The 504 flip lives in the gateway postamble, which is concatenated *after* user +VCL. A user-supplied `vcl_backend_error` that ends with `return (deliver)` +terminates VCL execution before the postamble runs, and the response keeps +Varnish's 503. Branch on `bereq.http.X-Ghost-Timeout` if you need custom +handling for timed-out routes: + +```vcl +sub vcl_backend_error { + if (bereq.http.X-Ghost-Timeout) { + set beresp.status = 504; + set beresp.http.Content-Type = "application/json"; + set beresp.body = {"{"error": "upstream timeout"}"}; + return (deliver); + } +} +``` + +`X-Ghost-Timeout` is stripped from client requests in `vcl_recv` before routing, +so it cannot be spoofed. Like the cache policy headers, it stays on `bereq` +through the fetch and is therefore visible to the backend. diff --git a/ghost/src/config.rs b/ghost/src/config.rs index a6283ae9..a8098b24 100644 --- a/ghost/src/config.rs +++ b/ghost/src/config.rs @@ -228,6 +228,9 @@ pub struct Route { /// Cache policy from VarnishCachePolicy. None means pass-through (no caching). #[serde(default)] pub cache_policy: Option, + /// Effective route timeout in milliseconds. Absent means varnishd's defaults. + #[serde(default)] + pub backend_timeout_ms: Option, } /// All routing rules for a single hostname (e.g., "api.example.com"). diff --git a/ghost/src/director.rs b/ghost/src/director.rs index 0bffe98d..9e66a09a 100644 --- a/ghost/src/director.rs +++ b/ghost/src/director.rs @@ -202,6 +202,8 @@ pub struct RouteEntry { pub cache_policy: Option, /// Pre-compiled bypass header rules (extracted from cache_policy at config load time). pub bypass_headers: Vec, + /// Effective route timeout in milliseconds, bridged to bereq via X-Ghost-Timeout. + pub backend_timeout_ms: Option, } /// Map of vhost directors for two-tier routing @@ -348,6 +350,7 @@ pub fn build_vhost_directors( rule_index: route.rule_index, cache_policy: route.cache_policy.clone(), bypass_headers, + backend_timeout_ms: route.backend_timeout_ms, }); } @@ -377,6 +380,7 @@ pub fn build_vhost_directors( rule_index: i32::MAX, cache_policy: None, bypass_headers: Vec::new(), + backend_timeout_ms: None, }); } diff --git a/ghost/src/vhost_director.rs b/ghost/src/vhost_director.rs index e488d4e6..b8ec3a75 100644 --- a/ghost/src/vhost_director.rs +++ b/ghost/src/vhost_director.rs @@ -32,6 +32,7 @@ pub struct RouteMatchResult<'a> { pub route_name: Option<&'a str>, pub cache_policy: Option<&'a crate::config::CachePolicy>, pub bypass_headers: &'a [crate::director::BypassHeaderCompiled], + pub backend_timeout_ms: Option, } /// Result returned by route_request to the caller (recv/resolve). @@ -387,6 +388,16 @@ impl VhostDirector { // Determine cache behavior from policy let pass = apply_cache_policy_headers(http, &match_result, &query_string_owned); + // Per-route backend timeout (HTTPRoute timeouts.backendRequest), bridged + // to bereq. vcl_backend_fetch turns this into first_byte_timeout / + // between_bytes_timeout, and the postamble vcl_backend_error uses its + // presence to report 504 rather than Varnish's default 503. + // Must unset first since set_header() appends a header slot. + if let Some(ms) = match_result.backend_timeout_ms { + http.unset_header("X-Ghost-Timeout"); + let _ = http.set_header("X-Ghost-Timeout", &format!("{}ms", ms)); + } + // Select backend using two-level weighted random: // Level 1: pick a group by weight // Level 2: pick a random pod within the selected group @@ -521,6 +532,7 @@ fn match_routes<'a>( route_name: route.route_name.as_deref(), cache_policy: route.cache_policy.as_ref(), bypass_headers: &route.bypass_headers, + backend_timeout_ms: route.backend_timeout_ms, }); } @@ -1155,6 +1167,7 @@ mod tests { rule_index: 0, cache_policy: None, bypass_headers: Vec::new(), + backend_timeout_ms: None, }]; // This test doesn't use HttpHeaders, so we can't fully test it here @@ -1182,6 +1195,7 @@ mod tests { rule_index: 0, cache_policy: None, bypass_headers: Vec::new(), + backend_timeout_ms: None, }]; // Verify route structure @@ -1212,6 +1226,7 @@ mod tests { rule_index: 0, cache_policy: None, bypass_headers: Vec::new(), + backend_timeout_ms: None, }], backend_pool.clone(), None, @@ -1311,6 +1326,7 @@ mod tests { route_name: Some("default/my-route"), cache_policy: None, bypass_headers: &[], + backend_timeout_ms: None, }; assert_eq!(result.backend_groups.len(), 1); diff --git a/ghost/tests/test_backend_timeout.vtc b/ghost/tests/test_backend_timeout.vtc new file mode 100644 index 00000000..bb4624f5 --- /dev/null +++ b/ghost/tests/test_backend_timeout.vtc @@ -0,0 +1,140 @@ +varnishtest "per-route backendRequest timeout maps to 504" + +# Slow backend: holds the connection open well past the route's 500ms timeout. +# Deliberately never sends a response — writing one after Varnish has already +# given up and closed the backend connection races with the test harness. +server s_slow { + rxreq + delay 2 +} -start + +# Fast backend on a route that also carries a timeout: must not be affected. +server s_fast { + rxreq + txresp -body "on-time" +} -start + +# Slow backend on a route with NO timeout configured: proves the 504 flip is +# scoped to timeout routes and does not turn every fetch failure into a 504. +server s_untimed { + rxreq + delay 1 + txresp -body "untimed" +} -start + +shell { + cat > ${tmpdir}/ghost.json < 504 flip in vcl_backend_error. +# Global first_byte_timeout is raised so the /untimed route is governed by the +# route config (i.e. no timeout) rather than by a short varnishd default. +varnish v1 -arg "-p thread_pool_stack=160k" -arg "-p first_byte_timeout=10" -vcl { + import ghost from "${vmod}"; + import std; + + backend dummy none; + + sub vcl_init { + ghost.init("${tmpdir}/ghost.json"); + new router = ghost.ghost_backend(); + } + + sub vcl_recv { + unset req.http.X-Ghost-Timeout; + if (req.url == "/.varnish-ghost/reload") { + if (router.reload()) { + return (synth(200, "OK")); + } else { + return (synth(500, "Reload failed")); + } + } + set req.backend_hint = router.recv(); + return (pass); + } + + sub vcl_backend_fetch { + if (bereq.http.X-Ghost-Timeout) { + set bereq.first_byte_timeout = std.duration(bereq.http.X-Ghost-Timeout, 60s); + set bereq.between_bytes_timeout = std.duration(bereq.http.X-Ghost-Timeout, 60s); + } + } + + sub vcl_backend_error { + if (bereq.http.X-Ghost-Timeout) { + set beresp.status = 504; + set beresp.reason = "Gateway Timeout"; + set beresp.ttl = 0s; + } + } +} -start + +client c_reload { + txreq -url "/.varnish-ghost/reload" + rxresp + expect resp.status == 200 +} -run + +# Backend exceeds the route timeout -> 504, not 503. +client c_slow { + txreq -url "/slow" -hdr "Host: api.example.com" + rxresp + expect resp.status == 504 + expect resp.reason == "Gateway Timeout" +} -run + +# Same timeout, backend responds in time -> untouched. +client c_fast { + txreq -url "/fast" -hdr "Host: api.example.com" + rxresp + expect resp.status == 200 + expect resp.body == "on-time" +} -run + +# No timeout on the route -> slow backend still succeeds. +client c_untimed { + txreq -url "/untimed" -hdr "Host: api.example.com" + rxresp + expect resp.status == 200 + expect resp.body == "untimed" +} -run diff --git a/internal/ghost/config.go b/internal/ghost/config.go index 1220ba98..cd51526b 100644 --- a/internal/ghost/config.go +++ b/internal/ghost/config.go @@ -198,6 +198,8 @@ type Route struct { RuleIndex int `json:"rule_index"` // Original rule ordering for tiebreaking CachePolicy *CachePolicy `json:"cache_policy,omitempty"` // Caching behavior from VarnishCachePolicy BackendTLS *BackendTLS `json:"backend_tls,omitempty"` // TLS config from BackendTLSPolicy + // BackendTimeoutMs is the effective rule timeout in ms; 0 means unset. + BackendTimeoutMs int `json:"backend_timeout_ms,omitempty"` // ExternalProxy is set when the route's backendRef points to a Service of // type ExternalName. The chaperone passes this through to ghost.json // without performing EndpointSlice lookup. @@ -230,6 +232,8 @@ type RouteBackends struct { Priority int `json:"priority"` RuleIndex int `json:"rule_index"` CachePolicy *CachePolicy `json:"cache_policy,omitempty"` // Caching behavior from VarnishCachePolicy + // BackendTimeoutMs mirrors Route.BackendTimeoutMs. + BackendTimeoutMs int `json:"backend_timeout_ms,omitempty"` } // VHostConfig represents a virtual host with path-based routing in ghost.json. diff --git a/internal/ghost/generator.go b/internal/ghost/generator.go index bf8f4cab..947560b2 100644 --- a/internal/ghost/generator.go +++ b/internal/ghost/generator.go @@ -116,6 +116,9 @@ func mergeRoutesByMatchCriteria(routes []Route, endpoints ServiceEndpoints) []Ro cachePolicy string // JSON serialization of CachePolicy priority int ruleIndex int + // In the key because RouteBackends takes it from the first route in the + // group — merging routes with different timeouts would silently drop one. + backendTimeoutMs int } // NOTE: BackendTLS is intentionally NOT part of the merge key. Each BackendGroup @@ -127,15 +130,16 @@ func mergeRoutesByMatchCriteria(routes []Route, endpoints ServiceEndpoints) []Ro grouped := make(map[routeKey][]Route) for _, route := range routes { key := routeKey{ - pathMatch: serializePathMatch(route.PathMatch), - method: serializeMethod(route.Method), - headers: serializeHeaders(route.Headers), - queryParams: serializeQueryParams(route.QueryParams), - filters: serializeFilters(route.Filters), - listeners: serializeListeners(route.Listeners), - cachePolicy: serializeCachePolicy(route.CachePolicy), - priority: route.Priority, - ruleIndex: route.RuleIndex, + pathMatch: serializePathMatch(route.PathMatch), + method: serializeMethod(route.Method), + headers: serializeHeaders(route.Headers), + queryParams: serializeQueryParams(route.QueryParams), + filters: serializeFilters(route.Filters), + listeners: serializeListeners(route.Listeners), + cachePolicy: serializeCachePolicy(route.CachePolicy), + priority: route.Priority, + ruleIndex: route.RuleIndex, + backendTimeoutMs: route.BackendTimeoutMs, } grouped[key] = append(grouped[key], route) } @@ -155,17 +159,18 @@ func mergeRoutesByMatchCriteria(routes []Route, endpoints ServiceEndpoints) []Ro // Use the first route's match criteria (all routes in group have identical criteria) firstRoute := routeGroup[0] result = append(result, RouteBackends{ - PathMatch: firstRoute.PathMatch, - Method: firstRoute.Method, - Headers: firstRoute.Headers, - QueryParams: firstRoute.QueryParams, - Filters: firstRoute.Filters, - BackendGroups: groups, - Listeners: firstRoute.Listeners, - RouteName: firstRoute.RouteName, - Priority: key.priority, - RuleIndex: key.ruleIndex, - CachePolicy: firstRoute.CachePolicy, + PathMatch: firstRoute.PathMatch, + Method: firstRoute.Method, + Headers: firstRoute.Headers, + QueryParams: firstRoute.QueryParams, + Filters: firstRoute.Filters, + BackendGroups: groups, + Listeners: firstRoute.Listeners, + RouteName: firstRoute.RouteName, + Priority: key.priority, + RuleIndex: key.ruleIndex, + CachePolicy: firstRoute.CachePolicy, + BackendTimeoutMs: key.backendTimeoutMs, }) } diff --git a/internal/ghost/generator_test.go b/internal/ghost/generator_test.go index 85752913..914cc215 100644 --- a/internal/ghost/generator_test.go +++ b/internal/ghost/generator_test.go @@ -774,3 +774,69 @@ func TestBackendTLSMergesIntoWeightedGroups(t *testing.T) { t.Errorf("expected weights {90,10}, got %v", weights) } } + +func TestGenerateBackendTimeoutPassthrough(t *testing.T) { + // Two backendRefs in the same rule (identical match criteria and rule index) + // must merge into ONE route with two weighted groups, carrying the timeout. + routingConfig := &RoutingConfig{ + Version: 2, + VHosts: map[string]VHostRouting{ + "api.example.com": { + Routes: []Route{ + { + PathMatch: &PathMatch{Type: PathMatchPathPrefix, Value: "/timed"}, + Service: "api-v1", + Namespace: "default", + Port: 8080, + Weight: 50, + Priority: 10300, + BackendTimeoutMs: 500, + }, + { + PathMatch: &PathMatch{Type: PathMatchPathPrefix, Value: "/timed"}, + Service: "api-v2", + Namespace: "default", + Port: 8080, + Weight: 50, + Priority: 10300, + BackendTimeoutMs: 500, + }, + { + PathMatch: &PathMatch{Type: PathMatchPathPrefix, Value: "/untimed"}, + Service: "api-v1", + Namespace: "default", + Port: 8080, + Weight: 100, + Priority: 10300, + }, + }, + }, + }, + } + + endpoints := ServiceEndpoints{ + "default/api-v1": {{IP: "10.0.0.1", Port: 8080}}, + "default/api-v2": {{IP: "10.0.0.2", Port: 8080}}, + } + + config := Generate(routingConfig, endpoints) + + byPath := make(map[string]RouteBackends) + for _, r := range config.VHosts["api.example.com"].Routes { + byPath[r.PathMatch.Value] = r + } + if len(byPath) != 2 { + t.Fatalf("expected 2 merged routes, got %d", len(byPath)) + } + + timed := byPath["/timed"] + if timed.BackendTimeoutMs != 500 { + t.Errorf("/timed: BackendTimeoutMs = %d, want 500", timed.BackendTimeoutMs) + } + if len(timed.BackendGroups) != 2 { + t.Errorf("/timed: expected 2 weighted groups, got %d", len(timed.BackendGroups)) + } + if got := byPath["/untimed"].BackendTimeoutMs; got != 0 { + t.Errorf("/untimed: BackendTimeoutMs = %d, want 0", got) + } +} diff --git a/internal/vcl/generator.go b/internal/vcl/generator.go index 0ff6bb9d..a03e1d93 100644 --- a/internal/vcl/generator.go +++ b/internal/vcl/generator.go @@ -3,8 +3,10 @@ package vcl import ( _ "embed" "fmt" + "math" "slices" "strings" + "time" "github.com/varnish/gateway/internal/ghost" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" @@ -154,6 +156,11 @@ func CollectHTTPRouteBackends(routes []gatewayv1.HTTPRoute, gateway *gatewayv1.G ruleName = string(*rule.Name) } + // Timeouts are per-rule, so every route this rule emits carries the + // same value. Stamped once at the bottom of the loop rather than in + // each of the five struct literals below. + timeoutStart := len(collectedRoutes) + // Process each match in the rule if len(rule.Matches) == 0 { // No matches specified - create default route with PathPrefix "/" @@ -411,6 +418,12 @@ func CollectHTTPRouteBackends(routes []gatewayv1.HTTPRoute, gateway *gatewayv1.G } } } + if ms := routeBackendTimeoutMs(rule.Timeouts); ms != 0 { + for i := timeoutStart; i < len(collectedRoutes); i++ { + collectedRoutes[i].BackendTimeoutMs = ms + } + } + ruleIndex++ } } @@ -439,6 +452,56 @@ func CollectHTTPRouteBackends(routes []gatewayv1.HTTPRoute, gateway *gatewayv1.G return collectedRoutes } +// routeBackendTimeoutMs converts an HTTPRoute rule's timeouts into milliseconds for +// routing.json. Returns 0 when no timeout applies, and 0 is serialized as absent. +// +// request and backendRequest map onto the same Varnish fetch timeouts, so the +// tighter of the two wins — the spec requires backendRequest <= request, but a +// route that violates that must not end up with the looser bound. See +// docs/reference/httproute-timeouts.md. +func routeBackendTimeoutMs(t *gatewayv1.HTTPRouteTimeouts) int { + if t == nil { + return 0 + } + ms := durationMs(t.Request) + if be := durationMs(t.BackendRequest); be != 0 && (ms == 0 || be < ms) { + ms = be + } + return ms +} + +// maxTimeoutMs is the largest value ghost can represent: backend_timeout_ms +// deserializes into a u32, so anything past this fails to parse. Roughly 49.7 days. +const maxTimeoutMs = math.MaxUint32 + +// durationMs parses a GEP-2257 duration into milliseconds. Returns 0 when unset, +// unparseable, or "0s", and saturates at maxTimeoutMs. +// +// Gateway API defines "0s" as "disable the timeout". Varnish has no way to uncap +// a fetch — first_byte_timeout/between_bytes_timeout always apply — so a disabled +// route is emitted as absent and inherits varnishd's global defaults rather than +// running unbounded. +// +// The GEP-2257 pattern allows up to four 5-digit components, so a CRD-valid +// duration ("2000h") can exceed ghost's u32. Saturating keeps that route absurd +// but harmless; letting it through would fail the whole ghost.json parse and +// freeze routing updates for every route on the gateway, not just this one. +func durationMs(d *gatewayv1.Duration) int { + if d == nil { + return 0 + } + // GEP-2257 durations are a subset of time.ParseDuration's grammar. + parsed, err := time.ParseDuration(string(*d)) + if err != nil || parsed <= 0 { + return 0 + } + ms := parsed.Milliseconds() + if ms > maxTimeoutMs { + return maxTimeoutMs + } + return int(ms) +} + // filterValidBackends returns backend refs that have a valid Kind/Group and are not blocked. func filterValidBackends(backendRefs []gatewayv1.HTTPBackendRef, routeNS string, blockedRefs map[string]bool) []gatewayv1.HTTPBackendRef { var valid []gatewayv1.HTTPBackendRef diff --git a/internal/vcl/generator_test.go b/internal/vcl/generator_test.go index 33d2e44f..c89eb250 100644 --- a/internal/vcl/generator_test.go +++ b/internal/vcl/generator_test.go @@ -101,9 +101,33 @@ func TestGenerate_GhostReloadHandler(t *testing.T) { t.Error("expected vcl_recv to return synth(500) on failed reload") } - // Should NOT have vcl_backend_error (reload handled in vcl_recv) + // The preamble runs BEFORE user VCL, so a return here would stop user VCL from + // ever running. The 504 flip lives in the postamble instead. if strings.Contains(result, "sub vcl_backend_error {") { - t.Error("should not have vcl_backend_error (reload handled in vcl_recv)") + t.Error("should not have vcl_backend_error in the preamble (see postamble.vcl)") + } +} + +func TestMergePostambleBackendError(t *testing.T) { + // The 504 flip must land after user VCL so a user-defined vcl_backend_error + // runs first and can take over with its own return. + merged := Merge(Generate(), "sub vcl_backend_error { set beresp.http.X-User = \"1\"; }") + + userIdx := strings.Index(merged, `set beresp.http.X-User = "1";`) + flipIdx := strings.Index(merged, "set beresp.status = 504;") + if userIdx == -1 { + t.Fatal("expected user vcl_backend_error in merged VCL") + } + if flipIdx == -1 { + t.Fatal("expected postamble 504 flip in merged VCL") + } + if flipIdx < userIdx { + t.Error("postamble vcl_backend_error must be concatenated after user VCL") + } + + // The flip is scoped to routes that actually carry a timeout. + if !strings.Contains(merged, "if (bereq.http.X-Ghost-Timeout) {") { + t.Error("expected the 504 flip to be guarded on X-Ghost-Timeout") } } @@ -139,6 +163,24 @@ func TestGenerate_DefaultGhostConfigPath(t *testing.T) { } } +func TestGenerate_RouteTimeoutSetsAllFetchTimeouts(t *testing.T) { + result := Generate() + + // All three must move together: leaving connect_timeout at varnishd's 3.5s + // global lets an unreachable pod outlive a shorter route timeout. + for _, want := range []string{ + "set bereq.connect_timeout = std.duration(bereq.http.X-Ghost-Timeout, 3.5s);", + "set bereq.first_byte_timeout = std.duration(bereq.http.X-Ghost-Timeout, 60s);", + "set bereq.between_bytes_timeout = std.duration(bereq.http.X-Ghost-Timeout, 60s);", + // A client-supplied X-Ghost-Timeout must never reach the fetch. + "unset req.http.X-Ghost-Timeout;", + } { + if !strings.Contains(result, want) { + t.Errorf("expected %q in generated VCL", want) + } + } +} + func TestGenerate_DeterministicOutput(t *testing.T) { first := Generate() @@ -1249,3 +1291,125 @@ func TestNoMatchRuleMatchesExplicitSlashPriority(t *testing.T) { t.Errorf("no-match (%d) and explicit-/ (%d) priorities must be equal", noMatchPrio, explicitPrio) } } + +func TestRouteBackendTimeoutMs(t *testing.T) { + tests := []struct { + name string + timeouts *gatewayv1.HTTPRouteTimeouts + want int + }{ + {"nil timeouts", nil, 0}, + {"request only", &gatewayv1.HTTPRouteTimeouts{Request: ptr(gatewayv1.Duration("5s"))}, 5000}, + {"request disabled", &gatewayv1.HTTPRouteTimeouts{Request: ptr(gatewayv1.Duration("0s"))}, 0}, + {"both set, backendRequest tighter", &gatewayv1.HTTPRouteTimeouts{ + Request: ptr(gatewayv1.Duration("5s")), + BackendRequest: ptr(gatewayv1.Duration("500ms")), + }, 500}, + {"both set, request tighter", &gatewayv1.HTTPRouteTimeouts{ + Request: ptr(gatewayv1.Duration("1s")), + BackendRequest: ptr(gatewayv1.Duration("30s")), + }, 1000}, + {"request set, backendRequest disabled", &gatewayv1.HTTPRouteTimeouts{ + Request: ptr(gatewayv1.Duration("2s")), + BackendRequest: ptr(gatewayv1.Duration("0s")), + }, 2000}, + {"sub-second", &gatewayv1.HTTPRouteTimeouts{BackendRequest: ptr(gatewayv1.Duration("500ms"))}, 500}, + {"whole seconds", &gatewayv1.HTTPRouteTimeouts{BackendRequest: ptr(gatewayv1.Duration("3s"))}, 3000}, + {"compound", &gatewayv1.HTTPRouteTimeouts{BackendRequest: ptr(gatewayv1.Duration("1m30s"))}, 90000}, + {"zero disables", &gatewayv1.HTTPRouteTimeouts{BackendRequest: ptr(gatewayv1.Duration("0s"))}, 0}, + {"unparseable", &gatewayv1.HTTPRouteTimeouts{BackendRequest: ptr(gatewayv1.Duration("banana"))}, 0}, + {"negative", &gatewayv1.HTTPRouteTimeouts{BackendRequest: ptr(gatewayv1.Duration("-5s"))}, 0}, + // The GEP-2257 pattern permits durations past ghost's u32 backend_timeout_ms. + // 1193h is the last hour that still fits; 1194h saturates instead of + // overflowing, which would fail the whole ghost.json parse. + {"just under the u32 ceiling", &gatewayv1.HTTPRouteTimeouts{ + BackendRequest: ptr(gatewayv1.Duration("1193h")), + }, 4294800000}, + {"saturates past the u32 ceiling", &gatewayv1.HTTPRouteTimeouts{ + BackendRequest: ptr(gatewayv1.Duration("1194h")), + }, maxTimeoutMs}, + {"saturated request still loses to a tighter backendRequest", &gatewayv1.HTTPRouteTimeouts{ + Request: ptr(gatewayv1.Duration("99999h")), + BackendRequest: ptr(gatewayv1.Duration("500ms")), + }, 500}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := routeBackendTimeoutMs(tt.timeouts); got != tt.want { + t.Errorf("routeBackendTimeoutMs() = %d, want %d", got, tt.want) + } + }) + } +} + +func TestCollectHTTPRouteBackends_BackendTimeout(t *testing.T) { + prefixType := gatewayv1.PathMatchPathPrefix + backendRef := []gatewayv1.HTTPBackendRef{ + {BackendRef: gatewayv1.BackendRef{BackendObjectReference: gatewayv1.BackendObjectReference{ + Name: "api-service", Port: ptr(gatewayv1.PortNumber(8080)), + }}}, + } + + routes := []gatewayv1.HTTPRoute{ + { + ObjectMeta: metav1.ObjectMeta{Name: "route-1", Namespace: "default"}, + Spec: gatewayv1.HTTPRouteSpec{ + Hostnames: []gatewayv1.Hostname{"api.example.com"}, + Rules: []gatewayv1.HTTPRouteRule{ + { + Matches: []gatewayv1.HTTPRouteMatch{ + {Path: &gatewayv1.HTTPPathMatch{Type: &prefixType, Value: ptr("/timed")}}, + }, + BackendRefs: backendRef, + Timeouts: &gatewayv1.HTTPRouteTimeouts{BackendRequest: ptr(gatewayv1.Duration("500ms"))}, + }, + { + Matches: []gatewayv1.HTTPRouteMatch{ + {Path: &gatewayv1.HTTPPathMatch{Type: &prefixType, Value: ptr("/disabled")}}, + }, + BackendRefs: backendRef, + Timeouts: &gatewayv1.HTTPRouteTimeouts{BackendRequest: ptr(gatewayv1.Duration("0s"))}, + }, + { + Matches: []gatewayv1.HTTPRouteMatch{ + {Path: &gatewayv1.HTTPPathMatch{Type: &prefixType, Value: ptr("/untimed")}}, + }, + BackendRefs: backendRef, + }, + }, + }, + }, + } + + collectedRoutes := CollectHTTPRouteBackends(routes, nil, "default", nil, nil, nil) + + got := make(map[string]int) + for _, r := range collectedRoutes { + if r.PathMatch == nil { + t.Fatalf("expected a path match on every route, got %+v", r) + } + got[r.PathMatch.Value] = r.BackendTimeoutMs + } + + want := map[string]int{"/timed": 500, "/disabled": 0, "/untimed": 0} + for path, wantMs := range want { + if got[path] != wantMs { + t.Errorf("route %s: BackendTimeoutMs = %d, want %d", path, got[path], wantMs) + } + } + + // 0 must serialize as absent, not as a literal 0 ghost would bridge into bereq. + for _, r := range collectedRoutes { + if r.PathMatch.Value != "/disabled" { + continue + } + data, err := json.Marshal(r) + if err != nil { + t.Fatalf("json.Marshal(route): %v", err) + } + if strings.Contains(string(data), "backend_timeout_ms") { + t.Errorf("disabled timeout must be omitted from routing.json, got %s", data) + } + } +} diff --git a/internal/vcl/postamble.vcl b/internal/vcl/postamble.vcl index 70d64f45..935de702 100644 --- a/internal/vcl/postamble.vcl +++ b/internal/vcl/postamble.vcl @@ -7,3 +7,19 @@ sub vcl_recv { return (pass); } } + +sub vcl_backend_error { + # Report 504 rather than Varnish's default 503 on routes carrying a timeout. + # Only status and reason are touched — deliberately no return(deliver) — so + # builtin.vcl still renders the error body, and a user vcl_backend_error that + # returns first keeps full control (its return means this never runs). + # + # Cannot distinguish a timeout from any other fetch failure on such a route: + # a refused connection also reports 504. Varnish exposes no failure reason + # in vcl_backend_error. + if (bereq.http.X-Ghost-Timeout) { + set beresp.status = 504; + set beresp.reason = "Gateway Timeout"; + set beresp.ttl = 0s; + } +} diff --git a/internal/vcl/preamble.vcl b/internal/vcl/preamble.vcl index f6f6cd39..2e0ef924 100644 --- a/internal/vcl/preamble.vcl +++ b/internal/vcl/preamble.vcl @@ -29,6 +29,7 @@ sub vcl_recv { unset req.http.X-Ghost-Grace; unset req.http.X-Ghost-Keep; unset req.http.X-Ghost-Cache-Key-Extra; + unset req.http.X-Ghost-Timeout; unset req.http.X-Ghost-Filter-Context; unset req.http.X-Ghost-Redirect-Config; unset req.http.X-Ghost-Error; @@ -107,6 +108,27 @@ sub vcl_backend_fetch { # because vcl_backend_response needs to read them. They are cleaned up # at the end of vcl_backend_response instead. unset bereq.http.X-Ghost-Pass; + + # Per-route timeout. Ghost sets X-Ghost-Timeout in vcl_recv because backends + # are pooled by address:port and cannot carry per-route timeouts themselves. + # + # Deliberately NOT unset here: it must survive the fetch so the postamble + # vcl_backend_error can tell a timed-out route apart from an ordinary 503. + # + # connect_timeout is included so an unreachable pod fails inside the route's + # budget instead of varnishd's 3.5s global. Gateway API scopes backendRequest + # to after request headers are sent, but the bound users want is their own + # wall-clock wait. + # + # Each std.duration() fallback is that parameter's varnishd default, never 0s: + # a malformed value must not become "time out immediately". + # ponytail: between_bytes bounds the gap between body bytes, not the total — + # a slow drip still streams forever, and SSE on a timed-out route gets cut. + if (bereq.http.X-Ghost-Timeout) { + set bereq.connect_timeout = std.duration(bereq.http.X-Ghost-Timeout, 3.5s); + set bereq.first_byte_timeout = std.duration(bereq.http.X-Ghost-Timeout, 60s); + set bereq.between_bytes_timeout = std.duration(bereq.http.X-Ghost-Timeout, 60s); + } } sub vcl_backend_response { @@ -151,6 +173,7 @@ sub vcl_backend_response { unset bereq.http.X-Ghost-Forced-TTL; unset bereq.http.X-Ghost-Grace; unset bereq.http.X-Ghost-Keep; + unset bereq.http.X-Ghost-Timeout; } sub vcl_deliver { diff --git a/mkdocs.yml b/mkdocs.yml index f921e028..0fc44852 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -134,6 +134,7 @@ nav: - Troubleshooting: operations/troubleshooting.md - Reference: - HTTPRoute Filters: reference/httproute-filters.md + - HTTPRoute Timeouts: reference/httproute-timeouts.md - GatewayClassParameters: reference/gatewayclassparameters.md - VarnishCachePolicy: reference/varnishcachepolicy.md - VarnishCacheInvalidation: reference/varnishcacheinvalidation.md