fix: fetch and override env config from metadata service and env files (

#7823)

pkg/azclient/arm_conf.go

@@ -45,10 +45,6 @@ type ARMClientConfig struct {
 	CloudProviderBackoffRetries int32 `json:"cloudProviderBackoffRetries,omitempty" yaml:"cloudProviderBackoffRetries,omitempty"`
 	// Backoff duration
 	CloudProviderBackoffDuration int `json:"cloudProviderBackoffDuration,omitempty" yaml:"cloudProviderBackoffDuration,omitempty"`
-	//Storage suffix
-	StorageSuffix *string `json:"storageSuffix,omitempty" yaml:"storageSuffix,omitempty"`
-	//ACRLoginServer
-	ACRLoginServer *string `json:"acrLoginServer,omitempty" yaml:"containerRegistrySuffix,omitempty"`
 }
 
 func (config *ARMClientConfig) GetTenantID() string {
@@ -59,30 +55,30 @@ func (config *ARMClientConfig) GetTenantID() string {
 	return config.TenantID
 }
 
-func GetAzCoreClientOption(armConfig *ARMClientConfig) (*policy.ClientOptions, error) {
+func GetAzCoreClientOption(armConfig *ARMClientConfig) (*policy.ClientOptions, *Environment, error) {
+	var env *Environment
+	var err error
 	//Get default settings
-	azCoreClientConfig := utils.GetDefaultAzCoreClientOption()
+	clientConfig := utils.GetDefaultAzCoreClientOption()
 	if armConfig != nil {
 		//update user agent header
 		if userAgent := strings.TrimSpace(armConfig.UserAgent); userAgent != "" {
-			azCoreClientConfig.Telemetry.Disabled = true
-			azCoreClientConfig.PerCallPolicies = append(azCoreClientConfig.PerCallPolicies, useragent.NewCustomUserAgentPolicy(userAgent))
+			clientConfig.Telemetry.Disabled = true
+			clientConfig.PerCallPolicies = append(clientConfig.PerCallPolicies, useragent.NewCustomUserAgentPolicy(userAgent))
 		}
 		//set cloud
-		cloudConfig, err := GetAzureCloudConfigAndBackfillARMClientConfig(armConfig)
+		clientConfig.Cloud, env, err = GetAzureCloudConfigAndEnvConfig(armConfig)
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
-		azCoreClientConfig.Cloud = *cloudConfig
 		if armConfig.CloudProviderBackoff && armConfig.CloudProviderBackoffDuration > 0 {
-			azCoreClientConfig.Retry.RetryDelay = time.Duration(armConfig.CloudProviderBackoffDuration) * time.Second
+			clientConfig.Retry.RetryDelay = time.Duration(armConfig.CloudProviderBackoffDuration) * time.Second
 		}
 		if armConfig.CloudProviderBackoff && armConfig.CloudProviderBackoffRetries > 0 {
-			azCoreClientConfig.Retry.MaxRetries = armConfig.CloudProviderBackoffRetries
+			clientConfig.Retry.MaxRetries = armConfig.CloudProviderBackoffRetries
 		}
-
 	}
-	return &azCoreClientConfig, nil
+	return &clientConfig, env, nil
 }
 
 func IsMultiTenant(armConfig *ARMClientConfig) bool {

pkg/azclient/auth.go

@@ -37,18 +37,12 @@ type AuthProvider struct {
 }
 
 func NewAuthProvider(armConfig *ARMClientConfig, config *AzureAuthConfig, clientOptionsMutFn ...func(option *policy.ClientOptions)) (*AuthProvider, error) {
-	clientOption, err := GetAzCoreClientOption(armConfig)
+	clientOption, _, err := GetAzCoreClientOption(armConfig)
 	if err != nil {
 		return nil, err
 	}
 	for _, fn := range clientOptionsMutFn {
 		fn(clientOption)
-		if clientOption == nil {
-			clientOption, err = GetAzCoreClientOption(armConfig)
-			if err != nil {
-				return nil, err
-			}
-		}
 	}
 	var computeCredential azcore.TokenCredential
 	var networkTokenCredential azcore.TokenCredential

pkg/azclient/client-gen/generator/clientfactory.go

@@ -120,7 +120,9 @@ func (generator ClientFactoryGenerator) Generate(ctx *genall.GenerationContext)
 		}
 
 		codeimportList["github.com/Azure/azure-sdk-for-go/sdk/azcore"] = make(map[string]struct{})
+		codeimportList["github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud"] = make(map[string]struct{})
 		codeimportList["github.com/Azure/azure-sdk-for-go/sdk/azcore/arm"] = make(map[string]struct{})
+		codeimportList["sigs.k8s.io/cloud-provider-azure/pkg/azclient/utils"] = make(map[string]struct{})
 		codeimportList["sigs.k8s.io/cloud-provider-azure/pkg/azclient/policy/ratelimit"] = make(map[string]struct{})
 		codeimportList["github.com/Azure/azure-sdk-for-go/sdk/azidentity"] = make(map[string]struct{})
 
@@ -141,12 +143,9 @@ func (generator ClientFactoryGenerator) Generate(ctx *genall.GenerationContext)
 		}
 		defer file.Close()
 		testimportList := make(map[string]map[string]struct{})
-		for k, v := range importList {
-			testimportList[k] = v
-		}
-
 		testimportList["github.com/onsi/ginkgo/v2"] = map[string]struct{}{}
 		testimportList["github.com/onsi/gomega"] = map[string]struct{}{}
+		testimportList["github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud"] = make(map[string]struct{})
 
 		err = DumpHeaderToWriter(ctx, file, generator.HeaderFile, testimportList, "azclient")
 		if err != nil {
@@ -187,6 +186,7 @@ var AbstractClientFactoryImplTemplate = template.Must(template.New("object-facto
 	`
 type ClientFactoryImpl struct {
 	armConfig     *ARMClientConfig
+	cloudConfig   cloud.Configuration
 	factoryConfig *ClientFactoryConfig
 	cred               azcore.TokenCredential
 	clientOptionsMutFn []func(option *arm.ClientOptions)
@@ -199,7 +199,7 @@ type ClientFactoryImpl struct {
 	{{end -}}
 }
 
-func NewClientFactory(config *ClientFactoryConfig, armConfig *ARMClientConfig, cred azcore.TokenCredential, clientOptionsMutFn ...func(option *arm.ClientOptions)) (ClientFactory,error) {
+func NewClientFactory(config *ClientFactoryConfig, armConfig *ARMClientConfig,cloud cloud.Configuration, cred azcore.TokenCredential, clientOptionsMutFn ...func(option *arm.ClientOptions)) (ClientFactory,error) {
 	if config == nil {
 		config = &ClientFactoryConfig{}
 	}
@@ -212,6 +212,7 @@ func NewClientFactory(config *ClientFactoryConfig, armConfig *ARMClientConfig, c
 	factory := &ClientFactoryImpl{
 		armConfig: 	   armConfig,
 		factoryConfig: config,
+		cloudConfig: 	  cloud,
 		cred:          cred,
 		clientOptionsMutFn: clientOptionsMutFn,
 	}
@@ -240,10 +241,11 @@ func NewClientFactory(config *ClientFactoryConfig, armConfig *ARMClientConfig, c
 {{- end }}
 func (factory *ClientFactoryImpl) create{{$resource}}Client(subscription string)({{.PkgAlias}}.{{.InterfaceTypeName}},error) {
 	//initialize {{.PkgAlias}}
-	options, err := GetDefaultResourceClientOption(factory.armConfig, factory.factoryConfig)
+	options, err := GetDefaultResourceClientOption(factory.armConfig)
 	if err != nil {
 		return nil, err
 	}
+	options.Cloud = factory.cloudConfig
 	{{if $client.AzureStackCloudAPIVersion}}
 	if factory.armConfig != nil && strings.EqualFold(factory.armConfig.Cloud, utils.AzureStackCloudName) {
 		options.ClientOptions.APIVersion = {{.PkgAlias}}.AzureStackCloudAPIVersion
@@ -319,7 +321,7 @@ ginkgo.When("config is nil", func() {
 			{{- $resource = $client.SubResource -}}
 			{{- end -}}
 ginkgo.It("should create factory instance without painc - {{$resource}}", func() {
-				factory, err := NewClientFactory(nil, nil, nil)
+				factory, err := NewClientFactory(nil, nil,cloud.AzurePublic, nil)
 				gomega.Expect(err).NotTo(gomega.HaveOccurred())
 				gomega.Expect(factory).NotTo(gomega.BeNil())
 				client := factory.Get{{$resource}}Client()

pkg/azclient/cloud.go

@@ -42,24 +42,24 @@ const (
 )
 
 func AzureCloudConfigFromName(cloudName string) *cloud.Configuration {
-	if cloudName == "" {
-		return &cloud.AzurePublic
-	}
 	cloudName = strings.ToUpper(strings.TrimSpace(cloudName))
 	if cloudConfig, ok := EnvironmentMapping[cloudName]; ok {
 		return cloudConfig
 	}
-	return nil
+	return &cloud.AzurePublic
 }
 
-// OverrideAzureCloudConfigFromMetadataService returns cloud config from url
+// OverrideAzureCloudConfigAndEnvConfigFromMetadataService returns cloud config and environment config from url
 // track2 sdk will add this one in the near future https://github.com/Azure/azure-sdk-for-go/issues/20959
-func OverrideAzureCloudConfigFromMetadataService(armconfig *ARMClientConfig, config *cloud.Configuration) error {
-	if armconfig == nil || armconfig.ResourceManagerEndpoint == "" {
+// cloud and env should not be empty
+// it should never return an empty config
+func OverrideAzureCloudConfigAndEnvConfigFromMetadataService(endpoint, cloudName string, cloudConfig *cloud.Configuration, env *Environment) error {
+	// If the ResourceManagerEndpoint is not set, we should not query the metadata service
+	if endpoint == "" {
 		return nil
 	}
 
-	managementEndpoint := fmt.Sprintf("%s%s", strings.TrimSuffix(armconfig.ResourceManagerEndpoint, "/"), "/metadata/endpoints?api-version=2019-05-01")
+	managementEndpoint := fmt.Sprintf("%s%s", strings.TrimSuffix(endpoint, "/"), "/metadata/endpoints?api-version=2019-05-01")
 	res, err := http.Get(managementEndpoint) //nolint
 	if err != nil {
 		return err
@@ -86,35 +86,38 @@ func OverrideAzureCloudConfigFromMetadataService(armconfig *ARMClientConfig, con
 	}
 
 	for _, item := range metadata {
-		if armconfig.Cloud == "" || strings.EqualFold(item.Name, armconfig.Cloud) {
+		if cloudName == "" || strings.EqualFold(item.Name, cloudName) {
 			// We use the endpoint to build our config, but on ASH the config returned
 			// does not contain the endpoint, and this is not accounted for. This
 			// ultimately unsets it for the returned config, causing the bootstrap of
 			// the provider to fail. Instead, check if the endpoint is returned, and if
 			// It is not then set it.
 			if item.ResourceManager == "" {
-				item.ResourceManager = armconfig.ResourceManagerEndpoint
+				item.ResourceManager = endpoint
 			}
-			config.Services[cloud.ResourceManager] = cloud.ServiceConfiguration{
+			cloudConfig.Services[cloud.ResourceManager] = cloud.ServiceConfiguration{
 				Endpoint: item.ResourceManager,
 				Audience: item.Authentication.Audiences[0],
 			}
+			env.ResourceManagerEndpoint = item.ResourceManager
+			env.TokenAudience = item.Authentication.Audiences[0]
 			if item.Authentication.LoginEndpoint != "" {
-				config.ActiveDirectoryAuthorityHost = item.Authentication.LoginEndpoint
+				cloudConfig.ActiveDirectoryAuthorityHost = item.Authentication.LoginEndpoint
+				env.ActiveDirectoryEndpoint = item.Authentication.LoginEndpoint
 			}
-			if item.Suffixes.Storage != nil && armconfig.StorageSuffix == nil {
-				armconfig.StorageSuffix = item.Suffixes.Storage
+			if item.Suffixes.Storage != nil {
+				env.StorageEndpointSuffix = *item.Suffixes.Storage
 			}
-			if item.Suffixes.AcrLoginServer != nil && armconfig.ACRLoginServer == nil {
-				armconfig.ACRLoginServer = item.Suffixes.AcrLoginServer
+			if item.Suffixes.AcrLoginServer != nil {
+				env.ContainerRegistryDNSSuffix = *item.Suffixes.AcrLoginServer
 			}
 			return nil
 		}
 	}
 	return nil
 }
 
-func OverrideAzureCloudConfigFromEnv(armconfig *ARMClientConfig, config *cloud.Configuration) error {
+func OverrideAzureCloudConfigFromEnv(config *cloud.Configuration, env *Environment) error {
 	envFilePath, ok := os.LookupEnv(EnvironmentFilepathName)
 	if !ok {
 		return nil
@@ -123,54 +126,44 @@ func OverrideAzureCloudConfigFromEnv(armconfig *ARMClientConfig, config *cloud.C
 	if err != nil {
 		return err
 	}
-	var envConfig Environment
-	if err = json.Unmarshal(content, &envConfig); err != nil {
+	if err = json.Unmarshal(content, env); err != nil {
 		return err
 	}
-	if len(envConfig.ActiveDirectoryEndpoint) > 0 {
-		config.ActiveDirectoryAuthorityHost = envConfig.ActiveDirectoryEndpoint
-	}
-	if len(envConfig.ResourceManagerEndpoint) > 0 && len(envConfig.TokenAudience) > 0 {
+	if len(env.ResourceManagerEndpoint) > 0 && len(env.TokenAudience) > 0 {
 		config.Services[cloud.ResourceManager] = cloud.ServiceConfiguration{
-			Endpoint: envConfig.ResourceManagerEndpoint,
-			Audience: envConfig.TokenAudience,
+			Endpoint: env.ResourceManagerEndpoint,
+			Audience: env.TokenAudience,
 		}
 	}
-	if len(envConfig.StorageEndpointSuffix) > 0 {
-		armconfig.StorageSuffix = &envConfig.StorageEndpointSuffix
-	}
-	if len(envConfig.ContainerRegistryDNSSuffix) > 0 {
-		armconfig.ACRLoginServer = &envConfig.ContainerRegistryDNSSuffix
+	if len(env.ActiveDirectoryEndpoint) > 0 {
+		config.ActiveDirectoryAuthorityHost = env.ActiveDirectoryEndpoint
 	}
 	return nil
 }
 
-// GetAzureCloudConfigAndBackfillArmClientConfig retrieves the Azure cloud configuration based on the provided ARM client configuration.
-// If the ARM client configuration is nil, it returns the default Azure public cloud configuration.
-// It attempts to override the cloud configuration using metadata service and environment variables.
-//
-// Parameters:
-// - armConfig: A pointer to an ARMClientConfig struct containing the ARM client configuration.
-//
-// Returns:
-// - A pointer to a cloud.Configuration struct representing the Azure cloud configuration.
-// - An error if there is an issue overriding the cloud configuration from metadata service or environment variables.
-func GetAzureCloudConfigAndBackfillARMClientConfig(armConfig *ARMClientConfig) (*cloud.Configuration, error) {
-	config := &cloud.AzurePublic
+func GetAzureCloudConfigAndEnvConfig(armConfig *ARMClientConfig) (cloud.Configuration, *Environment, error) {
+	env := &Environment{}
+	var cloudName string
+	if armConfig != nil {
+		cloudName = armConfig.Cloud
+	}
+	config := AzureCloudConfigFromName(cloudName)
 	if armConfig == nil {
-		return config, nil
+		return *config, nil, nil
 	}
-	config = AzureCloudConfigFromName(armConfig.Cloud)
-	if err := OverrideAzureCloudConfigFromMetadataService(armConfig, config); err != nil {
-		return nil, err
+
+	err := OverrideAzureCloudConfigAndEnvConfigFromMetadataService(armConfig.ResourceManagerEndpoint, cloudName, config, env)
+	if err != nil {
+		return *config, nil, err
 	}
-	err := OverrideAzureCloudConfigFromEnv(armConfig, config)
-	return config, err
+	err = OverrideAzureCloudConfigFromEnv(config, env)
+	return *config, env, err
 }
 
 // Environment represents a set of endpoints for each of Azure's Clouds.
 type Environment struct {
 	Name                       string `json:"name"`
+	ServiceManagementEndpoint  string `json:"serviceManagementEndpoint"`
 	ResourceManagerEndpoint    string `json:"resourceManagerEndpoint"`
 	ActiveDirectoryEndpoint    string `json:"activeDirectoryEndpoint"`
 	StorageEndpointSuffix      string `json:"storageEndpointSuffix"`