Skip to content

Add session persistence conformance tests #4482

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
salonichf5 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add session persistence conformance tests #4482

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a0143d
add session persistence conformance tests
salonichf5 Feb 3, 2026
007d458
fix github pages and remove TODO
salonichf5 Feb 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions geps/gep-1619/index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1064,8 +1064,6 @@ supported by some implementations due to their current designs.

### Conformance Details
Copy link
Contributor

@gcs278 gcs278 Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this section. I think the template has changed a couple times, and what you have below is fine.

Suggested change
### Conformance Details


TODO

### Open Questions

- What happens when session persistence causes traffic splitting scenarios to overload a backend?
Expand All @@ -1077,6 +1075,24 @@ TODO
- How do we provide a standard way to communicate that an implementation does not support Route Rule API?
- Do we want something conceptually similar to the `IncompatibleFilters` reason?

Copy link
Contributor

@DamianSawicki DamianSawicki Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not directly relevant to the changes in this PR, but before graduating the feature to Standard it would be good to have clarity about field names. I see sessionPersistence.sessionName and sessionPersistence.name used interchangeably in the GEP.

In sessionPersistence.sessionName we are repeating "session", which is generally discouraged. It also makes a misleading impression that this feature is about establishing some "session".

To be in line with the comment in the code snippet, it should ideally be sessionPersistence.tokenName, but that's probably overly technical, so perhaps sessionPersistence.name is close enough.

Copy link
Contributor

@gcs278 gcs278 Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DamianSawicki I agree with your points. I think name would work better. I can take this as an action when I update the GEP for #4268.

## Conformance Details

### Feature Names
Copy link
Contributor

@DamianSawicki DamianSawicki Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the support in GRPCRoute and BackendLBPolicy? Is the intention here to prepare conformance tests for the HTTPRoute part and graduate it selectively to Standard, while keeping the rest in Experimental?

Copy link
Contributor Author

@salonichf5 salonichf5 Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's been some discussion around BackendLBPolicy to be retired so its been skipped on purpose. I need to add these for GRPR too, good catch
to just get started initial recommendation was this

Copy link
Contributor

@gcs278 gcs278 Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with delaying BackendTrafficPolicy, as that is being re-considered. For GRPCRoute, you could add it here in this PR if you feel it's simple/similar enough, however I fine to add it in a follow-up PR.


* HTTPRouteSessionPersistence - Base feature for session persistence support
* HTTPRouteSessionPersistenceCookie - Cookie-based session persistence (Core)
Copy link
Contributor

@DamianSawicki DamianSawicki Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is meant here by (Core)? As per

gateway-api/geps/gep-1619/index.md

Lines 619 to 622 in d8bb5d9

// SessionPersistence defines and configures session persistence
// for the route rule.
//
// Support: Extended
Session Persistence is in Extended rather than Core support on HTTPRoute, and similarly for GRPCRoute and BackendLBPolicy. As per https://gateway-api.sigs.k8s.io/concepts/conformance/#overlapping-support-levels, the entire feature should be in Extended support.

Copy link
Contributor

@gcs278 gcs278 Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right - you should actually remove HTTPRouteSessionPersistenceCookie altogether. The base/generic feature HTTPRouteSessionPersistence is the core functionality of the extended feature, which includes Cookie persistence, so to be consistent with other features, we should keep

HTTPRoute (Core)
│
└─ HTTPRouteSessionPersistence (Extended)
      │   ├── Type: Cookie (core for HTTPRouteSessionPersistence)
      │   └── LifetimeType: Session (core for HTTPRouteSessionPersistence)
      │
      └── HTTPRouteSessionPersistenceCookieLifetimeTypePermanent (Extended)
          └── LifetimeType: Permanent (Extended)

* HTTPRouteSessionPersistenceCookieLifetimeTypePermanent - Permanent Lifetime for cookie-based session persistence (extended)

Copy link
Contributor

