-
Notifications
You must be signed in to change notification settings - Fork 664
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.
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
base: main
Are you sure you want to change the base?
Add session persistence conformance tests #4482
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: salonichf5 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @salonichf5. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Thanks @salonichf5! /ok-to-test |
|
@robscott: GitHub didn't allow me to request PR reviews from the following users: DamianSawicki. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
geps/gep-1619/index.md
Outdated
|
|
||
| | 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, HTTPRouteSessionPersistenceCookiePermanentLifetime | | ||
|
|
||
| | 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) Request to /a establishes session to Pod-X, 2) Request to /b may route to any pod (Pod-X or Pod-Y), 3) Subsequent requests to /a with session cookie MUST still route to Pod-X, 4) Subsequent requests to /b with its session cookie MUST route to whichever pod was selected for /b. | HTTPRouteSessionPersistence | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Request to /b may route to any pod (Pod-X or Pod-Y)
To confirm this, we'd also need to send multiple requests. It may be obvious to the author, but perhaps it would be good to spell it out in the GEP for all readers.
Subsequent requests to /b with its session cookie MUST route to whichever pod was selected for /b
Well, with multiple requests it will be multiple pods.
DamianSawicki
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more thoughts.
| | :---- | :---- | :---- | | ||
| | 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 | |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| @@ -1077,6 +1077,29 @@ 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? | |||
|
|
|||
There was a problem hiding this comment.
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.
| ### Feature Names | ||
|
|
||
| * HTTPRouteSessionPersistence - Base feature for session persistence support | ||
| * HTTPRouteSessionPersistenceCookie - Cookie-based session persistence (Core) |
There was a problem hiding this comment.
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 |
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.
|
|
||
| ## Conformance Details | ||
|
|
||
| ### Feature Names |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
|
@DamianSawicki some things might feel unclear but we are also waiting on @gcs278 update of the GEP as per this issue I have made a note that we need:
@gcs278 I think in your update of the GEP you could tackle point 3. and 4. , if that is okay? |
cb7e0a3 to
007d458
Compare
What type of PR is this?
/kind gep
/kind test
Optionally add one or more of the following kinds if applicable:
/area conformance-test
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #4258
Does this PR introduce a user-facing change?:
NONE