diff --git a/management/server/account.go b/management/server/account.go index ab79a67890b..1f8fc497be5 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -200,7 +200,7 @@ type UserInfo struct { // from the ACL peers that have distribution groups associated with the peer ID. // Please mind, that the returned route.Route objects will contain Peer.Key instead of Peer.ID. func (a *Account) getRoutesToSync(peerID string, aclPeers []*Peer) []*route.Route { - routes, peerDisabledRoutes := a.getEnabledAndDisabledRoutesByPeer(peerID) + routes, peerDisabledRoutes := a.getRoutingPeerRoutes(peerID) peerRoutesMembership := make(lookupMap) for _, r := range append(routes, peerDisabledRoutes...) { peerRoutesMembership[route.GetHAUniqueID(r)] = struct{}{} @@ -208,7 +208,7 @@ func (a *Account) getRoutesToSync(peerID string, aclPeers []*Peer) []*route.Rout groupListMap := a.getPeerGroups(peerID) for _, peer := range aclPeers { - activeRoutes, _ := a.getEnabledAndDisabledRoutesByPeer(peer.ID) + activeRoutes, _ := a.getRoutingPeerRoutes(peer.ID) groupFilteredRoutes := a.filterRoutesByGroups(activeRoutes, groupListMap) filteredRoutes := a.filterRoutesFromPeersOfSameHAGroup(groupFilteredRoutes, peerRoutesMembership) routes = append(routes, filteredRoutes...) @@ -244,20 +244,32 @@ func (a *Account) filterRoutesByGroups(routes []*route.Route, groupListMap looku return filteredRoutes } -// getEnabledAndDisabledRoutesByPeer returns the enabled and disabled lists of routes that belong to a peer. +// getRoutingPeerRoutes returns the enabled and disabled lists of routes that the given routing peer serves // Please mind, that the returned route.Route objects will contain Peer.Key instead of Peer.ID. -func (a *Account) getEnabledAndDisabledRoutesByPeer(peerID string) ([]*route.Route, []*route.Route) { - var enabledRoutes []*route.Route - var disabledRoutes []*route.Route +// If the given is not a routing peer, then the lists are empty. +func (a *Account) getRoutingPeerRoutes(peerID string) (enabledRoutes []*route.Route, disabledRoutes []*route.Route) { + + peer := a.GetPeer(peerID) + if peer == nil { + log.Errorf("peer %s that doesn't exist under account %s", peerID, a.Id) + return enabledRoutes, disabledRoutes + } + + // currently we support only linux routing peers + if peer.Meta.GoOS != "linux" { + return enabledRoutes, disabledRoutes + } + + seenRoute := make(map[string]struct{}) takeRoute := func(r *route.Route, id string) { - peer := a.GetPeer(peerID) - if peer == nil { - log.Errorf("route %s has peer %s that doesn't exist under account %s", r.ID, peerID, a.Id) + if _, ok := seenRoute[r.ID]; ok { return } + seenRoute[r.ID] = struct{}{} if r.Enabled { + r.Peer = peer.Key enabledRoutes = append(enabledRoutes, r) return } @@ -265,25 +277,30 @@ func (a *Account) getEnabledAndDisabledRoutesByPeer(peerID string) ([]*route.Rou } for _, r := range a.Routes { - if len(r.PeerGroups) != 0 { - for _, groupID := range r.PeerGroups { - group := a.GetGroup(groupID) - if group == nil { - log.Errorf("route %s has peers group %s that doesn't exist under account %s", r.ID, groupID, a.Id) + for _, groupID := range r.PeerGroups { + group := a.GetGroup(groupID) + if group == nil { + log.Errorf("route %s has peers group %s that doesn't exist under account %s", r.ID, groupID, a.Id) + continue + } + for _, id := range group.Peers { + if id != peerID { continue } - for _, id := range group.Peers { - if id == peerID { - takeRoute(r, id) - break - } - } + + newPeerRoute := r.Copy() + newPeerRoute.Peer = id + newPeerRoute.PeerGroups = nil + newPeerRoute.ID = r.ID + ":" + id // we have to provide unique route id when distribute network map + takeRoute(newPeerRoute, id) + break } } if r.Peer == peerID { - takeRoute(r, peerID) + takeRoute(r.Copy(), peerID) } } + return enabledRoutes, disabledRoutes } @@ -319,50 +336,7 @@ func (a *Account) GetPeerNetworkMap(peerID, dnsDomain string) *NetworkMap { peersToConnect = append(peersToConnect, p) } - routes := a.getRoutesToSync(peerID, peersToConnect) - - takePeer := func(id string) (*Peer, bool) { - peer := a.GetPeer(id) - if peer == nil || peer.Meta.GoOS != "linux" { - return nil, false - } - return peer, true - } - - // We need to set Peer.Key instead of Peer.ID because this object will be sent to agents as part of a network map. - // Ideally we should have a separate field for that, but fine for now. - var routesUpdate []*route.Route - seenPeers := make(map[string]bool) - for _, r := range routes { - if r.Peer != "" { - peer, valid := takePeer(r.Peer) - if !valid { - continue - } - rCopy := r.Copy() - rCopy.Peer = peer.Key // client expects the key - routesUpdate = append(routesUpdate, rCopy) - continue - } - for _, groupID := range r.PeerGroups { - if group := a.GetGroup(groupID); group != nil { - for _, peerId := range group.Peers { - peer, valid := takePeer(peerId) - if !valid { - continue - } - - if _, ok := seenPeers[peer.ID]; !ok { - rCopy := r.Copy() - rCopy.ID = r.ID + ":" + peer.ID // we have to provide unit route id when distribute network map - rCopy.Peer = peer.Key // client expects the key - routesUpdate = append(routesUpdate, rCopy) - } - seenPeers[peer.ID] = true - } - } - } - } + routesUpdate := a.getRoutesToSync(peerID, peersToConnect) dnsManagementStatus := a.getPeerDNSManagementStatus(peerID) dnsUpdate := nbdns.Config{ diff --git a/management/server/account_test.go b/management/server/account_test.go index d55734685d9..331df2017dd 100644 --- a/management/server/account_test.go +++ b/management/server/account_test.go @@ -1237,7 +1237,7 @@ func TestAccount_GetRoutesToSync(t *testing.T) { } account := &Account{ Peers: map[string]*Peer{ - "peer-1": {Key: "peer-1"}, "peer-2": {Key: "peer-2"}, "peer-3": {Key: "peer-1"}, + "peer-1": {Key: "peer-1", Meta: PeerSystemMeta{GoOS: "linux"}}, "peer-2": {Key: "peer-2", Meta: PeerSystemMeta{GoOS: "linux"}}, "peer-3": {Key: "peer-1", Meta: PeerSystemMeta{GoOS: "linux"}}, }, Groups: map[string]*Group{"group1": {ID: "group1", Peers: []string{"peer-1", "peer-2"}}}, Routes: map[string]*route.Route{ diff --git a/management/server/route.go b/management/server/route.go index 79c207c9bad..6b5aa982d64 100644 --- a/management/server/route.go +++ b/management/server/route.go @@ -98,7 +98,7 @@ func (am *DefaultAccountManager) checkRoutePrefixExistsForPeers(account *Account // check that the peers from peerGroupIDs groups are not the same peers we saw in routesWithPrefix for _, id := range group.Peers { if _, ok := seenPeers[id]; ok { - peer := account.GetPeer(peerID) + peer := account.GetPeer(id) if peer == nil { return status.Errorf(status.InvalidArgument, "peer with ID %s not found", peerID) } diff --git a/management/server/route_test.go b/management/server/route_test.go index 32f15843bfd..00ef3e93a4d 100644 --- a/management/server/route_test.go +++ b/management/server/route_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/rs/xid" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/netbirdio/netbird/management/server/activity" @@ -48,11 +49,12 @@ func TestCreateRoute(t *testing.T) { } testCases := []struct { - name string - inputArgs input - shouldCreate bool - errFunc require.ErrorAssertionFunc - expectedRoute *route.Route + name string + inputArgs input + createInitRoute bool + shouldCreate bool + errFunc require.ErrorAssertionFunc + expectedRoute *route.Route }{ { name: "Happy Path", @@ -164,8 +166,9 @@ func TestCreateRoute(t *testing.T) { enabled: true, groups: []string{routeGroup1}, }, - errFunc: require.Error, - shouldCreate: false, + createInitRoute: true, + errFunc: require.Error, + shouldCreate: false, }, { name: "Bad Peers Group already has this route", @@ -179,8 +182,9 @@ func TestCreateRoute(t *testing.T) { enabled: true, groups: []string{routeGroup1}, }, - errFunc: require.Error, - shouldCreate: false, + createInitRoute: true, + errFunc: require.Error, + shouldCreate: false, }, { name: "Empty Peer Should Create", @@ -326,6 +330,18 @@ func TestCreateRoute(t *testing.T) { t.Errorf("failed to init testing account: %s", err) } + if testCase.createInitRoute { + groupAll, errInit := account.GetGroupAll() + if errInit != nil { + t.Errorf("failed to get group all: %s", errInit) + } + _, errInit = am.CreateRoute(account.Id, existingNetwork, "", []string{routeGroup3, routeGroup4}, + "", existingRouteID, false, 1000, []string{groupAll.ID}, true, userID) + if errInit != nil { + t.Errorf("failed to create init route: %s", errInit) + } + } + outRoute, err := am.CreateRoute( account.Id, testCase.inputArgs.network, @@ -370,17 +386,18 @@ func TestSaveRoute(t *testing.T) { validGroupHA2 := routeGroupHA2 testCases := []struct { - name string - existingRoute *route.Route - newPeer *string - newPeerGroups []string - newMetric *int - newPrefix *netip.Prefix - newGroups []string - skipCopying bool - shouldCreate bool - errFunc require.ErrorAssertionFunc - expectedRoute *route.Route + name string + existingRoute *route.Route + createInitRoute bool + newPeer *string + newPeerGroups []string + newMetric *int + newPrefix *netip.Prefix + newGroups []string + skipCopying bool + shouldCreate bool + errFunc require.ErrorAssertionFunc + expectedRoute *route.Route }{ { name: "Happy Path", @@ -645,8 +662,9 @@ func TestSaveRoute(t *testing.T) { Enabled: true, Groups: []string{routeGroup1}, }, - newPeer: &validUsedPeer, - errFunc: require.Error, + createInitRoute: true, + newPeer: &validUsedPeer, + errFunc: require.Error, }, { name: "Do not allow to modify existing route with a peers group from another route", @@ -662,8 +680,9 @@ func TestSaveRoute(t *testing.T) { Enabled: true, Groups: []string{routeGroup1}, }, - newPeerGroups: []string{routeGroup4}, - errFunc: require.Error, + createInitRoute: true, + newPeerGroups: []string{routeGroup4}, + errFunc: require.Error, }, } for _, testCase := range testCases { @@ -678,6 +697,21 @@ func TestSaveRoute(t *testing.T) { t.Error("failed to init testing account") } + if testCase.createInitRoute { + account.Routes["initRoute"] = &route.Route{ + ID: "initRoute", + Network: netip.MustParsePrefix(existingNetwork), + NetID: existingRouteID, + NetworkType: route.IPv4Network, + PeerGroups: []string{routeGroup4}, + Description: "super", + Masquerade: false, + Metric: 9999, + Enabled: true, + Groups: []string{routeGroup1}, + } + } + account.Routes[testCase.existingRoute.ID] = testCase.existingRoute err = am.Store.SaveAccount(account) @@ -811,15 +845,15 @@ func TestGetNetworkMap_RouteSyncPeerGroups(t *testing.T) { peer1Routes, err := am.GetNetworkMap(peer1ID) require.NoError(t, err) - require.Len(t, peer1Routes.Routes, 3, "HA route should have more than 1 routes") + assert.Len(t, peer1Routes.Routes, 1, "HA route should have 1 server route") peer2Routes, err := am.GetNetworkMap(peer2ID) require.NoError(t, err) - require.Len(t, peer2Routes.Routes, 3, "HA route should have more than 1 routes") + assert.Len(t, peer2Routes.Routes, 1, "HA route should have 1 server route") peer4Routes, err := am.GetNetworkMap(peer4ID) require.NoError(t, err) - require.Len(t, peer4Routes.Routes, 3, "HA route should have more than 1 routes") + assert.Len(t, peer4Routes.Routes, 1, "HA route should have 1 server route") groups, err := am.ListGroups(account.Id) require.NoError(t, err) @@ -838,32 +872,32 @@ func TestGetNetworkMap_RouteSyncPeerGroups(t *testing.T) { peer2RoutesAfterDelete, err := am.GetNetworkMap(peer2ID) require.NoError(t, err) - require.Len(t, peer2RoutesAfterDelete.Routes, 2, "after peer deletion group should have only 2 route") + assert.Len(t, peer2RoutesAfterDelete.Routes, 2, "after peer deletion group should have 2 client routes") err = am.GroupDeletePeer(account.Id, groupHA2.ID, peer4ID) require.NoError(t, err) peer2RoutesAfterDelete, err = am.GetNetworkMap(peer2ID) require.NoError(t, err) - require.Len(t, peer2RoutesAfterDelete.Routes, 1, "after peer deletion group should have only 1 route") + assert.Len(t, peer2RoutesAfterDelete.Routes, 1, "after peer deletion group should have only 1 route") err = am.GroupAddPeer(account.Id, groupHA2.ID, peer4ID) require.NoError(t, err) peer1RoutesAfterAdd, err := am.GetNetworkMap(peer1ID) require.NoError(t, err) - require.Len(t, peer1RoutesAfterAdd.Routes, 2, "HA route should have more than 1 route") + assert.Len(t, peer1RoutesAfterAdd.Routes, 1, "HA route should have more than 1 route") peer2RoutesAfterAdd, err := am.GetNetworkMap(peer2ID) require.NoError(t, err) - require.Len(t, peer2RoutesAfterAdd.Routes, 2, "HA route should have more than 1 route") + assert.Len(t, peer2RoutesAfterAdd.Routes, 2, "HA route should have 2 client routes") err = am.DeleteRoute(account.Id, newRoute.ID, userID) require.NoError(t, err) peer1DeletedRoute, err := am.GetNetworkMap(peer1ID) require.NoError(t, err) - require.Len(t, peer1DeletedRoute.Routes, 0, "we should receive one route for peer1") + assert.Len(t, peer1DeletedRoute.Routes, 0, "we should receive one route for peer1") } func TestGetNetworkMap_RouteSync(t *testing.T) { @@ -1193,11 +1227,5 @@ func initTestRouteAccount(t *testing.T, am *DefaultAccountManager) (*Account, er } } - _, err = am.CreateRoute(account.Id, existingNetwork, "", []string{routeGroup3, routeGroup4}, - "", existingRouteID, false, 1000, []string{groupAll.ID}, true, userID) - if err != nil { - return nil, err - } - return am.Store.GetAccount(account.Id) }