grpc: client integration tests, Grpc::AsyncClientImpl behavior changes.#2427
grpc: client integration tests, Grpc::AsyncClientImpl behavior changes.#2427htuch merged 6 commits intoenvoyproxy:masterfrom
Conversation
This PR is the next in the series of patches to integrate the Google gRPC client. There are two parts: 1. The bulk of the existing Grpc::AsyncClientImpl unit tests have been re-expressed as TCP loopback integration tests with a FakeUpstream. This allows direct reuse with the Google gRPC client libraries, since they are a blackbox that can't be easily mocked internally. The test is also now paramaterized by client type, with the Google gRPC implementation elided in this PR. 2. To produce behavioral parity between clients, Grpc::AsyncClientImpl has been modified where its behavior differed from the Google gRPC client (which is effectively the gRPC reference implementation). Specifically, initial/trailer metadata callbacks, error handling and HTTP -> gRPC status mapping behavior have been modified. Remote stream closes, even with OK grpc-status now terminate the stream regardless of local stream close behavior. Testing: Unit/integration tests as described above. Risk Level: Medium (behavior changes to Grpc::AsyncClientImpl, used for config discovery, rate limiting, access logs and metrics service). Signed-off-by: Harvey Tuch <htuch@google.com>
|
@htuch I will look later today, can you check failing tests? |
| return; | ||
| } | ||
|
|
||
| trailers_only_ = false; |
There was a problem hiding this comment.
This could be inaccurate. The gRPC response spec is Response → (Response-Headers *Length-Prefixed-Message Trailers) / Trailers-Only so it is possible that a non-trailers-only response with a headers followed by a trailers if a non-immediate error happens.
There was a problem hiding this comment.
Yeah, good point, I have some independent thread with the gRPC team on this specific scenario, I know how to fix this in Grpc::AsyncClientImpl, I don't know how to fix it in the Google gRPC integration though.
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
| void AsyncStreamImpl::onHeaders(Http::HeaderMapPtr&& headers, bool end_stream) { | ||
| ASSERT(!remote_closed_); | ||
| callbacks_.onReceiveInitialMetadata(std::move(headers)); | ||
| const auto http_response_status = Http::Utility::getResponseStatus(*headers); |
There was a problem hiding this comment.
I think this is a illegal use of headers as you moved it above.
There was a problem hiding this comment.
I agree, surprised the compiler/sanitizers didn't catch that. Will fix.
Signed-off-by: Harvey Tuch <htuch@google.com>
mattklein123
left a comment
There was a problem hiding this comment.
LGTM, nice tests. One question.
| Http::HeaderMapPtr empty_trailers = std::make_unique<Http::HeaderMapImpl>(); | ||
| callbacks_.onReceiveTrailingMetadata(std::move(empty_trailers)); | ||
| streamError(Status::GrpcStatus::Unknown); | ||
| return; |
There was a problem hiding this comment.
nit: return not needed
Trying to understand this change in general. So we might raise some data, but then fail the stream after that?
There was a problem hiding this comment.
Yeah; matching the behavior in Grpc::GoogleAsyncClientImpl here. It has to do with how we handle the situation of failure. The Google gRPC client basically just tells you something like "receives from now on aren't possible, so you must issue a Finish operation". You issue the Finish operation and it then gives you status and trailing metadata at some point in the future.
I'm not sure about the validity of trailing metadata on error scenarios like this, but I'm seeing the Google gRPC C++ client library deliver the message even in the above error scenario.
@vjpai for gRPC C++ team visibility.
There was a problem hiding this comment.
Sorry, I joined this conversation in the middle so I don't know the full-context, but it is our principle in gRPC that client-side Finish is always valid. It's always valid to know the outcome of the RPC that you started, and any trailing metadata that accompanies the Finish is valid whether or not it's a Status::OK response. Was that what this topic was about?
There was a problem hiding this comment.
@vjpai I think what you write is basically what we've implemented, so it's all good. Thanks for the clarification.
Signed-off-by: Harvey Tuch <htuch@google.com>
…r changes. (envoyproxy#2427)" This reverts commit c680300.
Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>
Signed-off-by: GitHub Action <noreply@github.com> Co-authored-by: jpsim <jpsim@users.noreply.github.com> Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: GitHub Action <noreply@github.com> Co-authored-by: jpsim <jpsim@users.noreply.github.com> Signed-off-by: JP Simard <jp@jpsim.com>
This PR is the next in the series of patches to integrate the Google gRPC client. There are two
parts:
The bulk of the existing Grpc::AsyncClientImpl unit tests have been re-expressed as TCP loopback
integration tests with a FakeUpstream. This allows direct reuse with the Google gRPC client
libraries, since they are a blackbox that can't be easily mocked internally. The test is also now
paramaterized by client type, with the Google gRPC implementation elided in this PR.
To produce behavioral parity between clients, Grpc::AsyncClientImpl has been modified where its
behavior differed from the Google gRPC client (which is effectively the gRPC reference
implementation). Specifically, initial/trailer metadata callbacks, error handling and HTTP ->
gRPC status mapping behavior have been modified. Remote stream closes, even with OK grpc-status
now terminate the stream regardless of local stream close behavior.
Testing: Unit/integration tests as described above.
Risk Level: Medium (behavior changes to Grpc::AsyncClientImpl, used for config discovery, rate limiting, access logs and metrics service).
Signed-off-by: Harvey Tuch htuch@google.com