jhump / protoreflect

Reflection (Rich Descriptors) for Go Protocol Buffers
Apache License 2.0
1.35k stars 172 forks source link

Protoreflect doesn't fall back to to v1alpha when a gRPC unimplemented response is returned #577

Closed jroper closed 1 year ago

jroper commented 1 year ago

Currently, protoreflect only falls back to v1alpha when an HTTP 404 error is returned, not when a gRPC unimplemented (12) error is returned. Note that the code is checking for a gRPC unimplemented error, but that is working because an HTTP 404 error is always translated to a gRPC unimplemented error, as per the spec.

The issue is that the error checking that does the fallback is done before the status code from the gRPC server is read. That is, it's done immediately after the HTTP response is received, but before the subsequent gRPC status trailer is read.

This means that a gRPC server that has been implemented in a strictly spec conforming way for gRPC, only returning gRPC errors when it receives a gRPC request, even if there's no corresponding gRPC service to handle the request (in which case, it accepts the request with an HTTP 200 and then immediately close the stream with an unimplemented status code), will not be detected as not supporting v1 of ServerReflection, and fallback to v1alpha will not be done.

To fix, the fallback logic should also be applied for when an unimplemented status code is received when the stream terminates.

jroper commented 1 year ago

Sorry, I just found my test code that was testing this was wrong, it is falling back correctly when a gRPC unimplemented response is returned. My bad.