armeria icon indicating copy to clipboard operation
armeria copied to clipboard

Map http code 429 to grpc RESOURCE_EXHAUSTED

Open yunjizhong opened this issue 1 year ago • 4 comments

hi team, in armeria's httpStatusToGrpcCode, it maps http 429 to grpc UNAVILABLE

However, 429 Too Manay Requests should be mapped to grpc 8 RESOURCE_EXHAUSTED, according to https://chromium.googlesource.com/external/github.com/grpc/grpc/+/refs/tags/v1.21.4-pre1/doc/statuscodes.md

yunjizhong avatar Oct 01 '24 02:10 yunjizhong

Hi, in the comments of the GrpcStatus.java, it says that Armeria has copied the contents of the GrpcUtil.java from the grpc-java project.

/**
     * Maps HTTP error response status codes to transport codes, as defined in <a
     * href="https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md">
     * http-grpc-status-mapping.md</a>. Never returns a status for which {@code status.isOk()} is
     * {@code true}.
     *
     * <p>Copied from
     * <a href="https://github.com/grpc/grpc-java/blob/master/core/src/main/java/io/grpc/internal/GrpcUtil.java">
     * GrpcUtil.java</a>
     */
    public static Status httpStatusToGrpcStatus(int httpStatusCode) {
        return httpStatusToGrpcCode(httpStatusCode).toStatus()
                                                   .withDescription("HTTP status code " + httpStatusCode);
    }

    private static Status.Code httpStatusToGrpcCode(int httpStatusCode) {
        if (httpStatusCode >= 100 && httpStatusCode < 200) {
            // 1xx. These headers should have been ignored.
            return Status.Code.INTERNAL;
        }
        switch (httpStatusCode) {
            case HttpURLConnection.HTTP_BAD_REQUEST:  // 400
            case 431: // Request Header Fields Too Large
                return Status.Code.INTERNAL;
            case HttpURLConnection.HTTP_UNAUTHORIZED:  // 401
                return Status.Code.UNAUTHENTICATED;
            case HttpURLConnection.HTTP_FORBIDDEN:  // 403
                return Status.Code.PERMISSION_DENIED;
            case HttpURLConnection.HTTP_NOT_FOUND:  // 404
                return Status.Code.UNIMPLEMENTED;
            case 429:  // Too Many Requests
            case HttpURLConnection.HTTP_BAD_GATEWAY:  // 502
            case HttpURLConnection.HTTP_UNAVAILABLE:  // 503
            case HttpURLConnection.HTTP_GATEWAY_TIMEOUT:  // 504
                return Status.Code.UNAVAILABLE;
            default:
                return Status.Code.UNKNOWN;
        }
    }

Why don't you open an issue in the grpc-java project to suggest a more detailed mapping? Thank you.

limminjeong98 avatar Oct 06 '24 04:10 limminjeong98

It looks like the mappings in statuscodes.md were removed long time ago: https://github.com/grpc/grpc/commit/bb04e070b3177d7bd8e69c4086dc420f662b8f28

The latest mappings are here: https://github.com/grpc/grpc/blob/v1.66.2/doc/http-grpc-status-mapping.md and 429 Too Many Requests is mapped into UNAVAILABLE.

That being said, we probably need to open an issue at https://github.com/grpc/grpc for clarification first?

trustin avatar Oct 07 '24 08:10 trustin

I asked a question at https://groups.google.com/g/grpc-io (pending approval). Let's wait for gRPC team's answer.

trustin avatar Oct 07 '24 08:10 trustin

Right, the docs say thats for mapping is for client side only (if a GRPC client receives a non 200 HTTP code, what status should it signal back to the calling code), not necessarily the other way around. I think its reasonable for us to map a server side middleware that throws a 429 to RESOURCE_EXHAUSTED status code when we send it on the wire back to the caller.

AngerM-DD avatar Oct 09 '24 16:10 AngerM-DD