From 12f29a9bca74635dbb1a92a50613a722324151c6 Mon Sep 17 00:00:00 2001 From: Cecile Robert-Michon Date: Wed, 4 Oct 2023 21:02:22 +0000 Subject: [PATCH] Separate IsManaged into its own interface Co-authored-by: Jon Huhn --- azure/interfaces.go | 6 ++- azure/mock_azure/azure_mock.go | 53 +++++++++++++++------ controllers/azurecluster_reconciler.go | 2 +- controllers/azurecluster_reconciler_test.go | 48 +++++++++++-------- controllers/helpers.go | 2 +- 5 files changed, 73 insertions(+), 38 deletions(-) diff --git a/azure/interfaces.go b/azure/interfaces.go index 17c729eb82c..7f1a9b93fda 100644 --- a/azure/interfaces.go +++ b/azure/interfaces.go @@ -41,10 +41,14 @@ type Pauser interface { // ServiceReconciler is an Azure service reconciler which can reconcile an Azure service. type ServiceReconciler interface { Name() string - IsManaged(ctx context.Context) (bool, error) Reconciler } +// BYOResourceReconciler is an interface for a reconciler that can determine if a resource is managed by CAPZ. +type BYOResourceReconciler interface { + IsManaged(context.Context) (bool, error) +} + // Authorizer is an interface which can get details such as subscription ID, base URI, and token // for authorizing to an Azure service. type Authorizer interface { diff --git a/azure/mock_azure/azure_mock.go b/azure/mock_azure/azure_mock.go index 109d2b7ec1d..6427a914057 100644 --- a/azure/mock_azure/azure_mock.go +++ b/azure/mock_azure/azure_mock.go @@ -162,21 +162,6 @@ func (mr *MockServiceReconcilerMockRecorder) Delete(ctx any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*MockServiceReconciler)(nil).Delete), ctx) } -// IsManaged mocks base method. -func (m *MockServiceReconciler) IsManaged(ctx context.Context) (bool, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "IsManaged", ctx) - ret0, _ := ret[0].(bool) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// IsManaged indicates an expected call of IsManaged. -func (mr *MockServiceReconcilerMockRecorder) IsManaged(ctx any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsManaged", reflect.TypeOf((*MockServiceReconciler)(nil).IsManaged), ctx) -} - // Name mocks base method. func (m *MockServiceReconciler) Name() string { m.ctrl.T.Helper() @@ -205,6 +190,44 @@ func (mr *MockServiceReconcilerMockRecorder) Reconcile(ctx any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Reconcile", reflect.TypeOf((*MockServiceReconciler)(nil).Reconcile), ctx) } +// MockBYOResourceReconciler is a mock of BYOResourceReconciler interface. +type MockBYOResourceReconciler struct { + ctrl *gomock.Controller + recorder *MockBYOResourceReconcilerMockRecorder +} + +// MockBYOResourceReconcilerMockRecorder is the mock recorder for MockBYOResourceReconciler. +type MockBYOResourceReconcilerMockRecorder struct { + mock *MockBYOResourceReconciler +} + +// NewMockBYOResourceReconciler creates a new mock instance. +func NewMockBYOResourceReconciler(ctrl *gomock.Controller) *MockBYOResourceReconciler { + mock := &MockBYOResourceReconciler{ctrl: ctrl} + mock.recorder = &MockBYOResourceReconcilerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockBYOResourceReconciler) EXPECT() *MockBYOResourceReconcilerMockRecorder { + return m.recorder +} + +// IsManaged mocks base method. +func (m *MockBYOResourceReconciler) IsManaged(arg0 context.Context) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "IsManaged", arg0) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// IsManaged indicates an expected call of IsManaged. +func (mr *MockBYOResourceReconcilerMockRecorder) IsManaged(arg0 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsManaged", reflect.TypeOf((*MockBYOResourceReconciler)(nil).IsManaged), arg0) +} + // MockAuthorizer is a mock of Authorizer interface. type MockAuthorizer struct { ctrl *gomock.Controller diff --git a/controllers/azurecluster_reconciler.go b/controllers/azurecluster_reconciler.go index 398ee1920b5..d8b320fcc61 100644 --- a/controllers/azurecluster_reconciler.go +++ b/controllers/azurecluster_reconciler.go @@ -168,7 +168,7 @@ func (s *azureClusterService) Delete(ctx context.Context) error { return errors.Wrap(err, "failed to get group service") } - if !ShouldDeleteIndividualResources(ctx, groupSvc, s.scope) { + if !ShouldDeleteIndividualResources(ctx, groupSvc.(azure.BYOResourceReconciler), s.scope) { // If the resource group is managed, delete it. // We need to explicitly delete vnet peerings, as it is not part of the resource group. vnetPeeringsSvc, err := GetService(vnetpeerings.ServiceName, s.services) diff --git a/controllers/azurecluster_reconciler_test.go b/controllers/azurecluster_reconciler_test.go index 9b60df51f94..b733de02d8f 100644 --- a/controllers/azurecluster_reconciler_test.go +++ b/controllers/azurecluster_reconciler_test.go @@ -180,10 +180,15 @@ func TestAzureClusterServiceDelete(t *testing.T) { namespace := "ns" resourceGroup := "rg" + type MockGroupsService struct { + *mock_azure.MockServiceReconciler + *mock_azure.MockBYOResourceReconciler + } + cases := map[string]struct { expectedError string clientBuilder func(g Gomega) client.Client - expect func(grp *mock_azure.MockServiceReconcilerMockRecorder, vpr *mock_azure.MockServiceReconcilerMockRecorder, one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) + expect func(grp MockGroupsService, vpr *mock_azure.MockServiceReconcilerMockRecorder, one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) }{ "Resource Group is deleted successfully": { expectedError: "", @@ -211,14 +216,14 @@ func TestAzureClusterServiceDelete(t *testing.T) { return c }, - expect: func(grp *mock_azure.MockServiceReconcilerMockRecorder, vpr *mock_azure.MockServiceReconcilerMockRecorder, _ *mock_azure.MockServiceReconcilerMockRecorder, _ *mock_azure.MockServiceReconcilerMockRecorder, _ *mock_azure.MockServiceReconcilerMockRecorder) { + expect: func(grp MockGroupsService, vpr *mock_azure.MockServiceReconcilerMockRecorder, _ *mock_azure.MockServiceReconcilerMockRecorder, _ *mock_azure.MockServiceReconcilerMockRecorder, _ *mock_azure.MockServiceReconcilerMockRecorder) { gomock.InOrder( - grp.Name().Return(groups.ServiceName), - grp.IsManaged(gomockinternal.AContext()).Return(true, nil), - grp.Name().Return(groups.ServiceName), + grp.MockServiceReconciler.EXPECT().Name().Return(groups.ServiceName), + grp.MockBYOResourceReconciler.EXPECT().IsManaged(gomockinternal.AContext()).Return(true, nil), + grp.MockServiceReconciler.EXPECT().Name().Return(groups.ServiceName), vpr.Name().Return(vnetpeerings.ServiceName), vpr.Delete(gomockinternal.AContext()).Return(nil), - grp.Delete(gomockinternal.AContext()).Return(nil)) + grp.MockServiceReconciler.EXPECT().Delete(gomockinternal.AContext()).Return(nil)) }, }, "Resource Group delete fails": { @@ -247,14 +252,14 @@ func TestAzureClusterServiceDelete(t *testing.T) { return c }, - expect: func(grp *mock_azure.MockServiceReconcilerMockRecorder, vpr *mock_azure.MockServiceReconcilerMockRecorder, _ *mock_azure.MockServiceReconcilerMockRecorder, _ *mock_azure.MockServiceReconcilerMockRecorder, _ *mock_azure.MockServiceReconcilerMockRecorder) { + expect: func(grp MockGroupsService, vpr *mock_azure.MockServiceReconcilerMockRecorder, _ *mock_azure.MockServiceReconcilerMockRecorder, _ *mock_azure.MockServiceReconcilerMockRecorder, _ *mock_azure.MockServiceReconcilerMockRecorder) { gomock.InOrder( - grp.Name().Return(groups.ServiceName), - grp.IsManaged(gomockinternal.AContext()).Return(true, nil), - grp.Name().Return(groups.ServiceName), + grp.MockServiceReconciler.EXPECT().Name().Return(groups.ServiceName), + grp.MockBYOResourceReconciler.EXPECT().IsManaged(gomockinternal.AContext()).Return(true, nil), + grp.MockServiceReconciler.EXPECT().Name().Return(groups.ServiceName), vpr.Name().Return(vnetpeerings.ServiceName), vpr.Delete(gomockinternal.AContext()).Return(nil), - grp.Delete(gomockinternal.AContext()).Return(errors.New("internal error"))) + grp.MockServiceReconciler.EXPECT().Delete(gomockinternal.AContext()).Return(errors.New("internal error"))) }, }, "Resource Group not owned by cluster": { @@ -280,15 +285,15 @@ func TestAzureClusterServiceDelete(t *testing.T) { return c }, - expect: func(grp *mock_azure.MockServiceReconcilerMockRecorder, vpr *mock_azure.MockServiceReconcilerMockRecorder, one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) { + expect: func(grp MockGroupsService, vpr *mock_azure.MockServiceReconcilerMockRecorder, one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) { gomock.InOrder( - grp.Name().Return(groups.ServiceName), - grp.IsManaged(gomockinternal.AContext()).Return(false, nil), + grp.MockServiceReconciler.EXPECT().Name().Return(groups.ServiceName), + grp.MockBYOResourceReconciler.EXPECT().IsManaged(gomockinternal.AContext()).Return(false, nil), three.Delete(gomockinternal.AContext()).Return(nil), two.Delete(gomockinternal.AContext()).Return(nil), one.Delete(gomockinternal.AContext()).Return(nil), vpr.Delete(gomockinternal.AContext()).Return(nil), - grp.Delete(gomockinternal.AContext()).Return(nil)) + grp.MockServiceReconciler.EXPECT().Delete(gomockinternal.AContext()).Return(nil)) }, }, "service delete fails": { @@ -314,10 +319,10 @@ func TestAzureClusterServiceDelete(t *testing.T) { return c }, - expect: func(grp *mock_azure.MockServiceReconcilerMockRecorder, vpr *mock_azure.MockServiceReconcilerMockRecorder, one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) { + expect: func(grp MockGroupsService, vpr *mock_azure.MockServiceReconcilerMockRecorder, one *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) { gomock.InOrder( - grp.Name().Return(groups.ServiceName), - grp.IsManaged(gomockinternal.AContext()).Return(false, nil), + grp.MockServiceReconciler.EXPECT().Name().Return(groups.ServiceName), + grp.MockBYOResourceReconciler.EXPECT().IsManaged(gomockinternal.AContext()).Return(false, nil), three.Delete(gomockinternal.AContext()).Return(nil), two.Delete(gomockinternal.AContext()).Return(errors.New("some error happened")), two.Name().Return("two")) @@ -333,13 +338,16 @@ func TestAzureClusterServiceDelete(t *testing.T) { t.Parallel() mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() - groupsMock := mock_azure.NewMockServiceReconciler(mockCtrl) + groupsMock := MockGroupsService{ + MockServiceReconciler: mock_azure.NewMockServiceReconciler(mockCtrl), + MockBYOResourceReconciler: mock_azure.NewMockBYOResourceReconciler(mockCtrl), + } vnetpeeringsMock := mock_azure.NewMockServiceReconciler(mockCtrl) svcOneMock := mock_azure.NewMockServiceReconciler(mockCtrl) svcTwoMock := mock_azure.NewMockServiceReconciler(mockCtrl) svcThreeMock := mock_azure.NewMockServiceReconciler(mockCtrl) - tc.expect(groupsMock.EXPECT(), vnetpeeringsMock.EXPECT(), svcOneMock.EXPECT(), svcTwoMock.EXPECT(), svcThreeMock.EXPECT()) + tc.expect(groupsMock, vnetpeeringsMock.EXPECT(), svcOneMock.EXPECT(), svcTwoMock.EXPECT(), svcThreeMock.EXPECT()) c := tc.clientBuilder(g) s := &azureClusterService{ diff --git a/controllers/helpers.go b/controllers/helpers.go index bd312b600f2..ef1a399a6ed 100644 --- a/controllers/helpers.go +++ b/controllers/helpers.go @@ -598,7 +598,7 @@ func GetAzureMachinePoolByName(ctx context.Context, c client.Client, namespace, // ShouldDeleteIndividualResources returns false if the resource group is managed and the whole cluster is being deleted // meaning that we can rely on a single resource group delete operation as opposed to deleting every individual VM resource. -func ShouldDeleteIndividualResources(ctx context.Context, grpSvc azure.ServiceReconciler, cluster azure.ClusterScoper) bool { +func ShouldDeleteIndividualResources(ctx context.Context, grpSvc azure.BYOResourceReconciler, cluster azure.ClusterScoper) bool { ctx, _, done := tele.StartSpanWithLogger(ctx, "controllers.ShouldDeleteIndividualResources") defer done()