Open MnameHZJ opened 2 years ago
moores
commented
1 year ago When will 4.0.5 have a publicly available milestone build available? I have not tried pulling milestone builds in before, but i can't find anything on the repository beyond 4.0.0-RCxx. I'm eager to pull this fix in an test it out.
Albertoimpl
commented
1 year ago @moores https://github.com/spring-cloud/spring-cloud-gateway/releases/tag/v4.0.5 was released and should contain the fix. Let us know how it went. Thanks!
moores
commented
1 year ago hi @Albertoimpl I'm trying to verify a request via BloomRPC client - using v4.0.5 of Spring Cloud Gateway. Requests work fine if there is no error. But if i intentionally throw an exception within a filter or configure the route endpoint to a non-existent DNS entry, the bloom client just hangs when the exception is propagated. It's acting like the connection is never closed. Should I assume that with the gRPC H2C configuration that a particular error status should be set and connection released/closed?
Should I expect the GRPCResponseHeadersFilter to be executed when a system error happens? Where is the exception handler for gRPC?
Albertoimpl
commented
1 year ago Thanks for the feedback @moores, would you mind providing a minimal reproducible example so I can take a better look at what headers are coming back when called through the gateway and compare the differences? Not sure if it is a problem with that specific client or if we are still not returning an expected header.
moores
commented
1 year ago Yes @Albertoimpl I'll do that. Can you also let me know where in spring cloud we are handling system exceptions for gRPC responses?
Albertoimpl
commented
1 year ago We do not have a custom exception handler for gRPC @moores, what I would expect is that the server sends a response with different statuses and headers if anything went wrong, and the gateway passes them through. gRPC itself does not have the concept of exception, it is all propagated through statuses and headers.
moores
commented
1 year ago Thanks, that makes sense. In the request/response attached below, I am throwing an exception in the application due to a DNS error on the route endpoint URL. The response looks like the same JSON response I would expect when running with HTTP/REST:
{
"timestamp": "2023-05-05T17:03:29.550+00:00",
"path": "/com.expedia.lodging.lips.contracts.generated.service.dds.RoomTypeRatePlanService/getRoomRatePlanDetails",
"status": 500,
"error": "Internal Server Error",
"requestId": "79db2fec-1"
}So should spring have a default exception handler for this when the protocol is gRPC? The content-type in the response is set to application/json.. ?
moores
commented
1 year ago I think the spring-cloud-gateway default exception handler should, by default, set the grpc-status and grpc-message headers to some value when an error occurs; doing this if the content-type is application/grpc.
I extended DefaultErrorAttributes and added code to add this response header if processing a grpc request (content-type=application/grpc). But that's suboptimal, as this class should probably not be mutating things. Overriding or extending DefaultErrorWebExceptionHandler seems really complicated - i don't see any support guidance in documentation on this.
grpc-response header to indicate the equivalent of a HTTP 429 Too Many Requests. Should the grpc-status response header be set to "8" RESOURCE_EXHAUSTED in this case? I'll have to make that work before I go into production with high volume clients making gRPC requests.metadata.response-timeout is exceeded, a DEADLINE_EXCEEDED response code "4" should be returned. Currently, a "2" is returned.moores
commented
1 year ago @Albertoimpl - Should I create a new issue regarding the concerns mentioned above?
Albertoimpl
commented
1 year ago Hi @moores, I think this issue covers well the problem, no need to create a new one. The packets were very useful and I can see that there is something wrong. But I am still not sure why an exception gets propagated when we are properly handling bad responses from a server, see: https://github.com/spring-cloud/spring-cloud-gateway/blob/81203031217b7e73e0fd4e5fa798f08d14ada054/spring-cloud-gateway-integration-tests/grpc/src/test/java/org/springframework/cloud/gateway/tests/grpc/GRPCApplicationTests.java#L75-L87 Would it be possible for you to provide a minimal reproducible sample so I can verify this for your case? Thanks!
moores
commented
1 year ago I'll try to do that this week.
The test above is returning a FAILED_PRECONDITION from the test stream observer - this works fine.
In my case, I'm trying to handle an internal exception- one that occurs before the request is actually routed to it's destination.
For example, if the netty client can't connect to the route destination because of a connect timeout or a DNS resolution failure.
Another difference is that I am not using TLS/SSL- configured like this: (there's a Kotlin extension function isGrpcRequest added to the ServerWebExchange - it's just checks for Content-Type application/grpc)
@Component
class H2CAwareNettyRoutingFilter(
httpClient: HttpClient,
headersFiltersProvider: ObjectProvider<List<HttpHeadersFilter>>,
properties: HttpClientProperties
) : NettyRoutingFilter(httpClient, headersFiltersProvider, properties) {
/**
* Overrides [getHttpClient] to return an H2C capable client if
* the request indicates it's a gRPC request.
*/
override fun getHttpClient(route: Route, exchange: ServerWebExchange): HttpClient =
super.getHttpClient(route, exchange).let { httpClient ->
if (exchange.isGrpcRequest()) {
// https://projectreactor.io/docs/netty/release/reference/index.html#_http2_2
httpClient.protocol(HttpProtocol.H2C)
} else {
httpClient
}
}
override fun getOrder() = super.getOrder() - 1
}
I did verify an IO error seems to be processed OK on main branch:
@Test
public void gRPCUnaryCallShouldHandleInternalException() throws SSLException {
ManagedChannel channel = createSecuredChannel(9999);
try {
HelloServiceGrpc.newBlockingStub(channel)
.hello(HelloRequest.newBuilder().setFirstName("Mickey").build());
}
catch (StatusRuntimeException e) {
Assertions.assertThat(UNAVAILABLE.getCode()).isEqualTo(e.getStatus().getCode());
Assertions.assertThat("io exception").isEqualTo(e.getStatus().getDescription());
}
}
spencergibb
commented
1 year ago @Albertoimpl @martamedio is this fixed by #2938 #2914?
Albertoimpl
commented
1 year ago @spencergibb If the error comes from the upstream grpc-server, it propagates well and everything works. However, when there is an exception within the gateway-server, we use the default exception handler and it may cause some problems in the grpc-client.
Asked @moores for a reproducer so I can take a deeper look.
moores
commented
1 year ago Sorry I have not worked on this issue for a while- I have been on vacation. I'll try to solve this - this week.
Seems to me that the spring org.springframework.boot.autoconfigure.web.reactive.error.DefaultErrorWebExceptionHandler knows nothing about gRPC, and it should. It should understand that we are using the gRPC protocol and return a grpc response header.
I extended DefaultErrorWebExceptionHandler and added an override on renderErrorResponse that sets proper gRPC response headers.
Something like this: (i have some kotlin extension functions on the exchange, etc)
override fun renderErrorResponse(request: ServerRequest): Mono<ServerResponse> =
if (request.exchange().isGrpcRequest()) {
val error = getErrorAttributes(request, getErrorAttributeOptions(request, MediaType.ALL))
ResponseErrorUtil.grpcServerResponseForError(
"${error["error"]?.toString() ?: ""} transactionId=${request.exchange().transactionId()}",
HttpStatus.valueOf(getHttpStatus(error))
)
} else {
super.renderErrorResponse(request)
}
fun grpcServerResponseForError(
errorMessage: String,
status: HttpStatus = HttpStatus.INTERNAL_SERVER_ERROR
): Mono<ServerResponse> {
return ServerResponse.status(status)
.header(HttpHeaders.TRAILER, TrailerHeaderValue)
.header(GrpcStatusHeader, grpcResponseCodeFor(status))
.header(GrpcMessageHeader, errorMessage)
.header(ContentTypeHeader, ContentTypeGrpc)
.build()
}
private fun grpcResponseCodeFor(status: HttpStatus): String =
when (status) {
HttpStatus.OK -> GrpcStatusOkCode
HttpStatus.UNAUTHORIZED -> GrpcStatusUnauthorizedRequestErrorCode
HttpStatus.BAD_REQUEST -> GrpcStatusInvalidRequestErrorCode
HttpStatus.NOT_FOUND -> GrpcStatusNotFound
HttpStatus.GATEWAY_TIMEOUT -> GrpcStatusDeadlineExceeded
HttpStatus.SERVICE_UNAVAILABLE -> GrpcStatusServiceUnavailableErrorCode
HttpStatus.INTERNAL_SERVER_ERROR -> GrpcStatusUnknownErrorCode
else -> GrpcStatusUnknownErrorCode
}
// See https://grpc.github.io/grpc/core/md_doc_statuscodes.html
// See https://chromium.googlesource.com/external/github.com/grpc/grpc/+/HEAD/doc/PROTOCOL-HTTP2.md
private const val GrpcStatusOkCode = "0"
private const val GrpcStatusUnknownErrorCode = "2"
private const val GrpcStatusInvalidRequestErrorCode = "3"
private const val GrpcStatusDeadlineExceeded = "4"
private const val GrpcStatusNotFound = "5"
private const val GrpcStatusUnauthorizedRequestErrorCode = "7"
private const val GrpcStatusServiceUnavailableErrorCode = "14"
private const val GrpcStatusHeader = "grpc-status"
private const val GrpcMessageHeader = "grpc-message"
private const val TrailerHeaderValue = "$GrpcStatusHeader,$GrpcMessageHeader"
This is a duplicate problem, but I can't find the solution.Anybody know?
When the grpc client requests to the gateway, the gateway occur an exception or check header illegal and then needs to return the message about to the grpc client. So how to build a ExceptionHandler similar to controlleradvice . in my case, If no additional processing is done, the grpc client will get a protocol-related exception