-
Notifications
You must be signed in to change notification settings - Fork 174
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
middleware: Implement proxy URI paths to RPC methods #859
Conversation
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
} | ||
} | ||
|
||
impl<S> Service<Request<Body>> for ProxyRequest<S> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it's also possible for third parties to construct their own middleware service with specific logic such as #811?
i.e, to parse params from the URI and pass the parsed params call the method with the parsed params instead of params in the JSON-RPC call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I don't see why not!
A ProxyPostRequest
middleware could exist for instance which takes args in the body, or this one could decode named params from querystring or whatever else :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, users can create a new layer extract the params from the request's URI for sure, and pass it down similar to how we do here! 😄
tests/tests/helpers.rs
Outdated
let middleware = tower::ServiceBuilder::new().layer(cors); | ||
let middleware = tower::ServiceBuilder::new() | ||
// Proxy `GET /health` requests to internal `system_health` method. | ||
.layer(ProxyRequestLayer::new("/health", "system_health")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this; nice and simple to use, and more general purpose :)
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
…are` Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, LGTM
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great; the boxing is much tidier IMO, and the code in general a solid improvement on havign the health stuff integrated :)
(it did occur to me that it adds a box for every single request, not just health ones, but I guess I still wouldn't worry unless benchmarks suffer; I have a feeling 1 box won't be noticable :))
This PR moves the internal logic of system health API to a
custom tower layer implementation.
The core of this implementation is the
ProxyRequst
, which proxiesGET/ path
requests to the specified RPC method calls.Request
The
GET /path
requests are modified into validPOST
requests forcalling the RPC method. This middleware adds appropriate headers to the
request, and completely alters the request
BODY
.Response
The response of the RPC method is stripped down to contain only the method's
response, removing any RPC 2.0 spec logic regarding the response's body.
Closes #765.