jupyter-server / kernel_gateway

Jupyter Kernel Gateway
http://jupyter-kernel-gateway.readthedocs.org/en/latest/
Other
501 stars 131 forks source link

Use jupyter server as base provider #384

Closed kevin-bates closed 9 months ago

kevin-bates commented 1 year ago

This pull request replaces notebook with jupyter_server as its base provider. This will be merged as part of Kernel Gateway's 3.0 release and provides the following changes:

Notes/Caveats/Status:

codecov[bot] commented 1 year ago

Codecov Report

Attention: 51 lines in your changes are missing coverage. Please review.

Comparison is base (9798448) 95.77% compared to head (414cfe2) 94.25%. Report is 1 commits behind head on main.

Files Patch % Lines
kernel_gateway/gatewayapp.py 58.03% 44 Missing and 3 partials :warning:
kernel_gateway/auth/identity.py 91.30% 1 Missing and 1 partial :warning:
...gateway/tests/notebook_http/swagger/test_parser.py 98.18% 1 Missing :warning:
kernel_gateway/tests/test_jupyter_websocket.py 99.70% 0 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #384 +/- ## ========================================== - Coverage 95.77% 94.25% -1.53% ========================================== Files 31 31 Lines 2273 2350 +77 Branches 0 308 +308 ========================================== + Hits 2177 2215 +38 - Misses 96 106 +10 - Partials 0 29 +29 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kevin-bates commented 1 year ago

Current test failures are due to this PR not being released yet.

echarles commented 1 year ago

Really hoping someone with knowledge about web auth programming, along with an understanding about the relationship of a gateway server to a "front-end" server can help out (adding 'help wanted' label).

@kevin-bates what kind of review do you need:

kevin-bates commented 1 year ago

I think moving the handlers and "base" services to Jupyter Server 2.0 will require some authentication and authorization changes. If my understanding is correct, we will need to convey the cookie used to track a user's authentication via the GatewayClient (which requires an update) so that that "user" flows.

Also, it would be ideal to have the gateways (including Enterprise Gateway here as well) work when the client application is a down-level Jupyter Server (or older Notebook) when the cookie is not present.

I also don't know if this will have ramifications on users that rely on a reverse proxy for securing their Gateway server. I'm hoping these changes won't have an impact there.

I'm not sure if this answers your questions. It's quite a gray area for me.

echarles commented 1 year ago

I'm not sure if this answers your questions. It's quite a gray area for me.

It does answer, thank you. btw The need for a cookie is breaking other use case which I would like to discuss, see https://github.com/jupyter-server/jupyter_server/issues/1245#issuecomment-1500896652

kevin-bates commented 1 year ago

btw The need for a cookie is breaking other use case which I would like to discuss, see https://github.com/jupyter-server/jupyter_server/issues/1245#issuecomment-1500896652

Yes, we'll definitely need to follow that conversation to see how this might impact the gateways.

kevin-bates commented 1 year ago

Hi @krassowski, thanks for asking about this work in today's Jupyter Server/Kernels meeting. I was happy to see that I had created a PR for this effort. Although all of the current check-boxes are checked, I believe additional tasks will come from the "Notes/Caveats/Status" section.

Any help would be appreciated. In that vane, I've sent an invite for your collaboration on this branch.

Zsailer commented 9 months ago
  • Only Jupyter Server 2.x+ will be supported. I've found its too difficult to support both forms of authentication/authorization that were changed across JS 1.x and 2.x.

I think this is completely reasonable. Let's make kernel_gateway 3.x be Jupyter Server ready, and (temporarily) maintain two branches.

  • I've broken my pick on the identity stuff. When running JKG with JS 2, the only way I can get requests working is to define the same auth token on both sides (e.g., --KernelGatewayApp.auth_token=foo for the gateway, and --GatewayClient.auth_token=foo for the server). Not setting auth_token results in 401 (Unauthorized).
  • I'm not familiar with older (JKG/Notebook) way authentication/authorization worked, but I suspect it was minimal and now, with the authorization logic embedded in the JS base handlers (which KG uses) things are a bit wonky.

I think both of these issues can be smoothed over by using a default, (essentially) no-op Identity Provider in Kernel Gateway. This IdentityProvider just ensures the auth_tokens match between the gateway server and the "client" server. That's all we need to get this out the door, no?

This will make KG work as it always has.

If someone would like to customize a KG to 1) track identity or 2) perform finer grained authorization, the new identity_provider and authorizer APIs give them the hooks to do it. I don't think we need to flow a more thorough user model through KG—at least not to release this PR.

@kevin-bates, I know your time is very limited these days, so I can pick up this PR and get it out the door.

kevin-bates commented 9 months ago

That would be fantastic @Zsailer, thank you!

Zsailer commented 9 months ago

I think this is ready to go. I'm not sure why tests aren't running.... looking into this.

Zsailer commented 9 months ago

We need https://github.com/jupyter-server/jupyter_server/pull/1311 to be merged and released in Jupyter Server before this PR will work.

Zsailer commented 9 months ago

All green baby!

Zsailer commented 9 months ago

Merging.

Zsailer commented 9 months ago

@kevin-bates are you able to add me as a maintainer on the PyPI package here?

Or @blink1073?

blink1073 commented 9 months ago

@Zsailer I moved the package to the Jupyter org and added the Jupyter Server team as a maintainer, you should have access now.

blink1073 commented 9 months ago

(I also added you as a member of that team).

Zsailer commented 9 months ago

Thanks, @blink1073! I'll cut a release first thing Monday.