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.

Sign up for GitHub

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

Jump to bottom

Enhance creation of RateLimitPolicy around missing limits. #1024

Merged
merged 3 commits into from
Nov 18, 2024
Merged

Enhance creation of RateLimitPolicy around missing limits. #1024

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c11c535
IMPROVE: validation
Boomatang Nov 14, 2024
dea4490
Integration tests.
Boomatang Nov 15, 2024
d79a7d0
Fix: grammer
Boomatang Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions api/v1/ratelimitpolicy_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 👀

// +kubebuilder:validation:XValidation:rule="has(self.overrides) ? has(self.overrides.limits) && size(self.overrides.limits) > 0 : true",message="At least one spec.overrides.limits most be defined"
eguzki marked this conversation as resolved.
Show resolved Hide resolved
// +kubebuilder:validation:XValidation:rule="has(self.defaults) ? has(self.defaults.limits) && size(self.defaults.limits) > 0 : true",message="At least one spec.defaults.limits most be defined"
type RateLimitPolicySpec struct {
// Reference to the object to which this policy applies.
// +kubebuilder:validation:XValidation:rule="self.group == 'gateway.networking.k8s.io'",message="Invalid targetRef.group. The only supported value is 'gateway.networking.k8s.io'"
Expand Down
2 changes: 1 addition & 1 deletion bundle/manifests/kuadrant-operator.clusterserviceversion.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ metadata:
capabilities: Basic Install
categories: Integration & Delivery
containerImage: quay.io/kuadrant/kuadrant-operator:latest
createdAt: "2024-11-13T16:31:14Z"
createdAt: "2024-11-14T19:01:18Z"
description: A Kubernetes Operator to manage the lifecycle of the Kuadrant system
operators.operatorframework.io/builder: operator-sdk-v1.32.0
operators.operatorframework.io/project_layout: go.kubebuilder.io/v3
Expand Down
9 changes: 9 additions & 0 deletions bundle/manifests/kuadrant.io_ratelimitpolicies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,15 @@ spec:
rule: '!(has(self.defaults) && has(self.overrides))'
- message: Overrides and implicit defaults are mutually exclusive
rule: '!(has(self.overrides) && has(self.limits))'
- message: At least one spec.limits most be defined
rule: '!(has(self.overrides) || has(self.defaults)) ? has(self.limits)
&& size(self.limits) > 0 : true'
- message: At least one spec.overrides.limits most be defined
rule: 'has(self.overrides) ? has(self.overrides.limits) && size(self.overrides.limits)
> 0 : true'
- message: At least one spec.defaults.limits most be defined
rule: 'has(self.defaults) ? has(self.defaults.limits) && size(self.defaults.limits)
> 0 : true'
status:
properties:
conditions:
Expand Down
9 changes: 9 additions & 0 deletions charts/kuadrant-operator/templates/manifests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7994,6 +7994,15 @@ spec:
rule: '!(has(self.defaults) && has(self.overrides))'
- message: Overrides and implicit defaults are mutually exclusive
rule: '!(has(self.overrides) && has(self.limits))'
- message: At least one spec.limits most be defined
rule: '!(has(self.overrides) || has(self.defaults)) ? has(self.limits)
&& size(self.limits) > 0 : true'
- message: At least one spec.overrides.limits most be defined
rule: 'has(self.overrides) ? has(self.overrides.limits) && size(self.overrides.limits)
> 0 : true'
- message: At least one spec.defaults.limits most be defined
rule: 'has(self.defaults) ? has(self.defaults.limits) && size(self.defaults.limits)
> 0 : true'
status:
properties:
conditions:
Expand Down
9 changes: 9 additions & 0 deletions config/crd/bases/kuadrant.io_ratelimitpolicies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,15 @@ spec:
rule: '!(has(self.defaults) && has(self.overrides))'
- message: Overrides and implicit defaults are mutually exclusive
rule: '!(has(self.overrides) && has(self.limits))'
- message: At least one spec.limits most be defined
rule: '!(has(self.overrides) || has(self.defaults)) ? has(self.limits)
&& size(self.limits) > 0 : true'
- message: At least one spec.overrides.limits most be defined
rule: 'has(self.overrides) ? has(self.overrides.limits) && size(self.overrides.limits)
> 0 : true'
- message: At least one spec.defaults.limits most be defined
rule: 'has(self.defaults) ? has(self.defaults.limits) && size(self.defaults.limits)
> 0 : true'
status:
properties:
conditions:
Expand Down
92 changes: 91 additions & 1 deletion tests/common/ratelimitpolicy/ratelimitpolicy_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -920,14 +920,25 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() {

Context("Spec TargetRef Validations", func() {
It("Valid policy targeting HTTPRoute", func(ctx SpecContext) {
policy := policyFactory()
policy := policyFactory(func(policy *kuadrantv1.RateLimitPolicy) {
policy.Spec.Limits = map[string]kuadrantv1.Limit{
"implicit": {
Rates: []kuadrantv1.Rate{{Limit: 2, Window: kuadrantv1.Duration("20s")}},
},
}
})
err := k8sClient.Create(ctx, policy)
Expect(err).To(BeNil())
}, testTimeOut)

It("Valid policy targeting Gateway", func(ctx SpecContext) {
policy := policyFactory(func(policy *kuadrantv1.RateLimitPolicy) {
policy.Spec.TargetRef.Kind = "Gateway"
policy.Spec.Limits = map[string]kuadrantv1.Limit{
"implicit": {
Rates: []kuadrantv1.Rate{{Limit: 2, Window: kuadrantv1.Duration("20s")}},
},
}
})
err := k8sClient.Create(ctx, policy)
Expect(err).To(BeNil())
Expand All @@ -952,6 +963,85 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() {
}, testTimeOut)
})

Context("Limits missing from configuration", func() {
It("Missing limits object", func(ctx SpecContext) {
policy := policyFactory(func(policy *kuadrantv1.RateLimitPolicy) {
policy.Spec.Limits = nil
})
err := k8sClient.Create(ctx, policy)
Expect(err).To(Not(BeNil()))
Expect(strings.Contains(err.Error(), "At least one spec.limits most be defined")).To(BeTrue())
}, testTimeOut)

It("Empty limits object created", func(ctx SpecContext) {
policy := policyFactory(func(policy *kuadrantv1.RateLimitPolicy) {
policy.Spec.Limits = map[string]kuadrantv1.Limit{}

})
err := k8sClient.Create(ctx, policy)
Expect(err).To(Not(BeNil()))
Expect(strings.Contains(err.Error(), "At least one spec.limits most be defined")).To(BeTrue())
}, testTimeOut)

It("Missing defaults.limits object", func(ctx SpecContext) {
policy := policyFactory(func(policy *kuadrantv1.RateLimitPolicy) {
policy.Spec.Limits = nil
policy.Spec.Defaults = &kuadrantv1.MergeableRateLimitPolicySpec{
RateLimitPolicySpecProper: kuadrantv1.RateLimitPolicySpecProper{
Limits: nil,
},
}
})
err := k8sClient.Create(ctx, policy)
Expect(err).To(Not(BeNil()))
Expect(strings.Contains(err.Error(), "At least one spec.defaults.limits most be defined")).To(BeTrue())
}, testTimeOut)

It("Empty defaults.limits object created", func(ctx SpecContext) {
policy := policyFactory(func(policy *kuadrantv1.RateLimitPolicy) {
policy.Spec.Limits = nil
policy.Spec.Defaults = &kuadrantv1.MergeableRateLimitPolicySpec{
RateLimitPolicySpecProper: kuadrantv1.RateLimitPolicySpecProper{
Limits: map[string]kuadrantv1.Limit{},
},
}

})
err := k8sClient.Create(ctx, policy)
Expect(err).To(Not(BeNil()))
Expect(strings.Contains(err.Error(), "At least one spec.defaults.limits most be defined")).To(BeTrue())
}, testTimeOut)

It("Missing overrides.limits object", func(ctx SpecContext) {
policy := policyFactory(func(policy *kuadrantv1.RateLimitPolicy) {
policy.Spec.Limits = nil
policy.Spec.Overrides = &kuadrantv1.MergeableRateLimitPolicySpec{
RateLimitPolicySpecProper: kuadrantv1.RateLimitPolicySpecProper{
Limits: nil,
},
}
})
err := k8sClient.Create(ctx, policy)
Expect(err).To(Not(BeNil()))
Expect(strings.Contains(err.Error(), "At least one spec.overrides.limits most be defined")).To(BeTrue())
}, testTimeOut)

It("Empty overrides.limits object created", func(ctx SpecContext) {
policy := policyFactory(func(policy *kuadrantv1.RateLimitPolicy) {
policy.Spec.Limits = nil
policy.Spec.Overrides = &kuadrantv1.MergeableRateLimitPolicySpec{
RateLimitPolicySpecProper: kuadrantv1.RateLimitPolicySpecProper{
Limits: map[string]kuadrantv1.Limit{},
},
}

})
err := k8sClient.Create(ctx, policy)
Expect(err).To(Not(BeNil()))
Expect(strings.Contains(err.Error(), "At least one spec.overrides.limits most be defined")).To(BeTrue())
}, testTimeOut)
})

Context("Defaults / Override validation", func() {
It("Valid - only implicit defaults defined", func(ctx SpecContext) {
policy := policyFactory(func(policy *kuadrantv1.RateLimitPolicy) {
Expand Down
Loading