add hub_url_local as hub_url currently represents both public and local #994
Conversation
manics
changed the title
manics
changed the title
|
Based on #1101 this problem is not unique to my test cluster! |
| @@ -84,7 +84,8 @@ def get_value(key, default=None): | |||
| c.BinderHub.build_namespace = os.environ['BUILD_NAMESPACE'] | |||
|
|
|||
| if c.BinderHub.auth_enabled: | |||
| hub_url = urlparse(c.BinderHub.hub_url) | |||
| hub_url = urlparse(c.BinderHub.hub_url_public if 'hub_url_public' in c.BinderHub | |||
| else c.BinderHub.hub_url) | |||
There was a problem hiding this comment.
Perhaps we can make hub_url_public have a @default decorated function returning self.hub_url? I think this logic is too messy to use whenever we want to use hub_url_public, so we don't have to duplicate logic about this.
|
@manics I want to resurrect my work to make BinderHub support automatic HTTPS acquisition using z2jh, and I'll need this if I recall correctly. If you resolve the merge conflicts I'll review this and help it get merged. |
|
I think a key point of review for this PR regards if we want to assume hub_url is a local URL and add a hub_url_public option, or assume it is a public url, and add a hub_url_local option. From what I see in #1139 and https://binderhub.readthedocs.io/en/latest/zero-to-binderhub/setup-binderhub.html#connect-binderhub-and-jupyterhub, I think perhaps it is better to add a hub_url_local and assume hub_url to be a public path. What do you think? @bitnik perhaps you have some input about this? |
|
@consideRatio I'm glad it's useful for more than just me 😄 . I'll resolve the conflicts and try your default suggestion. I don't have any preference on whether to add |
|
Lets go with Thank you for your work on this @manics ❤️ |
I dont have anything to add on, and I agree with @consideRatio |
consideRatio
changed the title
manics
force-pushed
the
hub_url_public
branch
from
a34bed6 to
3d10c9f
Compare
|
I've rebased and updated the PR to use |
There was a problem hiding this comment.
Excellent! I have verified this matches my old implementation as well.
For reference, my motivation was described something like this innotes I found.
hub_url_local introduced to solve an issue:
hub_url is used in launcher.py, in there it needs to be a public url because it will send that to the BinderHub visitor that is to get an URL for the JupyterHub session. It also needs to be an URL reachable from the binderhub itself wherever it is running. This is not always possible to accomplish, for example when setting up a autohttps test environment against a dummy ACME server.
So, this PR solves a problem for my CI testing an upcoming pr about automatic HTTPS cert acquisition.
|
I added one more commit. I've only partially tested this because k8s broke for unrelated reasons, but I think the PR is ready for review, especially since you already understand what's going on. |
| @@ -98,7 +98,7 @@ spec: | |||
| key: "binder.hub-token" | |||
| {{- if .Values.config.BinderHub.auth_enabled }} | |||
| - name: JUPYTERHUB_API_URL | |||
| value: {{ (print (.Values.config.BinderHub.hub_url | trimSuffix "/") "/hub/api/") }} | |||
| value: {{ (print (.Values.config.BinderHub.hub_url_local | default .Values.config.BinderHub.hub_url | trimSuffix "/") "/hub/api/") }} | |||
There was a problem hiding this comment.
Nice catch adding this as well!
There was a problem hiding this comment.
If auth_enabled, this URL is required to be mounted as an env var? Hmmm? Is this because the JupyterHub imported HubOAuth classes looks for this URL or similar?
Ah yes it seems so, this could been configured with c.HubAuth.api_url = c.BinderHub.hub_url_local someplace instead, because JUPYTERHUB_API_URL serves the purpose of a default value for the c.HubAuth.api_url traitlet.
It seems like...
- JUPYTERHUB_API_URL can be configured with c.HubAuth.api_url
- JUPYTERHUB_BASE_URL can be configured with c.HubAuth.hub_prefix
- JUPYTERHUB_CLIENT_ID can be configured with c.HubOAuth.oauth_client_id
- JUPYTERHUB_OAUTH_CALLBACK_URL can be configured with c.HubOAuth.oauth_redirect_uri
Well, okay, let's configure them through environment variables.
There was a problem hiding this comment.
@consideRatio I was confused by the apparent duplication between config and env-vars too. I only discovered the problem when deploying it to a test system.
I don't know if this is useful so opening it for comments.
I just ran Binderhub on a misconfigured test system where external ingress is limited by IP range. Unfortunately the IP range didn't include itself 😀
BinderHub uses the external JupyterHub API instead of the internal K8s service. Obviously the firewall will be fixed, but is this potentially useful anyway?