Merge pull request #45792 from rpothier/cidr_allocator_ipv6

Automatic merge from submit-queue (batch tested with PRs 46946, 45792). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Updating NewCIDRSet return a value

**What this PR does / why we need it**:
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.



**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #52348

**Special notes for your reviewer**:


**Release note**:

```release-note
```

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
+		return nil, ErrCIDRSetSubNetTooBig
-	} else {
-		maxCIDRs = 1 << uint32(subNetMaskSize-clusterMaskSize)
 	}
+	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

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 {

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 {

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)
 		}

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,

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.