Skip to content

[proxy] Proxy websocket traffic to slow-server deployment according to Sec-WebSocket-Protocol header #14778

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

Merged
merged 2 commits into from
Nov 21, 2022

Conversation

andrew-farries
Copy link
Contributor

@andrew-farries andrew-farries commented Nov 18, 2022

Description

As part of #9198 we want to send a subset of websocket traffic from the dashboard to an instance of server that runs with a higher latency connection to the Gitpod database.

This PR changes the proxy Caddyfile to reverse proxy traffic to the new slow-server deployment depending on the value of the Sec-WebSocket-Protocol header that is sent by the browser websocket client during the websocket connection handshake.

#14752 demonstrates an approach that allows to selectively set the value of this header for some users, based on the value of a feature flag.

Related Issue(s)

Part of #9198

How to test

In the browser console, establish a new websocket connection to the Gitpod JSON RPC API:

const sock = new WebSocket('wss://af-use-slofc5943d8fc.preview.gitpod-dev.com/api/gitpod', 'slow-database')

The network tab should show a new websocket connection with these headers sent during the handshake:

image

Sending and receiving through the new connection should now incur higher than usual latency:

const msg = JSON.stringify({"jsonrpc":"2.0","id":12,"method":"getWorkspaces","params":{"limit":50}})
sock.send(msg)

the network tab shows a gap of ~1s between message send and response.

Tailing the logs of pods in the slow-server deployment reveals that they are handling websocket traffic.

Release Notes

NONE

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-slow-database
  • /werft with-large-vm
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

Andrew Farries added 2 commits November 18, 2022 08:54
Add matchers for the `Sec-WebSocket-Protocol` header and use them in the
`reverse_proxy` directives.
@andrew-farries andrew-farries requested a review from a team November 18, 2022 09:17
@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-af-proxy-to-slow-server-based-on-ws-header.4 because the annotations in the pull request description changed
(with .werft/ from main)

@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Nov 18, 2022
@geropl
Copy link
Member

geropl commented Nov 18, 2022

I'm going to review this PR now.

@andrew-farries I think we should not re-use any of the existing headers like Sec-WebSocket-Protocol that already carry meaning, but rather make up our own like X-Gitpod-SlowServer or so, and inject that via https://addons.mozilla.org/en-US/firefox/addon/modify-header-value/ or similar extensions.

@@ -210,8 +210,21 @@ https://{$GITPOD_DOMAIN} {
handle @backend_wss {
gitpod.sec_websocket_key

@slow {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧡 Love this change - simply on-point 😄

I'd only suggest we user a custom header instead of the websocket one. Also, we should check this on other HTTP routes that are sent against server as well 👇 .

@andrew-farries
Copy link
Contributor Author

I think we should not re-use any of the existing headers like Sec-WebSocket-Protocol that already carry meaning, but rather make up our own like X-Gitpod-SlowServer or so, and inject that via https://addons.mozilla.org/en-US/firefox/addon/modify-header-value/ or similar extensions.

If we go down the route of using browser extensions to inject extra headers then we eliminate the possibility of being able to control the feature by using a feature flag. By using the Sec-WebSocket-Protocol header we don't need to use browser extensions and can roll this out more widely more easily (eg start with just webapp team members, then rollout to all Gitpodders).

I'd only suggest we user a custom header instead of the websocket one. Also, we should check this on other HTTP routes that are sent against server as well

This is an advantage of using a browser extension; with the Sec-WebSocket-Protocol header we can only check this on the websocket connection handshake. IMO, we should roll out this websocket-only version and then see if we need further validation. It may become clear that the latency is already too high.

order gitpod.headless_log_download before rewrite
order gitpod.configcat before rewrite
order gitpod.sec_websocket_key before header
order gitpod.cors_origin before header
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming the formatting is based on the Caddy fmt command

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct 👍

Copy link
Member

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

/hold in case you wanna wait for Gero

@roboquat roboquat merged commit cd03a46 into main Nov 21, 2022
@roboquat roboquat deleted the af/proxy-to-slow-server-based-on-ws-header branch November 21, 2022 13:53
@andrew-farries
Copy link
Contributor Author

andrew-farries commented Nov 21, 2022

Another race between tide and roboquat here 😭

@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production do-not-merge/hold release-note-none size/M team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants