Skip to content

Commit

Permalink
report imcomplete metadata in complete events for thrift_to_metadata …
Browse files Browse the repository at this point in the history 
…filter (envoyproxy#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 <[email protected]>
Signed-off-by: asingh-g <[email protected]>
  • Loading branch information
@JuniorHsu @asingh-g
JuniorHsu authored and asingh-g committed Aug 20, 2024
1 parent 17e10a0 commit 424d39c
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 12 deletions.
16 changes: 6 additions & 10 deletions source/extensions/filters/http/thrift_to_metadata/filter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions source/extensions/filters/http/thrift_to_metadata/filter.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
67 changes: 67 additions & 0 deletions test/extensions/filters/http/thrift_to_metadata/filter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::string, std::string>& 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);
Expand Down Expand Up @@ -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<std::string, std::string>& 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);
Expand Down

0 comments on commit 424d39c

Please sign in to comment.