Repository navigation
Conversation
Background revalidation in filters/cache looped the request back through skipper's own listener using ctx.Request(), which earlier filters (e.g. modPath) may have already mutated in place. If a route strips a path prefix before cache() runs, the replayed request no longer carries that prefix and permanently fails to match any route, so a stale entry (including a cached error response) can never be refreshed. Use ctx.OriginalRequest() for the actual revalidation fetch instead, falling back to ctx.Request() if it's nil. The cache key is unaffected and still intentionally reflects filter-mutated state. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Background revalidation looped every request back through skipper's own listener so the full filter chain could rerun on it. That re-entry has to re-match a route, which is fragile to anything an earlier filter changed (not just the modPath case zalando#4315 fixed) and was never guaranteed to work in the first place. For force-mode routes with a resolvable static backend (ctx.BackendUrl() non-empty), dispatch the revalidation fetch directly there instead, using the already filter-transformed ctx.Request() — no re-routing needed, so no route-matching failure is possible. RFC-mode routes and load-balanced/dynamic backends (ctx.BackendUrl() empty) keep today's self-loopback behavior via zalando#4315's revalidationRequest() fallback: RFC mode 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 for TTL purposes; LB/dynamic backends because there's no single backend URL to dial directly. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
CreateFilter only parses swrWindow for the 3-5 arg (force-mode) form, so RFC-mode entries always had a zero-width SWR window and could never reach the stale-serve-and-revalidate path, regardless of this PR's dispatch-target change for that path. Add resolveSWR, a resolveTTL-style sibling that keeps the operator swrWindow authoritative in force mode but, in RFC mode, honors the response's stale-while-revalidate directive (RFC 5861) parsed by parseCacheControl. Use it at all four sites that previously read f.swrWindow directly. revalidationDispatch's RFC-mode gating is unchanged: RFC mode still always self-loopbacks regardless of ctx.BackendUrl(), since a Response()-filter after cache() can rewrite Cache-Control and only the self-loopback path sees that rewritten value before doRevalidate reads it. That reasoning no longer depends on RFC-mode SWR being unreachable, so the comment and this PR's description are updated accordingly. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Direct-backend revalidation dispatch bypasses skipper's own proxy pipeline (Proxy.ServeHTTP), so it never produces the usual MeasureBackend*/MeasureServe metrics or an access-log line - those only fire for the self-loopback path's inner hop back through skipper's listener. Without this counter, direct-dispatch revalidation traffic was invisible to anything but the cache_revalidation trace span. Increment it in doRevalidate at the point backendURL is applied to the outgoing request, and assert it (==1 for direct dispatch, ==0 for self-loopback) in the existing dispatch-target tests. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Trim the metric description in doRevalidate's doc comment and its docs/operation/operation.md duplicate down to what's load-bearing. Also drop "now actually" from a test doc comment that referenced the PR's before/after state instead of just describing current behavior. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
larry-dalmeida
changed the title
larry-dalmeida
marked this pull request as ready for review
October 2, 2026 14:07
larry-dalmeida
changed the title
req.Header.Set(revalidateHeader, "1") ran unconditionally in doRevalidate, but only the self-loopback dispatch path ever strips it (cache()'s own Request() deletes it once the request re-enters skipper). The direct-dispatch path added for force-mode/static-backend revalidation never re-enters skipper, so X-Cache-Revalidate: 1 was being sent straight through to the real origin on every direct-dispatch revalidation. Move the header set into the self-loopback branch only, since direct dispatch has no use for it anyway. Extend TestCacheFilter_Revalidation_DirectDispatchToBackend to assert the header is absent from the request f.fetch receives. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
…erence The new stale-while-revalidate directive support cited "RFC 5861" without a section, while this file already cites "RFC 5861 §4" for stale-if-error a few lines away. Section 3 is the stale-while-revalidate extension; cite it the same way. docs/reference/filters.md's cache() section still described SWR only in force-mode terms (the swrWindow parameter). RFC mode now derives its own SWR window from the response's stale-while-revalidate directive instead of that dead code path - existing RFC-mode operators may see new background-revalidation traffic they haven't seen before, so note where the window comes from in each mode and cross-link cache.reval_backend_dispatch for the observability difference between direct-dispatch and self-loopback revalidation. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
szuecs
reviewed
Oct 3, 2026
| // 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} |
Member
There was a problem hiding this comment.
please add line breaks to make it more readable
Member
|
Should we document that if you modify path that you should run the proxy with PreserveOriginal Flag? |
Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
doRevalidate's direct-dispatch branch rewrote req.URL.Scheme/Host to dial the real backend, but left req.Host untouched. net/http sends req.Host verbatim as the wire Host header when set, falling back to req.URL.Host only when empty. The cloned request still carried the client-facing Host from the original inbound request, so the real backend received the client's Host instead of the one skipper's own proxy pipeline would send for this route (ctx.outgoingHost, via PreserveHost/route.Host) - breaking any backend that virtual-hosts by Host. Thread ctx.OutgoingHost() through revalidationDispatch -> enqueueRevalidation -> revalJob -> doRevalidate, mirroring how backendURL is already threaded, and set req.Host alongside the existing req.URL.Host rewrite. Self-loopback keeps outgoingHost empty, since it still needs the client-facing Host to re-match a route on re-entry. Extend TestCacheFilter_Revalidation_DirectDispatchToBackend to use a request with .Host pre-populated like a real inbound request, and assert the wire Host sent to the backend is ctx.OutgoingHost(), not the client's. Extend both revalidationDispatch unit tests to assert the new return value in each mode. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
larry-dalmeida
marked this pull request as draft
October 5, 2026 09:11
ctx.OriginalRequest()/ctx.OriginalResponse() only return non-nil when proxy.PreserveOriginal is set on Proxy.Flags, but no CLI/YAML option wired it onto the main listener - the only CLI-reachable use was -debug-listener, which enables it on its own separate diagnostic listener. Any filter or dataclient that needs the pre-filter-chain request after a path- or header-modifying filter has run (e.g. after modPath/setPath) was silently getting nil in production. Add -proxy-preserve-original / proxy-preserve-original, following the same pattern as -proxy-preserve-host. Off by default, since cloning the request/response metadata on every request has a cost. Document the flag and the general PreserveOriginal/OriginalRequest mechanism in operation.md, cross-linked from the modPath/setPath filter reference, which previously had no mention of it anywhere in docs/. Addresses szuecs's open question on PR zalando#4315 (referencing proxy/context.go:155). Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Dataclients fetch routing information from upstream sources and have no access to the per-request filter context, so they cannot call ctx.OriginalRequest()/ctx.OriginalResponse(). Only filters can. Addresses szuecs's review comments on PR zalando#4315. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
larry-dalmeida
marked this pull request as ready for review
October 5, 2026 12:05
larry-dalmeida
marked this pull request as draft
October 5, 2026 12:07
larry-dalmeida
marked this pull request as ready for review
October 5, 2026 12:21
Collaborator
Author
|
👍 |
Collaborator
Author
yes - updated doc too. |
Member
|
👍 |
This was referenced Oct 6, 2026
larry-dalmeida
added a commit
to larry-dalmeida/kubernetes-on-aws
that referenced
this pull request
Oct 8, 2026
Includes zalando/skipper#4315 (background cache revalidation fix) and zalando/skipper#4314. ref: zalando/skipper@v0.28.1...v0.28.29 Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
larry-dalmeida
added a commit
to larry-dalmeida/kubernetes-on-aws
that referenced
this pull request
Oct 8, 2026
Includes zalando/skipper#4315 (background cache revalidation fix) and zalando/skipper#4314. ref: zalando/skipper@v0.28.1...v0.28.29 Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Background revalidation (
stale-while-revalidate) infilters/cacheloops the request back through skipper's own listener so the full filter chain reruns. Two independent bugs made that unreliable:ctx.Request(), which earlier filters (e.g.modPathstripping a path prefix) may have already mutated. A route likepathSubtree: /api/contentful/+modPath("^/api/contentful", "")loses its prefix on replay, so the loopback permanently fails to match any route - stale entries, including cached error responses, can never refresh. Observed in production as a404replaying viaX-Cache-Status: HIT/STALEindefinitely as seen in the screenshot below.modPath.CreateFilteronly parsedswrWindowfor the force-mode form, so RFC-mode entries always had a zero-width SWR window andEntry.IsStalecould never be true - the stale-serve-and-revalidate path was dead code for RFC mode.Fix
ctx.OriginalRequest()for the revalidation fetch, falling back toctx.Request()if nil. The cache key is unchanged.ctx.BackendUrl()non-empty), dispatch the revalidation fetch directly there using the filter-transformed request - no re-routing, so no route-matching failure. RFC mode and load-balanced/dynamic backends keep self-loopback: RFC mode because aResponse()-filter aftercache()can rewriteCache-Control, and only self-loopback sees that beforedoRevalidatereads it; LB/dynamic backends because there's no single URL to dial directly.resolveSWR(sibling toresolveTTL): force mode keeps operatorswrWindowauthoritative; RFC mode now honors the response'sstale-while-revalidate=Ndirective (RFC 5861), parsed byparseCacheControl.cache.reval_backend_dispatchmetric, incremented on direct-dispatch revalidation, since that path bypassesProxy.ServeHTTPand produces none of the usual backend/access-log metrics the self-loopback path gets for free. Documented indocs/operation/operation.md.-proxy-preserve-original/proxy-preserve-originalflag, wiringproxy.PreserveOriginalonto the main listener'sProxy.Flags(previously only reachable via the separate-debug-listener).ctx.OriginalRequest()returnsnilwithout this flag set, so for RFC-mode and load-balanced/dynamic-backend routes - which still use self-loopback - this flag must be enabled for the first fix above to actually take effect, rather than silently falling back to the already-mutatedctx.Request(). Documented indocs/operation/operation.md("Preserving the Original Request") and cross-linked fromdocs/reference/filters.md's HTTP Path section (modPath/setPath).Testing
TestCacheFilter_Revalidation_UsesOriginalRequestPath- original-request fallback.TestCacheFilter_Revalidation_DirectDispatchToBackend/_LBBackendFallsBackToLoopback- dispatch-target selection, assertingcache.reval_backend_dispatch.TestRevalidationDispatch_RFCModeIgnoresBackendUrl/_ForceModeUsesBackendUrl- dispatch-gating unit tests.TestCacheFilter_Revalidation_RFCMode_StaleServedAndRevalidated- RFC-mode integration test proving the previously-dead path now fires end-to-end.TestParseCacheControl(extended) -stale-while-revalidateparsing cases.