Closed niklasad1 closed 11 months ago
Personally I prefer option 2) but open to suggestions
I like the general idea of 2, in that we can allow for middleware which is capable of tracking whatever details matter, and then leave the disconnection outside of middleware (after all; disconnecting could happen at any point between when calls are being made etc, whereas middleware only runs when a call is made).
Would removing HTTP middleware make some basic use cases more difficult? I guess the idea is that with jsonrpsee as a Service
, you can add middleware easily on top of it and don't need to have any interface/code in jsonrpsee for setting it, which sounds reasonable!
The current service I guess would handle the HTTP/WS stuff, and I think the example above for (2) is manually writing that code in the service (or some of it) in order to add whatever custom disconnect logic into it?
I think it'd be worth experimenting with anyway! If things are a bit too tricky there are probably things we can do to make life easier too
Would removing HTTP middleware make some basic use cases more difficult? I guess the idea is that with jsonrpsee as a Service, you can add middleware easily on top of it and don't need to have any interface/code in jsonrpsee for setting it, which sounds reasonable!
I guess we should keep the HTTP middleware API because we have a bunch of nice stuff such as the CORS, host filtering stuff which is much easier to use to provide your own service handler
The current service I guess would handle the HTTP/WS stuff, and I think the example above for (2) is manually writing that code in the service (or some of it) in order to add whatever custom disconnect logic into it?
Yes and I think it's much easier to write a hyper::service_fn
than to write custom tower middleware but the jsonpsee::TowerService isn't really required when one wants custom disconnect logic just that one can re-use some configurations from the builder.
Maybe it's possible to expose another "jsonrpsee::DisconnectableTowerService" instead having a such low-level API.
I guess we should keep the HTTP middleware API because we have a bunch of nice stuff such as the CORS, host filtering stuff which is much easier to use to provide your own service handler
I guess I'd keep it for now and see what comes out of experimenting with how to add disconnect logic etc!
Maybe it's possible to expose another "jsonrpsee::DisconnectableTowerService" instead having a such low-level API.
I'd have a go at the "low level" approach you had in mind anyway and just see how it works out; when there's some real code it'll be easier to see if there are any obvious things we can do to make it simpler or encapsulate some logic away or something :)
@niklasad1 Am I right assuming that currently there's no way to access values produced by HTTP middleware in JSON-RPC methods (e.g. to get user id retrieved by authentication middleware)?
@niklasad1 Am I right assuming that currently there's no way to access values produced by HTTP middleware in JSON-RPC methods (e.g. to get user id retrieved by authentication middleware)?
Yes, for latest jsonrpsee release you are correct but you could write specific HTTP middleware to take care of that but that works only properly for HTTP JSON-RPC calls and the API is very low-level but we recently merged a specific JSON-RPC middleware to deal with that.
The intention forward is that folks can do authentication with the new middleware that we recently merged #1215 but https://github.com/paritytech/jsonrpsee/pull/1224 is needed because we didn't want to expose the entire HTTP request in the middleware itself because it's already known when HTTP connection is made.
Instead folks have to launch a hyper::service_fn
and wrap whatever details one would need from HTTP context instead that we pass it in for every RPC call, you can have a look this example that does dummy HTTP authentication.
This way it's more flexible and "jsonrpsee" doesn't have clone and bunch data that may not be used anyway.
However, other suggestions are also welcome of course.
Option 2) is now merged and the API jsonrpsee will provide
Currently jsonrpsee hasn't any specific JSON-RPC middleware interface rather than to just a logger to easily provide grafana metrics and similar usecase.
Currently jsonrpsee has:
We have a bunch of issues where folks want middleware to inspect JSON-RPC calls including HTTP specific details such as the URI, HTTP Headers and such things...
The HTTP (tower) middleware works great for pure HTTP related things but as soon as one wants to parse a JSON-RPC request, it not as nice to work with. Then in context of
WebSocket connections
it's only useful for the WebSocket upgrade handshake and after that point it's not possible to understand what's going, restrict certain calls or to disconnect peer that is misbehaving doing plenty of expensive calls.The path forward is to replace the JSON-RPC logger with a more flexible JSON-RPC specific middleware which we have pending implementation for it but that alone won't solve disconnecting peers just deny calls.
To deal with disconnecting peers I could think of the following suggestions:
This suggestion is really tricky to implement with all the state that needs to be synchronized somehow and jsonrpsee may become really complicated. It's not really clear how to couple calls with disconnect_peer.
hyper::service::fn
then HTTP middleware interface can be removed but JSONRPC specific middleware is needed.The downside with this approach is that users needs to write plenty of code themselves but should be really flexible.
Any other suggestions?
//cc @jsdw @lexnv