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.

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

Reconcile resources #240

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
29 changes: 29 additions & 0 deletions controllers/authconfig_eventmapper.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package controllers

import (
"github.com/go-logr/logr"
"github.com/kuadrant/kuadrant-operator/pkg/common"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
)

// AuthConfigEventMapper is an EventHandler that maps AuthConfig objects events to Policy events.
type AuthConfigEventMapper struct {
Logger logr.Logger
}

func (m *AuthConfigEventMapper) MapToAuthPolicy(obj client.Object) []reconcile.Request {
return m.mapToPolicyRequest(obj, "authpolicy", common.AuthPoliciesBackRefAnnotation)
}

func (m *AuthConfigEventMapper) mapToPolicyRequest(obj client.Object, policyKind string, policyBackRefAnnotationName string) []reconcile.Request {
policyRef, found := common.ReadAnnotationsFromObject(obj)[policyBackRefAnnotationName]
if !found {
return []reconcile.Request{}
}

policyKey := common.NamespacedNameToObjectKey(policyRef, obj.GetNamespace())

m.Logger.V(1).Info("Processing object", "object", client.ObjectKeyFromObject(obj), policyKind, policyKey)
return []reconcile.Request{{NamespacedName: policyKey}}
}
5 changes: 5 additions & 0 deletions controllers/authpolicy_auth_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ func (r *AuthPolicyReconciler) desiredAuthConfig(ap *api.AuthPolicy, targetNetwo
return nil, err
}

policyKey := client.ObjectKeyFromObject(ap)

