search

jupyterhub / kubespawner

Kubernetes spawner for JupyterHub
https://jupyterhub-kubespawner.readthedocs.io
BSD 3-Clause "New" or "Revised" License
543 stars 304 forks source link

Updating KubeIngressProxy to new slug scheme (or not) #849

Open minrk opened 1 month ago

minrk commented 1 month ago

Proposed change

I'm actually proposing no change here, but writing it down so it can be discusses or referred to or implemented later.

@consideRatio pointed out to me that #744 did not touch the KubeIngressProxy, which also computes slugs and might have similar issues.

I've been looking into it, and given the test/support state of KubeIngressProxy, I think we shouldn't update it with the new slug scheme. At least not block a release on doing so until we hear about problems.

Background

KubeIngressProxy has an equivalent implementation of _expand_user_properties, namespace, and other configuration to KubeSpawner, but these implementations are not shared. As a result, equivalent templates may render differently in the two environments, and the validity guarantees added in #744 are not applied to the proxy.

However, the main motivating situation for #744 (especially making it the default) is that common_labels on KubeSpawner includes a derived value that could often be illegal with certain user or server names. KubeIngressProxy does not have this problem because there are no templated values in common_labels (they may be added in extra_labels, but there is little reason to do so).

And the name field already uses a safe scheme that is not configurable, derived only from the routespec, not the user or server (not all routes are associated with a user, e.g. services or the hub itself).

The second reason I don't think the proxy needs an update is that the changed labels in KubeSpawner do not affect the ingress proxy. This is where I thought the proxy might be broken by #744. The only real interface between the Spawner and Proxy is the URL returned by KubeSpawner.start, so changing how labels and names are calculated in KubeSpawner doesn't affect anything in the proxy.

Alternative options

Update KubeIngressProxy to use the new slug scheme.

One of the tricky bits is that the Spawner is not available to add_route. I do think I have a way to make it available, so that _expand_user_properties could call the Spawner method, significantly reducing the duplicate configurable code. I think some amount of namespace logic perhaps should also be shared there, I'm not sure. That gets tricky with the fact that not all routes are for Spawners. I don't feel comfortable making this change with the current state of KubeIngressProxy at the moment, since it doesn't really appear to solve a problem I'm aware of, unlike on the Spawner. I'd rather err on the side of no change than maybe break things without actually fixing anything.

Who would use this feature?

KubeIngressProxy users.

consideRatio commented 1 month ago

Thank you for thinking about this and proposing what to do, let's go with proceeding with a release and not doing anything until we have input related to this.