gRPC improve error status for non-200 h2 response status #2221
Conversation
| statusCode == GATEWAY_TIMEOUT.code() || statusCode == TOO_MANY_REQUESTS.code()) { | ||
| grpcStatusCode = UNAVAILABLE; | ||
| } else if (statusCode == UNAUTHORIZED.code()) { | ||
| grpcStatusCode = UNAUTHENTICATED; |
There was a problem hiding this comment.
I would map this to PERMISSION_DENIED which implies external action is required before it is wort attempting again. UNAUTHENTICATED that with authentication the result might succeed.
There was a problem hiding this comment.
I think UNAUTHENTICATED make sense in this case.
https://datatracker.ietf.org/doc/html/rfc7235#section-3.1
The 401 (Unauthorized) status code indicates that the request has not
been applied because it lacks valid authentication credentials for
the target resource.
Where 403 Forbidden means we authenticated the client, but permission was denied.
The unit test also confirms we use a consistent strategy to grpc-java.
|
test failure due to #2117 |
Scottmitch
force-pushed
the
grpc_200
branch
2 times, most recently
from
da15823
to
6c81cea
Compare
| @@ -49,7 +49,7 @@ final class HttpResponseUponGrpcRequestTest { | |||
| ServerContext serverContext = HttpServers.forAddress(localAddress(0)) | |||
| .protocols(h2Default()) | |||
| .listenAndAwait((ctx, request, responseFactory) -> | |||
| succeeded(responseFactory.badRequest().payloadBody(responsePayload, textSerializerUtf8()))); | |||
| succeeded(responseFactory.ok().payloadBody(responsePayload, textSerializerUtf8()))); | |||
There was a problem hiding this comment.
protocol compatibility now tests all http response codes, this test validates content-type (which is what is was asserting before this change too bcz we weren't validating http status codes)
servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcStatusCode.java
Outdated
Show resolved
Hide resolved
| } else if (statusCode == NOT_FOUND.code()) { | ||
| grpcStatusCode = UNIMPLEMENTED; | ||
| } else if (statusCode == BAD_REQUEST.code() || statusCode == REQUEST_HEADER_FIELDS_TOO_LARGE.code()) { | ||
| grpcStatusCode = INTERNAL; |
There was a problem hiding this comment.
I'm surprised grpc-java maps these cases to INTERNAL.
In HTTP, bad request usually means that client sent something incorrectly rather than a server-side error occurred. I would expect INVALID_ARGUMENT in both of these cases.
Also surprised that 412 Precondition Failed is not mapped to FAILED_PRECONDITION and many other 4xx/5xx mappings could have a better grpc-status code.
There was a problem hiding this comment.
I took a stab at providing better mappings. I would suggest we can refine the http response code mapping in followup as well.
| // grpc-java maps 1xx responses to error code INTERNAL, we currently map to UNKNOWN. The test server | ||
| // isn't following the http protocol by returning only a 1xx response and each framework catches | ||
| // this exception differently internally. | ||
| assertThat("mismatch for h2 response code: " + httpCode, stCode, equalTo(UNKNOWN.value())); |
There was a problem hiding this comment.
- Consider adding assertion for grpc status code too:
assertThat("mismatch for h2 response code: " + httpCode, grpcJavaCode, equalTo(INTERNAL.value()));- Can you please clarify what prevents us from mapping it to
INTERNALtoo?
There was a problem hiding this comment.
(1) -> done
(2) -> when there is a 1xx response w/out an expect request our client ignore the initial response headers, splicer sees a NettyBuffer and tries to cast it to HttpResponseMetaData (fails with ClassCastException) which gets mapped to unknown via DefaultGrpcClientBuilder$CatchAllHttpClientFilter.
Motivation: ServiceTalk currently assumes the response status is 200 when decoding into gRPC. This means we will fail during checking the content-type or during deserialization if an error is return from a middle proxy with a different encoding type. We can provide more informative gRPC error codes if we validate the h2 response status.
Motivation:
ServiceTalk currently assumes the response status is 200
when decoding into gRPC. This means we will fail during
checking the content-type or during deserialization if
an error is return from a middle proxy with a different
encoding type. We can provide more informative gRPC
error codes if we validate the h2 response status.