Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion bazel/repository_locations.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ REPOSITORY_LOCATIONS = dict(
urls = ["https://github.com/google/protobuf/archive/v3.5.0.tar.gz"],
),
envoy_api = dict(
commit = "3deffbd132b40fa544137902fdf83bec7442a87f",
commit = "05384e069b3ea910b04d1ee267aa4fc0fdf15103",
remote = "https://github.com/envoyproxy/data-plane-api",
),
grpc_httpjson_transcoding = dict(
Expand Down
15 changes: 15 additions & 0 deletions source/common/config/rds_json.cc
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,15 @@ void RdsJson::translateHeaderMatcher(const Json::Object& json_header_matcher,
JSON_UTIL_SET_BOOL(json_header_matcher, header_matcher, regex);
}

void RdsJson::translateQueryParameterMatcher(
const Json::Object& json_query_parameter_matcher,
envoy::api::v2::QueryParameterMatcher& query_parameter_matcher) {
json_query_parameter_matcher.validateSchema(Json::Schema::QUERY_PARAMETER_CONFIGURATION_SCHEMA);
JSON_UTIL_SET_STRING(json_query_parameter_matcher, query_parameter_matcher, name);
JSON_UTIL_SET_STRING(json_query_parameter_matcher, query_parameter_matcher, value);
JSON_UTIL_SET_BOOL(json_query_parameter_matcher, query_parameter_matcher, regex);
}

void RdsJson::translateRouteConfiguration(const Json::Object& json_route_config,
envoy::api::v2::RouteConfiguration& route_config) {
json_route_config.validateSchema(Json::Schema::ROUTE_CONFIGURATION_SCHEMA);
Expand Down Expand Up @@ -211,6 +220,12 @@ void RdsJson::translateRoute(const Json::Object& json_route, envoy::api::v2::Rou
translateHeaderMatcher(*json_header_matcher, *header_matcher);
}

for (const auto json_query_parameter_matcher :
json_route.getObjectArray("query_parameters", true)) {
auto* query_parameter_matcher = match->mutable_query_parameters()->Add();
translateQueryParameterMatcher(*json_query_parameter_matcher, *query_parameter_matcher);
}

bool has_redirect = false;
if (json_route.hasObject("host_redirect") || json_route.hasObject("path_redirect")) {
has_redirect = true;
Expand Down
9 changes: 9 additions & 0 deletions source/common/config/rds_json.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,15 @@ class RdsJson {
static void translateHeaderMatcher(const Json::Object& json_header_matcher,
envoy::api::v2::HeaderMatcher& header_matcher);

/**
* Translate a v1 JSON query parameter matcher object to v2 envoy::api::v2::QueryParameterMatcher.
* @param json_query_parameter_matcher source v1 JSON query parameter matcher object.
* @param query_parameter_matcher destination v2 envoy::api::v2::QueryParameterMatcher.
*/
static void
translateQueryParameterMatcher(const Json::Object& json_query_parameter_matcher,
envoy::api::v2::QueryParameterMatcher& query_parameter_matcher);

/**
* Translate a v1 JSON route configuration object to v2 envoy::api::v2::RouteConfiguration.
* @param json_route_config source v1 JSON route configuration object.
Expand Down
7 changes: 4 additions & 3 deletions source/common/http/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,12 @@ Utility::QueryParams Utility::parseQueryString(const std::string& url) {
if (end == std::string::npos) {
end = url.size();
}
absl::string_view param(url.c_str() + start, end - start);
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.

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?

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.

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.


size_t equal = url.find('=', start);
const size_t equal = param.find('=');
if (equal != std::string::npos) {
params.emplace(StringUtil::subspan(url, start, equal),
StringUtil::subspan(url, equal + 1, end));
params.emplace(StringUtil::subspan(url, start, start + equal),
StringUtil::subspan(url, start + equal + 1, end));
} else {
params.emplace(StringUtil::subspan(url, start, end), "");
}
Expand Down
21 changes: 21 additions & 0 deletions source/common/json/config_schemas.cc
Original file line number Diff line number Diff line change
Expand Up @@ -721,6 +721,13 @@ const std::string Json::Schema::ROUTE_ENTRY_CONFIGURATION_SCHEMA(R"EOF(
"type" : "object"
}
},
"query_parameters": {
"type": "array",
"minItems": 1,
"items": {
"type": "object"
}
},
"rate_limits" : {"type" : "array"},
"include_vh_rate_limits" : {"type" : "boolean"},
"hash_policy" : {
Expand Down Expand Up @@ -794,6 +801,20 @@ const std::string Json::Schema::HEADER_DATA_CONFIGURATION_SCHEMA(R"EOF(
}
)EOF");

const std::string Json::Schema::QUERY_PARAMETER_CONFIGURATION_SCHEMA(R"EOF(
{
"$schema" : "http://json-schema.org/schema#",
"type" : "object",
"properties" : {
"name" : {"type" : "string"},
"value" : {"type" : "string"},
"regex" : {"type" : "boolean"}
},
"required" : ["name"],
"additionalProperties" : false
}
)EOF");

const std::string Json::Schema::HTTP_RATE_LIMITS_CONFIGURATION_SCHEMA(R"EOF(
{
"$schema": "http://json-schema.org/schema#",
Expand Down
1 change: 1 addition & 0 deletions source/common/json/config_schemas.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ class Schema {
static const std::string HTTP_RATE_LIMITS_CONFIGURATION_SCHEMA;
static const std::string RDS_CONFIGURATION_SCHEMA;
static const std::string HEADER_DATA_CONFIGURATION_SCHEMA;
static const std::string QUERY_PARAMETER_CONFIGURATION_SCHEMA;

// HTTP Filter Schemas
static const std::string BUFFER_HTTP_FILTER_SCHEMA;
Expand Down
9 changes: 9 additions & 0 deletions source/common/router/config_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
}
Expand All @@ -315,6 +319,11 @@ bool RouteEntryImplBase::matchRoute(const Http::HeaderMap& headers, uint64_t ran
}

matches &= ConfigUtility::matchHeaders(headers, config_headers_);
if (!config_query_parameters_.empty()) {
Http::Utility::QueryParams query_parameters =
Http::Utility::parseQueryString(headers.Path()->value().c_str());
matches &= ConfigUtility::matchQueryParams(query_parameters, config_query_parameters_);
}

return matches;
}
Expand Down
1 change: 1 addition & 0 deletions source/common/router/config_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,7 @@ class RouteEntryImplBase : public RouteEntry,
const ShadowPolicyImpl shadow_policy_;
const Upstream::ResourcePriority priority_;
std::vector<ConfigUtility::HeaderData> config_headers_;
std::vector<ConfigUtility::QueryParameterMatcher> config_query_parameters_;
std::vector<WeightedClusterEntrySharedPtr> weighted_clusters_;
std::unique_ptr<const HashPolicyImpl> hash_policy_;
MetadataMatchCriteriaImplConstPtr metadata_match_criteria_;
Expand Down
26 changes: 26 additions & 0 deletions source/common/router/config_utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,20 @@
namespace Envoy {
namespace Router {

bool ConfigUtility::QueryParameterMatcher::matches(
const Http::Utility::QueryParams& request_query_params) const {
auto query_param = request_query_params.find(name_);
if (query_param == request_query_params.end()) {
return false;
} else if (is_regex_) {
return std::regex_match(query_param->second, regex_pattern_);
} else if (value_.length() == 0) {
return true;
} else {
return (value_ == query_param->second);
}
}

Upstream::ResourcePriority
ConfigUtility::parsePriority(const envoy::api::v2::RoutingPriority& priority) {
switch (priority) {
Expand Down Expand Up @@ -45,6 +59,18 @@ bool ConfigUtility::matchHeaders(const Http::HeaderMap& request_headers,
return matches;
}

bool ConfigUtility::matchQueryParams(
const Http::Utility::QueryParams& query_params,
const std::vector<QueryParameterMatcher>& config_query_params) {
for (const auto& config_query_param : config_query_params) {
if (!config_query_param.matches(query_params)) {
return false;
}
}

return true;
}

Http::Code ConfigUtility::parseRedirectResponseCode(
const envoy::api::v2::RedirectAction::RedirectResponseCode& code) {
switch (code) {
Expand Down
36 changes: 36 additions & 0 deletions source/common/router/config_utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -44,6 +45,31 @@ 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.
class QueryParameterMatcher {
public:
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)) {}

/**
* Check if the query parameters for a request contain a match for this
* QueryParameterMatcher.
* @param request_query_params supplies the parsed query parameters from a request.
* @return bool true if a match for this QueryParameterMatcher exists in request_query_params.
*/
bool matches(const Http::Utility::QueryParams& request_query_params) const;

private:
const std::string name_;
const std::string value_;
const std::regex regex_pattern_;
const bool is_regex_;
};

/**
* @return the resource priority parsed from proto.
*/
Expand All @@ -59,6 +85,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.
Expand Down
93 changes: 93 additions & 0 deletions test/common/router/config_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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");
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.

Can you add a test for multiple query params?

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.

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()
Expand Down