refactor controller

Author: bruelea

api/v1/ipaddressclaim_types.go

@@ -117,3 +117,10 @@ var ConditionIpAssignedFalse = metav1.Condition{
 	Reason:  "IPAddressCRNotCreated",
 	Message: "Failed to fetch new IP from NetBox",
 }
+
+var ConditionIpAssignedFalseSizeMissmatch = metav1.Condition{
+	Type:    "IPAssigned",
+	Status:  "False",
+	Reason:  "IPAddressCRNotCreated",
+	Message: "Size of restored IpRange does not match the requested size",
+}

api/v1/iprange_types.go

@@ -65,7 +65,7 @@ type IpRangeStatus struct {
 //+kubebuilder:printcolumn:name="ID",type=string,JSONPath=`.status.id`
 //+kubebuilder:printcolumn:name="URL",type=string,JSONPath=`.status.url`
 //+kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp`
-// +kubebuilder:resource:shortName=ipr
+// +kubebuilder:resource:shortName=ir
 
 // IpRange is the Schema for the ipranges API
 type IpRange struct {

api/v1/iprangeclaim_types.go

@@ -63,7 +63,7 @@ type IpRangeClaimStatus struct {
 //+kubebuilder:printcolumn:name="IpRangeAssigned",type=string,JSONPath=`.status.conditions[?(@.type=="IPAssigned")].status`
 //+kubebuilder:printcolumn:name="Ready",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].status`
 //+kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp`
-// +kubebuilder:resource:shortName=iprc
+// +kubebuilder:resource:shortName=irc
 
 // IpRangeClaim is the Schema for the iprangeclaims API
 type IpRangeClaim struct {

internal/controller/iprange_controller.go

@@ -19,12 +19,10 @@ package controller
 import (
 	"context"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"maps"
 	"strconv"
 	"strings"
-	"time"
 
 	netboxv1 "github.com/netbox-community/netbox-operator/api/v1"
 	"github.com/netbox-community/netbox-operator/pkg/config"
@@ -77,33 +75,13 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 
 	// if being deleted
 	if !o.ObjectMeta.DeletionTimestamp.IsZero() {
-		if controllerutil.ContainsFinalizer(o, IpRangeFinalizerName) {
-			if !o.Spec.PreserveInNetbox {
-				err := r.NetboxClient.DeleteIpRange(o.Status.IpRangeId)
-				if err != nil {
-					setConditionErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalseDeletionFailed, corev1.EventTypeWarning, err.Error())
-					if setConditionErr != nil {
-						return ctrl.Result{}, fmt.Errorf("error updating status: %w, when deletion of IpRange failed: %w", setConditionErr, err)
-					}
-
-					return ctrl.Result{Requeue: true}, nil
-				}
-			}
-
-			debugLogger.Info("removing the finalizer")
-			removed := controllerutil.RemoveFinalizer(o, IpRangeFinalizerName)
-			if !removed {
-				return ctrl.Result{}, errors.New("failed to remove the finalizer")
-			}
-
-			err = r.Update(ctx, o)
-			if err != nil {
-				return ctrl.Result{}, err
-			}
+		if o.Spec.PreserveInNetbox {
+			// if there's a finalizer remove it and return
+			// this can be the case if a CR used to have spec.preserveInNetbox set to false
+			return r.removeFinalizer(ctx, o)
 		}
 
-		// end loop if deletion timestamp is not zero
-		return ctrl.Result{}, nil
+		return r.deleteFromNetboxAndRemoveFinalizer(ctx, o)
 	}
 
 	// if PreserveIpInNetbox flag is false then register finalizer if not yet registered
@@ -118,43 +96,16 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 	// 1. try to lock lease of parent prefix if IpRange status condition is not true
 	// and IpRange is owned by an IpRangeClaim
 	or := o.ObjectMeta.OwnerReferences
-	var ll *leaselocker.LeaseLocker
 	if len(or) > 0 /* len(nil array) = 0 */ && !apismeta.IsStatusConditionTrue(o.Status.Conditions, "Ready") {
-		// get ip range claim
-		orLookupKey := types.NamespacedName{
-			Name:      or[0].Name,
-			Namespace: req.Namespace,
-		}
-		ipRangeClaim := &netboxv1.IpRangeClaim{}
-		err = r.Client.Get(ctx, orLookupKey, ipRangeClaim)
-		if err != nil {
-			return ctrl.Result{}, err
-		}
-
-		// get name of parent prefix
-		leaseLockerNSN := types.NamespacedName{
-			Name:      convertCIDRToLeaseLockName(ipRangeClaim.Spec.ParentPrefix),
-			Namespace: r.OperatorNamespace,
-		}
-		ll, err = leaselocker.NewLeaseLocker(r.RestConfig, leaseLockerNSN, req.NamespacedName.String())
+		ll, res, err := r.tryLockOnParentPrefix(ctx, o)
+		defer func() {
+			if ll != nil {
+				ll.Unlock()
+			}
+		}()
 		if err != nil {
-			return ctrl.Result{}, err
-		}
-
-		lockCtx, cancel := context.WithCancel(ctx)
-		defer cancel()
-
-		// create lock
-		locked := ll.TryLock(lockCtx)
-		if !locked {
-			logger.Info(fmt.Sprintf("failed to lock parent prefix %s", ipRangeClaim.Spec.ParentPrefix))
-			r.Recorder.Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", "failed to lock parent prefix %s",
-				ipRangeClaim.Spec.ParentPrefix)
-			return ctrl.Result{
-				RequeueAfter: 2 * time.Second,
-			}, nil
+			return res, err
 		}
-		debugLogger.Info(fmt.Sprintf("successfully locked parent prefix %s", ipRangeClaim.Spec.ParentPrefix))
 	}
 
 	// 2. reserve or update ip range in netbox
@@ -164,67 +115,34 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 		return ctrl.Result{}, err
 	}
 
-	ipRangeModel, err := generateNetboxIpRangeModelFromIpRangeSpec(&o.Spec, req, annotations[LastIpRangeMetadataAnnotationName])
+	ipRangeModel, err := r.generateNetboxIpRangeModelFromIpRangeSpec(ctx, o, req, annotations[LastIpRangeMetadataAnnotationName])
 	if err != nil {
 		return ctrl.Result{}, err
 	}
 
 	netboxIpRangeModel, err := r.NetboxClient.ReserveOrUpdateIpRange(ipRangeModel)
 	if err != nil {
-		updateStatusErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalse,
-			corev1.EventTypeWarning, fmt.Sprintf("%s-%s", o.Spec.StartAddress, o.Spec.EndAddress))
-		return ctrl.Result{}, fmt.Errorf("failed to update ip range status: %w, "+
-			"after reservation of ip in netbox failed: %w", updateStatusErr, err)
-	}
-
-	// 3. unlock lease of parent prefix
-	if ll != nil {
-		ll.Unlock()
+		err := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalse,
+			corev1.EventTypeWarning, fmt.Sprintf("%s-%s", o.Spec.StartAddress, o.Spec.EndAddress), err)
+		if err != nil {
+			return ctrl.Result{}, nil
+		}
 	}
 
 	// 4. update status fields
 	o.Status.IpRangeId = netboxIpRangeModel.ID
-	o.Status.IpRangeUrl = config.GetBaseUrl() + "/ipam/ip-rages/" + strconv.FormatInt(netboxIpRangeModel.ID, 10)
+	o.Status.IpRangeUrl = config.GetBaseUrl() + "/ipam/ip-ranges/" + strconv.FormatInt(netboxIpRangeModel.ID, 10)
 	err = r.Client.Status().Update(ctx, o)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
 
-	// update lastIpRangeMetadata annotation
-	if annotations == nil {
-		annotations = make(map[string]string)
-	}
-
-	if len(o.Spec.CustomFields) > 0 {
-		lastIpRangeMetadata, err := json.Marshal(o.Spec.CustomFields)
-		if err != nil {
-			return ctrl.Result{}, fmt.Errorf("failed to marshal lastIpRangeMetadata annotation: %w", err)
-		}
-
-		annotations[LastIpRangeMetadataAnnotationName] = string(lastIpRangeMetadata)
-	} else {
-		annotations[LastIpRangeMetadataAnnotationName] = "{}"
-	}
-
-	err = accessor.SetAnnotations(o, annotations)
+	err = r.updateLastMetadataAnnotation(ctx, annotations, o, accessor)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
 
-	// update object to store lastIpRangeMetadata annotation
-	if err := r.Update(ctx, o); err != nil {
-		return ctrl.Result{}, err
-	}
-
-	// check if created ip range contains entire description from spec
-	_, found := strings.CutPrefix(netboxIpRangeModel.Description, req.NamespacedName.String()+" // "+o.Spec.Description)
-	if !found {
-		r.Recorder.Event(o, corev1.EventTypeWarning, "IpRangeDescriptionTruncated", "ip range was created with truncated description")
-	}
-
-	debugLogger.Info(fmt.Sprintf("reserved ip range in netbox, start address: %s, end address: %s", o.Spec.StartAddress, o.Spec.EndAddress))
-
-	err = r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyTrue, corev1.EventTypeNormal, "")
+	err = r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyTrue, corev1.EventTypeNormal, "", nil)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
@@ -241,22 +159,33 @@ func (r *IpRangeReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Complete(r)
 }
 
-func (r *IpRangeReconciler) SetConditionAndCreateEvent(ctx context.Context, o *netboxv1.IpRange, condition metav1.Condition, eventType string, conditionMessageAppend string) error {
+func (r *IpRangeReconciler) SetConditionAndCreateEvent(ctx context.Context, o *netboxv1.IpRange, condition metav1.Condition, eventType string, conditionMessageAppend string, errIn error) error {
 	if len(conditionMessageAppend) > 0 {
 		condition.Message = condition.Message + ". " + conditionMessageAppend
 	}
+
+	if errIn != nil {
+		// log errors here, so we do not need to aggregate them in the reconcile loop with the error of updating the status
+		logger := log.FromContext(ctx)
+		logger.Error(errIn, condition.Message+errIn.Error())
+		condition.Message = condition.Message + ". Check the logs for more information."
+	}
+
 	conditionChanged := apismeta.SetStatusCondition(&o.Status.Conditions, condition)
 	if conditionChanged {
 		r.Recorder.Event(o, eventType, condition.Reason, condition.Message)
 	}
+
 	err := r.Client.Status().Update(ctx, o)
 	if err != nil {
 		return err
 	}
+
 	return nil
 }
 
-func generateNetboxIpRangeModelFromIpRangeSpec(spec *netboxv1.IpRangeSpec, req ctrl.Request, lastIpRangeMetadata string) (*models.IpRange, error) {
+func (r *IpRangeReconciler) generateNetboxIpRangeModelFromIpRangeSpec(ctx context.Context, o *netboxv1.IpRange, req ctrl.Request, lastIpRangeMetadata string) (*models.IpRange, error) {
+	debugLogger := log.FromContext(context.Background()).V(4)
 	// unmarshal lastIpRangeMetadata json string to map[string]string
 	lastAppliedCustomFields := make(map[string]string)
 	if lastIpRangeMetadata != "" {
@@ -266,8 +195,8 @@ func generateNetboxIpRangeModelFromIpRangeSpec(spec *netboxv1.IpRangeSpec, req c
 	}
 
 	netboxCustomFields := make(map[string]string)
-	if len(spec.CustomFields) > 0 {
-		netboxCustomFields = maps.Clone(spec.CustomFields)
+	if len(o.Spec.CustomFields) > 0 {
+		netboxCustomFields = maps.Clone(o.Spec.CustomFields)
 	}
 
 	// if a custom field was removed from the spec, add it with an empty value
@@ -279,14 +208,134 @@ func generateNetboxIpRangeModelFromIpRangeSpec(spec *netboxv1.IpRangeSpec, req c
 		}
 	}
 
+	description := api.TruncateDescription(req.NamespacedName.String() + " // " + o.Spec.Description)
+
+	// check if created ip range contains entire description from spec
+	_, found := strings.CutPrefix(description, req.NamespacedName.String()+" // "+o.Spec.Description)
+	if !found {
+		r.Recorder.Event(o, corev1.EventTypeWarning, "IpRangeDescriptionTruncated", "ip range was created with truncated description")
+	}
+
+	debugLogger.Info(fmt.Sprintf("reserved ip range in netbox, start address: %s, end address: %s", o.Spec.StartAddress, o.Spec.EndAddress))
+
 	return &models.IpRange{
-		StartAddress: spec.StartAddress,
-		EndAddress:   spec.EndAddress,
+		StartAddress: o.Spec.StartAddress,
+		EndAddress:   o.Spec.EndAddress,
 		Metadata: &models.NetboxMetadata{
-			Comments:    spec.Comments,
+			Comments:    o.Spec.Comments,
 			Custom:      netboxCustomFields,
-			Description: req.NamespacedName.String() + " // " + spec.Description,
-			Tenant:      spec.Tenant,
+			Description: description,
+			Tenant:      o.Spec.Tenant,
 		},
 	}, nil
 }
+
+func (r *IpRangeReconciler) deleteFromNetboxAndRemoveFinalizer(ctx context.Context, o *netboxv1.IpRange) (ctrl.Result, error) {
+	err := r.NetboxClient.DeleteIpRange(o.Status.IpRangeId)
+	if err != nil {
+		err = r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalseDeletionFailed,
+			corev1.EventTypeWarning, "", err)
+		if err != nil {
+			return ctrl.Result{}, err
+		}
+
+		return ctrl.Result{Requeue: true}, nil
+	}
+
+	res, err := r.removeFinalizer(ctx, o)
+	if err != nil {
+		return res, err
+	}
+
+	err = r.Update(ctx, o)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	return ctrl.Result{}, nil
+}
+
+func (r *IpRangeReconciler) tryLockOnParentPrefix(ctx context.Context, o *netboxv1.IpRange) (*leaselocker.LeaseLocker, ctrl.Result, error) {
+	// determine NamespacedName of IpRangeClaim owning the IpRange CR
+	orLookupKey := types.NamespacedName{
+		Name:      o.ObjectMeta.OwnerReferences[0].Name,
+		Namespace: o.Namespace,
+	}
+
+	logger := log.FromContext(ctx)
+	debugLogger := log.FromContext(ctx).V(4)
+
+	ipRangeClaim := &netboxv1.IpRangeClaim{}
+	err := r.Client.Get(ctx, orLookupKey, ipRangeClaim)
+	if err != nil {
+		return nil, ctrl.Result{}, err
+	}
+
+	// get name of parent prefix
+	leaseLockerNSN := types.NamespacedName{
+		Name:      convertCIDRToLeaseLockName(ipRangeClaim.Spec.ParentPrefix),
+		Namespace: r.OperatorNamespace,
+	}
+	ll, err := leaselocker.NewLeaseLocker(r.RestConfig, leaseLockerNSN, orLookupKey.String())
+	if err != nil {
+		return nil, ctrl.Result{}, err
+	}
+
+	lockCtx, cancel := context.WithCancel(ctx)
+	defer cancel()
+
+	// create lock
+	locked := ll.TryLock(lockCtx)
+	if !locked {
+		logger.Info(fmt.Sprintf("failed to lock parent prefix %s", ipRangeClaim.Spec.ParentPrefix))
+		r.Recorder.Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", "failed to lock parent prefix %s",
+			ipRangeClaim.Spec.ParentPrefix)
+		return ll, ctrl.Result{}, nil
+	}
+	debugLogger.Info(fmt.Sprintf("successfully locked parent prefix %s", ipRangeClaim.Spec.ParentPrefix))
+
+	return ll, ctrl.Result{}, nil
+}
+
+func (r *IpRangeReconciler) updateLastMetadataAnnotation(ctx context.Context, annotations map[string]string, o *netboxv1.IpRange, accessor apismeta.MetadataAccessor) error {
+	// update lastIpRangeMetadata annotation
+	if annotations == nil {
+		annotations = make(map[string]string)
+	}
+
+	if len(o.Spec.CustomFields) > 0 {
+		lastIpRangeMetadata, err := json.Marshal(o.Spec.CustomFields)
+		if err != nil {
+			return fmt.Errorf("failed to marshal lastIpRangeMetadata annotation: %w", err)
+		}
+
+		annotations[LastIpRangeMetadataAnnotationName] = string(lastIpRangeMetadata)
+	} else {
+		annotations[LastIpRangeMetadataAnnotationName] = "{}"
+	}
+
+	err := accessor.SetAnnotations(o, annotations)
+	if err != nil {
+		return err
+	}
+
+	// update object to store lastIpRangeMetadata annotation
+	if err := r.Update(ctx, o); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func (r *IpRangeReconciler) removeFinalizer(ctx context.Context, o *netboxv1.IpRange) (ctrl.Result, error) {
+	debugLogger := log.FromContext(ctx).V(4)
+	if controllerutil.ContainsFinalizer(o, IpRangeFinalizerName) {
+		debugLogger.Info("removing the finalizer")
+		controllerutil.RemoveFinalizer(o, IpRangeFinalizerName)
+		if err := r.Update(ctx, o); err != nil {
+			return ctrl.Result{}, err
+		}
+	}
+
+	return ctrl.Result{}, nil
+}