reverse bridge: use local reply instead of ContinueAndEndStream (#13205…

…) (#13414) Instead of returning ContinueAndEndStream we can respond with a local reply. This removes the only use case of ContinueAndEndStream in non-test code. Signed-off-by: Snow Pettersen <[email protected]>
envoyproxy · Oct 6, 2020 · 602dfea · 602dfea
1 parent 3fac88f
commit 602dfea
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 32 deletions.
diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -26,6 +26,7 @@ Minor Behavior Changes
   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>`.
 * ext_authz_filter: added :ref:`disable_request_body_buffering <envoy_v3_api_field_extensions.filters.http.ext_authz.v3.CheckSettings.disable_request_body_buffering>` to disable request data buffering per-route.
+* 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,17 +136,11 @@ 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_) {
-      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_);
-      }
+      decoder_callbacks_->sendLocalReply(Http::Code::OK, badContentTypeMessage(headers), nullptr,
+                                         Grpc::Status::WellKnownGrpcStatus::Unknown,
+                                         RcDetails::get().GrpcBridgeFailedContentType);
 
-      decoder_callbacks_->streamInfo().setResponseCodeDetails(
-          RcDetails::get().GrpcBridgeFailedContentType);
-      return Http::FilterHeadersStatus::ContinueAndEndStream;
+      return Http::FilterHeadersStatus::StopIteration;
     }
 
     // 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,9 +532,6 @@ 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")));
   }
 
   {
@@ -549,15 +546,14 @@ TEST_F(ReverseBridgeTest, GrpcRequestBadResponseNoContentType) {
   EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(trailers));
 
   Http::TestResponseHeaderMapImpl headers({{":status", "400"}});
-  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"));
+  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));
 }
 
 // Tests that a gRPC is downgraded to application/x-protobuf and that if the response
@@ -583,9 +579,6 @@ 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")));
   }
 
   {
@@ -601,13 +594,15 @@ TEST_F(ReverseBridgeTest, GrpcRequestBadResponse) {
 
   Http::TestResponseHeaderMapImpl headers(
       {{":status", "400"}, {"content-type", "application/json"}});
-  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"));
+  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));
 }
 
 // Tests that the filter passes a GRPC request through without modification because it is disabled