From f57ea8fbd24529b822197fd40a00d6b1018b29ad Mon Sep 17 00:00:00 2001 From: John Esmet Date: Tue, 22 Dec 2020 15:57:25 +0000 Subject: [PATCH 01/11] ratelimit: add dynamic-metadata to ratelimit response Signed-off-by: John Esmet --- api/envoy/service/ratelimit/v3/rls.proto | 11 ++++- .../envoy/service/ratelimit/v3/rls.proto | 11 ++++- .../filters/common/ratelimit/ratelimit.h | 4 +- .../common/ratelimit/ratelimit_impl.cc | 9 +++- .../filters/http/ratelimit/ratelimit.cc | 8 +++- .../filters/http/ratelimit/ratelimit.h | 3 +- .../filters/network/ratelimit/BUILD | 1 + .../filters/network/ratelimit/ratelimit.cc | 11 ++++- .../filters/network/ratelimit/ratelimit.h | 3 +- .../network/thrift_proxy/filters/BUILD | 1 + .../thrift_proxy/filters/ratelimit/BUILD | 1 + .../filters/ratelimit/ratelimit.cc | 9 +++- .../filters/ratelimit/ratelimit.h | 3 +- .../common/ratelimit/ratelimit_impl_test.cc | 14 +++--- .../filters/http/ratelimit/ratelimit_test.cc | 45 ++++++++++--------- .../network/ratelimit/ratelimit_test.cc | 14 +++--- .../filters/ratelimit/ratelimit_test.cc | 16 +++---- 17 files changed, 108 insertions(+), 56 deletions(-) diff --git a/api/envoy/service/ratelimit/v3/rls.proto b/api/envoy/service/ratelimit/v3/rls.proto index 7379368fd4c1..a7dab9f29303 100644 --- a/api/envoy/service/ratelimit/v3/rls.proto +++ b/api/envoy/service/ratelimit/v3/rls.proto @@ -6,6 +6,7 @@ import "envoy/config/core/v3/base.proto"; import "envoy/extensions/common/ratelimit/v3/ratelimit.proto"; import "google/protobuf/duration.proto"; +import "google/protobuf/struct.proto"; import "udpa/annotations/status.proto"; import "udpa/annotations/versioning.proto"; @@ -51,7 +52,7 @@ message RateLimitRequest { } // A response from a ShouldRateLimit call. -// [#next-free-field: 6] +// [#next-free-field: 7] message RateLimitResponse { option (udpa.annotations.versioning).previous_message_type = "envoy.service.ratelimit.v2.RateLimitResponse"; @@ -135,4 +136,12 @@ message RateLimitResponse { // A response body to send to the downstream client when the response code is not OK. bytes raw_body = 5; + + // Optional response metadata that will be emitted as dynamic metadata to be consumed by the next + // filter. This metadata lives in a namespace specified by the canonical name of extension filter + // that requires it: + // + // - :ref:`envoy.filters.http.ratelimit ` for HTTP filter. + // - :ref:`envoy.filters.network.ratelimit ` for network filter. + google.protobuf.Struct dynamic_metadata = 6; } diff --git a/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto b/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto index 7379368fd4c1..a7dab9f29303 100644 --- a/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto +++ b/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto @@ -6,6 +6,7 @@ import "envoy/config/core/v3/base.proto"; import "envoy/extensions/common/ratelimit/v3/ratelimit.proto"; import "google/protobuf/duration.proto"; +import "google/protobuf/struct.proto"; import "udpa/annotations/status.proto"; import "udpa/annotations/versioning.proto"; @@ -51,7 +52,7 @@ message RateLimitRequest { } // A response from a ShouldRateLimit call. -// [#next-free-field: 6] +// [#next-free-field: 7] message RateLimitResponse { option (udpa.annotations.versioning).previous_message_type = "envoy.service.ratelimit.v2.RateLimitResponse"; @@ -135,4 +136,12 @@ message RateLimitResponse { // A response body to send to the downstream client when the response code is not OK. bytes raw_body = 5; + + // Optional response metadata that will be emitted as dynamic metadata to be consumed by the next + // filter. This metadata lives in a namespace specified by the canonical name of extension filter + // that requires it: + // + // - :ref:`envoy.filters.http.ratelimit ` for HTTP filter. + // - :ref:`envoy.filters.network.ratelimit ` for network filter. + google.protobuf.Struct dynamic_metadata = 6; } diff --git a/source/extensions/filters/common/ratelimit/ratelimit.h b/source/extensions/filters/common/ratelimit/ratelimit.h index 964cc25a7bd6..9ca803b8cd55 100644 --- a/source/extensions/filters/common/ratelimit/ratelimit.h +++ b/source/extensions/filters/common/ratelimit/ratelimit.h @@ -35,6 +35,7 @@ enum class LimitStatus { using DescriptorStatusList = std::vector; using DescriptorStatusListPtr = std::unique_ptr; +using DynamicMetadataPtr = std::unique_ptr; /** * Async callbacks used during limit() calls. @@ -57,7 +58,8 @@ class RequestCallbacks { virtual void complete(LimitStatus status, DescriptorStatusListPtr&& descriptor_statuses, Http::ResponseHeaderMapPtr&& response_headers_to_add, Http::RequestHeaderMapPtr&& request_headers_to_add, - const std::string& response_body) PURE; + const std::string& response_body, + DynamicMetadataPtr&& dynamic_metadata) PURE; }; /** diff --git a/source/extensions/filters/common/ratelimit/ratelimit_impl.cc b/source/extensions/filters/common/ratelimit/ratelimit_impl.cc index 4aef806f60d1..dd83d3117c1a 100644 --- a/source/extensions/filters/common/ratelimit/ratelimit_impl.cc +++ b/source/extensions/filters/common/ratelimit/ratelimit_impl.cc @@ -105,15 +105,20 @@ void GrpcClientImpl::onSuccess( DescriptorStatusListPtr descriptor_statuses = std::make_unique( response->statuses().begin(), response->statuses().end()); + // TODO(esmet): This dynamic metadata copy is probably unnecessary, but let's just following the + // existing pattern of copying parameters over as unique pointers for now. + DynamicMetadataPtr dynamic_metadata = + std::make_unique(response->dynamic_metadata()); callbacks_->complete(status, std::move(descriptor_statuses), std::move(response_headers_to_add), - std::move(request_headers_to_add), response->raw_body()); + std::move(request_headers_to_add), response->raw_body(), + std::move(dynamic_metadata)); callbacks_ = nullptr; } void GrpcClientImpl::onFailure(Grpc::Status::GrpcStatus status, const std::string&, Tracing::Span&) { ASSERT(status != Grpc::Status::WellKnownGrpcStatus::Ok); - callbacks_->complete(LimitStatus::Error, nullptr, nullptr, nullptr, EMPTY_STRING); + callbacks_->complete(LimitStatus::Error, nullptr, nullptr, nullptr, EMPTY_STRING, nullptr); callbacks_ = nullptr; } diff --git a/source/extensions/filters/http/ratelimit/ratelimit.cc b/source/extensions/filters/http/ratelimit/ratelimit.cc index b72bb82c4b75..a7565d3cc4db 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.cc +++ b/source/extensions/filters/http/ratelimit/ratelimit.cc @@ -144,7 +144,8 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status, Filters::Common::RateLimit::DescriptorStatusListPtr&& descriptor_statuses, Http::ResponseHeaderMapPtr&& response_headers_to_add, Http::RequestHeaderMapPtr&& request_headers_to_add, - const std::string& response_body) { + const std::string& response_body, + Filters::Common::RateLimit::DynamicMetadataPtr&& dynamic_metadata) { state_ = State::Complete; response_headers_to_add_ = std::move(response_headers_to_add); Http::HeaderMapPtr req_headers_to_add = std::move(request_headers_to_add); @@ -192,6 +193,11 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status, descriptor_statuses = nullptr; } + if (dynamic_metadata != nullptr && !dynamic_metadata->fields().empty()) { + callbacks_->streamInfo().setDynamicMetadata(HttpFilterNames::get().RateLimit, + *dynamic_metadata); + } + if (status == Filters::Common::RateLimit::LimitStatus::OverLimit && config_->runtime().snapshot().featureEnabled("ratelimit.http_filter_enforcing", 100)) { state_ = State::Responded; diff --git a/source/extensions/filters/http/ratelimit/ratelimit.h b/source/extensions/filters/http/ratelimit/ratelimit.h index 5623cd2a9840..e555f566bec1 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.h +++ b/source/extensions/filters/http/ratelimit/ratelimit.h @@ -149,7 +149,8 @@ class Filter : public Http::StreamFilter, public Filters::Common::RateLimit::Req Filters::Common::RateLimit::DescriptorStatusListPtr&& descriptor_statuses, Http::ResponseHeaderMapPtr&& response_headers_to_add, Http::RequestHeaderMapPtr&& request_headers_to_add, - const std::string& response_body) override; + const std::string& response_body, + Filters::Common::RateLimit::DynamicMetadataPtr&& dynamic_metadata) override; private: void initiateCall(const Http::RequestHeaderMap& headers); diff --git a/source/extensions/filters/network/ratelimit/BUILD b/source/extensions/filters/network/ratelimit/BUILD index 035bfad5b6f7..4cbe47dcbd01 100644 --- a/source/extensions/filters/network/ratelimit/BUILD +++ b/source/extensions/filters/network/ratelimit/BUILD @@ -30,6 +30,7 @@ envoy_cc_library( "//include/envoy/stats:stats_macros", "//source/common/tracing:http_tracer_lib", "//source/extensions/filters/common/ratelimit:ratelimit_client_interface", + "//source/extensions/filters/network:well_known_names", "@envoy_api//envoy/extensions/filters/network/ratelimit/v3:pkg_cc_proto", ], ) diff --git a/source/extensions/filters/network/ratelimit/ratelimit.cc b/source/extensions/filters/network/ratelimit/ratelimit.cc index 01d1ed73324e..f8df0c4f71e3 100644 --- a/source/extensions/filters/network/ratelimit/ratelimit.cc +++ b/source/extensions/filters/network/ratelimit/ratelimit.cc @@ -9,6 +9,8 @@ #include "common/common/fmt.h" #include "common/tracing/http_tracer_impl.h" +#include "extensions/filters/network/well_known_names.h" + namespace Envoy { namespace Extensions { namespace NetworkFilters { @@ -72,8 +74,8 @@ void Filter::onEvent(Network::ConnectionEvent event) { void Filter::complete(Filters::Common::RateLimit::LimitStatus status, Filters::Common::RateLimit::DescriptorStatusListPtr&&, - Http::ResponseHeaderMapPtr&&, Http::RequestHeaderMapPtr&&, - const std::string&) { + Http::ResponseHeaderMapPtr&&, Http::RequestHeaderMapPtr&&, const std::string&, + Filters::Common::RateLimit::DynamicMetadataPtr&& dynamic_metadata) { status_ = Status::Complete; config_->stats().active_.dec(); @@ -89,6 +91,11 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status, break; } + if (dynamic_metadata != nullptr && !dynamic_metadata->fields().empty()) { + filter_callbacks_->connection().streamInfo().setDynamicMetadata( + NetworkFilterNames::get().RateLimit, *dynamic_metadata); + } + if (status == Filters::Common::RateLimit::LimitStatus::OverLimit && config_->runtime().snapshot().featureEnabled("ratelimit.tcp_filter_enforcing", 100)) { config_->stats().cx_closed_.inc(); diff --git a/source/extensions/filters/network/ratelimit/ratelimit.h b/source/extensions/filters/network/ratelimit/ratelimit.h index d1029bfbf295..168be8008ff5 100644 --- a/source/extensions/filters/network/ratelimit/ratelimit.h +++ b/source/extensions/filters/network/ratelimit/ratelimit.h @@ -95,7 +95,8 @@ class Filter : public Network::ReadFilter, Filters::Common::RateLimit::DescriptorStatusListPtr&& descriptor_statuses, Http::ResponseHeaderMapPtr&& response_headers_to_add, Http::RequestHeaderMapPtr&& request_headers_to_add, - const std::string& response_body) override; + const std::string& response_body, + Filters::Common::RateLimit::DynamicMetadataPtr&& dynamic_metadata) override; private: enum class Status { NotStarted, Calling, Complete }; diff --git a/source/extensions/filters/network/thrift_proxy/filters/BUILD b/source/extensions/filters/network/thrift_proxy/filters/BUILD index a1b91d286809..36e94c0a648c 100644 --- a/source/extensions/filters/network/thrift_proxy/filters/BUILD +++ b/source/extensions/filters/network/thrift_proxy/filters/BUILD @@ -39,6 +39,7 @@ envoy_cc_library( "//source/extensions/filters/network/thrift_proxy:decoder_events_lib", "//source/extensions/filters/network/thrift_proxy:protocol_interface", "//source/extensions/filters/network/thrift_proxy:thrift_lib", + "//source/extensions/filters/network/thrift_proxy/filters:well_known_names", "//source/extensions/filters/network/thrift_proxy/router:router_interface", ], ) diff --git a/source/extensions/filters/network/thrift_proxy/filters/ratelimit/BUILD b/source/extensions/filters/network/thrift_proxy/filters/ratelimit/BUILD index 02425a8b069e..9cec57074740 100644 --- a/source/extensions/filters/network/thrift_proxy/filters/ratelimit/BUILD +++ b/source/extensions/filters/network/thrift_proxy/filters/ratelimit/BUILD @@ -22,6 +22,7 @@ envoy_cc_library( "//source/extensions/filters/common/ratelimit:stat_names_lib", "//source/extensions/filters/network/thrift_proxy:app_exception_lib", "//source/extensions/filters/network/thrift_proxy/filters:pass_through_filter_lib", + "//source/extensions/filters/network/thrift_proxy/filters:well_known_names", "//source/extensions/filters/network/thrift_proxy/router:router_ratelimit_interface", "@envoy_api//envoy/extensions/filters/network/thrift_proxy/filters/ratelimit/v3:pkg_cc_proto", ], diff --git a/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.cc b/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.cc index a9cf61aca3e3..99db7bcf31b9 100644 --- a/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.cc +++ b/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.cc @@ -3,6 +3,7 @@ #include "common/tracing/http_tracer_impl.h" #include "extensions/filters/network/thrift_proxy/app_exception_impl.h" +#include "extensions/filters/network/thrift_proxy/filters/well_known_names.h" #include "extensions/filters/network/thrift_proxy/router/router.h" #include "extensions/filters/network/thrift_proxy/router/router_ratelimit.h" @@ -62,7 +63,8 @@ void Filter::onDestroy() { void Filter::complete(Filters::Common::RateLimit::LimitStatus status, Filters::Common::RateLimit::DescriptorStatusListPtr&& descriptor_statuses, Http::ResponseHeaderMapPtr&& response_headers_to_add, - Http::RequestHeaderMapPtr&& request_headers_to_add, const std::string&) { + Http::RequestHeaderMapPtr&& request_headers_to_add, const std::string&, + Filters::Common::RateLimit::DynamicMetadataPtr&& dynamic_metadata) { // TODO(zuercher): Store headers to append to a response. Adding them to a local reply (over // limit or error) is a matter of modifying the callbacks to allow it. Adding them to an upstream // response requires either response (aka encoder) filters or some other mechanism. @@ -103,6 +105,11 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status, break; } + if (dynamic_metadata != nullptr && !dynamic_metadata->fields().empty()) { + decoder_callbacks_->streamInfo().setDynamicMetadata( + ThriftProxy::ThriftFilters::ThriftFilterNames::get().RATE_LIMIT, *dynamic_metadata); + } + if (!initiating_call_) { decoder_callbacks_->continueDecoding(); } diff --git a/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.h b/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.h index abecfb38ac56..8fe178752c12 100644 --- a/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.h +++ b/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.h @@ -80,7 +80,8 @@ class Filter : public ThriftProxy::ThriftFilters::PassThroughDecoderFilter, Filters::Common::RateLimit::DescriptorStatusListPtr&& descriptor_statuses, Http::ResponseHeaderMapPtr&& response_headers_to_add, Http::RequestHeaderMapPtr&& request_headers_to_add, - const std::string& response_body) override; + const std::string& response_body, + Filters::Common::RateLimit::DynamicMetadataPtr&& dynamic_metadata) override; private: void initiateCall(const ThriftProxy::MessageMetadata& metadata); diff --git a/test/extensions/filters/common/ratelimit/ratelimit_impl_test.cc b/test/extensions/filters/common/ratelimit/ratelimit_impl_test.cc index 1d514e0673c1..c4cd2b9d420e 100644 --- a/test/extensions/filters/common/ratelimit/ratelimit_impl_test.cc +++ b/test/extensions/filters/common/ratelimit/ratelimit_impl_test.cc @@ -39,16 +39,16 @@ class MockRequestCallbacks : public RequestCallbacks { void complete(LimitStatus status, DescriptorStatusListPtr&& descriptor_statuses, Http::ResponseHeaderMapPtr&& response_headers_to_add, Http::RequestHeaderMapPtr&& request_headers_to_add, - const std::string& response_body) override { + const std::string& response_body, DynamicMetadataPtr&& dynamic_metadata) override { complete_(status, descriptor_statuses.get(), response_headers_to_add.get(), - request_headers_to_add.get(), response_body); + request_headers_to_add.get(), response_body, dynamic_metadata.get()); } MOCK_METHOD(void, complete_, (LimitStatus status, const DescriptorStatusList* descriptor_statuses, const Http::ResponseHeaderMap* response_headers_to_add, const Http::RequestHeaderMap* request_headers_to_add, - const std::string& response_body)); + const std::string& response_body, const ProtobufWkt::Struct* dynamic_metadata)); }; class RateLimitGrpcClientTest : public testing::Test { @@ -93,7 +93,7 @@ TEST_F(RateLimitGrpcClientTest, Basic) { response = std::make_unique(); response->set_overall_code(envoy::service::ratelimit::v3::RateLimitResponse::OVER_LIMIT); EXPECT_CALL(span_, setTag(Eq("ratelimit_status"), Eq("over_limit"))); - EXPECT_CALL(request_callbacks_, complete_(LimitStatus::OverLimit, _, _, _, _)); + EXPECT_CALL(request_callbacks_, complete_(LimitStatus::OverLimit, _, _, _, _, _)); client_.onSuccess(std::move(response), span_); } @@ -112,7 +112,7 @@ TEST_F(RateLimitGrpcClientTest, Basic) { response = std::make_unique(); response->set_overall_code(envoy::service::ratelimit::v3::RateLimitResponse::OK); EXPECT_CALL(span_, setTag(Eq("ratelimit_status"), Eq("ok"))); - EXPECT_CALL(request_callbacks_, complete_(LimitStatus::OK, _, _, _, _)); + EXPECT_CALL(request_callbacks_, complete_(LimitStatus::OK, _, _, _, _, _)); client_.onSuccess(std::move(response), span_); } @@ -129,7 +129,7 @@ TEST_F(RateLimitGrpcClientTest, Basic) { Tracing::NullSpan::instance(), stream_info_); response = std::make_unique(); - EXPECT_CALL(request_callbacks_, complete_(LimitStatus::Error, _, _, _, _)); + EXPECT_CALL(request_callbacks_, complete_(LimitStatus::Error, _, _, _, _, _)); client_.onFailure(Grpc::Status::Unknown, "", span_); } @@ -152,7 +152,7 @@ TEST_F(RateLimitGrpcClientTest, Basic) { response = std::make_unique(); response->set_overall_code(envoy::service::ratelimit::v3::RateLimitResponse::OK); EXPECT_CALL(span_, setTag(Eq("ratelimit_status"), Eq("ok"))); - EXPECT_CALL(request_callbacks_, complete_(LimitStatus::OK, _, _, _, _)); + EXPECT_CALL(request_callbacks_, complete_(LimitStatus::OK, _, _, _, _, _)); client_.onSuccess(std::move(response), span_); } } diff --git a/test/extensions/filters/http/ratelimit/ratelimit_test.cc b/test/extensions/filters/http/ratelimit/ratelimit_test.cc index 25462a3ce725..394f85991d9a 100644 --- a/test/extensions/filters/http/ratelimit/ratelimit_test.cc +++ b/test/extensions/filters/http/ratelimit/ratelimit_test.cc @@ -234,7 +234,7 @@ TEST_F(HttpRateLimitFilterTest, OkResponse) { setResponseFlag(StreamInfo::ResponseFlag::RateLimited)) .Times(0); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); EXPECT_EQ( 1U, filter_callbacks_.clusterInfo()->statsScope().counterFromStatName(ratelimit_ok_).value()); @@ -285,7 +285,8 @@ TEST_F(HttpRateLimitFilterTest, OkResponseWithHeaders) { request_callbacks_->complete( Filters::Common::RateLimit::LimitStatus::OK, nullptr, Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl(*rl_headers)}, - Http::RequestHeaderMapPtr{new Http::TestRequestHeaderMapImpl(*request_headers_to_add)}, ""); + Http::RequestHeaderMapPtr{new Http::TestRequestHeaderMapImpl(*request_headers_to_add)}, "", + nullptr); Http::TestResponseHeaderMapImpl expected_headers(*rl_headers); Http::TestResponseHeaderMapImpl response_headers; EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, false)); @@ -341,7 +342,7 @@ TEST_F(HttpRateLimitFilterTest, OkResponseWithFilterHeaders) { auto descriptor_statuses_ptr = std::make_unique(descriptor_statuses); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OK, - std::move(descriptor_statuses_ptr), nullptr, nullptr, ""); + std::move(descriptor_statuses_ptr), nullptr, nullptr, "", nullptr); Http::TestResponseHeaderMapImpl expected_headers{ {"x-ratelimit-limit", "1, 1;w=60;name=\"first\", 4;w=3600;name=\"second\""}, @@ -368,7 +369,7 @@ TEST_F(HttpRateLimitFilterTest, ImmediateOkResponse) { .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void { callbacks.complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -399,7 +400,7 @@ TEST_F(HttpRateLimitFilterTest, ImmediateErrorResponse) { .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void { callbacks.complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -438,7 +439,7 @@ TEST_F(HttpRateLimitFilterTest, ErrorResponse) { EXPECT_CALL(filter_callbacks_, continueDecoding()); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data_, false)); EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers_)); @@ -471,7 +472,7 @@ TEST_F(HttpRateLimitFilterTest, ErrorResponseWithFailureModeAllowOff) { filter_->decodeHeaders(request_headers_, false)); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); EXPECT_CALL(filter_callbacks_.stream_info_, setResponseFlag(StreamInfo::ResponseFlag::RateLimitServiceError)) @@ -512,7 +513,7 @@ TEST_F(HttpRateLimitFilterTest, LimitResponse) { setResponseFlag(StreamInfo::ResponseFlag::RateLimited)); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OverLimit, nullptr, - std::move(h), nullptr, ""); + std::move(h), nullptr, "", nullptr); EXPECT_EQ(1U, filter_callbacks_.clusterInfo() ->statsScope() @@ -564,7 +565,7 @@ TEST_F(HttpRateLimitFilterTest, LimitResponseWithHeaders) { Http::ResponseHeaderMapPtr h{new Http::TestResponseHeaderMapImpl(*rl_headers)}; Http::RequestHeaderMapPtr uh{new Http::TestRequestHeaderMapImpl(*request_headers_to_add)}; request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OverLimit, nullptr, - std::move(h), std::move(uh), ""); + std::move(h), std::move(uh), "", nullptr); EXPECT_THAT(*request_headers_to_add, Not(IsSubsetOfHeaders(request_headers_))); EXPECT_EQ(1U, filter_callbacks_.clusterInfo() @@ -627,7 +628,7 @@ TEST_F(HttpRateLimitFilterTest, LimitResponseWithBody) { Http::ResponseHeaderMapPtr h{new Http::TestResponseHeaderMapImpl(*rl_headers)}; Http::RequestHeaderMapPtr uh{new Http::TestRequestHeaderMapImpl(*request_headers_to_add)}; request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OverLimit, nullptr, - std::move(h), std::move(uh), response_body); + std::move(h), std::move(uh), response_body, nullptr); EXPECT_THAT(*request_headers_to_add, Not(IsSubsetOfHeaders(request_headers_))); EXPECT_EQ(1U, filter_callbacks_.clusterInfo() @@ -696,7 +697,7 @@ TEST_F(HttpRateLimitFilterTest, LimitResponseWithBodyAndContentType) { Http::ResponseHeaderMapPtr h{new Http::TestResponseHeaderMapImpl(*rl_headers)}; Http::RequestHeaderMapPtr uh{new Http::TestRequestHeaderMapImpl(*request_headers_to_add)}; request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OverLimit, nullptr, - std::move(h), std::move(uh), response_body); + std::move(h), std::move(uh), response_body, nullptr); EXPECT_THAT(*request_headers_to_add, Not(IsSubsetOfHeaders(request_headers_))); EXPECT_EQ(1U, filter_callbacks_.clusterInfo() @@ -750,7 +751,7 @@ TEST_F(HttpRateLimitFilterTest, LimitResponseWithFilterHeaders) { auto descriptor_statuses_ptr = std::make_unique(descriptor_statuses); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OverLimit, - std::move(descriptor_statuses_ptr), nullptr, nullptr, ""); + std::move(descriptor_statuses_ptr), nullptr, nullptr, "", nullptr); EXPECT_EQ(1U, filter_callbacks_.clusterInfo() ->statsScope() .counterFromStatName(ratelimit_over_limit_) @@ -786,7 +787,7 @@ TEST_F(HttpRateLimitFilterTest, LimitResponseWithoutEnvoyRateLimitedHeader) { setResponseFlag(StreamInfo::ResponseFlag::RateLimited)); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OverLimit, nullptr, - std::move(h), nullptr, ""); + std::move(h), nullptr, "", nullptr); EXPECT_EQ(1U, filter_callbacks_.clusterInfo() ->statsScope() @@ -821,7 +822,7 @@ TEST_F(HttpRateLimitFilterTest, LimitResponseRuntimeDisabled) { EXPECT_CALL(filter_callbacks_, continueDecoding()); Http::ResponseHeaderMapPtr h{new Http::TestResponseHeaderMapImpl()}; request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OverLimit, nullptr, - std::move(h), nullptr, ""); + std::move(h), nullptr, "", nullptr); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data_, false)); EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers_)); @@ -971,7 +972,7 @@ TEST_F(HttpRateLimitFilterTest, InternalRequestType) { .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void { callbacks.complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -1015,7 +1016,7 @@ TEST_F(HttpRateLimitFilterTest, ExternalRequestType) { .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void { callbacks.complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -1069,7 +1070,7 @@ TEST_F(HttpRateLimitFilterTest, DEPRECATED_FEATURE_TEST(ExcludeVirtualHost)) { .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void { callbacks.complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -1120,7 +1121,7 @@ TEST_F(HttpRateLimitFilterTest, OverrideVHRateLimitOptionWithRouteRateLimitSet) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void { callbacks.complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -1171,7 +1172,7 @@ TEST_F(HttpRateLimitFilterTest, OverrideVHRateLimitOptionWithoutRouteRateLimit) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void { callbacks.complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -1219,7 +1220,7 @@ TEST_F(HttpRateLimitFilterTest, IncludeVHRateLimitOptionWithOnlyVHRateLimitSet) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void { callbacks.complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -1269,7 +1270,7 @@ TEST_F(HttpRateLimitFilterTest, IncludeVHRateLimitOptionWithRouteAndVHRateLimitS .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void { callbacks.complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -1317,7 +1318,7 @@ TEST_F(HttpRateLimitFilterTest, IgnoreVHRateLimitOptionWithRouteRateLimitSet) { .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void { callbacks.complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); diff --git a/test/extensions/filters/network/ratelimit/ratelimit_test.cc b/test/extensions/filters/network/ratelimit/ratelimit_test.cc index 019ad7000909..7bb99df6a6bb 100644 --- a/test/extensions/filters/network/ratelimit/ratelimit_test.cc +++ b/test/extensions/filters/network/ratelimit/ratelimit_test.cc @@ -115,7 +115,7 @@ TEST_F(RateLimitFilterTest, OK) { EXPECT_CALL(filter_callbacks_, continueReading()); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); @@ -143,7 +143,7 @@ TEST_F(RateLimitFilterTest, OverLimit) { EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::NoFlush)); EXPECT_CALL(*client_, cancel()).Times(0); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OverLimit, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); @@ -172,7 +172,7 @@ TEST_F(RateLimitFilterTest, OverLimitNotEnforcing) { EXPECT_CALL(*client_, cancel()).Times(0); EXPECT_CALL(filter_callbacks_, continueReading()); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OverLimit, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); @@ -197,7 +197,7 @@ TEST_F(RateLimitFilterTest, Error) { EXPECT_CALL(filter_callbacks_, continueReading()); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); @@ -238,7 +238,7 @@ TEST_F(RateLimitFilterTest, ImmediateOK) { .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void { callbacks.complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); }))); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); @@ -262,7 +262,7 @@ TEST_F(RateLimitFilterTest, ImmediateError) { .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void { callbacks.complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); }))); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); @@ -305,7 +305,7 @@ TEST_F(RateLimitFilterTest, ErrorResponseWithFailureModeAllowOff) { Buffer::OwnedImpl data("hello"); EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data, false)); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); diff --git a/test/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit_test.cc b/test/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit_test.cc index c56c9383b330..09c33d467d49 100644 --- a/test/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit_test.cc +++ b/test/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit_test.cc @@ -227,7 +227,7 @@ TEST_F(ThriftRateLimitFilterTest, OkResponse) { setResponseFlag(StreamInfo::ResponseFlag::RateLimited)) .Times(0); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("ratelimit.ok").value()); @@ -247,7 +247,7 @@ TEST_F(ThriftRateLimitFilterTest, ImmediateOkResponse) { .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void { callbacks.complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -271,7 +271,7 @@ TEST_F(ThriftRateLimitFilterTest, ImmediateErrorResponse) { .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void { callbacks.complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -301,7 +301,7 @@ TEST_F(ThriftRateLimitFilterTest, ErrorResponse) { EXPECT_CALL(filter_callbacks_, continueDecoding()); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); EXPECT_EQ(ThriftProxy::FilterStatus::Continue, filter_->messageEnd()); EXPECT_CALL(filter_callbacks_.stream_info_, @@ -339,7 +339,7 @@ TEST_F(ThriftRateLimitFilterTest, ErrorResponseWithFailureModeAllowOff) { EXPECT_CALL(filter_callbacks_.stream_info_, setResponseFlag(StreamInfo::ResponseFlag::RateLimitServiceError)); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); EXPECT_EQ( 1U, @@ -373,7 +373,7 @@ TEST_F(ThriftRateLimitFilterTest, LimitResponse) { EXPECT_CALL(filter_callbacks_.stream_info_, setResponseFlag(StreamInfo::ResponseFlag::RateLimited)); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OverLimit, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("ratelimit.over_limit") @@ -406,7 +406,7 @@ TEST_F(ThriftRateLimitFilterTest, LimitResponseWithHeaders) { Http::ResponseHeaderMapPtr h{new Http::TestResponseHeaderMapImpl(*rl_headers)}; request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OverLimit, nullptr, - std::move(h), nullptr, ""); + std::move(h), nullptr, "", nullptr); EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("ratelimit.over_limit") @@ -431,7 +431,7 @@ TEST_F(ThriftRateLimitFilterTest, LimitResponseRuntimeDisabled) { .WillOnce(Return(false)); EXPECT_CALL(filter_callbacks_, continueDecoding()); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OverLimit, nullptr, nullptr, - nullptr, ""); + nullptr, "", nullptr); EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("ratelimit.over_limit") From d06e749687eaa720f54f2f78817413e8acef290d Mon Sep 17 00:00:00 2001 From: John Esmet Date: Tue, 22 Dec 2020 20:12:46 +0000 Subject: [PATCH 02/11] Add unit test for HTTP ratelimit for dynamic metadata Signed-off-by: John Esmet --- .../filters/http/ratelimit/ratelimit_test.cc | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/test/extensions/filters/http/ratelimit/ratelimit_test.cc b/test/extensions/filters/http/ratelimit/ratelimit_test.cc index 394f85991d9a..06feeac7954d 100644 --- a/test/extensions/filters/http/ratelimit/ratelimit_test.cc +++ b/test/extensions/filters/http/ratelimit/ratelimit_test.cc @@ -528,6 +528,58 @@ TEST_F(HttpRateLimitFilterTest, LimitResponse) { EXPECT_EQ("request_rate_limited", filter_callbacks_.details()); } +TEST_F(HttpRateLimitFilterTest, LimitResponseWithDynamicMetadata) { + SetUpTest(filter_config_); + InSequence s; + + EXPECT_CALL(route_rate_limit_, populateDescriptors(_, _, _, _, _, _)) + .WillOnce(SetArgReferee<1>(descriptor_)); + EXPECT_CALL(*client_, limit(_, _, _, _, _)) + .WillOnce( + WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void { + request_callbacks_ = &callbacks; + }))); + + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_->decodeHeaders(request_headers_, false)); + + Filters::Common::RateLimit::DynamicMetadataPtr dynamic_metadata = + std::make_unique(); + auto* fields = dynamic_metadata->mutable_fields(); + (*fields)["name"] = ValueUtil::stringValue("my-limit"); + (*fields)["x"] = ValueUtil::numberValue(3); + EXPECT_CALL(filter_callbacks_.stream_info_, setDynamicMetadata(_, _)) + .WillOnce(Invoke([&dynamic_metadata](const std::string& ns, + const ProtobufWkt::Struct& returned_dynamic_metadata) { + EXPECT_EQ(ns, HttpFilterNames::get().RateLimit); + EXPECT_TRUE(TestUtility::protoEqual(returned_dynamic_metadata, *dynamic_metadata)); + })); + + Http::ResponseHeaderMapPtr h{new Http::TestResponseHeaderMapImpl()}; + Http::TestResponseHeaderMapImpl response_headers{ + {":status", "429"}, + {"x-envoy-ratelimited", Http::Headers::get().EnvoyRateLimitedValues.True}}; + EXPECT_CALL(filter_callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), true)); + EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); + EXPECT_CALL(filter_callbacks_.stream_info_, + setResponseFlag(StreamInfo::ResponseFlag::RateLimited)); + + request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OverLimit, nullptr, + std::move(h), nullptr, "", std::move(dynamic_metadata)); + + EXPECT_EQ(1U, filter_callbacks_.clusterInfo() + ->statsScope() + .counterFromStatName(ratelimit_over_limit_) + .value()); + EXPECT_EQ( + 1U, + filter_callbacks_.clusterInfo()->statsScope().counterFromStatName(upstream_rq_4xx_).value()); + EXPECT_EQ( + 1U, + filter_callbacks_.clusterInfo()->statsScope().counterFromStatName(upstream_rq_429_).value()); + EXPECT_EQ("request_rate_limited", filter_callbacks_.details()); +} + TEST_F(HttpRateLimitFilterTest, LimitResponseWithHeaders) { SetUpTest(filter_config_); InSequence s; From 836fa900701ea9a3e24d30fce23e5faa5b367ad7 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Tue, 22 Dec 2020 20:44:32 +0000 Subject: [PATCH 03/11] Add unit test for network ratelimit with dynamic metadata Signed-off-by: John Esmet --- .../filters/network/ratelimit/BUILD | 2 + .../network/ratelimit/ratelimit_test.cc | 42 +++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/test/extensions/filters/network/ratelimit/BUILD b/test/extensions/filters/network/ratelimit/BUILD index 99a0f31eb85b..a6c072c91182 100644 --- a/test/extensions/filters/network/ratelimit/BUILD +++ b/test/extensions/filters/network/ratelimit/BUILD @@ -19,11 +19,13 @@ envoy_extension_cc_test( "//source/common/buffer:buffer_lib", "//source/common/event:dispatcher_lib", "//source/common/stats:stats_lib", + "//source/extensions/filters/network:well_known_names", "//source/extensions/filters/network/ratelimit:ratelimit_lib", "//test/extensions/filters/common/ratelimit:ratelimit_mocks", "//test/mocks/network:network_mocks", "//test/mocks/ratelimit:ratelimit_mocks", "//test/mocks/runtime:runtime_mocks", + "//test/mocks/stream_info:stream_info_mocks", "//test/mocks/tracing:tracing_mocks", "@envoy_api//envoy/extensions/filters/network/ratelimit/v3:pkg_cc_proto", ], diff --git a/test/extensions/filters/network/ratelimit/ratelimit_test.cc b/test/extensions/filters/network/ratelimit/ratelimit_test.cc index 7bb99df6a6bb..d0a5741b5c2d 100644 --- a/test/extensions/filters/network/ratelimit/ratelimit_test.cc +++ b/test/extensions/filters/network/ratelimit/ratelimit_test.cc @@ -8,11 +8,13 @@ #include "common/buffer/buffer_impl.h" #include "extensions/filters/network/ratelimit/ratelimit.h" +#include "extensions/filters/network/well_known_names.h" #include "test/extensions/filters/common/ratelimit/mocks.h" #include "test/mocks/network/mocks.h" #include "test/mocks/ratelimit/mocks.h" #include "test/mocks/runtime/mocks.h" +#include "test/mocks/stream_info/mocks.h" #include "test/mocks/tracing/mocks.h" #include "test/test_common/printers.h" @@ -152,6 +154,46 @@ TEST_F(RateLimitFilterTest, OverLimit) { EXPECT_EQ(1U, stats_store_.counter("ratelimit.name.cx_closed").value()); } +TEST_F(RateLimitFilterTest, OverLimitWithDynamicMetadata) { + InSequence s; + SetUpTest(filter_config_); + + EXPECT_CALL(*client_, limit(_, "foo", _, _, _)) + .WillOnce( + WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void { + request_callbacks_ = &callbacks; + }))); + + EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onNewConnection()); + Buffer::OwnedImpl data("hello"); + EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data, false)); + + Filters::Common::RateLimit::DynamicMetadataPtr dynamic_metadata = + std::make_unique(); + auto* fields = dynamic_metadata->mutable_fields(); + (*fields)["name"] = ValueUtil::stringValue("my-limit"); + (*fields)["x"] = ValueUtil::numberValue(3); + NiceMock stream_info; + EXPECT_CALL(filter_callbacks_.connection_, streamInfo()).WillOnce(ReturnRef(stream_info)); + EXPECT_CALL(stream_info, setDynamicMetadata(_, _)) + .WillOnce(Invoke([&dynamic_metadata](const std::string& ns, + const ProtobufWkt::Struct& returned_dynamic_metadata) { + EXPECT_EQ(ns, NetworkFilterNames::get().RateLimit); + EXPECT_TRUE(TestUtility::protoEqual(returned_dynamic_metadata, *dynamic_metadata)); + })); + + EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::NoFlush)); + EXPECT_CALL(*client_, cancel()).Times(0); + request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OverLimit, nullptr, nullptr, + nullptr, "", std::move(dynamic_metadata)); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); + + EXPECT_EQ(1U, stats_store_.counter("ratelimit.name.total").value()); + EXPECT_EQ(1U, stats_store_.counter("ratelimit.name.over_limit").value()); + EXPECT_EQ(1U, stats_store_.counter("ratelimit.name.cx_closed").value()); +} + TEST_F(RateLimitFilterTest, OverLimitNotEnforcing) { InSequence s; SetUpTest(filter_config_); From 77e6cc7ef6b148766c69ccbca74834aff1461692 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Tue, 22 Dec 2020 21:56:38 +0000 Subject: [PATCH 04/11] Add a thrift proxy test. Move dynamic metadata setting to higher up in every implementation for consistency. Signed-off-by: John Esmet --- .../filters/http/ratelimit/ratelimit.cc | 10 ++--- .../filters/network/ratelimit/ratelimit.cc | 10 ++--- .../filters/ratelimit/ratelimit.cc | 10 ++--- .../thrift_proxy/filters/ratelimit/BUILD | 1 + .../filters/ratelimit/ratelimit_test.cc | 44 +++++++++++++++++++ 5 files changed, 60 insertions(+), 15 deletions(-) diff --git a/source/extensions/filters/http/ratelimit/ratelimit.cc b/source/extensions/filters/http/ratelimit/ratelimit.cc index a7565d3cc4db..9be10f2c63a7 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.cc +++ b/source/extensions/filters/http/ratelimit/ratelimit.cc @@ -152,6 +152,11 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status, Stats::StatName empty_stat_name; Filters::Common::RateLimit::StatNames& stat_names = config_->statNames(); + if (dynamic_metadata != nullptr && !dynamic_metadata->fields().empty()) { + callbacks_->streamInfo().setDynamicMetadata(HttpFilterNames::get().RateLimit, + *dynamic_metadata); + } + switch (status) { case Filters::Common::RateLimit::LimitStatus::OK: cluster_->statsScope().counterFromStatName(stat_names.ok_).inc(); @@ -193,11 +198,6 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status, descriptor_statuses = nullptr; } - if (dynamic_metadata != nullptr && !dynamic_metadata->fields().empty()) { - callbacks_->streamInfo().setDynamicMetadata(HttpFilterNames::get().RateLimit, - *dynamic_metadata); - } - if (status == Filters::Common::RateLimit::LimitStatus::OverLimit && config_->runtime().snapshot().featureEnabled("ratelimit.http_filter_enforcing", 100)) { state_ = State::Responded; diff --git a/source/extensions/filters/network/ratelimit/ratelimit.cc b/source/extensions/filters/network/ratelimit/ratelimit.cc index f8df0c4f71e3..1ac7accd7d11 100644 --- a/source/extensions/filters/network/ratelimit/ratelimit.cc +++ b/source/extensions/filters/network/ratelimit/ratelimit.cc @@ -76,6 +76,11 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status, Filters::Common::RateLimit::DescriptorStatusListPtr&&, Http::ResponseHeaderMapPtr&&, Http::RequestHeaderMapPtr&&, const std::string&, Filters::Common::RateLimit::DynamicMetadataPtr&& dynamic_metadata) { + if (dynamic_metadata != nullptr && !dynamic_metadata->fields().empty()) { + filter_callbacks_->connection().streamInfo().setDynamicMetadata( + NetworkFilterNames::get().RateLimit, *dynamic_metadata); + } + status_ = Status::Complete; config_->stats().active_.dec(); @@ -91,11 +96,6 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status, break; } - if (dynamic_metadata != nullptr && !dynamic_metadata->fields().empty()) { - filter_callbacks_->connection().streamInfo().setDynamicMetadata( - NetworkFilterNames::get().RateLimit, *dynamic_metadata); - } - if (status == Filters::Common::RateLimit::LimitStatus::OverLimit && config_->runtime().snapshot().featureEnabled("ratelimit.tcp_filter_enforcing", 100)) { config_->stats().cx_closed_.inc(); diff --git a/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.cc b/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.cc index 99db7bcf31b9..0328d484dc21 100644 --- a/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.cc +++ b/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.cc @@ -72,6 +72,11 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status, UNREFERENCED_PARAMETER(response_headers_to_add); UNREFERENCED_PARAMETER(request_headers_to_add); + if (dynamic_metadata != nullptr && !dynamic_metadata->fields().empty()) { + decoder_callbacks_->streamInfo().setDynamicMetadata( + ThriftProxy::ThriftFilters::ThriftFilterNames::get().RATE_LIMIT, *dynamic_metadata); + } + state_ = State::Complete; Filters::Common::RateLimit::StatNames& stat_names = config_->statNames(); @@ -105,11 +110,6 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status, break; } - if (dynamic_metadata != nullptr && !dynamic_metadata->fields().empty()) { - decoder_callbacks_->streamInfo().setDynamicMetadata( - ThriftProxy::ThriftFilters::ThriftFilterNames::get().RATE_LIMIT, *dynamic_metadata); - } - if (!initiating_call_) { decoder_callbacks_->continueDecoding(); } diff --git a/test/extensions/filters/network/thrift_proxy/filters/ratelimit/BUILD b/test/extensions/filters/network/thrift_proxy/filters/ratelimit/BUILD index 8028511adca9..f3aa06ba8df0 100644 --- a/test/extensions/filters/network/thrift_proxy/filters/ratelimit/BUILD +++ b/test/extensions/filters/network/thrift_proxy/filters/ratelimit/BUILD @@ -19,6 +19,7 @@ envoy_extension_cc_test( "//source/common/buffer:buffer_lib", "//source/common/common:empty_string", "//source/common/http:headers_lib", + "//source/extensions/filters/network/thrift_proxy/filters:well_known_names", "//source/extensions/filters/network/thrift_proxy/filters/ratelimit:ratelimit_lib", "//test/extensions/filters/common/ratelimit:ratelimit_mocks", "//test/extensions/filters/network/thrift_proxy:mocks", diff --git a/test/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit_test.cc b/test/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit_test.cc index 09c33d467d49..07db070f0d77 100644 --- a/test/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit_test.cc +++ b/test/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit_test.cc @@ -10,6 +10,7 @@ #include "extensions/filters/network/thrift_proxy/app_exception_impl.h" #include "extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.h" +#include "extensions/filters/network/thrift_proxy/filters/well_known_names.h" #include "test/extensions/filters/common/ratelimit/mocks.h" #include "test/extensions/filters/network/thrift_proxy/mocks.h" @@ -316,6 +317,49 @@ TEST_F(ThriftRateLimitFilterTest, ErrorResponse) { .value()); } +TEST_F(ThriftRateLimitFilterTest, ErrorResponseWithDynamicMetadata) { + setupTest(filter_config_); + InSequence s; + + EXPECT_CALL(route_rate_limit_, populateDescriptors(_, _, _, _, _)) + .WillOnce(SetArgReferee<1>(descriptor_)); + EXPECT_CALL(*client_, limit(_, _, _, _, _)) + .WillOnce( + WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void { + request_callbacks_ = &callbacks; + }))); + + EXPECT_EQ(ThriftProxy::FilterStatus::StopIteration, filter_->messageBegin(request_metadata_)); + + Filters::Common::RateLimit::DynamicMetadataPtr dynamic_metadata = + std::make_unique(); + auto* fields = dynamic_metadata->mutable_fields(); + (*fields)["name"] = ValueUtil::stringValue("my-limit"); + (*fields)["x"] = ValueUtil::numberValue(3); + EXPECT_CALL(filter_callbacks_.stream_info_, setDynamicMetadata(_, _)) + .WillOnce(Invoke([&dynamic_metadata](const std::string& ns, + const ProtobufWkt::Struct& returned_dynamic_metadata) { + EXPECT_EQ(ns, ThriftProxy::ThriftFilters::ThriftFilterNames::get().RATE_LIMIT); + EXPECT_TRUE(TestUtility::protoEqual(returned_dynamic_metadata, *dynamic_metadata)); + })); + + EXPECT_CALL(filter_callbacks_, continueDecoding()); + request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr, + nullptr, "", std::move(dynamic_metadata)); + + EXPECT_EQ(ThriftProxy::FilterStatus::Continue, filter_->messageEnd()); + EXPECT_CALL(filter_callbacks_.stream_info_, + setResponseFlag(StreamInfo::ResponseFlag::RateLimited)) + .Times(0); + + EXPECT_EQ( + 1U, + cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("ratelimit.error").value()); + EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_ + .counter("ratelimit.failure_mode_allowed") + .value()); +} + TEST_F(ThriftRateLimitFilterTest, ErrorResponseWithFailureModeAllowOff) { setupTest(fail_close_config_); InSequence s; From 59a3bdf134b7a41d9814592d70fed93bb9ed95af Mon Sep 17 00:00:00 2001 From: John Esmet Date: Mon, 4 Jan 2021 17:49:20 +0000 Subject: [PATCH 05/11] Add changelog Signed-off-by: John Esmet --- docs/root/version_history/current.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 452cb21bcfcf..03fc6c9c86a2 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -86,6 +86,7 @@ New Features * ratelimit: added support for use of various :ref:`metadata ` as a ratelimit action. * ratelimit: added :ref:`disable_x_envoy_ratelimited_header ` option to disable `X-Envoy-RateLimited` header. * ratelimit: added :ref:`body ` field to support custom response bodies for non-OK responses from the external ratelimit service. +* ratelimit: added :ref:`dynamic_metadata ` field to support setting dynamic metadata from the ratelimit service. * router: added support for regex rewrites during HTTP redirects using :ref:`regex_rewrite `. * sds: improved support for atomic :ref:`key rotations ` and added configurable rotation triggers for :ref:`TlsCertificate ` and From 2a9f9e5e1485933318923d56fadb3107aeb1dd99 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Mon, 4 Jan 2021 22:43:31 +0000 Subject: [PATCH 06/11] Call out the Thrift filter namespace Signed-off-by: John Esmet --- api/envoy/service/ratelimit/v3/rls.proto | 1 + generated_api_shadow/envoy/service/ratelimit/v3/rls.proto | 1 + 2 files changed, 2 insertions(+) diff --git a/api/envoy/service/ratelimit/v3/rls.proto b/api/envoy/service/ratelimit/v3/rls.proto index a7dab9f29303..7b52bdab7ff5 100644 --- a/api/envoy/service/ratelimit/v3/rls.proto +++ b/api/envoy/service/ratelimit/v3/rls.proto @@ -143,5 +143,6 @@ message RateLimitResponse { // // - :ref:`envoy.filters.http.ratelimit ` for HTTP filter. // - :ref:`envoy.filters.network.ratelimit ` for network filter. + // - :ref:`envoy.filters.thrift.rate_limit ` for Thrift filter. google.protobuf.Struct dynamic_metadata = 6; } diff --git a/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto b/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto index a7dab9f29303..7b52bdab7ff5 100644 --- a/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto +++ b/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto @@ -143,5 +143,6 @@ message RateLimitResponse { // // - :ref:`envoy.filters.http.ratelimit ` for HTTP filter. // - :ref:`envoy.filters.network.ratelimit ` for network filter. + // - :ref:`envoy.filters.thrift.rate_limit ` for Thrift filter. google.protobuf.Struct dynamic_metadata = 6; } From 23308fd4bb91874866165a8621c92678abe10831 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Tue, 5 Jan 2021 21:40:15 +0000 Subject: [PATCH 07/11] Add docs links Signed-off-by: John Esmet --- api/envoy/service/ratelimit/v3/rls.proto | 2 +- .../http/http_filters/rate_limit_filter.rst | 9 +++++++++ .../listeners/network_filters/rate_limit_filter.rst | 9 +++++++++ .../other_protocols/thrift_filters/rate_limit_filter.rst | 9 +++++++++ 4 files changed, 28 insertions(+), 1 deletion(-) diff --git a/api/envoy/service/ratelimit/v3/rls.proto b/api/envoy/service/ratelimit/v3/rls.proto index 7b52bdab7ff5..0f040612ecec 100644 --- a/api/envoy/service/ratelimit/v3/rls.proto +++ b/api/envoy/service/ratelimit/v3/rls.proto @@ -143,6 +143,6 @@ message RateLimitResponse { // // - :ref:`envoy.filters.http.ratelimit ` for HTTP filter. // - :ref:`envoy.filters.network.ratelimit ` for network filter. - // - :ref:`envoy.filters.thrift.rate_limit ` for Thrift filter. + // - :ref:`envoy.filters.thrift.rate_limit ` for Thrift filter. google.protobuf.Struct dynamic_metadata = 6; } diff --git a/docs/root/configuration/http/http_filters/rate_limit_filter.rst b/docs/root/configuration/http/http_filters/rate_limit_filter.rst index 0896f9a5b86d..b1d6d6b11c5a 100644 --- a/docs/root/configuration/http/http_filters/rate_limit_filter.rst +++ b/docs/root/configuration/http/http_filters/rate_limit_filter.rst @@ -138,6 +138,15 @@ The rate limit filter outputs statistics in the *cluster.. failure_mode_allowed, Counter, "Total requests that were error(s) but were allowed through because of :ref:`failure_mode_deny ` set to false." +Dynamic Metadata +---------------- +.. _config_http_filters_ratelimit_dynamic_metadata: + +The ratelimit filter emits dynamic metadata as an opaque ``google.protobuf.Struct`` +*only* when the gRPC ratelimit service returns a :ref:`RateLimitResponse +` with a filled :ref:`dynamic_metadata +` field. + Runtime ------- diff --git a/docs/root/configuration/listeners/network_filters/rate_limit_filter.rst b/docs/root/configuration/listeners/network_filters/rate_limit_filter.rst index 1a94053f0c5a..38d8683c7a47 100644 --- a/docs/root/configuration/listeners/network_filters/rate_limit_filter.rst +++ b/docs/root/configuration/listeners/network_filters/rate_limit_filter.rst @@ -43,3 +43,12 @@ ratelimit.tcp_filter_enabled ratelimit.tcp_filter_enforcing % of connections that will call the rate limit service and enforce the decision. Defaults to 100. This can be used to test what would happen before fully enforcing the outcome. + +Dynamic Metadata +---------------- +.. _config_network_filters_ratelimit_dynamic_metadata: + +The ratelimit filter emits dynamic metadata as an opaque ``google.protobuf.Struct`` +*only* when the gRPC ratelimit service returns a :ref:`CheckResponse +` with a filled :ref:`dynamic_metadata +` field. \ No newline at end of file diff --git a/docs/root/configuration/other_protocols/thrift_filters/rate_limit_filter.rst b/docs/root/configuration/other_protocols/thrift_filters/rate_limit_filter.rst index 366059c65a0d..9bc4189a51f2 100644 --- a/docs/root/configuration/other_protocols/thrift_filters/rate_limit_filter.rst +++ b/docs/root/configuration/other_protocols/thrift_filters/rate_limit_filter.rst @@ -39,3 +39,12 @@ The filter outputs statistics in the *cluster..ratelimit.* of :ref:`failure_mode_deny ` set to false." + +Dynamic Metadata +---------------- +.. _config_thrift_filters_rate_limit_dynamic_metadata: + +The ratelimit filter emits dynamic metadata as an opaque ``google.protobuf.Struct`` +*only* when the gRPC ratelimit service returns a :ref:`CheckResponse +` with a filled :ref:`dynamic_metadata +` field. \ No newline at end of file From 42c7d0fbe34f4af54625c851d84eafa13fd6af16 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Tue, 5 Jan 2021 21:51:52 +0000 Subject: [PATCH 08/11] Fixup Signed-off-by: John Esmet --- generated_api_shadow/envoy/service/ratelimit/v3/rls.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto b/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto index 7b52bdab7ff5..0f040612ecec 100644 --- a/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto +++ b/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto @@ -143,6 +143,6 @@ message RateLimitResponse { // // - :ref:`envoy.filters.http.ratelimit ` for HTTP filter. // - :ref:`envoy.filters.network.ratelimit ` for network filter. - // - :ref:`envoy.filters.thrift.rate_limit ` for Thrift filter. + // - :ref:`envoy.filters.thrift.rate_limit ` for Thrift filter. google.protobuf.Struct dynamic_metadata = 6; } From a72ced7f9188990f394257eeaf4b2b1b1e820df8 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Wed, 6 Jan 2021 02:19:55 +0000 Subject: [PATCH 09/11] Fix one more test Signed-off-by: John Esmet --- test/common/network/filter_manager_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/network/filter_manager_impl_test.cc b/test/common/network/filter_manager_impl_test.cc index 6655c7b6388d..e8857509a554 100644 --- a/test/common/network/filter_manager_impl_test.cc +++ b/test/common/network/filter_manager_impl_test.cc @@ -416,7 +416,7 @@ stat_prefix: name .WillOnce(Return(&conn_pool)); request_callbacks->complete(Extensions::Filters::Common::RateLimit::LimitStatus::OK, nullptr, - nullptr, nullptr, ""); + nullptr, nullptr, "", nullptr); conn_pool.poolReady(upstream_connection); From 2d20194c14e12397ab339d6676967ce20f687b65 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Wed, 6 Jan 2021 13:12:29 +0000 Subject: [PATCH 10/11] Kick CI Signed-off-by: John Esmet From d9947049ea2e8b44a07c5cbd29d1cfc9317eef5c Mon Sep 17 00:00:00 2001 From: John Esmet Date: Wed, 6 Jan 2021 22:58:37 +0000 Subject: [PATCH 11/11] Guard dynamic_metadata creation with has_dynamic_metadata Signed-off-by: John Esmet --- .../extensions/filters/common/ratelimit/ratelimit_impl.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/extensions/filters/common/ratelimit/ratelimit_impl.cc b/source/extensions/filters/common/ratelimit/ratelimit_impl.cc index dd83d3117c1a..055d2e8cdd55 100644 --- a/source/extensions/filters/common/ratelimit/ratelimit_impl.cc +++ b/source/extensions/filters/common/ratelimit/ratelimit_impl.cc @@ -105,10 +105,10 @@ void GrpcClientImpl::onSuccess( DescriptorStatusListPtr descriptor_statuses = std::make_unique( response->statuses().begin(), response->statuses().end()); - // TODO(esmet): This dynamic metadata copy is probably unnecessary, but let's just following the - // existing pattern of copying parameters over as unique pointers for now. DynamicMetadataPtr dynamic_metadata = - std::make_unique(response->dynamic_metadata()); + response->has_dynamic_metadata() + ? std::make_unique(response->dynamic_metadata()) + : nullptr; callbacks_->complete(status, std::move(descriptor_statuses), std::move(response_headers_to_add), std::move(request_headers_to_add), response->raw_body(), std::move(dynamic_metadata));