From ddebbfd806b5813cc0f6d67ae9608be393729922 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Thu, 12 Mar 2020 11:27:48 -0400 Subject: [PATCH 1/3] update for APIs being moved to utilnet Several of the functions in pkg/registry/core/service/ipallocator were moved to k8s.io/utils/net, but then the original code was never updated to used to the vendored versions. (utilnet's version of RangeSize does not have the IPv6 special case that the original code did, so we need to move that to NewAllocatorCIDRRange now.) --- .../core/service/ipallocator/allocator.go | 47 +++++-------------- .../service/ipallocator/allocator_test.go | 45 ------------------ 2 files changed, 11 insertions(+), 81 deletions(-) diff --git a/pkg/registry/core/service/ipallocator/allocator.go b/pkg/registry/core/service/ipallocator/allocator.go index 732a089ac38..4ff99757d18 100644 --- a/pkg/registry/core/service/ipallocator/allocator.go +++ b/pkg/registry/core/service/ipallocator/allocator.go @@ -82,10 +82,17 @@ type Range struct { // NewAllocatorCIDRRange creates a Range over a net.IPNet, calling allocatorFactory to construct the backing store. func NewAllocatorCIDRRange(cidr *net.IPNet, allocatorFactory allocator.AllocatorFactory) (*Range, error) { - max := RangeSize(cidr) - base := bigForIP(cidr.IP) + max := utilnet.RangeSize(cidr) + base := utilnet.BigForIP(cidr.IP) rangeSpec := cidr.String() + if utilnet.IsIPv6CIDR(cidr) { + // Limit the max size, since the allocator keeps a bitmap of that size. + if max > 65536 { + max = 65536 + } + } + r := Range{ net: cidr, base: base.Add(base, big.NewInt(1)), // don't use the network base @@ -171,7 +178,7 @@ func (r *Range) AllocateNext() (net.IP, error) { if !ok { return nil, ErrFull } - return addIPOffset(r.base, offset), nil + return utilnet.AddIPOffset(r.base, offset), nil } // Release releases the IP back to the pool. Releasing an @@ -247,40 +254,8 @@ func (r *Range) contains(ip net.IP) (bool, int) { return true, offset } -// bigForIP creates a big.Int based on the provided net.IP -func bigForIP(ip net.IP) *big.Int { - b := ip.To4() - if b == nil { - b = ip.To16() - } - return big.NewInt(0).SetBytes(b) -} - -// addIPOffset adds the provided integer offset to a base big.Int representing a -// net.IP -func addIPOffset(base *big.Int, offset int) net.IP { - return net.IP(big.NewInt(0).Add(base, big.NewInt(int64(offset))).Bytes()) -} - // calculateIPOffset calculates the integer offset of ip from base such that // base + offset = ip. It requires ip >= base. func calculateIPOffset(base *big.Int, ip net.IP) int { - return int(big.NewInt(0).Sub(bigForIP(ip), base).Int64()) -} - -// RangeSize returns the size of a range in valid addresses. -func RangeSize(subnet *net.IPNet) int64 { - ones, bits := subnet.Mask.Size() - if bits == 32 && (bits-ones) >= 31 || bits == 128 && (bits-ones) >= 127 { - return 0 - } - // For IPv6, the max size will be limited to 65536 - // This is due to the allocator keeping track of all the - // allocated IP's in a bitmap. This will keep the size of - // the bitmap to 64k. - if bits == 128 && (bits-ones) >= 16 { - return int64(1) << uint(16) - } else { - return int64(1) << uint(bits-ones) - } + return int(big.NewInt(0).Sub(utilnet.BigForIP(ip), base).Int64()) } diff --git a/pkg/registry/core/service/ipallocator/allocator_test.go b/pkg/registry/core/service/ipallocator/allocator_test.go index 86527154c77..a45cf9c60c4 100644 --- a/pkg/registry/core/service/ipallocator/allocator_test.go +++ b/pkg/registry/core/service/ipallocator/allocator_test.go @@ -213,51 +213,6 @@ func TestAllocateSmall(t *testing.T) { t.Logf("allocated: %v", found) } -func TestRangeSize(t *testing.T) { - testCases := []struct { - name string - cidr string - addrs int64 - }{ - { - name: "supported IPv4 cidr", - cidr: "192.168.1.0/24", - addrs: 256, - }, - { - name: "supported large IPv4 cidr", - cidr: "10.96.0.0/12", - addrs: 1048576, - }, - { - name: "unsupported IPv4 cidr", - cidr: "192.168.1.0/1", - addrs: 0, - }, - { - name: "supported IPv6 cidr", - cidr: "2001:db8::/48", - addrs: 65536, - }, - { - name: "unsupported IPv6 mask", - cidr: "2001:db8::/1", - addrs: 0, - }, - } - - for _, tc := range testCases { - _, cidr, err := net.ParseCIDR(tc.cidr) - if err != nil { - t.Errorf("failed to parse cidr for test %s, unexpected error: '%s'", tc.name, err) - } - if size := RangeSize(cidr); size != tc.addrs { - t.Errorf("test %s failed. %s should have a range size of %d, got %d", - tc.name, tc.cidr, tc.addrs, size) - } - } -} - func TestForEach(t *testing.T) { _, cidr, err := net.ParseCIDR("192.168.1.0/24") if err != nil { From 4a7c86c105f49972a5d7b8150cdba59eafb8a0fd Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Thu, 12 Mar 2020 14:52:34 -0400 Subject: [PATCH 2/3] make test a bit more generic --- .../service/ipallocator/allocator_test.go | 55 +++++++++---------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/pkg/registry/core/service/ipallocator/allocator_test.go b/pkg/registry/core/service/ipallocator/allocator_test.go index a45cf9c60c4..e1f93dee2cd 100644 --- a/pkg/registry/core/service/ipallocator/allocator_test.go +++ b/pkg/registry/core/service/ipallocator/allocator_test.go @@ -30,29 +30,34 @@ func TestAllocate(t *testing.T) { cidr string free int released string - outOfRange1 string - outOfRange2 string - outOfRange3 string + outOfRange []string alreadyAllocated string }{ { - name: "IPv4", - cidr: "192.168.1.0/24", - free: 254, - released: "192.168.1.5", - outOfRange1: "192.168.0.1", - outOfRange2: "192.168.1.0", - outOfRange3: "192.168.1.255", + name: "IPv4", + cidr: "192.168.1.0/24", + free: 254, + released: "192.168.1.5", + outOfRange: []string{ + "192.168.0.1", // not in 192.168.1.0/24 + "192.168.1.0", // reserved (base address) + "192.168.1.255", // reserved (broadcast address) + "192.168.2.2", // not in 192.168.1.0/24 + }, alreadyAllocated: "192.168.1.1", }, { - name: "IPv6", - cidr: "2001:db8:1::/48", - free: 65534, - released: "2001:db8:1::5", - outOfRange1: "2001:db8::1", - outOfRange2: "2001:db8:1::", - outOfRange3: "2001:db8:1::ffff", + name: "IPv6", + cidr: "2001:db8:1::/48", + free: 65534, + released: "2001:db8:1::5", + outOfRange: []string{ + "2001:db8::1", // not in 2001:db8:1::/48 + "2001:db8:1::", // reserved (base address) + "2001:db8:1::ffff", // reserved (broadcast address) + "2001:db8:1::1:0", // not in the low 16 bits of 2001:db8:1::/48 + "2001:db8:2::2", // not in 2001:db8:1::/48 + }, alreadyAllocated: "2001:db8:1::1", }, } @@ -119,21 +124,15 @@ func TestAllocate(t *testing.T) { if err := r.Release(released); err != nil { t.Fatal(err) } - err = r.Allocate(net.ParseIP(tc.outOfRange1)) - if _, ok := err.(*ErrNotInRange); !ok { - t.Fatal(err) + for _, outOfRange := range tc.outOfRange { + err = r.Allocate(net.ParseIP(outOfRange)) + if _, ok := err.(*ErrNotInRange); !ok { + t.Fatal(err) + } } if err := r.Allocate(net.ParseIP(tc.alreadyAllocated)); err != ErrAllocated { t.Fatal(err) } - err = r.Allocate(net.ParseIP(tc.outOfRange2)) - if _, ok := err.(*ErrNotInRange); !ok { - t.Fatal(err) - } - err = r.Allocate(net.ParseIP(tc.outOfRange3)) - if _, ok := err.(*ErrNotInRange); !ok { - t.Fatal(err) - } if f := r.Free(); f != 1 { t.Errorf("Test %s unexpected free %d", tc.name, f) } From f6dcc1c07e0a2d3c583cb90e1cdb7ec4718625ce Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Wed, 8 Apr 2020 12:44:38 -0400 Subject: [PATCH 3/3] Minor tweak to IPv6 service IP allocation The service allocator skips the "broadcast address" in the service CIDR, but that concept only applies to IPv4 addressing. --- pkg/registry/core/service/ipallocator/allocator.go | 11 +++++++++-- .../core/service/ipallocator/allocator_test.go | 11 +++++------ .../service/ipallocator/controller/repair_test.go | 2 +- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/pkg/registry/core/service/ipallocator/allocator.go b/pkg/registry/core/service/ipallocator/allocator.go index 4ff99757d18..634569f767a 100644 --- a/pkg/registry/core/service/ipallocator/allocator.go +++ b/pkg/registry/core/service/ipallocator/allocator.go @@ -91,12 +91,19 @@ func NewAllocatorCIDRRange(cidr *net.IPNet, allocatorFactory allocator.Allocator if max > 65536 { max = 65536 } + } else { + // Don't use the IPv4 network's broadcast address. + max-- } + // Don't use the network's ".0" address. + base.Add(base, big.NewInt(1)) + max-- + r := Range{ net: cidr, - base: base.Add(base, big.NewInt(1)), // don't use the network base - max: maximum(0, int(max-2)), // don't use the network broadcast, + base: base, + max: maximum(0, int(max)), } var err error r.alloc, err = allocatorFactory(r.max, rangeSpec) diff --git a/pkg/registry/core/service/ipallocator/allocator_test.go b/pkg/registry/core/service/ipallocator/allocator_test.go index e1f93dee2cd..a66b1f51575 100644 --- a/pkg/registry/core/service/ipallocator/allocator_test.go +++ b/pkg/registry/core/service/ipallocator/allocator_test.go @@ -49,14 +49,13 @@ func TestAllocate(t *testing.T) { { name: "IPv6", cidr: "2001:db8:1::/48", - free: 65534, + free: 65535, released: "2001:db8:1::5", outOfRange: []string{ - "2001:db8::1", // not in 2001:db8:1::/48 - "2001:db8:1::", // reserved (base address) - "2001:db8:1::ffff", // reserved (broadcast address) - "2001:db8:1::1:0", // not in the low 16 bits of 2001:db8:1::/48 - "2001:db8:2::2", // not in 2001:db8:1::/48 + "2001:db8::1", // not in 2001:db8:1::/48 + "2001:db8:1::", // reserved (base address) + "2001:db8:1::1:0", // not in the low 16 bits of 2001:db8:1::/48 + "2001:db8:2::2", // not in 2001:db8:1::/48 }, alreadyAllocated: "2001:db8:1::1", }, diff --git a/pkg/registry/core/service/ipallocator/controller/repair_test.go b/pkg/registry/core/service/ipallocator/controller/repair_test.go index 26ef1bd34c7..9b83282e5c5 100644 --- a/pkg/registry/core/service/ipallocator/controller/repair_test.go +++ b/pkg/registry/core/service/ipallocator/controller/repair_test.go @@ -529,7 +529,7 @@ func TestRepairWithExistingDualStack(t *testing.T) { if !secondaryAfter.Has(net.ParseIP("2000::1")) || !secondaryAfter.Has(net.ParseIP("2000::2")) { t.Errorf("unexpected ipallocator state: %#v", secondaryAfter) } - if free := secondaryAfter.Free(); free != 65532 { + if free := secondaryAfter.Free(); free != 65533 { t.Errorf("unexpected ipallocator state: %d free (number of free ips is not 65532)", free) }