feature/conn25: stop adding multiple entries for same domain+dst

We should only add one entry to our magic ips for each domain+dst and
look up any existing entry instead of always creating a new one.

Fixes tailscale/corp#34252
Signed-off-by: Fran Bull <fran@tailscale.com>

feature/conn25/conn25.go

@@ -12,7 +12,6 @@ import (
 	"errors"
 	"net/http"
 	"net/netip"
-	"strings"
 	"sync"
 
 	"go4.org/netipx"
@@ -310,8 +309,8 @@ const AppConnectorsExperimentalAttrName = "tailscale.com/app-connectors-experime
 type config struct {
 	isConfigured      bool
 	apps              []appctype.Conn25Attr
-	appsByDomain      map[string][]string
+	appsByDomain      map[dnsname.FQDN][]string
-	selfRoutedDomains set.Set[string]
+	selfRoutedDomains set.Set[dnsname.FQDN]
 }
 
 func configFromNodeView(n tailcfg.NodeView) (config, error) {
@@ -326,8 +325,8 @@ func configFromNodeView(n tailcfg.NodeView) (config, error) {
 	cfg := config{
 		isConfigured:      true,
 		apps:              apps,
-		appsByDomain:      map[string][]string{},
+		appsByDomain:      map[dnsname.FQDN][]string{},
-		selfRoutedDomains: set.Set[string]{},
+		selfRoutedDomains: set.Set[dnsname.FQDN]{},
 	}
 	for _, app := range apps {
 		selfMatchesTags := false
@@ -342,10 +341,9 @@ func configFromNodeView(n tailcfg.NodeView) (config, error) {
 			if err != nil {
 				return config{}, err
 			}
-			key := fqdn.WithTrailingDot()
+			mak.Set(&cfg.appsByDomain, fqdn, append(cfg.appsByDomain[fqdn], app.Name))
-			mak.Set(&cfg.appsByDomain, key, append(cfg.appsByDomain[key], app.Name))
 			if selfMatchesTags {
-				cfg.selfRoutedDomains.Add(key)
+				cfg.selfRoutedDomains.Add(fqdn)
 			}
 		}
 	}
@@ -362,9 +360,8 @@ type client struct {
 	mu            sync.Mutex // protects the fields below
 	magicIPPool   *ippool
 	transitIPPool *ippool
-	// map of magic IP -> (transit IP, app)
+	assignments   addrAssignments
-	magicIPs map[netip.Addr]appAddr
+	config        config
-	config   config
 }
 
 func (c *client) isConfigured() bool {
@@ -407,13 +404,7 @@ func (c *client) reconfig(newCfg config) error {
 	return nil
 }
 
-func (c *client) setMagicIP(magicAddr, transitAddr netip.Addr, app string) {
+func (c *client) isConnectorDomain(domain dnsname.FQDN) bool {
-	c.mu.Lock()
-	defer c.mu.Unlock()
-	mak.Set(&c.magicIPs, magicAddr, appAddr{addr: transitAddr, app: app})
-}
-
-func (c *client) isConnectorDomain(domain string) bool {
 	c.mu.Lock()
 	defer c.mu.Unlock()
 	appNames, ok := c.config.appsByDomain[domain]
@@ -424,9 +415,12 @@ func (c *client) isConnectorDomain(domain string) bool {
 // for this domain+dst address, so that this client can use conn25 connectors.
 // It checks that this domain should be routed and that this client is not itself a connector for the domain
 // and generally if it is valid to make the assignment.
-func (c *client) reserveAddresses(domain string, dst netip.Addr) (addrs, error) {
+func (c *client) reserveAddresses(domain dnsname.FQDN, dst netip.Addr) (addrs, error) {
 	c.mu.Lock()
 	defer c.mu.Unlock()
+	if existing, ok := c.assignments.lookupByDomainDst(domain, dst); ok {
+		return existing, nil
+	}
 	appNames, _ := c.config.appsByDomain[domain]
 	// only reserve for first app
 	app := appNames[0]
@@ -438,17 +432,20 @@ func (c *client) reserveAddresses(domain string, dst netip.Addr) (addrs, error)
 	if err != nil {
 		return addrs{}, err
 	}
-	addrs := addrs{
+	as := addrs{
 		dst:     dst,
 		magic:   mip,
 		transit: tip,
 		app:     app,
+		domain:  domain,
 	}
-	return addrs, nil
+	if err := c.assignments.insert(as); err != nil {
+		return addrs{}, err
+	}
+	return as, nil
 }
 
 func (c *client) enqueueAddressAssignment(addrs addrs) {
-	c.setMagicIP(addrs.magic, addrs.transit, addrs.app)
 	// TODO(fran) 2026-02-03 asynchronously send peerapi req to connector to
 	// allocate these addresses for us.
 }
@@ -483,8 +480,12 @@ func (c *client) mapDNSResponse(buf []byte) []byte {
 
 		switch h.Type {
 		case dnsmessage.TypeA:
-			domain := strings.ToLower(h.Name.String())
+			domain, err := dnsname.ToFQDN(h.Name.String())
-			if len(domain) == 0 || !c.isConnectorDomain(domain) {
+			if err != nil {
+				c.logf("bad dnsname: %v", err)
+				return buf
+			}
+			if !c.isConnectorDomain(domain) {
 				if err := p.SkipAnswer(); err != nil {
 					c.logf("error parsing dns response: %v", err)
 					return buf
@@ -540,9 +541,44 @@ type addrs struct {
 	dst     netip.Addr
 	magic   netip.Addr
 	transit netip.Addr
+	domain  dnsname.FQDN
 	app     string
 }
 
 func (c addrs) isValid() bool {
 	return c.dst.IsValid()
 }
+
+// domainDst is a key for looking up an existing address assignment by the
+// DNS response domain and destination IP pair.
+type domainDst struct {
+	domain dnsname.FQDN
+	dst    netip.Addr
+}
+
+// addrAssignments is the collection of addrs assigned by this client
+// supporting lookup by magicip or domain+dst
+type addrAssignments struct {
+	byMagicIP   map[netip.Addr]addrs
+	byDomainDst map[domainDst]addrs
+}
+
+func (a *addrAssignments) insert(as addrs) error {
+	// we likely will want to allow overwriting in the future when we
+	// have address expiry, but for now this should not happen
+	if _, ok := a.byMagicIP[as.magic]; ok {
+		return errors.New("byMagicIP key exists")
+	}
+	ddst := domainDst{domain: as.domain, dst: as.dst}
+	if _, ok := a.byDomainDst[ddst]; ok {
+		return errors.New("byDomainDst key exists")
+	}
+	mak.Set(&a.byMagicIP, as.magic, as)
+	mak.Set(&a.byDomainDst, ddst, as)
+	return nil
+}
+
+func (a *addrAssignments) lookupByDomainDst(domain dnsname.FQDN, dst netip.Addr) (addrs, bool) {
+	v, ok := a.byDomainDst[domainDst{domain: domain, dst: dst}]
+	return v, ok
+}

feature/conn25/conn25_test.go

@@ -16,6 +16,8 @@ import (
 	"tailscale.com/tailcfg"
 	"tailscale.com/types/appctype"
 	"tailscale.com/types/logger"
+	"tailscale.com/util/dnsname"
+	"tailscale.com/util/must"
 	"tailscale.com/util/set"
 )
 
@@ -206,34 +208,16 @@ func TestTransitIPTargetUnknownTIP(t *testing.T) {
 	}
 }
 
-func TestSetMagicIP(t *testing.T) {
-	c := newConn25(logger.Discard)
-	mip := netip.MustParseAddr("0.0.0.1")
-	tip := netip.MustParseAddr("0.0.0.2")
-	app := "a"
-	c.client.setMagicIP(mip, tip, app)
-	val, ok := c.client.magicIPs[mip]
-	if !ok {
-		t.Fatal("expected there to be a value stored for the magic IP")
-	}
-	if val.addr != tip {
-		t.Fatalf("want %v, got %v", tip, val.addr)
-	}
-	if val.app != app {
-		t.Fatalf("want %s, got %s", app, val.app)
-	}
-}
-
 func TestReserveIPs(t *testing.T) {
 	c := newConn25(logger.Discard)
 	c.client.magicIPPool = newIPPool(mustIPSetFromPrefix("100.64.0.0/24"))
 	c.client.transitIPPool = newIPPool(mustIPSetFromPrefix("169.254.0.0/24"))
-	mbd := map[string][]string{}
+	mbd := map[dnsname.FQDN][]string{}
 	mbd["example.com."] = []string{"a"}
 	c.client.config.appsByDomain = mbd
 
 	dst := netip.MustParseAddr("0.0.0.1")
-	con, err := c.client.reserveAddresses("example.com.", dst)
+	addrs, err := c.client.reserveAddresses("example.com.", dst)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -242,18 +226,22 @@ func TestReserveIPs(t *testing.T) {
 	wantMagic := netip.MustParseAddr("100.64.0.0")    // first from magic pool
 	wantTransit := netip.MustParseAddr("169.254.0.0") // first from transit pool
 	wantApp := "a"                                    // the app name related to example.com.
+	wantDomain := must.Get(dnsname.ToFQDN("example.com."))
 
-	if wantDst != con.dst {
+	if wantDst != addrs.dst {
-		t.Errorf("want %v, got %v", wantDst, con.dst)
+		t.Errorf("want %v, got %v", wantDst, addrs.dst)
 	}
-	if wantMagic != con.magic {
+	if wantMagic != addrs.magic {
-		t.Errorf("want %v, got %v", wantMagic, con.magic)
+		t.Errorf("want %v, got %v", wantMagic, addrs.magic)
 	}
-	if wantTransit != con.transit {
+	if wantTransit != addrs.transit {
-		t.Errorf("want %v, got %v", wantTransit, con.transit)
+		t.Errorf("want %v, got %v", wantTransit, addrs.transit)
 	}
-	if wantApp != con.app {
+	if wantApp != addrs.app {
-		t.Errorf("want %s, got %s", wantApp, con.app)
+		t.Errorf("want %s, got %s", wantApp, addrs.app)
+	}
+	if wantDomain != addrs.domain {
+		t.Errorf("want %s, got %s", wantDomain, addrs.domain)
 	}
 }
 
@@ -287,8 +275,8 @@ func TestConfigReconfig(t *testing.T) {
 		cfg                   []appctype.Conn25Attr
 		tags                  []string
 		wantErr               bool
-		wantAppsByDomain      map[string][]string
+		wantAppsByDomain      map[dnsname.FQDN][]string
-		wantSelfRoutedDomains set.Set[string]
+		wantSelfRoutedDomains set.Set[dnsname.FQDN]
 	}{
 		{
 			name:    "bad-config",
@@ -302,11 +290,11 @@ func TestConfigReconfig(t *testing.T) {
 				{Name: "two", Domains: []string{"b.example.com"}, Connectors: []string{"tag:two"}},
 			},
 			tags: []string{"tag:one"},
-			wantAppsByDomain: map[string][]string{
+			wantAppsByDomain: map[dnsname.FQDN][]string{
 				"a.example.com.": {"one"},
 				"b.example.com.": {"two"},
 			},
-			wantSelfRoutedDomains: set.SetOf([]string{"a.example.com."}),
+			wantSelfRoutedDomains: set.SetOf([]dnsname.FQDN{"a.example.com."}),
 		},
 		{
 			name: "more-complex",
@@ -317,7 +305,7 @@ func TestConfigReconfig(t *testing.T) {
 				{Name: "four", Domains: []string{"4.b.example.com", "4.d.example.com"}, Connectors: []string{"tag:four"}},
 			},
 			tags: []string{"tag:onea", "tag:four", "tag:unrelated"},
-			wantAppsByDomain: map[string][]string{
+			wantAppsByDomain: map[dnsname.FQDN][]string{
 				"1.a.example.com.": {"one"},
 				"1.b.example.com.": {"one", "three"},
 				"1.c.example.com.": {"three"},
@@ -326,7 +314,7 @@ func TestConfigReconfig(t *testing.T) {
 				"4.b.example.com.": {"four"},
 				"4.d.example.com.": {"four"},
 			},
-			wantSelfRoutedDomains: set.SetOf([]string{"1.a.example.com.", "1.b.example.com.", "4.b.example.com.", "4.d.example.com."}),
+			wantSelfRoutedDomains: set.SetOf([]dnsname.FQDN{"1.a.example.com.", "1.b.example.com.", "4.b.example.com.", "4.d.example.com."}),
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
@@ -431,18 +419,24 @@ func TestMapDNSResponse(t *testing.T) {
 	}
 
 	for _, tt := range []struct {
-		name         string
+		name          string
-		domain       string
+		domain        string
-		addrs        []dnsmessage.AResource
+		addrs         []dnsmessage.AResource
-		wantMagicIPs map[netip.Addr]appAddr
+		wantByMagicIP map[netip.Addr]addrs
 	}{
 		{
 			name:   "one-ip-matches",
 			domain: "example.com.",
 			addrs:  []dnsmessage.AResource{{A: [4]byte{1, 0, 0, 0}}},
 			// these are 'expected' because they are the beginning of the provided pools
-			wantMagicIPs: map[netip.Addr]appAddr{
+			wantByMagicIP: map[netip.Addr]addrs{
-				netip.MustParseAddr("100.64.0.0"): {app: "app1", addr: netip.MustParseAddr("100.64.0.40")},
+				netip.MustParseAddr("100.64.0.0"): {
+					domain:  "example.com.",
+					dst:     netip.MustParseAddr("1.0.0.0"),
+					magic:   netip.MustParseAddr("100.64.0.0"),
+					transit: netip.MustParseAddr("100.64.0.40"),
+					app:     "app1",
+				},
 			},
 		},
 		{
@@ -452,9 +446,21 @@ func TestMapDNSResponse(t *testing.T) {
 				{A: [4]byte{1, 0, 0, 0}},
 				{A: [4]byte{2, 0, 0, 0}},
 			},
-			wantMagicIPs: map[netip.Addr]appAddr{
+			wantByMagicIP: map[netip.Addr]addrs{
-				netip.MustParseAddr("100.64.0.0"): {app: "app1", addr: netip.MustParseAddr("100.64.0.40")},
+				netip.MustParseAddr("100.64.0.0"): {
-				netip.MustParseAddr("100.64.0.1"): {app: "app1", addr: netip.MustParseAddr("100.64.0.41")},
+					domain:  "example.com.",
+					dst:     netip.MustParseAddr("1.0.0.0"),
+					magic:   netip.MustParseAddr("100.64.0.0"),
+					transit: netip.MustParseAddr("100.64.0.40"),
+					app:     "app1",
+				},
+				netip.MustParseAddr("100.64.0.1"): {
+					domain:  "example.com.",
+					dst:     netip.MustParseAddr("2.0.0.0"),
+					magic:   netip.MustParseAddr("100.64.0.1"),
+					transit: netip.MustParseAddr("100.64.0.41"),
+					app:     "app1",
+				},
 			},
 		},
 		{
@@ -482,9 +488,37 @@ func TestMapDNSResponse(t *testing.T) {
 			if !reflect.DeepEqual(dnsResp, bs) {
 				t.Fatal("shouldn't be changing the bytes (yet)")
 			}
-			if diff := cmp.Diff(tt.wantMagicIPs, c.client.magicIPs, cmpopts.EquateComparable(appAddr{}, netip.Addr{})); diff != "" {
+			if diff := cmp.Diff(tt.wantByMagicIP, c.client.assignments.byMagicIP, cmpopts.EquateComparable(addrs{}, netip.Addr{})); diff != "" {
-				t.Errorf("magicIPs diff (-want, +got):\n%s", diff)
+				t.Errorf("byMagicIP diff (-want, +got):\n%s", diff)
 			}
 		})
 	}
 }
+
+func TestReserveAddressesDeduplicated(t *testing.T) {
+	c := newConn25(logger.Discard)
+	c.client.magicIPPool = newIPPool(mustIPSetFromPrefix("100.64.0.0/24"))
+	c.client.transitIPPool = newIPPool(mustIPSetFromPrefix("169.254.0.0/24"))
+	c.client.config.appsByDomain = map[dnsname.FQDN][]string{"example.com.": {"a"}}
+
+	dst := netip.MustParseAddr("0.0.0.1")
+	first, err := c.client.reserveAddresses("example.com.", dst)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	second, err := c.client.reserveAddresses("example.com.", dst)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if first != second {
+		t.Errorf("expected same addrs on repeated call, got first=%v second=%v", first, second)
+	}
+	if got := len(c.client.assignments.byMagicIP); got != 1 {
+		t.Errorf("want 1 entry in byMagicIP, got %d", got)
+	}
+	if got := len(c.client.assignments.byDomainDst); got != 1 {
+		t.Errorf("want 1 entry in byDomainDst, got %d", got)
+	}
+}