Enhance creation of RateLimitPolicy around missing limits. (#1024)

* IMPROVE: validation Ensure the user can't create a ratelimit policy without at least one limit. * Integration tests. Add integration tests for the six states that the user can try add invalid limit blocks. This does not check if the limits are valid only that they are there. --------- Signed-off-by: Jim Fitzpatrick <[email protected]>
Kuadrant · Nov 18, 2024 · c8d0838 · c8d0838
1 parent 6890020
commit c8d0838
Show file tree

Hide file tree

Showing 6 changed files with 122 additions and 2 deletions.
diff --git a/api/v1/ratelimitpolicy_types.go b/api/v1/ratelimitpolicy_types.go
@@ -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 must 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 must be defined"
+// +kubebuilder:validation:XValidation:rule="has(self.defaults) ? has(self.defaults.limits) && size(self.defaults.limits) > 0 : true",message="At least one spec.defaults.limits must 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'"

diff --git a/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml b/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml
@@ -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-18T10:54:46Z"
     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

diff --git a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml
@@ -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 must be defined
+              rule: '!(has(self.overrides) || has(self.defaults)) ? has(self.limits)
+                && size(self.limits) > 0 : true'
+            - message: At least one spec.overrides.limits must be defined
+              rule: 'has(self.overrides) ? has(self.overrides.limits) && size(self.overrides.limits)
+                > 0 : true'
+            - message: At least one spec.defaults.limits must be defined
+              rule: 'has(self.defaults) ? has(self.defaults.limits) && size(self.defaults.limits)
+                > 0 : true'
           status:
             properties:
               conditions:

diff --git a/charts/kuadrant-operator/templates/manifests.yaml b/charts/kuadrant-operator/templates/manifests.yaml
@@ -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 must be defined
+              rule: '!(has(self.overrides) || has(self.defaults)) ? has(self.limits)
+                && size(self.limits) > 0 : true'
+            - message: At least one spec.overrides.limits must be defined
+              rule: 'has(self.overrides) ? has(self.overrides.limits) && size(self.overrides.limits)
+                > 0 : true'
+            - message: At least one spec.defaults.limits must be defined
+              rule: 'has(self.defaults) ? has(self.defaults.limits) && size(self.defaults.limits)
+                > 0 : true'
           status:
             properties:
               conditions:

diff --git a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml
@@ -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 must be defined
+              rule: '!(has(self.overrides) || has(self.defaults)) ? has(self.limits)
+                && size(self.limits) > 0 : true'
+            - message: At least one spec.overrides.limits must be defined
+              rule: 'has(self.overrides) ? has(self.overrides.limits) && size(self.overrides.limits)
+                > 0 : true'
+            - message: At least one spec.defaults.limits must be defined
+              rule: 'has(self.defaults) ? has(self.defaults.limits) && size(self.defaults.limits)
+                > 0 : true'
           status:
             properties:
               conditions:

diff --git a/tests/common/ratelimitpolicy/ratelimitpolicy_controller_test.go b/tests/common/ratelimitpolicy/ratelimitpolicy_controller_test.go
@@ -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())
@@ -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 must 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 must 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 must 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 must 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 must 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 must 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) {