ipn/ipnlocal: only call UpdateEndpoints when the endpoints change

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>

ipn/ipnlocal/local.go

@@ -620,11 +620,16 @@ func (b *LocalBackend) setWgengineStatus(s *wgengine.Status, err error) {
 	es := b.parseWgStatusLocked(s)
 	cc := b.cc
 	b.engineStatus = es
-	b.endpoints = append([]tailcfg.Endpoint{}, s.LocalAddrs...)
+	needUpdateEndpoints := !endpointsEqual(s.LocalAddrs, b.endpoints)
+	if needUpdateEndpoints {
+		b.endpoints = append([]tailcfg.Endpoint{}, s.LocalAddrs...)
+	}
 	b.mu.Unlock()
 
 	if cc != nil {
-		cc.UpdateEndpoints(0, s.LocalAddrs)
+		if needUpdateEndpoints {
+			cc.UpdateEndpoints(0, s.LocalAddrs)
+		}
 		b.stateMachine()
 	}
 
@@ -635,6 +640,18 @@ func (b *LocalBackend) setWgengineStatus(s *wgengine.Status, err error) {
 	b.send(ipn.Notify{Engine: &es})
 }
 
+func endpointsEqual(x, y []tailcfg.Endpoint) bool {
+	if len(x) != len(y) {
+		return false
+	}
+	for i := range x {
+		if x[i] != y[i] {
+			return false
+		}
+	}
+	return true
+}
+
 func (b *LocalBackend) SetNotifyCallback(notify func(ipn.Notify)) {
 	b.mu.Lock()
 	defer b.mu.Unlock()

ipn/ipnlocal/state_test.go

@@ -412,11 +412,7 @@ func TestStateMachine(t *testing.T) {
 	b.StartLoginInteractive()
 	{
 		nn := notifies.drain(1)
-		// BUG: UpdateEndpoints shouldn't be called yet.
-		// We're still not logged in so there's nothing we can do
-		// with it. (And empirically, it's providing an empty list
-		// of endpoints.)
-		c.Assert([]string{"UpdateEndpoints", "unpause"}, qt.DeepEquals, cc.getCalls())
+		c.Assert([]string{"unpause"}, qt.DeepEquals, cc.getCalls())
 		c.Assert(nn[0].BrowseToURL, qt.Not(qt.IsNil))
 		c.Assert(url1, qt.Equals, *nn[0].BrowseToURL)
 	}
@@ -441,8 +437,7 @@ func TestStateMachine(t *testing.T) {
 	url2 := "http://localhost:1/2"
 	cc.send(nil, url2, false, nil)
 	{
-		// BUG: UpdateEndpoints again, this is getting silly.
-		c.Assert([]string{"UpdateEndpoints", "unpause", "unpause"}, qt.DeepEquals, cc.getCalls())
+		c.Assert([]string{"unpause", "unpause"}, qt.DeepEquals, cc.getCalls())
 
 		// This time, backend should emit it to the UI right away,
 		// because the UI is anxiously awaiting a new URL to visit.
@@ -462,8 +457,6 @@ func TestStateMachine(t *testing.T) {
 	cc.send(nil, "", true, &netmap.NetworkMap{})
 	{
 		nn := notifies.drain(3)
-		// BUG: still too soon for UpdateEndpoints.
-		//
 		// Arguably it makes sense to unpause now, since the machine
 		// authorization status is part of the netmap.
 		//
@@ -472,7 +465,7 @@ func TestStateMachine(t *testing.T) {
 		// wait until it gets into Starting.
 		// TODO: (Currently this test doesn't detect that bug, but
 		// it's visible in the logs)
-		c.Assert([]string{"unpause", "unpause", "UpdateEndpoints", "unpause"}, qt.DeepEquals, cc.getCalls())
+		c.Assert([]string{"unpause", "unpause", "unpause"}, qt.DeepEquals, cc.getCalls())
 		c.Assert(nn[0].LoginFinished, qt.Not(qt.IsNil))
 		c.Assert(nn[1].Prefs, qt.Not(qt.IsNil))
 		c.Assert(nn[2].State, qt.Not(qt.IsNil))
@@ -494,7 +487,7 @@ func TestStateMachine(t *testing.T) {
 	})
 	{
 		nn := notifies.drain(1)
-		c.Assert([]string{"unpause", "unpause", "UpdateEndpoints", "unpause"}, qt.DeepEquals, cc.getCalls())
+		c.Assert([]string{"unpause", "unpause", "unpause"}, qt.DeepEquals, cc.getCalls())
 		c.Assert(nn[0].State, qt.Not(qt.IsNil))
 		c.Assert(ipn.Starting, qt.Equals, *nn[0].State)
 	}
@@ -534,9 +527,8 @@ func TestStateMachine(t *testing.T) {
 	})
 	{
 		nn := notifies.drain(2)
-		// BUG: UpdateEndpoints isn't needed here.
 		// BUG: Login isn't needed here. We never logged out.
-		c.Assert([]string{"Login", "unpause", "UpdateEndpoints", "unpause"}, qt.DeepEquals, cc.getCalls())
+		c.Assert([]string{"Login", "unpause", "unpause"}, qt.DeepEquals, cc.getCalls())
 		// BUG: I would expect Prefs to change first, and state after.
 		c.Assert(nn[0].State, qt.Not(qt.IsNil))
 		c.Assert(nn[1].Prefs, qt.Not(qt.IsNil))
@@ -677,9 +669,8 @@ func TestStateMachine(t *testing.T) {
 	c.Assert(b.Start(ipn.Options{StateKey: ipn.GlobalDaemonStateKey}), qt.IsNil)
 	{
 		// BUG: We already called Shutdown(), no need to do it again.
-		// BUG: Way too soon for UpdateEndpoints.
 		// BUG: don't unpause because we're not logged in.
-		c.Assert([]string{"Shutdown", "unpause", "New", "UpdateEndpoints", "unpause"}, qt.DeepEquals, cc.getCalls())
+		c.Assert([]string{"Shutdown", "unpause", "New", "unpause"}, qt.DeepEquals, cc.getCalls())
 
 		nn := notifies.drain(2)
 		c.Assert(cc.getCalls(), qt.HasLen, 0)
@@ -739,13 +730,12 @@ func TestStateMachine(t *testing.T) {
 	{
 		// NOTE: cc.Shutdown() is correct here, since we didn't call
 		// b.Shutdown() explicitly ourselves.
-		// BUG: UpdateEndpoints should be called here since we're not WantRunning.
 		// Note: unpause happens because ipn needs to get at least one netmap
 		//  on startup, otherwise UIs can't show the node list, login
 		//  name, etc when in state ipn.Stopped.
 		//  Arguably they shouldn't try. But they currently do.
 		nn := notifies.drain(2)
-		c.Assert([]string{"Shutdown", "unpause", "New", "UpdateEndpoints", "Login", "unpause"}, qt.DeepEquals, cc.getCalls())
+		c.Assert([]string{"Shutdown", "unpause", "New", "Login", "unpause"}, qt.DeepEquals, cc.getCalls())
 		c.Assert(cc.getCalls(), qt.HasLen, 0)
 		c.Assert(nn[0].Prefs, qt.Not(qt.IsNil))
 		c.Assert(nn[1].State, qt.Not(qt.IsNil))
@@ -863,7 +853,7 @@ func TestStateMachine(t *testing.T) {
 	{
 		// NOTE: cc.Shutdown() is correct here, since we didn't call
 		// b.Shutdown() ourselves.
-		c.Assert([]string{"Shutdown", "unpause", "New", "UpdateEndpoints", "Login", "unpause"}, qt.DeepEquals, cc.getCalls())
+		c.Assert([]string{"Shutdown", "unpause", "New", "Login", "unpause"}, qt.DeepEquals, cc.getCalls())
 
 		nn := notifies.drain(1)
 		c.Assert(cc.getCalls(), qt.HasLen, 0)