diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 7ed7c1e07fd1..561004e3e552 100644 --- 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 // `. -* 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 ` a new string matcher type which matches if the value of the string has the substring mentioned in contains matcher. * http: added :ref:`contains ` 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 ` to :ref:`local reply mapper ` 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 index b8119639e8c1..d13b360ee07f 100644 --- 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 index 599e16a845f3..87bad1121e1e 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 @@ -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::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::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