extern/netbird
1
0
Fork 0
mirror of https://github.com/netbirdio/netbird.git synced 2024-10-05 09:42:09 +02:00
Code Issues Packages Projects Releases Wiki Activity

Fix routing groups expand and filtering (#1203)

Browse Source 
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>
This commit is contained in:
Maycon Santos 2023-10-09 14:39:41 +02:00 committed by GitHub
parent f7e6cdcbf0
commit 3c485dc7a1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 107 additions and 105 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

104
management/server/account.go
View File

@ -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{

2
management/server/account_test.go
View File

@ -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{

2
management/server/route.go
View File

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

104
management/server/route_test.go
View File

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