Clone the tshd gRPC client to allow inspecting errors by gzdunek · Pull Request #39229 · gravitational/teleport

gzdunek · 2024-03-12T12:05:10Z

Part 1/2 of https://github.com/gravitational/teleport.e/issues/853

The task of this PR is to provide utility functions for wrapping each gRPC call so we can inspect their errors on the renderer side.
Why do we need all these clone* functions? Because when the error goes through the electron context bridge (is a bridge between preload and renderer) most of its properties is removed, so we can't inspect gRPC code or metadata.

The solution for this is converting the error to an object before its crosses the bridge.

Additionally, I switched the client from grpc-js to protobuf-ts. It has a more modern API that uses promises and abort signals. The conversion of createClient is done in the second PR.

Also, emitting our custom errors addresses #30753. The codes are no longer visible, but if we want to bring them back (in a nicer form) we can do it.

I wanted split the changes into something that is easier to review, this PR contains the essential parts: adds functions to convert the calls and abort signals, converts interceptors to the form required by protobuf-ts, replaces the check in isRetryable with the check on error metadata.

Note: this PR doesn't pass TS check, it requires the second PR.

…the context bridge

…ones

…ToRetryableError`

It's needed because now we also log a passwordless login stream call.

avatus

I need to dive in more but so far looks good

gzdunek · 2024-03-14T11:23:46Z

@ravicious helped me with creating a function to wrap the entire client, instead of each method manually.
That's a great improvement, it reduces the amount of boilerplate in createClient.ts even further.

I updated both PRs.

ravicious

Nice job, the code in this PR looks very good. I submitted a bunch of minor suggestions.

ravicious · 2024-03-15T14:51:42Z

+ * A unary RPC call. Can be passed over the context bridge.
+ * Errors are converted to `TshdRpcError` objects.
+ */
+export type CloneableUnaryCall<I extends object, O extends object> = Pick<


Do types for specific calls need to be exported?

I wanted to say that I need a CloneableUnaryCall for types of reportEvent, but actually I don't.
I can do this:

reportUsageEvent: CloneableClient<ITerminalServiceClient>['reportUsageEvent'];

instead of this:

reportUsageEvent: ( input: ReportUsageEventRequest, options?: CloneableRpcOptions ) => CloneableUnaryCall<ReportUsageEventRequest, apiService.EmptyResponse>;

This should be fine for now until we replace createClient. In my opinion, when defining types it's better to avoid to referring to types of other "entities".

It's pretty much like Law of Demeter but for types. Such things are not even possible in languages designed with types in mind from the start. 😏

ravicious · 2024-03-15T14:55:17Z

+    if (error.name === 'RpcError') {
+      const e = error as RpcError;


if (error instanceof RpcError) should work here, shouldn't it? Since we're still on the preload side.

I'm sure that I tried this, but it didn't want to work 🤔
But I was wrong, it works perfectly fine, I don't know, maybe I was checking it on the renderer side 🤷‍♂️

…e context bridge (#39230) * Regenerate protos with the `protobuf-ts` client * Enable `strictBindCallApply` so `.bind()` results have correct types (instead of `any`) This is needed for wrapping calls in `createClient`. * Use `protobuf-ts` client instead of `grpc-js` one * Convert `createClient` to use `protobuf-ts` response style, clone each call * Switch callsites to `cloneAbortSignal` * Replace `error.message` checks with a proper check on the error status code * Clone the entire client instead of each method separately * Correct the error `cause` in `ResourceSearchError` * Remove `params.sortBy` defaults, always set `startKey` to string * Use a simpler type for `reportUsageEvent`

# Conflicts: # web/packages/teleterm/src/services/tshd/createClient.ts

public-teleport-github-review-bot · 2024-03-18T16:53:08Z

@gzdunek See the table below for backport results.

Branch	Result
branch/v15	Failed

* Add functions to clone every gRPC method, so they can be passed over the context bridge * Replace `@grpc/grpc-js` interceptors with `@protobuf-ts/runtime-rpc` ones * Replace string check with check on metadata, add missing `AddMetadataToRetryableError` * Add `pin` to sensitive properties It's needed because now we also log a passwordless login stream call. * Add `cloneClient` method to clone the entire gRPC client * Switch tests to `TshdRetryableError` * Improve comments * Move `cloneClient` and `cloneAbortSignal` to the top of the file * Simplify condition * Lowercase logs * Check if the error is `RpcError` using `instanceof` * Log error object instead of `[object Object]` * Do not export call specific types * Wrap gRPC calls in `createClient` to allow passing the errors over the context bridge (#39230) * Regenerate protos with the `protobuf-ts` client * Enable `strictBindCallApply` so `.bind()` results have correct types (instead of `any`) This is needed for wrapping calls in `createClient`. * Use `protobuf-ts` client instead of `grpc-js` one * Convert `createClient` to use `protobuf-ts` response style, clone each call * Switch callsites to `cloneAbortSignal` * Replace `error.message` checks with a proper check on the error status code * Clone the entire client instead of each method separately * Correct the error `cause` in `ResourceSearchError` * Remove `params.sortBy` defaults, always set `startKey` to string * Use a simpler type for `reportUsageEvent` * Fix interceptor test * Ignore TS error in `highbar.ts` (cherry picked from commit 784d7ac)

…ss denied" (#39720) * Clone the tshd gRPC client to allow inspecting errors (#39229) * Add functions to clone every gRPC method, so they can be passed over the context bridge * Replace `@grpc/grpc-js` interceptors with `@protobuf-ts/runtime-rpc` ones * Replace string check with check on metadata, add missing `AddMetadataToRetryableError` * Add `pin` to sensitive properties It's needed because now we also log a passwordless login stream call. * Add `cloneClient` method to clone the entire gRPC client * Switch tests to `TshdRetryableError` * Improve comments * Move `cloneClient` and `cloneAbortSignal` to the top of the file * Simplify condition * Lowercase logs * Check if the error is `RpcError` using `instanceof` * Log error object instead of `[object Object]` * Do not export call specific types * Wrap gRPC calls in `createClient` to allow passing the errors over the context bridge (#39230) * Regenerate protos with the `protobuf-ts` client * Enable `strictBindCallApply` so `.bind()` results have correct types (instead of `any`) This is needed for wrapping calls in `createClient`. * Use `protobuf-ts` client instead of `grpc-js` one * Convert `createClient` to use `protobuf-ts` response style, clone each call * Switch callsites to `cloneAbortSignal` * Replace `error.message` checks with a proper check on the error status code * Clone the entire client instead of each method separately * Correct the error `cause` in `ResourceSearchError` * Remove `params.sortBy` defaults, always set `startKey` to string * Use a simpler type for `reportUsageEvent` * Fix interceptor test * Ignore TS error in `highbar.ts` (cherry picked from commit 784d7ac) * Do not replace `AccessDenied` error messages with generic "access denied" (#39558) * Do not replace `AccessDenied` error messages with generic "access denied" * Replace `error.code` checks with `isTshdRpcError(error, code)` * Add `ABORTED` and `UNAUTHENTICATED` codes * Improve `isTshdRpcError` documentation * Do not catch `retryWithRelogin` errors in `Setup.tsx` (cherry picked from commit 48c05b6)

gzdunek added 3 commits March 12, 2024 12:33

Add functions to clone every gRPC method, so they can be passed over …

057034a

…the context bridge

Replace @grpc/grpc-js interceptors with @protobuf-ts/runtime-rpc …

103f47a

…ones

Replace string check with check on metadata, add missing `AddMetadata…

