Envoy rate limit descriptor fix by karbon0x · Pull Request #11153 · envoyproxy/envoy

karbon0x · 2020-05-11T22:49:51Z

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: It resolves - #10124 indirectly by adding an extra config flag to RequestHeaders through which it is possible for descriptors to be sent on a partial match
Risk Level: low - optional feature
Testing: unit tests added
Docs Changes: Added
Release Notes: Added
[Optional Fixes #Issue] Fixes #10124
[Optional Deprecated:]

Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

karbon0x · 2020-05-11T22:50:45Z

Need to add associated documentation changes for the new configuration option. Opening PR so that @ramaraochavali can view it.

Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

repokitteh-read-only · 2020-05-13T00:54:45Z

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

🐱

Caused by: #11153 was synchronize by rohan07.

see: more, trace.

ramaraochavali · 2020-05-13T07:11:49Z

      // The key to use in the descriptor entry.
      string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}];
+
+      // The optional parameter which determines if the RLS is called if a request


If set to true, Envoy skips the descriptor while calling rate limiting service when header is not present in the request. Defaults to false, which skips calling rate limiting service if this header is not present in the request.

ramaraochavali · 2020-05-13T07:16:41Z

                                              const Network::Address::Instance&) const {
  const Http::HeaderEntry* header_value = headers.get(header_name_);
-  if (!header_value) {
+  if (!header_value && !skip_if_absent_) {


nit: Simplify it like this and add a comment

if (!header_value) { return skip_if_absent_; }

ramaraochavali · 2020-05-13T07:20:20Z

 }

+// Validate that a descriptor is added if at least one request header has value.
+TEST_F(RateLimitPolicyEntryTest, RequestHeadersPartialMatchWithSkipIfAbsent) {


/s/RequestHeadersPartialMatchWithSkipIfAbsent/RequestHeadersWithSkipIfAbsent

ramaraochavali · 2020-05-13T07:21:25Z

+              testing::ContainerEq(descriptors_));
+}
+
+TEST_F(RateLimitPolicyEntryTest, RequestHeadersPartialMatchWithoutSkipIfAbsent) {


/s/RequestHeadersPartialMatchWithoutSkipIfAbsent/RequestHeadersWithDefaultSkipIfAbsent and add a comment above the test

ramaraochavali · 2020-05-13T07:22:09Z

+              testing::ContainerEq(descriptors_));
+}
+
+TEST_F(RateLimitPolicyEntryTest, RequestHeadersMatchDefaultSkipIfAbsent) {


This is not needed

ramaraochavali · 2020-05-13T07:22:39Z

+  EXPECT_TRUE(descriptors_.empty());
+}
+
+TEST_F(RateLimitPolicyEntryTest, RequestHeadersCompleteMatch) {


This also should have been tested already

ramaraochavali · 2020-05-13T07:24:20Z

Add a version history entry https://github.com/envoyproxy/envoy/blob/master/docs/root/version_history/current.rst in alpha order

Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

ramaraochavali · 2020-05-13T09:29:47Z

 * prometheus stats: fix the sort order of output lines to comply with the standard.
 * request_id: added to :ref:`always_set_request_id_in_response setting <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.always_set_request_id_in_response>`
-  to set :ref:`x-request-id <config_http_conn_man_headers_x-request-id>` header in response even if
+  to set :ref:`x-request-id <config_http_conn_man_headers_x-request-id>` header in response even if skip_if_absent 


not needed change looks like?

Yup, pushing the fix, accidental change

Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

htuch

API changes LGTM modulo two tiny nits.

htuch · 2020-05-13T15:56:19Z

      string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}];
+
+      // If set to true, Envoy skips the descriptor while calling rate limiting service
+      // when header is not present in the request. Defaults to false, which skips calling


Suggested change

// when header is not present in the request. Defaults to false, which skips calling

// when header is not present in the request. By default this skips calling

i.e. it's redundant to say that the default of a primitive proto3 type is false, but your description of the actual behavior is very helpful.

Sounds good, @htuch! Pushed the changes as you suggested! Thanks!

htuch · 2020-05-13T15:58:03Z

  tracing is not forced.
 * router: allow retries of streaming or incomplete requests. This removes stat `rq_retry_skipped_request_not_complete`.
 * router: allow retries by default when upstream responds with :ref:`x-envoy-overloaded <config_http_filters_router_x-envoy-overloaded_set>`.
+* router: allow Rate Limiting service to be called in case of missing request header for a descriptor if the :ref:`skip_if_absent <envoy_v3_api_field_config.route.v3.RateLimit.Action.RequestHeaders.skip_if_absent>` field is set to true.


Nit: Rate Limiting SErvice

Signed-off-by: Rohan Seth <rohan@Rohans-MacBook-Pro.local>

mattklein123 · 2020-05-18T21:20:13Z

@rohan07 please never force push. It makes reviews much more difficult. Thanks!

mattklein123

Thanks one more comment.

/wait

mattklein123 · 2020-05-18T21:20:59Z

                                              const Network::Address::Instance&) const {
  const Http::HeaderEntry* header_value = headers.get(header_name_);
+
+  // If header is not present and skip_if_absent_ is true, ignore the descriptor


This comment is still not correct. Returning has no bearing on whether skip_if_absent_ is true?

mattklein123 · 2020-05-20T18:49:50Z

Thanks please merge master and force push to fix DCO.

/wait

karbon0x · 2020-05-20T18:58:02Z

Apologies @mattklein123 - My previous force push was also because of the DCO. I was experiencing some performance issues with my old machine so had to get a new one.

Signed-off-by: Rohan Seth <rohan@Rohans-MacBook-Pro.local>

…descriptor-fix Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

mattklein123

Thanks!

karbon0x · 2020-05-20T22:43:12Z

Hi @mattklein123 - Apologies again, it wasn't allowing me to sign off on my previous commit using my new machine for some reason, it worked from my previous machine. Thanks for your help!

karbon0x · 2020-05-21T01:31:54Z

Hi @mattklein123 - I am seeing some of the other PR's which are open facing the same issue with circleci/coverage failing (with 96.7%, lower than the 97% threshold). I believe during my earlier commits, the particular test was passing. Can you please advise the future steps I should take?

ramaraochavali · 2020-05-21T04:26:06Z

/retest

repokitteh-read-only · 2020-05-21T04:26:10Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #11153 (comment) was created by @ramaraochavali.

see: more, trace.

…descriptor-fix Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

Rohan Seth added 7 commits May 11, 2020 09:56

Add skip_if_absent flag to request headers

787c962

Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

Adding skip_if_absent

e0ef3e8

Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

Adding skip_if_absent to router_ratelimit

5fdb921

Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

Clang format fix

5fcdbc8

Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

Clang format fix

2eb2da7

Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

Adding generated files

a8b1a12

Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

Adding generated files

be0f75c

Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

karbon0x marked this pull request as draft May 11, 2020 23:07

dio assigned ramaraochavali May 12, 2020

Clang-formating changes

c6173f6

Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

repokitteh-read-only Bot added the api label May 13, 2020

karbon0x marked this pull request as ready for review May 13, 2020 06:54

karbon0x marked this pull request as draft May 13, 2020 06:55

ramaraochavali reviewed May 13, 2020

View reviewed changes

Rohan Seth added 3 commits May 13, 2020 01:11

Modifying comment

415de69

Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

Adding comments and release notes

9db6ec9

Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

Adding release notes

8b1e906

Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

karbon0x marked this pull request as ready for review May 13, 2020 09:23

ramaraochavali reviewed May 13, 2020

View reviewed changes

Rohan Seth added 2 commits May 13, 2020 02:35

Updating release notes

5e2eb3a

Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

Fixing accidental release note change

d9495ef

Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

htuch reviewed May 13, 2020

View reviewed changes

repokitteh-read-only Bot added waiting and removed waiting labels May 17, 2020

Updating comment

9cfaeaa

Signed-off-by: Rohan Seth <rohan@Rohans-MacBook-Pro.local>

karbon0x force-pushed the envoy-rate-limit-descriptor-fix branch from 26eebbf to 9cfaeaa Compare May 18, 2020 20:15

mattklein123 requested changes May 18, 2020

View reviewed changes

repokitteh-read-only Bot added waiting and removed waiting labels May 18, 2020

repokitteh-read-only Bot added the waiting label May 20, 2020

repokitteh-read-only Bot removed the waiting label May 20, 2020

karbon0x force-pushed the envoy-rate-limit-descriptor-fix branch 3 times, most recently from c7c58a2 to a83afa4 Compare May 20, 2020 19:28

Add descriptive comment

281b9e4

Signed-off-by: Rohan Seth <rohan@Rohans-MacBook-Pro.local>

mattklein123 added the waiting label May 20, 2020

karbon0x force-pushed the envoy-rate-limit-descriptor-fix branch from ca3ca83 to 281b9e4 Compare May 20, 2020 21:07

repokitteh-read-only Bot removed the waiting label May 20, 2020

Rohan Seth added 3 commits May 20, 2020 14:14

Merge remote-tracking branch 'upstream/master' into envoy-rate-limit-…

a60210c

…descriptor-fix Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

router_ratelimit formatting fix

df712d2

Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

Fix current.rst

2360171

Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

mattklein123 approved these changes May 20, 2020

View reviewed changes

repokitteh-read-only Bot removed the api label May 20, 2020

Merge remote-tracking branch 'upstream/master' into envoy-rate-limit-…

e80328b

…descriptor-fix Signed-off-by: Rohan Seth <rohan.seth@salesforce.com>

mattklein123 merged commit 77e436f into envoyproxy:master May 21, 2020

	// when header is not present in the request. Defaults to false, which skips calling
	// when header is not present in the request. By default this skips calling

Conversation

karbon0x commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karbon0x commented May 11, 2020

Uh oh!

repokitteh-read-only Bot commented May 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramaraochavali May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ramaraochavali commented May 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch 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!

mattklein123 commented May 18, 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!

mattklein123 commented May 20, 2020

Uh oh!

karbon0x commented May 20, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

karbon0x commented May 20, 2020

Uh oh!

karbon0x commented May 21, 2020

Uh oh!

ramaraochavali commented May 21, 2020

Uh oh!

repokitteh-read-only Bot commented May 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

karbon0x commented May 11, 2020 •

edited

Loading

ramaraochavali May 13, 2020 •

edited

Loading