ipn/ipnlocal: remove some dead code (legacyBackend methods) from LocalBackend

Nothing used it.

Updates #11649

Change-Id: Ic1c331d947974cd7d4738ff3aafe9c498853689e
Signed-off-by: Brad Fitzpatrick <[email protected]>

ipn/backend.go

@@ -60,7 +60,7 @@ type NotifyWatchOpt uint64
 const (
 	// NotifyWatchEngineUpdates, if set, causes Engine updates to be sent to the
 	// client either regularly or when they change, without having to ask for
-	// each one via RequestEngineStatus.
+	// each one via Engine.RequestStatus.
 	NotifyWatchEngineUpdates NotifyWatchOpt = 1 << iota
 
 	NotifyInitialState  // if set, the first Notify message (sent immediately) will contain the current State + BrowseToURL + SessionID

ipn/fake_test.go

@@ -1,101 +0,0 @@
-// Copyright (c) Tailscale Inc & AUTHORS
-// SPDX-License-Identifier: BSD-3-Clause
-
-package ipn
-
-import (
-	"tailscale.com/tailcfg"
-	"tailscale.com/types/netmap"
-)
-
-type FakeBackend struct {
-	serverURL string
-	notify    func(n Notify)
-	live      bool
-}
-
-func (b *FakeBackend) Start(opts Options) error {
-	b.serverURL = opts.LegacyMigrationPrefs.ControlURLOrDefault()
-	if b.notify == nil {
-		panic("FakeBackend.Start: SetNotifyCallback not called")
-	}
-	nl := NeedsLogin
-	if b.notify != nil {
-		p := opts.LegacyMigrationPrefs.View()
-		b.notify(Notify{Prefs: &p})
-		b.notify(Notify{State: &nl})
-	}
-	return nil
-}
-
-func (b *FakeBackend) SetNotifyCallback(notify func(Notify)) {
-	if notify == nil {
-		panic("FakeBackend.SetNotifyCallback: notify is nil")
-	}
-	b.notify = notify
-}
-
-func (b *FakeBackend) newState(s State) {
-	if b.notify != nil {
-		b.notify(Notify{State: &s})
-	}
-	if s == Running {
-		b.live = true
-	} else {
-		b.live = false
-	}
-}
-
-func (b *FakeBackend) StartLoginInteractive() {
-	u := b.serverURL + "/this/is/fake"
-	if b.notify != nil {
-		b.notify(Notify{BrowseToURL: &u})
-	}
-	b.login()
-}
-
-func (b *FakeBackend) Login(token *tailcfg.Oauth2Token) {
-	b.login()
-}
-
-func (b *FakeBackend) login() {
-	b.newState(NeedsMachineAuth)
-	b.newState(Stopped)
-	// TODO(apenwarr): Fill in a more interesting netmap here.
-	if b.notify != nil {
-		b.notify(Notify{NetMap: &netmap.NetworkMap{}})
-	}
-	b.newState(Starting)
-	// TODO(apenwarr): Fill in a more interesting status.
-	if b.notify != nil {
-		b.notify(Notify{Engine: &EngineStatus{}})
-	}
-	b.newState(Running)
-}
-
-func (b *FakeBackend) Logout() {
-	b.newState(NeedsLogin)
-}
-
-func (b *FakeBackend) SetPrefs(new *Prefs) {
-	if new == nil {
-		panic("FakeBackend.SetPrefs got nil prefs")
-	}
-
-	if b.notify != nil {
-		p := new.View()
-		b.notify(Notify{Prefs: &p})
-	}
-	if new.WantRunning && !b.live {
-		b.newState(Starting)
-		b.newState(Running)
-	} else if !new.WantRunning && b.live {
-		b.newState(Stopped)
-	}
-}
-
-func (b *FakeBackend) RequestEngineStatus() {
-	if b.notify != nil {
-		b.notify(Notify{Engine: &EngineStatus{}})
-	}
-}

ipn/ipnlocal/local.go

@@ -2366,7 +2366,7 @@ func (b *LocalBackend) WatchNotifications(ctx context.Context, mask ipn.NotifyWa
 	}
 }
 
