CLOUDP-306517: Fix greediness for IP Access List and Network Peerings

internal/controller/atlasproject/atlasproject_controller.go

@@ -348,17 +348,18 @@ func logIfWarning(ctx *workflow.Context, result workflow.Result) {
 	}
 }
 
-func lastSpecFrom(atlasProject *akov2.AtlasProject, annotation string) (*akov2.AtlasProjectSpec, error) {
+func lastAppliedSpecFrom(atlasProject *akov2.AtlasProject) (*akov2.AtlasProjectSpec, error) {
 	var lastApplied akov2.AtlasProject
-	ann, ok := atlasProject.GetAnnotations()[annotation]
+	ann, ok := atlasProject.GetAnnotations()[customresource.AnnotationLastAppliedConfiguration]
 
 	if !ok {
 		return nil, nil
 	}
 
 	err := json.Unmarshal([]byte(ann), &lastApplied.Spec)
 	if err != nil {
-		return nil, fmt.Errorf("error reading AtlasProject Spec from annotation [%s]: %w", annotation, err)
+		return nil, fmt.Errorf("error reading AtlasProject Spec from annotation [%s]: %w",
+			customresource.AnnotationLastAppliedConfiguration, err)
 	}
 
 	return &lastApplied.Spec, nil

internal/controller/atlasproject/atlasproject_controller_test.go

@@ -2,6 +2,7 @@ package atlasproject
 
 import (
 	"context"
+	"encoding/json"
 	"net/http"
 	"reflect"
 	"testing"
@@ -418,11 +419,19 @@ func TestLastSpecFrom(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			p := &akov2.AtlasProject{}
 			p.WithAnnotations(tt.annotations)
-			lastSpec, err := lastSpecFrom(p, "mongodb.com/last-applied-configuration")
+			lastSpec, err := lastAppliedSpecFrom(p)
 			if err != nil {
 				assert.ErrorContains(t, err, tt.expectedError)
 			}
 			assert.Equal(t, tt.expectedLastSpec, lastSpec)
 		})
 	}
 }
+
+func jsonize(t *testing.T, obj any) string {
+	t.Helper()
+
+	js, err := json.Marshal(obj)
+	require.NoError(t, err)
+	return string(js)
+}

internal/controller/atlasproject/custom_roles_test.go

@@ -6,13 +6,13 @@ import (
 	"net/http"
 	"testing"
 
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
 	"go.mongodb.org/atlas-sdk/v20231115008/admin"
 	"go.mongodb.org/atlas-sdk/v20231115008/mockadmin"
 	"go.uber.org/zap/zaptest"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	akov2 "github.com/mongodb/mongodb-atlas-kubernetes/v2/api/v1"
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/internal/controller/customresource"
@@ -449,3 +449,114 @@ func TestEnsureCustomRoles(t *testing.T) {
 		})
 	}
 }
