Skip to content

Commit

Permalink
rfc: RateLimitPolicy cleanup
Browse files Browse the repository at this point in the history
  • Loading branch information
@guicassolato
guicassolato committed Feb 6, 2023
1 parent 953f620 commit 0cc37bd
Showing 1 changed file with 331 additions and 0 deletions.
331 changes: 331 additions & 0 deletions rfcs/0000-rlp-cleanup.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,331 @@
# RFC RateLimitPolicy cleanup

- Feature Name: `rlp-v2`
- Start Date: (fill me in with today's date, YYYY-MM-DD)
- RFC PR: [Kuadrant/architecture#0000](https://github.com/Kuadrant/architecture/pull/0000)
- Issue tracking: [Kuadrant/architecture#0000](https://github.com/Kuadrant/architecture/issues/0000)

## Summary
[summary]: #summary

Proposal of clean-up of Kuadrant's `RateLimitPolicy` API (RLP) for improved UX.

## Motivation
[motivation]: #motivation

The [`RateLimitPolicy`](https://pkg.go.dev/github.com/kuadrant/kuadrant-operator@v0.2.1/api/v1beta1#RateLimitPolicy) API (v1beta1), particularly its [`RateLimit`](https://pkg.go.dev/github.com/kuadrant/kuadrant-operator@v0.2.1/api/v1beta1#RateLimit) type used in `ratelimitpolicy.spec.rateLimits`, designed in part to fit the underlying implementation based on the Envoy [Rate limit](https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/rate_limit_filter) filter, has been proven to be (i) _unnecessarily complex_, as well as (ii) possibly _limiting for the extension of the API_ either beyond this specific implementation and/or for supporting use cases of rate limiting not contemplated in the original design, such as comprising the concept of _weights_.

Users of the `RateLimitPolicy` will immediatelly recognize elements of Envoy's Rate limit API in the definitions of the `RateLimit` type, with almost 1:1 correspondence between the [`Configuration`](https://pkg.go.dev/github.com/kuadrant/kuadrant-operator@v0.2.1/api/v1beta1#Configuration) and [`Rule`](https://pkg.go.dev/github.com/kuadrant/kuadrant-operator@v0.2.1/api/v1beta1#Rule) types and their counterparts in the Envoy configuration. Although compatibility between those continue to be desired, the leakage of such implementation details to the level of the API could otherwise be avoided to provide a better abstraction for the context in vogue, where activators ("matchers") and payload ("descriptors") are defined by users in a same document instead of in different configurations of different components.

Furthermore, the also related [`Limit`](https://pkg.go.dev/github.com/kuadrant/kuadrant-operator@v0.2.1/api/v1beta1#Limit) type – used as well in the `RateLimitPolicy`'s `RateLimit` – implies presently a logical relationship between its inner concepts – i.e. conditions and variables on one side, and limits themselves on the other – that otherwise could be shaped in a different manner, to provide (i) clearer understanding of the meaning of these concepts by the user as well as (ii) to avoid repetition.

### Goals of cleaning up the RLP

1. Decouple the API from the underlying implementation - i.e. provide a more generic abstraction
2. Simplify the API - i.e. DRY, straight to the point (for defining rate limits)
3. Consistency with Kuadrant's [AuthPolicy](https://pkg.go.dev/github.com/kuadrant/kuadrant-operator@v0.2.1/api/v1beta1#AuthPolicy) API - i.e. same language, similar UX

### Current WIP to consider while discussing the cleaning up of the RLP

1. Inheritance of the network matching rules from the upstream network object (https://github.com/Kuadrant/architecture/pull/4)
2. Improvements to RateLimitPolicy - aka "minimal working example" without `configurations`, `limits.conditions` and `limits.variables` (https://github.com/Kuadrant/kuadrant-operator/issues/133)
3. Implement `skip_if_absent` for the RequestHeaders action (https://github.com/Kuadrant/wasm-shim/issues/29)
4. Rate Limiting across clusters ([doc](https://docs.google.com/document/d/1pqCODRAkNUTLB6lJfRWcv3d2Hlj9WqsGySmjrP707Vk/edit#heading=h.nzpgr3pef6uy))

## Guide-level explanation
[guide-level-explanation]: #guide-level-explanation

Given the following HTTPRoute:

```yaml
apiVersion: gateway.networking.k8s.io/v1alpha2
kind: HTTPRoute
metadata:
name: toystore
labels:
app: toystore
spec:
parentRefs:
- name: istio-ingressgateway
namespace: istio-system
hostnames: ["*.toystore.com"]
rules:
- matches:
- path:
type: PathPrefix
value: "/toys"
method: GET
- path:
type: PathPrefix
value: "/toys"
method: POST
backendRefs:
- name: toystore
port: 80
```
These are examples of RateLimitPolicies targeting the route:
**Example 1.** Minimal example - unconditional, unqualified rate limiting
```yaml
apiVersion: kuadrant.io/v2beta1
kind: RateLimitPolicy
metadata:
name: toystore
spec:
targetRef:
group: gateway.networking.k8s.io
kind: HTTPRoute
name: toystore
limits:
- rates:
- limit: 5
unit: minute
```
**Example 2.** All you can get - conditions, counter qualifiers, multiple rates, and weights
```yaml
apiVersion: kuadrant.io/v2beta1
kind: RateLimitPolicy
metadata:
name: toystore
spec:
targetRef:
group: gateway.networking.k8s.io
kind: HTTPRoute
name: toystore
limits:
- name: writers
selectors:
# this is a condition
- selector: request.http.method
operator: eq
value: POST
# this is counter qualifier
- selector: metadata.auth.identity.username
rates:
- limit: 5
duration: 1
unit: minute
- limit: 100
duration: 12
unit: hour
weight: 1

- name: readers
selectors:
- selector: request.http.method
operator: eq
value: GET
- selector: metadata.auth.identity.username
rates:
- limit: 50
duration: 1
unit: minute
weight: 1

- name: read-expensive
selectors:
- selector: request.http.method
operator: eq
value: GET
- selector: request.http.path
operator: eq
value: /toys/expensive
rates:
- limit: 10
duration: 1
unit: minute
weight: 1
```
## Reference-level explanation
[reference-level-explanation]: #reference-level-explanation
### Differences and similarities compared to current RateLimitPolicy
<table>
<thead>
<tr>
<th>Current</th>
<th>New</th>
<th>Reason</th>
</tr>
</thead>
<tbody>
<tr>
<td>1:1 relation between <i>Limit</i> (object) and the actual <i>Rate limit</i> (value)</td>
<td><i>Rate</i> becomes a detail of <i>Limit</i> where each limit may define one or more rates (1:N)</td>
<td>
<ul>
<li>It allows to reuse <i>selectors</i> (previously "conditions" and "variables") for groups of rate limits</li>
</ul>
</td>
</tr>
<tr>
<td><i>Conditions</i> and <i>variables</i> in separate fields of the limit</td>
<td><i>Selectors</i> – with provided relational value ("conditions") or without provided relational value ("counter qualifiers")</td>
<td>
<ul>
<li>Simpler structure for defining the "scope" of a group of rate limits</li>
</ul>
</td>
</tr>
<tr>
<td>Parsed conditions</td>
<td>Structured condition fields composed of <code>selector</code>, <code>operator</code> and <code>value</code></td>
<td>
<ul>
<li>Feels more K8s-native</li>
<li>Consistent with <a href="https://pkg.go.dev/github.com/kuadrant/authorino/api/v1beta1#JSONPatternExpression">github.com/kuadrant/authorino/api/v1beta1#JSONPatternExpression</a></li>
<li>No need for a parser (only if implemented by Limitador)</li>
</ul>
</td>
</tr>
<tr>
<td><i>Configurations</i> as a list of variables assignments ("actions" for building "descriptors")</td>
<td>Descriptors computed from <i>selectors</i> effectively used to specify conditions and counter qualifiers</td>
<td>
<ul>
<li>Abstract Envoy-specific concepts of "actions" and "descriptors"</li>
<li>No room for useless invariant descriptors (e.g. "limited = 1")</li>
<li>Source value of the selectors defined from an implicit "context" data structure</li>
</ul>
</td>
</tr>
<tr>
<td>Key-value descriptors</td>
<td>Structured descriptors from a "context" data structure</td>
<td>
<ul>
<li>Consistent with Authorino's <a href="https://github.com/Kuadrant/authorino/blob/main/docs/architecture.md#the-authorization-json">Authorization JSON (#context)</a></li>
</ul>
</td>
</tr>
<tr>
<td><code>maxValue</code></td>
<td><code>limit</code></td>
<td></td>
</tr>
<tr>
<td><code>seconds</code></td>
<td><code>duration</code> and <code>unit</code></td>
<td>
<ul>
<li>Support for more units beyond seconds</li>
<li><code>duration: 1</code> by default</li>
</ul>
</td>
</tr>
<tr>
<td>No <i>weights</i> (or implicit <code>weight: 1</code>)</td>
<td><code>weight: VALUE</code></td>
<td>
<ul>
<li>Placeholder for (more complex) weighting systems in the future – e.g. %, 𝑓(), etc</li>
<li>Migration path from other RL solutions, e.g. 3scale (?)</li>
</ul>
</td>
</tr>
</tbody>
</table>
## Drawbacks
[drawbacks]: #drawbacks
Why should we *not* do this?
## Rationale and alternatives
[rationale-and-alternatives]: #rationale-and-alternatives
### Possible variations for the selectors (conditions and counter qualifiers)
The main drivers behind the proposed "selectors" designed based on structured condition expressions and single-field for conditions and counter qualifiers (variation "A" below) were:
1. Consistency with the Authorino API
2. Simplicity
Nonetheless here are a few alternative variations to consider:
<table>
<thead>
<tr>
<th></th>
<th>Structured condition expressions</th>
<th>Parsed condition expressions</th>
</tr>
</thead>
<tbody>
<tr>
<th>Single field</th>
<td>
<b>A</b>
<pre>
selectors:
- selector: request.http.method
operator: eq
value: GET
- selector: metadata.auth.identity.username</pre>
</td>
<td>
<b>B</b>
<pre>
selectors:
- request.http.method == "GET"
- metadata.auth.identity.username</pre>
</td>
</tr>
<tr>
<th>Distinct fields</th>
<td>
<b>C</b>
<pre>
conditions:
- selector: request.http.method
operator: eq
value: GET
counters:
- metadata.auth.identity.username</pre>
</td>
<td>
<b>D</b>
<pre>
conditions:
- request.http.method == "GET"
counters:
- metadata.auth.identity.username</pre>
</td>
</tr>
</tbody>
</table>
## Prior art
[prior-art]: #prior-art
Discuss prior art, both the good and the bad, in relation to this proposal.
A few examples of what this can include are:
- Does another project have a similar feature?
- What can be learned from it? What's good? What's less optimal?
- Papers: Are there any published papers or great posts that discuss this? If you have some relevant papers to refer to, this can serve as a more detailed theoretical background.
This section is intended to encourage you as an author to think about the lessons from other tentatives - successful or not, provide readers of your RFC with a fuller picture.
Note that while precedent set by other projects is some motivation, it does not on its own motivate an RFC.
## Unresolved questions
[unresolved-questions]: #unresolved-questions
- What parts of the design do you expect to resolve through the RFC process before this gets merged?
- What parts of the design do you expect to resolve through the implementation of this feature before stabilization?
- What related issues do you consider out of scope for this RFC that could be addressed in the future independently of the solution that comes out of this RFC?
## Future possibilities
[future-possibilities]: #future-possibilities
Think about what the natural extension and evolution of your proposal would be and how it would affect the platform and project as a whole. Try to use this section as a tool to further consider all possible interactions with the project and its components in your proposal. Also consider how this all fits into the roadmap for the project and of the relevant sub-team.
This is also a good place to "dump ideas", if they are out of scope for the RFC you are writing but otherwise related.
Note that having something written down in the future-possibilities section is not a reason to accept the current or a future RFC; such notes should be in the section on motivation or rationale in this or subsequent RFCs. The section merely provides additional information.

0 comments on commit 0cc37bd

Please sign in to comment.