RLP Defaults (#456)

* feat: rlp default field

* refactor: rlp GetLimits based on implicit or default rules

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

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

* refactor: use default limits field for rlp controller tests

* docs: update rlp reference

* refactor: common spec validation

* test: RLP atomic default integration test

* refactor: remove unneccessary getters

* fix: use common spec limits for wasm rules

* docs: address comments with new defaults field

* fix: load image for docker use explicitly

.github/workflows/test.yaml

@@ -79,7 +79,7 @@ jobs:
       - name: Create k8s Kind Cluster
         uses: helm/kind-action@v1.2.0
         with:
-          version: v0.20.0
+          version: v0.22.0
           config: utils/kind-cluster.yaml
           cluster_name: ${{ env.KIND_CLUSTER_NAME }}
           wait: 120s

Makefile

@@ -229,7 +229,7 @@ $(OPM):
 opm: $(OPM) ## Download opm locally if necessary.
 
 KIND = $(PROJECT_PATH)/bin/kind
-KIND_VERSION = v0.20.0
+KIND_VERSION = v0.22.0
 $(KIND):
 	$(call go-install-tool,$(KIND),sigs.k8s.io/kind@$(KIND_VERSION))
 
@@ -393,7 +393,7 @@ run: generate fmt vet ## Run a controller from your host.
 	go run ./main.go
 
 docker-build: ## Build docker image with the manager.
-	docker build -t $(IMG) .
+	docker build -t $(IMG) .  --load
 
 docker-push: ## Push docker image with the manager.
 	docker push $(IMG)

api/v1beta2/ratelimitpolicy_types.go

@@ -121,12 +121,25 @@ 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"
 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'"
 	// +kubebuilder:validation:XValidation:rule="self.kind == 'HTTPRoute' || self.kind == 'Gateway'",message="Invalid targetRef.kind. The only supported values are 'HTTPRoute' and 'Gateway'"
 	TargetRef gatewayapiv1alpha2.PolicyTargetReference `json:"targetRef"`
 
+	// Defaults define explicit default values for this policy and for policies inheriting this policy.
+	// Defaults are mutually exclusive with implicit defaults defined by RateLimitPolicyCommonSpec.
+	// +optional
+	Defaults *RateLimitPolicyCommonSpec `json:"defaults,omitempty"`
+
+	// RateLimitPolicyCommonSpec defines implicit default values for this policy and for policies inheriting this policy.
+	// RateLimitPolicyCommonSpec is mutually exclusive with explicit defaults defined by Defaults.
+	RateLimitPolicyCommonSpec `json:""`
+}
+
+// RateLimitPolicyCommonSpec contains common shared fields.
+type RateLimitPolicyCommonSpec struct {
 	// Limits holds the struct of limits indexed by a unique name
 	// +optional
 	// +kubebuilder:validation:MaxProperties=14
@@ -237,7 +250,7 @@ func (r *RateLimitPolicy) GetWrappedNamespace() gatewayapiv1.Namespace {
 
 func (r *RateLimitPolicy) GetRulesHostnames() (ruleHosts []string) {
 	ruleHosts = make([]string, 0)
-	for _, limit := range r.Spec.Limits {
+	for _, limit := range r.Spec.CommonSpec().Limits {
 		for _, routeSelector := range limit.RouteSelectors {
 			convertHostnamesToString := func(gwHostnames []gatewayapiv1.Hostname) []string {
 				hostnames := make([]string, 0, len(gwHostnames))
@@ -264,6 +277,18 @@ func (r *RateLimitPolicy) DirectReferenceAnnotationName() string {
 	return RateLimitPolicyDirectReferenceAnnotationName
 }
 
+// CommonSpec returns the Default RateLimitPolicyCommonSpec if it is defined.
+// Otherwise, it returns the RateLimitPolicyCommonSpec from the spec.
+// This function should be used instead of accessing the fields directly, so that either the explicit or implicit default
+// is returned.
+func (r *RateLimitPolicySpec) CommonSpec() *RateLimitPolicyCommonSpec {
+	if r.Defaults != nil {
+		return r.Defaults
+	}
+
+	return &r.RateLimitPolicyCommonSpec
+}
+
 func init() {
 	SchemeBuilder.Register(&RateLimitPolicy{}, &RateLimitPolicyList{})
 }

api/v1beta2/ratelimitpolicy_types_test.go

@@ -6,15 +6,16 @@ import (
 	"strings"
 	"testing"
 
+	"gotest.tools/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1"
 	gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
 
 	"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(),
@@ -31,32 +32,36 @@ 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)
+		}
+	})
 }
 
 func TestRateLimitPolicyListGetItems(t *testing.T) {
@@ -75,3 +80,47 @@ func TestRateLimitPolicyListGetItems(t *testing.T) {
 		t.Errorf("Expected item to be a Policy")
 	}
 }
+
+func TestRateLimitPolicy_GetLimits(t *testing.T) {
+	const name = "policy"
+	var (
+		defaultLimits = map[string]Limit{
+			"default": {
+				Rates: []Rate{{Limit: 10, Duration: 1, Unit: "seconds"}},
+			},
+		}
+		implicitLimits = map[string]Limit{
+			"implicit": {
+				Rates: []Rate{{Limit: 20, Duration: 2, Unit: "minutes"}},
+			},
+		}
+	)
+
+	t.Run("No limits defined", func(subT *testing.T) {
+		r := testBuildBasicHTTPRouteRLP(name, nil)
+		assert.DeepEqual(subT, r.Spec.CommonSpec().Limits, map[string]Limit(nil))
+	})
+	t.Run("Defaults defined", func(subT *testing.T) {
+		r := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) {
+			policy.Spec.Defaults = &RateLimitPolicyCommonSpec{
+				Limits: defaultLimits,
+			}
+		})
+		assert.DeepEqual(subT, r.Spec.CommonSpec().Limits, defaultLimits)
+	})
+	t.Run("Implicit rules defined", func(subT *testing.T) {
+		r := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) {
+			policy.Spec.Limits = implicitLimits
+		})
+		assert.DeepEqual(subT, r.Spec.CommonSpec().Limits, implicitLimits)
+	})
+	t.Run("Default rules takes precedence over implicit rules if validation is somehow bypassed", func(subT *testing.T) {
+		r := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) {
+			policy.Spec.Defaults = &RateLimitPolicyCommonSpec{
+				Limits: defaultLimits,
+			}
+			policy.Spec.Limits = implicitLimits
+		})
+		assert.DeepEqual(subT, r.Spec.CommonSpec().Limits, defaultLimits)
+	})
+}