Wrap gRPC calls in `createClient` to allow passing the errors over the context bridge #39230

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

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

The main task of this PR is conversion from grpc-js client to protobuf-ts and then wrapping the calls, so their errors can be inspected.
Changing the client allowed us to remove a lot of the boilerplate in createClient.ts, we also no longer need mapUsageEvent.ts and createFileTransferStream.ts.

…(instead of `any`) This is needed for wrapping calls in `createClient`.

…h call

…s code

ravicious · 2024-03-12T14:28:56Z

.gitattributes

@@ -8,3 +8,4 @@ docs/theme/js/theme.js linguist-vendored=false
 *_pb.d.ts linguist-generated
 *_pb.ts linguist-generated
 *_pb.grpc-*.ts linguist-generated
+*_pb.client.ts linguist-generated


FYI, you'll have to update the bot as well, see gravitational/shared-workflows#220.

Just a reminder about updating the bot.

Yeah, I will open a PR soon.

gravitational/shared-workflows#234

ravicious

The code looks good! I'd like to test the app though, so I'll continue the review tomorrow.

web/packages/teleterm/src/services/tshd/createClient.ts

ravicious · 2024-03-14T14:30:26Z

web/packages/teleterm/src/services/tshd/createClient.ts

+
+            // Following cases should never happen but just in case?
+            case api.PasswordlessPrompt.UNSPECIFIED:
+              stream.requests.complete();


Is complete the same as cancel?

