Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RateLimitPolicy v1beta4 #951

Open
alexsnaps opened this issue Oct 22, 2024 · 6 comments
Open

RateLimitPolicy v1beta4 #951

alexsnaps opened this issue Oct 22, 2024 · 6 comments

Comments

@alexsnaps
Copy link
Member

alexsnaps commented Oct 22, 2024

Ideally our APIs feel similar... This proposal here is heavily influenced by the introduction of CEL in AuthConfig, as these types will "leak" to our AuthPolicy eventually, here's how this could look like for RateLimitPolicy:

apiVersion: kuadrant.io/v1beta4
kind: RateLimitPolicy
metadata:
  name: toystore-gw
  namespace: gateway-system
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: Gateway
    name: kuadrant-ingressgateway
  when:
    - predicate: auth.identity.group == "admin" # a list of "top-level" `Predicate`s
  limits:
    "expensive-operation":
      rates:
        - limit: 2
          window: 30s # a duration as we now express them elsewhere and in GwAPI
      when:
        - predicate: request.method == "POST" # a list of per-limit `Predicate`s as were our `Condition`s, but CEL

    "limit-per-ip":
      rates:
        - limit: 5
          window: 30s
      counters:
        - expression: source.remote_address # this maps more or less to what we do in authorino wrt `properties` et al 
@eguzki
Copy link
Contributor

eguzki commented Oct 22, 2024

We can keep v1beta3 as it has not been released yet. v1beta4 also works for me

@alexsnaps
Copy link
Member Author

expression & predicate are redundant here, but might still be desirable because of the AuthConfig... wdyt @eguzki @guicassolato ?

@eguzki
Copy link
Contributor

eguzki commented Oct 22, 2024

do we want the counter to be a CEL expression? What is the point of being CEL expression? Maybe it is enough with one of the following?

counters:
- attribute: source.remote_address 

or

counters:
- source.remote_address 

@alexsnaps
Copy link
Member Author

do we want the counter to be a CEL expression?

how are you example not CEL expresssions? what's source.remote_address? a... Selector again?

@eguzki
Copy link
Contributor

eguzki commented Oct 22, 2024

I was thinking on a simple selector for counters. Yes. But maybe we can leverage CEL to do some transformations

counters:
- dayOfTheWeek(request.time)

Could be handy

@alexsnaps
Copy link
Member Author

alexsnaps commented Oct 22, 2024

Right, plus there is no benefit in treating the two differently, more so that they look exactly the same!
Question is, how do we send the data when:

counters:
 - expression: request

I could argue for JSON serialization (on a single line) as this is how Authorino currently sends the data back too. But I would think that CEL literals would be better. Good news is that it pretty much becomes opaque to the user at this stage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

No branches or pull requests

2 participants