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

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 · Sep 24, 2020 · 07f7c59 · 07f7c59
1 parent e107c60
commit 07f7c59
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
@@ -20,6 +20,7 @@ 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,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