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

middleware: Implement proxy URI paths to RPC methods #859

Merged
merged 12 commits into from
Aug 24, 2022
Merged

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Aug 22, 2022

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 proxies
GET/ path requests to the specified RPC method calls.

Request

The GET /path requests are modified into valid POST requests for
calling 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.

@lexnv lexnv requested a review from a team as a code owner August 22, 2022 10:40
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
lexnv added 3 commits August 22, 2022 13:52
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>
http-server/src/lib.rs Outdated Show resolved Hide resolved
}
}

impl<S> Service<Request<Body>> for ProxyRequest<S>
Copy link
Member

@niklasad1 niklasad1 Aug 22, 2022

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?

Copy link
Collaborator

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 :)

Copy link
Contributor Author

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! 😄

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"))
Copy link
Collaborator

@jsdw jsdw Aug 22, 2022

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 :)

lexnv added 6 commits August 22, 2022 16:41
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>
http-server/Cargo.toml Outdated Show resolved Hide resolved
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, LGTM

@lexnv lexnv self-assigned this Aug 22, 2022
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Copy link
Collaborator

@jsdw jsdw left a 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 :))

@lexnv lexnv merged commit 7eb5d47 into master Aug 24, 2022
@lexnv lexnv deleted the 765_proxy_layer branch August 24, 2022 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[http server]: health_api is rather confusing because it's not exclusive for /health
3 participants