From 787c9629e31a659e9f03b56150ed9bbc4aa40fff Mon Sep 17 00:00:00 2001 From: Rohan Seth Date: Mon, 11 May 2020 09:56:35 -0700 Subject: [PATCH 01/19] Add skip_if_absent flag to request headers Signed-off-by: Rohan Seth --- api/envoy/config/route/v3/route_components.proto | 2 ++ 1 file changed, 2 insertions(+) diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index 3f82fbd80fb0..99d7cde052b0 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -1385,6 +1385,8 @@ message RateLimit { // The key to use in the descriptor entry. string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; + + google.protobuf.BoolValue skip_if_absent = 3; } // The following descriptor entry is appended to the descriptor and is populated using the From e0ef3e8bf221a16504bb6bc3c603d68090976954 Mon Sep 17 00:00:00 2001 From: Rohan Seth Date: Mon, 11 May 2020 12:02:44 -0700 Subject: [PATCH 02/19] Adding skip_if_absent Signed-off-by: Rohan Seth --- api/envoy/config/route/v3/route_components.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index 99d7cde052b0..35e1905bcc84 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -1386,7 +1386,7 @@ message RateLimit { // The key to use in the descriptor entry. string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; - google.protobuf.BoolValue skip_if_absent = 3; + bool skip_if_absent = 3; } // The following descriptor entry is appended to the descriptor and is populated using the From 5fdb921f9448680d1fb9de05bdc278e03e68d613 Mon Sep 17 00:00:00 2001 From: Rohan Seth Date: Mon, 11 May 2020 14:33:49 -0700 Subject: [PATCH 03/19] Adding skip_if_absent to router_ratelimit Signed-off-by: Rohan Seth --- .../config/route/v3/route_components.proto | 2 + .../route/v4alpha/route_components.proto | 4 + source/common/router/router_ratelimit.cc | 6 +- source/common/router/router_ratelimit.h | 3 +- test/common/router/router_ratelimit_test.cc | 94 +++++++++++++++++++ 5 files changed, 107 insertions(+), 2 deletions(-) diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index 35e1905bcc84..79dd9508b3ea 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -1386,6 +1386,8 @@ message RateLimit { // The key to use in the descriptor entry. string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; + // Optional parameter which determines if the RLS is called if a request header + // is not present. Defaults to false to ensure no breaking behaviour bool skip_if_absent = 3; } diff --git a/api/envoy/config/route/v4alpha/route_components.proto b/api/envoy/config/route/v4alpha/route_components.proto index 6e1b1f9f5a0a..3630c76a393f 100644 --- a/api/envoy/config/route/v4alpha/route_components.proto +++ b/api/envoy/config/route/v4alpha/route_components.proto @@ -1393,6 +1393,10 @@ message RateLimit { // The key to use in the descriptor entry. string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; + + // Optional parameter which determines if the RLS is called if a request header + // is not present. Defaults to false to ensure no breaking behaviour + bool skip_if_absent = 3; } // The following descriptor entry is appended to the descriptor and is populated using the diff --git a/source/common/router/router_ratelimit.cc b/source/common/router/router_ratelimit.cc index 259834270e77..eb02429115a6 100644 --- a/source/common/router/router_ratelimit.cc +++ b/source/common/router/router_ratelimit.cc @@ -38,9 +38,13 @@ bool RequestHeadersAction::populateDescriptor(const Router::RouteEntry&, const Http::HeaderMap& headers, const Network::Address::Instance&) const { const Http::HeaderEntry* header_value = headers.get(header_name_); - if (!header_value) { + if (!header_value && !skip_if_absent_) { return false; } + + if(!header_value && skip_if_absent_) { + return true; + } descriptor.entries_.push_back( {descriptor_key_, std::string(header_value->value().getStringView())}); diff --git a/source/common/router/router_ratelimit.h b/source/common/router/router_ratelimit.h index 0d7826f67be9..2749ca1aebac 100644 --- a/source/common/router/router_ratelimit.h +++ b/source/common/router/router_ratelimit.h @@ -42,7 +42,7 @@ class DestinationClusterAction : public RateLimitAction { class RequestHeadersAction : public RateLimitAction { public: RequestHeadersAction(const envoy::config::route::v3::RateLimit::Action::RequestHeaders& action) - : header_name_(action.header_name()), descriptor_key_(action.descriptor_key()) {} + : header_name_(action.header_name()), descriptor_key_(action.descriptor_key()), skip_if_absent_(action.skip_if_absent()) {} // Router::RateLimitAction bool populateDescriptor(const Router::RouteEntry& route, RateLimit::Descriptor& descriptor, @@ -52,6 +52,7 @@ class RequestHeadersAction : public RateLimitAction { private: const Http::LowerCaseString header_name_; const std::string descriptor_key_; + const bool skip_if_absent_; }; /** diff --git a/test/common/router/router_ratelimit_test.cc b/test/common/router/router_ratelimit_test.cc index 1205fe0707f5..302679ca7896 100644 --- a/test/common/router/router_ratelimit_test.cc +++ b/test/common/router/router_ratelimit_test.cc @@ -366,6 +366,100 @@ TEST_F(RateLimitPolicyEntryTest, RequestHeaders) { testing::ContainerEq(descriptors_)); } + +// Validate that a descriptor is added if at least one request header has value. +TEST_F(RateLimitPolicyEntryTest, RequestHeadersPartialMatchWithSkipIfAbsent) { + const std::string yaml = R"EOF( +actions: +- request_headers: + header_name: x-header-name + descriptor_key: my_header_name + skip_if_absent: false +- request_headers: + header_name: x-header + descriptor_key: my_header + skip_if_absent: true + )EOF"; + + setupTest(yaml); + Http::TestHeaderMapImpl header{{"x-header-name", "test_value"}}; + + rate_limit_entry_->populateDescriptors(route_, descriptors_, "service_cluster", header, + default_remote_address_); + EXPECT_THAT(std::vector({{{{"my_header_name", "test_value"}}}}), + testing::ContainerEq(descriptors_)); +} + + +TEST_F(RateLimitPolicyEntryTest, RequestHeadersPartialMatchWithoutSkipIfAbsent) { + const std::string yaml = R"EOF( +actions: +- request_headers: + header_name: x-header-name + descriptor_key: my_header_name + skip_if_absent: false +- request_headers: + header_name: x-header + descriptor_key: my_header + skip_if_absent: false + )EOF"; + + setupTest(yaml); + Http::TestHeaderMapImpl header{{"x-header-test", "test_value"}}; + + rate_limit_entry_->populateDescriptors(route_, descriptors_, "service_cluster", header, + default_remote_address_); + EXPECT_TRUE(descriptors_.empty()); +} + + +TEST_F(RateLimitPolicyEntryTest, RequestHeadersCompleteMatch) { + const std::string yaml = R"EOF( +actions: +- request_headers: + header_name: x-header-name + descriptor_key: my_header_name + skip_if_absent: false +- request_headers: + header_name: x-header + descriptor_key: my_header + skip_if_absent: false + )EOF"; + + setupTest(yaml); + Http::TestHeaderMapImpl header{{"x-header-name", "test_value"}, {"x-header", "test_value"}}; + + rate_limit_entry_->populateDescriptors(route_, descriptors_, "service_cluster", header, + default_remote_address_); + EXPECT_THAT(std::vector({{{{"my_header_name", "test_value"}, {"my_header", "test_value"}}}}), + testing::ContainerEq(descriptors_)); +} + + + + +TEST_F(RateLimitPolicyEntryTest, RequestHeadersMatchDefaultSkipIfAbsent) { + const std::string yaml = R"EOF( +actions: +- request_headers: + header_name: x-header-name + descriptor_key: my_header_name +- request_headers: + header_name: x-header + descriptor_key: my_header + )EOF"; + + setupTest(yaml); + Http::TestHeaderMapImpl header{{"x-header-name", "test_value"}, {"x-header", "test_value"}}; + + rate_limit_entry_->populateDescriptors(route_, descriptors_, "service_cluster", header, + default_remote_address_); + EXPECT_THAT(std::vector({{{{"my_header_name", "test_value"}, {"my_header", "test_value"}}}}), + testing::ContainerEq(descriptors_)); +} + + + TEST_F(RateLimitPolicyEntryTest, RequestHeadersNoMatch) { const std::string yaml = R"EOF( actions: From 5fcdbc843e22a7a2f42b233fa442833d61b844e9 Mon Sep 17 00:00:00 2001 From: Rohan Seth Date: Mon, 11 May 2020 14:37:58 -0700 Subject: [PATCH 04/19] Clang format fix Signed-off-by: Rohan Seth --- api/envoy/config/route/v3/route_components.proto | 2 +- api/envoy/config/route/v4alpha/route_components.proto | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index 79dd9508b3ea..b95da1f6ec71 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -1387,7 +1387,7 @@ message RateLimit { string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; // Optional parameter which determines if the RLS is called if a request header - // is not present. Defaults to false to ensure no breaking behaviour + // is not present. Defaults to false to ensure no breaking behaviour. bool skip_if_absent = 3; } diff --git a/api/envoy/config/route/v4alpha/route_components.proto b/api/envoy/config/route/v4alpha/route_components.proto index 3630c76a393f..0e48c3280bce 100644 --- a/api/envoy/config/route/v4alpha/route_components.proto +++ b/api/envoy/config/route/v4alpha/route_components.proto @@ -1395,7 +1395,7 @@ message RateLimit { string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; // Optional parameter which determines if the RLS is called if a request header - // is not present. Defaults to false to ensure no breaking behaviour + // is not present. Defaults to false to ensure no breaking behaviour. bool skip_if_absent = 3; } From 2eb2da72acb4df6fa0cbb4e0c159b6d868ec3148 Mon Sep 17 00:00:00 2001 From: Rohan Seth Date: Mon, 11 May 2020 14:39:49 -0700 Subject: [PATCH 05/19] Clang format fix Signed-off-by: Rohan Seth --- api/envoy/config/route/v3/route_components.proto | 4 ++-- api/envoy/config/route/v4alpha/route_components.proto | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index b95da1f6ec71..89f4c4e888d0 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -1386,8 +1386,8 @@ message RateLimit { // The key to use in the descriptor entry. string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; - // Optional parameter which determines if the RLS is called if a request header - // is not present. Defaults to false to ensure no breaking behaviour. + // The optional parameter which determines if the RLS is called if a request + // header is not present. The value defaults to false to ensure no breaking behaviour. bool skip_if_absent = 3; } diff --git a/api/envoy/config/route/v4alpha/route_components.proto b/api/envoy/config/route/v4alpha/route_components.proto index 0e48c3280bce..d49546cc0f8e 100644 --- a/api/envoy/config/route/v4alpha/route_components.proto +++ b/api/envoy/config/route/v4alpha/route_components.proto @@ -1394,8 +1394,8 @@ message RateLimit { // The key to use in the descriptor entry. string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; - // Optional parameter which determines if the RLS is called if a request header - // is not present. Defaults to false to ensure no breaking behaviour. + // The optional parameter which determines if the RLS is called if a request + // header is not present. The value defaults to false to ensure no breaking behaviour. bool skip_if_absent = 3; } From a8b1a123ac40d9f1f8f86c8293d36ffe9097b18d Mon Sep 17 00:00:00 2001 From: Rohan Seth Date: Mon, 11 May 2020 14:58:52 -0700 Subject: [PATCH 06/19] Adding generated files Signed-off-by: Rohan Seth --- .../envoy/config/route/v3/route_components.proto | 2 ++ .../envoy/config/route/v4alpha/route_components.proto | 2 ++ 2 files changed, 4 insertions(+) diff --git a/generated_api_shadow/envoy/config/route/v3/route_components.proto b/generated_api_shadow/envoy/config/route/v3/route_components.proto index 00d4f5e628a7..083cf2bd5e7e 100644 --- a/generated_api_shadow/envoy/config/route/v3/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v3/route_components.proto @@ -1397,6 +1397,8 @@ message RateLimit { // The key to use in the descriptor entry. string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; + + bool skip_if_absent = 3; } // The following descriptor entry is appended to the descriptor and is populated using the diff --git a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto index 6e1b1f9f5a0a..b71dce6a33c2 100644 --- a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto @@ -1393,6 +1393,8 @@ message RateLimit { // The key to use in the descriptor entry. string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; + + bool skip_if_absent = 3; } // The following descriptor entry is appended to the descriptor and is populated using the From be0f75cce05fd90c7415d2ffd969d47a8faac2b0 Mon Sep 17 00:00:00 2001 From: Rohan Seth Date: Mon, 11 May 2020 15:26:51 -0700 Subject: [PATCH 07/19] Adding generated files Signed-off-by: Rohan Seth --- .../envoy/config/route/v3/route_components.proto | 2 ++ .../envoy/config/route/v4alpha/route_components.proto | 2 ++ 2 files changed, 4 insertions(+) diff --git a/generated_api_shadow/envoy/config/route/v3/route_components.proto b/generated_api_shadow/envoy/config/route/v3/route_components.proto index 083cf2bd5e7e..dfa1f6419ce2 100644 --- a/generated_api_shadow/envoy/config/route/v3/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v3/route_components.proto @@ -1398,6 +1398,8 @@ message RateLimit { // The key to use in the descriptor entry. string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; + // The optional parameter which determines if the RLS is called if a request + // header is not present. The value defaults to false to ensure no breaking behaviour. bool skip_if_absent = 3; } diff --git a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto index b71dce6a33c2..d49546cc0f8e 100644 --- a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto @@ -1394,6 +1394,8 @@ message RateLimit { // The key to use in the descriptor entry. string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; + // The optional parameter which determines if the RLS is called if a request + // header is not present. The value defaults to false to ensure no breaking behaviour. bool skip_if_absent = 3; } From c6173f6dd1ccf0a677f93657b96e42e6590631fe Mon Sep 17 00:00:00 2001 From: Rohan Seth Date: Tue, 12 May 2020 17:46:12 -0700 Subject: [PATCH 08/19] Clang-formating changes Signed-off-by: Rohan Seth --- source/common/router/router_ratelimit.cc | 4 ++-- source/common/router/router_ratelimit.h | 3 ++- test/common/router/router_ratelimit_test.cc | 14 ++++---------- 3 files changed, 8 insertions(+), 13 deletions(-) diff --git a/source/common/router/router_ratelimit.cc b/source/common/router/router_ratelimit.cc index eb02429115a6..48a9c1195c0a 100644 --- a/source/common/router/router_ratelimit.cc +++ b/source/common/router/router_ratelimit.cc @@ -41,8 +41,8 @@ bool RequestHeadersAction::populateDescriptor(const Router::RouteEntry&, if (!header_value && !skip_if_absent_) { return false; } - - if(!header_value && skip_if_absent_) { + + if (!header_value && skip_if_absent_) { return true; } diff --git a/source/common/router/router_ratelimit.h b/source/common/router/router_ratelimit.h index 2749ca1aebac..df42e898952b 100644 --- a/source/common/router/router_ratelimit.h +++ b/source/common/router/router_ratelimit.h @@ -42,7 +42,8 @@ class DestinationClusterAction : public RateLimitAction { class RequestHeadersAction : public RateLimitAction { public: RequestHeadersAction(const envoy::config::route::v3::RateLimit::Action::RequestHeaders& action) - : header_name_(action.header_name()), descriptor_key_(action.descriptor_key()), skip_if_absent_(action.skip_if_absent()) {} + : header_name_(action.header_name()), descriptor_key_(action.descriptor_key()), + skip_if_absent_(action.skip_if_absent()) {} // Router::RateLimitAction bool populateDescriptor(const Router::RouteEntry& route, RateLimit::Descriptor& descriptor, diff --git a/test/common/router/router_ratelimit_test.cc b/test/common/router/router_ratelimit_test.cc index 302679ca7896..d85d60a0922f 100644 --- a/test/common/router/router_ratelimit_test.cc +++ b/test/common/router/router_ratelimit_test.cc @@ -366,7 +366,6 @@ TEST_F(RateLimitPolicyEntryTest, RequestHeaders) { testing::ContainerEq(descriptors_)); } - // Validate that a descriptor is added if at least one request header has value. TEST_F(RateLimitPolicyEntryTest, RequestHeadersPartialMatchWithSkipIfAbsent) { const std::string yaml = R"EOF( @@ -390,7 +389,6 @@ TEST_F(RateLimitPolicyEntryTest, RequestHeadersPartialMatchWithSkipIfAbsent) { testing::ContainerEq(descriptors_)); } - TEST_F(RateLimitPolicyEntryTest, RequestHeadersPartialMatchWithoutSkipIfAbsent) { const std::string yaml = R"EOF( actions: @@ -412,7 +410,6 @@ TEST_F(RateLimitPolicyEntryTest, RequestHeadersPartialMatchWithoutSkipIfAbsent) EXPECT_TRUE(descriptors_.empty()); } - TEST_F(RateLimitPolicyEntryTest, RequestHeadersCompleteMatch) { const std::string yaml = R"EOF( actions: @@ -431,13 +428,11 @@ TEST_F(RateLimitPolicyEntryTest, RequestHeadersCompleteMatch) { rate_limit_entry_->populateDescriptors(route_, descriptors_, "service_cluster", header, default_remote_address_); - EXPECT_THAT(std::vector({{{{"my_header_name", "test_value"}, {"my_header", "test_value"}}}}), + EXPECT_THAT(std::vector( + {{{{"my_header_name", "test_value"}, {"my_header", "test_value"}}}}), testing::ContainerEq(descriptors_)); } - - - TEST_F(RateLimitPolicyEntryTest, RequestHeadersMatchDefaultSkipIfAbsent) { const std::string yaml = R"EOF( actions: @@ -454,12 +449,11 @@ TEST_F(RateLimitPolicyEntryTest, RequestHeadersMatchDefaultSkipIfAbsent) { rate_limit_entry_->populateDescriptors(route_, descriptors_, "service_cluster", header, default_remote_address_); - EXPECT_THAT(std::vector({{{{"my_header_name", "test_value"}, {"my_header", "test_value"}}}}), + EXPECT_THAT(std::vector( + {{{{"my_header_name", "test_value"}, {"my_header", "test_value"}}}}), testing::ContainerEq(descriptors_)); } - - TEST_F(RateLimitPolicyEntryTest, RequestHeadersNoMatch) { const std::string yaml = R"EOF( actions: From 415de695c7dc98bdfbcf66c136d4052bbd95185c Mon Sep 17 00:00:00 2001 From: Rohan Seth Date: Wed, 13 May 2020 01:11:44 -0700 Subject: [PATCH 09/19] Modifying comment Signed-off-by: Rohan Seth --- api/envoy/config/route/v3/route_components.proto | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index 89f4c4e888d0..00b501d092a4 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -1386,8 +1386,9 @@ message RateLimit { // The key to use in the descriptor entry. string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; - // The optional parameter which determines if the RLS is called if a request - // header is not present. The value defaults to false to ensure no breaking behaviour. + // If set to true, Envoy skips the descriptor while calling rate limiting service + // when header is not present in the request. Defaults to false, which skips calling + // rate limiting service if this header is not present in the request. bool skip_if_absent = 3; } From 9db6ec93b7f9d98ddb2e74be8ab71f9f886f664d Mon Sep 17 00:00:00 2001 From: Rohan Seth Date: Wed, 13 May 2020 02:03:48 -0700 Subject: [PATCH 10/19] Adding comments and release notes Signed-off-by: Rohan Seth --- .../config/route/v3/route_components.proto | 6 +-- .../route/v4alpha/route_components.proto | 5 +- docs/root/version_history/current.rst | 3 +- .../config/route/v3/route_components.proto | 5 +- .../route/v4alpha/route_components.proto | 5 +- source/common/router/router_ratelimit.cc | 9 ++-- test/common/router/router_ratelimit_test.cc | 53 +++---------------- 7 files changed, 23 insertions(+), 63 deletions(-) diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index 00b501d092a4..349c48711da5 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -1386,9 +1386,9 @@ message RateLimit { // The key to use in the descriptor entry. string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; - // If set to true, Envoy skips the descriptor while calling rate limiting service - // when header is not present in the request. Defaults to false, which skips calling - // rate limiting service if this header is not present in the request. + // If set to true, Envoy skips the descriptor while calling rate limiting service + // when header is not present in the request. Defaults to false, which skips calling + // rate limiting service if this header is not present in the request. bool skip_if_absent = 3; } diff --git a/api/envoy/config/route/v4alpha/route_components.proto b/api/envoy/config/route/v4alpha/route_components.proto index d49546cc0f8e..23e500513f0b 100644 --- a/api/envoy/config/route/v4alpha/route_components.proto +++ b/api/envoy/config/route/v4alpha/route_components.proto @@ -1394,8 +1394,9 @@ message RateLimit { // The key to use in the descriptor entry. string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; - // The optional parameter which determines if the RLS is called if a request - // header is not present. The value defaults to false to ensure no breaking behaviour. + // If set to true, Envoy skips the descriptor while calling rate limiting service + // when header is not present in the request. Defaults to false, which skips calling + // rate limiting service if this header is not present in the request. bool skip_if_absent = 3; } diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 61d3eb38b5b4..1178e3ce8a91 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -39,10 +39,11 @@ Changes * network filters: added a :ref:`rocketmq proxy filter `. * prometheus stats: fix the sort order of output lines to comply with the standard. * request_id: added to :ref:`always_set_request_id_in_response setting ` - to set :ref:`x-request-id ` header in response even if + to set :ref:`x-request-id ` header in response even if skip_if_absent tracing is not forced. * router: allow retries of streaming or incomplete requests. This removes stat `rq_retry_skipped_request_not_complete`. * router: allow retries by default when upstream responds with :ref:`x-envoy-overloaded `. +* router: allow Rate Limiting service to be called in case of missing request header for a descriptor if the :ref:`skip_if_absent ` field is set to true. It defaults to false. * runtime: add new gauge :ref:`deprecated_feature_seen_since_process_start ` that gets reset across hot restarts. * stats: added the option to :ref:`report counters as deltas ` to the metrics service stats sink. * tracing: tracing configuration has been made fully dynamic and every HTTP connection manager diff --git a/generated_api_shadow/envoy/config/route/v3/route_components.proto b/generated_api_shadow/envoy/config/route/v3/route_components.proto index dfa1f6419ce2..8e1dd2ea643d 100644 --- a/generated_api_shadow/envoy/config/route/v3/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v3/route_components.proto @@ -1398,8 +1398,9 @@ message RateLimit { // The key to use in the descriptor entry. string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; - // The optional parameter which determines if the RLS is called if a request - // header is not present. The value defaults to false to ensure no breaking behaviour. + // If set to true, Envoy skips the descriptor while calling rate limiting service + // when header is not present in the request. Defaults to false, which skips calling + // rate limiting service if this header is not present in the request. bool skip_if_absent = 3; } diff --git a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto index d49546cc0f8e..23e500513f0b 100644 --- a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto @@ -1394,8 +1394,9 @@ message RateLimit { // The key to use in the descriptor entry. string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; - // The optional parameter which determines if the RLS is called if a request - // header is not present. The value defaults to false to ensure no breaking behaviour. + // If set to true, Envoy skips the descriptor while calling rate limiting service + // when header is not present in the request. Defaults to false, which skips calling + // rate limiting service if this header is not present in the request. bool skip_if_absent = 3; } diff --git a/source/common/router/router_ratelimit.cc b/source/common/router/router_ratelimit.cc index 48a9c1195c0a..7a0256508e13 100644 --- a/source/common/router/router_ratelimit.cc +++ b/source/common/router/router_ratelimit.cc @@ -38,14 +38,11 @@ bool RequestHeadersAction::populateDescriptor(const Router::RouteEntry&, const Http::HeaderMap& headers, const Network::Address::Instance&) const { const Http::HeaderEntry* header_value = headers.get(header_name_); - if (!header_value && !skip_if_absent_) { - return false; - } - if (!header_value && skip_if_absent_) { - return true; + // If header is not present and skip_if_absent_ is not set to true, short circuit here and return. + if (!header_value) { + return skip_if_absent_; } - descriptor.entries_.push_back( {descriptor_key_, std::string(header_value->value().getStringView())}); return true; diff --git a/test/common/router/router_ratelimit_test.cc b/test/common/router/router_ratelimit_test.cc index d85d60a0922f..20954ec61ffb 100644 --- a/test/common/router/router_ratelimit_test.cc +++ b/test/common/router/router_ratelimit_test.cc @@ -366,8 +366,9 @@ TEST_F(RateLimitPolicyEntryTest, RequestHeaders) { testing::ContainerEq(descriptors_)); } -// Validate that a descriptor is added if at least one request header has value. -TEST_F(RateLimitPolicyEntryTest, RequestHeadersPartialMatchWithSkipIfAbsent) { +// Validate that a descriptor is added if the missing request header +// has skip_if_absent set to true +TEST_F(RateLimitPolicyEntryTest, RequestHeadersWithSkipIfAbsent) { const std::string yaml = R"EOF( actions: - request_headers: @@ -389,7 +390,9 @@ TEST_F(RateLimitPolicyEntryTest, RequestHeadersPartialMatchWithSkipIfAbsent) { testing::ContainerEq(descriptors_)); } -TEST_F(RateLimitPolicyEntryTest, RequestHeadersPartialMatchWithoutSkipIfAbsent) { +// Tests if the descriptors are added if one of the headers is missing +// and skip_if_absent is set to default value which is false +TEST_F(RateLimitPolicyEntryTest, RequestHeadersWithDefaultSkipIfAbsent) { const std::string yaml = R"EOF( actions: - request_headers: @@ -410,50 +413,6 @@ TEST_F(RateLimitPolicyEntryTest, RequestHeadersPartialMatchWithoutSkipIfAbsent) EXPECT_TRUE(descriptors_.empty()); } -TEST_F(RateLimitPolicyEntryTest, RequestHeadersCompleteMatch) { - const std::string yaml = R"EOF( -actions: -- request_headers: - header_name: x-header-name - descriptor_key: my_header_name - skip_if_absent: false -- request_headers: - header_name: x-header - descriptor_key: my_header - skip_if_absent: false - )EOF"; - - setupTest(yaml); - Http::TestHeaderMapImpl header{{"x-header-name", "test_value"}, {"x-header", "test_value"}}; - - rate_limit_entry_->populateDescriptors(route_, descriptors_, "service_cluster", header, - default_remote_address_); - EXPECT_THAT(std::vector( - {{{{"my_header_name", "test_value"}, {"my_header", "test_value"}}}}), - testing::ContainerEq(descriptors_)); -} - -TEST_F(RateLimitPolicyEntryTest, RequestHeadersMatchDefaultSkipIfAbsent) { - const std::string yaml = R"EOF( -actions: -- request_headers: - header_name: x-header-name - descriptor_key: my_header_name -- request_headers: - header_name: x-header - descriptor_key: my_header - )EOF"; - - setupTest(yaml); - Http::TestHeaderMapImpl header{{"x-header-name", "test_value"}, {"x-header", "test_value"}}; - - rate_limit_entry_->populateDescriptors(route_, descriptors_, "service_cluster", header, - default_remote_address_); - EXPECT_THAT(std::vector( - {{{{"my_header_name", "test_value"}, {"my_header", "test_value"}}}}), - testing::ContainerEq(descriptors_)); -} - TEST_F(RateLimitPolicyEntryTest, RequestHeadersNoMatch) { const std::string yaml = R"EOF( actions: From 8b1e90648622cb51ed04156910da0614a5366d85 Mon Sep 17 00:00:00 2001 From: Rohan Seth Date: Wed, 13 May 2020 02:06:34 -0700 Subject: [PATCH 11/19] Adding release notes Signed-off-by: Rohan Seth --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 1178e3ce8a91..7ee18a9f49b3 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -43,7 +43,7 @@ Changes tracing is not forced. * router: allow retries of streaming or incomplete requests. This removes stat `rq_retry_skipped_request_not_complete`. * router: allow retries by default when upstream responds with :ref:`x-envoy-overloaded `. -* router: allow Rate Limiting service to be called in case of missing request header for a descriptor if the :ref:`skip_if_absent ` field is set to true. It defaults to false. +* router: allow Rate Limiting service to be called in case of missing request header for a descriptor if the :ref:`skip_if_absent ` field is set to true. * runtime: add new gauge :ref:`deprecated_feature_seen_since_process_start ` that gets reset across hot restarts. * stats: added the option to :ref:`report counters as deltas ` to the metrics service stats sink. * tracing: tracing configuration has been made fully dynamic and every HTTP connection manager From 5e2eb3a60e036573461091212c009043463d8646 Mon Sep 17 00:00:00 2001 From: Rohan Seth Date: Wed, 13 May 2020 02:35:56 -0700 Subject: [PATCH 12/19] Updating release notes Signed-off-by: Rohan Seth --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 7ee18a9f49b3..f7446a1b098d 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -43,7 +43,7 @@ Changes tracing is not forced. * router: allow retries of streaming or incomplete requests. This removes stat `rq_retry_skipped_request_not_complete`. * router: allow retries by default when upstream responds with :ref:`x-envoy-overloaded `. -* router: allow Rate Limiting service to be called in case of missing request header for a descriptor if the :ref:`skip_if_absent ` field is set to true. +* router: allow Rate Limiting service to be called in case of missing request header for a descriptor if the :ref:`skip_if_absent ` field is set to true. * runtime: add new gauge :ref:`deprecated_feature_seen_since_process_start ` that gets reset across hot restarts. * stats: added the option to :ref:`report counters as deltas ` to the metrics service stats sink. * tracing: tracing configuration has been made fully dynamic and every HTTP connection manager From d9495efbef1f9ab39173f2723d7d15a72a5c6838 Mon Sep 17 00:00:00 2001 From: Rohan Seth Date: Wed, 13 May 2020 02:47:50 -0700 Subject: [PATCH 13/19] Fixing accidental release note change Signed-off-by: Rohan Seth --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index f7446a1b098d..bbce53797f3c 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -39,7 +39,7 @@ Changes * network filters: added a :ref:`rocketmq proxy filter `. * prometheus stats: fix the sort order of output lines to comply with the standard. * request_id: added to :ref:`always_set_request_id_in_response setting ` - to set :ref:`x-request-id ` header in response even if skip_if_absent + to set :ref:`x-request-id ` header in response even if tracing is not forced. * router: allow retries of streaming or incomplete requests. This removes stat `rq_retry_skipped_request_not_complete`. * router: allow retries by default when upstream responds with :ref:`x-envoy-overloaded `. From 5abb2ff2f33bfaa0fc2e41985857b746ca1a7e87 Mon Sep 17 00:00:00 2001 From: Rohan Seth Date: Wed, 13 May 2020 09:06:00 -0700 Subject: [PATCH 14/19] Modifying proto file comment Signed-off-by: Rohan Seth --- api/envoy/config/route/v3/route_components.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index 349c48711da5..97d5bb0a3f19 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -1387,7 +1387,7 @@ message RateLimit { string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; // If set to true, Envoy skips the descriptor while calling rate limiting service - // when header is not present in the request. Defaults to false, which skips calling + // when header is not present in the request. By default it skips calling the // rate limiting service if this header is not present in the request. bool skip_if_absent = 3; } From 102860f02414074e02bc0c965e52097e45c0089b Mon Sep 17 00:00:00 2001 From: Rohan Seth Date: Wed, 13 May 2020 09:32:23 -0700 Subject: [PATCH 15/19] Modifying comments Signed-off-by: Rohan Seth --- api/envoy/config/route/v4alpha/route_components.proto | 2 +- docs/root/version_history/current.rst | 2 +- .../envoy/config/route/v3/route_components.proto | 2 +- .../envoy/config/route/v4alpha/route_components.proto | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api/envoy/config/route/v4alpha/route_components.proto b/api/envoy/config/route/v4alpha/route_components.proto index 23e500513f0b..032251bd8c4e 100644 --- a/api/envoy/config/route/v4alpha/route_components.proto +++ b/api/envoy/config/route/v4alpha/route_components.proto @@ -1395,7 +1395,7 @@ message RateLimit { string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; // If set to true, Envoy skips the descriptor while calling rate limiting service - // when header is not present in the request. Defaults to false, which skips calling + // when header is not present in the request. By default it skips calling the // rate limiting service if this header is not present in the request. bool skip_if_absent = 3; } diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index bbce53797f3c..71fb8cc0166e 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -43,7 +43,7 @@ Changes tracing is not forced. * router: allow retries of streaming or incomplete requests. This removes stat `rq_retry_skipped_request_not_complete`. * router: allow retries by default when upstream responds with :ref:`x-envoy-overloaded `. -* router: allow Rate Limiting service to be called in case of missing request header for a descriptor if the :ref:`skip_if_absent ` field is set to true. +* router: allow Rate Limiting Service to be called in case of missing request header for a descriptor if the :ref:`skip_if_absent ` field is set to true. * runtime: add new gauge :ref:`deprecated_feature_seen_since_process_start ` that gets reset across hot restarts. * stats: added the option to :ref:`report counters as deltas ` to the metrics service stats sink. * tracing: tracing configuration has been made fully dynamic and every HTTP connection manager diff --git a/generated_api_shadow/envoy/config/route/v3/route_components.proto b/generated_api_shadow/envoy/config/route/v3/route_components.proto index 8e1dd2ea643d..3a4e40dfc00f 100644 --- a/generated_api_shadow/envoy/config/route/v3/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v3/route_components.proto @@ -1399,7 +1399,7 @@ message RateLimit { string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; // If set to true, Envoy skips the descriptor while calling rate limiting service - // when header is not present in the request. Defaults to false, which skips calling + // when header is not present in the request. By default it skips calling the // rate limiting service if this header is not present in the request. bool skip_if_absent = 3; } diff --git a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto index 23e500513f0b..032251bd8c4e 100644 --- a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto @@ -1395,7 +1395,7 @@ message RateLimit { string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}]; // If set to true, Envoy skips the descriptor while calling rate limiting service - // when header is not present in the request. Defaults to false, which skips calling + // when header is not present in the request. By default it skips calling the // rate limiting service if this header is not present in the request. bool skip_if_absent = 3; } From 9cfaeaafbb129ae50a69cf7e6d6a896984fc80ec Mon Sep 17 00:00:00 2001 From: Rohan Seth Date: Mon, 18 May 2020 13:12:25 -0700 Subject: [PATCH 16/19] Updating comment Signed-off-by: Rohan Seth --- source/common/router/router_ratelimit.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/router/router_ratelimit.cc b/source/common/router/router_ratelimit.cc index 7a0256508e13..42dbe15dad33 100644 --- a/source/common/router/router_ratelimit.cc +++ b/source/common/router/router_ratelimit.cc @@ -39,7 +39,7 @@ bool RequestHeadersAction::populateDescriptor(const Router::RouteEntry&, const Network::Address::Instance&) const { const Http::HeaderEntry* header_value = headers.get(header_name_); - // If header is not present and skip_if_absent_ is not set to true, short circuit here and return. + // If header is not present and skip_if_absent_ is true, ignore the descriptor if (!header_value) { return skip_if_absent_; } From 281b9e460b2b6662d9a8d683b157c4c878f94a14 Mon Sep 17 00:00:00 2001 From: Rohan Seth Date: Wed, 20 May 2020 11:37:34 -0700 Subject: [PATCH 17/19] Add descriptive comment Signed-off-by: Rohan Seth --- source/common/router/router_ratelimit.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/router/router_ratelimit.cc b/source/common/router/router_ratelimit.cc index 42dbe15dad33..d55dde2a381b 100644 --- a/source/common/router/router_ratelimit.cc +++ b/source/common/router/router_ratelimit.cc @@ -39,7 +39,8 @@ bool RequestHeadersAction::populateDescriptor(const Router::RouteEntry&, const Network::Address::Instance&) const { const Http::HeaderEntry* header_value = headers.get(header_name_); - // If header is not present and skip_if_absent_ is true, ignore the descriptor + // If header is not present in the request and if skip_if_absent is true skip this descriptor, + // while calling rate limiting service. If skip_if_absent is false, do not call rate limiting service. if (!header_value) { return skip_if_absent_; } From df712d2d56e971b35b374782719835478f7ddc70 Mon Sep 17 00:00:00 2001 From: Rohan Seth Date: Wed, 20 May 2020 14:15:40 -0700 Subject: [PATCH 18/19] router_ratelimit formatting fix Signed-off-by: Rohan Seth --- source/common/router/router_ratelimit.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/source/common/router/router_ratelimit.cc b/source/common/router/router_ratelimit.cc index d55dde2a381b..c883ce894210 100644 --- a/source/common/router/router_ratelimit.cc +++ b/source/common/router/router_ratelimit.cc @@ -39,8 +39,9 @@ bool RequestHeadersAction::populateDescriptor(const Router::RouteEntry&, const Network::Address::Instance&) const { const Http::HeaderEntry* header_value = headers.get(header_name_); - // If header is not present in the request and if skip_if_absent is true skip this descriptor, - // while calling rate limiting service. If skip_if_absent is false, do not call rate limiting service. + // If header is not present in the request and if skip_if_absent is true skip this descriptor, + // while calling rate limiting service. If skip_if_absent is false, do not call rate limiting + // service. if (!header_value) { return skip_if_absent_; } From 2360171641f0c3e9bba37e51b8a6efcc0828843d Mon Sep 17 00:00:00 2001 From: Rohan Seth Date: Wed, 20 May 2020 14:28:40 -0700 Subject: [PATCH 19/19] Fix current.rst Signed-off-by: Rohan Seth --- docs/root/version_history/current.rst | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index cbe200ec4433..befbc5ef6dd1 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -73,11 +73,9 @@ New Features * request_id: added to :ref:`always_set_request_id_in_response setting ` to set :ref:`x-request-id ` header in response even if tracing is not forced. -* router: allow retries of streaming or incomplete requests. This removes stat `rq_retry_skipped_request_not_complete`. -* router: allow retries by default when upstream responds with :ref:`x-envoy-overloaded `. -* router: allow Rate Limiting Service to be called in case of missing request header for a descriptor if the :ref:`skip_if_absent ` field is set to true. * router: add support for RESPONSE_FLAGS and RESPONSE_CODE_DETAILS :ref:`header formatters `. +* router: allow Rate Limiting Service to be called in case of missing request header for a descriptor if the :ref:`skip_if_absent ` field is set to true. * router: more fine grained internal redirect configs are added to the :ref`internal_redirect_policy ` field. * runtime: add new gauge :ref:`deprecated_feature_seen_since_process_start ` that gets reset across hot restarts.