Add support for X-RateLimit-* headers in ratelimit filter #12410

Pchelolo · 2020-07-31T17:53:42Z

Adds support for X-RateLimit-* headers described in the draft RFC. The X-RateLimit-Limit header contains the quota-policy per RFC. The descriptor name is included in the quota policy under the name key. X-RateLimit-Reset header is emitted, but it would need a followup in the ratelimit service, which I will do once this is merged.

Commit Message: Add support for X-RateLimit-* headers in ratelimit filter

Risk Level: Low, new codepaths are disabled by default

Testing: unit&integration tests created covering new features

Docs Changes: docs for the new config variable added

Release Notes: added to current.rst

Fixes #12356

Signed-off-by: Petr Pchelko ppchelko@wikimedia.org

@numerodix fyi

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

repokitteh-read-only · 2020-07-31T17:53:48Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #12410 was opened by Pchelolo.

see: more, trace.

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

mattklein123 · 2020-07-31T22:25:37Z

Please check CI, thank you!

/wait

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

mattklein123

Very cool. Thanks for working on this. A few comments to get started.

/wait

mattklein123 · 2020-08-03T18:04:50Z

api/envoy/service/ratelimit/v3/rls.proto

+    // Number of seconds until reset of the current limit window.
+    uint32 seconds_until_reset = 4;


WDYT about using a protobuf duration here? I think that would future proof it a bit better?

mattklein123 · 2020-08-03T18:05:33Z

api/envoy/extensions/filters/http/ratelimit/v3/rate_limit.proto

+  // The headers are specified in the `draft RFC
+  // <https://tools.ietf.org/id/draft-polli-ratelimit-headers-00.html>`_
+  // Defaults to false.
+  bool enable_response_headers = 8;


Can we make the name of this field more specific? enable_x_ratelimit_headers or similar? It's a little unclear what this does from the field name.

mattklein123 · 2020-08-03T18:05:54Z

api/envoy/extensions/filters/http/ratelimit/v3/rate_limit.proto

@@ -64,4 +64,10 @@ message RateLimit {
  // success.
  config.ratelimit.v3.RateLimitServiceConfig rate_limit_service = 7
      [(validate.rules).message = {required: true}];
+
+  // If set to true enables emitting X-RateLimit-* headers by the filter.


To avoid having to read the RFC in all cases, can you spell out a bit more what headers are emitted and what they do?

mattklein123 · 2020-08-03T18:06:54Z

include/envoy/http/header_map.h

+  HEADER_FUNC(XRateLimitLimit)                                                                     \
+  HEADER_FUNC(XRateLimitRemaining)                                                                 \
+  HEADER_FUNC(XRateLimitReset)


This should be defined as custom inline headers for the ratelimit header vs. global inline headers. See RegisterCustomInlineHeader

mattklein123 · 2020-08-03T18:08:54Z

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

+      response_headers_to_add_ = Http::ResponseHeaderMapImpl::create();
+    }
+    Http::HeaderUtility::addHeaders(*response_headers_to_add_, *rate_limit_headers);
+    rate_limit_headers = nullptr;


nit: this goes out of scope and it not necessary

mattklein123 · 2020-08-03T18:15:54Z

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

+    result->setXRateLimitLimit(rate_limit_limit);
+    result->setXRateLimitRemaining(min_remaining_limit_status.value().limit_remaining());
+    result->setXRateLimitReset(min_remaining_limit_status.value().seconds_until_reset());


FYI if these are just being set on an empty map, these don't need to be inline at all. Just directly use the various add* functions.

mattklein123 · 2020-08-03T18:17:13Z

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

+
+  absl::optional<envoy::service::ratelimit::v3::RateLimitResponse_DescriptorStatus>
+      min_remaining_limit_status;
+  std::ostringstream quotaPolicy;


nit: quota_policy.

Also please avoid using ostringstream. Prefer the absl::String functions instead for this type of case.

mattklein123 · 2020-08-03T18:17:36Z

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

