Envoy: grpc-json transcoder: convert grpc-message to body in response

[maheshkanote]

Title: Envoy's grpc-json transcoder

Description:

I've a REST client and a gRPC server and I'm using Envoy's grpc-json transcoder.
When an exception is thrown by gRPC server with gRPC error code and gRPC error message,
on the client side I receive corresponding HTTP error status code (grpc-error-code converted to http error code) and error message like "Invalid userid" is set in response header "grpc-message".
Now I want gRPC error message to appear in body rather than response header. Is it possible with Envoy? If yes, How can I do it?

[mattklein123]

@lizan

[lizan]

No, currently Envoy's grpc-json filter doesn't support converting grpc-message into body.

This will be a feature request that we can work in future.

[swarno]

I too have the same problem using the grpc-json transcoder. I am trying to figure out on how to send the Error Response as part of Body for RestClient to consume.

Can you please put this on priority and there are will be lot of Rest Clients that are consuming our GRPC calls via envoy transcoder.

[mattklein123]

Switching over to help wanted / feature request.

[baranov1ch]

I've looked at this a bit.

2 caveats here:
1) we need to send body (serialized rpc.Status) from encodeTrailers(). When doing unary grpc calls, transcoder buffers everything up to encodeTrailers() call, which means that response headers are still buffered.
If one just calls encoder_callbacks_->addEncodedData(), response body will be put into the wire before response headers, due to how ConnectionManagerImpl::ActiveStream::addEncodedData works:

if (state_.filter_call_state_ == 0 ||
      (state_.filter_call_state_ & FilterCallState::EncodeHeaders) ||
      (state_.filter_call_state_ & FilterCallState::EncodeData)) {
    // Make sure if this triggers watermarks, the correct action is taken.
    state_.encoder_filters_streaming_ = streaming;
    // If no call is happening or we are in the decode headers/data callback, buffer the data.
    // Inline processing happens in the decodeHeaders() callback if necessary.
    filter.commonHandleBufferData(data);
  } else if (state_.filter_call_state_ & FilterCallState::EncodeTrailers) {
    // In this case we need to inline dispatch the data to further filters. If those filters
    // choose to buffer/stop iteration that's fine.
    encodeData(&filter, data, false);
  } else {
    ...
  }

TL;DR: if StreamEncoderFilterCallbacks::addEncodedData() is called inside filter's encodeTrailers method, data will be sent immediately, without consulting request/response state, i.e. that response headers haven't been sent yet.

To mitigate this, one cloud use StreamEncoderFilterCallbacks::continueEncoding to * Continue iterating through the filter chain with buffered headers and body data. before sending serialized status. But again, when it is called inside filter's encodeTrailers, it leads to immediate http stream termination, which, later causes assertion failure. The reason lies in ConnectionManagerImpl::ActiveStreamFilterBase::commonContinue, which does the following:

if (trailers()) {
    doTrailers();
}

doTrailers() in turn re-enters ConnectionManagerImpl::ActiveStream::encodeTrailers, asks codec (response_encoder_) to encode trailers, which calls Http1::ServerConnectionImpl::onEncodeComplete, which resets its active_request_ fields. But since we're already inside of ConnectionManagerImpl::ActiveStream::encodeTrailers doing filter iteration, response_encoder_->encodeTrailers(); will be called once more and will cause assertion failure (Http1::ServerConnectionImpl::active_request_ must be set).

Perhaps there is a way to fix such behavior, but it does not look easy/obvious at least to me.

The other workaround would be to buffer body inside ConnectionManagerImpl::ActiveStream::addEncodedData when called inside encodeTrailers, just like it is done in other cases. Or even buffer body for trailers call iff the filter is already buffering (stopped_ = true). I've checked the last option - it works (and does not break any existing tests:)) But I'm not sure if it really does not break anything.

2) rpc.Status descriptor must present in proto-desriptor used by transcoder. The easiest way is to make the user to provide it themselves. OTOH, we could make life easier, since all google.rpc stuff already present inside envoy's own proto descriptor set (default global private descriptor set where all compiled proto-messages register themselves in). But I wasn't able to find a way to get them out of there to add automagically to descriptor set provided by the user.

