From 2f292b001f5d3dab2193d74f8d8aec6b3213ef26 Mon Sep 17 00:00:00 2001 From: mrWinston Date: Thu, 18 Jul 2024 15:52:18 +0200 Subject: [PATCH 1/3] generate subnets now takes existing subnets into account --- pkg/util/azureclient/mgmt/network/subnets.go | 1 + pkg/util/cluster/cluster.go | 102 +++++++++++++++++-- 2 files changed, 93 insertions(+), 10 deletions(-) diff --git a/pkg/util/azureclient/mgmt/network/subnets.go b/pkg/util/azureclient/mgmt/network/subnets.go index 71319b0d0c3..61678697cae 100644 --- a/pkg/util/azureclient/mgmt/network/subnets.go +++ b/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 } diff --git a/pkg/util/cluster/cluster.go b/pkg/util/cluster/cluster.go index 810787ccc1b..43221031ba3 100644 --- a/pkg/util/cluster/cluster.go +++ b/pkg/util/cluster/cluster.go @@ -11,6 +11,7 @@ import ( "errors" "fmt" "math/rand" + "net" "net/http" "os" "strings" @@ -345,22 +346,103 @@ func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName str return nil } +// 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 { + _, cidrNet, err := net.ParseCIDR(cidr) + if err != nil { + return false + } + + for _, snet := range ipRanges { + if snet.Contains(cidrNet.IP) || cidrNet.Contains(snet.IP) { + return true + } + } + return false +} + +// 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.TODO(), c.env.ResourceGroup(), "dev-vnet") + if err != nil { + return allSubnets, err + } + allSubnets = append(allSubnets, availSnetResults.Values()...) + for availSnetResults.NotDone() { + err = availSnetResults.NextWithContext(context.TODO()) + if err != nil { + break + } + allSubnets = append(allSubnets, availSnetResults.Values()...) + } + + return allSubnets, 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] // 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) + } + + ipRanges := []*net.IPNet{} + for _, snet := range allSubnets { + ipRanges = append(ipRanges, GetIpRangesFromSubnet(snet)...) + } + + for i := 1; i < 100; 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) + if !ipRangesContainCIDR(ipRanges, workerSubnet) && !ipRangesContainCIDR(ipRanges, masterSubnet) { + return vnetPrefix, masterSubnet, workerSubnet + } } - 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 + + return vnetPrefix, masterSubnet, workerSubnet } func (c *Cluster) Delete(ctx context.Context, vnetResourceGroup, clusterName string) error { From 858084a19871a8d3d0bf87bff4231565fe680dbf Mon Sep 17 00:00:00 2001 From: mrWinston Date: Mon, 29 Jul 2024 14:19:50 +0200 Subject: [PATCH 2/3] incorporate feedback, add mocks for subnet client --- pkg/util/cluster/cluster.go | 55 +++++++++++++------ .../mocks/azureclient/mgmt/network/network.go | 15 +++++ 2 files changed, 52 insertions(+), 18 deletions(-) diff --git a/pkg/util/cluster/cluster.go b/pkg/util/cluster/cluster.go index 43221031ba3..f15585f6109 100644 --- a/pkg/util/cluster/cluster.go +++ b/pkg/util/cluster/cluster.go @@ -67,6 +67,9 @@ 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 { @@ -201,7 +204,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 { @@ -347,25 +354,25 @@ func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName str } // 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 { +func ipRangesContainCIDR(ipRanges []*net.IPNet, cidr string) (bool, error){ _, cidrNet, err := net.ParseCIDR(cidr) if err != nil { - return false + return false, err } for _, snet := range ipRanges { if snet.Contains(cidrNet.IP) || cidrNet.Contains(snet.IP) { - return true + return true, nil } } - return false + return false, nil } -// GetIpRangesFromSubnet converts a given azure subnet to a list if IPNets. +// 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 { +func GetIPRangesFromSubnet(subnet mgmtnetwork.Subnet) []*net.IPNet { ipRanges := []*net.IPNet{} if subnet.AddressPrefix != nil { _, ipRange, err := net.ParseCIDR(*subnet.AddressPrefix) @@ -392,13 +399,13 @@ func GetIpRangesFromSubnet(subnet mgmtnetwork.Subnet) []*net.IPNet { // `dev-vnet` in the current resource group func (c *Cluster) getAllDevSubnets() ([]mgmtnetwork.Subnet, error) { allSubnets := []mgmtnetwork.Subnet{} - availSnetResults, err := c.subnets.List(context.TODO(), c.env.ResourceGroup(), "dev-vnet") + 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.TODO()) + err = availSnetResults.NextWithContext(context.Background()) if err != nil { break } @@ -409,9 +416,10 @@ func (c *Cluster) getAllDevSubnets() ([]mgmtnetwork.Subnet, error) { } -func (c *Cluster) generateSubnets() (vnetPrefix string, masterSubnet string, workerSubnet string) { - // pick a random 23 in range [10.3.0.0, 10.127.255.0] - // 10.0.0.0/16 is used by dev-vnet to host CI +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 @@ -422,10 +430,11 @@ func (c *Cluster) generateSubnets() (vnetPrefix string, masterSubnet string, wor ipRanges := []*net.IPNet{} for _, snet := range allSubnets { - ipRanges = append(ipRanges, GetIpRangesFromSubnet(snet)...) + ipRanges = append(ipRanges, GetIPRangesFromSubnet(snet)...) } - for i := 1; i < 100; i++ { + + for i := 1; i < GenerateSubnetMaxTries; i++ { var x, y int // Local Dev clusters are limited to /16 dev-vnet if !c.ci { @@ -437,14 +446,24 @@ func (c *Cluster) generateSubnets() (vnetPrefix string, masterSubnet string, wor 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) - if !ipRangesContainCIDR(ipRanges, workerSubnet) && !ipRangesContainCIDR(ipRanges, masterSubnet) { - return vnetPrefix, masterSubnet, workerSubnet - } + + 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 + 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 { c.log.Infof("Deleting cluster %s in resource group %s", clusterName, vnetResourceGroup) var errs []error diff --git a/pkg/util/mocks/azureclient/mgmt/network/network.go b/pkg/util/mocks/azureclient/mgmt/network/network.go index e038f364540..484a98d295a 100644 --- a/pkg/util/mocks/azureclient/mgmt/network/network.go +++ b/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 From c157292ecd6c076e1b2066c01d1ac40457aaa3aa Mon Sep 17 00:00:00 2001 From: mrWinston Date: Mon, 29 Jul 2024 14:54:09 +0200 Subject: [PATCH 3/3] Fix formatting --- pkg/util/azureclient/mgmt/network/subnets.go | 2 +- pkg/util/cluster/cluster.go | 39 +++++++++----------- 2 files changed, 18 insertions(+), 23 deletions(-) diff --git a/pkg/util/azureclient/mgmt/network/subnets.go b/pkg/util/azureclient/mgmt/network/subnets.go index 61678697cae..ec51b67a544 100644 --- a/pkg/util/azureclient/mgmt/network/subnets.go +++ b/pkg/util/azureclient/mgmt/network/subnets.go @@ -15,7 +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) + List(ctx context.Context, resourceGroupName string, virtualNetworkName string) (result mgmtnetwork.SubnetListResultPage, err error) SubnetsClientAddons } diff --git a/pkg/util/cluster/cluster.go b/pkg/util/cluster/cluster.go index f15585f6109..589f8e80b90 100644 --- a/pkg/util/cluster/cluster.go +++ b/pkg/util/cluster/cluster.go @@ -67,7 +67,6 @@ type Cluster struct { vaultsClient armkeyvault.VaultsClient } - const GenerateSubnetMaxTries = 100 func New(log *logrus.Entry, environment env.Core, ci bool) (*Cluster, error) { @@ -205,10 +204,10 @@ func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName str } addressPrefix, masterSubnet, workerSubnet, err := c.generateSubnets() - - if err != nil { - return err - } + + if err != nil { + return err + } var kvName string if len(vnetResourceGroup) > 10 { @@ -354,7 +353,7 @@ func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName str } // 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){ +func ipRangesContainCIDR(ipRanges []*net.IPNet, cidr string) (bool, error) { _, cidrNet, err := net.ParseCIDR(cidr) if err != nil { return false, err @@ -394,7 +393,6 @@ func GetIPRangesFromSubnet(subnet mgmtnetwork.Subnet) []*net.IPNet { 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) { @@ -415,11 +413,10 @@ func (c *Cluster) getAllDevSubnets() ([]mgmtnetwork.Subnet, error) { 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 + // 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 @@ -433,7 +430,6 @@ func (c *Cluster) generateSubnets() (vnetPrefix string, masterSubnet string, wor ipRanges = append(ipRanges, GetIPRangesFromSubnet(snet)...) } - for i := 1; i < GenerateSubnetMaxTries; i++ { var x, y int // Local Dev clusters are limited to /16 dev-vnet @@ -447,23 +443,22 @@ func (c *Cluster) generateSubnets() (vnetPrefix string, masterSubnet string, wor 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 - } + masterSubnetOverlaps, err := ipRangesContainCIDR(ipRanges, workerSubnet) + if err != nil || masterSubnetOverlaps { + continue + } - workerSubnetOverlaps, err := ipRangesContainCIDR(ipRanges, workerSubnet) - if err != nil || workerSubnetOverlaps { - 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 { c.log.Infof("Deleting cluster %s in resource group %s", clusterName, vnetResourceGroup) var errs []error