From 5adf015f34203ecf0aadd0fe8430ecbd4fe1fa1f Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 20 Apr 2017 15:34:31 -0700 Subject: [PATCH 1/6] initial --- .../http_conn_man/route_config/rate_limits.rst | 12 +++++++++--- source/common/router/router_ratelimit.cc | 18 +++++++++++------- source/common/router/router_ratelimit.h | 1 + 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/docs/configuration/http_conn_man/route_config/rate_limits.rst b/docs/configuration/http_conn_man/route_config/rate_limits.rst index 5729e2e844c86..a49494eb4f8e3 100644 --- a/docs/configuration/http_conn_man/route_config/rate_limits.rst +++ b/docs/configuration/http_conn_man/route_config/rate_limits.rst @@ -141,7 +141,7 @@ Generic Key descriptor_value - *(required, string)* The value to use in the descriptor entry. + *(required, string)* The value to use in the descriptor entry. The following descriptor entry is appended to the descriptor: @@ -157,15 +157,21 @@ Header Value Match { "type": "header_value_match", "descriptor_value" : "...", + "header_present" : "...", "headers" : [] } descriptor_value - *(required, string)* The value to use in the descriptor entry. + *(required, string)* The value to use in the descriptor entry. + +header_present + *(optional, boolean)* Specifies if the action should be checking for the headers being present. If + set to true, the action will append a descriptor entry if the headers match. If set to + false, the action will append a descriptor entry if the headers do not match. :ref:`headers` - *(required, array)* Specifies a set of headers that the rate limit action should match on. The + *(required, array)* Specifies a set of headers that the rate limit action should match on. The action will check the request's headers against all the specified headers in the config. A match will happen if all the headers in the config 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/source/common/router/router_ratelimit.cc b/source/common/router/router_ratelimit.cc index ef4d61527e6ab..e91cd448f011f 100644 --- a/source/common/router/router_ratelimit.cc +++ b/source/common/router/router_ratelimit.cc @@ -62,7 +62,8 @@ bool GenericKeyAction::populateDescriptor(const Router::RouteEntry&, } HeaderValueMatchAction::HeaderValueMatchAction(const Json::Object& action) - : descriptor_value_(action.getString("descriptor_value")) { + : descriptor_value_(action.getString("descriptor_value")), + header_present_(action.getBoolean("header_present", true)) { std::vector config_headers = action.getObjectArray("headers"); for (const Json::ObjectPtr& header_map : config_headers) { action_headers_.push_back(*header_map); @@ -73,12 +74,15 @@ bool HeaderValueMatchAction::populateDescriptor(const Router::RouteEntry&, ::RateLimit::Descriptor& descriptor, const std::string&, const Http::HeaderMap& headers, const std::string&) const { - if (ConfigUtility::matchHeaders(headers, action_headers_)) { - descriptor.entries_.push_back({"header_match", descriptor_value_}); - return true; - } else { - return false; - } + bool headers_match = ConfigUtility::matchHeaders(headers, action_headers_); + + if (headers_match && he) + if (ConfigUtility::matchHeaders(headers, action_headers_)) { + descriptor.entries_.push_back({"header_match", descriptor_value_}); + return true; + } else { + return false; + } } RateLimitPolicyEntryImpl::RateLimitPolicyEntryImpl(const Json::Object& config) diff --git a/source/common/router/router_ratelimit.h b/source/common/router/router_ratelimit.h index 9fe5ff2662185..b8285d761aca3 100644 --- a/source/common/router/router_ratelimit.h +++ b/source/common/router/router_ratelimit.h @@ -97,6 +97,7 @@ class HeaderValueMatchAction : public RateLimitAction { private: const std::string descriptor_value_; + const bool header_present_; std::vector action_headers_; }; From ae8ae40b6352015a2beff5c9aad23eb5da0c9dd5 Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 20 Apr 2017 17:03:20 -0700 Subject: [PATCH 2/6] added tests --- .../route_config/rate_limits.rst | 22 ++++---- source/common/router/router_ratelimit.cc | 17 +++--- source/common/router/router_ratelimit.h | 2 +- test/common/router/router_ratelimit_test.cc | 55 +++++++++++++++++++ 4 files changed, 75 insertions(+), 21 deletions(-) diff --git a/docs/configuration/http_conn_man/route_config/rate_limits.rst b/docs/configuration/http_conn_man/route_config/rate_limits.rst index a49494eb4f8e3..07c55f1afb202 100644 --- a/docs/configuration/http_conn_man/route_config/rate_limits.rst +++ b/docs/configuration/http_conn_man/route_config/rate_limits.rst @@ -157,7 +157,7 @@ Header Value Match { "type": "header_value_match", "descriptor_value" : "...", - "header_present" : "...", + "headers_present" : "...", "headers" : [] } @@ -165,19 +165,21 @@ Header Value Match descriptor_value *(required, string)* The value to use in the descriptor entry. -header_present - *(optional, boolean)* Specifies if the action should be checking for the headers being present. If - set to true, the action will append a descriptor entry if the headers match. If set to - false, the action will append a descriptor entry if the headers do not match. +headers_present + *(optional, boolean)* Specifies when the action should append a descriptor based on the + :ref:`headers` matching or not. If set to true, + the action will append a descriptor entry when the request does match the headers. + If set to false, the action will append a descriptor entry when the request does not match the + headers. The default value is true. :ref:`headers` *(required, array)* Specifies a set of headers that the rate limit action should match on. The - action will check the request's headers against all the specified headers in the config. A match - will happen if all the headers in the config are present in the request with the same values (or - based on presence if the ``value`` field is not in the config). + action will check the request's headers against all the specified headers in the config. A match + will happen if all the headers in the config are present in the request with the same values (or + based on presence if the ``value`` field is not in the config). -The following descriptor entry is appended to the descriptor if the request matches the headers -specified in the action config: +The following descriptor entry is appended to the descriptor if the headers match and +headers_present is true or the headers do not match and headers_present is false: .. code-block:: cpp diff --git a/source/common/router/router_ratelimit.cc b/source/common/router/router_ratelimit.cc index e91cd448f011f..9db5d73bc2b98 100644 --- a/source/common/router/router_ratelimit.cc +++ b/source/common/router/router_ratelimit.cc @@ -63,7 +63,7 @@ bool GenericKeyAction::populateDescriptor(const Router::RouteEntry&, HeaderValueMatchAction::HeaderValueMatchAction(const Json::Object& action) : descriptor_value_(action.getString("descriptor_value")), - header_present_(action.getBoolean("header_present", true)) { + headers_present_(action.getBoolean("headers_present", true)) { std::vector config_headers = action.getObjectArray("headers"); for (const Json::ObjectPtr& header_map : config_headers) { action_headers_.push_back(*header_map); @@ -74,15 +74,12 @@ bool HeaderValueMatchAction::populateDescriptor(const Router::RouteEntry&, ::RateLimit::Descriptor& descriptor, const std::string&, const Http::HeaderMap& headers, const std::string&) const { - bool headers_match = ConfigUtility::matchHeaders(headers, action_headers_); - - if (headers_match && he) - if (ConfigUtility::matchHeaders(headers, action_headers_)) { - descriptor.entries_.push_back({"header_match", descriptor_value_}); - return true; - } else { - return false; - } + if (headers_present_ == ConfigUtility::matchHeaders(headers, action_headers_)) { + descriptor.entries_.push_back({"header_match", descriptor_value_}); + return true; + } else { + return false; + } } RateLimitPolicyEntryImpl::RateLimitPolicyEntryImpl(const Json::Object& config) diff --git a/source/common/router/router_ratelimit.h b/source/common/router/router_ratelimit.h index b8285d761aca3..5e67d90b28d48 100644 --- a/source/common/router/router_ratelimit.h +++ b/source/common/router/router_ratelimit.h @@ -97,7 +97,7 @@ class HeaderValueMatchAction : public RateLimitAction { private: const std::string descriptor_value_; - const bool header_present_; + const bool headers_present_; std::vector action_headers_; }; diff --git a/test/common/router/router_ratelimit_test.cc b/test/common/router/router_ratelimit_test.cc index 9082a80e1cae0..4fbe70c6deba2 100644 --- a/test/common/router/router_ratelimit_test.cc +++ b/test/common/router/router_ratelimit_test.cc @@ -552,6 +552,61 @@ TEST_F(RateLimitPolicyEntryTest, HeaderValueMatchNoMatch) { EXPECT_TRUE(descriptors_.empty()); } +TEST_F(RateLimitPolicyEntryTest, HeaderValueMatchHeadersNotPresent) { + std::string json = R"EOF( + { + "actions": [ + { + "type": "header_value_match", + "descriptor_value": "fake_value", + "headers_present": false, + "headers": [ + { + "name": "x-header-name", + "value": "test_value", + "regex": false + } + ] + } + ] + } + )EOF"; + + SetUpTest(json); + Http::TestHeaderMapImpl header{{"x-header-name", "fake_value"}}; + + rate_limit_entry_->populateDescriptors(route_, descriptors_, "", header, ""); + EXPECT_THAT(std::vector<::RateLimit::Descriptor>({{{{"header_match", "fake_value"}}}}), + testing::ContainerEq(descriptors_)); +} + +TEST_F(RateLimitPolicyEntryTest, HeaderValueMatchHeadersPresent) { + std::string json = R"EOF( + { + "actions": [ + { + "type": "header_value_match", + "descriptor_value": "fake_value", + "headers_present": false, + "headers": [ + { + "name": "x-header-name", + "value": "test_value", + "regex": false + } + ] + } + ] + } + )EOF"; + + SetUpTest(json); + Http::TestHeaderMapImpl header{{"x-header-name", "test_value"}}; + + rate_limit_entry_->populateDescriptors(route_, descriptors_, "", header, ""); + EXPECT_TRUE(descriptors_.empty()); +} + TEST_F(RateLimitPolicyEntryTest, CompoundActions) { std::string json = R"EOF( { From 5a3f09e3d70343029ccb036494e097bdd4014492 Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 20 Apr 2017 17:45:30 -0700 Subject: [PATCH 3/6] update documentation --- .../http_conn_man/route_config/rate_limits.rst | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/docs/configuration/http_conn_man/route_config/rate_limits.rst b/docs/configuration/http_conn_man/route_config/rate_limits.rst index 07c55f1afb202..e9494096525cc 100644 --- a/docs/configuration/http_conn_man/route_config/rate_limits.rst +++ b/docs/configuration/http_conn_man/route_config/rate_limits.rst @@ -167,10 +167,10 @@ descriptor_value headers_present *(optional, boolean)* Specifies when the action should append a descriptor based on the - :ref:`headers` matching or not. If set to true, - the action will append a descriptor entry when the request does match the headers. - If set to false, the action will append a descriptor entry when the request does not match the - headers. The default value is true. + :ref:`headers` matching. If set to true, + the action will append a descriptor entry when the request matches the headers. If set to false, + the action will append a descriptor entry when the request does not match the headers. The default + value is true. :ref:`headers` *(required, array)* Specifies a set of headers that the rate limit action should match on. The @@ -178,9 +178,7 @@ headers_present will happen if all the headers in the config are present in the request with the same values (or based on presence if the ``value`` field is not in the config). -The following descriptor entry is appended to the descriptor if the headers match and -headers_present is true or the headers do not match and headers_present is false: - +The following descriptor entry is appended to the descriptor: .. code-block:: cpp ("header_match", "") From 540ab2749285c5b5e02cc62bb5386b145731c8dd Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 20 Apr 2017 17:56:27 -0700 Subject: [PATCH 4/6] update fields --- .../http_conn_man/route_config/rate_limits.rst | 6 +++--- source/common/json/config_schemas.cc | 1 + source/common/router/router_ratelimit.cc | 4 ++-- source/common/router/router_ratelimit.h | 2 +- test/common/router/router_ratelimit_test.cc | 4 ++-- 5 files changed, 9 insertions(+), 8 deletions(-) diff --git a/docs/configuration/http_conn_man/route_config/rate_limits.rst b/docs/configuration/http_conn_man/route_config/rate_limits.rst index e9494096525cc..4071b7fd9c302 100644 --- a/docs/configuration/http_conn_man/route_config/rate_limits.rst +++ b/docs/configuration/http_conn_man/route_config/rate_limits.rst @@ -165,9 +165,9 @@ Header Value Match descriptor_value *(required, string)* The value to use in the descriptor entry. -headers_present - *(optional, boolean)* Specifies when the action should append a descriptor based on the - :ref:`headers` matching. If set to true, +expect_match + *(optional, boolean)* Specifies if the action should append a descriptor when it expects a match + to the :ref:`headers`. If set to true, the action will append a descriptor entry when the request matches the headers. If set to false, the action will append a descriptor entry when the request does not match the headers. The default value is true. diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index 63e22e0b06c05..28709c1ee2d2a 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -721,6 +721,7 @@ const std::string Json::Schema::HTTP_RATE_LIMITS_CONFIGURATION_SCHEMA(R"EOF( "enum" : ["header_value_match"] }, "descriptor_value" : {"type" : "string"}, + "expect_match" : {"type" : "boolean"}, "headers" : { "type" : "array", "minItems" : 1, diff --git a/source/common/router/router_ratelimit.cc b/source/common/router/router_ratelimit.cc index 9db5d73bc2b98..6e4cc82fe9677 100644 --- a/source/common/router/router_ratelimit.cc +++ b/source/common/router/router_ratelimit.cc @@ -63,7 +63,7 @@ bool GenericKeyAction::populateDescriptor(const Router::RouteEntry&, HeaderValueMatchAction::HeaderValueMatchAction(const Json::Object& action) : descriptor_value_(action.getString("descriptor_value")), - headers_present_(action.getBoolean("headers_present", true)) { + expect_match_(action.getBoolean("expect_match", true)) { std::vector config_headers = action.getObjectArray("headers"); for (const Json::ObjectPtr& header_map : config_headers) { action_headers_.push_back(*header_map); @@ -74,7 +74,7 @@ bool HeaderValueMatchAction::populateDescriptor(const Router::RouteEntry&, ::RateLimit::Descriptor& descriptor, const std::string&, const Http::HeaderMap& headers, const std::string&) const { - if (headers_present_ == ConfigUtility::matchHeaders(headers, action_headers_)) { + if (expect_match_ == ConfigUtility::matchHeaders(headers, action_headers_)) { descriptor.entries_.push_back({"header_match", descriptor_value_}); return true; } else { diff --git a/source/common/router/router_ratelimit.h b/source/common/router/router_ratelimit.h index 5e67d90b28d48..65d7620539a2c 100644 --- a/source/common/router/router_ratelimit.h +++ b/source/common/router/router_ratelimit.h @@ -97,7 +97,7 @@ class HeaderValueMatchAction : public RateLimitAction { private: const std::string descriptor_value_; - const bool headers_present_; + const bool expect_match_; std::vector action_headers_; }; diff --git a/test/common/router/router_ratelimit_test.cc b/test/common/router/router_ratelimit_test.cc index 4fbe70c6deba2..b04108564176b 100644 --- a/test/common/router/router_ratelimit_test.cc +++ b/test/common/router/router_ratelimit_test.cc @@ -559,7 +559,7 @@ TEST_F(RateLimitPolicyEntryTest, HeaderValueMatchHeadersNotPresent) { { "type": "header_value_match", "descriptor_value": "fake_value", - "headers_present": false, + "expect_match": false, "headers": [ { "name": "x-header-name", @@ -587,7 +587,7 @@ TEST_F(RateLimitPolicyEntryTest, HeaderValueMatchHeadersPresent) { { "type": "header_value_match", "descriptor_value": "fake_value", - "headers_present": false, + "expect_match": false, "headers": [ { "name": "x-header-name", From cf204e0c68dd8238c8fec2a93242a1247bdde637 Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 20 Apr 2017 17:57:28 -0700 Subject: [PATCH 5/6] update documentation --- docs/configuration/http_conn_man/route_config/rate_limits.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/configuration/http_conn_man/route_config/rate_limits.rst b/docs/configuration/http_conn_man/route_config/rate_limits.rst index 4071b7fd9c302..565525010c269 100644 --- a/docs/configuration/http_conn_man/route_config/rate_limits.rst +++ b/docs/configuration/http_conn_man/route_config/rate_limits.rst @@ -157,7 +157,7 @@ Header Value Match { "type": "header_value_match", "descriptor_value" : "...", - "headers_present" : "...", + "expect_match" : "...", "headers" : [] } From 24b9ec873abe36a1150afe8398099d74cef2da71 Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Fri, 21 Apr 2017 13:28:09 -0700 Subject: [PATCH 6/6] Address Jose's pr comments --- .../http_conn_man/route_config/rate_limits.rst | 9 ++++----- test/common/router/router_ratelimit_test.cc | 4 ++-- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/docs/configuration/http_conn_man/route_config/rate_limits.rst b/docs/configuration/http_conn_man/route_config/rate_limits.rst index 565525010c269..936b312d97e03 100644 --- a/docs/configuration/http_conn_man/route_config/rate_limits.rst +++ b/docs/configuration/http_conn_man/route_config/rate_limits.rst @@ -166,11 +166,10 @@ descriptor_value *(required, string)* The value to use in the descriptor entry. expect_match - *(optional, boolean)* Specifies if the action should append a descriptor when it expects a match - to the :ref:`headers`. If set to true, - the action will append a descriptor entry when the request matches the headers. If set to false, - the action will append a descriptor entry when the request does not match the headers. The default - value is true. + *(optional, boolean)* If set to true, the action will append a descriptor entry when the request + matches the :ref:`headers`. If set to false, + the action will append a descriptor entry when the request does not match the + :ref:`headers`. The default value is true. :ref:`headers` *(required, array)* Specifies a set of headers that the rate limit action should match on. The diff --git a/test/common/router/router_ratelimit_test.cc b/test/common/router/router_ratelimit_test.cc index b04108564176b..34ee86250aa3f 100644 --- a/test/common/router/router_ratelimit_test.cc +++ b/test/common/router/router_ratelimit_test.cc @@ -546,7 +546,7 @@ TEST_F(RateLimitPolicyEntryTest, HeaderValueMatchNoMatch) { )EOF"; SetUpTest(json); - Http::TestHeaderMapImpl header{{"x-header-name", "fake_value"}}; + Http::TestHeaderMapImpl header{{"x-header-name", "not_same_value"}}; rate_limit_entry_->populateDescriptors(route_, descriptors_, "", header, ""); EXPECT_TRUE(descriptors_.empty()); @@ -573,7 +573,7 @@ TEST_F(RateLimitPolicyEntryTest, HeaderValueMatchHeadersNotPresent) { )EOF"; SetUpTest(json); - Http::TestHeaderMapImpl header{{"x-header-name", "fake_value"}}; + Http::TestHeaderMapImpl header{{"x-header-name", "not_same_value"}}; rate_limit_entry_->populateDescriptors(route_, descriptors_, "", header, ""); EXPECT_THAT(std::vector<::RateLimit::Descriptor>({{{{"header_match", "fake_value"}}}}),