ErrorWriter.IsSupported is overly strict #689
Labels
bug
Something isn't working
Comments
jhump
added a commit
that referenced
this issue
Feb 16, 2024
This largely undoes a recent change to do more validation of trailers-only responses (#685), which disallows a body or trailers in what appeared to be a trailers-only response. In that change, a trailers-only response was identified by the presence of a "grpc-status" key in the headers. In this PR, a trailers-only response is instead defined by the lack of body and trailers (not the presence of a "grpc-status" header). This PR also tweaks some other error scenarios: * If trailers (or an end-stream message) is completely missing from a response, it's considered an `internal` error. But if trailers are present, but the "grpc-status" key is missing, it's considered an issue determining the status, which is an `unknown` error. * Similarly, if a response content-type doesn't appear to be the right protocol (like it may have come from a non-RPC server), the error code is now `unknown`. But if it looks like the right protocol but uses the wrong sub-format/codec, it's an `internal` error. * Note that in grpc-go, this behavior is also seen in the client, but this PR doesn't attempt to address that in the connect-go client. Instead, that change can be made when #689 is addressed. This PR also now makes connect-go more strict about the "compressed" flag in a streaming protocol when there was no compression algorithm negotiated. Previously, this library was lenient and did not consider it an error if the message in question was empty (zero bytes). But to correctly adhere to gRPC semantics, it must report this case as an `internal `error.
emcfarlane
added a commit
that referenced
this issue
Mar 12, 2024
This PR changes the ErrorWriter to be more lenient with classifying protocols. Errors codecs are agnostic to the codec used. Therefore we avoid checking the codec in classifying the request. IsSupported will return true for an unknown codec which allows the server to encode a better error message to the client. If not supported a 415 error response could be used to match gRPC server like handling. If not supported and trying to write an error the ErrorWriter will default to connects unary encoding (consistent with current behaviour). Fixes #689
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Currently
ErrorWriter.IsSupporteduses the currently supported codecs (provided in the form of handler options) to compute all of the possible content-types that the server would support. However, this is not necessary. There are only four protocol types, and the actual registered codecs aren't necessary in order to classify them:application/grpcor starts withapplication/grpc+then it's a gRPC request.application/grpc-webor starts withapplication/grpc-web+then it's a gRPC-Web request.application/connect+then it's a Connect streaming request.application/and matches none of the above OR it has aConnect-Protocol-Version: 1header OR it uses a GET HTTP method and includes aconnect=v1query param then it's a Connect unary request.IsSupportedreturn false.In all of the above cases, the actual supported codecs are irrelevant because, once classified as above, an error can be correctly written (error formats are codec-agnostic).
Addressing this would also enable error handling that more closely resembles gRPC. In gRPC servers, if the content-type is not a gRPC content-type (i.e. it is not
application/grpcand does not start withapplication/grpc+) then the server sends back a 415 HTTP status code, which clients will then classify as an unknown error. If it's a gRPC content-type, but not a supported codec, the server sends back a gRPC error (HTTP status code 200, error code and message in HTTP trailers) with a code of "internal". So with the above logic, when the content-type is not supported, anErrorWritercould be used to write an RPC internal error and a 415 HTTP status code only used if theErrorWriterreports that the request is unsupported.The text was updated successfully, but these errors were encountered: