Include base_url at start of kernelspec resources path

[bloomsa]

Resolves #1111

This ended up being a pretty minimal change. Now when returning kernelspecs from an EG, jupyter-server prepends its own base_url to the path so that any clients requesting those resources hits the proper endpoint on jupyter-server. I did not have to change anything on the gatewayClient side because JUPYTER_GATEWAY_KERNELSPECS_RESOURCE_ENDPOINT is handled already, and the addition of jupyter-server base_url in the resource path doesn't need to be handled because it's already excluded by the gateway resource handler when getting resources

Screenshots

This was taken with Jupyter-lab pointing to my local install of Jupyter-server, which was configured with `--ServerApp.base_url=base`. I also tested the case where no `base_url` was present.

Tests

I didn't modify any test cases, but it doesn't look like any failed. I'm guessing that's because the tests don't have a resources block in the kernelspec. Would you like me to add that as part of this PR?

[codecov]

Codecov Report

Base: 80.04% // Head: 79.99% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (2c8cb2e) compared to base (a55bc58).
Patch coverage: 92.30% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1124      +/-   ##
==========================================
- Coverage   80.04%   79.99%   -0.06%     
==========================================
  Files          68       68              
  Lines        8040     8053      +13     
  Branches     1588     1591       +3     
==========================================
+ Hits         6436     6442       +6     
- Misses       1182     1186       +4     
- Partials      422      425       +3     

Impacted Files	Coverage Δ
jupyter_server/gateway/managers.py	82.97% <92.30%> (+0.29%)	⬆️
...ter_server/services/kernels/connection/channels.py	58.49% <0.00%> (-1.11%)	⬇️
jupyter_server/serverapp.py	79.82% <0.00%> (-0.09%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[kevin-bates]

hi @bloomsa - thanks for the PR! I've gone ahead and queued myself as a reviewer.

At quick glance, I guess I was hoping we could contain these "gateway-related" changes in the gateway directory since the if block you've modified implies the kernelspecs are from a gateway. I'd rather us not "leak" gateway-specific logic outside jupyter_server/gateway. As I noted earlier, the location for these changes would be in GatewayKernelSpecManager and I believe you can access the local base_url value from its self.parent.base_url attribute.

Also, I don't see logic to handle the replacement of a potential base_url that is configured on the Gateway server and included in its kernelspecs responses. I think we need to handle that scenario as well. Thank you.

[bloomsa]

At quick glance, I guess I was hoping we could contain these "gateway-related" changes in the gateway directory since the if block you've modified implies the kernelspecs are from a gateway. I'd rather us not "leak" gateway-specific logic outside jupyter_server/gateway. As I noted earlier, the location for these changes would be in GatewayKernelSpecManager and I believe you can access the local base_url value from its self.parent.base_url attribute.

I totally agree on this point. I initially tried to keep the changes limited to that class but I couldn't figure out how to get the server's base_url from there. I'll go ahead and make that change asap.

Also, I don't see logic to handle the replacement of a potential base_url that is configured on the Gateway server and included in its kernelspecs responses. I think we need to handle that scenario as well. Thank you

With regards to this, I believe that base url would always be part of the JUPYTER_GATEWAY_URL that is configured when the Jupyter-server starts. If it wasn't there or in JUPYTER_GATEWAY_KERNELSPECS_RESOURCE_ENDPOINT, we would have to persist it between requests somehow.
To that point though, these changes to jupyter-server would trickle down to EG right? (I think that's part of what you're implying) so when kernelspecs are requested of EG, they will include the base_url for it, if configured. I will have to strip that out of the kernelspecs response from EG, probably in the same method where I add the jupyter-server's base_url.

Thank you for the quick review and pointing me in the right direction. It's much appreciated :) @kevin-bates

[kevin-bates]

With regards to this, I believe that base url would always be part of the JUPYTER_GATEWAY_URL that is configured when the Jupyter-server starts.

Correct. Using the example below in which my EG was launched using --EnterpriseGatewayApp.base_url=foo/fighters, I would reference it when starting jupyter server using --gateway-url=http://localhost:8889/foo/fighters.

To that point though, these changes to jupyter-server would trickle down to EG right? (I think that's part of what you're implying) so when kernelspecs are requested of EG, they will include the base_url for it, if configured. I will have to strip that out of the kernelspecs response from EG, probably in the same method where I add the jupyter-server's base_url.

Yes. So with foo/fighters as the base url, the kernelspecs returned to jupyter server from EG would have resource entries like this (using the R kernel on K8s as an example):

      "resources": {
        "kernel.js": "foo/fighters/kernelspecs/k8s_r/kernel.js",
        "logo-64x64": "foo/fighters/kernelspecs/k8s_r/logo-64x64.png"
      }

where, if starting jupyter server with --ServerApp.base_url=bar, would require replacement of foo/fighters/kernelspecs with bar/kernelspecs.

Regarding this clause: these changes to jupyter-server would trickle down to EG. Just to clarify, no changes to EG should be required. I suspect you know that and was just stating that the EG resources may also be prefixed with EG's relative base-url.

[bloomsa]

This is ready for re-review. I tried to avoid the nested for loop in the helper method but I couldn't figure out a better way of doing it since the json response object from a Gateway has a nested structure. The code now handle the following situations:

1. jupyter_server has a base_url, the gateway does not - jupyter_server base_url is prepended to the resources paths returned by Gateway
2. jupyter_server does not have a base_url, the gateway also does not - nothing happens in the helper method
3. jupyter_server has a base_url, the gateway does as well - jupyter_server replaces the gateway's base_url with its own. this allows the client to reach the right endpoint on jupyter_server. The gateway base_url gets re-added to the url path when the server builds the resource request for the gateway
4. jupyter_server does not have a base_url, the gateway does - jupyter_server strips the gateway base_url from the resources path, this allows the client to reach the right endpoint on jupyter_server. The gateway base_url gets re-added to the url path when the server builds the resource request for the gateway

screenshots

this is an example of situation 3 listed above

[kevin-bates]

Hi @bloomsa - this is looking good and works well. I just had some comments around the logging and perhaps improving on the test at some point. Thanks.

[kevin-bates]

Could we please make this logging conditional on there being a replacement? Otherwise, this will log for 80% of cases when base_url values are not in play.

[bloomsa]

good call. I'll add a conditional check.

[kevin-bates]

Thanks @bloomsa!

[welcome]

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

[lresende]

@meeseeksdev please backport to 1.x