feat: rlp validation to prevent both implicit and explicit default rules

Kuadrant · Mar 12, 2024 · 764c603 · 764c603
1 parent 037737b
commit 764c603
Show file tree

Hide file tree

Showing 2 changed files with 78 additions and 18 deletions.
diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go
@@ -142,6 +142,11 @@ 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.
@@ -204,6 +209,10 @@ 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
 }
 

diff --git a/api/v1beta2/ratelimitpolicy_types_test.go b/api/v1beta2/ratelimitpolicy_types_test.go
@@ -14,8 +14,8 @@ import (
 	"github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant"
 )
 
-func testBuildBasicRLP(name string, kind gatewayapiv1.Kind) *RateLimitPolicy {
-	return &RateLimitPolicy{
+func testBuildBasicRLP(name string, kind gatewayapiv1.Kind, mutateFn func(*RateLimitPolicy)) *RateLimitPolicy {
+	p := &RateLimitPolicy{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "RateLimitPolicy",
 			APIVersion: GroupVersion.String(),
@@ -32,32 +32,83 @@ func testBuildBasicRLP(name string, kind gatewayapiv1.Kind) *RateLimitPolicy {
 			},
 		},
 	}
-}
 
-func testBuildBasicGatewayRLP(name string) *RateLimitPolicy {
-	return testBuildBasicRLP(name, gatewayapiv1.Kind("Gateway"))
+	if mutateFn != nil {
+		mutateFn(p)
+	}
+
+	return p
 }
 
-func testBuildBasicHTTPRouteRLP(name string) *RateLimitPolicy {
-	return testBuildBasicRLP(name, gatewayapiv1.Kind("HTTPRoute"))
+func testBuildBasicHTTPRouteRLP(name string, mutateFn func(*RateLimitPolicy)) *RateLimitPolicy {
+	return testBuildBasicRLP(name, "HTTPRoute", mutateFn)
 }
 
 // TestRateLimitPolicyValidation calls rlp.Validate()
 // for a valid return value.
 func TestRateLimitPolicyValidation(t *testing.T) {
 	name := "httproute-a"
 
-	// Different namespace
-	rlp := testBuildBasicHTTPRouteRLP(name)
-	otherNS := gatewayapiv1.Namespace(rlp.GetNamespace() + "other")
-	rlp.Spec.TargetRef.Namespace = &otherNS
-	err := rlp.Validate()
-	if err == nil {
-		t.Fatal(`rlp.Validate() did not return error and should`)
-	}
-	if !strings.Contains(err.Error(), "invalid targetRef.Namespace") {
-		t.Fatalf(`rlp.Validate() did not return expected error. Instead: %v`, err)
-	}
+	t.Run("Invalid - Different namespace", func(subT *testing.T) {
+		rlp := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) {
+			otherNS := gatewayapiv1.Namespace(policy.GetNamespace() + "other")
+			policy.Spec.TargetRef.Namespace = &otherNS
+		})
+		err := rlp.Validate()
+		if err == nil {
+			subT.Fatal(`rlp.Validate() did not return error and should`)
+		}
+		if !strings.Contains(err.Error(), "invalid targetRef.Namespace") {
+			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) {