kubespawner icon indicating copy to clipboard operation
kubespawner copied to clipboard

Support {server_base_url} variable substitution

Open yuvipanda opened this issue 3 years ago • 13 comments

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.

yuvipanda avatar Feb 04 '22 10:02 yuvipanda

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

yuvipanda avatar Feb 04 '22 11:02 yuvipanda

Hmmmm... Sometimes you can mount k8s metadata as an env, for example, maybe about local IP address. Maybe that would be relevant?

consideRatio avatar Feb 04 '22 12:02 consideRatio

@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.

yuvipanda avatar Feb 04 '22 12:02 yuvipanda

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.

minrk avatar Feb 04 '22 13:02 minrk

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 avatar Feb 04 '22 13:02 minrk

@minrk aha, that's what I actually want! I pushed in a change using that.

yuvipanda avatar Feb 04 '22 14:02 yuvipanda

@minrk do you know if these test failures are mock failures or real failures?

yuvipanda avatar Feb 07 '22 07:02 yuvipanda

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 avatar Mar 10 '22 14:03 minrk

@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?

yuvipanda avatar Mar 10 '22 19:03 yuvipanda

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.

consideRatio avatar Mar 10 '22 20:03 consideRatio

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.

yuvipanda avatar Mar 10 '22 20:03 yuvipanda

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 avatar Mar 10 '22 20:03 manics

@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.

consideRatio avatar Mar 10 '22 20:03 consideRatio

I'm going to let this one go! Thanks for the comments everyone.

yuvipanda avatar Oct 18 '22 03:10 yuvipanda