diff --git a/config/config.go b/config/config.go index 2dfd3a91d1..09c909f875 100644 --- a/config/config.go +++ b/config/config.go @@ -46,6 +46,7 @@ type Config struct { Insecure bool `yaml:"insecure"` AllowInsecureBackends bool `yaml:"allow-insecure-backends"` ProxyPreserveHost bool `yaml:"proxy-preserve-host"` + ProxyPreserveOriginal bool `yaml:"proxy-preserve-original"` DevMode bool `yaml:"dev-mode"` SupportListener string `yaml:"support-listener"` DebugListener string `yaml:"debug-listener"` @@ -456,6 +457,7 @@ func NewConfig() *Config { flag.BoolVar(&cfg.Insecure, "insecure", false, "flag indicating to ignore the verification of the TLS certificates of the backend services") flag.BoolVar(&cfg.AllowInsecureBackends, "allow-insecure-backends", false, "enables the per-route proxySSLVerifyOff() filter that skips TLS certificate verification for individual backends; disabled by default") flag.BoolVar(&cfg.ProxyPreserveHost, "proxy-preserve-host", false, "flag indicating to preserve the incoming request 'Host' header in the outgoing requests") + flag.BoolVar(&cfg.ProxyPreserveOriginal, "proxy-preserve-original", false, "flag indicating to preserve the original, pre-filter-chain request and response metadata, accessible to filters via ctx.OriginalRequest()/ctx.OriginalResponse(); required by any filter that needs the request as it was before path or header modifying filters ran; adds a per-request metadata clone") flag.BoolVar(&cfg.DevMode, "dev-mode", false, "enables developer time behavior, like unbuffered routing updates") flag.StringVar(&cfg.SupportListener, "support-listener", ":9911", "network address used for exposing the /metrics endpoint. An empty value disables support endpoint.") flag.StringVar(&cfg.DebugListener, "debug-listener", "", "when this address is set, skipper starts an additional listener returning the original and transformed requests") @@ -1310,6 +1312,10 @@ func (c *Config) ToOptions() skipper.Options { options.ProxyFlags |= proxy.PreserveHost } + if c.ProxyPreserveOriginal { + options.ProxyFlags |= proxy.PreserveOriginal + } + if c.RemoveHopHeaders { options.ProxyFlags |= proxy.HopHeadersRemoval } diff --git a/docs/operation/operation.md b/docs/operation/operation.md index c280ca40e9..a8bea6416d 100644 --- a/docs/operation/operation.md +++ b/docs/operation/operation.md @@ -1695,6 +1695,22 @@ If the second interpretation gets considered the right way, and the other one a bug, then the default value for this flag may become to be on. +## Preserving the Original Request + +Filters that need the request or response as they were before any filter in the +route ran can call `ctx.OriginalRequest()` / `ctx.OriginalResponse()`. By default these return +`nil`: cloning that metadata on every request has a cost, so it is opt-in. + +Set `-proxy-preserve-original` to enable it on the main listener. The `-debug-listener` (see +[Debugging Requests](#debugging-requests)) always has this behavior enabled internally, regardless +of this flag, since it needs the original request to produce its diff output. + +Any filter that rewrites the request, such as +[`modPath`](../reference/filters.md#modpath) or [`setPath`](../reference/filters.md#setpath), +makes the pre-filter-chain request unavailable through `ctx.Request()` to filters +running later in the chain. Those later filters need `-proxy-preserve-original` set to recover it +via `ctx.OriginalRequest()`; without the flag it silently returns `nil`. + ## Debugging Requests Skipper provides [filters](../reference/filters.md), that can change @@ -2056,6 +2072,11 @@ flowchart TD - `cache.reval_dropped`: Counter, revalidation jobs dropped because the queue was full or body read failed - `cache.reval_error`: Counter, background revalidation fetch failures - `cache.reval_duration`: Histogram, end-to-end duration of each background revalidation job +- `cache.reval_backend_dispatch`: Counter, revalidation fetches sent directly to the + backend, bypassing skipper's proxy pipeline (force mode, static backend + only) - so they're invisible to `MeasureBackend*` metrics and the access + log. Self-loopback revalidations aren't counted here since they go + through the listener normally. **L2 (if Valkey or Redis is configured):** diff --git a/docs/reference/filters.md b/docs/reference/filters.md index 206dc12ade..bfc41fd412 100644 --- a/docs/reference/filters.md +++ b/docs/reference/filters.md @@ -446,6 +446,12 @@ approach of certain LB implementations. ## HTTP Path + +Filters in this section rewrite the request path. Once one of them has run, the original, +pre-filter-chain request is only reachable from later filters through +`ctx.OriginalRequest()`, which stays `nil` unless the `-proxy-preserve-original` flag is set +(`false` by default). See [Preserving the Original Request](../operation/operation.md#preserving-the-original-request). + ### modPath Replace all matched regex expressions in the path. @@ -4102,7 +4108,17 @@ routes. Stale-while-revalidate (SWR): a stale entry within the SWR window is served immediately while a background fetch refreshes the entry. Concurrent cold-miss -requests for the same key are coalesced into a single upstream fetch. +requests for the same key are coalesced into a single upstream fetch. The SWR +window's source depends on the mode: in force mode it's the operator-supplied +`swrWindow` parameter below; in RFC mode it's the response's own +[`stale-while-revalidate`](https://www.rfc-editor.org/rfc/rfc5861#section-3) +`Cache-Control` directive — if the upstream never sends that directive, RFC-mode +entries have no SWR window and simply expire at TTL. The background fetch is +dispatched directly to the route's backend when possible (force mode with a +static backend), otherwise it loops back through skipper's own listener; see +`cache.reval_backend_dispatch` in the +[operation guide](../operation/operation.md#cache) for the observability +difference between the two. Unsafe methods (`POST`, `PUT`, `DELETE`, `PATCH`) invalidate the cached entry on success. `HEAD 200` freshens stored headers without replacing the body. diff --git a/filters/cache/cache_control.go b/filters/cache/cache_control.go index 5d3c47fad8..4932f175fb 100644 --- a/filters/cache/cache_control.go +++ b/filters/cache/cache_control.go @@ -8,14 +8,15 @@ import ( ) type cacheDirectives struct { - noStore bool - noCache bool - private bool - mustRevalidate bool - proxyRevalidate bool - public bool - maxAge int64 // -1 = not present; 0 means max-age=0 - sMaxAge int64 // -1 = not present + noStore bool + noCache bool + private bool + mustRevalidate bool + proxyRevalidate bool + public bool + maxAge int64 // -1 = not present; 0 means max-age=0 + sMaxAge int64 // -1 = not present + staleWhileRevalidate int64 // -1 = not present (RFC 5861 §3) } type requestCacheDirectives struct { @@ -68,7 +69,11 @@ func parseRequestCacheControl(h http.Header) requestCacheDirectives { // Uses Header.Values to handle multiple header lines; matches names // case-insensitively per RFC 9111 §5.2. func parseCacheControl(h http.Header) cacheDirectives { - d := cacheDirectives{maxAge: -1, sMaxAge: -1} + d := cacheDirectives{ + maxAge: -1, + sMaxAge: -1, + staleWhileRevalidate: -1, + } for _, line := range h.Values("Cache-Control") { for token := range strings.SplitSeq(line, ",") { parts := strings.SplitN(strings.TrimSpace(token), "=", 2) @@ -98,6 +103,12 @@ func parseCacheControl(h http.Header) cacheDirectives { d.sMaxAge = v } } + case "stale-while-revalidate": + if len(parts) == 2 { + if v, err := strconv.ParseInt(strings.TrimSpace(parts[1]), 10, 64); err == nil { + d.staleWhileRevalidate = v + } + } } } } diff --git a/filters/cache/cache_control_test.go b/filters/cache/cache_control_test.go index f93df6a2bd..d807234ebc 100644 --- a/filters/cache/cache_control_test.go +++ b/filters/cache/cache_control_test.go @@ -59,19 +59,23 @@ func TestParseCacheControl(t *testing.T) { header http.Header want cacheDirectives }{ - {"no-store", http.Header{"Cache-Control": {"no-store"}}, cacheDirectives{noStore: true, maxAge: -1, sMaxAge: -1}}, - {"no-cache", http.Header{"Cache-Control": {"no-cache"}}, cacheDirectives{noCache: true, maxAge: -1, sMaxAge: -1}}, - {"private", http.Header{"Cache-Control": {"private"}}, cacheDirectives{private: true, maxAge: -1, sMaxAge: -1}}, - {"must-revalidate", http.Header{"Cache-Control": {"must-revalidate"}}, cacheDirectives{mustRevalidate: true, maxAge: -1, sMaxAge: -1}}, - {"comma-separated", http.Header{"Cache-Control": {"no-store, must-revalidate"}}, cacheDirectives{noStore: true, mustRevalidate: true, maxAge: -1, sMaxAge: -1}}, - {"multiple lines", http.Header{"Cache-Control": {"no-cache", "must-revalidate"}}, cacheDirectives{noCache: true, mustRevalidate: true, maxAge: -1, sMaxAge: -1}}, - {"case-insensitive", http.Header{"Cache-Control": {"NO-STORE"}}, cacheDirectives{noStore: true, maxAge: -1, sMaxAge: -1}}, - {"value suffix stripped", http.Header{"Cache-Control": {`no-cache="x-private"`}}, cacheDirectives{noCache: true, maxAge: -1, sMaxAge: -1}}, - {"empty", http.Header{}, cacheDirectives{maxAge: -1, sMaxAge: -1}}, - {"max-age=3600", http.Header{"Cache-Control": {"max-age=3600"}}, cacheDirectives{maxAge: 3600, sMaxAge: -1}}, - {"s-maxage=60", http.Header{"Cache-Control": {"s-maxage=60"}}, cacheDirectives{maxAge: -1, sMaxAge: 60}}, - {"max-age=3.4", http.Header{"Cache-Control": {"max-age=3.4"}}, cacheDirectives{maxAge: -1, sMaxAge: -1}}, // malformed: ParseInt fails, sentinel unchanged - {"max-age=0,max-age=5", http.Header{"Cache-Control": {"max-age=0, max-age=5"}}, cacheDirectives{maxAge: 5, sMaxAge: -1}}, // last-write-wins; no duplicate guard + {"no-store", http.Header{"Cache-Control": {"no-store"}}, cacheDirectives{noStore: true, maxAge: -1, sMaxAge: -1, staleWhileRevalidate: -1}}, + {"no-cache", http.Header{"Cache-Control": {"no-cache"}}, cacheDirectives{noCache: true, maxAge: -1, sMaxAge: -1, staleWhileRevalidate: -1}}, + {"private", http.Header{"Cache-Control": {"private"}}, cacheDirectives{private: true, maxAge: -1, sMaxAge: -1, staleWhileRevalidate: -1}}, + {"must-revalidate", http.Header{"Cache-Control": {"must-revalidate"}}, cacheDirectives{mustRevalidate: true, maxAge: -1, sMaxAge: -1, staleWhileRevalidate: -1}}, + {"comma-separated", http.Header{"Cache-Control": {"no-store, must-revalidate"}}, cacheDirectives{noStore: true, mustRevalidate: true, maxAge: -1, sMaxAge: -1, staleWhileRevalidate: -1}}, + {"multiple lines", http.Header{"Cache-Control": {"no-cache", "must-revalidate"}}, cacheDirectives{noCache: true, mustRevalidate: true, maxAge: -1, sMaxAge: -1, staleWhileRevalidate: -1}}, + {"case-insensitive", http.Header{"Cache-Control": {"NO-STORE"}}, cacheDirectives{noStore: true, maxAge: -1, sMaxAge: -1, staleWhileRevalidate: -1}}, + {"value suffix stripped", http.Header{"Cache-Control": {`no-cache="x-private"`}}, cacheDirectives{noCache: true, maxAge: -1, sMaxAge: -1, staleWhileRevalidate: -1}}, + {"empty", http.Header{}, cacheDirectives{maxAge: -1, sMaxAge: -1, staleWhileRevalidate: -1}}, + {"max-age=3600", http.Header{"Cache-Control": {"max-age=3600"}}, cacheDirectives{maxAge: 3600, sMaxAge: -1, staleWhileRevalidate: -1}}, + {"s-maxage=60", http.Header{"Cache-Control": {"s-maxage=60"}}, cacheDirectives{maxAge: -1, sMaxAge: 60, staleWhileRevalidate: -1}}, + {"max-age=3.4", http.Header{"Cache-Control": {"max-age=3.4"}}, cacheDirectives{maxAge: -1, sMaxAge: -1, staleWhileRevalidate: -1}}, // malformed: ParseInt fails, sentinel unchanged + {"max-age=0,max-age=5", http.Header{"Cache-Control": {"max-age=0, max-age=5"}}, cacheDirectives{maxAge: 5, sMaxAge: -1, staleWhileRevalidate: -1}}, // last-write-wins; no duplicate guard + {"stale-while-revalidate=3600", http.Header{"Cache-Control": {"stale-while-revalidate=3600"}}, cacheDirectives{maxAge: -1, sMaxAge: -1, staleWhileRevalidate: 3600}}, + {"stale-while-revalidate=0", http.Header{"Cache-Control": {"stale-while-revalidate=0"}}, cacheDirectives{maxAge: -1, sMaxAge: -1, staleWhileRevalidate: 0}}, + {"stale-while-revalidate malformed", http.Header{"Cache-Control": {"stale-while-revalidate=bad"}}, cacheDirectives{maxAge: -1, sMaxAge: -1, staleWhileRevalidate: -1}}, + {"max-age and stale-while-revalidate", http.Header{"Cache-Control": {"max-age=300, stale-while-revalidate=60"}}, cacheDirectives{maxAge: 300, sMaxAge: -1, staleWhileRevalidate: 60}}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/filters/cache/filter.go b/filters/cache/filter.go index a61932ee5d..f27d88333f 100644 --- a/filters/cache/filter.go +++ b/filters/cache/filter.go @@ -246,7 +246,7 @@ func (s *cacheSpec) revalidationWorker() { if job.filter != nil { s.metrics.MeasureSince("cache.reval_wait_duration", job.enqueuedAt) start := time.Now() - job.filter.doRevalidate(job.key, job.req) + job.filter.doRevalidate(job.key, job.req, job.backendURL, job.outgoingHost) s.metrics.MeasureSince("cache.reval_duration", start) } case <-s.ctx.Done(): @@ -275,10 +275,12 @@ func (s *cacheSpec) updateMetrics() { } type revalJob struct { - key string - req *http.Request // cloned via Request.Clone - filter *cacheFilter // instance whose doRevalidate to call - enqueuedAt time.Time // wall-clock time the job entered the queue; used to measure wait time + key string + req *http.Request // cloned via Request.Clone + backendURL string // non-empty: dial here directly instead of looping through listenAddr + outgoingHost string // Host header to send with backendURL; see revalidationDispatch + filter *cacheFilter // instance whose doRevalidate to call + enqueuedAt time.Time // wall-clock time the job entered the queue; used to measure wait time } // cacheFilter does not implement filters.FilterCloser. The routing layer calls @@ -411,11 +413,13 @@ func (f *cacheFilter) Request(ctx filters.FilterContext) { Request: ctx.Request(), // link response to originating request per net/http convention } ctx.Serve(notModified) - f.enqueueRevalidation(key, ctx.Request()) + revalReq, backendURL, outgoingHost := revalidationDispatch(ctx, f.rfcMode) + f.enqueueRevalidation(key, revalReq, backendURL, outgoingHost) return } ctx.Serve(headBodyOmitted(method, rsp)) - f.enqueueRevalidation(key, ctx.Request()) + revalReq, backendURL, outgoingHost := revalidationDispatch(ctx, f.rfcMode) + f.enqueueRevalidation(key, revalReq, backendURL, outgoingHost) return } @@ -516,10 +520,7 @@ func (f *cacheFilter) coalesce(ctx filters.FilterContext, key string) { }, nil } ttl, shouldStore := f.resolveTTL(resp.StatusCode, resp.Header, directives) - swr := f.swrWindow - if resp.StatusCode != http.StatusOK { - swr = 0 - } + swr := f.resolveSWR(resp.StatusCode, directives) cia := correctedInitialAge(requestTime, responseTime, resp.Header) coalescedHeader := resp.Header.Clone() stripHopByHop(coalescedHeader) @@ -679,6 +680,7 @@ func (f *cacheFilter) Response(ctx filters.FilterContext) { if !shouldStore { return } + swr := f.resolveSWR(rsp.StatusCode, directives) if ctx.Request().Header.Get("Authorization") != "" && !directives.public && !directives.mustRevalidate { return @@ -706,7 +708,7 @@ func (f *cacheFilter) Response(ctx filters.FilterContext) { sentinel := &Entry{ CreatedAt: time.Now(), TTL: ttl, - StaleWhileRevalidate: f.swrWindow, + StaleWhileRevalidate: swr, VaryHeaders: varyNames, } if err := f.storage.Set(ctx.Request().Context(), "vary:"+baseKey, sentinel); err != nil { @@ -715,10 +717,6 @@ func (f *cacheFilter) Response(ctx filters.FilterContext) { } } - swr := f.swrWindow - if rsp.StatusCode != http.StatusOK { - swr = 0 - } responseTime := time.Now() var requestTime time.Time if rt, ok := ctx.StateBag()[stateBagRequestTime].(time.Time); ok { @@ -749,17 +747,76 @@ func (f *cacheFilter) Response(ctx filters.FilterContext) { } } +// revalidationDispatch decides how background revalidation should reach the +// origin: directly at the route's resolved backend when safe (force mode, +// static network backend — ctx.BackendUrl() non-empty), or via self-loopback +// through skipper's own listener otherwise (RFC mode, or a load-balanced / +// dynamic backend with no single resolvable URL — BackendUrl() is empty for +// both per its own doc comment). +// +// The two branches deliberately use different request objects. Direct +// dispatch uses ctx.Request() — already transformed by earlier filters +// (modPath, setRequestHeader, ...) into exactly what should be sent to the +// backend. Using the pre-filter request here would be wrong: it would send +// the client-facing path/host straight to the backend, bypassing whatever +// those filters did. The self-loopback fallback still needs revalidationRequest +// (ctx.OriginalRequest(), falling back to ctx.Request()) because it has to +// re-match a route, which requires the pre-mutation path. +// +// outgoingHost is the Host header to send with the direct-dispatch request — +// ctx.OutgoingHost() (the same value the main proxy pipeline would use for this +// route), not the client-facing Host that ctx.Request() still carries. It is +// empty for self-loopback, which must keep the client-facing Host to re-match +// a route on re-entry. +// +// Note: RFC mode always uses self-loopback, never direct dispatch, even when +// ctx.BackendUrl() is non-empty. A Response()-filter positioned after cache() +// could rewrite Cache-Control; only the self-loopback path re-runs the full +// filter chain and sees that rewritten value before doRevalidate reads it for +// TTL and stale-while-revalidate purposes (resolveTTL, resolveSWR). Direct +// dispatch would read the pre-rewrite upstream headers instead. +func revalidationDispatch(ctx filters.FilterContext, rfcMode bool) (req *http.Request, backendURL string, outgoingHost string) { + if !rfcMode { + if b := ctx.BackendUrl(); b != "" { + return ctx.Request(), b, ctx.OutgoingHost() + } + } + return revalidationRequest(ctx), "", "" +} + +// revalidationRequest returns the request to replay for background revalidation. +// It must be ctx.OriginalRequest(), not ctx.Request(): by the time this filter's +// Request() runs, earlier filters (e.g. modPath stripping a path prefix before +// forwarding) may have already mutated ctx.Request() in place. doRevalidate loops +// the request back through skipper's own listener so the full filter chain reruns +// on it; replaying the already-mutated request can no longer match the route that +// produced that mutation, so revalidation permanently fails with a routing error +// and a stale entry (e.g. a cached error) never gets refreshed. +// ctx.OriginalRequest() can be nil per its interface contract, so fall back to +// ctx.Request() rather than passing nil into enqueueRevalidation. +func revalidationRequest(ctx filters.FilterContext) *http.Request { + if orig := ctx.OriginalRequest(); orig != nil { + return orig + } + return ctx.Request() +} + // enqueueRevalidation sends a revalidation job to the background worker. // The request is cloned in the calling goroutine before orig is released. +// backendURL and outgoingHost, when non-empty, tell doRevalidate to dispatch +// directly to that backend using that Host header instead of looping back +// through skipper's own listener (see revalidationDispatch). // If the queue is full the job is dropped and reval_dropped is incremented. // The closure captures f.doRevalidate so the spec-level worker respects this route's config. -func (f *cacheFilter) enqueueRevalidation(key string, orig *http.Request) { +func (f *cacheFilter) enqueueRevalidation(key string, orig *http.Request, backendURL, outgoingHost string) { cloned := orig.Clone(context.Background()) job := revalJob{ - key: key, - req: cloned, - filter: f, - enqueuedAt: time.Now(), + key: key, + req: cloned, + backendURL: backendURL, + outgoingHost: outgoingHost, + filter: f, + enqueuedAt: time.Now(), } select { case f.revalJobs <- job: @@ -773,11 +830,42 @@ func (f *cacheFilter) enqueueRevalidation(key string, orig *http.Request) { // doRevalidate revalidates key against the upstream. It sends a conditional // request (If-None-Match / If-Modified-Since) when the stored entry carries // validators; a 304 response reuses the stored payload and merges new headers. -func (f *cacheFilter) doRevalidate(key string, req *http.Request) { +// backendURL, when non-empty, is dialed directly using outgoingHost as the +// Host header, bypassing skipper's proxy pipeline (force mode, static backend +// only) - so it's invisible to MeasureBackend* metrics and the access log; +// cache.reval_backend_dispatch is incremented instead. Otherwise the request +// loops back through skipper's own listener (f.listenAddr), which is counted +// normally. +func (f *cacheFilter) doRevalidate(key string, req *http.Request, backendURL, outgoingHost string) { f.revalSF.Do(key, func() (any, error) { //nolint:errcheck - req.Header.Set(revalidateHeader, "1") - req.URL.Scheme = "http" - req.URL.Host = f.listenAddr + if backendURL != "" { + u, err := url.Parse(backendURL) + if err != nil { + f.metrics.IncCounter("cache.reval_error") + log.WithFields(log.Fields{ + "backendURL": backendURL, + }).WithError(err).Warn("cache: invalid backend URL for direct revalidation dispatch") + return nil, nil + } + req.URL.Scheme, req.URL.Host = u.Scheme, u.Host + // req.Host, not just req.URL.Host: net/http sends req.Host verbatim as + // the wire Host header when set, falling back to req.URL.Host only when + // empty. Left untouched, req.Host would still carry the client-facing + // Host from ctx.Request() (populated by the HTTP server on the original + // inbound request), so the real backend would see the client's Host + // instead of the one skipper's own proxy pipeline would send for this + // route (ctx.OutgoingHost(), applied here via outgoingHost). + req.Host = outgoingHost + f.metrics.IncCounter("cache.reval_backend_dispatch") + } else { + // revalidateHeader only matters for self-loopback: it tells this + // filter's own Request() to skip the cache lookup when the request + // re-enters skipper. Direct dispatch never re-enters skipper, so + // setting it there would just leak an internal header to the real + // origin. + req.Header.Set(revalidateHeader, "1") + req.URL.Scheme, req.URL.Host = "http", f.listenAddr + } req.RequestURI = "" if stored, err := f.storage.Get(context.Background(), key); err == nil && stored != nil { @@ -852,7 +940,7 @@ func (f *cacheFilter) doRevalidate(key string, req *http.Request) { Payload: body, CreatedAt: responseTime, TTL: ttl, - StaleWhileRevalidate: f.swrWindow, + StaleWhileRevalidate: f.resolveSWR(statusCode, directives), StaleIfError: f.staleIfError, ETag: responseHeader.Get("ETag"), LastModified: responseHeader.Get("Last-Modified"), @@ -922,6 +1010,26 @@ func (f *cacheFilter) resolveTTL(statusCode int, header http.Header, directives return ttl, true } +// resolveSWR decides the stale-while-revalidate window for a stored entry. +// +// Force mode (rfcMode==false): operator swrWindow is authoritative, matching +// resolveTTL's force-mode behavior. +// +// RFC mode (rfcMode==true): honors the response's stale-while-revalidate +// directive (RFC 5861 §3) when present; otherwise there is no SWR window. +func (f *cacheFilter) resolveSWR(statusCode int, directives cacheDirectives) time.Duration { + if statusCode != http.StatusOK { + return 0 + } + if !f.rfcMode { + return f.swrWindow + } + if directives.staleWhileRevalidate >= 0 { + return time.Duration(directives.staleWhileRevalidate) * time.Second + } + return 0 +} + // cacheKey builds a deterministic cache key from the route ID and request. // routeID is included so entries from different routes never collide when all // routes share the same LRUStorage instance. diff --git a/filters/cache/filter_test.go b/filters/cache/filter_test.go index 17e878daf4..0ee36d40c1 100644 --- a/filters/cache/filter_test.go +++ b/filters/cache/filter_test.go @@ -960,6 +960,293 @@ func TestCacheFilter_ConditionalRevalidation_LastModified_304(t *testing.T) { }) } +// TestCacheFilter_Revalidation_UsesOriginalRequestPath guards against a bug where +// background revalidation replayed ctx.Request() instead of ctx.OriginalRequest(). +// When an earlier filter (e.g. modPath) strips a path prefix before cache() runs, +// ctx.Request() no longer carries the prefix the real route requires, so looping +// the mutated request back through skipper's own router permanently fails to +// match any route and the stale entry can never be refreshed. (RouteGroup +// example: pathSubtree /api/contentful/ + modPath("^/api/contentful", "") before +// cache().) +// +// This exercises revalidationDispatch's self-loopback fallback specifically: +// FBackendUrl is left unset (empty), the same as a load-balanced or dynamic +// backend where ctx.BackendUrl() has no single resolvable URL. See +// TestCacheFilter_Revalidation_DirectDispatchToBackend for the force-mode + +// static-backend case, which bypasses this path entirely. +func TestCacheFilter_Revalidation_UsesOriginalRequestPath(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + f := newTestFilter(t, time.Millisecond, 15*time.Second, time.Hour) + + // Simulate the request as cache() sees it after an earlier modPath-style + // filter already stripped "/api/contentful" in place, versus the request + // the client actually sent (still carrying the prefix). + mutated, _ := http.NewRequest("GET", "http://backend.example/unify-footers/external/pl_PL", nil) + original, _ := http.NewRequest("GET", "http://backend.example/api/contentful/unify-footers/external/pl_PL", nil) + + newStaleCtx := func() *filtertest.Context { + return &filtertest.Context{ + FRequest: mutated, + FOriginalRequest: original, + FStateBag: make(map[string]any), + FMetrics: &metricstest.MockMetrics{}, + } + } + + ctx1 := newStaleCtx() + f.Request(ctx1) + ctx1.FResponse = upstreamResponseCC(http.StatusOK, `{"data":"v1"}`, "public, max-age=300") + f.Response(ctx1) + + // Advance past TTL but inside the SWR window, so the next request serves + // stale and triggers a background revalidation. + time.Sleep(2 * time.Millisecond) + + var revalidatedPath string + f.fetch = func(req *http.Request) (*http.Response, error) { + revalidatedPath = req.URL.Path + return upstreamResponseCC(http.StatusOK, `{"data":"v2"}`, "public, max-age=300"), nil + } + + ctx2 := newStaleCtx() + f.Request(ctx2) + synctest.Wait() + + if !ctx2.FServed { + t.Fatal("expected stale entry to be served") + } + const wantPath = "/api/contentful/unify-footers/external/pl_PL" + if revalidatedPath != wantPath { + t.Fatalf("revalidation request used path %q, want the original pre-filter path %q", revalidatedPath, wantPath) + } + }) +} + +// TestCacheFilter_Revalidation_DirectDispatchToBackend covers the force-mode + +// static-backend case: revalidation should dial ctx.BackendUrl() directly +// instead of looping back through skipper's own listener. This sidesteps the +// self-loopback routing failure entirely, regardless of which upstream filter +// mutated the request. It also asserts the internal revalidateHeader never +// reaches the real backend on this path, since direct dispatch never re-enters +// skipper's own Request() handler to strip it, and that the wire Host header +// is rewritten to ctx.OutgoingHost() rather than left as the client-facing +// Host a real inbound request would carry. +func TestCacheFilter_Revalidation_DirectDispatchToBackend(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + f := newTestFilter(t, time.Millisecond, 15*time.Second, time.Hour) + mockMetrics := &metricstest.MockMetrics{} + f.metrics = mockMetrics + + req, _ := http.NewRequest("GET", "http://backend.example/spaces/abc/entries", nil) + req.Host = "shop.example" // as a real inbound request would carry the client-facing Host + + newStaleCtx := func() *filtertest.Context { + return &filtertest.Context{ + FRequest: req, + FBackendUrl: "https://origin.example", + FOutgoingHost: "origin.example:443", + FStateBag: make(map[string]any), + FMetrics: &metricstest.MockMetrics{}, + } + } + + ctx1 := newStaleCtx() + f.Request(ctx1) + ctx1.FResponse = upstreamResponseCC(http.StatusOK, `{"data":"v1"}`, "public, max-age=300") + f.Response(ctx1) + + time.Sleep(2 * time.Millisecond) + + var revalidatedScheme, revalidatedURLHost, revalidatedWireHost, revalidateHeaderValue string + f.fetch = func(req *http.Request) (*http.Response, error) { + revalidatedScheme, revalidatedURLHost = req.URL.Scheme, req.URL.Host + revalidatedWireHost = req.Host + revalidateHeaderValue = req.Header.Get(revalidateHeader) + return upstreamResponseCC(http.StatusOK, `{"data":"v2"}`, "public, max-age=300"), nil + } + + ctx2 := newStaleCtx() + f.Request(ctx2) + synctest.Wait() + + if !ctx2.FServed { + t.Fatal("expected stale entry to be served") + } + if revalidatedScheme != "https" || revalidatedURLHost != "origin.example" { + t.Fatalf("revalidation request went to %s://%s, want https://origin.example (ctx.BackendUrl(), not the listener)", revalidatedScheme, revalidatedURLHost) + } + if revalidatedWireHost != "origin.example:443" { + t.Fatalf("revalidation request carried wire Host %q, want ctx.OutgoingHost() %q, not the client-facing Host", revalidatedWireHost, "origin.example:443") + } + if revalidateHeaderValue != "" { + t.Fatalf("direct-dispatch request carried %s=%q to the real backend; this internal header must only be set for self-loopback", revalidateHeader, revalidateHeaderValue) + } + mockMetrics.WithCounters(func(counters map[string]int64) { + if counters["cache.reval_backend_dispatch"] != 1 { + t.Errorf("expected cache.reval_backend_dispatch==1, got %d", counters["cache.reval_backend_dispatch"]) + } + }) + }) +} + +// TestCacheFilter_Revalidation_LBBackendFallsBackToLoopback covers a +// load-balanced or dynamic backend: ctx.BackendUrl() is empty (per its own +// doc comment, there's no single resolvable URL to dial), so even in force +// mode revalidation must still fall back to self-loopback rather than +// breaking on an empty host. +func TestCacheFilter_Revalidation_LBBackendFallsBackToLoopback(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + f := newTestFilter(t, time.Millisecond, 15*time.Second, time.Hour) + mockMetrics := &metricstest.MockMetrics{} + f.metrics = mockMetrics + + req, _ := http.NewRequest("GET", "http://backend.example/spaces/abc/entries", nil) + + newStaleCtx := func() *filtertest.Context { + return &filtertest.Context{ + FRequest: req, + FBackendUrl: "", // load-balanced/dynamic backend: no single URL + FStateBag: make(map[string]any), + FMetrics: &metricstest.MockMetrics{}, + } + } + + ctx1 := newStaleCtx() + f.Request(ctx1) + ctx1.FResponse = upstreamResponseCC(http.StatusOK, `{"data":"v1"}`, "public, max-age=300") + f.Response(ctx1) + + time.Sleep(2 * time.Millisecond) + + var revalidatedHost string + f.fetch = func(req *http.Request) (*http.Response, error) { + revalidatedHost = req.URL.Host + return upstreamResponseCC(http.StatusOK, `{"data":"v2"}`, "public, max-age=300"), nil + } + + ctx2 := newStaleCtx() + f.Request(ctx2) + synctest.Wait() + + if !ctx2.FServed { + t.Fatal("expected stale entry to be served") + } + if revalidatedHost != "localhost:9090" { + t.Fatalf("revalidation request went to host %q, want the listener (localhost:9090) since BackendUrl() is empty", revalidatedHost) + } + mockMetrics.WithCounters(func(counters map[string]int64) { + if counters["cache.reval_backend_dispatch"] != 0 { + t.Errorf("expected cache.reval_backend_dispatch==0 for self-loopback, got %d", counters["cache.reval_backend_dispatch"]) + } + }) + }) +} + +// TestCacheFilter_Revalidation_RFCMode_StaleServedAndRevalidated covers RFC +// mode background revalidation: resolveSWR honors the response's +// stale-while-revalidate directive (RFC 5861 §3), giving an RFC-mode entry a +// non-zero SWR window. BackendUrl() is set but must still be ignored +// (self-loopback) per revalidationDispatch's RFC-mode gating. +func TestCacheFilter_Revalidation_RFCMode_StaleServedAndRevalidated(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + f := newTestFilterRFC(t, 0, 0, 0) + + req, _ := http.NewRequest("GET", "http://backend.example/spaces/abc/entries", nil) + newStaleCtx := func() *filtertest.Context { + return &filtertest.Context{ + FRequest: req, + FBackendUrl: "https://origin.example", // present, but RFC mode must ignore it + FStateBag: make(map[string]any), + FMetrics: &metricstest.MockMetrics{}, + } + } + + ctx1 := newStaleCtx() + f.Request(ctx1) + ctx1.FResponse = upstreamResponseCC(http.StatusOK, `{"data":"v1"}`, "max-age=0, stale-while-revalidate=3600") + f.Response(ctx1) + + time.Sleep(time.Millisecond) + + var revalidatedHost string + f.fetch = func(req *http.Request) (*http.Response, error) { + revalidatedHost = req.URL.Host + return upstreamResponseCC(http.StatusOK, `{"data":"v2"}`, "max-age=0, stale-while-revalidate=3600"), nil + } + + ctx2 := newStaleCtx() + f.Request(ctx2) + synctest.Wait() + + if !ctx2.FServed { + t.Fatal("expected stale entry to be served") + } + if ctx2.FResponse.Header.Get("X-Cache-Status") != "STALE" { + t.Fatalf("expected STALE, got %q", ctx2.FResponse.Header.Get("X-Cache-Status")) + } + if revalidatedHost != "localhost:9090" { + t.Fatalf("revalidation request went to host %q, want the listener (localhost:9090); RFC mode must ignore BackendUrl()", revalidatedHost) + } + }) +} + +// TestRevalidationDispatch_RFCModeIgnoresBackendUrl is a direct unit test of +// revalidationDispatch's mode gating, not an integration test through +// Request()/Response() (see TestCacheFilter_Revalidation_RFCMode_StaleServedAndRevalidated +// for that). RFC mode always ignores BackendUrl() because a Response()-filter +// positioned after cache() could rewrite Cache-Control, and only the +// self-loopback path sees that rewritten value before doRevalidate reads it. +func TestRevalidationDispatch_RFCModeIgnoresBackendUrl(t *testing.T) { + req, _ := http.NewRequest("GET", "http://backend.example/spaces/abc/entries", nil) + ctx := &filtertest.Context{ + FRequest: req, + FOriginalRequest: req, + FBackendUrl: "https://origin.example", // present, but RFC mode must ignore it + FOutgoingHost: "origin.example", // present, but RFC mode must ignore it too + } + + gotReq, gotBackendURL, gotOutgoingHost := revalidationDispatch(ctx, true /* rfcMode */) + + if gotBackendURL != "" { + t.Fatalf("rfcMode=true: got backendURL %q, want empty (must fall back to self-loopback)", gotBackendURL) + } + if gotOutgoingHost != "" { + t.Fatalf("rfcMode=true: got outgoingHost %q, want empty (self-loopback keeps the client-facing Host)", gotOutgoingHost) + } + if gotReq != req { + t.Fatalf("rfcMode=true: got a different request than revalidationRequest(ctx) would return") + } +} + +// TestRevalidationDispatch_ForceModeUsesBackendUrl is the force-mode +// counterpart: with a resolvable static backend, dispatch should go directly +// there using ctx.Request() (not ctx.OriginalRequest()), since ctx.Request() +// is what earlier filters already transformed into the backend-ready form. +// It should also report ctx.OutgoingHost() as the Host header to send, not +// the client-facing Host ctx.Request() still carries. +func TestRevalidationDispatch_ForceModeUsesBackendUrl(t *testing.T) { + mutated, _ := http.NewRequest("GET", "http://backend.example/spaces/abc/entries", nil) + original, _ := http.NewRequest("GET", "http://backend.example/api/contentful/spaces/abc/entries", nil) + ctx := &filtertest.Context{ + FRequest: mutated, + FOriginalRequest: original, + FBackendUrl: "https://origin.example", + FOutgoingHost: "origin.example", + } + + gotReq, gotBackendURL, gotOutgoingHost := revalidationDispatch(ctx, false /* rfcMode */) + + if gotBackendURL != "https://origin.example" { + t.Fatalf("rfcMode=false, BackendUrl set: got backendURL %q, want %q", gotBackendURL, "https://origin.example") + } + if gotOutgoingHost != "origin.example" { + t.Fatalf("rfcMode=false, BackendUrl set: got outgoingHost %q, want %q", gotOutgoingHost, "origin.example") + } + if gotReq != mutated { + t.Fatalf("rfcMode=false, BackendUrl set: should use ctx.Request() (filter-transformed), not ctx.OriginalRequest()") + } +} + func TestCacheFilter_RevalidationError_MetricIncremented(t *testing.T) { url := newLocalBackend(t) + "/spaces/abc/entries/reval-err" synctest.Test(t, func(t *testing.T) {