Separate IsManaged into its own interface

Co-authored-by: Jon Huhn <[email protected]>
kubernetes-sigs · Oct 4, 2023 · 12f29a9 · 12f29a9
1 parent 2efe829
commit 12f29a9
Show file tree

Hide file tree

Showing 5 changed files with 73 additions and 38 deletions.
diff --git 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
diff --git 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
@@ -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
@@ -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()