Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

grpc UNAVAILABLE status returned when calling grpc-json rpc with invalid Number (Long, Integer, ...) #5379

Closed
xhanin opened this issue Jan 10, 2024 · 6 comments · Fixed by #5554
Labels
defect sprint Issues for OSS Sprint participants

Comments

@xhanin
Copy link

xhanin commented Jan 10, 2024

When using HttpJsonTranscoding with grpc, if we call an rpc with an invalid value for a number, we get a grpc status UNAVAILABLE and no log, which makes the diagnostic very cumbersome.

Exemple:

service MyService {
    rpc TestArmeriaGrpcIntFailure(TestArmeriaGrpcIntFailureRequest) returns (TestArmeriaGrpcIntFailureResponse) {}
}

message TestArmeriaGrpcIntFailureRequest {
    int64 number = 1;
}

message TestArmeriaGrpcIntFailureResponse {

}

Create an armeria server with this proto file, enable HttpJsonTranscoding, and call

curl -XPOST -H 'content-type: application/json; charset=utf-8; protocol=gRPC' http://localhost:8080'/MyService/TestArmeriaGrpcIntFailure' -d '{
  "number": 10.12
}'

We get:

< HTTP/1.1 503 Service Unavailable
< content-type: application/json; charset=utf-8
< grpc-status: 14
< content-length: 37
< server: Armeria/1.26.2
< date: Wed, 10 Jan 2024 15:21:11 GMT
<

{"code":14,"grpc-code":"UNAVAILABLE"}

Note the 10.12, which is a valid json Number, but can't be parsed to a Long.

I've tried to check the source code, and I see that a Long.parseLong is used, and it seems the NumberFormatException raised is catched here:

still I don't know why it results in a UNAVAILABLE status, but at least it would be nice to be able to log something in such case (or have a custom mapping exception handler).

@eottabom
Copy link
Contributor

eottabom commented Jan 10, 2024

I think it's the wrong request.

To use HttpJsonTranscoding, the HttpRule must be defined.

import "google/api/annotations.proto";
service MyService {
    rpc TestArmeriaGrpcIntFailure(TestArmeriaGrpcIntFailureRequest) returns (TestArmeriaGrpcIntFailureResponse) {
        option (google.api.http) = {
            post: "/test/armeria" // Add path
            body: "number"
        };
    };
}

message TestArmeriaGrpcIntFailureRequest {
    int64 number = 1;
}

message TestArmeriaGrpcIntFailureResponse {
    int64 number = 1;
}

curl -XPOST -H 'content-type: application/json; charset=utf-8; http://localhost:8080/test/armeria -d '{
"number": 10.12
}'

client (rest call) -> (in server) grpc transcoding -> (in server) gRPC call

gRPC code 14 = The service is currently unavailable. This is most likely a transient condition, which can be corrected by retrying with a backoff. Note that it is not always safe to retry non-idempotent operations.
https://grpc.github.io/grpc/core/md_doc_statuscodes.html

I think it would be good to refer to this.

https://github.com/line/armeria/blob/main/grpc/src/test/proto/testing/grpc/transcoding.proto

https://github.com/line/armeria/blob/main/grpc/src/test/java/com/linecorp/armeria/it/grpc/GrpcHttpJsonTranscodingServiceAnnotatedAuthServiceTest.java

https://learn.microsoft.com/ko-kr/aspnet/core/grpc/json-transcoding-binding?view=aspnetcore-8.0

@xhanin
Copy link
Author

xhanin commented Jan 11, 2024

My description was probably not clear enough, sorry for that. The option (google.api.http) is not required, we already use the json call successfully as long as the number value can be converted to a Long.

This works:

curl -XPOST -H 'content-type: application/json; charset=utf-8; protocol=gRPC' http://localhost:8080'/MyService/TestArmeriaGrpcIntFailure' -d '{
  "number": 10
}'

This doesn't work:

curl -XPOST -H 'content-type: application/json; charset=utf-8; protocol=gRPC' http://localhost:8080'/MyService/TestArmeriaGrpcIntFailure' -d '{
  "number": 10.12
}'

I'm perfectly fine that it doesn't work, because 10.12 is not a valid int64. The problem is that it fails with nothing in the logs, no option to catch the conversion failure, and a misleading 503 http error code. It took us several hours in a production environment to figure out why we were getting 503 errors, first investigating the servers and networks stability, before noticing that the caller had just added decimals to the number.

Does it make more sense with those additional details?

@trustin trustin added the defect label Mar 11, 2024
@ikhoon
Copy link
Contributor

ikhoon commented Mar 27, 2024

The invalid input causes InvalidProtocolBufferException which is a subtype of IOException.
IOException is translated into UNAVAILABLE.

return Status.UNAVAILABLE.withCause(t);
}

You can use GrpcExceptionHandlerFunction as a workaround to convert InvalidProtocolBufferException to a proper gRPC status until we fix it.

@xhanin
Copy link
Author

xhanin commented Mar 27, 2024

Thank you for your investigation and the workaround!

@jaeseung-bae
Copy link
Contributor

INVALID_ARGUMENT seems a proper return code among other codes(https://grpc.io/docs/guides/status-codes/).
Is it okay to return INVALID_ARGUMENT when InvalidProtocolBufferException is thrown?
Can I try?

  • InvalidProtocolBufferException: Thrown when a protocol message being parsed is invalid in some way, e.g. it contains a malformed varint or a negative byte length.
  • INVALID_ARGUMENT: The client specified an invalid argument. Note that this differs from FAILED_PRECONDITIONINVALID_ARGUMENT indicates arguments that are problematic regardless of the state of the system (e.g., a malformed file name).

@jrhee17 jrhee17 added the sprint Issues for OSS Sprint participants label Apr 1, 2024
@ikhoon
Copy link
Contributor

ikhoon commented Apr 1, 2024

INVALID_ARGUMENT sounds good to me.

jrhee17 pushed a commit that referenced this issue Apr 8, 2024
Motivation:
- Closes #5379 

Modifications:

- Add corner case(`InvalidProtocolBufferException`) handling logic that
returns `INVALID_ARGUMENT` to give more helpful description to clients.
- Modify `rpc EchoWrappers()` in transcoding.proto to accept
http-post-binding in order to add integration test for type-mismatched
json value
- Add unit-test, integration-test

Result:

- Closes #5379 
- Describe the consequences that a user will face after this PR is
merged.
- User gets `INVALID_ARGUMENT` instead of `UNAVAILABLE` for parsing
exception when using `HttpJsonTranscoding`.
<!--
Visit this URL to learn more about how to write a pull request
description:

https://armeria.dev/community/developer-guide#how-to-write-pull-request-description
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect sprint Issues for OSS Sprint participants
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants