Skip to content

[public-api] Refactor to use connect handlers #13692

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

Closed
wants to merge 1 commit into from

Conversation

easyCZ
Copy link
Member

@easyCZ easyCZ commented Oct 8, 2022

Description

In this PR, we 1-1 map the old implementation in gRPC to the new implementation with Connect. Changes are deliberately not made to streamline the tests and simplify to keep the PR cleaner.

In general, the changes are the following (see docs)

  1. Use the generated ServiceHandlers
  2. Wrap requests and responses in connect.NewRequest or connect.NewResponse
  3. Use Connects error codes
  4. Use HTTP server in tests

This PR also changes default routing of api.<domain> requests to the HTTP server, this has the following effect:

  • Connect handlers handle both gRPC and HTTP traffic - this is fine, as Connect is designed for this
  • Depending on the rollout sequencing, there can be a brief period where requests would fail. This is OK, as we don't get any traffic on this yet anyway.

Related Issue(s)

How to test

Unit tests

  1. Log in to preview and run the following in the console to get an access token:
	await window._gp.gitpodService.server.generateNewGitpodToken({ type: 1, scopes: ["function:getGitpodTokenScopes",
	"function:getWorkspace",
	"function:getWorkspaces",
	"function:listenForWorkspaceInstanceUpdates",
	"resource:default",]})

To make subsequent steps readily runnable, export the token in your shell:

export BEARER_TOKEN="<token>"

Validate we can reach the new APIs over cURL:

curl \
    --header 'Content-Type: application/json' \
    --header 'Authorization: Bearer $BEARER_TOKEN' \
    --data '{}' \
    https://api.mp-public-ca4417e0d2.preview.gitpod-dev.com/gitpod.v1.WorkspacesService/ListWorkspaces | jq

{}

We're doing a list call, and we don't have workspaces so an empty list response is expected.

Validate we can reach the APIs using a gRPC client

gpctl api workspaces list --address api.mp-public-ca4417e0d2.preview.gitpod-dev.com:443 --token $BEARER_TOKEN 

ID        Owner        ContextURL

Again, we're not getting any workspaces back because we've not started any but the important part is we're not getting an error.

Release Notes

NONE

Documentation

Werft options:

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

@easyCZ easyCZ force-pushed the mp/public-api-use-connect-instead-of-grpc branch from 9ff7a33 to 0fedcab Compare October 8, 2022 21:59
@easyCZ easyCZ changed the base branch from main to mp/public-api-connect-auth October 8, 2022 21:59
@roboquat roboquat added size/L and removed size/XXL labels Oct 8, 2022
@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-mp-public-api-use-connect-instead-of-grpc.3 because the annotations in the pull request description changed
(with .werft/ from main)

@easyCZ easyCZ force-pushed the mp/public-api-connect-auth branch 2 times, most recently from 6755ce7 to d7b5c50 Compare October 9, 2022 20:09
@easyCZ easyCZ force-pushed the mp/public-api-use-connect-instead-of-grpc branch from 0fedcab to aa389ea Compare October 9, 2022 20:26
@roboquat roboquat added size/XL and removed size/L labels Oct 9, 2022
@easyCZ easyCZ force-pushed the mp/public-api-use-connect-instead-of-grpc branch 2 times, most recently from 22a3a8d to e3ceb72 Compare October 9, 2022 21:54
requireErrorStatusCode(t, codes.Unimplemented, err)
}

func TestPublicAPIServer_WorkspaceServiceHandler(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Diff shows wierd. This test is removed as it's already tested in TestPublicAPIServer_v1_WorkspaceService

require.Equalf(t, expected, st.Code(), "expected: %s but got: %s", expected.String(), st.String())
func requireErrorStatusCode(t *testing.T, expected connect.Code, err error) {
t.Helper()
if expected == 0 && err == nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

Connect doesn't define the default Error Code value as OK, so we have to do this extra check..

@easyCZ easyCZ force-pushed the mp/public-api-use-connect-instead-of-grpc branch 3 times, most recently from 36a19ad to 2eed6c9 Compare October 10, 2022 06:44
@@ -51,7 +51,7 @@ func newPublicAPIConn() (*grpc.ClientConn, error) {
opts := []grpc.DialOption{
// attach token to requests to auth
grpc.WithUnaryInterceptor(func(ctx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error {
withAuth := metadata.AppendToOutgoingContext(ctx, "authorization", publicApiCmdOpts.token)
withAuth := metadata.AppendToOutgoingContext(ctx, "authorization", "Bearer "+publicApiCmdOpts.token)
Copy link
Member Author

@easyCZ easyCZ Oct 10, 2022

Choose a reason for hiding this comment

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

Was not correctly adding the Bearer prefix, and we weren't also correctly checking for it server side.

@easyCZ easyCZ marked this pull request as ready for review October 10, 2022 06:48
@easyCZ easyCZ requested review from a team October 10, 2022 06:48
@github-actions github-actions bot added team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team labels Oct 10, 2022
Base automatically changed from mp/public-api-connect-auth to main October 10, 2022 14:54
@roboquat roboquat added size/XXL and removed size/XL labels Oct 10, 2022
@easyCZ easyCZ force-pushed the mp/public-api-use-connect-instead-of-grpc branch from 2eed6c9 to 66b1831 Compare October 10, 2022 15:05
@roboquat roboquat added size/XL and removed size/XXL labels Oct 10, 2022
@easyCZ
Copy link
Member Author

easyCZ commented Oct 10, 2022

Closing as werft got into an inconcistent state. Replacement PR is here

@easyCZ easyCZ closed this Oct 10, 2022
@easyCZ easyCZ deleted the mp/public-api-use-connect-instead-of-grpc branch December 8, 2022 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none size/XL team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants