diff --git a/pkg/cloud/azure/azure.go b/pkg/cloud/azure/azure.go index 0a4ff602d..c9cb043aa 100644 --- a/pkg/cloud/azure/azure.go +++ b/pkg/cloud/azure/azure.go @@ -2,12 +2,20 @@ package azure import ( "embed" + "encoding/json" "fmt" + "slices" + "strings" "github.com/asaskevich/govalidator" + configv1 "github.com/openshift/api/config/v1" appsv1 "k8s.io/api/apps/v1" + "k8s.io/apimachinery/pkg/util/validation/field" "sigs.k8s.io/controller-runtime/pkg/client" + azureconsts "sigs.k8s.io/cloud-provider-azure/pkg/consts" + azure "sigs.k8s.io/cloud-provider-azure/pkg/provider" + "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/cloud/common" "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/config" ) @@ -23,6 +31,25 @@ var ( } ) +var ( + validAzureCloudNames = map[configv1.AzureCloudEnvironment]struct{}{ + configv1.AzurePublicCloud: struct{}{}, + configv1.AzureUSGovernmentCloud: struct{}{}, + configv1.AzureChinaCloud: struct{}{}, + configv1.AzureGermanCloud: struct{}{}, + configv1.AzureStackCloud: struct{}{}, + } + + validAzureCloudNameValues = func() []string { + v := make([]string, 0, len(validAzureCloudNames)) + for n := range validAzureCloudNames { + v = append(v, string(n)) + } + slices.Sort(v) + return v + }() +) + type imagesReference struct { CloudControllerManager string `valid:"required"` CloudControllerManagerOperator string `valid:"required"` @@ -85,3 +112,70 @@ func NewProviderAssets(config config.OperatorConfig) (common.CloudProviderAssets } return assets, nil } + +// IsAzure ensures that the underlying platform is Azure. It will fail if the +// CloudName is AzureStack as we handle it separately with it's own +// CloudConfigTransformer. +func IsAzure(infra *configv1.Infrastructure) bool { + if infra.Status.PlatformStatus != nil { + if infra.Status.PlatformStatus.Type == configv1.AzurePlatformType && + (infra.Status.PlatformStatus.Azure.CloudName != configv1.AzureStackCloud) { + return true + } + } + return false +} + +func CloudConfigTransformer(source string, infra *configv1.Infrastructure, network *configv1.Network) (string, error) { + if !IsAzure(infra) { + return "", fmt.Errorf("invalid platform, expected CloudName to be %s", configv1.AzurePublicCloud) + } + + var cfg azure.Config + if err := json.Unmarshal([]byte(source), &cfg); err != nil { + return "", fmt.Errorf("failed to unmarshal the cloud.conf: %w", err) + } + + // We are copying the behaviour from CCO's transformer we need to: + // 1. Ensure that the Cloud is set in the cloud.conf + // i. If it is set, verify that it is valid and does not conflict with the + // infrastructure config. If it conflicts, we want to error + // ii. If it is not set, default to public cloud (configv1.AzurePublicCloud) + // + // 2. Verify the cloud name set in the infra config is valid, if it is not + // bail with an informative error + + // Verify the cloud name set in the infra config is valid + cloud := configv1.AzurePublicCloud + if azurePlatform := infra.Status.PlatformStatus.Azure; azurePlatform != nil { + if c := azurePlatform.CloudName; c != "" { + if _, ok := validAzureCloudNames[c]; !ok { + return "", field.NotSupported(field.NewPath("status", "platformStatus", "azure", "cloudName"), c, validAzureCloudNameValues) + } + cloud = c + } + } + + // Ensure cloud set in cloud.conf matches infra + if cfg.Cloud != "" { + if !strings.EqualFold(string(cloud), cfg.Cloud) { + return "", + fmt.Errorf(`invalid user-provided cloud.conf: \"cloud\" field in user-provided + cloud.conf conflicts with infrastructure object`) + } + } + cfg.Cloud = string(cloud) + + // If the virtual machine type is not set we need to make sure it uses the + // "standard" instance type. See OCPBUGS-25483 and OCPBUGS-20213 for more + // information + if cfg.VMType == "" { + cfg.VMType = azureconsts.VMTypeStandard + } + + cfgbytes, err := json.Marshal(cfg) + if err != nil { + return "", fmt.Errorf("failed to marshal the cloud.conf: %w", err) + } + return string(cfgbytes), nil +} diff --git a/pkg/cloud/azure/azure_test.go b/pkg/cloud/azure/azure_test.go index c5aca923f..21d2cb218 100644 --- a/pkg/cloud/azure/azure_test.go +++ b/pkg/cloud/azure/azure_test.go @@ -1,14 +1,26 @@ package azure import ( + "encoding/json" + "fmt" "testing" + . "github.com/onsi/gomega" configv1 "github.com/openshift/api/config/v1" "github.com/stretchr/testify/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + azure "sigs.k8s.io/cloud-provider-azure/pkg/provider" + + ratelimitconfig "sigs.k8s.io/cloud-provider-azure/pkg/provider/config" "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/config" ) +const ( + infraCloudConfName = "test-config" + infraCloudConfKey = "foo" +) + func TestResourcesRenderingSmoke(t *testing.T) { tc := []struct { @@ -79,3 +91,155 @@ func TestResourcesRenderingSmoke(t *testing.T) { }) } } + +func makeInfrastructureResource(platform configv1.PlatformType, cloudName configv1.AzureCloudEnvironment) *configv1.Infrastructure { + cfg := configv1.Infrastructure{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + }, + Status: configv1.InfrastructureStatus{ + PlatformStatus: &configv1.PlatformStatus{ + Type: platform, + }, + }, + Spec: configv1.InfrastructureSpec{ + CloudConfig: configv1.ConfigMapFileReference{ + Name: infraCloudConfName, + Key: infraCloudConfKey, + }, + PlatformSpec: configv1.PlatformSpec{ + Type: platform, + }, + }, + } + + if platform == configv1.AzurePlatformType { + cfg.Status.PlatformStatus.Azure = &configv1.AzurePlatformStatus{ + CloudName: cloudName, + } + } + + return &cfg +} + +// This test is a little complicated with all the JSON marshalling and +// unmarshalling, but it is necessary due to the nature of how this data +// is stored in Kuberenetes. The ConfigMaps containing the cloud config +// will have string encoded JSON objects in them, due to the non-deterministic +// natue of map object in Go we will need to examine the data instead of +// comparing strings. +func TestCloudConfigTransformer(t *testing.T) { + tc := []struct { + name string + source azure.Config + expected azure.Config + infra *configv1.Infrastructure + errMsg string + }{ + { + name: "Non Azure returns an error", + source: azure.Config{}, + infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzureStackCloud), + errMsg: fmt.Sprintf("invalid platform, expected CloudName to be %s", configv1.AzurePublicCloud), + }, + { + name: "Azure sets the vmType to standard and cloud to AzurePublicCloud when neither is set", + source: azure.Config{}, + expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}}, + infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzurePublicCloud), + }, + { + name: "Azure doesn't modify vmType if user set", + source: azure.Config{VMType: "vmss"}, + expected: azure.Config{VMType: "vmss", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}}, + infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzurePublicCloud), + }, + { + name: "Azure sets the cloud to AzurePublicCloud and keeps existing fields", + source: azure.Config{ + ResourceGroup: "test-rg", + }, + expected: azure.Config{VMType: "standard", ResourceGroup: "test-rg", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}}, + infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzurePublicCloud), + }, + { + name: "Azure keeps the cloud set to AzurePublicCloud", + source: azure.Config{AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}}, + expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}}, + infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzurePublicCloud), + }, + { + name: "Azure keeps the cloud set to US Gov cloud", + source: azure.Config{AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzureUSGovernmentCloud)}}, + expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzureUSGovernmentCloud)}}, + infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzureUSGovernmentCloud), + }, + { + name: "Azure keeps the cloud set to China cloud", + source: azure.Config{AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzureChinaCloud)}}, + expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzureChinaCloud)}}, + infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzureChinaCloud), + }, + { + name: "Azure keeps the cloud set to German cloud", + source: azure.Config{AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzureGermanCloud)}}, + expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzureGermanCloud)}}, + infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzureGermanCloud), + }, + { + name: "Azure throws an error if the infra has an invalid cloud", + source: azure.Config{}, + infra: makeInfrastructureResource(configv1.AzurePlatformType, "AzureAnotherCloud"), + errMsg: "status.platformStatus.azure.cloudName: Unsupported value: \"AzureAnotherCloud\": supported values: \"AzureChinaCloud\", \"AzureGermanCloud\", \"AzurePublicCloud\", \"AzureStackCloud\", \"AzureUSGovernmentCloud\"", + }, + { + name: "Azure keeps the cloud set in the source when there is not one set in infrastructure", + source: azure.Config{AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}}, + expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}}, + infra: makeInfrastructureResource(configv1.AzurePlatformType, ""), + }, + { + name: "Azure sets the cloud to match the infrastructure if an empty string is provided in source", + source: azure.Config{AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: ""}}, + expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}}, + infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzurePublicCloud), + }, + { + name: "Azure sets the cloud to match the infrastructure if an empty string is provided in source and the infrastructure is non standard", + source: azure.Config{AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: ""}}, + expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzureUSGovernmentCloud)}}, + infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzureUSGovernmentCloud), + }, + { + name: "Azure returns an error if the source config conflicts with the infrastructure", + source: azure.Config{AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}}, + infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzureUSGovernmentCloud), + errMsg: "invalid user-provided cloud.conf: \\\"cloud\\\" field in user-provided\n\t\t\t\tcloud.conf conflicts with infrastructure object", + }, + { + name: "Azure keeps the cloud set to AzurePublicCloud if the source is upper case", + source: azure.Config{AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: "AZUREPUBLICCLOUD"}}, + expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}}, + infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzurePublicCloud), + }, + } + + for _, tc := range tc { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + + src, err := json.Marshal(tc.source) + g.Expect(err).NotTo(HaveOccurred(), "Marshal of source data should succeed") + + actual, err := CloudConfigTransformer(string(src), tc.infra, nil) + if tc.errMsg != "" { + g.Expect(err).Should(MatchError(tc.errMsg)) + g.Expect(actual).Should(Equal("")) + } else { + var observed azure.Config + g.Expect(json.Unmarshal([]byte(actual), &observed)).To(Succeed(), "Unmarshal of observed data should succeed") + g.Expect(observed).Should(Equal(tc.expected)) + } + }) + } +} diff --git a/pkg/cloud/cloud.go b/pkg/cloud/cloud.go index 666b07bf0..8bc69807a 100644 --- a/pkg/cloud/cloud.go +++ b/pkg/cloud/cloud.go @@ -53,7 +53,7 @@ func GetCloudConfigTransformer(platformStatus *configv1.PlatformStatus) (cloudCo if azurestack.IsAzureStackHub(platformStatus) { return azurestack.CloudConfigTransformer, true, nil } - return nil, true, nil + return azure.CloudConfigTransformer, true, nil case configv1.GCPPlatformType: return common.NoOpTransformer, false, nil case configv1.IBMCloudPlatformType: diff --git a/pkg/controllers/cloud_config_sync_controller_test.go b/pkg/controllers/cloud_config_sync_controller_test.go index c9b785b18..8bb722a91 100644 --- a/pkg/controllers/cloud_config_sync_controller_test.go +++ b/pkg/controllers/cloud_config_sync_controller_test.go @@ -23,6 +23,8 @@ import ( const ( infraCloudConfName = "test-config" infraCloudConfKey = "foo" + + defaultAzureConfig = `{"cloud":"AzurePublicCloud","tenantId":"0000000-0000-0000-0000-000000000000","subscriptionId":"0000000-0000-0000-0000-000000000000","vmType":"standard","putVMSSVMBatchSize":0,"enableMigrateToIPBasedBackendPoolAPI":false}` ) func makeInfrastructureResource(platform configv1.PlatformType) *configv1.Infrastructure { @@ -54,6 +56,20 @@ func makeNetworkResource() *configv1.Network { } func makeInfraStatus(platform configv1.PlatformType) configv1.InfrastructureStatus { + if platform == configv1.AzurePlatformType { + return configv1.InfrastructureStatus{ + PlatformStatus: &configv1.PlatformStatus{ + Type: platform, + Azure: &configv1.AzurePlatformStatus{ + CloudName: configv1.AzurePublicCloud, + }, + }, + Platform: platform, + InfrastructureTopology: configv1.HighlyAvailableTopologyMode, + ControlPlaneTopology: configv1.HighlyAvailableTopologyMode, + } + } + return configv1.InfrastructureStatus{ PlatformStatus: &configv1.PlatformStatus{ Type: platform, @@ -68,14 +84,14 @@ func makeInfraCloudConfig() *corev1.ConfigMap { return &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{ Name: infraCloudConfName, Namespace: OpenshiftConfigNamespace, - }, Data: map[string]string{infraCloudConfKey: "bar"}} + }, Data: map[string]string{infraCloudConfKey: defaultAzureConfig}} } func makeManagedCloudConfig() *corev1.ConfigMap { return &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{ Name: managedCloudConfigMapName, Namespace: OpenshiftManagedConfigNamespace, - }, Data: map[string]string{"cloud.conf": "bar"}} + }, Data: map[string]string{"cloud.conf": defaultAzureConfig}} } var _ = Describe("isCloudConfigEqual reconciler method", func() { @@ -128,7 +144,7 @@ var _ = Describe("prepareSourceConfigMap reconciler method", func() { It("config preparation should not touch extra fields in infra ConfigMap", func() { extendedInfraConfig := infraCloudConfig.DeepCopy() - extendedInfraConfig.Data = map[string]string{infraCloudConfKey: "bar", "bar": "baz"} + extendedInfraConfig.Data = map[string]string{infraCloudConfKey: "{}", "{}": "{}"} preparedConfig, err := reconciler.prepareSourceConfigMap(extendedInfraConfig, infra) Expect(err).Should(Succeed()) _, ok := preparedConfig.Data[defaultConfigKey] @@ -254,13 +270,14 @@ var _ = Describe("Cloud config sync controller", func() { if err != nil { return false, err } - return syncedCloudConfigMap.Data[defaultConfigKey] == "bar", nil + return syncedCloudConfigMap.Data[defaultConfigKey] == defaultAzureConfig, nil }).Should(BeTrue()) }) It("config should be synced up if managed cloud config changed", func() { + changedConfigString := `{"cloud":"AzurePublicCloud","tenantId":"0000000-1234-1234-0000-000000000000","subscriptionId":"0000000-0000-0000-0000-000000000000","vmType":"standard","putVMSSVMBatchSize":0,"enableMigrateToIPBasedBackendPoolAPI":false}` changedManagedConfig := managedCloudConfig.DeepCopy() - changedManagedConfig.Data = map[string]string{"cloud.conf": "managed one changed"} + changedManagedConfig.Data = map[string]string{"cloud.conf": changedConfigString} Expect(cl.Update(ctx, changedManagedConfig)).To(Succeed()) Eventually(func() (bool, error) { @@ -269,7 +286,7 @@ var _ = Describe("Cloud config sync controller", func() { if err != nil { return false, err } - return syncedCloudConfigMap.Data[defaultConfigKey] == "managed one changed", nil + return syncedCloudConfigMap.Data[defaultConfigKey] == changedConfigString, nil }).Should(BeTrue()) }) @@ -279,14 +296,15 @@ var _ = Describe("Cloud config sync controller", func() { return cl.Get(ctx, syncedConfigMapKey, syncedCloudConfigMap) }, timeout).Should(Succeed()) - syncedCloudConfigMap.Data = map[string]string{"foo": "baz"} + changedConfigString := `{"cloud":"AzurePublicCloud","tenantId":"0000000-1234-1234-0000-000000000000","subscriptionId":"0000000-0000-0000-0000-000000000000","vmType":"standard","putVMSSVMBatchSize":0,"enableMigrateToIPBasedBackendPoolAPI":false}` + syncedCloudConfigMap.Data = map[string]string{"foo": changedConfigString} Expect(cl.Update(ctx, syncedCloudConfigMap)).To(Succeed()) Eventually(func() (bool, error) { err := cl.Get(ctx, syncedConfigMapKey, syncedCloudConfigMap) if err != nil { return false, err } - return syncedCloudConfigMap.Data[defaultConfigKey] == "bar", nil + return syncedCloudConfigMap.Data[defaultConfigKey] == defaultAzureConfig, nil }).Should(BeTrue()) Expect(cl.Delete(ctx, syncedCloudConfigMap)).To(Succeed()) @@ -295,7 +313,7 @@ var _ = Describe("Cloud config sync controller", func() { if err != nil { return false, err } - return syncedCloudConfigMap.Data[defaultConfigKey] == "bar", nil + return syncedCloudConfigMap.Data[defaultConfigKey] == defaultAzureConfig, nil }).Should(BeTrue()) }) @@ -318,8 +336,9 @@ var _ = Describe("Cloud config sync controller", func() { Expect(cl.Delete(ctx, managedCloudConfig)).Should(Succeed()) managedCloudConfig = nil + changedInfraConfigString := `{"cloud":"AzurePublicCloud","tenantId":"0000000-1234-1234-0000-000000000000","subscriptionId":"0000000-0000-0000-0000-000000000000","vmType":"standard","putVMSSVMBatchSize":0,"enableMigrateToIPBasedBackendPoolAPI":false}` changedInfraConfig := infraCloudConfig.DeepCopy() - changedInfraConfig.Data = map[string]string{infraCloudConfKey: "infra one changed"} + changedInfraConfig.Data = map[string]string{infraCloudConfKey: changedInfraConfigString} Expect(cl.Update(ctx, changedInfraConfig)).Should(Succeed()) Eventually(func() (bool, error) { @@ -328,14 +347,16 @@ var _ = Describe("Cloud config sync controller", func() { if err != nil { return false, err } - return syncedCloudConfigMap.Data[defaultConfigKey] == "infra one changed", nil + return syncedCloudConfigMap.Data[defaultConfigKey] == changedInfraConfigString, nil }).Should(BeTrue()) }) It("all keys from cloud-config should be synced", func() { + + changedInfraConfigString := `{"cloud":"AzurePublicCloud","tenantId":"0000000-1234-1234-0000-000000000000","subscriptionId":"0000000-0000-0000-0000-000000000000","vmType":"standard","putVMSSVMBatchSize":0,"enableMigrateToIPBasedBackendPoolAPI":false}` changedManagedConfig := managedCloudConfig.DeepCopy() changedManagedConfig.Data = map[string]string{ - infraCloudConfKey: "infra config", cloudProviderConfigCABundleConfigMapKey: "some pem there", + infraCloudConfKey: changedInfraConfigString, cloudProviderConfigCABundleConfigMapKey: "some pem there", "baz": "fizz", } Expect(cl.Update(ctx, changedManagedConfig)).Should(Succeed())