grpc-web: Fix 503 handling for gRPC Web text

docs/root/version_history/current.rst

@@ -23,6 +23,7 @@ Bug Fixes
 
 * active http health checks: properly handles HTTP/2 GOAWAY frames from the upstream. Previously a GOAWAY frame due to a graceful listener drain could cause improper failed health checks due to streams being refused by the upstream on a connection that is going away. To revert to old GOAWAY handling behavior, set the runtime feature `envoy.reloadable_features.health_check.graceful_goaway_handling` to false.
 * 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.
+* 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.
 * 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.
 * upstream: fix handling of moving endpoints between priorities when active health checks are enabled. Previously moving to a higher numbered priority was a NOOP, and moving to a lower numbered priority caused an abort.

source/common/runtime/runtime_features.cc

@@ -64,6 +64,7 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.always_nodelay",
     "envoy.reloadable_features.check_ocsp_policy",
     "envoy.reloadable_features.disable_tls_inspector_injection",
+    "envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling",
     "envoy.reloadable_features.hcm_stream_error_on_invalid_message",
     "envoy.reloadable_features.health_check.graceful_goaway_handling",
     "envoy.reloadable_features.http_default_alpn",

source/extensions/filters/http/grpc_web/grpc_web_filter.cc

@@ -7,16 +7,40 @@
 #include "common/common/assert.h"
 #include "common/common/base64.h"
 #include "common/common/empty_string.h"
+#include "common/common/enum_to_int.h"
 #include "common/common/utility.h"
+#include "common/grpc/common.h"
 #include "common/grpc/context_impl.h"
 #include "common/http/headers.h"
 #include "common/http/utility.h"
