Revert "reverse bridge: use local reply instead of ContinueAndEndStre…

…am (#13205)" (#13262) This reverts commit 07f7c59. Signed-off-by: Snow Pettersen <[email protected]>
envoyproxy · Sep 24, 2020 · 51c1fa0 · 51c1fa0
1 parent a9a79b3
commit 51c1fa0
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 22 deletions.
diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -20,7 +20,6 @@ Minor Behavior Changes
 * ext_authz filter: request timeout will now count from the time the check request is created, instead of when it becomes active. This makes sure that the timeout is enforced even if the ext_authz cluster's circuit breaker is engaged.
   This behavior can be reverted by setting runtime feature ``envoy.reloadable_features.ext_authz_measure_timeout_on_check_created`` to false. When enabled, a new `ext_authz.timeout` stat is counted when timeout occurs. See :ref:`stats
   // <config_http_filters_ext_authz_stats>`.
-* grpc reverse bridge: upstream headers will no longer be propagated when the response is missing or contains an unexpected content-type.
 * http: added :ref:`contains <envoy_api_msg_type.matcher.StringMatcher>` a new string matcher type which matches if the value of the string has the substring mentioned in contains matcher.
 * http: added :ref:`contains <envoy_api_msg_route.HeaderMatcher>` a new header matcher type which matches if the value of the header has the substring mentioned in contains matcher.
 * http: added :ref:`headers_to_add <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.ResponseMapper.headers_to_add>` to :ref:`local reply mapper <config_http_conn_man_local_reply>` to allow its users to add/append/override response HTTP headers to local replies.

diff --git a/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc b/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc
@@ -136,11 +136,17 @@ Http::FilterHeadersStatus Filter::encodeHeaders(Http::ResponseHeaderMap& headers
     // If the response from upstream does not have the correct content-type,
     // perform an early return with a useful error message in grpc-message.
     if (content_type != upstream_content_type_) {
-      decoder_callbacks_->sendLocalReply(Http::Code::OK, badContentTypeMessage(headers), nullptr,
-                                         Grpc::Status::WellKnownGrpcStatus::Unknown,
-                                         RcDetails::get().GrpcBridgeFailedContentType);
+      headers.setGrpcMessage(badContentTypeMessage(headers));
+      headers.setGrpcStatus(Envoy::Grpc::Status::WellKnownGrpcStatus::Unknown);
+      headers.setStatus(enumToInt(Http::Code::OK));
+
+      if (!content_type.empty()) {
+        headers.setContentType(content_type_);
+      }
 
-      return Http::FilterHeadersStatus::StopIteration;
+      decoder_callbacks_->streamInfo().setResponseCodeDetails(
+          RcDetails::get().GrpcBridgeFailedContentType);
+      return Http::FilterHeadersStatus::ContinueAndEndStream;
     }
 
     // Restore the content-type to match what the downstream sent.

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
@@ -532,6 +532,9 @@ TEST_F(ReverseBridgeTest, GrpcRequestBadResponseNoContentType) {
     buffer.add("abcdefgh", 8);
     EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(buffer, false));
     EXPECT_EQ("fgh", buffer.toString());
+    EXPECT_CALL(decoder_callbacks_, streamInfo());
+    EXPECT_CALL(decoder_callbacks_.stream_info_,
+                setResponseCodeDetails(absl::string_view("grpc_bridge_content_type_wrong")));
   }
 
   {
@@ -546,14 +549,15 @@ TEST_F(ReverseBridgeTest, GrpcRequestBadResponseNoContentType) {
   EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(trailers));
 
   Http::TestResponseHeaderMapImpl headers({{":status", "400"}});
-  EXPECT_CALL(
-      decoder_callbacks_,
-      sendLocalReply(
-          Http::Code::OK,
-          "envoy reverse bridge: upstream responded with no content-type header, status code 400",
-          _, absl::make_optional(static_cast<Grpc::Status::GrpcStatus>(Grpc::Status::Unknown)), _));
-  EXPECT_CALL(decoder_callbacks_, encodeHeaders_(_, _));
-  EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->encodeHeaders(headers, false));
+  EXPECT_EQ(Http::FilterHeadersStatus::ContinueAndEndStream,
+            filter_->encodeHeaders(headers, false));
+  EXPECT_THAT(headers, HeaderValueOf(Http::Headers::get().Status, "200"));
+  EXPECT_THAT(headers, HeaderValueOf(Http::Headers::get().GrpcStatus, "2"));
+  EXPECT_THAT(
+      headers,
+      HeaderValueOf(
+          Http::Headers::get().GrpcMessage,
+          "envoy reverse bridge: upstream responded with no content-type header, status code 400"));
 }
 
 // Tests that a gRPC is downgraded to application/x-protobuf and that if the response
@@ -579,6 +583,9 @@ TEST_F(ReverseBridgeTest, GrpcRequestBadResponse) {
     buffer.add("abcdefgh", 8);
     EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(buffer, false));
     EXPECT_EQ("fgh", buffer.toString());
+    EXPECT_CALL(decoder_callbacks_, streamInfo());
+    EXPECT_CALL(decoder_callbacks_.stream_info_,
+                setResponseCodeDetails(absl::string_view("grpc_bridge_content_type_wrong")));
   }
 
   {
@@ -594,15 +601,13 @@ TEST_F(ReverseBridgeTest, GrpcRequestBadResponse) {
 
   Http::TestResponseHeaderMapImpl headers(
       {{":status", "400"}, {"content-type", "application/json"}});
-  EXPECT_CALL(
-      decoder_callbacks_,
-      sendLocalReply(
-          Http::Code::OK,
-          "envoy reverse bridge: upstream responded with unsupported "
-          "content-type application/json, status code 400",
-          _, absl::make_optional(static_cast<Grpc::Status::GrpcStatus>(Grpc::Status::Unknown)), _));
-  EXPECT_CALL(decoder_callbacks_, encodeHeaders_(_, _));
-  EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->encodeHeaders(headers, false));
+  EXPECT_EQ(Http::FilterHeadersStatus::ContinueAndEndStream,
+            filter_->encodeHeaders(headers, false));
+  EXPECT_THAT(headers, HeaderValueOf(Http::Headers::get().Status, "200"));
+  EXPECT_THAT(headers, HeaderValueOf(Http::Headers::get().GrpcStatus, "2"));
+  EXPECT_THAT(headers, HeaderValueOf(Http::Headers::get().GrpcMessage,
+                                     "envoy reverse bridge: upstream responded with unsupported "
+                                     "content-type application/json, status code 400"));
 }
 
 // Tests that the filter passes a GRPC request through without modification because it is disabled