address review comments

gardener · Jun 12, 2024 · 3c43287 · 3c43287
1 parent 1e5c473
commit 3c43287
Show file tree

Hide file tree

Showing 5 changed files with 22 additions and 163 deletions.
diff --git a/pkg/azure/api/providerspec.go b/pkg/azure/api/providerspec.go
@@ -69,8 +69,6 @@ type AzureProviderSpec struct {
 	ResourceGroup string `json:"resourceGroup,omitempty"`
 	// SubnetInfo contains the configuration for an existing subnet.
 	SubnetInfo AzureSubnetInfo `json:"subnetInfo,omitempty"`
-	// CloudConfiguration contains config that controls which cloud to connect to
-	CloudConfiguration *CloudConfiguration `json:"cloudConfiguration,omitempty"`
 }
 
 // AzureVirtualMachineProperties describes the properties of a Virtual Machine.
@@ -312,22 +310,8 @@ type AzureDiagnosticsProfile struct {
 	StorageURI *string `json:"storageURI,omitempty"`
 }
 
-// The (currently) supported values for the names of clouds to use in the CloudConfiguration.
-const (
-	AzureChinaCloudName  string = "AzureChina"
-	AzureGovCloudName    string = "AzureGovernment"
-	AzurePublicCloudName string = "AzurePublic"
-)
-
-// The known prefixes in of region names for the various instances.
+// The known prefixes of region names for the various instances.
 var (
 	AzureGovRegionPrefixes   = []string{"usgov", "usdod", "ussec"}
 	AzureChinaRegionPrefixes = []string{"china"}
 )
