From 74273014fd5bd26e66b8bf08828c8a18171c5872 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Mon, 9 Jan 2017 13:10:15 -0500 Subject: [PATCH 1/6] rename function vars in ConfigUtility::matchHeaders for clarity --- source/common/router/config_impl.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index e34738d408d14..348cefb201fcb 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -48,17 +48,17 @@ Upstream::ResourcePriority ConfigUtility::parsePriority(const Json::Object& conf } } -bool ConfigUtility::matchHeaders(const Http::HeaderMap& headers, - const std::vector request_headers) { +bool ConfigUtility::matchHeaders(const Http::HeaderMap& request_headers, + const std::vector config_headers) { bool matches = true; - if (!request_headers.empty()) { - for (const HeaderData& header_data : request_headers) { - const Http::HeaderEntry* header = headers.get(header_data.name_); - if (header_data.value_ == EMPTY_STRING) { + if (!config_headers.empty()) { + for (const HeaderData& cfg_header_data : config_headers) { + const Http::HeaderEntry* header = request_headers.get(cfg_header_data.name_); + if (cfg_header_data.value_ == EMPTY_STRING) { matches &= (header != nullptr); } else { - matches &= (header != nullptr) && (header->value() == header_data.value_.c_str()); + matches &= (header != nullptr) && (header->value() == cfg_header_data.value_.c_str()); } if (!matches) { break; From fc05c9bee39e32a525a34efb54a84c83d792b9ac Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Mon, 9 Jan 2017 13:12:17 -0500 Subject: [PATCH 2/6] add regex support for request header matching --- source/common/router/config_impl.cc | 3 ++- source/common/router/config_impl.h | 3 ++- test/common/router/config_impl_test.cc | 16 +++++++++++++++- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 348cefb201fcb..eed7a70590ace 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -58,7 +58,8 @@ bool ConfigUtility::matchHeaders(const Http::HeaderMap& request_headers, if (cfg_header_data.value_ == EMPTY_STRING) { matches &= (header != nullptr); } else { - matches &= (header != nullptr) && (header->value() == cfg_header_data.value_.c_str()); + matches &= (header != nullptr) && + std::regex_match(header->value().c_str(), cfg_header_data.pattern_); } if (!matches) { break; diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index baac21ff39982..289bfb31b0467 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -47,10 +47,11 @@ class ConfigUtility { public: struct HeaderData { HeaderData(const Http::LowerCaseString& name, const std::string& value) - : name_(name), value_(value) {} + : name_(name), value_(value), pattern_(value_, std::regex::optimize) {} const Http::LowerCaseString name_; const std::string value_; + const std::regex pattern_; }; /** diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index aca68929f94f5..7a6f7f5dee3bb 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -3,8 +3,8 @@ #include "common/json/json_loader.h" #include "common/router/config_impl.h" -#include "test/mocks/upstream/mocks.h" #include "test/mocks/runtime/mocks.h" +#include "test/mocks/upstream/mocks.h" #include "test/test_common/utility.h" using testing::_; @@ -436,6 +436,13 @@ TEST(RouteMatcherTest, HeaderMatchedRouting) { {"name": "test_header_presence"} ] }, + { + "prefix": "/", + "cluster": "local_service_with_header_pattern", + "headers" : [ + {"name": "test_header_pattern", "value": "^user=test-\\d+$"} + ] + }, { "prefix": "/", "cluster": "local_service_without_headers" @@ -484,6 +491,13 @@ TEST(RouteMatcherTest, HeaderMatchedRouting) { EXPECT_EQ("local_service_with_empty_headers", config.routeForRequest(headers, 0)->clusterName()); } + + { + Http::TestHeaderMapImpl headers = genHeaders("www.lyft.com", "/", "GET"); + headers.addViaCopy("test_header_pattern", "user=test-1223"); + EXPECT_EQ("local_service_with_header_pattern", + config.routeForRequest(headers, 0)->clusterName()); + } } TEST(RouteMatcherTest, ContentType) { From d129ac4d6c7800872bef2e9909c2d6a6b3d8e7dc Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Mon, 9 Jan 2017 16:41:48 -0500 Subject: [PATCH 3/6] Make regex an opt-in via config value --- source/common/http/filter/fault_filter.cc | 3 ++- source/common/router/config_impl.cc | 9 +++++++-- source/common/router/config_impl.h | 5 +++-- test/common/router/config_impl_test.cc | 19 ++++++++++++++++--- 4 files changed, 28 insertions(+), 8 deletions(-) diff --git a/source/common/http/filter/fault_filter.cc b/source/common/http/filter/fault_filter.cc index 36ae4ab7f9cc2..3ddba4244c6ff 100644 --- a/source/common/http/filter/fault_filter.cc +++ b/source/common/http/filter/fault_filter.cc @@ -60,7 +60,8 @@ FaultFilterConfig::FaultFilterConfig(const Json::Object& json_config, Runtime::L for (const Json::ObjectPtr& header_map : config_headers) { // allow header value to be empty, allows matching to be only based on header presence. fault_filter_headers_.emplace_back(Http::LowerCaseString(header_map->getString("name")), - header_map->getString("value", EMPTY_STRING)); + header_map->getString("value", EMPTY_STRING), + header_map->getBoolean("regex", false)); } } } diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index eed7a70590ace..145cd015b7b62 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -57,9 +57,11 @@ bool ConfigUtility::matchHeaders(const Http::HeaderMap& request_headers, const Http::HeaderEntry* header = request_headers.get(cfg_header_data.name_); if (cfg_header_data.value_ == EMPTY_STRING) { matches &= (header != nullptr); - } else { + } else if (cfg_header_data.isregex_) { matches &= (header != nullptr) && std::regex_match(header->value().c_str(), cfg_header_data.pattern_); + } else { + matches &= (header != nullptr) && (header->value() == cfg_header_data.value_.c_str()); } if (!matches) { break; @@ -92,8 +94,11 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost, const Json: std::vector config_headers = route.getObjectArray("headers"); for (const Json::ObjectPtr& header_map : config_headers) { // allow header value to be empty, allows matching to be only based on header presence. + // Regex is an opt-in. Unless explicitly mentioned, we will use header values for exact string + // matches. config_headers_.emplace_back(Http::LowerCaseString(header_map->getString("name")), - header_map->getString("value", EMPTY_STRING)); + header_map->getString("value", EMPTY_STRING), + header_map->getBoolean("regex", false)); } } } diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 289bfb31b0467..91f052d3d4dd4 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -46,12 +46,13 @@ class SslRedirector : public RedirectEntry { class ConfigUtility { public: struct HeaderData { - HeaderData(const Http::LowerCaseString& name, const std::string& value) - : name_(name), value_(value), pattern_(value_, std::regex::optimize) {} + HeaderData(const Http::LowerCaseString& name, const std::string& value, const bool isregex) + : name_(name), value_(value), pattern_(value_, std::regex::optimize), isregex_(isregex) {} const Http::LowerCaseString name_; const std::string value_; const std::regex pattern_; + const bool isregex_; }; /** diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 7a6f7f5dee3bb..8d525f27498f0 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -438,9 +438,16 @@ TEST(RouteMatcherTest, HeaderMatchedRouting) { }, { "prefix": "/", - "cluster": "local_service_with_header_pattern", + "cluster": "local_service_with_header_pattern_set_regex", "headers" : [ - {"name": "test_header_pattern", "value": "^user=test-\\d+$"} + {"name": "test_header_pattern", "value": "^user=test-\\d+$", "regex": true} + ] + }, + { + "prefix": "/", + "cluster": "local_service_with_header_pattern_unset_regex", + "headers" : [ + {"name": "test_header_pattern", "value": "^customer=test-\\d+$"} ] }, { @@ -495,9 +502,15 @@ TEST(RouteMatcherTest, HeaderMatchedRouting) { { Http::TestHeaderMapImpl headers = genHeaders("www.lyft.com", "/", "GET"); headers.addViaCopy("test_header_pattern", "user=test-1223"); - EXPECT_EQ("local_service_with_header_pattern", + EXPECT_EQ("local_service_with_header_pattern_set_regex", config.routeForRequest(headers, 0)->clusterName()); } + + { + Http::TestHeaderMapImpl headers = genHeaders("www.lyft.com", "/", "GET"); + headers.addViaCopy("test_header_pattern", "customer=test-1223"); + EXPECT_EQ("local_service_without_headers", config.routeForRequest(headers, 0)->clusterName()); + } } TEST(RouteMatcherTest, ContentType) { From 0191fbcfdc26c0a759d94d0589b703ae00a15d1f Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Mon, 9 Jan 2017 16:42:24 -0500 Subject: [PATCH 4/6] document config option for regex based header match --- docs/configuration/http_conn_man/route_config/route.rst | 6 +++++- docs/configuration/http_filters/fault_filter.rst | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/docs/configuration/http_conn_man/route_config/route.rst b/docs/configuration/http_conn_man/route_config/route.rst index e27eacaab5135..e15d688aaf0c6 100644 --- a/docs/configuration/http_conn_man/route_config/route.rst +++ b/docs/configuration/http_conn_man/route_config/route.rst @@ -198,7 +198,7 @@ The router can match a request to a route based on headers specified in the rout .. code-block:: json [ - {"name": "...", "value": "..."} + {"name": "...", "value": "...", "regex": "..."} ] name @@ -208,6 +208,10 @@ value *(optional, string)* Specifies the value of the header. If the value is absent a request that has the *name* header will match, regardless of the header's value. +regex + *(optional, boolean)* Specifies whether the header value is a regular + expression or not. Defaults to false. + The router will check the request's headers against all the specified headers in the route config. A match will happen if all the headers in the route are present in the request with the same values (or based on presence if the ``value`` field is not in the config). diff --git a/docs/configuration/http_filters/fault_filter.rst b/docs/configuration/http_filters/fault_filter.rst index 9749d0f67525e..8aad97a27cc1e 100644 --- a/docs/configuration/http_filters/fault_filter.rst +++ b/docs/configuration/http_filters/fault_filter.rst @@ -119,7 +119,7 @@ actual fault injection further depend on the values of *abort_percent* and .. code-block:: json [ - {"name": "...", "value": "..."} + {"name": "...", "value": "...", "regex": "..."} ] name @@ -130,6 +130,10 @@ value absent a request that has the *name* header will match, regardless of the header's value. +regex + *(optional, boolean)* Specifies whether the header value is a regular expression + or not. Defaults to false. + The filter will check the request's headers against all the specified headers in the filter config. A match will happen if all the headers in the config are present in the request with the same values (or based on From 145015bd596f64cbb0a9d08a72e7d7554eff835a Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 10 Jan 2017 08:56:31 -0500 Subject: [PATCH 5/6] add links to regex grammar used for header value --- docs/configuration/http_conn_man/route_config/route.rst | 3 ++- docs/configuration/http_filters/fault_filter.rst | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/configuration/http_conn_man/route_config/route.rst b/docs/configuration/http_conn_man/route_config/route.rst index e15d688aaf0c6..f162237b464ec 100644 --- a/docs/configuration/http_conn_man/route_config/route.rst +++ b/docs/configuration/http_conn_man/route_config/route.rst @@ -210,7 +210,8 @@ value regex *(optional, boolean)* Specifies whether the header value is a regular - expression or not. Defaults to false. + expression or not. Defaults to false. The regex grammar used in the value field + is defined `here `_. The router will check the request's headers against all the specified headers in the route config. A match will happen if all the headers in the route are present in diff --git a/docs/configuration/http_filters/fault_filter.rst b/docs/configuration/http_filters/fault_filter.rst index 8aad97a27cc1e..ceb2463784462 100644 --- a/docs/configuration/http_filters/fault_filter.rst +++ b/docs/configuration/http_filters/fault_filter.rst @@ -132,7 +132,8 @@ value regex *(optional, boolean)* Specifies whether the header value is a regular expression - or not. Defaults to false. + or not. Defaults to false. The regex grammar used in the value field + is defined `here `_. The filter will check the request's headers against all the specified headers in the filter config. A match will happen if all the headers in the From 88d5037c3ad45d60d2d2210000a602ed0581b137 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 10 Jan 2017 09:04:22 -0500 Subject: [PATCH 6/6] address PR comments from Matt --- source/common/router/config_impl.cc | 10 +++++----- source/common/router/config_impl.h | 9 +++++---- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 145cd015b7b62..398bc8fe1f66c 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -55,13 +55,13 @@ bool ConfigUtility::matchHeaders(const Http::HeaderMap& request_headers, if (!config_headers.empty()) { for (const HeaderData& cfg_header_data : config_headers) { const Http::HeaderEntry* header = request_headers.get(cfg_header_data.name_); - if (cfg_header_data.value_ == EMPTY_STRING) { + if (cfg_header_data.value_.empty()) { matches &= (header != nullptr); - } else if (cfg_header_data.isregex_) { - matches &= (header != nullptr) && - std::regex_match(header->value().c_str(), cfg_header_data.pattern_); - } else { + } else if (!cfg_header_data.is_regex_) { matches &= (header != nullptr) && (header->value() == cfg_header_data.value_.c_str()); + } else { + matches &= (header != nullptr) && + std::regex_match(header->value().c_str(), cfg_header_data.regex_pattern_); } if (!matches) { break; diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 91f052d3d4dd4..33c14887e5746 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -46,13 +46,14 @@ class SslRedirector : public RedirectEntry { class ConfigUtility { public: struct HeaderData { - HeaderData(const Http::LowerCaseString& name, const std::string& value, const bool isregex) - : name_(name), value_(value), pattern_(value_, std::regex::optimize), isregex_(isregex) {} + HeaderData(const Http::LowerCaseString& name, const std::string& value, const bool is_regex) + : name_(name), value_(value), regex_pattern_(value_, std::regex::optimize), + is_regex_(is_regex) {} const Http::LowerCaseString name_; const std::string value_; - const std::regex pattern_; - const bool isregex_; + const std::regex regex_pattern_; + const bool is_regex_; }; /**