Skip to content
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

Migrate from GRPC to JSON RPC (with binary variant for higher performance) #495

Closed
CMCDragonkai opened this issue Nov 25, 2022 · 35 comments
Closed
Assignees
Labels
development Standard development epic Big issue with multiple subissues r&d:polykey:core activity 1 Secret Vault Sharing and Secret History Management r&d:polykey:core activity 3 Peer to Peer Federated Hierarchy

Comments

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Nov 25, 2022

Specification

Due to various issues with GRPC, and a combined effort to replace both the networking in #234 and transport agnostic RPC #249, this epic covers all the problems we have with GRPC and changes we will need to do to the RPC handling architecture.

We want to go with JSON RPC because it is going to be the most suitable for third party integration.

We can have a binary protocol format for increased performance when this is detected. Candidates are:

  • BSON
  • CBOR
  • Protobuf
  • MessagePack

We need to choose one based on benchmarks, and UI/UX of converting to our internal datasets, reliability of third party libraries, portability of parsing and generating libraries and wide-deployment.

Additional context

See all sub issues under this epic.

Tasks

  1. Fix all subtasks under this epic.
  2. Once this epic is satisfactorily done and specced, close all irrelevant issues.
  3. Our migration should address all the problems we've had with GRPC and what we have been using GRPC for.
@CMCDragonkai CMCDragonkai added development Standard development epic Big issue with multiple subissues labels Nov 25, 2022
@CMCDragonkai
Copy link
Member Author

This now takes priority over all GRPC problems.

@CMCDragonkai
Copy link
Member Author

Because we have pre-existing experience with protobuf. And we have a large corpus of protobuf definitions already. (Even though we will be reworking those with respect to JSON RPC) Then protobuf is probably a good choice for our binary alternative encoding.

However the choice of protobuf library is important too.

Our existing stack is pretty complex involving protoc, the protoc plugin for generating TS code, the google-protobufruntime library... etc.

This all seemingly can be replaced with 1 library: https://github.com/bufbuild/protobuf-es.

@CMCDragonkai
Copy link
Member Author

There are some things we need further specify above the JSON RPC spec: https://www.jsonrpc.org/specification. Because this is light on the details.

Firstly if we were to use the params, then the params should always be an array in order to match our JS functions take parameters. Meaning f({a, b}, c) would be called with params: [{a, b}, c].

Secondly the error structure doesn't quite match our serialiation atm. We can stuff our entire error serialisation into the data property, but then copy the first level message to the message (prefixed by the exception name), and also set a generic application-error code for the code. Unless it's one of the reserved errors.

Furthermore, we have to consider what usage is the notifications, and batching and request objects in relation to our streaming usage as we are currently doing.

@CMCDragonkai
Copy link
Member Author

The request object is an open spec. We could add additional properties to a request object, in order to cover the usage of metadata we have in GRPC. In GRPC there's leading and trailing metadata: #249 (comment)

But if we do with the request object, we could actually add metadata to every single request object.

@CMCDragonkai
Copy link
Member Author

The response object can also be extended with meta.

@CMCDragonkai
Copy link
Member Author

Solving this problem should also involve bringing in rxjs properly.

@CMCDragonkai
Copy link
Member Author

Still to spec out:

  1. Protobuf vs JSON muxing/demuxing, should these be separated per-stream? If so, how does one identify whether it is JSON or Protobuf in the first place, how to disambiguate?
  2. The addition of the meta property to all request objects and response objects for JSON RPC to enable passing meta, but this also means there's no such thing as "leading" or "trailing" metadata, every request/response can have metadata.
  3. The removal of "id" from requests/response objects thus everything is just a notification. The entire stream itself can have an ID though, and we do have stream IDs as well connection IDs. 1 connection ID - multiple stream IDs... and we end up not following the JSON RPC spec to the letter.

@CMCDragonkai CMCDragonkai mentioned this issue Dec 8, 2022
14 tasks
@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Jan 3, 2023

I've recently created a WIP QUICStream in js-quic, details in this comment: MatrixAI/js-quic#1 (comment).

Basically web streams doesn't have a native "duplex" stream. The only closest is the ReadableWritablePair.

So the first order of business is to develop a function that maps;

ReadableWritablePair<Uint8Array, Uint8Array>

To

ReadableWritablePair<JSON, JSON>

Where JSON is the JSON RPC message format. Although we have some requirements regarding leading headers and trailing headers discussed in #249 (#249 (comment)).

@CMCDragonkai
Copy link
Member Author

@tegefaulkes start prototyping this implementation with an new PR targetting this epic. I think having a new rpc directory as opposed to the grpc directory can be useful.

@CMCDragonkai
Copy link
Member Author

For collection handlers see #237.

For most handlers see #450.

@CMCDragonkai
Copy link
Member Author

@tegefaulkes is this the larger epic? Please link all new issues created involving RPC migration here including the recent issues you created.

@tegefaulkes
Copy link
Contributor

I linked the new RPC issues to this epic.

@CMCDragonkai
Copy link
Member Author

Also pending is single class files should be using default export. The migration of client handlers kept the GRPC style handlers. But with agent handler migration we should switch over properly.

@CMCDragonkai
Copy link
Member Author

With the latest merges, the demos shouild still work right? CLI demo and agent to agent demo we sent to Mercury.

@CMCDragonkai CMCDragonkai self-assigned this Apr 6, 2023
@CMCDragonkai
Copy link
Member Author

I added MatrixAI/js-rpc#1 to this epic too.

@CMCDragonkai
Copy link
Member Author

Just a clarification, do we actually have a binary protocol atm using protobuf? Or do we just switch from JSONRPC to raw binary stream such as during file upload?

@tegefaulkes can you expand on the relevant handlers and where this is applied.

@tegefaulkes
Copy link
Contributor

We don't have a protocol per say. But we can send anything over the raw streams. That could be protobuf, BISON or whatever.

AFAIK the demo should still work. All the relevant tests are still working.

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Apr 6, 2023

Can you explain how this works in the context of file uploading? Explain in reference to one of the client service handlers. What happens during the streaming?

It should be client streaming right? Then initial message is JSON, then subsequently it's just chunks of the raw file being uploaded? How does one signal end of file? Just by ending stream? I need more clarity on this.

@tegefaulkes
Copy link
Contributor

It's down to implementation of the handlers. but you'd include metadata in the header message and then just start writing the contents of the file to the stream. and then end the writable of the RPC stream to signal the end. You can if you wanted to build an extra layer of protocol over this. You will also have to deal with Endianness if you don't serialise the data with something like protobuf but thats a normal concern for raw binary streams.

@CMCDragonkai
Copy link
Member Author

Ok so how does it currently work with respect to pk secrets create or pk secrets get?

What are the handlers currently implemented to do?

Are we dealing with endianess? Is it possible that the client side may be a different endianess compared to the server side? Like if we are on an intel machine and send it to a different kind of machine could that cause problems?

@tegefaulkes
Copy link
Contributor

It's possible? I'm not sure, its definitely something to look out for.

As for the handlers. I think anything that would be sending data such as secrets is implemented pretty simply right now. create and get are sending the secrets contents in a binary string and not even streaming it. So It needs to be changed to do streaming.

@CMCDragonkai
Copy link
Member Author

Ok so we need to change them to streaming and we also need to change them to using protobuf which at the very least would fix any issues with endianness.

This reminds me of the HTTP style uploads which switch from http request to multipart upload format.

In this sense we should do something very similar. Have a parameter that indicates the type of request.

@tegefaulkes
Copy link
Contributor

New issue at MatrixAI/js-rpc#2.

@tegefaulkes
Copy link
Contributor

I think this is what you wanted to be tagged in @CMCDragonkai

tegefaulkes added a commit that referenced this issue May 23, 2023
* Related #512
* Related #495

[ci skip]
tegefaulkes added a commit that referenced this issue May 23, 2023
* Related #512
* Related #495

[ci skip]
tegefaulkes added a commit that referenced this issue May 25, 2023
* Related #512
* Related #495
* Related #234

[ci skip]
tegefaulkes added a commit that referenced this issue Jun 5, 2023
* Related #512
* Related #495

[ci skip]
tegefaulkes added a commit that referenced this issue Jun 5, 2023
* Related #512
* Related #495

[ci skip]
tegefaulkes added a commit that referenced this issue Jun 5, 2023
* Related #512
* Related #495
* Related #234

[ci skip]
@CMCDragonkai
Copy link
Member Author

Can #525 address this as well? It's one big QUIC integration plus RPC migration.

