Skip to content

[proxy] Match /api/v1 in the @backend_wss matcher #14974

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 28, 2022
Merged

[proxy] Match /api/v1 in the @backend_wss matcher #14974

merged 1 commit into from
Nov 28, 2022

Conversation

andrew-farries
Copy link
Contributor

@andrew-farries andrew-farries commented Nov 25, 2022 •
edited
Loading

Description

Websocket Upgrade requests can hit either /api/gitpod (from server) or /api/v1 (from the public API).

This matcher change ensures that websocket requests to api/v1 are handled by the matcher block intended to handle websocket requests rather than the block for api/* requests.

Websocket Upgrade requests to api/v1 worked without this change because the handler for api/* handled those requests correctly, but this won't be the case after #14897 (which makes changes to the /api/* handler) lands.

Related Issue(s)

Part of #9198

How to test

The public API RPCs that delegate to server over the JSON-RPC API still work.

Release Notes

NONE

Documentation

Werft options:

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

Match /api/v1 in the @backend_wss matcher
67a4956 
Websocket Upgrade requests can hit either `/api/gitpod` (from `server`)
or `/api/v1` (from the public API).

This matcher change ensures that the request is handled by the intended
matcher block rather than the block for `api/*` requests below.
@andrew-farries andrew-farries requested a review from a team November 25, 2022 16:31
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Nov 25, 2022
@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-af-caddyy-add-path-to-path-matcher.1 because the annotations in the pull request description changed
(with .werft/ from main)

@andrew-farries andrew-farries mentioned this pull request Nov 25, 2022
Merged
5 tasks
@easyCZ
Copy link
Member

easyCZ commented Nov 25, 2022

The Public API uses both endpoints, depending on the type of Auth token. Currently server doesn't uniformly support Bearer and Session auth so it's spread across these. Shouldn't cause any issues but just an FYI

@geropl
Copy link
Member

geropl commented Nov 28, 2022

Testing this PR. The only differences I see are the upstream_connection, upstream_headers or CORS header (templates). Only the later should influence behavior, though. The best test I could think of is trying to connect from a local IDE.

@andrew-farries Might be worth asking the IDE team for their opinion on this PR.

@andrew-farries andrew-farries requested a review from a team November 28, 2022 08:24
@andrew-farries
Copy link
Contributor Author

Have tagged the IDE team for review.

@geropl
Copy link
Member

geropl commented Nov 28, 2022

preview env is down 😢

@geropl
Copy link
Member

geropl commented Nov 28, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-af-caddyy-add-path-to-path-matcher.2
(with .werft/ from main)

geropl
geropl approved these changes Nov 28, 2022
View reviewed changes
Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Tested starting a workspace with browers and local VsCode, all worked fine. 👍

/hold for further review by IDE team

@andrew-farries
Copy link
Contributor Author

Checked that the public API is still able to talk to server over a websocket connection to /v1:

http POST https://api.af-caddyy-07cce80379.preview.gitpod-dev.com/gitpod.experimental.v1.TeamsService/GetTeam teamId="23bf92ee-4f08-456f-b498-1f89637b2900" Authorization:'Bearer <>'

(using a bearer token rather than session auth forces the public api to use the v1 endpoint).

@geropl
Copy link
Member

geropl commented Nov 28, 2022

@andrew-farries After additional testing I'm confident we're not breaking anybody with this 🙃 I would be happy to move forward with this 👍

@andrew-farries andrew-farries removed the request for review from a team November 28, 2022 14:20
@roboquat roboquat merged commit 79e4e37 into main Nov 28, 2022
@roboquat roboquat deleted the af/caddyy-add-path-to-path-matcher branch November 28, 2022 14:21
@roboquat roboquat added deployed: IDE IDE change is running in production deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Nov 28, 2022
This was referenced Nov 29, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geropl geropl geropl approved these changes

Assignees
No one assigned
Labels
deployed: IDE IDE change is running in production deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/XS team: IDE team: webapp Issue belongs to the WebApp team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@andrew-farries @easyCZ @geropl @roboquat