Skip to content

Commit 67d02e9

Browse files
josvazgjosvazg
authored
CLOUDP-306517: Fix greediness for IP Access List and Network Peerings (#2197)
* Add non greedy testing Signed-off-by: jose.vazquez <[email protected]> * Make Network peering less greedy Signed-off-by: jose.vazquez <[email protected]> * Fix IP Access List to be greedy when active --------- Signed-off-by: jose.vazquez <[email protected]>
1 parent dc9c61b commit 67d02e9

9 files changed

+604
-49
lines changed

‎internal/controller/atlasproject/atlasproject_controller.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -348,17 +348,18 @@ func logIfWarning(ctx *workflow.Context, result workflow.Result) {
348348
}
349349
}
350350

351-
func lastSpecFrom(atlasProject *akov2.AtlasProject, annotation string) (*akov2.AtlasProjectSpec, error) {
351+
func lastAppliedSpecFrom(atlasProject *akov2.AtlasProject) (*akov2.AtlasProjectSpec, error) {
352352
var lastApplied akov2.AtlasProject
353-
ann, ok := atlasProject.GetAnnotations()[annotation]
353+
ann, ok := atlasProject.GetAnnotations()[customresource.AnnotationLastAppliedConfiguration]
354354

355355
if !ok {
356356
return nil, nil
357357
}
358358

359359
err := json.Unmarshal([]byte(ann), &lastApplied.Spec)
360360
if err != nil {
361-
return nil, fmt.Errorf("error reading AtlasProject Spec from annotation [%s]: %w", annotation, err)
361+
return nil, fmt.Errorf("error reading AtlasProject Spec from annotation [%s]: %w",
362+
customresource.AnnotationLastAppliedConfiguration, err)
362363
}
363364

364365
return &lastApplied.Spec, nil

‎internal/controller/atlasproject/atlasproject_controller_test.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package atlasproject
22

33
import (
44
"context"
5+
"encoding/json"
56
"net/http"
67
"reflect"
78
"testing"
@@ -418,11 +419,19 @@ func TestLastSpecFrom(t *testing.T) {
418419
t.Run(name, func(t *testing.T) {
419420
p := &akov2.AtlasProject{}
420421
p.WithAnnotations(tt.annotations)
421-
lastSpec, err := lastSpecFrom(p, "mongodb.com/last-applied-configuration")
422+
lastSpec, err := lastAppliedSpecFrom(p)
422423
if err != nil {
423424
assert.ErrorContains(t, err, tt.expectedError)
424425
}
425426
assert.Equal(t, tt.expectedLastSpec, lastSpec)
426427
})
427428
}
428429
}
430+
431+
func jsonize(t *testing.T, obj any) string {
432+
t.Helper()
433+
434+
js, err := json.Marshal(obj)
435+
require.NoError(t, err)
436+
return string(js)
437+
}

‎internal/controller/atlasproject/custom_roles_test.go

Lines changed: 113 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,13 @@ import (
66
"net/http"
77
"testing"
88

9-
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
10-
119
"github.com/stretchr/testify/assert"
1210
"github.com/stretchr/testify/mock"
11+
"github.com/stretchr/testify/require"
1312
"go.mongodb.org/atlas-sdk/v20231115008/admin"
1413
"go.mongodb.org/atlas-sdk/v20231115008/mockadmin"
1514
"go.uber.org/zap/zaptest"
15+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1616

1717
akov2 "github.com/mongodb/mongodb-atlas-kubernetes/v2/api/v1"
1818
"github.com/mongodb/mongodb-atlas-kubernetes/v2/internal/controller/customresource"
@@ -449,3 +449,114 @@ func TestEnsureCustomRoles(t *testing.T) {
449449
})
450450
}
451451
}
452+
453+
func TestCustomRolesNonGreedyBehaviour(t *testing.T) {
454+
for _, tc := range []struct {
455+
title string
456+
lastAppliedCustomRoles []string
457+
specCustomRoles []string
458+
atlasCustomRoles []string
459+
wantRemoved []string
460+
}{
461+
{
462+
title: "no last applied no removal in Atlas",
463+
lastAppliedCustomRoles: []string{},
464+
specCustomRoles: []string{},
465+
atlasCustomRoles: []string{"cr1", "cr2"},
466+
wantRemoved: []string{},
467+
},
468+
{
469+
title: "removed from last applied removes from Atlas",
470+
lastAppliedCustomRoles: []string{"cr1", "cr2"},
471+
specCustomRoles: []string{"cr1"},
472+
atlasCustomRoles: []string{"cr1", "cr2"},
473+
wantRemoved: []string{"cr2"},
474+
},
475+
{
476+
title: "removed all from last applied removes all from Atlas",
477+
lastAppliedCustomRoles: []string{"cr1", "cr2"},
478+
specCustomRoles: []string{},
479+
atlasCustomRoles: []string{"cr1", "cr2"},
480+
wantRemoved: []string{"cr1", "cr2"},
481+
},
482+
{
483+
title: "not in last applied not removed from Atlas",
484+
lastAppliedCustomRoles: []string{"cr1"},
485+
specCustomRoles: []string{"cr1"},
486+
atlasCustomRoles: []string{"cr1", "cr2"},
487+
wantRemoved: []string{},
488+
},
489+
} {
490+
t.Run(tc.title, func(t *testing.T) {
491+
prj := newCustomRolesTestProject(tc.specCustomRoles)
492+
lastPrj := newCustomRolesTestProject(tc.lastAppliedCustomRoles)
493+
prj.Annotations[customresource.AnnotationLastAppliedConfiguration] = jsonize(t, lastPrj.Spec)
494+
495+
roleAPI := mockadmin.NewCustomDatabaseRolesApi(t)
496+
roleAPI.EXPECT().ListCustomDatabaseRoles(mock.Anything, mock.Anything).
497+
Return(admin.ListCustomDatabaseRolesApiRequest{ApiService: roleAPI}).Once()
498+
roleAPI.EXPECT().ListCustomDatabaseRolesExecute(
499+
mock.AnythingOfType("admin.ListCustomDatabaseRolesApiRequest")).Return(
500+
synthesizeAtlasCustomRoles(tc.atlasCustomRoles), nil, nil,
501+
).Once()
502+
503+
removals := len(tc.wantRemoved)
504+
if removals > 0 {
505+
roleAPI.EXPECT().DeleteCustomDatabaseRole(
506+
mock.Anything, mock.Anything, mock.Anything,
507+
).Return(admin.DeleteCustomDatabaseRoleApiRequest{ApiService: roleAPI}).Times(removals)
508+
roleAPI.EXPECT().DeleteCustomDatabaseRoleExecute(
509+
mock.AnythingOfType("admin.DeleteCustomDatabaseRoleApiRequest")).Return(
510+
nil, nil,
511+
).Times(removals)
512+
}
513+
514+
workflowCtx := workflow.Context{
515+
Log: zaptest.NewLogger(t).Sugar(),
516+
Context: context.Background(),
517+
SdkClient: &admin.APIClient{
518+
CustomDatabaseRolesApi: roleAPI,
519+
},
520+
}
521+
522+
result := ensureCustomRoles(&workflowCtx, prj)
523+
require.Equal(t, workflow.OK(), result)
524+
})
525+
}
526+
}
527+
528+
func newCustomRolesTestProject(customRoles []string) *akov2.AtlasProject {
529+
return &akov2.AtlasProject{
530+
ObjectMeta: metav1.ObjectMeta{
531+
Annotations: map[string]string{},
532+
},
533+
Spec: akov2.AtlasProjectSpec{
534+
Name: "test-project",
535+
CustomRoles: synthesizeCustomRoles(customRoles),
536+
},
537+
}
538+
}
539+
540+
func synthesizeCustomRoles(customRoles []string) []akov2.CustomRole {
541+
crs := make([]akov2.CustomRole, 0, len(customRoles))
542+
for _, name := range customRoles {
543+
crs = append(crs, akov2.CustomRole{
544+
Name: name,
545+
InheritedRoles: []akov2.Role{},
546+
Actions: []akov2.Action{},
547+
})
548+
}
549+
return crs
550+
}
551+
552+
func synthesizeAtlasCustomRoles(customRoles []string) []admin.UserCustomDBRole {
553+
atlasRoles := make([]admin.UserCustomDBRole, 0, len(customRoles))
554+
for _, name := range customRoles {
555+
atlasRoles = append(atlasRoles, admin.UserCustomDBRole{
556+
RoleName: name,
557+
Actions: &[]admin.DatabasePrivilegeAction{},
558+
InheritedRoles: &[]admin.DatabaseInheritedRole{},
559+
})
560+
}
561+
return atlasRoles
562+
}