+    }
+    uint32_t window = convertRateLimitUnit(status.current_limit().unit());
+    if (window) {
+      fmt::print(quotaPolicy, ", {:d};{:s}={:d}", status.current_limit().requests_per_unit(),


Can you comment/reference the spec for what this is doing? It's not clear to the casual reader.

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

mattklein123

Few more comments, thanks. Also looks like clang-tidy is still broken on the current main branch, not your fault. cc @lizan

/wait

api/envoy/service/ratelimit/v3/rls.proto

api/envoy/extensions/filters/http/ratelimit/v3/rate_limit.proto

lizan · 2020-08-04T21:03:17Z

tidy passes on my local with latest change, feel free to ignore while I'm investigating why it fails in AZP.

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

Pchelolo · 2020-08-04T21:33:13Z

tidy passes on my local with latest change, feel free to ignore while I'm investigating why it fails in AZP.

Thank you. Same here, I was banging my head over it and boiling my laptop running it locally :)

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

mattklein123

Thanks!

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

unleashed · 2020-08-05T07:41:57Z

Just a minor comment, since IIRC the diff is not going to be important for this PR, but version 03 of the draft was published about two months ago.

Pchelolo · 2020-08-05T13:12:35Z

@unleashed oh! I guess we replace everything with v3 to start off from the latest. I'll make another PR.

Followup for a new feature introduced by #12410 Apologies for not noticing that a later draft was introduced recently. I think we should start with supporting the latest available spec draft, so update version 2 to version 3. The change is technically backwards-incompatible, but the new feature was introduced one day ago, nobody could have been so fast to depend on it. Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

…#12410) Adds support for X-RateLimit-* headers described in the draft RFC. The X-RateLimit-Limit header contains the quota-policy per RFC. The descriptor name is included in the quota policy under the name key. X-RateLimit-Reset header is emitted, but it would need a followup in the ratelimit service, which I will do once this is merged. Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

Followup for a new feature introduced by envoyproxy#12410 Apologies for not noticing that a later draft was introduced recently. I think we should start with supporting the latest available spec draft, so update version 2 to version 3. The change is technically backwards-incompatible, but the new feature was introduced one day ago, nobody could have been so fast to depend on it. Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

…#12410) Adds support for X-RateLimit-* headers described in the draft RFC. The X-RateLimit-Limit header contains the quota-policy per RFC. The descriptor name is included in the quota policy under the name key. X-RateLimit-Reset header is emitted, but it would need a followup in the ratelimit service, which I will do once this is merged. Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org> Signed-off-by: chaoqinli <chaoqinli@google.com>

Followup for a new feature introduced by envoyproxy#12410 Apologies for not noticing that a later draft was introduced recently. I think we should start with supporting the latest available spec draft, so update version 2 to version 3. The change is technically backwards-incompatible, but the new feature was introduced one day ago, nobody could have been so fast to depend on it. Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org> Signed-off-by: chaoqinli <chaoqinli@google.com>

Add support for X-RateLimit-* headers in ratelimit filter

be5e062

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

repokitteh-read-only bot added the api label Jul 31, 2020

mattklein123 self-assigned this Jul 31, 2020

Pchelolo added 6 commits July 31, 2020 13:12

Fix docs and fix format

92ee7c6

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

Fix docs

f5f585f

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

Link release notes to the specific config key

5fc461d

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

Link release notes

58baad6

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

Fix spelling error

189c672

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

Fix thrift proxy filter

7c2097b

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

Pchelolo requested a review from zuercher as a code owner July 31, 2020 21:40

Fix network proxy filter

687fbfe

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

repokitteh-read-only bot added the waiting label Jul 31, 2020

Fix format

453bc4b

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

repokitteh-read-only bot removed the waiting label Jul 31, 2020

Pchelolo added 6 commits July 31, 2020 17:45

Fix network ratelimit tests

1f50f60

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

Merge branch 'master' into ratelimit_headers

81e382c

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

Fix network filter_impl test

b60298f

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

Fix memory leak in ratelimit_test

ef91c24

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

Replace typedef with using, use fmt::print for header formatting

c38d281

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

Fix format

209311e

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

mattklein123 requested changes Aug 3, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 3, 2020

Address review comments

7c63f35

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

repokitteh-read-only bot removed the waiting label Aug 3, 2020

Pchelolo added 4 commits August 3, 2020 15:02

Fix docs

d9d0ce8

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

Fix docs again

8b6deb5

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

Kick CI

187e123

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

Kick CI

93b1434

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

mattklein123 requested changes Aug 4, 2020

View reviewed changes

api/envoy/service/ratelimit/v3/rls.proto Outdated Show resolved Hide resolved

api/envoy/extensions/filters/http/ratelimit/v3/rate_limit.proto Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Aug 4, 2020

Switch config to a rfc version enum

c339ffd

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

repokitteh-read-only bot removed the waiting label Aug 4, 2020

Fix docs

fc4ea6b

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

mattklein123 previously approved these changes Aug 4, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Aug 4, 2020

Fix docs external links

4b5570a

Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>

Pchelolo dismissed mattklein123’s stale review via 4b5570a August 4, 2020 22:38

repokitteh-read-only bot added the api label Aug 4, 2020

mattklein123 approved these changes Aug 4, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Aug 4, 2020

mattklein123 merged commit 9f40563 into envoyproxy:master Aug 5, 2020

Pchelolo mentioned this pull request Aug 5, 2020

Switch RateLimit headers spec version to latest #12493

Merged

ioggstream mentioned this pull request Aug 8, 2020

Add ratelimit headers. traefik/traefik#6111

Closed

2 tasks

ioggstream mentioned this pull request Dec 30, 2020

Current ratelimit headers landscape ietf-wg-httpapi/ratelimit-headers#25

Closed

9 tasks

zhxie mentioned this pull request Oct 14, 2021

ratelimit: add support for x-ratelimit-* headers in local rate limiting #18157

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for X-RateLimit-* headers in ratelimit filter #12410

Add support for X-RateLimit-* headers in ratelimit filter #12410

Pchelolo commented Jul 31, 2020 •

edited

Loading

repokitteh-read-only bot commented Jul 31, 2020

mattklein123 commented Jul 31, 2020

mattklein123 left a comment

mattklein123 Aug 3, 2020

mattklein123 Aug 3, 2020

mattklein123 Aug 3, 2020

mattklein123 Aug 3, 2020

mattklein123 Aug 3, 2020

mattklein123 Aug 3, 2020

mattklein123 Aug 3, 2020

mattklein123 Aug 3, 2020

mattklein123 left a comment

lizan commented Aug 4, 2020

Pchelolo commented Aug 4, 2020

mattklein123 left a comment

unleashed commented Aug 5, 2020

Pchelolo commented Aug 5, 2020

		// Number of seconds until reset of the current limit window.
		uint32 seconds_until_reset = 4;

Add support for X-RateLimit-* headers in ratelimit filter #12410

Add support for X-RateLimit-* headers in ratelimit filter #12410

Conversation

Pchelolo commented Jul 31, 2020 • edited Loading

repokitteh-read-only bot commented Jul 31, 2020

mattklein123 commented Jul 31, 2020

mattklein123 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

lizan commented Aug 4, 2020

Pchelolo commented Aug 4, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

unleashed commented Aug 5, 2020

Pchelolo commented Aug 5, 2020

Pchelolo commented Jul 31, 2020 •

edited

Loading