-
Notifications
You must be signed in to change notification settings - Fork 5.5k
routing: enable routing on query arg match (#2120) #2258
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,13 +57,14 @@ Utility::QueryParams Utility::parseQueryString(const std::string& url) { | |
| if (end == std::string::npos) { | ||
| end = url.size(); | ||
| } | ||
| auto param = StringUtil::subspan(url, start, end); | ||
|
Member
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. Nit: |
||
|
|
||
| size_t equal = url.find('=', start); | ||
| size_t equal = param.find('='); | ||
|
Member
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. Nit: |
||
| if (equal != std::string::npos) { | ||
| params.emplace(StringUtil::subspan(url, start, equal), | ||
| StringUtil::subspan(url, equal + 1, end)); | ||
| params.emplace(StringUtil::subspan(param, 0, equal), | ||
| StringUtil::subspan(param, equal + 1, param.length())); | ||
| } else { | ||
| params.emplace(StringUtil::subspan(url, start, end), ""); | ||
| params.emplace(param, ""); | ||
| } | ||
|
|
||
| start = end + 1; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -291,6 +291,10 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost, | |
| config_headers_.push_back(header_map); | ||
| } | ||
|
|
||
| for (const auto& query_parameter : route.match().query_parameters()) { | ||
| config_query_parameters_.push_back(query_parameter); | ||
| } | ||
|
|
||
| if (!route.route().hash_policy().empty()) { | ||
| hash_policy_.reset(new HashPolicyImpl(route.route().hash_policy())); | ||
| } | ||
|
|
@@ -315,6 +319,11 @@ bool RouteEntryImplBase::matchRoute(const Http::HeaderMap& headers, uint64_t ran | |
| } | ||
|
|
||
| matches &= ConfigUtility::matchHeaders(headers, config_headers_); | ||
| if (config_query_parameters_.size() != 0) { | ||
|
Member
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. Nit: |
||
| Http::Utility::QueryParams query_parameters = | ||
| Http::Utility::parseQueryString(headers.Path()->value().c_str()); | ||
| matches &= ConfigUtility::matchQueryParams(query_parameters, config_query_parameters_); | ||
| } | ||
|
|
||
| return matches; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
| #include "common/common/empty_string.h" | ||
| #include "common/config/rds_json.h" | ||
| #include "common/http/headers.h" | ||
| #include "common/http/utility.h" | ||
| #include "common/protobuf/utility.h" | ||
|
|
||
| #include "api/rds.pb.h" | ||
|
|
@@ -44,6 +45,21 @@ class ConfigUtility { | |
| const bool is_regex_; | ||
| }; | ||
|
|
||
| // A QueryParameterMatcher specifies one "name" or "name=value" element | ||
| // to match in a request's query string. It is the optimized, runtime | ||
| // equivalent of the QueryParameterMatcher proto in the RDS v2 API. | ||
| struct QueryParameterMatcher { | ||
| QueryParameterMatcher(const envoy::api::v2::QueryParameterMatcher& config) | ||
| : name_(config.name()), value_(config.value()), | ||
| regex_pattern_(value_, std::regex::optimize), | ||
| is_regex_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, regex, false)) {} | ||
|
|
||
| const std::string name_; | ||
|
Member
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. Can some of the inner logic in
Contributor
Author
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. Sounds good; I've just posted an update that encapsulates the logic in this object. We should probably do the same with |
||
| const std::string value_; | ||
| const std::regex regex_pattern_; | ||
| const bool is_regex_; | ||
| }; | ||
|
|
||
| /** | ||
| * @return the resource priority parsed from proto. | ||
| */ | ||
|
|
@@ -59,6 +75,16 @@ class ConfigUtility { | |
| static bool matchHeaders(const Http::HeaderMap& request_headers, | ||
| const std::vector<HeaderData>& config_headers); | ||
|
|
||
| /** | ||
| * See if the query parameters specified in the config are present in a request. | ||
| * @param query_params supplies the query parameters from the request's query string. | ||
| * @param config_params supplies the list of configured query param conditions on which to match. | ||
| * @return bool true if all the query params (and values) in the config_params are found in the | ||
| * query_params | ||
| */ | ||
| static bool matchQueryParams(const Http::Utility::QueryParams& query_params, | ||
| const std::vector<QueryParameterMatcher>& config_query_params); | ||
|
|
||
| /** | ||
| * Returns the redirect HTTP Status Code enum parsed from proto. | ||
| * @param code supplies the RedirectResponseCode enum. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -995,6 +995,99 @@ TEST(RouteMatcherTest, HeaderMatchedRouting) { | |
| } | ||
| } | ||
|
|
||
| TEST(RouteMatcherTest, QueryParamMatchedRouting) { | ||
| std::string json = R"EOF( | ||
| { | ||
| "virtual_hosts": [ | ||
| { | ||
| "name": "local_service", | ||
| "domains": ["*"], | ||
| "routes": [ | ||
| { | ||
| "prefix": "/", | ||
| "cluster": "local_service_with_multiple_query_parameters", | ||
| "query_parameters": [ | ||
| {"name": "id", "value": "\\d+[02468]", "regex": true}, | ||
| {"name": "debug"} | ||
| ] | ||
| }, | ||
| { | ||
| "prefix": "/", | ||
| "cluster": "local_service_with_query_parameter", | ||
| "query_parameters": [ | ||
| {"name": "param", "value": "test"} | ||
| ] | ||
| }, | ||
| { | ||
| "prefix": "/", | ||
| "cluster": "local_service_with_valueless_query_parameter", | ||
| "query_parameters": [ | ||
| {"name": "debug"} | ||
| ] | ||
| }, | ||
| { | ||
| "prefix": "/", | ||
| "cluster": "local_service_without_query_parameters" | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| )EOF"; | ||
|
|
||
| NiceMock<Runtime::MockLoader> runtime; | ||
| NiceMock<Upstream::MockClusterManager> cm; | ||
| ConfigImpl config(parseRouteConfigurationFromJson(json), runtime, cm, true); | ||
|
|
||
| { | ||
| Http::TestHeaderMapImpl headers = genHeaders("example.com", "/", "GET"); | ||
| EXPECT_EQ("local_service_without_query_parameters", | ||
| config.route(headers, 0)->routeEntry()->clusterName()); | ||
| } | ||
|
|
||
| { | ||
| Http::TestHeaderMapImpl headers = genHeaders("example.com", "/?", "GET"); | ||
| EXPECT_EQ("local_service_without_query_parameters", | ||
| config.route(headers, 0)->routeEntry()->clusterName()); | ||
| } | ||
|
|
||
| { | ||
| Http::TestHeaderMapImpl headers = genHeaders("example.com", "/?param=testing", "GET"); | ||
| EXPECT_EQ("local_service_without_query_parameters", | ||
| config.route(headers, 0)->routeEntry()->clusterName()); | ||
| } | ||
|
|
||
| { | ||
| Http::TestHeaderMapImpl headers = genHeaders("example.com", "/?param=test", "GET"); | ||
|
Member
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. Can you add a test for multiple query params?
Contributor
Author
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 put a couple of multi-param tests a bit later in config_impl_test.cc: all parameters match and proper subset of parameters match. |
||
| EXPECT_EQ("local_service_with_query_parameter", | ||
| config.route(headers, 0)->routeEntry()->clusterName()); | ||
| } | ||
|
|
||
| { | ||
| Http::TestHeaderMapImpl headers = genHeaders("example.com", "/?debug", "GET"); | ||
| EXPECT_EQ("local_service_with_valueless_query_parameter", | ||
| config.route(headers, 0)->routeEntry()->clusterName()); | ||
| } | ||
|
|
||
| { | ||
| Http::TestHeaderMapImpl headers = genHeaders("example.com", "/?debug=2", "GET"); | ||
| EXPECT_EQ("local_service_with_valueless_query_parameter", | ||
| config.route(headers, 0)->routeEntry()->clusterName()); | ||
| } | ||
|
|
||
| { | ||
| Http::TestHeaderMapImpl headers = genHeaders("example.com", "/?param=test&debug&id=01", "GET"); | ||
| EXPECT_EQ("local_service_with_query_parameter", | ||
| config.route(headers, 0)->routeEntry()->clusterName()); | ||
| } | ||
|
|
||
| { | ||
| Http::TestHeaderMapImpl headers = genHeaders("example.com", "/?param=test&debug&id=02", "GET"); | ||
| EXPECT_EQ("local_service_with_multiple_query_parameters", | ||
| config.route(headers, 0)->routeEntry()->clusterName()); | ||
| } | ||
| } | ||
|
|
||
| class RouterMatcherHashPolicyTest : public testing::Test { | ||
| public: | ||
| RouterMatcherHashPolicyTest() | ||
|
|
||
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: s/header matcher/query parameter matcher/