Skip to content
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

GEP 1619: distinguish 'session' cookies. #2747

Open
costinm opened this issue Jan 29, 2024 · 5 comments
Open

GEP 1619: distinguish 'session' cookies. #2747

costinm opened this issue Jan 29, 2024 · 5 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@costinm
Copy link

costinm commented Jan 29, 2024

The current spec defines expiration for the cookie - and mentions the Expiry attribute of the cookie ( which seems to suggest that the expiration will be based on Expiry header or have an expiration based on the server settings ).

https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies defiles 'session cookies' as cookies without an Expires or max-age -

https://gdpr.eu/cookies/ also defines "session cookies" as 'expire when closing the browser or session ends" and has different requirements ("While it is not required to obtain consent for these cookies, what they do and why they are necessary should be explained to the user.")

What would you like to be added:

We should add an explicit field to indicate 'session cookies' - they may still have a shorter expiration, but will be based on the content of the cookie and not the "Expiry" attribute.

Alternative: remove the references to "Expiry" cookie attribute and document that only 'session cookies' are supported. For the use cases discussed ( load balancer stickiness), very long persistent cookies do not help anyways - they are used for long-term user login and tracking.

Why this is needed:

To avoid ambiguity and confusion between the 2 kinds of cookies.

@costinm costinm added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 29, 2024
@costinm
Copy link
Author

costinm commented Feb 1, 2024

Another use case for session cookies from a customer: they need the behavior of 'when browser is closed, session is gone'.
For Istio I am planning to default to session cookies - most non-browser clients are unlikely to support the correct semantics for 'permanent cookies' and MaxAge in particular for pods without persistent storage.
I would wait for other implementations to figure out how the expiration in the specs can be implemented properly and how it interacts with the new expiration that envoy implements.

@gcs278
Copy link
Contributor

gcs278 commented Feb 5, 2024

Thanks @costinm. This is something we will document in GEP-1619.

Just to recap the problem as I understand it, the current GEP calls the Expires / Max-Age attribute in TTL section (which I'm currently working on fixing since TTL isn't an actual attribute...). The goal described by this section is that new AbsoluteTimeoutSeconds field would be generic enough to be used by implementations that set the Expires / Max-Age attributes (such as Istio/Envoy with TTL) AND implementations that track session lifetime via the cookie value (such as HaProxy with maxlife).

The intended dual use of this AbsoluteTimeoutSeconds is problematic because some implementations will create session cookies while others (i.e. Envoy) will create a persistent cookie (via Max-Age attribute). This is a API portability problem.

This is where designing the API gets tricky for me. Should we try to solve both cases (persistent and session cookies) in our first iteration? It seems like session cookies are the lowest common denominator here. I lean toward the simpler approach by documenting that only session cookies are supported (your alternative). Envoy now has the logic that encodes an expiration in the cookie value via envoyproxy/envoy#26761, so implementing AbsoluteTimeoutSeconds for a session cookie is partially done for Envoy (just need to not set Max-Age...).

I think we should keep it simple in the first API iteration, then if an implementation/users ask for the ability to configure persistent cookies with good reasoning, add a cookie-type (persistent vs. session) in the next iteration of the API. I'm trying to avoid adding API knobs that may not ever be sufficiently used. But I invite folks to speak up if you know this would be problematic for your implementation or use case.

@shaneutt do you concur with my API design philosophy here?

@costinm
Copy link
Author

costinm commented Feb 5, 2024

For persistent cookies - if we really must support them - it may be good to define the use cases and behavior.

The intended use is to provide stickiness to a backend. What happens if I set it to 1 day but the backend is restarted 1 hour later ? Or set it to 1 month ? Few backends don't get updated in a month.

And while browsers have storage and can persist the cookie for a month - non-browser clients like pods usually can't. Are we adding a requirement on clients to persist - or just ignoring the persistent and max age semantics - and if that is the case, what exactly do we expect the semantics to be ?

I think not having any user set expiration is the best option - making it clear that the cookie should be usable as long as the backend it points to has not been restarted, which is really what most users want.

@gcs278
Copy link
Contributor

gcs278 commented Feb 6, 2024

I think not having any user set expiration is the best option - making it clear that the cookie should be usable as long as the backend it points to has not been restarted, which is really what most users want.

Hmm. I do appreciate a simpler approach. I agree we need use cases for persistent cookies and/or for session cookies with timeout/expiration. I think it's best to start with the bare minimum in the first iteration of the API and try to grow the API organically with use cases from users actually using the API. I'm planning to remove references to timeouts right now to keep things simple.

I'll try to address this in #2634 or a future PR.

@costinm
Copy link
Author

costinm commented Feb 6, 2024

Thanks - I opened a PR in istio to only use session cookies as well, will update if there is any pushback.

