add check for tenant existence

Author: Hoanganh.Mai

internal/controller/expected_netboxmock_calls_test.go

@@ -234,7 +234,7 @@ func mockTenancyTenancyTenantsList(tenancyMock *mock_interfaces.MockTenancyInter
 }
 
 // -----------------------------
-// Restet Mock Functions
+// Reset Mock Functions
 // -----------------------------
 
 func resetMockFunctions(ipamMockA *mock_interfaces.MockIpamInterface, ipamMockB *mock_interfaces.MockIpamInterface, tenancyMock *mock_interfaces.MockTenancyInterface) {

internal/controller/ipaddress_controller.go

@@ -101,6 +101,15 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		return ctrl.Result{}, nil
 	}
 
+	// if PreserveIpInNetbox flag is false then register finalizer if not yet registered
+	if !o.Spec.PreserveInNetbox && !controllerutil.ContainsFinalizer(o, IpAddressFinalizerName) {
+		debugLogger.Info("adding the finalizer")
+		controllerutil.AddFinalizer(o, IpAddressFinalizerName)
+		if err := r.Update(ctx, o); err != nil {
+			return ctrl.Result{}, err
+		}
+	}
+
 	// 1. try to lock lease of parent prefix if IpAddress status condition is not true
 	// and IpAddress is owned by an IpAddressClaim
 	or := o.ObjectMeta.OwnerReferences
@@ -193,15 +202,6 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		r.Recorder.Event(o, corev1.EventTypeWarning, "IpDescriptionTruncated", "ip address was created with truncated description")
 	}
 
-	// if PreserveIpInNetbox flag is false then register finalizer if not yet registered
-	if !o.Spec.PreserveInNetbox && !controllerutil.ContainsFinalizer(o, IpAddressFinalizerName) {
-		debugLogger.Info("adding the finalizer")
-		controllerutil.AddFinalizer(o, IpAddressFinalizerName)
-		if err := r.Update(ctx, o); err != nil {
-			return ctrl.Result{}, err
-		}
-	}
-
 	debugLogger.Info(fmt.Sprintf("reserved ip address in netbox, ip: %s", o.Spec.IpAddress))
 
 	// 3. unlock lease of parent prefix

internal/controller/ipaddressclaim_controller.go

@@ -137,9 +137,14 @@ func (r *IpAddressClaimReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 					},
 				})
 			if err != nil {
-				return ctrl.Result{}, err
+				setConditionErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpAssignedFalse, corev1.EventTypeWarning, err.Error())
+				if setConditionErr != nil {
+					return ctrl.Result{}, fmt.Errorf("error updating status: %w, when getting an available IP address failed: %w", setConditionErr, err)
+				}
+
+				return ctrl.Result{Requeue: true}, nil
 			}
-			debugLogger.Info(fmt.Sprintf("ip address is not reserved in netbox, assignined new ip address: %s", ipAddressModel.IpAddress))
+			debugLogger.Info(fmt.Sprintf("ip address is not reserved in netbox, assigned new ip address: %s", ipAddressModel.IpAddress))
 		} else {
 			// 5.b reassign reserved ip address from netbox
 			// do nothing, ip address restored

internal/controller/prefix_controller.go

@@ -99,6 +99,15 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 		return ctrl.Result{}, nil
 	}
 
+	// register finalizer if not yet registered
+	if !prefix.Spec.PreserveInNetbox && !controllerutil.ContainsFinalizer(prefix, PrefixFinalizerName) {
+		debugLogger.Info("adding the finalizer")
+		controllerutil.AddFinalizer(prefix, PrefixFinalizerName)
+		if err := r.Update(ctx, prefix); err != nil {
+			return ctrl.Result{}, err
+		}
+	}
+
 	/*
 		1. try to lock the lease of the parent prefix if all of the following conditions are met
 			- the prefix is owned by at least 1 prefixClaim
@@ -188,15 +197,6 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 		r.Recorder.Event(prefix, corev1.EventTypeWarning, "PrefixDescriptionTruncated", "prefix was created with truncated description")
 	}
 
-	// register finalizer if not yet registered
-	if !prefix.Spec.PreserveInNetbox && !controllerutil.ContainsFinalizer(prefix, PrefixFinalizerName) {
-		debugLogger.Info("adding the finalizer")
-		controllerutil.AddFinalizer(prefix, PrefixFinalizerName)
-		if err := r.Update(ctx, prefix); err != nil {
-			return ctrl.Result{}, err
-		}
-	}
-
 	debugLogger.Info(fmt.Sprintf("reserved prefix in netbox, prefix: %s", prefix.Spec.Prefix))
 
 	/* 3. unlock lease of parent prefix */

