From 3e9b46f8d82fb683ef53255223b85542359ca80f Mon Sep 17 00:00:00 2001 From: Maycon Santos Date: Thu, 1 Jun 2023 16:00:44 +0200 Subject: [PATCH] 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 | 5 + client/internal/routemanager/client.go | 19 +- client/internal/routemanager/client_test.go | 199 ++++++++++++++++++++ release_files/darwin_pkg/postinstall | 1 + 4 files changed, 220 insertions(+), 4 deletions(-) create mode 100644 client/internal/routemanager/client_test.go diff --git a/client/internal/peer/status.go b/client/internal/peer/status.go index 2b311ff09..6e436ba6e 100644 --- a/client/internal/peer/status.go +++ b/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) diff --git a/client/internal/routemanager/client.go b/client/internal/routemanager/client.go index 7ef2d6d4e..cf4cbe91b 100644 --- a/client/internal/routemanager/client.go +++ b/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) } diff --git a/client/internal/routemanager/client_test.go b/client/internal/routemanager/client_test.go new file mode 100644 index 000000000..3700d72ec --- /dev/null +++ b/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) + } + }) + } +} diff --git a/release_files/darwin_pkg/postinstall b/release_files/darwin_pkg/postinstall index 98c40f7e1..503a2a2b5 100755 --- a/release_files/darwin_pkg/postinstall +++ b/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..."