Skip to content

Commit

Permalink
refactor: RLP CEL for override - only allowed for gateways and mutual…
Browse files Browse the repository at this point in the history 
…ly exclusive with defaults
  • Loading branch information
@KevFan
KevFan committed Apr 12, 2024
1 parent a7ae2ee commit c7b7552
Show file tree
Hide file tree
Showing 5 changed files with 134 additions and 25 deletions.
4 changes: 3 additions & 1 deletion api/v1beta2/ratelimitpolicy_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'"
Expand Down
8 changes: 6 additions & 2 deletions bundle/manifests/kuadrant.io_ratelimitpolicies.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
8 changes: 6 additions & 2 deletions config/crd/bases/kuadrant.io_ratelimitpolicies.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion controllers/helper_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func CreateNamespaceWithContext(ctx context.Context, namespace *string) {
TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Namespace"},
ObjectMeta: metav1.ObjectMeta{GenerateName: "test-namespace-"},
}
Expect(testClient().Create(context.Background(), nsObject)).To(Succeed())
Expect(testClient().Create(ctx, nsObject)).To(Succeed())

*namespace = nsObject.Name
}
Expand Down
137 changes: 118 additions & 19 deletions controllers/ratelimitpolicy_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ var _ = Describe("RateLimitPolicy controller", func() {
BeforeEach(beforeEachCallback, NodeTimeout(time.Minute))
AfterEach(func(ctx SpecContext) {
DeleteNamespaceCallbackWithContext(ctx, &testNamespace)
}, NodeTimeout(time.Minute))
}, NodeTimeout(3*time.Minute))

Context("RLP targeting HTTPRoute", func() {
It("Creates all the resources for a basic HTTPRoute and RateLimitPolicy", func() {
Expand Down Expand Up @@ -329,12 +329,14 @@ var _ = Describe("RateLimitPolicy controller", func() {
})

Context("RLP Overrides", func() {
It("Gateway atomic override - gateway overrides exist and then route policy created", func(ctx SpecContext) {
BeforeEach(func(ctx SpecContext) {
// create httproute
httpRoute := testBuildBasicHttpRoute(routeName, gwName, testNamespace, []string{"*.example.com"})
Expect(k8sClient.Create(ctx, httpRoute)).To(Succeed())
Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(httpRoute))).WithContext(ctx).Should(BeTrue())
}, NodeTimeout(time.Minute))

It("Gateway atomic override - gateway overrides exist and then route policy created", func(ctx SpecContext) {
// create GW RLP
gwRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) {
policy.Spec.TargetRef.Kind = "Gateway"
Expand Down Expand Up @@ -405,11 +407,6 @@ var _ = Describe("RateLimitPolicy controller", func() {
}, SpecTimeout(time.Minute))

It("Gateway atomic override - route policy exits and then gateway policy created", func(ctx SpecContext) {
// create httproute
httpRoute := testBuildBasicHttpRoute(routeName, gwName, testNamespace, []string{"*.example.com"})
Expect(k8sClient.Create(ctx, httpRoute)).To(Succeed())
Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(httpRoute))).WithContext(ctx).Should(BeTrue())

// Create HTTPRoute RLP
routeRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) {
policy.Name = "httproute-rlp"
Expand Down Expand Up @@ -480,11 +477,6 @@ var _ = Describe("RateLimitPolicy controller", func() {
}, SpecTimeout(time.Minute))

It("Gateway atomic override - gateway defaults turned into overrides later on", func(ctx SpecContext) {
// create httproute
httpRoute := testBuildBasicHttpRoute(routeName, gwName, testNamespace, []string{"*.example.com"})
Expect(k8sClient.Create(ctx, httpRoute)).To(Succeed())
Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(httpRoute))).WithContext(ctx).Should(BeTrue())

// Create HTTPRoute RLP
routeRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) {
policy.Name = "httproute-rlp"
Expand Down Expand Up @@ -558,6 +550,82 @@ var _ = Describe("RateLimitPolicy controller", func() {
}))
})
}, SpecTimeout(time.Minute))

It("Gateway atomic override - gateway overrides turned into defaults later on", func(ctx SpecContext) {
// Create HTTPRoute RLP
routeRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) {
policy.Name = "httproute-rlp"
policy.Spec.CommonSpec().Limits = map[string]kuadrantv1beta2.Limit{
"l1": {
Rates: []kuadrantv1beta2.Rate{
{
Limit: 10, Duration: 5, Unit: "second",
},
},
},
}
})
Expect(k8sClient.Create(ctx, routeRLP)).To(Succeed())
rlpKey := client.ObjectKey{Name: routeRLP.Name, Namespace: testNamespace}
Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue())

// create GW RLP
gwRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) {
policy.Spec.Defaults = nil
policy.Spec.TargetRef.Kind = "Gateway"
policy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(gwName)
policy.Spec.Overrides = &kuadrantv1beta2.RateLimitPolicyCommonSpec{
Limits: map[string]kuadrantv1beta2.Limit{
"l1": {
Rates: []kuadrantv1beta2.Rate{
{
Limit: 1, Duration: 3, Unit: "minute",
},
},
},
},
}
})
Expect(k8sClient.Create(ctx, gwRLP)).To(Succeed())
rlpKey = client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace}
Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue())

