control/controlclient: accept key if last seen on exist node is absent (#19402)

On some nodes (found via natlab), the existing nodes last seen could be
unset. For these cases, we would want to accept the key and write a last
seen. This was breaking the cached netmap natlab tests.

Updates #12639

Signed-off-by: Claus Lensbøl <claus@tailscale.com>

3 files changed, 20 insertions, 5 deletions

diff --git a/control/controlclient/map.go b/control/controlclient/map.go
index 84a7c2e8b..17c223fe3 100644
--- a/control/controlclient/map.go
+++ b/control/controlclient/map.go
@@ -459,8 +459,9 @@ func (ms *mapSession) removeUnwantedDiscoUpdates(resp *tailcfg.MapResponse, viaT
 		}
 
 		// Accept if:
-		// - lastSeen moved forward in time.
-		if existingLastSeen, ok := existingNode.LastSeen().GetOk(); ok &&
+		// - if we don't have a last seen to compare against on the existing node.
+		// - OR lastSeen moved forward in time.
+		if existingLastSeen, ok := existingNode.LastSeen().GetOk(); !ok ||
 			change.LastSeen.After(existingLastSeen) {
 			acceptedDiscoUpdates = append(acceptedDiscoUpdates, change)
 		}
diff --git a/control/controlclient/map_test.go b/control/controlclient/map_test.go
index 679c48c17..8cf0156d8 100644
--- a/control/controlclient/map_test.go
+++ b/control/controlclient/map_test.go
@@ -717,6 +717,15 @@ func TestUpdateDiscoForNode(t *testing.T) {
 			wantUpdate:      true,
 			wantKeyChanged:  false,
 		},
+		{
+			name:           "no_initial_last_seen",
+			initialOnline:  false,
+			updateDiscoKey: true,
+			updateOnline:   false,
+			updateLastSeen: time.Now(),
+			wantUpdate:     true,
+			wantKeyChanged: true,
+		},
 	}
 
 	for _, tt := range tests {
@@ -736,7 +745,9 @@ func TestUpdateDiscoForNode(t *testing.T) {
 					Key:      key.NewNode().Public(),
 					DiscoKey: oldKey.Public(),
 					Online:   &tt.initialOnline,
-					LastSeen: &tt.initialLastSeen,
+				}
+				if !tt.initialLastSeen.IsZero() {
+					node.LastSeen = &tt.initialLastSeen
 				}
 
 				if nm := ms.netmapForResponse(&tailcfg.MapResponse{
diff --git a/tstest/integration/nat/nat_test.go b/tstest/integration/nat/nat_test.go
index 1ba8d1629..8eca5742f 100644
--- a/tstest/integration/nat/nat_test.go
+++ b/tstest/integration/nat/nat_test.go
@@ -28,7 +28,6 @@ import (
 	"golang.org/x/mod/modfile"
 	"golang.org/x/sync/errgroup"
 	"tailscale.com/client/tailscale"
-	"tailscale.com/envknob"
 	"tailscale.com/ipn/ipnstate"
 	"tailscale.com/syncs"
 	"tailscale.com/tailcfg"
@@ -792,8 +791,12 @@ func TestEasyEasy(t *testing.T) {
 	nt.want(routeDirect)
 }
 
+// TestTwoEasyNoControlDiscoRotate tests a situation where two nodes have been
+// online and connected through control, but then loose control access and also
+// rotate keys. It is not a perfect proxy for a cached node, as the node will
+// still have a mapState and not use the backup method of inserting keys into
+// the engine directly.
 func TestTwoEasyNoControlDiscoRotate(t *testing.T) {
-	envknob.Setenv("TS_USE_CACHED_NETMAP", "1")
 	nt := newNatTest(t)
 	nt.runTailscaleConnectivityTest(easyNoControlDiscoRotate, easyNoControlDiscoRotate)
 	nt.want(routeDirect)