internal/controller/prefixclaim_controller.go

@@ -135,7 +135,12 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 					},
 				})
 			if err != nil {
-				return ctrl.Result{}, err
+				setConditionErr := r.SetConditionAndCreateEvent(ctx, prefixClaim, netboxv1.ConditionPrefixAssignedFalse, corev1.EventTypeWarning, err.Error())
+				if setConditionErr != nil {
+					return ctrl.Result{}, fmt.Errorf("error updating status: %w, when getting an available Prefix failed: %w", setConditionErr, err)
+				}
+
+				return ctrl.Result{Requeue: true}, nil
 			}
 			debugLogger.Info(fmt.Sprintf("prefix is not reserved in netbox, assignined new prefix: %s", prefixModel.Prefix))
 		} else {

pkg/netbox/api/ip_address_claim.go

@@ -23,6 +23,7 @@ import (
 
 	"github.com/netbox-community/go-netbox/v3/netbox/client/ipam"
 	"github.com/netbox-community/netbox-operator/pkg/netbox/models"
+	"github.com/netbox-community/netbox-operator/pkg/netbox/utils"
 )
 
 const (
@@ -57,6 +58,10 @@ func (r *NetboxClient) RestoreExistingIpByHash(customFieldName string, hash stri
 
 // GetAvailableIpAddressByClaim searches an available IpAddress in Netbox matching IpAddressClaim requirements
 func (r *NetboxClient) GetAvailableIpAddressByClaim(ipAddressClaim *models.IPAddressClaim) (*models.IPAddress, error) {
+	_, err := r.GetTenantDetails(ipAddressClaim.Metadata.Tenant)
+	if err != nil {
+		return nil, err
+	}
 
 	responseParentPrefix, err := r.GetPrefix(&models.Prefix{
 		Prefix:   ipAddressClaim.ParentPrefix,
@@ -66,7 +71,7 @@ func (r *NetboxClient) GetAvailableIpAddressByClaim(ipAddressClaim *models.IPAdd
 		return nil, err
 	}
 	if len(responseParentPrefix.Payload.Results) == 0 {
-		return nil, errors.New("parent prefix not found")
+		return nil, utils.NetboxNotFoundError("parent prefix")
 	}
 
 	parentPrefixId := responseParentPrefix.Payload.Results[0].ID

pkg/netbox/api/ip_address_claim_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package api
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/netbox-community/go-netbox/v3/netbox/client/ipam"
@@ -166,8 +167,10 @@ func TestIPAddressClaim(t *testing.T) {
 			},
 		})
 
+		expectedErrMsg := "failed to fetch parent prefix: not found"
+
 		// assert error
-		AssertError(t, err, "parent prefix not found")
+		AssertError(t, err, expectedErrMsg)
 		// assert nil output
 		assert.Nil(t, actual)
 	})
@@ -231,3 +234,78 @@ func TestIPAddressClaim(t *testing.T) {
 		assert.Equal(t, ipAddressRestore, actual.IpAddress)
 	})
 }
