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

Attach a context to requests #171

Closed
wants to merge 2 commits into from
Closed

Attach a context to requests #171

wants to merge 2 commits into from

Conversation

Farenjihn
Copy link

Motivation

Being able to access various things (ip address of the peer or other things) in server interceptors / future middleware implementations in tonic. This is mainly for logging, access control, etc...

This would solve #71.

I need this feature so I went ahead and implemented it in the way described below. It may not fit with what tonic intends to provide in the long term but since it's done, I may as well create this PR.

The implementation has some overlap with #85, since it's inspired by it, but tries to be a bit more flexible.

Solution

Create a Ctx struct which is populated according to configuration passed to the Server builder. This struct is added to the Request's extensions so it becomes available to interceptors.

Here's what the current usage looks like:

let mut builder = Server::builder();
builder
    .with_ctx_field(CtxField::PeerAddr) // Add the PeerAddr field in the Ctx struct
    .interceptor_fn(|svc, req| {
        let ctx: &Ctx = req.extensions().get().unwrap();
        // do something
    })
    .add_service(/* service */)
    .serve(/* addr */)
    .await?;

The context struct can be extended with more fields, which can also be gated behind features. This PR only adds a field to read the peer's ip address.

Open questions

Provided the current implementation is acceptable for merging:

  1. Would it make sense to do something similar for clients ?
  2. @jen20 If this gets merged, how should we proceed for Allow access to peer identity in a gRPC Server Interceptor #85 ?

Let me know what you think.

@LucioFranco
Copy link
Member

Hey! Thank you for this! I have another proposal #177 and we've been working on this in #85. I will see if I can get this done before 0.1 release so you can achieve the same thing. I'd like to see the server do this implicitly instead of needing to pass a Ctx.

@Farenjihn
Copy link
Author

Closing since there is a different plan

@Farenjihn Farenjihn closed this Dec 12, 2019
@LucioFranco
Copy link
Member

Thank you <3

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.

2 participants