Skip to content

thrift router: Support request subset lb #18541

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
mattklein123 merged 17 commits into from
Oct 15, 2021
Merged

thrift router: Support request subset lb #18541

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
9ec0944
thrift router: Support request subset lb
xiaoma2015 Oct 9, 2021
abb822d
unit test
xiaoma2015 Oct 11, 2021
58df464
format fix
xiaoma2015 Oct 11, 2021
f68bb91
format fix
xiaoma2015 Oct 11, 2021
2aa1ed4
unit test
xiaoma2015 Oct 12, 2021
3ea7ccc
unit test
xiaoma2015 Oct 12, 2021
4342443
format fix
xiaoma2015 Oct 12, 2021
fe993ff
merge main branch
xiaoma2015 Oct 13, 2021
e4bf7a3
update some comments
xiaoma2015 Oct 13, 2021
98f5eab
format fix
xiaoma2015 Oct 13, 2021
99a1ce9
update docs
xiaoma2015 Oct 14, 2021
43bbed8
Merge branch 'main' of https://github.com/envoyproxy/envoy into add_t…
xiaoma2015 Oct 14, 2021
1f4e49e
Merge branch 'main' into add_thrift_metadata_filter
xiaoma2015 Oct 14, 2021
3ec0de9
update docs
xiaoma2015 Oct 14, 2021
dc26d39
Merge branch 'add_thrift_metadata_filter' of github.com:xiaoma2015/en…
xiaoma2015 Oct 14, 2021
8b0af2d
update docs
xiaoma2015 Oct 14, 2021
825a0f0
Merge branch 'main' of https://github.com/envoyproxy/envoy into add_t…
xiaoma2015 Oct 15, 2021
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
22 changes: 21 additions & 1 deletion source/extensions/filters/network/thrift_proxy/router/router_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "envoy/upstream/load_balancer.h"

#include "source/common/http/header_utility.h"
#include "source/common/router/metadatamatchcriteria_impl.h"
#include "source/common/upstream/load_balancer_impl.h"
#include "source/extensions/filters/network/thrift_proxy/conn_manager.h"
#include "source/extensions/filters/network/thrift_proxy/filters/filter.h"
Expand Down Expand Up @@ -271,7 +272,25 @@ class Router : public Tcp::ConnectionPool::UpstreamCallbacks,
// Upstream::LoadBalancerContext
const Network::Connection* downstreamConnection() const override;
const Envoy::Router::MetadataMatchCriteria* metadataMatchCriteria() override {
return route_entry_ ? route_entry_->metadataMatchCriteria() : nullptr;
const Envoy::Router::MetadataMatchCriteria* route_criteria =
(route_entry_ != nullptr) ? route_entry_->metadataMatchCriteria() : nullptr;

// Support request subset lb in thrift
Comment thread
xiaoma2015 marked this conversation as resolved.
Outdated
const auto& request_metadata = callbacks_->streamInfo().dynamicMetadata().filter_metadata();
const auto filter_it = request_metadata.find(Envoy::Config::MetadataFilters::get().ENVOY_LB);

if (filter_it != request_metadata.end()) {
if (route_criteria != nullptr) {
metadata_match_criteria_ = route_criteria->mergeMatchCriteria(filter_it->second);
} else {
metadata_match_criteria_ =
std::make_unique<Envoy::Router::MetadataMatchCriteriaImpl>(filter_it->second);
}
} else {
return route_criteria;
Comment thread
xiaoma2015 marked this conversation as resolved.
Outdated
}

return metadata_match_criteria_.get();

@rgs1 rgs1 Oct 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can simplify this a bit:

    if (filter_it == request_metadata.end()) {
      return route_criteria;
    }

    metadata_match_criteria_ = route_criteria != nullptr ? route_criteria->mergeMatchCriteria(filter_it->second) : std::make_unique<Envoy::Router::MetadataMatchCriteriaImpl>(filter_it->second);
    return metadata_match_criteria_.get();

@xiaoma2015 xiaoma2015 Oct 13, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using ifelse is better?

@xiaoma2015 xiaoma2015 Oct 13, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx for review
using ifelse is better?

@rgs1 rgs1 Oct 13, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah looks great -- I am just a fan of the ternary operator 😁.

}

// Tcp::ConnectionPool::UpstreamCallbacks
Expand All @@ -287,6 +306,7 @@ class Router : public Tcp::ConnectionPool::UpstreamCallbacks,
std::unique_ptr<UpstreamResponseCallbacksImpl> upstream_response_callbacks_{};
RouteConstSharedPtr route_{};
const RouteEntry* route_entry_{};
Envoy::Router::MetadataMatchCriteriaConstPtr metadata_match_criteria_;

std::unique_ptr<UpstreamRequest> upstream_request_;
Buffer::OwnedImpl upstream_request_buffer_;
Expand Down
119 changes: 119 additions & 0 deletions test/extensions/filters/network/thrift_proxy/router_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,89 @@ class ThriftRouterTestBase {
metadata_->setSequenceId(sequence_id);
}

void verifyMetadataMatchCriteriaFromRequest(bool route_entry_has_match) {
ProtobufWkt::Struct request_struct;
ProtobufWkt::Value val;

// Populate metadata like StreamInfo.setDynamicMetadata() would.
auto& fields_map = *request_struct.mutable_fields();
val.set_string_value("v3.1");
fields_map["version"] = val;
val.set_string_value("devel");
fields_map["stage"] = val;
(*callbacks_.stream_info_.metadata_
.mutable_filter_metadata())[Envoy::Config::MetadataFilters::get().ENVOY_LB] =
request_struct;

// Populate route entry's metadata which will be overridden.
val.set_string_value("v3.0");
fields_map = *request_struct.mutable_fields();
fields_map["version"] = val;
Envoy::Router::MetadataMatchCriteriaImpl route_entry_matches(request_struct);
Comment thread
xiaoma2015 marked this conversation as resolved.

if (route_entry_has_match) {
ON_CALL(route_entry_, metadataMatchCriteria()).WillByDefault(Return(&route_entry_matches));
} else {
ON_CALL(route_entry_, metadataMatchCriteria()).WillByDefault(Return(nullptr));
}

auto match = router_->metadataMatchCriteria()->metadataMatchCriteria();
EXPECT_EQ(match.size(), 2);
auto it = match.begin();

// Note: metadataMatchCriteria() keeps its entries sorted, so the order for checks
// below matters.

// `stage` was only set by the request, not by the route entry.
EXPECT_EQ((*it)->name(), "stage");
EXPECT_EQ((*it)->value().value().string_value(), "devel");
it++;

// `version` should be what came from the request, overriding the route entry.

@wbpcode wbpcode Oct 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May

`version` should be what came from the request and override the route entry's.

?

EXPECT_EQ((*it)->name(), "version");
EXPECT_EQ((*it)->value().value().string_value(), "v3.1");
}

void verifyMetadataMatchCriteriaFromRoute(bool route_entry_has_match) {
ProtobufWkt::Struct route_struct;
ProtobufWkt::Value val;

// Populate metadata like StreamInfo.setDynamicMetadata() would.
Comment thread
xiaoma2015 marked this conversation as resolved.
Outdated
auto& fields_map = *route_struct.mutable_fields();
val.set_string_value("v3.1");
fields_map["version"] = val;
val.set_string_value("devel");
fields_map["stage"] = val;

Envoy::Router::MetadataMatchCriteriaImpl route_entry_matches(route_struct);

if (route_entry_has_match) {
ON_CALL(route_entry_, metadataMatchCriteria()).WillByDefault(Return(&route_entry_matches));

EXPECT_NE(nullptr, router_->metadataMatchCriteria());
auto match = router_->metadataMatchCriteria()->metadataMatchCriteria();
EXPECT_EQ(match.size(), 2);
auto it = match.begin();

// Note: metadataMatchCriteria() keeps its entries sorted, so the order for checks
// below matters.

// `stage` was set by the route entry.
EXPECT_EQ((*it)->name(), "stage");
EXPECT_EQ((*it)->value().value().string_value(), "devel");
it++;

// `version` was set by the route entry.
EXPECT_EQ((*it)->name(), "version");
EXPECT_EQ((*it)->value().value().string_value(), "v3.1");
} else {
ON_CALL(callbacks_.route_->route_entry_, metadataMatchCriteria())
.WillByDefault(Return(nullptr));

EXPECT_EQ(nullptr, router_->metadataMatchCriteria());
}
Comment on lines +159 to +228

@wbpcode wbpcode Oct 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, where is the verify of metadata match criteria?

@xiaoma2015 xiaoma2015 Oct 12, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test code coming soon

@xiaoma2015 xiaoma2015 Oct 12, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/updated

}

void startRequest(MessageType msg_type, std::string method = "method",
const bool strip_service_name = false,
const TransportType transport_type = TransportType::Framed,
Expand Down Expand Up @@ -697,6 +780,42 @@ TEST_F(ThriftRouterTest, NoCluster) {
EXPECT_EQ(1U, context_.scope().counterFromString("test.unknown_cluster").value());
}

// Test for request metadata is not empty
Comment thread
xiaoma2015 marked this conversation as resolved.
Outdated
TEST_F(ThriftRouterTest, MetadataMatchCriteriaFromRequest) {
Comment thread
xiaoma2015 marked this conversation as resolved.
initializeRouter();
initializeMetadata(MessageType::Call);

// Test case for router filter metadata is not empty

@wbpcode wbpcode Oct 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be this comment is unnecessary?

verifyMetadataMatchCriteriaFromRequest(true);
}

// Test for request metadata is not empty
Comment thread
xiaoma2015 marked this conversation as resolved.
Outdated
TEST_F(ThriftRouterTest, MetadataMatchCriteriaFromRequestNoRouteEntryMatch) {
Comment thread
xiaoma2015 marked this conversation as resolved.
initializeRouter();
initializeMetadata(MessageType::Call);

// Test case for router filter metadata is empty

@wbpcode wbpcode Oct 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be this comment is unnecessary?

verifyMetadataMatchCriteriaFromRequest(false);
}

// Test for request metadata is empty
Comment thread
xiaoma2015 marked this conversation as resolved.
Outdated
TEST_F(ThriftRouterTest, MetadataMatchCriteriaFromRoute) {
initializeRouter();
startRequest(MessageType::Call);

// Test case for router filter metadata is not empty

@wbpcode wbpcode Oct 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be this comment is unnecessary?

verifyMetadataMatchCriteriaFromRoute(true);
}

// Test for request metadata is empty

@wbpcode wbpcode Oct 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May Test the case where both route metadata match criteria and dynamic metadata match criteria are empty. ?

TEST_F(ThriftRouterTest, MetadataMatchCriteriaFromRouteNoRouteEntryMatch) {
initializeRouter();
startRequest(MessageType::Call);

// Test case for router filter metadata is empty

@wbpcode wbpcode Oct 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be this comment is unnecessary?

verifyMetadataMatchCriteriaFromRoute(false);
}

TEST_F(ThriftRouterTest, ClusterMaintenanceMode) {
initializeRouter();
initializeMetadata(MessageType::Call);
Expand Down