From d6ac397f94cf7197e4a1443097352a458294a0c6 Mon Sep 17 00:00:00 2001 From: Jonas Riedel <138458199+jriedel-ionos@users.noreply.github.com> Date: Mon, 15 Jul 2024 13:06:56 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20Handle=20IP=20blocks=20based=20on?= =?UTF-8?q?=20VDC=20location=20(#172)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **What is the purpose of this pull request/Why do we need it?** If you specify a different VDC ID in the IonosCloudMachine spec, IP blocks are now correctly booked in this location. **Issue #, if available:** #154 **Description of changes:** * IP blocks are booked in the location specified in the IonosCloudMachine spec * Location is written into the Machine status **Special notes for your reviewer:** **Checklist:** - [ ] Documentation updated - [x] Unit Tests added - [ ] E2E Tests added - [x] Includes [emojis](https://github.com/kubernetes-sigs/kubebuilder-release-tools?tab=readme-ov-file#kubebuilder-project-versioning) --- api/v1alpha1/ionoscloudmachine_types.go | 4 ++ api/v1alpha1/ionoscloudmachine_types_test.go | 1 + ...e.cluster.x-k8s.io_ionoscloudmachines.yaml | 4 ++ internal/ionoscloud/client.go | 2 + internal/ionoscloud/client/client.go | 14 +++++ internal/ionoscloud/clienttest/mock_client.go | 57 +++++++++++++++++++ internal/service/cloud/ipblock.go | 36 +++++++++++- internal/service/cloud/ipblock_test.go | 7 +++ internal/service/cloud/network_test.go | 2 + internal/service/cloud/suite_test.go | 5 ++ 10 files changed, 129 insertions(+), 3 deletions(-) diff --git a/api/v1alpha1/ionoscloudmachine_types.go b/api/v1alpha1/ionoscloudmachine_types.go index 1658306a..5e96e94e 100644 --- a/api/v1alpha1/ionoscloudmachine_types.go +++ b/api/v1alpha1/ionoscloudmachine_types.go @@ -277,6 +277,10 @@ type IonosCloudMachineStatus struct { // cloud resource that is being provisioned. //+optional CurrentRequest *ProvisioningRequest `json:"currentRequest,omitempty"` + + // Location is the location of the datacenter the VM is provisioned in. + //+optional + Location string `json:"location"` } // MachineNetworkInfo contains information about the network configuration of the VM. diff --git a/api/v1alpha1/ionoscloudmachine_types_test.go b/api/v1alpha1/ionoscloudmachine_types_test.go index db38c274..a0b23e8d 100644 --- a/api/v1alpha1/ionoscloudmachine_types_test.go +++ b/api/v1alpha1/ionoscloudmachine_types_test.go @@ -451,6 +451,7 @@ var _ = Describe("IonosCloudMachine Tests", func() { } m.Status.FailureReason = ptr.To(errors.InvalidConfigurationMachineError) m.Status.FailureMessage = ptr.To("Failure message") + m.Status.Location = "de/fra" m.Status.MachineNetworkInfo = &MachineNetworkInfo{ NICInfo: []NICInfo{ diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_ionoscloudmachines.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_ionoscloudmachines.yaml index 8be7386e..2419c0c1 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_ionoscloudmachines.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_ionoscloudmachines.yaml @@ -317,6 +317,10 @@ spec: can be added as events to the IonosCloudMachine object and/or logged in the controller's output. type: string + location: + description: Location is the location of the datacenter the VM is + provisioned in. + type: string machineNetworkInfo: description: |- MachineNetworkInfo contains information about the network configuration of the VM. diff --git a/internal/ionoscloud/client.go b/internal/ionoscloud/client.go index 49f1fe24..5bf92ff8 100644 --- a/internal/ionoscloud/client.go +++ b/internal/ionoscloud/client.go @@ -66,4 +66,6 @@ type Client interface { GetRequests(ctx context.Context, method, path string) ([]sdk.Request, error) // PatchNIC updates the NIC identified by nicID with the provided properties, returning the request location. PatchNIC(ctx context.Context, datacenterID, serverID, nicID string, properties sdk.NicProperties) (string, error) + // GetDatacenterLocationByID returns the location of the data center identified by datacenterID. + GetDatacenterLocationByID(ctx context.Context, datacenterID string) (string, error) } diff --git a/internal/ionoscloud/client/client.go b/internal/ionoscloud/client/client.go index d6e0657a..e8ef3059 100644 --- a/internal/ionoscloud/client/client.go +++ b/internal/ionoscloud/client/client.go @@ -463,3 +463,17 @@ func validateNICParameters(datacenterID, serverID, nicID string) (err error) { return nil } + +// GetDatacenterLocationByID returns the location of the data center identified by datacenterID. +func (c *IonosCloudClient) GetDatacenterLocationByID(ctx context.Context, datacenterID string) (string, error) { + if datacenterID == "" { + return "", errDatacenterIDIsEmpty + } + + datacenter, _, err := c.API.DataCentersApi.DatacentersFindById(ctx, datacenterID).Execute() + if err != nil { + return "", fmt.Errorf(apiCallErrWrapper, err) + } + + return *datacenter.Properties.Location, nil +} diff --git a/internal/ionoscloud/clienttest/mock_client.go b/internal/ionoscloud/clienttest/mock_client.go index 48eb3f4b..011f0d07 100644 --- a/internal/ionoscloud/clienttest/mock_client.go +++ b/internal/ionoscloud/clienttest/mock_client.go @@ -455,6 +455,63 @@ func (_c *MockClient_DeleteVolume_Call) RunAndReturn(run func(context.Context, s return _c } +// GetDatacenterLocationByID provides a mock function with given fields: ctx, datacenterID +func (_m *MockClient) GetDatacenterLocationByID(ctx context.Context, datacenterID string) (string, error) { + ret := _m.Called(ctx, datacenterID) + + if len(ret) == 0 { + panic("no return value specified for GetDatacenterLocationByID") + } + + var r0 string + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, string) (string, error)); ok { + return rf(ctx, datacenterID) + } + if rf, ok := ret.Get(0).(func(context.Context, string) string); ok { + r0 = rf(ctx, datacenterID) + } else { + r0 = ret.Get(0).(string) + } + + if rf, ok := ret.Get(1).(func(context.Context, string) error); ok { + r1 = rf(ctx, datacenterID) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// MockClient_GetDatacenterLocationByID_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GetDatacenterLocationByID' +type MockClient_GetDatacenterLocationByID_Call struct { + *mock.Call +} + +// GetDatacenterLocationByID is a helper method to define mock.On call +// - ctx context.Context +// - datacenterID string +func (_e *MockClient_Expecter) GetDatacenterLocationByID(ctx interface{}, datacenterID interface{}) *MockClient_GetDatacenterLocationByID_Call { + return &MockClient_GetDatacenterLocationByID_Call{Call: _e.mock.On("GetDatacenterLocationByID", ctx, datacenterID)} +} + +func (_c *MockClient_GetDatacenterLocationByID_Call) Run(run func(ctx context.Context, datacenterID string)) *MockClient_GetDatacenterLocationByID_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(string)) + }) + return _c +} + +func (_c *MockClient_GetDatacenterLocationByID_Call) Return(_a0 string, _a1 error) *MockClient_GetDatacenterLocationByID_Call { + _c.Call.Return(_a0, _a1) + return _c +} + +func (_c *MockClient_GetDatacenterLocationByID_Call) RunAndReturn(run func(context.Context, string) (string, error)) *MockClient_GetDatacenterLocationByID_Call { + _c.Call.Return(run) + return _c +} + // GetIPBlock provides a mock function with given fields: ctx, ipBlockID func (_m *MockClient) GetIPBlock(ctx context.Context, ipBlockID string) (*ionoscloud.IpBlock, error) { ret := _m.Called(ctx, ipBlockID) diff --git a/internal/service/cloud/ipblock.go b/internal/service/cloud/ipblock.go index 44b887d0..a8df3e7e 100644 --- a/internal/service/cloud/ipblock.go +++ b/internal/service/cloud/ipblock.go @@ -237,9 +237,14 @@ func (s *Service) getFailoverIPBlock(ctx context.Context, ms *scope.Machine) (*s return nil, fmt.Errorf("failed to list IP blocks: %w", err) } + location, err := s.getLocation(ctx, ms) + if err != nil { + return nil, err + } + for _, block := range ptr.Deref(blocks.GetItems(), nil) { props := block.GetProperties() - if ptr.Deref(props.GetLocation(), "") != ms.ClusterScope.Location() { + if ptr.Deref(props.GetLocation(), "") != location { continue } if ptr.Deref(props.GetName(), "") == s.failoverIPBlockName(ms) { @@ -334,9 +339,16 @@ func (s *Service) reserveControlPlaneEndpointIPBlock(ctx context.Context, cs *sc func (s *Service) reserveMachineDeploymentFailoverIPBlock(ctx context.Context, ms *scope.Machine) error { log := s.logger.WithName("reserveMachineDeploymentFailoverIPBlock") + location, err := s.getLocation(ctx, ms) + if err != nil { + return err + } + + ms.IonosMachine.Status.Location = location + return s.reserveIPBlock( ctx, s.failoverIPBlockName(ms), - ms.ClusterScope.Location(), log, + location, log, ms.IonosMachine.SetCurrentRequest, ) } @@ -401,10 +413,14 @@ func (s *Service) getLatestControlPlaneEndpointIPBlockCreationRequest( // getLatestFailoverIPBlockCreateRequest returns the latest failover IP block creation request. func (s *Service) getLatestFailoverIPBlockCreateRequest(ctx context.Context, ms *scope.Machine) (*requestInfo, error) { + location, err := s.getLocation(ctx, ms) + if err != nil { + return nil, err + } return s.getLatestIPBlockRequestByNameAndLocation( ctx, http.MethodPost, s.failoverIPBlockName(ms), - ms.ClusterScope.Location(), + location, ) } @@ -460,3 +476,17 @@ func ignoreErrUserSetIPNotFound(err error) error { } return err } + +// getLocation checks if the location of the machine is already set in the status. +// If not, it queries Cloud API to get the location of the datacenter. +func (s *Service) getLocation(ctx context.Context, ms *scope.Machine) (string, error) { + if ms.IonosMachine.Status.Location == "" { + location, err := s.ionosClient.GetDatacenterLocationByID(ctx, ms.IonosMachine.Spec.DatacenterID) + if err != nil { + return "", fmt.Errorf("failed to get location of datacenter: %w", err) + } + ms.IonosMachine.Status.Location = location + return location, nil + } + return ms.IonosMachine.Status.Location, nil +} diff --git a/internal/service/cloud/ipblock_test.go b/internal/service/cloud/ipblock_test.go index 6b64380d..28dbcce4 100644 --- a/internal/service/cloud/ipblock_test.go +++ b/internal/service/cloud/ipblock_test.go @@ -169,6 +169,7 @@ func (s *ipBlockTestSuite) TestReserveMachineDeploymentIPBlockRequestSuccess() { } labels[clusterv1.MachineDeploymentNameLabel] = "test-deployment" + s.mockGetDatacenterLocationByIDCall(exampleDatacenterID).Return(exampleLocation, nil).Once() s.mockReserveIPBlockCall( s.service.failoverIPBlockName(s.machineScope), s.clusterScope.Location(), @@ -178,6 +179,7 @@ func (s *ipBlockTestSuite) TestReserveMachineDeploymentIPBlockRequestSuccess() { s.NoError(err) req := s.machineScope.IonosMachine.Status.CurrentRequest s.validateRequest(http.MethodPost, sdk.RequestStatusQueued, requestPath, req) + s.Equal(exampleLocation, s.machineScope.IonosMachine.Status.Location) } func (s *ipBlockTestSuite) validateRequest(method, state, path string, req *infrav1.ProvisioningRequest) { @@ -399,6 +401,7 @@ func (s *ipBlockTestSuite) TestReconcileFailoverIPBlockDeletion() { s.infraMachine.Spec.FailoverIP = ptr.To(infrav1.CloudResourceConfigAuto) ipBlock := exampleIPBlockWithName(s.service.failoverIPBlockName(s.machineScope)) + s.mockGetDatacenterLocationByIDCall(exampleDatacenterID).Return(exampleLocation, nil).Once() s.mockListIPBlocksCall().Return(&sdk.IpBlocks{Items: &[]sdk.IpBlock{*ipBlock}}, nil).Once() s.mockGetIPBlocksRequestsDeleteCall(exampleIPBlockID).Return(nil, nil).Once() s.mockDeleteIPBlockCall().Return(exampleRequestPath, nil).Once() @@ -418,6 +421,7 @@ func (s *ipBlockTestSuite) TestReconcileFailoverIPBlockDeletionSkipped() { NicUuid: nil, }} + s.mockGetDatacenterLocationByIDCall(exampleDatacenterID).Return(exampleLocation, nil).Once() s.mockListIPBlocksCall().Return(&sdk.IpBlocks{Items: &[]sdk.IpBlock{*ipBlock}}, nil).Once() s.mockGetIPBlocksRequestsDeleteCall(exampleIPBlockID).Return(nil, nil).Once() s.mockListLANsCall().Return(&sdk.Lans{Items: &[]sdk.Lan{lan}}, nil).Once() @@ -430,6 +434,7 @@ func (s *ipBlockTestSuite) TestReconcileFailoverIPBlockDeletionSkipped() { func (s *ipBlockTestSuite) TestReconcileFailoverIPBlockDeletionPendingCreation() { s.infraMachine.Spec.FailoverIP = ptr.To(infrav1.CloudResourceConfigAuto) + s.mockGetDatacenterLocationByIDCall(exampleDatacenterID).Return(exampleLocation, nil).Once() s.mockListIPBlocksCall().Return(nil, nil).Once() s.mockGetIPBlocksRequestsPostCall().Return([]sdk.Request{ s.buildIPBlockRequestWithName( @@ -446,6 +451,7 @@ func (s *ipBlockTestSuite) TestReconcileFailoverIPBlockDeletionPendingDeletion() s.infraMachine.Spec.FailoverIP = ptr.To(infrav1.CloudResourceConfigAuto) ipBlock := exampleIPBlockWithName(s.service.failoverIPBlockName(s.machineScope)) + s.mockGetDatacenterLocationByIDCall(exampleDatacenterID).Return(exampleLocation, nil).Once() s.mockListIPBlocksCall().Return(&sdk.IpBlocks{Items: &[]sdk.IpBlock{*ipBlock}}, nil).Once() deleteRequest := s.buildIPBlockRequestWithName( @@ -473,6 +479,7 @@ func (s *ipBlockTestSuite) TestReconcileFailoverIPBlockDeletionDeletionFinished( s.infraMachine.Spec.FailoverIP = ptr.To(infrav1.CloudResourceConfigAuto) ipBlock := exampleIPBlockWithName(s.service.failoverIPBlockName(s.machineScope)) + s.mockGetDatacenterLocationByIDCall(exampleDatacenterID).Return(exampleLocation, nil).Once() s.mockListIPBlocksCall().Return(&sdk.IpBlocks{Items: &[]sdk.IpBlock{*ipBlock}}, nil).Once() deleteRequest := s.buildIPBlockRequestWithName("", sdk.RequestStatusDone, http.MethodDelete, exampleIPBlockID) diff --git a/internal/service/cloud/network_test.go b/internal/service/cloud/network_test.go index fdf5ea05..2eab32cd 100644 --- a/internal/service/cloud/network_test.go +++ b/internal/service/cloud/network_test.go @@ -248,6 +248,7 @@ func (s *lanSuite) TestReconcileIPFailoverForWorkerWithAUTOSettings() { s.infraMachine.Spec.FailoverIP = ptr.To(infrav1.CloudResourceConfigAuto) testServer := s.defaultServer(s.infraMachine, exampleDHCPIP, exampleWorkerFailoverIP) testLAN := s.exampleLAN() + s.mockGetDatacenterLocationByIDCall(exampleDatacenterID).Return(exampleLocation, nil) ipBlock := s.exampleIPBlock() @@ -285,6 +286,7 @@ func (s *lanSuite) TestReconcileIPFailoverReserveIPBlock() { s.infraMachine.SetLabels(map[string]string{clusterv1.MachineDeploymentNameLabel: deploymentLabel}) s.infraMachine.Spec.FailoverIP = ptr.To(infrav1.CloudResourceConfigAuto) + s.mockGetDatacenterLocationByIDCall(exampleDatacenterID).Return(exampleLocation, nil).Once() s.mockListIPBlocksCall().Return(nil, nil).Once() s.mockGetIPBlocksRequestsPostCall().Return(nil, nil).Once() s.mockReserveIPBlockCall(s.service.failoverIPBlockName(s.machineScope), s.clusterScope.Location()). diff --git a/internal/service/cloud/suite_test.go b/internal/service/cloud/suite_test.go index 14c68c79..633e7760 100644 --- a/internal/service/cloud/suite_test.go +++ b/internal/service/cloud/suite_test.go @@ -73,6 +73,7 @@ const ( exampleSecondaryServerID = "dd426c63-cd1d-4c02-aca3-13b4a27c2ebd" exampleRequestPath = "/test" exampleLocation = "de/txl" + exampleDatacenterID = "ccf27092-34e8-499e-a2f5-2bdee9d34a12" ) type ServiceTestSuite struct { @@ -370,3 +371,7 @@ func (s *ServiceTestSuite) mockGetServerCall(serverID string) *clienttest.MockCl func (s *ServiceTestSuite) mockListLANsCall() *clienttest.MockClient_ListLANs_Call { return s.ionosClient.EXPECT().ListLANs(s.ctx, s.machineScope.DatacenterID()) } + +func (s *ServiceTestSuite) mockGetDatacenterLocationByIDCall(datacenterID string) *clienttest.MockClient_GetDatacenterLocationByID_Call { + return s.ionosClient.EXPECT().GetDatacenterLocationByID(s.ctx, datacenterID) +}