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

Expose grpc_health_v1 descriptor set in tonic-health for use with the reflection service #620

Merged

Conversation

byblakeorriver
Copy link
Contributor

@byblakeorriver byblakeorriver commented Apr 30, 2021

Motivation

Fixes #619

When using Kubernetes a readiness and liveness check should be defined for a deployment. This is usually done by having an endpoint to hit that will return the serving status. For gRPC there are some common tools used to hit these endpoints (grpcurl or grpc_health_probe). These tools require either a .protoset file for the service that they are going to interact with, or they require the service to have a reflection service registered. The health check is usually handled by hitting the endpoint of the grpc health service to see if it is serving or not. In order to register the health service with the reflection service using the tonic-reflection crate you have to have the file descriptor set binaries of the health.proto file. It seems appropriate for this to be produced by the tonic-health crate.

Solution

In order to make this work, I just needed to create the file descriptor binaries in the build.rs file and then expose it in the proto mod of the lib.rs file.

.gitignore Outdated Show resolved Hide resolved
@davidpdrsn
Copy link
Member

I'm not very familiar with tonic-health. @jen20 are you up for taking a look at this?

@@ -28,6 +28,10 @@ pub mod proto {
#![allow(unreachable_pub)]
#![allow(missing_docs)]
tonic::include_proto!("grpc.health.v1");

#[allow(dead_code)]
pub const GRPC_HEALTH_V1_FILE_DESCRIPTOR_SET: &'static [u8] =
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to include a test here maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that. It will be a few days before I have the time, but I will get that worked in.

@LucioFranco
Copy link
Member

@byblakeorriver I think we can merge this if you want to add that one test?

@ar3s3ru
Copy link

ar3s3ru commented Jun 16, 2021

@LucioFranco is the test really necessary? After all it's just including something generated by tonic-build 👀

@byblakeorriver
Copy link
Contributor Author

byblakeorriver commented Jun 24, 2021

Sorry I have been swamped. I am not sure when I would be able to get that test written. I am okay with waiting until I can do that.

fn main() -> Result<(), Box<dyn std::error::Error>> {
let grpc_health_v1_descriptor_set_path: PathBuf =
PathBuf::from(env::var("OUT_DIR").unwrap()).join("grpc_health_v1.bin");
Copy link

Choose a reason for hiding this comment

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

OUT_DIR might not be set except in the build environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty sure it is set when build.rs is ran.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OUT_DIR — If the package has a build script, this is set to the folder where the build script should place its output. See below for more information. (Only set during compilation.)

Found here: https://doc.rust-lang.org/cargo/reference/environment-variables.html#:~:text=OUT_DIR%20%E2%80%94%20If%20the%20package%20has,(Only%20set%20during%20compilation.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But, if you want, I can add a check instead of using .unwrap() here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My only question in that case would be, what behavior would you expect if the OUT_DIR didn't exist?

Would there just be an error saying that the OUT_DIR wasn't set and not produce the descriptor set?

What would say about this @LucioFranco?

Copy link
Member

Choose a reason for hiding this comment

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

I think we can trust that OUT_DIR will be there, because this script is also compiling the proto I think its safe.

@LucioFranco LucioFranco merged commit 6ee638d into hyperium:master Jul 13, 2021
@byblakeorriver
Copy link
Contributor Author

Thanks @LucioFranco!!!

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.

Expose grpc_health_v1 descriptor set in tonic-health for use with the reflection service
5 participants