// check limits - overridden - should contain GW RLP values
limitadorKey := client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace}
existingLimitador := &limitadorv1alpha1.Limitador{}
Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed())
Expect(existingLimitador.Spec.Limits).To(ContainElements(limitadorv1alpha1.RateLimit{
MaxValue: 1,
Seconds: 180,
Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP),
Conditions: []string{`limit.l1__2804bad6 == "1"`},
Variables: []string{},
Name: rlptools.LimitsNameFromRLP(routeRLP),
}))

updatedGRLP := &kuadrantv1beta2.RateLimitPolicy{}
Expect(k8sClient.Get(ctx, client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace}, updatedGRLP)).To(Succeed())
Eventually(func(g Gomega) {
updatedGRLP.Spec.Defaults = updatedGRLP.Spec.Overrides.DeepCopy()
updatedGRLP.Spec.Overrides = nil
g.Expect(k8sClient.Update(ctx, updatedGRLP)).To(Succeed())
}).WithContext(ctx).Should(Succeed())

Eventually(func(g Gomega) {
// check limits - should contain HTTPRoute RLP values
limitadorKey = client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace}
existingLimitador = &limitadorv1alpha1.Limitador{}
g.Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed())
g.Expect(existingLimitador.Spec.Limits).To(ContainElements(limitadorv1alpha1.RateLimit{
MaxValue: 10,
Seconds: 5,
Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP),
Conditions: []string{`limit.l1__2804bad6 == "1"`},
Variables: []string{},
Name: rlptools.LimitsNameFromRLP(routeRLP),
}))
})
}, SpecTimeout(2*time.Minute))
})

Context("RLP accepted condition reasons", func() {
Expand Down Expand Up @@ -616,7 +684,7 @@ var _ = Describe("RateLimitPolicy controller", func() {
Eventually(assertAcceptedConditionFalse(ctx, rlp, string(gatewayapiv1alpha2.PolicyReasonInvalid),
fmt.Sprintf("RateLimitPolicy target is invalid: invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", targetRefNamespace)),
).WithContext(ctx).Should(Succeed())
}, SpecTimeout(time.Minute))
}, SpecTimeout(2*time.Minute))
})

Context("When RLP switches target from one HTTPRoute to another HTTPRoute", func() {
Expand Down Expand Up @@ -1145,10 +1213,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{
Expand All @@ -1165,10 +1233,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": {
Expand All @@ -1177,15 +1247,44 @@ 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"}},
},
},
}
})
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())
})
})

Expand Down

0 comments on commit c7b7552

Please sign in to comment.