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

Improvements to RateLimitPolicy #133

Closed
pehala opened this issue Jan 11, 2023 · 4 comments · Fixed by #144
Closed

Improvements to RateLimitPolicy #133

pehala opened this issue Jan 11, 2023 · 4 comments · Fixed by #144
Assignees
Labels
area/api Changes user facing APIs kind/enhancement New feature or request

Comments

@pehala
Copy link
Contributor

pehala commented Jan 11, 2023

Currently, the minimal working example (At least what I could do) looks like this. However, this could be much easier for the simple limit I want to enforce.

apiVersion: kuadrant.io/v1beta1
kind: RateLimitPolicy
metadata:
  name: gw-limit
  namespace: istio-system
spec:
  rateLimits:
    - configurations:
        - actions:
            - generic_key:
                descriptor_key: limited
                descriptor_value: '1'
      limits:
        - conditions: []
          maxValue: 5
          seconds: 10
          variables: []
  targetRef:
    group: gateway.networking.k8s.io
    kind: Gateway
    name: istio-ingressgateway

Notable problems:

  • configurations cannot be empty or the rate limit won't work, and it is not always needed
    • Solution 1: If it is empty, Kuadrant could generate a static action key.
    • Solution 2: Make Kuadrant throw an error if it is empty.
  • conditions and variables are not required but need to be specified at least as an empty list:
    • Solution: Make them optional and if missing, assign an empty list.

Please let me know what you think. These are just what I see as problems from my perspective.

@pehala
Copy link
Contributor Author

pehala commented Jan 17, 2023

In my opinion, the simple limit should look like this:

apiVersion: kuadrant.io/v1beta1
kind: RateLimitPolicy
metadata:
  name: gw-limit
  namespace: istio-system
spec:
  rateLimits:
    - limits:
        - maxValue: 5
          seconds: 10
  targetRef:
    group: gateway.networking.k8s.io
    kind: Gateway
    name: istio-ingressgateway

@maleck13
Copy link
Collaborator

Yes as something that applies to all requests on the target this reads well to me. As discussed on the call, it seemed possible for the kuadrant controller to be able to generate a descriptor if there were none present and could use the namespace and resource name as that key

@thomasmaas
Copy link
Contributor

@alexsnaps alexsnaps moved this from Needs refinement to To do in Kuadrant Service Protection Jan 19, 2023
@alexsnaps alexsnaps added area/api Changes user facing APIs and removed area/api Changes user facing APIs labels Jan 19, 2023
@eguzki eguzki moved this from To do to In Progress in Kuadrant Service Protection Jan 26, 2023
@eguzki eguzki self-assigned this Jan 26, 2023
@eguzki eguzki mentioned this issue Jan 27, 2023
2 tasks
@github-project-automation github-project-automation bot moved this from In Progress to Done in Kuadrant Service Protection Feb 13, 2023
@thomasmaas thomasmaas reopened this Feb 13, 2023
@github-project-automation github-project-automation bot moved this from Done to To test in Kuadrant Service Protection Feb 13, 2023
@pehala
Copy link
Contributor Author

pehala commented May 2, 2023

Tested, PR: Kuadrant/testsuite#202

@pehala pehala moved this from To test to Done in Kuadrant Service Protection May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Changes user facing APIs kind/enhancement New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants