grpc_http1_bridge: correctly set downstream status for trailers-only …

…gRPC responses (envoyproxy#14903)

The gRPC bridge implementation will now propagate gRPC failure status when the response is
trailers-only, which is an optimization allowed by the spec

Signed-off-by: John Esmet <john.esmet@gmail.com>

docs/root/version_history/current.rst

@@ -56,6 +56,7 @@ Bug Fixes
 * buffer: tighten network connection read and write buffer high watermarks in preparation to more careful enforcement of read limits. Buffer high-watermark is now set to the exact configured value; previously it was set to value + 1.
 * fault injection: stop counting as active fault after delay elapsed. Previously fault injection filter continues to count the injected delay as an active fault even after it has elapsed. This produces incorrect output statistics and impacts the max number of consecutive faults allowed (e.g., for long-lived streams). This change decreases the active fault count when the delay fault is the only active and has gone finished.
 * grpc-web: fix local reply and non-proto-encoded gRPC response handling for small response bodies. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling` to false.
+* grpc_http_bridge: the downstream HTTP status is now correctly set for trailers-only responses from the upstream.
 * http: disallowing "host:" in request_headers_to_add for behavioral consistency with rejecting :authority header. This behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_host_like_authority` to false.
 * http: reverting a behavioral change where upstream connect timeouts were temporarily treated differently from other connection failures. The change back to the original behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure` to false.
 * listener: prevent crashing when an unknown listener config proto is received and debug logging is enabled.

source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc

@@ -44,16 +44,28 @@ Http::FilterHeadersStatus Http1BridgeFilter::encodeHeaders(Http::ResponseHeaderM
     chargeStat(headers);
   }
 
-  if (!do_bridging_ || end_stream) {
+  if (!do_bridging_) {
+    return Http::FilterHeadersStatus::Continue;
+  }
+
+  response_headers_ = &headers;
+  if (end_stream) {
+    // We may still need to set an http status and content length based on gRPC trailers
+    // present in the response headers. This is known as a gRPC trailers-only response.
+    // If the grpc status is non-zero, this will change the response code.
+    updateHttpStatusAndContentLength(headers);
     return Http::FilterHeadersStatus::Continue;
   } else {
-    response_headers_ = &headers;
     return Http::FilterHeadersStatus::StopIteration;
   }
 }
 
 Http::FilterDataStatus Http1BridgeFilter::encodeData(Buffer::Instance&, bool end_stream) {
-  if (!do_bridging_ || end_stream) {
+  if (!do_bridging_) {
+    return Http::FilterDataStatus::Continue;
+  }
+
+  if (end_stream) {
     return Http::FilterDataStatus::Continue;
   } else {
     // Buffer until the complete request has been processed.
@@ -67,35 +79,62 @@ Http::FilterTrailersStatus Http1BridgeFilter::encodeTrailers(Http::ResponseTrail
   }
 
   if (do_bridging_) {
-    // Here we check for grpc-status. If it's not zero, we change the response code. We assume
-    // that if a reset comes in and we disconnect the HTTP/1.1 client it will raise some type
-    // of exception/error that the response was not complete.
-    const Http::HeaderEntry* grpc_status_header = trailers.GrpcStatus();
-    if (grpc_status_header) {
-      uint64_t grpc_status_code;
-      if (!absl::SimpleAtoi(grpc_status_header->value().getStringView(), &grpc_status_code) ||
-          grpc_status_code != 0) {
-        response_headers_->setStatus(enumToInt(Http::Code::ServiceUnavailable));
-      }
-      response_headers_->setGrpcStatus(grpc_status_header->value().getStringView());
-    }
-
-    const Http::HeaderEntry* grpc_message_header = trailers.GrpcMessage();
-    if (grpc_message_header) {
-      response_headers_->setGrpcMessage(grpc_message_header->value().getStringView());
-    }
-
-    // Since we are buffering, set content-length so that HTTP/1.1 callers can better determine
-    // if this is a complete response.
-    response_headers_->setContentLength(
-        encoder_callbacks_->encodingBuffer() ? encoder_callbacks_->encodingBuffer()->length() : 0);
+    // We're bridging, so we need to process trailers and set the http status, content length,
+    // grpc status, and grpc message from those trailers.
+    doResponseTrailers(trailers);
   }
 
   // NOTE: We will still write the trailers, but the HTTP/1.1 codec will just eat them and end
   //       the chunk encoded response which is what we want.
   return Http::FilterTrailersStatus::Continue;
 }
 
+void Http1BridgeFilter::updateHttpStatusAndContentLength(
+    const Http::ResponseHeaderOrTrailerMap& trailers) {
+  // Here we check for grpc-status. If it's not zero, we change the response code. We assume
+  // that if a reset comes in and we disconnect the HTTP/1.1 client it will raise some type
+  // of exception/error that the response was not complete.
+  const Http::HeaderEntry* grpc_status_header = trailers.GrpcStatus();
+  if (grpc_status_header) {
+    uint64_t grpc_status_code;
+    if (!absl::SimpleAtoi(grpc_status_header->value().getStringView(), &grpc_status_code) ||
+        grpc_status_code != 0) {
+      response_headers_->setStatus(enumToInt(Http::Code::ServiceUnavailable));
+    }
+  }
+
+  // Since we are buffering, set content-length so that HTTP/1.1 callers can better determine
+  // if this is a complete response.
+  response_headers_->setContentLength(
+      encoder_callbacks_->encodingBuffer() ? encoder_callbacks_->encodingBuffer()->length() : 0);
+}
+
+// Set grpc response headers based on incoming trailers. Sometimes, the incoming
+// trailers are in fact upstream headers, in the case of a gRPC trailers-only response.
+void Http1BridgeFilter::updateGrpcStatusAndMessage(
+    const Http::ResponseHeaderOrTrailerMap& trailers) {
+  const Http::HeaderEntry* grpc_status_header = trailers.GrpcStatus();
+  if (grpc_status_header) {
+    response_headers_->setGrpcStatus(grpc_status_header->value().getStringView());
+  }
+
+  const Http::HeaderEntry* grpc_message_header = trailers.GrpcMessage();
+  if (grpc_message_header) {
+    response_headers_->setGrpcMessage(grpc_message_header->value().getStringView());
+  }
+}
+
+// Process response trailers. This involves setting an appropriate http status and content length,
+// as well as gRPC status and message headers.
+void Http1BridgeFilter::doResponseTrailers(const Http::ResponseHeaderOrTrailerMap& trailers) {
+  // First we need to set an HTTP status based on the gRPC status in `trailers`.
+  // We also set content length based on whether there exists an encoding buffer.
+  updateHttpStatusAndContentLength(trailers);
+
+  // Finally we set the grpc status and message headers based on `trailers`.
+  updateGrpcStatusAndMessage(trailers);
+}
+
 void Http1BridgeFilter::setupStatTracking(const Http::RequestHeaderMap& headers) {
   cluster_ = decoder_callbacks_->clusterInfo();
   if (!cluster_) {

source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.h

@@ -52,6 +52,9 @@ class Http1BridgeFilter : public Http::StreamFilter {
 private:
   void chargeStat(const Http::ResponseHeaderOrTrailerMap& headers);
   void setupStatTracking(const Http::RequestHeaderMap& headers);
+  void doResponseTrailers(const Http::ResponseHeaderOrTrailerMap& trailers);
+  void updateGrpcStatusAndMessage(const Http::ResponseHeaderOrTrailerMap& trailers);
+  void updateHttpStatusAndContentLength(const Http::ResponseHeaderOrTrailerMap& trailers);
 
   Http::StreamDecoderFilterCallbacks* decoder_callbacks_{};
   Http::StreamEncoderFilterCallbacks* encoder_callbacks_{};

test/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter_test.cc

@@ -224,6 +224,29 @@ TEST_F(GrpcHttp1BridgeFilterTest, HandlingBadGrpcStatus) {
   EXPECT_EQ("foo", response_headers.get_("grpc-message"));
 }
 
+// Regression test for https://github.com/envoyproxy/envoy/issues/14872 testing trailers-only
+// responses
+TEST_F(GrpcHttp1BridgeFilterTest, HandlingBadGrpcStatusTrailersOnlyResponse) {
+  Http::TestRequestHeaderMapImpl request_headers{
+      {"content-type", "application/grpc"},
+      {":path", "/lyft.users.BadCompanions/GetBadCompanions"}};
+  EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false));
+  Buffer::OwnedImpl data("hello");
+  EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(data, false));
+  Http::TestRequestTrailerMapImpl request_trailers{{"hello", "world"}};
+  EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.decodeTrailers(request_trailers));
+
+  // gRPC responses may optimize and put would-be-trailers in the headers frame if there are no data
+  // frames. The gRPC spec refers to this a a "Trailers-Only" response.
+  Http::TestResponseHeaderMapImpl response_headers{
+      {":status", "200"}, {"grpc-status", "1"}, {"grpc-message", "foo"}};
+  EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, true));
+  EXPECT_EQ("503", response_headers.get_(":status"));
+  EXPECT_EQ("0", response_headers.get_("content-length"));
+  EXPECT_EQ("1", response_headers.get_("grpc-status"));
+  EXPECT_EQ("foo", response_headers.get_("grpc-message"));
+}
+
 } // namespace
 } // namespace GrpcHttp1Bridge
 } // namespace HttpFilters