From b9aa7421d695ca2c3d5e68fb9152187a06b17375 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Sun, 14 Apr 2024 19:47:32 -0700 Subject: [PATCH] ipn/ipnlocal: remove some dead code (legacyBackend methods) from LocalBackend Nothing used it. Updates #11649 Change-Id: Ic1c331d947974cd7d4738ff3aafe9c498853689e Signed-off-by: Brad Fitzpatrick --- ipn/backend.go | 2 +- ipn/fake_test.go | 101 ---------------------------------- ipn/ipnlocal/local.go | 31 ++--------- ipn/ipnlocal/local_test.go | 30 ++++------ ipn/ipnlocal/loglines_test.go | 5 +- ipn/localapi/localapi.go | 14 ----- ipn/prefs.go | 2 +- 7 files changed, 21 insertions(+), 164 deletions(-) delete mode 100644 ipn/fake_test.go diff --git a/ipn/backend.go b/ipn/backend.go index 6e1c301f7..764579ef7 100644 --- a/ipn/backend.go +++ b/ipn/backend.go @@ -60,7 +60,7 @@ type EngineStatus struct { 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 diff --git a/ipn/fake_test.go b/ipn/fake_test.go deleted file mode 100644 index 5e1ef0637..000000000 --- a/ipn/fake_test.go +++ /dev/null @@ -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{}}) - } -} diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 80bdc0457..a8faac51c 100644 --- a/ipn/ipnlocal/local.go +++ b/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, diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index 231d5354a..a595cb771 100644 --- a/ipn/ipnlocal/local_test.go +++ b/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) +} diff --git a/ipn/ipnlocal/loglines_test.go b/ipn/ipnlocal/loglines_test.go index 361791858..159a9e211 100644 --- a/ipn/ipnlocal/loglines_test.go +++ b/ipn/ipnlocal/loglines_test.go @@ -26,7 +26,6 @@ // 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()) } diff --git a/ipn/localapi/localapi.go b/ipn/localapi/localapi.go index 6910bf1d9..2d3ca70db 100644 --- a/ipn/localapi/localapi.go +++ b/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) diff --git a/ipn/prefs.go b/ipn/prefs.go index 2fa605a50..bf46b34f7 100644 --- a/ipn/prefs.go +++ b/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