From 261e07b4b573a11d9c18b3c2e15dfc4a42339f47 Mon Sep 17 00:00:00 2001 From: "Hoanganh.Mai" Date: Mon, 23 Sep 2024 11:20:38 +0200 Subject: [PATCH] add check for tenant existence --- .../expected_netboxmock_calls_test.go | 2 +- .../controller/ipaddressclaim_controller.go | 9 +- internal/controller/prefixclaim_controller.go | 7 +- pkg/netbox/api/ip_address_claim.go | 7 +- pkg/netbox/api/ip_address_claim_test.go | 80 +++++++++++++- pkg/netbox/api/prefix_claim.go | 5 + pkg/netbox/api/prefix_claim_test.go | 101 ++++++++++++++++++ pkg/netbox/api/tenancy.go | 4 +- pkg/netbox/api/tenancy_test.go | 2 +- pkg/netbox/utils/error.go | 9 +- 10 files changed, 215 insertions(+), 11 deletions(-) diff --git a/internal/controller/expected_netboxmock_calls_test.go b/internal/controller/expected_netboxmock_calls_test.go index e4b51acf..7d45c4c8 100644 --- a/internal/controller/expected_netboxmock_calls_test.go +++ b/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) { diff --git a/internal/controller/ipaddressclaim_controller.go b/internal/controller/ipaddressclaim_controller.go index b0d0d2a5..bf88be31 100644 --- a/internal/controller/ipaddressclaim_controller.go +++ b/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 diff --git a/internal/controller/prefixclaim_controller.go b/internal/controller/prefixclaim_controller.go index 5e1fb5e3..4eb3542c 100644 --- a/internal/controller/prefixclaim_controller.go +++ b/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 { diff --git a/pkg/netbox/api/ip_address_claim.go b/pkg/netbox/api/ip_address_claim.go index 2c77db82..f1ea353b 100644 --- a/pkg/netbox/api/ip_address_claim.go +++ b/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 diff --git a/pkg/netbox/api/ip_address_claim_test.go b/pkg/netbox/api/ip_address_claim_test.go index 3631a3f1..526a0748 100644 --- a/pkg/netbox/api/ip_address_claim_test.go +++ b/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)) + }) +} diff --git a/pkg/netbox/api/prefix_claim.go b/pkg/netbox/api/prefix_claim.go index ccac1ba0..e5191ecf 100644 --- a/pkg/netbox/api/prefix_claim.go +++ b/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, diff --git a/pkg/netbox/api/prefix_claim_test.go b/pkg/netbox/api/prefix_claim_test.go index 7f4812b5..831f4367 100644 --- a/pkg/netbox/api/prefix_claim_test.go +++ b/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)) +} diff --git a/pkg/netbox/api/tenancy.go b/pkg/netbox/api/tenancy.go index 016fa3c4..12b816a6 100644 --- a/pkg/netbox/api/tenancy.go +++ b/pkg/netbox/api/tenancy.go @@ -17,8 +17,6 @@ limitations under the License. package api import ( - "errors" - "github.com/netbox-community/go-netbox/v3/netbox/client/tenancy" "github.com/netbox-community/netbox-operator/pkg/netbox/models" @@ -32,7 +30,7 @@ func (r *NetboxClient) GetTenantDetails(name string) (*models.Tenant, error) { return nil, utils.NetboxError("failed to fetch Tenant details", err) } if len(response.Payload.Results) == 0 { - return nil, errors.New("tenant not found") + return nil, utils.NetboxNotFoundError("tenant") } return &models.Tenant{ diff --git a/pkg/netbox/api/tenancy_test.go b/pkg/netbox/api/tenancy_test.go index c2428c1b..d8238dbf 100644 --- a/pkg/netbox/api/tenancy_test.go +++ b/pkg/netbox/api/tenancy_test.go @@ -80,5 +80,5 @@ func TestTenancy_GetWrongTenantDetails(t *testing.T) { actual, err := netboxClient.GetTenantDetails(wrongTenant) assert.Nil(t, actual) - assert.EqualError(t, err, "tenant not found") + assert.EqualError(t, err, "failed to fetch tenant: not found") } diff --git a/pkg/netbox/utils/error.go b/pkg/netbox/utils/error.go index 223ed4f8..71befab3 100644 --- a/pkg/netbox/utils/error.go +++ b/pkg/netbox/utils/error.go @@ -17,10 +17,17 @@ limitations under the License. package utils import ( + "fmt" + "github.com/pkg/errors" ) +var ErrNotFound = errors.New("not found") + func NetboxError(message string, err error) error { + return fmt.Errorf(message+": %w", err) +} - return errors.Errorf(message, err.Error()) +func NetboxNotFoundError(name string) error { + return NetboxError("failed to fetch "+name, ErrNotFound) }