Make grpc_http1_reverse_bridge return HTTP 200. (#31047)

gRPC clients expect that request status will be communicated using the gRPC status header, and that HTTP status for responses should always be 200 for properly behaving servers. This commit updates the grpc_http1_reverse_bridge to follow this behavior. Risk Level: medium Testing: unit test, manually tested with grpcurl Docs Changes: n/a Release Notes: updated Runtime guard: envoy.reloadable_features.grpc_http1_reverse_bridge_change_http_status Signed-off-by: Henry Qin <[email protected]> Signed-off-by: Henry Qin <[email protected]>
envoyproxy · Jan 18, 2024 · 7a86e8a · 7a86e8a
1 parent c0a4200
commit 7a86e8a
Show file tree

Hide file tree

Showing 4 changed files with 13 additions and 0 deletions.
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -16,5 +16,10 @@ removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`
 
 new_features:
+- area: grpc reverse bridge
+  change: |
+    Change HTTP status to 200 to respect the gRPC protocol. This may cause problems for incorrect gRPC clients expecting the filter
+    to preserve HTTP 1.1 responses.  This behavioral change can be temporarily reverted by setting runtime guard
+    ``envoy.reloadable_features.grpc_http1_reverse_bridge_change_http_status`` to false.
 
 deprecated:
diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -48,6 +48,7 @@ RUNTIME_GUARD(envoy_reloadable_features_enable_connect_udp_support);
 RUNTIME_GUARD(envoy_reloadable_features_enable_intermediate_ca);
 RUNTIME_GUARD(envoy_reloadable_features_enable_zone_routing_different_zone_counts);
 RUNTIME_GUARD(envoy_reloadable_features_ext_authz_http_send_original_xff);
+RUNTIME_GUARD(envoy_reloadable_features_grpc_http1_reverse_bridge_change_http_status);
 RUNTIME_GUARD(envoy_reloadable_features_grpc_http1_reverse_bridge_handle_empty_response);
 RUNTIME_GUARD(envoy_reloadable_features_handle_uppercase_scheme);
 RUNTIME_GUARD(envoy_reloadable_features_hmac_base64_encoding_only);

diff --git a/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc b/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc
@@ -181,6 +181,12 @@ Http::FilterHeadersStatus Filter::encodeHeaders(Http::ResponseHeaderMap& headers
     // until then.
     grpc_status_ = grpcStatusFromHeaders(headers);
 
+    // gRPC clients expect that the HTTP status will always be 200.
+    if (Runtime::runtimeFeatureEnabled(
+            "envoy.reloadable_features.grpc_http1_reverse_bridge_change_http_status")) {
+      headers.setStatus(enumToInt(Http::Code::OK));
+    }
+
     if (Runtime::runtimeFeatureEnabled(
             "envoy.reloadable_features.grpc_http1_reverse_bridge_handle_empty_response")) {
       // This is a header-only response, and we should prepend the gRPC frame

diff --git a/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_test.cc b/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_test.cc
@@ -481,6 +481,7 @@ TEST_F(ReverseBridgeTest, GrpcRequestInternalError) {
       {{":status", "400"}, {"content-type", "application/x-protobuf"}});
   EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(headers, false));
   EXPECT_THAT(headers, HeaderValueOf(Http::Headers::get().ContentType, "application/grpc"));
+  EXPECT_THAT(headers, HeaderValueOf(Http::Headers::get().Status, "200"));
 
   {
     // First few calls should drain the buffer