Add Ratelimit API

[hzxuzhonghu]

Based on the design

[istio-policy-bot]

😊 Welcome @hzxuzhonghu! This is either your first contribution to the Istio api repo, or it's been awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

[istio-testing]

@hzxuzhonghu: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
release-notes_api	ab20dd7842150524051994d84797257b37063860	link	/test release-notes_api

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[hzxuzhonghu]

cc @gargnupur @mandarjog @bianpengyuan @douglas-reid @howardjohn @ramaraochavali

[costinm]

On Mon, Dec 7, 2020 at 4:47 AM Zhonghu Xu [email protected] wrote:

@hzxuzhonghu commented on this pull request.

In networking/v1alpha3/virtual_service.proto https://github.com/istio/api/pull/1767#discussion_r537479254:

• // If not set it defaults to ‘generic_key’ as the descriptor key.

• string descriptor_key = 1;

• // The value to use in the descriptor entry.

• string descriptor_value = 2 [(google.api.field_behavior) = REQUIRED];

• }

+}

+// HTTPLocalRateLimit used to rate limit the HTTP requests locally

+message HTTPLocalRateLimit {

• // StatusCode allows for a custom HTTP response status code to the downstream client

• // when the request has been rate limited. Defaults to 429 (TooManyRequests).

• int32 status_code = 1;

• // The token bucket configuration to use for rate limiting requests.

• TokenBucket token_bucket = 2 [(google.api.field_behavior) = REQUIRED];

Yeah, request attributes is not supported with envoy's local rate limit.

From an API perspective, the question is if this is a fundamental design/intentional choice, that needs to be reflected in the API design - or just a limitation of the current implementation.

I strongly suspect it's the later.

In addition, advanced rate limit ( as API ) may use a combination of local token bucket and remote calls.

I think it is critical to agree that a Istio RateLimit API should be an abstraction that allows multiple implementations - including 'proxy-less' or non-envoy proxies, as well as different implementations inside envoy ( WASM, other styles ).

So I would also use a single proto to represent both rate limits, and maybe add a section to the design doc with the previous rate limit API we had ( when mixer provided an implementation ), and maybe an analysis of what was wrong with the old API and explain how we want to improve.

I also strongly believe in "less is more" - so maybe we can start with a smaller API, and later if we find the need to have 4 we can add the others ( i.e. dedicated TCP pair, and local/remote differentiation)

[costinm]

On Mon, Dec 7, 2020 at 4:20 AM Zhonghu Xu [email protected] wrote:

@hzxuzhonghu commented on this pull request.

In mesh/v1alpha1/config.proto https://github.com/istio/api/pull/1767#discussion_r537463500:

@@ -631,3 +635,27 @@ message Certificate { // multiple DNS names. repeated string dns_names = 2; }

+// RateLimitService describes the configuration for an external rate limit service provider. +message RateLimitService {

Generally, i think this should be global. It is easy to operate. I understand ProxyConfig is used to generate configs for proxy in proxy side. But rls is used in istiod. As ExtensionProvider , it is only for external authz.

Why ? In a large org, MeshConfig is operated by the 'istiod admin team'. We are trying to reduce the pressure on the istiod team ( which may be an external vendor for 'managed istiod' ). Without Istio a user has the ability to deploy its own redis ( or other rate limit service ) and configure its workloads to use them without requesting a central team to make changes ( that involve running operator and in practice restart Istio - at least with current istioctl or helm, since MeshConfig is overridden )

ExtensionProvider: I agree, it was designed for external authz and unfortunately uses a generic and confusing name that suggest that all external providers will be covered. To be fair, there is a need for a common style and design for all integrations - I don't think ExtensionProvider in MeshConfig is the right one, in part for the reasons above but there are far more issues. However telemetry team is attempting to use it, and TOC may override my objections, so I have to ask the question: if TOC decides ExtensionProvider is required, this PR will also be affected.

[costinm]

On Mon, Dec 7, 2020 at 4:20 AM Zhonghu Xu [email protected] wrote:

@hzxuzhonghu commented on this pull request.

In mesh/v1alpha1/config.proto https://github.com/istio/api/pull/1767#discussion_r537463500:

@@ -631,3 +635,27 @@ message Certificate { // multiple DNS names. repeated string dns_names = 2; }

+// RateLimitService describes the configuration for an external rate limit service provider. +message RateLimitService {

Generally, i think this should be global. It is easy to operate. I understand ProxyConfig is used to generate configs for proxy in proxy side. But rls is used in istiod. As ExtensionProvider , it is only for external authz.

MeshConfig.default is also gobal, and as easy to operate as fields in MeshConfig ( yes, users have the ability to override - but they don't have to).

ProxyConfig is intended for configs affecting the proxy - doesn't matter who generates the configs. For example we discussed the option to generate the bootstrap in istiod, and tracing which is now in bootstrap is moving to proper dynamic config.

I think we are confusing implementation details ( where some code happens to run or how API is implemented ) with the intent. For example if we wanted Istiod itself to make rate limit calls - I would put it in MeshConfig, because it would clearly not be a config of the proxies. But everything that affects the Proxy - I would keep in ProxyConfig.

[gargnupur]

We have 2 open design docs, and now a PR, for the same feature. Can we keep discussions in one place?

Agreed... it might be easier to just have a meeting and sort it out.. @hzxuzhonghu : what do you think? Looks like networking group sync up is coming this week, we can use time after that?

Docs in review:

1. https://docs.google.com/document/d/1628LHcwuCvTRFhk8rsQKQUmgCxnY6loPFSxliADQDIc/edit?ts=5fc57f4f#heading=h.ob0qozngww1j
2. https://docs.google.com/document/d/1N4eeCuBme6ljXDlnBjObHRuOLhLcawkV7pP7TQDiO64/edit#heading=h.xw1gqgyqs5b

[hzxuzhonghu]

The meeting time is not friendly for my timezone(1am for me), can we make it like PST 17:00

[gargnupur]

@hzxuzhonghu : We are discussing, good time to meet.. will get back to you in a day or two..

[costinm]

We can discuss in the networking meeting, currently at 9 PST.

PST 17:00 sounds good too - maybe we should discuss moving the networking meeting at this time if it is more convenient for people. I don't think we have a lot of participants from Europe, but East Coast people may be unhappy.

I am starting to lean towards simply defining separate protos, in dedicated packages (rate.istio.io/v1alpha1) - with an eye towards adopting external specs or converging with other APIs in this space instead of istio-only - and moving them out of istio.io space. Similar with what we plan for Gateway ( moving towards K8S CR)

On Mon, Dec 7, 2020 at 6:48 PM Zhonghu Xu [email protected] wrote:

The meeting time is not friendly for my timezone(1am for me), can we make it like PST 17:00

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/istio/api/pull/1767#issuecomment-740325452, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUR2VRCQJWZFZWCFHPA53STWHYZANCNFSM4UKGNZ4Q .

[hzxuzhonghu]

I am starting to lean towards simply defining separate protos, in dedicated packages (rate.istio.io/v1alpha1)

I am all for this, what's more, we should provide a ratelimit server which can also consume this api, so users donot need to config twice(both in istio and the rate limit server side). Previously when we have mixer, this is natural.

[hzxuzhonghu]

@costinm @gargnupur This is a separate API that is much direct and without coupling with VS. It can be applied on inbound/outbound based on users' choice. https://docs.google.com/document/d/1ySvR6s-6Ngs0Uaj_e3-8MaM4bkm44I_HkHLuP6fABVA/edit#

[istio-testing]

@hzxuzhonghu: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[SpecialYang]

How is this pr going? Need this feature, too.

[hzxuzhonghu]

@kyessenov @mandarjog @gargnupur @costinm @howardjohn I am not sure how to push this feature forward, I am starting working on it since last year, and it has been half year passed, and we haven't come to any agreement. Most users including us are using envoyfilter or designed a separate CRD, and translate it to envoyflter, which is very bad UX.

cc @istio/technical-oversight-committee is this still on the roadmap?

[kyessenov]

@hzxuzhonghu It's still we something we want. Besides the concerns around the low level details of actions/descriptors, we need to figure out how to decouple rate limit policy from networking APIs.

[hzxuzhonghu]

@kyessenov You are absolutely quite right, with a separate API, maybe we could implement rate limit as a plugin as auth does.

[jwendell]

Is this obsolete by https://github.com/istio/api/pull/2028 ?

[hzxuzhonghu]

I think so, though it has some downsides