jupyter-server / jupyter_server

The backend—i.e. core services, APIs, and REST endpoints—to Jupyter web applications.
https://jupyter-server.readthedocs.io
BSD 3-Clause "New" or "Revised" License
489 stars 308 forks source link

websocket subprotocol negotiation when using gateway #1310

Closed epignot closed 10 months ago

epignot commented 1 year ago

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

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

welcome[bot] commented 1 year ago

Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! :hugs:
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! :wave:
Welcome to the Jupyter community! :tada:

kevin-bates commented 1 year ago

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.