🌱 Improve Unit tests (#145)

**What is the purpose of this pull request/Why do we need it?**

Improve the current unit tests a bit

**Checklist:**
- [x] Unit Tests added
- [x] Includes
[emojis](https://github.com/kubernetes-sigs/kubebuilder-release-tools?tab=readme-ov-file#kubebuilder-project-versioning)

---------

Co-authored-by: Jonas Riedel <[email protected]>

api/v1alpha1/ionoscloudcluster_types_test.go

@@ -83,6 +83,12 @@ var _ = Describe("IonosCloudCluster", func() {
 			Expect(k8sClient.Create(context.Background(), cluster)).
 				Should(MatchError(ContainSubstring("credentialsRef.name must be provided")))
 		})
+		It("should not allow creating clusters with empty location", func() {
+			cluster := defaultCluster()
+			cluster.Spec.Location = ""
+			Expect(k8sClient.Create(context.Background(), cluster)).
+				Should(MatchError(ContainSubstring("spec.location in body should be at least 1 chars long")))
+		})
 	})
 
 	Context("Update", func() {

internal/service/cloud/ipblock.go

@@ -100,6 +100,10 @@ func (s *Service) ReconcileControlPlaneEndpointDeletion(
 		s.getControlPlaneEndpointIPBlock,
 		s.getLatestControlPlaneEndpointIPBlockCreationRequest,
 	)
+
+	if ignoreErrUserSetIPNotFound(ignoreNotFound(err)) != nil {
+		return false, err
+	}
 	// NOTE(gfariasalves): we ignore the error if it is a "user set IP not found" error, because it doesn't matter here.
 	// This error is only relevant when we are trying to create a new IP block. If it shows up here, it means that:
 	// a) this IP block was created by the user, and they have deleted it, or,
@@ -111,10 +115,6 @@ func (s *Service) ReconcileControlPlaneEndpointDeletion(
 		return false, nil
 	}
 
-	if ignoreErrUserSetIPNotFound(ignoreNotFound(err)) != nil {
-		return false, err
-	}
-
 	// NOTE: this check covers the case where customers have set the control plane endpoint IP themselves.
 	// If this is the case we don't request for the deletion of the IP block.
 	if ipBlock != nil && ptr.Deref(ipBlock.GetProperties().GetName(), unknownValue) != s.controlPlaneEndpointIPBlockName(cs) {

internal/service/cloud/ipblock_test.go

@@ -233,6 +233,18 @@ func (s *ipBlockTestSuite) TestGetLatestIPBlockDeletionRequestRequest() {
 	s.NotNil(info)
 }
 
+func (s *ipBlockTestSuite) TestReconcileControlPlaneEndpointUnavailable() {
+	block := exampleIPBlock()
+	block.GetMetadata().SetState(sdk.Busy)
+
+	s.mockListIPBlocksCall().Return(&sdk.IpBlocks{
+		Items: &[]sdk.IpBlock{*block},
+	}, nil).Once()
+	requeue, err := s.service.ReconcileControlPlaneEndpoint(s.ctx, s.clusterScope)
+	s.True(requeue)
+	s.NoError(err)
+}
+
 func (s *ipBlockTestSuite) TestReconcileControlPlaneEndpointUserSetIP() {
 	block := exampleIPBlock()
 	block.Properties.Name = ptr.To("asdf")
@@ -371,6 +383,18 @@ func (s *ipBlockTestSuite) TestReconcileControlPlaneEndpointDeletionRequestNewDe
 	s.Equal(exampleRequestPath, s.clusterScope.IonosCluster.Status.CurrentClusterRequest.RequestPath)
 }
 
+func (s *ipBlockTestSuite) TestReconcileControlPlaneEndpointDeletionErrorOnDuplicateIPBlock() {
+	block := exampleIPBlock()
+	dup := exampleIPBlock()
+	s.mockListIPBlocksCall().Return(&sdk.IpBlocks{Items: &[]sdk.IpBlock{
+		*block,
+		*dup,
+	}}, nil).Once()
+	requeue, err := s.service.ReconcileControlPlaneEndpointDeletion(s.ctx, s.clusterScope)
+	s.False(requeue)
+	s.Error(err)
+}
+
 func (s *ipBlockTestSuite) TestReconcileFailoverIPBlockDeletion() {
 	s.infraMachine.Spec.FailoverIP = ptr.To(infrav1.CloudResourceConfigAuto)
 	ipBlock := exampleIPBlockWithName(s.service.failoverIPBlockName(s.machineScope))
@@ -460,6 +484,13 @@ func (s *ipBlockTestSuite) TestReconcileFailoverIPBlockDeletionDeletionFinished(
 	s.Nil(s.machineScope.IonosMachine.Status.CurrentRequest)
 }
 
+func (s *ipBlockTestSuite) TestReconcileFailoverIPBlockDeletionShouldSkipDeletion() {
+	requeue, err := s.service.ReconcileFailoverIPBlockDeletion(s.ctx, s.machineScope)
+	s.NoError(err)
+	s.False(requeue)
+	s.Nil(s.machineScope.IonosMachine.Status.CurrentRequest)
+}
+
 func (s *ipBlockTestSuite) mockDeleteIPBlockCall() *clienttest.MockClient_DeleteIPBlock_Call {
 	return s.ionosClient.EXPECT().DeleteIPBlock(s.ctx, exampleIPBlockID)
 }

internal/service/cloud/network_test.go

@@ -371,7 +371,7 @@ func setControlPlaneLabel(ctx context.Context, k8sClient client.Client, machine
 	return k8sClient.Update(ctx, machine)
 }
 
-func (s *lanSuite) reconcileIPFailoverDeletion(testServer *sdk.Server, testLAN sdk.Lan) {
+func (s *lanSuite) reconcileIPFailoverDeletion(testServer *sdk.Server, testLAN sdk.Lan) (requeue bool, err error) {
 	s.mockGetServerCall(exampleServerID).Return(testServer, nil).Once()
 	s.mockListLANsCall().Return(&sdk.Lans{Items: &[]sdk.Lan{testLAN}}, nil).Once()
 	s.setupSuccessfulLANPatchMocks()
@@ -384,9 +384,7 @@ func (s *lanSuite) reconcileIPFailoverDeletion(testServer *sdk.Server, testLAN s
 	}
 
 	s.mockPatchLANCall(props).Return(exampleRequestPath, nil).Once()
-	s.assertSuccessfulDeletion(
-		s.service.ReconcileIPFailoverDeletion(s.ctx, s.machineScope),
-	)
+	return s.service.ReconcileIPFailoverDeletion(s.ctx, s.machineScope)
 }
 
 func (s *lanSuite) TestReconcileIPFailoverDeletionWorker() {
@@ -409,7 +407,7 @@ func (s *lanSuite) TestReconcileIPFailoverDeletionWorker() {
 		NicUuid: ptr.To(exampleNICID),
 	}}
 
-	s.reconcileIPFailoverDeletion(testServer, testLAN)
+	s.assertSuccessfulDeletion(s.reconcileIPFailoverDeletion(testServer, testLAN))
 }
 
 func (s *lanSuite) TestReconcileIPFailoverDeletionControlPlane() {
@@ -430,7 +428,7 @@ func (s *lanSuite) TestReconcileIPFailoverDeletionControlPlane() {
 		NicUuid: ptr.To(exampleNICID),
 	}}
 
-	s.reconcileIPFailoverDeletion(testServer, testLAN)
+	s.assertSuccessfulDeletion(s.reconcileIPFailoverDeletion(testServer, testLAN))
 }
 
 func (s *lanSuite) TestReconcileIPFailoverDeletionControlPlaneSwitchNIC() {
@@ -444,7 +442,7 @@ func (s *lanSuite) TestReconcileIPFailoverDeletionControlPlaneSwitchNIC() {
 	newIonosMachine.SetName("test-machine-2")
 	newIonosMachine.SetResourceVersion("")
 	newIonosMachine.SetCreationTimestamp(metav1.NewTime(time.Now()))
-	newIonosMachine.Spec.ProviderID = ptr.To("ionos://" + exampleSecondaryServerID)
+	newIonosMachine.Spec.ProviderID = ptr.To("ionos://" + exampleSecondaryServerID) //nolint: goconst
 	err = s.k8sClient.Create(s.ctx, newIonosMachine)
 	s.NoError(err)
 
@@ -484,6 +482,41 @@ func (s *lanSuite) TestReconcileIPFailoverDeletionControlPlaneSwitchNIC() {
 	)
 }
 
+func (s *lanSuite) TestReconcileFailoverDeletionWorkerNoSwap() {
+	const deploymentLabel = "test-deployment"
+	labels := s.infraMachine.GetLabels()
+	labels[clusterv1.MachineDeploymentNameLabel] = deploymentLabel
+	s.infraMachine.SetLabels(labels)
+
+	s.infraMachine.Spec.FailoverIP = ptr.To(exampleWorkerFailoverIP)
+
+	secondaryMachine := s.infraMachine.DeepCopy()
+	secondaryMachine.SetName("test-machine-2")
+	secondaryMachine.SetResourceVersion("")
+	secondaryMachine.SetCreationTimestamp(metav1.NewTime(time.Now()))
+	testServer := s.defaultServer(s.infraMachine, exampleDHCPIP, exampleWorkerFailoverIP)
+
+	s.NoError(s.k8sClient.Create(s.ctx, secondaryMachine))
+	s.NoError(s.k8sClient.Update(s.ctx, s.infraMachine))
+
+	testLAN := s.exampleLAN()
+	testLAN.Properties.IpFailover = &[]sdk.IPFailover{{
+		Ip:      ptr.To(exampleArbitraryIP),
+		NicUuid: ptr.To(arbitraryNICID),
+	}, {
+		Ip:      ptr.To(exampleWorkerFailoverIP),
+		NicUuid: ptr.To(exampleSecondaryNICID),
+	}}
+
+	s.mockGetServerCall(exampleServerID).Return(testServer, nil).Once()
+	s.mockListLANsCall().Return(&sdk.Lans{Items: &[]sdk.Lan{testLAN}}, nil).Once()
+	s.mockGetLANPatchRequestCall().Return(nil, nil).Once()
+	requeue, err := s.service.ReconcileIPFailoverDeletion(s.ctx, s.machineScope)
+
+	s.NoError(err)
+	s.False(requeue)
+}
+
 func (s *lanSuite) setupSuccessfulLANPatchMocks() {
 	s.T().Helper()
 	patchRequest := s.examplePatchRequest(sdk.RequestStatusDone)

internal/service/cloud/server_test.go

@@ -22,6 +22,7 @@ import (
 	"path"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	sdk "github.com/ionos-cloud/sdk-go/v6"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/suite"
@@ -152,9 +153,9 @@ func (s *serverSuite) TestReconcileServerRequestDoneStateAvailableTurnedOff() {
 func (s *serverSuite) TestReconcileEnterpriseServerNoRequest() {
 	s.prepareReconcileServerRequestTest()
 	s.mockGetServerCreationRequestCall().Return([]sdk.Request{}, nil)
-	s.mockCreateServerCall(infrav1.ServerTypeEnterprise).Return(&sdk.Server{Id: ptr.To("12345")}, "location/to/server", nil)
+	s.mockCreateServerCall(s.defaultServerComponents()).Return(&sdk.Server{Id: ptr.To("12345")}, "location/to/server", nil)
 	s.mockListLANsCall().Return(&sdk.Lans{Items: &[]sdk.Lan{{
-		Id: ptr.To("1"),
+		Id: ptr.To(exampleLANID),
 		Properties: &sdk.LanProperties{
 			Name:   ptr.To(s.service.lanName(s.clusterScope.Cluster)),
 			Public: ptr.To(true),
@@ -170,9 +171,12 @@ func (s *serverSuite) TestReconcileEnterpriseServerNoRequest() {
 func (s *serverSuite) TestReconcileVCPUServerNoRequest() {
 	s.prepareReconcileServerRequestTest()
 	s.mockGetServerCreationRequestCall().Return([]sdk.Request{}, nil)
-	s.mockCreateServerCall(infrav1.ServerTypeVCPU).Return(&sdk.Server{Id: ptr.To("12345")}, "location/to/server", nil)
+
+	props, entities := s.defaultServerComponents()
+	props.Type = ptr.To(infrav1.ServerTypeVCPU.String())
+	s.mockCreateServerCall(props, entities).Return(&sdk.Server{Id: ptr.To("12345")}, "location/to/server", nil)
 	s.mockListLANsCall().Return(&sdk.Lans{Items: &[]sdk.Lan{{
-		Id: ptr.To("1"),
+		Id: ptr.To(exampleLANID),
 		Properties: &sdk.LanProperties{
 			Name:   ptr.To(s.service.lanName(s.clusterScope.Cluster)),
 			Public: ptr.To(true),
@@ -444,12 +448,15 @@ func (s *serverSuite) mockGetServerDeletionRequestCall(serverID string) *clientt
 		http.MethodDelete, path.Join(s.service.serversURL(s.machineScope.DatacenterID()), serverID))
 }
 
-func (s *serverSuite) mockCreateServerCall(serverType infrav1.ServerType) *clienttest.MockClient_CreateServer_Call {
+func (s *serverSuite) mockCreateServerCall(
+	properties sdk.ServerProperties,
+	entities sdk.ServerEntities,
+) *clienttest.MockClient_CreateServer_Call {
 	return s.ionosClient.EXPECT().CreateServer(
 		s.ctx,
 		s.machineScope.DatacenterID(),
-		mock.MatchedBy(hasServerType(serverType)),
-		mock.Anything,
+		properties,
+		mock.MatchedBy(ignoreBootstrapData(entities)),
 	)
 }
 
@@ -482,8 +489,12 @@ func (s *serverSuite) examplePostRequest(status string) sdk.Request {
 	return s.exampleRequest(opts)
 }
 
-func hasServerType(serverType infrav1.ServerType) func(properties sdk.ServerProperties) bool {
-	return func(properties sdk.ServerProperties) bool {
-		return ptr.Deref(properties.Type, "") == serverType.String()
+func ignoreBootstrapData(expected sdk.ServerEntities) func(sdk.ServerEntities) bool {
+	return func(entities sdk.ServerEntities) bool {
+		items := entities.GetVolumes().GetItems()
+		for i := range ptr.Deref(items, []sdk.Volume{}) {
+			(*items)[i].GetProperties().UserData = nil
+		}
+		return cmp.Diff(expected, entities) == ""
 	}
 }

internal/service/cloud/suite_test.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"net/http"
 	"path"
+	"strconv"
 	"testing"
 
 	"github.com/go-logr/logr"
@@ -136,7 +137,7 @@ func (s *ServiceTestSuite) SetupTest() {
 		Spec: clusterv1.MachineSpec{
 			ClusterName: s.capiCluster.Name,
 			Version:     ptr.To("v1.26.12"),
-			ProviderID:  ptr.To("ionos://dd426c63-cd1d-4c02-aca3-13b4a27c2ebf"),
+			ProviderID:  ptr.To("ionos://" + exampleServerID),
 		},
 	}
 	s.infraMachine = &infrav1.IonosCloudMachine{
@@ -149,7 +150,7 @@ func (s *ServiceTestSuite) SetupTest() {
 			},
 		},
 		Spec: infrav1.IonosCloudMachineSpec{
-			ProviderID:       ptr.To("ionos://dd426c63-cd1d-4c02-aca3-13b4a27c2ebf"),
+			ProviderID:       ptr.To("ionos://" + exampleServerID),
 			DatacenterID:     "ccf27092-34e8-499e-a2f5-2bdee9d34a12",
 			NumCores:         2,
 			AvailabilityZone: infrav1.AvailabilityZoneAuto,
@@ -300,6 +301,44 @@ func (s *ServiceTestSuite) exampleLAN() sdk.Lan {
 	}
 }
 
+func (s *ServiceTestSuite) defaultServerComponents() (sdk.ServerProperties, sdk.ServerEntities) {
+	m := s.machineScope.IonosMachine
+	props := sdk.ServerProperties{
+		AvailabilityZone: ptr.To(m.Spec.AvailabilityZone.String()),
+		Cores:            ptr.To(m.Spec.NumCores),
+		CpuFamily:        m.Spec.CPUFamily,
+		Name:             ptr.To(m.Name),
+		Ram:              ptr.To(m.Spec.MemoryMB),
+		Type:             ptr.To(m.Spec.Type.String()),
+	}
+
+	lanID, _ := strconv.ParseInt(exampleLANID, 10, 32)
+
+	entities := sdk.ServerEntities{
+		Nics: &sdk.Nics{Items: &[]sdk.Nic{{
+			Properties: &sdk.NicProperties{
+				Lan:  ptr.To(int32(lanID)),
+				Name: ptr.To(s.service.nicName(m)),
+				Dhcp: ptr.To(true),
+			},
+		}}},
+		Volumes: &sdk.AttachedVolumes{
+			Items: &[]sdk.Volume{{
+				Properties: &sdk.VolumeProperties{
+					AvailabilityZone: ptr.To(m.Spec.Disk.AvailabilityZone.String()),
+					Image:            ptr.To(m.Spec.Disk.Image.ID),
+					Name:             ptr.To(s.service.volumeName(m)),
+					Type:             ptr.To(m.Spec.Disk.DiskType.String()),
+					Size:             ptr.To(float32(m.Spec.Disk.SizeGB)),
+					UserData:         nil,
+				},
+			}},
+		},
+	}
+
+	return props, entities
+}
+
 func (s *ServiceTestSuite) mockGetIPBlocksRequestsPostCall() *clienttest.MockClient_GetRequests_Call {
 	return s.ionosClient.EXPECT().GetRequests(s.ctx, http.MethodPost, ipBlocksPath)
 }