proxy: always return 200 status code to gRPC client #102
Conversation
It turns out that sending a non-200 HTTP status code was against the gRPC spec and the older versions of the `grpc` library just didn't validate that. The validation was added in v1.40.0, which is the version that we couldn't update to before. With this fix the error is still parsed correctly on the client side. But this requires a small change to the L402 spec because the status code is no longer 402.
| @@ -437,7 +437,12 @@ func sendDirectResponse(w http.ResponseWriter, r *http.Request, | |||
| w.Header().Set(hdrGrpcStatus, strconv.Itoa(int(codes.Internal))) | |||
| w.Header().Set(hdrGrpcMessage, errInfo) | |||
|
|
|||
| w.WriteHeader(statusCode) | |||
| // As per the gRPC spec, we need to send a 200 OK status code | |||
There was a problem hiding this comment.
Ahh this makes a ton of sense, the spec also has this fragment shortly after:
Implementations should expect broken deployments to send non-200 HTTP status codes in responses as well as a variety of non-GRPC content-types and to omit Status & Status-Message. Implementations must synthesize a Status & Status-Message to propagate to the application layer when this occurs.
| // header fields are enough to inform any gRPC compliant client | ||
| // about the error. See: | ||
| // https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses | ||
| w.WriteHeader(http.StatusOK) |
There was a problem hiding this comment.
Will this be backwards compatible with the prior users of this package? I wonder if we should consider making this into a sub-module?
There was a problem hiding this comment.
Yes, this should be backward compatible, since a gRPC client only looks at the Grpc-Status and Grpc-Message anyway. AFAIK this was always true, but newer clients just started validating the HTTP status code as well.
But this is what I aim to confirm with an itest and an older client.
| @@ -437,7 +437,12 @@ func sendDirectResponse(w http.ResponseWriter, r *http.Request, | |||
| w.Header().Set(hdrGrpcStatus, strconv.Itoa(int(codes.Internal))) | |||
| w.Header().Set(hdrGrpcMessage, errInfo) | |||
|
|
|||
| w.WriteHeader(statusCode) | |||
| // As per the gRPC spec, we need to send a 200 OK status code | |||
There was a problem hiding this comment.
Based on this: https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md
Should the above instead use an UNKNOWN status/error code?
There was a problem hiding this comment.
Yes, probably. Or we could even send our own Grpc-Status code (for example 402). But because all clients out there explicitly check for the codes.Internal code at the moment, we would need to upgrade all clients first. Or parse the User-Agent field which contains the version of the grpc library. But that just seemed too brittle to me.
There was a problem hiding this comment.
Gotcha, let's stick with this for now then. When we refine the bLIP we can make sure to note these aspects for backwards compat.
|
I verified that this works with both the Loop and Pool server backend integration tests 🎉 |
Fixes #62.
It turns out that sending a non-200 HTTP status code was against the
gRPC spec and the older versions of the
grpclibrary just didn'tvalidate that. The validation was added in v1.40.0, which is the version
that we couldn't update to before.
With this fix the error is still parsed correctly on the client side.
But this requires a small change to the L402 spec because the status
code is no longer 402.
See grpc/grpc-go#4474 and https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses.
I'll also open a PR in the L402 spec repo and will test this in one of our server integration test setups.