Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use jupyter server as base provider #384

Merged
merged 32 commits into from
Jan 4, 2024

Conversation

kevin-bates
Copy link
Member

@kevin-bates kevin-bates commented Mar 16, 2023

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:

  • Replaces notebook with jupyter_server as its base provider
  • Replaces @gen.coroutine/yield with async/await
  • Removes Python 3.7 support and adds Python 3.10 and 3.11
  • Replaces unittest with pytest and adds coverage diagnostics
  • Adds support for a default identity provider so Jupyter Server 2+ can be used
  • Removes python b/c for 2.7 support
  • Investigate test flakiness (race condition between pool startup and tests)

Notes/Caveats/Status:

  • 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'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.
  • 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 kevin-bates marked this pull request as draft March 16, 2023 00:19
@codecov
Copy link

codecov bot commented Mar 16, 2023

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 ⚠️
kernel_gateway/auth/identity.py 91.30% 1 Missing and 1 partial ⚠️
...gateway/tests/notebook_http/swagger/test_parser.py 98.18% 1 Missing ⚠️
kernel_gateway/tests/test_jupyter_websocket.py 99.70% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kevin-bates
Copy link
Member Author

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

@echarles
Copy link
Member

echarles commented Apr 9, 2023

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:

  • Works like before as a notebook/lab user?
  • Something more specific about authentication/authorization?
  • Anything else?

@kevin-bates
Copy link
Member Author

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
Copy link
Member

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 jupyter-server/jupyter_server#1245 (comment)

@kevin-bates
Copy link
Member Author

btw The need for a cookie is breaking other use case which I would like to discuss, see jupyter-server/jupyter_server#1245 (comment)

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

@kevin-bates
Copy link
Member Author

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
Copy link
Member

Zsailer commented Jan 2, 2024

  • 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
Copy link
Member Author

That would be fantastic @Zsailer, thank you!

@Zsailer Zsailer marked this pull request as ready for review January 4, 2024 00:08
@Zsailer Zsailer closed this Jan 4, 2024
@Zsailer Zsailer reopened this Jan 4, 2024
@Zsailer
Copy link
Member

Zsailer commented Jan 4, 2024

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

@Zsailer Zsailer force-pushed the use-jupyter-server branch from b9aca67 to 43975c5 Compare January 4, 2024 00:27
@Zsailer
Copy link
Member

Zsailer commented Jan 4, 2024

We need jupyter-server/jupyter_server#1311 to be merged and released in Jupyter Server before this PR will work.

@Zsailer
Copy link
Member

Zsailer commented Jan 4, 2024

All green baby!

@Zsailer
Copy link
Member

Zsailer commented Jan 4, 2024

Merging.

@Zsailer Zsailer merged commit d455f4e into jupyter-server:main Jan 4, 2024
8 checks passed
@Zsailer
Copy link
Member

Zsailer commented Jan 4, 2024

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

Or @blink1073?

@Zsailer Zsailer mentioned this pull request Jan 4, 2024
@blink1073
Copy link
Contributor

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

@blink1073
Copy link
Contributor

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

@Zsailer
Copy link
Member

Zsailer commented Jan 6, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants