From 799973a68d48b9434ea20f24cbeb068d5aba16b3 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 6 Apr 2021 22:11:50 -0700 Subject: [PATCH] ipn: move Options.Notify to its own method We already had SetNotifyCallback elsewhere on controlclient, so use that name. Baby steps towards some CLI refactor work. Updates tailscale/tailscale#1436 --- cmd/tailscale/cli/up.go | 71 ++++++++++++++++++++-------------------- cmd/tailscale/cli/web.go | 42 ++++++++++++------------ ipn/backend.go | 5 +-- ipn/fake_test.go | 51 +++++++++++++++++++++-------- ipn/handle.go | 26 ++++++++++----- ipn/ipnlocal/local.go | 7 +++- ipn/message.go | 4 +-- ipn/message_test.go | 6 ++-- 8 files changed, 122 insertions(+), 90 deletions(-) diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index e3d7da919..086617c1f 100644 --- a/cmd/tailscale/cli/up.go +++ b/cmd/tailscale/cli/up.go @@ -238,44 +238,44 @@ func runUp(ctx context.Context, args []string) error { startLoginInteractive := func() { loginOnce.Do(func() { bc.StartLoginInteractive() }) } bc.SetPrefs(prefs) - - opts := ipn.Options{ - StateKey: ipn.GlobalDaemonStateKey, - AuthKey: upArgs.authKey, - Notify: func(n ipn.Notify) { - if n.ErrMessage != nil { - msg := *n.ErrMessage - if msg == ipn.ErrMsgPermissionDenied { - switch runtime.GOOS { - case "windows": - msg += " (Tailscale service in use by other user?)" - default: - msg += " (try 'sudo tailscale up [...]')" - } + bc.SetNotifyCallback(func(n ipn.Notify) { + if n.ErrMessage != nil { + msg := *n.ErrMessage + if msg == ipn.ErrMsgPermissionDenied { + switch runtime.GOOS { + case "windows": + msg += " (Tailscale service in use by other user?)" + default: + msg += " (try 'sudo tailscale up [...]')" } - fatalf("backend error: %v\n", msg) } - if s := n.State; s != nil { - switch *s { - case ipn.NeedsLogin: - printed = true - startLoginInteractive() - case ipn.NeedsMachineAuth: - printed = true - fmt.Fprintf(os.Stderr, "\nTo authorize your machine, visit (as admin):\n\n\t%s/admin/machines\n\n", upArgs.server) - case ipn.Starting, ipn.Running: - // Done full authentication process - if printed { - // Only need to print an update if we printed the "please click" message earlier. - fmt.Fprintf(os.Stderr, "Success.\n") - } - cancel() + fatalf("backend error: %v\n", msg) + } + if s := n.State; s != nil { + switch *s { + case ipn.NeedsLogin: + printed = true + startLoginInteractive() + case ipn.NeedsMachineAuth: + printed = true + fmt.Fprintf(os.Stderr, "\nTo authorize your machine, visit (as admin):\n\n\t%s/admin/machines\n\n", upArgs.server) + case ipn.Starting, ipn.Running: + // Done full authentication process + if printed { + // Only need to print an update if we printed the "please click" message earlier. + fmt.Fprintf(os.Stderr, "Success.\n") } + cancel() } - if url := n.BrowseToURL; url != nil { - fmt.Fprintf(os.Stderr, "\nTo authenticate, visit:\n\n\t%s\n\n", *url) - } - }, + } + if url := n.BrowseToURL; url != nil { + fmt.Fprintf(os.Stderr, "\nTo authenticate, visit:\n\n\t%s\n\n", *url) + } + }) + + opts := ipn.Options{ + StateKey: ipn.GlobalDaemonStateKey, + AuthKey: upArgs.authKey, } // On Windows, we still run in mostly the "legacy" way that @@ -293,8 +293,7 @@ func runUp(ctx context.Context, args []string) error { opts.Prefs = prefs } - // We still have to Start right now because it's the only way to - // set up notifications and whatnot. This causes a bunch of churn + // We still have to Start for now. This causes a bunch of churn // every time the CLI touches anything. // // TODO(danderson): redo the frontend/backend API to assume diff --git a/cmd/tailscale/cli/web.go b/cmd/tailscale/cli/web.go index 96aab1553..f6d8bb72e 100644 --- a/cmd/tailscale/cli/web.go +++ b/cmd/tailscale/cli/web.go @@ -259,30 +259,30 @@ func tailscaleUp(ctx context.Context) (authURL string, retErr error) { c, bc, ctx, cancel := connect(ctx) defer cancel() + bc.SetNotifyCallback(func(n ipn.Notify) { + if n.ErrMessage != nil { + msg := *n.ErrMessage + if msg == ipn.ErrMsgPermissionDenied { + switch runtime.GOOS { + case "windows": + msg += " (Tailscale service in use by other user?)" + default: + msg += " (try 'sudo tailscale up [...]')" + } + } + retErr = fmt.Errorf("backend error: %v", msg) + cancel() + } else if url := n.BrowseToURL; url != nil { + authURL = *url + cancel() + } + }) + bc.SetPrefs(prefs) - opts := ipn.Options{ + bc.Start(ipn.Options{ StateKey: ipn.GlobalDaemonStateKey, - Notify: func(n ipn.Notify) { - if n.ErrMessage != nil { - msg := *n.ErrMessage - if msg == ipn.ErrMsgPermissionDenied { - switch runtime.GOOS { - case "windows": - msg += " (Tailscale service in use by other user?)" - default: - msg += " (try 'sudo tailscale up [...]')" - } - } - retErr = fmt.Errorf("backend error: %v", msg) - cancel() - } else if url := n.BrowseToURL; url != nil { - authURL = *url - cancel() - } - }, - } - bc.Start(opts) + }) bc.StartLoginInteractive() pump(ctx, bc, c) diff --git a/ipn/backend.go b/ipn/backend.go index e900ffe5e..a592809c5 100644 --- a/ipn/backend.go +++ b/ipn/backend.go @@ -122,8 +122,6 @@ type Options struct { // TODO(danderson): remove some time after the transition to // tailscaled is done. LegacyConfigPath string - // Notify is called when backend events happen. - Notify func(Notify) `json:"-"` // HTTPTestClient is an optional HTTP client to pass to controlclient // (for tests only). HTTPTestClient *http.Client @@ -135,6 +133,9 @@ type Options struct { // (It has nothing to do with the interface between the backends // and the cloud control plane.) type Backend interface { + // SetNotifyCallback sets the callback to be called on updates + // from the backend to the client. + SetNotifyCallback(func(Notify)) // Start starts or restarts the backend, typically when a // frontend client connects. Start(Options) error diff --git a/ipn/fake_test.go b/ipn/fake_test.go index 6e7e2809f..c1e0b50da 100644 --- a/ipn/fake_test.go +++ b/ipn/fake_test.go @@ -5,7 +5,6 @@ package ipn import ( - "log" "time" "tailscale.com/ipn/ipnstate" @@ -21,18 +20,28 @@ type FakeBackend struct { func (b *FakeBackend) Start(opts Options) error { b.serverURL = opts.Prefs.ControlURL - if opts.Notify == nil { - log.Fatalf("FakeBackend.Start: opts.Notify is nil\n") + if b.notify == nil { + panic("FakeBackend.Start: SetNotifyCallback not called") } - b.notify = opts.Notify - b.notify(Notify{Prefs: opts.Prefs}) nl := NeedsLogin - b.notify(Notify{State: &nl}) + if b.notify != nil { + b.notify(Notify{Prefs: opts.Prefs}) + 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) { - b.notify(Notify{State: &s}) + if b.notify != nil { + b.notify(Notify{State: &s}) + } if s == Running { b.live = true } else { @@ -42,7 +51,9 @@ func (b *FakeBackend) newState(s State) { func (b *FakeBackend) StartLoginInteractive() { u := b.serverURL + "/this/is/fake" - b.notify(Notify{BrowseToURL: &u}) + if b.notify != nil { + b.notify(Notify{BrowseToURL: &u}) + } b.login() } @@ -54,10 +65,14 @@ func (b *FakeBackend) login() { b.newState(NeedsMachineAuth) b.newState(Stopped) // TODO(apenwarr): Fill in a more interesting netmap here. - b.notify(Notify{NetMap: &netmap.NetworkMap{}}) + if b.notify != nil { + b.notify(Notify{NetMap: &netmap.NetworkMap{}}) + } b.newState(Starting) // TODO(apenwarr): Fill in a more interesting status. - b.notify(Notify{Engine: &EngineStatus{}}) + if b.notify != nil { + b.notify(Notify{Engine: &EngineStatus{}}) + } b.newState(Running) } @@ -70,7 +85,9 @@ func (b *FakeBackend) SetPrefs(new *Prefs) { panic("FakeBackend.SetPrefs got nil prefs") } - b.notify(Notify{Prefs: new.Clone()}) + if b.notify != nil { + b.notify(Notify{Prefs: new.Clone()}) + } if new.WantRunning && !b.live { b.newState(Starting) b.newState(Running) @@ -87,13 +104,19 @@ func (b *FakeBackend) EditPrefs(mp *MaskedPrefs) { } func (b *FakeBackend) RequestEngineStatus() { - b.notify(Notify{Engine: &EngineStatus{}}) + if b.notify != nil { + b.notify(Notify{Engine: &EngineStatus{}}) + } } func (b *FakeBackend) FakeExpireAfter(x time.Duration) { - b.notify(Notify{NetMap: &netmap.NetworkMap{}}) + if b.notify != nil { + b.notify(Notify{NetMap: &netmap.NetworkMap{}}) + } } func (b *FakeBackend) Ping(ip string, useTSMP bool) { - b.notify(Notify{PingResult: &ipnstate.PingResult{}}) + if b.notify != nil { + b.notify(Notify{PingResult: &ipnstate.PingResult{}}) + } } diff --git a/ipn/handle.go b/ipn/handle.go index 54a61140e..756dca5b0 100644 --- a/ipn/handle.go +++ b/ipn/handle.go @@ -15,25 +15,26 @@ import ( ) type Handle struct { - frontendLogID string - b Backend - xnotify func(Notify) - logf logger.Logf + b Backend + logf logger.Logf // Mutex protects everything below mu sync.Mutex + xnotify func(Notify) + frontendLogID string netmapCache *netmap.NetworkMap engineStatusCache EngineStatus stateCache State prefsCache *Prefs } -func NewHandle(b Backend, logf logger.Logf, opts Options) (*Handle, error) { +func NewHandle(b Backend, logf logger.Logf, notify func(Notify), opts Options) (*Handle, error) { h := &Handle{ b: b, logf: logf, } + h.SetNotifyCallback(notify) err := h.Start(opts) if err != nil { return nil, err @@ -42,18 +43,25 @@ func NewHandle(b Backend, logf logger.Logf, opts Options) (*Handle, error) { return h, nil } +func (h *Handle) SetNotifyCallback(notify func(Notify)) { + h.mu.Lock() + h.xnotify = notify + h.mu.Unlock() + + h.b.SetNotifyCallback(h.notify) +} + func (h *Handle) Start(opts Options) error { + h.mu.Lock() h.frontendLogID = opts.FrontendLogID - h.xnotify = opts.Notify h.netmapCache = nil h.engineStatusCache = EngineStatus{} h.stateCache = NoState if opts.Prefs != nil { h.prefsCache = opts.Prefs.Clone() } - xopts := opts - xopts.Notify = h.notify - return h.b.Start(xopts) + h.mu.Unlock() + return h.b.Start(opts) } func (h *Handle) Reset() { diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 2e3318540..4ce9e2345 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -519,6 +519,12 @@ func (b *LocalBackend) setWgengineStatus(s *wgengine.Status, err error) { b.send(ipn.Notify{Engine: &es}) } +func (b *LocalBackend) SetNotifyCallback(notify func(ipn.Notify)) { + b.mu.Lock() + defer b.mu.Unlock() + b.notify = notify +} + // Start applies the configuration specified in opts, and starts the // state machine. // @@ -585,7 +591,6 @@ func (b *LocalBackend) Start(opts ipn.Options) error { } applyPrefsToHostinfo(hostinfo, b.prefs) - b.notify = opts.Notify b.setNetMapLocked(nil) persistv := b.prefs.Persist b.mu.Unlock() diff --git a/ipn/message.go b/ipn/message.go index 29c709c12..8628b6f84 100644 --- a/ipn/message.go +++ b/ipn/message.go @@ -189,8 +189,8 @@ func (bs *BackendServer) GotCommand(ctx context.Context, cmd *Command) error { bs.GotQuit = true return errors.New("Quit command received") } else if c := cmd.Start; c != nil { + bs.b.SetNotifyCallback(bs.send) opts := c.Opts - opts.Notify = bs.send return bs.b.Start(opts) } else if c := cmd.StartLoginInteractive; c != nil { bs.b.StartLoginInteractive() @@ -287,8 +287,6 @@ func (bc *BackendClient) Quit() error { } func (bc *BackendClient) Start(opts Options) error { - bc.notify = opts.Notify - opts.Notify = nil // server can't call our function pointer bc.send(Command{Start: &StartArgs{Opts: opts}}) return nil // remote Start() errors must be handled remotely } diff --git a/ipn/message_test.go b/ipn/message_test.go index 59a9eef25..1cc2d6fc2 100644 --- a/ipn/message_test.go +++ b/ipn/message_test.go @@ -90,13 +90,11 @@ func TestClientServer(t *testing.T) { bc = NewBackendClient(clogf, clientToServer) ch := make(chan Notify, 256) - h, err := NewHandle(bc, clogf, Options{ + notify := func(n Notify) { ch <- n } + h, err := NewHandle(bc, clogf, notify, Options{ Prefs: &Prefs{ ControlURL: "http://example.com/fake", }, - Notify: func(n Notify) { - ch <- n - }, }) if err != nil { t.Fatalf("NewHandle error: %v\n", err)