gcs278 added a commit to gcs278/gateway-api that referenced this issue Feb 15, 2024
Fix confusing "TTL" section to reflect the Expires / Max-Age cookie
attribute. Specify language that cookies MUST be session cookies and
move timeout APIs to alternatives section. Addresses
github.com/kubernetes-sigs/issues/2747.
gcs278 added a commit to gcs278/gateway-api that referenced this issue Feb 22, 2024
Fix confusing "TTL" section to reflect the Expires / Max-Age cookie
attribute. Specify language that cookies MUST be session cookies and
move timeout APIs to alternatives section. Addresses
github.com/kubernetes-sigs/issues/2747.
gcs278 added a commit to gcs278/gateway-api that referenced this issue Feb 28, 2024
Fix confusing "TTL" section to reflect the Expires / Max-Age cookie
attribute. Specify language that cookies MUST be session cookies and
move timeout APIs to alternatives section. Addresses
github.com/kubernetes-sigs/issues/2747.
gcs278 added a commit to gcs278/gateway-api that referenced this issue Feb 29, 2024
Fix confusing "TTL" section to reflect the Expires / Max-Age cookie
attribute. Addresses
github.com/kubernetes-sigs/issues/2747.
gcs278 added a commit to gcs278/gateway-api that referenced this issue Feb 29, 2024
Fix confusing "TTL" section to reflect the Expires / Max-Age cookie
attribute. Add `LifetimeType` API field to specify lifetime type of
cookie.

Addresses github.com/kubernetes-sigs/issues/2747.
gcs278 added a commit to gcs278/gateway-api that referenced this issue Feb 29, 2024
Fix confusing "TTL" section to reflect the Expires / Max-Age cookie
attribute. Add `LifetimeType` API field to specify between session and
persistent cookies.

Addresses github.com/kubernetes-sigs/issues/2747.
gcs278 added a commit to gcs278/gateway-api that referenced this issue Feb 29, 2024
Fix confusing "TTL" section to reflect the Expires / Max-Age cookie
attribute. Add `LifetimeType` API field to specify between session and
persistent cookies.

Addresses github.com/kubernetes-sigs/issues/2747.
gcs278 added a commit to gcs278/gateway-api that referenced this issue Mar 5, 2024
Fix confusing "TTL" section to reflect the Expires / Max-Age cookie
attribute. Add `LifetimeType` API field to specify between session and
persistent cookies.

Addresses github.com/kubernetes-sigs/issues/2747.
gcs278 added a commit to gcs278/gateway-api that referenced this issue Mar 7, 2024
Fix confusing "TTL" section to reflect the Expires / Max-Age cookie
attribute. Add `LifetimeType` API field to specify between session and
persistent cookies.

Fixes kubernetes-sigs#2747
gcs278 added a commit to gcs278/gateway-api that referenced this issue Mar 7, 2024
Fix confusing "TTL" section to reflect the Expires / Max-Age cookie
attribute. Add `LifetimeType` API field to specify between session and
persistent cookies.

Fixes kubernetes-sigs#2747
gcs278 added a commit to gcs278/gateway-api that referenced this issue Mar 11, 2024
Fix confusing "TTL" section to reflect the Expires / Max-Age cookie
attribute. Add `LifetimeType` API field to specify between session and
persistent cookies.

Relates to kubernetes-sigs#2747
k8s-ci-robot pushed a commit that referenced this issue Mar 15, 2024
* GEP-1619: Fix links, formatting, metadata.yaml

Make links relative when they can be and fix missing whitespace which
caused issue with markdown rendering. Fix up metadata.yaml.

* GEP-1619: Add new implementations

Add new implementations to implementations table with their session
persistence API details.

* GEP-1619: Answer question on forms of Session Persistence

Answer "Should we leave room for configuring different forms of Session
Persistence?" open question by adding a new API field `Type` to allow
users to specify the type of session persistence, as well as adding a
paragraph in API Granularity section for further discussion.

Also add graduation criteria for standard channel graduation regarding mesh.

* GEP-1619: Distinguish between session vs. permanent cookies

Fix confusing "TTL" section to reflect the Expires / Max-Age cookie
attribute. Add `LifetimeType` API field to specify between session and
persistent cookies.

Relates to #2747

* GEP-1619: Update timeouts to use Duration

Update AbsoluteTimeoutSeconds and IdleTimeoutSeconds to use the standard
Duration field from GEP-2257. Rename them to AbsoluteTimeout and
IdleTimeout to reflect this change.

* GEP-1619: Adopt RFC8174 language

RFC8174 defines words such MUST, SHOULD, MAY need to be in all caps in
order to convey a specific meaning.

* GEP-1619: Answer open question about unhealthy backends

Add details about how to handle failure case behavior and fix up TODO
and Open Questions sections

* GEP-1619: Refactor SessionPersistencePolicy into BackendLBPolicy

Approach now uses more generic BackendLBPolicy which supports only
attaching to Service. Add API for adding session persistence
configuration inline to HTTPRoute and GRPCRoute rule.

Add new edge cases for naming collision and two route rules sharing
persistence.

* GEP-1619: Relax traffic splitting edge case

Relax the session persistence traffic splitting edge case guidelines
to allow implementations flexibility for scenarios within the same route
rule.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

No branches or pull requests

2 participants