+
+func TestCustomRolesNonGreedyBehaviour(t *testing.T) {
+	for _, tc := range []struct {
+		title                  string
+		lastAppliedCustomRoles []string
+		specCustomRoles        []string
+		atlasCustomRoles       []string
+		wantRemoved            []string
+	}{
+		{
+			title:                  "no last applied no removal in Atlas",
+			lastAppliedCustomRoles: []string{},
+			specCustomRoles:        []string{},
+			atlasCustomRoles:       []string{"cr1", "cr2"},
+			wantRemoved:            []string{},
+		},
+		{
+			title:                  "removed from last applied removes from Atlas",
+			lastAppliedCustomRoles: []string{"cr1", "cr2"},
+			specCustomRoles:        []string{"cr1"},
+			atlasCustomRoles:       []string{"cr1", "cr2"},
+			wantRemoved:            []string{"cr2"},
+		},
+		{
+			title:                  "removed all from last applied removes all from Atlas",
+			lastAppliedCustomRoles: []string{"cr1", "cr2"},
+			specCustomRoles:        []string{},
+			atlasCustomRoles:       []string{"cr1", "cr2"},
+			wantRemoved:            []string{"cr1", "cr2"},
+		},
+		{
+			title:                  "not in last applied not removed from Atlas",
+			lastAppliedCustomRoles: []string{"cr1"},
+			specCustomRoles:        []string{"cr1"},
+			atlasCustomRoles:       []string{"cr1", "cr2"},
+			wantRemoved:            []string{},
+		},
+	} {
+		t.Run(tc.title, func(t *testing.T) {
+			prj := newCustomRolesTestProject(tc.specCustomRoles)
+			lastPrj := newCustomRolesTestProject(tc.lastAppliedCustomRoles)
+			prj.Annotations[customresource.AnnotationLastAppliedConfiguration] = jsonize(t, lastPrj.Spec)
+
+			roleAPI := mockadmin.NewCustomDatabaseRolesApi(t)
+			roleAPI.EXPECT().ListCustomDatabaseRoles(mock.Anything, mock.Anything).
+				Return(admin.ListCustomDatabaseRolesApiRequest{ApiService: roleAPI}).Once()
+			roleAPI.EXPECT().ListCustomDatabaseRolesExecute(
+				mock.AnythingOfType("admin.ListCustomDatabaseRolesApiRequest")).Return(
+				synthesizeAtlasCustomRoles(tc.atlasCustomRoles), nil, nil,
+			).Once()
+
+			removals := len(tc.wantRemoved)
+			if removals > 0 {
+				roleAPI.EXPECT().DeleteCustomDatabaseRole(
+					mock.Anything, mock.Anything, mock.Anything,
+				).Return(admin.DeleteCustomDatabaseRoleApiRequest{ApiService: roleAPI}).Times(removals)
+				roleAPI.EXPECT().DeleteCustomDatabaseRoleExecute(
+					mock.AnythingOfType("admin.DeleteCustomDatabaseRoleApiRequest")).Return(
+					nil, nil,
+				).Times(removals)
+			}
+
+			workflowCtx := workflow.Context{
+				Log:     zaptest.NewLogger(t).Sugar(),
+				Context: context.Background(),
+				SdkClient: &admin.APIClient{
+					CustomDatabaseRolesApi: roleAPI,
+				},
+			}
+
+			result := ensureCustomRoles(&workflowCtx, prj)
+			require.Equal(t, workflow.OK(), result)
+		})
+	}
+}
+
+func newCustomRolesTestProject(customRoles []string) *akov2.AtlasProject {
+	return &akov2.AtlasProject{
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: map[string]string{},
+		},
+		Spec: akov2.AtlasProjectSpec{
+			Name:        "test-project",
+			CustomRoles: synthesizeCustomRoles(customRoles),
+		},
+	}
+}
+
+func synthesizeCustomRoles(customRoles []string) []akov2.CustomRole {
+	crs := make([]akov2.CustomRole, 0, len(customRoles))
+	for _, name := range customRoles {
+		crs = append(crs, akov2.CustomRole{
+			Name:           name,
+			InheritedRoles: []akov2.Role{},
+			Actions:        []akov2.Action{},
+		})
+	}
+	return crs
+}
+
+func synthesizeAtlasCustomRoles(customRoles []string) []admin.UserCustomDBRole {
+	atlasRoles := make([]admin.UserCustomDBRole, 0, len(customRoles))
+	for _, name := range customRoles {
+		atlasRoles = append(atlasRoles, admin.UserCustomDBRole{
+			RoleName:       name,
+			Actions:        &[]admin.DatabasePrivilegeAction{},
+			InheritedRoles: &[]admin.DatabaseInheritedRole{},
+		})
+	}
+	return atlasRoles
+}

internal/controller/atlasproject/ipaccess_list.go

@@ -55,15 +55,14 @@ func (i *ipAccessListController) configure(current, desired ipaccesslist.IPAcces
 		return i.terminate(workflow.ProjectIPNotCreatedInAtlas, err)
 	}
 
