From 75ea002bdaa72cb0c46c70be65144f96d6f3ec37 Mon Sep 17 00:00:00 2001 From: Pengfei Ni Date: Mon, 4 Mar 2019 14:48:14 +0800 Subject: [PATCH] Convert resource group name in Azure providerID to lower case --- .../cloudprovider/azure/azure_agent_pool.go | 16 ++++-- .../cloudprovider/azure/azure_cache.go | 6 +- .../azure/azure_cloud_provider.go | 6 -- .../azure/azure_container_service_pool.go | 10 +++- .../cloudprovider/azure/azure_fakes.go | 2 +- .../cloudprovider/azure/azure_scale_set.go | 11 +++- .../cloudprovider/azure/azure_util.go | 18 +++++- .../cloudprovider/azure/azure_util_test.go | 55 +++++++++++++++++++ 8 files changed, 105 insertions(+), 19 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/azure/azure_agent_pool.go b/cluster-autoscaler/cloudprovider/azure/azure_agent_pool.go index 6c9a7336016b..8378442c8c6b 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_agent_pool.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_agent_pool.go @@ -133,7 +133,11 @@ func (as *AgentPool) GetVMIndexes() ([]int, map[int]string, error) { } indexes = append(indexes, index) - indexToVM[index] = "azure://" + strings.ToLower(*instance.ID) + resourceID, err := convertResourceGroupNameToLower("azure://" + *instance.ID) + if err != nil { + return nil, nil, err + } + indexToVM[index] = resourceID } sortedIndexes := sort.IntSlice(indexes) @@ -398,9 +402,13 @@ func (as *AgentPool) Nodes() ([]cloudprovider.Instance, error) { continue } - // To keep consistent with providerID from kubernetes cloud provider, do not convert ID to lower case. - name := "azure://" + strings.ToLower(*instance.ID) - nodes = append(nodes, cloudprovider.Instance{Id: name}) + // To keep consistent with providerID from kubernetes cloud provider, convert + // resourceGroupName in the ID to lower case. + resourceID, err := convertResourceGroupNameToLower("azure://" + *instance.ID) + if err != nil { + return nil, err + } + nodes = append(nodes, cloudprovider.Instance{Id: resourceID}) } return nodes, nil diff --git a/cluster-autoscaler/cloudprovider/azure/azure_cache.go b/cluster-autoscaler/cloudprovider/azure/azure_cache.go index 6f4fa56bb9ba..9a598bd8490a 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_cache.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_cache.go @@ -118,7 +118,11 @@ func (m *asgCache) FindForInstance(instance *azureRef, vmType string) (cloudprov m.mutex.Lock() defer m.mutex.Unlock() - inst := azureRef{Name: strings.ToLower(instance.Name)} + resourceID, err := convertResourceGroupNameToLower(instance.Name) + if err != nil { + return nil, err + } + inst := azureRef{Name: resourceID} if m.notInRegisteredAsg[inst] { // We already know we don't own this instance. Return early and avoid // additional calls. diff --git a/cluster-autoscaler/cloudprovider/azure/azure_cloud_provider.go b/cluster-autoscaler/cloudprovider/azure/azure_cloud_provider.go index 15dd6aa57357..2a112ecd61fc 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_cloud_provider.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_cloud_provider.go @@ -19,7 +19,6 @@ package azure import ( "io" "os" - "strings" "k8s.io/klog" @@ -111,11 +110,6 @@ func (azure *AzureCloudProvider) Refresh() error { return azure.azureManager.Refresh() } -// GetInstanceID gets the instance ID for the specified node. -func (azure *AzureCloudProvider) GetInstanceID(node *apiv1.Node) string { - return strings.ToLower(node.Spec.ProviderID) -} - // azureRef contains a reference to some entity in Azure world. type azureRef struct { Name string diff --git a/cluster-autoscaler/cloudprovider/azure/azure_container_service_pool.go b/cluster-autoscaler/cloudprovider/azure/azure_container_service_pool.go index cb382baa453b..00ea08957b3a 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_container_service_pool.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_container_service_pool.go @@ -270,7 +270,7 @@ func (agentPool *ContainerServiceAgentPool) SetNodeCount(count int) (err error) func (agentPool *ContainerServiceAgentPool) GetProviderID(name string) string { //TODO: come with a generic way to make it work with provider id formats // in different version of k8s. - return "azure://" + strings.ToLower(name) + return "azure://" + name } //GetName extracts the name of the node (a format which underlying cloud service understands) @@ -419,7 +419,13 @@ func (agentPool *ContainerServiceAgentPool) GetNodes() ([]string, error) { for _, node := range vmList { klog.V(5).Infof("Node Name: %s, ID: %s", *node.Name, *node.ID) if agentPool.IsContainerServiceNode(node.Tags) { - providerID := agentPool.GetProviderID(*node.ID) + providerID, err := convertResourceGroupNameToLower(agentPool.GetProviderID(*node.ID)) + if err != nil { + // This shouldn't happen. Log a waring message for tracking. + klog.Warningf("GetNodes.convertResourceGroupNameToLower failed with error: %v", err) + continue + } + klog.V(5).Infof("Returning back the providerID: %s", providerID) nodeArray = append(nodeArray, providerID) } diff --git a/cluster-autoscaler/cloudprovider/azure/azure_fakes.go b/cluster-autoscaler/cloudprovider/azure/azure_fakes.go index 395f01a4c0b4..dd1f532bb88c 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_fakes.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_fakes.go @@ -30,7 +30,7 @@ import ( ) const ( - fakeVirtualMachineScaleSetVMID = "/subscriptions/test-subscription-id/resourcegroups/test-asg/providers/microsoft.compute/virtualmachinescalesets/agents/virtualmachines/0" + fakeVirtualMachineScaleSetVMID = "/subscriptions/test-subscription-id/resourceGroups/test-asg/providers/Microsoft.Compute/virtualMachineScaleSets/agents/virtualMachines/0" ) // VirtualMachineScaleSetsClientMock mocks for VirtualMachineScaleSetsClient. diff --git a/cluster-autoscaler/cloudprovider/azure/azure_scale_set.go b/cluster-autoscaler/cloudprovider/azure/azure_scale_set.go index 72b555afba33..7118aa935634 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_scale_set.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_scale_set.go @@ -211,7 +211,14 @@ func (scaleSet *ScaleSet) GetScaleSetVms() ([]string, error) { continue } - allVMs = append(allVMs, *vm.ID) + resourceID, err := convertResourceGroupNameToLower(*vm.ID) + if err != nil { + // This shouldn't happen. Log a waring message for tracking. + klog.Warningf("GetScaleSetVms.convertResourceGroupNameToLower failed with error: %v", err) + continue + } + + allVMs = append(allVMs, resourceID) } return allVMs, nil @@ -456,7 +463,7 @@ func (scaleSet *ScaleSet) Nodes() ([]cloudprovider.Instance, error) { instances := make([]cloudprovider.Instance, 0, len(vms)) for i := range vms { - name := "azure://" + strings.ToLower(vms[i]) + name := "azure://" + vms[i] instances = append(instances, cloudprovider.Instance{Id: name}) } diff --git a/cluster-autoscaler/cloudprovider/azure/azure_util.go b/cluster-autoscaler/cloudprovider/azure/azure_util.go index 61892118a321..edb44940ac79 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_util.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_util.go @@ -78,9 +78,10 @@ const ( ) var ( - vmnameLinuxRegexp = regexp.MustCompile(k8sLinuxVMNamingFormat) - vmnameWindowsRegexp = regexp.MustCompile(k8sWindowsVMNamingFormat) - oldvmnameWindowsRegexp = regexp.MustCompile(k8sWindowsOldVMNamingFormat) + vmnameLinuxRegexp = regexp.MustCompile(k8sLinuxVMNamingFormat) + vmnameWindowsRegexp = regexp.MustCompile(k8sWindowsVMNamingFormat) + oldvmnameWindowsRegexp = regexp.MustCompile(k8sWindowsOldVMNamingFormat) + azureResourceGroupNameRE = regexp.MustCompile(`.*/subscriptions/(?:.*)/resourceGroups/(.+)/providers/(?:.*)`) ) //AzUtil consists of utility functions which utilizes clients to different services. @@ -628,3 +629,14 @@ func isSuccessHTTPResponse(resp *http.Response, err error) (isSuccess bool, real // This shouldn't happen, it only ensures all exceptions are handled. return false, fmt.Errorf("failed with unknown error") } + +// convertResourceGroupNameToLower converts the resource group name in the resource ID to be lowered. +func convertResourceGroupNameToLower(resourceID string) (string, error) { + matches := azureResourceGroupNameRE.FindStringSubmatch(resourceID) + if len(matches) != 2 { + return "", fmt.Errorf("%q isn't in Azure resource ID format", resourceID) + } + + resourceGroup := matches[1] + return strings.Replace(resourceID, resourceGroup, strings.ToLower(resourceGroup), 1), nil +} diff --git a/cluster-autoscaler/cloudprovider/azure/azure_util_test.go b/cluster-autoscaler/cloudprovider/azure/azure_util_test.go index c93cb0f3825f..05efdb0f0fe3 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_util_test.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_util_test.go @@ -189,3 +189,58 @@ func TestIsSuccessResponse(t *testing.T) { assert.Equal(t, test.expectedError, realError, "[%s] expected: %v, saw: %v", test.name, realError, test.expectedError) } } +func TestConvertResourceGroupNameToLower(t *testing.T) { + tests := []struct { + desc string + resourceID string + expected string + expectError bool + }{ + { + desc: "empty string should report error", + resourceID: "", + expectError: true, + }, + { + desc: "resourceID not in Azure format should report error", + resourceID: "invalid-id", + expectError: true, + }, + { + desc: "providerID not in Azure format should report error", + resourceID: "azure://invalid-id", + expectError: true, + }, + { + desc: "resource group name in VM providerID should be converted", + resourceID: "azure:///subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myResourceGroupName/providers/Microsoft.Compute/virtualMachines/k8s-agent-AAAAAAAA-0", + expected: "azure:///subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myresourcegroupname/providers/Microsoft.Compute/virtualMachines/k8s-agent-AAAAAAAA-0", + }, + { + desc: "resource group name in VM resourceID should be converted", + resourceID: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myResourceGroupName/providers/Microsoft.Compute/virtualMachines/k8s-agent-AAAAAAAA-0", + expected: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myresourcegroupname/providers/Microsoft.Compute/virtualMachines/k8s-agent-AAAAAAAA-0", + }, + { + desc: "resource group name in VMSS providerID should be converted", + resourceID: "azure:///subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myResourceGroupName/providers/Microsoft.Compute/virtualMachineScaleSets/myScaleSetName/virtualMachines/156", + expected: "azure:///subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myresourcegroupname/providers/Microsoft.Compute/virtualMachineScaleSets/myScaleSetName/virtualMachines/156", + }, + { + desc: "resource group name in VMSS resourceID should be converted", + resourceID: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myResourceGroupName/providers/Microsoft.Compute/virtualMachineScaleSets/myScaleSetName/virtualMachines/156", + expected: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myresourcegroupname/providers/Microsoft.Compute/virtualMachineScaleSets/myScaleSetName/virtualMachines/156", + }, + } + + for _, test := range tests { + real, err := convertResourceGroupNameToLower(test.resourceID) + if test.expectError { + assert.NotNil(t, err, test.desc) + continue + } + + assert.Nil(t, err, test.desc) + assert.Equal(t, test.expected, real, test.desc) + } +}