feature/conn25: keep addrAssignments through pool reconfig

Fixes tailscale/corp#40250

Signed-off-by: Fran Bull <fran@tailscale.com>

feature/conn25/conn25.go

@@ -707,10 +707,10 @@ func (c *client) reconfig() {
 
 	ipSets := c.getIPSets()
 
-	c.v4MagicIPPool = newIPPool(ipSets.v4Magic)
+	c.v4MagicIPPool = c.v4MagicIPPool.reconfig(ipSets.v4Magic)
-	c.v4TransitIPPool = newIPPool(ipSets.v4Transit)
+	c.v4TransitIPPool = c.v4TransitIPPool.reconfig(ipSets.v4Transit)
-	c.v6MagicIPPool = newIPPool(ipSets.v6Magic)
+	c.v6MagicIPPool = c.v6MagicIPPool.reconfig(ipSets.v6Magic)
-	c.v6TransitIPPool = newIPPool(ipSets.v6Transit)
+	c.v6TransitIPPool = c.v6TransitIPPool.reconfig(ipSets.v6Transit)
 }
 
 // getAppsForConnectorDomain returns the slice of app names which match the

feature/conn25/conn25_test.go

@@ -5,6 +5,7 @@ package conn25
 
 import (
 	"encoding/json"
+	"errors"
 	"net/http"
 	"net/http/httptest"
 	"net/netip"
@@ -2368,3 +2369,69 @@ func TestGetMagicRange(t *testing.T) {
 		}
 	}
 }
+
+func TestReconfigDoesNotReissueInUseAddresses(t *testing.T) {
+	appName := "app1"
+	mustRange := func(from, to string) netipx.IPRange {
+		return netipx.IPRangeFrom(netip.MustParseAddr(from), netip.MustParseAddr(to))
+	}
+	beforeRangeV4 := mustRange("0.0.0.1", "0.0.0.3")
+	beforeRangeV6 := mustRange("::1", "::3")
+	afterRangeV4 := mustRange("0.0.0.4", "0.0.0.7")
+	afterRangeV6 := mustRange("::4", "::7")
+	makeNodeFromMagicRange := func(v4, v6 netipx.IPRange) tailcfg.NodeView {
+		return makeSelfNode(t, []appctype.Conn25Attr{{
+			Name:            appName,
+			Connectors:      []string{"tag:woo"},
+			Domains:         []string{"example.com"},
+			V4MagicIPPool:   []netipx.IPRange{v4},
+			V6MagicIPPool:   []netipx.IPRange{v6},
+			V4TransitIPPool: []netipx.IPRange{mustRange("169.254.0.0", "169.254.0.10")},
+			V6TransitIPPool: []netipx.IPRange{mustRange("fd7a:115c:a1e0:a99c:0200::", "fd7a:115c:a1e0:a99c:0200::10")},
+		}}, []string{})
+	}
+	domain := must.Get(dnsname.ToFQDN("example.com."))
+
+	for _, tt := range []struct {
+		name   string
+		dstOne netip.Addr
+		dstTwo netip.Addr
+	}{
+		{
+			name:   "v4",
+			dstOne: netip.MustParseAddr("0.0.0.100"),
+			dstTwo: netip.MustParseAddr("0.0.0.101"),
+		},
+		{
+			name:   "v6",
+			dstOne: netip.MustParseAddr("::100"),
+			dstTwo: netip.MustParseAddr("::101"),
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			c := newConn25(t.Logf)
+			ext := &extension{
+				conn25: c,
+			}
+
+			_, err := c.client.reserveAddresses(appName, domain, tt.dstOne)
+			if !errors.Is(err, errUninitializedIPPool) {
+				t.Fatalf("want %v, got %v", errUninitializedIPPool, err)
+			}
+
+			ext.onSelfChange(makeNodeFromMagicRange(beforeRangeV4, beforeRangeV6))
+			beforeAddrs, err := c.client.reserveAddresses(appName, domain, tt.dstOne)
+			if err != nil {
+				t.Fatal(err)
+			}
+			ext.onSelfChange(makeNodeFromMagicRange(afterRangeV4, afterRangeV6))
+			afterAddrs, err := c.client.reserveAddresses(appName, domain, tt.dstTwo)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if afterAddrs.magic == beforeAddrs.magic {
+				t.Errorf("pool reissued magic: %v that was already assigned", beforeAddrs.magic)
+			}
+		})
+	}
+}

feature/conn25/ippool.go

