From e66d50cca8862846c927e26529eff8577fc94ed0 Mon Sep 17 00:00:00 2001 From: KevFan Date: Wed, 13 Mar 2024 11:49:15 +0000 Subject: [PATCH] refactor: CEL for validation of RLP implicit and explicit default mutual exclusivity --- api/v1beta2/ratelimitpolicy_types.go | 12 +---- api/v1beta2/ratelimitpolicy_types_test.go | 47 ------------------- ...adrant-operator.clusterserviceversion.yaml | 2 +- .../kuadrant.io_ratelimitpolicies.yaml | 2 + .../bases/kuadrant.io_ratelimitpolicies.yaml | 2 + .../ratelimitpolicy_controller_test.go | 42 +++++++++++++++++ 6 files changed, 49 insertions(+), 58 deletions(-) diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go index 999e2d4a8..2f5457a5e 100644 --- a/api/v1beta2/ratelimitpolicy_types.go +++ b/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 } diff --git a/api/v1beta2/ratelimitpolicy_types_test.go b/api/v1beta2/ratelimitpolicy_types_test.go index 95c37f501..5bd71ce57 100644 --- a/api/v1beta2/ratelimitpolicy_types_test.go +++ b/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) { diff --git a/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml b/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml index 75601a406..97b80d87b 100644 --- a/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml +++ b/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 diff --git a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml index cf7f67e32..fa041147a 100644 --- a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml +++ b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml @@ -834,6 +834,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: diff --git a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml index 87eaabda1..34aa4788c 100644 --- a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml +++ b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml @@ -833,6 +833,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: diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index a1c0562b3..1173e2e3e 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/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() {