ratelimit: Add ratelimit custom response headers #4015

surki · 2018-08-01T14:42:24Z

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 #3555

Signed-off-by: Suresh Kumar [email protected]

surki · 2018-08-01T14:49:06Z

Looks like I need clang-format-5.0? I used latest clang-format (6.0.1) and pre-push hook passes.

dio · 2018-08-01T15:09:24Z

@surki yes.

Probably you have CLANG_FORMAT env var set? Since:

envoy/tools/check_format.py

Line 26 in da3c1ea

CLANG_FORMAT_PATH = os.getenv("CLANG_FORMAT", "clang-format-5.0")

junr03 · 2018-08-01T19:46:22Z

@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 <[email protected]>

surki · 2018-08-02T10:29:29Z

@junr03 Fixed all the failures

junr03

thanks! a few comments

junr03 · 2018-08-01T18:56:03Z

test/common/ratelimit/ratelimit_impl_test.cc


 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); }


why are we using this indirection here?

junr03 · 2018-08-02T22:57:33Z

source/extensions/filters/http/ratelimit/ratelimit.cc

+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 {


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?

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?

junr03 · 2018-08-02T22:59:27Z

test/common/ratelimit/ratelimit_impl_test.cc

@@ -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());


Why do we have this indirection?

MOCK_METHOD* don't seem to allow rvalue reference params. This re-dispatch handles that.

Signed-off-by: Suresh Kumar <[email protected]>

stale · 2018-08-10T10:37:00Z

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!

mattklein123 · 2018-08-10T16:10:50Z

@junr03 friendly ping. PTAL.

junr03

my comments are addressed. @zuercher can you take a look?

junr03 · 2018-08-10T16:24:14Z

source/extensions/filters/http/ratelimit/ratelimit.cc

@@ -195,6 +168,25 @@ void Filter::populateRateLimitDescriptors(const Router::RateLimitPolicy& rate_li
  }
 }

+void Filter::addHeaders(Http::HeaderMap& headers) {


I think this should probably go in the header map utilities.

zuercher

Sorry, I reviewed this but managed to not actually submit it.

zuercher · 2018-08-10T17:21:57Z

source/extensions/filters/http/ratelimit/ratelimit.cc

+        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));


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.

@zuercher Sure, moved it there (and added unit test for it). Please take a look.

Signed-off-by: Suresh Kumar <[email protected]>

zuercher

Looks good. Thanks for moving that code.

mattklein123

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!

mattklein123 · 2018-08-14T22:24:38Z

source/common/ratelimit/ratelimit_impl.cc

@@ -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>();


Can we optimize the case where there are no headers to add and pass nullptr here?

Signed-off-by: Suresh Kumar <[email protected]>

surki · 2018-08-16T10:03:51Z

Added perf change and test coverage for encoder callbacks. Thanks

mattklein123

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 <[email protected]>

mattklein123 assigned junr03 Aug 1, 2018

surki force-pushed the ratelimit_headers branch from 02818ac to b43d561 Compare August 2, 2018 09:18

junr03 reviewed Aug 2, 2018

View reviewed changes

Avoid header addition code duplication

5519480

Signed-off-by: Suresh Kumar <[email protected]>

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 10, 2018

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 10, 2018

junr03 reviewed Aug 10, 2018

View reviewed changes

junr03 assigned zuercher Aug 10, 2018

zuercher reviewed Aug 13, 2018

View reviewed changes

Added addHeaders utility function in Envoy::Http::HeaderUtility

ae4e1aa

Signed-off-by: Suresh Kumar <[email protected]>

zuercher previously approved these changes Aug 14, 2018

View reviewed changes

mattklein123 requested changes Aug 14, 2018

View reviewed changes

Add test coverage for encoder and perf improvement for no headers

e7ddb39

Signed-off-by: Suresh Kumar <[email protected]>

surki dismissed zuercher’s stale review via e7ddb39 August 16, 2018 08:52

mattklein123 approved these changes Aug 16, 2018

View reviewed changes

mattklein123 merged commit 71152b7 into envoyproxy:master Aug 16, 2018

lvijay mentioned this pull request Sep 27, 2018

bring api and service protos up-to-date with data-plane-api envoyproxy/java-control-plane#68

Closed

armsnyder mentioned this pull request Nov 15, 2018

Add X-RateLimit-* response headers as an opt-in feature envoyproxy/ratelimit#56

Closed

junr03 mentioned this pull request Apr 8, 2019

Add X-RateLimit-* response headers as an opt-in feature envoyproxy/ratelimit#77

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ratelimit: Add ratelimit custom response headers #4015

ratelimit: Add ratelimit custom response headers #4015

surki commented Aug 1, 2018

surki commented Aug 1, 2018

dio commented Aug 1, 2018

junr03 commented Aug 1, 2018

surki commented Aug 2, 2018

junr03 left a comment

junr03 Aug 1, 2018

junr03 Aug 2, 2018

surki Aug 3, 2018

junr03 Aug 2, 2018

surki Aug 3, 2018

stale bot commented Aug 10, 2018

mattklein123 commented Aug 10, 2018

junr03 left a comment

junr03 Aug 10, 2018

zuercher left a comment

zuercher Aug 10, 2018

surki Aug 14, 2018

zuercher left a comment

mattklein123 left a comment

mattklein123 Aug 14, 2018

surki commented Aug 16, 2018

mattklein123 left a comment

ratelimit: Add ratelimit custom response headers #4015

ratelimit: Add ratelimit custom response headers #4015

Conversation

surki commented Aug 1, 2018

surki commented Aug 1, 2018

dio commented Aug 1, 2018

junr03 commented Aug 1, 2018

surki commented Aug 2, 2018

junr03 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Aug 10, 2018

mattklein123 commented Aug 10, 2018

junr03 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuercher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuercher left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surki commented Aug 16, 2018

mattklein123 left a comment

Choose a reason for hiding this comment