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

transport: expose connection metadata #71

Closed
domodwyer opened this issue Oct 12, 2019 · 4 comments · Fixed by #186
Closed

transport: expose connection metadata #71

domodwyer opened this issue Oct 12, 2019 · 4 comments · Fixed by #186
Milestone

Comments

@domodwyer
Copy link
Contributor

Feature Request

Crates

  • tonic

Motivation

Both the C++ gRPC client, and the Go client (and probably more) expose the peer address and other connection metadata such as the mTLS certificate used by the client - these features combined are great for certificate-based access control and request source filtering / audit logs / metrics / etc.

It looks like there was a brief discussion for the C++ implementation on how to expose the peer address in a way that could be used across multiple transports (both IP and unix domain sockets are particularly common) that would be wise to take into account too - they ended up returning a string URI for each transport. The Go implementation handles this nicely using the net.Addr abstraction in the standard library that covers various transports.

Proposal

Add a peer-like field/method to the Request struct.

Perhaps we would express the different transports as enum variants with their associated addresses? Something like:

enum PeerAddr {
    Tcp(std::net::SocketAddr),
    Unix(std::os::unix::net::SocketAddr),
}

I'm not too sure what is best for the mTLS certificate as #47 points out (and #48 fixes), attempting to abstract away the different TLS types for each backend into a common type is probably not the best direction to take - that said, maybe each TLS implementation can have a method on the Request that returns their native certificate type, and place them behind feature flags in the same way the with_rustls/with_openssl methods are.

Alternatives

As noted above, the C++ implementation returns a string URI and leaves the caller to parse it according to the expected value. This requires unnecessarily verbose code for the caller to extract the address, I hope we'd be able to do something a little nicer!

@LucioFranco
Copy link
Member

Thanks for opening this detailed issue! So I am really cautious about introducing something like a PeerAddr to request since it's designed intentionally to not expose any details about the transport/encodings.

For the client, I believe the channel can expose who it's connected too but this also might not make sense if you're load balancing.

For the server, hyper itself provides a way to get nonexclusive access to the IO type which then can provide the remote address. Via this, we could probably provide some middleware to extract this and attach it to the metadata of every inbound request. What do you think about that? This would require that we somehow serialize the peer address into some form of bytes but I think this might provide the most flexible solution.

@domodwyer
Copy link
Contributor Author

Hi @LucioFranco

I actually took a peek at using an interceptor to populate the metadata exactly as you suggest, but currently it has the http layer Request only so went looking elsewhere - I think it would be a good way of opting into the functionality (it's safe to say it's not needed by everyone) and feels quite natural.

I must admit I have only been considering the server side of this until now - I've not used the client yet, so can't really suggest anything meaningful.

Dom

@LucioFranco
Copy link
Member

Right so this gets a bit funky ill try to explain a bit here:

So in tower, we use two types of Service's for servers. We use an outter MakeService that creates some inner service for each connection, that inner service then gets dispatched aka called for each inbound request. The outter MakeService contains a target. You can see some of this here https://github.com/hyperium/tonic/blob/master/tonic/src/transport/server.rs#L384 and you will notice that the type T is actually ignored here. In fact this with hyper turns out to be the IO type given by a & reference. So this is a bit lower level then what I'd like do to with interceptors but this is totally possible. So if you wrap your generated MakeService from codegen like I do here you can extract and then inject. Let me know if that makes sense :)

@domodwyer
Copy link
Contributor Author

Great - makes perfect sense.

I'll take a look some time in the next week, thanks for the pointers!

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 a pull request may close this issue.

2 participants