[client] Fix HA router switch (#3889)

* Fix HA router switch.

- Simplify the notification filter logic.
Always send notification if a state has been changed

- Remove IP changes check because we never modify

* Notify only the proper listeners

* Fix test

* Fix TestGetPeerStateChangeNotifierLogic test

* Before lazy connection, when the peer disconnected, the status switched to disconnected.
After implementing lazy connection, the peer state is connecting, so we did not decrease the reference counters on the routes.

* When switch to idle notify the route mgr

Author: pappz

client/internal/peer/status.go

@@ -289,11 +289,7 @@ func (d *Status) UpdatePeerState(receivedState State) error {
 		return errors.New("peer doesn't exist")
 	}
 
-	if receivedState.IP != "" {
-		peerState.IP = receivedState.IP
-	}
-
-	skipNotification := shouldSkipNotify(receivedState.ConnStatus, peerState)
+	oldState := peerState.ConnStatus
 
 	if receivedState.ConnStatus != peerState.ConnStatus {
 		peerState.ConnStatus = receivedState.ConnStatus
@@ -309,11 +305,15 @@ func (d *Status) UpdatePeerState(receivedState State) error {
 
 	d.peers[receivedState.PubKey] = peerState
 
-	if skipNotification {
-		return nil
+	if hasConnStatusChanged(oldState, receivedState.ConnStatus) {
+		d.notifyPeerListChanged()
 	}
 
-	d.notifyPeerListChanged()
+	// when we close the connection we will not notify the router manager
+	if receivedState.ConnStatus == StatusIdle {
+		d.notifyPeerStateChangeListeners(receivedState.PubKey)
+
+	}
 	return nil
 }
 
@@ -380,11 +380,8 @@ func (d *Status) UpdatePeerICEState(receivedState State) error {
 		return errors.New("peer doesn't exist")
 	}
 
-	if receivedState.IP != "" {
-		peerState.IP = receivedState.IP
-	}
-
-	skipNotification := shouldSkipNotify(receivedState.ConnStatus, peerState)
+	oldState := peerState.ConnStatus
+	oldIsRelayed := peerState.Relayed
 
 	peerState.ConnStatus = receivedState.ConnStatus
 	peerState.ConnStatusUpdate = receivedState.ConnStatusUpdate
@@ -397,12 +394,13 @@ func (d *Status) UpdatePeerICEState(receivedState State) error {
 
 	d.peers[receivedState.PubKey] = peerState
 
-	if skipNotification {
-		return nil
+	if hasConnStatusChanged(oldState, receivedState.ConnStatus) {
+		d.notifyPeerListChanged()
 	}
 
-	d.notifyPeerStateChangeListeners(receivedState.PubKey)
-	d.notifyPeerListChanged()
+	if hasStatusOrRelayedChange(oldState, receivedState.ConnStatus, oldIsRelayed, receivedState.Relayed) {
+		d.notifyPeerStateChangeListeners(receivedState.PubKey)
+	}
 	return nil
 }
 
@@ -415,7 +413,8 @@ func (d *Status) UpdatePeerRelayedState(receivedState State) error {
 		return errors.New("peer doesn't exist")
 	}
 
-	skipNotification := shouldSkipNotify(receivedState.ConnStatus, peerState)
+	oldState := peerState.ConnStatus
+	oldIsRelayed := peerState.Relayed
 
 	peerState.ConnStatus = receivedState.ConnStatus
 	peerState.ConnStatusUpdate = receivedState.ConnStatusUpdate
@@ -425,12 +424,13 @@ func (d *Status) UpdatePeerRelayedState(receivedState State) error {
 
 	d.peers[receivedState.PubKey] = peerState
 
-	if skipNotification {
-		return nil
+	if hasConnStatusChanged(oldState, receivedState.ConnStatus) {
+		d.notifyPeerListChanged()
 	}
 
-	d.notifyPeerStateChangeListeners(receivedState.PubKey)
-	d.notifyPeerListChanged()
+	if hasStatusOrRelayedChange(oldState, receivedState.ConnStatus, oldIsRelayed, receivedState.Relayed) {
+		d.notifyPeerStateChangeListeners(receivedState.PubKey)
+	}
 	return nil
 }
 
@@ -443,7 +443,8 @@ func (d *Status) UpdatePeerRelayedStateToDisconnected(receivedState State) error
 		return errors.New("peer doesn't exist")
 	}
 
-	skipNotification := shouldSkipNotify(receivedState.ConnStatus, peerState)
+	oldState := peerState.ConnStatus
+	oldIsRelayed := peerState.Relayed
 
 	peerState.ConnStatus = receivedState.ConnStatus
 	peerState.Relayed = receivedState.Relayed
@@ -452,12 +453,13 @@ func (d *Status) UpdatePeerRelayedStateToDisconnected(receivedState State) error
 
 	d.peers[receivedState.PubKey] = peerState
 
-	if skipNotification {
-		return nil
+	if hasConnStatusChanged(oldState, receivedState.ConnStatus) {
+		d.notifyPeerListChanged()
 	}
 
-	d.notifyPeerStateChangeListeners(receivedState.PubKey)
-	d.notifyPeerListChanged()
+	if hasStatusOrRelayedChange(oldState, receivedState.ConnStatus, oldIsRelayed, receivedState.Relayed) {
+		d.notifyPeerStateChangeListeners(receivedState.PubKey)
+	}
 	return nil
 }
 
@@ -470,7 +472,8 @@ func (d *Status) UpdatePeerICEStateToDisconnected(receivedState State) error {
 		return errors.New("peer doesn't exist")
 	}
 
-	skipNotification := shouldSkipNotify(receivedState.ConnStatus, peerState)
+	oldState := peerState.ConnStatus
+	oldIsRelayed := peerState.Relayed
 
 	peerState.ConnStatus = receivedState.ConnStatus
 	peerState.Relayed = receivedState.Relayed
@@ -482,12 +485,13 @@ func (d *Status) UpdatePeerICEStateToDisconnected(receivedState State) error {
 
 	d.peers[receivedState.PubKey] = peerState
 
-	if skipNotification {
-		return nil
+	if hasConnStatusChanged(oldState, receivedState.ConnStatus) {
+		d.notifyPeerListChanged()
 	}
 
-	d.notifyPeerStateChangeListeners(receivedState.PubKey)
-	d.notifyPeerListChanged()
+	if hasStatusOrRelayedChange(oldState, receivedState.ConnStatus, oldIsRelayed, receivedState.Relayed) {
+		d.notifyPeerStateChangeListeners(receivedState.PubKey)
+	}
 	return nil
 }
 
@@ -510,17 +514,12 @@ func (d *Status) UpdateWireGuardPeerState(pubKey string, wgStats configurer.WGSt
 	return nil
 }
 
-func shouldSkipNotify(receivedConnStatus ConnStatus, curr State) bool {
-	switch {
-	case receivedConnStatus == StatusConnecting:
-		return true
-	case receivedConnStatus == StatusIdle && curr.ConnStatus == StatusConnecting:
-		return true
-	case receivedConnStatus == StatusIdle && curr.ConnStatus == StatusIdle:
-		return curr.IP != ""
-	default:
-		return false
-	}
+func hasStatusOrRelayedChange(oldConnStatus, newConnStatus ConnStatus, oldRelayed, newRelayed bool) bool {
+	return oldRelayed != newRelayed || hasConnStatusChanged(newConnStatus, oldConnStatus)
+}
+
+func hasConnStatusChanged(oldStatus, newStatus ConnStatus) bool {
+	return newStatus != oldStatus
 }
 
 // UpdatePeerFQDN update peer's state fqdn only

client/internal/peer/status_test.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"sync"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -42,16 +43,16 @@ func TestGetPeer(t *testing.T) {
 func TestUpdatePeerState(t *testing.T) {
 	key := "abc"
 	ip := "10.10.10.10"
+	fqdn := "peer-a.netbird.local"
 	status := NewRecorder("https://mgm")
+	_ = status.AddPeer(key, fqdn, ip)
+
 	peerState := State{
-		PubKey: key,
-		Mux:    new(sync.RWMutex),
+		PubKey:           key,
+		ConnStatusUpdate: time.Now(),
+		ConnStatus:       StatusConnecting,
 	}
 
-	status.peers[key] = peerState
-
-	peerState.IP = ip
-
 	err := status.UpdatePeerState(peerState)
 	assert.NoError(t, err, "shouldn't return error")
 
@@ -83,17 +84,17 @@ func TestGetPeerStateChangeNotifierLogic(t *testing.T) {
 	key := "abc"
 	ip := "10.10.10.10"
 	status := NewRecorder("https://mgm")
-	peerState := State{
-		PubKey: key,
-		Mux:    new(sync.RWMutex),
-	}
-
-	status.peers[key] = peerState
+	_ = status.AddPeer(key, "abc.netbird", ip)
 
 	ch := status.GetPeerStateChangeNotifier(key)
 	assert.NotNil(t, ch, "channel shouldn't be nil")
 
-	peerState.IP = ip
+	peerState := State{
+		PubKey:           key,
+		ConnStatus:       StatusConnecting,
+		Relayed:          false,
+		ConnStatusUpdate: time.Now(),
+	}
 
 	err := status.UpdatePeerRelayedStateToDisconnected(peerState)
 	assert.NoError(t, err, "shouldn't return error")

client/internal/routemanager/client.go

@@ -232,7 +232,7 @@ func (c *clientNetwork) watchPeerStatusChanges(ctx context.Context, peerKey stri
 			return
 		case <-c.statusRecorder.GetPeerStateChangeNotifier(peerKey):
 			state, err := c.statusRecorder.GetPeer(peerKey)
-			if err != nil || state.ConnStatus == peer.StatusConnecting {
+			if err != nil {
 				continue
 			}
 			peerStateUpdate <- struct{}{}