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.

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

Jump to bottom

fix: set clusterKey cookie at first request #143

Merged
merged 2 commits into from
Oct 29, 2024
Merged

fix: set clusterKey cookie at first request #143

merged 2 commits into from
Oct 29, 2024

Conversation

mcollovati
Copy link
Contributor

Description

Currently clusterKey cookie is set only when an HTTP session exists. In addition to the cookie, the key is also set as a session attribute so that SessionSerializer can fetch it later.
However, if two or more concurrent requests are sent from the client, all of them create a new cookie, but the session attribute gets the value of the first one. This causes the distributed session to be written with a wrong key and when there a server switch the session cannot be restored. This change sets the clusterKey cookie even if there is not yet an HTTP session, so the key is only defined during first request and remains stable with subsequent calls; once the HTTP session is create, the key is stored as an attribute.

Potentially fixes #111

See https://github.com/vaadin/kubernetes-kit/issues/111\#issuecomment-2074909790

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

Additional for Feature type of change

  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

@mcollovati
fix: set clusterKey cookie at first request
3aa8377 
Currently clusterKey cookie is set only when an HTTP session exists.
In addition to the cookie, the key is also set as a session attribute
so that SessionSerializer can fetch it later.
However, if two or more concurrent requests are sent from the client,
all of them create a new cookie, but the session attribute gets the value
of the first one. This causes the distributed session to be written with
a wrong key and when there a server switch the session cannot be restored.
This change sets the clusterKey cookie even if there is not yet an HTTP
session, so the key is only defined during first request and remains stable
with subsequent calls; once the HTTP session is create, the key is stored
as an attribute.

Potentially fixes #111

See https://github.com/vaadin/kubernetes-kit/issues/111\#issuecomment-2074909790
@jorgheymans
Copy link

Should the clusterKey cookie name be configurable as well then to avoid similar clashes when a user has sessions to different applications in the context of a portal for example ?

@mcollovati
Copy link
Contributor Author

A configurable cookie name requires a bigger refactoring, since the COOKIE_NAME constant is used in several places (e.g. PushSessionTracker).
I propose not to do it in the context of this PR, and create a new ticket.
But I leave the decision to the Kubernetes Kit Team

@jorgheymans
Copy link

With this fix, failover is now done correctly to another node, great work !

For reference, to easily test failover i'm using this haproxy.cfg:

frontend http-in
   bind *:8080
   mode http
   default_backend servers

backend servers
   mode http
   balance roundrobin
   cookie HA_STICKY insert indirect nocache
   server server1 127.0.0.1:8088 check cookie s1
   server server2 127.0.0.1:8089 check cookie s2

@mcollovati
Copy link
Contributor Author

@jorgheymans Thank you very much for your precious help in debugging this issue

@jorgheymans
Copy link

A configurable cookie name requires a bigger refactoring, since the COOKIE_NAME constant is used in several places (e.g. PushSessionTracker). I propose not to do it in the context of this PR, and create a new ticket. But I leave the decision to the Kubernetes Kit Team

See #145

@jorgheymans
Copy link

Any chance this PR can get some attention for inclusion in a next release ? Eagerly waiting for it 👍

@tamasmak tamasmak merged commit b6e467b into main Oct 29, 2024
2 checks passed
@tamasmak tamasmak deleted the fix/set-clusterket-cookie-at-first-request branch October 29, 2024 15:15
tamasmak added a commit that referenced this pull request Oct 29, 2024
@tamasmak
backport: Set clusterKey cookie at first request (#143)
2e6efae
@tamasmak tamasmak mentioned this pull request Oct 29, 2024
Merged
tamasmak added a commit that referenced this pull request Oct 30, 2024
@tamasmak
backport: Set clusterKey cookie at first request (#143) (#150)
58e743d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tamasmak tamasmak tamasmak approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Vaadin Push fails during session deserialization
3 participants
@mcollovati @jorgheymans @tamasmak