refactor: RLP CEL for override - only allowed for gateways and mutual…

…ly exclusive with defaults
Kuadrant · Apr 12, 2024 · 365f209 · 365f209
1 parent fe2a40e
commit 365f209
Show file tree

Hide file tree

Showing 5 changed files with 53 additions and 12 deletions.
diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go
@@ -122,7 +122,9 @@ func (l Limit) CountersAsStringList() []string {
 // RateLimitPolicySpec defines the desired state of RateLimitPolicy
 // +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.limits) || !self.limits.exists(x, has(self.limits[x].routeSelectors))",message="route selectors not supported when targeting a Gateway"
 // +kubebuilder:validation:XValidation:rule="!(has(self.defaults) && has(self.limits))",message="Implicit and explicit defaults are mutually exclusive"
-// +kubebuilder:validation:XValidation:rule="!(has(self.overrides) && has(self.limits))",message="Overrides and implicit defaults are mutually exclusive. Use defaults block instead to set defaults explicitly"
+// +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) && self.targetRef.kind != 'Gateway')",message="Overrides are only allowed for policies targeting a Gateway resource"
 type RateLimitPolicySpec struct {
 	// TargetRef identifies an API object to apply policy to.
 	// +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.io_ratelimitpolicies.yaml b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml
@@ -1290,9 +1290,13 @@ spec:
                 has(self.limits[x].routeSelectors))
             - message: Implicit and explicit defaults are mutually exclusive
               rule: '!(has(self.defaults) && has(self.limits))'
-            - message: Overrides and implicit defaults are mutually exclusive. Use
-                defaults block instead to set defaults explicitly
+            - message: Overrides and explicit defaults are mutually exclusive
+              rule: '!(has(self.defaults) && has(self.overrides))'
+            - message: Overrides and implicit defaults are mutually exclusive
               rule: '!(has(self.overrides) && has(self.limits))'
+            - message: Overrides are only allowed for policies targeting a Gateway
+                resource
+              rule: '!(has(self.overrides) && self.targetRef.kind != ''Gateway'')'
           status:
             description: RateLimitPolicyStatus defines the observed state of RateLimitPolicy
             properties:

diff --git a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml
@@ -1289,9 +1289,13 @@ spec:
                 has(self.limits[x].routeSelectors))
             - message: Implicit and explicit defaults are mutually exclusive
               rule: '!(has(self.defaults) && has(self.limits))'
-            - message: Overrides and implicit defaults are mutually exclusive. Use
-                defaults block instead to set defaults explicitly
+            - message: Overrides and explicit defaults are mutually exclusive
+              rule: '!(has(self.defaults) && has(self.overrides))'
+            - message: Overrides and implicit defaults are mutually exclusive
               rule: '!(has(self.overrides) && has(self.limits))'
+            - message: Overrides are only allowed for policies targeting a Gateway
+                resource
+              rule: '!(has(self.overrides) && self.targetRef.kind != ''Gateway'')'
           status:
             description: RateLimitPolicyStatus defines the observed state of RateLimitPolicy
             properties:

diff --git a/controllers/helper_test.go b/controllers/helper_test.go
@@ -93,7 +93,7 @@ func CreateNamespaceWithContext(ctx context.Context, namespace *string) {
 		TypeMeta:   metav1.TypeMeta{APIVersion: "v1", Kind: "Namespace"},
 		ObjectMeta: metav1.ObjectMeta{GenerateName: "test-namespace-"},
 	}
-	Expect(testClient().Create(context.Background(), nsObject)).To(Succeed())
+	Expect(testClient().Create(ctx, nsObject)).To(Succeed())
 
 	*namespace = nsObject.Name
 }

diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go
@@ -726,10 +726,10 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() {
 			})
 			err := k8sClient.Create(ctx, policy)
 			Expect(err).ToNot(BeNil())
-			Expect(strings.Contains(err.Error(), "Implicit and explicit defaults are mutually exclusive")).To(BeTrue())
+			Expect(err.Error()).To(ContainSubstring("Implicit and explicit defaults are mutually exclusive"))
 		})
 
-		It("Valid - explicit default and override defined", func(ctx SpecContext) {
+		It("Invalid - explicit default and override defined", func(ctx SpecContext) {
 			policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) {
 				policy.Spec.Defaults = &kuadrantv1beta2.RateLimitPolicyCommonSpec{
 					Limits: map[string]kuadrantv1beta2.Limit{
@@ -746,10 +746,12 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() {
 					},
 				}
 			})
-			Expect(k8sClient.Create(ctx, policy)).To(Succeed())
+			err := k8sClient.Create(ctx, policy)
+			Expect(err).ToNot(BeNil())
+			Expect(err.Error()).To(ContainSubstring("Overrides and explicit defaults are mutually exclusive"))
 		})
 
-		It("Invalid - implicit default and explicit override are mutually exclusive", func(ctx SpecContext) {
+		It("Invalid - implicit default and override defined", func(ctx SpecContext) {
 			policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) {
 				policy.Spec.Limits = map[string]kuadrantv1beta2.Limit{
 					"implicit": {
@@ -758,15 +760,44 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() {
 				}
 				policy.Spec.Overrides = &kuadrantv1beta2.RateLimitPolicyCommonSpec{
 					Limits: map[string]kuadrantv1beta2.Limit{
-						"explicit": {
+						"overrides": {
 							Rates: []kuadrantv1beta2.Rate{{Limit: 1, Duration: 10, Unit: "second"}},
 						},
 					},
 				}
 			})
 			err := k8sClient.Create(ctx, policy)
 			Expect(err).ToNot(BeNil())
-			Expect(strings.Contains(err.Error(), "Overrides and implicit defaults are mutually exclusive. Use defaults block instead to set defaults explicitly")).To(BeTrue())
+			Expect(err.Error()).To(ContainSubstring("Overrides and implicit defaults are mutually exclusive"))
+		})
+
+		It("Invalid - policy override targeting resource other than Gateway", func(ctx SpecContext) {
+			policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) {
+				policy.Spec.Overrides = &kuadrantv1beta2.RateLimitPolicyCommonSpec{
+					Limits: map[string]kuadrantv1beta2.Limit{
+						"implicit": {
+							Rates: []kuadrantv1beta2.Rate{{Limit: 1, Duration: 10, Unit: "second"}},
+						},
+					},
+				}
+			})
+			err := k8sClient.Create(ctx, policy)
+			Expect(err).ToNot(BeNil())
+			Expect(err.Error()).To(ContainSubstring("Overrides are only allowed for policies targeting a Gateway resource"))
+		})
+
+		It("Valid - policy override targeting Gateway", func(ctx SpecContext) {
+			policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) {
+				policy.Spec.TargetRef.Kind = "Gateway"
+				policy.Spec.Overrides = &kuadrantv1beta2.RateLimitPolicyCommonSpec{
+					Limits: map[string]kuadrantv1beta2.Limit{
+						"override": {
+							Rates: []kuadrantv1beta2.Rate{{Limit: 1, Duration: 10, Unit: "second"}},
+						},
+					},
+				}
+			})
+			Expect(k8sClient.Create(ctx, policy)).To(Succeed())
 		})
 	})