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

Map http code 429 to grpc RESOURCE_EXHAUSTED #5928

Open
yunjizhong opened this issue Oct 1, 2024 · 4 comments
Open

Map http code 429 to grpc RESOURCE_EXHAUSTED #5928

yunjizhong opened this issue Oct 1, 2024 · 4 comments
Labels

Comments

@yunjizhong
Copy link

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

@limminjeong98
Copy link

limminjeong98 commented Oct 6, 2024

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.

@trustin trustin added the defect label Oct 7, 2024
@trustin
Copy link
Member

trustin commented Oct 7, 2024

It looks like the mappings in statuscodes.md were removed long time ago:
grpc/grpc@bb04e07

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
Copy link
Member

trustin commented Oct 7, 2024

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

@AngerM-DD
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants