Skip to content

Commit

Permalink
Use a list for Azure services in AzureCluster and AzureMachine reconc…
Browse files Browse the repository at this point in the history 
…ilers
  • Loading branch information
Cecile Robert-Michon committed Mar 4, 2022
1 parent d0b9621 commit 3cc357c
Show file tree
Hide file tree
Showing 4 changed files with 316 additions and 252 deletions.
146 changes: 32 additions & 114 deletions controllers/azurecluster_reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
}

Expand All @@ -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
Expand All @@ -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")
Expand Down
151 changes: 87 additions & 64 deletions controllers/azurecluster_reconciler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")))
},
},
}
Expand All @@ -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())
Expand Down
Loading

0 comments on commit 3cc357c

Please sign in to comment.