From dcb8d9b31a7449f908e6efc2f9d5ab8dc6adefb1 Mon Sep 17 00:00:00 2001 From: Euan Harris Date: Wed, 23 May 2018 16:21:31 +0100 Subject: [PATCH 1/2] ipam, types: Expand documentation Signed-off-by: Euan Harris --- ipam/allocator.go | 10 ++++++++-- ipam/structures.go | 3 ++- types/types.go | 2 ++ 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/ipam/allocator.go b/ipam/allocator.go index 7213148bcc..44070119dc 100644 --- a/ipam/allocator.go +++ b/ipam/allocator.go @@ -203,6 +203,10 @@ func (a *Allocator) GetDefaultAddressSpaces() (string, string, error) { } // RequestPool returns an address pool along with its unique id. +// addressSpace must be a valid address space name and must not be the empty string. +// If pool is the empty string then the default predefined pool for addressSpace will be used, otherwise pool must be a valid IP address and length in CIDR notation. +// If subPool is not empty, it must be a valid IP address and length in CIDR notation which is a sub-range of pool. +// subPool must be empty if pool is empty. func (a *Allocator) RequestPool(addressSpace, pool, subPool string, options map[string]string, v6 bool) (string, *net.IPNet, map[string]string, error) { logrus.Debugf("RequestPool(%s, %s, %s, %v, %t)", addressSpace, pool, subPool, options, v6) @@ -283,8 +287,8 @@ retry: return remove() } -// Given the address space, returns the local or global PoolConfig based on the -// address space is local or global. AddressSpace locality is being registered with IPAM out of band. +// Given the address space, returns the local or global PoolConfig based on whether the +// address space is local or global. AddressSpace locality is registered with IPAM out of band. func (a *Allocator) getAddrSpace(as string) (*addrSpace, error) { a.Lock() defer a.Unlock() @@ -295,6 +299,8 @@ func (a *Allocator) getAddrSpace(as string) (*addrSpace, error) { return aSpace, nil } +// parsePoolRequest parses and validates a request to create a new pool under addressSpace and returns +// a SubnetKey, network and range describing the request. func (a *Allocator) parsePoolRequest(addressSpace, pool, subPool string, v6 bool) (*SubnetKey, *net.IPNet, *AddressRange, error) { var ( nw *net.IPNet diff --git a/ipam/structures.go b/ipam/structures.go index 455a16ca65..2e6d75eaa4 100644 --- a/ipam/structures.go +++ b/ipam/structures.go @@ -257,6 +257,7 @@ func (aSpace *addrSpace) New() datastore.KVObject { } } +// updatePoolDBOnAdd returns a closure which will add the subnet k to the address space when executed. func (aSpace *addrSpace) updatePoolDBOnAdd(k SubnetKey, nw *net.IPNet, ipr *AddressRange, pdf bool) (func() error, error) { aSpace.Lock() defer aSpace.Unlock() @@ -281,7 +282,7 @@ func (aSpace *addrSpace) updatePoolDBOnAdd(k SubnetKey, nw *net.IPNet, ipr *Addr return func() error { return aSpace.alloc.insertBitMask(k, nw) }, nil } - // This is a new non-master pool + // This is a new non-master pool (subPool) p := &PoolData{ ParentKey: SubnetKey{AddressSpace: k.AddressSpace, Subnet: k.Subnet}, Pool: nw, diff --git a/types/types.go b/types/types.go index 5968545ba5..b102ba4c39 100644 --- a/types/types.go +++ b/types/types.go @@ -332,6 +332,8 @@ func CompareIPNet(a, b *net.IPNet) bool { } // GetMinimalIP returns the address in its shortest form +// If ip contains an IPv4-mapped IPv6 address, the 4-octet form of the IPv4 address will be returned. +// Otherwise ip is returned unchanged. func GetMinimalIP(ip net.IP) net.IP { if ip != nil && ip.To4() != nil { return ip.To4() From 3be8d7efc25e5e2a79dc72803e0e3b81494184e3 Mon Sep 17 00:00:00 2001 From: Euan Harris Date: Wed, 30 May 2018 15:51:04 +0100 Subject: [PATCH 2/2] ipam: Test rejection of overlapping pool requests TestOverlappingRequests checks that pool requests which are supersets or subsets of existing allocations, and those which overlap with existing allocations at the beginning or the end. Multiple allocation is now tested by TestOverlappingRequests, so TestDoublePoolRelease only needs to test double releasing. Signed-off-by: Euan Harris --- ipam/allocator_test.go | 106 +++++++++++++++++++++++++++++++++-------- 1 file changed, 87 insertions(+), 19 deletions(-) diff --git a/ipam/allocator_test.go b/ipam/allocator_test.go index 65f63f8643..5eea16c9f9 100644 --- a/ipam/allocator_test.go +++ b/ipam/allocator_test.go @@ -286,28 +286,17 @@ func TestAddSubnets(t *testing.T) { // been released raises an error. func TestDoublePoolRelease(t *testing.T) { for _, store := range []bool{false, true} { - for _, repeats := range []int{0, 1, 10} { - a, err := getAllocator(store) - assert.NoError(t, err) - - // Request initial pool allocation - pid0, _, _, err := a.RequestPool(localAddressSpace, "10.0.0.0/8", "", nil, false) - assert.NoError(t, err) + a, err := getAllocator(store) + assert.NoError(t, err) - // Re-request the same pool - for i := 0; i < repeats; i++ { - _, _, _, err := a.RequestPool(localAddressSpace, "10.0.0.0/8", "", nil, false) - assert.Error(t, err) - } + pid0, _, _, err := a.RequestPool(localAddressSpace, "10.0.0.0/8", "", nil, false) + assert.NoError(t, err) - // Release the initial request - err = a.ReleasePool(pid0) - assert.NoError(t, err) + err = a.ReleasePool(pid0) + assert.NoError(t, err) - // Releasing again fails - err = a.ReleasePool(pid0) - assert.Error(t, err) - } + err = a.ReleasePool(pid0) + assert.Error(t, err) } } @@ -987,6 +976,85 @@ func TestRequest(t *testing.T) { } } +// TestOverlappingRequests tests that overlapping subnets cannot be allocated. +// Requests for subnets which are supersets or subsets of existing allocations, +// or which overlap at the beginning or end, should not be permitted. +func TestOverlappingRequests(t *testing.T) { + input := []struct { + environment []string + subnet string + ok bool + }{ + // IPv4 + // Previously allocated network does not overlap with request + {[]string{"10.0.0.0/8"}, "11.0.0.0/8", true}, + {[]string{"74.0.0.0/7"}, "9.111.99.72/30", true}, + {[]string{"110.192.0.0/10"}, "16.0.0.0/10", true}, + + // Previously allocated network entirely contains request + {[]string{"10.0.0.0/8"}, "10.0.0.0/8", false}, // exact overlap + {[]string{"0.0.0.0/1"}, "16.182.0.0/15", false}, + {[]string{"16.0.0.0/4"}, "17.11.66.0/23", false}, + + // Previously allocated network overlaps beginning of request + {[]string{"0.0.0.0/1"}, "0.0.0.0/0", false}, + {[]string{"64.0.0.0/6"}, "64.0.0.0/3", false}, + {[]string{"112.0.0.0/6"}, "112.0.0.0/4", false}, + + // Previously allocated network overlaps end of request + {[]string{"96.0.0.0/3"}, "0.0.0.0/1", false}, + {[]string{"192.0.0.0/2"}, "128.0.0.0/1", false}, + {[]string{"95.0.0.0/8"}, "92.0.0.0/6", false}, + + // Previously allocated network entirely contained within request + {[]string{"10.0.0.0/8"}, "10.0.0.0/6", false}, // non-canonical + {[]string{"10.0.0.0/8"}, "8.0.0.0/6", false}, // canonical + {[]string{"25.173.144.0/20"}, "0.0.0.0/0", false}, + + // IPv6 + // Previously allocated network entirely contains request + {[]string{"::/0"}, "f656:3484:c878:a05:e540:a6ed:4d70:3740/123", false}, + {[]string{"8000::/1"}, "8fe8:e7c4:5779::/49", false}, + {[]string{"f000::/4"}, "ffc7:6000::/19", false}, + + // Previously allocated network overlaps beginning of request + {[]string{"::/2"}, "::/0", false}, + {[]string{"::/3"}, "::/1", false}, + {[]string{"::/6"}, "::/5", false}, + + // Previously allocated network overlaps end of request + {[]string{"c000::/2"}, "8000::/1", false}, + {[]string{"7c00::/6"}, "::/1", false}, + {[]string{"cf80::/9"}, "c000::/4", false}, + + // Previously allocated network entirely contained within request + {[]string{"ff77:93f8::/29"}, "::/0", false}, + {[]string{"9287:2e20:5134:fab6:9061:a0c6:bfe3:9400/119"}, "8000::/1", false}, + {[]string{"3ea1:bfa9:8691:d1c6:8c46:519b:db6d:e700/120"}, "3000::/4", false}, + } + + for _, store := range []bool{false, true} { + for _, tc := range input { + a, err := getAllocator(store) + assert.NoError(t, err) + + // Set up some existing allocations. This should always succeed. + for _, env := range tc.environment { + _, _, _, err = a.RequestPool(localAddressSpace, env, "", nil, false) + assert.NoError(t, err) + } + + // Make the test allocation. + _, _, _, err = a.RequestPool(localAddressSpace, tc.subnet, "", nil, false) + if tc.ok { + assert.NoError(t, err) + } else { + assert.Error(t, err) + } + } + } +} + func TestRelease(t *testing.T) { var ( subnet = "192.168.0.0/23"