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

Support subdomain_host (CHP needs --host-routing) #3548

Merged
merged 7 commits into from
Oct 24, 2024

Conversation

manics
Copy link
Member

@manics manics commented Oct 19, 2024

The JupyterHub v5 upgrade doc says

All JupyterHub deployments which care about protecting users from each other are encouraged to enable per-user domains, if possible, as this provides the best isolation between user servers.

which sounds like a strong recommendation. This is not currently supported by Z2JH as CHP needs the --host-routing flag. Since CHP is not managed by JupyterHub this isn't automatically configured
https://github.com/jupyterhub/jupyterhub/blob/ab43f6beb8571e2b831801089d61144b15951b85/jupyterhub/proxy.py#L738-L739

https://discourse.jupyter.org/t/user-subdomains-oauth-state-missing/29328/4

The main downsides of using hub.config.subdomain_host as the condition instead of a dedicated parameter such as hub.subdomainHost (analogous to hub.baseUrl qhich also requires configuring multiple chart manifests) are it's not part of the schema so won't appear in the generated chart documentation, and it also adds dependencies across manifests with hub.config which I don't think we have, so this increases the maintenance complexity.

@manics manics added the new label Oct 19, 2024
@minrk
Copy link
Member

minrk commented Oct 21, 2024

it's not part of the schema so won't appear in the generated chart documentation

Can we add a couple of fields to the schema in hub.config that have significant consequences (with additional_properties: true)?

It probably makes sense to have a prose docs section on enabling this and the consequences.

@manics
Copy link
Member Author

manics commented Oct 21, 2024

@manics manics changed the title Draft: Support subdomain_host (CHP needs --host-routing) Support subdomain_host (CHP needs --host-routing) Oct 21, 2024
@manics manics marked this pull request as ready for review October 21, 2024 17:51
@manics
Copy link
Member Author

manics commented Oct 21, 2024

Can you think of an easy test we can add?

@minrk
Copy link
Member

minrk commented Oct 22, 2024

If we just want a really simple test, I think we can start a cluster with subdomains enabled and make some requests to the cluster ip setting the Host header to the expected domain, e.g.

# server_url from hub api will have the user's domain in it when subdomains are enabled
# server_url = "http://user.subdomain:port/user/name"
user_host = urlparse(server_url).netloc
user_path = urlparse(server_url).path
requests.get(f"http://{proxy_ip}:{port}{path}", headers={"Host": user_host})

Or if we can expose a localhost port to the service, we could use *.localhost.jovyan.org like we do in the jupyterhub subdomain tests.

@manics
Copy link
Member Author

manics commented Oct 23, 2024

Thanks for the suggestion- I've added a test using the Host header.

Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

Nice! Looks like it works. Anything more you want to add?

@manics
Copy link
Member Author

manics commented Oct 24, 2024

No, I think this is good enough!

@minrk minrk merged commit 014de3c into jupyterhub:main Oct 24, 2024
14 checks passed
consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Oct 24, 2024
@manics manics deleted the subdomain-host branch October 24, 2024 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants