Skip to content

Commit

Permalink
Fix override RLPs on multiple gateway parents (#659)
Browse files Browse the repository at this point in the history
* Fix override RLPs on multiple gateway parents

* fix: all rlps should go to the limits config - maybe there are overridden for one gateway but not for all gateways

* rollback simplified wasm config based on fallback to overrides, in favour of duplicate limit definitions for each independent domain/namespace

* fix: RLP Enforced condition with multiple gateway parents

* go mod tidy
  • Loading branch information
guicassolato authored Jun 5, 2024
1 parent ec5351d commit 9ad525d
Show file tree
Hide file tree
Showing 19 changed files with 533 additions and 321 deletions.
35 changes: 11 additions & 24 deletions controllers/rate_limiting_wasmplugin_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"sort"

"github.com/go-logr/logr"
"github.com/google/uuid"
istioextensionsv1alpha1 "istio.io/api/extensions/v1alpha1"
istioclientgoextensionv1alpha1 "istio.io/client-go/pkg/apis/extensions/v1alpha1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
Expand Down Expand Up @@ -61,7 +62,7 @@ type RateLimitingWASMPluginReconciler struct {
// For more details, check Reconcile and its Result here:
// - https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/reconcile
func (r *RateLimitingWASMPluginReconciler) Reconcile(eventCtx context.Context, req ctrl.Request) (ctrl.Result, error) {
logger := r.Logger().WithValues("Gateway", req.NamespacedName)
logger := r.Logger().WithValues("Gateway", req.NamespacedName, "request id", uuid.NewString())
logger.Info("Reconciling rate limiting WASMPlugin")
ctx := logr.NewContext(eventCtx, logger)

Expand Down Expand Up @@ -174,7 +175,7 @@ func (r *RateLimitingWASMPluginReconciler) wasmPluginConfig(ctx context.Context,

for _, policy := range rateLimitPolicies {
rlp := policy.(*kuadrantv1beta2.RateLimitPolicy)
wasmRLP, err := r.wasmRateLimitPolicy(ctx, t, rlp, gw, rateLimitPolicies)
wasmRLP, err := r.wasmRateLimitPolicy(ctx, t, rlp, gw)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -219,7 +220,7 @@ func (r *RateLimitingWASMPluginReconciler) topologyIndexesFromGateway(ctx contex

policies := utils.Map(rlpList.Items, func(p kuadrantv1beta2.RateLimitPolicy) kuadrantgatewayapi.Policy { return &p })

t, err := kuadrantgatewayapi.NewTopology(
topology, err := kuadrantgatewayapi.NewTopology(
kuadrantgatewayapi.WithGateways([]*gatewayapiv1.Gateway{gw}),
kuadrantgatewayapi.WithRoutes(utils.Map(routeList.Items, ptr.To[gatewayapiv1.HTTPRoute])),
kuadrantgatewayapi.WithPolicies(policies),
Expand All @@ -229,12 +230,15 @@ func (r *RateLimitingWASMPluginReconciler) topologyIndexesFromGateway(ctx contex
return nil, err
}

return kuadrantgatewayapi.NewTopologyIndexes(t), nil
}
overriddenTopology, err := rlptools.ApplyOverrides(topology, gw)
if err != nil {
return nil, err
}

func (r *RateLimitingWASMPluginReconciler) wasmRateLimitPolicy(ctx context.Context, t *kuadrantgatewayapi.TopologyIndexes, rlp *kuadrantv1beta2.RateLimitPolicy, gw *gatewayapiv1.Gateway, affectedPolices []kuadrantgatewayapi.Policy) (*wasm.RateLimitPolicy, error) {
logger, _ := logr.FromContext(ctx)
return kuadrantgatewayapi.NewTopologyIndexes(overriddenTopology), nil
}

func (r *RateLimitingWASMPluginReconciler) wasmRateLimitPolicy(ctx context.Context, t *kuadrantgatewayapi.TopologyIndexes, rlp *kuadrantv1beta2.RateLimitPolicy, gw *gatewayapiv1.Gateway) (*wasm.RateLimitPolicy, error) {
route, err := r.routeFromRLP(ctx, t, rlp, gw)
if err != nil {
return nil, err
Expand Down Expand Up @@ -267,22 +271,6 @@ func (r *RateLimitingWASMPluginReconciler) wasmRateLimitPolicy(ctx context.Conte
routeWithEffectiveHostnames := route.DeepCopy()
routeWithEffectiveHostnames.Spec.Hostnames = hostnames

// Policy limits may be overridden by a gateway policy for route policies
if kuadrantgatewayapi.IsTargetRefHTTPRoute(rlp.GetTargetRef()) {
filteredPolicies := utils.Filter(affectedPolices, func(p kuadrantgatewayapi.Policy) bool {
return kuadrantgatewayapi.IsTargetRefGateway(p.GetTargetRef()) && p.GetUID() != rlp.GetUID()
})

for _, policy := range filteredPolicies {
p := policy.(*kuadrantv1beta2.RateLimitPolicy)
if p.Spec.Overrides != nil {
rlp.Spec.CommonSpec().Limits = p.Spec.Overrides.Limits
logger.V(1).Info("applying overrides from parent policy", "parentPolicy", client.ObjectKeyFromObject(p))
break
}
}
}

rules := rlptools.WasmRules(rlp, routeWithEffectiveHostnames)
if len(rules) == 0 {
// no need to add the policy if there are no rules; a rlp can return no rules if all its limits fail to match any route rule
Expand Down Expand Up @@ -324,7 +312,6 @@ func (r *RateLimitingWASMPluginReconciler) routeFromRLP(ctx context.Context, t *
for idx := range untargetedRoutes {
untargetedRules = append(untargetedRules, untargetedRoutes[idx].Spec.Rules...)
}

gwHostnamesTmp := kuadrantgatewayapi.TargetHostnames(gw)
gwHostnames := utils.Map(gwHostnamesTmp, func(str string) gatewayapiv1.Hostname { return gatewayapiv1.Hostname(str) })
route = &gatewayapiv1.HTTPRoute{
Expand Down
3 changes: 2 additions & 1 deletion controllers/ratelimitpolicy_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"fmt"

"github.com/go-logr/logr"
"github.com/google/uuid"
apierrors "k8s.io/apimachinery/pkg/api/errors"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -60,7 +61,7 @@ type RateLimitPolicyReconciler struct {
// For more details, check Reconcile and its Result here:
// - https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/reconcile
func (r *RateLimitPolicyReconciler) Reconcile(eventCtx context.Context, req ctrl.Request) (ctrl.Result, error) {
logger := r.Logger().WithValues("RateLimitPolicy", req.NamespacedName)
logger := r.Logger().WithValues("RateLimitPolicy", req.NamespacedName, "request id", uuid.NewString())
logger.Info("Reconciling RateLimitPolicy")
ctx := logr.NewContext(eventCtx, logger)

Expand Down
171 changes: 158 additions & 13 deletions controllers/ratelimitpolicy_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,12 +88,15 @@ var _ = Describe("RateLimitPolicy controller", Ordered, func() {
}
}

limitadorContainsLimit := func(ctx context.Context, limit limitadorv1alpha1.RateLimit) func(g Gomega) {
limitadorContainsLimit := func(ctx context.Context, limits ...limitadorv1alpha1.RateLimit) func(g Gomega) {
return func(g Gomega) {
limitadorKey := client.ObjectKey{Name: common.LimitadorName, Namespace: kuadrantInstallationNS}
existingLimitador := &limitadorv1alpha1.Limitador{}
g.Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed())
g.Expect(existingLimitador.Spec.Limits).To(ContainElements(limit))
for i := range limits {
limit := limits[i]
g.Expect(existingLimitador.Spec.Limits).To(ContainElements(limit))
}
}
}

Expand Down Expand Up @@ -157,7 +160,7 @@ var _ = Describe("RateLimitPolicy controller", Ordered, func() {
MaxValue: 1,
Seconds: 3 * 60,
Namespace: rlptools.LimitsNamespaceFromRLP(rlp),
Conditions: []string{`limit.l1__2804bad6 == "1"`},
Conditions: []string{fmt.Sprintf(`%s == "1"`, rlptools.LimitNameToLimitadorIdentifier(rlpKey, "l1"))},
Variables: []string{},
Name: rlptools.LimitsNameFromRLP(rlp),
}))
Expand Down Expand Up @@ -243,7 +246,7 @@ var _ = Describe("RateLimitPolicy controller", Ordered, func() {
MaxValue: 1,
Seconds: 3 * 60,
Namespace: rlptools.LimitsNamespaceFromRLP(rlp),
Conditions: []string{`limit.l1__2804bad6 == "1"`},
Conditions: []string{fmt.Sprintf(`%s == "1"`, rlptools.LimitNameToLimitadorIdentifier(rlpKey, "l1"))},
Variables: []string{},
Name: rlptools.LimitsNameFromRLP(rlp),
}))
Expand Down Expand Up @@ -293,7 +296,7 @@ var _ = Describe("RateLimitPolicy controller", Ordered, func() {
MaxValue: 1,
Seconds: 3 * 60,
Namespace: rlptools.LimitsNamespaceFromRLP(rlp),
Conditions: []string{`limit.l1__2804bad6 == "1"`},
Conditions: []string{fmt.Sprintf(`%s == "1"`, rlptools.LimitNameToLimitadorIdentifier(rlpKey, "l1"))},
Variables: []string{},
Name: rlptools.LimitsNameFromRLP(rlp),
}))
Expand Down Expand Up @@ -368,7 +371,7 @@ var _ = Describe("RateLimitPolicy controller", Ordered, func() {
MaxValue: 10,
Seconds: 5,
Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP),
Conditions: []string{`limit.l1__2804bad6 == "1"`},
Conditions: []string{fmt.Sprintf(`%s == "1"`, rlptools.LimitNameToLimitadorIdentifier(routeRLPKey, "l1"))},
Variables: []string{},
Name: rlptools.LimitsNameFromRLP(routeRLP),
})).WithContext(ctx).Should(Succeed())
Expand Down Expand Up @@ -483,7 +486,7 @@ var _ = Describe("RateLimitPolicy controller", Ordered, func() {
MaxValue: 1,
Seconds: 180,
Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP),
Conditions: []string{`limit.l1__2804bad6 == "1"`},
Conditions: []string{fmt.Sprintf(`%s == "1"`, rlptools.LimitNameToLimitadorIdentifier(routeRLPKey, "l1"))},
Variables: []string{},
Name: rlptools.LimitsNameFromRLP(routeRLP),
})).WithContext(ctx).Should(Succeed())
Expand All @@ -505,7 +508,7 @@ var _ = Describe("RateLimitPolicy controller", Ordered, func() {
MaxValue: 10,
Seconds: 5,
Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP),
Conditions: []string{`limit.route__8a84e406 == "1"`},
Conditions: []string{fmt.Sprintf(`%s == "1"`, rlptools.LimitNameToLimitadorIdentifier(routeRLPKey, "route"))},
Variables: []string{},
Name: rlptools.LimitsNameFromRLP(routeRLP),
})).WithContext(ctx).Should(Succeed())
Expand Down Expand Up @@ -540,7 +543,7 @@ var _ = Describe("RateLimitPolicy controller", Ordered, func() {
MaxValue: 1,
Seconds: 180,
Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP),
Conditions: []string{`limit.l1__2804bad6 == "1"`},
Conditions: []string{fmt.Sprintf(`%s == "1"`, rlptools.LimitNameToLimitadorIdentifier(routeRLPKey, "l1"))},
Variables: []string{},
Name: rlptools.LimitsNameFromRLP(routeRLP),
})).WithContext(ctx).Should(Succeed())
Expand Down Expand Up @@ -579,7 +582,7 @@ var _ = Describe("RateLimitPolicy controller", Ordered, func() {
MaxValue: 10,
Seconds: 5,
Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP),
Conditions: []string{`limit.route__8a84e406 == "1"`},
Conditions: []string{fmt.Sprintf(`%s == "1"`, rlptools.LimitNameToLimitadorIdentifier(routeRLPKey, "route"))},
Variables: []string{},
Name: rlptools.LimitsNameFromRLP(routeRLP),
})).WithContext(ctx).Should(Succeed())
Expand All @@ -602,7 +605,7 @@ var _ = Describe("RateLimitPolicy controller", Ordered, func() {
MaxValue: 1,
Seconds: 180,
Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP),
Conditions: []string{`limit.l1__2804bad6 == "1"`},
Conditions: []string{fmt.Sprintf(`%s == "1"`, rlptools.LimitNameToLimitadorIdentifier(routeRLPKey, "l1"))},
Variables: []string{},
Name: rlptools.LimitsNameFromRLP(routeRLP),
})).WithContext(ctx).Should(Succeed())
Expand All @@ -628,7 +631,7 @@ var _ = Describe("RateLimitPolicy controller", Ordered, func() {
MaxValue: 1,
Seconds: 180,
Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP),
Conditions: []string{`limit.l1__2804bad6 == "1"`},
Conditions: []string{fmt.Sprintf(`%s == "1"`, rlptools.LimitNameToLimitadorIdentifier(routeRLPKey, "l1"))},
Variables: []string{},
Name: rlptools.LimitsNameFromRLP(routeRLP),
})).WithContext(ctx).Should(Succeed())
Expand All @@ -651,7 +654,7 @@ var _ = Describe("RateLimitPolicy controller", Ordered, func() {
MaxValue: 10,
Seconds: 5,
Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP),
Conditions: []string{`limit.route__8a84e406 == "1"`},
Conditions: []string{fmt.Sprintf(`%s == "1"`, rlptools.LimitNameToLimitadorIdentifier(routeRLPKey, "route"))},
Variables: []string{},
Name: rlptools.LimitsNameFromRLP(routeRLP),
})).WithContext(ctx).Should(Succeed())
Expand Down Expand Up @@ -1208,6 +1211,148 @@ var _ = Describe("RateLimitPolicy controller", Ordered, func() {
Eventually(assertPolicyIsAcceptedAndEnforced(ctx, rlpAKey)).WithContext(ctx).Should(BeTrue())
}, testTimeOut)
})

