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

Feature Request: support empty GET request. #31

Closed
anzboi opened this issue Nov 27, 2022 · 4 comments
Closed

Feature Request: support empty GET request. #31

anzboi opened this issue Nov 27, 2022 · 4 comments

Comments

@anzboi
Copy link

anzboi commented Nov 27, 2022

grpchealth as is provides a pretty convenient up check that could be used for checking whether a server is running. This is pretty useful for kube or docker compose.

Right now, the "minimal" request needed looks like...

$ curl -X POST http://HOSTNAME/grpc.health.v1.Health/Check -d "{}" -H "Content-Type: application:json"
# 200 OK

It would be nice if minimality could be brought down to...

$ curl -X GET http://HOSTNAME/grpc.health.v1.Health/Check

NOTE: Setting -X GET causes 405 Method Not Allowed
Removing the body and content-type causes 415 Unsupported Media Type

@akshayjshah
Copy link
Member

Totally understand wanting health checking to be as simple as possible - shorter's better! Even without built-in GET support, I think you can shorten your k8s and Docker compose configurations a fair bit.

  • Kuberbetes supports gRPC health checks natively - you don't need to write a curl command at all!
  • With recent cURL releases (anything more recent than version 7.82.0), you can shorten the command you have above to curl --json '{}' http://HOSTNAME/grpc.health.v1.Health/Check. (Note that this command is using the Connect RPC protocol, so it won't work with grpc-go, grpc-java, etc. servers.)
  • The cURL commands you're using are only checking that the HTTP server is up - they're not checking the health of any specific protobuf service. If that's all you care about and you'd prefer to health check with cURL, you don't actually need any of the functionality of this package. Why not simplify and use a standard GET /health instead?
func ok(_ http.ResponseWriter, _ *http.Request) {}

func main() {
  mux := http.NewServeMux()
  // mount any other RPC handlers here, and then...
  mux.Handle("/health", http.HandlerFunc(ok))
  http.ListenAndServe(":8080", mux)
}

@solarhell
Copy link

Checked the code and found that if we want to be compatible with http GET, then the changes are significant.

I agree with @akshayjshah that you can make simplifications in the application code. And the connect-go core lib is still simple.

@jamesrom
Copy link

jamesrom commented Sep 20, 2024

It would be nice to be able to hit the Check endpoint in the browser and it just work.

@jhump
Copy link
Member

jhump commented Sep 20, 2024

This repo's purpose is to provide an implementation of the grpc.health.v1.Health service, mainly for use with servers that use the connectrpc.com/connect module. This is an RPC endpoint, and the definition of the RPC is provided and maintained by the gRPC organization. It is not intended to be a general HTTP health-check endpoint.

Wrapping a grpchealth.Checker in a simple HTTP GET endpoint is already trivial to do. If that's what you need, you could do something like this in your code:

// HealthEndpoint returns a handler that responds with 200 if the requested
// service is okay and "503 Service Unavailable" otherwise. The service is
// indicated via "service" query parameter. If it is absent, the checker is
// queried with an empty service name. If the checker returns an error
// instead of a status, the result will be "500 Internal Server Error".
func HealthEndpoint(checker grpchealth.Checker) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		resp, err := checker.Check(r.Context(), &grpchealth.CheckRequest{
			Service: r.URL.Query().Get("service"),
		})
		if err != nil {
			http.Error(w, err.Error(), http.StatusInternalServerError)
			return
		}
		if resp.Status != grpchealth.StatusServing {
			w.WriteHeader(http.StatusServiceUnavailable)
		} // else, 200 OK status is implied
	})
}

But, given the purpose of this repo, that is not really in scope for exported API provided by this module.

@jhump jhump closed this as not planned Won't fix, can't repro, duplicate, stale Sep 20, 2024
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

No branches or pull requests

5 participants