From 3ab28e89614f25d8249152a1097f6715f2b85a02 Mon Sep 17 00:00:00 2001 From: Milan Pavlik Date: Fri, 6 May 2022 13:33:04 +0000 Subject: [PATCH] [public-api] Convert server errors to appropriate RPC errors --- .../public-api-server/pkg/apiv1/workspace.go | 14 +++- .../pkg/apiv1/workspace_test.go | 77 +++++++++---------- .../public-api-server/pkg/proxy/errors.go | 35 +++++++++ .../pkg/proxy/errors_test.go | 36 +++++++++ 4 files changed, 119 insertions(+), 43 deletions(-) create mode 100644 components/public-api-server/pkg/proxy/errors.go create mode 100644 components/public-api-server/pkg/proxy/errors_test.go diff --git a/components/public-api-server/pkg/apiv1/workspace.go b/components/public-api-server/pkg/apiv1/workspace.go index 4ad06de183b470..f84b104442cd7d 100644 --- a/components/public-api-server/pkg/apiv1/workspace.go +++ b/components/public-api-server/pkg/apiv1/workspace.go @@ -8,6 +8,7 @@ import ( "context" "github.com/gitpod-io/gitpod/public-api-server/pkg/proxy" v1 "github.com/gitpod-io/gitpod/public-api/v1" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" "google.golang.org/grpc/status" @@ -27,6 +28,7 @@ type WorkspaceService struct { } func (w *WorkspaceService) GetWorkspace(ctx context.Context, r *v1.GetWorkspaceRequest) (*v1.GetWorkspaceResponse, error) { + logger := ctxlogrus.Extract(ctx) token, err := bearerTokenFromContext(ctx) if err != nil { return nil, err @@ -34,12 +36,22 @@ func (w *WorkspaceService) GetWorkspace(ctx context.Context, r *v1.GetWorkspaceR server, err := w.connectionPool.Get(ctx, token) if err != nil { + logger.WithError(err).Error("Failed to get connection to server.") return nil, status.Error(codes.Internal, "failed to establish connection to downstream services") } workspace, err := server.GetWorkspace(ctx, r.GetWorkspaceId()) if err != nil { - return nil, status.Error(codes.NotFound, "failed to get workspace") + logger.WithError(err).Error("Failed to get workspace.") + converted := proxy.ConvertError(err) + switch status.Code(converted) { + case codes.PermissionDenied: + return nil, status.Error(codes.PermissionDenied, "insufficient permission to access workspace") + case codes.NotFound: + return nil, status.Error(codes.NotFound, "workspace does not exist") + default: + return nil, status.Error(codes.Internal, "unable to retrieve workspace") + } } return &v1.GetWorkspaceResponse{ diff --git a/components/public-api-server/pkg/apiv1/workspace_test.go b/components/public-api-server/pkg/apiv1/workspace_test.go index f606f8dc38ed51..5554d7eff48466 100644 --- a/components/public-api-server/pkg/apiv1/workspace_test.go +++ b/components/public-api-server/pkg/apiv1/workspace_test.go @@ -6,7 +6,7 @@ package apiv1 import ( "context" - "fmt" + "errors" "github.com/gitpod-io/gitpod/common-go/baseserver" gitpod "github.com/gitpod-io/gitpod/gitpod-protocol" v1 "github.com/gitpod-io/gitpod/public-api/v1" @@ -37,24 +37,10 @@ func TestWorkspaceService_GetWorkspace(t *testing.T) { foundWorkspaceID: { LatestInstance: &gitpod.WorkspaceInstance{}, Workspace: &gitpod.Workspace{ - BaseImageNameResolved: "", - BasedOnPrebuildID: "", - BasedOnSnapshotID: "", - Config: nil, - ContentDeletedTime: "", - Context: nil, - ContextURL: contextURL, - CreationTime: "", - Deleted: false, - Description: description, - ID: foundWorkspaceID, - ImageNameResolved: "", - ImageSource: nil, - OwnerID: ownerID, - Pinned: false, - Shareable: false, - SoftDeleted: "", - Type: "", + ContextURL: contextURL, + Description: description, + ID: foundWorkspaceID, + OwnerID: ownerID, }, }, }}, @@ -68,39 +54,44 @@ func TestWorkspaceService_GetWorkspace(t *testing.T) { client := v1.NewWorkspacesServiceClient(conn) ctx := metadata.AppendToOutgoingContext(context.Background(), "authorization", bearerToken) - scenarios := []struct { - name string + type Expectation struct { + Code codes.Code + Response *v1.GetWorkspaceResponse + } + scenarios := []struct { + name string WorkspaceID string - - ErrorCode codes.Code - Response *v1.GetWorkspaceResponse + Expect Expectation }{ { name: "returns a workspace when workspace is found by ID", WorkspaceID: foundWorkspaceID, - ErrorCode: codes.OK, - Response: &v1.GetWorkspaceResponse{ - Result: &v1.Workspace{ - WorkspaceId: foundWorkspaceID, - OwnerId: ownerID, - ProjectId: "", - Context: &v1.WorkspaceContext{ - ContextUrl: contextURL, - Details: &v1.WorkspaceContext_Git_{Git: &v1.WorkspaceContext_Git{ - NormalizedContextUrl: contextURL, - Commit: "", - }}, + Expect: Expectation{ + Code: codes.OK, + Response: &v1.GetWorkspaceResponse{ + Result: &v1.Workspace{ + WorkspaceId: foundWorkspaceID, + OwnerId: ownerID, + ProjectId: "", + Context: &v1.WorkspaceContext{ + ContextUrl: contextURL, + Details: &v1.WorkspaceContext_Git_{Git: &v1.WorkspaceContext_Git{ + NormalizedContextUrl: contextURL, + Commit: "", + }}, + }, + Description: description, }, - Description: description, }, }, }, { name: "not found when workspace is not found by ID", WorkspaceID: "some-not-found-workspace-id", - ErrorCode: codes.NotFound, - Response: nil, + Expect: Expectation{ + Code: codes.NotFound, + }, }, } @@ -109,8 +100,10 @@ func TestWorkspaceService_GetWorkspace(t *testing.T) { resp, err := client.GetWorkspace(ctx, &v1.GetWorkspaceRequest{ WorkspaceId: scenario.WorkspaceID, }) - require.Equal(t, scenario.ErrorCode, status.Code(err), "status code must match") - if diff := cmp.Diff(scenario.Response, resp, protocmp.Transform()); diff != "" { + if diff := cmp.Diff(scenario.Expect, Expectation{ + Code: status.Code(err), + Response: resp, + }, protocmp.Transform()); diff != "" { t.Errorf("unexpected difference:\n%v", diff) } }) @@ -134,7 +127,7 @@ type FakeGitpodAPI struct { func (f *FakeGitpodAPI) GetWorkspace(ctx context.Context, id string) (res *gitpod.WorkspaceInfo, err error) { w, ok := f.workspaces[id] if !ok { - return nil, fmt.Errorf("workspace not found") + return nil, errors.New("code 404") } return w, nil diff --git a/components/public-api-server/pkg/proxy/errors.go b/components/public-api-server/pkg/proxy/errors.go new file mode 100644 index 00000000000000..9a3f2b556b138a --- /dev/null +++ b/components/public-api-server/pkg/proxy/errors.go @@ -0,0 +1,35 @@ +// Copyright (c) 2022 Gitpod GmbH. All rights reserved. +// Licensed under the GNU Affero General Public License (AGPL). +// See License-AGPL.txt in the project root for license information. + +package proxy + +import ( + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + "strings" +) + +func ConvertError(err error) error { + if err == nil { + return nil + } + + s := err.Error() + + if strings.Contains(s, "code 401") { + return status.Error(codes.PermissionDenied, s) + } + + // components/gitpod-protocol/src/messaging/error.ts + if strings.Contains(s, "code 404") { + return status.Error(codes.NotFound, s) + } + + // components/gitpod-messagebus/src/jsonrpc-server.ts#47 + if strings.Contains(s, "code -32603") { + return status.Error(codes.Internal, s) + } + + return status.Error(codes.Internal, s) +} diff --git a/components/public-api-server/pkg/proxy/errors_test.go b/components/public-api-server/pkg/proxy/errors_test.go new file mode 100644 index 00000000000000..da723bdda2b16c --- /dev/null +++ b/components/public-api-server/pkg/proxy/errors_test.go @@ -0,0 +1,36 @@ +// Copyright (c) 2022 Gitpod GmbH. All rights reserved. +// Licensed under the GNU Affero General Public License (AGPL). +// See License-AGPL.txt in the project root for license information. + +package proxy + +import ( + "errors" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + "testing" +) + +func TestConvertError(t *testing.T) { + scenarios := []struct { + WebsocketError error + ExpectedStatus codes.Code + }{ + { + WebsocketError: errors.New("reconnecting-ws: bad handshake: code 401 - URL: wss://main.preview.gitpod-dev.com/api/v1 - headers: map[Authorization:[Bearer foo] Origin:[http://main.preview.gitpod-dev.com/]]"), + ExpectedStatus: codes.PermissionDenied, + }, + { + WebsocketError: errors.New("jsonrpc2: code -32603 message: Request getWorkspace failed with message: No workspace with id 'some-id' found."), + ExpectedStatus: codes.Internal, + }, + } + + for _, s := range scenarios { + converted := ConvertError(s.WebsocketError) + require.Equal(t, s.ExpectedStatus, status.Code(converted)) + // the error message should remain the same + require.Equal(t, s.WebsocketError.Error(), status.Convert(converted).Message()) + } +}