From 996715491b432a64724c3dc6002443a7da0e8f72 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Tue, 22 Sep 2020 11:39:13 -0400 Subject: [PATCH 01/19] ratelimit: support returning custom response bodies for non-OK responses from the external ratelimit service, similar to ext_authz. Signed-off-by: John Esmet Signed-off-by: John Esmet --- api/envoy/service/ratelimit/v3/rls.proto | 4 + .../envoy/service/ratelimit/v3/rls.proto | 4 + .../filters/common/ratelimit/ratelimit.h | 3 +- .../common/ratelimit/ratelimit_impl.cc | 4 +- .../filters/http/ratelimit/ratelimit.cc | 35 +++- .../filters/http/ratelimit/ratelimit.h | 5 +- .../filters/network/ratelimit/ratelimit.cc | 3 +- .../filters/network/ratelimit/ratelimit.h | 3 +- .../filters/ratelimit/ratelimit.cc | 2 +- .../filters/ratelimit/ratelimit.h | 3 +- .../filters/http/ratelimit/ratelimit_test.cc | 175 ++++++++++++++++-- test/test_common/utility.h | 5 + 12 files changed, 210 insertions(+), 36 deletions(-) diff --git a/api/envoy/service/ratelimit/v3/rls.proto b/api/envoy/service/ratelimit/v3/rls.proto index 42f24cfb0805c..d92d8ce2986ab 100644 --- a/api/envoy/service/ratelimit/v3/rls.proto +++ b/api/envoy/service/ratelimit/v3/rls.proto @@ -51,6 +51,7 @@ message RateLimitRequest { } // A response from a ShouldRateLimit call. +// [#next-free-field: 6] message RateLimitResponse { option (udpa.annotations.versioning).previous_message_type = "envoy.service.ratelimit.v2.RateLimitResponse"; @@ -131,4 +132,7 @@ message RateLimitResponse { // A list of headers to add to the request when forwarded 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; } diff --git a/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto b/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto index 42f24cfb0805c..d92d8ce2986ab 100644 --- a/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto +++ b/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto @@ -51,6 +51,7 @@ message RateLimitRequest { } // A response from a ShouldRateLimit call. +// [#next-free-field: 6] message RateLimitResponse { option (udpa.annotations.versioning).previous_message_type = "envoy.service.ratelimit.v2.RateLimitResponse"; @@ -131,4 +132,7 @@ message RateLimitResponse { // A list of headers to add to the request when forwarded 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; } diff --git a/source/extensions/filters/common/ratelimit/ratelimit.h b/source/extensions/filters/common/ratelimit/ratelimit.h index 068cd369b643c..45177563bfb12 100644 --- a/source/extensions/filters/common/ratelimit/ratelimit.h +++ b/source/extensions/filters/common/ratelimit/ratelimit.h @@ -49,7 +49,8 @@ class RequestCallbacks { */ virtual void complete(LimitStatus status, DescriptorStatusListPtr&& descriptor_statuses, Http::ResponseHeaderMapPtr&& response_headers_to_add, - Http::RequestHeaderMapPtr&& request_headers_to_add) PURE; + Http::RequestHeaderMapPtr&& request_headers_to_add, + const std::string& response_body) PURE; }; /** diff --git a/source/extensions/filters/common/ratelimit/ratelimit_impl.cc b/source/extensions/filters/common/ratelimit/ratelimit_impl.cc index d4c3f5afdaa32..321474468aed1 100644 --- a/source/extensions/filters/common/ratelimit/ratelimit_impl.cc +++ b/source/extensions/filters/common/ratelimit/ratelimit_impl.cc @@ -106,14 +106,14 @@ void GrpcClientImpl::onSuccess( DescriptorStatusListPtr descriptor_statuses = std::make_unique( response->statuses().begin(), response->statuses().end()); callbacks_->complete(status, std::move(descriptor_statuses), std::move(response_headers_to_add), - std::move(request_headers_to_add)); + std::move(request_headers_to_add), response->body()); 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); + callbacks_->complete(LimitStatus::Error, nullptr, nullptr, nullptr, ""); callbacks_ = nullptr; } diff --git a/source/extensions/filters/http/ratelimit/ratelimit.cc b/source/extensions/filters/http/ratelimit/ratelimit.cc index 8430f47243a85..347b8f4c94e19 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.cc +++ b/source/extensions/filters/http/ratelimit/ratelimit.cc @@ -115,7 +115,7 @@ Http::FilterHeadersStatus Filter::encode100ContinueHeaders(Http::ResponseHeaderM } Http::FilterHeadersStatus Filter::encodeHeaders(Http::ResponseHeaderMap& headers, bool) { - populateResponseHeaders(headers); + populateResponseHeaders(headers, false); return Http::FilterHeadersStatus::Continue; } @@ -143,7 +143,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) { + Http::RequestHeaderMapPtr&& request_headers_to_add, + const std::string& response_body) { 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); @@ -194,9 +195,16 @@ 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, "", - [this](Http::HeaderMap& headers) { populateResponseHeaders(headers); }, + Http::Code::TooManyRequests, response_body, + [this, overwrite_content_type](Http::HeaderMap& headers) { + populateResponseHeaders(headers, overwrite_content_type); + }, config_->rateLimitedGrpcStatus(), RcDetails::get().RateLimited); callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::RateLimited); } else if (status == Filters::Common::RateLimit::LimitStatus::Error) { @@ -208,8 +216,8 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status, } } else { state_ = State::Responded; - callbacks_->sendLocalReply(Http::Code::InternalServerError, "", nullptr, absl::nullopt, - RcDetails::get().RateLimitError); + callbacks_->sendLocalReply(Http::Code::InternalServerError, response_body, nullptr, + absl::nullopt, RcDetails::get().RateLimitError); callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::RateLimitServiceError); } } else if (!initiating_call_) { @@ -236,8 +244,21 @@ void Filter::populateRateLimitDescriptors(const Router::RateLimitPolicy& rate_li } } -void Filter::populateResponseHeaders(Http::HeaderMap& response_headers) { +void Filter::populateResponseHeaders(Http::HeaderMap& response_headers, + bool overwrite_content_type) { 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. + // + // 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).empty())) { + response_headers.remove(Http::Headers::get().ContentType); + } Http::HeaderUtility::addHeaders(response_headers, *response_headers_to_add_); response_headers_to_add_ = nullptr; } diff --git a/source/extensions/filters/http/ratelimit/ratelimit.h b/source/extensions/filters/http/ratelimit/ratelimit.h index 058eb793569a9..c2b3a9d106b5e 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.h +++ b/source/extensions/filters/http/ratelimit/ratelimit.h @@ -148,7 +148,8 @@ class Filter : public Http::StreamFilter, public Filters::Common::RateLimit::Req void complete(Filters::Common::RateLimit::LimitStatus status, Filters::Common::RateLimit::DescriptorStatusListPtr&& descriptor_statuses, Http::ResponseHeaderMapPtr&& response_headers_to_add, - Http::RequestHeaderMapPtr&& request_headers_to_add) override; + Http::RequestHeaderMapPtr&& request_headers_to_add, + const std::string& response_body) override; private: void initiateCall(const Http::RequestHeaderMap& headers); @@ -156,7 +157,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); + void populateResponseHeaders(Http::HeaderMap& response_headers, bool overwrite_content_type); void appendRequestHeaders(Http::HeaderMapPtr& request_headers_to_add); VhRateLimitOptions getVirtualHostRateLimitOption(const Router::RouteConstSharedPtr& route); diff --git a/source/extensions/filters/network/ratelimit/ratelimit.cc b/source/extensions/filters/network/ratelimit/ratelimit.cc index 00ed50a9f60c4..01d1ed73324e5 100644 --- a/source/extensions/filters/network/ratelimit/ratelimit.cc +++ b/source/extensions/filters/network/ratelimit/ratelimit.cc @@ -72,7 +72,8 @@ void Filter::onEvent(Network::ConnectionEvent event) { void Filter::complete(Filters::Common::RateLimit::LimitStatus status, Filters::Common::RateLimit::DescriptorStatusListPtr&&, - Http::ResponseHeaderMapPtr&&, Http::RequestHeaderMapPtr&&) { + Http::ResponseHeaderMapPtr&&, Http::RequestHeaderMapPtr&&, + const std::string&) { 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 eba34f4348671..d1029bfbf2952 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, Filters::Common::RateLimit::DescriptorStatusListPtr&& descriptor_statuses, Http::ResponseHeaderMapPtr&& response_headers_to_add, - Http::RequestHeaderMapPtr&& request_headers_to_add) override; + Http::RequestHeaderMapPtr&& request_headers_to_add, + const std::string& response_body) override; private: enum class Status { NotStarted, Calling, Complete }; 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 c775dff935346..a9cf61aca3e3f 100644 --- a/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.cc +++ b/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.cc @@ -62,7 +62,7 @@ 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) { + Http::RequestHeaderMapPtr&& request_headers_to_add, const std::string&) { // 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. 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 caa5333cda651..abecfb38ac56f 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, Filters::Common::RateLimit::DescriptorStatusListPtr&& descriptor_statuses, Http::ResponseHeaderMapPtr&& response_headers_to_add, - Http::RequestHeaderMapPtr&& request_headers_to_add) override; + Http::RequestHeaderMapPtr&& request_headers_to_add, + const std::string& response_body) override; private: void initiateCall(const ThriftProxy::MessageMetadata& metadata); diff --git a/test/extensions/filters/http/ratelimit/ratelimit_test.cc b/test/extensions/filters/http/ratelimit/ratelimit_test.cc index 4eff428507211..8cfe437b934a1 100644 --- a/test/extensions/filters/http/ratelimit/ratelimit_test.cc +++ b/test/extensions/filters/http/ratelimit/ratelimit_test.cc @@ -236,7 +236,7 @@ TEST_F(HttpRateLimitFilterTest, OkResponse) { setResponseFlag(StreamInfo::ResponseFlag::RateLimited)) .Times(0); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr, - nullptr); + nullptr, ""); EXPECT_EQ( 1U, filter_callbacks_.clusterInfo()->statsScope().counterFromStatName(ratelimit_ok_).value()); @@ -289,7 +289,7 @@ 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)}, ""); Http::TestResponseHeaderMapImpl expected_headers(*rl_headers); Http::TestResponseHeaderMapImpl response_headers; EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, false)); @@ -347,7 +347,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, ""); Http::TestResponseHeaderMapImpl expected_headers{ {"x-ratelimit-limit", "1, 1;w=60;name=\"first\", 4;w=3600;name=\"second\""}, @@ -374,7 +374,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, ""); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -405,7 +405,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, ""); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -444,7 +444,7 @@ TEST_F(HttpRateLimitFilterTest, ErrorResponse) { EXPECT_CALL(filter_callbacks_, continueDecoding()); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr, - nullptr); + nullptr, ""); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data_, false)); EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers_)); @@ -477,7 +477,7 @@ TEST_F(HttpRateLimitFilterTest, ErrorResponseWithFailureModeAllowOff) { filter_->decodeHeaders(request_headers_, false)); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr, - nullptr); + nullptr, ""); EXPECT_CALL(filter_callbacks_.stream_info_, setResponseFlag(StreamInfo::ResponseFlag::RateLimitServiceError)) @@ -518,7 +518,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, ""); EXPECT_EQ(1U, filter_callbacks_.clusterInfo() ->statsScope() @@ -570,7 +570,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), ""); EXPECT_THAT(*request_headers_to_add, Not(IsSubsetOfHeaders(request_headers_))); EXPECT_EQ(1U, filter_callbacks_.clusterInfo() @@ -585,6 +585,141 @@ TEST_F(HttpRateLimitFilterTest, LimitResponseWithHeaders) { filter_callbacks_.clusterInfo()->statsScope().counterFromStatName(upstream_rq_429_).value()); } +// New tests! + +TEST_F(HttpRateLimitFilterTest, LimitResponseWithBody) { + 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)); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, + filter_->encode100ContinueHeaders(response_headers_)); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers_, false)); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(response_data_, false)); + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->encodeTrailers(response_trailers_)); + + const std::string response_body = "this is a custom over limit response body."; + const std::string content_length = std::to_string(response_body.length()); + Http::HeaderMapPtr rl_headers{new Http::TestResponseHeaderMapImpl{ + {"x-ratelimit-limit", "1000"}, {"x-ratelimit-remaining", "0"}, {"retry-after", "33"}}}; + Http::TestResponseHeaderMapImpl expected_headers{}; + // We construct the expected_headers map in careful order, because HeaderMapEqualRef below + // compares two header maps in order. In practice, content-length and content-type headers + // are added before additional ratelimit headers and the final x-envoy-ratelimited header. + expected_headers.addCopy(":status", "429"); + expected_headers.addCopy("content-length", std::string(content_length)); + expected_headers.addCopy("content-type", "text/plain"); + expected_headers.copyFrom(*rl_headers); + expected_headers.addCopy("x-envoy-ratelimited", Http::Headers::get().EnvoyRateLimitedValues.True); + + EXPECT_CALL(filter_callbacks_, encodeHeaders_(HeaderMapEqualRef(&expected_headers), false)); + EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); + EXPECT_CALL(filter_callbacks_, encodeData(_, true)) + .WillOnce( + Invoke([&](Buffer::Instance& data, bool) { EXPECT_EQ(data.toString(), response_body); })); + EXPECT_CALL(filter_callbacks_.stream_info_, + setResponseFlag(StreamInfo::ResponseFlag::RateLimited)); + + Http::HeaderMapPtr request_headers_to_add{ + new Http::TestRequestHeaderMapImpl{{"x-rls-rate-limited", "true"}}}; + + 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); + + EXPECT_THAT(*request_headers_to_add, Not(IsSubsetOfHeaders(request_headers_))); + 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()); +} + +TEST_F(HttpRateLimitFilterTest, LimitResponseWithBodyAndContentType) { + 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)); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, + filter_->encode100ContinueHeaders(response_headers_)); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers_, false)); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(response_data_, false)); + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->encodeTrailers(response_trailers_)); + + const std::string response_body = "{ \"message\": \"this is a custom over limit response body as " + "json.\", \"retry-after\": \"33\" }"; + const std::string content_length = std::to_string(response_body.length()); + Http::HeaderMapPtr rl_headers{ + new Http::TestResponseHeaderMapImpl{{"content-type", "application/json"}, + {"x-ratelimit-limit", "1000"}, + {"x-ratelimit-remaining", "0"}, + {"retry-after", "33"}}}; + Http::TestResponseHeaderMapImpl expected_headers{}; + // We construct the expected_headers map in careful order, because HeaderMapEqualRef below + // compares two header maps in order. In practice, content-length and content-type headers + // are added before additional ratelimit headers and the final x-envoy-ratelimited header. + // Additionally, we skip explicitly adding content-type here because it's already part of + // `rl_headers` above. + expected_headers.addCopy(":status", "429"); + expected_headers.addCopy("content-length", std::string(content_length)); + expected_headers.copyFrom(*rl_headers); + expected_headers.addCopy("x-envoy-ratelimited", Http::Headers::get().EnvoyRateLimitedValues.True); + + EXPECT_CALL(filter_callbacks_, encodeHeaders_(HeaderMapEqualRef(&expected_headers), false)); + EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); + EXPECT_CALL(filter_callbacks_, encodeData(_, true)) + .WillOnce( + Invoke([&](Buffer::Instance& data, bool) { EXPECT_EQ(data.toString(), response_body); })); + EXPECT_CALL(filter_callbacks_.stream_info_, + setResponseFlag(StreamInfo::ResponseFlag::RateLimited)); + + Http::HeaderMapPtr request_headers_to_add{ + new Http::TestRequestHeaderMapImpl{{"x-rls-rate-limited", "true"}}}; + + 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); + + EXPECT_THAT(*request_headers_to_add, Not(IsSubsetOfHeaders(request_headers_))); + 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()); +} + +// End of new tests + TEST_F(HttpRateLimitFilterTest, LimitResponseWithFilterHeaders) { SetUpTest(enable_x_ratelimit_headers_config_); InSequence s; @@ -624,7 +759,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, ""); EXPECT_EQ(1U, filter_callbacks_.clusterInfo() ->statsScope() .counterFromStatName(ratelimit_over_limit_) @@ -660,7 +795,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, ""); EXPECT_EQ(1U, filter_callbacks_.clusterInfo() ->statsScope() @@ -695,7 +830,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, ""); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data_, false)); EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers_)); @@ -847,7 +982,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, ""); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -893,7 +1028,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, ""); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -947,7 +1082,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, ""); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -998,7 +1133,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, ""); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -1050,7 +1185,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, ""); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -1099,7 +1234,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, ""); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -1150,7 +1285,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, ""); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -1198,7 +1333,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, ""); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 6b4abcd7c63c8..7580a576e56d5 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -857,6 +857,11 @@ template class TestHeaderMapImplBase : public Inte HeaderMapImpl::copyFrom(*header_map_, rhs); header_map_->verifyByteSizeInternalForTest(); } + void copyFrom(const TestHeaderMapImplBase& rhs) { copyFrom(*rhs.header_map_); } + void copyFrom(const HeaderMap& rhs) { + HeaderMapImpl::copyFrom(*header_map_, rhs); + header_map_->verifyByteSizeInternalForTest(); + } TestHeaderMapImplBase& operator=(const TestHeaderMapImplBase& rhs) { if (this == &rhs) { return *this; From d6bdebf1526d9cd37f9f91cfb5d901dccd6151c4 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Wed, 25 Nov 2020 19:24:34 +0000 Subject: [PATCH 02/19] test/common/network/filter_manager_impl_test: fix 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 ba0aae2d6e73e..985b5502078ab 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, ""); conn_pool.poolReady(upstream_connection); From 9b29a79bd04d1f5a8b23885b5db8342bab057ce4 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Wed, 25 Nov 2020 19:27:52 +0000 Subject: [PATCH 03/19] test/extensions/filters/network/thrift_proxy: fix test Signed-off-by: John Esmet --- .../filters/ratelimit/ratelimit_test.cc | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) 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 29a863e1f68bc..5e4c2ed04f503 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 @@ -228,7 +228,7 @@ TEST_F(ThriftRateLimitFilterTest, OkResponse) { setResponseFlag(StreamInfo::ResponseFlag::RateLimited)) .Times(0); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr, - nullptr); + nullptr, ""); EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("ratelimit.ok").value()); @@ -248,7 +248,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, ""); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -272,7 +272,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, ""); }))); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -302,7 +302,7 @@ TEST_F(ThriftRateLimitFilterTest, ErrorResponse) { EXPECT_CALL(filter_callbacks_, continueDecoding()); request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::Error, nullptr, nullptr, - nullptr); + nullptr, ""); EXPECT_EQ(ThriftProxy::FilterStatus::Continue, filter_->messageEnd()); EXPECT_CALL(filter_callbacks_.stream_info_, @@ -340,7 +340,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, ""); EXPECT_EQ( 1U, @@ -374,7 +374,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, ""); EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("ratelimit.over_limit") @@ -407,7 +407,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, ""); EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("ratelimit.over_limit") @@ -432,7 +432,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, ""); EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("ratelimit.over_limit") From c76e1f3be0175083be711f8e597ac0b55550f2d4 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Wed, 25 Nov 2020 20:04:53 +0000 Subject: [PATCH 04/19] test/extensions/filters/common/ratelimit/ratelimit_impl_test: fix test Signed-off-by: John Esmet --- .../common/ratelimit/ratelimit_impl_test.cc | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/test/extensions/filters/common/ratelimit/ratelimit_impl_test.cc b/test/extensions/filters/common/ratelimit/ratelimit_impl_test.cc index 319596f436a98..1d514e0673c19 100644 --- a/test/extensions/filters/common/ratelimit/ratelimit_impl_test.cc +++ b/test/extensions/filters/common/ratelimit/ratelimit_impl_test.cc @@ -38,15 +38,17 @@ class MockRequestCallbacks : public RequestCallbacks { public: void complete(LimitStatus status, DescriptorStatusListPtr&& descriptor_statuses, Http::ResponseHeaderMapPtr&& response_headers_to_add, - Http::RequestHeaderMapPtr&& request_headers_to_add) override { + Http::RequestHeaderMapPtr&& request_headers_to_add, + const std::string& response_body) override { complete_(status, descriptor_statuses.get(), response_headers_to_add.get(), - request_headers_to_add.get()); + request_headers_to_add.get(), response_body); } 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 Http::RequestHeaderMap* request_headers_to_add, + const std::string& response_body)); }; class RateLimitGrpcClientTest : public testing::Test { @@ -91,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_); } @@ -110,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_); } @@ -127,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_); } @@ -150,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_); } } From b8134f9af26c74d6fad809326c3921b7085a819a Mon Sep 17 00:00:00 2001 From: John Esmet Date: Mon, 30 Nov 2020 20:20:54 +0000 Subject: [PATCH 05/19] test/extensions/filters/network/ratelimit/ratelimit_test: fix test Signed-off-by: John Esmet --- .../filters/network/ratelimit/ratelimit_test.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/extensions/filters/network/ratelimit/ratelimit_test.cc b/test/extensions/filters/network/ratelimit/ratelimit_test.cc index f2255e356cd2d..7b1a07324650b 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, ""); 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, ""); 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, ""); 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, ""); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); @@ -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, ""); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); From f4fb93d674b9bc580b78cc262a11db510c804b9f Mon Sep 17 00:00:00 2001 From: John Esmet Date: Wed, 2 Dec 2020 19:26:40 +0000 Subject: [PATCH 06/19] ratelimit: rename overwrite_content_type -> from_local_reply Signed-off-by: John Esmet --- .../filters/http/ratelimit/ratelimit.cc | 27 +++++++------------ .../filters/http/ratelimit/ratelimit.h | 2 +- 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/source/extensions/filters/http/ratelimit/ratelimit.cc b/source/extensions/filters/http/ratelimit/ratelimit.cc index 347b8f4c94e19..c1c71350fdf35 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.cc +++ b/source/extensions/filters/http/ratelimit/ratelimit.cc @@ -195,16 +195,9 @@ 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, true); }, config_->rateLimitedGrpcStatus(), RcDetails::get().RateLimited); callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::RateLimited); } else if (status == Filters::Common::RateLimit::LimitStatus::Error) { @@ -244,18 +237,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 && + // 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_).get(Http::Headers::get().ContentType).empty())) { response_headers.remove(Http::Headers::get().ContentType); } diff --git a/source/extensions/filters/http/ratelimit/ratelimit.h b/source/extensions/filters/http/ratelimit/ratelimit.h index c2b3a9d106b5e..5623cd2a98406 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.h +++ b/source/extensions/filters/http/ratelimit/ratelimit.h @@ -157,7 +157,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); VhRateLimitOptions getVirtualHostRateLimitOption(const Router::RouteConstSharedPtr& route); From 32cb168cbf32944ccaeb7b5aec1003be3a71e53e Mon Sep 17 00:00:00 2001 From: John Esmet Date: Wed, 2 Dec 2020 19:34:39 +0000 Subject: [PATCH 07/19] fix test Signed-off-by: John Esmet --- test/extensions/filters/network/ratelimit/ratelimit_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/extensions/filters/network/ratelimit/ratelimit_test.cc b/test/extensions/filters/network/ratelimit/ratelimit_test.cc index 7b1a07324650b..019ad70009096 100644 --- a/test/extensions/filters/network/ratelimit/ratelimit_test.cc +++ b/test/extensions/filters/network/ratelimit/ratelimit_test.cc @@ -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, ""); }))); 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, ""); }))); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); From 68eec790d5adb4b95a7b71ac91ca4f7c373bb217 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Wed, 2 Dec 2020 20:41:42 +0000 Subject: [PATCH 08/19] Simplify Signed-off-by: John Esmet --- source/extensions/filters/http/ratelimit/ratelimit.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/extensions/filters/http/ratelimit/ratelimit.cc b/source/extensions/filters/http/ratelimit/ratelimit.cc index c1c71350fdf35..382b42148a828 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.cc +++ b/source/extensions/filters/http/ratelimit/ratelimit.cc @@ -246,8 +246,7 @@ void Filter::populateResponseHeaders(Http::HeaderMap& response_headers, bool fro // 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_).get(Http::Headers::get().ContentType).empty())) { + 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_); From d3409358f5825766f4db9b614224f2a34c8bb3d8 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Wed, 2 Dec 2020 23:40:02 +0000 Subject: [PATCH 09/19] Add release notes 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 02135001dcf6d..ff0698f3efed3 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -71,6 +71,7 @@ New Features * overload: add :ref:`envoy.overload_actions.reduce_timeouts ` overload action to enable scaling timeouts down with load. * 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. * sds: improved support for atomic :ref:`key rotations ` and added configurable rotation triggers for :ref:`TlsCertificate ` and :ref:`CertificateValidationContext `. From e1492f125a9f6b46832abb5116fa222f81301340 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Thu, 3 Dec 2020 17:27:31 +0000 Subject: [PATCH 10/19] Fix release notes Signed-off-by: John Esmet --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index ff0698f3efed3..e96648ba4fba1 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -71,7 +71,7 @@ New Features * overload: add :ref:`envoy.overload_actions.reduce_timeouts ` overload action to enable scaling timeouts down with load. * 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:`body ` field to support custom response bodies for non-OK responses from the external ratelimit service. * sds: improved support for atomic :ref:`key rotations ` and added configurable rotation triggers for :ref:`TlsCertificate ` and :ref:`CertificateValidationContext `. From 1fd94966f2a414b6d5d940bd2202c4e675bfc6ea Mon Sep 17 00:00:00 2001 From: John Esmet Date: Thu, 3 Dec 2020 17:32:14 +0000 Subject: [PATCH 11/19] Fix Signed-off-by: John Esmet --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index e96648ba4fba1..7f97fd8322e5f 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -71,7 +71,7 @@ New Features * overload: add :ref:`envoy.overload_actions.reduce_timeouts ` overload action to enable scaling timeouts down with load. * 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:`body ` field to support custom response bodies for non-OK responses from the external ratelimit service. * sds: improved support for atomic :ref:`key rotations ` and added configurable rotation triggers for :ref:`TlsCertificate ` and :ref:`CertificateValidationContext `. From 88fa661ee695dc984e81f7816488fe6b21295b8b Mon Sep 17 00:00:00 2001 From: John Esmet Date: Thu, 3 Dec 2020 17:43:31 +0000 Subject: [PATCH 12/19] Clean up test comments Signed-off-by: John Esmet --- test/extensions/filters/http/ratelimit/ratelimit_test.cc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/extensions/filters/http/ratelimit/ratelimit_test.cc b/test/extensions/filters/http/ratelimit/ratelimit_test.cc index 8cfe437b934a1..3b89c55455578 100644 --- a/test/extensions/filters/http/ratelimit/ratelimit_test.cc +++ b/test/extensions/filters/http/ratelimit/ratelimit_test.cc @@ -585,8 +585,6 @@ TEST_F(HttpRateLimitFilterTest, LimitResponseWithHeaders) { filter_callbacks_.clusterInfo()->statsScope().counterFromStatName(upstream_rq_429_).value()); } -// New tests! - TEST_F(HttpRateLimitFilterTest, LimitResponseWithBody) { SetUpTest(filter_config_); InSequence s; @@ -718,8 +716,6 @@ TEST_F(HttpRateLimitFilterTest, LimitResponseWithBodyAndContentType) { filter_callbacks_.clusterInfo()->statsScope().counterFromStatName(upstream_rq_429_).value()); } -// End of new tests - TEST_F(HttpRateLimitFilterTest, LimitResponseWithFilterHeaders) { SetUpTest(enable_x_ratelimit_headers_config_); InSequence s; From 7346fa68c1f0a1ee167e01dcf4cbd29f0ce9a3ae Mon Sep 17 00:00:00 2001 From: John Esmet Date: Thu, 3 Dec 2020 21:24:18 +0000 Subject: [PATCH 13/19] Raw string literal Signed-off-by: John Esmet --- test/extensions/filters/http/ratelimit/ratelimit_test.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/extensions/filters/http/ratelimit/ratelimit_test.cc b/test/extensions/filters/http/ratelimit/ratelimit_test.cc index 3b89c55455578..bdc389f5bd59a 100644 --- a/test/extensions/filters/http/ratelimit/ratelimit_test.cc +++ b/test/extensions/filters/http/ratelimit/ratelimit_test.cc @@ -668,8 +668,9 @@ TEST_F(HttpRateLimitFilterTest, LimitResponseWithBodyAndContentType) { EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(response_data_, false)); EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->encodeTrailers(response_trailers_)); - const std::string response_body = "{ \"message\": \"this is a custom over limit response body as " - "json.\", \"retry-after\": \"33\" }"; + const std::string response_body = R"EOF( + { "message": "this is a custom over limit response body as json.", "retry-after": "33" } + )EOF"; const std::string content_length = std::to_string(response_body.length()); Http::HeaderMapPtr rl_headers{ new Http::TestResponseHeaderMapImpl{{"content-type", "application/json"}, From 7c018bc5fec3f7666bf6b2fe9596447115703961 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Fri, 11 Dec 2020 03:24:01 +0000 Subject: [PATCH 14/19] body -> body_bytes Signed-off-by: John Esmet --- api/envoy/service/ratelimit/v3/rls.proto | 2 +- generated_api_shadow/envoy/service/ratelimit/v3/rls.proto | 2 +- source/extensions/filters/common/ratelimit/ratelimit_impl.cc | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/envoy/service/ratelimit/v3/rls.proto b/api/envoy/service/ratelimit/v3/rls.proto index d92d8ce2986ab..88c845069d607 100644 --- a/api/envoy/service/ratelimit/v3/rls.proto +++ b/api/envoy/service/ratelimit/v3/rls.proto @@ -134,5 +134,5 @@ 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 body_bytes = 5; } diff --git a/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto b/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto index d92d8ce2986ab..88c845069d607 100644 --- a/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto +++ b/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto @@ -134,5 +134,5 @@ 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 body_bytes = 5; } diff --git a/source/extensions/filters/common/ratelimit/ratelimit_impl.cc b/source/extensions/filters/common/ratelimit/ratelimit_impl.cc index 321474468aed1..b4a53aa6f9096 100644 --- a/source/extensions/filters/common/ratelimit/ratelimit_impl.cc +++ b/source/extensions/filters/common/ratelimit/ratelimit_impl.cc @@ -106,7 +106,7 @@ void GrpcClientImpl::onSuccess( DescriptorStatusListPtr descriptor_statuses = std::make_unique( response->statuses().begin(), response->statuses().end()); callbacks_->complete(status, std::move(descriptor_statuses), std::move(response_headers_to_add), - std::move(request_headers_to_add), response->body()); + std::move(request_headers_to_add), response->body_bytes()); callbacks_ = nullptr; } From 582eb4278e3edfb7c997c2a183018e584c38247e Mon Sep 17 00:00:00 2001 From: John Esmet Date: Fri, 11 Dec 2020 16:09:36 +0000 Subject: [PATCH 15/19] body -> raw_body Signed-off-by: John Esmet --- api/envoy/service/ratelimit/v3/rls.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/envoy/service/ratelimit/v3/rls.proto b/api/envoy/service/ratelimit/v3/rls.proto index 88c845069d607..7379368fd4c1a 100644 --- a/api/envoy/service/ratelimit/v3/rls.proto +++ b/api/envoy/service/ratelimit/v3/rls.proto @@ -134,5 +134,5 @@ 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. - bytes body_bytes = 5; + bytes raw_body = 5; } From c2a236a0980d01341105ca8e876c33996f5a972f Mon Sep 17 00:00:00 2001 From: John Esmet Date: Fri, 11 Dec 2020 16:18:39 +0000 Subject: [PATCH 16/19] Address review comments Signed-off-by: John Esmet --- .../envoy/service/ratelimit/v3/rls.proto | 2 +- .../extensions/filters/common/ratelimit/ratelimit.h | 11 +++++++++-- .../filters/common/ratelimit/ratelimit_impl.cc | 4 ++-- source/extensions/filters/http/ratelimit/ratelimit.cc | 6 ++++-- 4 files changed, 16 insertions(+), 7 deletions(-) diff --git a/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto b/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto index 88c845069d607..7379368fd4c1a 100644 --- a/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto +++ b/generated_api_shadow/envoy/service/ratelimit/v3/rls.proto @@ -134,5 +134,5 @@ 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. - bytes body_bytes = 5; + bytes raw_body = 5; } diff --git a/source/extensions/filters/common/ratelimit/ratelimit.h b/source/extensions/filters/common/ratelimit/ratelimit.h index 45177563bfb12..964cc25a7bd6c 100644 --- a/source/extensions/filters/common/ratelimit/ratelimit.h +++ b/source/extensions/filters/common/ratelimit/ratelimit.h @@ -44,8 +44,15 @@ class RequestCallbacks { virtual ~RequestCallbacks() = default; /** - * Called when a limit request is complete. The resulting status, - * response headers and request headers to be forwarded to the upstream are supplied. + * Called when a limit request is complete. The resulting status, response headers + * and request headers to be forwarded to the upstream are supplied. + * + * @status The ratelimit status + * @descriptor_statuses The descriptor statuses + * @response_headers_to_add The headers to add to the downstream response, for non-OK statuses + * @request_headers_to_add The headers to add to the upstream request, if not ratelimited + * @response_body The response body to use for the downstream response, for non-OK statuses. May + * contain non UTF-8 values (e.g. binary data). */ virtual void complete(LimitStatus status, DescriptorStatusListPtr&& descriptor_statuses, Http::ResponseHeaderMapPtr&& response_headers_to_add, diff --git a/source/extensions/filters/common/ratelimit/ratelimit_impl.cc b/source/extensions/filters/common/ratelimit/ratelimit_impl.cc index b4a53aa6f9096..4aef806f60d19 100644 --- a/source/extensions/filters/common/ratelimit/ratelimit_impl.cc +++ b/source/extensions/filters/common/ratelimit/ratelimit_impl.cc @@ -106,14 +106,14 @@ void GrpcClientImpl::onSuccess( DescriptorStatusListPtr descriptor_statuses = std::make_unique( response->statuses().begin(), response->statuses().end()); callbacks_->complete(status, std::move(descriptor_statuses), std::move(response_headers_to_add), - std::move(request_headers_to_add), response->body_bytes()); + std::move(request_headers_to_add), response->raw_body()); 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, ""); + callbacks_->complete(LimitStatus::Error, nullptr, nullptr, nullptr, EMPTY_STRING); callbacks_ = nullptr; } diff --git a/source/extensions/filters/http/ratelimit/ratelimit.cc b/source/extensions/filters/http/ratelimit/ratelimit.cc index 382b42148a828..b72bb82c4b752 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.cc +++ b/source/extensions/filters/http/ratelimit/ratelimit.cc @@ -115,7 +115,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; } @@ -197,7 +197,9 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status, state_ = State::Responded; callbacks_->sendLocalReply( Http::Code::TooManyRequests, response_body, - [this](Http::HeaderMap& headers) { populateResponseHeaders(headers, true); }, + [this](Http::HeaderMap& headers) { + populateResponseHeaders(headers, /*from_local_reply=*/true); + }, config_->rateLimitedGrpcStatus(), RcDetails::get().RateLimited); callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::RateLimited); } else if (status == Filters::Common::RateLimit::LimitStatus::Error) { From 108e9ec82589984f265b51d6cc3c050e75b232ea Mon Sep 17 00:00:00 2001 From: John Esmet Date: Fri, 11 Dec 2020 22:42:59 +0000 Subject: [PATCH 17/19] Fix docs link Signed-off-by: John Esmet --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 7f97fd8322e5f..6299fc508b7cf 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -71,7 +71,7 @@ New Features * overload: add :ref:`envoy.overload_actions.reduce_timeouts ` overload action to enable scaling timeouts down with load. * 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:`body ` field to support custom response bodies for non-OK responses from the external ratelimit service. * sds: improved support for atomic :ref:`key rotations ` and added configurable rotation triggers for :ref:`TlsCertificate ` and :ref:`CertificateValidationContext `. From d4e5eb27e4c11bb0a7b1996ea99708a26afd8e49 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Sat, 12 Dec 2020 04:09:02 +0000 Subject: [PATCH 18/19] Kick CI Signed-off-by: John Esmet From 2b49ffe961e583e7301b039573c812d5d80f5084 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Sat, 12 Dec 2020 04:09:26 +0000 Subject: [PATCH 19/19] Kick CI Signed-off-by: John Esmet