302c8c1

…ToRetryableError`

gzdunek added teleport-connect Issues related to Teleport Connect. no-changelog Indicates that a PR does not require a changelog entry backport/branch/v15 labels Mar 12, 2024

github-actions Bot requested review from ibeckermayer and ravicious March 12, 2024 12:05

github-actions Bot added size/md ui labels Mar 12, 2024

gzdunek requested review from avatus and removed request for ibeckermayer March 12, 2024 12:05

Add pin to sensitive properties

da990ca

It's needed because now we also log a passwordless login stream call.

avatus reviewed Mar 12, 2024

View reviewed changes

Comment thread lib/teleterm/services/clientcache/clientcache.go

gzdunek added 2 commits March 14, 2024 12:12

Add cloneClient method to clone the entire gRPC client

0b8753a

Switch tests to TshdRetryableError

4f102d5

avatus approved these changes Mar 14, 2024

View reviewed changes

ravicious approved these changes Mar 15, 2024

View reviewed changes

gzdunek and others added 11 commits March 18, 2024 09:40

Improve comments

957bb52

Move cloneClient and cloneAbortSignal to the top of the file

0575954

Simplify condition

40ff155

Lowercase logs

964e18e

Check if the error is RpcError using instanceof

cd09012

Log error object instead of [object Object]

e537892

Do not export call specific types

e3c2466

Merge branch 'master' into gzdunek/cloneable-client

f187ce2

# Conflicts: # web/packages/teleterm/src/services/tshd/createClient.ts

Fix interceptor test

f91b2fa

Ignore TS error in highbar.ts

0404715

gzdunek changed the title ~~Add functions to wrap gRPC calls to allow inspecting errors~~ Clone the tshd gRPC client to allow inspecting errors Mar 18, 2024

gzdunek enabled auto-merge March 18, 2024 16:19

gzdunek added this pull request to the merge queue Mar 18, 2024

Merged via the queue into master with commit 784d7ac Mar 18, 2024

gzdunek deleted the gzdunek/cloneable-client branch March 18, 2024 16:52

gzdunek mentioned this pull request Mar 19, 2024

Do not replace AccessDenied error messages with generic "access denied" #39558

Merged

This was referenced Mar 22, 2024

[v15] Do not replace AccessDenied error messages with generic "access denied" #39720

Merged

Connect: Do not show gRPC status numbers in UI, prettify status codes #30753

Closed

gzdunek mentioned this pull request Mar 26, 2024

Remove custom tshd types and createTshdClient boilerplate #39828

Merged

gzdunek mentioned this pull request Apr 5, 2024

Do not stringify log messages in the browser console #40265

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clone the tshd gRPC client to allow inspecting errors#39229

Clone the tshd gRPC client to allow inspecting errors#39229
gzdunek merged 17 commits intomasterfrom
gzdunek/cloneable-client

gzdunek commented Mar 12, 2024 •

edited

Loading

Uh oh!

avatus left a comment

Uh oh!

Uh oh!

gzdunek commented Mar 14, 2024

Uh oh!

ravicious left a comment

Uh oh!

Uh oh!

Uh oh!

ravicious Mar 15, 2024

Uh oh!

gzdunek Mar 18, 2024

Uh oh!

ravicious Mar 18, 2024

Uh oh!

Uh oh!

ravicious Mar 15, 2024

Uh oh!

gzdunek Mar 18, 2024

Uh oh!

Uh oh!

Uh oh!

public-teleport-github-review-bot Bot commented Mar 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		if (error.name === 'RpcError') {
		const e = error as RpcError;

Conversation

gzdunek commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avatus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gzdunek commented Mar 14, 2024

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ravicious Mar 15, 2024

Choose a reason for hiding this comment

Uh oh!

gzdunek Mar 18, 2024

Choose a reason for hiding this comment

Uh oh!

ravicious Mar 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ravicious Mar 15, 2024

Choose a reason for hiding this comment

Uh oh!

gzdunek Mar 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

public-teleport-github-review-bot Bot commented Mar 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gzdunek commented Mar 12, 2024 •

edited

Loading