feat: Implement gRPC Reflection Service #340

jen20 · 2020-05-05T04:37:07Z

Motivation

Issue #165 describes the motivations for the gRPC Reflection Service, along with the upstream definition.

Solution

This pull request implements a new crate, tonic-reflection which implements the gRPC Reflection Service and provides a builder for registering protocol buffer FileDescriptorProtos. In the first instance, this relies on danburkert/prost#311, or a similar method, to embed encoded versions of the descriptor in code.

This is an early work in progress, and only implements the ListServices and FileContainingSymbol aspects of the reflection API, though this is enough to interrogate the shape of services and messages using grpcurl. The new example server can be used with grpcurl to verify this:

$ grpcurl -plaintext '[::1]:50052' list
grpc.reflection.v1alpha.ServerReflection
helloworld.Greeter

$ grpcurl -plaintext '[::1]:50052' describe helloworld.Greeter
helloworld.Greeter is a service:
// The greeting service definition.
service Greeter {
  // Sends a greeting
  rpc SayHello ( .helloworld.HelloRequest ) returns ( .helloworld.HelloReply );
}

$ grpcurl -plaintext '[::1]:50052' describe helloworld.HelloRequest                                                                                             
helloworld.HelloRequest is a message:
// The request message containing the user's name.
message HelloRequest {
  string name = 1;
}

Currently the implementation is fairly rough - the following work is known as needed:

Implement remaining request types
Handle dependencies when processing file descriptors during build
Write end to end tests
Remove temporary usages of thiserror when all variants are known

~~To make this usable, we'll also likely depend on the upstream prost pull request being merged, and the prost-types crate being generated with descriptor embedding enabled.~~

CI will definitely fail, since it references locally modified crates in the Cargo.toml file for now - this PR is currently intended to make progress public for interested parties rather than being in a state to merge!

tonic-reflection/build.rs

tonic-reflection/src/lib.rs

tonic-reflection/src/server.rs

LucioFranco

looks great, would be good to see how we could test this too.

jen20 · 2020-05-21T04:43:13Z

I've done some update work on this, including redirecting prost correctly in order that CI can run before https://github.com/danburkert/prost/pull/326 is merged. Using FileDescriptorSet instead of FileDescriptorProto solves some of the issues. Still not fully implemented however, so I'm going to leave this as a draft PR.

LucioFranco · 2020-06-03T15:57:23Z

@jen20 sorry for the delay, what is the status on this? What are we blocked on?

jen20 · 2020-06-05T01:00:26Z

@LucioFranco No worries. To actually release this we're blocked on the prost change upstream, but there are also some tests which need writing which I've not yet done.

kinosang · 2020-08-01T15:20:47Z

any update here?

jen20 · 2020-08-02T13:41:18Z

Hi @kinosang, I'm still waiting for https://github.com/danburkert/prost/pull/326 to land to be able to make much progress in getting this merged here.

jen20 · 2020-11-28T17:35:48Z

This is still blocked by danburkert/prost#326 which itself is now blocked by danburkert/prost#397.

sgg · 2021-01-12T21:17:40Z

@jen20 I believe that with danburkert/prost#409 (included w/ prost v0.7), this change is now possible 😄.

Do you have the time to drive this? If not, I don't mind picking it up.

This change updates jen20's work in hyperium#340 to work with the file descriptor set changes that landed in prost 0.7. * Updates branch with new changes from master * Updates server to use tokio 1.0 symantics * `tonic_build::Builder::include_file_descriptor_set` -> `file_descriptor_set_path`, which matchesprost.

This change updates jen20's work in hyperium#340 to work with the file descriptor set changes that landed in prost 0.7. * Updates branch with new changes from master * Updates server to use tokio 1.0 symantics * `tonic_build::Builder::include_file_descriptor_set` -> `file_descriptor_set_path`, which matchesprost. * The reflection server now returns UNIMPLEMENTED rather than NOT_FOUND when unsupported methods are called.

This commit adds a new option, `include_file_descriptor_set` to the tonic build configuration to in order to include the encoded protocol buffers `FileDescriptorSet` in `OUT_DIR`. It also adds a new macro `include_file_descriptor_set!`, to make it easy to include the well-known output path as a byte array in application source code. Co-authored-by: Samani G. Gikandi <[email protected]>

jen20 · 2021-01-23T19:05:16Z

Hi @sgg! I've rebased this PR and squashed the various changes into logical commits that can land on master. Since there is some of your code in here (from #539), I've added you as a co-author in both Cargo.toml (for tonic-reflection - I assume the addition to tonic-health in #539 was a typo!), and as a co-author of the commits to reflect the contributions. Thanks for picking it up!

sgg · 2021-01-23T19:10:57Z

@jen20 Sweet! I'll close my PR and we can track the work here.

I assume the addition to tonic-health in #539 was a typo!

It most certainly was! 😆

jen20 · 2021-01-23T19:14:54Z

Thanks @ssg! Good news, after rebase etc, the original set of tests with the reflection-server example are still working:

$ grpcurl -plaintext '[::1]:50052' list
grpc.reflection.v1alpha.ServerReflection
helloworld.Greeter

$ grpcurl -plaintext '[::1]:50052' describe helloworld.Greeter
helloworld.Greeter is a service:
// The greeting service definition.
service Greeter {
  // Sends a greeting
  rpc SayHello ( .helloworld.HelloRequest ) returns ( .helloworld.HelloReply );
}

$ grpcurl -plaintext '[::1]:50052' describe helloworld.HelloRequest
helloworld.HelloRequest is a message:
// The request message containing the user's name.
message HelloRequest {
  string name = 1;
}

This should be ready for review shortly.

This commit adds an implementation, tests and example server for the `tonic-reflection` crate, which implements the server side of the GRPC Server Reflection Protocol. Co-authored-by: Samani G. Gikandi <[email protected]>

cawfeecoder · 2021-02-11T04:04:28Z

I'm interested to see this make it in :)

LucioFranco · 2021-02-12T16:01:53Z

tonic-build/src/prost.rs

@@ -203,6 +205,13 @@ impl Builder {
        self
    }

+    /// Generate a file containing the encoded `prost_types::FileDescriptorSet` for protocol buffers
+    /// modules. This is required for implementing gRPC Server Reflection.
+    pub fn file_descriptor_set_path(mut self, path: impl AsRef<Path>) -> Self {


I wonder if we should just make this an option to add the descriptor to the generated proto file as a constant?

@LucioFranco Perhaps I'm misunderstanding this - wouldn't prost need to support that too?

If we are going through prost to generate this and it puts it into its own file then yes. I may have misunderstood.

jen20 added the C-enhancement Category: New feature or request label May 5, 2020

jen20 requested a review from LucioFranco May 5, 2020 04:37

jen20 self-assigned this May 5, 2020

jen20 marked this pull request as draft May 5, 2020 04:37

jen20 linked an issue May 5, 2020 that may be closed by this pull request

Support gRPC reflection #165

Closed