Skip to content

Fix null node for list of struct in payload_to_metadaata filter #32309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
KBaichoo merged 1 commit into from
Mar 4, 2024
Merged

Fix null node for list of struct in payload_to_metadaata filter #32309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 19 additions & 15 deletions ...ns/filters/network/thrift_proxy/filters/payload_to_metadata/payload_to_metadata_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ bool Rule::matches(const ThriftProxy::MessageMetadata& metadata) const {
}

FilterStatus TrieMatchHandler::messageEnd() {
ASSERT(steps_ == 0);
ENVOY_LOG(trace, "TrieMatchHandler messageEnd");
parent_.handleOnMissing();
complete_ = true;
Expand All @@ -110,13 +111,12 @@ FilterStatus TrieMatchHandler::messageEnd() {

FilterStatus TrieMatchHandler::structBegin(absl::string_view) {
ENVOY_LOG(trace, "TrieMatchHandler structBegin id: {}, steps: {}",
last_field_id_.has_value() ? std::to_string(last_field_id_.value())
: "top_level_struct",
steps_);
field_ids_.empty() ? "top_level_struct" : std::to_string(field_ids_.back()), steps_);
ASSERT(steps_ >= 0);
assertNode();
if (last_field_id_.has_value()) {
if (steps_ == 0 && node_->children_.find(last_field_id_.value()) != node_->children_.end()) {
node_ = node_->children_[last_field_id_.value()];
if (!field_ids_.empty()) {
if (steps_ == 0 && node_->children_.find(field_ids_.back()) != node_->children_.end()) {
node_ = node_->children_[field_ids_.back()];
ENVOY_LOG(trace, "name: {}", node_->name_);
} else {
steps_++;
Expand All @@ -136,37 +136,41 @@ FilterStatus TrieMatchHandler::structEnd() {
// last decoder event
node_ = nullptr;
}
ASSERT(steps_ >= 0);
return FilterStatus::Continue;
}

FilterStatus TrieMatchHandler::fieldBegin(absl::string_view, FieldType&, int16_t& field_id) {
last_field_id_ = field_id;
ENVOY_LOG(trace, "TrieMatchHandler fieldBegin id: {}", field_id);
field_ids_.push_back(field_id);
return FilterStatus::Continue;
}

FilterStatus TrieMatchHandler::fieldEnd() {
last_field_id_.reset();
ENVOY_LOG(trace, "TrieMatchHandler fieldEnd");
field_ids_.pop_back();
return FilterStatus::Continue;
}

FilterStatus TrieMatchHandler::stringValue(absl::string_view value) {
assertLastFieldId();
ENVOY_LOG(trace, "TrieMatchHandler stringValue id:{} value:{}", last_field_id_.value(), value);
ENVOY_LOG(trace, "TrieMatchHandler stringValue id:{} value:{}", field_ids_.back(), value);
return handleString(static_cast<std::string>(value));
}

template <typename NumberType> FilterStatus TrieMatchHandler::numberValue(NumberType value) {
assertLastFieldId();
ENVOY_LOG(trace, "TrieMatchHandler numberValue id:{} value:{}", last_field_id_.value(), value);
ENVOY_LOG(trace, "TrieMatchHandler numberValue id:{} value:{}", field_ids_.back(), value);
return handleString(std::to_string(value));
}

FilterStatus TrieMatchHandler::handleString(std::string value) {
ASSERT(steps_ >= 0);
assertNode();
assertLastFieldId();
if (steps_ == 0 && node_->children_.find(last_field_id_.value()) != node_->children_.end() &&
!node_->children_[last_field_id_.value()]->rule_ids_.empty()) {
auto on_present_node = node_->children_[last_field_id_.value()];
if (steps_ == 0 && node_->children_.find(field_ids_.back()) != node_->children_.end() &&
!node_->children_[field_ids_.back()]->rule_ids_.empty()) {
auto on_present_node = node_->children_[field_ids_.back()];
ENVOY_LOG(trace, "name: {}", on_present_node->name_);
parent_.handleOnPresent(std::move(value), on_present_node->rule_ids_);
}
Expand All @@ -180,8 +184,8 @@ void TrieMatchHandler::assertNode() {
}

void TrieMatchHandler::assertLastFieldId() {
if (!last_field_id_.has_value()) {
throw EnvoyException("payload to metadata filter: invalid trie state, last_field_id is null");
if (field_ids_.empty()) {
throw EnvoyException("payload to metadata filter: invalid trie state, field_ids_ is null");
}
}

Expand Down
5 changes: 3 additions & 2 deletions ...ons/filters/network/thrift_proxy/filters/payload_to_metadata/payload_to_metadata_filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,15 +135,16 @@ class TrieMatchHandler : public DecoderCallbacks,
}

FilterStatus handleContainerEnd() {
ASSERT(steps_ > 0, "unmatched container end");
steps_--;
return FilterStatus::Continue;
}

MetadataHandler& parent_;
TrieSharedPtr node_;
bool complete_{false};
absl::optional<int16_t> last_field_id_;
uint16_t steps_{0};
std::vector<int16_t> field_ids_;
int16_t steps_{0};
Comment thread
JuniorHsu marked this conversation as resolved.
};

const uint32_t MAX_PAYLOAD_VALUE_LEN = 8 * 1024;
Expand Down
67 changes: 66 additions & 1 deletion ...lters/network/thrift_proxy/filters/payload_to_metadata/payload_to_metadata_filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ class PayloadToMetadataTest : public testing::Test,
decoder->onData(buffer, underflow);
}

void writeMessageMultiStruct() {
void writeMessageMultiStruct(bool list_of_struct = false) {
Buffer::OwnedImpl buffer;
auto metadata_ptr =
std::make_shared<Extensions::NetworkFilters::ThriftProxy::MessageMetadata>();
Expand Down Expand Up @@ -229,6 +229,25 @@ class PayloadToMetadataTest : public testing::Test,
proto->writeString(msg, "qux");
proto->writeFieldEnd(msg);

if (list_of_struct) {
proto->writeFieldBegin(msg, "list_of_struct", FieldType::List, 2);
const int list_size = 4;
proto->writeListBegin(msg, FieldType::Struct, 4);

// In the list of struct, each struct has a single field "check".
for (int i = 0; i < list_size; i++) {
proto->writeStructBegin(msg, "data");
proto->writeFieldBegin(msg, "check", FieldType::String, 1);
proto->writeString(msg, "check" + std::to_string(i));
proto->writeFieldEnd(msg);
proto->writeFieldBegin(msg, "", FieldType::Stop, 0); // data stop field
proto->writeStructEnd(msg);
}

proto->writeListEnd(msg);
proto->writeFieldEnd(msg);
}

proto->writeFieldBegin(msg, "", FieldType::Stop, 0); // request stop field
proto->writeStructEnd(msg);
proto->writeFieldEnd(msg);
Expand Down Expand Up @@ -1316,6 +1335,52 @@ TEST_F(PayloadToMetadataTest, MultipleRulesOnMultiStruct) {
filter_->onDestroy();
}

TEST_F(PayloadToMetadataTest, MultipleRulesOnListOfStruct) {
const std::string request_config_yaml = R"EOF(
request_rules:
- method_name: unmatched_foo
field_selector:
name: request
id: 2
child:
name: baz
id: 1
on_present:
metadata_namespace: envoy.lb
key: baz
- method_name: unmatched_foo2
field_selector:
name: request
id: 2
child:
name: baz
id: 1
on_present:
metadata_namespace: envoy.lb
key: baz
- method_name: foo
field_selector:
name: request
id: 2
child:
name: baz
id: 1
on_present:
metadata_namespace: envoy.lb
key: baz
)EOF";

const std::map<std::string, std::string> expected = {{"baz", "qux"}};

initializeFilter(request_config_yaml);
EXPECT_CALL(req_info_, setDynamicMetadata("envoy.lb", MapEq(expected)));
EXPECT_CALL(decoder_callbacks_, streamInfo()).WillRepeatedly(ReturnRef(req_info_));

// Add list of struct
writeMessageMultiStruct(true);
filter_->onDestroy();
}

TEST_F(PayloadToMetadataTest, MalformPayloadWontCrash) {
const std::string request_config_yaml = R"EOF(
request_rules:
Expand Down