control/controlclient,ipn/ipnlocal,tstest/integration: test node removal

Adds tests that only a 404 map response wraps ErrNodeRemoved, that the
node-removed warning and NeedsLogin state are cleared by a new netmap or
a profile switch, and that a node restarted after it was deleted shows
the warning and reaches NeedsLogin.

Updates #18700

Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>

control/controlclient/controlclient_test.go

@@ -595,6 +595,43 @@ func TestMapRequestRateLimited(t *testing.T) {
 	}
 }
 
+// TestMapRequestNodeRemoved verifies that only a 404 response to a map request
+// returns an error wrapping ErrNodeRemoved.
+func TestMapRequestNodeRemoved(t *testing.T) {
+	tests := []struct {
+		name        string
+		status      int
+		wantRemoved bool
+	}{
+		{name: "404", status: http.StatusNotFound, wantRemoved: true},
+		{name: "500", status: http.StatusInternalServerError, wantRemoved: false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			d := newDirectForTestControl(t, &testcontrol.Server{
+				MaybeRejectRequest: testcontrol.RejectRequestForPath("/machine/map", func() (int, string, string) {
+					return tt.status, "", "node not found"
+				}),
+			})
+
+			ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second)
+			defer cancel()
+
+			if _, err := d.TryLogin(ctx, LoginEphemeral); err != nil {
+				t.Fatalf("TryLogin: %v", err)
+			}
+
+			_, err := d.FetchNetMapForTest(ctx)
+			if err == nil {
+				t.Fatal("expected error on map request, got nil")
+			}
+			if got := errors.Is(err, ErrNodeRemoved); got != tt.wantRemoved {
+				t.Fatalf("errors.Is(%v, ErrNodeRemoved) = %v, want %v", err, got, tt.wantRemoved)
+			}
+		})
+	}
+}
+
 func connectProxyTo(t testing.TB, target, backendAddrPort string, reqs *atomic.Int64) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if r.RequestURI != target {

ipn/ipnlocal/state_test.go

@@ -1259,6 +1259,8 @@ func TestEngineReconfigOnStateChange(t *testing.T) {
 		wantPeers     []key.NodePublic
 		wantRouterCfg *router.Config
 		wantDNSCfg    *dns.Config
+
+		wantNodeRemoved bool // whether the node-removed warning is set
 	}{
 		{
 			name: "Initial",
@@ -1545,6 +1547,63 @@ func TestEngineReconfigOnStateChange(t *testing.T) {
 			wantRouterCfg: &router.Config{},
 			wantDNSCfg:    &dns.Config{},
 		},
+		{
+			name: "Start/Connect/Login/NodeRemoved",
+			steps: func(t *testing.T, lb *LocalBackend, cc func() *mockControl) {
+				mustDo(t)(lb.Start(ipn.Options{}))
+				mustDo2(t)(lb.EditPrefs(connect))
+				cc().authenticated(node1)
+				cc().send(sendOpt{err: fmt.Errorf("%w: initial fetch failed 404", controlclient.ErrNodeRemoved)})
+			},
+			// If control deletes the node, we want to disconnect:
+			wantState:       ipn.NeedsLogin,
+			wantCfg:         &wgcfg.Config{},
+			wantRouterCfg:   &router.Config{},
+			wantDNSCfg:      &dns.Config{},
+			wantNodeRemoved: true,
+		},
+		{
+			name: "Start/Connect/Login/NodeRemoved/NewProfile",
+			steps: func(t *testing.T, lb *LocalBackend, cc func() *mockControl) {
+				mustDo(t)(lb.Start(ipn.Options{}))
+				mustDo2(t)(lb.EditPrefs(connect))
+				cc().authenticated(node1)
+				cc().send(sendOpt{err: fmt.Errorf("%w: initial fetch failed 404", controlclient.ErrNodeRemoved)})
+				mustDo(t)(lb.NewProfile())
+			},
+			// The new profile is a different node, so the warning is cleared:
+			wantState:     ipn.NeedsLogin,
+			wantCfg:       &wgcfg.Config{},
+			wantRouterCfg: &router.Config{},
+			wantDNSCfg:    &dns.Config{},
+		},
+		{
+			name: "Start/Connect/Login/NodeRemoved/Login",
+			steps: func(t *testing.T, lb *LocalBackend, cc func() *mockControl) {
+				mustDo(t)(lb.Start(ipn.Options{}))
+				mustDo2(t)(lb.EditPrefs(connect))
+				cc().authenticated(node1)
+				cc().send(sendOpt{err: fmt.Errorf("%w: initial fetch failed 404", controlclient.ErrNodeRemoved)})
+				cc().authenticated(node1)
+			},
+			// A new netmap means the node is known to control again:
+			wantState: ipn.Starting,
+			wantCfg: &wgcfg.Config{
+				Addresses: node1.SelfNode.Addresses().AsSlice(),
+			},
+			wantRouterCfg: &router.Config{
+				SNATSubnetRoutes: true,
+				NetfilterMode:    preftype.NetfilterOn,
+				LocalAddrs:       node1.SelfNode.Addresses().AsSlice(),
+				Routes:           routesWithQuad100(),
+			},
+			wantDNSCfg: &dns.Config{
+				AcceptDNS:             true,
+				Routes:                map[dnsname.FQDN][]*dnstype.Resolver{},
+				Hosts:                 map[dnsname.FQDN][]netip.Addr{},
+				MagicDNSHostsUnrouted: true,
+			},
+		},
 	}
 
 	for _, tt := range tests {
@@ -1559,6 +1618,9 @@ func TestEngineReconfigOnStateChange(t *testing.T) {
 			if gotState := lb.State(); gotState != tt.wantState {
 				t.Errorf("State: got %v; want %v", gotState, tt.wantState)
 			}
+			if got := lb.HealthTracker().IsUnhealthy(nodeRemovedWarnable); got != tt.wantNodeRemoved {
+				t.Errorf("node-removed warning set = %v; want %v", got, tt.wantNodeRemoved)
+			}
 
 			// Peers are not part of wgcfg.Config; the engine learns
 			// them from the config source installed by LocalBackend

tstest/integration/integration_test.go

@@ -46,6 +46,7 @@ import (
 	"tailscale.com/net/udprelay/status"
 	"tailscale.com/tailcfg"
 	"tailscale.com/tailcfg/nodecap"
+	"tailscale.com/tsconst"
 	"tailscale.com/tstest"
 	"tailscale.com/tstest/integration/testcontrol"
 	"tailscale.com/types/key"
@@ -1316,6 +1317,56 @@ func TestNoControlConnWhenDown(t *testing.T) {
 	d2.MustCleanShutdown(t)
 }
 
+// TestNodeRemovedHealth tests that once control starts answering map requests
+// with a 404, as it does after the node has been deleted, the node-removed
+// health warning shows up on the IPN bus.
+func TestNodeRemovedHealth(t *testing.T) {
+	tstest.Parallel(t)
+	var removed atomic.Bool
+	env := NewTestEnv(t, ConfigureControl(func(control *testcontrol.Server) {
+		control.MaybeRejectRequest = testcontrol.RejectRequestForPath("/machine/map", func() (int, string, string) {
+			if removed.Load() {
+				return http.StatusNotFound, "", "node not found"
+			}
+			return 0, "", ""
+		})
+	}))
+	n1 := NewTestNode(t, env)
+
+	d1 := n1.StartDaemon()
+	n1.AwaitResponding()
+	n1.MustUp()
+	n1.AwaitRunning()
+
+	// Restart so that the node starts a new map poll after the removal.
+	removed.Store(true)
+	d1.MustCleanShutdown(t)
+	d2 := n1.StartDaemon()
+	n1.AwaitResponding()
+
+	ctx, cancel := context.WithTimeout(t.Context(), 30*time.Second)
+	defer cancel()
+	w, err := n1.LocalClient().WatchIPNBus(ctx, ipn.NotifyInitialHealthState)
+	if err != nil {
+		t.Fatalf("WatchIPNBus: %v", err)
+	}
+	defer w.Close()
+	for {
+		n, err := w.Next()
+		if err != nil {
+			t.Fatalf("waiting for %q health warning: %v", tsconst.HealthWarnableNodeRemoved, err)
+		}
+		if n.Health != nil {
+			if _, ok := n.Health.Warnings[tsconst.HealthWarnableNodeRemoved]; ok {
+				break
+			}
+		}
+	}
+	n1.AwaitNeedsLogin()
+
+	d2.MustCleanShutdown(t)
+}
+
 // Issue 2137: make sure Windows tailscaled works with the CLI alone,
 // without the GUI to kick off a Start.
 func TestOneNodeUpWindowsStyle(t *testing.T) {