-
-// CloudConfiguration contains detailed config for the cloud to connect to. Currently we only support selection of well-
-// known Azure-instances by name, but this could be extended in future to support private clouds.
-type CloudConfiguration struct {
-	// Name is the name of the cloud to connect to, e.g. "AzurePublic" or "AzureChina".
-	Name string `json:"name,omitempty"`
-}
diff --git a/pkg/azure/api/validation/validation.go b/pkg/azure/api/validation/validation.go
@@ -9,7 +9,6 @@ package validation
 
 import (
 	"fmt"
-	"slices"
 	"strings"
 
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5"
@@ -52,10 +51,6 @@ func ValidateProviderSpec(spec api.AzureProviderSpec) field.ErrorList {
 	allErrs = append(allErrs, validateProperties(spec.Properties, specPath.Child("properties"))...)
 	allErrs = append(allErrs, validateTags(spec.Tags, specPath.Child("tags"))...)
 
-	if spec.CloudConfiguration != nil {
-		allErrs = append(allErrs, validateCloudConfiguration(*spec.CloudConfiguration, specPath.Child("cloudConfiguration"))...)
-	}
-
 	return allErrs
 }
 
@@ -126,17 +121,6 @@ func validateProperties(properties api.AzureVirtualMachineProperties, fldPath *f
 	return allErrs
 }
 
-func validateCloudConfiguration(cloudConfiguration api.CloudConfiguration, fldPath *field.Path) field.ErrorList {
-	var allErrs field.ErrorList
-	knownCloudInstances := []string{api.AzurePublicCloudName, api.AzureChinaCloudName, api.AzureGovCloudName}
-
-	if cloudName := cloudConfiguration.Name; !slices.Contains(knownCloudInstances, cloudName) {
-		allErrs = append(allErrs, field.NotSupported(fldPath.Child("name"), cloudName, knownCloudInstances))
-	}
-
-	return allErrs
-}
-
 func validateSecurityProfile(securityProfile *api.AzureSecurityProfile, fldPath *field.Path) field.ErrorList {
 	var allErrs field.ErrorList
 	if securityProfile == nil {

diff --git a/pkg/azure/provider/helpers/driver.go b/pkg/azure/provider/helpers/driver.go
@@ -43,9 +43,7 @@ func ExtractProviderSpecAndConnectConfig(mcc *v1alpha1.MachineClass, secret *cor
 		err                  error
 		providerSpec         api.AzureProviderSpec
 		connectConfig        access.ConnectConfig
-		cloudConfiguration   *api.CloudConfiguration
-		azCloudConfiguration cloud.Configuration
-		region               *string
+		azCloudConfiguration cloud.Configuration = cloud.AzurePublic
 	)
 	// validate provider Spec provider. Exit early if it is not azure.
 	if err = validation.ValidateMachineClassProvider(mcc); err != nil {
@@ -60,14 +58,8 @@ func ExtractProviderSpecAndConnectConfig(mcc *v1alpha1.MachineClass, secret *cor
 		return api.AzureProviderSpec{}, access.ConnectConfig{}, err
 	}
 
-	if providerSpec.CloudConfiguration != nil {
-		cloudConfiguration = providerSpec.CloudConfiguration
-	}
 	if mcc != nil && mcc.NodeTemplate != nil {
-		region = &mcc.NodeTemplate.Region
-	}
-	if azCloudConfiguration, err = DetermineCloudConfiguration(cloudConfiguration, region); err != nil {
-		return api.AzureProviderSpec{}, access.ConnectConfig{}, err
+		azCloudConfiguration = CloudConfigurationFromRegion(mcc.NodeTemplate.Region)
 	}
 
 	connectConfig.ClientOptions.Cloud = azCloudConfiguration

diff --git a/pkg/azure/provider/helpers/providerspec.go b/pkg/azure/provider/helpers/providerspec.go
@@ -5,41 +5,15 @@
 package helpers
 
 import (
-	"fmt"
 	"strings"
 
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud"
 
 	"github.com/gardener/machine-controller-manager-provider-azure/pkg/azure/api"
 )
 
-// DetermineCloudConfiguration returns the Azure cloud.Configuration corresponding to the instance given by the provided input. If both cloudConfiguration and
-// region are provided, cloudConfiguration takes precedence.
-func DetermineCloudConfiguration(cloudConfiguration *api.CloudConfiguration, region *string) (cloud.Configuration, error) {
-
-	if cloudConfiguration != nil {
-		cloudConfigurationName := cloudConfiguration.Name
-		switch {
-		case strings.EqualFold(cloudConfigurationName, api.AzurePublicCloudName):
-			return cloud.AzurePublic, nil
-		case strings.EqualFold(cloudConfigurationName, api.AzureGovCloudName):
-			return cloud.AzureGovernment, nil
-		case strings.EqualFold(cloudConfigurationName, api.AzureChinaCloudName):
-			return cloud.AzureChina, nil
-
-		default:
-			return cloud.Configuration{}, fmt.Errorf("unknown cloud configuration name '%s'", cloudConfigurationName)
-		}
-	} else if region != nil {
-		return cloudConfigurationFromRegion(*region), nil
-	} else {
-		// Fallback, this case should only occur during testing as we expect the region to always be given in an actual live scenario.
-		return cloud.AzurePublic, nil
-	}
-}
-
-// cloudConfigurationFromRegion returns a matching cloudConfiguration corresponding to a well known cloud instance for the given region
-func cloudConfigurationFromRegion(region string) cloud.Configuration {
+// CloudConfigurationFromRegion returns a matching cloudConfiguration corresponding to a well known cloud instance for the given region
+func CloudConfigurationFromRegion(region string) cloud.Configuration {
 	switch {
 	case hasAnyPrefix(region, api.AzureGovRegionPrefixes...):
 		return cloud.AzureGovernment

diff --git a/pkg/azure/provider/helpers/providerspec_test.go b/pkg/azure/provider/helpers/providerspec_test.go
@@ -8,103 +8,28 @@ import (
 	"testing"
 
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud"
-	"github.com/gardener/machine-controller-manager-provider-azure/pkg/azure/api"
 	. "github.com/onsi/gomega"
-	"k8s.io/utils/ptr"
 )
 
-func TestNilConfig(t *testing.T) {
-	g := NewWithT(t)
-
-	var (
-		testConfig *api.CloudConfiguration = nil
-		testRegion                         = ptr.To("Foo")
-	)
-
-	configuration, err := DetermineCloudConfiguration(testConfig, testRegion)
-
-	g.Expect(err).ToNot(HaveOccurred())
-	g.Expect(configuration).To(Equal(cloud.AzurePublic))
-}
-
-func TestNilRegion(t *testing.T) {
-	g := NewWithT(t)
-
-	var (
-		testConfig         = &api.CloudConfiguration{Name: api.AzurePublicCloudName}
-		testRegion *string = nil
-	)
-
-	configuration, err := DetermineCloudConfiguration(testConfig, testRegion)
-
-	g.Expect(err).ToNot(HaveOccurred())
-	g.Expect(configuration).To(Equal(cloud.AzurePublic))
-}
-
-func TestNilConfigAndRegion(t *testing.T) {
-	g := NewWithT(t)
-
-	var (
-		testConfig *api.CloudConfiguration = nil
-		testRegion *string                 = nil
-	)
-
-	configuration, err := DetermineCloudConfiguration(testConfig, testRegion)
-
-	g.Expect(err).ToNot(HaveOccurred())
-	g.Expect(configuration).To(Equal(cloud.AzurePublic))
-}
-
-func TestInvalidConfigName(t *testing.T) {
-	g := NewWithT(t)
-
-	var (
-		testConfig         = &api.CloudConfiguration{Name: "Foo"}
-		testRegion *string = nil
-	)
-
-	_, err := DetermineCloudConfiguration(testConfig, testRegion)
-
-	g.Expect(err).To(HaveOccurred())
-}
-
-func TestPredefinedClouds(t *testing.T) {
-	g := NewWithT(t)
-
-	var (
-		testPublicConfiguration         = &api.CloudConfiguration{Name: api.AzurePublicCloudName}
-		testGovConfiguration            = &api.CloudConfiguration{Name: api.AzureGovCloudName}
-		testChinaConfigration           = &api.CloudConfiguration{Name: api.AzureChinaCloudName}
-		testRegion              *string = nil
-	)
-
-	configuration, err := DetermineCloudConfiguration(testPublicConfiguration, testRegion)
-
-	g.Expect(err).ToNot(HaveOccurred())
-	g.Expect(configuration).To(Equal(cloud.AzurePublic))
-
-	configuration, err = DetermineCloudConfiguration(testGovConfiguration, testRegion)
-
-	g.Expect(err).ToNot(HaveOccurred())
-	g.Expect(configuration).To(Equal(cloud.AzureGovernment))
-
-	configuration, err = DetermineCloudConfiguration(testChinaConfigration, testRegion)
-
-	g.Expect(err).ToNot(HaveOccurred())
-	g.Expect(configuration).To(Equal(cloud.AzureChina))
-}
-
 func TestRegionMatching(t *testing.T) {
 	g := NewWithT(t)
 
-	var (
-		testConfig *api.CloudConfiguration = nil
-		testRegion *string                 = ptr.To("ussecFoo")
-	)
-
-	configuration, err := DetermineCloudConfiguration(testConfig, testRegion)
-
-	g.Expect(err).ToNot(HaveOccurred())
-	g.Expect(configuration).To(Equal(cloud.AzureGovernment))
+	type testData struct {
+		region         string
+		expectedOutput cloud.Configuration
+	}
+
+	tests := []testData{
+		{region: "westeurope", expectedOutput: cloud.AzurePublic},
+		{region: "USSecFoo", expectedOutput: cloud.AzureGovernment},
+		{region: "USGovBar", expectedOutput: cloud.AzureGovernment},
+		{region: "USDoDFoobar", expectedOutput: cloud.AzureGovernment},
+		{region: "ChinaEastBar", expectedOutput: cloud.AzureChina},
+	}
+
+	for _, t := range tests {
+		configuration := CloudConfigurationFromRegion(t.region)
+		g.Expect(configuration).To(Equal(t.expectedOutput))
+	}
 
 }