diff --git a/azure/services/groups/groups.go b/azure/services/groups/groups.go index 90cb95b852da..357e7f4b636c 100644 --- a/azure/services/groups/groups.go +++ b/azure/services/groups/groups.go @@ -97,7 +97,21 @@ func (s *Service) Delete(ctx context.Context) error { // IsManaged returns true if the ASO ResourceGroup was created by CAPZ, // meaning that the resource group's lifecycle is managed. func (s *Service) IsManaged(ctx context.Context) (bool, error) { - return aso.IsManaged(ctx, s.Scope.GetClient(), s.Scope.GroupSpec(), s.Scope.ClusterName()) + managed, err := aso.IsManaged(ctx, s.Scope.GetClient(), s.Scope.GroupSpec(), s.Scope.ClusterName()) + if err != nil { + return managed, err + } + + // For ASO, "managed" only tells us that we're allowed to delete the ASO + // resource. We also need to check that deleting the ASO resource will really + // delete the underlying resource group by checking the ASO reconcile-policy. + spec := s.Scope.GroupSpec() + group := spec.ResourceRef() + err = s.Scope.GetClient().Get(ctx, client.ObjectKeyFromObject(group), group) + if err != nil { + return false, err + } + return group.GetAnnotations()[asoannotations.ReconcilePolicy] != string(asoannotations.ReconcilePolicyManage), nil } var _ azure.Pauser = (*Service)(nil) @@ -110,23 +124,3 @@ func (s *Service) Pause(ctx context.Context) error { } return aso.PauseResource(ctx, s.Scope.GetClient(), groupSpec, s.Scope.ClusterName(), ServiceName) } - -// ShouldDeleteIndividualResources returns false if the resource group is -// managed and reconciled by ASO, meaning that we can rely on a single resource -// group delete operation as opposed to deleting every individual resource. -func (s *Service) ShouldDeleteIndividualResources(ctx context.Context) bool { - // Since this is a best effort attempt to speed up delete, we don't fail the delete if we can't get the RG status. - // Instead, take the long way and delete all resources one by one. - managed, err := s.IsManaged(ctx) - if err != nil || !managed { - return true - } - - // For ASO, "managed" only tells us that we're allowed to delete the ASO - // resource. We also need to check that deleting the ASO resource will really - // delete the underlying resource group by checking the ASO reconcile-policy. - spec := s.Scope.GroupSpec() - group := spec.ResourceRef() - err = s.Scope.GetClient().Get(ctx, client.ObjectKeyFromObject(group), group) - return err != nil || group.GetAnnotations()[asoannotations.ReconcilePolicy] != string(asoannotations.ReconcilePolicyManage) -} diff --git a/controllers/azurecluster_reconciler.go b/controllers/azurecluster_reconciler.go index 4bc0f3037048..398ee1920b5c 100644 --- a/controllers/azurecluster_reconciler.go +++ b/controllers/azurecluster_reconciler.go @@ -163,7 +163,7 @@ func (s *azureClusterService) Delete(ctx context.Context) error { ctx, _, done := tele.StartSpanWithLogger(ctx, "controllers.azureClusterService.Delete") defer done() - groupSvc, err := GetService[*groups.Service](groups.ServiceName, s.services) + groupSvc, err := GetService(groups.ServiceName, s.services) if err != nil { return errors.Wrap(err, "failed to get group service") } @@ -171,7 +171,7 @@ func (s *azureClusterService) Delete(ctx context.Context) error { if !ShouldDeleteIndividualResources(ctx, groupSvc, 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.Service](vnetpeerings.ServiceName, s.services) + vnetPeeringsSvc, err := GetService(vnetpeerings.ServiceName, s.services) if err != nil { return errors.Wrap(err, "failed to get vnet peerings service") } diff --git a/controllers/azurecluster_reconciler_test.go b/controllers/azurecluster_reconciler_test.go index c0ee7709bafe..9b60df51f946 100644 --- a/controllers/azurecluster_reconciler_test.go +++ b/controllers/azurecluster_reconciler_test.go @@ -213,10 +213,11 @@ func TestAzureClusterServiceDelete(t *testing.T) { }, expect: func(grp *mock_azure.MockServiceReconcilerMockRecorder, 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), vpr.Name().Return(vnetpeerings.ServiceName), vpr.Delete(gomockinternal.AContext()).Return(nil), - grp.Name().Return(groups.ServiceName), grp.Delete(gomockinternal.AContext()).Return(nil)) }, }, @@ -248,10 +249,11 @@ func TestAzureClusterServiceDelete(t *testing.T) { }, expect: func(grp *mock_azure.MockServiceReconcilerMockRecorder, 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), vpr.Name().Return(vnetpeerings.ServiceName), vpr.Delete(gomockinternal.AContext()).Return(nil), - grp.Name().Return(groups.ServiceName), grp.Delete(gomockinternal.AContext()).Return(errors.New("internal error"))) }, }, @@ -280,6 +282,8 @@ func TestAzureClusterServiceDelete(t *testing.T) { }, expect: func(grp *mock_azure.MockServiceReconcilerMockRecorder, 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), three.Delete(gomockinternal.AContext()).Return(nil), two.Delete(gomockinternal.AContext()).Return(nil), one.Delete(gomockinternal.AContext()).Return(nil), @@ -310,8 +314,10 @@ func TestAzureClusterServiceDelete(t *testing.T) { return c }, - expect: func(_ *mock_azure.MockServiceReconcilerMockRecorder, _ *mock_azure.MockServiceReconcilerMockRecorder, _ *mock_azure.MockServiceReconcilerMockRecorder, two *mock_azure.MockServiceReconcilerMockRecorder, three *mock_azure.MockServiceReconcilerMockRecorder) { + expect: func(grp *mock_azure.MockServiceReconcilerMockRecorder, 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), three.Delete(gomockinternal.AContext()).Return(nil), two.Delete(gomockinternal.AContext()).Return(errors.New("some error happened")), two.Name().Return("two")) diff --git a/controllers/azuremachine_reconciler.go b/controllers/azuremachine_reconciler.go index fe09b70c8407..9b43bbb5c085 100644 --- a/controllers/azuremachine_reconciler.go +++ b/controllers/azuremachine_reconciler.go @@ -156,7 +156,7 @@ func (s *azureMachineService) delete(ctx context.Context) error { ctx, log, done := tele.StartSpanWithLogger(ctx, "controllers.azureMachineService.delete") defer done() - groupSvc, err := GetService[*groups.Service](groups.ServiceName, s.services) + groupSvc, err := GetService(groups.ServiceName, s.services) if err != nil { return errors.Wrap(err, "failed to get group service") } diff --git a/controllers/helpers.go b/controllers/helpers.go index 05718f990948..bd312b600f25 100644 --- a/controllers/helpers.go +++ b/controllers/helpers.go @@ -37,7 +37,6 @@ import ( 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/scope" - "sigs.k8s.io/cluster-api-provider-azure/azure/services/groups" infrav1exp "sigs.k8s.io/cluster-api-provider-azure/exp/api/v1beta1" "sigs.k8s.io/cluster-api-provider-azure/feature" "sigs.k8s.io/cluster-api-provider-azure/pkg/coalescing" @@ -599,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, groupSvc *groups.Service, cluster azure.ClusterScoper) bool { +func ShouldDeleteIndividualResources(ctx context.Context, grpSvc azure.ServiceReconciler, cluster azure.ClusterScoper) bool { ctx, _, done := tele.StartSpanWithLogger(ctx, "controllers.ShouldDeleteIndividualResources") defer done() @@ -607,7 +606,10 @@ func ShouldDeleteIndividualResources(ctx context.Context, groupSvc *groups.Servi return true } - return groupSvc.ShouldDeleteIndividualResources(ctx) + managed, err := grpSvc.IsManaged(ctx) + // Since this is a best effort attempt to speed up delete, we don't fail the delete if we can't get the RG status. + // Instead, take the long way and delete all resources one by one. + return err != nil || !managed } // GetClusterIdentityFromRef returns the AzureClusterIdentity referenced by the AzureCluster. @@ -1062,16 +1064,11 @@ func ClusterPauseChangeAndInfrastructureReady(log logr.Logger) predicate.Funcs { } // GetService returns the service with the given name and type. -func GetService[T azure.ServiceReconciler](name string, services []azure.ServiceReconciler) (T, error) { - var result T +func GetService(name string, services []azure.ServiceReconciler) (azure.ServiceReconciler, error) { for _, service := range services { if service.Name() == name { - result, ok := service.(T) - if !ok { - return result, errors.Errorf("service %s is not of type %T", name, result) - } - return result, nil + return service, nil } } - return result, errors.Errorf("service %s not found", name) + return nil, errors.Errorf("service %s not found", name) } diff --git a/exp/controllers/azuremachinepool_reconciler.go b/exp/controllers/azuremachinepool_reconciler.go index 8265674df0dc..888735fe337b 100644 --- a/exp/controllers/azuremachinepool_reconciler.go +++ b/exp/controllers/azuremachinepool_reconciler.go @@ -106,7 +106,7 @@ func (s *azureMachinePoolService) Delete(ctx context.Context) error { ctx, log, done := tele.StartSpanWithLogger(ctx, "controllers.azureMachinePoolService.Delete") defer done() - groupSvc, err := infracontroller.GetService[*groups.Service](groups.ServiceName, s.services) + groupSvc, err := infracontroller.GetService(groups.ServiceName, s.services) if err != nil { return errors.Wrap(err, "failed to get group service") }