From 1c7063daf625626c4c959327ea6da33fd8849c6f Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 22 Sep 2016 12:30:27 -0700 Subject: [PATCH 01/10] Squashed commit of the following: commit 764e66055a9dcbaed259b8758121bc46f636830b Author: Constance Caramanolis Date: Thu Sep 22 12:29:10 2016 -0700 Add docs commit 3322f38d6765868b779f27b3be03a9e1b914dd24 Author: Constance Caramanolis Date: Tue Sep 20 17:35:02 2016 -0700 First ratelimit commit --- .../http_conn_man/route_config/route.rst | 8 +- .../http_filters/rate_limit_filter.rst | 30 ++++- include/envoy/router/router.h | 5 + source/common/http/filter/ratelimit.cc | 19 ++- source/common/http/filter/ratelimit.h | 23 +++- source/common/router/config_impl.cc | 1 + source/common/router/config_impl.h | 4 + test/common/http/filter/ratelimit_test.cc | 109 +++++++++++++++++- test/mocks/router/mocks.h | 4 + 9 files changed, 194 insertions(+), 9 deletions(-) diff --git a/docs/configuration/http_conn_man/route_config/route.rst b/docs/configuration/http_conn_man/route_config/route.rst index 836e3e41f6c29..6ea9290ea9fc2 100644 --- a/docs/configuration/http_conn_man/route_config/route.rst +++ b/docs/configuration/http_conn_man/route_config/route.rst @@ -153,7 +153,8 @@ Global rate limit :ref:`architecture overview `. .. code-block:: json { - "global": "..." + "global": "...", + "rate_limit_key": "..." } global @@ -161,6 +162,11 @@ global request that matches this route. This information is used by the :ref:`rate limit filter ` if it is installed. Defaults to false if not specified. +rate_limit_key + *(optional, string)* Specifies a descriptor value to be used for request header rate limiting. + This information is used by :ref:`rate limit filter + ` if it is installed. + .. _config_http_conn_man_route_table_route_shadow: Shadow diff --git a/docs/configuration/http_filters/rate_limit_filter.rst b/docs/configuration/http_filters/rate_limit_filter.rst index c8fe975a36df7..a64f9f65cf823 100644 --- a/docs/configuration/http_filters/rate_limit_filter.rst +++ b/docs/configuration/http_filters/rate_limit_filter.rst @@ -40,8 +40,8 @@ Actions } type - *(required, string) The type of rate limit action to perform. The currently supported action - type is *service_to_service*. + *(required, string)* The type of rate limit action to perform. The currently supported action + types are *service_to_service* and *request_headers*. Service to service ^^^^^^^^^^^^^^^^^^ @@ -60,6 +60,32 @@ The following descriptors are sent: is derived from the :option:`--service-cluster` option. +Request Headers +^^^^^^^^^^^^^^^ + +.. code-block:: json + + { + "type": "request_headers", + "header_name": "...", + "descriptor_key" : "..." + } + +header_name + *(required, string)* The header name to be queried from the request header and used to + populate the descriptor value for the *descriptor_key*. + +descriptor_key + *(required, string)* The key to use in the descriptor. + +The following descriptor is sent when a header contains a key that matches the *header_name*: + + * ("", "") + +If *rate_limit_key* is set in the :ref:`route `, the following descriptor is sent: + + * ("rate_limit", ""), ("", "") + Statistics ---------- diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 7bda1768e22d7..505406f35e4f5 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -90,6 +90,11 @@ class RateLimitPolicy { * @return whether the global rate limiting service should be called for the owning route. */ virtual bool doGlobalLimiting() const PURE; + + /** + * @return the rate limit key for a service, if it exists. + */ + virtual const std::string& rateLimitKey() const PURE; }; /** diff --git a/source/common/http/filter/ratelimit.cc b/source/common/http/filter/ratelimit.cc index 70fb1de03a223..7a06c2b2c13eb 100644 --- a/source/common/http/filter/ratelimit.cc +++ b/source/common/http/filter/ratelimit.cc @@ -16,7 +16,7 @@ const Http::HeaderMapImpl Filter::TOO_MANY_REQUESTS_HEADER{ void ServiceToServiceAction::populateDescriptors(const Router::RouteEntry& route, std::vector<::RateLimit::Descriptor>& descriptors, - FilterConfig& config) { + FilterConfig& config, const HeaderMap&) { // We limit on 2 dimensions. // 1) All calls to the given cluster. // 2) Calls to the given cluster and from this cluster. @@ -26,6 +26,19 @@ void ServiceToServiceAction::populateDescriptors(const Router::RouteEntry& route {{{"to_cluster", route.clusterName()}, {"from_cluster", config.localServiceCluster()}}}); } +void RequestHeadersAction::populateDescriptors(const Router::RouteEntry& route, + std::vector<::RateLimit::Descriptor>& descriptors, + FilterConfig&, const HeaderMap& headers) { + if (headers.has(header_name_)) { + if (!route.rateLimitPolicy().rateLimitKey().empty()) { + descriptors.push_back({{{"rate_limit_key", route.rateLimitPolicy().rateLimitKey()}, + {descriptor_key_, headers.get(header_name_)}}}); + } else { + descriptors.push_back({{{descriptor_key_, headers.get(header_name_)}}}); + } + } +} + FilterConfig::FilterConfig(const Json::Object& config, const std::string& local_service_cluster, Stats::Store& stats_store, Runtime::Loader& runtime) : domain_(config.getString("domain")), local_service_cluster_(local_service_cluster), @@ -34,6 +47,8 @@ FilterConfig::FilterConfig(const Json::Object& config, const std::string& local_ std::string type = action.getString("type"); if (type == "service_to_service") { actions_.emplace_back(new ServiceToServiceAction()); + } else if (type == "request_headers") { + actions_.emplace_back(new RequestHeadersAction(action)); } else { throw EnvoyException(fmt::format("unknown http rate limit filter action '{}'", type)); } @@ -49,7 +64,7 @@ FilterHeadersStatus Filter::decodeHeaders(HeaderMap& headers, bool) { if (route && route->rateLimitPolicy().doGlobalLimiting()) { std::vector<::RateLimit::Descriptor> descriptors; for (const ActionPtr& action : config_->actions()) { - action->populateDescriptors(*route, descriptors, *config_); + action->populateDescriptors(*route, descriptors, *config_, headers); } if (!descriptors.empty()) { diff --git a/source/common/http/filter/ratelimit.h b/source/common/http/filter/ratelimit.h index 5ae78cea4e6eb..ba0297a0b7973 100644 --- a/source/common/http/filter/ratelimit.h +++ b/source/common/http/filter/ratelimit.h @@ -27,7 +27,7 @@ class Action { */ virtual void populateDescriptors(const Router::RouteEntry& route, std::vector<::RateLimit::Descriptor>& descriptors, - FilterConfig& config) PURE; + FilterConfig& config, const HeaderMap& headers) PURE; }; typedef std::unique_ptr ActionPtr; @@ -39,10 +39,27 @@ class ServiceToServiceAction : public Action { public: // Action void populateDescriptors(const Router::RouteEntry& route, - std::vector<::RateLimit::Descriptor>& descriptors, - FilterConfig& config) override; + std::vector<::RateLimit::Descriptor>& descriptors, FilterConfig& config, + const HeaderMap&) override; }; +/** + * Action for request headers rate limiting. + */ +class RequestHeadersAction : public Action { +public: + RequestHeadersAction(const Json::Object& action) + : header_name_(action.getString("header_name")), + descriptor_key_(action.getString("descriptor_key")) {} + // Action + void populateDescriptors(const Router::RouteEntry& route, + std::vector<::RateLimit::Descriptor>& descriptors, FilterConfig& config, + const HeaderMap& headers) override; + +private: + const LowerCaseString header_name_; + const std::string descriptor_key_; +}; /** * Global configuration for the HTTP rate limit filter. */ diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index a3fd0b5eedfeb..f05213330158d 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -34,6 +34,7 @@ RateLimitPolicyImpl::RateLimitPolicyImpl(const Json::Object& config) { } do_global_limiting_ = config.getObject("rate_limit").getBoolean("global", false); + rate_limit_key_ = config.getObject("rate_limit").getString("rate_limit_key", ""); } ShadowPolicyImpl::ShadowPolicyImpl(const Json::Object& config) { diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index dc02bb309b03d..8a49b0c861988 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -127,8 +127,12 @@ class RateLimitPolicyImpl : public RateLimitPolicy { // Router::RateLimitPolicy bool doGlobalLimiting() const override { return do_global_limiting_; } + // Router::RateLimitPolicy + const std::string& rateLimitKey() const override { return rate_limit_key_; } + private: bool do_global_limiting_{}; + std::string rate_limit_key_; }; /** diff --git a/test/common/http/filter/ratelimit_test.cc b/test/common/http/filter/ratelimit_test.cc index 41f16803e1667..b5193f101a138 100644 --- a/test/common/http/filter/ratelimit_test.cc +++ b/test/common/http/filter/ratelimit_test.cc @@ -16,7 +16,7 @@ using testing::WithArgs; namespace Http { namespace RateLimit { -TEST(HttpRateLimitFilterBadConfigTest, All) { +TEST(HttpRateLimitFilterBadConfigTest, BadType) { std::string json = R"EOF( { "domain": "foo", @@ -32,6 +32,24 @@ TEST(HttpRateLimitFilterBadConfigTest, All) { EXPECT_THROW(FilterConfig(config, "service_cluster", stats_store, runtime), EnvoyException); } +TEST(HttpRateLimitFilterBadConfigTest, NoDescriptorKey) { + std::string json = R"EOF( + { + "domain": "foo", + "actions": [ + {"type": "request_headers", + "header_name" : "test" + } + ] + } + )EOF"; + + Json::StringLoader config(json); + Stats::IsolatedStoreImpl stats_store; + NiceMock runtime; + EXPECT_THROW(FilterConfig(config, "service_cluster", stats_store, runtime), EnvoyException); +} + class HttpRateLimitFilterTest : public testing::Test { public: HttpRateLimitFilterTest() { @@ -215,5 +233,94 @@ TEST_F(HttpRateLimitFilterTest, ResetDuringCall) { filter_callbacks_.reset_callback_(); } +class HttpRateLimitFilterRequestHeadersTest : public testing::Test { +public: + HttpRateLimitFilterRequestHeadersTest() { + std::string json = R"EOF( + { + "domain": "foobar", + "actions": [ + { + "type": "request_headers", + "header_name": "x-header-name", + "descriptor_key" : "my_header_name" + } + ] + } + )EOF"; + + ON_CALL(runtime_.snapshot_, featureEnabled("ratelimit.http_filter_enabled", 100)) + .WillByDefault(Return(true)); + ON_CALL(runtime_.snapshot_, featureEnabled("ratelimit.http_filter_enforcing", 100)) + .WillByDefault(Return(true)); + + Json::StringLoader config(json); + config_.reset(new FilterConfig(config, "service_cluster", stats_store_, runtime_)); + + client_ = new ::RateLimit::MockClient(); + filter_.reset(new Filter(config_, ::RateLimit::ClientPtr{client_})); + filter_->setDecoderFilterCallbacks(filter_callbacks_); + } + + FilterConfigPtr config_; + ::RateLimit::MockClient* client_; + std::unique_ptr filter_; + NiceMock filter_callbacks_; + ::RateLimit::RequestCallbacks* request_callbacks_{}; + HeaderMapImpl request_headers_{{"x-header-name", "test_value"}}; + Buffer::OwnedImpl data_; + Stats::IsolatedStoreImpl stats_store_; + NiceMock runtime_; +}; + +TEST_F(HttpRateLimitFilterRequestHeadersTest, OkResponse) { + filter_callbacks_.route_table_.route_entry_.rate_limit_policy_.do_global_limiting_ = true; + + EXPECT_CALL(*client_, + limit(_, "foobar", testing::ContainerEq(std::vector<::RateLimit::Descriptor>{ + {{{"my_header_name", "test_value"}}}}))) + .WillOnce(WithArgs<0>(Invoke([&](::RateLimit::RequestCallbacks& callbacks) + -> void { request_callbacks_ = &callbacks; }))); + + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); + EXPECT_EQ(FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(data_, false)); + EXPECT_EQ(FilterTrailersStatus::StopIteration, filter_->decodeTrailers(request_headers_)); + + EXPECT_CALL(filter_callbacks_, continueDecoding()); + request_callbacks_->complete(::RateLimit::LimitStatus::OK); + + EXPECT_EQ(1U, stats_store_.counter("cluster.fake_cluster.ratelimit.ok").value()); +} + +TEST_F(HttpRateLimitFilterRequestHeadersTest, RateLimitKeyOkResponse) { + filter_callbacks_.route_table_.route_entry_.rate_limit_policy_.do_global_limiting_ = true; + filter_callbacks_.route_table_.route_entry_.rate_limit_policy_.rate_limit_key_ = "test_key"; + + EXPECT_CALL(*client_, + limit(_, "foobar", + testing::ContainerEq(std::vector<::RateLimit::Descriptor>{ + {{{"rate_limit_key", "test_key"}, {"my_header_name", "test_value"}}}}))) + .WillOnce(WithArgs<0>(Invoke([&](::RateLimit::RequestCallbacks& callbacks) + -> void { request_callbacks_ = &callbacks; }))); + + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); + EXPECT_EQ(FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(data_, false)); + EXPECT_EQ(FilterTrailersStatus::StopIteration, filter_->decodeTrailers(request_headers_)); + + EXPECT_CALL(filter_callbacks_, continueDecoding()); + request_callbacks_->complete(::RateLimit::LimitStatus::OK); + + EXPECT_EQ(1U, stats_store_.counter("cluster.fake_cluster.ratelimit.ok").value()); +} + +TEST_F(HttpRateLimitFilterRequestHeadersTest, NoRateLimitHeaderMatch) { + filter_callbacks_.route_table_.route_entry_.rate_limit_policy_.do_global_limiting_ = true; + HeaderMapImpl empty_request_header; + EXPECT_CALL(*client_, limit(_, _, _)).Times(0); + EXPECT_EQ(FilterHeadersStatus::Continue, filter_->decodeHeaders(empty_request_header, false)); + EXPECT_EQ(FilterDataStatus::Continue, filter_->decodeData(data_, false)); + EXPECT_EQ(FilterTrailersStatus::Continue, filter_->decodeTrailers(empty_request_header)); +} + } // RateLimit } // Http diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 84e233ce35d8a..e31360cf8c849 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -44,7 +44,11 @@ class TestRateLimitPolicy : public RateLimitPolicy { // Router::RateLimitPolicy bool doGlobalLimiting() const override { return do_global_limiting_; } + // Router::RateLimitPolicy + const std::string& rateLimitKey() const override { return rate_limit_key_; } + bool do_global_limiting_{}; + std::string rate_limit_key_; }; class TestShadowPolicy : public ShadowPolicy { From 8567fdfe963341aedb82a78ad89e74a7e33d119b Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 22 Sep 2016 15:33:51 -0700 Subject: [PATCH 02/10] Address prr comments --- source/common/http/filter/ratelimit.cc | 16 ++-- source/common/router/config_impl.cc | 9 -- source/common/router/config_impl.h | 8 +- test/common/http/filter/ratelimit_test.cc | 112 +++++++++++----------- 4 files changed, 68 insertions(+), 77 deletions(-) diff --git a/source/common/http/filter/ratelimit.cc b/source/common/http/filter/ratelimit.cc index 7a06c2b2c13eb..57d1cf4b87959 100644 --- a/source/common/http/filter/ratelimit.cc +++ b/source/common/http/filter/ratelimit.cc @@ -29,13 +29,15 @@ void ServiceToServiceAction::populateDescriptors(const Router::RouteEntry& route void RequestHeadersAction::populateDescriptors(const Router::RouteEntry& route, std::vector<::RateLimit::Descriptor>& descriptors, FilterConfig&, const HeaderMap& headers) { - if (headers.has(header_name_)) { - if (!route.rateLimitPolicy().rateLimitKey().empty()) { - descriptors.push_back({{{"rate_limit_key", route.rateLimitPolicy().rateLimitKey()}, - {descriptor_key_, headers.get(header_name_)}}}); - } else { - descriptors.push_back({{{descriptor_key_, headers.get(header_name_)}}}); - } + std::string header_value = headers.get(header_name_); + if (header_value.empty()) { + return; + } + + descriptors.push_back({{{descriptor_key_, header_value}}}); + if (!route.rateLimitPolicy().rateLimitKey().empty()) { + descriptors.push_back({{{"rate_limit_key", route.rateLimitPolicy().rateLimitKey()}, + {descriptor_key_, header_value}}}); } } diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index f05213330158d..57d50d28f7adc 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -28,15 +28,6 @@ RetryPolicyImpl::RetryPolicyImpl(const Json::Object& config) { retry_on_ = RetryStateImpl::parseRetryOn(config.getObject("retry_policy").getString("retry_on")); } -RateLimitPolicyImpl::RateLimitPolicyImpl(const Json::Object& config) { - if (!config.hasObject("rate_limit")) { - return; - } - - do_global_limiting_ = config.getObject("rate_limit").getBoolean("global", false); - rate_limit_key_ = config.getObject("rate_limit").getString("rate_limit_key", ""); -} - ShadowPolicyImpl::ShadowPolicyImpl(const Json::Object& config) { if (!config.hasObject("shadow")) { return; diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 8a49b0c861988..3b0c2f715d4f7 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -122,7 +122,9 @@ class RetryPolicyImpl : public RetryPolicy { */ class RateLimitPolicyImpl : public RateLimitPolicy { public: - RateLimitPolicyImpl(const Json::Object& config); + RateLimitPolicyImpl(const Json::Object& config) + : do_global_limiting_(config.getObject("rate_limit", true).getBoolean("global", false)), + rate_limit_key_(config.getObject("rate_limit", true).getString("rate_limit_key", "")) {} // Router::RateLimitPolicy bool doGlobalLimiting() const override { return do_global_limiting_; } @@ -131,8 +133,8 @@ class RateLimitPolicyImpl : public RateLimitPolicy { const std::string& rateLimitKey() const override { return rate_limit_key_; } private: - bool do_global_limiting_{}; - std::string rate_limit_key_; + const bool do_global_limiting_{}; + const std::string rate_limit_key_; }; /** diff --git a/test/common/http/filter/ratelimit_test.cc b/test/common/http/filter/ratelimit_test.cc index b5193f101a138..6a798e8bdb833 100644 --- a/test/common/http/filter/ratelimit_test.cc +++ b/test/common/http/filter/ratelimit_test.cc @@ -53,20 +53,13 @@ TEST(HttpRateLimitFilterBadConfigTest, NoDescriptorKey) { class HttpRateLimitFilterTest : public testing::Test { public: HttpRateLimitFilterTest() { - std::string json = R"EOF( - { - "domain": "foo", - "actions": [ - {"type": "service_to_service"} - ] - } - )EOF"; - ON_CALL(runtime_.snapshot_, featureEnabled("ratelimit.http_filter_enabled", 100)) .WillByDefault(Return(true)); ON_CALL(runtime_.snapshot_, featureEnabled("ratelimit.http_filter_enforcing", 100)) .WillByDefault(Return(true)); + } + void SetUpTest(const std::string json) { Json::StringLoader config(json); config_.reset(new FilterConfig(config, "service_cluster", stats_store_, runtime_)); @@ -75,6 +68,28 @@ class HttpRateLimitFilterTest : public testing::Test { filter_->setDecoderFilterCallbacks(filter_callbacks_); } + const std::string service_to_service_json = R"EOF( + { + "domain": "foo", + "actions": [ + {"type": "service_to_service"} + ] + } + )EOF"; + + const std::string request_headers_json = R"EOF( + { + "domain": "foobar", + "actions": [ + { + "type": "request_headers", + "header_name": "x-header-name", + "descriptor_key" : "my_header_name" + } + ] + } + )EOF"; + FilterConfigPtr config_; ::RateLimit::MockClient* client_; std::unique_ptr filter_; @@ -87,6 +102,8 @@ class HttpRateLimitFilterTest : public testing::Test { }; TEST_F(HttpRateLimitFilterTest, NoRoute) { + SetUpTest(service_to_service_json); + EXPECT_CALL(filter_callbacks_.route_table_, routeForRequest(_)).WillOnce(Return(nullptr)); EXPECT_EQ(FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); @@ -95,12 +112,16 @@ TEST_F(HttpRateLimitFilterTest, NoRoute) { } TEST_F(HttpRateLimitFilterTest, NoLimiting) { + SetUpTest(service_to_service_json); + EXPECT_EQ(FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); EXPECT_EQ(FilterDataStatus::Continue, filter_->decodeData(data_, false)); EXPECT_EQ(FilterTrailersStatus::Continue, filter_->decodeTrailers(request_headers_)); } TEST_F(HttpRateLimitFilterTest, RuntimeDisabled) { + SetUpTest(service_to_service_json); + EXPECT_CALL(runtime_.snapshot_, featureEnabled("ratelimit.http_filter_enabled", 100)) .WillOnce(Return(false)); EXPECT_EQ(FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); @@ -109,6 +130,7 @@ TEST_F(HttpRateLimitFilterTest, RuntimeDisabled) { } TEST_F(HttpRateLimitFilterTest, OkResponse) { + SetUpTest(service_to_service_json); InSequence s; filter_callbacks_.route_table_.route_entry_.rate_limit_policy_.do_global_limiting_ = true; @@ -132,6 +154,7 @@ TEST_F(HttpRateLimitFilterTest, OkResponse) { } TEST_F(HttpRateLimitFilterTest, ImmediateOkResponse) { + SetUpTest(service_to_service_json); InSequence s; filter_callbacks_.route_table_.route_entry_.rate_limit_policy_.do_global_limiting_ = true; @@ -154,6 +177,7 @@ TEST_F(HttpRateLimitFilterTest, ImmediateOkResponse) { } TEST_F(HttpRateLimitFilterTest, ErrorResponse) { + SetUpTest(service_to_service_json); InSequence s; filter_callbacks_.route_table_.route_entry_.rate_limit_policy_.do_global_limiting_ = true; @@ -174,6 +198,7 @@ TEST_F(HttpRateLimitFilterTest, ErrorResponse) { } TEST_F(HttpRateLimitFilterTest, LimitResponse) { + SetUpTest(service_to_service_json); InSequence s; filter_callbacks_.route_table_.route_entry_.rate_limit_policy_.do_global_limiting_ = true; @@ -195,6 +220,7 @@ TEST_F(HttpRateLimitFilterTest, LimitResponse) { } TEST_F(HttpRateLimitFilterTest, LimitResponseRuntimeDisabled) { + SetUpTest(service_to_service_json); InSequence s; filter_callbacks_.route_table_.route_entry_.rate_limit_policy_.do_global_limiting_ = true; @@ -219,6 +245,7 @@ TEST_F(HttpRateLimitFilterTest, LimitResponseRuntimeDisabled) { } TEST_F(HttpRateLimitFilterTest, ResetDuringCall) { + SetUpTest(service_to_service_json); InSequence s; filter_callbacks_.route_table_.route_entry_.rate_limit_policy_.do_global_limiting_ = true; @@ -233,47 +260,9 @@ TEST_F(HttpRateLimitFilterTest, ResetDuringCall) { filter_callbacks_.reset_callback_(); } -class HttpRateLimitFilterRequestHeadersTest : public testing::Test { -public: - HttpRateLimitFilterRequestHeadersTest() { - std::string json = R"EOF( - { - "domain": "foobar", - "actions": [ - { - "type": "request_headers", - "header_name": "x-header-name", - "descriptor_key" : "my_header_name" - } - ] - } - )EOF"; - - ON_CALL(runtime_.snapshot_, featureEnabled("ratelimit.http_filter_enabled", 100)) - .WillByDefault(Return(true)); - ON_CALL(runtime_.snapshot_, featureEnabled("ratelimit.http_filter_enforcing", 100)) - .WillByDefault(Return(true)); - - Json::StringLoader config(json); - config_.reset(new FilterConfig(config, "service_cluster", stats_store_, runtime_)); - - client_ = new ::RateLimit::MockClient(); - filter_.reset(new Filter(config_, ::RateLimit::ClientPtr{client_})); - filter_->setDecoderFilterCallbacks(filter_callbacks_); - } - - FilterConfigPtr config_; - ::RateLimit::MockClient* client_; - std::unique_ptr filter_; - NiceMock filter_callbacks_; - ::RateLimit::RequestCallbacks* request_callbacks_{}; - HeaderMapImpl request_headers_{{"x-header-name", "test_value"}}; - Buffer::OwnedImpl data_; - Stats::IsolatedStoreImpl stats_store_; - NiceMock runtime_; -}; +TEST_F(HttpRateLimitFilterTest, RequestHeaderOkResponse) { + SetUpTest(request_headers_json); -TEST_F(HttpRateLimitFilterRequestHeadersTest, OkResponse) { filter_callbacks_.route_table_.route_entry_.rate_limit_policy_.do_global_limiting_ = true; EXPECT_CALL(*client_, @@ -282,9 +271,10 @@ TEST_F(HttpRateLimitFilterRequestHeadersTest, OkResponse) { .WillOnce(WithArgs<0>(Invoke([&](::RateLimit::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); - EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); + HeaderMapImpl request_header{{"x-header-name", "test_value"}}; + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_header, false)); EXPECT_EQ(FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(data_, false)); - EXPECT_EQ(FilterTrailersStatus::StopIteration, filter_->decodeTrailers(request_headers_)); + EXPECT_EQ(FilterTrailersStatus::StopIteration, filter_->decodeTrailers(request_header)); EXPECT_CALL(filter_callbacks_, continueDecoding()); request_callbacks_->complete(::RateLimit::LimitStatus::OK); @@ -292,20 +282,24 @@ TEST_F(HttpRateLimitFilterRequestHeadersTest, OkResponse) { EXPECT_EQ(1U, stats_store_.counter("cluster.fake_cluster.ratelimit.ok").value()); } -TEST_F(HttpRateLimitFilterRequestHeadersTest, RateLimitKeyOkResponse) { +TEST_F(HttpRateLimitFilterTest, RateLimitKeyOkResponse) { + SetUpTest(request_headers_json); + filter_callbacks_.route_table_.route_entry_.rate_limit_policy_.do_global_limiting_ = true; filter_callbacks_.route_table_.route_entry_.rate_limit_policy_.rate_limit_key_ = "test_key"; EXPECT_CALL(*client_, limit(_, "foobar", testing::ContainerEq(std::vector<::RateLimit::Descriptor>{ + {{{"my_header_name", "test_value"}}}, {{{"rate_limit_key", "test_key"}, {"my_header_name", "test_value"}}}}))) .WillOnce(WithArgs<0>(Invoke([&](::RateLimit::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); - EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); + HeaderMapImpl request_header{{"x-header-name", "test_value"}}; + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_header, false)); EXPECT_EQ(FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(data_, false)); - EXPECT_EQ(FilterTrailersStatus::StopIteration, filter_->decodeTrailers(request_headers_)); + EXPECT_EQ(FilterTrailersStatus::StopIteration, filter_->decodeTrailers(request_header)); EXPECT_CALL(filter_callbacks_, continueDecoding()); request_callbacks_->complete(::RateLimit::LimitStatus::OK); @@ -313,13 +307,15 @@ TEST_F(HttpRateLimitFilterRequestHeadersTest, RateLimitKeyOkResponse) { EXPECT_EQ(1U, stats_store_.counter("cluster.fake_cluster.ratelimit.ok").value()); } -TEST_F(HttpRateLimitFilterRequestHeadersTest, NoRateLimitHeaderMatch) { +TEST_F(HttpRateLimitFilterTest, NoRateLimitHeaderMatch) { + SetUpTest(request_headers_json); filter_callbacks_.route_table_.route_entry_.rate_limit_policy_.do_global_limiting_ = true; - HeaderMapImpl empty_request_header; + EXPECT_CALL(*client_, limit(_, _, _)).Times(0); - EXPECT_EQ(FilterHeadersStatus::Continue, filter_->decodeHeaders(empty_request_header, false)); + + EXPECT_EQ(FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); EXPECT_EQ(FilterDataStatus::Continue, filter_->decodeData(data_, false)); - EXPECT_EQ(FilterTrailersStatus::Continue, filter_->decodeTrailers(empty_request_header)); + EXPECT_EQ(FilterTrailersStatus::Continue, filter_->decodeTrailers(request_headers_)); } } // RateLimit From 29076dfecdcb3826345f6e6749a16b4f0a73b297 Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 22 Sep 2016 16:02:56 -0700 Subject: [PATCH 03/10] rebase master --- source/common/http/async_client_impl.h | 1 + 1 file changed, 1 insertion(+) diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 93edfddc25e17..6db372f2bb9ab 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -61,6 +61,7 @@ class AsyncRequestImpl final : public AsyncClient::Request, struct NullRateLimitPolicy : public Router::RateLimitPolicy { // Router::RateLimitPolicy bool doGlobalLimiting() const override { return false; } + const std::string& rateLimitKey() const override { return EMPTY_STRING; } }; struct NullRetryPolicy : public Router::RetryPolicy { From ee452282b1cbe3b454d6ff74b57c4f803244c3d3 Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 22 Sep 2016 16:29:53 -0700 Subject: [PATCH 04/10] rename to route_key --- .../http_conn_man/route_config/route.rst | 6 +++--- include/envoy/router/router.h | 4 ++-- source/common/http/async_client_impl.h | 2 +- source/common/http/filter/ratelimit.cc | 9 ++++++--- source/common/router/config_impl.h | 8 ++++---- test/common/http/filter/ratelimit_test.cc | 12 ++++++------ test/mocks/router/mocks.h | 4 ++-- 7 files changed, 24 insertions(+), 21 deletions(-) diff --git a/docs/configuration/http_conn_man/route_config/route.rst b/docs/configuration/http_conn_man/route_config/route.rst index 6ea9290ea9fc2..fba788117b153 100644 --- a/docs/configuration/http_conn_man/route_config/route.rst +++ b/docs/configuration/http_conn_man/route_config/route.rst @@ -154,7 +154,7 @@ Global rate limit :ref:`architecture overview `. { "global": "...", - "rate_limit_key": "..." + "route_key": "..." } global @@ -162,8 +162,8 @@ global request that matches this route. This information is used by the :ref:`rate limit filter ` if it is installed. Defaults to false if not specified. -rate_limit_key - *(optional, string)* Specifies a descriptor value to be used for request header rate limiting. +route_key + *(optional, string)* Specifies a descriptor value to be used when rate limiting for a route. This information is used by :ref:`rate limit filter ` if it is installed. diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 505406f35e4f5..067682a928f90 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -92,9 +92,9 @@ class RateLimitPolicy { virtual bool doGlobalLimiting() const PURE; /** - * @return the rate limit key for a service, if it exists. + * @return the rate limit key for a service, if it exists. */ - virtual const std::string& rateLimitKey() const PURE; + virtual const std::string& routeKey() const PURE; }; /** diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 6db372f2bb9ab..bb9d5ff247c79 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -61,7 +61,7 @@ class AsyncRequestImpl final : public AsyncClient::Request, struct NullRateLimitPolicy : public Router::RateLimitPolicy { // Router::RateLimitPolicy bool doGlobalLimiting() const override { return false; } - const std::string& rateLimitKey() const override { return EMPTY_STRING; } + const std::string& routeKey() const override { return EMPTY_STRING; } }; struct NullRetryPolicy : public Router::RetryPolicy { diff --git a/source/common/http/filter/ratelimit.cc b/source/common/http/filter/ratelimit.cc index 57d1cf4b87959..d1b12399c7192 100644 --- a/source/common/http/filter/ratelimit.cc +++ b/source/common/http/filter/ratelimit.cc @@ -35,10 +35,13 @@ void RequestHeadersAction::populateDescriptors(const Router::RouteEntry& route, } descriptors.push_back({{{descriptor_key_, header_value}}}); - if (!route.rateLimitPolicy().rateLimitKey().empty()) { - descriptors.push_back({{{"rate_limit_key", route.rateLimitPolicy().rateLimitKey()}, - {descriptor_key_, header_value}}}); + + std::string route_key = route.rateLimitPolicy().routeKey(); + if (route_key.empty()) { + return; } + + descriptors.push_back({{{"route_key", route_key}, {descriptor_key_, header_value}}}); } FilterConfig::FilterConfig(const Json::Object& config, const std::string& local_service_cluster, diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 3b0c2f715d4f7..5957468adeb06 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -124,17 +124,17 @@ class RateLimitPolicyImpl : public RateLimitPolicy { public: RateLimitPolicyImpl(const Json::Object& config) : do_global_limiting_(config.getObject("rate_limit", true).getBoolean("global", false)), - rate_limit_key_(config.getObject("rate_limit", true).getString("rate_limit_key", "")) {} + route_key_(config.getObject("rate_limit", true).getString("route_key", "")) {} // Router::RateLimitPolicy bool doGlobalLimiting() const override { return do_global_limiting_; } // Router::RateLimitPolicy - const std::string& rateLimitKey() const override { return rate_limit_key_; } + const std::string& routeKey() const override { return route_key_; } private: - const bool do_global_limiting_{}; - const std::string rate_limit_key_; + const bool do_global_limiting_; + const std::string route_key_; }; /** diff --git a/test/common/http/filter/ratelimit_test.cc b/test/common/http/filter/ratelimit_test.cc index 6a798e8bdb833..e71e4fc388706 100644 --- a/test/common/http/filter/ratelimit_test.cc +++ b/test/common/http/filter/ratelimit_test.cc @@ -286,13 +286,13 @@ TEST_F(HttpRateLimitFilterTest, RateLimitKeyOkResponse) { SetUpTest(request_headers_json); filter_callbacks_.route_table_.route_entry_.rate_limit_policy_.do_global_limiting_ = true; - filter_callbacks_.route_table_.route_entry_.rate_limit_policy_.rate_limit_key_ = "test_key"; + filter_callbacks_.route_table_.route_entry_.rate_limit_policy_.route_key_ = "test_key"; - EXPECT_CALL(*client_, - limit(_, "foobar", - testing::ContainerEq(std::vector<::RateLimit::Descriptor>{ - {{{"my_header_name", "test_value"}}}, - {{{"rate_limit_key", "test_key"}, {"my_header_name", "test_value"}}}}))) + EXPECT_CALL( + *client_, + limit(_, "foobar", testing::ContainerEq(std::vector<::RateLimit::Descriptor>{ + {{{"my_header_name", "test_value"}}}, + {{{"route_key", "test_key"}, {"my_header_name", "test_value"}}}}))) .WillOnce(WithArgs<0>(Invoke([&](::RateLimit::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; }))); diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index e31360cf8c849..57060a0d539da 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -45,10 +45,10 @@ class TestRateLimitPolicy : public RateLimitPolicy { bool doGlobalLimiting() const override { return do_global_limiting_; } // Router::RateLimitPolicy - const std::string& rateLimitKey() const override { return rate_limit_key_; } + const std::string& routeKey() const override { return route_key_; } bool do_global_limiting_{}; - std::string rate_limit_key_; + std::string route_key_; }; class TestShadowPolicy : public ShadowPolicy { From 2b39fc69f805f5eb0e5bd2f5e3e02da4ae1ef21d Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 22 Sep 2016 16:31:28 -0700 Subject: [PATCH 05/10] update docs --- docs/configuration/http_filters/rate_limit_filter.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/configuration/http_filters/rate_limit_filter.rst b/docs/configuration/http_filters/rate_limit_filter.rst index a64f9f65cf823..734725d769037 100644 --- a/docs/configuration/http_filters/rate_limit_filter.rst +++ b/docs/configuration/http_filters/rate_limit_filter.rst @@ -72,7 +72,7 @@ Request Headers } header_name - *(required, string)* The header name to be queried from the request header and used to + *(required, string)* The header name to be queried from the request headers and used to populate the descriptor value for the *descriptor_key*. descriptor_key @@ -84,7 +84,7 @@ The following descriptor is sent when a header contains a key that matches the * If *rate_limit_key* is set in the :ref:`route `, the following descriptor is sent: - * ("rate_limit", ""), ("", "") + * ("route_key", ""), ("", "") Statistics ---------- From c286768cfa76b6683c9aa4b2b7a6e7f2b99b9244 Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 22 Sep 2016 16:32:24 -0700 Subject: [PATCH 06/10] update name --- docs/configuration/http_filters/rate_limit_filter.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/configuration/http_filters/rate_limit_filter.rst b/docs/configuration/http_filters/rate_limit_filter.rst index 734725d769037..67e3fb22c0576 100644 --- a/docs/configuration/http_filters/rate_limit_filter.rst +++ b/docs/configuration/http_filters/rate_limit_filter.rst @@ -82,7 +82,7 @@ The following descriptor is sent when a header contains a key that matches the * * ("", "") -If *rate_limit_key* is set in the :ref:`route `, the following descriptor is sent: +If *route_key* is set in the :ref:`route `, the following descriptor is sent: * ("route_key", ""), ("", "") From 2df67c766b04470cc8967eee482a8545c0728cc2 Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 22 Sep 2016 16:33:21 -0700 Subject: [PATCH 07/10] update comment --- docs/configuration/http_filters/rate_limit_filter.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/configuration/http_filters/rate_limit_filter.rst b/docs/configuration/http_filters/rate_limit_filter.rst index 67e3fb22c0576..86e80fdfb7b24 100644 --- a/docs/configuration/http_filters/rate_limit_filter.rst +++ b/docs/configuration/http_filters/rate_limit_filter.rst @@ -82,7 +82,8 @@ The following descriptor is sent when a header contains a key that matches the * * ("", "") -If *route_key* is set in the :ref:`route `, the following descriptor is sent: +If *route_key* is set in the :ref:`route `, the following +descriptor is sent as well: * ("route_key", ""), ("", "") From d83a754241e30d90165d14685d723c8c95e08a39 Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 22 Sep 2016 16:49:43 -0700 Subject: [PATCH 08/10] Spacing --- include/envoy/router/router.h | 2 +- source/common/http/filter/ratelimit.cc | 2 +- test/common/http/filter/ratelimit_test.cc | 5 +++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 067682a928f90..a80c1ca614053 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -92,7 +92,7 @@ class RateLimitPolicy { virtual bool doGlobalLimiting() const PURE; /** - * @return the rate limit key for a service, if it exists. + * @return the route key for a service, if it exists. */ virtual const std::string& routeKey() const PURE; }; diff --git a/source/common/http/filter/ratelimit.cc b/source/common/http/filter/ratelimit.cc index d1b12399c7192..0204413edebcf 100644 --- a/source/common/http/filter/ratelimit.cc +++ b/source/common/http/filter/ratelimit.cc @@ -36,7 +36,7 @@ void RequestHeadersAction::populateDescriptors(const Router::RouteEntry& route, descriptors.push_back({{{descriptor_key_, header_value}}}); - std::string route_key = route.rateLimitPolicy().routeKey(); + const std::string& route_key = route.rateLimitPolicy().routeKey(); if (route_key.empty()) { return; } diff --git a/test/common/http/filter/ratelimit_test.cc b/test/common/http/filter/ratelimit_test.cc index e71e4fc388706..501394b4cc0e5 100644 --- a/test/common/http/filter/ratelimit_test.cc +++ b/test/common/http/filter/ratelimit_test.cc @@ -37,8 +37,9 @@ TEST(HttpRateLimitFilterBadConfigTest, NoDescriptorKey) { { "domain": "foo", "actions": [ - {"type": "request_headers", - "header_name" : "test" + { + "type": "request_headers", + "header_name" : "test" } ] } From 24cadb04fd6c3471824e306fd48cf80e7f0e458d Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 22 Sep 2016 16:51:44 -0700 Subject: [PATCH 09/10] clarify comment --- include/envoy/router/router.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index a80c1ca614053..cd4358088b735 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -92,7 +92,7 @@ class RateLimitPolicy { virtual bool doGlobalLimiting() const PURE; /** - * @return the route key for a service, if it exists. + * @return the route key, if it exists. */ virtual const std::string& routeKey() const PURE; }; From f2216f181de50ecdd341b40d5c4f9f782f9034ef Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Thu, 22 Sep 2016 16:56:49 -0700 Subject: [PATCH 10/10] Update route.rst --- docs/configuration/http_conn_man/route_config/route.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/configuration/http_conn_man/route_config/route.rst b/docs/configuration/http_conn_man/route_config/route.rst index fba788117b153..473a6d33e5ad4 100644 --- a/docs/configuration/http_conn_man/route_config/route.rst +++ b/docs/configuration/http_conn_man/route_config/route.rst @@ -164,7 +164,7 @@ global route_key *(optional, string)* Specifies a descriptor value to be used when rate limiting for a route. - This information is used by :ref:`rate limit filter + This information is used by the :ref:`rate limit filter ` if it is installed. .. _config_http_conn_man_route_table_route_shadow: