Add support for X-RateLimit-* headers in ratelimit filter by Pchelolo · Pull Request #12410 · envoyproxy/envoy

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

  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

Petr Pchelko 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

Petr Pchelko 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

Petr Pchelko 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

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

Conversation

Pchelolo commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jul 31, 2020

Uh oh!

mattklein123 commented Jul 31, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lizan commented Aug 4, 2020

Uh oh!

Pchelolo commented Aug 4, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

unleashed commented Aug 5, 2020

Uh oh!

Pchelolo commented Aug 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Pchelolo commented Jul 31, 2020 •

edited

Loading