CLOUDP-264068: [Fix] Respect Teams deletion protection (#1722)

josvazg · web-flow · commit 94862b171dc1 · 2024-07-31T14:39:44.000+02:00
* CLOUDP-264068: Respect Teams deletion protection * Respect also the keep flag * Add e2e test coverage to fix * Fix linter
diff --git a/pkg/controller/atlasproject/teams.go b/pkg/controller/atlasproject/teams.go
@@ -10,6 +10,7 @@ import (
 	akov2 "github.com/mongodb/mongodb-atlas-kubernetes/v2/pkg/api/v1"
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/pkg/api/v1/common"
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/pkg/api/v1/status"
+	"github.com/mongodb/mongodb-atlas-kubernetes/v2/pkg/controller/customresource"
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/pkg/controller/statushandler"
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/pkg/controller/workflow"
 )
@@ -194,12 +195,16 @@ func (r *AtlasProjectReconciler) updateTeamState(ctx *workflow.Context, project
 	teamCtx.Client = atlasClient
 
 	if len(assignedProjects) == 0 {
-		log.Debugf("team %s has no project associated to it. removing from atlas.", team.Spec.Name)
-		_, err = teamCtx.Client.Teams.RemoveTeamFromOrganization(ctx.Context, orgID, team.Status.ID)
-		if err != nil {
-			return err
+		if customresource.IsResourcePolicyKeepOrDefault(project, r.ObjectDeletionProtection) {
+			log.Debug("team %s has no project associated, "+
+				"skipping deletion from Atlas due to ObjectDeletionProtection being set", team.Spec.Name)
+		} else {
+			log.Debugf("team %s has no project associated to it. removing from atlas.", team.Spec.Name)
+			_, err = teamCtx.Client.Teams.RemoveTeamFromOrganization(ctx.Context, orgID, team.Status.ID)
+			if err != nil {
+				return err
+			}
 		}
-
 		teamCtx.EnsureStatusOption(status.AtlasTeamUnsetID())
 	}
 
diff --git a/pkg/controller/atlasproject/teams_test.go b/pkg/controller/atlasproject/teams_test.go
@@ -19,19 +19,14 @@ import (
 	akov2 "github.com/mongodb/mongodb-atlas-kubernetes/v2/pkg/api/v1"
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/pkg/api/v1/common"
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/pkg/api/v1/status"
+	"github.com/mongodb/mongodb-atlas-kubernetes/v2/pkg/controller/customresource"
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/pkg/controller/workflow"
 )
 
 func TestUpdateTeamState(t *testing.T) {
 	t.Run("should not duplicate projects listed", func(t *testing.T) {
 		logger := zaptest.NewLogger(t).Sugar()
-		workflowCtx := &workflow.Context{
-			Context: context.Background(),
-			Log:     logger,
-		}
-		testScheme := runtime.NewScheme()
-		akov2.AddToScheme(testScheme)
-		corev1.AddToScheme(testScheme)
+		workflowCtx := defaultTestWorkflow(logger)
 		secret := &corev1.Secret{
 			ObjectMeta: metav1.ObjectMeta{
 				Name: "my-secret",
@@ -74,38 +69,25 @@ func TestUpdateTeamState(t *testing.T) {
 				return &mongodbatlas.Client{}, "0987654321", nil
 			},
 		}
-		k8sClient := fake.NewClientBuilder().
-			WithScheme(testScheme).
-			WithObjects(secret, project, team).
-			Build()
+		k8sClient := buildFakeKubernetesClient(secret, project, team)
 		reconciler := &AtlasProjectReconciler{
 			Client:        k8sClient,
 			Log:           logger,
 			AtlasProvider: atlasProvider,
 		}
-		teamRef := &common.ResourceRefNamespaced{
-			Name:      team.Name,
-			Namespace: "testNS",
-		}
 		// check we have exactly 1 project in status
 		assert.Equal(t, 1, len(team.Status.Projects))
 
 		// "reconcile" the team state and check we still have 1 project in status
-		err := reconciler.updateTeamState(workflowCtx, project, teamRef, false)
+		err := reconciler.updateTeamState(workflowCtx, project, reference(team), false)
 		assert.NoError(t, err)
 		k8sClient.Get(context.Background(), types.NamespacedName{Name: team.ObjectMeta.Name, Namespace: team.ObjectMeta.Namespace}, team)
 		assert.Equal(t, 1, len(team.Status.Projects))
 	})
 
 	t.Run("must remove a team from Atlas is a team is unassigned", func(t *testing.T) {
 		logger := zaptest.NewLogger(t).Sugar()
-		workflowCtx := &workflow.Context{
-			Context: context.Background(),
-			Log:     logger,
-		}
-		testScheme := runtime.NewScheme()
-		akov2.AddToScheme(testScheme)
-		corev1.AddToScheme(testScheme)
+		workflowCtx := defaultTestWorkflow(logger)
 		secret := &corev1.Secret{
 			ObjectMeta: metav1.ObjectMeta{
 				Name: "my-secret",
@@ -151,23 +133,124 @@ func TestUpdateTeamState(t *testing.T) {
 				}, "0987654321", nil
 			},
 		}
-		k8sClient := fake.NewClientBuilder().
-			WithScheme(testScheme).
-			WithObjects(secret, project, team).
-			Build()
+		k8sClient := buildFakeKubernetesClient(secret, project, team)
 		reconciler := &AtlasProjectReconciler{
 			Client:        k8sClient,
 			Log:           logger,
 			AtlasProvider: atlasProvider,
 		}
-		teamRef := &common.ResourceRefNamespaced{
-			Name:      team.Name,
-			Namespace: "testNS",
-		}
 
-		err := reconciler.updateTeamState(workflowCtx, project, teamRef, true)
+		err := reconciler.updateTeamState(workflowCtx, project, reference(team), true)
 		assert.NoError(t, err)
 		k8sClient.Get(context.Background(), types.NamespacedName{Name: team.ObjectMeta.Name, Namespace: team.ObjectMeta.Namespace}, team)
 		assert.Len(t, teamsMock.RemoveTeamFromOrganizationRequests, 1)
 	})
+
+	t.Run("must honor deletion protection flag for Teams", func(t *testing.T) {
+		for _, tc := range []struct {
+			title              string
+			deletionProtection bool
+			keepFlag           bool
+			expectRemoval      bool
+		}{
+			{
+				title:              "with deletion protection unassigned teams are not removed",
+				deletionProtection: true,
+				keepFlag:           false,
+				expectRemoval:      false,
+			},
+			{
+				title:              "without deletion protection unassigned teams are removed",
+				deletionProtection: false,
+				keepFlag:           false,
+				expectRemoval:      true,
+			},
+			{
+				title:              "with deletion protection & keep flag teams are not removed",
+				deletionProtection: false,
+				keepFlag:           true,
+				expectRemoval:      false,
+			},
+			{
+				title:              "without deletion protection but keep flag teams are not removed",
+				deletionProtection: true,
+				keepFlag:           true,
+				expectRemoval:      false,
+			},
+		} {
+			t.Run(tc.title, func(t *testing.T) {
+				logger := zaptest.NewLogger(t).Sugar()
+				workflowCtx := defaultTestWorkflow(logger)
+				project := &akov2.AtlasProject{
+					Spec: akov2.AtlasProjectSpec{
+						Name: "projectName",
+					},
+				}
+				team := &akov2.AtlasTeam{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "testTeam",
+						Namespace: "testNS",
+					},
+				}
+				teamsMock := &atlas.TeamsClientMock{
+					RemoveTeamFromOrganizationFunc: func(orgID string, teamID string) (*mongodbatlas.Response, error) {
+						return nil, nil
+					},
+					RemoveTeamFromOrganizationRequests: map[string]struct{}{},
+				}
+				atlasProvider := &atlas.TestProvider{
+					ClientFunc: func(secretRef *client.ObjectKey, log *zap.SugaredLogger) (*mongodbatlas.Client, string, error) {
+						return &mongodbatlas.Client{
+							Teams: teamsMock,
+						}, "0987654321", nil
+					},
+				}
+				reconciler := &AtlasProjectReconciler{
+					Client:                   buildFakeKubernetesClient(project, team),
+					Log:                      logger,
+					AtlasProvider:            atlasProvider,
+					ObjectDeletionProtection: tc.deletionProtection,
+				}
+				if tc.keepFlag {
+					customresource.SetAnnotation(project,
+						customresource.ResourcePolicyAnnotation, customresource.ResourcePolicyKeep)
+				}
+				err := reconciler.updateTeamState(workflowCtx, project, reference(team), true)
+				assert.NoError(t, err)
+				expectedRemovals := 0
+				if tc.expectRemoval {
+					expectedRemovals = 1
+				}
+				assert.Len(t, teamsMock.RemoveTeamFromOrganizationRequests, expectedRemovals)
+			})
+		}
+	})
+}
+
+func defaultTestWorkflow(logger *zap.SugaredLogger) *workflow.Context {
+	return &workflow.Context{
+		Context: context.Background(),
+		Log:     logger,
+	}
+}
+
+func defaultTestScheme() *runtime.Scheme {
+	scheme := runtime.NewScheme()
+	akov2.AddToScheme(scheme)
+	corev1.AddToScheme(scheme)
+	return scheme
+}
+
+func buildFakeKubernetesClient(objects ...client.Object) client.WithWatch {
+	return fake.NewClientBuilder().
+		WithScheme(defaultTestScheme()).
+		WithObjects(objects...).
+		Build()
+}
+
+func reference(obj client.Object) *common.ResourceRefNamespaced {
+	return &common.ResourceRefNamespaced{
+		Name:      obj.GetName(),
+		Namespace: obj.GetNamespace(),
+	}
 }
diff --git a/test/e2e/teams_test.go b/test/e2e/teams_test.go
@@ -1,17 +1,22 @@
 package e2e_test
 
 import (
+	"context"
+	"errors"
+	"fmt"
 	"time"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"go.mongodb.org/atlas-sdk/v20231115008/admin"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/pkg/api"
 	akov2 "github.com/mongodb/mongodb-atlas-kubernetes/v2/pkg/api/v1"
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/pkg/api/v1/common"
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/test/helper/e2e/actions"
+	"github.com/mongodb/mongodb-atlas-kubernetes/v2/test/helper/e2e/api/atlas"
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/test/helper/e2e/data"
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/test/helper/e2e/model"
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/test/helper/e2e/utils"
@@ -31,8 +36,6 @@ var _ = Describe("Teams", Label("teams"), func() {
 			Expect(actions.SaveTeamsToFile(testData.Context, testData.K8SClient, testData.Resources.Namespace)).Should(Succeed())
 		}
 		By("Delete Resources", func() {
-			actions.DeleteTestDataTeams(testData)
-			actions.DeleteTestDataProject(testData)
 			actions.AfterEachFinalCleanup([]model.TestDataProvider{*testData})
 		})
 	})
@@ -50,7 +53,7 @@ var _ = Describe("Teams", Label("teams"), func() {
 				model.NewEmptyAtlasKeyType().UseDefaultFullAccess(),
 				40000,
 				[]func(*model.TestDataProvider){},
-			).WithProject(data.DefaultProject()),
+			).WithProject(data.DefaultProject()).WithObjectDeletionProtection(true),
 			[]akov2.Team{
 				{
 					TeamRef: common.ResourceRefNamespaced{
@@ -120,10 +123,33 @@ func projectTeamsFlow(userData *model.TestDataProvider, teams []akov2.Team) {
 		Expect(userData.K8SClient.Update(userData.Context, userData.Project)).Should(Succeed())
 		Eventually(func(g Gomega) bool {
 			return ensureTeamsStatus(g, *userData, teams, teamWasRemoved)
-		}).WithTimeout(10*time.Minute).WithPolling(20*time.Second).Should(BeTrue(), "Team were not removed")
+		}).WithTimeout(10*time.Minute).WithPolling(20*time.Second).Should(BeTrue(), "Teams were not removed")
 
 		actions.CheckProjectConditionsNotSet(userData, api.ProjectTeamsReadyType)
 	})
+
+	if userData.ObjectDeletionProtection {
+		aClient := atlas.GetClientOrFail()
+		By("Cleanup Atlas Teams: which should have not been removed due to deletion protection", func() {
+			atlasTeams, err := listAtLeastNTeams(userData.Context, aClient, len(teams))
+			Expect(err).Should(Succeed())
+			Expect(clearAtlasTeams(teams, atlasTeams, aClient, userData)).Should(Succeed())
+		})
+
+		By("Cleanup Atlas Project: as deletion protection will skip that", func() {
+			Expect(userData.K8SClient.Get(userData.Context, types.NamespacedName{
+				Name:      userData.Project.Name,
+				Namespace: userData.Project.Namespace,
+			}, userData.Project)).Should(Succeed())
+			projectID := userData.Project.Status.ID
+			Expect(userData.K8SClient.Delete(userData.Context, userData.Project)).Should(Succeed())
+			_, _, err := aClient.Client.ProjectsApi.DeleteProject(userData.Context, projectID).Execute()
+			Expect(err).Should(Succeed())
+		})
+	} else {
+		actions.DeleteTestDataTeams(userData)
+		actions.DeleteTestDataProject(userData)
+	}
 }
 
 func ensureTeamsStatus(g Gomega, testData model.TestDataProvider, teams []akov2.Team, check func(res *akov2.AtlasTeam) bool) bool {
@@ -136,7 +162,6 @@ func ensureTeamsStatus(g Gomega, testData model.TestDataProvider, teams []akov2.
 			return false
 		}
 	}
-
 	return true
 }
 
@@ -147,3 +172,43 @@ func teamWasCreated(team *akov2.AtlasTeam) bool {
 func teamWasRemoved(team *akov2.AtlasTeam) bool {
 	return team.Status.ID == ""
 }
+
+func listAtLeastNTeams(ctx context.Context, aClient *atlas.Atlas, minTeams int) ([]admin.TeamResponse, error) {
+	results := []admin.TeamResponse{}
+	teamsReply, _, err := aClient.Client.TeamsApi.ListOrganizationTeams(ctx, aClient.OrgID).Execute()
+	if err != nil {
+		return results, fmt.Errorf("failed to list teams: %w", err)
+	}
+	total, ok := teamsReply.GetTotalCountOk()
+	if !ok {
+		return results, errors.New("no results")
+	}
+	if *total < minTeams {
+		return results, fmt.Errorf("not enough teams: expected %d but got %d", minTeams, *total)
+	}
+	return teamsReply.GetResults(), nil
+}
+
+func clearAtlasTeams(teams []akov2.Team, atlasTeams []admin.TeamResponse, aClient *atlas.Atlas, userData *model.TestDataProvider) error {
+	var errs error
+	for _, team := range teams {
+		foundAtlasTeam := findTeam(atlasTeams, team.TeamRef.Name)
+		if foundAtlasTeam == nil {
+			errs = errors.Join(errs, fmt.Errorf("failed to find expected Atlas team %s (was it wrongly removed?)", team.TeamRef.Name))
+		}
+		_, _, err := aClient.Client.TeamsApi.DeleteTeam(userData.Context, aClient.OrgID, foundAtlasTeam.GetId()).Execute()
+		if err != nil {
+			errs = errors.Join(errs, err)
+		}
+	}
+	return errs
+}
+
+func findTeam(atlasTeams []admin.TeamResponse, teamName string) *admin.TeamResponse {
+	for _, atlasTeam := range atlasTeams {
+		if teamName == atlasTeam.GetName() {
+			return &atlasTeam
+		}
+	}
+	return nil
+}
