From 3f53faa40cf511f3c760a3dab46636ecee630e3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Wei=C3=9Fe?= Date: Wed, 27 Sep 2023 12:21:56 +0200 Subject: [PATCH 1/2] Fix incorrect parsing of topology key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Weiße --- pkg/azuredisk/controllerserver.go | 7 ++++++- pkg/azureutils/azure_disk_utils.go | 14 +++++++++++++- pkg/azureutils/azure_disk_utils_test.go | 24 +++++++++++++++++++++++- 3 files changed, 42 insertions(+), 3 deletions(-) diff --git a/pkg/azuredisk/controllerserver.go b/pkg/azuredisk/controllerserver.go index 36c06da00..b2523f420 100644 --- a/pkg/azuredisk/controllerserver.go +++ b/pkg/azuredisk/controllerserver.go @@ -162,7 +162,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) // make volume scheduled on all 3 availability zones for i := 1; i <= 3; i++ { topology := &csi.Topology{ - Segments: map[string]string{topologyKey: fmt.Sprintf("%s-%d", diskParams.Location, i)}, + Segments: map[string]string{topologyKey: fmt.Sprintf("%s-%d", azureutils.LowerCaseRegion(diskParams.Location), i)}, } accessibleTopology = append(accessibleTopology, topology) } @@ -179,6 +179,11 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) } } + // convert diskZone to Azure compatible format + if diskZone != "" { + diskZone = azureutils.UpperCaseZone(diskParams.Location, diskZone) + } + if d.enableDiskCapacityCheck { if ok, err := d.checkDiskCapacity(ctx, diskParams.SubscriptionID, diskParams.ResourceGroup, diskParams.DiskName, requestGiB); !ok { return nil, err diff --git a/pkg/azureutils/azure_disk_utils.go b/pkg/azureutils/azure_disk_utils.go index 79b14c432..74029baf4 100644 --- a/pkg/azureutils/azure_disk_utils.go +++ b/pkg/azureutils/azure_disk_utils.go @@ -442,7 +442,7 @@ func IsValidAvailabilityZone(zone, region string) bool { index := strings.Index(zone, "-") return index > 0 && index < len(zone)-1 } - return strings.HasPrefix(zone, fmt.Sprintf("%s-", region)) + return strings.HasPrefix(zone, fmt.Sprintf("%s-", LowerCaseRegion(region))) } func IsValidDiskURI(diskURI string) error { @@ -718,6 +718,18 @@ func PickAvailabilityZone(requirement *csi.TopologyRequirement, region, topology return "" } +// LowerCaseRegion replaces a region string containing upper case characters and whitespaces with lowercase characters and no whitespaces. +// This is required because Kubernetes does not allow whitespaces or upper case characters as label values. +func LowerCaseRegion(region string) string { + return strings.ToLower(strings.ReplaceAll(region, " ", "")) +} + +// UpperCaseZone converts a Kubernetes conformant zone string to a format compatible with Azure. +func UpperCaseZone(upperCaseRegion string, lowerCaseZone string) string { + zoneSuffix := strings.TrimPrefix(lowerCaseZone, LowerCaseRegion(upperCaseRegion)) + return upperCaseRegion + zoneSuffix +} + func checkDiskName(diskName string) bool { length := len(diskName) diff --git a/pkg/azureutils/azure_disk_utils_test.go b/pkg/azureutils/azure_disk_utils_test.go index cb917c43b..f15edae1e 100644 --- a/pkg/azureutils/azure_disk_utils_test.go +++ b/pkg/azureutils/azure_disk_utils_test.go @@ -910,13 +910,14 @@ func TestIsAvailabilityZone(t *testing.T) { expected bool }{ {"empty string should return false", "", "eastus", false}, - {"wrong farmat should return false", "123", "eastus", false}, + {"wrong format should return false", "123", "eastus", false}, {"wrong location should return false", "chinanorth-1", "eastus", false}, {"correct zone should return true", "eastus-1", "eastus", true}, {"empty location should return true", "eastus-1", "", true}, {"empty location with fault domain should return false", "1", "", false}, {"empty location with wrong format should return false", "-1", "", false}, {"empty location with wrong format should return false", "eastus-", "", false}, + {"upper case format with white space should return true", "eastus-1 ", "East US", true}, } for _, test := range tests { @@ -1681,6 +1682,27 @@ func TestPickAvailabilityZone(t *testing.T) { } }, }, + { + name: "upper case region", + testFunc: func(t *testing.T) { + expectedresponse := "testregion-01" + region := "Test Region" + mp := make(map[string]string) + mp["N/A"] = "testregion-01" + topology := &csi.Topology{ + Segments: mp, + } + topologies := []*csi.Topology{} + topologies = append(topologies, topology) + req := &csi.TopologyRequirement{ + Preferred: topologies, + } + actualresponse := PickAvailabilityZone(req, region, "N/A") + if !reflect.DeepEqual(expectedresponse, actualresponse) { + t.Errorf("actualresponse: (%v), expectedresponse: (%v)", actualresponse, expectedresponse) + } + }, + }, } for _, tc := range testCases { t.Run(tc.name, tc.testFunc) From 0cff633684b471ce086c7f2ef3c773d6758b2379 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Wei=C3=9Fe?= Date: Thu, 28 Sep 2023 11:25:26 +0200 Subject: [PATCH 2/2] Ensure correct Location format when setting up Driver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Weiße --- pkg/azuredisk/controllerserver.go | 7 +------ pkg/azureutils/azure_disk_utils.go | 19 ++++++------------- pkg/azureutils/azure_disk_utils_test.go | 24 +----------------------- 3 files changed, 8 insertions(+), 42 deletions(-) diff --git a/pkg/azuredisk/controllerserver.go b/pkg/azuredisk/controllerserver.go index b2523f420..36c06da00 100644 --- a/pkg/azuredisk/controllerserver.go +++ b/pkg/azuredisk/controllerserver.go @@ -162,7 +162,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) // make volume scheduled on all 3 availability zones for i := 1; i <= 3; i++ { topology := &csi.Topology{ - Segments: map[string]string{topologyKey: fmt.Sprintf("%s-%d", azureutils.LowerCaseRegion(diskParams.Location), i)}, + Segments: map[string]string{topologyKey: fmt.Sprintf("%s-%d", diskParams.Location, i)}, } accessibleTopology = append(accessibleTopology, topology) } @@ -179,11 +179,6 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) } } - // convert diskZone to Azure compatible format - if diskZone != "" { - diskZone = azureutils.UpperCaseZone(diskParams.Location, diskZone) - } - if d.enableDiskCapacityCheck { if ok, err := d.checkDiskCapacity(ctx, diskParams.SubscriptionID, diskParams.ResourceGroup, diskParams.DiskName, requestGiB); !ok { return nil, err diff --git a/pkg/azureutils/azure_disk_utils.go b/pkg/azureutils/azure_disk_utils.go index 74029baf4..528fd9195 100644 --- a/pkg/azureutils/azure_disk_utils.go +++ b/pkg/azureutils/azure_disk_utils.go @@ -222,6 +222,11 @@ func GetCloudProviderFromClient(ctx context.Context, kubeClient *clientset.Clien return nil, fmt.Errorf("no cloud config provided, error: %v", err) } } else { + // Location may be either upper case with spaces (e.g. "East US") or lower case without spaces (e.g. "eastus") + // Kubernetes does not allow whitespaces or upper case characters in label values, e.g. for topology keys + // ensure Kubernetes compatible format for Location by enforcing lowercase-no-space format + config.Location = strings.ToLower(strings.ReplaceAll(config.Location, " ", "")) + // disable disk related rate limit config.DiskRateLimit = &azclients.RateLimitConfig{ CloudProviderRateLimit: false, @@ -442,7 +447,7 @@ func IsValidAvailabilityZone(zone, region string) bool { index := strings.Index(zone, "-") return index > 0 && index < len(zone)-1 } - return strings.HasPrefix(zone, fmt.Sprintf("%s-", LowerCaseRegion(region))) + return strings.HasPrefix(zone, fmt.Sprintf("%s-", region)) } func IsValidDiskURI(diskURI string) error { @@ -718,18 +723,6 @@ func PickAvailabilityZone(requirement *csi.TopologyRequirement, region, topology return "" } -// LowerCaseRegion replaces a region string containing upper case characters and whitespaces with lowercase characters and no whitespaces. -// This is required because Kubernetes does not allow whitespaces or upper case characters as label values. -func LowerCaseRegion(region string) string { - return strings.ToLower(strings.ReplaceAll(region, " ", "")) -} - -// UpperCaseZone converts a Kubernetes conformant zone string to a format compatible with Azure. -func UpperCaseZone(upperCaseRegion string, lowerCaseZone string) string { - zoneSuffix := strings.TrimPrefix(lowerCaseZone, LowerCaseRegion(upperCaseRegion)) - return upperCaseRegion + zoneSuffix -} - func checkDiskName(diskName string) bool { length := len(diskName) diff --git a/pkg/azureutils/azure_disk_utils_test.go b/pkg/azureutils/azure_disk_utils_test.go index f15edae1e..cb917c43b 100644 --- a/pkg/azureutils/azure_disk_utils_test.go +++ b/pkg/azureutils/azure_disk_utils_test.go @@ -910,14 +910,13 @@ func TestIsAvailabilityZone(t *testing.T) { expected bool }{ {"empty string should return false", "", "eastus", false}, - {"wrong format should return false", "123", "eastus", false}, + {"wrong farmat should return false", "123", "eastus", false}, {"wrong location should return false", "chinanorth-1", "eastus", false}, {"correct zone should return true", "eastus-1", "eastus", true}, {"empty location should return true", "eastus-1", "", true}, {"empty location with fault domain should return false", "1", "", false}, {"empty location with wrong format should return false", "-1", "", false}, {"empty location with wrong format should return false", "eastus-", "", false}, - {"upper case format with white space should return true", "eastus-1 ", "East US", true}, } for _, test := range tests { @@ -1682,27 +1681,6 @@ func TestPickAvailabilityZone(t *testing.T) { } }, }, - { - name: "upper case region", - testFunc: func(t *testing.T) { - expectedresponse := "testregion-01" - region := "Test Region" - mp := make(map[string]string) - mp["N/A"] = "testregion-01" - topology := &csi.Topology{ - Segments: mp, - } - topologies := []*csi.Topology{} - topologies = append(topologies, topology) - req := &csi.TopologyRequirement{ - Preferred: topologies, - } - actualresponse := PickAvailabilityZone(req, region, "N/A") - if !reflect.DeepEqual(expectedresponse, actualresponse) { - t.Errorf("actualresponse: (%v), expectedresponse: (%v)", actualresponse, expectedresponse) - } - }, - }, } for _, tc := range testCases { t.Run(tc.name, tc.testFunc)