gRPC programming guideline: When to convert status codes?

[Wensheng Tang]

Dear gRPC developers,

We notice that there are many cases gRPC or other status codes are being converted internally through our code analyzer.

For example, in

https://github.com/grpc/grpc/issues/23775

The error code from

```c

// may return GRPC_STATUS_INVALID_ARGUMENT

status = gsec_aead_crypter_decrypt_iovec(...);

if (status != GRPC_STATUS_OK) {

    return GRPC_STATUS_INTERNAL;
}

```

, the original status code is dropped. I believe such a conversion is not needed. Besides, it may not present the real errors to its client, which may not follow guidelines in https://grpc.github.io/grpc/core/md_doc_statuscodes.html.

On another hand, in the same function, it does keep the original status codes, some times.

```c

grpc_status_code status = verify_frame_header(...);
if (status != GRPC_STATUS_OK) {
    return status;
}

```

Your team closes my issues so quickly so I am not sure if here is the right place to report. Shall we confine a clear and general guideline of when to convert status codes internally?

[Mark D. Roth]

I don't actually see a problem here.  The fact that some of these internal functions may use grpc_status_code to return their intent to the caller doesn't mean that that same code should be returned from gRPC to the application.  As a hypothetical example (I haven't looked at the function and don't know what codes it might actually return), gsec_aead_crypter_decrypt_iovec() might return GRPC_STATUS_INVALID_ARGUMENT if the cryptographic data is invalid, but that's not one of the codes that we're allowed to return from gRPC, as per https://github.com/grpc/grpc/blob/master/doc/statuscodes.md.  I think GRPC_STATUS_INVALID is actually the right thing to return in that situation, because it reflects a clear bug of some sort in the gRPC stack.

If you have a specific use-case in which you are using gRPC and believe you are getting the wrong status code, you can file a bug about that.  But so far, what you're pointing out doesn't seem to reflect an actual problem.

--
You received this message because you are subscribed to the Google Groups "grpc.io" group.
To unsubscribe from this group and stop receiving emails from it, send an email to grpc-io+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/grpc-io/c0fcae1a-7070-43b0-949e-1b043b8f7233n%40googlegroups.com.

--

Mark D. Roth <ro...@google.com>
Software Engineer
Google, Inc.

[Wensheng Tang]

Hi Mark,

I fully understand that the gRPC does not allow some status codes to return to its user. However, the second function `verify_frame_header()` does return `GRPC_STATUS_FAILED_PRECONDITION`, which is also on the blocklist. Then this function does return some status codes that should not propagate a client. In fact, that is the thing that makes me confused about why such a conversion is needed. So I ask the community if gRPC does have clear criteria on when to convert status codes. I believe it can provide more guidance developers that use gRPC to do better handling with gRPC status codes.

Best regards,

[Mark D. Roth]

In general, we do not have a hard-and-fast rule about how implementation code handles this.  It's up to the developers and code reviewers to make sure the overall behavior is correct.

I don't see a problem with the presence of one function deep inside of the implementation returning a status that gRPC would not return from the application.  It's still quite possible for some other piece of code to remap the status before it gets returned to the application.

If you are seeing a specific case in which gRPC is returning an incorrect status code to the application, you can file an issue about that.  Otherwise, I don't see any concrete concern here.

To view this discussion on the web visit https://groups.google.com/d/msgid/grpc-io/ad2fd35b-8f99-4e6d-944e-81578ba2a0d8n%40googlegroups.com.

[Christopher Warrington - MSFT]

On Wednesday, August 19, 2020 at 10:32:53 AM UTC-7, Wensheng Tang wrote:

> I fully understand that the gRPC does not allow some status codes to
> return to its user. However, the second function `verify_frame_header()`
> does return `GRPC_STATUS_FAILED_PRECONDITION`, which is also on the
> blocklist. Then this function does return some status codes that should
> not propagate a client. In fact, that is the thing that makes me confused
> about why such a conversion is needed. So I ask the community if gRPC does
> have clear criteria on when to convert status codes. I believe it can
> provide more guidance developers that use gRPC to do better handling with
> gRPC status codes.

It's pretty common when handling errors from crypto code to map all failures
to a single visible response. It's a defense-in-depth minimization of
information leakage. I _suspect_ that's part of the reason that
gsec_aead_crypter_decrypt_iovec is mapped to an internal error, while
verify_frame_header errors are allowed to propagate.

--
Christopher Warrington
Microsoft Corp.