Context("HTTPRoute with multiple gateway parents", func() {
var (
gatewayAName = "gateway-a"
gatewayBName = "gateway-b"
targetedRouteName = "targeted-route"
untargetedRouteName = "untargeted-route"

gatewayA *gatewayapiv1.Gateway
gatewayB *gatewayapiv1.Gateway
targetedRoute *gatewayapiv1.HTTPRoute
untargetedRoute *gatewayapiv1.HTTPRoute
)

BeforeEach(func(ctx SpecContext) {
gatewayA = tests.BuildBasicGateway(gatewayAName, testNamespace, func(g *gatewayapiv1.Gateway) {
g.Spec.Listeners[0].Hostname = ptr.To(gatewayapiv1.Hostname("*.a.example.com"))
})
err := k8sClient.Create(ctx, gatewayA)
Expect(err).ToNot(HaveOccurred())
Eventually(tests.GatewayIsReady(ctx, testClient(), gatewayA)).WithContext(ctx).Should(BeTrue())

gatewayB = tests.BuildBasicGateway(gatewayBName, testNamespace, func(g *gatewayapiv1.Gateway) {
g.Spec.Listeners[0].Hostname = ptr.To(gatewayapiv1.Hostname("*.b.example.com"))
})
err = k8sClient.Create(ctx, gatewayB)
Expect(err).ToNot(HaveOccurred())
Eventually(tests.GatewayIsReady(ctx, testClient(), gatewayB)).WithContext(ctx).Should(BeTrue())

gatewayParentsFunc := func(r *gatewayapiv1.HTTPRoute) {
r.Spec.ParentRefs = []gatewayapiv1.ParentReference{
{Name: gatewayapiv1.ObjectName(gatewayAName)},
{Name: gatewayapiv1.ObjectName(gatewayBName)},
}
}

targetedRoute = tests.BuildBasicHttpRoute(targetedRouteName, gatewayAName, testNamespace, []string{"targeted.a.example.com", "targeted.b.example.com"}, gatewayParentsFunc)
err = k8sClient.Create(ctx, targetedRoute)
Expect(err).ToNot(HaveOccurred())
Eventually(tests.RouteIsAccepted(ctx, testClient(), client.ObjectKeyFromObject(targetedRoute))).WithContext(ctx).Should(BeTrue())

untargetedRoute = tests.BuildBasicHttpRoute(untargetedRouteName, gatewayAName, testNamespace, []string{"untargeted.a.example.com", "untargeted.b.example.com"}, gatewayParentsFunc)
err = k8sClient.Create(ctx, untargetedRoute)
Expect(err).ToNot(HaveOccurred())
Eventually(tests.RouteIsAccepted(ctx, testClient(), client.ObjectKeyFromObject(untargetedRoute))).WithContext(ctx).Should(BeTrue())
})

It("It defines route policy limits with gateway policy overrides", func(ctx SpecContext) {
rlpGatewayA := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) {
policy.ObjectMeta.Name = gatewayAName
policy.Spec.TargetRef.Kind = "Gateway"
policy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(gatewayAName)
policy.Spec.Defaults = nil
policy.Spec.Overrides = &kuadrantv1beta2.RateLimitPolicyCommonSpec{
Limits: map[string]kuadrantv1beta2.Limit{
"gw-a-1000rps": {
Rates: []kuadrantv1beta2.Rate{
{
Limit: 1000, Duration: 1, Unit: "second",
},
},
},
},
}
})
err := k8sClient.Create(ctx, rlpGatewayA)
Expect(err).ToNot(HaveOccurred())

rlpGatewayB := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) {
policy.ObjectMeta.Name = gatewayBName
policy.Spec.TargetRef.Kind = "Gateway"
policy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(gatewayBName)
policy.Spec.Defaults = nil
policy.Spec.Overrides = &kuadrantv1beta2.RateLimitPolicyCommonSpec{
Limits: map[string]kuadrantv1beta2.Limit{
"gw-b-100rps": {
Rates: []kuadrantv1beta2.Rate{
{
Limit: 100, Duration: 1, Unit: "second",
},
},
},
},
}
})
err = k8sClient.Create(ctx, rlpGatewayB)
Expect(err).ToNot(HaveOccurred())

