From 9adfed39e34b425065d772dedd9ca24a9a99ea29 Mon Sep 17 00:00:00 2001 From: Rahul Anand Date: Wed, 16 Feb 2022 19:32:11 +0530 Subject: [PATCH] Remove networkingRef from RateLimitPolicy (#86) * remove networkingRef from RateLimitPolicy * fix lint errors * update samples for RLP * update sample RLP to include vhost level RateLimit --- apis/apim/v1alpha1/ratelimitpolicy_types.go | 4 - apis/apim/v1alpha1/zz_generated.deepcopy.go | 5 - .../apim.kuadrant.io_ratelimitpolicies.yaml | 19 -- config/deploy/manifests.yaml | 19 -- .../apim_v1alpha1_ratelimitpolicy.yaml | 50 +++-- .../apim/ratelimitpolicy_controller.go | 199 ++++++++++-------- .../apim/ratelimitpolicy_finalizers.go | 142 ++++++++----- controllers/apim/ratelimitpolicy_mapper.go | 159 ++++++++++++++ controllers/apim/utils.go | 5 +- controllers/apim/virtualservice_controller.go | 40 +--- examples/toystore/ratelimitpolicy.yaml | 9 +- pkg/common/istio_utils.go | 4 - 12 files changed, 410 insertions(+), 245 deletions(-) create mode 100644 controllers/apim/ratelimitpolicy_mapper.go diff --git a/apis/apim/v1alpha1/ratelimitpolicy_types.go b/apis/apim/v1alpha1/ratelimitpolicy_types.go index 20e773855..8fe3eb7e8 100644 --- a/apis/apim/v1alpha1/ratelimitpolicy_types.go +++ b/apis/apim/v1alpha1/ratelimitpolicy_types.go @@ -87,10 +87,6 @@ type NetworkingRef struct { // RateLimitPolicySpec defines the desired state of RateLimitPolicy type RateLimitPolicySpec struct { - //+listType=map - //+listMapKey=type - //+listMapKey=name - NetworkingRef []NetworkingRef `json:"networkingRef,omitempty"` // route specific staging and actions //+listType=map //+listMapKey=name diff --git a/apis/apim/v1alpha1/zz_generated.deepcopy.go b/apis/apim/v1alpha1/zz_generated.deepcopy.go index c9a65c294..abb5edc85 100644 --- a/apis/apim/v1alpha1/zz_generated.deepcopy.go +++ b/apis/apim/v1alpha1/zz_generated.deepcopy.go @@ -273,11 +273,6 @@ func (in *RateLimitPolicyList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *RateLimitPolicySpec) DeepCopyInto(out *RateLimitPolicySpec) { *out = *in - if in.NetworkingRef != nil { - in, out := &in.NetworkingRef, &out.NetworkingRef - *out = make([]NetworkingRef, len(*in)) - copy(*out, *in) - } if in.Routes != nil { in, out := &in.Routes, &out.Routes *out = make([]Route, len(*in)) diff --git a/config/crd/bases/apim.kuadrant.io_ratelimitpolicies.yaml b/config/crd/bases/apim.kuadrant.io_ratelimitpolicies.yaml index ed72bd92c..b143c01b7 100644 --- a/config/crd/bases/apim.kuadrant.io_ratelimitpolicies.yaml +++ b/config/crd/bases/apim.kuadrant.io_ratelimitpolicies.yaml @@ -62,25 +62,6 @@ spec: - variables type: object type: array - networkingRef: - items: - properties: - name: - type: string - type: - enum: - - HTTPRoute - - VirtualService - type: string - required: - - name - - type - type: object - type: array - x-kubernetes-list-map-keys: - - type - - name - x-kubernetes-list-type: map rateLimits: description: RateLimits are used for all of the matching rules items: diff --git a/config/deploy/manifests.yaml b/config/deploy/manifests.yaml index 4ba2c1857..353408c8d 100644 --- a/config/deploy/manifests.yaml +++ b/config/deploy/manifests.yaml @@ -138,25 +138,6 @@ spec: - variables type: object type: array - networkingRef: - items: - properties: - name: - type: string - type: - enum: - - HTTPRoute - - VirtualService - type: string - required: - - name - - type - type: object - type: array - x-kubernetes-list-map-keys: - - type - - name - x-kubernetes-list-type: map rateLimits: description: RateLimits are used for all of the matching rules items: diff --git a/config/samples/apim_v1alpha1_ratelimitpolicy.yaml b/config/samples/apim_v1alpha1_ratelimitpolicy.yaml index aab33ba8c..d660f0d6c 100644 --- a/config/samples/apim_v1alpha1_ratelimitpolicy.yaml +++ b/config/samples/apim_v1alpha1_ratelimitpolicy.yaml @@ -1,30 +1,48 @@ +--- apiVersion: apim.kuadrant.io/v1alpha1 kind: RateLimitPolicy metadata: - name: ratelimitpolicy-sample - namespace: kuadrant-system - labels: - app: toystore + name: toystore spec: - networkingRef: - - type: VirtualService - name: toystore routes: - name: get-toy - stage: BOTH - actions: - - generic_key: - descriptor_key: get - descriptor_value: "yes" + rateLimits: + - stage: BOTH + actions: + - generic_key: + descriptor_key: get-toy + descriptor_value: "yes" + - generic_key: + descriptor_key: other-get-toy + descriptor_value: "yes" - name: add-toy - stage: POSTAUTH + rateLimits: + - stage: POSTAUTH + actions: + - generic_key: + descriptor_key: add-toy + descriptor_value: "yes" + - name: delete-toy + rateLimits: + - stage: PREAUTH + actions: + - generic_key: + descriptor_key: delete-toy + descriptor_value: "yes" + rateLimits: + - stage: PREAUTH actions: - generic_key: - descriptor_key: admin + descriptor_key: vhaction descriptor_value: "yes" limits: - - conditions: ["admin == yes"] + - conditions: ["get-toy == yes"] max_value: 2 - namespace: postauth + namespace: preauth + seconds: 15 + variables: [] + - condtitions: ["vhaction == yes"] + max_value: 4 + namespace: preauth seconds: 15 variables: [] diff --git a/controllers/apim/ratelimitpolicy_controller.go b/controllers/apim/ratelimitpolicy_controller.go index 308c0901d..cd89ef18c 100644 --- a/controllers/apim/ratelimitpolicy_controller.go +++ b/controllers/apim/ratelimitpolicy_controller.go @@ -20,7 +20,6 @@ import ( "context" "encoding/json" "fmt" - "strings" "github.com/go-logr/logr" "github.com/gogo/protobuf/types" @@ -43,8 +42,6 @@ const ( EnvoysHTTPConnectionManagerName = "envoy.filters.network.http_connection_manager" VSAnnotationRateLimitPolicy = "kuadrant.io/ratelimitpolicy" - - InvalidNetworkingRefTypeErr = "unknown networking reference type" ) // RateLimitPolicyReconciler reconciles a RateLimitPolicy object @@ -108,61 +105,128 @@ func (r *RateLimitPolicyReconciler) Reconcile(eventCtx context.Context, req ctrl return ctrl.Result{}, err } - for _, networkingRef := range rlp.Spec.NetworkingRef { - switch networkingRef.Type { - case apimv1alpha1.NetworkingRefTypeHR: - logger.Info("HTTPRoute is not implemented yet") // TODO(rahulanand16nov) - return ctrl.Result{}, nil - case apimv1alpha1.NetworkingRefTypeVS: - vsNamespacedName := client.ObjectKey{ - Namespace: rlp.Namespace, // VirtualService lookup is limited to RLP's namespace - Name: networkingRef.Name, - } + // Operation specific annotations must be removed if they were present + updateRequired := false + // check for delete operation for virtualservice + if vsName, present := rlp.Annotations[KuadrantDeleteVSAnnotation]; present { + vsNamespacedName := client.ObjectKey{ + Namespace: rlp.Namespace, // VirtualService lookup is limited to RLP's namespace + Name: vsName, + } + vsKey := vsNamespacedName.String() - var vs istio.VirtualService - if err := r.Client().Get(ctx, vsNamespacedName, &vs); err != nil { - if apierrors.IsNotFound(err) { - logger.Info("no VirtualService found", "lookup name", vsNamespacedName.String()) - return ctrl.Result{}, nil - } - logger.Error(err, "failed to get VirutalService") - return ctrl.Result{}, err + var vs istio.VirtualService + if err := r.Client().Get(ctx, vsNamespacedName, &vs); err != nil { + if apierrors.IsNotFound(err) { + logger.Info("no VirtualService found", "lookup name", vsNamespacedName.String()) + return ctrl.Result{}, nil } + logger.Error(err, "failed to get VirutalService") + return ctrl.Result{}, err + } + + if err := r.detachFromNetwork(ctx, vs.Spec.Gateways, vsKey, &rlp); err != nil { + logger.Error(err, "failed to detach RateLimitPolicy from VirtualService") + return ctrl.Result{}, err + } + + delete(rlp.Annotations, KuadrantDeleteVSAnnotation) + updateRequired = true + } - if err := r.reconcileWithVirtualService(ctx, &vs, &rlp); err != nil { - logger.Error(err, "failed to reconcile with VirtualService") - return ctrl.Result{}, err + // check for add operation for virtualservice + if vsName, present := rlp.Annotations[KuadrantAddVSAnnotation]; present { + vsNamespacedName := client.ObjectKey{ + Namespace: rlp.Namespace, + Name: vsName, + } + vsKey := vsNamespacedName.String() + + var vs istio.VirtualService + if err := r.Client().Get(ctx, vsNamespacedName, &vs); err != nil { + if apierrors.IsNotFound(err) { + logger.Info("no VirtualService found", "lookup name", vsNamespacedName.String()) + return ctrl.Result{}, nil } - default: - err := fmt.Errorf(InvalidNetworkingRefTypeErr) - logger.Error(err, "networking reconciliation failed") + logger.Error(err, "failed to get VirutalService") return ctrl.Result{}, err } + + if err := r.attachToNetwork(ctx, vs.Spec.Gateways, vs.Spec.Hosts, vsKey, &rlp); err != nil { + logger.Error(err, "failed to attach RateLimitPolicy to VirtualService") + return ctrl.Result{}, err + } + + delete(rlp.Annotations, KuadrantAddVSAnnotation) + updateRequired = true + } + + if updateRequired { + if err := r.Client().Update(ctx, &rlp); err != nil { + logger.Error(err, "failed to remove operation specific annotations from RateLimitPolicy") + return ctrl.Result{}, err + } + logger.Info("successfully removed operation specific annotations from RateLimitPolicy") } + + // TODO(rahulanand16nov): do the same as above for HTTPRoute + // TODO(rahulanand16nov): Status block should be used to store networking objects that referenced this RLP to prevent + // heavy calls to kube api for every reconciliation loop. Note, this should be done after add/delete operations. + logger.Info("successfully reconciled RateLimitPolicy") return ctrl.Result{}, nil } -func (r *RateLimitPolicyReconciler) reconcileWithVirtualService(ctx context.Context, vs *istio.VirtualService, rlp *apimv1alpha1.RateLimitPolicy) error { - logger := r.Logger() - rlpKey := client.ObjectKeyFromObject(rlp) +func (r *RateLimitPolicyReconciler) detachFromNetwork(ctx context.Context, gateways []string, owner string, rlp *apimv1alpha1.RateLimitPolicy) error { + logger := logr.FromContext(ctx) + ownerKey := common.NamespacedNameToObjectKey(owner, rlp.Namespace) + logger.Info("Detaching RateLimitPolicy from a network") + + for _, gw := range gateways { + gwKey := common.NamespacedNameToObjectKey(gw, rlp.Namespace) - _, ok := vs.Annotations[VSAnnotationRateLimitPolicy] - if !ok { - vs.Annotations[VSAnnotationRateLimitPolicy] = rlpKey.String() - if err := r.Client().Update(ctx, vs); err != nil { - logger.Error(err, "failed to add RateLimitPolicy annotation to VirtualService") + // fetch the filters patch + filtersPatchKey := client.ObjectKey{Namespace: gwKey.Namespace, Name: rlFiltersPatchName(gwKey.Name)} + filtersPatch := &istio.EnvoyFilter{} + if err := r.Client().Get(ctx, filtersPatchKey, filtersPatch); err != nil { + logger.Error(err, "failed to get ratelimit filters patch") return err } - logger.V(1).Info("successfully added RateLimitPolicy annotation to VirtualService") + + // remove the parentRef entry on filters patch + if err := r.removeParentRefEntry(ctx, filtersPatch, owner); err != nil { + logger.Error(err, "failed to remove parentRef entry on the ratelimit filters patch") + return err + } + logger.Info("successfully deleted/updated ratelimit filters patch") + + // fetch the ratelimits patch + ratelimitsPatchKey := client.ObjectKey{Namespace: gwKey.Namespace, Name: ratelimitsPatchName(gwKey.Name, ownerKey)} + ratelimitsPatch := &istio.EnvoyFilter{} + if err := r.Client().Get(ctx, ratelimitsPatchKey, ratelimitsPatch); err != nil { + logger.Error(err, "failed to get ratelimits patch") + return err + } + + // remove the parentRef entry on ratelimits patch + if err := r.removeParentRefEntry(ctx, ratelimitsPatch, owner); err != nil { + logger.Error(err, "failed to remove parentRef entry on the ratelimits patch") + return err + } + logger.Info("successfully deleted/updated ratelimit filters patch") } - // TODO(rahulanand16nov): store context of virtualservice in RLP's status block and manage envoy patches. + logger.Info("successfully detached RateLimitPolicy from specified gateways and hosts") + return nil +} + +func (r *RateLimitPolicyReconciler) attachToNetwork(ctx context.Context, gateways, hosts []string, owner string, rlp *apimv1alpha1.RateLimitPolicy) error { + logger := logr.FromContext(ctx) + ownerKey := common.NamespacedNameToObjectKey(owner, rlp.Namespace) + logger.Info("Attaching RateLimitPolicy to a network") - // create/update EnvoyFilter patches for each gateway - for _, gw := range vs.Spec.Gateways { - gwKey := common.NamespacedNameToObjectKey(gw, vs.Namespace) // Istio defaults to VirtualService's namespace + for _, gw := range gateways { + gwKey := common.NamespacedNameToObjectKey(gw, rlp.Namespace) gwLabels := gatewayLabels(ctx, r.Client(), gwKey) - owner := rlpKey.String() // create/update ratelimit filters patch // fetch already existing filters patch or create a new one @@ -180,64 +244,31 @@ func (r *RateLimitPolicyReconciler) reconcileWithVirtualService(ctx context.Cont } } - // make sure annotation map is initialized - filtersPatchOwnerList := []string{} - if filtersPatch.Annotations == nil { - filtersPatch.Annotations = make(map[string]string) - } - - if ogOwnerRlps, present := filtersPatch.Annotations[envoyFilterAnnotationOwnerRLPs]; present { - filtersPatchOwnerList = strings.Split(ogOwnerRlps, ownerRlpSeparator) - } - - // add the owner name if not present already - if !common.Contains(filtersPatchOwnerList, owner) { - filtersPatchOwnerList = append(filtersPatchOwnerList, owner) - } - finalOwnerVal := strings.Join(filtersPatchOwnerList, ownerRlpSeparator) - - filtersPatch.Annotations[envoyFilterAnnotationOwnerRLPs] = finalOwnerVal - if err := r.ReconcileResource(ctx, &istio.EnvoyFilter{}, filtersPatch, alwaysUpdateEnvoyPatches); err != nil { - logger.Error(err, "failed to create/update EnvoyFilter that patches-in ratelimit filters") + if err := r.addParentRefEntry(ctx, filtersPatch, owner); err != nil { + logger.Error(err, "failed to add ownerRLP entry to the ratelimit filters patch") return err } logger.Info("successfully created/updated ratelimit filters patch", "gateway", gwKey.String()) // create/update ratelimits patch - ratelimitsPatchKey := client.ObjectKey{Namespace: gwKey.Namespace, Name: ratelimitsPatchName(rlp.Name, gwKey.Name)} + ratelimitsPatchKey := client.ObjectKey{Namespace: gwKey.Namespace, Name: ratelimitsPatchName(gwKey.Name, ownerKey)} ratelimitsEnvoyFilter := &istio.EnvoyFilter{} if err := r.Client().Get(ctx, ratelimitsPatchKey, ratelimitsEnvoyFilter); err != nil { if !apierrors.IsNotFound(err) { logger.Error(err, "failed to get ratelimits patch") return err } - ratelimitsEnvoyFilter = desiredRatelimitsEnvoyFilter(rlp, vs.Spec.Hosts, gwKey, gwLabels) - } - - ratelimitsPatchOwnerList := []string{} - if ratelimitsEnvoyFilter.Annotations == nil { - ratelimitsEnvoyFilter.Annotations = make(map[string]string) + ratelimitsEnvoyFilter = desiredRatelimitsEnvoyFilter(rlp, hosts, gwKey, ownerKey, gwLabels) } - if ogOwnerRlps, present := ratelimitsEnvoyFilter.Annotations[envoyFilterAnnotationOwnerRLPs]; present { - ratelimitsPatchOwnerList = strings.Split(ogOwnerRlps, ownerRlpSeparator) - } - - // add the owner name if not present already - if !common.Contains(ratelimitsPatchOwnerList, owner) { - ratelimitsPatchOwnerList = append(ratelimitsPatchOwnerList, owner) - } - finalOwnerVal = strings.Join(ratelimitsPatchOwnerList, ownerRlpSeparator) - - ratelimitsEnvoyFilter.Annotations[envoyFilterAnnotationOwnerRLPs] = finalOwnerVal + // Note(rahulanand16nov): ratelimits patch don't require parentRef because they are unique per VirtualService if err := r.ReconcileResource(ctx, &istio.EnvoyFilter{}, ratelimitsEnvoyFilter, alwaysUpdateEnvoyPatches); err != nil { - logger.Error(err, "failed to create/update EnvoyFilter that patches-in ratelimits") + logger.Error(err, "failed to reconcile ratelimits patch") return err } logger.Info("successfully created/updated ratelimits patch", "gateway", gwKey.String()) } - - logger.Info("successfully reconciled RateLimitPolicy using attached VirtualService") + logger.Info("successfully attached RateLimitPolicy to specified gateways and hosts") return nil } @@ -351,8 +382,10 @@ func ratelimitFiltersPatch(gwKey client.ObjectKey, gwLabels map[string]string) ( return factory.EnvoyFilter(), nil } -func desiredRatelimitsEnvoyFilter(rlp *apimv1alpha1.RateLimitPolicy, vHostNames []string, gwKey client.ObjectKey, gwLabels map[string]string) *istio.EnvoyFilter { +func desiredRatelimitsEnvoyFilter(rlp *apimv1alpha1.RateLimitPolicy, vHostNames []string, gwKey, networkingKey client.ObjectKey, gwLabels map[string]string) *istio.EnvoyFilter { patches := make([]*networkingv1alpha3.EnvoyFilter_EnvoyConfigObjectPatch, 0) + + // route-level patches for _, host := range vHostNames { // TODO(eguzki): The VirtualHost name is generated by envoy from the // Virtualservice domain + gateway port information @@ -368,7 +401,7 @@ func desiredRatelimitsEnvoyFilter(rlp *apimv1alpha1.RateLimitPolicy, vHostNames } factory := common.EnvoyFilterFactory{ - ObjectName: ratelimitsPatchName(rlp.Name, gwKey.Name), + ObjectName: ratelimitsPatchName(gwKey.Name, networkingKey), Namespace: gwKey.Namespace, Patches: patches, Labels: gwLabels, diff --git a/controllers/apim/ratelimitpolicy_finalizers.go b/controllers/apim/ratelimitpolicy_finalizers.go index 14aecb372..e9a8e69d9 100644 --- a/controllers/apim/ratelimitpolicy_finalizers.go +++ b/controllers/apim/ratelimitpolicy_finalizers.go @@ -2,22 +2,22 @@ package apim import ( "context" - "fmt" "strings" "github.com/go-logr/logr" apimv1alpha1 "github.com/kuadrant/kuadrant-controller/apis/apim/v1alpha1" "github.com/kuadrant/kuadrant-controller/pkg/common" istio "istio.io/client-go/pkg/apis/networking/v1alpha3" + apierrors "k8s.io/apimachinery/pkg/api/errors" "sigs.k8s.io/controller-runtime/pkg/client" ) const ( patchesFinalizer = "kuadrant.io/ratelimitpatches" - ownerRlpSeparator = "," + ParentRefsSeparator = "," - envoyFilterAnnotationOwnerRLPs = "kuadrant.io/ownerRateLimitPolicies" + EnvoyFilterParentRefsAnnotation = "kuadrant.io/parentRefs" ) // finalizeEnvoyFilters makes sure orphan EnvoyFilter resources are not left when deleting the owner RateLimitPolicy. @@ -26,74 +26,72 @@ func (r *RateLimitPolicyReconciler) finalizeEnvoyFilters(ctx context.Context, rl logger.Info("Removing/Updating EnvoyFilter resources") ownerRlp := client.ObjectKeyFromObject(rlp).String() - for _, networkingRef := range rlp.Spec.NetworkingRef { - switch networkingRef.Type { - case apimv1alpha1.NetworkingRefTypeHR: - logger.Info("HTTPRoute is not implemented yet") // TODO(rahulanand16nov) + // TODO(rahulanand16nov): do the same for HTTPRoute + vsList := &istio.VirtualServiceList{} + if err := r.Client().List(ctx, vsList, &client.ListOptions{Namespace: rlp.Namespace}); err != nil { + logger.Error(err, "failed to list VirtualServices in RateLimitPolicy's namespace") + return err + } + + for idx := range vsList.Items { + if val, present := vsList.Items[idx].Annotations[KuadrantRateLimitPolicyAnnotation]; !present || (val != rlp.Name) { continue - case apimv1alpha1.NetworkingRefTypeVS: - logger.Info("Removing/Updating EnvoyFilter resources using VirtualService") - vs := istio.VirtualService{} - vsKey := client.ObjectKey{Namespace: rlp.Namespace, Name: networkingRef.Name} + } + vsKey := client.ObjectKeyFromObject(&vsList.Items[idx]) + for _, gateway := range vsList.Items[idx].Spec.Gateways { + gwKey := common.NamespacedNameToObjectKey(gateway, vsList.Items[idx].Namespace) - if err := r.Client().Get(ctx, vsKey, &vs); err != nil { - logger.Error(err, "failed to get VirutalService") - return err + filtersPatchkey := client.ObjectKey{ + Namespace: gwKey.Namespace, + Name: rlFiltersPatchName(gwKey.Name), } - - for _, gateway := range vs.Spec.Gateways { - gwKey := common.NamespacedNameToObjectKey(gateway, vs.Namespace) - - filtersPatchkey := client.ObjectKey{ - Namespace: gwKey.Namespace, - Name: rlFiltersPatchName(gwKey.Name), - } - filtersPatch := &istio.EnvoyFilter{} - if err := r.Client().Get(ctx, filtersPatchkey, filtersPatch); err != nil { - logger.Error(err, "failed to fetch ratelimit filters patch") + filtersPatch := &istio.EnvoyFilter{} + if err := r.Client().Get(ctx, filtersPatchkey, filtersPatch); err != nil { + if !apierrors.IsNotFound(err) { + logger.Error(err, "failed to get ratelimits filters patch") return err } + logger.V(1).Info("filters patch not found", "object key", filtersPatchkey.String()) + } + if err := r.removeParentRefEntry(ctx, filtersPatch, ownerRlp); err != nil { + logger.Error(err, "failed to remove parentRef on filters patch") + return err + } - if err := removeOwnerRlpEntry(ctx, r.Client(), filtersPatch, ownerRlp); err != nil { - logger.Error(err, "failed to remove ownerRLP tag on filters patch") - return err - } - - logger.Info("successfully removed ownerRLP entry on the filters patch") - - ratelimitsPatchKey := client.ObjectKey{ - Namespace: gwKey.Namespace, - Name: ratelimitsPatchName(rlp.Name, gwKey.Name), - } - ratelimitsPatch := &istio.EnvoyFilter{} - if err := r.Client().Get(ctx, ratelimitsPatchKey, ratelimitsPatch); err != nil { - logger.Error(err, "failed to fetch ratelimits patch") - return err - } + logger.Info("successfully removed parentRef entry on the filters patch") - if err := removeOwnerRlpEntry(ctx, r.Client(), ratelimitsPatch, ownerRlp); err != nil { - logger.Error(err, "failed to remove ownerRLP tag on ratelimits patch") + ratelimitsPatchKey := client.ObjectKey{ + Namespace: gwKey.Namespace, + Name: ratelimitsPatchName(gwKey.Name, vsKey), + } + ratelimitsPatch := &istio.EnvoyFilter{} + if err := r.Client().Get(ctx, ratelimitsPatchKey, ratelimitsPatch); err != nil { + if apierrors.IsNotFound(err) { + logger.Error(err, "failed to get ratelimits patch") return err } - logger.Info("successfully removed ownerRLP tag on ratelimits patch") + logger.V(1).Info("ratelimits patch not found", "object key", ratelimitsPatchKey.String()) + } + if err := r.removeParentRefEntry(ctx, ratelimitsPatch, ownerRlp); err != nil { + logger.Error(err, "failed to remove remove parentRef entry on ratelimits patch") + return err } - default: - return fmt.Errorf(InvalidNetworkingRefTypeErr) + logger.Info("successfully removed parentRef tag on ratelimits patch") } } return nil } -func removeOwnerRlpEntry(ctx context.Context, client client.Client, patch *istio.EnvoyFilter, owner string) error { +func (r *RateLimitPolicyReconciler) removeParentRefEntry(ctx context.Context, patch *istio.EnvoyFilter, owner string) error { logger := logr.FromContext(ctx) - logger.Info("removing ownerRLP entry from EnvoyFilter", "EnvoyFilter", patch.Name) + logger.Info("Removing parentRef entry from EnvoyFilter", "EnvoyFilter", patch.Name) // find the annotation - ownerRlpsVal, present := patch.Annotations[envoyFilterAnnotationOwnerRLPs] + ownerRlpsVal, present := patch.Annotations[EnvoyFilterParentRefsAnnotation] if !present { - logger.V(1).Info("Deleting the patch since ownerRLP annotation was not present to avoid orphans") + logger.V(1).Info("Deleting the patch since parentRef annotation was not present to avoid orphans") // if it's not deleted then the patch will remain as an orphan once all the rlps are removed. - if err := client.Delete(ctx, patch); err != nil { + if err := r.Client().Delete(ctx, patch); err != nil { logger.Error(err, "failed to delete the patch") return err } @@ -101,7 +99,7 @@ func removeOwnerRlpEntry(ctx context.Context, client client.Client, patch *istio } // split into array of RateLimitPolicy names - ownerRlps := strings.Split(ownerRlpsVal, ownerRlpSeparator) + ownerRlps := strings.Split(ownerRlpsVal, ParentRefsSeparator) // remove the target owner finalOwnerRlps := []string{} @@ -113,18 +111,48 @@ func removeOwnerRlpEntry(ctx context.Context, client client.Client, patch *istio } if len(finalOwnerRlps) == 0 { - logger.V(1).Info("Deleting filters patch because 0 ownerRLP entries found on it") - if err := client.Delete(ctx, patch); err != nil { + logger.V(1).Info("Deleting filters patch because 0 parentRef entries found on it") + if err := r.Client().Delete(ctx, patch); err != nil { logger.Error(err, "failed to delete the patch") return err } } else { - finalOwnerRlpsVal := strings.Join(finalOwnerRlps, ownerRlpSeparator) - patch.Annotations[envoyFilterAnnotationOwnerRLPs] = finalOwnerRlpsVal - if err := client.Update(ctx, patch); err != nil { + finalOwnerRlpsVal := strings.Join(finalOwnerRlps, ParentRefsSeparator) + patch.Annotations[EnvoyFilterParentRefsAnnotation] = finalOwnerRlpsVal + if err := r.Client().Update(ctx, patch); err != nil { logger.Error(err, "failed to update the patch") return err } } return nil } + +func (r *RateLimitPolicyReconciler) addParentRefEntry(ctx context.Context, patch *istio.EnvoyFilter, owner string) error { + logger := logr.FromContext(ctx) + logger.Info("Adding parentRef entry to EnvoyFilter", "EnvoyFilter", patch.Name) + + // make sure annotation map is initialized + patchOwnerList := []string{} + if patch.Annotations == nil { + patch.Annotations = make(map[string]string) + } + + if ogOwnerRlps, present := patch.Annotations[EnvoyFilterParentRefsAnnotation]; present { + patchOwnerList = strings.Split(ogOwnerRlps, ParentRefsSeparator) + } + + // add the owner name if not present already + if !common.Contains(patchOwnerList, owner) { + patchOwnerList = append(patchOwnerList, owner) + } + finalOwnerVal := strings.Join(patchOwnerList, ParentRefsSeparator) + + patch.Annotations[EnvoyFilterParentRefsAnnotation] = finalOwnerVal + if err := r.ReconcileResource(ctx, &istio.EnvoyFilter{}, patch, alwaysUpdateEnvoyPatches); err != nil { + logger.Error(err, "failed to create/update EnvoyFilter that patches-in ratelimit filters") + return err + } + + logger.Info("Successfully added parentRef entry to the EnvoyFilter") + return nil +} diff --git a/controllers/apim/ratelimitpolicy_mapper.go b/controllers/apim/ratelimitpolicy_mapper.go new file mode 100644 index 000000000..c415c5bc8 --- /dev/null +++ b/controllers/apim/ratelimitpolicy_mapper.go @@ -0,0 +1,159 @@ +package apim + +import ( + "context" + + "github.com/go-logr/logr" + apimv1alpha1 "github.com/kuadrant/kuadrant-controller/apis/apim/v1alpha1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/predicate" +) + +const ( + KuadrantAddVSAnnotation = "kuadrant.io/add-virtualservice" + KuadrantDeleteVSAnnotation = "kuadrant.io/delete-virtualservice" + KuadrantAddHRAnnotation = "kuadrant.io/add-httproute" + KuadrantDeleteHRAnnotation = "kuadrant.io/delete-httproute" +) + +// routingPredicate is used by routing objects' controllers to filter for +// Kuadrant annotations signaling API protection. +func routingPredicate(m *rateLimitPolicyMapper) predicate.Predicate { + return predicate.Funcs{ + CreateFunc: func(e event.CreateEvent) bool { + if _, toRateLimit := e.Object.GetAnnotations()[KuadrantRateLimitPolicyAnnotation]; toRateLimit { + if err := m.SignalCreate(e.Object); err != nil { + m.Logger.Error(err, "failed to signal create event to referenced RateLimitPolicy") + // lets still try for auth annotation + } + } + + // only create reconcile request for routing objects' controllers when auth + // annotation is present. + _, toProtect := e.Object.GetAnnotations()[KuadrantAuthProviderAnnotation] + return toProtect + }, + UpdateFunc: func(e event.UpdateEvent) bool { + _, toRateLimitOld := e.ObjectOld.GetAnnotations()[KuadrantRateLimitPolicyAnnotation] + _, toRateLimitNew := e.ObjectNew.GetAnnotations()[KuadrantRateLimitPolicyAnnotation] + if toRateLimitNew || toRateLimitOld { + if err := m.SignalUpdate(e.ObjectOld, e.ObjectNew); err != nil { + m.Logger.Error(err, "failed to signal update event to referenced RateLimitPolicy") + } + } + + _, toProtectOld := e.ObjectOld.GetAnnotations()[KuadrantAuthProviderAnnotation] + _, toProtectNew := e.ObjectNew.GetAnnotations()[KuadrantAuthProviderAnnotation] + return toProtectOld || toProtectNew + }, + DeleteFunc: func(e event.DeleteEvent) bool { + // If the object had the Kuadrant label, we need to handle its deletion + _, toRateLimit := e.Object.GetAnnotations()[KuadrantRateLimitPolicyAnnotation] + if toRateLimit { + if err := m.SignalDelete(e.Object); err != nil { + m.Logger.Error(err, "failed to signal delete event to referenced RateLimitPolicy") + } + } + + _, toProtect := e.Object.GetAnnotations()[KuadrantAuthProviderAnnotation] + return toProtect + }, + } +} + +// rateLimitPolicyMapper helps signal the change in the routing objects (VirtualService and HTTPRoute) +// to the referenced RateLimitPolicy in the annotation +type rateLimitPolicyMapper struct { + K8sClient client.Client + Logger logr.Logger +} + +func (m *rateLimitPolicyMapper) SignalCreate(obj client.Object) error { + addAnnotation := KuadrantAddVSAnnotation + if obj.GetObjectKind().GroupVersionKind().Kind == "HTTPRoute" { + addAnnotation = KuadrantAddHRAnnotation + } + rlpName := obj.GetAnnotations()[KuadrantRateLimitPolicyAnnotation] + m.Logger.Info("Signaling create event to RateLimitPolicy", "RateLimitPolicy", rlpName) + rlpKey := types.NamespacedName{ + Name: rlpName, + Namespace: obj.GetNamespace(), + } + + // get the referenced rlp + rlp := &apimv1alpha1.RateLimitPolicy{} + if err := m.K8sClient.Get(context.Background(), rlpKey, rlp); err != nil { + return err + } + + // signal addition by adding 'add' annotation + rlp.Annotations[addAnnotation] = obj.GetName() + err := m.K8sClient.Update(context.Background(), rlp) + return err +} + +func (m *rateLimitPolicyMapper) SignalDelete(obj client.Object) error { + deleteAnnotation := KuadrantDeleteVSAnnotation + if obj.GetObjectKind().GroupVersionKind().Kind == "HTTPRoute" { + deleteAnnotation = KuadrantAddHRAnnotation + } + rlpName := obj.GetAnnotations()[KuadrantRateLimitPolicyAnnotation] + m.Logger.Info("Signaling delete event to RateLimitPolicy", "RateLimitPolicy", rlpName) + rlpKey := types.NamespacedName{ + Name: rlpName, + Namespace: obj.GetNamespace(), + } + + // get the referenced rlp + rlp := &apimv1alpha1.RateLimitPolicy{} + if err := m.K8sClient.Get(context.Background(), rlpKey, rlp); err != nil { + return err + } + + // signal deletion by adding 'delete' annotation + rlp.Annotations[deleteAnnotation] = obj.GetName() + err := m.K8sClient.Update(context.Background(), rlp) + return err +} + +// SignalUpdate is used when either old or new object had/has the ratelimit annotaiton +func (m *rateLimitPolicyMapper) SignalUpdate(oldObj, newObj client.Object) error { + m.Logger.Info("Signaling update event to RateLimitPolicy") + oldRlpName, toRateLimitOld := oldObj.GetAnnotations()[KuadrantRateLimitPolicyAnnotation] + newRlpName, toRateLimitNew := newObj.GetAnnotations()[KuadrantRateLimitPolicyAnnotation] + + // case when rlp name is added (same as create event) + if !toRateLimitOld && toRateLimitNew { + if err := m.SignalCreate(newObj); err != nil { + return err + } + } + + // case when rlp name is removed (same as delete event) + if toRateLimitOld && !toRateLimitNew { + if err := m.SignalDelete(oldObj); err != nil { + return err + } + } + + // case when rlp name is changed (same as delete for old and create event for new) + if toRateLimitNew && toRateLimitOld && oldRlpName != newRlpName { + // signal deletion to old RLP + if err := m.SignalDelete(oldObj); err != nil { + if !apierrors.IsNotFound(err) { + return err + } + } + + // signal addition to new RLP + if err := m.SignalCreate(newObj); err != nil { + return err + } + } + + // case when there is no change in the annotation, no action is required + return nil +} diff --git a/controllers/apim/utils.go b/controllers/apim/utils.go index 0e65a8f17..f3c6ec514 100644 --- a/controllers/apim/utils.go +++ b/controllers/apim/utils.go @@ -26,8 +26,9 @@ func rlFiltersPatchName(gatewayName string) string { } // ratelimitsPatchName returns the name of EnvoyFilter adding in ratelimits to a gateway. -func ratelimitsPatchName(rlpName, gatewayName string) string { - return fmt.Sprintf("ratelimits-of-%s-in-%s", rlpName, gatewayName) +func ratelimitsPatchName(gwName string, networkKey client.ObjectKey) string { + // TODO(rahulanand16nov): make it unique if VS and HR have same name. + return fmt.Sprintf("ratelimits-on-%s-using-%s-%s", gwName, networkKey.Namespace, networkKey.Name) } // getAuthPolicyName generates the name of an AuthorizationPolicy using VirtualService info. diff --git a/controllers/apim/virtualservice_controller.go b/controllers/apim/virtualservice_controller.go index d55c3ec04..fb1c3a9c3 100644 --- a/controllers/apim/virtualservice_controller.go +++ b/controllers/apim/virtualservice_controller.go @@ -11,9 +11,6 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/predicate" networkingv1alpha3 "istio.io/api/networking/v1alpha3" securityv1beta1 "istio.io/api/security/v1beta1" @@ -27,7 +24,8 @@ import ( ) const ( - KuadrantAuthProviderAnnotation = "kuadrant.io/auth-provider" + KuadrantAuthProviderAnnotation = "kuadrant.io/auth-provider" + KuadrantRateLimitPolicyAnnotation = "kuadrant.io/ratelimitpolicy" ) //+kubebuilder:rbac:groups=networking.istio.io,resources=virtualservices,verbs=get;list;watch;update;patch @@ -86,7 +84,6 @@ func (r *VirtualServiceReconciler) Reconcile(eventCtx context.Context, req ctrl. return ctrl.Result{}, err } logger.Info("successfully reconciled AuthorizationPolicy") - return ctrl.Result{}, nil } @@ -144,15 +141,11 @@ func (r *VirtualServiceReconciler) reconcileAuthPolicy(ctx context.Context, logg authPolicy := istiosecurityv1beta1.AuthorizationPolicy{ ObjectMeta: metav1.ObjectMeta{ Name: getAuthPolicyName(gwKey.Name, vs.Name), - Namespace: vs.Namespace, + Namespace: gwKey.Namespace, }, Spec: authPolicySpec, } - if err := controllerutil.SetOwnerReference(vs, &authPolicy, r.Client().Scheme()); err != nil { - logger.Error(err, "failed to add owner ref to AuthorizationPolicy resource") - return err - } err := r.ReconcileResource(ctx, &istiosecurityv1beta1.AuthorizationPolicy{}, &authPolicy, alwaysUpdateAuthPolicy) if err != nil && !apierrors.IsAlreadyExists(err) { logger.Error(err, "ReconcileResource failed to create/update AuthorizationPolicy resource") @@ -177,30 +170,13 @@ func normalizeStringMatch(sm *networkingv1alpha3.StringMatch) string { // SetupWithManager sets up the controller with the Manager. func (r *VirtualServiceReconciler) SetupWithManager(mgr ctrl.Manager) error { + rlpMapper := &rateLimitPolicyMapper{ + K8sClient: r.Client(), + Logger: r.Logger(), + } return ctrl.NewControllerManagedBy(mgr). - For(&istionetworkingv1alpha3.VirtualService{}, builder.WithPredicates(virtualServicePredicate())). + For(&istionetworkingv1alpha3.VirtualService{}, builder.WithPredicates(routingPredicate(rlpMapper))). Owns(&istiosecurityv1beta1.AuthorizationPolicy{}). WithLogger(log.Log). // use base logger, the manager will add prefixes for watched sources Complete(r) } - -func virtualServicePredicate() predicate.Predicate { - return predicate.Funcs{ - CreateFunc: func(e event.CreateEvent) bool { - _, present := e.Object.GetAnnotations()[KuadrantAuthProviderAnnotation] - return present - }, - UpdateFunc: func(e event.UpdateEvent) bool { - if _, present := e.ObjectOld.GetAnnotations()[KuadrantAuthProviderAnnotation]; present { - return true - } - _, present := e.ObjectNew.GetAnnotations()[KuadrantAuthProviderAnnotation] - return present - }, - DeleteFunc: func(e event.DeleteEvent) bool { - // If the object had the Kuadrant label, we need to handle its deletion - _, present := e.Object.GetAnnotations()[KuadrantAuthProviderAnnotation] - return present - }, - } -} diff --git a/examples/toystore/ratelimitpolicy.yaml b/examples/toystore/ratelimitpolicy.yaml index 541a859f4..d660f0d6c 100644 --- a/examples/toystore/ratelimitpolicy.yaml +++ b/examples/toystore/ratelimitpolicy.yaml @@ -4,9 +4,6 @@ kind: RateLimitPolicy metadata: name: toystore spec: - networkingRef: - - type: VirtualService - name: toystore routes: - name: get-toy rateLimits: @@ -38,10 +35,14 @@ spec: - generic_key: descriptor_key: vhaction descriptor_value: "yes" - limits: - conditions: ["get-toy == yes"] max_value: 2 namespace: preauth seconds: 15 variables: [] + - condtitions: ["vhaction == yes"] + max_value: 4 + namespace: preauth + seconds: 15 + variables: [] diff --git a/pkg/common/istio_utils.go b/pkg/common/istio_utils.go index 36e876037..c28323bbc 100644 --- a/pkg/common/istio_utils.go +++ b/pkg/common/istio_utils.go @@ -27,10 +27,6 @@ type EnvoyFilterFactory struct { } func (v *EnvoyFilterFactory) EnvoyFilter() *istionetworkingv1alpha3.EnvoyFilter { - if len(v.Labels) == 0 { - // default to kuadrant labels to avoid replication where it's already used. - v.Labels = map[string]string{"istio": "kuadrant-system"} - } return &istionetworkingv1alpha3.EnvoyFilter{ TypeMeta: metav1.TypeMeta{ Kind: "EnvoyFilter",