Client RPC migration #509

tegefaulkes · 2023-02-27T01:28:56Z

Description

This PR deals with migrating the clientRPC handlers and code to using the new Agnostic RPC system.

It consists of taking the existing handler code within client/services and transplanting it into the new RPC system from #249 .

pagination

Any handlers that stream data need to support the pagination parameters and propagate them. The required parameters are seek: id, number: number, order: 'asc'|'desc'. Most code that streams an output with a generator already support pagination. But in some cases where it is not already supported it needs to be updated. As for the handler parameters, the message should include the following

type paginationMessage = {
id: string,
order: 'asc' | 'desc',
number: number,
}

Binary data

Currently there are two cases where sending binary data is applicable. The IDs such as NodeID, or VaultId. These are converted to an encoded string form just for readability of the message. Using the NodeIdEncoded and VaultIdEncoded for of the Ids.

The 2nd case is file contents. Currently these are converted to a binary string when provided within a message using Buffer.toString('binary'). This is pretty limiting to small amounts of data so it is subject to change. If we need to send larger files we need to stream it using a raw handler to implement it as a raw binary stream. This has complications such it not supporting any middleware so we need to implement authentication logic withing the raw handler.

Standard message structure

To standardise the message structure and types across all messages we are leveraging the typescripts type system. Since it's pretty powerful we can compose messages and message types together effectively.

For example we have basic type messages that can be composed to make more complex messages

type NodeIdMessage = {  
  nodeIdEncoded: NodeIdEncoded;  
};

export type AddressMessage = {
  host: string;
  port: number;
};

type NodeAddressMessage = NodeIdMessage & AddressMessage;

type NodesAddMessage = NodeAddressMessage & {  
  force?: boolean;  
  ping?: boolean;  
};

Issues Fixed

Related Transport Agnostic RPC #249
Related Transport Agnostic RPC #249
Fixes fix: node discovery warning message spam #514

Tasks

Final checklist

ghost · 2023-02-27T01:29:58Z

👇 Click on the image for a new way to code review

Legend

tegefaulkes · 2023-03-01T00:07:58Z

Re-based on staging after #506 merged. Staging is now the base branch for this PR.

tegefaulkes · 2023-03-03T03:56:16Z

Only two handlers have been converted to server streams so far.

Nodes get all
Notifications read

CMCDragonkai · 2023-03-03T04:16:22Z

Only two handlers have been converted to server streams so far.

Nodes get all

Notifications read

Should go into the task list so it's clearer.

tegefaulkes · 2023-03-07T01:04:56Z

While migrating tests I notices that the vaults clone, pull and scan handlers don't have tests. I'll need to create some for them.

CMCDragonkai · 2023-03-07T01:27:54Z

Those 2 are particularly tricky due to the GRPC hacks we had to do to allow things like git pull to work.

With the new RPC system, they have to simulate a stream for the git operations to run over.

Did you manage to simplify the whole architecture? That is a duplex stream that "switches" over to binary data, and then passes Git's HTTP protocol transparently over it?

There was also random hacks for getting exceptions out of isogit so that it would turn into GRPC exceptions, this should also be considered too.

CMCDragonkai · 2023-03-07T01:28:49Z

BTW the above is probably more related to the agent service, and not so much client service.

BUT the performance of vault cloning/sync was so bad previously. Please look into benchmarking this so we can have fast cloning and sync, and it should be much faster now without so many layers of abstraction once QUIC and our new RPC is fully integrated.

CMCDragonkai · 2023-03-07T07:37:31Z

All the fixmes are going to addressed in this PR?

CMCDragonkai · 2023-03-09T00:17:03Z

Hey @tegefaulkes remember address my comments above too. If there needs to be an issue for the agent migration and git tunneling, please remember to create it too.

tegefaulkes · 2023-03-09T06:10:07Z

I've created two new issues.
#512
#511

tegefaulkes · 2023-03-09T06:11:13Z

As for the FIXME's, there are two that are relevant this PR. one will be addressed and the 2nd one is addressed by #511 now so i'll remove it.

tegefaulkes · 2023-03-09T06:21:57Z

The ErrorPolykeyRemote expects to have some metadata about the connection. This includes nodeId, host port and method name. The problem is we don't have this information in the scope where this error is created.

So I have two options, I make this data optional so I don't have to fill it in. Or I add some way to provide this information to the RPCClient. for each call.

We should have the information available but it doesn't feel right that the RPCClient should have this information since it's an abstraction above any networking. If I had to add it in then the streamPairCreateCallback should be the thing providing it.

tegefaulkes · 2023-03-13T04:07:11Z

Just a note, when generating test data that needs to be stringified and parsed as JSON. Any values that are -0 become 0 and anything undefined will be removed. This has the effect of a round trip of stringify and parsing a JSON object will sometimes not be equal. This can result in random test failures.

CMCDragonkai · 2023-03-13T04:45:13Z

I know that fast check generators should have something dedicated to JSON. Not all JS objects is valid JSON. 12 Mar 2023 21:07:25 Brian Botha ***@***.***>:

…

