feat: allow overriding session persistence cookie Path via BackendTrafficPolicy#9446
feat: allow overriding session persistence cookie Path via BackendTrafficPolicy#9446Aias00 wants to merge 6 commits into
Conversation
…fficPolicy Envoy Gateway derives the session persistence cookie Path from the matched HTTPRoute path. When an upstream proxy rewrites the request path before it reaches Envoy Gateway, the derived Path no longer matches the client-facing URL, so the browser stops sending the cookie back and sticky sessions break. Add a sessionPersistence.cookie.path override to BackendTrafficPolicy that pins the cookie Path to a fixed value. It flows through the IR CookieBasedSessionPersistence.Path field to the Envoy cookie stateful session config, overriding the route-derived Path. It only affects routes that already enable session persistence via the Gateway API sessionPersistence field. Fixes envoyproxy#8580 Signed-off-by: liuhy <liuhongyu@apache.org>
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR adds an API and translation-path override to pin the session persistence cookie Path via BackendTrafficPolicy, fixing sticky sessions when an upstream proxy rewrites request paths before Envoy Gateway sees them (issue #8580).
Findings (ordered by severity):
- None.
Changes:
- Adds
spec.sessionPersistence.cookie.pathtoBackendTrafficPolicy(API, CRDs, and generated docs). - Extends IR (
CookieBasedSessionPersistence.Path) and xDS translation to use the override when set, otherwise falling back to the route-derived path. - Adds/updates translator and Gateway API golden tests plus a release-note fragment.
Reviewed changes
Copilot reviewed 9 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| site/content/en/latest/api/extension_types.md | Documents the new BackendTrafficPolicy.spec.sessionPersistence.cookie.path override and related types. |
| release-notes/current/new_features/8580-backendtrafficpolicy-session-persistence-cookie-path.md | Release note fragment for the new override capability. |
| internal/xds/translator/testdata/out/xds-ir/http-route-session-persistence.routes.yaml | Updates xDS golden output to assert overridden cookie Path is emitted. |
| internal/xds/translator/testdata/in/xds-ir/http-route-session-persistence.yaml | Adds xDS golden input case exercising cookie path: "/" override. |
| internal/xds/translator/session_persistence.go | Implements override handling when building Envoy cookie-based session state config. |
| internal/ir/zz_generated.deepcopy.go | Regenerates IR deepcopy to include the new cookie Path field. |
| internal/ir/xds.go | Extends IR cookie session persistence with optional Path. |
| internal/gatewayapi/testdata/backendtrafficpolicy-with-session-persistence-cookie-path.out.yaml | Adds Gateway API golden output asserting IR cookie path: "/" and policy Accepted. |
| internal/gatewayapi/testdata/backendtrafficpolicy-with-session-persistence-cookie-path.in.yaml | Adds Gateway API golden input for HTTPRoute + BTP sessionPersistence cookie path override. |
| internal/gatewayapi/backendtrafficpolicy.go | Applies session persistence overrides from BTP to routes that already enable session persistence. |
| charts/gateway-helm/charts/crds/crds/generated/gateway.envoyproxy.io_backendtrafficpolicies.yaml | Regenerates CRD schema to include spec.sessionPersistence.cookie.path. |
| charts/gateway-crds-helm/templates/generated/gateway.envoyproxy.io_backendtrafficpolicies.yaml | Regenerates CRD template schema to include spec.sessionPersistence.cookie.path. |
| api/v1alpha1/zz_generated.deepcopy.go | Regenerates API deepcopy for new SessionPersistenceOverride types and field. |
| api/v1alpha1/backendtrafficpolicy_types.go | Adds API types/field for spec.sessionPersistence.cookie.path override with length validations. |
Files not reviewed (2)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
- internal/ir/zz_generated.deepcopy.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 876cc4b948
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| When specified, this overrides the EnvoyProxy-level setting for the relevant targetRefs. | ||
| If not specified, the EnvoyProxy-level setting is used. | ||
| type: string | ||
| sessionPersistence: |
There was a problem hiding this comment.
Because make gen-check runs helm-template and then fails if git status --porcelain is non-empty (tools/make/lint.mk), adding this schema block also requires regenerating the Helm golden outputs. The committed test/helm/gateway-crds-helm/{all,envoy-gateway-crds,e2e}.out.yaml still go straight from routingType to targetRef, so make gen-check will render this new block and fail with uncommitted diffs.
Useful? React with 👍 / 👎.
| // the session cookie Path. | ||
| // | ||
| // This only affects routes that already enable session persistence via the | ||
| // Gateway API HTTPRoute (or GRPCRoute) sessionPersistence field. It does not |
There was a problem hiding this comment.
For GRPCRoute rules this documented case never works: processGRPCRouteRule builds IR routes without reading rule.SessionPersistence or setting irRoute.SessionPersistence, and this new override returns when that field is nil. A BackendTrafficPolicy targeting a GRPCRoute that uses cookie session persistence will be accepted but will not produce the requested cookie path, so either implement the GRPCRoute path or remove GRPCRoute from the contract.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9446 +/- ##
==========================================
+ Coverage 75.42% 75.45% +0.02%
==========================================
Files 252 252
Lines 41660 41673 +13
==========================================
+ Hits 31422 31443 +21
+ Misses 8109 8098 -11
- Partials 2129 2132 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Regenerate test/helm/gateway-crds-helm golden outputs so make gen-check passes with the new BackendTrafficPolicy sessionPersistence schema block. - Remove the GRPCRoute mention from the sessionPersistence override docs and helper comment, since session persistence IR is only populated for HTTPRoute rules; the override only applies to HTTPRoute-based session persistence. Signed-off-by: liuhy <liuhongyu@apache.org>
… docs The CRD manifests were not regenerated after removing the GRPCRoute mention from the sessionPersistence override doc comment, so make gen-check failed on the stale '(or GRPCRoute)' text. Regenerate the BackendTrafficPolicy CRDs and the gateway-crds-helm golden outputs to match the source. Signed-off-by: liuhy <liuhongyu@apache.org>
PR envoyproxy#9446 adds BackendTrafficPolicy.spec.sessionPersistence.cookie.path to override the session persistence cookie Path, but only unit/golden tests covered the translation chain. Add an end-to-end test that verifies the behavior the field exists for: the Set-Cookie Path the client actually receives. The test targets a self-contained HTTPRoute (cookie-based session persistence on /cookie-session-persistence-cookie-path) plus a BackendTrafficPolicy that pins sessionPersistence.cookie.path to "/". It asserts the Set-Cookie Path is "/" (not the route-derived path) and that sticky sessions still hold across 10 requests. It deliberately uses its own manifest so the existing SessionPersistenceTest cookie assertion (Path == route path) is unaffected. Signed-off-by: liuhy <liuhongyu@apache.org> Co-Authored-By: Claude <noreply@anthropic.com>
What this PR does
Fixes #8580.
Envoy Gateway derives the session persistence cookie
Pathfrom the matched HTTPRoute path. When an upstream edge/proxy rewrites the request path before it reaches Envoy Gateway (e.g. client requests/, edge rewrites to/foo/bar), the cookie is set withPath=/foo/bar. On the next request to/, the browser does not send the cookie back, so sticky sessions break even though the user-facing URL is still rooted at/.This adds a
sessionPersistence.cookie.pathoverride toBackendTrafficPolicythat pins the cookiePathto a fixed value, regardless of the matched route path.Changes
sessionPersistencefield onBackendTrafficPolicy(SessionPersistenceOverride→SessionPersistenceCookieOverride{path}).CookieBasedSessionPersistence.Path.Path, falling back to the route-derived path when unset.sessionPersistencefield (both route- and gateway-targeting policy paths). It does not enable session persistence on its own.Testing
/v4/foo/barwith apath: /override produces an Envoy cookie withPath=/.backendtrafficpolicy-with-session-persistence-cookie-path— HTTPRoute rule on/foo/bar+ BTPsessionPersistence.cookie.path: /yields IR cookiepath: /; policy isAccepted.go build ./...,go vet,gofmt, and the translator/gatewayapi/ir test suites pass.Release Notes
Added a release-note fragment under
release-notes/current/new_features/.