From 550539bd3aee6230b117c9b7d3094ba580ef3bde Mon Sep 17 00:00:00 2001 From: KevFan Date: Fri, 12 Apr 2024 14:07:30 +0100 Subject: [PATCH] refactor: RLP CEL for override - only allowed for gateways and mutually exclusive with defaults --- api/v1beta2/ratelimitpolicy_types.go | 4 +- .../kuadrant.io_ratelimitpolicies.yaml | 8 +++- .../bases/kuadrant.io_ratelimitpolicies.yaml | 8 +++- .../ratelimitpolicy_controller_test.go | 43 ++++++++++++++++--- 4 files changed, 52 insertions(+), 11 deletions(-) diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go index 21894ac86..b243f0bb9 100644 --- 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 index 95046bed1..a69c351f8 100644 --- 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 index c3fa6f35e..cd7a241f0 100644 --- 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/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index 03aed0573..0d8ac4fde 100644 --- 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,7 +760,7 @@ 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"}}, }, }, @@ -766,7 +768,36 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { }) 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()) }) })