From 80dc7a8d07d3304cfa93859693385992843d670c Mon Sep 17 00:00:00 2001
From: Fran Bull <fran@tailscale.com>
Date: Wed, 20 May 2026 11:13:34 -0700
Subject: [PATCH] feature/conn25: disallow addrs assignment overwriting.

We don't want addr assignments to be lost from the collection before
they can be returned to the IP pools, otherwise we will get orphan
addresses marked inUse in the pools that will never be returned.

Fixes tailscale/corp#39975

Signed-off-by: Fran Bull <fran@tailscale.com>
---
 feature/conn25/addrAssignments.go      | 20 +++++++-------------
 feature/conn25/addrAssignments_test.go | 16 +++-------------
 2 files changed, 10 insertions(+), 26 deletions(-)

diff --git a/feature/conn25/addrAssignments.go b/feature/conn25/addrAssignments.go
index 318cdd5a7..a6c637128 100644
--- a/feature/conn25/addrAssignments.go
+++ b/feature/conn25/addrAssignments.go
@@ -58,22 +58,16 @@ func (a *addrAssignments) insertWithExpiry(as *addrs, d time.Duration) error {
 	if !as.expiresAt.IsZero() && !as.expiresAt.Before(now) {
 		return errors.New("expiresAt already set")
 	}
-	// we don't expect for addresses to be reused before expiry
-	if existing, ok := a.byMagicIP[as.magic]; ok {
-		if !existing.expiresAt.Before(now) {
-			return errors.New("byMagicIP key exists")
-		}
+	// addresses must be removed (eg by popExpired) before they can be reused
+	if _, ok := a.byMagicIP[as.magic]; ok {
+		return errors.New("byMagicIP key exists")
 	}
 	ddst := domainDst{domain: as.domain, dst: as.dst}
-	if existing, ok := a.byDomainDst[ddst]; ok {
-		if !existing.expiresAt.Before(now) {
-			return errors.New("byDomainDst key exists")
-		}
+	if _, ok := a.byDomainDst[ddst]; ok {
+		return errors.New("byDomainDst key exists")
 	}
-	if existing, ok := a.byTransitIP[as.transit]; ok {
-		if !existing.expiresAt.Before(now) {
-			return errors.New("byTransitIP key exists")
-		}
+	if _, ok := a.byTransitIP[as.transit]; ok {
+		return errors.New("byTransitIP key exists")
 	}
 	as.expiresAt = now.Add(d)
 	mak.Set(&a.byMagicIP, as.magic, as)
diff --git a/feature/conn25/addrAssignments_test.go b/feature/conn25/addrAssignments_test.go
index 262354f55..4811564b1 100644
--- a/feature/conn25/addrAssignments_test.go
+++ b/feature/conn25/addrAssignments_test.go
@@ -51,20 +51,10 @@ func TestAssignmentsExpire(t *testing.T) {
 	if foundAsAfter.isValid() {
 		t.Fatal("expected zero val")
 	}
-	// Now we can reuse the addresses
+	// We should only be able to write old addresses again if they've been removed from the maps (eg with popExpired).
 	err = assignments.insert(as)
-	if err != nil {
-		t.Fatal(err)
-	}
-	foundAs, ok = assignments.lookupByMagicIP(as.magic)
-	if !ok {
-		t.Fatal("expected to find")
-	}
-	if foundAs.dst != as.dst {
-		t.Fatalf("want %v; got %v", as.dst, foundAs.dst)
-	}
-	if !foundAs.expiresAt.After(clock.Now()) {
-		t.Fatalf("expected foundAs to expire after now")
+	if err == nil {
+		t.Fatal("expected an error but got nil")
 	}
 }