From c8d083808daff46772254e223407c849b55020a7 Mon Sep 17 00:00:00 2001 From: Jim Fitzpatrick Date: Mon, 18 Nov 2024 12:37:00 +0000 Subject: [PATCH] Enhance creation of RateLimitPolicy around missing limits. (#1024) * IMPROVE: validation Ensure the user can't create a ratelimit policy without at least one limit. * Integration tests. Add integration tests for the six states that the user can try add invalid limit blocks. This does not check if the limits are valid only that they are there. --------- Signed-off-by: Jim Fitzpatrick --- api/v1/ratelimitpolicy_types.go | 3 + ...adrant-operator.clusterserviceversion.yaml | 2 +- .../kuadrant.io_ratelimitpolicies.yaml | 9 ++ .../templates/manifests.yaml | 9 ++ .../bases/kuadrant.io_ratelimitpolicies.yaml | 9 ++ .../ratelimitpolicy_controller_test.go | 92 ++++++++++++++++++- 6 files changed, 122 insertions(+), 2 deletions(-) diff --git a/api/v1/ratelimitpolicy_types.go b/api/v1/ratelimitpolicy_types.go index 4c47f9d66..aadc7c3f8 100644 --- a/api/v1/ratelimitpolicy_types.go +++ b/api/v1/ratelimitpolicy_types.go @@ -153,6 +153,9 @@ func (p *RateLimitPolicy) Kind() string { // +kubebuilder:validation:XValidation:rule="!(has(self.defaults) && has(self.limits))",message="Implicit and explicit defaults are mutually exclusive" // +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) || has(self.defaults)) ? has(self.limits) && size(self.limits) > 0 : true",message="At least one spec.limits must be defined" +// +kubebuilder:validation:XValidation:rule="has(self.overrides) ? has(self.overrides.limits) && size(self.overrides.limits) > 0 : true",message="At least one spec.overrides.limits must be defined" +// +kubebuilder:validation:XValidation:rule="has(self.defaults) ? has(self.defaults.limits) && size(self.defaults.limits) > 0 : true",message="At least one spec.defaults.limits must be defined" type RateLimitPolicySpec struct { // Reference to the object to which this policy applies. // +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-operator.clusterserviceversion.yaml b/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml index d58540342..6e82e5b49 100644 --- a/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml +++ b/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml @@ -109,7 +109,7 @@ metadata: capabilities: Basic Install categories: Integration & Delivery containerImage: quay.io/kuadrant/kuadrant-operator:latest - createdAt: "2024-11-13T16:31:14Z" + createdAt: "2024-11-18T10:54:46Z" description: A Kubernetes Operator to manage the lifecycle of the Kuadrant system operators.operatorframework.io/builder: operator-sdk-v1.32.0 operators.operatorframework.io/project_layout: go.kubebuilder.io/v3 diff --git a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml index a35db04e4..e1b4e85af 100644 --- a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml +++ b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml @@ -392,6 +392,15 @@ spec: rule: '!(has(self.defaults) && has(self.overrides))' - message: Overrides and implicit defaults are mutually exclusive rule: '!(has(self.overrides) && has(self.limits))' + - message: At least one spec.limits must be defined + rule: '!(has(self.overrides) || has(self.defaults)) ? has(self.limits) + && size(self.limits) > 0 : true' + - message: At least one spec.overrides.limits must be defined + rule: 'has(self.overrides) ? has(self.overrides.limits) && size(self.overrides.limits) + > 0 : true' + - message: At least one spec.defaults.limits must be defined + rule: 'has(self.defaults) ? has(self.defaults.limits) && size(self.defaults.limits) + > 0 : true' status: properties: conditions: diff --git a/charts/kuadrant-operator/templates/manifests.yaml b/charts/kuadrant-operator/templates/manifests.yaml index db39b9336..36a1019f0 100644 --- a/charts/kuadrant-operator/templates/manifests.yaml +++ b/charts/kuadrant-operator/templates/manifests.yaml @@ -7994,6 +7994,15 @@ spec: rule: '!(has(self.defaults) && has(self.overrides))' - message: Overrides and implicit defaults are mutually exclusive rule: '!(has(self.overrides) && has(self.limits))' + - message: At least one spec.limits must be defined + rule: '!(has(self.overrides) || has(self.defaults)) ? has(self.limits) + && size(self.limits) > 0 : true' + - message: At least one spec.overrides.limits must be defined + rule: 'has(self.overrides) ? has(self.overrides.limits) && size(self.overrides.limits) + > 0 : true' + - message: At least one spec.defaults.limits must be defined + rule: 'has(self.defaults) ? has(self.defaults.limits) && size(self.defaults.limits) + > 0 : true' status: properties: conditions: diff --git a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml index 9dbab91d8..4f11d07fe 100644 --- a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml +++ b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml @@ -391,6 +391,15 @@ spec: rule: '!(has(self.defaults) && has(self.overrides))' - message: Overrides and implicit defaults are mutually exclusive rule: '!(has(self.overrides) && has(self.limits))' + - message: At least one spec.limits must be defined + rule: '!(has(self.overrides) || has(self.defaults)) ? has(self.limits) + && size(self.limits) > 0 : true' + - message: At least one spec.overrides.limits must be defined + rule: 'has(self.overrides) ? has(self.overrides.limits) && size(self.overrides.limits) + > 0 : true' + - message: At least one spec.defaults.limits must be defined + rule: 'has(self.defaults) ? has(self.defaults.limits) && size(self.defaults.limits) + > 0 : true' status: properties: conditions: diff --git a/tests/common/ratelimitpolicy/ratelimitpolicy_controller_test.go b/tests/common/ratelimitpolicy/ratelimitpolicy_controller_test.go index 7ba2a736d..55d905470 100644 --- a/tests/common/ratelimitpolicy/ratelimitpolicy_controller_test.go +++ b/tests/common/ratelimitpolicy/ratelimitpolicy_controller_test.go @@ -920,7 +920,13 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { Context("Spec TargetRef Validations", func() { It("Valid policy targeting HTTPRoute", func(ctx SpecContext) { - policy := policyFactory() + policy := policyFactory(func(policy *kuadrantv1.RateLimitPolicy) { + policy.Spec.Limits = map[string]kuadrantv1.Limit{ + "implicit": { + Rates: []kuadrantv1.Rate{{Limit: 2, Window: kuadrantv1.Duration("20s")}}, + }, + } + }) err := k8sClient.Create(ctx, policy) Expect(err).To(BeNil()) }, testTimeOut) @@ -928,6 +934,11 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { It("Valid policy targeting Gateway", func(ctx SpecContext) { policy := policyFactory(func(policy *kuadrantv1.RateLimitPolicy) { policy.Spec.TargetRef.Kind = "Gateway" + policy.Spec.Limits = map[string]kuadrantv1.Limit{ + "implicit": { + Rates: []kuadrantv1.Rate{{Limit: 2, Window: kuadrantv1.Duration("20s")}}, + }, + } }) err := k8sClient.Create(ctx, policy) Expect(err).To(BeNil()) @@ -952,6 +963,85 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { }, testTimeOut) }) + Context("Limits missing from configuration", func() { + It("Missing limits object", func(ctx SpecContext) { + policy := policyFactory(func(policy *kuadrantv1.RateLimitPolicy) { + policy.Spec.Limits = nil + }) + err := k8sClient.Create(ctx, policy) + Expect(err).To(Not(BeNil())) + Expect(strings.Contains(err.Error(), "At least one spec.limits must be defined")).To(BeTrue()) + }, testTimeOut) + + It("Empty limits object created", func(ctx SpecContext) { + policy := policyFactory(func(policy *kuadrantv1.RateLimitPolicy) { + policy.Spec.Limits = map[string]kuadrantv1.Limit{} + + }) + err := k8sClient.Create(ctx, policy) + Expect(err).To(Not(BeNil())) + Expect(strings.Contains(err.Error(), "At least one spec.limits must be defined")).To(BeTrue()) + }, testTimeOut) + + It("Missing defaults.limits object", func(ctx SpecContext) { + policy := policyFactory(func(policy *kuadrantv1.RateLimitPolicy) { + policy.Spec.Limits = nil + policy.Spec.Defaults = &kuadrantv1.MergeableRateLimitPolicySpec{ + RateLimitPolicySpecProper: kuadrantv1.RateLimitPolicySpecProper{ + Limits: nil, + }, + } + }) + err := k8sClient.Create(ctx, policy) + Expect(err).To(Not(BeNil())) + Expect(strings.Contains(err.Error(), "At least one spec.defaults.limits must be defined")).To(BeTrue()) + }, testTimeOut) + + It("Empty defaults.limits object created", func(ctx SpecContext) { + policy := policyFactory(func(policy *kuadrantv1.RateLimitPolicy) { + policy.Spec.Limits = nil + policy.Spec.Defaults = &kuadrantv1.MergeableRateLimitPolicySpec{ + RateLimitPolicySpecProper: kuadrantv1.RateLimitPolicySpecProper{ + Limits: map[string]kuadrantv1.Limit{}, + }, + } + + }) + err := k8sClient.Create(ctx, policy) + Expect(err).To(Not(BeNil())) + Expect(strings.Contains(err.Error(), "At least one spec.defaults.limits must be defined")).To(BeTrue()) + }, testTimeOut) + + It("Missing overrides.limits object", func(ctx SpecContext) { + policy := policyFactory(func(policy *kuadrantv1.RateLimitPolicy) { + policy.Spec.Limits = nil + policy.Spec.Overrides = &kuadrantv1.MergeableRateLimitPolicySpec{ + RateLimitPolicySpecProper: kuadrantv1.RateLimitPolicySpecProper{ + Limits: nil, + }, + } + }) + err := k8sClient.Create(ctx, policy) + Expect(err).To(Not(BeNil())) + Expect(strings.Contains(err.Error(), "At least one spec.overrides.limits must be defined")).To(BeTrue()) + }, testTimeOut) + + It("Empty overrides.limits object created", func(ctx SpecContext) { + policy := policyFactory(func(policy *kuadrantv1.RateLimitPolicy) { + policy.Spec.Limits = nil + policy.Spec.Overrides = &kuadrantv1.MergeableRateLimitPolicySpec{ + RateLimitPolicySpecProper: kuadrantv1.RateLimitPolicySpecProper{ + Limits: map[string]kuadrantv1.Limit{}, + }, + } + + }) + err := k8sClient.Create(ctx, policy) + Expect(err).To(Not(BeNil())) + Expect(strings.Contains(err.Error(), "At least one spec.overrides.limits must be defined")).To(BeTrue()) + }, testTimeOut) + }) + Context("Defaults / Override validation", func() { It("Valid - only implicit defaults defined", func(ctx SpecContext) { policy := policyFactory(func(policy *kuadrantv1.RateLimitPolicy) {