‎internal/controller/atlasproject/ipaccess_list.go

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -55,15 +55,14 @@ func (i *ipAccessListController) configure(current, desired ipaccesslist.IPAcces
5555
return i.terminate(workflow.ProjectIPNotCreatedInAtlas, err)
5656
}
5757

58-
for key, entry := range current {
59-
if _, ok := i.lastApplied[key]; !ok {
60-
continue
61-
}
62-
63-
if _, ok := desired[key]; !ok {
64-
err = i.service.Delete(i.ctx.Context, i.project.ID(), entry)
65-
if err != nil {
66-
return i.terminate(workflow.ProjectIPNotCreatedInAtlas, err)
58+
managedEntries := len(i.lastApplied) > 0
59+
if managedEntries {
60+
for key, entry := range current {
61+
if _, ok := desired[key]; !ok {
62+
err = i.service.Delete(i.ctx.Context, i.project.ID(), entry)
63+
if err != nil {
64+
return i.terminate(workflow.ProjectIPNotCreatedInAtlas, err)
65+
}
6766
}
6867
}
6968
}
@@ -173,7 +172,7 @@ func shouldIPAccessListSkipReconciliation(atlasProject *akov2.AtlasProject) (boo
173172
}
174173

175174
func mapLastAppliedIPAccessList(atlasProject *akov2.AtlasProject) (ipaccesslist.IPAccessEntries, error) {
176-
lastApplied, err := lastSpecFrom(atlasProject, customresource.AnnotationLastAppliedConfiguration)
175+
lastApplied, err := lastAppliedSpecFrom(atlasProject)
177176
if err != nil {
178177
return nil, err
179178
}

‎internal/controller/atlasproject/ipaccess_list_test.go

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,19 @@ package atlasproject
33
import (
44
"context"
55
"errors"
6+
"fmt"
67
"net/http"
78
"testing"
89

910
"github.com/google/go-cmp/cmp"
1011
"github.com/google/go-cmp/cmp/cmpopts"
1112
"github.com/stretchr/testify/assert"
1213
"github.com/stretchr/testify/mock"
14+
"github.com/stretchr/testify/require"
1315
"go.mongodb.org/atlas-sdk/v20231115008/admin"
1416
"go.mongodb.org/atlas-sdk/v20231115008/mockadmin"
1517
"go.uber.org/zap/zaptest"
18+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1619

1720
"github.com/mongodb/mongodb-atlas-kubernetes/v2/api"
1821
akov2 "github.com/mongodb/mongodb-atlas-kubernetes/v2/api/v1"
@@ -570,3 +573,132 @@ func TestHasSkippedIPAccessListConfiguration(t *testing.T) {
570573
})
571574
}
572575
}
576+
577+
func TestIPAccessListNonGreedyBehaviour(t *testing.T) {
578+
for _, tc := range []struct {
579+
title string
580+
lastAppliedIPAccessList []string
581+
specIPAccessList []string
582+
atlasIPAccessList []string
583+
wantRemoved []string
584+
}{
585+
{
586+
title: "no last applied no removal in Atlas",
587+
lastAppliedIPAccessList: []string{},
588+
specIPAccessList: []string{},
589+
atlasIPAccessList: []string{"100.90.0.0/24", "101.99.0.0/24"},
590+
wantRemoved: []string{},
591+
},
592+
{
593+
title: "removed from last applied removes from Atlas",
594+
lastAppliedIPAccessList: []string{"100.90.0.0/24", "101.99.0.0/24"},
595+
specIPAccessList: []string{"100.90.0.0/24"},
596+
atlasIPAccessList: []string{"100.90.0.0/24", "101.99.0.0/24"},
597+
wantRemoved: []string{"101.99.0.0/24"},
598+
},
599+
{
600+
title: "removed all from last applied removes all from Atlas",
601+
lastAppliedIPAccessList: []string{"100.90.0.0/24", "101.99.0.0/24"},
602+
specIPAccessList: []string{},
603+
atlasIPAccessList: []string{"100.90.0.0/24", "101.99.0.0/24"},
604+
wantRemoved: []string{"100.90.0.0/24", "101.99.0.0/24"},
605+
},
606+
{
607+
title: "not in last applied still removed from Atlas",
608+
lastAppliedIPAccessList: []string{"100.90.0.0/24"},
609+
specIPAccessList: []string{"100.90.0.0/24"},
610+
atlasIPAccessList: []string{"100.90.0.0/24", "101.99.0.0/24"},
611+
wantRemoved: []string{"101.99.0.0/24"},
612+
},
613+
} {
614+
t.Run(tc.title, func(t *testing.T) {
615+
prj := newIPAccessListTestProject(tc.specIPAccessList)
616+
lastPrj := newIPAccessListTestProject(tc.lastAppliedIPAccessList)
617+
prj.Annotations[customresource.AnnotationLastAppliedConfiguration] = jsonize(t, lastPrj.Spec)
618+
619+
ipAccessAPI := mockadmin.NewProjectIPAccessListApi(t)
620+
ipAccessAPI.EXPECT().ListProjectIpAccessLists(mock.Anything, mock.Anything).
621+
Return(admin.ListProjectIpAccessListsApiRequest{ApiService: ipAccessAPI}).Once()
622+
ipAccessAPI.EXPECT().ListProjectIpAccessListsExecute(
623+
mock.AnythingOfType("admin.ListProjectIpAccessListsApiRequest")).Return(
624+
synthesizeAtlasIPAccessList(tc.atlasIPAccessList), nil, nil,
625+
).Once()
626+
// CreateProjectIpAccessList is a non destrutive operation, it does not remove entries
627+
ipAccessAPI.EXPECT().CreateProjectIpAccessList(mock.Anything, mock.Anything, mock.Anything).
628+
Return(admin.CreateProjectIpAccessListApiRequest{ApiService: ipAccessAPI}).Once()
629+
ipAccessAPI.EXPECT().CreateProjectIpAccessListExecute(
630+
mock.AnythingOfType("admin.CreateProjectIpAccessListApiRequest")).Return(
631+
nil, nil, nil,
632+
).Once()
633+
634+
removals := len(tc.wantRemoved)
635+
if removals > 0 {
636+
ipAccessAPI.EXPECT().DeleteProjectIpAccessList(
637+
mock.Anything, mock.Anything, mock.Anything,
638+
).Return(admin.DeleteProjectIpAccessListApiRequest{ApiService: ipAccessAPI}).Times(removals)
639+
ipAccessAPI.EXPECT().DeleteProjectIpAccessListExecute(
640+
mock.AnythingOfType("admin.DeleteProjectIpAccessListApiRequest")).Return(
641+
nil, nil, nil,
642+
).Times(removals)
643+
}
644+
645+
unset := len(tc.specIPAccessList) == 0
646+
if !unset {
647+
ipAccessAPI.EXPECT().GetProjectIpAccessListStatus(
648+
mock.Anything, mock.Anything, mock.Anything,
649+
).Return(admin.GetProjectIpAccessListStatusApiRequest{ApiService: ipAccessAPI}).Times(removals)
650+
ipAccessAPI.EXPECT().GetProjectIpAccessListStatusExecute(
651+
mock.AnythingOfType("admin.GetProjectIpAccessListStatusApiRequest")).Return(
652+
nil, nil, nil,
653+
).Times(removals)
654+
}
655+
656+
workflowCtx := workflow.Context{
657+
Log: zaptest.NewLogger(t).Sugar(),
658+
Context: context.Background(),
659+
SdkClient: &admin.APIClient{
660+
ProjectIPAccessListApi: ipAccessAPI,
661+
},
662+
}
663+
664+
result := handleIPAccessList(&workflowCtx, prj)
665+
require.Equal(t, workflow.OK(), result)
666+
})
667+
}
668+
}
669+
670+
func newIPAccessListTestProject(ipAccessList []string) *akov2.AtlasProject {
671+
return &akov2.AtlasProject{
672+
ObjectMeta: metav1.ObjectMeta{
673+
Annotations: map[string]string{},
674+
},
675+
Spec: akov2.AtlasProjectSpec{
676+
Name: "test-project",
677+
ProjectIPAccessList: synthesizeIPAccessList(ipAccessList),
678+
},
679+
}
680+
}
681+
682+
func synthesizeIPAccessList(ipAccessList []string) []project.IPAccessList {
683+
peers := make([]project.IPAccessList, 0, len(ipAccessList))
684+
for _, cidr := range ipAccessList {
685+
peers = append(peers, project.IPAccessList{
686+
CIDRBlock: cidr,
687+
Comment: fmt.Sprintf("fake CIDR block %s", cidr),
688+
})
689+
}
690+
return peers
691+
}
692+
693+
func synthesizeAtlasIPAccessList(peeringIDs []string) *admin.PaginatedNetworkAccess {
694+
atlasIPAccessList := make([]admin.NetworkPermissionEntry, 0, len(peeringIDs))
695+
for _, cidr := range peeringIDs {
696+
atlasIPAccessList = append(atlasIPAccessList, admin.NetworkPermissionEntry{
697+
CidrBlock: pointer.MakePtr(cidr),
698+
Comment: pointer.MakePtr(fmt.Sprintf("fake CIDR block %s", cidr)),
699+
})
700+
}
701+
return &admin.PaginatedNetworkAccess{
702+
Results: &atlasIPAccessList,
703+
}
704+
}

0 commit comments

Comments
 (0)