+
+func TestIPAddressClaim_GetNoAvailableIPAddressWithTenancyChecks(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	mockTenancy := mock_interfaces.NewMockTenancyInterface(ctrl)
+
+	parentPrefix := "10.112.140.0/24"
+	t.Run("No IP address asigned with an error when getting the tenant list", func(t *testing.T) {
+
+		tenantName := "Tenant1"
+
+		inputTenant := tenancy.NewTenancyTenantsListParams().WithName(&tenantName)
+
+		// expected error
+		expectedErrorMsg := "cannot get the list" // testcase-defined error
+
+		mockTenancy.EXPECT().TenancyTenantsList(inputTenant, nil).Return(nil, errors.New(expectedErrorMsg)).AnyTimes()
+
+		// init client
+		client := &NetboxClient{
+			Tenancy: mockTenancy,
+		}
+
+		actual, err := client.GetAvailableIpAddressByClaim(&models.IPAddressClaim{
+			ParentPrefix: parentPrefix,
+			Metadata: &models.NetboxMetadata{
+				Tenant: tenantName,
+			},
+		})
+
+		// assert error
+		assert.Containsf(t, err.Error(), expectedErrorMsg, "Error should contain: %v, got: %v", expectedErrorMsg, err.Error())
+		// assert nil output
+		assert.Equal(t, actual, (*models.IPAddress)(nil))
+	})
+
+	t.Run("No IP address asigned with non-existing tenant", func(t *testing.T) {
+
+		// non existing tenant
+		nonExistingTenant := "non-existing-tenant"
+
+		inputTenant := tenancy.NewTenancyTenantsListParams().WithName(&nonExistingTenant)
+
+		// empty tenant list
+		emptyTenantList := &tenancy.TenancyTenantsListOK{
+			Payload: &tenancy.TenancyTenantsListOKBody{
+				Results: []*netboxModels.Tenant{},
+			},
+		}
+
+		// expected error
+		expectedErrorMsg := "failed to fetch tenant: not found"
+
+		// mock empty list call
+		mockTenancy.EXPECT().TenancyTenantsList(inputTenant, nil).Return(emptyTenantList, nil).AnyTimes()
+
+		// init client
+		client := &NetboxClient{
+			Tenancy: mockTenancy,
+		}
+
+		actual, err := client.GetAvailableIpAddressByClaim(&models.IPAddressClaim{
+			ParentPrefix: parentPrefix,
+			Metadata: &models.NetboxMetadata{
+				Tenant: nonExistingTenant,
+			},
+		})
+
+		// assert error
+		assert.EqualErrorf(t, err, expectedErrorMsg, "Error should be: %v, got: %v", expectedErrorMsg, err)
+		// assert nil output
+		assert.Equal(t, actual, (*models.IPAddress)(nil))
+	})
+}

pkg/netbox/api/prefix_claim.go