@@ -20,6 +20,9 @@ var errNotOurAddress = errors.New("not our address")
 // errAddrExists is returned if a returned address is already in the returned pool.
 var errAddrExists = errors.New("address already returned")
 
+// errUninitializedIPPool is returned if the pool is used when it's not initialized
+var errUninitializedIPPool = errors.New("uninitialized ippool")
+
 // ipSetIterator allows for round robin iteration over all the addresses within a netipx.IPSet.
 // netipx.IPSet has a Ranges call that returns the "minimum and sorted set of IP ranges that covers [the set]".
 // netipx.IPRange is "an inclusive range of IP addresses from the same address family.". So we can iterate over
@@ -72,12 +75,23 @@ func newIPPool(ipset *netipx.IPSet) *ippool {
 }
 
 type ippool struct {
-	ipSet         *netipx.IPSet
+	// ipSet defines the addresses within the ippool, it is configured by the user.
+	ipSet *netipx.IPSet
+	// ipSetIterator keeps track of iteration through the ippool.
 	ipSetIterator *ipSetIterator
-	inUse         *set.Set[netip.Addr]
+	// inUse is a set of addresses that have been handed out and not yet returned.
+	// Addresses in inUse won't be returned from next.
+	// Addresses in inUse may no longer be in the ipSet definition of the pool bounds
+	// if the ippool has been reconfigured.
+	inUse *set.Set[netip.Addr]
 }
 
