-
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: Add ratelimit custom response headers #4015
Conversation
Looks like I need clang-format-5.0? I used latest clang-format (6.0.1) and pre-push hook passes. |
@surki can you take a look at the (non format) test failures? Thank you for the contribution! |
- Ability to add custom response headers from ratelimit service/filter - For both (LimitStatus::OK and LimitStatus::OverLimit) custom headers are added if RLS service sends headers - For LimitStatus:OK, we temporarily store the headers and add them to the response (via Filter::encodeHeaders()) *Risk Level*: Low *Testing*: unit and integration tests added. Verified with modified github.com/lyft/ratelimit service. Passes "bazel test //test/..." in Linux *Docs Changes*: protobuf documentation updated *Release Notes*: ratelimit: support for adding custom http headers from ratelimit service Fixes envoyproxy#3555 Signed-off-by: Suresh Kumar <suresh@freshdesk.com>
02818ac
to
b43d561
Compare
@junr03 Fixed all the failures |
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! a few comments
|
||
namespace Envoy { | ||
namespace RateLimit { | ||
|
||
class MockRequestCallbacks : public RequestCallbacks { | ||
public: | ||
MOCK_METHOD1(complete, void(LimitStatus status)); | ||
void complete(LimitStatus status, Http::HeaderMapPtr&& headers) { complete_(status, *headers); } |
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.
why are we using this indirection here?
Http::FilterHeadersStatus Filter::encodeHeaders(Http::HeaderMap& headers, bool) { | ||
if (headers_to_add_) { | ||
headers_to_add_->iterate( | ||
[](const Http::HeaderEntry& header, void* context) -> Http::HeaderMap::Iterate { |
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 same lambda is used here and in the local reply on overlimit, and is the same logic that happens in one of the HeaderMapImpl constructors. Can we reduce this duplication?
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 removed the duplication in ratelimit filter.
I couldn't find a method in HeaderMap that allows me to add another HeaderMap.
Short of adding such method, not sure if there is a way to avoid duplication between HeaderMapImpl and ratelimit.cc iterations?
Also any idea if I can "move" elements from one HeaderMap to another without doing the extra copy via HeaderString?
@@ -27,7 +27,11 @@ namespace RateLimit { | |||
|
|||
class MockRequestCallbacks : public RequestCallbacks { | |||
public: | |||
MOCK_METHOD1(complete, void(LimitStatus status)); | |||
void complete(LimitStatus status, Http::HeaderMapPtr&& headers) { | |||
complete_(status, headers.get()); |
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.
Why do we have this indirection?
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.
MOCK_METHOD* don't seem to allow rvalue reference params. This re-dispatch handles that.
Signed-off-by: Suresh Kumar <suresh@freshdesk.com>
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
@junr03 friendly ping. PTAL. |
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.
my comments are addressed. @zuercher can you take a look?
@@ -195,6 +168,25 @@ void Filter::populateRateLimitDescriptors(const Router::RateLimitPolicy& rate_li | |||
} | |||
} | |||
|
|||
void Filter::addHeaders(Http::HeaderMap& headers) { |
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 think this should probably go in the header map utilities.
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, I reviewed this but managed to not actually submit it.
k.setCopy(header.key().c_str(), header.key().size()); | ||
Http::HeaderString v; | ||
v.setCopy(header.value().c_str(), header.value().size()); | ||
static_cast<Http::HeaderMapImpl*>(context)->addViaMove(std::move(k), std::move(v)); |
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 cast in particular feels like it shouldn't be here. I think you could introduce a utility function in Envoy::Http::HeaderUtility so that it's contained in the implementing module.
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.
@zuercher Sure, moved it there (and added unit test for it). Please take a look.
Signed-off-by: Suresh Kumar <suresh@freshdesk.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. Thanks for moving that code.
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.
LGTM, one perf comment. Please also check code coverage for the rate limit filter to make sure all the new encode callbacks have coverage (including trailers). Thank you!
@@ -66,14 +67,18 @@ void GrpcClientImpl::onSuccess( | |||
span.setTag(Constants::get().TraceStatus, Constants::get().TraceOk); | |||
} | |||
|
|||
callbacks_->complete(status); | |||
Http::HeaderMapPtr headers = std::make_unique<Http::HeaderMapImpl>(); |
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 optimize the case where there are no headers to add and pass nullptr here?
Signed-off-by: Suresh Kumar <suresh@freshdesk.com>
Added perf change and test coverage for encoder callbacks. Thanks |
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.
Great, thank you!
…#1938) This is far from finished, but it reduces memory usage by ~10%. Pulling the following changes from github.com/envoyproxy/envoy: c1cc68d stats: refactoring MetricImpl without strings (envoyproxy#4190) 36809d8 fuzz: coverage profile generation instructions. (envoyproxy#4193) ba40cc9 upstream: rebuild cluster when health check config is changed (envoyproxy#4075) 05c0d52 build: use clang-6.0. (envoyproxy#4168) 01f403e thrift_proxy: introduce header transport (envoyproxy#4082) 564d256 tcp: allow connection pool callers to store protocol state (envoyproxy#4131) 3e1d643 configs: match latest API changes (envoyproxy#4185) bc6a10c Fix wrong mock function name. (envoyproxy#4187) e994c1c Bump yaml-cpp so it builds with Visual Studio 15.8 (envoyproxy#4182) 3d1325e Converting envoy configs to V2 (envoyproxy#2957) 8d0680f Add timestamp to HealthCheckEvent definition (envoyproxy#4119) 497efb9 server: handle non-EnvoyExceptions safely if thrown in constructor. (envoyproxy#4173) 6d6fafd config: strengthen validation for gRPC config sources. (envoyproxy#4171) 132302c fuzz: reduce log level when running under fuzz engine. (envoyproxy#4161) 7c04ac2 test: fix V6EmptyOptions in coverage with IPv6 enable (envoyproxy#4155) 1b2219b ci: remove deprecated bazel --batch option (envoyproxy#4166) 2db6a4c ci: update clang to version 6.0 in the Ubuntu build image. (envoyproxy#4157) 71152b7 ratelimit: Add ratelimit custom response headers (envoyproxy#4015) 3062874 ssl: make Ssl::Connection const everywhere (envoyproxy#4179) 706e262 Handle validation of non expiring tokens in jwt_authn filter (envoyproxy#4007) f06e958 fuzz: tag trivial fuzzers with no_fuzz. (envoyproxy#4156) 27fb1d3 thrift_proxy: add service name matching to router implementation (envoyproxy#4130) 8c189a5 Make over provisioning factor configurable (envoyproxy#4003) 6c08fb4 Making test less flaky (envoyproxy#4149) 592775b fuzz: bare bones HCM fuzzer. (envoyproxy#4118) For istio/istio#7912. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
service/filter
headers are added if RLS service sends headers
them to the response (via Filter::encodeHeaders())
Risk Level: Low
Testing: unit and integration tests added. Verified with modified
github.com/lyft/ratelimit service. Passes "bazel test //test/..." in
Linux
Docs Changes: protobuf documentation updated
Release Notes: ratelimit: support for adding custom http headers
from ratelimit service
Fixes #3555
Signed-off-by: Suresh Kumar suresh@freshdesk.com