@@ -67,6 +67,11 @@ func isRequestingTheEntireParentPrefix(prefixClaim *models.PrefixClaim) (bool, e
 
 // GetAvailablePrefixByClaim searches an available Prefix in Netbox matching PrefixClaim requirements
 func (r *NetboxClient) GetAvailablePrefixByClaim(prefixClaim *models.PrefixClaim) (*models.Prefix, error) {
+	_, err := r.GetTenantDetails(prefixClaim.Metadata.Tenant)
+	if err != nil {
+		return nil, err
+	}
+
 	responseParentPrefix, err := r.GetPrefix(&models.Prefix{
 		Prefix:   prefixClaim.ParentPrefix,
 		Metadata: prefixClaim.Metadata,

pkg/netbox/api/prefix_claim_test.go

@@ -38,6 +38,7 @@ func TestPrefixClaim_GetAvailablePrefixesByParentPrefix(t *testing.T) {
 	//prefix mock input
 	parentPrefixId := int64(3)
 	availablePrefixListInput := ipam.NewIpamPrefixesAvailablePrefixesListParams().WithID(parentPrefixId)
+
 	//prefix mock output
 	childPrefix1 := "10.112.140.0/24"
 	childPrefix2 := "10.120.180.0/24"
@@ -129,6 +130,9 @@ func TestPrefixClaim_GetAvailablePrefixByClaim_WithWrongParent(t *testing.T) {
 	actual, err := netboxClient.GetAvailablePrefixByClaim(&models.PrefixClaim{
 		ParentPrefix: prefix,
 		PrefixLength: "/28",
+		Metadata: &models.NetboxMetadata{
+			Tenant: tenantName,
+		},
 	})
 	assert.Nil(t, actual)
 	assert.EqualError(t, err, "parent prefix not found")
@@ -196,6 +200,9 @@ func TestPrefixClaim_GetBestFitPrefixByClaim(t *testing.T) {
 	actual, err := netboxClient.GetAvailablePrefixByClaim(&models.PrefixClaim{
 		ParentPrefix: parentPrefix,
 		PrefixLength: "/28",
+		Metadata: &models.NetboxMetadata{
+			Tenant: tenantName,
+		},
 	})
 
 	assert.Nil(t, err)
@@ -272,6 +279,9 @@ func TestPrefixClaim_GetBestFitPrefixByClaimNoAvailablePrefixMatchesSize(t *test
 	actual, err := netboxClient.GetAvailablePrefixByClaim(&models.PrefixClaim{
 		ParentPrefix: parentPrefix,
 		PrefixLength: "/28",
+		Metadata: &models.NetboxMetadata{
+			Tenant: tenantName,
+		},
 	})
 
 	assert.Nil(t, err)
@@ -340,6 +350,9 @@ func TestPrefixClaim_GetBestFitPrefixByClaimNoAvailablePrefixMatchesSizeCriteria
 	_, err := netboxClient.GetAvailablePrefixByClaim(&models.PrefixClaim{
 		ParentPrefix: parentPrefix,
 		PrefixLength: "/28",
+		Metadata: &models.NetboxMetadata{
+			Tenant: tenantName,
+		},
 	})
 
 	assert.True(t, errors.Is(err, ErrNoPrefixMatchsSizeCriteria))
@@ -415,6 +428,9 @@ func TestPrefixClaim_GetBestFitPrefixByClaimInvalidFormatFromNetbox(t *testing.T
 	actual, err := netboxClient.GetAvailablePrefixByClaim(&models.PrefixClaim{
 		ParentPrefix: parentPrefix,
 		PrefixLength: "/28",
+		Metadata: &models.NetboxMetadata{
+			Tenant: tenantName,
+		},
 	})
 
 	assert.Nil(t, err)
@@ -483,7 +499,92 @@ func TestPrefixClaim_GetBestFitPrefixByClaimInvalidPrefixClaim(t *testing.T) {
 	_, err := netboxClient.GetAvailablePrefixByClaim(&models.PrefixClaim{
 		ParentPrefix: parentPrefix,
 		PrefixLength: "/28.",
+		Metadata: &models.NetboxMetadata{
+			Tenant: tenantName,
+		},
 	})
 
 	assert.True(t, errors.Is(err, strconv.ErrSyntax))
 }
+
+func TestPrefixClaim_GetNoAvailablePrefixesWithNonExistingTenant(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	mockTenancy := mock_interfaces.NewMockTenancyInterface(ctrl)
+
+	// non-existing tenant
+	tenantName := "non-existing-tenant"
+
+	inputTenant := tenancy.NewTenancyTenantsListParams().WithName(&tenantName)
+
+	// expected error
+	expectedErrorMsg := "failed to fetch tenant: not found"
+
+	// empty tenant list
+	emptyTenantList := &tenancy.TenancyTenantsListOK{
+		Payload: &tenancy.TenancyTenantsListOKBody{
+			Results: []*netboxModels.Tenant{},
+		},
+	}
+
+	parentPrefix := "10.112.140.0/24"
+
+	mockTenancy.EXPECT().TenancyTenantsList(inputTenant, nil).Return(emptyTenantList, nil).AnyTimes()
+
+	netboxClient := &NetboxClient{
+		Tenancy: mockTenancy,
+	}
+
+	prefix, err := netboxClient.GetAvailablePrefixByClaim(&models.PrefixClaim{
+		ParentPrefix: parentPrefix,
+		PrefixLength: "/28.",
+		Metadata: &models.NetboxMetadata{
+			Tenant: tenantName,
+		},
+	})
+
+	assert.EqualErrorf(t, err, expectedErrorMsg, "Error should be: %v, got: %v", expectedErrorMsg, err)
+	assert.Equal(t, prefix, (*models.Prefix)(nil))
+}
+
+func TestPrefixClaim_GetNoAvailablePrefixesWithErrorWhenGettingTenantList(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	mockTenancy := mock_interfaces.NewMockTenancyInterface(ctrl)
+
+	// non-existing tenant
+	tenantName := "non-existing tenant"
+
+	inputTenant := tenancy.NewTenancyTenantsListParams().WithName(&tenantName)
+
+	// expected errors
+	getTenantDetailsErrorMsg := "failed to fetch Tenant details"
+	tenancyTenantsListErrorMsg := "cannot get the list" // testcase defined error
+
+	parentPrefix := "10.112.140.0/24"
+
+	mockTenancy.EXPECT().TenancyTenantsList(inputTenant, nil).Return(nil, errors.New(tenancyTenantsListErrorMsg)).AnyTimes()
+
+	netboxClient := &NetboxClient{
+		Tenancy: mockTenancy,
+	}
+
+	prefix, err := netboxClient.GetAvailablePrefixByClaim(&models.PrefixClaim{
+		ParentPrefix: parentPrefix,
+		PrefixLength: "/28.",
+		Metadata: &models.NetboxMetadata{
+			Tenant: tenantName,
+		},
+	})
+
+	// assert 1st level error - GetTenantDetails()
+	assert.Containsf(t, err.Error(), getTenantDetailsErrorMsg, "expected error containing %q, got %s", getTenantDetailsErrorMsg, err)
+
+	// assert 2nd level error - TenanyTenantsList()
+	assert.Containsf(t, err.Error(), tenancyTenantsListErrorMsg, "expected error containing %q, got %s", tenancyTenantsListErrorMsg, err)
+
+	// assert nil output
+	assert.Equal(t, prefix, (*models.Prefix)(nil))
+}