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

Allow client-side parsing of the grpc-status-details-bin #5485

Closed
koiuo opened this issue Jul 7, 2022 · 7 comments · Fixed by #6662
Closed

Allow client-side parsing of the grpc-status-details-bin #5485

koiuo opened this issue Jul 7, 2022 · 7 comments · Fixed by #6662
Assignees
Labels
P2 Type: Feature New features or improvements in behavior

Comments

@koiuo
Copy link

koiuo commented Jul 7, 2022

Please see the FAQ in our main README.md before submitting your issue.

Use case(s) - what problem will this feature solve?

Integrating go service into an existing infrastructure with proto services written in C++, Java, Python, where the format of the grpc-status-details-bin is not enforced.

This would be consistent with other clients, that only suggest the expected format, but do not enforce it.

Proposed Solution

Right now we can't even detect, whether grpc-status-details-bin can be parsed correctly.
The simplest solution would be a flag to disable the extended errors support entirely

func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) {
  ...
case "grpc-status-details-bin":
  if grpcStatusDetailsEnabled {
    statusGen, err = decodeGRPCStatusDetails(hf.Value)
    if err != nil {
      headerError = fmt.Sprintf("transport: malformed grpc-status-details-bin: %v", err)
    }
  } else {
    mdata[hf.Name] = append(mdata[hf.Name], hf.Value)
  }
...
@koiuo koiuo added the Type: Feature New features or improvements in behavior label Jul 7, 2022
@dfawley
Copy link
Member

dfawley commented Jul 7, 2022

This would be consistent with other clients, that only suggest the expected format, but do not enforce it.

Unfortunately, this feature was not well defined (e.g. there was no gRFC) when it was implemented. The three languages all have different implementations based on their interpretations of the definition. Your best bet for now for cross-language compatibility is to always use a google.rpc.Status proto for this field. As for unifying the implementations, it would probably be worth filing a bug in the github.com/grpc/grpc repo about this. It is a known issue, but we haven't had many/any users complain about the differences in behavior, and it would be helpful to have more examples of users impacted by it.

The simplest solution would be a flag to disable the extended errors support entirely

Since this is a reserved header, I don't believe we should be passing it to the application.

Why not use a custom trailer key instead?

@koiuo
Copy link
Author

koiuo commented Jul 8, 2022

Doug, thanks a lot for your quick response.

Why not use a custom trailer key instead?

It's an existing ecosystem of thousands of apps developed in other languages which already use grpc-status-details-bin to pass custom proprietary entity.

Effectively we can't use Go in this ecosystem due to how grpc-go handles grpc-status-details-bin (well, we maintain our own fork, which is highly unsustainable).

Since this is a reserved header, I don't believe we should be passing it to the application.

Can you suggest any workarounds for this?

It would be very helpful to get access to reserved metadata on some level.
One option I can think of is to expose such metadata in the interceptor. And it can be cleaned up somewhere later in the processing chain.
Another is exposing a flag (grpc.CallOption or even env var) to pass reserved metadata to the caller (for those clients who know, what they are doing).


Another issue I'd like to hightlight about the current implementation, is that it can not identify unmarshalling issues. It unmarshalls our proprietary instance into google.rpc.Status without errors, but later, since the data does not make sense, entire request fails with Unexpected EOF (not even an invalid header format error). I can file a separate issue with a reproducing scenario to improve invalid headers handling if you think it's something worth investigating/fixing.

@dfawley
Copy link
Member

dfawley commented Jul 12, 2022

I brought this issue up with the other language leads. We didn't decide on anything concrete yet, but we will discuss it further later this week and come up with a plan that can work for all the languages. I'm hopeful we'll be able to make things work more gracefully with the implementations that allowed arbitrary binary data without breaking existing users.

Another issue I'd like to hightlight about the current implementation, is that it can not identify unmarshalling issues. It unmarshalls our proprietary instance into google.rpc.Status without errors, but later, since the data does not make sense, entire request fails with Unexpected EOF (not even an invalid header format error).

I'm definitely curious about this, as it's surprising to me that it would get past the unmarshalling step but eventually fail the RPC due to the status.

@dfawley
Copy link
Member

dfawley commented Nov 28, 2022

Sorry for the late response here. We would like to provide a way for the user to access the raw grpc-status-details-bin via a new field in the status.Status, and not error if it's of the "wrong" type, but our team doesn't have the time to take on this task right now. In the short to medium term, if someone would like to submit a mini-design and then do the work here, we can guide & review. Thanks!

@dfawley dfawley removed their assignment Nov 28, 2022
@CemGurhan
Copy link

I'd like to give this a go! @dfawley I was wondering, when you say a new field in the status.Status type, do you mean the type defined in internal here, or do you mean the proto definition, which I'm assuming is defined here?

@dfawley
Copy link
Member

dfawley commented Sep 21, 2023

So sorry I missed your message @CemGurhan .. I'm going to try to take this on this week to clean up some other discrepancies I found between languages in the general handling of statuses.

@CemGurhan
Copy link

No problem at all. If you need any help, feel free to let me know!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P2 Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants