Skip to content

Commit

Permalink
Fix routing groups expand and filtering (netbirdio#1203)
Browse files Browse the repository at this point in the history 
This PR fixes an issue were only one route containing routing groups was being synced to peers.
It also prevents sending routes for peers that aren't connect via ACL.
Moved all checks to Account.getEnabledAndDisabledRoutesByPeer.

Co-authored-by: Yury Gargay <yury.gargay@gmail.com>
Co-authored-by: braginini <bangvalo@gmail.com>
  • Loading branch information
@mlsmaycon @surik @braginini
3 people authored Oct 9, 2023
1 parent 56ada09 commit cfda5ce
Show file tree
Hide file tree
Showing 4 changed files with 107 additions and 105 deletions.
104 changes: 39 additions & 65 deletions management/server/account.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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...)
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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{
Expand Down
2 changes: 1 addition & 1 deletion management/server/account_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
2 changes: 1 addition & 1 deletion management/server/route.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
104 changes: 66 additions & 38 deletions management/server/route_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -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 {
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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) {
Expand Down Expand Up @@ -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)
}

0 comments on commit cfda5ce

Please sign in to comment.