Fix routing groups expand and filtering

management/server/account.go

@@ -200,15 +200,15 @@ 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{}{}
 	}
 
 	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,46 +244,63 @@ 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
 		}
 		disabledRoutes = append(disabledRoutes, r)
 	}
 
 	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{

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{

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)
 				}

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)
 }