From 3cc357c4e5b8610b137cb4c5b79a31c0711d9bbe Mon Sep 17 00:00:00 2001 From: Cecile Robert-Michon Date: Wed, 2 Mar 2022 16:05:12 -0800 Subject: [PATCH] Use a list for Azure services in AzureCluster and AzureMachine reconcilers --- controllers/azurecluster_reconciler.go | 146 ++++------------- controllers/azurecluster_reconciler_test.go | 151 +++++++++-------- controllers/azuremachine_reconciler.go | 101 ++++-------- controllers/azuremachine_reconciler_test.go | 170 ++++++++++++++++++++ 4 files changed, 316 insertions(+), 252 deletions(-) create mode 100644 controllers/azuremachine_reconciler_test.go diff --git a/controllers/azurecluster_reconciler.go b/controllers/azurecluster_reconciler.go index 26e6d5b5063..8aa16141897 100644 --- a/controllers/azurecluster_reconciler.go +++ b/controllers/azurecluster_reconciler.go @@ -41,20 +41,11 @@ import ( // azureClusterService is the reconciler called by the AzureCluster controller. type azureClusterService struct { - scope *scope.ClusterScope - groupsSvc azure.Reconciler - vnetSvc azure.Reconciler - securityGroupSvc azure.Reconciler - routeTableSvc azure.Reconciler - subnetsSvc azure.Reconciler - publicIPSvc azure.Reconciler - loadBalancerSvc azure.Reconciler - privateDNSSvc azure.Reconciler - bastionSvc azure.Reconciler - skuCache *resourceskus.Cache - natGatewaySvc azure.Reconciler - peeringsSvc azure.Reconciler - tagsSvc azure.Reconciler + scope *scope.ClusterScope + // services is the list of services that are reconciled by this controller. + // The order of the services is important as it determines the order in which the services are reconciled. + services []azure.Reconciler + skuCache *resourceskus.Cache } // newAzureClusterService populates all the services based on input scope. @@ -63,22 +54,23 @@ func newAzureClusterService(scope *scope.ClusterScope) (*azureClusterService, er if err != nil { return nil, errors.Wrap(err, "failed creating a NewCache") } - return &azureClusterService{ - scope: scope, - groupsSvc: groups.New(scope), - vnetSvc: virtualnetworks.New(scope), - securityGroupSvc: securitygroups.New(scope), - routeTableSvc: routetables.New(scope), - natGatewaySvc: natgateways.New(scope), - subnetsSvc: subnets.New(scope), - publicIPSvc: publicips.New(scope), - loadBalancerSvc: loadbalancers.New(scope), - privateDNSSvc: privatedns.New(scope), - bastionSvc: bastionhosts.New(scope), - skuCache: skuCache, - peeringsSvc: vnetpeerings.New(scope), - tagsSvc: tags.New(scope), + scope: scope, + services: []azure.Reconciler{ + groups.New(scope), + virtualnetworks.New(scope), + securitygroups.New(scope), + routetables.New(scope), + publicips.New(scope), + natgateways.New(scope), + subnets.New(scope), + vnetpeerings.New(scope), + loadbalancers.New(scope), + privatedns.New(scope), + bastionhosts.New(scope), + tags.New(scope), + }, + skuCache: skuCache, }, nil } @@ -96,52 +88,10 @@ func (s *azureClusterService) Reconcile(ctx context.Context) error { s.scope.SetDNSName() s.scope.SetControlPlaneSecurityRules() - if err := s.groupsSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "failed to reconcile resource group") - } - - if err := s.vnetSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "failed to reconcile virtual network") - } - - if err := s.securityGroupSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "failed to reconcile network security group") - } - - if err := s.routeTableSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "failed to reconcile route table") - } - - if err := s.publicIPSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "failed to reconcile public IP") - } - - if err := s.natGatewaySvc.Reconcile(ctx); err != nil { - return errors.Wrapf(err, "failed to reconcile NAT gateway") - } - - if err := s.subnetsSvc.Reconcile(ctx); err != nil { - return errors.Wrapf(err, "failed to reconcile subnet") - } - - if err := s.peeringsSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "failed to reconcile peerings") - } - - if err := s.loadBalancerSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "failed to reconcile load balancer") - } - - if err := s.privateDNSSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "failed to reconcile private dns") - } - - if err := s.bastionSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "failed to reconcile bastion") - } - - if err := s.tagsSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "unable to update tags") + for _, service := range s.services { + if err := service.Reconcile(ctx); err != nil { + return errors.Wrap(err, "failed to reconcile AzureCluster service") + } } return nil @@ -152,46 +102,14 @@ func (s *azureClusterService) Delete(ctx context.Context) error { ctx, _, done := tele.StartSpanWithLogger(ctx, "controllers.azureClusterService.Delete") defer done() - if err := s.groupsSvc.Delete(ctx); err != nil { + if err := s.services[0].Delete(ctx); err != nil { if errors.Is(err, azure.ErrNotOwned) { - if err := s.bastionSvc.Delete(ctx); err != nil { - return errors.Wrap(err, "failed to delete bastion") - } - - if err := s.privateDNSSvc.Delete(ctx); err != nil { - return errors.Wrap(err, "failed to delete private dns") - } - - if err := s.loadBalancerSvc.Delete(ctx); err != nil { - return errors.Wrap(err, "failed to delete load balancer") - } - - if err := s.peeringsSvc.Delete(ctx); err != nil { - return errors.Wrap(err, "failed to delete peerings") - } - - if err := s.subnetsSvc.Delete(ctx); err != nil { - return errors.Wrap(err, "failed to delete subnet") - } - - if err := s.natGatewaySvc.Delete(ctx); err != nil { - return errors.Wrapf(err, "failed to delete NAT gateway") - } - - if err := s.publicIPSvc.Delete(ctx); err != nil { - return errors.Wrapf(err, "failed to delete public IP") - } - - if err := s.routeTableSvc.Delete(ctx); err != nil { - return errors.Wrap(err, "failed to delete route table") - } - - if err := s.securityGroupSvc.Delete(ctx); err != nil { - return errors.Wrap(err, "failed to delete network security group") - } - - if err := s.vnetSvc.Delete(ctx); err != nil { - return errors.Wrap(err, "failed to delete virtual network") + // If the resource group is not managed we need to delete resources inside the group one by one. + // services are deleted in reverse order from the order in which they are reconciled. + for i := len(s.services) - 1; i >= 1; i-- { + if err := s.services[i].Delete(ctx); err != nil { + return errors.Wrap(err, "failed to delete AzureCluster service") + } } } else { return errors.Wrap(err, "failed to delete resource group") diff --git a/controllers/azurecluster_reconciler_test.go b/controllers/azurecluster_reconciler_test.go index 58e6a42857a..0c19efea599 100644 --- a/controllers/azurecluster_reconciler_test.go +++ b/controllers/azurecluster_reconciler_test.go @@ -30,72 +30,107 @@ import ( "sigs.k8s.io/cluster-api-provider-azure/azure/scope" "sigs.k8s.io/cluster-api-provider-azure/azure/services/resourceskus" gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) -type expect func(grp *mock_azure.MockReconcilerMockRecorder, vnet *mock_azure.MockReconcilerMockRecorder, sg *mock_azure.MockReconcilerMockRecorder, rt *mock_azure.MockReconcilerMockRecorder, sn *mock_azure.MockReconcilerMockRecorder, natg *mock_azure.MockReconcilerMockRecorder, pip *mock_azure.MockReconcilerMockRecorder, lb *mock_azure.MockReconcilerMockRecorder, dns *mock_azure.MockReconcilerMockRecorder, bastion *mock_azure.MockReconcilerMockRecorder, peer *mock_azure.MockReconcilerMockRecorder) +func TestAzureClusterServiceReconcile(t *testing.T) { + cases := map[string]struct { + expectedError string + expect func(one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) + }{ + "all services are reconciled in order": { + expectedError: "", + expect: func(one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) { + gomock.InOrder( + one.Reconcile(gomockinternal.AContext()).Return(nil), + two.Reconcile(gomockinternal.AContext()).Return(nil), + three.Reconcile(gomockinternal.AContext()).Return(nil)) + }, + }, + "service reconcile fails": { + expectedError: "failed to reconcile AzureCluster service: some error happened", + expect: func(one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) { + gomock.InOrder( + one.Reconcile(gomockinternal.AContext()).Return(nil), + two.Reconcile(gomockinternal.AContext()).Return(errors.New("some error happened"))) + }, + }, + } + + for name, tc := range cases { + tc := tc + t.Run(name, func(t *testing.T) { + g := NewWithT(t) + + t.Parallel() + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + svcOneMock := mock_azure.NewMockReconciler(mockCtrl) + svcTwoMock := mock_azure.NewMockReconciler(mockCtrl) + svcThreeMock := mock_azure.NewMockReconciler(mockCtrl) + + tc.expect(svcOneMock.EXPECT(), svcTwoMock.EXPECT(), svcThreeMock.EXPECT()) + + s := &azureClusterService{ + scope: &scope.ClusterScope{ + Cluster: &clusterv1.Cluster{}, + AzureCluster: &infrav1.AzureCluster{}, + }, + services: []azure.Reconciler{ + svcOneMock, + svcTwoMock, + svcThreeMock, + }, + skuCache: resourceskus.NewStaticCache([]compute.ResourceSku{}, ""), + } + + err := s.Reconcile(context.TODO()) + if tc.expectedError != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err).To(MatchError(tc.expectedError)) + } else { + g.Expect(err).NotTo(HaveOccurred()) + } + }) + } +} -func TestAzureClusterReconcilerDelete(t *testing.T) { +func TestAzureClusterServiceDelete(t *testing.T) { cases := map[string]struct { expectedError string - expect expect + expect func(grp *mock_azure.MockReconcilerMockRecorder, one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) }{ "Resource Group is deleted successfully": { expectedError: "", - expect: func(grp *mock_azure.MockReconcilerMockRecorder, vnet *mock_azure.MockReconcilerMockRecorder, sg *mock_azure.MockReconcilerMockRecorder, rt *mock_azure.MockReconcilerMockRecorder, sn *mock_azure.MockReconcilerMockRecorder, natg *mock_azure.MockReconcilerMockRecorder, pip *mock_azure.MockReconcilerMockRecorder, lb *mock_azure.MockReconcilerMockRecorder, dns *mock_azure.MockReconcilerMockRecorder, bastion *mock_azure.MockReconcilerMockRecorder, peer *mock_azure.MockReconcilerMockRecorder) { + expect: func(grp *mock_azure.MockReconcilerMockRecorder, one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) { gomock.InOrder( grp.Delete(gomockinternal.AContext()).Return(nil)) }, }, "Resource Group delete fails": { expectedError: "failed to delete resource group: internal error", - expect: func(grp *mock_azure.MockReconcilerMockRecorder, vnet *mock_azure.MockReconcilerMockRecorder, sg *mock_azure.MockReconcilerMockRecorder, rt *mock_azure.MockReconcilerMockRecorder, sn *mock_azure.MockReconcilerMockRecorder, natg *mock_azure.MockReconcilerMockRecorder, pip *mock_azure.MockReconcilerMockRecorder, lb *mock_azure.MockReconcilerMockRecorder, dns *mock_azure.MockReconcilerMockRecorder, bastion *mock_azure.MockReconcilerMockRecorder, peer *mock_azure.MockReconcilerMockRecorder) { + expect: func(grp *mock_azure.MockReconcilerMockRecorder, one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) { gomock.InOrder( grp.Delete(gomockinternal.AContext()).Return(errors.New("internal error"))) }, }, "Resource Group not owned by cluster": { expectedError: "", - expect: func(grp *mock_azure.MockReconcilerMockRecorder, vnet *mock_azure.MockReconcilerMockRecorder, sg *mock_azure.MockReconcilerMockRecorder, rt *mock_azure.MockReconcilerMockRecorder, sn *mock_azure.MockReconcilerMockRecorder, natg *mock_azure.MockReconcilerMockRecorder, pip *mock_azure.MockReconcilerMockRecorder, lb *mock_azure.MockReconcilerMockRecorder, dns *mock_azure.MockReconcilerMockRecorder, bastion *mock_azure.MockReconcilerMockRecorder, peer *mock_azure.MockReconcilerMockRecorder) { + expect: func(grp *mock_azure.MockReconcilerMockRecorder, one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) { gomock.InOrder( grp.Delete(gomockinternal.AContext()).Return(azure.ErrNotOwned), - bastion.Delete(gomockinternal.AContext()), - dns.Delete(gomockinternal.AContext()), - lb.Delete(gomockinternal.AContext()), - peer.Delete(gomockinternal.AContext()), - sn.Delete(gomockinternal.AContext()), - natg.Delete(gomockinternal.AContext()), - pip.Delete(gomockinternal.AContext()), - rt.Delete(gomockinternal.AContext()), - sg.Delete(gomockinternal.AContext()), - vnet.Delete(gomockinternal.AContext()), - ) + three.Delete(gomockinternal.AContext()).Return(nil), + two.Delete(gomockinternal.AContext()).Return(nil), + one.Delete(gomockinternal.AContext()).Return(nil)) }, }, - "Load Balancer delete fails": { - expectedError: "failed to delete load balancer: some error happened", - expect: func(grp *mock_azure.MockReconcilerMockRecorder, vnet *mock_azure.MockReconcilerMockRecorder, sg *mock_azure.MockReconcilerMockRecorder, rt *mock_azure.MockReconcilerMockRecorder, sn *mock_azure.MockReconcilerMockRecorder, pip *mock_azure.MockReconcilerMockRecorder, natg *mock_azure.MockReconcilerMockRecorder, lb *mock_azure.MockReconcilerMockRecorder, dns *mock_azure.MockReconcilerMockRecorder, bastion *mock_azure.MockReconcilerMockRecorder, peer *mock_azure.MockReconcilerMockRecorder) { + "service delete fails": { + expectedError: "failed to delete AzureCluster service: some error happened", + expect: func(grp *mock_azure.MockReconcilerMockRecorder, one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) { gomock.InOrder( grp.Delete(gomockinternal.AContext()).Return(azure.ErrNotOwned), - bastion.Delete(gomockinternal.AContext()), - dns.Delete(gomockinternal.AContext()), - lb.Delete(gomockinternal.AContext()).Return(errors.New("some error happened")), - ) - }, - }, - "Route table delete fails": { - expectedError: "failed to delete route table: some error happened", - expect: func(grp *mock_azure.MockReconcilerMockRecorder, vnet *mock_azure.MockReconcilerMockRecorder, sg *mock_azure.MockReconcilerMockRecorder, rt *mock_azure.MockReconcilerMockRecorder, sn *mock_azure.MockReconcilerMockRecorder, pip *mock_azure.MockReconcilerMockRecorder, natg *mock_azure.MockReconcilerMockRecorder, lb *mock_azure.MockReconcilerMockRecorder, dns *mock_azure.MockReconcilerMockRecorder, bastion *mock_azure.MockReconcilerMockRecorder, peer *mock_azure.MockReconcilerMockRecorder) { - gomock.InOrder( - grp.Delete(gomockinternal.AContext()).Return(azure.ErrNotOwned), - bastion.Delete(gomockinternal.AContext()), - dns.Delete(gomockinternal.AContext()), - lb.Delete(gomockinternal.AContext()), - peer.Delete(gomockinternal.AContext()), - sn.Delete(gomockinternal.AContext()), - pip.Delete(gomockinternal.AContext()), - natg.Delete(gomockinternal.AContext()), - rt.Delete(gomockinternal.AContext()).Return(errors.New("some error happened")), - ) + three.Delete(gomockinternal.AContext()).Return(nil), + two.Delete(gomockinternal.AContext()).Return(errors.New("some error happened"))) }, }, } @@ -109,35 +144,23 @@ func TestAzureClusterReconcilerDelete(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() groupsMock := mock_azure.NewMockReconciler(mockCtrl) - vnetMock := mock_azure.NewMockReconciler(mockCtrl) - sgMock := mock_azure.NewMockReconciler(mockCtrl) - rtMock := mock_azure.NewMockReconciler(mockCtrl) - subnetsMock := mock_azure.NewMockReconciler(mockCtrl) - natGatewaysMock := mock_azure.NewMockReconciler(mockCtrl) - publicIPMock := mock_azure.NewMockReconciler(mockCtrl) - lbMock := mock_azure.NewMockReconciler(mockCtrl) - dnsMock := mock_azure.NewMockReconciler(mockCtrl) - bastionMock := mock_azure.NewMockReconciler(mockCtrl) - peeringsMock := mock_azure.NewMockReconciler(mockCtrl) - - tc.expect(groupsMock.EXPECT(), vnetMock.EXPECT(), sgMock.EXPECT(), rtMock.EXPECT(), subnetsMock.EXPECT(), natGatewaysMock.EXPECT(), publicIPMock.EXPECT(), lbMock.EXPECT(), dnsMock.EXPECT(), bastionMock.EXPECT(), peeringsMock.EXPECT()) + svcOneMock := mock_azure.NewMockReconciler(mockCtrl) + svcTwoMock := mock_azure.NewMockReconciler(mockCtrl) + svcThreeMock := mock_azure.NewMockReconciler(mockCtrl) + + tc.expect(groupsMock.EXPECT(), svcOneMock.EXPECT(), svcTwoMock.EXPECT(), svcThreeMock.EXPECT()) s := &azureClusterService{ scope: &scope.ClusterScope{ AzureCluster: &infrav1.AzureCluster{}, }, - groupsSvc: groupsMock, - vnetSvc: vnetMock, - securityGroupSvc: sgMock, - routeTableSvc: rtMock, - natGatewaySvc: natGatewaysMock, - subnetsSvc: subnetsMock, - publicIPSvc: publicIPMock, - loadBalancerSvc: lbMock, - privateDNSSvc: dnsMock, - bastionSvc: bastionMock, - peeringsSvc: peeringsMock, - skuCache: resourceskus.NewStaticCache([]compute.ResourceSku{}, ""), + services: []azure.Reconciler{ + groupsMock, + svcOneMock, + svcTwoMock, + svcThreeMock, + }, + skuCache: resourceskus.NewStaticCache([]compute.ResourceSku{}, ""), } err := s.Delete(context.TODO()) diff --git a/controllers/azuremachine_reconciler.go b/controllers/azuremachine_reconciler.go index 456f818036a..08c4ee1ec3d 100644 --- a/controllers/azuremachine_reconciler.go +++ b/controllers/azuremachine_reconciler.go @@ -37,17 +37,11 @@ import ( // azureMachineService is the group of services called by the AzureMachine controller. type azureMachineService struct { - scope *scope.MachineScope - networkInterfacesSvc azure.Reconciler - inboundNatRulesSvc azure.Reconciler - virtualMachinesSvc azure.Reconciler - roleAssignmentsSvc azure.Reconciler - disksSvc azure.Reconciler - publicIPsSvc azure.Reconciler - tagsSvc azure.Reconciler - vmExtensionsSvc azure.Reconciler - availabilitySetsSvc azure.Reconciler - skuCache *resourceskus.Cache + scope *scope.MachineScope + // services is the list of services to be reconciled. + // The order of the services is important as it determines the order in which the services are reconciled. + services []azure.Reconciler + skuCache *resourceskus.Cache } var _ azure.Reconciler = (*azureMachineService)(nil) @@ -60,17 +54,19 @@ func newAzureMachineService(machineScope *scope.MachineScope) (*azureMachineServ } return &azureMachineService{ - scope: machineScope, - inboundNatRulesSvc: inboundnatrules.New(machineScope), - networkInterfacesSvc: networkinterfaces.New(machineScope, cache), - virtualMachinesSvc: virtualmachines.New(machineScope), - roleAssignmentsSvc: roleassignments.New(machineScope), - disksSvc: disks.New(machineScope), - publicIPsSvc: publicips.New(machineScope), - tagsSvc: tags.New(machineScope), - vmExtensionsSvc: vmextensions.New(machineScope), - availabilitySetsSvc: availabilitysets.New(machineScope, cache), - skuCache: cache, + scope: machineScope, + services: []azure.Reconciler{ + publicips.New(machineScope), + inboundnatrules.New(machineScope), + networkinterfaces.New(machineScope, cache), + availabilitysets.New(machineScope, cache), + disks.New(machineScope), + virtualmachines.New(machineScope), + roleassignments.New(machineScope), + vmextensions.New(machineScope), + tags.New(machineScope), + }, + skuCache: cache, }, nil } @@ -83,36 +79,10 @@ func (s *azureMachineService) Reconcile(ctx context.Context) error { return errors.Wrap(err, "failed defaulting subnet name") } - if err := s.publicIPsSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "failed to create public IP") - } - - if err := s.inboundNatRulesSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "failed to create inbound NAT rule") - } - - if err := s.networkInterfacesSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "failed to create network interface") - } - - if err := s.availabilitySetsSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "failed to create availability set") - } - - if err := s.virtualMachinesSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "failed to create virtual machine") - } - - if err := s.roleAssignmentsSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "unable to create role assignment") - } - - if err := s.vmExtensionsSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "unable to create vm extension") - } - - if err := s.tagsSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "unable to update tags") + for _, service := range s.services { + if err := service.Reconcile(ctx); err != nil { + return errors.Wrap(err, "failed to reconcile AzureMachine service") + } } return nil @@ -123,28 +93,11 @@ func (s *azureMachineService) Delete(ctx context.Context) error { ctx, _, done := tele.StartSpanWithLogger(ctx, "controllers.azureMachineService.Delete") defer done() - if err := s.virtualMachinesSvc.Delete(ctx); err != nil { - return errors.Wrap(err, "failed to delete machine") - } - - if err := s.networkInterfacesSvc.Delete(ctx); err != nil { - return errors.Wrap(err, "failed to delete network interface") - } - - if err := s.inboundNatRulesSvc.Delete(ctx); err != nil { - return errors.Wrap(err, "failed to delete inbound NAT rule") - } - - if err := s.publicIPsSvc.Delete(ctx); err != nil { - return errors.Wrap(err, "failed to delete public IPs") - } - - if err := s.disksSvc.Delete(ctx); err != nil { - return errors.Wrap(err, "failed to delete OS disk") - } - - if err := s.availabilitySetsSvc.Delete(ctx); err != nil { - return errors.Wrap(err, "failed to delete availability set") + // Delete services in reverse order of creation. + for i := len(s.services) - 1; i >= 0; i-- { + if err := s.services[i].Delete(ctx); err != nil { + return errors.Wrap(err, "failed to delete AzureMachine service") + } } return nil diff --git a/controllers/azuremachine_reconciler_test.go b/controllers/azuremachine_reconciler_test.go new file mode 100644 index 00000000000..cdb72fa0901 --- /dev/null +++ b/controllers/azuremachine_reconciler_test.go @@ -0,0 +1,170 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controllers + +import ( + "context" + "errors" + "testing" + + "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-04-01/compute" + "github.com/golang/mock/gomock" + . "github.com/onsi/gomega" + infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" + "sigs.k8s.io/cluster-api-provider-azure/azure" + "sigs.k8s.io/cluster-api-provider-azure/azure/mock_azure" + "sigs.k8s.io/cluster-api-provider-azure/azure/scope" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/resourceskus" + gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" +) + +func TestAzureMachineServiceReconcile(t *testing.T) { + cases := map[string]struct { + expectedError string + expect func(one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) + }{ + "all services are reconciled in order": { + expectedError: "", + expect: func(one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) { + gomock.InOrder( + one.Reconcile(gomockinternal.AContext()).Return(nil), + two.Reconcile(gomockinternal.AContext()).Return(nil), + three.Reconcile(gomockinternal.AContext()).Return(nil)) + }, + }, + "service reconcile fails": { + expectedError: "failed to reconcile AzureMachine service: some error happened", + expect: func(one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) { + gomock.InOrder( + one.Reconcile(gomockinternal.AContext()).Return(nil), + two.Reconcile(gomockinternal.AContext()).Return(errors.New("some error happened"))) + }, + }, + } + + for name, tc := range cases { + tc := tc + t.Run(name, func(t *testing.T) { + g := NewWithT(t) + + t.Parallel() + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + svcOneMock := mock_azure.NewMockReconciler(mockCtrl) + svcTwoMock := mock_azure.NewMockReconciler(mockCtrl) + svcThreeMock := mock_azure.NewMockReconciler(mockCtrl) + + tc.expect(svcOneMock.EXPECT(), svcTwoMock.EXPECT(), svcThreeMock.EXPECT()) + + s := &azureMachineService{ + scope: &scope.MachineScope{ + ClusterScoper: &scope.ClusterScope{ + AzureCluster: &infrav1.AzureCluster{}, + Cluster: &clusterv1.Cluster{}, + }, + Machine: &clusterv1.Machine{}, + AzureMachine: &infrav1.AzureMachine{ + Spec: infrav1.AzureMachineSpec{ + SubnetName: "test-subnet", + }, + }, + }, + services: []azure.Reconciler{ + svcOneMock, + svcTwoMock, + svcThreeMock, + }, + skuCache: resourceskus.NewStaticCache([]compute.ResourceSku{}, ""), + } + + err := s.Reconcile(context.TODO()) + if tc.expectedError != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err).To(MatchError(tc.expectedError)) + } else { + g.Expect(err).NotTo(HaveOccurred()) + } + }) + } +} + +func TestAzureMachineServiceDelete(t *testing.T) { + cases := map[string]struct { + expectedError string + expect func(one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) + }{ + "all services deleted in order": { + expectedError: "", + expect: func(one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) { + gomock.InOrder( + three.Delete(gomockinternal.AContext()).Return(nil), + two.Delete(gomockinternal.AContext()).Return(nil), + one.Delete(gomockinternal.AContext()).Return(nil)) + }, + }, + "service delete fails": { + expectedError: "failed to delete AzureMachine service: some error happened", + expect: func(one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) { + gomock.InOrder( + three.Delete(gomockinternal.AContext()).Return(nil), + two.Delete(gomockinternal.AContext()).Return(errors.New("some error happened"))) + }, + }, + } + + for name, tc := range cases { + tc := tc + t.Run(name, func(t *testing.T) { + g := NewWithT(t) + + t.Parallel() + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + svcOneMock := mock_azure.NewMockReconciler(mockCtrl) + svcTwoMock := mock_azure.NewMockReconciler(mockCtrl) + svcThreeMock := mock_azure.NewMockReconciler(mockCtrl) + + tc.expect(svcOneMock.EXPECT(), svcTwoMock.EXPECT(), svcThreeMock.EXPECT()) + + s := &azureMachineService{ + scope: &scope.MachineScope{ + ClusterScoper: &scope.ClusterScope{ + AzureCluster: &infrav1.AzureCluster{}, + Cluster: &clusterv1.Cluster{}, + }, + Machine: &clusterv1.Machine{}, + AzureMachine: &infrav1.AzureMachine{}, + }, + services: []azure.Reconciler{ + svcOneMock, + svcTwoMock, + svcThreeMock, + }, + skuCache: resourceskus.NewStaticCache([]compute.ResourceSku{}, ""), + } + + err := s.Delete(context.TODO()) + if tc.expectedError != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err).To(MatchError(tc.expectedError)) + } else { + g.Expect(err).NotTo(HaveOccurred()) + } + }) + } +}