Prevent peer updates on flapping status and fix route score logic (#920)

Prevent peer updates if the status is not changing from disconnected to connected and vice versa.

Fixed route score calculation, added tests and changed the log message

fixed installer /usr/local/bin creation

client/internal/peer/status.go

@@ -146,6 +146,11 @@ func (d *Status) UpdatePeerState(receivedState State) error {
 
 	d.peers[receivedState.PubKey] = peerState
 
+	if receivedState.ConnStatus == StatusConnecting ||
+		(receivedState.ConnStatus == StatusDisconnected && peerState.ConnStatus == StatusConnecting) {
+		return nil
+	}
+
 	ch, found := d.changeNotify[receivedState.PubKey]
 	if found && ch != nil {
 		close(ch)

client/internal/routemanager/client.go

@@ -71,7 +71,7 @@ func (c *clientNetwork) getRouterPeerStatuses() map[string]routerPeerStatus {
 }
 
 func (c *clientNetwork) getBestRouteFromStatuses(routePeerStatuses map[string]routerPeerStatus) string {
-	var chosen string
+	chosen := ""
 	chosenScore := 0
 
 	currID := ""
@@ -85,17 +85,26 @@ func (c *clientNetwork) getBestRouteFromStatuses(routePeerStatuses map[string]ro
 		if !found || !peerStatus.connected {
 			continue
 		}
+
 		if r.Metric < route.MaxMetric {
 			metricDiff := route.MaxMetric - r.Metric
 			tempScore = metricDiff * 10
 		}
+
 		if !peerStatus.relayed {
 			tempScore++
 		}
-		if !peerStatus.direct {
+
+		if peerStatus.direct {
 			tempScore++
 		}
-		if tempScore > chosenScore || (tempScore == chosenScore && currID == r.ID) {
+
+		if tempScore > chosenScore || (tempScore == chosenScore && r.ID == currID) {
+			chosen = r.ID
+			chosenScore = tempScore
+		}
+
+		if chosen == "" && currID == "" {
 			chosen = r.ID
 			chosenScore = tempScore
 		}
@@ -106,7 +115,9 @@ func (c *clientNetwork) getBestRouteFromStatuses(routePeerStatuses map[string]ro
 		for _, r := range c.routes {
 			peers = append(peers, r.Peer)
 		}
-		log.Warnf("no route was chosen for network %s because no peers from list %s were connected", c.network, peers)
+
+		log.Warnf("the network %s has not been assigned a routing peer as no peers from the list %s are currently connected", c.network, peers)
+
 	} else if chosen != currID {
 		log.Infof("new chosen route is %s with peer %s with score %d", chosen, c.routes[chosen].Peer, chosenScore)
 	}

client/internal/routemanager/client_test.go

@@ -0,0 +1,199 @@
+package routemanager
+
+import (
+	"net/netip"
+	"testing"
+
+	"github.com/netbirdio/netbird/route"
+)
+
+func TestGetBestrouteFromStatuses(t *testing.T) {
+
+	testCases := []struct {
+		name            string
+		statuses        map[string]routerPeerStatus
+		expectedRouteID string
+		currentRoute    *route.Route
+		existingRoutes  map[string]*route.Route
+	}{
+		{
+			name: "one route",
+			statuses: map[string]routerPeerStatus{
+				"route1": {
+					connected: true,
+					relayed:   false,
+					direct:    true,
+				},
+			},
+			existingRoutes: map[string]*route.Route{
+				"route1": {
+					ID:     "route1",
+					Metric: route.MaxMetric,
+					Peer:   "peer1",
+				},
+			},
+			currentRoute:    nil,
+			expectedRouteID: "route1",
+		},
+		{
+			name: "one connected routes with relayed and direct",
+			statuses: map[string]routerPeerStatus{
+				"route1": {
+					connected: true,
+					relayed:   true,
+					direct:    true,
+				},
+			},
+			existingRoutes: map[string]*route.Route{
+				"route1": {
+					ID:     "route1",
+					Metric: route.MaxMetric,
+					Peer:   "peer1",
+				},
+			},
+			currentRoute:    nil,
+			expectedRouteID: "route1",
+		},
+		{
+			name: "one connected routes with relayed and no direct",
+			statuses: map[string]routerPeerStatus{
+				"route1": {
+					connected: true,
+					relayed:   true,
+					direct:    false,
+				},
+			},
+			existingRoutes: map[string]*route.Route{
+				"route1": {
+					ID:     "route1",
+					Metric: route.MaxMetric,
+					Peer:   "peer1",
+				},
+			},
+			currentRoute:    nil,
+			expectedRouteID: "route1",
+		},
+		{
+			name: "no connected peers",
+			statuses: map[string]routerPeerStatus{
+				"route1": {
+					connected: false,
+					relayed:   false,
+					direct:    false,
+				},
+			},
+			existingRoutes: map[string]*route.Route{
+				"route1": {
+					ID:     "route1",
+					Metric: route.MaxMetric,
+					Peer:   "peer1",
+				},
+			},
+			currentRoute:    nil,
+			expectedRouteID: "",
+		},
+		{
+			name: "multiple connected peers with different metrics",
+			statuses: map[string]routerPeerStatus{
+				"route1": {
+					connected: true,
+					relayed:   false,
+					direct:    true,
+				},
+				"route2": {
+					connected: true,
+					relayed:   false,
+					direct:    true,
+				},
+			},
+			existingRoutes: map[string]*route.Route{
+				"route1": {
+					ID:     "route1",
+					Metric: 9000,
+					Peer:   "peer1",
+				},
+				"route2": {
+					ID:     "route2",
+					Metric: route.MaxMetric,
+					Peer:   "peer2",
+				},
+			},
+			currentRoute:    nil,
+			expectedRouteID: "route1",
+		},
+		{
+			name: "multiple connected peers with one relayed",
+			statuses: map[string]routerPeerStatus{
+				"route1": {
+					connected: true,
+					relayed:   false,
+					direct:    true,
+				},
+				"route2": {
+					connected: true,
+					relayed:   true,
+					direct:    true,
+				},
+			},
+			existingRoutes: map[string]*route.Route{
+				"route1": {
+					ID:     "route1",
+					Metric: route.MaxMetric,
+					Peer:   "peer1",
+				},
+				"route2": {
+					ID:     "route2",
+					Metric: route.MaxMetric,
+					Peer:   "peer2",
+				},
+			},
+			currentRoute:    nil,
+			expectedRouteID: "route1",
+		},
+		{
+			name: "multiple connected peers with one direct",
+			statuses: map[string]routerPeerStatus{
+				"route1": {
+					connected: true,
+					relayed:   false,
+					direct:    true,
+				},
+				"route2": {
+					connected: true,
+					relayed:   false,
+					direct:    false,
+				},
+			},
+			existingRoutes: map[string]*route.Route{
+				"route1": {
+					ID:     "route1",
+					Metric: route.MaxMetric,
+					Peer:   "peer1",
+				},
+				"route2": {
+					ID:     "route2",
+					Metric: route.MaxMetric,
+					Peer:   "peer2",
+				},
+			},
+			currentRoute:    nil,
+			expectedRouteID: "route1",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			// create new clientNetwork
+			client := &clientNetwork{
+				network:     netip.MustParsePrefix("192.168.0.0/24"),
+				routes:      tc.existingRoutes,
+				chosenRoute: tc.currentRoute,
+			}
+
+			chosenRoute := client.getBestRouteFromStatuses(tc.statuses)
+			if chosenRoute != tc.expectedRouteID {
+				t.Errorf("expected routeID %s, got %s", tc.expectedRouteID, chosenRoute)
+			}
+		})
+	}
+}

release_files/darwin_pkg/postinstall

@@ -5,6 +5,7 @@ AGENT=/usr/local/bin/netbird
 LOG_FILE=/var/log/netbird/client_install.log
 
 mkdir -p /var/log/netbird/
+mkdir -p /usr/local/bin/
 
 {
     echo "Installing NetBird..."