rate limit: add computed descriptors #14448

kyessenov · 2020-12-16T19:48:33Z

Signed-off-by: Kuat Yessenov kuat@google.com

Commit Message: Introduce computed descriptors to rate limits. Refactors the rate limit descriptor interface to accept StreamInfo and RequestHeaderMap. As a general refactor, adds an extension point for alternative rate limit descriptors to register.
Risk Level: low, not enabled unless used
Testing: unit
Docs Changes: yes
Release Notes: yes

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only · 2020-12-16T19:48:40Z

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

🐱

Caused by: #14448 was opened by kyessenov.

see: more, trace.

api/envoy/config/route/v3/route_components.proto

markdroth · 2020-12-16T20:17:28Z

I think I need some more context here. What is this going to be used for, and how do you expect that it will be used?

kyessenov · 2020-12-16T22:12:06Z

@markdroth Rate limit descriptors act as categories for requests, since token buckets are applied per category. We have a requirement to use source pod labels as a category. Our extension exposes this information in a filter state, so we need a way to extract this info from the filter state (which means traverse the filter state down to a field).

The current design relies on adding more and more oneof messages here, which we can avoid by just using a generic AST from CEL.

gargnupur · 2020-12-16T22:24:54Z

api/envoy/config/route/v3/route_components.proto

+      // The key to use in the descriptor entry.
+      string descriptor_key = 1 [(validate.rules).string = {min_len: 1}];
+
+      oneof expr_specifier {


why is this a oneof?

It's anticipating parsed_expr here which would introduce a dependency on expr proto. I can drop it since it's not needed right now.

If you think we're going to need the oneof later, let's keep it for now. We can't add it later without breaking the go proto API.

Also, I believe that we are actually already using the CEL parsed expression proto in other places in the API, so this is probably not a new external dependency. You might be able to just add it now.

The dependency is coming from a filter. This is a core proto. Thoughts @htuch?

If we don't want to depend on the CEL protos for the xDS core protos, another option might be to make this more extensible by simply adding a TypedExpressionConfig. In the long term, we could maybe even migrate the existing options here to be extensions as well.

@envoyproxy/api-shepherds, thoughts? I wonder if we should add something to the API review checklist in #14399 that prompts people to consider using TypedExtensionConfig instead of a oneof?

From an Envoy binary perspective, are we adding this to core router, or does it only apply to rate limit filter? Hard to tell from config. If core, @kyessenov would be nice to have clarity on when CEL can a reasonable number of the checkmarks in https://github.com/envoyproxy/envoy/blob/master/DEPENDENCY_POLICY.md#new-external-dependencies. Whatever is reasonable, but if this is going to become a core data plane dependency, it's probably good to use this opportunity to check in. CC @envoyproxy/dependency-shepherds

From an API perspective, I don't feel super strongly, CEL is a reasonably standard thing. One area to have some concern around is that using CEL is similar to regex; we're introducing dynamic evaluation of some expression, which ultimately might have security and inter-request isolation concerns. I don't know if this is in any way actionable, but certainly when communicating in comments/docs its worth pointing out what the runtime implications are. E.g. does evaluating the expression have a bounded amount of time for a given expression, or can input size affect it?

I believe the runtime change is scoped to global and local rate limit filters. So this is purely a question of a proto dependency, to avoid parsing overhead with a parsed expression-as-proto. Good point about dynamic evaluation, I'll document that.

Expression evaluation is linear in both input and expression size. The input here is request metadata.

The fact that rate limit stuff is in the main router is a historical artifact of before we had per-filter route config, etc. None of this should be here and I think there is a tracking issue to move it all out, but I don't think that should block this PR.

OK, I think this forces moving expr package into common from extensions, since it gets pulled in the implementation.

api/envoy/config/route/v3/route_components.proto

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2020-12-23T04:24:37Z

The PR is ready for review.

I wonder what I should do about the layering enforcement between core and extensions. It seems that rate limit descriptors are in the core as a historic artifact, so I'd need to pull "Expr" package into source/common to match it because it is a dependency from the router. Is that a reasonable approach? Ideally, rate limit descriptors would be another extension point and separated from the router, but I don't see an easy way to achieve that right now.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2021-01-05T18:51:03Z

Bringing this back to attention: I am looking how to resolve the layering violation in API with rate limit descriptors being in the core. There are a couple of ways around it:

Add an opaque Any extension to rate limit descriptors, register additional descriptors in rate limit extensions.
Keep as-is, move expr-related packages (CEL, other transitive stuff like ANTLR) as a "data-plane" dependency.

Opinions?

mattklein123 · 2021-01-05T19:04:02Z

Opinions?

Definitely not (2). As I already mentioned, the rate limit stuff is only in the core due to legacy reasons. It should all be removed. If you want to take one for the team we can do that (deprecate, move to filter specific route config).

If you aren't willing to do that I think (1) is our best option and we can add extensions there as @rgs1 is doing currently for access log formatters. In thinking about it we probably want an extension point here anyway, so I guess you could just go ahead and add it orthogonally to moving the stuff out of core.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2021-01-06T04:10:57Z

Thanks! Updated the PR to address a new extension point for rate limit descriptors and added computed descriptors as an extension. I did some basic refactoring around rate limit interfaces to make it look like other extension points.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2021-01-07T06:15:42Z

/retest

repokitteh-read-only · 2021-01-07T06:15:46Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14448 (comment) was created by @kyessenov.

see: more, trace.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2021-01-07T23:45:54Z

The coverage checker counts the #ifdef and NOT_REACHED_GCOVR_EXCL_LINE as no coverage for some reason. Any hint to work around it?

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2021-01-08T19:11:05Z

/retest

repokitteh-read-only · 2021-01-08T19:11:09Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14448 (comment) was created by @kyessenov.

see: more, trace.

kyessenov · 2021-01-08T20:52:50Z

/retest

repokitteh-read-only · 2021-01-08T20:52:54Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14448 (comment) was created by @kyessenov.

see: more, trace.

kyessenov · 2021-01-08T22:15:41Z

@lizan Could you please re-approve and merge? There's a merge conflict with #14588 that I need to fix.

kyessenov · 2021-01-10T20:33:00Z

@moderation Hi, do you have a minute to approve deps change in this PR?

moderation · 2021-01-10T20:42:07Z

Done @kyessenov. Metadata only deps change
/lgtm deps

mattklein123

Just skimmed, but this is great, thanks.

* master: http: support creating filters with match tree (envoyproxy#14430) [tls] Expose ServerContextImpl::selectTlsContext (envoyproxy#14592) docs: update ext_proc docs to reflect implementation status (envoyproxy#14636) filter manager: drop assert (envoyproxy#14633) kick off v1.18.0 (envoyproxy#14637) 1.17.0 release (envoyproxy#14624) Implement request header processing in ext_proc (envoyproxy#14385) http: expose encoded headers/trailers via callbacks (envoyproxy#14544) [fuzz] fix minor fuzz bugs (envoyproxy#14593) rate limit: add computed descriptors (envoyproxy#14448) tools: fill in the required args for Api::Impl (envoyproxy#14554) Bump envoy-build to current images (envoyproxy#14608) Signed-off-by: Michael Puncel <mpuncel@squareup.com>

rate limit: add compute descriptor

bb30a16

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only bot added the api label Dec 16, 2020

repokitteh-read-only bot assigned markdroth Dec 16, 2020

kyessenov commented Dec 16, 2020

View reviewed changes

api/envoy/config/route/v3/route_components.proto Outdated Show resolved Hide resolved

gargnupur reviewed Dec 16, 2020

View reviewed changes

markdroth reviewed Dec 17, 2020

View reviewed changes

api/envoy/config/route/v3/route_components.proto Outdated Show resolved Hide resolved

api/envoy/config/route/v3/route_components.proto Outdated Show resolved Hide resolved

kyessenov added 6 commits December 21, 2020 09:09

Merge remote-tracking branch 'upstream/master' into expr_descriptor

c2960e6

review

9dfde69

Signed-off-by: Kuat Yessenov <kuat@google.com>

refactor to use stream info

99e3300

Signed-off-by: Kuat Yessenov <kuat@google.com>

implement expression descriptor

8c9636f

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix layering over-fix

d8d9b6c

Signed-off-by: Kuat Yessenov <kuat@google.com>

add tests

85849b7

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov marked this pull request as ready for review December 23, 2020 04:26

kyessenov requested a review from lizan as a code owner December 23, 2020 04:26

kyessenov added 2 commits December 23, 2020 15:42

fix a test

42562f1

Signed-off-by: Kuat Yessenov <kuat@google.com>

Merge remote-tracking branch 'upstream/master' into expr_descriptor

cc1b103

kyessenov added 6 commits January 5, 2021 11:10

Merge remote-tracking branch 'upstream/master' into expr_descriptor

a41481e

change api

ed7c4cf

Signed-off-by: Kuat Yessenov <kuat@google.com>

refactor

b94a9c8

Signed-off-by: Kuat Yessenov <kuat@google.com>

update tests

c52977f

Signed-off-by: Kuat Yessenov <kuat@google.com>

merge fix

201379b

Signed-off-by: Kuat Yessenov <kuat@google.com>

reformat

a901644

Signed-off-by: Kuat Yessenov <kuat@google.com>

update deps

0c79bb7

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov added 2 commits January 7, 2021 11:20

mergef ix

fc45e99

Signed-off-by: Kuat Yessenov <kuat@google.com>

code format

0229e39

Signed-off-by: Kuat Yessenov <kuat@google.com>

lizan self-assigned this Jan 7, 2021

kyessenov added 2 commits January 7, 2021 14:30

rename api

218a286

Signed-off-by: Kuat Yessenov <kuat@google.com>

move descriptors

fd8399d

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only bot added the api label Jan 7, 2021

fix docs

614f52a

Signed-off-by: Kuat Yessenov <kuat@google.com>

lizan previously approved these changes Jan 7, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jan 7, 2021

coverage fixes

5c318ba

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov dismissed lizan’s stale review via 5c318ba January 8, 2021 01:39

repokitteh-read-only bot added the api label Jan 8, 2021

fix coverage

a212836

Signed-off-by: Kuat Yessenov <kuat@google.com>

lizan approved these changes Jan 8, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jan 8, 2021

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Jan 10, 2021

mattklein123 approved these changes Jan 10, 2021

View reviewed changes

mattklein123 merged commit 876a363 into envoyproxy:master Jan 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rate limit: add computed descriptors #14448

rate limit: add computed descriptors #14448

kyessenov commented Dec 16, 2020 •

edited

Loading

repokitteh-read-only bot commented Dec 16, 2020

markdroth commented Dec 16, 2020

kyessenov commented Dec 16, 2020 •

edited

Loading

gargnupur Dec 16, 2020

kyessenov Dec 16, 2020

markdroth Dec 17, 2020

markdroth Dec 17, 2020

kyessenov Dec 17, 2020

markdroth Dec 17, 2020

htuch Dec 17, 2020 •

edited

Loading

kyessenov Dec 17, 2020

mattklein123 Dec 17, 2020

kyessenov Dec 22, 2020

kyessenov commented Dec 23, 2020

kyessenov commented Jan 5, 2021

mattklein123 commented Jan 5, 2021

kyessenov commented Jan 6, 2021 •

edited

Loading

kyessenov commented Jan 7, 2021

repokitteh-read-only bot commented Jan 7, 2021

kyessenov commented Jan 7, 2021

kyessenov commented Jan 8, 2021

repokitteh-read-only bot commented Jan 8, 2021

kyessenov commented Jan 8, 2021

repokitteh-read-only bot commented Jan 8, 2021

kyessenov commented Jan 8, 2021

kyessenov commented Jan 10, 2021

moderation commented Jan 10, 2021

mattklein123 left a comment

rate limit: add computed descriptors #14448

rate limit: add computed descriptors #14448

Conversation

kyessenov commented Dec 16, 2020 • edited Loading

repokitteh-read-only bot commented Dec 16, 2020

markdroth commented Dec 16, 2020

kyessenov commented Dec 16, 2020 • edited Loading

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

htuch Dec 17, 2020 • edited Loading

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

kyessenov commented Dec 23, 2020

kyessenov commented Jan 5, 2021

mattklein123 commented Jan 5, 2021

kyessenov commented Jan 6, 2021 • edited Loading

kyessenov commented Jan 7, 2021

repokitteh-read-only bot commented Jan 7, 2021

kyessenov commented Jan 7, 2021

kyessenov commented Jan 8, 2021

repokitteh-read-only bot commented Jan 8, 2021

kyessenov commented Jan 8, 2021

repokitteh-read-only bot commented Jan 8, 2021

kyessenov commented Jan 8, 2021

kyessenov commented Jan 10, 2021

moderation commented Jan 10, 2021

mattklein123 left a comment

Choose a reason for hiding this comment

kyessenov commented Dec 16, 2020 •

edited

Loading

kyessenov commented Dec 16, 2020 •

edited

Loading

htuch Dec 17, 2020 •

edited

Loading

kyessenov commented Jan 6, 2021 •

edited

Loading