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

Allow to disable proxy pod and service creation #2904

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dolfinus
Copy link

@dolfinus dolfinus commented Oct 14, 2022

User may set up kubespawner.proxy.KubeIngressProxy which allows to use builtin k8s Ingress to create routes for hub, user notebooks and services. But this helm chart will always create proxy pod and service, which are useless in this case.

I've added proxy.enabled option allowing user to disable proxy service and pod creation. I haven't wrap all the items in proxy folder with this option, don't think that this will be useful

@welcome
Copy link

welcome bot commented Oct 14, 2022

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@consideRatio
Copy link
Member

consideRatio commented Oct 17, 2022

Thank you for your thorough work across the JupyterHub ecosystem @dolfinus! I think its reasonable to support an option to opt out of this functionality, allowing you to use the

A disclaimer: an advanced / low support option

We wrote a disclaimer in KubeIngressProxy to help manage expectations, and similarly I think it would be relevant for this Helm chart to clearly convey that this option is an advanced one, and users disabling proxy parts will need to mange the consequences themselves as it currently is out of scope to support other kinds of proxy configurations besides avoiding conflict with the default option.

A clarification: proxy pod / autohttps pod

If proxy.https.enabled and proxy.https.type=letsencrypt then there will be a autohttps pod and service as well. I believe disabling proxy should disable both at the same time and be documented to do so. I think practically this PR should be reviewed by verifying that all resources in the proxy folder are considered to be disabled by this flag, and then documented to be disabled by this flag.

Some basic guidance with links

While it would be an advanced option with little support, I think a single paragraph linking out to something is relevant. I think this documentation about JupyterHub proxy_class implementations could be relenvant.

A text proposal

```warning
This advanced option can disable essetial parts of the Helm chart, so you are required to understand how to replace those parts.
```

By default this Helm chart configures and starts a [configurable-http-proxy](https://github.com/jupyterhub/configurable-http-proxy) server to run in the `proxy` pod, and for JupyterHub to control it (`add_route`, `delete_route`, etc) with the [`JupyterHub.proxy_class`](https://jupyterhub.readthedocs.io/en/stable/api/proxy.html) called [ConfigurableHTTPProxy](https://github.com/jupyterhub/jupyterhub/blob/3.0.0/jupyterhub/proxy.py#L491).

By setting this option to false, you will need to configure `JupyterHub.proxy_class` yourself and ensure it is able to indirectly or directly control a proxy server managed by yourself. All resources templated in [templates/proxy](https://github.com/jupyterhub/zero-to-jupyterhub-k8s/tree/main/jupyterhub/templates/proxy) will be disabled, including the subfolder for `autohttps`.

This option was initially provided to facilitate use of [KubeIngressProxy](https://github.com/jupyterhub/kubespawner/blob/4.2.0/kubespawner/proxy.py#L43-L127), a JupyterHub Proxy class that speaks with the k8s api-server to create and delete k8s Ingress resources as a means of controlling traffic via a k8s cluster's Ingress controller.

@manics
Copy link
Member

manics commented Oct 17, 2022

If proxy.https.enabled and proxy.https.type=letsencrypt then there will be a autohttps pod and service as well. I believe disabling proxy should disable both at the same time and be documented to do so.

I think this makes sense. If we're adding supporting for unsupported configurations we should make them fairly high level, so that it's less likely that someone runs into an invalid configuration.

Probably the biggest thing missing from KubeIngressProxy in the KubeSpawner repo is some tests.

@dolfinus dolfinus changed the title Allow to disable proxy pod and service creation Draft: Allow to disable proxy pod and service creation Oct 22, 2022
@consideRatio consideRatio changed the title Draft: Allow to disable proxy pod and service creation Allow to disable proxy pod and service creation Oct 31, 2022
@consideRatio consideRatio marked this pull request as draft October 31, 2022 20:33
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.

None yet

3 participants