Fix: Move share cleanup to CleanupBeforeVPCDeletion phase to unblock VPC deletion

- Add CleanupBeforeVPCDeletion to SecurityPolicyService and LBInfraCleaner
- Remove share cleanup from CleanupInfraResources phase

Change-Id: Ib6814933c24252eba19483ec3d2abd27fbafa7de

Author: zhengxiexie

pkg/clean/clean_lb_infra.go

@@ -53,18 +53,28 @@ func keyFunc(obj interface{}) (string, error) {
 	}
 }
 
+// CleanupBeforeVPCDeletion cleans up LB-related shares before VPC deletion
+func (s *LBInfraCleaner) CleanupBeforeVPCDeletion(ctx context.Context) error {
+	s.log.Info("Cleaning up LB infra shares before VPC deletion")
+
+	// Clean up shares
+	if err := s.cleanupInfraShares(ctx); err != nil {
+		s.log.Error(err, "Failed to clean up infra shares")
+		return err
+	}
+
+	s.log.Info("Successfully cleaned up LB infra shares")
+	return nil
+}
+
 // CleanupInfraResources is to clean up the LB related resources created under path /infra, including,
-// group/share/cert/LBAppProfile/LBPersistentProfile/dlb virtual servers/dlb services/dlb groups/dlb pools
+// cert/LBAppProfile/LBPersistentProfile/dlb virtual servers/dlb services/dlb groups/dlb pools
 func (s *LBInfraCleaner) CleanupInfraResources(ctx context.Context) error {
 	// LB virtual server has dependencies on LB pool, so we can't delete vs and pool in parallel.
 	if err := s.cleanupInfraDLBVirtualServers(ctx); err != nil {
 		s.log.Error(err, "Failed to clean up DLB virtual servers")
 		return err
 	}
-	if err := s.cleanupInfraShares(ctx); err != nil {
-		s.log.Error(err, "Failed to clean up infra Shareds")
-		return err
-	}
 
 	parallelCleaners := []func(ctx context.Context) error{
 		s.cleanupInfraDLBPools,

pkg/clean/clean_test.go

@@ -213,7 +213,8 @@ func TestInitializeCleanupService_Success(t *testing.T) {
 	cleanupService, err := InitializeCleanupService(cf, nsxClient, &log)
 	assert.NoError(t, err)
 	assert.NotNil(t, cleanupService)
-	assert.Len(t, cleanupService.vpcPreCleaners, 6)
+	// vpcPreCleaners: SubnetPort, SubnetBinding, SubnetIPReservation, Inventory, SecurityPolicy, LBInfraCleaner, NSXServiceAccount, HealthCleaner = 8
+	assert.Len(t, cleanupService.vpcPreCleaners, 8)
 	assert.Len(t, cleanupService.vpcChildrenCleaners, 5)
 	assert.Len(t, cleanupService.infraCleaners, 2)
 }
@@ -256,7 +257,8 @@ func TestInitializeCleanupService_VPCError(t *testing.T) {
 	assert.NotNil(t, cleanupService)
 	// Note, the services added after VPCService should fail because of the error returned in `InitializeVPC`.
 	assert.Len(t, cleanupService.vpcChildrenCleaners, 3)
-	assert.Len(t, cleanupService.vpcPreCleaners, 3)
+	// vpcPreCleaners: SubnetPort, SubnetBinding, SubnetIPReservation, SecurityPolicy = 4 (services initialized before VPC error)
+	assert.Len(t, cleanupService.vpcPreCleaners, 4)
 	assert.Len(t, cleanupService.infraCleaners, 1)
 	assert.Equal(t, expectedError, cleanupService.svcErr)
 }

pkg/nsx/services/securitypolicy/cleanup.go

@@ -9,6 +9,49 @@ import (
 	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common"
 )
 
+// CleanupBeforeVPCDeletion cleans up SecurityPolicy, Rules, and Shares before VPC deletion to avoid dependency issues.
+// SecurityPolicy and Rules must be deleted first because Shares cannot be deleted while they are still being consumed.
+func (service *SecurityPolicyService) CleanupBeforeVPCDeletion(ctx context.Context) error {
+	log.Info("Cleaning up security policies, rules, and shares before VPC deletion")
+
+	if err := service.cleanupRulesByVPC(ctx, ""); err != nil {
+		log.Error(err, "Failed to clean up rules")
+		return err
+	}
+	log.Info("Successfully cleaned all rules")
+
+	if err := service.cleanupSecurityPoliciesByVPC(ctx, ""); err != nil {
+		log.Error(err, "Failed to clean up security policies")
+		return err
+	}
+	log.Info("Successfully cleaned all security policies")
+
+	// Step 3: Clean both project and infra shares
+	for _, config := range []struct {
+		store   *ShareStore
+		builder *common.PolicyTreeBuilder[*model.Share]
+		name    string
+	}{
+		{
+			store:   service.projectShareStore,
+			builder: service.projectShareBuilder,
+			name:    "project",
+		}, {
+			store:   service.infraShareStore,
+			builder: service.infraShareBuilder,
+			name:    "infra",
+		},
+	} {
+		if err := cleanShares(ctx, config.store, config.builder, service.NSXClient); err != nil {
+			log.Error(err, "Failed to clean shares", "type", config.name)
+			return err
+		}
+		log.Info("Successfully cleaned shares", "type", config.name)
+	}
+
+	return nil
+}
+
 // CleanupVPCChildResources is called when cleaning up the VPC related resources. For the resources in an auto-created
 // VPC, this function is called after the VPC is deleted on NSX, so the provider only needs to clean up with the local
 // cache. For the resources in a pre-created VPC, this function is called to delete resources on NSX and in the local cache.
@@ -105,22 +148,6 @@ func (service *SecurityPolicyService) cleanupGroupsByVPC(ctx context.Context, vp
 
 // CleanupInfraResources is to clean up the resources created by SecurityPolicyService under path /infra.
 func (service *SecurityPolicyService) CleanupInfraResources(ctx context.Context) error {
-	for _, config := range []struct {
-		store   *ShareStore
-		builder *common.PolicyTreeBuilder[*model.Share]
-	}{
-		{
-			store:   service.projectShareStore,
-			builder: service.projectShareBuilder,
-		}, {
-			store:   service.infraShareStore,
-			builder: service.infraShareBuilder,
-		},
-	} {
-		if err := cleanShares(ctx, config.store, config.builder, service.NSXClient); err != nil {
-			return err
-		}
-	}
 	for _, config := range []struct {
 		store   *GroupStore
 		builder *common.PolicyTreeBuilder[*model.Group]

pkg/nsx/services/securitypolicy/cleanup_test.go

@@ -19,8 +19,6 @@ import (
 
 var (
 	projectPath         = "/orgs/default/projects/project-1"
-	infraShareId        = "infra-share"
-	projectShareId      = "proj-share"
 	infraGroupId        = "infra-group"
 	projectGroupId      = "proj-group"
 	vpcPath             = fmt.Sprintf("%s/vpcs/vpc-1", projectPath)
@@ -46,22 +44,6 @@ func TestCleanupInfraResources(t *testing.T) {
 	orgRootClient := mock_org_root.NewMockOrgRootClient(ctrl)
 	svc := prepareServiceForCleanup(orgRootClient)
 
-	infraShare := &model.Share{
-		Id:         String(infraShareId),
-		Path:       String(fmt.Sprintf("/infra/shares/%s", infraShareId)),
-		ParentPath: String("/infra"),
-		Tags:       infraResourceTags,
-	}
-	svc.infraShareStore.Add(infraShare)
-
-	projectShare := &model.Share{
-		Id:         String(projectShareId),
-		Path:       String(fmt.Sprintf("%s/shares/%s", projectPath, infraShareId)),
-		ParentPath: String(projectPath),
-		Tags:       infraResourceTags,
-	}
-	svc.projectShareStore.Add(projectShare)
-
 	infraGroup := &model.Group{
 		Id:         String(infraGroupId),
 		Path:       String(fmt.Sprintf("/infra/domains/default/groups/%s", infraGroupId)),
@@ -78,25 +60,23 @@ func TestCleanupInfraResources(t *testing.T) {
 	}
 	svc.projectGroupStore.Add(projectGroup)
 
-	assert.Equal(t, 1, len(svc.infraShareStore.List()))
 	assert.Equal(t, 1, len(svc.infraGroupStore.List()))
 	assert.Equal(t, 1, len(svc.projectGroupStore.List()))
-	assert.Equal(t, 1, len(svc.projectShareStore.List()))
 
+	// infraGroupBuilder uses InfraClient.Patch
 	patches := gomonkey.ApplyMethodSeq(svc.NSXClient.InfraClient, "Patch", []gomonkey.OutputCell{{
 		Values: gomonkey.Params{nil},
-		Times:  2,
+		Times:  1,
 	}})
 	defer patches.Reset()
-	orgRootClient.EXPECT().Patch(gomock.Any(), gomock.Any()).Return(nil).Times(2)
+	// projectGroupBuilder uses OrgRootClient.Patch
+	orgRootClient.EXPECT().Patch(gomock.Any(), gomock.Any()).Return(nil).Times(1)
 
 	ctx := context.Background()
 	err := svc.CleanupInfraResources(ctx)
 	require.NoError(t, err)
-	assert.Equal(t, 0, len(svc.infraShareStore.List()))
 	assert.Equal(t, 0, len(svc.infraGroupStore.List()))
 	assert.Equal(t, 0, len(svc.projectGroupStore.List()))
-	assert.Equal(t, 0, len(svc.projectShareStore.List()))
 }
 
 func TestCleanupVPCChildResources(t *testing.T) {