Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix override RLPs on multiple gateway parents #659

Merged
merged 5 commits into from
Jun 5, 2024
Merged

Fix override RLPs on multiple gateway parents #659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3bf545f
Fix override RLPs on multiple gateway parents
guicassolato May 22, 2024
be5e146
fix: all rlps should go to the limits config - maybe there are overri…
guicassolato May 23, 2024
44d0e0c
rollback simplified wasm config based on fallback to overrides, in fa…
guicassolato May 23, 2024
34a2e47
fix: RLP Enforced condition with multiple gateway parents
guicassolato May 29, 2024
8c489c5
go mod tidy
guicassolato Jun 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 11 additions & 24 deletions controllers/rate_limiting_wasmplugin_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
"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 @@
// For more details, check Reconcile and its Result here:
// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.10.0/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())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that debugging reconciliation calls can be a PITA, so I'm slowly adding these to the functions.

logger.Info("Reconciling rate limiting WASMPlugin")
ctx := logr.NewContext(eventCtx, logger)

Expand Down Expand Up @@ -174,7 +175,7 @@

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 @@

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 @@
return nil, err
}

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

Check warning on line 235 in controllers/rate_limiting_wasmplugin_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/rate_limiting_wasmplugin_controller.go#L235 

Added line #L235 was not covered by tests
}

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 @@
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
guicassolato marked this conversation as resolved.
Show resolved Hide resolved
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 @@
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
View file
Open in desktop
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/controller-runtime@v0.10.0/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
View file
Open in desktop
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
Loading