From 18a95394df769a66f1c3a915bc3fa597a554531f Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Sun, 12 Jul 2026 19:13:44 +0000 Subject: [PATCH] ipn/ipnlocal: shut down old control client before starting new one LocalBackend.Start previously shut down the previous control client in a goroutine, letting it run concurrently with the new one. An in-flight lite map update carrying stale Hostinfo.RequestTags could then be processed by the control plane after the new client had already changed the node's tags. Control treats such a request as an invalid tag transition and expires the node key to force a reauth, so retagging a node with "tailscale up --advertise-tags" intermittently logged the machine out. Instead, detach the old client under b.mu and shut it down synchronously with the lock released, before creating the new client. Shutdown cancels the old client's in-flight requests and waits for its goroutines to exit, so the cancellation of any stale update reaches the server before the new client sends its first request. Per the deadlock history in #18052, Shutdown must not be called with b.mu held; this uses the same pattern as DisconnectControl. Also teach the testcontrol server to model the control plane's tag transition handling (including expiring the node key on an invalid transition and ignoring updates from canceled requests), add an integration test reproducing the race, and add an ipnlocal test verifying that Start waits for the old client to shut down. Updates #20365 Updates #18052 Signed-off-by: Brad Fitzpatrick Change-Id: If8c8e145bdadcef1b1b8fe6209453cf5f5a8d616 --- ipn/ipnlocal/local.go | 37 ++++- ipn/ipnlocal/state_test.go | 33 +++++ tstest/integration/integration_test.go | 132 ++++++++++++++++++ tstest/integration/testcontrol/testcontrol.go | 112 +++++++++++++++ 4 files changed, 311 insertions(+), 3 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 53e98e33e..798e43499 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -1150,14 +1150,28 @@ func (b *LocalBackend) shouldPauseControlClientLocked(prefs ipn.PrefsView) bool return false } -// DisconnectControl shuts down control client. This can be run before node shutdown to force control to consider this ndoe -// inactive. This can be used to ensure that nodes that are HA subnet router or app connector replicas are shutting -// down, clients switch over to other replicas whilst the existing connections are kept alive for some period of time. +// DisconnectControl shuts down the control client. This can be run before +// node shutdown to force control to consider this node inactive. This can +// be used to ensure that nodes that are HA subnet router or app connector +// replicas are shutting down, clients switch over to other replicas whilst +// the existing connections are kept alive for some period of time. +// +// Shutdown of the detached client is synchronous: it cancels the client's +// in-flight requests and waits for its goroutines to exit, but it does not +// wait for pending updates to be delivered. func (b *LocalBackend) DisconnectControl() { b.mu.Lock() cc := b.resetControlClientLocked() b.mu.Unlock() + // The Shutdown call must not run while b.mu is held, per the deadlock + // history in tailscale/tailscale#18052: controlclient.Auto's + // goroutines deliver callbacks into LocalBackend through an execqueue + // whose RunSync holds the queue mutex while the callback acquires + // b.mu, and Auto.Shutdown acquires that same queue mutex, so calling + // it with b.mu held inverts the lock order. A previous attempt to + // shut the client down synchronously inside resetControlClientLocked + // with b.mu held (#18127) deadlocked and was reverted (#18149). if cc != nil { cc.Shutdown() } @@ -3004,6 +3018,23 @@ func (b *LocalBackend) controlDebugFlags() []string { func (b *LocalBackend) Start(opts ipn.Options) error { defer b.CheckDeadlocks()() + // Shut down the previous control client, if any, before starting a + // new one, so the old client can't race with the new one. Without + // this, an in-flight lite map update carrying stale Hostinfo (notably + // RequestTags) could be processed by the control plane after the new + // client's requests, which made retagging with "tailscale up + // --advertise-tags" intermittently look like an invalid tag + // transition and log the node out (tailscale/tailscale#20365). + // + // TODO(bradfitz,nickkhyl): this is still racy if Start is called + // concurrently: whichever call loses the race to reacquire b.mu + // below then detaches the winner's new control client in startLocked + // and shuts it down in a goroutine, without the ordering guarantee + // that this call provides. This is a workaround until #18052 is + // properly fixed and a control client can be shut down synchronously + // with b.mu held. + b.DisconnectControl() + b.mu.Lock() defer b.mu.Unlock() return b.startLocked(opts) diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index 7ce4f590c..e768c70c5 100644 --- a/ipn/ipnlocal/state_test.go +++ b/ipn/ipnlocal/state_test.go @@ -352,6 +352,39 @@ func (b *LocalBackend) nonInteractiveLoginForStateTest() { cc.Login(b.loginFlags | controlclient.LoginInteractive) } +// TestStartShutsDownPreviousControlClient verifies that Start waits for the +// previous control client to fully shut down before creating a new one. +// +// If the old client is still alive when the new one starts, its in-flight +// requests (carrying stale Hostinfo, notably RequestTags) can race with the +// new client's requests at the control plane. That made retagging a node +// with "tailscale up --advertise-tags" intermittently log the node out +// (tailscale/tailscale#20365): a stale RequestTags update processed after +// the tag transition looks like an invalid transition, so the control +// server expires the node key. +func TestStartShutsDownPreviousControlClient(t *testing.T) { + const enableLogging = true + var cc *mockControl + b := newLocalBackendWithTestControl(t, enableLogging, func(tb testing.TB, opts controlclient.Options) controlclient.Client { + if cc != nil { + select { + case <-cc.shutdown: + default: + t.Errorf("new control client created before the previous one was shut down") + } + } + cc = newClient(t, opts) + return cc + }) + + for i := range 3 { + t.Logf("Start %d", i+1) + if err := b.Start(ipn.Options{}); err != nil { + t.Fatalf("Start: %v", err) + } + } +} + // A very precise test of the sequence of function calls generated by // ipnlocal.Local into its controlclient instance, and the events it // produces upstream into the UI. diff --git a/tstest/integration/integration_test.go b/tstest/integration/integration_test.go index cabb50cc7..192ecdd12 100644 --- a/tstest/integration/integration_test.go +++ b/tstest/integration/integration_test.go @@ -532,6 +532,138 @@ func TestOneNodeUpAuth(t *testing.T) { } } +// TestRetagStaleMapRequestRace reproduces tailscale/tailscale#20365: a node +// tagged tag:tag1, where tag:tag1 owns tag:tag2, is retagged with "tailscale +// up --advertise-tags=tag:tag2". This should always succeed, but sometimes +// the machine is logged out instead. +// +// The cause is a race: "tailscale up" makes LocalBackend.Start shut down the +// old control client asynchronously while the new one starts, so a lite map +// update carrying the old Hostinfo.RequestTags can still be in flight when +// the new client's requests retag the node. If control processes the stale +// update after the tag transition, it looks like a request to change the +// node's tags from tag:tag2 back to tag:tag1. That fails the tag ownership +// check (tag:tag2 doesn't own tag:tag1), and control expires the node's key +// to force reauthentication, logging the machine out. +// +// The test recreates that interleaving deterministically: it triggers a lite +// map update carrying the old tags (any routine hostinfo change does that), +// holds it at the server, retags the node, and only then lets the held +// update be processed. +func TestRetagStaleMapRequestRace(t *testing.T) { + tstest.Parallel(t) + + var ( + holdStale atomic.Bool + staleHeld = make(chan struct{}, 1) + staleRelease = make(chan struct{}) + staleDone = make(chan struct{}, 1) + ) + env := NewTestEnv(t, ConfigureControl(func(control *testcontrol.Server) { + control.TagOwners = map[string][]string{ + "tag:tag1": nil, + "tag:tag2": {"tag:tag1"}, + } + control.HoldMapRequest = func(req *tailcfg.MapRequest) (done func()) { + if req.Stream || req.Hostinfo == nil || !holdStale.Load() { + return nil + } + if !slices.Equal(req.Hostinfo.RequestTags, []string{"tag:tag1"}) { + return nil + } + select { + case staleHeld <- struct{}{}: + default: + } + <-staleRelease + return func() { + select { + case staleDone <- struct{}{}: + default: + } + } + } + })) + + n1 := NewTestNode(t, env) + d1 := n1.StartDaemon() + defer d1.MustCleanShutdown(t) + n1.AwaitResponding() + n1.MustUp("--advertise-tags=tag:tag1") + n1.AwaitRunning() + + nodes := env.Control.AllNodes() + if len(nodes) != 1 { + t.Fatalf("expected 1 node, got %d", len(nodes)) + } + origKey := nodes[0].Key + if got, want := nodes[0].Tags, []string{"tag:tag1"}; !slices.Equal(got, want) { + t.Fatalf("node tags = %v; want %v", got, want) + } + + // Make the current control client send a lite map update carrying the + // old RequestTags, as any routine hostinfo change does. Control holds + // it (per HoldMapRequest above) so that it's still outstanding when + // the node is retagged below. Shutting down that control client + // cancels the request but can't unsend it; control still has it. + holdStale.Store(true) + if out, err := n1.Tailscale("set", "--hostname=retag-race-test").CombinedOutput(); err != nil { + t.Fatalf("tailscale set: %v, %s", err, out) + } + select { + case <-staleHeld: + case <-time.After(10 * time.Second): + t.Fatal("timed out waiting for the stale map update to reach control") + } + + // Retag the node while the stale update is still outstanding. This + // doesn't block on the held update: the new control client's requests + // are processed while it is held. + n1.MustUp("--advertise-tags=tag:tag2") + if err := tstest.WaitFor(10*time.Second, func() error { + n := env.Control.Node(origKey) + if n == nil { + return fmt.Errorf("node %v not found in control", origKey.ShortString()) + } + if !slices.Equal(n.Tags, []string{"tag:tag2"}) { + return fmt.Errorf("node tags = %v; want [tag:tag2]", n.Tags) + } + return nil + }); err != nil { + t.Fatal(err) + } + + // Let control process the stale update, now that the retag has been + // committed, and wait for it to finish. + close(staleRelease) + select { + case <-staleDone: + case <-time.After(10 * time.Second): + t.Fatal("timed out waiting for control to process the stale map update") + } + + // The retag should stick, and the machine should stay logged in with + // the same node key. With the bug present, control instead expires the + // node key when it processes the stale update. + n := env.Control.Node(origKey) + if n == nil { + t.Fatalf("node %v disappeared from control", origKey.ShortString()) + } + if !n.KeyExpiry.IsZero() { + t.Fatalf("control expired the node key after processing a stale map update; the machine was logged out (issue 20365)") + } + if got, want := n.Tags, []string{"tag:tag2"}; !slices.Equal(got, want) { + t.Fatalf("node tags = %v; want %v", got, want) + } + st := n1.MustStatus() + if st.BackendState != "Running" { + t.Errorf("BackendState = %q; want Running", st.BackendState) + } + if st.Self.PublicKey != origKey { + t.Errorf("node key changed from %v to %v; the machine was logged out and re-registered", origKey.ShortString(), st.Self.PublicKey.ShortString()) + } +} + // Returns true if the error returned by [exec.Run] fails with a non-zero // exit code, false otherwise. func isNonZeroExitCode(err error) bool { diff --git a/tstest/integration/testcontrol/testcontrol.go b/tstest/integration/testcontrol/testcontrol.go index 0ad335ae3..4545f23ec 100644 --- a/tstest/integration/testcontrol/testcontrol.go +++ b/tstest/integration/testcontrol/testcontrol.go @@ -70,6 +70,29 @@ type Server struct { // belong to the same user. AllNodesSameUser bool + // TagOwners, if non-nil, enables modeling of the production control + // server's tag transition handling. Map keys are tags (e.g. "tag:foo") + // and values are the tags whose nodes are allowed to assign the key's + // tag. A node registering with Hostinfo.RequestTags gets those tags + // (signup-time ownership checks are not modeled). A later non-streaming + // map request whose Hostinfo.RequestTags differ from both the node's + // stored Hostinfo's RequestTags and its current tags is treated as a + // tag transition request: if the node's current tags own each requested + // tag, the node is retagged; otherwise its node key is expired to force + // reauthentication, as the production control server does. + // + // If nil, RequestTags in map requests are ignored. + TagOwners map[string][]string + + // HoldMapRequest, if non-nil, is called with each incoming MapRequest + // before the server starts processing it. It may block to delay + // processing, letting tests control the order in which concurrent map + // requests are handled. If it returns a non-nil func, the server calls + // it when it finishes handling the request. For streaming requests + // that is when the poll ends, so returning a done func is mostly + // useful for non-streaming requests. + HoldMapRequest func(*tailcfg.MapRequest) (done func()) + // AllOnline, if true, marks every peer entry in MapResponses as // Online=true. This is a coarse stand-in for the per-node // online/offline tracking that production control servers do based @@ -1027,6 +1050,11 @@ func (s *Server) serveRegister(w http.ResponseWriter, r *http.Request, mkey key. CapMap: capMap, Capabilities: slices.Collect(maps.Keys(capMap)), } + if s.TagOwners != nil && req.Hostinfo != nil { + // Trust the requested tags at signup; ownership checks + // against the registering user are not modeled. + node.Tags = slices.Clone(req.Hostinfo.RequestTags) + } if s.MagicDNSDomain != "" { node.Name = node.Name + "." + s.MagicDNSDomain + "." } @@ -1258,6 +1286,71 @@ func (s *Server) incrInServeMap(delta int) { s.inServeMap += delta } +// handleTagTransitionLocked models the production control server's handling +// of tag changes requested via Hostinfo.RequestTags in non-streaming map +// requests (see updateTags in the control server). A request whose +// RequestTags differ from both the node's stored Hostinfo's RequestTags and +// the node's current tags is a tag transition request. If the transition is +// permitted by s.TagOwners, the node is retagged; otherwise its node key is +// expired to force reauthentication. +// +// hi is the Hostinfo from the incoming request; node.Hostinfo is the +// previously stored one. s.mu must be held. +func (s *Server) handleTagTransitionLocked(node *tailcfg.Node, hi tailcfg.HostinfoView) { + var oldReqTags []string + if node.Hostinfo.Valid() { + oldReqTags = node.Hostinfo.RequestTags().AsSlice() + } + newReqTags := hi.RequestTags().AsSlice() + if tagsEqualAnyOrder(oldReqTags, newReqTags) || tagsEqualAnyOrder(newReqTags, node.Tags) { + return + } + if s.validTagTransition(node.Tags, newReqTags) { + s.logf("testcontrol: retagging node %v: %v -> %v", node.ID, node.Tags, newReqTags) + node.Tags = newReqTags + } else { + s.logf("testcontrol: invalid tag transition %v -> %v for node %v; expiring its node key", node.Tags, newReqTags, node.ID) + node.KeyExpiry = time.Now().Add(-time.Minute) + } +} + +// validTagTransition reports whether a node currently tagged cur may retag +// itself as want, per s.TagOwners: every requested tag must be owned by one +// of the node's current tags. An untagged node may claim any defined tag, +// standing in for the production server's checks against the requesting +// user, which this server doesn't model. Removing all tags is not allowed, +// matching the production server. +func (s *Server) validTagTransition(cur, want []string) bool { + if len(want) == 0 { + return len(cur) == 0 + } + for _, tag := range want { + owners, ok := s.TagOwners[tag] + if !ok { + return false + } + if len(cur) == 0 { + continue + } + owned := slices.ContainsFunc(cur, func(c string) bool { return slices.Contains(owners, c) }) + if !owned { + return false + } + } + return true +} + +// tagsEqualAnyOrder reports whether a and b contain the same tags, ignoring order. +func tagsEqualAnyOrder(a, b []string) bool { + if len(a) != len(b) { + return false + } + as, bs := slices.Clone(a), slices.Clone(b) + slices.Sort(as) + slices.Sort(bs) + return slices.Equal(as, bs) +} + // InServeMap returns the number of clients currently in a MapRequest HTTP handler. func (s *Server) InServeMap() int { s.mu.Lock() @@ -1289,6 +1382,12 @@ func (s *Server) serveMap(w http.ResponseWriter, r *http.Request, mkey key.Machi } s.mu.Unlock() + if s.HoldMapRequest != nil { + if done := s.HoldMapRequest(req); done != nil { + defer done() + } + } + if s.AltMapStream != nil { // The caller takes over the stream entirely; it must handle // keeping the HTTP response alive until ctx is done. @@ -1320,6 +1419,16 @@ func (s *Server) serveMap(w http.ResponseWriter, r *http.Request, mkey key.Machi streamingNonUpdate := req.Stream && req.Version >= 68 var peersToUpdate []tailcfg.NodeID if !req.ReadOnly && !streamingNonUpdate { + if ctx.Err() != nil { + // The client canceled the request (say, its control client + // was shut down mid-request when "tailscale up" or a + // profile switch created a new one), so its contents may + // predate newer requests that were already processed. + // Don't apply its Hostinfo/endpoints; they may be stale. + s.logf("testcontrol: dropping canceled map update from %v", req.NodeKey.ShortString()) + http.Error(w, "request canceled", 400) + return + } endpoints := filterInvalidIPv6Endpoints(req.Endpoints) var hi tailcfg.HostinfoView var newDERP int @@ -1339,6 +1448,9 @@ func (s *Server) serveMap(w http.ResponseWriter, r *http.Request, mkey key.Machi live.DiscoKey = req.DiscoKey live.Cap = req.Version if hi.Valid() { + if s.TagOwners != nil { + s.handleTagTransitionLocked(live, hi) + } live.Hostinfo = hi if newDERP != 0 { live.HomeDERP = newDERP