@gcs278 gcs278 Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably drop a note that this is still WIP, so folks aren't confused why it's not complete.

Suggested change
**Note**: Conformance tests are still WIP, and additional test coverage will be added in future PRs.

### Conformance tests

| Description | Outcome | Features |
| :---- | :---- | :---- |
| Simple Cookie Session Persistence: An HTTPRoute with sessionPersistence configured with type: Cookie (default) on a single backend in gateway-conformance-infra namespace. | HTTPRoute MUST have Accepted=True in parent status. First request MUST receive a Set-Cookie header in response. Subsequent requests with the cookie MUST route to the same backend pod. Verify by checking pod identity across N requests (N>=50). | HTTPRouteSessionPersistence, HTTPRouteSessionPersistenceCookie |
| Session Cookie Lifetime (Default): HTTPRoute with sessionPersistence and cookieConfig.lifetimeType: Session (default). | HTTPRoute MUST have Accepted=True in parent status. Cookie MUST NOT contain `expiry` attributes. | HTTPRouteSessionPersistence, HTTPRouteSessionPersistenceCookie |
Copy link
Contributor

@DamianSawicki DamianSawicki Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the case of two paths in one rule?

kind: HTTPRoute
metadata:
  name: routeX
spec:
  rules:
  - matches:
    - path:
      value: /a
    - path:
      value: /b
    backendRefs:
    - name: servicev1
    sessionPersistence:
      name: session

should they be shared? I guess it may be technically challenging because there can be only one path specified in Set-Cookie (link). Do we want to specify the behavior here or leave it as implementation specific?

Copy link
Contributor Author

@salonichf5 salonichf5 Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to keep it implementation specific only because how the path is determined for the session could be product specific. For NGINX, we take the longest common prefix of shared sessions for multiple path, and use that. But I would have to do some research for other implementations.

Copy link
Contributor

@gcs278 gcs278 Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, I want to update this in the GEP, it doesn't specify how or if the two paths on the same rule should share. I think it's likely we should prescribe sharing in the GEP, and then we can follow up with a new conformance test. For now, the current tests are fine.

| Multiple Weighted Backends - Initial Distribution Honored: HTTPRoute with sessionPersistence and multiple backendRefs with weights (e.g., 70/30) on a path. | HTTPRoute MUST have Accepted=True in parent status. Initial requests (without session cookie) MUST respect weight distribution (~70/30 within statistical tolerance). Once session is established, subsequent requests with cookie MUST route to the same backend regardless of weight configuration.| HTTPRouteSessionPersistence, HTTPRouteSessionPersistenceCookie |
| Session Persistence with cookieConfig.lifetimeType: Permanent and absoluteTimeout: 5min. | HTTPRoute MUST have Accepted=True in parent status. Response Set-Cookie header MUST contain `expiry` attribute. The expiry value MUST correspond to the configured absoluteTimeout duration. Session persistence MUST function correctly until cookie expires. | HTTPRouteSessionPersistence, HTTPRouteSessionPersistenceCookie, HTTPRouteSessionPersistenceCookieLifetimeTypePermanent |
Copy link
Contributor

@gcs278 gcs278 Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit consider moving this down to the bottom, to have the core tests first, then the extended.

| Session Persistence Scoped to Route Rule: HTTPRoute with two rules: Rule 1 (path /a) and Rule 2 (path /b), both with sessionPersistence configured, routing to the same backend Service with multiple pods. | HTTPRoute MUST have Accepted=True in parent status. Sessions MUST NOT be shared between /a and /b. Verify with: 1) Requests to /a establishes session to Pod-X, 2) Requests to /b may route to any pod (Pod-X or Pod-Y), 3) N Multiple requests to /a with session cookie MUST still route to Pod-X, 4) N Multiple requests to /b with its session cookie MUST route to whichever pod was selected for /b. | HTTPRouteSessionPersistence |

## TODO

The following are items that we intend to resolve in future revisions:
Expand Down