return &authorinoapi.AuthConfig{
TypeMeta: metav1.TypeMeta{
Kind: "AuthConfig",
Expand All @@ -75,6 +77,9 @@ func (r *AuthPolicyReconciler) desiredAuthConfig(ap *api.AuthPolicy, targetNetwo
ObjectMeta: metav1.ObjectMeta{
Name: authConfigName(client.ObjectKeyFromObject(ap)),
Namespace: ap.Namespace,
Annotations: map[string]string{
common.AuthPoliciesBackRefAnnotation: policyKey.String(),
},
},
Spec: authorinoapi.AuthConfigSpec{
Hosts: hosts,
Expand Down
8 changes: 8 additions & 0 deletions controllers/authpolicy_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"context"
"encoding/json"

authorinoapi "github.com/kuadrant/authorino/api/v1beta1"

"github.com/go-logr/logr"
apierrors "k8s.io/apimachinery/pkg/api/errors"
ctrl "sigs.k8s.io/controller-runtime"
Expand Down Expand Up @@ -199,6 +201,10 @@ func (r *AuthPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error {
Logger: r.Logger().WithName("gatewayEventMapper"),
}

authConfigEventMapper := &AuthConfigEventMapper{
Logger: r.Logger().WithName("authConfigEventMapper"),
}

return ctrl.NewControllerManagedBy(mgr).
For(&api.AuthPolicy{}).
Watches(
Expand All @@ -207,5 +213,7 @@ func (r *AuthPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error {
).
Watches(&source.Kind{Type: &gatewayapiv1beta1.Gateway{}},
handler.EnqueueRequestsFromMapFunc(gatewayEventMapper.MapToAuthPolicy)).
Watches(&source.Kind{Type: &authorinoapi.AuthConfig{}},
handler.EnqueueRequestsFromMapFunc(authConfigEventMapper.MapToAuthPolicy)).
Complete(r)
}
44 changes: 44 additions & 0 deletions controllers/limitador_eventmapper.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package controllers

import (
"encoding/json"

"github.com/go-logr/logr"
"github.com/kuadrant/kuadrant-operator/pkg/common"
limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
)

type LimitadorEventMapper struct {
Logger logr.Logger
}

func (m LimitadorEventMapper) MapToRateLimitPolicy(obj client.Object) []reconcile.Request {
limitador, ok := obj.(*limitadorv1alpha1.Limitador)
if !ok {
return []reconcile.Request{}
}

objAnnotations := limitador.GetAnnotations()
val, ok := objAnnotations[common.RateLimitPoliciesBackRefAnnotation]
if !ok {
return []reconcile.Request{}
}

var refs []client.ObjectKey
err := json.Unmarshal([]byte(val), &refs)
if err != nil {
return []reconcile.Request{}
}

requests := make([]reconcile.Request, 0)
for _, ref := range refs {
m.Logger.V(1).Info("MapRateLimitPolicy", "ratelimitpolicy", ref)
Copy link
Contributor

Choose a reason for hiding this comment

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

With the current implementation, you do not need to trigger one event per RLP, only one of them is enough to reconcile the entire Limitador CR. Each rate limit policy event is reading all gateways from the cluster and iterating over all the RLP references in the annotations of the gateways.

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 many explore this. What your saying is making sense but my mind is not allowing me to accept it as true. 😃 I will add any changes as needed.

requests = append(requests, reconcile.Request{
NamespacedName: ref,
})
break
}
return requests
}
19 changes: 17 additions & 2 deletions controllers/ratelimitpolicy_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"encoding/json"

"github.com/go-logr/logr"
limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -186,7 +187,7 @@ func (r *RateLimitPolicyReconciler) reconcileResources(ctx context.Context, rlp
return err
}

// set annotation of policies afftecting the gateway - should be the last step, only when all the reconciliation steps succeed
// set annotation of policies affecting the gateway - should be the last step, only when all the reconciliation steps succeed
return r.ReconcileGatewayPolicyReferences(ctx, rlp, gatewayDiffObj)
}

Expand Down Expand Up @@ -216,7 +217,13 @@ func (r *RateLimitPolicyReconciler) deleteResources(ctx context.Context, rlp *ku
}
}

// update annotation of policies afftecting the gateway
// remove direct back ref from limitador CR
err = r.deleteLimitadorBackReference(ctx, rlp)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. So deleteLimitsreconcileLimitadorreconcileLimitadorBackRef adds the RLP ref to the back-ref annotation and performs an update on the Limitador resource, then we call deleteLimitadorBackReference, which removes the same RLP ref from the annotation and updates the Limitador resource again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is correct and I should explain my reasoning. The reconcileLimitador function runs no matter what action is being carried out let it be create, update or delete. The label removal logic could be in the reconcileLimitadorBackRef but doing this would mean having recreate the logic for checking if the resource has being deleted. As it is the deleteResources --> deleteLimitadorBackReference only get called when a RLP is deleted there was no need to any more checks in the deleteLimitadorBackReference. I am aware there are more api calls with what I have done but as its only on deletion and not every reconcile I felt the trade off was worth it.

Do you want me to move the label deletion logic to the reconcileLimitadorBackRef?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want me to move the label deletion logic to the reconcileLimitadorBackRef?

I would wait. As I said in my other comment, if we're going to watch and reconcile changes on all derived resources – the Limitador CR is just one of then! –, I see more of this pattern of annotating/de-annotating ahead. I think we can aim for more generic functions and types that we can reuse in more places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the community call my understanding is we wont be using the https://github.com/Kuadrant/gateway-api-machinery till after MVP. So I thinking I address the other issue raised now and create an issue to follow up with a refactor of the labelling once we can use the gateway-api-machinery. Would you be happy with this approach?

Copy link
Contributor

Choose a reason for hiding this comment

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

This won't be complete anyway until we cover for all kinds of resources that, once modified directly, can break with the source of truth. I understand that whereas one is a refactoring, while the other prevents what could be perceived as a bug, I still think those two things are intimately related and better if tackled together. I'm also afraid of the explosion of annotations while others seem to be addressing the issue differently. IOW, let's not amend it quickly due to release pressure and do this the right way. The fix can be part of a post-release patch if we want.

if err != nil {
return err
}

// update annotation of policies affecting the gateway
return r.ReconcileGatewayPolicyReferences(ctx, rlp, gatewayDiffObj)
}

Expand All @@ -241,6 +248,11 @@ func (r *RateLimitPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error {
Logger: r.Logger().WithName("gatewayRateLimitPolicyEventMapper"),
Client: r.Client(),
}

limitadorEventMapper := &LimitadorEventMapper{
Logger: r.Logger().WithName("limitadorEventMapper"),
}

return ctrl.NewControllerManagedBy(mgr).
For(&kuadrantv1beta2.RateLimitPolicy{}).
Watches(
Expand All @@ -258,5 +270,8 @@ func (r *RateLimitPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error {
&source.Kind{Type: &kuadrantv1beta2.RateLimitPolicy{}},
handler.EnqueueRequestsFromMapFunc(gatewayRateLimtPolicyEventMapper.MapRouteRateLimitPolicy),
).
Watches(&source.Kind{Type: &limitadorv1alpha1.Limitador{}},
handler.EnqueueRequestsFromMapFunc(limitadorEventMapper.MapToRateLimitPolicy),
).
Complete(r)
}
59 changes: 57 additions & 2 deletions controllers/ratelimitpolicy_limits.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ func (r *RateLimitPolicyReconciler) reconcileLimitador(ctx context.Context, rlp
return err
}

rplsBackref, err := common.BuildBackRefs(rlpRefs)
if err != nil {
return err
}

// get the current limitador cr for the kuadrant instance so we can compare if it needs to be updated
logger.V(1).Info("get kuadrant namespace")
var kuadrantNamespace string
Expand All @@ -68,13 +73,17 @@ func (r *RateLimitPolicyReconciler) reconcileLimitador(ctx context.Context, rlp
return err
}

// return if limitador is up to date
if rlptools.Equal(rateLimitIndex.ToRateLimits(), limitador.Spec.Limits) {
update := false
update = update || !common.AnnotationEqual(common.RateLimitPoliciesBackRefAnnotation, rplsBackref, limitador.Annotations)
update = update || !rlptools.Equal(rateLimitIndex.ToRateLimits(), limitador.Spec.Limits)
// return if limitador is up-to-date
if !update {
logger.V(1).Info("limitador is up to date, skipping update")
return nil
}

// update limitador
limitador.SetAnnotations(common.AddAnnotation(common.RateLimitPoliciesBackRefAnnotation, rplsBackref, limitador.Annotations))
limitador.Spec.Limits = rateLimitIndex.ToRateLimits()
err = r.UpdateResource(ctx, limitador)
logger.V(1).Info("update limitador", "limitador", limitadorKey, "err", err)
Expand Down Expand Up @@ -108,3 +117,49 @@ func (r *RateLimitPolicyReconciler) buildRateLimitIndex(ctx context.Context, rlp

return rateLimitIndex, nil
}

func (r *RateLimitPolicyReconciler) deleteLimitadorBackReference(ctx context.Context, policy client.Object) error {
kuadrantNamespace, err := common.GetKuadrantNamespace(policy)
if err != nil {
return err
}

limitadorList, err := r.listLimitadorByNamespace(ctx, kuadrantNamespace)
if err != nil {
return err
}

policyKey := client.ObjectKeyFromObject(policy)
updateList, err := rlptools.RemoveRLPLabelsFromLimitadorList(limitadorList, policyKey)
if err != nil {
return err
}

err = r.updateLimitadorCRs(ctx, updateList)
if err != nil {
return err
}

return nil
}

func (r *RateLimitPolicyReconciler) updateLimitadorCRs(ctx context.Context, updateList limitadorv1alpha1.LimitadorList) error {
for index := range updateList.Items {
err := r.Client().Update(ctx, &updateList.Items[index])
if err != nil {
return err
}
}
return nil
}

func (r *RateLimitPolicyReconciler) listLimitadorByNamespace(ctx context.Context, namespace string) (limitadorv1alpha1.LimitadorList, error) {
limitadorList := limitadorv1alpha1.LimitadorList{}
listOptions := &client.ListOptions{Namespace: namespace}

err := r.Client().List(ctx, &limitadorList, listOptions)
if err != nil {
return limitadorv1alpha1.LimitadorList{}, err
}
return limitadorList, nil
}
32 changes: 32 additions & 0 deletions pkg/common/annotations.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package common

func AddAnnotation(annotation string, value string, annotations map[string]string) map[string]string {
_, ok := annotations[annotation]
if !ok && len(value) == 0 {
return annotations
}

if len(value) == 0 {
delete(annotations, annotation)
return annotations
}

if annotations == nil {
annotations = map[string]string{}
}
annotations[annotation] = value
return annotations
}

func AnnotationEqual(annotation string, value string, annotations map[string]string) bool {
val, ok := annotations[annotation]
if !ok && len(annotation) == 0 {
return true
}

if val == value {
return true
}

return false
}
26 changes: 26 additions & 0 deletions pkg/common/back_reference.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package common

import (
"encoding/json"
"sigs.k8s.io/controller-runtime/pkg/client"
)

func BuildBackRefs(refs []client.ObjectKey) (string, error) {
if len(refs) == 0 {
return "", nil
}

var uniqueKeys []client.ObjectKey

for _, v := range refs {
if !Contains(uniqueKeys, v) {
uniqueKeys = append(uniqueKeys, v)
}
}

serialized, err := json.Marshal(uniqueKeys)
if err != nil {
return "", err
}
return string(serialized), nil
}
40 changes: 40 additions & 0 deletions pkg/rlptools/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,12 @@ package rlptools
import (
"crypto/sha256"
"encoding/hex"
"encoding/json"
"fmt"
"unicode"

"sigs.k8s.io/controller-runtime/pkg/client"

limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1"

kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2"
Expand Down Expand Up @@ -88,3 +91,40 @@ func rateToSeconds(rate kuadrantv1beta2.Rate) (maxValue int, seconds int) {

return
}

func RemoveRLPLabelsFromLimitadorList(limitadorList limitadorv1alpha1.LimitadorList, policyKey client.ObjectKey) (limitadorv1alpha1.LimitadorList, error) {
var updateList limitadorv1alpha1.LimitadorList
for index := range limitadorList.Items {
limitador := limitadorList.Items[index]
objAnnotations := limitador.GetAnnotations()
val, ok := objAnnotations[common.RateLimitPoliciesBackRefAnnotation]
if !ok {
continue
}

var refs []client.ObjectKey
err := json.Unmarshal([]byte(val), &refs)
if err != nil {
return updateList, err
}
refID := common.FindObjectKey(refs, policyKey)
if refID != len(refs) {
// remove index
refs = append(refs[:refID], refs[refID+1:]...)

if len(refs) > 0 {
serialized, err := json.Marshal(refs)
if err != nil {
return updateList, err
}
objAnnotations[common.RateLimitPoliciesBackRefAnnotation] = string(serialized)
} else {
delete(objAnnotations, common.RateLimitPoliciesBackRefAnnotation)
}

limitador.SetAnnotations(objAnnotations)
updateList.Items = append(updateList.Items, limitador)
}
}
return updateList, nil
}
Loading