+// next returns the next available address from within the ippool.
+// next will return errPoolExhausted if there are no more unused addresses.
 func (ipp *ippool) next() (netip.Addr, error) {
+	if ipp == nil || ipp.ipSetIterator == nil {
+		return netip.Addr{}, errUninitializedIPPool
+	}
 	a, err := ipp.ipSetIterator.next()
 	if err != nil {
 		return netip.Addr{}, err
@@ -96,13 +110,33 @@ func (ipp *ippool) next() (netip.Addr, error) {
 	return a, nil
 }
 
+// returnAddr puts an address back into the ippool, that address will
+// now be available to be handed out when we iterate back around to it.
+// returnAddr will return an error if the provided address is not one
+// that's currently in inUse.
 func (ipp *ippool) returnAddr(a netip.Addr) error {
+	if ipp.inUse.Contains(a) {
+		ipp.inUse.Delete(a)
+		return nil
+	}
 	if !ipp.ipSet.Contains(a) {
 		return errNotOurAddress
 	}
-	if !ipp.inUse.Contains(a) {
+	return errAddrExists
-		return errAddrExists
+}
-	}
+
-	ipp.inUse.Delete(a)
+// reconfig changes the definition of the addresses that are in the ippool
-	return nil
+// while keeping track of the addresses that are currently in inUse.
+func (ipp *ippool) reconfig(ipSet *netipx.IPSet) *ippool {
+	if ipp != nil && ipSet != nil && ipSet.Equal(ipp.ipSet) {
+		// in the common case that the definition has not changed, do nothing.
+		return ipp
+	}
+	newPool := newIPPool(ipSet)
+	if ipp != nil {
+		// even if the definition of which addresses are in the pool has changed
+		// we don't want to lose track of which addresses are currently in use
+		newPool.inUse = ipp.inUse
+	}
+	return newPool
 }

feature/conn25/ippool_test.go

@@ -106,6 +106,26 @@ func TestReturnAddr(t *testing.T) {
 	}
 }
 
+func expectAddrNext(t *testing.T, ipp *ippool, addrString string) {
+	t.Helper()
+	got, err := ipp.next()
+	if err != nil {
+		t.Fatalf("expected nil error, got: %v", err)
+	}
+	want := netip.MustParseAddr(addrString)
+	if want != got {
+		t.Fatalf("want %v; got %v", want, got)
+	}
+}
+
+func expectErrPoolExhaustedNext(t *testing.T, ipp *ippool) {
+	t.Helper()
+	_, err := ipp.next()
+	if !errors.Is(err, errPoolExhausted) {
+		t.Fatalf("expected errPoolExhausted; got %v", err)
+	}
+}
+
 // TestGettingReturnedAddresses tests that when addresses are returned to the IP Pool
 // they are then handed out in the order they were returned.
 func TestGettingReturnedAddresses(t *testing.T) {
@@ -113,33 +133,67 @@ func TestGettingReturnedAddresses(t *testing.T) {
 	isb.AddRange(netipx.IPRangeFrom(netip.MustParseAddr("192.168.0.0"), netip.MustParseAddr("192.168.0.4")))
 	ipset := must.Get(isb.IPSet())
 	ipp := newIPPool(ipset)
-	expectAddrNext := func(addrString string) {
+	expectAddrNext(t, ipp, "192.168.0.0")
-		t.Helper()
+	expectAddrNext(t, ipp, "192.168.0.1")
-		got, err := ipp.next()
+	expectAddrNext(t, ipp, "192.168.0.2")
-		if err != nil {
+	expectAddrNext(t, ipp, "192.168.0.3")
-			t.Fatalf("expected nil error, got: %v", err)
+	expectAddrNext(t, ipp, "192.168.0.4")
-		}
+	expectErrPoolExhaustedNext(t, ipp)
-		want := netip.MustParseAddr(addrString)
-		if want != got {
-			t.Fatalf("want %v; got %v", want, got)
-		}
-	}
-	expectErrPoolExhaustedNext := func() {
-		t.Helper()
-		_, err := ipp.next()
-		if !errors.Is(err, errPoolExhausted) {
-			t.Fatalf("expected errPoolExhausted; got %v", err)
-		}
-	}
-	expectAddrNext("192.168.0.0")
-	expectAddrNext("192.168.0.1")
-	expectAddrNext("192.168.0.2")
-	expectAddrNext("192.168.0.3")
-	expectAddrNext("192.168.0.4")
-	expectErrPoolExhaustedNext()
 	ipp.returnAddr(netip.MustParseAddr("192.168.0.2"))
 	ipp.returnAddr(netip.MustParseAddr("192.168.0.4"))
-	expectAddrNext("192.168.0.2")
+	expectAddrNext(t, ipp, "192.168.0.2")
-	expectAddrNext("192.168.0.4")
+	expectAddrNext(t, ipp, "192.168.0.4")
-	expectErrPoolExhaustedNext()
+	expectErrPoolExhaustedNext(t, ipp)
+}
+
+func TestIPPoolReconfig(t *testing.T) {
+	var isb netipx.IPSetBuilder
+	isb.AddRange(netipx.IPRangeFrom(netip.MustParseAddr("192.168.0.0"), netip.MustParseAddr("192.168.0.4")))
+	ipsetOne := must.Get(isb.IPSet())
+	ipsetOneClone := must.Get(isb.IPSet())
+	isb = netipx.IPSetBuilder{}
+	isb.AddRange(netipx.IPRangeFrom(netip.MustParseAddr("192.168.0.7"), netip.MustParseAddr("192.168.0.10")))
+	ipsetTwo := must.Get(isb.IPSet())
+
+	var ipp *ippool
+	ipp = ipp.reconfig(ipsetOne)
+	if ipp.ipSet != ipsetOne {
+		t.Fatalf("want %v, got %v", ipsetOne, ipp.ipSet)
+	}
+	expectAddrNext(t, ipp, "192.168.0.0")
+
+	// check that we don't lose iterator state when we reconfig with the same ranges
+	expectAddrNext(t, ipp, "192.168.0.1")
+	ipp.returnAddr(netip.MustParseAddr("192.168.0.1"))
+	ipp = ipp.reconfig(ipsetOneClone)
+	expectAddrNext(t, ipp, "192.168.0.2")
+
+	// when we reconfig with different ranges, we only hand out addresses from the new ranges
+	ipp = ipp.reconfig(ipsetTwo)
+	if ipp.ipSet != ipsetTwo {
+		t.Fatalf("want %v, got %v", ipsetTwo, ipp.ipSet)
+	}
+	expectAddrNext(t, ipp, "192.168.0.7")
+	expectAddrNext(t, ipp, "192.168.0.8")
+	expectAddrNext(t, ipp, "192.168.0.9")
+	expectAddrNext(t, ipp, "192.168.0.10")
+	expectErrPoolExhaustedNext(t, ipp)
+
+	// but we have not lost track of the fact that the old addresses are in use
+	if !ipp.inUse.Contains(netip.MustParseAddr("192.168.0.0")) {
+		t.Fatalf("expected inUse to still have the address")
+	}
+
+	// old addresses can be returned
+	ipp.returnAddr(netip.MustParseAddr("192.168.0.0"))
+
+	// but they are not handed out again
+	expectErrPoolExhaustedNext(t, ipp)
+	if ipp.inUse.Contains(netip.MustParseAddr("192.168.0.0")) {
+		t.Fatalf("expected inUse to no longer have the address")
+	}
+
+	// returning addresses from the new ranges works as normal
+	ipp.returnAddr(netip.MustParseAddr("192.168.0.9"))
+	expectAddrNext(t, ipp, "192.168.0.9")
 }