diff --git a/feature/conn25/conn25.go b/feature/conn25/conn25.go index 944dc888d..cc1b38a71 100644 --- a/feature/conn25/conn25.go +++ b/feature/conn25/conn25.go @@ -707,10 +707,10 @@ func (c *client) reconfig() { ipSets := c.getIPSets() - c.v4MagicIPPool = newIPPool(ipSets.v4Magic) - c.v4TransitIPPool = newIPPool(ipSets.v4Transit) - c.v6MagicIPPool = newIPPool(ipSets.v6Magic) - c.v6TransitIPPool = newIPPool(ipSets.v6Transit) + c.v4MagicIPPool = c.v4MagicIPPool.reconfig(ipSets.v4Magic) + c.v4TransitIPPool = c.v4TransitIPPool.reconfig(ipSets.v4Transit) + c.v6MagicIPPool = c.v6MagicIPPool.reconfig(ipSets.v6Magic) + c.v6TransitIPPool = c.v6TransitIPPool.reconfig(ipSets.v6Transit) } // getAppsForConnectorDomain returns the slice of app names which match the diff --git a/feature/conn25/conn25_test.go b/feature/conn25/conn25_test.go index c8d20962c..b3414d3da 100644 --- a/feature/conn25/conn25_test.go +++ b/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) + } + }) + } +} diff --git a/feature/conn25/ippool.go b/feature/conn25/ippool.go index 4ae8918d4..72149cbee 100644 --- a/feature/conn25/ippool.go +++ b/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 - } - ipp.inUse.Delete(a) - return nil + return errAddrExists +} + +// reconfig changes the definition of the addresses that are in the ippool +// 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 } diff --git a/feature/conn25/ippool_test.go b/feature/conn25/ippool_test.go index 431ea6998..1cc9845f5 100644 --- a/feature/conn25/ippool_test.go +++ b/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) { - 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) - } - } - 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() + expectAddrNext(t, ipp, "192.168.0.0") + expectAddrNext(t, ipp, "192.168.0.1") + expectAddrNext(t, ipp, "192.168.0.2") + expectAddrNext(t, ipp, "192.168.0.3") + expectAddrNext(t, ipp, "192.168.0.4") + expectErrPoolExhaustedNext(t, ipp) ipp.returnAddr(netip.MustParseAddr("192.168.0.2")) ipp.returnAddr(netip.MustParseAddr("192.168.0.4")) - expectAddrNext("192.168.0.2") - expectAddrNext("192.168.0.4") - expectErrPoolExhaustedNext() + expectAddrNext(t, ipp, "192.168.0.2") + expectAddrNext(t, ipp, "192.168.0.4") + 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") }