From ef8936ce6adb6562e0fd98a7ed8e798c6f16ce39 Mon Sep 17 00:00:00 2001 From: Robert Pothier Date: Sun, 14 May 2017 08:37:11 -0400 Subject: [PATCH] Updating NewCIDRSet return value for IPv6, NewCIDRSet was updated to return an error if the subnet mask size is too big. In this case, NewCIDRSet will fail and return an error. --- pkg/controller/node/ipam/cidrset/cidr_set.go | 15 ++++--- .../node/ipam/cidrset/cidr_set_test.go | 45 ++++++++++++++----- pkg/controller/node/ipam/controller.go | 7 ++- pkg/controller/node/ipam/controller_test.go | 5 ++- pkg/controller/node/ipam/range_allocator.go | 6 ++- pkg/controller/node/ipam/sync/sync_test.go | 9 ++-- 6 files changed, 63 insertions(+), 24 deletions(-) diff --git a/pkg/controller/node/ipam/cidrset/cidr_set.go b/pkg/controller/node/ipam/cidrset/cidr_set.go index c977fd08b89..088c62511b5 100644 --- a/pkg/controller/node/ipam/cidrset/cidr_set.go +++ b/pkg/controller/node/ipam/cidrset/cidr_set.go @@ -48,30 +48,33 @@ const ( ) var ( - // ErrCIDRRangeNoCIDRsRemaining occurs when there are no more space + // ErrCIDRRangeNoCIDRsRemaining occurs when there is no more space // to allocate CIDR ranges. ErrCIDRRangeNoCIDRsRemaining = errors.New( "CIDR allocation failed; there are no remaining CIDRs left to allocate in the accepted range") + // ErrCIDRSetSubNetTooBig occurs when the subnet mask size is too + // big compared to the CIDR mask size. + ErrCIDRSetSubNetTooBig = errors.New( + "New CIDR set failed; the node CIDR size is too big") ) // NewCIDRSet creates a new CidrSet. -func NewCIDRSet(clusterCIDR *net.IPNet, subNetMaskSize int) *CidrSet { +func NewCIDRSet(clusterCIDR *net.IPNet, subNetMaskSize int) (*CidrSet, error) { clusterMask := clusterCIDR.Mask clusterMaskSize, _ := clusterMask.Size() var maxCIDRs int if (clusterCIDR.IP.To4() == nil) && (subNetMaskSize-clusterMaskSize > clusterSubnetMaxDiff) { - maxCIDRs = 0 - } else { - maxCIDRs = 1 << uint32(subNetMaskSize-clusterMaskSize) + return nil, ErrCIDRSetSubNetTooBig } + maxCIDRs = 1 << uint32(subNetMaskSize-clusterMaskSize) return &CidrSet{ clusterCIDR: clusterCIDR, clusterIP: clusterCIDR.IP, clusterMaskSize: clusterMaskSize, maxCIDRs: maxCIDRs, subNetMaskSize: subNetMaskSize, - } + }, nil } // TODO: Remove this function when upgrading to go 1.9 diff --git a/pkg/controller/node/ipam/cidrset/cidr_set_test.go b/pkg/controller/node/ipam/cidrset/cidr_set_test.go index a79cc100a14..826eeba9bd8 100644 --- a/pkg/controller/node/ipam/cidrset/cidr_set_test.go +++ b/pkg/controller/node/ipam/cidrset/cidr_set_test.go @@ -47,8 +47,10 @@ func TestCIDRSetFullyAllocated(t *testing.T) { } for _, tc := range cases { _, clusterCIDR, _ := net.ParseCIDR(tc.clusterCIDRStr) - a := NewCIDRSet(clusterCIDR, tc.subNetMaskSize) - + a, err := NewCIDRSet(clusterCIDR, tc.subNetMaskSize) + if err != nil { + t.Fatalf("unexpected error: %v for %v", err, tc.description) + } p, err := a.AllocateNext() if err != nil { t.Fatalf("unexpected error: %v for %v", err, tc.description) @@ -196,7 +198,10 @@ func TestIndexToCIDRBlock(t *testing.T) { } for _, tc := range cases { _, clusterCIDR, _ := net.ParseCIDR(tc.clusterCIDRStr) - a := NewCIDRSet(clusterCIDR, tc.subnetMaskSize) + a, err := NewCIDRSet(clusterCIDR, tc.subnetMaskSize) + if err != nil { + t.Fatalf("error for %v ", tc.description) + } cidr := a.indexToCIDRBlock(tc.index) if cidr.String() != tc.CIDRBlock { t.Fatalf("error for %v index %d %s", tc.description, tc.index, cidr.String()) @@ -220,7 +225,10 @@ func TestCIDRSet_RandomishAllocation(t *testing.T) { } for _, tc := range cases { _, clusterCIDR, _ := net.ParseCIDR(tc.clusterCIDRStr) - a := NewCIDRSet(clusterCIDR, 24) + a, err := NewCIDRSet(clusterCIDR, 24) + if err != nil { + t.Fatalf("Error allocating CIDRSet for %v", tc.description) + } // allocate all the CIDRs var cidrs []*net.IPNet @@ -232,7 +240,7 @@ func TestCIDRSet_RandomishAllocation(t *testing.T) { } } - var err error + //var err error _, err = a.AllocateNext() if err == nil { t.Fatalf("expected error because of fully-allocated range for %v", tc.description) @@ -278,8 +286,10 @@ func TestCIDRSet_AllocationOccupied(t *testing.T) { } for _, tc := range cases { _, clusterCIDR, _ := net.ParseCIDR(tc.clusterCIDRStr) - a := NewCIDRSet(clusterCIDR, 24) - + a, err := NewCIDRSet(clusterCIDR, 24) + if err != nil { + t.Fatalf("Error allocating CIDRSet for %v", tc.description) + } // allocate all the CIDRs var cidrs []*net.IPNet var numCIDRs = 256 @@ -292,7 +302,7 @@ func TestCIDRSet_AllocationOccupied(t *testing.T) { } } - var err error + //var err error _, err = a.AllocateNext() if err == nil { t.Fatalf("expected error because of fully-allocated range for %v", tc.description) @@ -457,8 +467,10 @@ func TestGetBitforCIDR(t *testing.T) { t.Fatalf("unexpected error: %v for %v", err, tc.description) } - cs := NewCIDRSet(clusterCIDR, tc.subNetMaskSize) - + cs, err := NewCIDRSet(clusterCIDR, tc.subNetMaskSize) + if err != nil { + t.Fatalf("Error allocating CIDRSet for %v", tc.description) + } _, subnetCIDR, err := net.ParseCIDR(tc.subNetCIDRStr) if err != nil { t.Fatalf("unexpected error: %v for %v", err, tc.description) @@ -625,7 +637,10 @@ func TestOccupy(t *testing.T) { t.Fatalf("unexpected error: %v for %v", err, tc.description) } - cs := NewCIDRSet(clusterCIDR, tc.subNetMaskSize) + cs, err := NewCIDRSet(clusterCIDR, tc.subNetMaskSize) + if err != nil { + t.Fatalf("Error allocating CIDRSet for %v", tc.description) + } _, subnetCIDR, err := net.ParseCIDR(tc.subNetCIDRStr) if err != nil { @@ -686,7 +701,13 @@ func TestCIDRSetv6(t *testing.T) { } for _, tc := range cases { _, clusterCIDR, _ := net.ParseCIDR(tc.clusterCIDRStr) - a := NewCIDRSet(clusterCIDR, tc.subNetMaskSize) + a, err := NewCIDRSet(clusterCIDR, tc.subNetMaskSize) + if err != nil { + if tc.expectErr { + continue + } + t.Fatalf("Error allocating CIDRSet for %v", tc.description) + } p, err := a.AllocateNext() if err == nil && tc.expectErr { diff --git a/pkg/controller/node/ipam/controller.go b/pkg/controller/node/ipam/controller.go index f8a2375678f..4b1221b6781 100644 --- a/pkg/controller/node/ipam/controller.go +++ b/pkg/controller/node/ipam/controller.go @@ -76,11 +76,16 @@ func NewController( return nil, fmt.Errorf("cloud IPAM controller does not support %q provider", cloud.ProviderName()) } + set, err := cidrset.NewCIDRSet(clusterCIDR, nodeCIDRMaskSize) + if err != nil { + return nil, err + } + c := &Controller{ config: config, adapter: newAdapter(kubeClient, gceCloud), syncers: make(map[string]*nodesync.NodeSync), - set: cidrset.NewCIDRSet(clusterCIDR, nodeCIDRMaskSize), + set: set, } if err := occupyServiceCIDR(c.set, clusterCIDR, serviceCIDR); err != nil { diff --git a/pkg/controller/node/ipam/controller_test.go b/pkg/controller/node/ipam/controller_test.go index 22baf2081b0..14fbb4340f3 100644 --- a/pkg/controller/node/ipam/controller_test.go +++ b/pkg/controller/node/ipam/controller_test.go @@ -37,7 +37,10 @@ TestCase: {"10.2.0.0/24"}, } { serviceCIDR := test.MustParseCIDR(tc.serviceCIDR) - set := cidrset.NewCIDRSet(test.MustParseCIDR(clusterCIDR), 24) + set, err := cidrset.NewCIDRSet(test.MustParseCIDR(clusterCIDR), 24) + if err != nil { + t.Errorf("test case %+v: NewCIDRSet() = %v, want nil", tc, err) + } if err := occupyServiceCIDR(set, test.MustParseCIDR(clusterCIDR), serviceCIDR); err != nil { t.Errorf("test case %+v: occupyServiceCIDR() = %v, want nil", tc, err) } diff --git a/pkg/controller/node/ipam/range_allocator.go b/pkg/controller/node/ipam/range_allocator.go index 5fdb90145a6..102eac43d8e 100644 --- a/pkg/controller/node/ipam/range_allocator.go +++ b/pkg/controller/node/ipam/range_allocator.go @@ -70,9 +70,13 @@ func NewCIDRRangeAllocator(client clientset.Interface, clusterCIDR *net.IPNet, s glog.V(0).Infof("Sending events to api server.") eventBroadcaster.StartRecordingToSink(&v1core.EventSinkImpl{Interface: v1core.New(client.CoreV1().RESTClient()).Events("")}) + set, err := cidrset.NewCIDRSet(clusterCIDR, subNetMaskSize) + if err != nil { + return nil, err + } ra := &rangeAllocator{ client: client, - cidrs: cidrset.NewCIDRSet(clusterCIDR, subNetMaskSize), + cidrs: set, clusterCIDR: clusterCIDR, nodeCIDRUpdateChannel: make(chan nodeAndCIDR, cidrUpdateQueueSize), recorder: recorder, diff --git a/pkg/controller/node/ipam/sync/sync_test.go b/pkg/controller/node/ipam/sync/sync_test.go index 15d3e1b5b35..d3268480439 100644 --- a/pkg/controller/node/ipam/sync/sync_test.go +++ b/pkg/controller/node/ipam/sync/sync_test.go @@ -194,7 +194,8 @@ func TestNodeSyncUpdate(t *testing.T) { wantError: false, }, } { - sync := New(&tc.fake, &tc.fake, &tc.fake, tc.mode, "node1", cidrset.NewCIDRSet(clusterCIDRRange, 24)) + cidr, _ := cidrset.NewCIDRSet(clusterCIDRRange, 24) + sync := New(&tc.fake, &tc.fake, &tc.fake, tc.mode, "node1", cidr) doneChan := make(chan struct{}) // Do a single step of the loop. @@ -225,7 +226,8 @@ func TestNodeSyncResync(t *testing.T) { resyncTimeout: time.Millisecond, reportChan: make(chan struct{}), } - sync := New(fake, fake, fake, SyncFromCluster, "node1", cidrset.NewCIDRSet(clusterCIDRRange, 24)) + cidr, _ := cidrset.NewCIDRSet(clusterCIDRRange, 24) + sync := New(fake, fake, fake, SyncFromCluster, "node1", cidr) doneChan := make(chan struct{}) go sync.Loop(doneChan) @@ -267,7 +269,8 @@ func TestNodeSyncDelete(t *testing.T) { }, }, } { - sync := New(&tc.fake, &tc.fake, &tc.fake, tc.mode, "node1", cidrset.NewCIDRSet(clusterCIDRRange, 24)) + cidr, _ := cidrset.NewCIDRSet(clusterCIDRRange, 24) + sync := New(&tc.fake, &tc.fake, &tc.fake, tc.mode, "node1", cidr) doneChan := make(chan struct{}) // Do a single step of the loop.