-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ratelimit: support returning custom response bodies for non-OK responses from the external ratelimit service #14189
Changes from 12 commits
9967154
d6bdebf
9b29a79
c76e1f3
b8134f9
f4fb93d
32cb168
68eec79
d340935
e1492f1
1fd9496
88fa661
7346fa6
7c018bc
582eb42
c2a236a
108e9ec
d4e5eb2
2b49ffe
b3143e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional: While you're here, could you add Doxygen style-param to this? I think it will be useful to note that |
||
const std::string& response_body) PURE; | ||
}; | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -106,14 +106,14 @@ void GrpcClientImpl::onSuccess( | |
DescriptorStatusListPtr descriptor_statuses = std::make_unique<DescriptorStatusList>( | ||
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, ""); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit. EMPTY_STRING |
||
callbacks_ = nullptr; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -115,7 +115,7 @@ Http::FilterHeadersStatus Filter::encode100ContinueHeaders(Http::ResponseHeaderM | |
} | ||
|
||
Http::FilterHeadersStatus Filter::encodeHeaders(Http::ResponseHeaderMap& headers, bool) { | ||
populateResponseHeaders(headers); | ||
populateResponseHeaders(headers, false); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional. Nit. |
||
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); | ||
|
@@ -195,8 +196,8 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status, | |
config_->runtime().snapshot().featureEnabled("ratelimit.http_filter_enforcing", 100)) { | ||
state_ = State::Responded; | ||
callbacks_->sendLocalReply( | ||
Http::Code::TooManyRequests, "", | ||
[this](Http::HeaderMap& headers) { populateResponseHeaders(headers); }, | ||
Http::Code::TooManyRequests, response_body, | ||
[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) { | ||
|
@@ -208,8 +209,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 +237,18 @@ void Filter::populateRateLimitDescriptors(const Router::RateLimitPolicy& rate_li | |
} | ||
} | ||
|
||
void Filter::populateResponseHeaders(Http::HeaderMap& response_headers) { | ||
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 we're | ||
// populating response headers for a local reply, overwrite the existing | ||
// content-type header. | ||
// | ||
// 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_); | ||
response_headers_to_add_ = nullptr; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a
DataSource
(e.g. see what's done inDirectResponseAction
)?There are other places in the API where we even allow things like log template values to be included and JSON returned, e.g.
SubstitutionFormatString
in HCM local reply mapper. That could be another reasonable way to future proof and align with the existing precedent for this kind of body.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This body comes from the external ratelimit service so I think the better comparison would be the auth response message in the ext_authz filter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add more context, I think the
DirectResponseAction
is used to configure a response generated locally at Envoy. That means you can configure a data source local to the Envoy container and have access to variables within Envoy'sSubstitutionFormatString
. Since thisbody
is configuring a response generated externally in theRateLimitService
as part of itsRateLimitResponse
, it makes sense to represent that as just astring
on the wire as protobuf (as we do in ext_authz https://github.com/envoyproxy/envoy/blob/master/api/envoy/service/auth/v3/external_auth.proto#L59).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, makes sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please make this
bytes
and not a string? We have endless problems where we have things that proto can't encode in a string field and technically body can be raw bytes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. We'll want to make the same change to ext_authz by adding a new
body_bytes
field and deprecating the existing stringbody
field. For consistency, I'll change this field to also bebody_bytes
with typebytes
. This way ext_authz and ratelimit will have a similar UX (which was part of the goal of this PR). Does that work?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you whether you want to do the deprecation dance on extauth but for new API I would definitely do it with bytes.