Merge pull request #3706 from Azure/mrwinston/hotfix-dev-cluster-create-better-subnet-creation

[Dev Tooling] generateSubnets should check if subnets are already taken

pkg/util/azureclient/mgmt/network/subnets.go

@@ -15,6 +15,7 @@ import (
 // SubnetsClient is a minimal interface for azure SubnetsClient
 type SubnetsClient interface {
 	Get(ctx context.Context, resourceGroupName string, virtualNetworkName string, subnetName string, expand string) (result mgmtnetwork.Subnet, err error)
+	List(ctx context.Context, resourceGroupName string, virtualNetworkName string) (result mgmtnetwork.SubnetListResultPage, err error)
 	SubnetsClientAddons
 }
 

pkg/util/cluster/cluster.go

@@ -11,6 +11,7 @@ import (
 	"errors"
 	"fmt"
 	"math/rand"
+	"net"
 	"net/http"
 	"os"
 	"strings"
@@ -66,6 +67,8 @@ type Cluster struct {
 	vaultsClient         armkeyvault.VaultsClient
 }
 
+const GenerateSubnetMaxTries = 100
+
 func New(log *logrus.Entry, environment env.Core, ci bool) (*Cluster, error) {
 	if env.IsLocalDevelopmentMode() {
 		if err := env.ValidateVars("AZURE_FP_CLIENT_ID"); err != nil {
@@ -200,7 +203,11 @@ func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName str
 		return err
 	}
 
-	addressPrefix, masterSubnet, workerSubnet := c.generateSubnets()
+	addressPrefix, masterSubnet, workerSubnet, err := c.generateSubnets()
+
+	if err != nil {
+		return err
+	}
 
 	var kvName string
 	if len(vnetResourceGroup) > 10 {
@@ -345,22 +352,111 @@ func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName str
 	return nil
 }
 
-func (c *Cluster) generateSubnets() (vnetPrefix string, masterSubnet string, workerSubnet string) {
-	// pick a random 23 in range [10.3.0.0, 10.127.255.0]
+// ipRangesContainCIDR checks, weather any of the ipRanges overlap with the cidr string. In case cidr isn't valid, false is returned.
+func ipRangesContainCIDR(ipRanges []*net.IPNet, cidr string) (bool, error) {
+	_, cidrNet, err := net.ParseCIDR(cidr)
+	if err != nil {
+		return false, err
+	}
+
+	for _, snet := range ipRanges {
+		if snet.Contains(cidrNet.IP) || cidrNet.Contains(snet.IP) {
+			return true, nil
+		}
+	}
+	return false, nil
+}
+
+// GetIPRangesFromSubnet converts a given azure subnet to a list if IPNets.
+// Because an az subnet can cover multiple ipranges, we need to return a slice
+// instead of just a single ip range. This function never errors. If something
+// goes wrong, it instead returns an empty list.
+func GetIPRangesFromSubnet(subnet mgmtnetwork.Subnet) []*net.IPNet {
+	ipRanges := []*net.IPNet{}
+	if subnet.AddressPrefix != nil {
+		_, ipRange, err := net.ParseCIDR(*subnet.AddressPrefix)
+		if err == nil {
+			ipRanges = append(ipRanges, ipRange)
+		}
+	}
+
+	if subnet.AddressPrefixes == nil {
+		return ipRanges
+	}
+
+	for _, snetPrefix := range *subnet.AddressPrefixes {
+		_, ipRange, err := net.ParseCIDR(snetPrefix)
+		if err == nil {
+			ipRanges = append(ipRanges, ipRange)
+		}
+	}
+	return ipRanges
+}
+
+// getAllDevSubnets queries azure to retrieve all subnets assigned the vnet
+// `dev-vnet` in the current resource group
+func (c *Cluster) getAllDevSubnets() ([]mgmtnetwork.Subnet, error) {
+	allSubnets := []mgmtnetwork.Subnet{}
+	availSnetResults, err := c.subnets.List(context.Background(), c.env.ResourceGroup(), "dev-vnet")
+	if err != nil {
+		return allSubnets, err
+	}
+	allSubnets = append(allSubnets, availSnetResults.Values()...)
+	for availSnetResults.NotDone() {
+		err = availSnetResults.NextWithContext(context.Background())
+		if err != nil {
+			break
+		}
+		allSubnets = append(allSubnets, availSnetResults.Values()...)
+	}
+
+	return allSubnets, nil
+}
+
+func (c *Cluster) generateSubnets() (vnetPrefix string, masterSubnet string, workerSubnet string, err error) {
+	// pick a random 23 in range [10.3.0.0, 10.127.255.0], making sure it doesn't
+	// conflict with other subnets present in out dev-vnet
 	// 10.0.0.0/16 is used by dev-vnet to host CI
 	// 10.1.0.0/24 is used by rp-vnet to host Proxy VM
 	// 10.2.0.0/24 is used by dev-vpn-vnet to host VirtualNetworkGateway
-	var x, y int
-	// Local Dev clusters are limited to /16 dev-vnet
-	if !c.ci {
-		x, y = 0, 2*rand.Intn(128)
-	} else {
-		x, y = rand.Intn((124))+3, 2*rand.Intn(128)
+
+	allSubnets, err := c.getAllDevSubnets()
+	if err != nil {
+		c.log.Warnf("Error getting existing subnets. Continuing regardless: %v", err)
 	}
-	vnetPrefix = fmt.Sprintf("10.%d.%d.0/23", x, y)
-	masterSubnet = fmt.Sprintf("10.%d.%d.0/24", x, y)
-	workerSubnet = fmt.Sprintf("10.%d.%d.0/24", x, y+1)
-	return
+
+	ipRanges := []*net.IPNet{}
+	for _, snet := range allSubnets {
+		ipRanges = append(ipRanges, GetIPRangesFromSubnet(snet)...)
+	}
+
+	for i := 1; i < GenerateSubnetMaxTries; i++ {
+		var x, y int
+		// Local Dev clusters are limited to /16 dev-vnet
+		if !c.ci {
+			x, y = 0, 2*rand.Intn(128)
+		} else {
+			x, y = rand.Intn((124))+3, 2*rand.Intn(128)
+		}
+		c.log.Infof("Generate Subnet try: %d\n", i)
+		vnetPrefix = fmt.Sprintf("10.%d.%d.0/23", x, y)
+		masterSubnet = fmt.Sprintf("10.%d.%d.0/24", x, y)
+		workerSubnet = fmt.Sprintf("10.%d.%d.0/24", x, y+1)
+
+		masterSubnetOverlaps, err := ipRangesContainCIDR(ipRanges, workerSubnet)
+		if err != nil || masterSubnetOverlaps {
+			continue
+		}
+
+		workerSubnetOverlaps, err := ipRangesContainCIDR(ipRanges, workerSubnet)
+		if err != nil || workerSubnetOverlaps {
+			continue
+		}
+
+		return vnetPrefix, masterSubnet, workerSubnet, nil
+	}
+
+	return vnetPrefix, masterSubnet, workerSubnet, fmt.Errorf("was not able to generate master and worker subnets after %v tries", GenerateSubnetMaxTries)
 }
 
 func (c *Cluster) Delete(ctx context.Context, vnetResourceGroup, clusterName string) error {

pkg/util/mocks/azureclient/mgmt/network/network.go

@@ -516,6 +516,21 @@ func (mr *MockSubnetsClientMockRecorder) Get(arg0, arg1, arg2, arg3, arg4 interf
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockSubnetsClient)(nil).Get), arg0, arg1, arg2, arg3, arg4)
 }
 
+// List mocks base method.
+func (m *MockSubnetsClient) List(arg0 context.Context, arg1, arg2 string) (network.SubnetListResultPage, error) {
+	m.ctrl.T.Helper()
+	ret := m.ctrl.Call(m, "List", arg0, arg1, arg2)
+	ret0, _ := ret[0].(network.SubnetListResultPage)
+	ret1, _ := ret[1].(error)
+	return ret0, ret1
+}
+
+// List indicates an expected call of List.
+func (mr *MockSubnetsClientMockRecorder) List(arg0, arg1, arg2 interface{}) *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "List", reflect.TypeOf((*MockSubnetsClient)(nil).List), arg0, arg1, arg2)
+}
+
 // MockVirtualNetworksClient is a mock of VirtualNetworksClient interface.
 type MockVirtualNetworksClient struct {
 	ctrl     *gomock.Controller