Just a note, when generating test data that needs to be stringified and parsed as JSON. Any values that are *-0* become *0* and anything *undefined* will be removed. This has the effect of a round trip of stringify and parsing a JSON object will sometimes not be equal. This can result in random test failures. — Reply to this email directly, view it on GitHub[#509 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAE4OHISUKBYWCXDATHZNZLW32MPVANCNFSM6AAAAAAVIZTAVI]. You are receiving this because you commented.[Tracking image][https://github.com/notifications/beacon/AAE4OHLH2YKJLYMW64XI3TTW32MPVA5CNFSM6AAAAAAVIZTAVKWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTSXLF2QM.gif]

tegefaulkes · 2023-03-13T05:43:22Z

I think I'll leave removing the GRPC code for the agent migration. It's much easier to remove the whole GRPC and network domain at that stage. It does mean we have some failing GRPC tests hanging about.

Otherwise this should be good to merge after some clean up.

- created agent handlers and tests - adding gestalts handlers - migrating gestalts handlers - migrating identities handlers - adding identities handlers - adding identities handlers - adding keys handlers - migrating nodes handlers - migrating notifications handlers - migrating vaults handlers - migrating vaults handlers [ci skip]

- migrating keys tests - migrating nodes tests - migrating notifications tests - migrating notifications tests - migrating vaults tests [ci skip]

- removing client GRPC and using agnostic RPC - updating agent bin code - updating identities bin code - updating keys bin code - updating nodes bin code - migrating secrets bin code - migrating vaults bin code [ci skip]

[ci skip]

tegefaulkes · 2023-03-13T06:30:09Z

Cleaned up commit history

tegefaulkes · 2023-03-13T06:31:35Z

This should be good to merge now.

tegefaulkes · 2023-03-13T06:50:44Z

I think #510 is the next thing for me to work on. That is assuming @CMCDragonkai is still working on the Quic stuff otherwise I can start on that. I can make a start on #512 but I can't test it works without Quic working. #511 is low priority, things still work without it. We just miss out on some expected behaviour with key changes.

CMCDragonkai · 2023-03-13T07:05:39Z

I won't be getting to QUIC anytime soon. So you can start on #510.

tegefaulkes · 2023-03-24T02:27:30Z

Cleaning up history now. Almost ready for merge.

… injected into the `PolykeyAgent`. [ci skip]

[ci skip]

Previously the `clientManifest` depended on the whole code base due to the handlers. This has now been seperated out, so it only depends on types. [ci skip]

…d one for now [ci skip]

[ci skip]

…ient` [ci skip]

… better for debugging

… `detail` property, made `RPCServer` extend `EventTarget`

…etPort()` and other fixes [ci skip]

[ci skip]

…il reconstruction via transform stream, and reverse pair propagation of cancellation event

[ci skip]

tegefaulkes · 2023-03-24T02:53:21Z

This is ready to merge. All that is left to do is house keeping.

CMCDragonkai · 2023-03-24T03:30:47Z

What housekeeping?

Enabling CI.

tegefaulkes · 2023-03-24T03:35:54Z

I just had to update or create issues before I could resolve the last review comment. it's done now.

I checked and there is no docs changes commited.

I've re-enabled CI so this should be good to merge any time now.

tegefaulkes self-assigned this Feb 27, 2023

tegefaulkes force-pushed the feature-client_migration branch from ecf309c to b734d2d Compare February 28, 2023 08:12

tegefaulkes changed the base branch from feature-websocket_client to staging March 1, 2023 00:04

tegefaulkes force-pushed the feature-client_migration branch from b734d2d to 9258456 Compare March 1, 2023 00:07

This was referenced Mar 9, 2023

Updating TLSconfig for WebSocketServer #511

Closed

Agent RPC migration #512

Closed

tegefaulkes added 4 commits March 13, 2023 17:28

tests: migrating handler tests

ed608e2

- migrating keys tests - migrating nodes tests - migrating notifications tests - migrating notifications tests - migrating vaults tests [ci skip]

feat: updating client RPC usage to use agnostic RPC

7e050e8

- removing client GRPC and using agnostic RPC - updating agent bin code - updating identities bin code - updating keys bin code - updating nodes bin code - migrating secrets bin code - migrating vaults bin code [ci skip]

fix: fixing failing tests

cccc5c5

[ci skip]

tegefaulkes force-pushed the feature-client_migration branch from 9b9b0b2 to cccc5c5 Compare March 13, 2023 06:29

tegefaulkes and others added 18 commits March 24, 2023 13:35

fix: rpcServerClient and webSocketServerClient are not dependency…

95b772d

… injected into the `PolykeyAgent`. [ci skip]

fix: rpcClient is now dependency injected into PolykeyClient

b00b9c8

[ci skip]

fix: changed JsonRpc to JSONRPC

da925ee

style: spacing between handler types

95c00dc

fix: clientManifest avoids needless dependencies

9415794

Previously the `clientManifest` depended on the whole code base due to the handlers. This has now been seperated out, so it only depends on types. [ci skip]

fix: outputFormatter handles ErrorPolykeyRemote as well as the ol…

9df3302

…d one for now [ci skip]

fix: renamed ErrorRPCClient to ErrorClient

bc1ee3f

[ci skip]

fix: removing a disabled test

86d1cb1

[ci skip]

fix: updated docblocks and parameter names for RPCServer and `RPCCl…

c253996

…ient` [ci skip]

chore: removed useless comments about XML RPC error codes

a709b91

fix(rpc): await the handler promises in the async generators, this is…

6a12022

… better for debugging

fix(rpc): moved RPCErrorEvent to rpc/events, collapsed Error to…

ae5816b

… `detail` property, made `RPCServer` extend `EventTarget`

chore: expanding RPCServer commentary

428c170

fix: made WebSocketServer host and port into getHost() and `g…

7352f1f

…etPort()` and other fixes [ci skip]

chore: updating doc comments for RPC message types

a2ae510

[ci skip]

fix: change of head, tail destructuring for raw handlers, head and ta…

8e19b76

…il reconstruction via transform stream, and reverse pair propagation of cancellation event

fix: RPCServer handling output stream cancellation

b0876c8

[ci skip]

fix: cleaning up temp errors in websocket domain

a6f69f2

[ci skip]

tegefaulkes force-pushed the feature-client_migration branch from 73b162f to 043066d Compare March 24, 2023 02:35

tegefaulkes mentioned this pull request Aug 18, 2023

Make rpc handlers abstract arrow function properties. MatrixAI/js-rpc#5

Closed

fix: handler receives signal if output stream fails

60e3d13

Enabling CI.

tegefaulkes force-pushed the feature-client_migration branch from 043066d to 60e3d13 Compare March 24, 2023 03:34

tegefaulkes merged commit 4e2ab40 into staging Mar 24, 2023

This was referenced Aug 11, 2023

RPC Raw Streams for Binary Data MatrixAI/js-rpc#2

Closed

Use default exports for handler classes #520

Closed

More complex error codes logic for closing WebSocketConnections/WebSocketStreams MatrixAI/js-ws#4

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client RPC migration #509

Client RPC migration #509

tegefaulkes commented Feb 27, 2023 •

edited

Loading

ghost commented Feb 27, 2023 •

edited by ghost

Loading

Legend

tegefaulkes commented Mar 1, 2023 •

edited

Loading

tegefaulkes commented Mar 3, 2023

CMCDragonkai commented Mar 3, 2023

tegefaulkes commented Mar 7, 2023 •

edited

Loading

CMCDragonkai commented Mar 7, 2023

CMCDragonkai commented Mar 7, 2023

CMCDragonkai commented Mar 7, 2023

CMCDragonkai commented Mar 9, 2023

tegefaulkes commented Mar 9, 2023

tegefaulkes commented Mar 9, 2023

tegefaulkes commented Mar 9, 2023

tegefaulkes commented Mar 13, 2023

CMCDragonkai commented Mar 13, 2023 via email

tegefaulkes commented Mar 13, 2023

tegefaulkes commented Mar 13, 2023

tegefaulkes commented Mar 13, 2023

tegefaulkes commented Mar 13, 2023

CMCDragonkai commented Mar 13, 2023

tegefaulkes commented Mar 24, 2023

tegefaulkes commented Mar 24, 2023

CMCDragonkai commented Mar 24, 2023

tegefaulkes commented Mar 24, 2023

Client RPC migration #509

Client RPC migration #509

Conversation

tegefaulkes commented Feb 27, 2023 • edited Loading

Description

pagination

Binary data

Standard message structure

Issues Fixed

Tasks

Final checklist

ghost commented Feb 27, 2023 • edited by ghost Loading

Legend

tegefaulkes commented Mar 1, 2023 • edited Loading

tegefaulkes commented Mar 3, 2023

CMCDragonkai commented Mar 3, 2023

tegefaulkes commented Mar 7, 2023 • edited Loading

CMCDragonkai commented Mar 7, 2023

CMCDragonkai commented Mar 7, 2023

CMCDragonkai commented Mar 7, 2023

CMCDragonkai commented Mar 9, 2023

tegefaulkes commented Mar 9, 2023

tegefaulkes commented Mar 9, 2023

tegefaulkes commented Mar 9, 2023

tegefaulkes commented Mar 13, 2023

CMCDragonkai commented Mar 13, 2023 via email

tegefaulkes commented Mar 13, 2023

tegefaulkes commented Mar 13, 2023

tegefaulkes commented Mar 13, 2023

tegefaulkes commented Mar 13, 2023

CMCDragonkai commented Mar 13, 2023

tegefaulkes commented Mar 24, 2023

tegefaulkes commented Mar 24, 2023

CMCDragonkai commented Mar 24, 2023

tegefaulkes commented Mar 24, 2023

tegefaulkes commented Feb 27, 2023 •

edited

Loading

ghost commented Feb 27, 2023 •

edited by ghost

Loading

tegefaulkes commented Mar 1, 2023 •

edited

Loading

tegefaulkes commented Mar 7, 2023 •

edited

Loading