From 381ca5101697b6d4583477a506771ee9cb1cd4b1 Mon Sep 17 00:00:00 2001 From: junsun-ms <33297523+junsun-ms@users.noreply.github.com> Date: Tue, 20 Feb 2018 17:06:34 -0800 Subject: [PATCH] Revert static IP allocation logic in Azure CNI, PR 1966. --- docs/kubernetes/features.md | 20 ++++++- .../release/default/definition.json | 2 +- examples/vnet/kubernetesvnet-azure-cni.json | 2 +- parts/k8s/kubernetesagentresourcesvmas.t | 3 - parts/k8s/kubernetesagentvars.t | 1 - parts/k8s/kubernetesmasterresources.t | 7 +-- parts/k8s/kubernetesmastervars.t | 4 -- parts/k8s/kuberneteswinagentresourcesvmas.t | 3 - pkg/acsengine/const.go | 2 +- pkg/acsengine/defaults.go | 30 ++++++---- pkg/acsengine/engine.go | 58 ------------------- 11 files changed, 41 insertions(+), 91 deletions(-) diff --git a/docs/kubernetes/features.md b/docs/kubernetes/features.md index ad2239d141..eee4e6290e 100644 --- a/docs/kubernetes/features.md +++ b/docs/kubernetes/features.md @@ -169,15 +169,29 @@ Per default Calico still allows all communication within the cluster. Using Kube *Note: Custom VNET for Kubernetes Windows cluster has a [known issue](https://github.com/Azure/acs-engine/issues/1767).* -ACS Engine supports deploying into an existing VNET. Operators must specify the ARM path/id of Subnets for the `masterProfile` and any `agentPoolProfiles`, as well as the first IP address to use for static IP allocation in `firstConsecutiveStaticIP`. Please note that in any azure subnet, the first four and the last ip address is reserved and can not be used. Additionally, each POD now gets the IP address from the Subnet. As a result, for the master nodes, enough IP addresses (equal to `ipAddressCount`) should be available beyond `firstConsecutiveStaticIP`. By default, the `ipAddressCount` has a value of 30, and can be changed if desired. Furthermore, to prevent source address NAT'ing within the VNET, we assign to the `vnetCidr` property in `masterProfile` the CIDR block that represents the usable address space in the existing VNET. +ACS Engine supports deploying into an existing VNET. Operators must specify the ARM path/id of Subnets for the `masterProfile` and any `agentPoolProfiles`, as well as the first IP address to use for IP static IP allocation in `firstConsecutiveStaticIP`. Additionally, to prevent source address NAT'ing within the VNET, we assign to the `vnetCidr` property in `masterProfile` the CIDR block that represents the usable address space in the existing VNET. -See below profiles as an example: +Depending upon the size of the VNET address space, during deployment, it is possible to experience IP address assignment collision between the required Kubernetes static IPs (one each per master and one for the API server load balancer, if more than one masters) and Azure CNI-assigned dynamic IPs (one for each NIC on the agent nodes). In practice, the larger the VNET the less likely this is to happen; some detail, and then a guideline. + +First, the detail: + +* Azure CNI assigns dynamic IP addresses from the "beginning" of the subnet IP address space (specifically, it looks for available addresses starting at ".4" ["10.0.0.4" in a "10.0.0.0/24" network]) +* acs-engine will require a range of up to 16 unused IP addresses in multi-master scenarios (1 per master for up to 5 masters, and then the next 10 IP addresses immediately following the "last" master for headroom reservation, and finally 1 more for the load balancer immediately adjacent to the afore-described _n_ masters+10 sequence) to successfully scaffold the network stack for your cluster + +A guideline that will remove the danger of IP address allocation collision during deployment: + +* If possible, assign to the `firstConsecutiveStaticIP` configuration property an IP address that is near the "end" of the available IP address space in the desired subnet. + * For example, if the desired subnet is a `/24`, choose the "239" address in that network space + +In larger subnets (e.g., `/16`) it's not as practically useful to push static IP assignment to the very "end" of large subnet, but as long as it's not in the "first" `/24` (for example) your deployment will be resilient to this edge case behavior. + +Before provisioning, modify the `masterProfile` and `agentPoolProfiles` to match the above requirements, with the below being a representative example: ```json "masterProfile": { ... "vnetSubnetId": "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME/subnets/MASTER_SUBNET_NAME", - "firstConsecutiveStaticIP": "10.239.0.5", + "firstConsecutiveStaticIP": "10.239.255.239", "vnetCidr": "10.239.0.0/16", ... }, diff --git a/examples/e2e-tests/kubernetes/release/default/definition.json b/examples/e2e-tests/kubernetes/release/default/definition.json index 1137239e88..d2fd382bc2 100644 --- a/examples/e2e-tests/kubernetes/release/default/definition.json +++ b/examples/e2e-tests/kubernetes/release/default/definition.json @@ -57,7 +57,7 @@ "vmSize": "Standard_D2_v2", "OSDiskSizeGB": 200, "vnetSubnetId": "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME/subnets/SUBNET_NAME", - "firstConsecutiveStaticIP": "10.239.0.4", + "firstConsecutiveStaticIP": "10.239.255.239", "vnetCidr": "10.239.0.0/16" }, "agentPoolProfiles": [ diff --git a/examples/vnet/kubernetesvnet-azure-cni.json b/examples/vnet/kubernetesvnet-azure-cni.json index 9450ef164e..f2cccee088 100644 --- a/examples/vnet/kubernetesvnet-azure-cni.json +++ b/examples/vnet/kubernetesvnet-azure-cni.json @@ -9,7 +9,7 @@ "dnsPrefix": "test", "vmSize": "Standard_D2_v2", "vnetSubnetId": "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME/subnets/SUBNET_NAME", - "firstConsecutiveStaticIP": "10.239.255.10", + "firstConsecutiveStaticIP": "10.239.255.239", "vnetCidr": "10.239.0.0/16" }, "agentPoolProfiles": [ diff --git a/parts/k8s/kubernetesagentresourcesvmas.t b/parts/k8s/kubernetesagentresourcesvmas.t index cf169a2c91..7788127923 100644 --- a/parts/k8s/kubernetesagentresourcesvmas.t +++ b/parts/k8s/kubernetesagentresourcesvmas.t @@ -9,9 +9,6 @@ "[variables('nsgID')]" {{else}} "[variables('vnetID')]" -{{end}} -{{range $masterOffset := loop 1 GetMasterCount}} - ,"[concat(variables('masterNICNamePrefix'), sub({{$masterOffset}}, 1))]" {{end}} ], "location": "[variables('location')]", diff --git a/parts/k8s/kubernetesagentvars.t b/parts/k8s/kubernetesagentvars.t index 8e435826d8..4e3261df78 100644 --- a/parts/k8s/kubernetesagentvars.t +++ b/parts/k8s/kubernetesagentvars.t @@ -24,5 +24,4 @@ "{{.Name}}osImageSKU": "[parameters('{{.Name}}osImageSKU')]", "{{.Name}}osImagePublisher": "[parameters('{{.Name}}osImagePublisher')]", "{{.Name}}osImageVersion": "[parameters('{{.Name}}osImageVersion')]", - "masterNICNamePrefix": "[concat(variables('orchestratorName'), '-master-', variables('nameSuffix'), '-', 'nic-')]", diff --git a/parts/k8s/kubernetesmasterresources.t b/parts/k8s/kubernetesmasterresources.t index 50cb41e553..25665806a2 100644 --- a/parts/k8s/kubernetesmasterresources.t +++ b/parts/k8s/kubernetesmasterresources.t @@ -338,14 +338,13 @@ } } {{if IsAzureCNI}} - {{range $seq := loop 1 .MasterProfile.IPAddressCount}} + {{range $seq := loop 2 .MasterProfile.IPAddressCount}} , { - "name": "[concat('ipconfig', add({{$seq}}, 1))]", + "name": "ipconfig{{$seq}}", "properties": { - "privateIPAddress": "[variables('masterSecondaryAddrs')[add(mul(copyIndex(variables('masterOffset')), variables('ipAddressCount')), sub({{$seq}}, 1))]]", "primary": false, - "privateIPAllocationMethod": "Static", + "privateIPAllocationMethod": "Dynamic", "subnet": { "id": "[variables('vnetSubnetID')]" } diff --git a/parts/k8s/kubernetesmastervars.t b/parts/k8s/kubernetesmastervars.t index 25c86c303c..4a7a0c651a 100644 --- a/parts/k8s/kubernetesmastervars.t +++ b/parts/k8s/kubernetesmastervars.t @@ -218,7 +218,6 @@ "orchestratorNameVersionTag": "{{.OrchestratorProfile.OrchestratorType}}:{{.OrchestratorProfile.OrchestratorVersion}}", {{if IsAzureCNI}} "allocateNodeCidrs": false, - "ipAddressCount": {{.MasterProfile.IPAddressCount}}, {{else}} "allocateNodeCidrs": true, {{end}} @@ -305,9 +304,6 @@ "[concat(variables('masterFirstAddrPrefix'), add(3, int(variables('masterFirstAddrOctet4'))))]", "[concat(variables('masterFirstAddrPrefix'), add(4, int(variables('masterFirstAddrOctet4'))))]" ], -{{if IsAzureCNI}} - "masterSecondaryAddrs": [{{GetMasterSecondaryIP}}], -{{end}} "masterEtcdServerPort": {{GetMasterEtcdServerPort}}, "masterEtcdClientPort": {{GetMasterEtcdClientPort}}, "masterEtcdPeerURLs":[ diff --git a/parts/k8s/kuberneteswinagentresourcesvmas.t b/parts/k8s/kuberneteswinagentresourcesvmas.t index 41426b31cb..7f65df9e56 100644 --- a/parts/k8s/kuberneteswinagentresourcesvmas.t +++ b/parts/k8s/kuberneteswinagentresourcesvmas.t @@ -9,9 +9,6 @@ "[variables('nsgID')]" {{else}} "[variables('vnetID')]" -{{end}} -{{range $masterOffset := loop 1 GetMasterCount}} - ,"[concat(variables('masterNICNamePrefix'), sub({{$masterOffset}}, 1))]" {{end}} ], "location": "[variables('location')]", diff --git a/pkg/acsengine/const.go b/pkg/acsengine/const.go index 20c5ea0439..c1377e9ee0 100644 --- a/pkg/acsengine/const.go +++ b/pkg/acsengine/const.go @@ -18,7 +18,7 @@ const ( // DefaultNonMasqueradeCidr specifies the subnet that should not be masqueraded on host DefaultNonMasqueradeCidr = "10.0.0.0/8" // DefaultFirstConsecutiveKubernetesStaticIP specifies the static IP address on Kubernetes master 0 - DefaultFirstConsecutiveKubernetesStaticIP = "10.240.0.4" + DefaultFirstConsecutiveKubernetesStaticIP = "10.240.255.5" // DefaultAgentSubnetTemplate specifies a default agent subnet DefaultAgentSubnetTemplate = "10.%d.0.0/16" // DefaultKubernetesSubnet specifies the default subnet used for all masters, agents and pods diff --git a/pkg/acsengine/defaults.go b/pkg/acsengine/defaults.go index 178526a0fd..387ddbdb6e 100644 --- a/pkg/acsengine/defaults.go +++ b/pkg/acsengine/defaults.go @@ -2,7 +2,6 @@ package acsengine import ( "bytes" - "encoding/binary" "fmt" "net" "sort" @@ -488,6 +487,9 @@ func setMasterNetworkDefaults(a *api.Properties) { // Set the default number of IP addresses allocated for masters. if a.MasterProfile.IPAddressCount == 0 { + // Allocate one IP address for the node. + a.MasterProfile.IPAddressCount = 1 + // Allocate IP addresses for pods if VNET integration is enabled. if a.OrchestratorProfile.IsAzureCNI() { if a.OrchestratorProfile.OrchestratorType == api.Kubernetes { @@ -685,23 +687,27 @@ func certsAlreadyPresent(c *api.CertificateProfile, m int) map[string]bool { // getFirstConsecutiveStaticIPAddress returns the first static IP address of the given subnet. func getFirstConsecutiveStaticIPAddress(subnetStr string) string { - ip, _, err := net.ParseCIDR(subnetStr) + _, subnet, err := net.ParseCIDR(subnetStr) if err != nil { return DefaultFirstConsecutiveKubernetesStaticIP } - ipv4 := ip.To4() - if ipv4 == nil { - return DefaultFirstConsecutiveKubernetesStaticIP - } + // Find the first and last octet of the host bits. + ones, bits := subnet.Mask.Size() + firstOctet := ones / 8 + lastOctet := bits/8 - 1 - ipint := binary.BigEndian.Uint32(ipv4) + // Set the remaining host bits in the first octet. + subnet.IP[firstOctet] |= (1 << byte((8 - (ones % 8)))) - 1 + + // Fill the intermediate octets with 1s and last octet with offset. This is done so to match + // the existing behavior of allocating static IP addresses from the last /24 of the subnet. + for i := firstOctet + 1; i < lastOctet; i++ { + subnet.IP[i] = 255 + } + subnet.IP[lastOctet] = DefaultKubernetesFirstConsecutiveStaticIPOffset - // First 4 IPs will be reserved in Azure and cannot use. - ipint += 4 - startIP := make(net.IP, 4) - binary.BigEndian.PutUint32(startIP, ipint) - return startIP.String() + return subnet.IP.String() } func getAddonsIndexByName(addons []api.KubernetesAddon, name string) int { diff --git a/pkg/acsengine/engine.go b/pkg/acsengine/engine.go index 96f113e960..0ae860e5f7 100644 --- a/pkg/acsengine/engine.go +++ b/pkg/acsengine/engine.go @@ -4,7 +4,6 @@ import ( "bytes" "compress/gzip" "encoding/base64" - "encoding/binary" "encoding/json" "errors" "fmt" @@ -12,7 +11,6 @@ import ( "io/ioutil" "log" "math/rand" - "net" "net/http" "regexp" "runtime/debug" @@ -877,62 +875,6 @@ func (t *TemplateGenerator) getTemplateFuncMap(cs *api.ContainerService) templat } return strings.TrimSuffix(buf.String(), ", ") }, - "GetMasterCount": func() int { - masterProfile := cs.Properties.MasterProfile - if masterProfile == nil { - return 0 - } - return masterProfile.Count - }, - "GetMasterSecondaryIP": func() string { - var ips string - var ipint uint32 - - profile := cs.Properties.MasterProfile - if profile == nil { - return "" - } - - ip := net.ParseIP(profile.FirstConsecutiveStaticIP) - ipv4 := ip.To4() - if ipv4 != nil { - ipint = binary.BigEndian.Uint32(ipv4) - } else { - log.Fatalf("Net IP To4() function returns nil") - } - - // Make space for first few IPs. - ipint += uint32(profile.Count) + uint32(DefaultInternalLbStaticIPOffset) - 1 - startIP := make(net.IP, 4) - binary.BigEndian.PutUint32(startIP, ipint) - - totalIPCount := profile.Count * profile.IPAddressCount - ipint = 0 - - // Generate All secondary IPs that master will use. - for count := totalIPCount; count > 0; count-- { - ipv4 = startIP.To4() - if ipv4 != nil { - ipint = binary.BigEndian.Uint32(ipv4) - } else { - log.Fatalf("Net IP To4() function returns nil for startIP") - } - - ipint++ - newIP := make(net.IP, 4) - binary.BigEndian.PutUint32(newIP, ipint) - - if ips != "" { - ips = ips + "," + "\"" + newIP.String() + "\"" - } else { - ips = "\"" + newIP.String() + "\"" - } - - startIP = newIP - } - - return ips - }, "RequiresFakeAgentOutput": func() bool { return cs.Properties.OrchestratorProfile.OrchestratorType == api.Kubernetes },