zero-to-jupyterhub-k8s icon indicating copy to clipboard operation
zero-to-jupyterhub-k8s copied to clipboard

Allow to disable proxy pod and service creation

Open dolfinus opened this issue 3 years ago • 3 comments
trafficstars

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

dolfinus avatar Oct 14 '22 14:10 dolfinus

Thanks for submitting your first pull request! You are awesome! :hugs:
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! :wave:
Welcome to the Jupyter community! :tada:

welcome[bot] avatar Oct 14 '22 14:10 welcome[bot]

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.

consideRatio avatar Oct 17 '22 06:10 consideRatio

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.

manics avatar Oct 17 '22 14:10 manics