From e980fde4c307b911566a9f6ce854b61f1365603a Mon Sep 17 00:00:00 2001 From: Francesco Torta <62566275+fra98@users.noreply.github.com> Date: Mon, 18 Nov 2024 14:58:49 +0100 Subject: [PATCH] fixup! fixup! fixup! refactor: updated IP controller for new ipam --- .../networking/ip-controller/ip_controller.go | 132 +++++++++++------- 1 file changed, 80 insertions(+), 52 deletions(-) diff --git a/pkg/liqo-controller-manager/networking/ip-controller/ip_controller.go b/pkg/liqo-controller-manager/networking/ip-controller/ip_controller.go index 0139b8aa46..bb80b4f410 100644 --- a/pkg/liqo-controller-manager/networking/ip-controller/ip_controller.go +++ b/pkg/liqo-controller-manager/networking/ip-controller/ip_controller.go @@ -83,76 +83,98 @@ func (r *IPReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Re return ctrl.Result{}, err } + // Get the CIDR of the Network referenced by the IP, or default to the + // external CIDR of the local cluster. networkRef, cidr, err := r.handleNetworkRef(ctx, &ip) - if err != nil { - klog.Errorf("error while handling NetworkRef for IP %q: %v", req.NamespacedName, err) + if client.IgnoreNotFound(err) != nil { + klog.Errorf("error while handling network reference for IP %q: %v", req.NamespacedName, err) return ctrl.Result{}, err } - if ip.GetDeletionTimestamp().IsZero() { - if !controllerutil.ContainsFinalizer(&ip, ipamIPFinalizer) { - // Add finalizer to prevent deletion without releasing the IP. - controllerutil.AddFinalizer(&ip, ipamIPFinalizer) + networkExists := !apierrors.IsNotFound(err) + deleting := !ip.GetDeletionTimestamp().IsZero() + + // The resource is being deleted or the referenced network does not exist: + // - call the IPAM to release the IP if it set, and empty the status. + // - remove eventual finalizers from the resource. + if deleting || !networkExists { + if err := r.handleIPStatusDeletion(ctx, &ip); err != nil { + klog.Errorf("error while handling IP status deletion %q: %v", req.NamespacedName, err) + return ctrl.Result{}, err + } + + // Remove finalizer from the IP resource. + if controllerutil.ContainsFinalizer(&ip, ipamIPFinalizer) { + controllerutil.RemoveFinalizer(&ip, ipamIPFinalizer) // Update the IP object if err := r.Update(ctx, &ip); err != nil { - klog.Errorf("error while adding finalizer to IP %q: %v", req.NamespacedName, err) + klog.Errorf("error while removing finalizer from IP %q: %v", req.NamespacedName, err) return ctrl.Result{}, err } - klog.Infof("finalizer %q correctly added to IP %q", ipamIPFinalizer, req.NamespacedName) - // We return immediately and wait for the next reconcile to eventually update the status. - return ctrl.Result{}, nil + klog.Infof("finalizer %q correctly removed from IP %q", ipamIPFinalizer, req.NamespacedName) } - // Add network reference to the IP in the labels. This is used to trigger the reconciliation - // of the IP by watching deletion events of the father Network. - if ip.Labels == nil { - ip.Labels = make(map[string]string) - } - ip.Labels[consts.NetworkNamespaceLabelKey] = networkRef.Namespace - ip.Labels[consts.NetworkNameLabelKey] = networkRef.Name - if err := r.Update(ctx, &ip); err != nil { - klog.Errorf("error while updating IP %q: %v", req.NamespacedName, err) + // Raise an error if the IP is referencing a non-existing network and it is not being deleted. + if !deleting { + err := fmt.Errorf("network referenced by IP %q does not exist", req.NamespacedName) + klog.Error(err) return ctrl.Result{}, err } - if ip.Status.IP == "" { - if err := r.forgeIPStatus(ctx, &ip, cidr); err != nil { - klog.Errorf("error while forging IP status for IP %q: %v", req.NamespacedName, err) - return ctrl.Result{}, err - } + // We do not have to delete possible service and endpointslice associated, as already deleted by + // the Kubernetes garbage collector (since they are owned by the IP resource). - if err := r.Client.Status().Update(ctx, &ip); err != nil { - klog.Errorf("error while updating IP %q: %v", req.NamespacedName, err) - return ctrl.Result{}, err - } - klog.Infof("updated IP %q", req.NamespacedName) - } + return ctrl.Result{}, nil + } - // Create service and associated endpointslice if the template is defined - if err := r.handleAssociatedService(ctx, &ip); err != nil { - klog.Errorf("error while handling associated service for IP %q: %v", req.NamespacedName, err) + if !controllerutil.ContainsFinalizer(&ip, ipamIPFinalizer) { + // Add finalizer to prevent deletion without releasing the IP. + controllerutil.AddFinalizer(&ip, ipamIPFinalizer) + + // Update the IP object + if err := r.Update(ctx, &ip); err != nil { + klog.Errorf("error while adding finalizer to IP %q: %v", req.NamespacedName, err) return ctrl.Result{}, err } - } else if controllerutil.ContainsFinalizer(&ip, ipamIPFinalizer) { - // the resource is being deleted, but the finalizer is present: - // - release the IP from the IPAM. - // - remove finalizer from the resource. - if err := r.handleDelete(ctx, &ip); err != nil { - klog.Errorf("error while handling deletion of IP %q: %v", req.NamespacedName, err) + klog.Infof("finalizer %q correctly added to IP %q", ipamIPFinalizer, req.NamespacedName) + + // We return immediately and wait for the next reconcile to eventually update the status. + return ctrl.Result{}, nil + } + + // Add network reference to the IP in the labels. This is used to trigger the reconciliation + // of the IP by watching deletion events of the father Network. + if ip.Labels == nil { + ip.Labels = make(map[string]string) + } + ip.Labels[consts.NetworkNamespaceLabelKey] = networkRef.Namespace + ip.Labels[consts.NetworkNameLabelKey] = networkRef.Name + + if err := r.Update(ctx, &ip); err != nil { + klog.Errorf("error while updating IP %q: %v", req.NamespacedName, err) + return ctrl.Result{}, err + } + + // Forge the IP status if it is not set yet. + if ip.Status.IP == "" { + if err := r.forgeIPStatus(ctx, &ip, cidr); err != nil { + klog.Errorf("error while forging IP status for IP %q: %v", req.NamespacedName, err) return ctrl.Result{}, err } - // Update the IP object - if err := r.Update(ctx, &ip); err != nil { - klog.Errorf("error while removing finalizer from IP %q: %v", req.NamespacedName, err) + if err := r.Client.Status().Update(ctx, &ip); err != nil { + klog.Errorf("error while updating IP %q: %v", req.NamespacedName, err) return ctrl.Result{}, err } - klog.Infof("finalizer %q correctly removed from IP %q", ipamIPFinalizer, req.NamespacedName) + klog.Infof("updated IP %q", req.NamespacedName) + } - // We do not have to delete possible service and endpointslice associated, as already deleted by - // the Kubernetes garbage collector (since they are owned by the IP resource). + // Create service and associated endpointslice if the template is defined + if err := r.handleAssociatedService(ctx, &ip); err != nil { + klog.Errorf("error while handling associated service for IP %q: %v", req.NamespacedName, err) + return ctrl.Result{}, err } return ctrl.Result{}, nil @@ -244,18 +266,24 @@ func (r *IPReconciler) forgeIPStatus(ctx context.Context, ip *ipamv1alpha1.IP, c return nil } -// handleDelete handles the deletion of the IP resource. It call the IPAM to release the IP. -func (r *IPReconciler) handleDelete(ctx context.Context, ip *ipamv1alpha1.IP) error { +// handleIPStatusDeletion handles the deletion of the IP status. It call the IPAM to release the IP +// and empties the status. +func (r *IPReconciler) handleIPStatusDeletion(ctx context.Context, ip *ipamv1alpha1.IP) error { if ip.Status.IP != "" && ip.Status.CIDR != "" { if err := deleteIP(ctx, r.ipamClient, ip.Status.IP, ip.Status.CIDR); err != nil { return err } - } - // Remove status and finalizer, and update the object. - ip.Status.IP = "" - ip.Status.CIDR = "" - controllerutil.RemoveFinalizer(ip, ipamIPFinalizer) + // Remove status and finalizer, and update the object. + ip.Status = ipamv1alpha1.IPStatus{} + + // Update the IP status + if err := r.Client.Status().Update(ctx, ip); err != nil { + return err + } + + klog.Infof("IP %q status correctly cleaned", client.ObjectKeyFromObject(ip)) + } return nil }