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

Record and pass stickiness cookies for load balancers #967

Closed
wjsi opened this issue Sep 8, 2022 · 6 comments
Closed

Record and pass stickiness cookies for load balancers #967

wjsi opened this issue Sep 8, 2022 · 6 comments

Comments

@wjsi
Copy link
Contributor

wjsi commented Sep 8, 2022

Problem

Jupyter enterprise gateway now relies on Service.spec.sessionAffinity to make sure same backend pod is visited, which relies on client IP address to distribute pod access. However, when the environment becomes more complicated, for instance, service pods are hidden behind multiple layers of gateways or proxies, it is often not possible to locate the original client IP. Therefore kernels created on one pod will be lost when the load balancer chooses another pod when HA is not available, or a number of gateway pods are connected to the same kernel.

Proposed Solution

To resolve this issue, cloud providers like AWS provides a mechanism relying on client cookies to identify which server the client actually visits on their load balancers. When requesting with certain cookies, the load balancer will use the server used previously to hold the request.

To utilize this functionality, we need to record certain cookies (as well as expiration time) in GatewayClient when receiving responses from enterprise gateways, and add a Cookie header when doing gateway_request.

I'm now working on this in my own fork and would like to submit a PR for this.

@welcome
Copy link

welcome bot commented Sep 8, 2022

Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please try to follow the issue template as it helps other other community members to contribute more effectively.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also an intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@kevin-bates
Copy link
Member

Hi @wjsi - thank you for opening this issue.

It turns out that I am correctly working on a PR that will enable the ability to plugin your own "token renewer class" to allow for expiration tokens. In this PR, all aspects of the authorization header are configurable, and I would like to invite you to take a look at my local PR: kevin-bates#13. (I'm waiting for my colleagues to ensure the API satisfies their requirements, which is why I haven't submitted it to jupyter_server.)

I'm hoping this approach would address your needs as well, but, if not, perhaps we can work together to arrive at a solution.

@wjsi
Copy link
Contributor Author

wjsi commented Sep 9, 2022

@kevin-bates thanks for the quick reply. After reviewing your code I find it does not work for my case as the change is not in Authorization header.

Current implementation of enterprise gateway requires clients to stick to one gateway node, and in k8s deployment this is achieved by using Service.spec.serviceAffinity. When gateways are deployed behind a load balancer, we need another mechanism to make sure this stickiness work. For instance, AWS provides duration-based stickiness. The process is shown below (I have not inspected into application-based stickiness yet):

Stickiness Cookie

For the initial request, the client received a server identifier with some cookies. The load balancer decides subsequent nodes by the cookies sent by the client. Therefore GatewayClient need to store and insert cookies for next queries.

I've written some code to show the changes can be done: main...wjsi:jupyter_server:enh/stickiness_cookie .

@kevin-bates
Copy link
Member

Hi @wjsi - thank you for your detailed response. I agree that your need is more about cookie management in general and does not warrant the need for 3rd-party plugin functionality.

Since this seems generally useful, I suggest you submit a pull request relative to your changes. I did have a couple of questions regarding those changes that might lead to some changes prior to the PR's creation.

  1. If we'd be managing cookie expirations in general when use_stickiness_cookie is enabled but stickiness_cookie_name is not set, could we forgo the need for stickiness_cookie_name altogether and drive this by a single boolean, something like manage_cookie_expiration? It seems that stickiness_cookie_name is only used to produce the single informational message, but, when set, it becomes the ONLY cookie managed. As a result, its presence changes the behavior and it seems better to manage all expirations or just the one. If we decide to use just the one, then we could trigger this logic when stickiness_cookie_name is set and remove the boolean.
  2. Naming. I understand the topic is session stickiness but I think we should change stickiness to sticky in the two configurables (depending on what we deem necessary per item 1).

We look forward to your pull request. Thank you for your contribution.

@wjsi
Copy link
Contributor Author

wjsi commented Sep 10, 2022

@kevin-bates thanks for the comments. In the pull request I merged two options into accept_cookies as for the client side, we do not have any hints to decide whether the cookie is for stickiness or not. Also made expiring a default behavior.

@wjsi
Copy link
Contributor Author

wjsi commented Sep 14, 2022

Fixed in #969.

@wjsi wjsi closed this as completed Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants