Skip to content

Commit 4de54ee

Browse files
robmryrobmry
committed
Fix duplicate subnet allocations
Keep allocated subnets in-order, so that they're not mistakenly reallocated due to a gap in the list where misplaced subnets should have been. Introduced in 9d288b5. The iterator over allocated subnets was incremented too early, this change moves it past three clauses in addrSpace.allocatePredefinedPool(). The three new unit tests correspond to a separate failure caused by incrementing before each of them. Signed-off-by: Rob Murray <[email protected]>
1 parent ff1e2c0 commit 4de54ee

2 files changed

Lines changed: 113 additions & 2 deletions

File tree

‎libnetwork/ipams/defaultipam/address_space.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,8 +164,6 @@ func (aSpace *addrSpace) allocatePredefinedPool(reserved []netip.Prefix) (netip.
164164
pdf := aSpace.predefined[pdfID]
165165

166166
if allocated.Overlaps(pdf.Base) {
167-
it.Inc()
168-
169167
if allocated.Bits() <= pdf.Base.Bits() {
170168
// The current 'allocated' prefix is bigger than the 'pdf'
171169
// network, thus the block is fully overlapped.
@@ -195,6 +193,8 @@ func (aSpace *addrSpace) allocatePredefinedPool(reserved []netip.Prefix) (netip.
195193
return makeAlloc(afterPrev), nil
196194
}
197195

196+
it.Inc()
197+
198198
if netiputil.LastAddr(allocated) == netiputil.LastAddr(pdf.Base) {
199199
// The last address of the current 'allocated' prefix is the
200200
// same as the last address of the 'pdf' network, it's fully

‎libnetwork/ipams/defaultipam/address_space_test.go‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package defaultipam
22

33
import (
4+
"fmt"
45
"net/netip"
56
"testing"
67

@@ -387,3 +388,113 @@ func TestStaticAllocation(t *testing.T) {
387388
netip.MustParsePrefix("192.168.3.0/24"),
388389
}, cmpopts.EquateComparable(netip.Prefix{})))
389390
}
391+
392+
// Regression test for https://github.com/moby/moby/issues/48069
393+
func TestPoolAllocateAndRelease(t *testing.T) {
394+
type testClosures struct {
395+
alloc func(netname string)
396+
release func(netname string)
397+
}
398+
399+
testcases := []struct {
400+
name string
401+
predefined []*ipamutils.NetworkToSplit
402+
reserved []netip.Prefix
403+
calls []func(tcs testClosures)
404+
tcs testClosures
405+
}{
406+
{
407+
name: "allocate after reserved",
408+
predefined: []*ipamutils.NetworkToSplit{
409+
{Base: netip.MustParsePrefix("10.0.0.0/24"), Size: 24},
410+
{Base: netip.MustParsePrefix("10.0.1.0/24"), Size: 24},
411+
{Base: netip.MustParsePrefix("10.1.0.0/16"), Size: 24},
412+
},
413+
reserved: []netip.Prefix{
414+
netip.MustParsePrefix("10.0.0.0/16"),
415+
},
416+
calls: []func(tcs testClosures){
417+
func(tcs testClosures) { tcs.alloc("n1") },
418+
func(tcs testClosures) { tcs.alloc("n2") },
419+
},
420+
},
421+
{
422+
name: "reallocate first subnet",
423+
predefined: []*ipamutils.NetworkToSplit{
424+
{Base: netip.MustParsePrefix("10.0.0.0/8"), Size: 24},
425+
},
426+
calls: []func(tcs testClosures){
427+
func(tcs testClosures) { tcs.alloc("n1") },
428+
func(tcs testClosures) { tcs.alloc("n2") },
429+
func(tcs testClosures) { tcs.alloc("n3") },
430+
func(tcs testClosures) { tcs.release("n1") },
431+
func(tcs testClosures) { tcs.alloc("n4") },
432+
func(tcs testClosures) { tcs.alloc("n5") },
433+
},
434+
},
435+
{
436+
name: "reallocate after release",
437+
predefined: []*ipamutils.NetworkToSplit{
438+
{Base: netip.MustParsePrefix("10.0.0.0/8"), Size: 24},
439+
},
440+
calls: []func(tcs testClosures){
441+
func(tcs testClosures) { tcs.alloc("n1") },
442+
func(tcs testClosures) { tcs.alloc("n2") },
443+
func(tcs testClosures) { tcs.alloc("n3") },
444+
func(tcs testClosures) { tcs.alloc("n4") },
445+
func(tcs testClosures) { tcs.release("n2") },
446+
func(tcs testClosures) { tcs.release("n3") },
447+
func(tcs testClosures) { tcs.alloc("n5") },
448+
func(tcs testClosures) { tcs.alloc("n6") },
449+
func(tcs testClosures) { tcs.alloc("n7") },
450+
},
451+
},
452+
}
453+
454+
for _, tc := range testcases {
455+
t.Run(tc.name, func(t *testing.T) {
456+
as, err := newAddrSpace(tc.predefined)
457+
assert.NilError(t, err)
458+
subnetToNetname := map[netip.Prefix]string{}
459+
netnameToSubnet := map[string]netip.Prefix{}
460+
461+
// To avoid passing as,subnetToNetname,netnameToSubnet into each of the
462+
// functions in tc.calls (cluttering the list of testcases), create closures
463+
// that use them and pass those.
464+
tcs := testClosures{
465+
// Allocate a pool for netname, check that a subnet is returned that
466+
// isn't already allocated, and doesn't overlap with a reserved range.
467+
alloc: func(netname string) {
468+
subnet, err := as.allocatePredefinedPool(tc.reserved)
469+
assert.NilError(t, err)
470+
471+
otherNetname, exists := subnetToNetname[subnet]
472+
assert.Assert(t, !exists, fmt.Sprintf(
473+
"subnet %s allocated to %s, reallocated for %s", subnet, otherNetname, netname))
474+
for _, reserved := range tc.reserved {
475+
assert.Assert(t, !reserved.Overlaps(subnet),
476+
fmt.Sprintf("subnet %s allocated in reserved range %s", subnet, reserved))
477+
}
478+
479+
subnetToNetname[subnet] = netname
480+
netnameToSubnet[netname] = subnet
481+
},
482+
// Release a pool for a netname - the test must ensure netname currently
483+
// has an allocated subnet.
484+
release: func(netname string) {
485+
subnet, ok := netnameToSubnet[netname]
486+
assert.Assert(t, ok)
487+
err := as.releaseSubnet(subnet, netip.Prefix{})
488+
assert.NilError(t, err)
489+
490+
delete(netnameToSubnet, netname)
491+
delete(subnetToNetname, subnet)
492+
},
493+
}
494+
495+
for _, f := range tc.calls {
496+
f(tcs)
497+
}
498+
})
499+
}
500+
}

0 commit comments

Comments
 (0)