refactor: CEL for validation of RLP implicit and explicit default mutual exclusivity

Author: KevFan

api/v1beta2/ratelimitpolicy_types.go

@@ -120,6 +120,7 @@ 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.limits) && has(self.limits))",message="Implicit and explicit defaults are mutually exclusive"
 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'"
@@ -128,7 +129,7 @@ type RateLimitPolicySpec struct {
 
 	// Defaults define explicit default values for this policy and for policies inheriting this policy
 	// +optional
-	Defaults CommonSpec `json:"defaults"`
+	Defaults CommonSpec `json:"defaults,omitempty"`
 
 	// Implicit default
 	CommonSpec `json:""`
@@ -142,11 +143,6 @@ type CommonSpec struct {
 	Limits map[string]Limit `json:"limits,omitempty"`
 }
 
-// IsUsed determines if any fields within CommonSpec is used
-func (c CommonSpec) IsUsed() bool {
-	return c.Limits != nil
-}
-
 // RateLimitPolicyStatus defines the observed state of RateLimitPolicy
 type RateLimitPolicyStatus struct {
 	// ObservedGeneration reflects the generation of the most recently observed spec.
@@ -209,10 +205,6 @@ func (r *RateLimitPolicy) Validate() error {
 		return fmt.Errorf("invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", *r.Spec.TargetRef.Namespace)
 	}
 
-	if r.Spec.Defaults.IsUsed() && r.Spec.CommonSpec.IsUsed() {
-		return fmt.Errorf("cannot use implicit defaults if explicit defaults are defined")
-	}
-
 	return nil
 }
 

api/v1beta2/ratelimitpolicy_types_test.go

@@ -62,53 +62,6 @@ func TestRateLimitPolicyValidation(t *testing.T) {
 			subT.Fatalf(`rlp.Validate() did not return expected error. Instead: %v`, err)
 		}
 	})
-
-	t.Run("Valid - no implicit or explicit defaults", func(subT *testing.T) {
-		rlp := testBuildBasicHTTPRouteRLP(name, nil)
-		if err := rlp.Validate(); err != nil {
-			subT.Fatalf(`rlp.Validate() did return error and should not: %v`, err)
-		}
-	})
-
-	t.Run("Valid - Implicit defaults only", func(subT *testing.T) {
-		rlp := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) {
-			policy.Spec.Limits = map[string]Limit{
-				"implicit": {Rates: []Rate{{Limit: 0}}},
-			}
-		})
-		if err := rlp.Validate(); err != nil {
-			subT.Fatalf(`rlp.Validate() did return error and should not: %v`, err)
-		}
-	})
-
-	t.Run("Valid - Explicit defaults only", func(subT *testing.T) {
-		rlp := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) {
-			policy.Spec.Defaults.Limits = map[string]Limit{
-				"explicit": {Rates: []Rate{{Limit: 1}}},
-			}
-		})
-		if err := rlp.Validate(); err != nil {
-			subT.Fatalf(`rlp.Validate() did return error and should not: %v`, err)
-		}
-	})
-
-	t.Run("Invalid - Implicit and explicit defaults ", func(subT *testing.T) {
-		rlp := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) {
-			policy.Spec.Limits = map[string]Limit{
-				"implicit": {Rates: []Rate{{Limit: 0}}},
-			}
-			policy.Spec.Defaults.Limits = map[string]Limit{
-				"explicit": {Rates: []Rate{{Limit: 1}}},
-			}
-		})
-		err := rlp.Validate()
-		if err == nil {
-			subT.Fatal(`rlp.Validate() did not return error and should`)
-		}
-		if !strings.Contains(err.Error(), "cannot use implicit defaults if explicit defaults are defined") {
-			subT.Fatalf(`rlp.Validate() did not return expected error. Instead: %v`, err)
-		}
-	})
 }
 
 func TestRateLimitPolicyListGetItems(t *testing.T) {

bundle/manifests/kuadrant-operator.clusterserviceversion.yaml

@@ -106,7 +106,7 @@ metadata:
     capabilities: Basic Install
     categories: Integration & Delivery
     containerImage: quay.io/kuadrant/kuadrant-operator:latest
-    createdAt: "2024-03-12T12:20:22Z"
+    createdAt: "2024-03-13T11:55:14Z"
     operators.operatorframework.io/builder: operator-sdk-v1.32.0
     operators.operatorframework.io/project_layout: go.kubebuilder.io/v3
     repository: https://github.com/Kuadrant/kuadrant-operator

bundle/manifests/kuadrant.io_ratelimitpolicies.yaml

@@ -868,6 +868,8 @@ spec:
             - message: route selectors not supported when targeting a Gateway
               rule: self.targetRef.kind != 'Gateway' || !has(self.limits) || !self.limits.exists(x,
                 has(self.limits[x].routeSelectors))
+            - message: Implicit and explicit defaults are mutually exclusive
+              rule: '!(has(self.defaults.limits) && has(self.limits))'
           status:
             description: RateLimitPolicyStatus defines the observed state of RateLimitPolicy
             properties:

config/crd/bases/kuadrant.io_ratelimitpolicies.yaml

@@ -867,6 +867,8 @@ spec:
             - message: route selectors not supported when targeting a Gateway
               rule: self.targetRef.kind != 'Gateway' || !has(self.limits) || !self.limits.exists(x,
                 has(self.limits[x].routeSelectors))
+            - message: Implicit and explicit defaults are mutually exclusive
+              rule: '!(has(self.defaults.limits) && has(self.limits))'
           status:
             description: RateLimitPolicyStatus defines the observed state of RateLimitPolicy
             properties:

controllers/ratelimitpolicy_controller_test.go

@@ -706,6 +706,48 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() {
 			Expect(err).To(Not(BeNil()))
 			Expect(strings.Contains(err.Error(), "Invalid targetRef.kind. The only supported values are 'HTTPRoute' and 'Gateway'")).To(BeTrue())
 		})