+#include "common/runtime/runtime_features.h"
 
 namespace Envoy {
 namespace Extensions {
 namespace HttpFilters {
 namespace GrpcWeb {
 
+namespace {
+
+// This is the maximum buffered plaintext data length when we have buffered data in the encoding
+// buffer. This is effectively used (to limit the length of grpc-message) only when we have encoding
+// buffer filled with data. The value is arbitrarily chosen. This can be made configurable when it
+// is required.
+constexpr uint64_t MAX_BUFFERED_PLAINTEXT_LENGTH = 16384;
+
+// This builds grpc-message header value from body data.
+std::string buildGrpcMessage(Buffer::Instance& body_data) {
+  const uint64_t message_length = body_data.length();
+  std::string message;
+  message.reserve(message_length);
+  message.resize(message_length);
+  body_data.copyOut(0, message_length, message.data());
+
+  return Http::Utility::PercentEncoding::encode(message);
+}
+
+} // namespace
+
 Http::RegisterCustomInlineHeader<Http::CustomInlineHeaderRegistry::Type::RequestHeaders>
     accept_handle(Http::CustomHeaders::get().Accept);
 Http::RegisterCustomInlineHeader<Http::CustomInlineHeaderRegistry::Type::RequestHeaders>
@@ -54,6 +78,81 @@ bool GrpcWebFilter::isGrpcWebRequest(const Http::RequestHeaderMap& headers) {
   return false;
 }
 
+bool GrpcWebFilter::isProtoEncodedGrpcWebResponseHeaders(
+    const Http::ResponseHeaderMap& headers) const {
+  // We expect the response headers to have 200 OK status (a valid gRPC, also gRPC-Web, response
+  // needs to have 200 OK status
+  // https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses) and contain
+  // proto-encoded gRPC-Web content-type.
+  return Http::Utility::getResponseStatus(headers) == enumToInt(Http::Code::OK) &&
+         hasProtoEncodedGrpcWebContentType(headers);
+}
+
+// TODO(dio): Move this as a shared utility function.
+bool GrpcWebFilter::hasProtoEncodedGrpcWebContentType(
+    const Http::RequestOrResponseHeaderMap& headers) const {
+  const Http::HeaderEntry* content_type = headers.ContentType();
+  if (content_type != nullptr) {
+    absl::string_view content_type_value = content_type->value().getStringView();
+    // We ignore "parameter" value. Note that "*( ";" parameter )" indicates that there can be
+    // multiple parameters.
+    absl::string_view current_content_type =
+        StringUtil::rtrim(content_type_value.substr(0, content_type_value.find_first_of(';')));
+    // We expect only proto encoding response
+    // https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-WEB.md. And the value of media-type is
+    // case-sensitive https://tools.ietf.org/html/rfc2616#section-3.7.
+    return StringUtil::CaseInsensitiveCompare()(
+               current_content_type, Http::Headers::get().ContentTypeValues.GrpcWebProto) ||
+           StringUtil::CaseInsensitiveCompare()(current_content_type,
+                                                Http::Headers::get().ContentTypeValues.GrpcWeb);
+  }
+  return false;
+}
+
+// If response headers do not contain valid response headers, it needs transformation.
+bool GrpcWebFilter::needsTransformationForNonProtoEncodedResponse(Http::ResponseHeaderMap& headers,
+                                                                  bool end_stream) const {
+  return Runtime::runtimeFeatureEnabled(
+             "envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling") &&
+         // We transform the response unless it is already a gRPC or proto-encoded gRPC-Web
+         // response.
+         !Grpc::Common::isGrpcResponseHeaders(headers, end_stream) &&
+         !isProtoEncodedGrpcWebResponseHeaders(headers);
+}
+
+void GrpcWebFilter::mergeAndLimitNonProtoEncodedResponseData(Buffer::OwnedImpl& output,
+                                                             Buffer::Instance* last_data) {
+  const auto* encoding_buffer = encoder_callbacks_->encodingBuffer();
+  if (encoding_buffer != nullptr) {
+    if (last_data != nullptr) {
+      encoder_callbacks_->addEncodedData(*last_data, false);
+    }
+    encoder_callbacks_->modifyEncodingBuffer([&output](Buffer::Instance& buffered) {
+      // When we have buffered data (encoding buffer is filled), we limit the final buffer length.
+      output.move(buffered, MAX_BUFFERED_PLAINTEXT_LENGTH);
+      buffered.drain(buffered.length());
+    });
+  } else if (last_data != nullptr) {
+    // In the case of local reply and when the response only contains a single data chunk,
+    // "encoding_buffer" is nullptr and we only have filled "last_data".
+    output.move(*last_data);
+    last_data->drain(last_data->length());
+  }
+}
+
+void GrpcWebFilter::setTransformedNonProtoEncodedResponseHeaders(Buffer::Instance* data) {
+  Buffer::OwnedImpl merged_data;
+  // When we have buffered data in encoding buffer, we limit the length of the output to be smaller
+  // than MAX_BUFFERED_PLAINTEXT_LENGTH. However, when we only have "last" data, we send it all.
+  mergeAndLimitNonProtoEncodedResponseData(merged_data, data);
+
+  const std::string grpc_message = buildGrpcMessage(merged_data);
+  response_headers_->setGrpcMessage(grpc_message);
+  response_headers_->setGrpcStatus(Grpc::Utility::httpToGrpcStatus(
+      enumToInt(Http::Utility::getResponseStatus(*response_headers_))));
+  response_headers_->setContentLength(0);
+}
+
 // Implements StreamDecoderFilter.
 // TODO(fengli): Implements the subtypes of gRPC-Web content-type other than proto, like +json, etc.
 Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::RequestHeaderMap& headers, bool) {
@@ -143,27 +242,60 @@ Http::FilterDataStatus GrpcWebFilter::decodeData(Buffer::Instance& data, bool en
 }
 
 // Implements StreamEncoderFilter.
-Http::FilterHeadersStatus GrpcWebFilter::encodeHeaders(Http::ResponseHeaderMap& headers, bool) {
+Http::FilterHeadersStatus GrpcWebFilter::encodeHeaders(Http::ResponseHeaderMap& headers,
+                                                       bool end_stream) {
   if (!is_grpc_web_request_) {
     return Http::FilterHeadersStatus::Continue;
   }
 
   if (doStatTracking()) {
     chargeStat(headers);
   }
+
+  needs_transformation_for_non_proto_encoded_response_ =
+      needsTransformationForNonProtoEncodedResponse(headers, end_stream);
+
   if (is_text_response_) {
     headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.GrpcWebTextProto);
   } else {
     headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.GrpcWebProto);
   }
-  return Http::FilterHeadersStatus::Continue;
+
+  if (end_stream || !needs_transformation_for_non_proto_encoded_response_) {
+    return Http::FilterHeadersStatus::Continue;
+  }
+
+  response_headers_ = &headers;
+  return Http::FilterHeadersStatus::StopIteration;
 }
 
-Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool) {
+Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool end_stream) {
   if (!is_grpc_web_request_) {
     return Http::FilterDataStatus::Continue;
   }
 
+  // When the upstream response (this is also relevant for local reply, since gRPC-Web request is
+  // not a gRPC request which makes the local reply's is_grpc_request set to false) is not a gRPC
+  // response, we set the "grpc-message" header with the upstream body content.
+  if (needs_transformation_for_non_proto_encoded_response_) {
+    const auto* encoding_buffer = encoder_callbacks_->encodingBuffer();
+    if (!end_stream) {
+      // We limit the buffered data in encoding buffer here to eliminate the possibility of
+      // buffering too large data from upstream. Note that the buffered data here will be
+      // transformed as grpc-message later.
+      if (encoding_buffer != nullptr &&
+          encoding_buffer->length() >= MAX_BUFFERED_PLAINTEXT_LENGTH) {
+        return Http::FilterDataStatus::StopIterationNoBuffer;
+      }
+      return Http::FilterDataStatus::StopIterationAndBuffer;
+    }
+
+    ASSERT(response_headers_ != nullptr);
+    needs_transformation_for_non_proto_encoded_response_ = false;
+    setTransformedNonProtoEncodedResponseHeaders(&data);
+    return Http::FilterDataStatus::Continue;
+  }
+
   if (!is_text_response_) {
     // No additional transcoding required if gRPC-Web client asked for binary response.
     return Http::FilterDataStatus::Continue;
@@ -202,6 +334,11 @@ Http::FilterTrailersStatus GrpcWebFilter::encodeTrailers(Http::ResponseTrailerMa
     chargeStat(trailers);
   }
 
+  if (needs_transformation_for_non_proto_encoded_response_) {
+    setTransformedNonProtoEncodedResponseHeaders(nullptr);
+    return Http::FilterTrailersStatus::Continue;
+  }
+
   // Trailers are expected to come all in once, and will be encoded into one single trailers frame.
   // Trailers in the trailers frame are separated by `CRLFs`.
   Buffer::OwnedImpl temp;

source/extensions/filters/http/grpc_web/grpc_web_filter.h

@@ -56,6 +56,13 @@ class GrpcWebFilter : public Http::StreamFilter, NonCopyable {
   void chargeStat(const Http::ResponseHeaderOrTrailerMap& headers);
   void setupStatTracking(const Http::RequestHeaderMap& headers);
   bool isGrpcWebRequest(const Http::RequestHeaderMap& headers);
+  bool isProtoEncodedGrpcWebResponseHeaders(const Http::ResponseHeaderMap& headers) const;
+  bool hasProtoEncodedGrpcWebContentType(const Http::RequestOrResponseHeaderMap& headers) const;
+  bool needsTransformationForNonProtoEncodedResponse(Http::ResponseHeaderMap& headers,
+                                                     bool end_stream) const;
+  void mergeAndLimitNonProtoEncodedResponseData(Buffer::OwnedImpl& output,
+                                                Buffer::Instance* last_data);
+  void setTransformedNonProtoEncodedResponseHeaders(Buffer::Instance* data);
 
   static const uint8_t GRPC_WEB_TRAILER;
   const absl::flat_hash_set<std::string>& gRpcWebContentTypes() const;
@@ -65,11 +72,13 @@ class GrpcWebFilter : public Http::StreamFilter, NonCopyable {
   Http::StreamEncoderFilterCallbacks* encoder_callbacks_{};
   bool is_text_request_{};
   bool is_text_response_{};
+  bool needs_transformation_for_non_proto_encoded_response_{};
   Buffer::OwnedImpl decoding_buffer_;
   Grpc::Decoder decoder_;
   absl::optional<Grpc::Context::RequestStatNames> request_stat_names_;
   bool is_grpc_web_request_{};
   Grpc::Context& context_;
+  Http::ResponseHeaderMap* response_headers_{nullptr};
 };
 
 } // namespace GrpcWeb

test/extensions/filters/http/grpc_web/BUILD

@@ -19,6 +19,7 @@ envoy_extension_cc_test(
         "//source/extensions/filters/http/grpc_web:grpc_web_filter_lib",
         "//test/mocks/http:http_mocks",
         "//test/test_common:global_lib",
+        "//test/test_common:test_runtime_lib",
         "//test/test_common:utility_lib",
     ],
 )