From 4e0dca5eced74059b8b24f04029e13f64c3020d7 Mon Sep 17 00:00:00 2001 From: Cecile Robert-Michon Date: Tue, 8 Mar 2022 14:07:16 -0800 Subject: [PATCH] add azuremachinepool and azuremanagedcontrolplane controllers --- azure/interfaces.go | 11 +- azure/mock_azure/azure_mock.go | 69 ++----- azure/services/scalesets/scalesets.go | 4 +- azure/services/scalesets/scalesets_test.go | 2 +- controllers/azurecluster_reconciler.go | 4 +- controllers/azurecluster_reconciler_test.go | 34 ++-- controllers/azuremachine_reconciler.go | 4 +- controllers/azuremachine_reconciler_test.go | 28 +-- .../azuremachinepool_controller_unit_test.go | 2 +- .../azuremachinepool_reconciler.go | 42 ++--- .../azuremachinepool_reconciler_test.go | 176 ++++++++++++++++++ .../azuremachinepoolmachine_controller.go | 4 +- .../azuremanagedcontrolplane_reconciler.go | 64 +++---- 13 files changed, 282 insertions(+), 162 deletions(-) create mode 100644 exp/controllers/azuremachinepool_reconciler_test.go diff --git a/azure/interfaces.go b/azure/interfaces.go index 230fba9d90ca..a10fe0ab6c54 100644 --- a/azure/interfaces.go +++ b/azure/interfaces.go @@ -24,19 +24,16 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) -// Reconciler is a generic interface used by components offering a type of service. -// Example: virtualnetworks service would offer Reconcile/Delete methods. +// Reconciler is a generic interface for a controller reconciler which has Reconcile and Delete methods. type Reconciler interface { - Name() string Reconcile(ctx context.Context) error Delete(ctx context.Context) error } -// CredentialGetter is a Service which knows how to retrieve credentials for an Azure -// resource in a resource group. -type CredentialGetter interface { +// ServiceReconciler is an Azure service reconciler which can reconcile an Azure service. +type ServiceReconciler interface { + Name() string Reconciler - GetCredentials(ctx context.Context, group string, cluster string) ([]byte, error) } // Authorizer is an interface which can get the subscription ID, base URI, and authorizer for an Azure service. diff --git a/azure/mock_azure/azure_mock.go b/azure/mock_azure/azure_mock.go index 23ffdb568493..8c853e2faa09 100644 --- a/azure/mock_azure/azure_mock.go +++ b/azure/mock_azure/azure_mock.go @@ -67,20 +67,6 @@ func (mr *MockReconcilerMockRecorder) Delete(ctx interface{}) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*MockReconciler)(nil).Delete), ctx) } -// Name mocks base method. -func (m *MockReconciler) Name() string { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Name") - ret0, _ := ret[0].(string) - return ret0 -} - -// Name indicates an expected call of Name. -func (mr *MockReconcilerMockRecorder) Name() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Name", reflect.TypeOf((*MockReconciler)(nil).Name)) -} - // Reconcile mocks base method. func (m *MockReconciler) Reconcile(ctx context.Context) error { m.ctrl.T.Helper() @@ -95,31 +81,31 @@ func (mr *MockReconcilerMockRecorder) Reconcile(ctx interface{}) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Reconcile", reflect.TypeOf((*MockReconciler)(nil).Reconcile), ctx) } -// MockCredentialGetter is a mock of CredentialGetter interface. -type MockCredentialGetter struct { +// MockServiceReconciler is a mock of ServiceReconciler interface. +type MockServiceReconciler struct { ctrl *gomock.Controller - recorder *MockCredentialGetterMockRecorder + recorder *MockServiceReconcilerMockRecorder } -// MockCredentialGetterMockRecorder is the mock recorder for MockCredentialGetter. -type MockCredentialGetterMockRecorder struct { - mock *MockCredentialGetter +// MockServiceReconcilerMockRecorder is the mock recorder for MockServiceReconciler. +type MockServiceReconcilerMockRecorder struct { + mock *MockServiceReconciler } -// NewMockCredentialGetter creates a new mock instance. -func NewMockCredentialGetter(ctrl *gomock.Controller) *MockCredentialGetter { - mock := &MockCredentialGetter{ctrl: ctrl} - mock.recorder = &MockCredentialGetterMockRecorder{mock} +// NewMockServiceReconciler creates a new mock instance. +func NewMockServiceReconciler(ctrl *gomock.Controller) *MockServiceReconciler { + mock := &MockServiceReconciler{ctrl: ctrl} + mock.recorder = &MockServiceReconcilerMockRecorder{mock} return mock } // EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockCredentialGetter) EXPECT() *MockCredentialGetterMockRecorder { +func (m *MockServiceReconciler) EXPECT() *MockServiceReconcilerMockRecorder { return m.recorder } // Delete mocks base method. -func (m *MockCredentialGetter) Delete(ctx context.Context) error { +func (m *MockServiceReconciler) Delete(ctx context.Context) error { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Delete", ctx) ret0, _ := ret[0].(error) @@ -127,28 +113,13 @@ func (m *MockCredentialGetter) Delete(ctx context.Context) error { } // Delete indicates an expected call of Delete. -func (mr *MockCredentialGetterMockRecorder) Delete(ctx interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*MockCredentialGetter)(nil).Delete), ctx) -} - -// GetCredentials mocks base method. -func (m *MockCredentialGetter) GetCredentials(ctx context.Context, group, cluster string) ([]byte, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetCredentials", ctx, group, cluster) - ret0, _ := ret[0].([]byte) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetCredentials indicates an expected call of GetCredentials. -func (mr *MockCredentialGetterMockRecorder) GetCredentials(ctx, group, cluster interface{}) *gomock.Call { +func (mr *MockServiceReconcilerMockRecorder) Delete(ctx interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetCredentials", reflect.TypeOf((*MockCredentialGetter)(nil).GetCredentials), ctx, group, cluster) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*MockServiceReconciler)(nil).Delete), ctx) } // Name mocks base method. -func (m *MockCredentialGetter) Name() string { +func (m *MockServiceReconciler) Name() string { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Name") ret0, _ := ret[0].(string) @@ -156,13 +127,13 @@ func (m *MockCredentialGetter) Name() string { } // Name indicates an expected call of Name. -func (mr *MockCredentialGetterMockRecorder) Name() *gomock.Call { +func (mr *MockServiceReconcilerMockRecorder) Name() *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Name", reflect.TypeOf((*MockCredentialGetter)(nil).Name)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Name", reflect.TypeOf((*MockServiceReconciler)(nil).Name)) } // Reconcile mocks base method. -func (m *MockCredentialGetter) Reconcile(ctx context.Context) error { +func (m *MockServiceReconciler) Reconcile(ctx context.Context) error { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Reconcile", ctx) ret0, _ := ret[0].(error) @@ -170,9 +141,9 @@ func (m *MockCredentialGetter) Reconcile(ctx context.Context) error { } // Reconcile indicates an expected call of Reconcile. -func (mr *MockCredentialGetterMockRecorder) Reconcile(ctx interface{}) *gomock.Call { +func (mr *MockServiceReconcilerMockRecorder) Reconcile(ctx interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Reconcile", reflect.TypeOf((*MockCredentialGetter)(nil).Reconcile), ctx) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Reconcile", reflect.TypeOf((*MockServiceReconciler)(nil).Reconcile), ctx) } // MockAuthorizer is a mock of Authorizer interface. diff --git a/azure/services/scalesets/scalesets.go b/azure/services/scalesets/scalesets.go index c1a81edec452..ea0362951910 100644 --- a/azure/services/scalesets/scalesets.go +++ b/azure/services/scalesets/scalesets.go @@ -61,8 +61,8 @@ type ( } ) -// NewService creates a new service. -func NewService(scope ScaleSetScope, skuCache *resourceskus.Cache) *Service { +// New creates a new service. +func New(scope ScaleSetScope, skuCache *resourceskus.Cache) *Service { return &Service{ Client: NewClient(scope), Scope: scope, diff --git a/azure/services/scalesets/scalesets_test.go b/azure/services/scalesets/scalesets_test.go index d49591f2ed5b..2bb1e5de3718 100644 --- a/azure/services/scalesets/scalesets_test.go +++ b/azure/services/scalesets/scalesets_test.go @@ -92,7 +92,7 @@ func TestNewService(t *testing.T) { ClusterScope: s, }) g.Expect(err).NotTo(HaveOccurred()) - actual := NewService(mps, resourceskus.NewStaticCache(nil, "")) + actual := New(mps, resourceskus.NewStaticCache(nil, "")) g.Expect(actual).ToNot(BeNil()) } diff --git a/controllers/azurecluster_reconciler.go b/controllers/azurecluster_reconciler.go index 4ad6fd5a503f..2f6c229ccd95 100644 --- a/controllers/azurecluster_reconciler.go +++ b/controllers/azurecluster_reconciler.go @@ -44,7 +44,7 @@ type azureClusterService struct { 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 + services []azure.ServiceReconciler skuCache *resourceskus.Cache } @@ -56,7 +56,7 @@ func newAzureClusterService(scope *scope.ClusterScope) (*azureClusterService, er } return &azureClusterService{ scope: scope, - services: []azure.Reconciler{ + services: []azure.ServiceReconciler{ groups.New(scope), virtualnetworks.New(scope), securitygroups.New(scope), diff --git a/controllers/azurecluster_reconciler_test.go b/controllers/azurecluster_reconciler_test.go index eb6155acdd13..1eb06c3b270f 100644 --- a/controllers/azurecluster_reconciler_test.go +++ b/controllers/azurecluster_reconciler_test.go @@ -37,11 +37,11 @@ import ( 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) + expect func(one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) }{ "all services are reconciled in order": { expectedError: "", - expect: func(one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) { + expect: func(one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) { gomock.InOrder( one.Reconcile(gomockinternal.AContext()).Return(nil), two.Reconcile(gomockinternal.AContext()).Return(nil), @@ -50,7 +50,7 @@ func TestAzureClusterServiceReconcile(t *testing.T) { }, "service reconcile fails": { expectedError: "failed to reconcile AzureCluster service two: some error happened", - expect: func(one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) { + expect: func(one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) { gomock.InOrder( one.Reconcile(gomockinternal.AContext()).Return(nil), two.Reconcile(gomockinternal.AContext()).Return(errors.New("some error happened")), @@ -67,9 +67,9 @@ func TestAzureClusterServiceReconcile(t *testing.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) + svcOneMock := mock_azure.NewMockServiceReconciler(mockCtrl) + svcTwoMock := mock_azure.NewMockServiceReconciler(mockCtrl) + svcThreeMock := mock_azure.NewMockServiceReconciler(mockCtrl) tc.expect(svcOneMock.EXPECT(), svcTwoMock.EXPECT(), svcThreeMock.EXPECT()) @@ -78,7 +78,7 @@ func TestAzureClusterServiceReconcile(t *testing.T) { Cluster: &clusterv1.Cluster{}, AzureCluster: &infrav1.AzureCluster{}, }, - services: []azure.Reconciler{ + services: []azure.ServiceReconciler{ svcOneMock, svcTwoMock, svcThreeMock, @@ -100,11 +100,11 @@ func TestAzureClusterServiceReconcile(t *testing.T) { func TestAzureClusterServiceDelete(t *testing.T) { cases := map[string]struct { expectedError string - expect func(grp *mock_azure.MockReconcilerMockRecorder, one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) + expect func(grp *mock_azure.MockServiceReconcilerMockRecorder, one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) }{ "Resource Group is deleted successfully": { expectedError: "", - expect: func(grp *mock_azure.MockReconcilerMockRecorder, one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) { + expect: func(grp *mock_azure.MockServiceReconcilerMockRecorder, one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) { gomock.InOrder( grp.Name().Return(groups.ServiceName), grp.Delete(gomockinternal.AContext()).Return(nil)) @@ -112,7 +112,7 @@ func TestAzureClusterServiceDelete(t *testing.T) { }, "Resource Group delete fails": { expectedError: "failed to delete resource group: internal error", - expect: func(grp *mock_azure.MockReconcilerMockRecorder, one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) { + expect: func(grp *mock_azure.MockServiceReconcilerMockRecorder, one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) { gomock.InOrder( grp.Name().Return(groups.ServiceName), grp.Delete(gomockinternal.AContext()).Return(errors.New("internal error"))) @@ -120,7 +120,7 @@ func TestAzureClusterServiceDelete(t *testing.T) { }, "Resource Group not owned by cluster": { expectedError: "", - expect: func(grp *mock_azure.MockReconcilerMockRecorder, one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) { + expect: func(grp *mock_azure.MockServiceReconcilerMockRecorder, one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) { gomock.InOrder( grp.Name().Return(groups.ServiceName), grp.Delete(gomockinternal.AContext()).Return(azure.ErrNotOwned), @@ -131,7 +131,7 @@ func TestAzureClusterServiceDelete(t *testing.T) { }, "service delete fails": { expectedError: "failed to delete AzureCluster service two: some error happened", - expect: func(grp *mock_azure.MockReconcilerMockRecorder, one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) { + expect: func(grp *mock_azure.MockServiceReconcilerMockRecorder, one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) { gomock.InOrder( grp.Name().Return(groups.ServiceName), grp.Delete(gomockinternal.AContext()).Return(azure.ErrNotOwned), @@ -150,10 +150,10 @@ func TestAzureClusterServiceDelete(t *testing.T) { t.Parallel() mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() - groupsMock := mock_azure.NewMockReconciler(mockCtrl) - svcOneMock := mock_azure.NewMockReconciler(mockCtrl) - svcTwoMock := mock_azure.NewMockReconciler(mockCtrl) - svcThreeMock := mock_azure.NewMockReconciler(mockCtrl) + groupsMock := mock_azure.NewMockServiceReconciler(mockCtrl) + svcOneMock := mock_azure.NewMockServiceReconciler(mockCtrl) + svcTwoMock := mock_azure.NewMockServiceReconciler(mockCtrl) + svcThreeMock := mock_azure.NewMockServiceReconciler(mockCtrl) tc.expect(groupsMock.EXPECT(), svcOneMock.EXPECT(), svcTwoMock.EXPECT(), svcThreeMock.EXPECT()) @@ -161,7 +161,7 @@ func TestAzureClusterServiceDelete(t *testing.T) { scope: &scope.ClusterScope{ AzureCluster: &infrav1.AzureCluster{}, }, - services: []azure.Reconciler{ + services: []azure.ServiceReconciler{ groupsMock, svcOneMock, svcTwoMock, diff --git a/controllers/azuremachine_reconciler.go b/controllers/azuremachine_reconciler.go index 474c5f98aaea..e80accfad0d4 100644 --- a/controllers/azuremachine_reconciler.go +++ b/controllers/azuremachine_reconciler.go @@ -40,7 +40,7 @@ type azureMachineService struct { 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 + services []azure.ServiceReconciler skuCache *resourceskus.Cache } @@ -53,7 +53,7 @@ func newAzureMachineService(machineScope *scope.MachineScope) (*azureMachineServ return &azureMachineService{ scope: machineScope, - services: []azure.Reconciler{ + services: []azure.ServiceReconciler{ publicips.New(machineScope), inboundnatrules.New(machineScope), networkinterfaces.New(machineScope, cache), diff --git a/controllers/azuremachine_reconciler_test.go b/controllers/azuremachine_reconciler_test.go index e13d851b93be..54371bee8892 100644 --- a/controllers/azuremachine_reconciler_test.go +++ b/controllers/azuremachine_reconciler_test.go @@ -36,11 +36,11 @@ import ( 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) + expect func(one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) }{ "all services are reconciled in order": { expectedError: "", - expect: func(one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) { + expect: func(one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) { gomock.InOrder( one.Reconcile(gomockinternal.AContext()).Return(nil), two.Reconcile(gomockinternal.AContext()).Return(nil), @@ -49,7 +49,7 @@ func TestAzureMachineServiceReconcile(t *testing.T) { }, "service reconcile fails": { expectedError: "failed to reconcile AzureMachine service foo: some error happened", - expect: func(one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) { + expect: func(one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) { gomock.InOrder( one.Reconcile(gomockinternal.AContext()).Return(nil), two.Reconcile(gomockinternal.AContext()).Return(errors.New("some error happened")), @@ -66,9 +66,9 @@ func TestAzureMachineServiceReconcile(t *testing.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) + svcOneMock := mock_azure.NewMockServiceReconciler(mockCtrl) + svcTwoMock := mock_azure.NewMockServiceReconciler(mockCtrl) + svcThreeMock := mock_azure.NewMockServiceReconciler(mockCtrl) tc.expect(svcOneMock.EXPECT(), svcTwoMock.EXPECT(), svcThreeMock.EXPECT()) @@ -85,7 +85,7 @@ func TestAzureMachineServiceReconcile(t *testing.T) { }, }, }, - services: []azure.Reconciler{ + services: []azure.ServiceReconciler{ svcOneMock, svcTwoMock, svcThreeMock, @@ -107,11 +107,11 @@ func TestAzureMachineServiceReconcile(t *testing.T) { 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) + expect func(one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) }{ "all services deleted in order": { expectedError: "", - expect: func(one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) { + expect: func(one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) { gomock.InOrder( three.Delete(gomockinternal.AContext()).Return(nil), two.Delete(gomockinternal.AContext()).Return(nil), @@ -120,7 +120,7 @@ func TestAzureMachineServiceDelete(t *testing.T) { }, "service delete fails": { expectedError: "failed to delete AzureMachine service test-service-two: some error happened", - expect: func(one *mock_azure.MockReconcilerMockRecorder, two *mock_azure.MockReconcilerMockRecorder, three *mock_azure.MockReconcilerMockRecorder) { + expect: func(one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) { gomock.InOrder( three.Delete(gomockinternal.AContext()).Return(nil), two.Delete(gomockinternal.AContext()).Return(errors.New("some error happened")), @@ -137,9 +137,9 @@ func TestAzureMachineServiceDelete(t *testing.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) + svcOneMock := mock_azure.NewMockServiceReconciler(mockCtrl) + svcTwoMock := mock_azure.NewMockServiceReconciler(mockCtrl) + svcThreeMock := mock_azure.NewMockServiceReconciler(mockCtrl) tc.expect(svcOneMock.EXPECT(), svcTwoMock.EXPECT(), svcThreeMock.EXPECT()) @@ -152,7 +152,7 @@ func TestAzureMachineServiceDelete(t *testing.T) { Machine: &clusterv1.Machine{}, AzureMachine: &infrav1.AzureMachine{}, }, - services: []azure.Reconciler{ + services: []azure.ServiceReconciler{ svcOneMock, svcTwoMock, svcThreeMock, diff --git a/exp/controllers/azuremachinepool_controller_unit_test.go b/exp/controllers/azuremachinepool_controller_unit_test.go index 725e5f6cf551..0624a124a8f3 100644 --- a/exp/controllers/azuremachinepool_controller_unit_test.go +++ b/exp/controllers/azuremachinepool_controller_unit_test.go @@ -67,7 +67,7 @@ func Test_newAzureMachinePoolService(t *testing.T) { g := NewWithT(t) g.Expect(err).NotTo(HaveOccurred()) g.Expect(subject).NotTo(BeNil()) - g.Expect(subject.virtualMachinesScaleSetSvc).NotTo(BeNil()) + g.Expect(subject.services).NotTo(HaveLen(0)) g.Expect(subject.skuCache).NotTo(BeNil()) } diff --git a/exp/controllers/azuremachinepool_reconciler.go b/exp/controllers/azuremachinepool_reconciler.go index 5bd2e9f573eb..5221da6bcbb1 100644 --- a/exp/controllers/azuremachinepool_reconciler.go +++ b/exp/controllers/azuremachinepool_reconciler.go @@ -31,11 +31,9 @@ import ( // azureMachinePoolService is the group of services called by the AzureMachinePool controller. type azureMachinePoolService struct { - scope *scope.MachinePoolScope - virtualMachinesScaleSetSvc azure.Reconciler - skuCache *resourceskus.Cache - roleAssignmentsSvc azure.Reconciler - vmssExtensionSvc azure.Reconciler + scope *scope.MachinePoolScope + skuCache *resourceskus.Cache + services []azure.ServiceReconciler } // newAzureMachinePoolService populates all the services based on input scope. @@ -46,11 +44,13 @@ func newAzureMachinePoolService(machinePoolScope *scope.MachinePoolScope) (*azur } return &azureMachinePoolService{ - scope: machinePoolScope, - virtualMachinesScaleSetSvc: scalesets.NewService(machinePoolScope, cache), - skuCache: cache, - roleAssignmentsSvc: roleassignments.New(machinePoolScope), - vmssExtensionSvc: vmssextensions.New(machinePoolScope), + scope: machinePoolScope, + services: []azure.ServiceReconciler{ + scalesets.New(machinePoolScope, cache), + roleassignments.New(machinePoolScope), + vmssextensions.New(machinePoolScope), + }, + skuCache: cache, }, nil } @@ -63,16 +63,10 @@ func (s *azureMachinePoolService) Reconcile(ctx context.Context) error { return errors.Wrap(err, "failed defaulting subnet name") } - if err := s.virtualMachinesScaleSetSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "failed to create scale set") - } - - if err := s.roleAssignmentsSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "unable to create role assignment") - } - - if err := s.vmssExtensionSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "unable to create vmss extension") + for _, service := range s.services { + if err := service.Reconcile(ctx); err != nil { + return errors.Wrapf(err, "failed to reconcile AzureMachinePool service %s", service.Name()) + } } return nil @@ -83,8 +77,12 @@ func (s *azureMachinePoolService) Delete(ctx context.Context) error { ctx, _, done := tele.StartSpanWithLogger(ctx, "controllers.azureMachinePoolService.Delete") defer done() - if err := s.virtualMachinesScaleSetSvc.Delete(ctx); err != nil { - return errors.Wrap(err, "failed to delete scale 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.Wrapf(err, "failed to delete AzureMachinePool service %s", s.services[i].Name()) + } } + return nil } diff --git a/exp/controllers/azuremachinepool_reconciler_test.go b/exp/controllers/azuremachinepool_reconciler_test.go new file mode 100644 index 000000000000..cba52e79fd2a --- /dev/null +++ b/exp/controllers/azuremachinepool_reconciler_test.go @@ -0,0 +1,176 @@ +/* +Copyright 2022 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" + infrav1exp "sigs.k8s.io/cluster-api-provider-azure/exp/api/v1beta1" + gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + capiv1exp "sigs.k8s.io/cluster-api/exp/api/v1beta1" +) + +func TestAzureMachinePoolServiceReconcile(t *testing.T) { + cases := map[string]struct { + expectedError string + expect func(one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) + }{ + "all services are reconciled in order": { + expectedError: "", + expect: func(one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) { + 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 AzureMachinePool service foo: some error happened", + expect: func(one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) { + gomock.InOrder( + one.Reconcile(gomockinternal.AContext()).Return(nil), + two.Reconcile(gomockinternal.AContext()).Return(errors.New("some error happened")), + two.Name().Return("foo")) + }, + }, + } + + 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.NewMockServiceReconciler(mockCtrl) + svcTwoMock := mock_azure.NewMockServiceReconciler(mockCtrl) + svcThreeMock := mock_azure.NewMockServiceReconciler(mockCtrl) + + tc.expect(svcOneMock.EXPECT(), svcTwoMock.EXPECT(), svcThreeMock.EXPECT()) + + s := &azureMachinePoolService{ + scope: &scope.MachinePoolScope{ + ClusterScoper: &scope.ClusterScope{ + AzureCluster: &infrav1.AzureCluster{}, + Cluster: &clusterv1.Cluster{}, + }, + MachinePool: &capiv1exp.MachinePool{}, + AzureMachinePool: &infrav1exp.AzureMachinePool{ + Spec: infrav1exp.AzureMachinePoolSpec{ + Template: infrav1exp.AzureMachinePoolMachineTemplate{ + SubnetName: "test-subnet", + }, + }, + }, + }, + services: []azure.ServiceReconciler{ + 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 TestAzureMachinePoolServiceDelete(t *testing.T) { + cases := map[string]struct { + expectedError string + expect func(one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) + }{ + "all services deleted in order": { + expectedError: "", + expect: func(one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) { + 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 AzureMachinePool service test-service-two: some error happened", + expect: func(one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) { + gomock.InOrder( + three.Delete(gomockinternal.AContext()).Return(nil), + two.Delete(gomockinternal.AContext()).Return(errors.New("some error happened")), + two.Name().Return("test-service-two")) + }, + }, + } + + 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.NewMockServiceReconciler(mockCtrl) + svcTwoMock := mock_azure.NewMockServiceReconciler(mockCtrl) + svcThreeMock := mock_azure.NewMockServiceReconciler(mockCtrl) + + tc.expect(svcOneMock.EXPECT(), svcTwoMock.EXPECT(), svcThreeMock.EXPECT()) + + s := &azureMachinePoolService{ + scope: &scope.MachinePoolScope{ + ClusterScoper: &scope.ClusterScope{ + AzureCluster: &infrav1.AzureCluster{}, + Cluster: &clusterv1.Cluster{}, + }, + MachinePool: &capiv1exp.MachinePool{}, + AzureMachinePool: &infrav1exp.AzureMachinePool{}, + }, + services: []azure.ServiceReconciler{ + 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()) + } + }) + } +} diff --git a/exp/controllers/azuremachinepoolmachine_controller.go b/exp/controllers/azuremachinepoolmachine_controller.go index 921773ee2902..2cff54672cc9 100644 --- a/exp/controllers/azuremachinepoolmachine_controller.go +++ b/exp/controllers/azuremachinepoolmachine_controller.go @@ -48,7 +48,7 @@ import ( ) type ( - azureMachinePoolMachineReconcilerFactory func(*scope.MachinePoolMachineScope) *azureMachinePoolMachineReconciler + azureMachinePoolMachineReconcilerFactory func(*scope.MachinePoolMachineScope) azure.Reconciler // AzureMachinePoolMachineController handles Kubernetes change events for AzureMachinePoolMachine resources. AzureMachinePoolMachineController struct { @@ -339,7 +339,7 @@ func (ampmr *AzureMachinePoolMachineController) reconcileDelete(ctx context.Cont return reconcile.Result{}, nil } -func newAzureMachinePoolMachineReconciler(scope *scope.MachinePoolMachineScope) *azureMachinePoolMachineReconciler { +func newAzureMachinePoolMachineReconciler(scope *scope.MachinePoolMachineScope) azure.Reconciler { return &azureMachinePoolMachineReconciler{ Scope: scope, scalesetVMsService: scalesetvms.NewService(scope), diff --git a/exp/controllers/azuremanagedcontrolplane_reconciler.go b/exp/controllers/azuremanagedcontrolplane_reconciler.go index cb8c4aafe629..afc0b301d38a 100644 --- a/exp/controllers/azuremanagedcontrolplane_reconciler.go +++ b/exp/controllers/azuremanagedcontrolplane_reconciler.go @@ -35,25 +35,23 @@ import ( // azureManagedControlPlaneService contains the services required by the cluster controller. type azureManagedControlPlaneService struct { - kubeclient client.Client - scope managedclusters.ManagedClusterScope - managedClustersSvc azure.Reconciler - groupsSvc azure.Reconciler - vnetSvc azure.Reconciler - subnetsSvc azure.Reconciler - tagsSvc azure.Reconciler + kubeclient client.Client + scope managedclusters.ManagedClusterScope + services []azure.ServiceReconciler } // newAzureManagedControlPlaneReconciler populates all the services based on input scope. func newAzureManagedControlPlaneReconciler(scope *scope.ManagedControlPlaneScope) *azureManagedControlPlaneService { return &azureManagedControlPlaneService{ - kubeclient: scope.Client, - scope: scope, - managedClustersSvc: managedclusters.New(scope), - groupsSvc: groups.New(scope), - vnetSvc: virtualnetworks.New(scope), - subnetsSvc: subnets.New(scope), - tagsSvc: tags.New(scope), + kubeclient: scope.Client, + scope: scope, + services: []azure.ServiceReconciler{ + groups.New(scope), + virtualnetworks.New(scope), + subnets.New(scope), + managedclusters.New(scope), + tags.New(scope), + }, } } @@ -62,31 +60,16 @@ func (r *azureManagedControlPlaneService) Reconcile(ctx context.Context) error { ctx, _, done := tele.StartSpanWithLogger(ctx, "controllers.azureManagedControlPlaneService.Reconcile") defer done() - if err := r.groupsSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "failed to reconcile managed cluster resource group") - } - - if err := r.vnetSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "failed to reconcile virtual network") - } - - if err := r.subnetsSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "failed to reconcile subnet") - } - - // Send to Azure for create/update. - if err := r.managedClustersSvc.Reconcile(ctx); err != nil { - return errors.Wrapf(err, "failed to reconcile managed cluster") + for _, service := range r.services { + if err := service.Reconcile(ctx); err != nil { + return errors.Wrapf(err, "failed to reconcile AzureManagedControlPlane service %s", service.Name()) + } } if err := r.reconcileKubeconfig(ctx); err != nil { return errors.Wrap(err, "failed to reconcile kubeconfig secret") } - if err := r.tagsSvc.Reconcile(ctx); err != nil { - return errors.Wrap(err, "unable to update tags") - } - return nil } @@ -95,16 +78,11 @@ func (r *azureManagedControlPlaneService) Delete(ctx context.Context) error { ctx, _, done := tele.StartSpanWithLogger(ctx, "controllers.azureManagedControlPlaneService.Delete") defer done() - if err := r.managedClustersSvc.Delete(ctx); err != nil { - return errors.Wrapf(err, "failed to delete managed cluster") - } - - if err := r.vnetSvc.Delete(ctx); err != nil { - return errors.Wrap(err, "failed to delete virtual network") - } - - if err := r.groupsSvc.Delete(ctx); err != nil && !errors.Is(err, azure.ErrNotOwned) { - return errors.Wrap(err, "failed to delete managed cluster resource group") + // Delete services in reverse order of creation. + for i := len(r.services) - 1; i >= 0; i-- { + if err := r.services[i].Delete(ctx); err != nil { + return errors.Wrapf(err, "failed to delete AzureManagedControlPlane service %s", r.services[i].Name()) + } } return nil