-
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
ratelimit: support returning custom response bodies for non-OK responses from the external ratelimit service #14189
Conversation
Hi @esmet, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
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.
Thanks!
// 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; |
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.
What's the reasoning for not treating a content-type in the RateLimiterResponse as authoritative when the body is empty?
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.
The ratelimit service can only specify response headers to add, without the option to append or override. Because there's no option, the behavior is "append". But append doesn't really make sense for content-type here when we know sendLocalReply is going to always add with text/plain. Then, later, our code may add the actual content-type as it was received from the RLS.
The response header config is here https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/ratelimit/v3/rls.proto#service-ratelimit-v3-ratelimitresponse - note HeaderValue instead of HeaderValueOption
I think it makes sense to make the content-type overwrite any value present here. Maybe the right thing to do long term is both to migrate the RLS service to use HeaderValueOptions (just because, so it is similar to ext_authz) and also to clean up the interaction between sendLocalReply and this function.
If we rename overwrite_content_type
to is_local_response
(or just local_response
), do you think the behavior can/should stay as it is implemented here?
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.
Sorry for the slow reply, I got slammed this week.
I think you're right that the RLS should be able to specified whether headers should be replaced or appended.
I'm just trying to figure out if there's a use case for a rejection setting content-type with an empty body. I think I would find it surprising if it didn't overwrite just because there was no body. And thus, I think if you do the param rename, for the local response case you should treat content-type from RLS as authoritative even if body is missing.
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. I will rename the variable to from_local_reply
(or something else if you prefer) and then go ahead and make the content-type authoritative in that case, even with no body.
Do you think there's value in following up with a PR to migrate the RLS to HeaderValueOptions? If so, what's your opinion on migration/strategy? I could either add a new field or possibly make a breaking change for v4, right?
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.
I don't feel I'm in a position to pass judgement on whether switching to HeaderValueOption is worth the migration effort for users. To the extent that you can find people using RLS and poll them, I think that'd be the way to go.
As far as the mechanics of the change go, I think breaking changes are only allowed within an alpha version if the field was never part of a released version. So you wouldn't be allowed to do that here. I think the way we'd do this is to add the new field and mark the existing one deprecated. When v4 becomes authoritative, we'd add translation to convert the deprecated v3 field under the covers with warnings, stats, etc (there's precedent for all this).
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.
That all makes sense. Thanks for the explanation. I will look into adding a new field.
non-OK responses from the external ratelimit service, similar to ext_authz. Signed-off-by: John Esmet <johnesmet@datawire.io> Signed-off-by: John Esmet <john.esmet@gmail.com>
Signed-off-by: John Esmet <john.esmet@gmail.com>
Signed-off-by: John Esmet <john.esmet@gmail.com>
Signed-off-by: John Esmet <john.esmet@gmail.com>
Signed-off-by: John Esmet <john.esmet@gmail.com>
Signed-off-by: John Esmet <john.esmet@gmail.com>
Signed-off-by: John Esmet <john.esmet@gmail.com>
Signed-off-by: John Esmet <john.esmet@gmail.com>
c9230fe
to
1fd9496
Compare
@zuercher I believe this is ready for final review :) |
Signed-off-by: John Esmet <john.esmet@gmail.com>
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.
One optional nit and there's a test failure to resolve. I restart the test. Not sure if a master merge would help if it fails again.
We also need @envoyproxy/api-shepherds review for the API change.
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\" }"; |
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.
nit: Consider using a raw string literal: R"EOF({"message": "...", "retry-after": "33"})EOF";
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 do
Signed-off-by: John Esmet <john.esmet@gmail.com>
I addressed the optional nit and it looks like CI is green now. |
@@ -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; |
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 in DirectResponseAction
)?
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's SubstitutionFormatString
. Since this body
is configuring a response generated externally in the RateLimitService
as part of its RateLimitResponse
, it makes sense to represent that as just a string
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 string body
field. For consistency, I'll change this field to also be body_bytes
with type bytes
. 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.
/lgtm api |
@dio want to give this one a second pass? |
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.
One small API comment, thank you!
/wait
@@ -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; |
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.
Signed-off-by: John Esmet <john.esmet@gmail.com>
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.
Looks good overall. A doubt on naming and a couple of optional nits
@@ -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. | |||
bytes body_bytes = 5; |
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 is great, but not sure about the name. _bytes
usually is used in envoy to denote "how many bytes". In ext_authz we use raw_body
. Though, I don't feel strongly about this.
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.
I'll change it to raw_body
. I looked at ext_authz and you are correct that we have raw_body
for the body field that represents the original client request body as it is forwarded to the authorization service. We still only have a string body
for the response body to send to the client on denied responses, so I'll still consider adding raw_body
to that message for consistency with the ratelimit service.
@@ -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 comment
The 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 response_body
can contain non-UTF8 value.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
nit. EMPTY_STRING
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Optional. Nit. populateResponseHeaders(headers, /*from_local_reply=*/false);
. Following @htuch's way on annotating boolean params, whch I found realy useful for code reading. :).
Signed-off-by: John Esmet <john.esmet@gmail.com>
Signed-off-by: John Esmet <john.esmet@gmail.com>
Signed-off-by: John Esmet <john.esmet@gmail.com>
…body Signed-off-by: John Esmet <john.esmet@gmail.com>
I addressed the nits and renamed the field to |
I updated the field to use |
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.
Thanks!
@mattklein123 do you mind giving this a quick pass to verify the changes requested to use |
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.
Thanks!
* master: (49 commits) sds: allow multiple init managers share sds target (envoyproxy#14357) [http] Remove legacy codecs (envoyproxy#14381) http2: Add integration tests for METADATA and RST_STREAM frame flood mitigation for upstream servers (envoyproxy#14365) test: start dissolving :printers_include rule. (envoyproxy#14429) integration tests: re-enable set_node_on_first_message_only (envoyproxy#14270) formatter: add a formatter that returns a google::protobuf::Struct rather than a string (envoyproxy#14258) ratelimit: support returning custom response bodies for non-OK responses from the external ratelimit service (envoyproxy#14189) deps: update protobuf to 3.14 (envoyproxy#14253) stream_info: add setResponseCode and update local_reply to take a normal StreamInfo (envoyproxy#14402) http: alpn upstream (envoyproxy#13922) Moved starttls integration test to test/extensions/transport_sockets/starttls. (envoyproxy#14425) generic conn pool: directly use thread local cluster (envoyproxy#14423) wasm: add mathetake to CODEOWNERS (envoyproxy#14427) wasm: clear route cache when modifying HTTP request headers. (envoyproxy#14318) tls: disable TLS inspector injection (envoyproxy#14404) aggregate cluster: cleanups (envoyproxy#14411) Mark starttls_integration_test flaky on Windows (envoyproxy#14419) tcp: improved unit testing (envoyproxy#14415) config: making protocol config explicit (envoyproxy#14362) wasm: dead code (envoyproxy#14407) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Commit Message: ratelimit: support returning custom response bodies for non-OK responses from the external ratelimit service, similar to ext_authz.
Additional Description: This change forces the content-type from the ratelimit service to be authoritative even though the proto field has type HeaderValue, which normally does not support header overrides. This feels less surprising than having the content-type value appended to any existing value, and also makes the code play nice with sendLocalReply, which sets content-type to text/plain by default.
Risk Level: Low (opt-in field, existing behavior preserved)
Testing: unit
Docs Changes: API protos documented, in line with existing fields
Release Notes: ratelimit: added :ref:
body <envoy_v3_api_field_service.ratelimit.v3.RateLimitResponse.body>
field to support custom response bodies for non-OK responses from the external ratelimit service.Fixes #14188