+
+		It("Valid only implicit defaults", func() {
+			policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) {
+				policy.Spec.Limits = map[string]kuadrantv1beta2.Limit{
+					"implicit": {
+						Rates: []kuadrantv1beta2.Rate{{Limit: 2, Duration: 20, Unit: "second"}},
+					},
+				}
+			})
+			err := k8sClient.Create(context.Background(), policy)
+			Expect(err).To(BeNil())
+		})
+
+		It("Valid only explicit defaults", func() {
+			policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) {
+				policy.Spec.Defaults.Limits = map[string]kuadrantv1beta2.Limit{
+					"explicit": {
+						Rates: []kuadrantv1beta2.Rate{{Limit: 1, Duration: 10, Unit: "second"}},
+					},
+				}
+			})
+			err := k8sClient.Create(context.Background(), policy)
+			Expect(err).To(BeNil())
+		})
+
+		It("Invalid implicit and explicit defaults are mutually exclusive", func() {
+			policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) {
+				policy.Spec.Defaults.Limits = map[string]kuadrantv1beta2.Limit{
+					"explicit": {
+						Rates: []kuadrantv1beta2.Rate{{Limit: 1, Duration: 10, Unit: "second"}},
+					},
+				}
+				policy.Spec.Limits = map[string]kuadrantv1beta2.Limit{
+					"implicit": {
+						Rates: []kuadrantv1beta2.Rate{{Limit: 2, Duration: 20, Unit: "second"}},
+					},
+				}
+			})
+			err := k8sClient.Create(context.Background(), policy)
+			Expect(err).To(Not(BeNil()))
+			Expect(strings.Contains(err.Error(), "Implicit and explicit defaults are mutually exclusive")).To(BeTrue())
+		})
 	})
 
 	Context("Route Selector Validation", func() {

pkg/rlptools/utils.go

@@ -8,9 +8,8 @@ import (
 
 	limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1"
 
-	"github.com/kuadrant/kuadrant-operator/pkg/library/utils"
-
 	kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2"
+	"github.com/kuadrant/kuadrant-operator/pkg/library/utils"
 )
 
 const (

pkg/rlptools/utils_test.go

@@ -10,9 +10,8 @@ import (
 	limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
-	"github.com/kuadrant/kuadrant-operator/pkg/library/utils"
-
 	kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2"
+	"github.com/kuadrant/kuadrant-operator/pkg/library/utils"
 )
 
 func testRLP_1Limit_1Rate(ns, name string) *kuadrantv1beta2.RateLimitPolicy {