routing: enable routing on query arg match (#2120)#2258
routing: enable routing on query arg match (#2120)#2258htuch merged 4 commits intoenvoyproxy:masterfrom brian-pane:query-routing
Conversation
Signed-off-by: Brian Pane <bpane@pinterest.com>
Signed-off-by: Brian Pane <bpane@pinterest.com>
htuch
left a comment
There was a problem hiding this comment.
Generally looks good, thanks for the contribution.
source/common/config/rds_json.h
Outdated
| envoy::api::v2::HeaderMatcher& header_matcher); | ||
|
|
||
| /** | ||
| * Translate a v1 JSON header matcher object to v2 envoy::api::v2::QueryParameterMatcher. |
There was a problem hiding this comment.
Nit: s/header matcher/query parameter matcher/
source/common/http/utility.cc
Outdated
| if (end == std::string::npos) { | ||
| end = url.size(); | ||
| } | ||
| auto param = StringUtil::subspan(url, start, end); |
There was a problem hiding this comment.
Nit: absl::string_view might be more efficient.
| regex_pattern_(value_, std::regex::optimize), | ||
| is_regex_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, regex, false)) {} | ||
|
|
||
| const std::string name_; |
There was a problem hiding this comment.
Can some of the inner logic in ConfigUtility::matchQueryParams be moved to a method here?
There was a problem hiding this comment.
Sounds good; I've just posted an update that encapsulates the logic in this object. We should probably do the same with struct HeaderData and ConfigUtility::matchHeaders, but I'll make a separate PR for that.
…cient, and improve the encapsulation of QueryParameterMatcher. Signed-off-by: Brian Pane <bpane@pinterest.com>
htuch
left a comment
There was a problem hiding this comment.
Rad, looks good to go after comments.
source/common/router/config_impl.cc
Outdated
| } | ||
|
|
||
| matches &= ConfigUtility::matchHeaders(headers, config_headers_); | ||
| if (config_query_parameters_.size() != 0) { |
There was a problem hiding this comment.
Nit: if (!config_query_parameters_.empty()) {
source/common/http/utility.cc
Outdated
| absl::string_view param(url.c_str() + start, end - start); | ||
|
|
||
| size_t equal = url.find('=', start); | ||
| size_t equal = param.find('='); |
| } | ||
|
|
||
| { | ||
| Http::TestHeaderMapImpl headers = genHeaders("example.com", "/?param=test", "GET"); |
There was a problem hiding this comment.
Can you add a test for multiple query params?
There was a problem hiding this comment.
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.
| if (end == std::string::npos) { | ||
| end = url.size(); | ||
| } | ||
| absl::string_view param(url.c_str() + start, end - start); |
There was a problem hiding this comment.
Is the idea here to avoid a bug where we could see for the = beyond the next query param? If so, can you add a test for this?
There was a problem hiding this comment.
Yes, prior to this, parseQueryString had a bug whereby it would interpret x&y=z as having two key-value variables:
- key=
y, value=z - key=
x&y, value=z
This PR contains a test for that, on line 1079 of config_impl_test.cc.
Signed-off-by: Brian Pane <bpane@pinterest.com>
This pulls in #21070 to update bazel to `6.0.0-pre.20220414.2` which is the second-most recent 6.x rolling release. Bazel release: https://github.com/bazelbuild/bazel/releases/tag/6.0.0-pre.20220414.2 Diff: f17b32f...efbbb04 Signed-off-by: JP Simard <jp@jpsim.com>
This pulls in #21070 to update bazel to `6.0.0-pre.20220414.2` which is the second-most recent 6.x rolling release. Bazel release: https://github.com/bazelbuild/bazel/releases/tag/6.0.0-pre.20220414.2 Diff: f17b32f...efbbb04 Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: Brian Pane bpane@pinterest.com
Description:
This change support route selection based on request query string parameters,
implementing the API changes in data-plane-api commit 05384e0.
Risk Level: Medium
Testing:
New unit tests are included in this PR.