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

feat: Implement gRPC Reflection Service #340

Merged
merged 2 commits into from Feb 16, 2021
Merged

Conversation

jen20
Copy link
Collaborator

@jen20 jen20 commented May 5, 2020

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!

@jen20 jen20 added the C-enhancement Category: New feature or request label May 5, 2020
@jen20 jen20 requested a review from LucioFranco May 5, 2020 04:37
@jen20 jen20 self-assigned this May 5, 2020
@jen20 jen20 marked this pull request as draft May 5, 2020 04:37
@jen20 jen20 linked an issue May 5, 2020 that may be closed by this pull request
Copy link
Member

@LucioFranco LucioFranco 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, would be good to see how we could test this too.

@jen20
Copy link
Collaborator Author

jen20 commented May 21, 2020

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
Copy link
Member

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

@jen20
Copy link
Collaborator Author

jen20 commented Jun 5, 2020

@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
Copy link

kinosang commented Aug 1, 2020

any update here?

@jen20
Copy link
Collaborator Author

jen20 commented Aug 2, 2020

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
Copy link
Collaborator Author

jen20 commented Nov 28, 2020

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

@sgg
Copy link
Contributor

sgg commented Jan 12, 2021

@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.

sgg added a commit to sgg/tonic that referenced this pull request Jan 16, 2021
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.
sgg added a commit to sgg/tonic that referenced this pull request Jan 16, 2021
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 <samani@gojulas.com>
@jen20
Copy link
Collaborator Author

jen20 commented Jan 23, 2021

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
Copy link
Contributor

sgg commented Jan 23, 2021

@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
Copy link
Collaborator Author

jen20 commented Jan 23, 2021

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.

@jen20 jen20 force-pushed the jen20/tonic-reflection branch 2 times, most recently from 1a7cfb7 to e0e4ed1 Compare January 24, 2021 21:36
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 <samani@gojulas.com>
@jen20 jen20 marked this pull request as ready for review January 24, 2021 21:49
@cawfeecoder
Copy link

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

@@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support gRPC reflection
5 participants