From 43f9c25fd2d39f6ac0d0b08701ba7a6dcc231874 Mon Sep 17 00:00:00 2001 From: Jordan Whited Date: Fri, 3 Jun 2022 10:52:07 -0700 Subject: [PATCH] cmd/tailscale: surface authentication errors in status.Health (#4748) Fixes #3713 Signed-off-by: Jordan Whited --- cmd/tailscale/cli/status.go | 14 ++++++++------ cmd/tailscale/cli/up.go | 14 ++++++++++++-- control/controlclient/auto.go | 4 ++++ health/health.go | 15 ++++++++++++++- ipn/ipnlocal/local.go | 4 +++- 5 files changed, 41 insertions(+), 10 deletions(-) diff --git a/cmd/tailscale/cli/status.go b/cmd/tailscale/cli/status.go index af65ea71f..e0f05d195 100644 --- a/cmd/tailscale/cli/status.go +++ b/cmd/tailscale/cli/status.go @@ -128,12 +128,8 @@ func runStatus(ctx context.Context, args []string) error { return err } - description, ok := isRunningOrStarting(st) - if !ok { - outln(description) - os.Exit(1) - } - + // print health check information prior to checking LocalBackend state as + // it may provide an explanation to the user if we choose to exit early if len(st.Health) > 0 { printf("# Health check:\n") for _, m := range st.Health { @@ -142,6 +138,12 @@ func runStatus(ctx context.Context, args []string) error { outln() } + description, ok := isRunningOrStarting(st) + if !ok { + outln(description) + os.Exit(1) + } + var buf bytes.Buffer f := func(format string, a ...any) { fmt.Fprintf(&buf, format, a...) } printPS := func(ps *ipnstate.PeerStatus) { diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index a233745d7..8f9f93516 100644 --- a/cmd/tailscale/cli/up.go +++ b/cmd/tailscale/cli/up.go @@ -19,6 +19,7 @@ import ( "sort" "strings" "sync" + "time" shellquote "github.com/kballard/go-shellquote" "github.com/peterbourgon/ff/v3/ffcli" @@ -114,7 +115,7 @@ func newUpFlagSet(goos string, upArgs *upArgsT) *flag.FlagSet { case "windows": upf.BoolVar(&upArgs.forceDaemon, "unattended", false, "run in \"Unattended Mode\" where Tailscale keeps running even after the current GUI user logs out (Windows-only)") } - + upf.DurationVar(&upArgs.timeout, "timeout", 0, "maximum amount of time to wait for tailscaled to enter a Running state; default (0s) blocks forever") registerAcceptRiskFlag(upf) return upf } @@ -148,6 +149,7 @@ type upArgsT struct { hostname string opUser string json bool + timeout time.Duration } func (a upArgsT) getAuthKey() (string, error) { @@ -646,6 +648,12 @@ func runUp(ctx context.Context, args []string) error { // need to prioritize reads from 'running' if it's // readable; its send does happen before the pump mechanism // shuts down. (Issue 2333) + var timeoutCh <-chan time.Time + if upArgs.timeout > 0 { + timeoutTimer := time.NewTimer(upArgs.timeout) + defer timeoutTimer.Stop() + timeoutCh = timeoutTimer.C + } select { case <-running: return nil @@ -663,6 +671,8 @@ func runUp(ctx context.Context, args []string) error { default: } return err + case <-timeoutCh: + return errors.New(`timeout waiting for Tailscale service to enter a Running state; check health with "tailscale status"`) } } @@ -719,7 +729,7 @@ func addPrefFlagMapping(flagName string, prefNames ...string) { // correspond to an ipn.Pref. func preflessFlag(flagName string) bool { switch flagName { - case "auth-key", "force-reauth", "reset", "qr", "json", "accept-risk": + case "auth-key", "force-reauth", "reset", "qr", "json", "timeout", "accept-risk": return true } return false diff --git a/control/controlclient/auto.go b/control/controlclient/auto.go index 96b5808f8..995838ddb 100644 --- a/control/controlclient/auto.go +++ b/control/controlclient/auto.go @@ -289,6 +289,7 @@ func (c *Auto) authRoutine() { } if goal == nil { + health.SetAuthRoutineInError(nil) // Wait for user to Login or Logout. <-ctx.Done() c.logf("[v1] authRoutine: context done.") @@ -296,6 +297,7 @@ func (c *Auto) authRoutine() { } if !goal.wantLoggedIn { + health.SetAuthRoutineInError(nil) err := c.direct.TryLogout(ctx) goal.sendLogoutError(err) if err != nil { @@ -334,6 +336,7 @@ func (c *Auto) authRoutine() { f = "TryLogin" } if err != nil { + health.SetAuthRoutineInError(err) report(err, f) bo.BackOff(ctx, err) continue @@ -358,6 +361,7 @@ func (c *Auto) authRoutine() { } // success + health.SetAuthRoutineInError(nil) c.mu.Lock() c.loggedIn = true c.loginGoal = nil diff --git a/health/health.go b/health/health.go index aaafdefb2..ba5bed636 100644 --- a/health/health.go +++ b/health/health.go @@ -45,6 +45,7 @@ var ( anyInterfaceUp = true // until told otherwise udp4Unbound bool controlHealth []string + lastLoginErr error ) // Subsystem is the name of a subsystem whose health can be monitored. @@ -287,6 +288,15 @@ func SetUDP4Unbound(unbound bool) { selfCheckLocked() } +// SetAuthRoutineInError records the latest error encountered as a result of a +// login attempt. Providing a nil error indicates successful login, or that +// being logged in w/coordination is not currently desired. +func SetAuthRoutineInError(err error) { + mu.Lock() + defer mu.Unlock() + lastLoginErr = err +} + func timerSelfCheck() { mu.Lock() defer mu.Unlock() @@ -321,9 +331,12 @@ func overallErrorLocked() error { if !anyInterfaceUp { return errors.New("network down") } - if ipnState != "Running" || !ipnWantRunning { + if !ipnWantRunning { return fmt.Errorf("state=%v, wantRunning=%v", ipnState, ipnWantRunning) } + if lastLoginErr != nil { + return fmt.Errorf("not logged in, last login error=%v", lastLoginErr) + } now := time.Now() if !inMapPoll && (lastMapPollEndedAt.IsZero() || now.Sub(lastMapPollEndedAt) > 10*time.Second) { return errors.New("not in map poll") diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 6766fac98..e89ccad0a 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -2680,12 +2680,14 @@ func (b *LocalBackend) enterState(newState ipn.State) { b.maybePauseControlClientLocked() b.mu.Unlock() + // prefs may change irrespective of state; WantRunning should be explicitly + // set before potential early return even if the state is unchanged. + health.SetIPNState(newState.String(), prefs.WantRunning) if oldState == newState { return } b.logf("Switching ipn state %v -> %v (WantRunning=%v, nm=%v)", oldState, newState, prefs.WantRunning, netMap != nil) - health.SetIPNState(newState.String(), prefs.WantRunning) b.send(ipn.Notify{State: &newState}) switch newState {