improvements based on review

Author: bruelea

api/v1/iprange_types.go

@@ -67,7 +67,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=ir
+// +kubebuilder:resource:shortName=ipr
 
 // IpRange is the Schema for the ipranges API
 type IpRange struct {
@@ -94,20 +94,20 @@ func init() {
 var ConditionIpRangeReadyTrue = metav1.Condition{
 	Type:    "Ready",
 	Status:  "True",
-	Reason:  "IpRangeReservedInNetbox",
-	Message: "Ip Range was reserved/updated in NetBox",
+	Reason:  "IPRangeReservedInNetbox",
+	Message: "IP Range was reserved/updated in NetBox",
 }
 
 var ConditionIpRangeReadyFalse = metav1.Condition{
 	Type:    "Ready",
 	Status:  "False",
-	Reason:  "FailedToReserveIpRangeInNetbox",
-	Message: "Failed to reserve Ip Range in NetBox",
+	Reason:  "FailedToReserveIPRangeInNetbox",
+	Message: "Failed to reserve IP Range in NetBox",
 }
 
 var ConditionIpRangeReadyFalseDeletionFailed = metav1.Condition{
 	Type:    "Ready",
 	Status:  "False",
-	Reason:  "FailedToDeleteIpRangeInNetbox",
-	Message: "Failed to delete Ip Range in NetBox",
+	Reason:  "FailedToDeleteIPRangeInNetbox",
+	Message: "Failed to delete IP Range in NetBox",
 }

api/v1/iprangeclaim_types.go

@@ -74,10 +74,10 @@ type IpRangeClaimStatus struct {
 //+kubebuilder:subresource:status
 //+kubebuilder:storageversion
 //+kubebuilder:printcolumn:name="IpRange",type=string,JSONPath=`.status.ipRange`
-//+kubebuilder:printcolumn:name="IpRangeAssigned",type=string,JSONPath=`.status.conditions[?(@.type=="IpRangeAssigned")].status`
+//+kubebuilder:printcolumn:name="IpRangeAssigned",type=string,JSONPath=`.status.conditions[?(@.type=="IPRangeAssigned")].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=irc
+// +kubebuilder:resource:shortName=iprc
 
 // IpRangeClaim is the Schema for the iprangeclaims API
 type IpRangeClaim struct {
@@ -104,41 +104,41 @@ func init() {
 var ConditionIpRangeClaimReadyTrue = metav1.Condition{
 	Type:    "Ready",
 	Status:  "True",
-	Reason:  "IpRangeResourceReady",
-	Message: "Ip Range Resource is ready",
+	Reason:  "IPRangeResourceReady",
+	Message: "IP Range Resource is ready",
 }
 
 var ConditionIpRangeClaimReadyFalse = metav1.Condition{
 	Type:    "Ready",
 	Status:  "False",
-	Reason:  "IpRangeResourceNotReady",
-	Message: "Ip Range Resource is not ready",
+	Reason:  "IPRangeResourceNotReady",
+	Message: "IP Range Resource is not ready",
 }
 
 var ConditionIpRangeClaimReadyFalseStatusGen = metav1.Condition{
 	Type:    "Ready",
 	Status:  "False",
-	Reason:  "IpRangeClaimStatusGenerationFailed",
-	Message: "Failed to generate Ip Range Status",
+	Reason:  "IPRangeClaimStatusGenerationFailed",
+	Message: "Failed to generate IP Range Status",
 }
 
 var ConditionIpRangeAssignedTrue = metav1.Condition{
-	Type:    "IpRangeAssigned",
+	Type:    "IPRangeAssigned",
 	Status:  "True",
-	Reason:  "IpRangeCRCreated",
-	Message: "New Ip Range fetched from NetBox and IpRange CR was created",
+	Reason:  "IPRangeCRCreated",
+	Message: "New IP Range fetched from NetBox and IpRange CR was created",
 }
 
 var ConditionIpRangeAssignedFalse = metav1.Condition{
-	Type:    "IpRangeAssigned",
+	Type:    "IPRangeAssigned",
 	Status:  "False",
-	Reason:  "IpRangeCRNotCreated",
-	Message: "Failed to fetch new Ip Range from NetBox",
+	Reason:  "IPRangeCRNotCreated",
+	Message: "Failed to fetch new IP Range from NetBox",
 }
 
 var ConditionIpRangeAssignedFalseSizeMissmatch = metav1.Condition{
-	Type:    "IpRangeAssigned",
+	Type:    "IPRangeAssigned",
 	Status:  "False",
-	Reason:  "IpRangeCRNotCreated",
-	Message: "Assigned/Restored ip range has less available ip addresses than requested",
+	Reason:  "IPRangeCRNotCreated",
+	Message: "Assigned/Restored IP range has less available IP addresses than requested",
 }

config/crd/bases/netbox.dev_iprangeclaims.yaml

@@ -12,15 +12,15 @@ spec:
     listKind: IpRangeClaimList
     plural: iprangeclaims
     shortNames:
-    - irc
+    - iprc
     singular: iprangeclaim
   scope: Namespaced
   versions:
   - additionalPrinterColumns:
     - jsonPath: .status.ipRange
       name: IpRange
       type: string
-    - jsonPath: .status.conditions[?(@.type=="IpRangeAssigned")].status
+    - jsonPath: .status.conditions[?(@.type=="IPRangeAssigned")].status
       name: IpRangeAssigned
       type: string
     - jsonPath: .status.conditions[?(@.type=="Ready")].status

config/crd/bases/netbox.dev_ipranges.yaml

@@ -12,7 +12,7 @@ spec:
     listKind: IpRangeList
     plural: ipranges
     shortNames:
-    - ir
+    - ipr
     singular: iprange
   scope: Namespaced
   versions:

internal/controller/ipaddress_controller.go

@@ -45,7 +45,7 @@ import (
 )
 
 const IpAddressFinalizerName = "ipaddress.netbox.dev/finalizer"
-const LastIpAddressMetadataAnnotationName = "ipaddress.netbox.dev/last-ip-address-metadata"
+const LastIpAddressManagedCustomFieldsAnnotationName = "ipaddress.netbox.dev/managed-custom-fields"
 
 // IpAddressReconciler reconciles a IpAddress object
 type IpAddressReconciler struct {
@@ -163,7 +163,7 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		return ctrl.Result{}, err
 	}
 
-	ipAddressModel, err := generateNetboxIpAddressModelFromIpAddressSpec(&o.Spec, req, annotations[LastIpAddressMetadataAnnotationName])
+	ipAddressModel, err := generateNetboxIpAddressModelFromIpAddressSpec(&o.Spec, req, annotations[LastIpAddressManagedCustomFieldsAnnotationName])
 	if err != nil {
 		return ctrl.Result{}, err
 	}
@@ -189,7 +189,11 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		return ctrl.Result{}, err
 	}
 
-	annotations, err = updateLastMetadataAnnotation(annotations, o.Spec.CustomFields)
+	if annotations == nil {
+		annotations = make(map[string]string, 1)
+	}
+
+	annotations[LastIpAddressManagedCustomFieldsAnnotationName], err = generateLastMetadataAnnotation(o.Spec.CustomFields)
 	if err != nil {
 		logger.Error(err, "failed to update last metadata annotation")
 		return ctrl.Result{Requeue: true}, nil

internal/controller/iprange_controller.go

@@ -38,12 +38,11 @@ import (
 	"k8s.io/client-go/tools/record"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
 
 const IpRangeFinalizerName = "iprange.netbox.dev/finalizer"
-const LastIpRangeMetadataAnnotationName = "iprange.netbox.dev/last-ip-range-metadata"
+const LastIpRangeManagedCustomFieldsAnnotationName = "iprange.netbox.dev/managed-custom-fields"
 
 // IpRangeReconciler reconciles a IpRange object
 type IpRangeReconciler struct {
@@ -58,6 +57,7 @@ type IpRangeReconciler struct {
 //+kubebuilder:rbac:groups=netbox.dev,resources=ipranges,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=netbox.dev,resources=ipranges/status,verbs=get;update;patch
 //+kubebuilder:rbac:groups=netbox.dev,resources=ipranges/finalizers,verbs=update
+//+kubebuilder:rbac:groups=core,resources=events,verbs=create;patch
 
 // Reconcile is part of the main kubernetes reconciliation loop which aims to
 // move the current state of the cluster closer to the desired state.
@@ -74,43 +74,25 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 
 	// if being deleted
 	if !o.ObjectMeta.DeletionTimestamp.IsZero() {
-		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 ctrl.Result{}, r.removeFinalizer(ctx, o)
-		}
-
-		err := r.NetboxClient.DeleteIpRange(o.Status.IpRangeId)
-		if err != nil {
-			err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalseDeletionFailed,
-				corev1.EventTypeWarning, "", err)
+		if !o.Spec.PreserveInNetbox {
+			err := r.NetboxClient.DeleteIpRange(o.Status.IpRangeId)
 			if err != nil {
-				return ctrl.Result{}, err
-			}
+				err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalseDeletionFailed,
+					corev1.EventTypeWarning, "", err)
+				if err != nil {
+					return ctrl.Result{}, err
+				}
 
-			return ctrl.Result{Requeue: true}, nil
-		}
-
-		err = r.removeFinalizer(ctx, o)
-		if err != nil {
-			return ctrl.Result{}, err
-		}
-
-		err = r.Update(ctx, o)
-		if err != nil {
-			return ctrl.Result{}, err
+				return ctrl.Result{Requeue: true}, nil
+			}
 		}
 
-		return ctrl.Result{}, nil
+		return ctrl.Result{}, removeFinalizer(ctx, r.Client, o, IpRangeFinalizerName)
 	}
 
 	// if PreserveIpInNetbox flag is false then register finalizer if not yet registered
-	if !o.Spec.PreserveInNetbox && !controllerutil.ContainsFinalizer(o, IpRangeFinalizerName) {
-		logger.V(4).Info("adding the finalizer")
-		controllerutil.AddFinalizer(o, IpRangeFinalizerName)
-		if err := r.Update(ctx, o); err != nil {
-			return ctrl.Result{}, err
-		}
+	if !o.Spec.PreserveInNetbox {
+		err = addFinalizer(ctx, r.Client, o, IpRangeFinalizerName)
 	}
 
 	// 1. try to lock lease of parent prefix if IpRange status condition is not true
@@ -119,24 +101,12 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 	var ll *leaselocker.LeaseLocker
 	if len(or) > 0 && !apismeta.IsStatusConditionTrue(o.Status.Conditions, "Ready") {
 
-		// determine NamespacedName of IpRangeClaim owning the IpRange CR
-		orLookupKey := types.NamespacedName{
-			Name:      o.ObjectMeta.OwnerReferences[0].Name,
-			Namespace: o.Namespace,
-		}
-
-		ipRangeClaim := &netboxv1.IpRangeClaim{}
-		err := r.Client.Get(ctx, orLookupKey, ipRangeClaim)
+		leaseLockerNSN, owner, parentPrefix, err := r.getLeaseLockerNSNandOwner(ctx, o)
 		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, orLookupKey.String())
+		ll, err = leaselocker.NewLeaseLocker(r.RestConfig, leaseLockerNSN, owner)
 		if err != nil {
 			return ctrl.Result{}, err
 		}
@@ -147,12 +117,12 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 		// create lock
 		locked := ll.TryLock(lockCtx)
 		if !locked {
-			logger.Info(fmt.Sprintf("failed to lock parent prefix %s", ipRangeClaim.Spec.ParentPrefix))
+			logger.Info(fmt.Sprintf("failed to lock parent prefix %s", parentPrefix))
 			r.Recorder.Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", "failed to lock parent prefix %s",
-				ipRangeClaim.Spec.ParentPrefix)
+				parentPrefix)
 			return ctrl.Result{Requeue: true}, nil
 		}
-		logger.V(4).Info(fmt.Sprintf("successfully locked parent prefix %s", ipRangeClaim.Spec.ParentPrefix))
+		logger.V(4).Info(fmt.Sprintf("successfully locked parent prefix %s", parentPrefix))
 	}
 
 	// 2. reserve or update ip range in netbox
@@ -162,15 +132,15 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 		return ctrl.Result{}, err
 	}
 
-	ipRangeModel, err := r.generateNetboxIpRangeModelFromIpRangeSpec(o, req, annotations[LastIpRangeMetadataAnnotationName])
+	ipRangeModel, err := r.generateNetboxIpRangeModelFromIpRangeSpec(o, req, annotations[LastIpRangeManagedCustomFieldsAnnotationName])
 	if err != nil {
 		return ctrl.Result{}, err
 	}
 
 	netboxIpRangeModel, err := r.NetboxClient.ReserveOrUpdateIpRange(ipRangeModel)
 	if err != nil {
 		err := r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalse,
-			corev1.EventTypeWarning, fmt.Sprintf("%s-%s", o.Spec.StartAddress, o.Spec.EndAddress), err)
+			corev1.EventTypeWarning, fmt.Sprintf("%s-%s ", o.Spec.StartAddress, o.Spec.EndAddress), err)
 		if err != nil {
 			return ctrl.Result{}, err
 		}
@@ -189,7 +159,11 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 		return ctrl.Result{}, err
 	}
 
-	annotations, err = updateLastMetadataAnnotation(annotations, o.Spec.CustomFields)
+	if annotations == nil {
+		annotations = make(map[string]string, 1)
+	}
+
+	annotations[LastIpRangeManagedCustomFieldsAnnotationName], err = generateLastMetadataAnnotation(o.Spec.CustomFields)
 	if err != nil {
 		logger.Error(err, "failed to update last metadata annotation")
 		return ctrl.Result{Requeue: true}, nil
@@ -287,15 +261,24 @@ func (r *IpRangeReconciler) generateNetboxIpRangeModelFromIpRangeSpec(o *netboxv
 	}, nil
 }
 
-func (r *IpRangeReconciler) removeFinalizer(ctx context.Context, o *netboxv1.IpRange) error {
-	logger := log.FromContext(ctx)
-	if controllerutil.ContainsFinalizer(o, IpRangeFinalizerName) {
-		logger.V(4).Info("removing the finalizer")
-		controllerutil.RemoveFinalizer(o, IpRangeFinalizerName)
-		if err := r.Update(ctx, o); err != nil {
-			return err
-		}
+func (r *IpRangeReconciler) getLeaseLockerNSNandOwner(ctx context.Context, o *netboxv1.IpRange) (types.NamespacedName, string, string, error) {
+
+	orLookupKey := types.NamespacedName{
+		Name:      o.ObjectMeta.OwnerReferences[0].Name,
+		Namespace: o.Namespace,
+	}
+
+	ipRangeClaim := &netboxv1.IpRangeClaim{}
+	err := r.Client.Get(ctx, orLookupKey, ipRangeClaim)
+	if err != nil {
+		return types.NamespacedName{}, "", "", err
+	}
+
+	// get name of parent prefix
+	leaseLockerNSN := types.NamespacedName{
+		Name:      convertCIDRToLeaseLockName(ipRangeClaim.Spec.ParentPrefix),
+		Namespace: r.OperatorNamespace,
 	}
 
-	return nil
+	return leaseLockerNSN, orLookupKey.String(), ipRangeClaim.Spec.ParentPrefix, nil
 }