-// pollRequestEngineStatus calls b.RequestEngineStatus every 2 seconds until ctx
+// pollRequestEngineStatus calls b.e.RequestStatus every 2 seconds until ctx
 // is done.
 func (b *LocalBackend) pollRequestEngineStatus(ctx context.Context) {
 	ticker, tickerChannel := b.clock.NewTicker(2 * time.Second)
@@ -2374,7 +2374,7 @@ func (b *LocalBackend) pollRequestEngineStatus(ctx context.Context) {
 	for {
 		select {
 		case <-tickerChannel:
-			b.RequestEngineStatus()
+			b.e.RequestStatus()
 		case <-ctx.Done():
 			return
 		}
@@ -3222,7 +3222,7 @@ func (b *LocalBackend) editPrefsLockedOnEntry(mp *ipn.MaskedPrefs, unlock unlock
 		return stripKeysFromPrefs(p0), nil
 	}
 	b.logf("EditPrefs: %v", mp.Pretty())
-	newPrefs := b.setPrefsLockedOnEntry("EditPrefs", p1, unlock)
+	newPrefs := b.setPrefsLockedOnEntry(p1, unlock)
 
 	// Note: don't perform any actions for the new prefs here. Not
 	// every prefs change goes through EditPrefs. Put your actions
@@ -3249,17 +3249,6 @@ func (b *LocalBackend) checkProfileNameLocked(p *ipn.Prefs) error {
 	return nil
 }
 
-// SetPrefs saves new user preferences and propagates them throughout
-// the system. Implements Backend.
-func (b *LocalBackend) SetPrefs(newp *ipn.Prefs) {
-	if newp == nil {
-		panic("SetPrefs got nil prefs")
-	}
-	unlock := b.lockAndGetUnlock()
-	defer unlock()
-	b.setPrefsLockedOnEntry("SetPrefs", newp, unlock)
-}
-
 // wantIngressLocked reports whether this node has ingress configured. This bool
 // is sent to the coordination server (in Hostinfo.WireIngress) as an
 // optimization hint to know primarily which nodes are NOT using ingress, to
@@ -3277,7 +3266,7 @@ func (b *LocalBackend) wantIngressLocked() bool {
 // setPrefsLockedOnEntry requires b.mu be held to call it, but it
 // unlocks b.mu when done. newp ownership passes to this function.
 // It returns a readonly copy of the new prefs.
-func (b *LocalBackend) setPrefsLockedOnEntry(caller string, newp *ipn.Prefs, unlock unlockOnce) ipn.PrefsView {
+func (b *LocalBackend) setPrefsLockedOnEntry(newp *ipn.Prefs, unlock unlockOnce) ipn.PrefsView {
 	defer unlock()
 
 	netMap := b.netMap
@@ -3305,10 +3294,6 @@ func (b *LocalBackend) setPrefsLockedOnEntry(caller string, newp *ipn.Prefs, unl
 	hostInfoChanged := !oldHi.Equal(newHi)
 	cc := b.cc
 
-	// [GRINDER STATS LINE] - please don't remove (used for log parsing)
-	if caller == "SetPrefs" {
-		b.logf("SetPrefs: %v", newp.Pretty())
-	}
 	b.updateFilterLocked(netMap, newp.View())
 
 	if oldp.ShouldSSHBeRunning() && !newp.ShouldSSHBeRunning() {
@@ -4470,11 +4455,6 @@ func (b *LocalBackend) nextStateLocked() ipn.State {
 	}
 }
 
-// RequestEngineStatus implements Backend.
-func (b *LocalBackend) RequestEngineStatus() {
-	b.e.RequestStatus()
-}
-
 // stateMachine updates the state machine state based on other things
 // that have happened. It is invoked from the various callbacks that
 // feed events into LocalBackend.
@@ -4559,11 +4539,12 @@ func (b *LocalBackend) requestEngineStatusAndWait() {
 	b.logf("requestEngineStatusAndWait")
 
 	b.statusLock.Lock()
+	defer b.statusLock.Unlock()
+
 	go b.e.RequestStatus()
 	b.logf("requestEngineStatusAndWait: waiting...")
 	b.statusChanged.Wait() // temporarily releases lock while waiting
 	b.logf("requestEngineStatusAndWait: got status update.")
-	b.statusLock.Unlock()
 }
 
 // setControlClientLocked sets the control client to cc,

ipn/ipnlocal/local_test.go

@@ -863,15 +863,6 @@ type legacyBackend interface {
 	// flow. This should trigger a new BrowseToURL notification
 	// eventually.
 	StartLoginInteractive()
-	// SetPrefs installs a new set of user preferences, including
-	// WantRunning. This may cause the wireguard engine to
-	// reconfigure or stop.
-	SetPrefs(*ipn.Prefs)
-	// RequestEngineStatus polls for an update from the wireguard
-	// engine. Only needed if you want to display byte
-	// counts. Connection events are emitted automatically without
-	// polling.
-	RequestEngineStatus()
 }
 
 // Verify that LocalBackend still implements the legacyBackend interface
@@ -1799,7 +1790,8 @@ func TestSetExitNodeIDPolicy(t *testing.T) {
 			b.netMap = test.nm
 			b.pm = pm
 			changed := setExitNodeID(b.pm.prefs.AsStruct(), test.nm)
-			b.SetPrefs(pm.CurrentPrefs().AsStruct())
+			b.SetPrefsForTest(pm.CurrentPrefs().AsStruct())
+
 			if got := b.pm.prefs.ExitNodeID(); got != tailcfg.StableNodeID(test.exitNodeIDWant) {
 				t.Errorf("got %v want %v", got, test.exitNodeIDWant)
 			}
@@ -2062,14 +2054,6 @@ func TestApplySysPolicy(t *testing.T) {
 				}
 			})
 
-			t.Run("set prefs", func(t *testing.T) {
-				b := newTestBackend(t)
-				b.SetPrefs(tt.prefs.Clone())
-				if !b.Prefs().Equals(tt.wantPrefs.View()) {
-					t.Errorf("prefs=%v, want %v", b.Prefs().Pretty(), tt.wantPrefs.Pretty())
-				}
-			})
-
 			t.Run("status update", func(t *testing.T) {
 				// Profile manager fills in blank ControlURL but it's not set
 				// in most test cases to avoid cluttering them, so adjust for
@@ -2639,6 +2623,14 @@ func TestRoundTraffic(t *testing.T) {
 				t.Errorf("unexpected rounding got %v want %v", result, tt.want)
 			}
 		})
+	}
+}
 
+func (b *LocalBackend) SetPrefsForTest(newp *ipn.Prefs) {
+	if newp == nil {
+		panic("SetPrefsForTest got nil prefs")
 	}
+	unlock := b.lockAndGetUnlock()
+	defer unlock()
+	b.setPrefsLockedOnEntry(newp, unlock)
 }

ipn/ipnlocal/loglines_test.go

@@ -26,7 +26,6 @@ import (
 // functions.
 func TestLocalLogLines(t *testing.T) {
 	logListen := tstest.NewLogLineTracker(t.Logf, []string{
-		"SetPrefs: %v",
 		"[v1] peer keys: %s",
 		"[v1] v%v peers: %v",
 	})
@@ -81,7 +80,7 @@ func TestLocalLogLines(t *testing.T) {
 	persist := &persist.Persist{}
 	prefs := ipn.NewPrefs()
 	prefs.Persist = persist
-	lb.SetPrefs(prefs)
+	lb.SetPrefsForTest(prefs)
 
 	t.Run("after_prefs", testWantRemain("[v1] peer keys: %s", "[v1] v%v peers: %v"))
 
@@ -111,5 +110,5 @@ func TestLocalLogLines(t *testing.T) {
 		}},
 	})
 	lb.mu.Unlock()
-	t.Run("after_second_peer_status", testWantRemain("SetPrefs: %v"))
+	t.Run("after_second_peer_status", testWantRemain())
 }

ipn/localapi/localapi.go

@@ -584,20 +584,6 @@ func (h *Handler) serveDebug(w http.ResponseWriter, r *http.Request) {
 		err = h.b.DebugRebind()
 	case "restun":
 		err = h.b.DebugReSTUN()
-	case "enginestatus":
-		// serveRequestEngineStatus kicks off a call to RequestEngineStatus (via
-		// LocalBackend => UserspaceEngine => LocalBackend =>
-		// ipn.Notify{Engine}).
-		//
-		// This is a temporary (2022-11-25) measure for the Windows client's
-		// move to the LocalAPI HTTP interface. It was polling this over the IPN
-		// bus before every 2 seconds which is wasteful. We should add a bit to
-		// WatchIPNMask instead to let an IPN bus watcher say that it's
-		// interested in that info and then only send it on demand, not via
-		// polling. But for now we keep this interface because that's what the
-		// client already did. A future change will remove this, so don't depend
-		// on it.
-		h.b.RequestEngineStatus()
 	case "notify":
 		var n ipn.Notify
 		err = json.NewDecoder(r.Body).Decode(&n)

ipn/prefs.go

@@ -58,7 +58,7 @@ type Prefs struct {
 	// is used. It's set non-empty once the daemon has been started
 	// for the first time.
 	//
-	// TODO(apenwarr): Make it safe to update this with SetPrefs().
+	// TODO(apenwarr): Make it safe to update this with EditPrefs().
 	// Right now, you have to pass it in the initial prefs in Start(),
 	// which is the only code that actually uses the ControlURL value.
 	// It would be more consistent to restart controlclient