There's alot of subissues under this epic that we could address, but some of it can be pushed to later.

@tegefaulkes
Copy link
Contributor

I could but I'd rather not 'kitchen sink' PRs.

tegefaulkes added a commit that referenced this issue Jul 7, 2023
* Related #512
* Related #495

[ci skip]
tegefaulkes added a commit that referenced this issue Jul 7, 2023
* Related #512
* Related #495

[ci skip]
tegefaulkes added a commit that referenced this issue Jul 7, 2023
* Related #512
* Related #495
* Related #234

[ci skip]
@CMCDragonkai CMCDragonkai added r&d:polykey:core activity 1 Secret Vault Sharing and Secret History Management r&d:polykey:core activity 3 Peer to Peer Federated Hierarchy labels Jul 9, 2023
tegefaulkes added a commit that referenced this issue Jul 10, 2023
* Related #512
* Related #495

[ci skip]
tegefaulkes added a commit that referenced this issue Jul 10, 2023
* Related #512
* Related #495

[ci skip]
tegefaulkes added a commit that referenced this issue Jul 10, 2023
* Related #512
* Related #495
* Related #234

[ci skip]
@CMCDragonkai
Copy link
Member Author

After some discussion regarding MatrixAI/js-rpc#2. It is now understood that we don't need to embed protobuf or cbor or some binary variant into our RPC.

The RPC can already just drop down to byte level streams (this functionality is tracked in MatrixAI/js-rpc#2). Once it is dropped to the binary level, it is up to the RPC caller and handlers to decide to use whatever data format they want. If they wish to use protobuf they can, if they wish to use CBOR they can. But the RPC system itself doesn't require this functionality by default.

There are 2 usecases for using Protobuf/CBOR:

  1. When you need to chunk up binary data, and expect the other side to be able to stream process the chunks. This requires a message-framing capability, and so both sides are likely to use a stream generator and stream parser to work with them. The byte stream provided by the RPC system does not have any message framing capability in it, unless you consider 1 byte to be a message frame.
  2. When you are working with mixed binary and structured data. So like structured messages that have somethings that is binary. However we are currently using base64url encoded data mostly. ATM, there are actually 2 binary encodings we use with JSON. One is the the Buffer.toJSON way, where we produce a { type: 'Buffer', data: [1, 2, 3] }, and the other is the base64url encoded data that is used as part of JOSE tokens. The former is used more for database operations, while the latter is currently only used for tokens.

For usecase 1, we should prefer to use protobuf or cbor, and not mix the usage. Protobuf has more wider acceptance, but CBOR has more support for the native types of JS. CBOR is likely to be used in the DB when MatrixAI/js-db#58 is done.

For usecase 2, for simplicity I believe we should migrate to using the base64url style rather than the Buffer.toJSON way, and then it should be standardised as part of our development documentation. It's lot more space-wise efficient, and is more expected for RPC calls.

tegefaulkes added a commit that referenced this issue Jul 20, 2023
* Related #512
* Related #495

[ci skip]
tegefaulkes added a commit that referenced this issue Jul 20, 2023
* Related #512
* Related #495
* Related #234

[ci skip]
@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Aug 1, 2023

As per the discussion in MatrixAI/js-ws#3

Do note that there's quite a few levels of encapsulation here for websocket transport.

  1. TCP/TLS - where MTLS is available
  2. HTTP handshake - GET request, and 101 response
  3. Websocket binary frames
  4. Payload protocol - JSON-RPC
  5. IF you end up dropping to binary raw stream, at this point JSON-RPC is the handshake, and you can now use protobuf or cbor or whatever.

Error management should be specified too.

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Aug 1, 2023

There should be tests for uploading and downloading files using the raw stream from Polykey client to Polykey agent, and then synchronising those vaults to agent to agent.

In both hops, they should be dropping down to raw streams.

@CMCDragonkai
Copy link
Member Author

This is done with rpc and ws integrated. No more GRPC is in PK.

@CMCDragonkai
Copy link
Member Author

Note I removed the pagination issue out of this. Those are usability issues to be applied separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development epic Big issue with multiple subissues r&d:polykey:core activity 1 Secret Vault Sharing and Secret History Management r&d:polykey:core activity 3 Peer to Peer Federated Hierarchy
Development

No branches or pull requests

2 participants