From 52482df58152ad77e146ddea575aa34ee73b777a Mon Sep 17 00:00:00 2001 From: lubedacht <132355999+lubedacht@users.noreply.github.com> Date: Thu, 4 Jul 2024 09:50:46 +0200 Subject: [PATCH] :seedling: 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 <138458199+jriedel-ionos@users.noreply.github.com> --- api/v1alpha1/ionoscloudcluster_types_test.go | 6 +++ internal/service/cloud/ipblock.go | 8 ++-- internal/service/cloud/ipblock_test.go | 31 +++++++++++++ internal/service/cloud/network_test.go | 47 +++++++++++++++++--- internal/service/cloud/server_test.go | 31 ++++++++----- internal/service/cloud/suite_test.go | 43 +++++++++++++++++- 6 files changed, 143 insertions(+), 23 deletions(-) diff --git a/api/v1alpha1/ionoscloudcluster_types_test.go b/api/v1alpha1/ionoscloudcluster_types_test.go index 447a6407..7bab6cf0 100644 --- a/api/v1alpha1/ionoscloudcluster_types_test.go +++ b/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() { diff --git a/internal/service/cloud/ipblock.go b/internal/service/cloud/ipblock.go index 6a2ca3a4..44b887d0 100644 --- a/internal/service/cloud/ipblock.go +++ b/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) { diff --git a/internal/service/cloud/ipblock_test.go b/internal/service/cloud/ipblock_test.go index 1dc5436d..6b64380d 100644 --- a/internal/service/cloud/ipblock_test.go +++ b/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) } diff --git a/internal/service/cloud/network_test.go b/internal/service/cloud/network_test.go index 4e61d5c6..fdf5ea05 100644 --- a/internal/service/cloud/network_test.go +++ b/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) diff --git a/internal/service/cloud/server_test.go b/internal/service/cloud/server_test.go index 96bed245..8db3f89a 100644 --- a/internal/service/cloud/server_test.go +++ b/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) == "" } } diff --git a/internal/service/cloud/suite_test.go b/internal/service/cloud/suite_test.go index 20fa68fc..14c68c79 100644 --- a/internal/service/cloud/suite_test.go +++ b/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) }