-	for key, entry := range current {
-		if _, ok := i.lastApplied[key]; !ok {
-			continue
-		}
-
-		if _, ok := desired[key]; !ok {
-			err = i.service.Delete(i.ctx.Context, i.project.ID(), entry)
-			if err != nil {
-				return i.terminate(workflow.ProjectIPNotCreatedInAtlas, err)
+	managedEntries := len(i.lastApplied) > 0
+	if managedEntries {
+		for key, entry := range current {
+			if _, ok := desired[key]; !ok {
+				err = i.service.Delete(i.ctx.Context, i.project.ID(), entry)
+				if err != nil {
+					return i.terminate(workflow.ProjectIPNotCreatedInAtlas, err)
+				}
 			}
 		}
 	}
@@ -173,7 +172,7 @@ func shouldIPAccessListSkipReconciliation(atlasProject *akov2.AtlasProject) (boo
 }
 
 func mapLastAppliedIPAccessList(atlasProject *akov2.AtlasProject) (ipaccesslist.IPAccessEntries, error) {
-	lastApplied, err := lastSpecFrom(atlasProject, customresource.AnnotationLastAppliedConfiguration)
+	lastApplied, err := lastAppliedSpecFrom(atlasProject)
 	if err != nil {
 		return nil, err
 	}

internal/controller/atlasproject/ipaccess_list_test.go

@@ -3,16 +3,19 @@ package atlasproject
 import (
 	"context"
 	"errors"
+	"fmt"
 	"net/http"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
 	"go.mongodb.org/atlas-sdk/v20231115008/admin"
 	"go.mongodb.org/atlas-sdk/v20231115008/mockadmin"
 	"go.uber.org/zap/zaptest"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/api"
 	akov2 "github.com/mongodb/mongodb-atlas-kubernetes/v2/api/v1"
@@ -570,3 +573,132 @@ func TestHasSkippedIPAccessListConfiguration(t *testing.T) {
 		})
 	}
 }
+
+func TestIPAccessListNonGreedyBehaviour(t *testing.T) {
+	for _, tc := range []struct {
+		title                   string
+		lastAppliedIPAccessList []string
+		specIPAccessList        []string
+		atlasIPAccessList       []string
+		wantRemoved             []string
+	}{
+		{
+			title:                   "no last applied no removal in Atlas",
+			lastAppliedIPAccessList: []string{},
+			specIPAccessList:        []string{},
+			atlasIPAccessList:       []string{"100.90.0.0/24", "101.99.0.0/24"},
+			wantRemoved:             []string{},
+		},
+		{
+			title:                   "removed from last applied removes from Atlas",
+			lastAppliedIPAccessList: []string{"100.90.0.0/24", "101.99.0.0/24"},
+			specIPAccessList:        []string{"100.90.0.0/24"},
+			atlasIPAccessList:       []string{"100.90.0.0/24", "101.99.0.0/24"},
+			wantRemoved:             []string{"101.99.0.0/24"},
+		},
+		{
+			title:                   "removed all from last applied removes all from Atlas",
+			lastAppliedIPAccessList: []string{"100.90.0.0/24", "101.99.0.0/24"},
+			specIPAccessList:        []string{},
+			atlasIPAccessList:       []string{"100.90.0.0/24", "101.99.0.0/24"},
+			wantRemoved:             []string{"100.90.0.0/24", "101.99.0.0/24"},
+		},
+		{
+			title:                   "not in last applied still removed from Atlas",
+			lastAppliedIPAccessList: []string{"100.90.0.0/24"},
+			specIPAccessList:        []string{"100.90.0.0/24"},
+			atlasIPAccessList:       []string{"100.90.0.0/24", "101.99.0.0/24"},
+			wantRemoved:             []string{"101.99.0.0/24"},
+		},
+	} {
+		t.Run(tc.title, func(t *testing.T) {
+			prj := newIPAccessListTestProject(tc.specIPAccessList)
+			lastPrj := newIPAccessListTestProject(tc.lastAppliedIPAccessList)
+			prj.Annotations[customresource.AnnotationLastAppliedConfiguration] = jsonize(t, lastPrj.Spec)
+
+			ipAccessAPI := mockadmin.NewProjectIPAccessListApi(t)
+			ipAccessAPI.EXPECT().ListProjectIpAccessLists(mock.Anything, mock.Anything).
+				Return(admin.ListProjectIpAccessListsApiRequest{ApiService: ipAccessAPI}).Once()
+			ipAccessAPI.EXPECT().ListProjectIpAccessListsExecute(
+				mock.AnythingOfType("admin.ListProjectIpAccessListsApiRequest")).Return(
+				synthesizeAtlasIPAccessList(tc.atlasIPAccessList), nil, nil,
+			).Once()
+			// CreateProjectIpAccessList is a non destrutive operation, it does not remove entries
+			ipAccessAPI.EXPECT().CreateProjectIpAccessList(mock.Anything, mock.Anything, mock.Anything).
+				Return(admin.CreateProjectIpAccessListApiRequest{ApiService: ipAccessAPI}).Once()
+			ipAccessAPI.EXPECT().CreateProjectIpAccessListExecute(
+				mock.AnythingOfType("admin.CreateProjectIpAccessListApiRequest")).Return(
+				nil, nil, nil,
+			).Once()
+
+			removals := len(tc.wantRemoved)
+			if removals > 0 {
+				ipAccessAPI.EXPECT().DeleteProjectIpAccessList(
+					mock.Anything, mock.Anything, mock.Anything,
+				).Return(admin.DeleteProjectIpAccessListApiRequest{ApiService: ipAccessAPI}).Times(removals)
+				ipAccessAPI.EXPECT().DeleteProjectIpAccessListExecute(
+					mock.AnythingOfType("admin.DeleteProjectIpAccessListApiRequest")).Return(
+					nil, nil, nil,
+				).Times(removals)
+			}
+
+			unset := len(tc.specIPAccessList) == 0
+			if !unset {
+				ipAccessAPI.EXPECT().GetProjectIpAccessListStatus(
+					mock.Anything, mock.Anything, mock.Anything,
+				).Return(admin.GetProjectIpAccessListStatusApiRequest{ApiService: ipAccessAPI}).Times(removals)
+				ipAccessAPI.EXPECT().GetProjectIpAccessListStatusExecute(
+					mock.AnythingOfType("admin.GetProjectIpAccessListStatusApiRequest")).Return(
+					nil, nil, nil,
+				).Times(removals)
+			}
+
+			workflowCtx := workflow.Context{
+				Log:     zaptest.NewLogger(t).Sugar(),
+				Context: context.Background(),
+				SdkClient: &admin.APIClient{
+					ProjectIPAccessListApi: ipAccessAPI,
+				},
+			}
+
+			result := handleIPAccessList(&workflowCtx, prj)
+			require.Equal(t, workflow.OK(), result)
+		})
+	}
+}
+
+func newIPAccessListTestProject(ipAccessList []string) *akov2.AtlasProject {
+	return &akov2.AtlasProject{
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: map[string]string{},
+		},
+		Spec: akov2.AtlasProjectSpec{
+			Name:                "test-project",
+			ProjectIPAccessList: synthesizeIPAccessList(ipAccessList),
+		},
+	}
+}
+
+func synthesizeIPAccessList(ipAccessList []string) []project.IPAccessList {
+	peers := make([]project.IPAccessList, 0, len(ipAccessList))
+	for _, cidr := range ipAccessList {
+		peers = append(peers, project.IPAccessList{
+			CIDRBlock: cidr,
+			Comment:   fmt.Sprintf("fake CIDR block %s", cidr),
+		})
+	}
+	return peers
+}
+
+func synthesizeAtlasIPAccessList(peeringIDs []string) *admin.PaginatedNetworkAccess {
+	atlasIPAccessList := make([]admin.NetworkPermissionEntry, 0, len(peeringIDs))
+	for _, cidr := range peeringIDs {
+		atlasIPAccessList = append(atlasIPAccessList, admin.NetworkPermissionEntry{
+			CidrBlock: pointer.MakePtr(cidr),
+			Comment:   pointer.MakePtr(fmt.Sprintf("fake CIDR block %s", cidr)),
+		})
+	}
+	return &admin.PaginatedNetworkAccess{
+		Results: &atlasIPAccessList,
+	}
+}