From dccbfe526cfba3ea70f19c604545cb74af516f9d Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Tue, 17 Dec 2024 17:46:55 +0100 Subject: [PATCH 01/11] properly add and remove resources from groups --- management/cmd/management.go | 4 +- management/server/groups/manager.go | 17 ++- .../handlers/networks/resources_handler.go | 25 ---- .../server/networks/resources/manager.go | 124 +++++++++++++----- .../networks/resources/types/resource.go | 11 +- management/server/store/sql_store.go | 48 +++++++ management/server/store/sql_store_test.go | 34 ++++- management/server/store/store.go | 4 +- 8 files changed, 201 insertions(+), 66 deletions(-) diff --git a/management/cmd/management.go b/management/cmd/management.go index 3eb52eb9012..5301485cb0b 100644 --- a/management/cmd/management.go +++ b/management/cmd/management.go @@ -276,10 +276,10 @@ var ( userManager := users.NewManager(store) settingsManager := settings.NewManager(store) permissionsManager := permissions.NewManager(userManager, settingsManager) - resourcesManager := resources.NewManager(store, permissionsManager, accountManager) + groupsManager := groups.NewManager(store, permissionsManager) + resourcesManager := resources.NewManager(store, permissionsManager, groupsManager, accountManager) routersManager := routers.NewManager(store, permissionsManager, accountManager) networksManager := networks.NewManager(store, permissionsManager, resourcesManager) - groupsManager := groups.NewManager(store, permissionsManager) httpAPIHandler, err := httpapi.APIHandler(ctx, accountManager, networksManager, resourcesManager, routersManager, groupsManager, geo, *jwtValidator, appMetrics, httpAPIAuthCfg, integratedPeerValidator) if err != nil { diff --git a/management/server/groups/manager.go b/management/server/groups/manager.go index 9052770649a..8da4439529b 100644 --- a/management/server/groups/manager.go +++ b/management/server/groups/manager.go @@ -12,7 +12,10 @@ import ( type Manager interface { GetAllGroups(ctx context.Context, accountID, userID string) (map[string]*types.Group, error) + GetResourceGroupsInTransaction(ctx context.Context, transaction store.Store, lockingStrength store.LockingStrength, accountID, resourceID string) ([]*types.Group, error) AddResourceToGroup(ctx context.Context, accountID, userID, groupID string, resourceID *types.Resource) error + AddResourceToGroupInTransaction(ctx context.Context, transaction store.Store, accountID, groupID string, resourceID *types.Resource) error + RemoveResourceFromGroupInTransaction(ctx context.Context, transaction store.Store, accountID, groupID, resourceID string) error } type managerImpl struct { @@ -58,7 +61,19 @@ func (m *managerImpl) AddResourceToGroup(ctx context.Context, accountID, userID, return err } - return m.store.AddResourceToGroup(ctx, accountID, groupID, resource) + return m.AddResourceToGroupInTransaction(ctx, m.store, accountID, groupID, resource) +} + +func (m *managerImpl) AddResourceToGroupInTransaction(ctx context.Context, transaction store.Store, accountID, groupID string, resource *types.Resource) error { + return transaction.AddResourceToGroup(ctx, accountID, groupID, resource) +} + +func (m *managerImpl) RemoveResourceFromGroupInTransaction(ctx context.Context, transaction store.Store, accountID, groupID, resourceID string) error { + return transaction.RemoveResourceFromGroup(ctx, accountID, groupID, resourceID) +} + +func (m *managerImpl) GetResourceGroupsInTransaction(ctx context.Context, transaction store.Store, lockingStrength store.LockingStrength, accountID, resourceID string) ([]*types.Group, error) { + return transaction.GetResourceGroups(ctx, lockingStrength, accountID, resourceID) } func ToGroupsInfo(groups map[string]*types.Group, id string) []api.GroupMinimum { diff --git a/management/server/http/handlers/networks/resources_handler.go b/management/server/http/handlers/networks/resources_handler.go index 53fbb7b93f2..a0dc9a10def 100644 --- a/management/server/http/handlers/networks/resources_handler.go +++ b/management/server/http/handlers/networks/resources_handler.go @@ -14,7 +14,6 @@ import ( "github.com/netbirdio/netbird/management/server/jwtclaims" "github.com/netbirdio/netbird/management/server/networks/resources" "github.com/netbirdio/netbird/management/server/networks/resources/types" - nbtypes "github.com/netbirdio/netbird/management/server/types" ) type resourceHandler struct { @@ -130,18 +129,6 @@ func (h *resourceHandler) createResource(w http.ResponseWriter, r *http.Request) return } - res := nbtypes.Resource{ - ID: resource.ID, - Type: resource.Type.String(), - } - for _, groupID := range req.Groups { - err = h.groupsManager.AddResourceToGroup(r.Context(), accountID, userID, groupID, &res) - if err != nil { - util.WriteError(r.Context(), err, w) - return - } - } - grps, err := h.groupsManager.GetAllGroups(r.Context(), accountID, userID) if err != nil { util.WriteError(r.Context(), err, w) @@ -205,18 +192,6 @@ func (h *resourceHandler) updateResource(w http.ResponseWriter, r *http.Request) return } - res := nbtypes.Resource{ - ID: resource.ID, - Type: resource.Type.String(), - } - for _, groupID := range req.Groups { - err = h.groupsManager.AddResourceToGroup(r.Context(), accountID, userID, groupID, &res) - if err != nil { - util.WriteError(r.Context(), err, w) - return - } - } - grps, err := h.groupsManager.GetAllGroups(r.Context(), accountID, userID) if err != nil { util.WriteError(r.Context(), err, w) diff --git a/management/server/networks/resources/manager.go b/management/server/networks/resources/manager.go index 044c2dbb719..7d415d6a5c1 100644 --- a/management/server/networks/resources/manager.go +++ b/management/server/networks/resources/manager.go @@ -6,10 +6,13 @@ import ( "fmt" s "github.com/netbirdio/netbird/management/server" + "github.com/netbirdio/netbird/management/server/groups" "github.com/netbirdio/netbird/management/server/networks/resources/types" "github.com/netbirdio/netbird/management/server/permissions" "github.com/netbirdio/netbird/management/server/status" "github.com/netbirdio/netbird/management/server/store" + nbtypes "github.com/netbirdio/netbird/management/server/types" + "github.com/netbirdio/netbird/management/server/util" ) type Manager interface { @@ -26,13 +29,15 @@ type Manager interface { type managerImpl struct { store store.Store permissionsManager permissions.Manager + groupsManager groups.Manager accountManager s.AccountManager } -func NewManager(store store.Store, permissionsManager permissions.Manager, accountManager s.AccountManager) Manager { +func NewManager(store store.Store, permissionsManager permissions.Manager, groupsManager groups.Manager, accountManager s.AccountManager) Manager { return &managerImpl{ store: store, permissionsManager: permissionsManager, + groupsManager: groupsManager, accountManager: accountManager, } } @@ -92,17 +97,35 @@ func (m *managerImpl) CreateResource(ctx context.Context, userID string, resourc return nil, status.NewPermissionDeniedError() } - resource, err = types.NewNetworkResource(resource.AccountID, resource.NetworkID, resource.Name, resource.Description, resource.Address) + resource, err = types.NewNetworkResource(resource.AccountID, resource.NetworkID, resource.Name, resource.Description, resource.Address, resource.GroupIDs) if err != nil { return nil, fmt.Errorf("failed to create new network resource: %w", err) } - _, err = m.store.GetNetworkResourceByName(ctx, store.LockingStrengthShare, resource.AccountID, resource.Name) - if err == nil { - return nil, errors.New("resource already exists") - } - - err = m.store.SaveNetworkResource(ctx, store.LockingStrengthUpdate, resource) + err = m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error { + _, err = transaction.GetNetworkResourceByName(ctx, store.LockingStrengthShare, resource.AccountID, resource.Name) + if err == nil { + return errors.New("resource already exists") + } + + err = transaction.SaveNetworkResource(ctx, store.LockingStrengthUpdate, resource) + if err != nil { + return fmt.Errorf("failed to save network resource: %w", err) + } + + res := nbtypes.Resource{ + ID: resource.ID, + Type: resource.Type.String(), + } + for _, groupID := range resource.GroupIDs { + err = m.groupsManager.AddResourceToGroupInTransaction(ctx, transaction, resource.AccountID, groupID, &res) + if err != nil { + return fmt.Errorf("failed to add resource to group: %w", err) + } + } + + return nil + }) if err != nil { return nil, fmt.Errorf("failed to create network resource: %w", err) } @@ -151,17 +174,55 @@ func (m *managerImpl) UpdateResource(ctx context.Context, userID string, resourc resource.Domain = domain resource.Prefix = prefix - _, err = m.store.GetNetworkResourceByID(ctx, store.LockingStrengthShare, resource.AccountID, resource.ID) - if err != nil { - return nil, fmt.Errorf("failed to get network resource: %w", err) - } - - oldResource, err := m.store.GetNetworkResourceByName(ctx, store.LockingStrengthShare, resource.AccountID, resource.Name) - if err == nil && oldResource.ID != resource.ID { - return nil, errors.New("new resource name already exists") - } - - err = m.store.SaveNetworkResource(ctx, store.LockingStrengthUpdate, resource) + err = m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error { + _, err = transaction.GetNetworkResourceByID(ctx, store.LockingStrengthShare, resource.AccountID, resource.ID) + if err != nil { + return fmt.Errorf("failed to get network resource: %w", err) + } + + oldResource, err := transaction.GetNetworkResourceByName(ctx, store.LockingStrengthShare, resource.AccountID, resource.Name) + if err == nil && oldResource.ID != resource.ID { + return errors.New("new resource name already exists") + } + + err = transaction.SaveNetworkResource(ctx, store.LockingStrengthUpdate, resource) + if err != nil { + return fmt.Errorf("failed to save network resource: %w", err) + } + + res := nbtypes.Resource{ + ID: resource.ID, + Type: resource.Type.String(), + } + + oldResourceGroups, err := m.groupsManager.GetResourceGroupsInTransaction(ctx, transaction, store.LockingStrengthUpdate, oldResource.AccountID, oldResource.ID) + if err != nil { + return fmt.Errorf("failed to get resource groups: %w", err) + } + + oldGroupsIds := make([]string, 0) + for _, group := range oldResourceGroups { + oldGroupsIds = append(oldGroupsIds, group.ID) + } + + groupsToAdd := util.Difference(resource.GroupIDs, oldGroupsIds) + for _, groupID := range groupsToAdd { + err = m.groupsManager.AddResourceToGroupInTransaction(ctx, transaction, resource.AccountID, groupID, &res) + if err != nil { + return fmt.Errorf("failed to add resource to group: %w", err) + } + } + + groupsToRemove := util.Difference(oldGroupsIds, resource.GroupIDs) + for _, groupID := range groupsToRemove { + err = m.groupsManager.RemoveResourceFromGroupInTransaction(ctx, transaction, resource.AccountID, groupID, res.ID) + if err != nil { + return fmt.Errorf("failed to add resource to group: %w", err) + } + } + + return nil + }) if err != nil { return nil, fmt.Errorf("failed to update network resource: %w", err) } @@ -186,13 +247,13 @@ func (m *managerImpl) DeleteResource(ctx context.Context, accountID, userID, net err = m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error { return m.DeleteResourceInTransaction(ctx, transaction, accountID, networkID, resourceID) }) - if err != nil { + if err != nil { return fmt.Errorf("failed to delete network resource: %w", err) } - - go m.accountManager.UpdateAccountPeers(ctx, accountID) - - return nil + + go m.accountManager.UpdateAccountPeers(ctx, accountID) + + return nil } func (m *managerImpl) DeleteResourceInTransaction(ctx context.Context, transaction store.Store, accountID, networkID, resourceID string) error { @@ -205,15 +266,12 @@ func (m *managerImpl) DeleteResourceInTransaction(ctx context.Context, transacti return errors.New("resource not part of network") } - account, err := transaction.GetAccount(ctx, accountID) - if err != nil { - return fmt.Errorf("failed to get account: %w", err) - } - account.DeleteResource(resource.ID) - - err = transaction.SaveAccount(ctx, account) - if err != nil { - return fmt.Errorf("failed to save account: %w", err) + groups, err := m.groupsManager.GetResourceGroupsInTransaction(ctx, transaction, store.LockingStrengthUpdate, accountID, resourceID) + for _, group := range groups { + err = m.groupsManager.RemoveResourceFromGroupInTransaction(ctx, transaction, accountID, group.ID, resourceID) + if err != nil { + return fmt.Errorf("failed to remove resource from group: %w", err) + } } err = transaction.IncrementNetworkSerial(ctx, store.LockingStrengthUpdate, accountID) diff --git a/management/server/networks/resources/types/resource.go b/management/server/networks/resources/types/resource.go index a1413dfd242..4ea14bf82e9 100644 --- a/management/server/networks/resources/types/resource.go +++ b/management/server/networks/resources/types/resource.go @@ -6,11 +6,12 @@ import ( "net/netip" "regexp" + "github.com/rs/xid" + nbDomain "github.com/netbirdio/netbird/management/domain" routerTypes "github.com/netbirdio/netbird/management/server/networks/routers/types" nbpeer "github.com/netbirdio/netbird/management/server/peer" "github.com/netbirdio/netbird/route" - "github.com/rs/xid" "github.com/netbirdio/netbird/management/server/http/api" ) @@ -34,12 +35,13 @@ type NetworkResource struct { Name string Description string Type NetworkResourceType - Address string `gorm:"-"` + Address string `gorm:"-"` + GroupIDs []string `gorm:"-"` Domain string Prefix netip.Prefix `gorm:"serializer:json"` } -func NewNetworkResource(accountID, networkID, name, description, address string) (*NetworkResource, error) { +func NewNetworkResource(accountID, networkID, name, description, address string, groupIDs []string) (*NetworkResource, error) { resourceType, domain, prefix, err := GetResourceType(address) if err != nil { return nil, fmt.Errorf("invalid address: %w", err) @@ -55,6 +57,7 @@ func NewNetworkResource(accountID, networkID, name, description, address string) Address: address, Domain: domain, Prefix: prefix, + GroupIDs: groupIDs, }, nil } @@ -81,6 +84,7 @@ func (n *NetworkResource) FromAPIRequest(req *api.NetworkResourceRequest) { n.Description = *req.Description } n.Address = req.Address + n.GroupIDs = req.Groups } func (n *NetworkResource) Copy() *NetworkResource { @@ -94,6 +98,7 @@ func (n *NetworkResource) Copy() *NetworkResource { Address: n.Address, Domain: n.Domain, Prefix: n.Prefix, + GroupIDs: n.GroupIDs, } } diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index ed4d2fb28c6..62b004f9c94 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -588,6 +588,25 @@ func (s *SqlStore) GetAccountGroups(ctx context.Context, lockStrength LockingStr return groups, nil } +func (s *SqlStore) GetResourceGroups(ctx context.Context, lockStrength LockingStrength, accountID, resourceID string) ([]*types.Group, error) { + var groups []*types.Group + + likePattern := `%"ID":"` + resourceID + `"%` + + result := s.db.Clauses(clause.Locking{Strength: string(lockStrength)}). + Where("resources LIKE ?", likePattern). + Find(&groups) + + if result.Error != nil { + if errors.Is(result.Error, gorm.ErrRecordNotFound) { + return nil, nil + } + return nil, result.Error + } + + return groups, nil +} + func (s *SqlStore) GetAllAccounts(ctx context.Context) (all []*types.Account) { var accounts []types.Account result := s.db.Find(&accounts) @@ -1019,6 +1038,7 @@ func (s *SqlStore) IncrementSetupKeyUsage(ctx context.Context, setupKeyID string return nil } +// AddPeerToAllGroup adds a peer to the 'All' group. Method always needs to run in a transaction func (s *SqlStore) AddPeerToAllGroup(ctx context.Context, accountID string, peerID string) error { var group types.Group result := s.db.Where("account_id = ? AND name = ?", accountID, "All").First(&group) @@ -1044,6 +1064,7 @@ func (s *SqlStore) AddPeerToAllGroup(ctx context.Context, accountID string, peer return nil } +// AddPeerToGroup adds a peer to a group. Method always needs to run in a transaction func (s *SqlStore) AddPeerToGroup(ctx context.Context, accountId string, peerId string, groupID string) error { var group types.Group result := s.db.Where(accountAndIDQueryCondition, accountId, groupID).First(&group) @@ -1070,6 +1091,7 @@ func (s *SqlStore) AddPeerToGroup(ctx context.Context, accountId string, peerId return nil } +// AddResourceToGroup adds a resource to a group. Method always needs to run n a transaction func (s *SqlStore) AddResourceToGroup(ctx context.Context, accountId string, groupID string, resource *types.Resource) error { var group types.Group result := s.db.Where(accountAndIDQueryCondition, accountId, groupID).First(&group) @@ -1096,6 +1118,32 @@ func (s *SqlStore) AddResourceToGroup(ctx context.Context, accountId string, gro return nil } +// RemoveResourceFromGroup removes a resource from a group. Method always needs to run in a transaction +func (s *SqlStore) RemoveResourceFromGroup(ctx context.Context, accountId string, groupID string, resourceID string) error { + var group types.Group + result := s.db.Where(accountAndIDQueryCondition, accountId, groupID).First(&group) + if result.Error != nil { + if errors.Is(result.Error, gorm.ErrRecordNotFound) { + return status.NewGroupNotFoundError(groupID) + } + + return status.Errorf(status.Internal, "issue finding group: %s", result.Error) + } + + for i, res := range group.Resources { + if res.ID == resourceID { + group.Resources = append(group.Resources[:i], group.Resources[i+1:]...) + break + } + } + + if err := s.db.Save(&group).Error; err != nil { + return status.Errorf(status.Internal, "issue updating group: %s", err) + } + + return nil +} + // GetUserPeers retrieves peers for a user. func (s *SqlStore) GetUserPeers(ctx context.Context, lockStrength LockingStrength, accountID, userID string) ([]*nbpeer.Peer, error) { return getRecords[*nbpeer.Peer](s.db.Where("user_id = ?", userID), lockStrength, accountID) diff --git a/management/server/store/sql_store_test.go b/management/server/store/sql_store_test.go index ec4b49534bd..845bc8fd474 100644 --- a/management/server/store/sql_store_test.go +++ b/management/server/store/sql_store_test.go @@ -2494,7 +2494,7 @@ func TestSqlStore_SaveNetworkResource(t *testing.T) { accountID := "bf1c8084-ba50-4ce7-9439-34653001fc3b" networkID := "ct286bi7qv930dsrrug0" - netResource, err := resourceTypes.NewNetworkResource(accountID, networkID, "resource-name", "", "example.com") + netResource, err := resourceTypes.NewNetworkResource(accountID, networkID, "resource-name", "", "example.com", []string{}) require.NoError(t, err) err = store.SaveNetworkResource(context.Background(), LockingStrengthUpdate, netResource) @@ -2529,3 +2529,35 @@ func TestSqlStore_DeleteNetworkResource(t *testing.T) { require.Equal(t, status.NotFound, sErr.Type()) require.Nil(t, netResource) } + +func TestSqlStore_AddAndRemoveResourceFromGroup(t *testing.T) { + store, cleanup, err := NewTestStoreFromSQL(context.Background(), "../testdata/store.sql", t.TempDir()) + require.NoError(t, err) + t.Cleanup(cleanup) + + accountID := "bf1c8084-ba50-4ce7-9439-34653001fc3b" + resourceId := "ctc4nci7qv9061u6ilfg" + groupID := "cs1tnh0hhcjnqoiuebeg" + + res := &types.Resource{ + ID: resourceId, + Type: "host", + } + err = store.AddResourceToGroup(context.Background(), accountID, groupID, res) + require.NoError(t, err) + + group, err := store.GetGroupByID(context.Background(), LockingStrengthShare, accountID, groupID) + require.NoError(t, err) + require.Contains(t, group.Resources, *res) + + groups, err := store.GetResourceGroups(context.Background(), LockingStrengthShare, accountID, resourceId) + require.NoError(t, err) + require.Len(t, groups, 1) + + err = store.RemoveResourceFromGroup(context.Background(), accountID, groupID, res.ID) + require.NoError(t, err) + + group, err = store.GetGroupByID(context.Background(), LockingStrengthShare, accountID, groupID) + require.NoError(t, err) + require.NotContains(t, group.Resources, *res) +} diff --git a/management/server/store/store.go b/management/server/store/store.go index 6b0e862c413..d9dc6b8f7b8 100644 --- a/management/server/store/store.go +++ b/management/server/store/store.go @@ -74,7 +74,8 @@ type Store interface { DeleteTokenID2UserIDIndex(tokenID string) error GetAccountGroups(ctx context.Context, lockStrength LockingStrength, accountID string) ([]*types.Group, error) - GetGroupByID(ctx context.Context, lockStrength LockingStrength, groupID, accountID string) (*types.Group, error) + GetResourceGroups(ctx context.Context, lockStrength LockingStrength, accountID, resourceID string) ([]*types.Group, error) + GetGroupByID(ctx context.Context, lockStrength LockingStrength, accountID, groupID string) (*types.Group, error) GetGroupByName(ctx context.Context, lockStrength LockingStrength, groupName, accountID string) (*types.Group, error) GetGroupsByIDs(ctx context.Context, lockStrength LockingStrength, accountID string, groupIDs []string) (map[string]*types.Group, error) SaveGroups(ctx context.Context, lockStrength LockingStrength, groups []*types.Group) error @@ -99,6 +100,7 @@ type Store interface { AddPeerToAllGroup(ctx context.Context, accountID string, peerID string) error AddPeerToGroup(ctx context.Context, accountId string, peerId string, groupID string) error AddResourceToGroup(ctx context.Context, accountId string, groupID string, resource *types.Resource) error + RemoveResourceFromGroup(ctx context.Context, accountId string, groupID string, resourceID string) error AddPeerToAccount(ctx context.Context, peer *nbpeer.Peer) error GetPeerByPeerPubKey(ctx context.Context, lockStrength LockingStrength, peerKey string) (*nbpeer.Peer, error) GetUserPeers(ctx context.Context, lockStrength LockingStrength, accountID, userID string) ([]*nbpeer.Peer, error) From 22cbbf889a84bec8f71fc612865f7e17f4322dda Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Tue, 17 Dec 2024 17:55:05 +0100 Subject: [PATCH 02/11] extract groups update into separate method --- .../server/networks/resources/manager.go | 68 ++++++++++--------- 1 file changed, 36 insertions(+), 32 deletions(-) diff --git a/management/server/networks/resources/manager.go b/management/server/networks/resources/manager.go index 7d415d6a5c1..3ec88663c6e 100644 --- a/management/server/networks/resources/manager.go +++ b/management/server/networks/resources/manager.go @@ -190,38 +190,7 @@ func (m *managerImpl) UpdateResource(ctx context.Context, userID string, resourc return fmt.Errorf("failed to save network resource: %w", err) } - res := nbtypes.Resource{ - ID: resource.ID, - Type: resource.Type.String(), - } - - oldResourceGroups, err := m.groupsManager.GetResourceGroupsInTransaction(ctx, transaction, store.LockingStrengthUpdate, oldResource.AccountID, oldResource.ID) - if err != nil { - return fmt.Errorf("failed to get resource groups: %w", err) - } - - oldGroupsIds := make([]string, 0) - for _, group := range oldResourceGroups { - oldGroupsIds = append(oldGroupsIds, group.ID) - } - - groupsToAdd := util.Difference(resource.GroupIDs, oldGroupsIds) - for _, groupID := range groupsToAdd { - err = m.groupsManager.AddResourceToGroupInTransaction(ctx, transaction, resource.AccountID, groupID, &res) - if err != nil { - return fmt.Errorf("failed to add resource to group: %w", err) - } - } - - groupsToRemove := util.Difference(oldGroupsIds, resource.GroupIDs) - for _, groupID := range groupsToRemove { - err = m.groupsManager.RemoveResourceFromGroupInTransaction(ctx, transaction, resource.AccountID, groupID, res.ID) - if err != nil { - return fmt.Errorf("failed to add resource to group: %w", err) - } - } - - return nil + return m.updateResourceGroups(ctx, transaction, resource, oldResource) }) if err != nil { return nil, fmt.Errorf("failed to update network resource: %w", err) @@ -232,6 +201,41 @@ func (m *managerImpl) UpdateResource(ctx context.Context, userID string, resourc return resource, nil } +func (m *managerImpl) updateResourceGroups(ctx context.Context, transaction store.Store, newResource, oldResource *types.NetworkResource) error { + res := nbtypes.Resource{ + ID: newResource.ID, + Type: newResource.Type.String(), + } + + oldResourceGroups, err := m.groupsManager.GetResourceGroupsInTransaction(ctx, transaction, store.LockingStrengthUpdate, oldResource.AccountID, oldResource.ID) + if err != nil { + return fmt.Errorf("failed to get resource groups: %w", err) + } + + oldGroupsIds := make([]string, 0) + for _, group := range oldResourceGroups { + oldGroupsIds = append(oldGroupsIds, group.ID) + } + + groupsToAdd := util.Difference(newResource.GroupIDs, oldGroupsIds) + for _, groupID := range groupsToAdd { + err = m.groupsManager.AddResourceToGroupInTransaction(ctx, transaction, newResource.AccountID, groupID, &res) + if err != nil { + return fmt.Errorf("failed to add resource to group: %w", err) + } + } + + groupsToRemove := util.Difference(oldGroupsIds, newResource.GroupIDs) + for _, groupID := range groupsToRemove { + err = m.groupsManager.RemoveResourceFromGroupInTransaction(ctx, transaction, newResource.AccountID, groupID, res.ID) + if err != nil { + return fmt.Errorf("failed to add resource to group: %w", err) + } + } + + return nil +} + func (m *managerImpl) DeleteResource(ctx context.Context, accountID, userID, networkID, resourceID string) error { ok, err := m.permissionsManager.ValidateUserPermissions(ctx, accountID, userID, permissions.Networks, permissions.Write) if err != nil { From 40ab0411984eab00c92b52bcd1f064339e5a80f5 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Tue, 17 Dec 2024 18:04:24 +0100 Subject: [PATCH 03/11] fix tests --- management/server/groups/manager.go | 27 +++++++ management/server/networks/manager_test.go | 31 +++++--- .../server/networks/resources/manager_test.go | 71 ++++++++++++------- 3 files changed, 92 insertions(+), 37 deletions(-) diff --git a/management/server/groups/manager.go b/management/server/groups/manager.go index 8da4439529b..1ec36a2f243 100644 --- a/management/server/groups/manager.go +++ b/management/server/groups/manager.go @@ -23,6 +23,9 @@ type managerImpl struct { permissionsManager permissions.Manager } +type mockManager struct { +} + func NewManager(store store.Store, permissionsManager permissions.Manager) Manager { return &managerImpl{ store: store, @@ -112,3 +115,27 @@ func ToGroupsInfo(groups map[string]*types.Group, id string) []api.GroupMinimum } return groupsInfo } + +func (m *mockManager) GetAllGroups(ctx context.Context, accountID, userID string) (map[string]*types.Group, error) { + return nil, nil +} + +func (m *mockManager) GetResourceGroupsInTransaction(ctx context.Context, transaction store.Store, lockingStrength store.LockingStrength, accountID, resourceID string) ([]*types.Group, error) { + return nil, nil +} + +func (m *mockManager) AddResourceToGroup(ctx context.Context, accountID, userID, groupID string, resourceID *types.Resource) error { + return nil +} + +func (m *mockManager) AddResourceToGroupInTransaction(ctx context.Context, transaction store.Store, accountID, groupID string, resourceID *types.Resource) error { + return nil +} + +func (m *mockManager) RemoveResourceFromGroupInTransaction(ctx context.Context, transaction store.Store, accountID, groupID, resourceID string) error { + return nil +} + +func NewManagerMock() Manager { + return &mockManager{} +} diff --git a/management/server/networks/manager_test.go b/management/server/networks/manager_test.go index af1ce1caeea..0c0c5cad960 100644 --- a/management/server/networks/manager_test.go +++ b/management/server/networks/manager_test.go @@ -6,6 +6,7 @@ import ( "github.com/stretchr/testify/require" + "github.com/netbirdio/netbird/management/server/groups" "github.com/netbirdio/netbird/management/server/mock_server" "github.com/netbirdio/netbird/management/server/networks/resources" "github.com/netbirdio/netbird/management/server/networks/types" @@ -25,7 +26,8 @@ func Test_GetAllNetworksReturnsNetworks(t *testing.T) { t.Cleanup(cleanUp) am := mock_server.MockAccountManager{} permissionsManager := permissions.NewManagerMock() - resourcesManager := resources.NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + resourcesManager := resources.NewManager(s, permissionsManager, groupsManager, &am) manager := NewManager(s, permissionsManager, resourcesManager) networks, err := manager.GetAllNetworks(ctx, accountID, userID) @@ -46,7 +48,8 @@ func Test_GetAllNetworksReturnsPermissionDenied(t *testing.T) { t.Cleanup(cleanUp) am := mock_server.MockAccountManager{} permissionsManager := permissions.NewManagerMock() - resourcesManager := resources.NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + resourcesManager := resources.NewManager(s, permissionsManager, groupsManager, &am) manager := NewManager(s, permissionsManager, resourcesManager) networks, err := manager.GetAllNetworks(ctx, accountID, userID) @@ -67,7 +70,8 @@ func Test_GetNetworkReturnsNetwork(t *testing.T) { t.Cleanup(cleanUp) am := mock_server.MockAccountManager{} permissionsManager := permissions.NewManagerMock() - resourcesManager := resources.NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + resourcesManager := resources.NewManager(s, permissionsManager, groupsManager, &am) manager := NewManager(s, permissionsManager, resourcesManager) networks, err := manager.GetNetwork(ctx, accountID, userID, networkID) @@ -88,7 +92,8 @@ func Test_GetNetworkReturnsPermissionDenied(t *testing.T) { t.Cleanup(cleanUp) am := mock_server.MockAccountManager{} permissionsManager := permissions.NewManagerMock() - resourcesManager := resources.NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + resourcesManager := resources.NewManager(s, permissionsManager, groupsManager, &am) manager := NewManager(s, permissionsManager, resourcesManager) network, err := manager.GetNetwork(ctx, accountID, userID, networkID) @@ -111,7 +116,8 @@ func Test_CreateNetworkSuccessfully(t *testing.T) { t.Cleanup(cleanUp) am := mock_server.MockAccountManager{} permissionsManager := permissions.NewManagerMock() - resourcesManager := resources.NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + resourcesManager := resources.NewManager(s, permissionsManager, groupsManager, &am) manager := NewManager(s, permissionsManager, resourcesManager) createdNetwork, err := manager.CreateNetwork(ctx, userID, network) @@ -134,7 +140,8 @@ func Test_CreateNetworkFailsWithPermissionDenied(t *testing.T) { t.Cleanup(cleanUp) am := mock_server.MockAccountManager{} permissionsManager := permissions.NewManagerMock() - resourcesManager := resources.NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + resourcesManager := resources.NewManager(s, permissionsManager, groupsManager, &am) manager := NewManager(s, permissionsManager, resourcesManager) createdNetwork, err := manager.CreateNetwork(ctx, userID, network) @@ -155,7 +162,8 @@ func Test_DeleteNetworkSuccessfully(t *testing.T) { t.Cleanup(cleanUp) am := mock_server.MockAccountManager{} permissionsManager := permissions.NewManagerMock() - resourcesManager := resources.NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + resourcesManager := resources.NewManager(s, permissionsManager, groupsManager, &am) manager := NewManager(s, permissionsManager, resourcesManager) err = manager.DeleteNetwork(ctx, accountID, userID, networkID) @@ -175,7 +183,8 @@ func Test_DeleteNetworkFailsWithPermissionDenied(t *testing.T) { t.Cleanup(cleanUp) am := mock_server.MockAccountManager{} permissionsManager := permissions.NewManagerMock() - resourcesManager := resources.NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + resourcesManager := resources.NewManager(s, permissionsManager, groupsManager, &am) manager := NewManager(s, permissionsManager, resourcesManager) err = manager.DeleteNetwork(ctx, accountID, userID, networkID) @@ -198,7 +207,8 @@ func Test_UpdateNetworkSuccessfully(t *testing.T) { t.Cleanup(cleanUp) am := mock_server.MockAccountManager{} permissionsManager := permissions.NewManagerMock() - resourcesManager := resources.NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + resourcesManager := resources.NewManager(s, permissionsManager, groupsManager, &am) manager := NewManager(s, permissionsManager, resourcesManager) updatedNetwork, err := manager.UpdateNetwork(ctx, userID, network) @@ -223,7 +233,8 @@ func Test_UpdateNetworkFailsWithPermissionDenied(t *testing.T) { am := mock_server.MockAccountManager{} permissionsManager := permissions.NewManagerMock() - resourcesManager := resources.NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + resourcesManager := resources.NewManager(s, permissionsManager, groupsManager, &am) manager := NewManager(s, permissionsManager, resourcesManager) updatedNetwork, err := manager.UpdateNetwork(ctx, userID, network) diff --git a/management/server/networks/resources/manager_test.go b/management/server/networks/resources/manager_test.go index e9ce8d28019..a921506365c 100644 --- a/management/server/networks/resources/manager_test.go +++ b/management/server/networks/resources/manager_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/require" + "github.com/netbirdio/netbird/management/server/groups" "github.com/netbirdio/netbird/management/server/mock_server" "github.com/netbirdio/netbird/management/server/networks/resources/types" "github.com/netbirdio/netbird/management/server/permissions" @@ -20,14 +21,15 @@ func Test_GetAllResourcesInNetworkReturnsResources(t *testing.T) { userID := "allowedUser" networkID := "testNetworkId" - s, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) + store, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) if err != nil { t.Fatal(err) } t.Cleanup(cleanUp) permissionsManager := permissions.NewManagerMock() am := mock_server.MockAccountManager{} - manager := NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + manager := NewManager(store, permissionsManager, groupsManager, &am) resources, err := manager.GetAllResourcesInNetwork(ctx, accountID, userID, networkID) require.NoError(t, err) @@ -40,14 +42,15 @@ func Test_GetAllResourcesInNetworkReturnsPermissionDenied(t *testing.T) { userID := "invalidUser" networkID := "testNetworkId" - s, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) + store, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) if err != nil { t.Fatal(err) } t.Cleanup(cleanUp) permissionsManager := permissions.NewManagerMock() am := mock_server.MockAccountManager{} - manager := NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + manager := NewManager(store, permissionsManager, groupsManager, &am) resources, err := manager.GetAllResourcesInNetwork(ctx, accountID, userID, networkID) require.Error(t, err) @@ -59,14 +62,15 @@ func Test_GetAllResourcesInAccountReturnsResources(t *testing.T) { accountID := "testAccountId" userID := "allowedUser" - s, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) + store, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) if err != nil { t.Fatal(err) } t.Cleanup(cleanUp) permissionsManager := permissions.NewManagerMock() am := mock_server.MockAccountManager{} - manager := NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + manager := NewManager(store, permissionsManager, groupsManager, &am) resources, err := manager.GetAllResourcesInAccount(ctx, accountID, userID) require.NoError(t, err) @@ -78,14 +82,15 @@ func Test_GetAllResourcesInAccountReturnsPermissionDenied(t *testing.T) { accountID := "testAccountId" userID := "invalidUser" - s, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) + store, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) if err != nil { t.Fatal(err) } t.Cleanup(cleanUp) permissionsManager := permissions.NewManagerMock() am := mock_server.MockAccountManager{} - manager := NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + manager := NewManager(store, permissionsManager, groupsManager, &am) resources, err := manager.GetAllResourcesInAccount(ctx, accountID, userID) require.Error(t, err) @@ -100,14 +105,15 @@ func Test_GetResourceInNetworkReturnsResources(t *testing.T) { networkID := "testNetworkId" resourceID := "testResourceId" - s, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) + store, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) if err != nil { t.Fatal(err) } t.Cleanup(cleanUp) permissionsManager := permissions.NewManagerMock() am := mock_server.MockAccountManager{} - manager := NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + manager := NewManager(store, permissionsManager, groupsManager, &am) resource, err := manager.GetResource(ctx, accountID, userID, networkID, resourceID) require.NoError(t, err) @@ -121,14 +127,15 @@ func Test_GetResourceInNetworkReturnsPermissionDenied(t *testing.T) { networkID := "testNetworkId" resourceID := "testResourceId" - s, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) + store, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) if err != nil { t.Fatal(err) } t.Cleanup(cleanUp) permissionsManager := permissions.NewManagerMock() am := mock_server.MockAccountManager{} - manager := NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + manager := NewManager(store, permissionsManager, groupsManager, &am) resources, err := manager.GetResource(ctx, accountID, userID, networkID, resourceID) require.Error(t, err) @@ -154,7 +161,8 @@ func Test_CreateResourceSuccessfully(t *testing.T) { t.Cleanup(cleanUp) permissionsManager := permissions.NewManagerMock() am := mock_server.MockAccountManager{} - manager := NewManager(store, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + manager := NewManager(store, permissionsManager, groupsManager, &am) createdResource, err := manager.CreateResource(ctx, userID, resource) require.NoError(t, err) @@ -179,7 +187,8 @@ func Test_CreateResourceFailsWithPermissionDenied(t *testing.T) { t.Cleanup(cleanUp) permissionsManager := permissions.NewManagerMock() am := mock_server.MockAccountManager{} - manager := NewManager(store, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + manager := NewManager(store, permissionsManager, groupsManager, &am) createdResource, err := manager.CreateResource(ctx, userID, resource) require.Error(t, err) @@ -205,7 +214,8 @@ func Test_CreateResourceFailsWithInvalidAddress(t *testing.T) { t.Cleanup(cleanUp) permissionsManager := permissions.NewManagerMock() am := mock_server.MockAccountManager{} - manager := NewManager(store, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + manager := NewManager(store, permissionsManager, groupsManager, &am) createdResource, err := manager.CreateResource(ctx, userID, resource) require.Error(t, err) @@ -230,7 +240,8 @@ func Test_CreateResourceFailsWithUsedName(t *testing.T) { t.Cleanup(cleanUp) permissionsManager := permissions.NewManagerMock() am := mock_server.MockAccountManager{} - manager := NewManager(store, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + manager := NewManager(store, permissionsManager, groupsManager, &am) createdResource, err := manager.CreateResource(ctx, userID, resource) require.Error(t, err) @@ -252,14 +263,15 @@ func Test_UpdateResourceSuccessfully(t *testing.T) { Address: "1.2.3.0/24", } - s, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) + store, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) if err != nil { t.Fatal(err) } t.Cleanup(cleanUp) permissionsManager := permissions.NewManagerMock() am := mock_server.MockAccountManager{} - manager := NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + manager := NewManager(store, permissionsManager, groupsManager, &am) updatedResource, err := manager.UpdateResource(ctx, userID, resource) require.NoError(t, err) @@ -283,14 +295,15 @@ func Test_UpdateResourceFailsWithResourceNotFound(t *testing.T) { Address: "1.2.3.0/24", } - s, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) + store, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) if err != nil { t.Fatal(err) } t.Cleanup(cleanUp) permissionsManager := permissions.NewManagerMock() am := mock_server.MockAccountManager{} - manager := NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + manager := NewManager(store, permissionsManager, groupsManager, &am) updatedResource, err := manager.UpdateResource(ctx, userID, resource) require.Error(t, err) @@ -312,14 +325,15 @@ func Test_UpdateResourceFailsWithNameInUse(t *testing.T) { Address: "1.2.3.0/24", } - s, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) + store, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) if err != nil { t.Fatal(err) } t.Cleanup(cleanUp) permissionsManager := permissions.NewManagerMock() am := mock_server.MockAccountManager{} - manager := NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + manager := NewManager(store, permissionsManager, groupsManager, &am) updatedResource, err := manager.UpdateResource(ctx, userID, resource) require.Error(t, err) @@ -341,14 +355,15 @@ func Test_UpdateResourceFailsWithPermissionDenied(t *testing.T) { Address: "1.2.3.0/24", } - s, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) + store, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) if err != nil { t.Fatal(err) } t.Cleanup(cleanUp) permissionsManager := permissions.NewManagerMock() am := mock_server.MockAccountManager{} - manager := NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + manager := NewManager(store, permissionsManager, groupsManager, &am) updatedResource, err := manager.UpdateResource(ctx, userID, resource) require.Error(t, err) @@ -362,14 +377,15 @@ func Test_DeleteResourceSuccessfully(t *testing.T) { networkID := "testNetworkId" resourceID := "testResourceId" - s, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) + store, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "../../testdata/networks.sql", t.TempDir()) if err != nil { t.Fatal(err) } t.Cleanup(cleanUp) permissionsManager := permissions.NewManagerMock() am := mock_server.MockAccountManager{} - manager := NewManager(s, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + manager := NewManager(store, permissionsManager, groupsManager, &am) err = manager.DeleteResource(ctx, accountID, userID, networkID, resourceID) require.NoError(t, err) @@ -389,7 +405,8 @@ func Test_DeleteResourceFailsWithPermissionDenied(t *testing.T) { t.Cleanup(cleanUp) permissionsManager := permissions.NewManagerMock() am := mock_server.MockAccountManager{} - manager := NewManager(store, permissionsManager, &am) + groupsManager := groups.NewManagerMock() + manager := NewManager(store, permissionsManager, groupsManager, &am) err = manager.DeleteResource(ctx, accountID, userID, networkID, resourceID) require.Error(t, err) From cf739116cb853ff08488965a6a7c3a34583c0aca Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Tue, 17 Dec 2024 18:13:48 +0100 Subject: [PATCH 04/11] fix mock and error handling --- management/server/groups/manager.go | 4 ++-- management/server/networks/resources/manager.go | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/management/server/groups/manager.go b/management/server/groups/manager.go index 1ec36a2f243..b72faf4a260 100644 --- a/management/server/groups/manager.go +++ b/management/server/groups/manager.go @@ -117,11 +117,11 @@ func ToGroupsInfo(groups map[string]*types.Group, id string) []api.GroupMinimum } func (m *mockManager) GetAllGroups(ctx context.Context, accountID, userID string) (map[string]*types.Group, error) { - return nil, nil + return map[string]*types.Group{}, nil } func (m *mockManager) GetResourceGroupsInTransaction(ctx context.Context, transaction store.Store, lockingStrength store.LockingStrength, accountID, resourceID string) ([]*types.Group, error) { - return nil, nil + return []*types.Group{}, nil } func (m *mockManager) AddResourceToGroup(ctx context.Context, accountID, userID, groupID string, resourceID *types.Resource) error { diff --git a/management/server/networks/resources/manager.go b/management/server/networks/resources/manager.go index 3ec88663c6e..b50e4074544 100644 --- a/management/server/networks/resources/manager.go +++ b/management/server/networks/resources/manager.go @@ -271,6 +271,10 @@ func (m *managerImpl) DeleteResourceInTransaction(ctx context.Context, transacti } groups, err := m.groupsManager.GetResourceGroupsInTransaction(ctx, transaction, store.LockingStrengthUpdate, accountID, resourceID) + if err != nil { + return fmt.Errorf("failed to get resource groups: %w", err) + } + for _, group := range groups { err = m.groupsManager.RemoveResourceFromGroupInTransaction(ctx, transaction, accountID, group.ID, resourceID) if err != nil { From 7bbdc3c5ac968a450509a348d0be9ef30419d3c9 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Tue, 17 Dec 2024 18:34:26 +0100 Subject: [PATCH 05/11] fix nullpointer on group change while name change --- management/server/networks/resources/manager.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/management/server/networks/resources/manager.go b/management/server/networks/resources/manager.go index b50e4074544..30222ae75f8 100644 --- a/management/server/networks/resources/manager.go +++ b/management/server/networks/resources/manager.go @@ -185,6 +185,11 @@ func (m *managerImpl) UpdateResource(ctx context.Context, userID string, resourc return errors.New("new resource name already exists") } + oldResource, err = transaction.GetNetworkResourceByID(ctx, store.LockingStrengthShare, resource.AccountID, resource.ID) + if err != nil { + return fmt.Errorf("failed to get network resource: %w", err) + } + err = transaction.SaveNetworkResource(ctx, store.LockingStrengthUpdate, resource) if err != nil { return fmt.Errorf("failed to save network resource: %w", err) From 44fba5df2e6ac48221fc7dbfc3d8099eb7f6215a Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Tue, 17 Dec 2024 18:41:49 +0100 Subject: [PATCH 06/11] fix manager test --- management/server/networks/resources/manager_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/management/server/networks/resources/manager_test.go b/management/server/networks/resources/manager_test.go index a921506365c..993cd65df87 100644 --- a/management/server/networks/resources/manager_test.go +++ b/management/server/networks/resources/manager_test.go @@ -2,7 +2,6 @@ package resources import ( "context" - "errors" "testing" "github.com/stretchr/testify/require" @@ -337,7 +336,6 @@ func Test_UpdateResourceFailsWithNameInUse(t *testing.T) { updatedResource, err := manager.UpdateResource(ctx, userID, resource) require.Error(t, err) - require.Equal(t, errors.New("new resource name already exists"), err) require.Nil(t, updatedResource) } From fc1b4d7b7b17100a30421c9ff8f4e73eb5970588 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Tue, 17 Dec 2024 22:35:15 +0100 Subject: [PATCH 07/11] add activity events --- management/cmd/management.go | 4 +- management/server/activity/codes.go | 30 +++++ management/server/groups/manager.go | 57 ++++++++-- management/server/networks/manager.go | 65 +++++++++-- .../server/networks/resources/manager.go | 104 ++++++++++++++---- .../networks/resources/types/resource.go | 4 + management/server/networks/routers/manager.go | 88 +++++++++++++-- .../server/networks/routers/types/router.go | 4 + management/server/networks/types/network.go | 4 + management/server/status/error.go | 8 ++ 10 files changed, 318 insertions(+), 50 deletions(-) diff --git a/management/cmd/management.go b/management/cmd/management.go index 5301485cb0b..4f34009b7e1 100644 --- a/management/cmd/management.go +++ b/management/cmd/management.go @@ -276,10 +276,10 @@ var ( userManager := users.NewManager(store) settingsManager := settings.NewManager(store) permissionsManager := permissions.NewManager(userManager, settingsManager) - groupsManager := groups.NewManager(store, permissionsManager) + groupsManager := groups.NewManager(store, permissionsManager, accountManager) resourcesManager := resources.NewManager(store, permissionsManager, groupsManager, accountManager) routersManager := routers.NewManager(store, permissionsManager, accountManager) - networksManager := networks.NewManager(store, permissionsManager, resourcesManager) + networksManager := networks.NewManager(store, permissionsManager, resourcesManager, routersManager, accountManager) httpAPIHandler, err := httpapi.APIHandler(ctx, accountManager, networksManager, resourcesManager, routersManager, groupsManager, geo, *jwtValidator, appMetrics, httpAPIAuthCfg, integratedPeerValidator) if err != nil { diff --git a/management/server/activity/codes.go b/management/server/activity/codes.go index 2165eba9c44..5379a8dd81b 100644 --- a/management/server/activity/codes.go +++ b/management/server/activity/codes.go @@ -154,6 +154,21 @@ const ( AccountRoutingPeerDNSResolutionEnabled Activity = 71 AccountRoutingPeerDNSResolutionDisabled Activity = 72 + + NetworkCreated Activity = 73 + NetworkUpdated Activity = 74 + NetworkDeleted Activity = 75 + + NetworkResourceCreated Activity = 76 + NetworkResourceUpdated Activity = 77 + NetworkResourceDeleted Activity = 78 + + NetworkRouterCreated Activity = 79 + NetworkRouterUpdated Activity = 80 + NetworkRouterDeleted Activity = 81 + + ResourceAddedToGroup Activity = 82 + ResourceRemovedFromGroup Activity = 83 ) var activityMap = map[Activity]Code{ @@ -234,6 +249,21 @@ var activityMap = map[Activity]Code{ AccountRoutingPeerDNSResolutionEnabled: {"Account routing peer DNS resolution enabled", "account.setting.routing.peer.dns.resolution.enable"}, AccountRoutingPeerDNSResolutionDisabled: {"Account routing peer DNS resolution disabled", "account.setting.routing.peer.dns.resolution.disable"}, + + NetworkCreated: {"Network created", "network.create"}, + NetworkUpdated: {"Network updated", "network.update"}, + NetworkDeleted: {"Network deleted", "network.delete"}, + + NetworkResourceCreated: {"Network resource created", "network.resource.create"}, + NetworkResourceUpdated: {"Network resource updated", "network.resource.update"}, + NetworkResourceDeleted: {"Network resource deleted", "network.resource.delete"}, + + NetworkRouterCreated: {"Network router created", "network.router.create"}, + NetworkRouterUpdated: {"Network router updated", "network.router.update"}, + NetworkRouterDeleted: {"Network router deleted", "network.router.delete"}, + + ResourceAddedToGroup: {"Resource added to group", "resource.group.add"}, + ResourceRemovedFromGroup: {"Resource removed from group", "resource.group.delete"}, } // StringCode returns a string code of the activity diff --git a/management/server/groups/manager.go b/management/server/groups/manager.go index b72faf4a260..1162348bd07 100644 --- a/management/server/groups/manager.go +++ b/management/server/groups/manager.go @@ -4,6 +4,8 @@ import ( "context" "fmt" + s "github.com/netbirdio/netbird/management/server" + "github.com/netbirdio/netbird/management/server/activity" "github.com/netbirdio/netbird/management/server/http/api" "github.com/netbirdio/netbird/management/server/permissions" "github.com/netbirdio/netbird/management/server/store" @@ -14,22 +16,24 @@ type Manager interface { GetAllGroups(ctx context.Context, accountID, userID string) (map[string]*types.Group, error) GetResourceGroupsInTransaction(ctx context.Context, transaction store.Store, lockingStrength store.LockingStrength, accountID, resourceID string) ([]*types.Group, error) AddResourceToGroup(ctx context.Context, accountID, userID, groupID string, resourceID *types.Resource) error - AddResourceToGroupInTransaction(ctx context.Context, transaction store.Store, accountID, groupID string, resourceID *types.Resource) error - RemoveResourceFromGroupInTransaction(ctx context.Context, transaction store.Store, accountID, groupID, resourceID string) error + AddResourceToGroupInTransaction(ctx context.Context, transaction store.Store, accountID, groupID string, resourceID *types.Resource) (func(), error) + RemoveResourceFromGroupInTransaction(ctx context.Context, transaction store.Store, accountID, groupID, resourceID string) (func(), error) } type managerImpl struct { store store.Store permissionsManager permissions.Manager + accountManager s.AccountManager } type mockManager struct { } -func NewManager(store store.Store, permissionsManager permissions.Manager) Manager { +func NewManager(store store.Store, permissionsManager permissions.Manager, accountManager s.AccountManager) Manager { return &managerImpl{ store: store, permissionsManager: permissionsManager, + accountManager: accountManager, } } @@ -64,15 +68,40 @@ func (m *managerImpl) AddResourceToGroup(ctx context.Context, accountID, userID, return err } - return m.AddResourceToGroupInTransaction(ctx, m.store, accountID, groupID, resource) + event, err := m.AddResourceToGroupInTransaction(ctx, m.store, accountID, groupID, resource) + if err != nil { + return fmt.Errorf("error adding resource to group: %w", err) + } + + event() + + return nil } -func (m *managerImpl) AddResourceToGroupInTransaction(ctx context.Context, transaction store.Store, accountID, groupID string, resource *types.Resource) error { - return transaction.AddResourceToGroup(ctx, accountID, groupID, resource) +func (m *managerImpl) AddResourceToGroupInTransaction(ctx context.Context, transaction store.Store, accountID, groupID string, resource *types.Resource) (func(), error) { + err := transaction.AddResourceToGroup(ctx, accountID, groupID, resource) + if err != nil { + return nil, fmt.Errorf("error adding resource to group: %w", err) + } + + event := func() { + m.accountManager.StoreEvent(ctx, accountID, groupID, accountID, activity.ResourceAddedToGroup, nil) + } + + return event, nil } -func (m *managerImpl) RemoveResourceFromGroupInTransaction(ctx context.Context, transaction store.Store, accountID, groupID, resourceID string) error { - return transaction.RemoveResourceFromGroup(ctx, accountID, groupID, resourceID) +func (m *managerImpl) RemoveResourceFromGroupInTransaction(ctx context.Context, transaction store.Store, accountID, groupID, resourceID string) (func(), error) { + err := transaction.RemoveResourceFromGroup(ctx, accountID, groupID, resourceID) + if err != nil { + return nil, fmt.Errorf("error removing resource from group: %w", err) + } + + event := func() { + m.accountManager.StoreEvent(ctx, accountID, groupID, accountID, activity.ResourceRemovedFromGroup, nil) + } + + return event, nil } func (m *managerImpl) GetResourceGroupsInTransaction(ctx context.Context, transaction store.Store, lockingStrength store.LockingStrength, accountID, resourceID string) ([]*types.Group, error) { @@ -128,12 +157,16 @@ func (m *mockManager) AddResourceToGroup(ctx context.Context, accountID, userID, return nil } -func (m *mockManager) AddResourceToGroupInTransaction(ctx context.Context, transaction store.Store, accountID, groupID string, resourceID *types.Resource) error { - return nil +func (m *mockManager) AddResourceToGroupInTransaction(ctx context.Context, transaction store.Store, accountID, groupID string, resourceID *types.Resource) (func(), error) { + return func() { + // noop + }, nil } -func (m *mockManager) RemoveResourceFromGroupInTransaction(ctx context.Context, transaction store.Store, accountID, groupID, resourceID string) error { - return nil +func (m *mockManager) RemoveResourceFromGroupInTransaction(ctx context.Context, transaction store.Store, accountID, groupID, resourceID string) (func(), error) { + return func() { + // noop + }, nil } func NewManagerMock() Manager { diff --git a/management/server/networks/manager.go b/management/server/networks/manager.go index d5291d9dafb..da37ea5bb21 100644 --- a/management/server/networks/manager.go +++ b/management/server/networks/manager.go @@ -6,7 +6,10 @@ import ( "github.com/rs/xid" + s "github.com/netbirdio/netbird/management/server" + "github.com/netbirdio/netbird/management/server/activity" "github.com/netbirdio/netbird/management/server/networks/resources" + "github.com/netbirdio/netbird/management/server/networks/routers" "github.com/netbirdio/netbird/management/server/networks/types" "github.com/netbirdio/netbird/management/server/permissions" "github.com/netbirdio/netbird/management/server/status" @@ -23,15 +26,20 @@ type Manager interface { type managerImpl struct { store store.Store + eventStore activity.Store + accountManager s.AccountManager permissionsManager permissions.Manager resourcesManager resources.Manager + routersManager routers.Manager } -func NewManager(store store.Store, permissionsManager permissions.Manager, manager resources.Manager) Manager { +func NewManager(store store.Store, permissionsManager permissions.Manager, resourceManager resources.Manager, routersManager routers.Manager, accountManager s.AccountManager) Manager { return &managerImpl{ store: store, permissionsManager: permissionsManager, - resourcesManager: manager, + resourcesManager: resourceManager, + routersManager: routersManager, + accountManager: accountManager, } } @@ -58,7 +66,14 @@ func (m *managerImpl) CreateNetwork(ctx context.Context, userID string, network network.ID = xid.New().String() - return network, m.store.SaveNetwork(ctx, store.LockingStrengthUpdate, network) + err = m.store.SaveNetwork(ctx, store.LockingStrengthUpdate, network) + if err != nil { + return nil, fmt.Errorf("failed to save network: %w", err) + } + + m.accountManager.StoreEvent(ctx, userID, network.ID, network.AccountID, activity.NetworkCreated, network.EventMeta()) + + return network, nil } func (m *managerImpl) GetNetwork(ctx context.Context, accountID, userID, networkID string) (*types.Network, error) { @@ -82,6 +97,13 @@ func (m *managerImpl) UpdateNetwork(ctx context.Context, userID string, network return nil, status.NewPermissionDeniedError() } + _, err = m.store.GetNetworkByID(ctx, store.LockingStrengthUpdate, network.AccountID, network.ID) + if err != nil { + return nil, fmt.Errorf("failed to get network: %w", err) + } + + m.accountManager.StoreEvent(ctx, userID, network.ID, network.AccountID, activity.NetworkUpdated, network.EventMeta()) + return network, m.store.SaveNetwork(ctx, store.LockingStrengthUpdate, network) } @@ -94,20 +116,24 @@ func (m *managerImpl) DeleteNetwork(ctx context.Context, accountID, userID, netw return status.NewPermissionDeniedError() } - unlock := m.store.AcquireWriteLockByUID(ctx, accountID) - defer unlock() + network, err := m.store.GetNetworkByID(ctx, store.LockingStrengthUpdate, accountID, networkID) + if err != nil { + return fmt.Errorf("failed to get network: %w", err) + } - return m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error { + var eventsToStore []func() + err = m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error { resources, err := transaction.GetNetworkResourcesByNetID(ctx, store.LockingStrengthUpdate, accountID, networkID) if err != nil { return fmt.Errorf("failed to get resources in network: %w", err) } for _, resource := range resources { - err = m.resourcesManager.DeleteResourceInTransaction(ctx, transaction, accountID, networkID, resource.ID) + event, err := m.resourcesManager.DeleteResourceInTransaction(ctx, transaction, accountID, networkID, resource.ID) if err != nil { return fmt.Errorf("failed to delete resource: %w", err) } + eventsToStore = append(eventsToStore, event...) } routers, err := transaction.GetNetworkRoutersByNetID(ctx, store.LockingStrengthUpdate, accountID, networkID) @@ -116,12 +142,33 @@ func (m *managerImpl) DeleteNetwork(ctx context.Context, accountID, userID, netw } for _, router := range routers { - err = transaction.DeleteNetworkRouter(ctx, store.LockingStrengthUpdate, accountID, router.ID) + event, err := m.routersManager.DeleteRouterInTransaction(ctx, transaction, accountID, networkID, router.ID) if err != nil { return fmt.Errorf("failed to delete router: %w", err) } + eventsToStore = append(eventsToStore, event) } - return transaction.DeleteNetwork(ctx, store.LockingStrengthUpdate, accountID, networkID) + err = transaction.DeleteNetwork(ctx, store.LockingStrengthUpdate, accountID, networkID) + if err != nil { + return fmt.Errorf("failed to delete network: %w", err) + } + + eventsToStore = append(eventsToStore, func() { + m.accountManager.StoreEvent(ctx, userID, networkID, accountID, activity.NetworkDeleted, network.EventMeta()) + }) + + return nil }) + if err != nil { + return fmt.Errorf("failed to delete network: %w", err) + } + + for _, event := range eventsToStore { + event() + } + + go m.accountManager.UpdateAccountPeers(ctx, accountID) + + return nil } diff --git a/management/server/networks/resources/manager.go b/management/server/networks/resources/manager.go index 30222ae75f8..bf08f7b0ef1 100644 --- a/management/server/networks/resources/manager.go +++ b/management/server/networks/resources/manager.go @@ -6,6 +6,7 @@ import ( "fmt" s "github.com/netbirdio/netbird/management/server" + "github.com/netbirdio/netbird/management/server/activity" "github.com/netbirdio/netbird/management/server/groups" "github.com/netbirdio/netbird/management/server/networks/resources/types" "github.com/netbirdio/netbird/management/server/permissions" @@ -23,7 +24,7 @@ type Manager interface { GetResource(ctx context.Context, accountID, userID, networkID, resourceID string) (*types.NetworkResource, error) UpdateResource(ctx context.Context, userID string, resource *types.NetworkResource) (*types.NetworkResource, error) DeleteResource(ctx context.Context, accountID, userID, networkID, resourceID string) error - DeleteResourceInTransaction(ctx context.Context, transaction store.Store, accountID, networkID, resourceID string) error + DeleteResourceInTransaction(ctx context.Context, transaction store.Store, accountID, networkID, resourceID string) ([]func(), error) } type managerImpl struct { @@ -102,26 +103,39 @@ func (m *managerImpl) CreateResource(ctx context.Context, userID string, resourc return nil, fmt.Errorf("failed to create new network resource: %w", err) } + // var network *networkTypes.Network + var eventsToStore []func() err = m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error { _, err = transaction.GetNetworkResourceByName(ctx, store.LockingStrengthShare, resource.AccountID, resource.Name) if err == nil { return errors.New("resource already exists") } + network, err := transaction.GetNetworkByID(ctx, store.LockingStrengthUpdate, resource.AccountID, resource.NetworkID) + if err != nil { + return fmt.Errorf("failed to get network: %w", err) + } + err = transaction.SaveNetworkResource(ctx, store.LockingStrengthUpdate, resource) if err != nil { return fmt.Errorf("failed to save network resource: %w", err) } + event := func() { + m.accountManager.StoreEvent(ctx, userID, resource.ID, resource.AccountID, activity.NetworkResourceCreated, resource.EventMeta(network.Name)) + } + eventsToStore = append(eventsToStore, event) + res := nbtypes.Resource{ ID: resource.ID, Type: resource.Type.String(), } for _, groupID := range resource.GroupIDs { - err = m.groupsManager.AddResourceToGroupInTransaction(ctx, transaction, resource.AccountID, groupID, &res) + event, err := m.groupsManager.AddResourceToGroupInTransaction(ctx, transaction, resource.AccountID, groupID, &res) if err != nil { return fmt.Errorf("failed to add resource to group: %w", err) } + eventsToStore = append(eventsToStore, event) } return nil @@ -130,6 +144,10 @@ func (m *managerImpl) CreateResource(ctx context.Context, userID string, resourc return nil, fmt.Errorf("failed to create network resource: %w", err) } + for _, event := range eventsToStore { + event() + } + go m.accountManager.UpdateAccountPeers(ctx, resource.AccountID) return resource, nil @@ -174,8 +192,18 @@ func (m *managerImpl) UpdateResource(ctx context.Context, userID string, resourc resource.Domain = domain resource.Prefix = prefix + var eventsToStore []func() err = m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error { - _, err = transaction.GetNetworkResourceByID(ctx, store.LockingStrengthShare, resource.AccountID, resource.ID) + network, err := transaction.GetNetworkByID(ctx, store.LockingStrengthUpdate, resource.AccountID, resource.NetworkID) + if err != nil { + return fmt.Errorf("failed to get network: %w", err) + } + + if network.ID != resource.NetworkID { + return status.NewResourceNotPartOfNetworkError(resource.ID, resource.NetworkID) + } + + resource, err = transaction.GetNetworkResourceByID(ctx, store.LockingStrengthShare, resource.AccountID, resource.ID) if err != nil { return fmt.Errorf("failed to get network resource: %w", err) } @@ -195,8 +223,17 @@ func (m *managerImpl) UpdateResource(ctx context.Context, userID string, resourc return fmt.Errorf("failed to save network resource: %w", err) } - return m.updateResourceGroups(ctx, transaction, resource, oldResource) + events, err := m.updateResourceGroups(ctx, transaction, resource, oldResource) + if err != nil { + return fmt.Errorf("failed to update resource groups: %w", err) + } + eventsToStore = append(eventsToStore, events...) + eventsToStore = append(eventsToStore, func() { + m.accountManager.StoreEvent(ctx, userID, resource.ID, resource.AccountID, activity.NetworkResourceUpdated, resource.EventMeta(network.Name)) + }) + return nil }) + if err != nil { return nil, fmt.Errorf("failed to update network resource: %w", err) } @@ -206,7 +243,7 @@ func (m *managerImpl) UpdateResource(ctx context.Context, userID string, resourc return resource, nil } -func (m *managerImpl) updateResourceGroups(ctx context.Context, transaction store.Store, newResource, oldResource *types.NetworkResource) error { +func (m *managerImpl) updateResourceGroups(ctx context.Context, transaction store.Store, newResource, oldResource *types.NetworkResource) ([]func(), error) { res := nbtypes.Resource{ ID: newResource.ID, Type: newResource.Type.String(), @@ -214,7 +251,7 @@ func (m *managerImpl) updateResourceGroups(ctx context.Context, transaction stor oldResourceGroups, err := m.groupsManager.GetResourceGroupsInTransaction(ctx, transaction, store.LockingStrengthUpdate, oldResource.AccountID, oldResource.ID) if err != nil { - return fmt.Errorf("failed to get resource groups: %w", err) + return nil, fmt.Errorf("failed to get resource groups: %w", err) } oldGroupsIds := make([]string, 0) @@ -222,23 +259,26 @@ func (m *managerImpl) updateResourceGroups(ctx context.Context, transaction stor oldGroupsIds = append(oldGroupsIds, group.ID) } + var eventsToStore []func() groupsToAdd := util.Difference(newResource.GroupIDs, oldGroupsIds) for _, groupID := range groupsToAdd { - err = m.groupsManager.AddResourceToGroupInTransaction(ctx, transaction, newResource.AccountID, groupID, &res) + events, err := m.groupsManager.AddResourceToGroupInTransaction(ctx, transaction, newResource.AccountID, groupID, &res) if err != nil { - return fmt.Errorf("failed to add resource to group: %w", err) + return nil, fmt.Errorf("failed to add resource to group: %w", err) } + eventsToStore = append(eventsToStore, events) } groupsToRemove := util.Difference(oldGroupsIds, newResource.GroupIDs) for _, groupID := range groupsToRemove { - err = m.groupsManager.RemoveResourceFromGroupInTransaction(ctx, transaction, newResource.AccountID, groupID, res.ID) + events, err := m.groupsManager.RemoveResourceFromGroupInTransaction(ctx, transaction, newResource.AccountID, groupID, res.ID) if err != nil { - return fmt.Errorf("failed to add resource to group: %w", err) + return nil, fmt.Errorf("failed to add resource to group: %w", err) } + eventsToStore = append(eventsToStore, events) } - return nil + return eventsToStore, nil } func (m *managerImpl) DeleteResource(ctx context.Context, accountID, userID, networkID, resourceID string) error { @@ -253,44 +293,68 @@ func (m *managerImpl) DeleteResource(ctx context.Context, accountID, userID, net unlock := m.store.AcquireWriteLockByUID(ctx, accountID) defer unlock() + var events []func() err = m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error { - return m.DeleteResourceInTransaction(ctx, transaction, accountID, networkID, resourceID) + events, err = m.DeleteResourceInTransaction(ctx, transaction, accountID, networkID, resourceID) + return err + }) if err != nil { return fmt.Errorf("failed to delete network resource: %w", err) } + for _, event := range events { + event() + } + go m.accountManager.UpdateAccountPeers(ctx, accountID) return nil } -func (m *managerImpl) DeleteResourceInTransaction(ctx context.Context, transaction store.Store, accountID, networkID, resourceID string) error { +func (m *managerImpl) DeleteResourceInTransaction(ctx context.Context, transaction store.Store, accountID, networkID, resourceID string) ([]func(), error) { resource, err := transaction.GetNetworkResourceByID(ctx, store.LockingStrengthUpdate, accountID, resourceID) if err != nil { - return fmt.Errorf("failed to get network resource: %w", err) + return nil, fmt.Errorf("failed to get network resource: %w", err) + } + + network, err := transaction.GetNetworkByID(ctx, store.LockingStrengthUpdate, accountID, networkID) + if err != nil { + return nil, fmt.Errorf("failed to get network: %w", err) } if resource.NetworkID != networkID { - return errors.New("resource not part of network") + return nil, errors.New("resource not part of network") } groups, err := m.groupsManager.GetResourceGroupsInTransaction(ctx, transaction, store.LockingStrengthUpdate, accountID, resourceID) if err != nil { - return fmt.Errorf("failed to get resource groups: %w", err) + return nil, fmt.Errorf("failed to get resource groups: %w", err) } + var eventsToStore []func() + for _, group := range groups { - err = m.groupsManager.RemoveResourceFromGroupInTransaction(ctx, transaction, accountID, group.ID, resourceID) + event, err := m.groupsManager.RemoveResourceFromGroupInTransaction(ctx, transaction, accountID, group.ID, resourceID) if err != nil { - return fmt.Errorf("failed to remove resource from group: %w", err) + return nil, fmt.Errorf("failed to remove resource from group: %w", err) } + eventsToStore = append(eventsToStore, event) } err = transaction.IncrementNetworkSerial(ctx, store.LockingStrengthUpdate, accountID) if err != nil { - return fmt.Errorf("failed to increment network serial: %w", err) + return nil, fmt.Errorf("failed to increment network serial: %w", err) } - return transaction.DeleteNetworkResource(ctx, store.LockingStrengthUpdate, accountID, resourceID) + err = transaction.DeleteNetworkResource(ctx, store.LockingStrengthUpdate, accountID, resourceID) + if err != nil { + return nil, fmt.Errorf("failed to delete network resource: %w", err) + } + + eventsToStore = append(eventsToStore, func() { + m.accountManager.StoreEvent(ctx, accountID, resourceID, accountID, activity.NetworkResourceDeleted, resource.EventMeta(network.Name)) + }) + + return eventsToStore, nil } diff --git a/management/server/networks/resources/types/resource.go b/management/server/networks/resources/types/resource.go index 4ea14bf82e9..c8c19c951b5 100644 --- a/management/server/networks/resources/types/resource.go +++ b/management/server/networks/resources/types/resource.go @@ -142,6 +142,10 @@ func (n *NetworkResource) ToRoute(peer *nbpeer.Peer, router *routerTypes.Network return r } +func (n *NetworkResource) EventMeta(networkName string) map[string]any { + return map[string]any{"name": n.Name, "type": n.Type, "network_name": networkName} +} + // GetResourceType returns the type of the resource based on the address func GetResourceType(address string) (NetworkResourceType, string, netip.Prefix, error) { if prefix, err := netip.ParsePrefix(address); err == nil { diff --git a/management/server/networks/routers/manager.go b/management/server/networks/routers/manager.go index 2103beb06f6..46c7e7a7d3b 100644 --- a/management/server/networks/routers/manager.go +++ b/management/server/networks/routers/manager.go @@ -8,7 +8,9 @@ import ( "github.com/rs/xid" s "github.com/netbirdio/netbird/management/server" + "github.com/netbirdio/netbird/management/server/activity" "github.com/netbirdio/netbird/management/server/networks/routers/types" + networkTypes "github.com/netbirdio/netbird/management/server/networks/types" "github.com/netbirdio/netbird/management/server/permissions" "github.com/netbirdio/netbird/management/server/status" "github.com/netbirdio/netbird/management/server/store" @@ -21,6 +23,7 @@ type Manager interface { GetRouter(ctx context.Context, accountID, userID, networkID, routerID string) (*types.NetworkRouter, error) UpdateRouter(ctx context.Context, userID string, router *types.NetworkRouter) (*types.NetworkRouter, error) DeleteRouter(ctx context.Context, accountID, userID, networkID, routerID string) error + DeleteRouterInTransaction(ctx context.Context, transaction store.Store, accountID, networkID, routerID string) (func(), error) } type managerImpl struct { @@ -80,13 +83,32 @@ func (m *managerImpl) CreateRouter(ctx context.Context, userID string, router *t return nil, status.NewPermissionDeniedError() } - router.ID = xid.New().String() + var network *networkTypes.Network + err = m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error { + network, err = transaction.GetNetworkByID(ctx, store.LockingStrengthShare, router.AccountID, router.NetworkID) + if err != nil { + return fmt.Errorf("failed to get network: %w", err) + } - err = m.store.SaveNetworkRouter(ctx, store.LockingStrengthUpdate, router) + if network.ID != router.NetworkID { + return status.NewNetworkNotFoundError(router.NetworkID) + } + + router.ID = xid.New().String() + + err = transaction.SaveNetworkRouter(ctx, store.LockingStrengthUpdate, router) + if err != nil { + return fmt.Errorf("failed to create network router: %w", err) + } + + return nil + }) if err != nil { - return nil, fmt.Errorf("failed to create network router: %w", err) + return nil, err } + m.accountManager.StoreEvent(ctx, userID, router.ID, router.AccountID, activity.NetworkRouterCreated, router.EventMeta(network.Name)) + go m.accountManager.UpdateAccountPeers(ctx, router.AccountID) return router, nil @@ -122,11 +144,30 @@ func (m *managerImpl) UpdateRouter(ctx context.Context, userID string, router *t return nil, status.NewPermissionDeniedError() } - err = m.store.SaveNetworkRouter(ctx, store.LockingStrengthUpdate, router) + var network *networkTypes.Network + err = m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error { + network, err = transaction.GetNetworkByID(ctx, store.LockingStrengthShare, router.AccountID, router.NetworkID) + if err != nil { + return fmt.Errorf("failed to get network: %w", err) + } + + if network.ID != router.NetworkID { + return status.NewRouterNotPartOfNetworkError(router.ID, router.NetworkID) + } + + err = transaction.SaveNetworkRouter(ctx, store.LockingStrengthUpdate, router) + if err != nil { + return fmt.Errorf("failed to update network router: %w", err) + } + + return nil + }) if err != nil { - return nil, fmt.Errorf("failed to update network router: %w", err) + return nil, err } + m.accountManager.StoreEvent(ctx, userID, router.ID, router.AccountID, activity.NetworkRouterUpdated, router.EventMeta(network.Name)) + go m.accountManager.UpdateAccountPeers(ctx, router.AccountID) return router, nil @@ -141,12 +182,45 @@ func (m *managerImpl) DeleteRouter(ctx context.Context, accountID, userID, netwo return status.NewPermissionDeniedError() } - err = m.store.DeleteNetworkRouter(ctx, store.LockingStrengthUpdate, accountID, routerID) + var event func() + err = m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error { + event, err = m.DeleteRouterInTransaction(ctx, transaction, accountID, networkID, routerID) + return err + }) if err != nil { - return fmt.Errorf("failed to delete network router: %w", err) + return err } + event() + go m.accountManager.UpdateAccountPeers(ctx, accountID) return nil } + +func (m *managerImpl) DeleteRouterInTransaction(ctx context.Context, transaction store.Store, accountID, networkID, routerID string) (func(), error) { + network, err := transaction.GetNetworkByID(ctx, store.LockingStrengthShare, accountID, networkID) + if err != nil { + return nil, fmt.Errorf("failed to get network: %w", err) + } + + router, err := transaction.GetNetworkRouterByID(ctx, store.LockingStrengthUpdate, accountID, routerID) + if err != nil { + return nil, fmt.Errorf("failed to get network router: %w", err) + } + + if router.NetworkID != networkID { + return nil, status.NewRouterNotPartOfNetworkError(routerID, networkID) + } + + err = transaction.DeleteNetworkRouter(ctx, store.LockingStrengthUpdate, accountID, routerID) + if err != nil { + return nil, fmt.Errorf("failed to delete network router: %w", err) + } + + event := func() { + m.accountManager.StoreEvent(ctx, "", routerID, accountID, activity.NetworkRouterDeleted, router.EventMeta(network.Name)) + } + + return event, nil +} diff --git a/management/server/networks/routers/types/router.go b/management/server/networks/routers/types/router.go index b1491d2d17a..4c2e11e905b 100644 --- a/management/server/networks/routers/types/router.go +++ b/management/server/networks/routers/types/router.go @@ -68,3 +68,7 @@ func (n *NetworkRouter) Copy() *NetworkRouter { Metric: n.Metric, } } + +func (n *NetworkRouter) EventMeta(networkName string) map[string]any { + return map[string]any{"network_name": networkName} +} diff --git a/management/server/networks/types/network.go b/management/server/networks/types/network.go index d9525238205..66675e325a7 100644 --- a/management/server/networks/types/network.go +++ b/management/server/networks/types/network.go @@ -49,3 +49,7 @@ func (n *Network) Copy() *Network { Description: n.Description, } } + +func (n *Network) EventMeta() map[string]any { + return map[string]any{"name": n.Name} +} diff --git a/management/server/status/error.go b/management/server/status/error.go index d65931b5a6f..d9cab02315c 100644 --- a/management/server/status/error.go +++ b/management/server/status/error.go @@ -178,3 +178,11 @@ func NewPermissionDeniedError() error { func NewPermissionValidationError(err error) error { return Errorf(PermissionDenied, "failed to vlidate user permissions: %s", err) } + +func NewResourceNotPartOfNetworkError(resourceID, networkID string) error { + return Errorf(BadRequest, "resource %s is not part of the network %s", resourceID, networkID) +} + +func NewRouterNotPartOfNetworkError(routerID, networkID string) error { + return Errorf(BadRequest, "router %s is not part of the network %s", routerID, networkID) +} From 95a18b93cbe28072044f4e40fee8814fc736d91d Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Tue, 17 Dec 2024 22:38:44 +0100 Subject: [PATCH 08/11] add activity events --- management/server/networks/resources/manager.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/management/server/networks/resources/manager.go b/management/server/networks/resources/manager.go index bf08f7b0ef1..fc6e9e3fa60 100644 --- a/management/server/networks/resources/manager.go +++ b/management/server/networks/resources/manager.go @@ -227,10 +227,12 @@ func (m *managerImpl) UpdateResource(ctx context.Context, userID string, resourc if err != nil { return fmt.Errorf("failed to update resource groups: %w", err) } + eventsToStore = append(eventsToStore, events...) eventsToStore = append(eventsToStore, func() { m.accountManager.StoreEvent(ctx, userID, resource.ID, resource.AccountID, activity.NetworkResourceUpdated, resource.EventMeta(network.Name)) }) + return nil }) From 923e8fe4e90b41438cb3c55971b538949c59ad88 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Tue, 17 Dec 2024 23:23:05 +0100 Subject: [PATCH 09/11] add mock and fix test --- management/server/networks/manager_test.go | 31 ++++++++++------ management/server/networks/routers/manager.go | 35 +++++++++++++++++++ 2 files changed, 56 insertions(+), 10 deletions(-) diff --git a/management/server/networks/manager_test.go b/management/server/networks/manager_test.go index 0c0c5cad960..edd830c2564 100644 --- a/management/server/networks/manager_test.go +++ b/management/server/networks/manager_test.go @@ -9,6 +9,7 @@ import ( "github.com/netbirdio/netbird/management/server/groups" "github.com/netbirdio/netbird/management/server/mock_server" "github.com/netbirdio/netbird/management/server/networks/resources" + "github.com/netbirdio/netbird/management/server/networks/routers" "github.com/netbirdio/netbird/management/server/networks/types" "github.com/netbirdio/netbird/management/server/permissions" "github.com/netbirdio/netbird/management/server/store" @@ -27,8 +28,9 @@ func Test_GetAllNetworksReturnsNetworks(t *testing.T) { am := mock_server.MockAccountManager{} permissionsManager := permissions.NewManagerMock() groupsManager := groups.NewManagerMock() + routerManager := routers.NewManagerMock() resourcesManager := resources.NewManager(s, permissionsManager, groupsManager, &am) - manager := NewManager(s, permissionsManager, resourcesManager) + manager := NewManager(s, permissionsManager, resourcesManager, routerManager, &am) networks, err := manager.GetAllNetworks(ctx, accountID, userID) require.NoError(t, err) @@ -49,8 +51,9 @@ func Test_GetAllNetworksReturnsPermissionDenied(t *testing.T) { am := mock_server.MockAccountManager{} permissionsManager := permissions.NewManagerMock() groupsManager := groups.NewManagerMock() + routerManager := routers.NewManagerMock() resourcesManager := resources.NewManager(s, permissionsManager, groupsManager, &am) - manager := NewManager(s, permissionsManager, resourcesManager) + manager := NewManager(s, permissionsManager, resourcesManager, routerManager, &am) networks, err := manager.GetAllNetworks(ctx, accountID, userID) require.Error(t, err) @@ -71,8 +74,9 @@ func Test_GetNetworkReturnsNetwork(t *testing.T) { am := mock_server.MockAccountManager{} permissionsManager := permissions.NewManagerMock() groupsManager := groups.NewManagerMock() + routerManager := routers.NewManagerMock() resourcesManager := resources.NewManager(s, permissionsManager, groupsManager, &am) - manager := NewManager(s, permissionsManager, resourcesManager) + manager := NewManager(s, permissionsManager, resourcesManager, routerManager, &am) networks, err := manager.GetNetwork(ctx, accountID, userID, networkID) require.NoError(t, err) @@ -93,8 +97,9 @@ func Test_GetNetworkReturnsPermissionDenied(t *testing.T) { am := mock_server.MockAccountManager{} permissionsManager := permissions.NewManagerMock() groupsManager := groups.NewManagerMock() + routerManager := routers.NewManagerMock() resourcesManager := resources.NewManager(s, permissionsManager, groupsManager, &am) - manager := NewManager(s, permissionsManager, resourcesManager) + manager := NewManager(s, permissionsManager, resourcesManager, routerManager, &am) network, err := manager.GetNetwork(ctx, accountID, userID, networkID) require.Error(t, err) @@ -117,8 +122,9 @@ func Test_CreateNetworkSuccessfully(t *testing.T) { am := mock_server.MockAccountManager{} permissionsManager := permissions.NewManagerMock() groupsManager := groups.NewManagerMock() + routerManager := routers.NewManagerMock() resourcesManager := resources.NewManager(s, permissionsManager, groupsManager, &am) - manager := NewManager(s, permissionsManager, resourcesManager) + manager := NewManager(s, permissionsManager, resourcesManager, routerManager, &am) createdNetwork, err := manager.CreateNetwork(ctx, userID, network) require.NoError(t, err) @@ -141,8 +147,9 @@ func Test_CreateNetworkFailsWithPermissionDenied(t *testing.T) { am := mock_server.MockAccountManager{} permissionsManager := permissions.NewManagerMock() groupsManager := groups.NewManagerMock() + routerManager := routers.NewManagerMock() resourcesManager := resources.NewManager(s, permissionsManager, groupsManager, &am) - manager := NewManager(s, permissionsManager, resourcesManager) + manager := NewManager(s, permissionsManager, resourcesManager, routerManager, &am) createdNetwork, err := manager.CreateNetwork(ctx, userID, network) require.Error(t, err) @@ -163,8 +170,9 @@ func Test_DeleteNetworkSuccessfully(t *testing.T) { am := mock_server.MockAccountManager{} permissionsManager := permissions.NewManagerMock() groupsManager := groups.NewManagerMock() + routerManager := routers.NewManagerMock() resourcesManager := resources.NewManager(s, permissionsManager, groupsManager, &am) - manager := NewManager(s, permissionsManager, resourcesManager) + manager := NewManager(s, permissionsManager, resourcesManager, routerManager, &am) err = manager.DeleteNetwork(ctx, accountID, userID, networkID) require.NoError(t, err) @@ -184,8 +192,9 @@ func Test_DeleteNetworkFailsWithPermissionDenied(t *testing.T) { am := mock_server.MockAccountManager{} permissionsManager := permissions.NewManagerMock() groupsManager := groups.NewManagerMock() + routerManager := routers.NewManagerMock() resourcesManager := resources.NewManager(s, permissionsManager, groupsManager, &am) - manager := NewManager(s, permissionsManager, resourcesManager) + manager := NewManager(s, permissionsManager, resourcesManager, routerManager, &am) err = manager.DeleteNetwork(ctx, accountID, userID, networkID) require.Error(t, err) @@ -208,8 +217,9 @@ func Test_UpdateNetworkSuccessfully(t *testing.T) { am := mock_server.MockAccountManager{} permissionsManager := permissions.NewManagerMock() groupsManager := groups.NewManagerMock() + routerManager := routers.NewManagerMock() resourcesManager := resources.NewManager(s, permissionsManager, groupsManager, &am) - manager := NewManager(s, permissionsManager, resourcesManager) + manager := NewManager(s, permissionsManager, resourcesManager, routerManager, &am) updatedNetwork, err := manager.UpdateNetwork(ctx, userID, network) require.NoError(t, err) @@ -234,8 +244,9 @@ func Test_UpdateNetworkFailsWithPermissionDenied(t *testing.T) { am := mock_server.MockAccountManager{} permissionsManager := permissions.NewManagerMock() groupsManager := groups.NewManagerMock() + routerManager := routers.NewManagerMock() resourcesManager := resources.NewManager(s, permissionsManager, groupsManager, &am) - manager := NewManager(s, permissionsManager, resourcesManager) + manager := NewManager(s, permissionsManager, resourcesManager, routerManager, &am) updatedNetwork, err := manager.UpdateNetwork(ctx, userID, network) require.Error(t, err) diff --git a/management/server/networks/routers/manager.go b/management/server/networks/routers/manager.go index 46c7e7a7d3b..dc092d8ec35 100644 --- a/management/server/networks/routers/manager.go +++ b/management/server/networks/routers/manager.go @@ -32,6 +32,9 @@ type managerImpl struct { accountManager s.AccountManager } +type mockManager struct { +} + func NewManager(store store.Store, permissionsManager permissions.Manager, accountManager s.AccountManager) Manager { return &managerImpl{ store: store, @@ -224,3 +227,35 @@ func (m *managerImpl) DeleteRouterInTransaction(ctx context.Context, transaction return event, nil } + +func NewManagerMock() Manager { + return &mockManager{} +} + +func (m *mockManager) GetAllRoutersInNetwork(ctx context.Context, accountID, userID, networkID string) ([]*types.NetworkRouter, error) { + return []*types.NetworkRouter{}, nil +} + +func (m *mockManager) GetAllRoutersInAccount(ctx context.Context, accountID, userID string) (map[string][]*types.NetworkRouter, error) { + return map[string][]*types.NetworkRouter{}, nil +} + +func (m *mockManager) CreateRouter(ctx context.Context, userID string, router *types.NetworkRouter) (*types.NetworkRouter, error) { + return router, nil +} + +func (m *mockManager) GetRouter(ctx context.Context, accountID, userID, networkID, routerID string) (*types.NetworkRouter, error) { + return &types.NetworkRouter{}, nil +} + +func (m *mockManager) UpdateRouter(ctx context.Context, userID string, router *types.NetworkRouter) (*types.NetworkRouter, error) { + return router, nil +} + +func (m *mockManager) DeleteRouter(ctx context.Context, accountID, userID, networkID, routerID string) error { + return nil +} + +func (m *mockManager) DeleteRouterInTransaction(ctx context.Context, transaction store.Store, accountID, networkID, routerID string) (func(), error) { + return func() {}, nil +} From 69f058cbe18da4dac57f1395b8df22704ef6739f Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Tue, 17 Dec 2024 23:31:42 +0100 Subject: [PATCH 10/11] remove unused variable --- management/server/networks/manager.go | 1 - 1 file changed, 1 deletion(-) diff --git a/management/server/networks/manager.go b/management/server/networks/manager.go index da37ea5bb21..cc7b546a803 100644 --- a/management/server/networks/manager.go +++ b/management/server/networks/manager.go @@ -26,7 +26,6 @@ type Manager interface { type managerImpl struct { store store.Store - eventStore activity.Store accountManager s.AccountManager permissionsManager permissions.Manager resourcesManager resources.Manager From 3eed5bb9ec1e78af8cb2702d83a125d3a98873d0 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Tue, 17 Dec 2024 23:38:40 +0100 Subject: [PATCH 11/11] fix resource response value --- management/server/networks/resources/manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/management/server/networks/resources/manager.go b/management/server/networks/resources/manager.go index fc6e9e3fa60..f9adc0b83fd 100644 --- a/management/server/networks/resources/manager.go +++ b/management/server/networks/resources/manager.go @@ -203,7 +203,7 @@ func (m *managerImpl) UpdateResource(ctx context.Context, userID string, resourc return status.NewResourceNotPartOfNetworkError(resource.ID, resource.NetworkID) } - resource, err = transaction.GetNetworkResourceByID(ctx, store.LockingStrengthShare, resource.AccountID, resource.ID) + _, err = transaction.GetNetworkResourceByID(ctx, store.LockingStrengthShare, resource.AccountID, resource.ID) if err != nil { return fmt.Errorf("failed to get network resource: %w", err) }