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 bee4c9eeab401..5729e2e844c86 100644 --- a/docs/configuration/http_conn_man/route_config/rate_limits.rst +++ b/docs/configuration/http_conn_man/route_config/rate_limits.rst @@ -24,8 +24,9 @@ disable_key actions *(required, array)* A list of actions that are to be applied for this rate limit configuration. - Order matters as the actions are processed sequentially and the descriptor will be composed by - appending decriptor entries in that sequence. See :ref:`composing actions + Order matters as the actions are processed sequentially and the descriptor is composed by + appending descriptor entries in that sequence. If an action cannot append a descriptor entry, + no descriptor is generated for the configuration. See :ref:`composing actions ` for additional documentation. .. _config_http_conn_man_route_table_rate_limit_actions: @@ -185,11 +186,15 @@ Each action populates a descriptor entry. A vector of descriptor entries compose create more complex rate limit descriptors, actions can be composed in any order. The descriptor will be populated in the order the actions are specified in the configuration. +Example 1 +^^^^^^^^^ + For example, to generate the following descriptor: .. code-block:: cpp - ("generic_key", "some_value"), ("source_cluster", "from_cluster") + ("generic_key", "some_value") + ("source_cluster", "from_cluster") The configuration would be: @@ -207,9 +212,13 @@ The configuration would be: ] } -If an action doesn't append a descriptor entry, the next item in the action list will -be processed. For example given the following rate limit configuration, a request can -generate a few possible descriptors depending on what is present in the request. +Example 2 +^^^^^^^^^ + +If an action doesn't append a descriptor entry, no descriptor is generated for +the configuration. + +For the following configuration: .. code-block:: json @@ -228,17 +237,14 @@ generate a few possible descriptors depending on what is present in the request. ] } -For a request with :ref:`x-forwarded-for` set and the -trusted address is for example *127.0.0.1*, the following descriptor would be generated: +If a request did not set :ref:`x-forwarded-for`, +no descriptor is generated. -.. code-block:: cpp - - ("generic_key", "some_value"), ("remote_address", "127.0.0.1"), ("source_cluster", - "from_cluster") - -If a request did not set :ref:`x-forwarded-for`, the -following descriptor would be generated: +If a request sets :ref:`x-forwarded-for`, the +the following descriptor is generated: .. code-block:: cpp - ("generic_key", "some_value"), ("source_cluster", "from_cluster") + ("generic_key", "some_value") + ("remote_address", "") + ("source_cluster", "from_cluster") diff --git a/include/envoy/router/router_ratelimit.h b/include/envoy/router/router_ratelimit.h index 32bb27a690e69..7ad5cd52fa815 100644 --- a/include/envoy/router/router_ratelimit.h +++ b/include/envoy/router/router_ratelimit.h @@ -19,8 +19,9 @@ class RateLimitAction { * @param local_service_cluster supplies the name of the local service cluster. * @param headers supplies the header for the request. * @param remote_address supplies the trusted downstream address for the connection. + * @return true if the RateLimitAction populated the descriptor. */ - virtual void populateDescriptor(const RouteEntry& route, ::RateLimit::Descriptor& descriptor, + virtual bool populateDescriptor(const RouteEntry& route, ::RateLimit::Descriptor& descriptor, const std::string& local_service_cluster, const Http::HeaderMap& headers, const std::string& remote_address) const PURE; diff --git a/source/common/router/router_ratelimit.cc b/source/common/router/router_ratelimit.cc index 3c1020c0587cf..c2863e433a08c 100644 --- a/source/common/router/router_ratelimit.cc +++ b/source/common/router/router_ratelimit.cc @@ -8,47 +8,52 @@ namespace Router { const uint64_t RateLimitPolicyImpl::MAX_STAGE_NUMBER = 10UL; -void SourceClusterAction::populateDescriptor(const Router::RouteEntry&, +bool SourceClusterAction::populateDescriptor(const Router::RouteEntry&, ::RateLimit::Descriptor& descriptor, const std::string& local_service_cluster, const Http::HeaderMap&, const std::string&) const { descriptor.entries_.push_back({"source_cluster", local_service_cluster}); + return true; } -void DestinationClusterAction::populateDescriptor(const Router::RouteEntry& route, +bool DestinationClusterAction::populateDescriptor(const Router::RouteEntry& route, ::RateLimit::Descriptor& descriptor, const std::string&, const Http::HeaderMap&, const std::string&) const { descriptor.entries_.push_back({"destination_cluster", route.clusterName()}); + return true; } -void RequestHeadersAction::populateDescriptor(const Router::RouteEntry&, +bool RequestHeadersAction::populateDescriptor(const Router::RouteEntry&, ::RateLimit::Descriptor& descriptor, const std::string&, const Http::HeaderMap& headers, const std::string&) const { const Http::HeaderEntry* header_value = headers.get(header_name_); if (!header_value) { - return; + return false; } descriptor.entries_.push_back({descriptor_key_, header_value->value().c_str()}); + return true; } -void RemoteAddressAction::populateDescriptor(const Router::RouteEntry&, +bool RemoteAddressAction::populateDescriptor(const Router::RouteEntry&, ::RateLimit::Descriptor& descriptor, const std::string&, const Http::HeaderMap&, const std::string& remote_address) const { if (remote_address.empty()) { - return; + return false; } descriptor.entries_.push_back({"remote_address", remote_address}); + return true; } -void GenericKeyAction::populateDescriptor(const Router::RouteEntry&, +bool GenericKeyAction::populateDescriptor(const Router::RouteEntry&, ::RateLimit::Descriptor& descriptor, const std::string&, const Http::HeaderMap&, const std::string&) const { descriptor.entries_.push_back({"generic_key", descriptor_value_}); + return true; } HeaderValueMatchAction::HeaderValueMatchAction(const Json::Object& action) @@ -59,12 +64,15 @@ HeaderValueMatchAction::HeaderValueMatchAction(const Json::Object& action) } } -void HeaderValueMatchAction::populateDescriptor(const Router::RouteEntry&, +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; } } @@ -85,9 +93,8 @@ RateLimitPolicyEntryImpl::RateLimitPolicyEntryImpl(const Json::Object& config) } else if (type == "generic_key") { actions_.emplace_back(new GenericKeyAction(*action)); } else if (type == "header_value_match") { + ASSERT(type == "header_value_match"); actions_.emplace_back(new HeaderValueMatchAction(*action)); - } else { - throw EnvoyException(fmt::format("unknown http rate limit filter action '{}'", type)); } } } @@ -97,11 +104,17 @@ void RateLimitPolicyEntryImpl::populateDescriptors( const std::string& local_service_cluster, const Http::HeaderMap& headers, const std::string& remote_address) const { ::RateLimit::Descriptor descriptor; + bool result = true; for (const RateLimitActionPtr& action : actions_) { - action->populateDescriptor(route, descriptor, local_service_cluster, headers, remote_address); + result = result && + action->populateDescriptor(route, descriptor, local_service_cluster, headers, + remote_address); + if (!result) { + break; + } } - if (!descriptor.entries_.empty()) { + if (result) { descriptors.emplace_back(descriptor); } } diff --git a/source/common/router/router_ratelimit.h b/source/common/router/router_ratelimit.h index ec3f0cb26e1f0..f2f17fb23e0ea 100644 --- a/source/common/router/router_ratelimit.h +++ b/source/common/router/router_ratelimit.h @@ -15,7 +15,7 @@ namespace Router { class SourceClusterAction : public RateLimitAction { public: // Router::RateLimitAction - void populateDescriptor(const Router::RouteEntry& route, ::RateLimit::Descriptor& descriptor, + bool populateDescriptor(const Router::RouteEntry& route, ::RateLimit::Descriptor& descriptor, const std::string& local_service_cluster, const Http::HeaderMap& headers, const std::string& remote_address) const override; }; @@ -26,7 +26,7 @@ class SourceClusterAction : public RateLimitAction { class DestinationClusterAction : public RateLimitAction { public: // Router::RateLimitAction - void populateDescriptor(const Router::RouteEntry& route, ::RateLimit::Descriptor& descriptor, + bool populateDescriptor(const Router::RouteEntry& route, ::RateLimit::Descriptor& descriptor, const std::string& local_service_cluster, const Http::HeaderMap& headers, const std::string& remote_address) const override; }; @@ -41,7 +41,7 @@ class RequestHeadersAction : public RateLimitAction { descriptor_key_(action.getString("descriptor_key")) {} // Router::RateLimitAction - void populateDescriptor(const Router::RouteEntry& route, ::RateLimit::Descriptor& descriptor, + bool populateDescriptor(const Router::RouteEntry& route, ::RateLimit::Descriptor& descriptor, const std::string& local_service_cluster, const Http::HeaderMap& headers, const std::string& remote_address) const override; @@ -56,7 +56,7 @@ class RequestHeadersAction : public RateLimitAction { class RemoteAddressAction : public RateLimitAction { public: // Router::RateLimitAction - void populateDescriptor(const Router::RouteEntry& route, ::RateLimit::Descriptor& descriptor, + bool populateDescriptor(const Router::RouteEntry& route, ::RateLimit::Descriptor& descriptor, const std::string& local_service_cluster, const Http::HeaderMap& headers, const std::string& remote_address) const override; }; @@ -70,7 +70,7 @@ class GenericKeyAction : public RateLimitAction { : descriptor_value_(action.getString("descriptor_value")) {} // Router::RateLimitAction - void populateDescriptor(const Router::RouteEntry& route, ::RateLimit::Descriptor& descriptor, + bool populateDescriptor(const Router::RouteEntry& route, ::RateLimit::Descriptor& descriptor, const std::string& local_service_cluster, const Http::HeaderMap& headers, const std::string& remote_address) const override; @@ -86,7 +86,7 @@ class HeaderValueMatchAction : public RateLimitAction { HeaderValueMatchAction(const Json::Object& action); // Router::RateLimitAction - void populateDescriptor(const Router::RouteEntry& route, ::RateLimit::Descriptor& descriptor, + bool populateDescriptor(const Router::RouteEntry& route, ::RateLimit::Descriptor& descriptor, const std::string& local_service_cluster, const Http::HeaderMap& headers, const std::string& remote_address) const override; diff --git a/test/common/router/router_ratelimit_test.cc b/test/common/router/router_ratelimit_test.cc index cfd7968ac3c43..f430a80c4370e 100644 --- a/test/common/router/router_ratelimit_test.cc +++ b/test/common/router/router_ratelimit_test.cc @@ -567,4 +567,32 @@ TEST_F(RateLimitPolicyEntryTest, CompoundActions) { testing::ContainerEq(descriptors_)); } +TEST_F(RateLimitPolicyEntryTest, CompoundActionsNoDescriptor) { + std::string json = R"EOF( + { + "actions": [ + { + "type": "destination_cluster" + }, + { + "type": "header_value_match", + "descriptor_value": "fake_value", + "headers": [ + { + "name": "x-header-name", + "value": "test_value", + "regex": false + } + ] + } + ] + } + )EOF"; + + SetUpTest(json); + + rate_limit_entry_->populateDescriptors(route_, descriptors_, "service_cluster", header_, ""); + EXPECT_TRUE(descriptors_.empty()); +} + } // Router