-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
Use dynamicMetadata to construct metadataMatchCriteria() #3254
Changes from 1 commit
bde0c9c
1747131
be4fe35
f1f12e0
b261a23
663c922
b24da82
d29a4a5
8b7bb04
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
This enables filter writers to construct metadataMatchCriteria() from a RequestInfo (e.g.: header value). This is useful given that currently routes need to be statically matched when using metadata, e.g.: ``` --- match: prefix: / route: cluster: cluster-name metadata_match: filter_metadata: envoy.lb: version: '1.0' ``` For setups in which RDS isn't used and routes cannot be constructed dynamically, a filter can be used to match which endpoints should be used. So a filter could do something like: ``` auto header_value = std::string(header_entry->value().getStringView()); auto keyval = MessageUtil::keyValueStruct("version", header_value); callbacks_->requestInfo().setDynamicMetadata("envoy.lb", keyval); ``` to determine the version for this request. Signed-off-by: Raul Gutierrez Segales <[email protected]>
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -334,6 +334,9 @@ class MetadataMatchCriterion { | |
|
||
typedef std::shared_ptr<const MetadataMatchCriterion> MetadataMatchCriterionConstSharedPtr; | ||
|
||
class MetadataMatchCriteria; | ||
typedef std::unique_ptr<const MetadataMatchCriteria> MetadataMatchCriteriaConstPtr; | ||
|
||
class MetadataMatchCriteria { | ||
public: | ||
virtual ~MetadataMatchCriteria() {} | ||
|
@@ -345,6 +348,15 @@ class MetadataMatchCriteria { | |
*/ | ||
virtual const std::vector<MetadataMatchCriterionConstSharedPtr>& | ||
metadataMatchCriteria() const PURE; | ||
|
||
/** | ||
* Creates a new MetadataMatchCriteriaImpl, merging existing | ||
* metadata criteria this criteria. The result criteria is the | ||
* combination of both sets of criteria, with those from the | ||
* ProtobufWkt::Struct taking precedence. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
*/ | ||
virtual MetadataMatchCriteriaConstPtr | ||
mergeMatchCriteria(const ProtobufWkt::Struct& metadata_matches) const PURE; | ||
}; | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ | |
#include "common/common/hash.h" | ||
#include "common/common/hex.h" | ||
#include "common/common/logger.h" | ||
#include "common/config/well_known_names.h" | ||
#include "common/http/utility.h" | ||
#include "common/request_info/request_info_impl.h" | ||
|
||
|
@@ -161,8 +162,15 @@ class Filter : Logger::Loggable<Logger::Id::router>, | |
} | ||
return {}; | ||
} | ||
const Router::MetadataMatchCriteria* metadataMatchCriteria() const override { | ||
const Router::MetadataMatchCriteria* metadataMatchCriteria() override { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the retry case you may end up calling this method multiple times. Is it worth checking to see if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think checking for nullptr and returning early is worth it. Multiple calls to merge should yield the same result, but we can avoid the computation and creating new objects. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mattklein123 hold on -- can the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's ok, as long as metadataMatchCriteria() isn't invoked until LoadBalancer::chooseHost, which happens during Router::Filter::getConnPool in Filter::decodeHeaders and again in Filter::doRetry. The route_ and route_entry_ get set in decodeHeaders before load balancing, and I believe that once that happens they do not change. clearRouteCache operates on the Http::ConnectionManagerImpl::ActiveStreamFilterBase which ends up being the source of the route_ and therefore route_entry_ via its implementation of StreamFilterCallbacks::route(). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems right, can we add an ASSERT to this effect (i.e. if we get called twice, the route entry is the same)? |
||
if (route_entry_) { | ||
// The request's metadata, if present, takes precedence over the route's. | ||
const auto& request_metadata = callbacks_->requestInfo().dynamicMetadata().filter_metadata(); | ||
const auto filter_it = request_metadata.find(Envoy::Config::MetadataFilters::get().ENVOY_LB); | ||
if (filter_it != request_metadata.end()) { | ||
metadata_match_ = route_entry_->metadataMatchCriteria()->mergeMatchCriteria(filter_it->second); | ||
return metadata_match_.get(); | ||
} | ||
return route_entry_->metadataMatchCriteria(); | ||
} | ||
return nullptr; | ||
|
@@ -343,6 +351,7 @@ class Filter : Logger::Loggable<Logger::Id::router>, | |
MonotonicTime downstream_request_complete_time_; | ||
uint32_t buffer_limit_{0}; | ||
bool stream_destroyed_{}; | ||
MetadataMatchCriteriaConstPtr metadata_match_; | ||
|
||
// list of cookies to add to upstream headers | ||
std::vector<std::string> downstream_set_cookies_; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -466,8 +466,9 @@ TEST_F(RouterTest, AddMultipleCookies) { | |
TEST_F(RouterTest, MetadataNoOp) { EXPECT_EQ(nullptr, router_.metadataMatchCriteria()); } | ||
|
||
TEST_F(RouterTest, MetadataMatchCriteria) { | ||
MockMetadataMatchCriteria matches; | ||
envoy::api::v2::core::Metadata metadata; | ||
|
||
EXPECT_CALL(callbacks_.request_info_, dynamicMetadata()).WillRepeatedly(ReturnRef(metadata)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you should modify the mock in test/mocks/request_info to return this by default. |
||
ON_CALL(callbacks_.route_->route_entry_, metadataMatchCriteria()) | ||
.WillByDefault(Return(&callbacks_.route_->route_entry_.metadata_matches_criteria_)); | ||
EXPECT_CALL(cm_, httpConnPoolForCluster(_, _, _, _)) | ||
|
@@ -491,6 +492,9 @@ TEST_F(RouterTest, MetadataMatchCriteria) { | |
} | ||
|
||
TEST_F(RouterTest, NoMetadataMatchCriteria) { | ||
envoy::api::v2::core::Metadata metadata; | ||
|
||
EXPECT_CALL(callbacks_.request_info_, dynamicMetadata()).WillRepeatedly(ReturnRef(metadata)); | ||
ON_CALL(callbacks_.route_->route_entry_, metadataMatchCriteria()).WillByDefault(Return(nullptr)); | ||
EXPECT_CALL(cm_, httpConnPoolForCluster(_, _, _, _)) | ||
.WillOnce( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: "with this criteria"