kubespawner
kubespawner copied to clipboard
Support {server_base_url} variable substitution
Very helpful when you need to tell some sidecars containers, or even set some specific environment variable, where the full URL of the server itself must be known.
Well this doesn't work of course, because you might not know the server_url before starting... I think me and @minrk chatted about a related issue as well
Hmmmm... Sometimes you can mount k8s metadata as an env, for example, maybe about local IP address. Maybe that would be relevant?
@consideRatio this is when you need the URL the browser will be sending requests to, so internal IP won't work. https://github.com/2i2c-org/infrastructure/pull/978/files#diff-feb65d110fb361bde1d19fd758dcf1d3961c5c11e9607723acd154f8ee83212cR525 is the motivating example here.
Do you mean the public URL, or the internal URL? self.server.url is where the internal URL is stored (what proxy, hub connect to). You're right that that db record won't be populated until some time after start returns.
The public URL is (partly) knowable, and is accessible as user.server_url(server_name). If you aren't using subdomains, this will only be the path (/user/name[/server_name]), you'll have to get the proto://host from another source (not usually part of JupyterHub config, since it doesn't know or need to know what proxies, etc. might be involved in the actual public endpoint).
The internal connect URL may also be knowable, though it is not by default (e.g. using the pod name via DNS instead of the not-yet-assigned pod ip). If you specify get_pod_url as a function that doesn't take the pod into account, e.g.
# this should work by default, but is not the default behavior
def get_pod_url(spawner, pod):
return f"http://{spawner.dns_name}:{spawner.port}{spawner.user.server_url(spawner.name)}"
c.KubeSpawner.get_pod_url = get_pod_url
But because we can use the pod itself (and do by default by using pod.status.podIP) to derive this URL in general, we can't assume _get_pod_url() can be called before the pod exists.
Ah, If you just mean the server URL prefix (i.e. /user/username[/servername]/) that's accessible as self.server.base_url and is known ahead of time. I think it makes sense to have this as {base_url} in the template namespace.
@minrk aha, that's what I actually want! I pushed in a change using that.
@minrk do you know if these test failures are mock failures or real failures?
I don't feel strongly about base_url vs server_base_url! Just what I would have chosen if folks happen to agree. Also AOK to stick with it as-is. For context, the env variable storing the same value is called $JUPYTERHUB_SERVICE_PREFIX, and the single-user server property is called .base_url.
@minrk I want to possibly support hub_base_url as well in the future, which is why I specifically said server_base_url here.
@consideRatio what kinda test would you like for this? I see we already have https://github.com/jupyterhub/kubespawner/blob/88fd98125052612f177ee24d003270ce8429dd5e/tests/test_spawner.py#L584 which tests the process of variable expansion, but doesn't test individual expansion variables. Should I add to that?
Thanks for following up on this @yuvipanda!
Regarding tests, I mainly looked to see that the existing tests went green. I haven't looked into the failures more than conclude that it seemed related to the changes made in the PR. If you want to add a dedicated test for this or add some aspect to a test go for it but I didn't think of it as required for a merge.
oh lol I didn't actually notice these tests were failing! Sorry I lost context. I'm just going to convert this to draft, as I don't have time to look into this right now.
The tests are failing on main too https://github.com/jupyterhub/kubespawner/actions?query=branch%3Amain (as well as other PRs such as https://github.com/jupyterhub/kubespawner/pull/572) so something's broken somewhere.
@manics the main branch failures are related to https://github.com/jupyterhub/kubespawner/issues/582 and the new release of JupyterHub 2.2.0 that introduced a change that breaks tests in KubeSpawner.
There were also a few failures that has been resolved by a quickly fixed regression in kubernetes_asyncio.
This PR though has more tests failing than those on the main branch, directly related to accessing self.server.base_url when self.server was returning None.
I'm going to let this one go! Thanks for the comments everyone.