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

Enhance creation of RateLimitPolicy around missing limits. #1024

Merged
merged 3 commits into from
Nov 18, 2024

Conversation

Boomatang
Copy link
Contributor

@Boomatang Boomatang commented Nov 14, 2024

Ensure the user can create a ratelimit policy without at least one limit.

closes: #1023

Validate

Try apply the following policies to the installation. They should both fail with different failure messages. Any policy where the name is toystore-*1 should be applied to the cluster.

apiVersion: kuadrant.io/v1
kind: RateLimitPolicy
metadata:
  name: toystore-s1
  namespace: default
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: simple-toystore
  limits: 
    "limit-per-ip":
      rates:
      - limit: 5
        window: '10s'
---
apiVersion: kuadrant.io/v1
kind: RateLimitPolicy
metadata:
  name: toystore-s2
  namespace: default
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: simple-toystore
---
apiVersion: kuadrant.io/v1
kind: RateLimitPolicy
metadata:
  name: toystore-s3
  namespace: default
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: simple-toystore
  limits: {} 
---
apiVersion: kuadrant.io/v1
kind: RateLimitPolicy
metadata:
  name: toystore-d1
  namespace: default
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: simple-toystore
  defaults:
    limits: 
      "limit-per-ip":
        rates:
        - limit: 5
          window: '10s'
---
apiVersion: kuadrant.io/v1
kind: RateLimitPolicy
metadata:
  name: toystore-d2
  namespace: default
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: simple-toystore
  defaults: {}
---
apiVersion: kuadrant.io/v1
kind: RateLimitPolicy
metadata:
  name: toystore-d3
  namespace: default
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: simple-toystore
  defaults:
    limits: {}
---
apiVersion: kuadrant.io/v1
kind: RateLimitPolicy
metadata:
  name: toystore-o1
  namespace: default
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: simple-toystore
  overrides:
    limits: 
      "limit-per-ip":
        rates:
        - limit: 5
          window: '10s'
---
apiVersion: kuadrant.io/v1
kind: RateLimitPolicy
metadata:
  name: toystore-o2
  namespace: default
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: simple-toystore
  overrides: {}
---
apiVersion: kuadrant.io/v1
kind: RateLimitPolicy
metadata:
  name: toystore-o3
  namespace: default
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: simple-toystore
  overrides:
    limits:  {}
---

@Boomatang Boomatang self-assigned this Nov 14, 2024
Ensure the user can create a ratelimit policy without at least one limit.

Signed-off-by: Jim Fitzpatrick <jfitzpat@redhat.com>
@Boomatang Boomatang marked this pull request as ready for review November 14, 2024 19:19
Copy link
Contributor

@eguzki eguzki left a comment

Choose a reason for hiding this comment

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

I miss a integration test (with actual k8s running) in tests/common/ratelimitpolicy/ratelimitpolicy_controller_test.go for the following use cases:

  • no defaults, no overrides, no limits -> client return error on create
  • with defaults, no limits -> client return error on create
  • with overrides, no limits -> client return error on create

@Boomatang
Copy link
Contributor Author

I miss a integration test (with actual k8s running) in tests/common/ratelimitpolicy/ratelimitpolicy_controller_test.go for the following use cases:

I can add some test no problem.

@Boomatang Boomatang changed the title IMPROVE: validation Enhance creation of RateLimitPolicy around missing limits. Nov 15, 2024
Add integration tests for the six states that the user can try add invaild limit blocks.
This does not check if the limits are valid only that they are there.

Signed-off-by: Jim Fitzpatrick <jfitzpat@redhat.com>
@@ -153,6 +153,9 @@ func (p *RateLimitPolicy) Kind() string {
// +kubebuilder:validation:XValidation:rule="!(has(self.defaults) && has(self.limits))",message="Implicit and explicit defaults are mutually exclusive"
// +kubebuilder:validation:XValidation:rule="!(has(self.defaults) && has(self.overrides))",message="Overrides and explicit defaults are mutually exclusive"
// +kubebuilder:validation:XValidation:rule="!(has(self.overrides) && has(self.limits))",message="Overrides and implicit defaults are mutually exclusive"
// +kubebuilder:validation:XValidation:rule="!(has(self.overrides) || has(self.defaults)) ? has(self.limits) && size(self.limits) > 0 : true",message="At least one spec.limits most be defined"
Copy link
Contributor

Choose a reason for hiding this comment

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

**most** be defined 👀

Signed-off-by: Jim Fitzpatrick <jfitzpat@redhat.com>
Copy link
Contributor

@KevFan KevFan left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍

@Boomatang Boomatang merged commit c8d0838 into Kuadrant:main Nov 18, 2024
26 checks passed
@Boomatang Boomatang deleted the issue/1023 branch November 18, 2024 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Incorrect status.condition Message When `spec.limil is Missing in RLP
3 participants