From 78d33e60c649171a37a585f936ee6e8d86d4ed31 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Tue, 22 Dec 2020 15:57:25 +0000 Subject: [PATCH 1/2] ratelimit: backport raw_body and dynamic_metadata changes --- api/envoy/service/ratelimit/v2/rls.proto | 12 +++++- api/envoy/service/ratelimit/v3/rls.proto | 14 ++++++- .../envoy/service/ratelimit/v2/rls.proto | 12 +++++- .../envoy/service/ratelimit/v3/rls.proto | 14 ++++++- .../filters/common/ratelimit/ratelimit.h | 5 ++- .../common/ratelimit/ratelimit_impl.cc | 12 +++++- .../filters/http/ratelimit/ratelimit.cc | 40 +++++++++---------- .../filters/http/ratelimit/ratelimit.h | 5 ++- .../filters/network/ratelimit/BUILD | 1 + .../filters/network/ratelimit/ratelimit.cc | 13 ++++-- .../filters/network/ratelimit/ratelimit.h | 3 +- .../network/thrift_proxy/filters/BUILD | 1 + .../thrift_proxy/filters/ratelimit/BUILD | 1 + .../filters/ratelimit/ratelimit.cc | 10 ++++- .../filters/ratelimit/ratelimit.h | 3 +- .../network/filter_manager_impl_test.cc | 2 +- .../common/ratelimit/ratelimit_impl_test.cc | 16 ++++---- .../filters/http/ratelimit/ratelimit_test.cc | 28 ++++++------- .../network/ratelimit/ratelimit_test.cc | 14 +++---- 19 files changed, 139 insertions(+), 67 deletions(-) diff --git a/api/envoy/service/ratelimit/v2/rls.proto b/api/envoy/service/ratelimit/v2/rls.proto index 2038f334d870d..548422714bdd5 100644 --- a/api/envoy/service/ratelimit/v2/rls.proto +++ b/api/envoy/service/ratelimit/v2/rls.proto @@ -5,6 +5,8 @@ package envoy.service.ratelimit.v2; import "envoy/api/v2/core/base.proto"; import "envoy/api/v2/ratelimit/ratelimit.proto"; +import "google/protobuf/struct.proto"; + import "udpa/annotations/migrate.proto"; import "udpa/annotations/status.proto"; import "validate/validate.proto"; @@ -46,7 +48,7 @@ message RateLimitRequest { } // A response from a ShouldRateLimit call. -// [#next-free-field: 6] +// [#next-free-field: 7] message RateLimitResponse { enum Code { // The response code is not known. @@ -117,4 +119,12 @@ message RateLimitResponse { // A response body to send to the downstream client when the response code is not OK. string 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/api/envoy/service/ratelimit/v3/rls.proto b/api/envoy/service/ratelimit/v3/rls.proto index 22c95a90bf621..cd8ddb787d533 100644 --- a/api/envoy/service/ratelimit/v3/rls.proto +++ b/api/envoy/service/ratelimit/v3/rls.proto @@ -5,6 +5,8 @@ package envoy.service.ratelimit.v3; import "envoy/config/core/v3/base.proto"; import "envoy/extensions/common/ratelimit/v3/ratelimit.proto"; +import "google/protobuf/struct.proto"; + import "udpa/annotations/status.proto"; import "udpa/annotations/versioning.proto"; import "validate/validate.proto"; @@ -67,7 +69,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"; @@ -147,5 +149,13 @@ message RateLimitResponse { repeated config.core.v3.HeaderValue request_headers_to_add = 4; // A response body to send to the downstream client when the response code is not OK. - string body = 5; + 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/v2/rls.proto b/generated_api_shadow/envoy/service/ratelimit/v2/rls.proto index 2038f334d870d..548422714bdd5 100644 --- a/generated_api_shadow/envoy/service/ratelimit/v2/rls.proto +++ b/generated_api_shadow/envoy/service/ratelimit/v2/rls.proto @@ -5,6 +5,8 @@ package envoy.service.ratelimit.v2; import "envoy/api/v2/core/base.proto"; import "envoy/api/v2/ratelimit/ratelimit.proto"; +import "google/protobuf/struct.proto"; + import "udpa/annotations/migrate.proto"; import "udpa/annotations/status.proto"; import "validate/validate.proto"; @@ -46,7 +48,7 @@ message RateLimitRequest { } // A response from a ShouldRateLimit call. -// [#next-free-field: 6] +// [#next-free-field: 7] message RateLimitResponse { enum Code { // The response code is not known. @@ -117,4 +119,12 @@ message RateLimitResponse { // A response body to send to the downstream client when the response code is not OK. string 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 22c95a90bf621..cd8ddb787d533 100644 --- a/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto +++ b/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto @@ -5,6 +5,8 @@ package envoy.service.ratelimit.v3; import "envoy/config/core/v3/base.proto"; import "envoy/extensions/common/ratelimit/v3/ratelimit.proto"; +import "google/protobuf/struct.proto"; + import "udpa/annotations/status.proto"; import "udpa/annotations/versioning.proto"; import "validate/validate.proto"; @@ -67,7 +69,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"; @@ -147,5 +149,13 @@ message RateLimitResponse { repeated config.core.v3.HeaderValue request_headers_to_add = 4; // A response body to send to the downstream client when the response code is not OK. - string body = 5; + 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 0272273fa2e40..b172cdd84f976 100644 --- a/source/extensions/filters/common/ratelimit/ratelimit.h +++ b/source/extensions/filters/common/ratelimit/ratelimit.h @@ -30,6 +30,8 @@ enum class LimitStatus { OverLimit }; +using DynamicMetadataPtr = std::unique_ptr; + /** * Async callbacks used during limit() calls. */ @@ -43,7 +45,8 @@ class RequestCallbacks { */ virtual void complete(LimitStatus status, 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 2a78fc456d274..d9b2a19d1e588 100644 --- a/source/extensions/filters/common/ratelimit/ratelimit_impl.cc +++ b/source/extensions/filters/common/ratelimit/ratelimit_impl.cc @@ -103,15 +103,23 @@ void GrpcClientImpl::onSuccess( request_headers_to_add->addCopy(Http::LowerCaseString(h.key()), h.value()); } } + + // 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 = + response->has_dynamic_metadata() + ? std::make_unique(response->dynamic_metadata()) + : nullptr; callbacks_->complete(status, std::move(response_headers_to_add), - std::move(request_headers_to_add), response->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, ""); + callbacks_->complete(LimitStatus::Error, 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 8083b8b62662c..1e1cab83f5f96 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.cc +++ b/source/extensions/filters/http/ratelimit/ratelimit.cc @@ -12,6 +12,8 @@ #include "common/http/header_utility.h" #include "common/router/config_impl.h" +#include "extensions/filters/http/well_known_names.h" + namespace Envoy { namespace Extensions { namespace HttpFilters { @@ -99,7 +101,7 @@ Http::FilterHeadersStatus Filter::encode100ContinueHeaders(Http::ResponseHeaderM } Http::FilterHeadersStatus Filter::encodeHeaders(Http::ResponseHeaderMap& headers, bool) { - populateResponseHeaders(headers, false); + populateResponseHeaders(headers, /*from_local_reply=*/false); return Http::FilterHeadersStatus::Continue; } @@ -127,13 +129,19 @@ void Filter::onDestroy() { void Filter::complete(Filters::Common::RateLimit::LimitStatus status, 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); 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(); @@ -165,15 +173,10 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status, if (status == Filters::Common::RateLimit::LimitStatus::OverLimit && config_->runtime().snapshot().featureEnabled("ratelimit.http_filter_enforcing", 100)) { state_ = State::Responded; - // Always overwrite the content-type automatically set by sendLocalReply whenever - // we're sending back a response body here. We do this because any content-type - // coming from the ratelimit service should be treated as authoritative, and we - // must discard the default text/plain type set by default. - const bool overwrite_content_type = response_body.length() > 0; callbacks_->sendLocalReply( Http::Code::TooManyRequests, response_body, - [this, overwrite_content_type](Http::HeaderMap& headers) { - populateResponseHeaders(headers, overwrite_content_type); + [this](Http::HeaderMap& headers) { + populateResponseHeaders(headers, /*from_local_reply=*/true); }, config_->rateLimitedGrpcStatus(), RcDetails::get().RateLimited); callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::RateLimited); @@ -214,19 +217,16 @@ void Filter::populateRateLimitDescriptors(const Router::RateLimitPolicy& rate_li } } -void Filter::populateResponseHeaders(Http::HeaderMap& response_headers, - bool overwrite_content_type) { +void Filter::populateResponseHeaders(Http::HeaderMap& response_headers, bool from_local_reply) { if (response_headers_to_add_) { - // If the ratelimit service is sending back the content-type header and the caller - // wants to overwrite the existing content-type, do so here. + // If the ratelimit service is sending back the content-type header and we're + // populating response headers for a local reply, overwrite the existing + // content-type header. // - // We need to do this when using sendLocalReply because it sets the content-type header to - // text/plain whenever the response body is non-empty. This won't be correct for response - // bodies that actually have a different content-type, as expressed by the response headers - // coming from the ratelimit service. We fix it here to minimize impact surface for other - // callers of sendLocalReply that depend on the existing behavior. - if (overwrite_content_type && - (*response_headers_to_add_).get(Http::Headers::get().ContentType) != nullptr) { + // We do this because sendLocalReply initially sets content-type to text/plain + // whenever the response body is non-empty, but we want the content-type coming + // from the ratelimit service to be authoritative in this case. + if (from_local_reply && !response_headers_to_add_->getContentTypeValue().empty()) { response_headers.remove(Http::Headers::get().ContentType); } Http::HeaderUtility::addHeaders(response_headers, *response_headers_to_add_); diff --git a/source/extensions/filters/http/ratelimit/ratelimit.h b/source/extensions/filters/http/ratelimit/ratelimit.h index 5625dba9af669..1c34524854d2b 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.h +++ b/source/extensions/filters/http/ratelimit/ratelimit.h @@ -119,7 +119,8 @@ class Filter : public Http::StreamFilter, public Filters::Common::RateLimit::Req void complete(Filters::Common::RateLimit::LimitStatus status, 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); @@ -127,7 +128,7 @@ class Filter : public Http::StreamFilter, public Filters::Common::RateLimit::Req std::vector& descriptors, const Router::RouteEntry* route_entry, const Http::HeaderMap& headers) const; - void populateResponseHeaders(Http::HeaderMap& response_headers, bool overwrite_content_type); + void populateResponseHeaders(Http::HeaderMap& response_headers, bool from_local_reply); void appendRequestHeaders(Http::HeaderMapPtr& request_headers_to_add); Http::Context& httpContext() { return config_->httpContext(); } diff --git a/source/extensions/filters/network/ratelimit/BUILD b/source/extensions/filters/network/ratelimit/BUILD index 68f54558afa4c..1fd8697135ec5 100644 --- a/source/extensions/filters/network/ratelimit/BUILD +++ b/source/extensions/filters/network/ratelimit/BUILD @@ -24,6 +24,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 e3d534d6288f2..e882641b9a369 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 { @@ -69,9 +71,14 @@ void Filter::onEvent(Network::ConnectionEvent event) { } } -void Filter::complete(Filters::Common::RateLimit::LimitStatus status, Http::ResponseHeaderMapPtr&&, - Http::RequestHeaderMapPtr&&, - const std::string&) { +void Filter::complete(Filters::Common::RateLimit::LimitStatus status, + 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(); diff --git a/source/extensions/filters/network/ratelimit/ratelimit.h b/source/extensions/filters/network/ratelimit/ratelimit.h index 5aa190de84fca..f0da4855833aa 100644 --- a/source/extensions/filters/network/ratelimit/ratelimit.h +++ b/source/extensions/filters/network/ratelimit/ratelimit.h @@ -94,7 +94,8 @@ class Filter : public Network::ReadFilter, void complete(Filters::Common::RateLimit::LimitStatus status, 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 808e42dd8e989..6e4f575abb273 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 5c136b0a03533..592362e072f12 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 1e3f166df6d79..38bf500f43b82 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" @@ -59,14 +60,19 @@ void Filter::onDestroy() { void Filter::complete(Filters::Common::RateLimit::LimitStatus status, 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. 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(); 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 7a588f6bd9bc0..a3d5905c476e3 100644 --- a/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.h +++ b/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.h @@ -79,7 +79,8 @@ class Filter : public ThriftProxy::ThriftFilters::PassThroughDecoderFilter, void complete(Filters::Common::RateLimit::LimitStatus status, 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/common/network/filter_manager_impl_test.cc b/test/common/network/filter_manager_impl_test.cc index f8dea3442a22f..d1237d3fa92b3 100644 --- a/test/common/network/filter_manager_impl_test.cc +++ b/test/common/network/filter_manager_impl_test.cc @@ -415,7 +415,7 @@ stat_prefix: name .WillOnce(Return(&conn_pool)); request_callbacks->complete(Extensions::Filters::Common::RateLimit::LimitStatus::OK, nullptr, - nullptr); + nullptr, "", nullptr); conn_pool.poolReady(upstream_connection); diff --git a/test/extensions/filters/common/ratelimit/ratelimit_impl_test.cc b/test/extensions/filters/common/ratelimit/ratelimit_impl_test.cc index 27261b584bcf5..a94e82493f201 100644 --- a/test/extensions/filters/common/ratelimit/ratelimit_impl_test.cc +++ b/test/extensions/filters/common/ratelimit/ratelimit_impl_test.cc @@ -36,13 +36,15 @@ namespace { class MockRequestCallbacks : public RequestCallbacks { public: void complete(LimitStatus status, Http::ResponseHeaderMapPtr&& response_headers_to_add, - Http::RequestHeaderMapPtr&& request_headers_to_add) override { - complete_(status, response_headers_to_add.get(), request_headers_to_add.get()); + Http::RequestHeaderMapPtr&& request_headers_to_add, const std::string& body, + Filters::Common::RateLimit::DynamicMetadataPtr&& dynamic_metadata) override { + complete_(status, response_headers_to_add.get(), request_headers_to_add.get(), body, dynamic_metadata.get()); } MOCK_METHOD(void, complete_, (LimitStatus status, const Http::ResponseHeaderMap* response_headers_to_add, - const Http::RequestHeaderMap* request_headers_to_add)); + const Http::RequestHeaderMap* request_headers_to_add, const std::string& body, + const ProtobufWkt::Struct* dynamic_metadata)); }; class RateLimitGrpcClientTest : public testing::Test { @@ -85,7 +87,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_); } @@ -104,7 +106,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_); } @@ -121,7 +123,7 @@ TEST_F(RateLimitGrpcClientTest, Basic) { Tracing::NullSpan::instance()); response = std::make_unique(); - EXPECT_CALL(request_callbacks_, complete_(LimitStatus::Error, _, _)); + EXPECT_CALL(request_callbacks_, complete_(LimitStatus::Error, _, _, _, _)); client_.onFailure(Grpc::Status::Unknown, "", span_); } @@ -144,7 +146,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 d9e9efe4fb4b6..983f743f15021 100644 --- a/test/extensions/filters/http/ratelimit/ratelimit_test.cc +++ b/test/extensions/filters/http/ratelimit/ratelimit_test.cc @@ -223,7 +223,7 @@ TEST_F(HttpRateLimitFilterTest, OkResponse) { EXPECT_CALL(filter_callbacks_.stream_info_, setResponseFlag(StreamInfo::ResponseFlag::RateLimited)) .Times(0); - request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr, ""); + request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr, "", nullptr); EXPECT_EQ( 1U, filter_callbacks_.clusterInfo()->statsScope().counterFromStatName(ratelimit_ok_).value()); @@ -277,7 +277,7 @@ TEST_F(HttpRateLimitFilterTest, OkResponseWithHeaders) { Filters::Common::RateLimit::LimitStatus::OK, Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl(*rl_headers)}, 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)); @@ -302,7 +302,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); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -333,7 +333,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); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -372,7 +372,7 @@ TEST_F(HttpRateLimitFilterTest, ErrorResponse) { EXPECT_CALL(filter_callbacks_, continueDecoding()); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr, - ""); + "", nullptr); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data_, false)); EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers_)); @@ -404,7 +404,7 @@ TEST_F(HttpRateLimitFilterTest, ErrorResponseWithFailureModeAllowOff) { EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); - request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr, ""); + request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr, "", nullptr); EXPECT_CALL(filter_callbacks_.stream_info_, setResponseFlag(StreamInfo::ResponseFlag::RateLimitServiceError)) @@ -445,7 +445,7 @@ TEST_F(HttpRateLimitFilterTest, LimitResponse) { setResponseFlag(StreamInfo::ResponseFlag::RateLimited)); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OverLimit, std::move(h), - nullptr, ""); + nullptr, "", nullptr); EXPECT_EQ(1U, filter_callbacks_.clusterInfo() ->statsScope() @@ -497,7 +497,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, std::move(h), - std::move(uh), ""); + std::move(uh), "", nullptr); EXPECT_THAT(*request_headers_to_add, Not(IsSubsetOfHeaders(request_headers_))); EXPECT_EQ(1U, filter_callbacks_.clusterInfo() @@ -561,7 +561,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, - 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() @@ -626,7 +626,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, - 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() @@ -661,7 +661,7 @@ TEST_F(HttpRateLimitFilterTest, LimitResponseRuntimeDisabled) { EXPECT_CALL(filter_callbacks_, continueDecoding()); Http::ResponseHeaderMapPtr h{new Http::TestResponseHeaderMapImpl()}; request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OverLimit, std::move(h), - nullptr, ""); + nullptr, "", nullptr); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data_, false)); EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers_)); @@ -813,7 +813,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); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -859,7 +859,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); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -902,7 +902,7 @@ TEST_F(HttpRateLimitFilterTest, ExcludeVirtualHost) { .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void { callbacks.complete(Filters::Common::RateLimit::LimitStatus::OK, 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 e77f6080cba8d..c867753acd191 100644 --- a/test/extensions/filters/network/ratelimit/ratelimit_test.cc +++ b/test/extensions/filters/network/ratelimit/ratelimit_test.cc @@ -114,7 +114,7 @@ TEST_F(RateLimitFilterTest, OK) { EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data, false)); EXPECT_CALL(filter_callbacks_, continueReading()); - request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr); + request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr, "", nullptr); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); @@ -142,7 +142,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); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); @@ -171,7 +171,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); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); @@ -195,7 +195,7 @@ TEST_F(RateLimitFilterTest, Error) { EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data, false)); EXPECT_CALL(filter_callbacks_, continueReading()); - request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr); + request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr, "", nullptr); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); @@ -235,7 +235,7 @@ TEST_F(RateLimitFilterTest, ImmediateOK) { EXPECT_CALL(*client_, limit(_, "foo", _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void { - callbacks.complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr); + callbacks.complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr, "", nullptr); }))); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); @@ -258,7 +258,7 @@ TEST_F(RateLimitFilterTest, ImmediateError) { EXPECT_CALL(*client_, limit(_, "foo", _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void { - callbacks.complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr); + callbacks.complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr, "", nullptr); }))); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); @@ -300,7 +300,7 @@ TEST_F(RateLimitFilterTest, ErrorResponseWithFailureModeAllowOff) { EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onNewConnection()); Buffer::OwnedImpl data("hello"); EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data, false)); - request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr); + request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr, "", nullptr); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); From fd5fbf1c992c4d5118034b7a5a2d2a827fd247d0 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Thu, 7 Jan 2021 14:41:56 +0000 Subject: [PATCH 2/2] Use bytes type in ratelimit service v2 for body --- api/envoy/service/ratelimit/v2/rls.proto | 2 +- generated_api_shadow/envoy/service/ratelimit/v2/rls.proto | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/api/envoy/service/ratelimit/v2/rls.proto b/api/envoy/service/ratelimit/v2/rls.proto index 548422714bdd5..8096dab2a21b3 100644 --- a/api/envoy/service/ratelimit/v2/rls.proto +++ b/api/envoy/service/ratelimit/v2/rls.proto @@ -118,7 +118,7 @@ message RateLimitResponse { repeated api.v2.core.HeaderValue request_headers_to_add = 4; // A response body to send to the downstream client when the response code is not OK. - string body = 5; + 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 diff --git a/generated_api_shadow/envoy/service/ratelimit/v2/rls.proto b/generated_api_shadow/envoy/service/ratelimit/v2/rls.proto index 548422714bdd5..8096dab2a21b3 100644 --- a/generated_api_shadow/envoy/service/ratelimit/v2/rls.proto +++ b/generated_api_shadow/envoy/service/ratelimit/v2/rls.proto @@ -118,7 +118,7 @@ message RateLimitResponse { repeated api.v2.core.HeaderValue request_headers_to_add = 4; // A response body to send to the downstream client when the response code is not OK. - string body = 5; + 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