From 81cabf48ec1f0d306f7dcf0c8a58a6eae6594c76 Mon Sep 17 00:00:00 2001 From: Maisem Ali Date: Tue, 26 Oct 2021 10:19:35 -0700 Subject: [PATCH] control/controlclient,tailcfg: propagate registration errors to the frontend Signed-off-by: Maisem Ali --- control/controlclient/auto.go | 7 ++----- control/controlclient/direct.go | 3 +++ control/controlclient/status.go | 2 +- ipn/ipnlocal/local.go | 6 ++++-- ipn/ipnlocal/state_test.go | 4 +--- tailcfg/tailcfg.go | 4 ++++ tailcfg/tailcfg_clone.go | 1 + tstest/integration/integration_test.go | 4 ++-- 8 files changed, 18 insertions(+), 13 deletions(-) diff --git a/control/controlclient/auto.go b/control/controlclient/auto.go index ad117e08a..031be8434 100644 --- a/control/controlclient/auto.go +++ b/control/controlclient/auto.go @@ -281,7 +281,6 @@ func (c *Auto) authRoutine() { report := func(err error, msg string) { c.logf("[v1] %s: %v", msg, err) - err = fmt.Errorf("%s: %v", msg, err) // don't send status updates for context errors, // since context cancelation is always on purpose. if ctx.Err() == nil { @@ -431,7 +430,7 @@ func (c *Auto) mapRoutine() { report := func(err error, msg string) { c.logf("[v1] %s: %v", msg, err) - err = fmt.Errorf("%s: %v", msg, err) + err = fmt.Errorf("%s: %w", msg, err) // don't send status updates for context errors, // since context cancelation is always on purpose. if ctx.Err() == nil { @@ -599,9 +598,7 @@ func (c *Auto) sendStatus(who string, err error, url string, nm *netmap.NetworkM NetMap: nm, Hostinfo: hi, State: state, - } - if err != nil { - new.Err = err.Error() + Err: err, } if statusFunc != nil { statusFunc(new) diff --git a/control/controlclient/direct.go b/control/controlclient/direct.go index 85c4163eb..fc5511999 100644 --- a/control/controlclient/direct.go +++ b/control/controlclient/direct.go @@ -435,6 +435,9 @@ func (c *Direct) doLogin(ctx context.Context, opt loginOpt) (mustRegen bool, new c.logf("RegisterReq: got response; nodeKeyExpired=%v, machineAuthorized=%v; authURL=%v", resp.NodeKeyExpired, resp.MachineAuthorized, resp.AuthURL != "") + if resp.Error != "" { + return false, "", errors.New(resp.Error) + } if resp.NodeKeyExpired { if regen { return true, "", fmt.Errorf("weird: regen=true but server says NodeKeyExpired: %v", request.NodeKey) diff --git a/control/controlclient/status.go b/control/controlclient/status.go index 3cbe9261d..3c137ac98 100644 --- a/control/controlclient/status.go +++ b/control/controlclient/status.go @@ -67,7 +67,7 @@ type Status struct { _ structs.Incomparable LoginFinished *empty.Message // nonempty when login finishes LogoutFinished *empty.Message // nonempty when logout finishes - Err string + Err error URL string // interactive URL to visit to finish logging in NetMap *netmap.NetworkMap // server-pushed configuration diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 02affa3e2..0fdc34eeb 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -448,12 +448,14 @@ func (b *LocalBackend) SetDecompressor(fn func() (controlclient.Decompressor, er // Among other things, this is where we update the netmap, packet filters, DNS and DERP maps. func (b *LocalBackend) setClientStatus(st controlclient.Status) { // The following do not depend on any data for which we need to lock b. - if st.Err != "" { + if st.Err != nil { // TODO(crawshaw): display in the UI. - if st.Err == "EOF" { + if errors.Is(st.Err, io.EOF) { b.logf("[v1] Received error: EOF") } else { b.logf("Received error: %v", st.Err) + e := st.Err.Error() + b.send(ipn.Notify{ErrMessage: &e}) } return } diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index a8261d302..791caf0cc 100644 --- a/ipn/ipnlocal/state_test.go +++ b/ipn/ipnlocal/state_test.go @@ -137,9 +137,7 @@ func (cc *mockControl) send(err error, url string, loginFinished bool, nm *netma URL: url, NetMap: nm, Persist: &cc.persist, - } - if err != nil { - s.Err = err.Error() + Err: err, } if loginFinished { s.LoginFinished = &empty.Message{} diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go index d5e0a440c..2e1eae183 100644 --- a/tailcfg/tailcfg.go +++ b/tailcfg/tailcfg.go @@ -679,6 +679,10 @@ type RegisterResponse struct { NodeKeyExpired bool // if true, the NodeKey needs to be replaced MachineAuthorized bool // TODO(crawshaw): move to using MachineStatus AuthURL string // if set, authorization pending + + // Error indiciates that authorization failed. If this is non-empty, + // other status fields should be ignored. + Error string } // EndpointType distinguishes different sources of MapRequest.Endpoint values. diff --git a/tailcfg/tailcfg_clone.go b/tailcfg/tailcfg_clone.go index a9d179766..7f5237863 100644 --- a/tailcfg/tailcfg_clone.go +++ b/tailcfg/tailcfg_clone.go @@ -243,6 +243,7 @@ var _RegisterResponseCloneNeedsRegeneration = RegisterResponse(struct { NodeKeyExpired bool MachineAuthorized bool AuthURL string + Error string }{}) // Clone makes a deep copy of DERPRegion. diff --git a/tstest/integration/integration_test.go b/tstest/integration/integration_test.go index bd41ef324..18a7e0aa5 100644 --- a/tstest/integration/integration_test.go +++ b/tstest/integration/integration_test.go @@ -696,8 +696,8 @@ func (n *testNode) MustUp(extraArgs ...string) { } args = append(args, extraArgs...) t.Logf("Running %v ...", args) - if err := n.Tailscale(args...).Run(); err != nil { - t.Fatalf("up: %v", err) + if b, err := n.Tailscale(args...).CombinedOutput(); err != nil { + t.Fatalf("up: %v, %v", string(b), err) } }