Not really (but this doesn't seem to make a difference here).
complete: Complete / close the stream. Can only be called if there is no pending send(). No send() should follow this call.
cancel: Cancel the ongoing call. Results in the call ending with a CANCELLED status, unless it has already ended with some other status.

I think this doesn't matter here anyway, because just after calling these functions we do return reject(new Error(..), so any gRPC response is discarded.
However, I feel like this code is (and was) prone to race conditions. There is no guarantee that onMessage/onError handler is called after our rejection.

Ideal solution for this would be to add AbortController inside the handler and call abort for the cases above. Then combine its abortSignal with the abort signal from the callsite with AbortSignal.any.
Thanks to that, all errors would go the same route.
There two problems with this tho:

AbortSignal.any is supported since Node.js 20.3 (but we update soon).

The abort reason is not propagated, the only error we get is 'Cancelled on client' :(

So I think we have to stay with stream.requests.complete()

Oh I see what you mean about AbortSignal.any.

Is "Cancelled on client" generated by the library or by us? We could probably improve the UX a little bit by checking if the error is an abort error when displaying the error in the modal. But that's for another PR I suppose.

Is "Cancelled on client" generated by the library or by us?

It's generated by the library.

We could probably improve the UX a little bit by checking if the error is an abort error when displaying the error in the modal.

Hmm, but what that would give us? I guess, only a different error message?
I think that ideally it could work like this:

const internalAbortController = new AbortController(); const anySignal = Abort.any([internalAbortController.signal, abortSignal]); const stream = tshd.loginPasswordless({ abort: anySignal, }); stream.responses.onError(function (err: Error) { let e: Error | TshdRpcError; if (isTshdRpcError(err) && err.code === 'CANCELLED') { e = { ...err, message: `${err.message}: ${anySignal.reason}`, }; } else { e = err; } reject(e); })

But I would need to update electron & node first.

Yeah, it'd mostly be the error message. "Cancelled on client" sounds much worse than not showing the error at all if we know that the login process was just cancelled by the user.

We can think of it. For now, I will keep it as is, since we have this behavior from the beginning.

web/packages/teleterm/src/services/tshd/createClient.ts

ravicious · 2024-03-14T14:38:41Z

web/packages/teleterm/src/services/tshd/errors.ts

-  return (error.message as string)?.includes('access denied');
+import { isTshdRpcError } from './cloneableClient';
+
+export function isAccessDeniedError(error: unknown): boolean {


We should probably deprecate all functions in this file and make callsites use something like isTshdRpcError(error, 'NOT_FOUND'), where the second argument belongs to a discriminated union.

But this is not a PR which addresses the errors, we're focused on the gRPC client instead, right?

isTshdRpcError(error, 'NOT_FOUND')

That's a nice idea, I didn't think of it. But in general, I wanted to limit the number of changes and only updated the implementation here.

We can switch to isTshdRpcError(error, 'NOT_FOUND') in a separate PR.

ravicious

I tested passwordless login (by cancelling every step), headless auth and access requests. Everything appears to be in order.

(Though, sometimes headless requests stop working and I can't pinpoint why. As if tshd stopped listening to them. idk if it's something you perhaps ran into as well.)

ravicious · 2024-03-15T15:09:25Z

.gitattributes

@@ -8,3 +8,4 @@ docs/theme/js/theme.js linguist-vendored=false
 *_pb.d.ts linguist-generated
 *_pb.ts linguist-generated
 *_pb.grpc-*.ts linguist-generated
+*_pb.client.ts linguist-generated


Just a reminder about updating the bot.

ravicious · 2024-03-15T15:19:09Z

web/packages/teleterm/src/services/tshd/createClient.ts

+
+            // Following cases should never happen but just in case?
+            case api.PasswordlessPrompt.UNSPECIFIED:
+              stream.requests.complete();


Oh I see what you mean about AbortSignal.any.

Is "Cancelled on client" generated by the library or by us? We could probably improve the UX a little bit by checking if the error is an abort error when displaying the error in the modal. But that's for another PR I suppose.

* 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`

* 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 teleport-connect Issues related to Teleport Connect. no-changelog Indicates that a PR does not require a changelog entry labels Mar 12, 2024

gzdunek mentioned this pull request Mar 12, 2024

Clone the tshd gRPC client to allow inspecting errors #39229

Merged

github-actions bot requested review from ravicious and rudream March 12, 2024 12:05

github-actions bot added size/lg ui labels Mar 12, 2024

gzdunek requested review from avatus and removed request for rudream March 12, 2024 12:06

gzdunek added 5 commits March 12, 2024 13:19

Regenerate protos with the protobuf-ts client

0c8ef85

Enable strictBindCallApply so .bind() results have correct types …

aad4a7d

…(instead of `any`) This is needed for wrapping calls in `createClient`.

Use protobuf-ts client instead of grpc-js one

ed50634

Convert createClient to use protobuf-ts response style, clone eac…

dba0633

…h call

Switch callsites to cloneAbortSignal

f7cf8ac

gzdunek force-pushed the grzdunek/wrap-grpc-calls branch from 37232fd to f7cf8ac Compare March 12, 2024 12:19

Replace error.message checks with a proper check on the error statu…

3c0d7a4

…s code

ravicious reviewed Mar 12, 2024

View reviewed changes

ravicious self-requested a review March 12, 2024 14:31

avatus approved these changes Mar 12, 2024

View reviewed changes

gzdunek added 3 commits March 14, 2024 12:16

Merge branch 'gzdunek/cloneable-client' into grzdunek/wrap-grpc-calls

72956af

Clone the entire client instead of each method separately

1f02ef5

Correct the error cause in ResourceSearchError

2e8608a

ravicious reviewed Mar 14, 2024

View reviewed changes

ravicious self-requested a review March 14, 2024 14:42

Remove params.sortBy defaults, always set startKey to string

d2c79a5

ravicious approved these changes Mar 15, 2024

View reviewed changes

gzdunek added 2 commits March 18, 2024 10:01

Merge branch 'gzdunek/cloneable-client' into grzdunek/wrap-grpc-calls

343d53a

Use a simpler type for reportUsageEvent

e5982b3

gzdunek mentioned this pull request Mar 18, 2024

Ignore protobuf-ts client files during size check gravitational/shared-workflows#234

Merged

gzdunek merged commit ed2029c into gzdunek/cloneable-client Mar 18, 2024
35 of 36 checks passed

gzdunek deleted the grzdunek/wrap-grpc-calls branch March 18, 2024 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap gRPC calls in `createClient` to allow passing the errors over the context bridge #39230

Wrap gRPC calls in `createClient` to allow passing the errors over the context bridge #39230

gzdunek commented Mar 12, 2024

ravicious Mar 12, 2024

ravicious Mar 15, 2024

gzdunek Mar 18, 2024

gzdunek Mar 18, 2024

ravicious left a comment

ravicious Mar 14, 2024

gzdunek Mar 14, 2024

ravicious Mar 15, 2024

gzdunek Mar 18, 2024

ravicious Mar 18, 2024

gzdunek Mar 18, 2024

ravicious Mar 14, 2024

gzdunek Mar 14, 2024

ravicious left a comment

ravicious Mar 15, 2024

ravicious Mar 15, 2024

Wrap gRPC calls in createClient to allow passing the errors over the context bridge #39230

Wrap gRPC calls in createClient to allow passing the errors over the context bridge #39230

Conversation

gzdunek commented Mar 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravicious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravicious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wrap gRPC calls in `createClient` to allow passing the errors over the context bridge #39230

Wrap gRPC calls in `createClient` to allow passing the errors over the context bridge #39230