rlpTargetedRoute := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) {
policy.ObjectMeta.Name = targetedRouteName
policy.Spec.TargetRef.Kind = "HTTPRoute"
policy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(targetedRouteName)
policy.Spec.CommonSpec().Limits = map[string]kuadrantv1beta2.Limit{
"route-10rps": {
Rates: []kuadrantv1beta2.Rate{
{
Limit: 10, Duration: 1, Unit: "second",
},
},
},
}
})
err = k8sClient.Create(ctx, rlpTargetedRoute)
Expect(err).ToNot(HaveOccurred())

Eventually(limitadorContainsLimit(
ctx,
limitadorv1alpha1.RateLimit{
MaxValue: 1000,
Seconds: 1,
Namespace: rlptools.LimitsNamespaceFromRLP(rlpTargetedRoute),
Conditions: []string{fmt.Sprintf(`%s == "1"`, rlptools.LimitNameToLimitadorIdentifier(client.ObjectKeyFromObject(rlpTargetedRoute), "gw-a-1000rps"))},
Variables: []string{},
Name: rlptools.LimitsNameFromRLP(rlpTargetedRoute),
},
limitadorv1alpha1.RateLimit{
MaxValue: 100,
Seconds: 1,
Namespace: rlptools.LimitsNamespaceFromRLP(rlpTargetedRoute),
Conditions: []string{fmt.Sprintf(`%s == "1"`, rlptools.LimitNameToLimitadorIdentifier(client.ObjectKeyFromObject(rlpTargetedRoute), "gw-b-100rps"))},
Variables: []string{},
Name: rlptools.LimitsNameFromRLP(rlpTargetedRoute),
},
limitadorv1alpha1.RateLimit{ // FIXME(@guicassolato): we need to create one limit definition per gateway × route combination, not one per gateway × policy combination
MaxValue: 1000,
Seconds: 1,
Namespace: rlptools.LimitsNamespaceFromRLP(rlpGatewayA),
Conditions: []string{fmt.Sprintf(`%s == "1"`, rlptools.LimitNameToLimitadorIdentifier(client.ObjectKeyFromObject(rlpGatewayA), "gw-a-1000rps"))},
Variables: []string{},
Name: rlptools.LimitsNameFromRLP(rlpGatewayA),
},
limitadorv1alpha1.RateLimit{
MaxValue: 100,
Seconds: 1,
Namespace: rlptools.LimitsNamespaceFromRLP(rlpGatewayB),
Conditions: []string{fmt.Sprintf(`%s == "1"`, rlptools.LimitNameToLimitadorIdentifier(client.ObjectKeyFromObject(rlpGatewayB), "gw-b-100rps"))},
Variables: []string{},
Name: rlptools.LimitsNameFromRLP(rlpGatewayB),
},
)).WithContext(ctx).Should(Succeed())
})
})
})

var _ = Describe("RateLimitPolicy CEL Validations", func() {
Expand Down
Loading

0 comments on commit 9ad525d

Please sign in to comment.