From 424d39c673f336900a8918a681e375dc6ac6b426 Mon Sep 17 00:00:00 2001 From: Kuo-Chung Hsu Date: Tue, 6 Aug 2024 08:48:41 -0700 Subject: [PATCH] report imcomplete metadata in complete events for thrift_to_metadata filter (#35574) Periodically http rq/resp with no `end_stream` filter events nor trailers event occur, which breaks the some assumptions. Hence, move checks to `decodeComplete` and `encodeComplete` Risk Level: low Testing: new unit Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: kuochunghsu Signed-off-by: asingh-g --- .../filters/http/thrift_to_metadata/filter.cc | 16 ++--- .../filters/http/thrift_to_metadata/filter.h | 4 +- .../http/thrift_to_metadata/filter_test.cc | 67 +++++++++++++++++++ 3 files changed, 75 insertions(+), 12 deletions(-) diff --git a/source/extensions/filters/http/thrift_to_metadata/filter.cc b/source/extensions/filters/http/thrift_to_metadata/filter.cc index 6c4c0e858825..23753568d326 100644 --- a/source/extensions/filters/http/thrift_to_metadata/filter.cc +++ b/source/extensions/filters/http/thrift_to_metadata/filter.cc @@ -182,19 +182,17 @@ Http::FilterDataStatus Filter::decodeData(Buffer::Instance& data, bool end_strea : Http::FilterDataStatus::StopIterationAndBuffer; } -Http::FilterTrailersStatus Filter::decodeTrailers(Http::RequestTrailerMap&) { +void Filter::decodeComplete() { if (!config_->shouldParseRequestMetadata() || request_processing_finished_) { - return Http::FilterTrailersStatus::Continue; + return; } ENVOY_LOG(trace, - "thrift to metadata filter decodeTrailers: handle incomplete request thrift message"); + "thrift to metadata filter decodeComplete: handle incomplete request thrift message"); // Handle incomplete request thrift message while we reach here. handleAllOnMissing(config_->requestRules(), *decoder_callbacks_, request_processing_finished_); config_->rqstats().invalid_thrift_body_.inc(); - - return Http::FilterTrailersStatus::Continue; } Http::FilterHeadersStatus Filter::encodeHeaders(Http::ResponseHeaderMap& headers, bool end_stream) { @@ -246,19 +244,17 @@ Http::FilterDataStatus Filter::encodeData(Buffer::Instance& data, bool end_strea : Http::FilterDataStatus::StopIterationAndBuffer; } -Http::FilterTrailersStatus Filter::encodeTrailers(Http::ResponseTrailerMap&) { +void Filter::encodeComplete() { if (!config_->shouldParseResponseMetadata() || response_processing_finished_) { - return Http::FilterTrailersStatus::Continue; + return; } ENVOY_LOG(trace, - "thrift to metadata filter encodeTrailers: handle incomplete response thrift message"); + "thrift to metadata filter encodeComplete: handle incomplete response thrift message"); // Handle incomplete response thrift message while we reach here. handleAllOnMissing(config_->responseRules(), *encoder_callbacks_, response_processing_finished_); config_->respstats().invalid_thrift_body_.inc(); - - return Http::FilterTrailersStatus::Continue; } bool Filter::processData(Buffer::Instance& incoming_data, Buffer::Instance& buffer, diff --git a/source/extensions/filters/http/thrift_to_metadata/filter.h b/source/extensions/filters/http/thrift_to_metadata/filter.h index 18d89b78621d..fe28ce3767c7 100644 --- a/source/extensions/filters/http/thrift_to_metadata/filter.h +++ b/source/extensions/filters/http/thrift_to_metadata/filter.h @@ -181,11 +181,11 @@ class Filter : public Http::PassThroughFilter, Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers, bool end_stream) override; Http::FilterDataStatus decodeData(Buffer::Instance& data, bool end_stream) override; - Http::FilterTrailersStatus decodeTrailers(Http::RequestTrailerMap& trailers) override; + void decodeComplete() override; Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap& headers, bool end_stream) override; Http::FilterDataStatus encodeData(Buffer::Instance& data, bool end_stream) override; - Http::FilterTrailersStatus encodeTrailers(Http::ResponseTrailerMap& trailers) override; + void encodeComplete() override; // PassThroughDecoderEventHandler FilterStatus messageBegin(MessageMetadataSharedPtr metadata) override; diff --git a/test/extensions/filters/http/thrift_to_metadata/filter_test.cc b/test/extensions/filters/http/thrift_to_metadata/filter_test.cc index 3153e5920b9c..c24be7992947 100644 --- a/test/extensions/filters/http/thrift_to_metadata/filter_test.cc +++ b/test/extensions/filters/http/thrift_to_metadata/filter_test.cc @@ -485,6 +485,39 @@ TEST_P(FilterTest, IncompleteRequestWithTrailer) { Http::TestRequestTrailerMapImpl trailers{{"some", "trailer"}}; EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(trailers)); + filter_->decodeComplete(); + + EXPECT_EQ(getCounterValue("thrift_to_metadata.rq.success"), 0); + EXPECT_EQ(getCounterValue("thrift_to_metadata.rq.mismatched_content_type"), 0); + EXPECT_EQ(getCounterValue("thrift_to_metadata.rq.no_body"), 0); + EXPECT_EQ(getCounterValue("thrift_to_metadata.rq.invalid_thrift_body"), 1); +} + +TEST_P(FilterTest, IncompleteRequestWithEarlyComplete) { + const auto [transport_type, protocol_type] = GetParam(); + MessageType message_type = MessageType::Call; + + initializeFilter(config_yaml_); + const std::map& expected_metadata = { + {"protocol", "unknown"}, + {"transport", "unknown"}, + {"request_message_type", "unknown"}, + {"method_name", "unknown"}}; + + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_->decodeHeaders(request_headers_, false)); + EXPECT_CALL(decoder_callbacks_, streamInfo()).WillRepeatedly(ReturnRef(stream_info_)); + EXPECT_CALL(stream_info_, setDynamicMetadata("envoy.lb", MapEq(expected_metadata))); + + Buffer::OwnedImpl whole_message; + writeMessage(whole_message, transport_type, protocol_type, message_type); + Buffer::OwnedImpl buffer; + // incomplete message + buffer.move(whole_message, whole_message.length() / 2); + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(buffer, false)); + + filter_->decodeComplete(); + EXPECT_EQ(getCounterValue("thrift_to_metadata.rq.success"), 0); EXPECT_EQ(getCounterValue("thrift_to_metadata.rq.mismatched_content_type"), 0); EXPECT_EQ(getCounterValue("thrift_to_metadata.rq.no_body"), 0); @@ -518,6 +551,40 @@ TEST_P(FilterTest, IncompleteResponseWithTrailer) { Http::TestResponseTrailerMapImpl trailers{{"some", "trailer"}}; EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->encodeTrailers(trailers)); + filter_->encodeComplete(); + + EXPECT_EQ(getCounterValue("thrift_to_metadata.resp.success"), 0); + EXPECT_EQ(getCounterValue("thrift_to_metadata.resp.mismatched_content_type"), 0); + EXPECT_EQ(getCounterValue("thrift_to_metadata.resp.no_body"), 0); + EXPECT_EQ(getCounterValue("thrift_to_metadata.resp.invalid_thrift_body"), 1); +} + +TEST_P(FilterTest, IncompleteResponseEarlyComplete) { + const auto [transport_type, protocol_type] = GetParam(); + MessageType message_type = MessageType::Reply; + + initializeFilter(config_yaml_); + const std::map& expected_metadata = { + {"protocol", "unknown"}, + {"transport", "unknown"}, + {"response_message_type", "unknown"}, + {"method_name", "unknown"}, + {"response_reply_type", "unknown"}}; + + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_->encodeHeaders(response_headers_, false)); + EXPECT_CALL(encoder_callbacks_, streamInfo()).WillRepeatedly(ReturnRef(stream_info_)); + EXPECT_CALL(stream_info_, setDynamicMetadata(HttpFilterNames::get().ThriftToMetadata, + MapEq(expected_metadata))); + + Buffer::OwnedImpl whole_message; + writeMessage(whole_message, transport_type, protocol_type, message_type, ReplyType::Success); + Buffer::OwnedImpl buffer; + // incomplete message + buffer.move(whole_message, whole_message.length() / 2); + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_->encodeData(buffer, false)); + + filter_->encodeComplete(); EXPECT_EQ(getCounterValue("thrift_to_metadata.resp.success"), 0); EXPECT_EQ(getCounterValue("thrift_to_metadata.resp.mismatched_content_type"), 0);