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

websocket subprotocol negotiation when using gateway #1310

Closed
epignot opened this issue Aug 8, 2023 · 2 comments · Fixed by #1311
Closed

websocket subprotocol negotiation when using gateway #1310

epignot opened this issue Aug 8, 2023 · 2 comments · Fixed by #1311
Labels

Comments

@epignot
Copy link
Contributor

epignot commented Aug 8, 2023

Description

Jupyter server accepts v1.kernel.websocket.jupyter.org ws subprotocol regardless of what support the gateway and without informing it. Notebook client ends up with websocket messages not respecting the subprotocol accepted.

Reproduce

  1. Install latest JupyterLab 4 and latest Enterprise Gateway pip install --upgrade jupyter_enterprise_gateway
  2. Run it jupyter enterprisegateway
  3. Install latest JupyterLab 4: pip install --upgrade jupyterlab
  4. And run it against enterprise gateway jupyter lab --debug --GatewayClient.url="http://127.0.0.1:8888"
  5. Once JupyterLab UI opened in your browser, open your browser DevTool network panel
  6. On the UI, launch a notebook and try to run some code
  7. See that nothing happens

JupyterLab client fails to parse the messages :

serialize.js:64 Uncaught TypeError: Kernel message validation error: First argument to DataView constructor must be an ArrayBuffer
    at new DataView (<anonymous>)
    at Object.t [as deserializeV1KernelWebsocketJupyterOrg] (serialize.js:64:22)
    at l (serialize.js:50:28)
    at WebSocket._onWSMessage (default.js:165:39)

In the network panel, if you look at websocket connections for requests api/kernels/{kernel-id}/channels, in the response headers you will notice Jupyter Server accepted v1.kernel.websocket.jupyter.org protocol
Screenshot 2023-08-08 at 11 17 57

But when inspecting the messages, responses are actually json-serialized
Screenshot 2023-08-08 at 11 17 22

Expected behavior

  • Jupyter Server should respect ws subprotocol it accepts
  • Jupyter Server should not accept v1.kernel.websocket.jupyter.org subprotocol when using the GatewayClient

Explanation

Jupyter Server now supports the websocket subprotocol v1.kernel.websocket.jupyter.org in addition of the legacy one.
If a notebook client specifies it during in the Upgrade request, Jupyter Server will accept the v1.kernel.websocket.jupyter.org subprotocol by default, and it will become the KernelWebsocketHandler's selected_subprotocol
from tornado code:

    async def _accept_connection(self, handler: WebSocketHandler) -> None:
        subprotocol_header = handler.request.headers.get("Sec-WebSocket-Protocol")
        if subprotocol_header:
            subprotocols = [s.strip() for s in subprotocol_header.split(",")]
        else:
            subprotocols = []
        self.selected_subprotocol = handler.select_subprotocol(subprotocols)
        if self.selected_subprotocol:
            assert self.selected_subprotocol in subprotocols
            handler.set_header("Sec-WebSocket-Protocol", self.selected_subprotocol)

But in GatewayWebSocketConnection, ws connection is opened without specifying any protocols:

self.ws_future = cast(Future, tornado_websocket.websocket_connect(request))

Hence, the legacy protocol is used. I guess it's ok since the gateway WebSocketChannelsHandler just send messages back to the notebook client

Workaround

Run jupyter lab with option --GatewayWebSocketConnection.kernel_ws_protocol=""

Proposed fix

Force the websocket subprotocol for the gateway client:

class GatewayWebSocketConnection(BaseKernelWebsocketConnection):
    """Web socket connection that proxies to a kernel/enterprise gateway."""
    kernel_ws_protocol=""

This seems the simplest solution to me for now. (and that works)
I don't know how Jupyter Server could be aware of the gateway protocol support before the negotiation with notebook client.
Or if we should make the gateway client support the new protocol.

I will create the PR, if this fix is accepted

Context

  • Operating System and version: MacOS Ventura 13.4
  • Browser and version: Chrome 115
  • Jupyter Server version: 2.7.0
@epignot epignot added the bug label Aug 8, 2023
@welcome
Copy link

welcome bot commented Aug 8, 2023

Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please try to follow the issue template as it helps other other community members to contribute more effectively.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also an intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@kevin-bates
Copy link
Member

Hi @epignot - thank you for opening this issue along with your great analysis. I think we must go with the proposed fix at this time since both EG is running on server 1.x and JKG is still based on notebook. I'm assuming that once the gateways are built on Jupyter Server 2.x, we'd be okay opening things back up, although I'm not confident about that statement either.

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

Successfully merging a pull request may close this issue.

2 participants