[lizan]

@baranov1ch Thanks for investigating on this.

1. This sounds like a bug of HCM, which have to be fixed.
   > If one just calls encoder_callbacks_->addEncodedData(), response body will be put into the wire before response headers, due to how ConnectionManagerImpl::ActiveStream::addEncodedData works:

Aside from that, to be more clear, I think the grpc-message would be converted into http body _iff_ the response is trailer-only. If we already converted some of body, then it will be in header for unary and (unfortunately with the possibility to be swallowed) in trailer for streaming case.

1. You should be able to use google::rpc::Status directly (grpc_mux_impl.cc uses that), and use Protobuf::util::MessageToJsonString to convert it to JSON. No descriptor set is involved in this case.

[baranov1ch]

This sounds like a bug of HCM, which have to be fixed.

Will always buffering the body (received from StreamEncoderFilterCallbacks::addEncodedData()) be the acceptable fix? This is how HCM behaves when data is pushed from encodeHeaders/encodeData Or HCM should learn to push headers if addEncodedData is called from encodeTrailers callback?

You should be able to use google::rpc::Status directly (grpc_mux_impl.cc uses that)

in this case possible custom details (which are google.protobuf.Anys) will fail to transcode.

[lizan]

Will always buffering the body (received from StreamEncoderFilterCallbacks::addEncodedData()) be the acceptable fix? This is how HCM behaves when data is pushed from encodeHeaders/encodeData Or HCM should learn to push headers if addEncodedData is called from encodeTrailers callback?

I think that is fine, it should match what addDecodedData for decoder path as well.

in this case possible custom details (which are google.protobuf.Anys) will fail to transcode.

Are you trying to translate (non-well-documented) grpc-status-details-bin header? If not grpc-message is only an utf8 string so it goes to Status.message, and details will be always empty.

[baranov1ch]

Are you trying to translate (non-well-documented) grpc-status-details-bin header?

Yes. While it is indeed non-documented, C-based runtimes and grpc-go use it for serializing status with non-empty details, so de-facto it will work in most cases. Do you think it does not worth doing?

[dkharrat]

I've run into this issue as well. This issue makes handling error messages with REST clients quite inconvenient. There are two aspects to providing error details to REST clients:

• Error Message (provided by grpc-message, and maps to this Status field)
• Error Details (provided by grpc-status-details-bin, and maps to this Status field)

ESP handles both of these quite well. It would be great if this can be supported by Envoy as well. What's the timeline for supporting this?

[klesniewski]

I've run into this issue as well. We want to develop our APIs in gRPC following Google's API Design Guide, which allows us to use gRPC internally and expose a consistent REST interface to our customers (perhaps we expose gRPC as well in the future). As described in the section on errors, error responses are mapped to HTTP+JSON response of following structure (note the status object is nested in another object with "error" field):

{
  "error": {
    "code": 401,
    "message": "Request had invalid credentials.",
    "status": "UNAUTHENTICATED",
    "details": [{
      "@type": "type.googleapis.com/google.rpc.RetryInfo",
      ...
    }]
  }
}

Can we count on having similar behavior in Envoy proxy?

[shaneqld]

+1 on better support for errors. I too am following the Google API Design Guide and using envoy to transcode grpc-json. Having error details in http headers isn't very nice. A serialised Status object would be much nicer for our http1 friends!

(Would happily sponsor someone to implement this, if that's allowed)

[lizan]

@shaneqld +1, feel free to open a PR for this.

[shaneqld]

A few design questions:

1. Should we change the default behaviour, or perhaps have a printOptions option for outputting errors as JSON?
2. If we change the default behaviour, should we remove the HTTP headers containing the grpc errors? I fear this might break consumers expecting the previous behaviour.

[lizan]

1. Should we change the default behaviour, or perhaps have a printOptions option for outputting errors as JSON?

I'd add another options in the filter config, but not in printOptions.

1. If we change the default behaviour, should we remove the HTTP headers containing the grpc errors? I fear this might break consumers expecting the previous behaviour.

No.