From 7a86e8a5675367ed64c6b9bca0d6dbf8d4644597 Mon Sep 17 00:00:00 2001 From: Henry Qin Date: Thu, 18 Jan 2024 12:36:21 -0500 Subject: [PATCH] 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 Signed-off-by: Henry Qin --- changelogs/current.yaml | 5 +++++ source/common/runtime/runtime_features.cc | 1 + .../filters/http/grpc_http1_reverse_bridge/filter.cc | 6 ++++++ .../http/grpc_http1_reverse_bridge/reverse_bridge_test.cc | 1 + 4 files changed, 13 insertions(+) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index ba316d2be54e..f31fcd7bc596 100644 --- 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 ` 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 index 3990f2a06e27..febb801fd8d5 100644 --- 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 index f59cdd457508..d8a4f6758839 100644 --- 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 index b8bbd517e458..8c6609dbf17f 100644 --- 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