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

Simplify RateLimitPolicy #144

Merged
merged 7 commits into from
Feb 13, 2023
Merged

Simplify RateLimitPolicy #144

merged 7 commits into from
Feb 13, 2023

Conversation

eguzki
Copy link
Contributor

@eguzki eguzki commented Jan 27, 2023

What

Make some fields of the RateLimitPolicy optional to make the required CR object smaller and simpler. After this changes, the simplest RLP instance looks like this:

---
apiVersion: kuadrant.io/v1beta1
kind: RateLimitPolicy
metadata:
  name: toystore
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: toystore
  rateLimits:
    - limits:
        - maxValue: 5
          seconds: 10

Only targetRef and one limit object is required. Furthermore, the limit object does not require conditions and variables fields.

When the Kuadrant control plane finds a rate limit policy that does not specify any action configuration, the Kuadrant control plane
will assign a generic default action configuration for the traffic related to the targeted network resource. This default action configuration allows defining global limits for all the traffic related to the targeted network resource.

[BREAKING CHANGE] (or maybe not). Technically this is breaking the current v1beta1 API. However, I do not expect issues at all. The changes switch some fields from required to optional. Thus, any existing "old" RLP instance will still be valid with the "new" CRD. No need to conversion either.

TODO:

  • unittests and e2e tests
  • Doc

Fixes #133

Verification steps

run dev env

make local-setup

Create kuadrant CR

k apply -f - <<EOF
---
apiVersion: kuadrant.io/v1beta1
kind: Kuadrant
metadata:
  name: kuadrant
spec: {}
EOF

Deploy toystore upstream service

kubectl apply -f examples/toystore/toystore.yaml

Create HTTPRoute

k apply -f examples/toystore/httproute.yaml

Check toystore HTTPRoute works

kubectl port-forward -n istio-system service/istio-ingressgateway 9080:80 &
curl -v -H 'Host: api.toystore.com' http://localhost:9080/toy

It should return 200 OK.

Create simplified RLP

k apply -f - <<EOF
---
apiVersion: kuadrant.io/v1beta1
kind: RateLimitPolicy
metadata:
  name: toystore
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: toystore
  rateLimits:
    - limits:
        - maxValue: 5
          seconds: 10
EOF

Wait for the policy to be available (and avoid possible errors)

k wait --for=condition=available ratelimitpolicy/toystore

Validating the rate limit policy

Only 5 requests every 10 secs on rate-limited.toystore.com allowed.

curl -v -H 'Host: rate-limited.toystore.com' http://localhost:9080/toy

@eguzki eguzki requested a review from a team January 27, 2023 15:25
@eguzki eguzki force-pushed the rlp-default-gw-config branch from 1df7e2c to 7c6619b Compare January 30, 2023 16:42
@eguzki eguzki force-pushed the rlp-default-gw-config branch 2 times, most recently from a759480 to a2372f3 Compare February 9, 2023 11:02
@eguzki eguzki force-pushed the rlp-default-gw-config branch from d10d3d1 to e8a9fa8 Compare February 10, 2023 10:47
@eguzki eguzki marked this pull request as ready for review February 10, 2023 10:48
@eguzki
Copy link
Contributor Author

eguzki commented Feb 10, 2023

ready for review @Kuadrant/engineering

@maleck13
Copy link
Collaborator

@eguzki I don't feel qualified to approve the PR but I love this change and take a look through it looks reasonable

@didierofrivia
Copy link
Member

The verification steps are working 👍🏼

Copy link
Contributor

@guicassolato guicassolato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the comments. I think this is only still relevant, but even so I'm guessing it won't matter for too long.

Verification steps work. Approved.

@eguzki eguzki merged commit faf41ff into main Feb 13, 2023
@eguzki eguzki deleted the rlp-default-gw-config branch February 13, 2023 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improvements to RateLimitPolicy
4 participants