From f940a7caee1af7351853194313be8fb5a00a6cc6 Mon Sep 17 00:00:00 2001 From: Theo Barber-Bany Date: Wed, 20 Dec 2023 17:10:08 +0000 Subject: [PATCH 1/9] Adds CloudConfigTransformer for Azure This adds a CloudConfigTransformer for Azure that will set the VMType to 'standard' if unset. See #291, OCPBUGS-25483 and OCPBUGS-20213 for more information. --- pkg/cloud/azure/azure.go | 25 +++++++++++++++++++ pkg/cloud/cloud.go | 3 ++- .../cloud_config_sync_controller_test.go | 12 +++++++-- 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/pkg/cloud/azure/azure.go b/pkg/cloud/azure/azure.go index 0a4ff602d..fa97d9c5b 100644 --- a/pkg/cloud/azure/azure.go +++ b/pkg/cloud/azure/azure.go @@ -2,12 +2,17 @@ package azure import ( "embed" + "encoding/json" "fmt" "github.com/asaskevich/govalidator" + configv1 "github.com/openshift/api/config/v1" appsv1 "k8s.io/api/apps/v1" "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" ) @@ -85,3 +90,23 @@ func NewProviderAssets(config config.OperatorConfig) (common.CloudProviderAssets } return assets, nil } + +func CloudConfigTransformer(source string, infra *configv1.Infrastructure, network *configv1.Network) (string, error) { + var cfg azure.Config + if err := json.Unmarshal([]byte(source), &cfg); err != nil { + return "", fmt.Errorf("failed to unmarshal the cloud.conf: %w", err) + } + + // 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/cloud.go b/pkg/cloud/cloud.go index 666b07bf0..237037fe8 100644 --- a/pkg/cloud/cloud.go +++ b/pkg/cloud/cloud.go @@ -53,7 +53,8 @@ func GetCloudConfigTransformer(platformStatus *configv1.PlatformStatus) (cloudCo if azurestack.IsAzureStackHub(platformStatus) { return azurestack.CloudConfigTransformer, true, nil } - return nil, true, nil + // We need to use the azure CloudConfigTransformer to fix OCPBUGS-25483 + 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..b046da5c4 100644 --- a/pkg/controllers/cloud_config_sync_controller_test.go +++ b/pkg/controllers/cloud_config_sync_controller_test.go @@ -68,14 +68,22 @@ 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: `{ + "cloud":"AzurePublicCloud", + "tenantId": "0000000-0000-0000-0000-000000000000", + "subscriptionId": "0000000-0000-0000-0000-000000000000" +}`}} } 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": `{ + "cloud":"AzurePublicCloud", + "tenantId": "0000000-0000-0000-0000-000000000000", + "subscriptionId": "0000000-0000-0000-0000-000000000000" +}`}} } var _ = Describe("isCloudConfigEqual reconciler method", func() { From 64e61fe7f8981680ea4bbeccdb42cad7d5c19586 Mon Sep 17 00:00:00 2001 From: Theo Barber-Bany Date: Fri, 22 Dec 2023 15:11:04 +0000 Subject: [PATCH 2/9] Updates Azure CloudConfigTransformer to set cloud This change adds the functionality from the cluster-config-operator azure CloudConfigTransformer. The functionality we bring over is: 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 See https://github.com/openshift/cluster-config-operator/pull/133 for the PR that initially added this functionality. --- pkg/cloud/azure/azure.go | 68 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/pkg/cloud/azure/azure.go b/pkg/cloud/azure/azure.go index fa97d9c5b..61733643e 100644 --- a/pkg/cloud/azure/azure.go +++ b/pkg/cloud/azure/azure.go @@ -4,10 +4,12 @@ import ( "embed" "encoding/json" "fmt" + "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" @@ -28,6 +30,24 @@ var ( } ) +var ( + validAzureCloudNames = map[configv1.AzureCloudEnvironment]bool{ + configv1.AzurePublicCloud: true, + configv1.AzureUSGovernmentCloud: true, + configv1.AzureChinaCloud: true, + configv1.AzureGermanCloud: true, + configv1.AzureStackCloud: true, + } + + validAzureCloudNameValues = func() []string { + v := make([]string, 0, len(validAzureCloudNames)) + for n := range validAzureCloudNames { + v = append(v, string(n)) + } + return v + }() +) + type imagesReference struct { CloudControllerManager string `valid:"required"` CloudControllerManagerOperator string `valid:"required"` @@ -91,12 +111,60 @@ func NewProviderAssets(config config.OperatorConfig) (common.CloudProviderAssets return assets, nil } +func IsAzure(infra *configv1.Infrastructure) bool { + if infra.Status.PlatformStatus != nil && + infra.Status.PlatformStatus.Type == configv1.AzurePlatformType { + 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 to be Azure") + } + 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 !validAzureCloudNames[c] { + return "", field.NotSupported(field.NewPath("status", "platformStatus", "azure", "cloudName"), c, validAzureCloudNameValues) + } + cloud = c + } + } + + // Ensure Cloud is set in cloud.conf matches what is set in 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`) + } + } + + // TODO: Remove when you work this out (before merging) + // Why is cfg.Cloud not typed: type AzureCloudEnvironment string + + // At this point these should always be the same + // comparrison + 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 From d7cc0b91416f21d3c00cc9c09a43aa624ad729e7 Mon Sep 17 00:00:00 2001 From: Theo Barber-Bany Date: Fri, 22 Dec 2023 17:46:27 +0000 Subject: [PATCH 3/9] Adds vmType tests for Azure CloudConfigTransformer Adds tests for the vmType transformations. --- pkg/cloud/azure/azure.go | 5 +- pkg/cloud/azure/azure_test.go | 96 +++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 2 deletions(-) diff --git a/pkg/cloud/azure/azure.go b/pkg/cloud/azure/azure.go index 61733643e..c6adfc458 100644 --- a/pkg/cloud/azure/azure.go +++ b/pkg/cloud/azure/azure.go @@ -113,7 +113,8 @@ func NewProviderAssets(config config.OperatorConfig) (common.CloudProviderAssets func IsAzure(infra *configv1.Infrastructure) bool { if infra.Status.PlatformStatus != nil && - infra.Status.PlatformStatus.Type == configv1.AzurePlatformType { + infra.Status.PlatformStatus.Type == configv1.AzurePlatformType && + infra.Status.PlatformStatus.Azure.CloudName != configv1.AzureStackCloud { return true } return false @@ -121,7 +122,7 @@ func IsAzure(infra *configv1.Infrastructure) bool { func CloudConfigTransformer(source string, infra *configv1.Infrastructure, network *configv1.Network) (string, error) { if !IsAzure(infra) { - return "", fmt.Errorf("invalid platform, expected to be Azure") + return "", fmt.Errorf("invalid platform, expected CloudName to be %s", configv1.AzurePublicCloud) } var cfg azure.Config diff --git a/pkg/cloud/azure/azure_test.go b/pkg/cloud/azure/azure_test.go index c5aca923f..077b62154 100644 --- a/pkg/cloud/azure/azure_test.go +++ b/pkg/cloud/azure/azure_test.go @@ -1,12 +1,23 @@ 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" "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/config" + ratelimitconfig "sigs.k8s.io/cloud-provider-azure/pkg/provider/config" +) + +const ( + infraCloudConfName = "test-config" + infraCloudConfKey = "foo" ) func TestResourcesRenderingSmoke(t *testing.T) { @@ -79,3 +90,88 @@ 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: "Azure sets the vmType to standard", + 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: "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), + }, + } + + 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)) + } + }) + } +} From ab5bfe2e89a00d43ed86bc6ddeb6e972cd3b5f75 Mon Sep 17 00:00:00 2001 From: Theo Barber-Bany Date: Tue, 2 Jan 2024 15:14:53 +0000 Subject: [PATCH 4/9] Updates tests to return Azure PlatformStatus This change stops the tests from segfaulting, as the new config transformer for Azure expects a PlatformStatus to be set. --- pkg/cloud/azure/azure.go | 9 +++++---- .../cloud_config_sync_controller_test.go | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/pkg/cloud/azure/azure.go b/pkg/cloud/azure/azure.go index c6adfc458..05f54468d 100644 --- a/pkg/cloud/azure/azure.go +++ b/pkg/cloud/azure/azure.go @@ -112,10 +112,11 @@ func NewProviderAssets(config config.OperatorConfig) (common.CloudProviderAssets } func IsAzure(infra *configv1.Infrastructure) bool { - if infra.Status.PlatformStatus != nil && - infra.Status.PlatformStatus.Type == configv1.AzurePlatformType && - infra.Status.PlatformStatus.Azure.CloudName != configv1.AzureStackCloud { - return true + if infra.Status.PlatformStatus != nil { + if infra.Status.PlatformStatus.Type == configv1.AzurePlatformType && + (infra.Status.PlatformStatus.Azure.CloudName != configv1.AzureStackCloud) { + return true + } } return false } diff --git a/pkg/controllers/cloud_config_sync_controller_test.go b/pkg/controllers/cloud_config_sync_controller_test.go index b046da5c4..229551f92 100644 --- a/pkg/controllers/cloud_config_sync_controller_test.go +++ b/pkg/controllers/cloud_config_sync_controller_test.go @@ -54,6 +54,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, From 0e4bcb36b3ef9709f61cdb76b42fd3131d3bba3f Mon Sep 17 00:00:00 2001 From: Theo Barber-Bany Date: Tue, 2 Jan 2024 19:48:11 +0000 Subject: [PATCH 5/9] Fixes tests to provide valid JSON This changes the tests to work with valid JSON as it is now a requirement of the azure transformer. --- pkg/cloud/azure/azure_test.go | 3 +- pkg/cloud/cloud.go | 1 - .../cloud_config_sync_controller_test.go | 39 +++++++++---------- 3 files changed, 21 insertions(+), 22 deletions(-) diff --git a/pkg/cloud/azure/azure_test.go b/pkg/cloud/azure/azure_test.go index 077b62154..cc2bc87aa 100644 --- a/pkg/cloud/azure/azure_test.go +++ b/pkg/cloud/azure/azure_test.go @@ -11,8 +11,9 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" azure "sigs.k8s.io/cloud-provider-azure/pkg/provider" - "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/config" ratelimitconfig "sigs.k8s.io/cloud-provider-azure/pkg/provider/config" + + "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/config" ) const ( diff --git a/pkg/cloud/cloud.go b/pkg/cloud/cloud.go index 237037fe8..8bc69807a 100644 --- a/pkg/cloud/cloud.go +++ b/pkg/cloud/cloud.go @@ -53,7 +53,6 @@ func GetCloudConfigTransformer(platformStatus *configv1.PlatformStatus) (cloudCo if azurestack.IsAzureStackHub(platformStatus) { return azurestack.CloudConfigTransformer, true, nil } - // We need to use the azure CloudConfigTransformer to fix OCPBUGS-25483 return azure.CloudConfigTransformer, true, nil case configv1.GCPPlatformType: return common.NoOpTransformer, false, nil diff --git a/pkg/controllers/cloud_config_sync_controller_test.go b/pkg/controllers/cloud_config_sync_controller_test.go index 229551f92..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 { @@ -82,22 +84,14 @@ func makeInfraCloudConfig() *corev1.ConfigMap { return &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{ Name: infraCloudConfName, Namespace: OpenshiftConfigNamespace, - }, Data: map[string]string{infraCloudConfKey: `{ - "cloud":"AzurePublicCloud", - "tenantId": "0000000-0000-0000-0000-000000000000", - "subscriptionId": "0000000-0000-0000-0000-000000000000" -}`}} + }, 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": `{ - "cloud":"AzurePublicCloud", - "tenantId": "0000000-0000-0000-0000-000000000000", - "subscriptionId": "0000000-0000-0000-0000-000000000000" -}`}} + }, Data: map[string]string{"cloud.conf": defaultAzureConfig}} } var _ = Describe("isCloudConfigEqual reconciler method", func() { @@ -150,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] @@ -276,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) { @@ -291,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()) }) @@ -301,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()) @@ -317,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()) }) @@ -340,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) { @@ -350,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()) From ff9bf2f119731e1ca4f259890771ab6be5fa8805 Mon Sep 17 00:00:00 2001 From: Theo Barber-Bany Date: Wed, 3 Jan 2024 17:56:47 +0000 Subject: [PATCH 6/9] Updates comments --- pkg/cloud/azure/azure.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/pkg/cloud/azure/azure.go b/pkg/cloud/azure/azure.go index 05f54468d..d4f0f0a37 100644 --- a/pkg/cloud/azure/azure.go +++ b/pkg/cloud/azure/azure.go @@ -151,7 +151,7 @@ func CloudConfigTransformer(source string, infra *configv1.Infrastructure, netwo } } - // Ensure Cloud is set in cloud.conf matches what is set in infra + // Ensure cloud set in cloud.conf matches infra if cfg.Cloud != "" { if !strings.EqualFold(string(cloud), cfg.Cloud) { return "", @@ -160,13 +160,11 @@ func CloudConfigTransformer(source string, infra *configv1.Infrastructure, netwo } } + cfg.Cloud = string(cloud) + // TODO: Remove when you work this out (before merging) // Why is cfg.Cloud not typed: type AzureCloudEnvironment string - // At this point these should always be the same - // comparrison - 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 From af4ff30e530fd5a2af918135c4ac6170957eca7e Mon Sep 17 00:00:00 2001 From: Theo Barber-Bany Date: Wed, 3 Jan 2024 19:13:41 +0000 Subject: [PATCH 7/9] Adds cloud field tests for Azure CloudConfigTransformer This adds the tests from the CCO: https://github.com/openshift/cluster-config-operator/blob/master/pkg/operator/kube_cloud_config/azure_test.go which is where we have pulled the new functionality from. --- pkg/cloud/azure/azure_test.go | 93 +++++++++++++++++++++++++++++++++-- 1 file changed, 89 insertions(+), 4 deletions(-) diff --git a/pkg/cloud/azure/azure_test.go b/pkg/cloud/azure/azure_test.go index cc2bc87aa..a50d028c2 100644 --- a/pkg/cloud/azure/azure_test.go +++ b/pkg/cloud/azure/azure_test.go @@ -137,6 +137,12 @@ func TestCloudConfigTransformer(t *testing.T) { 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), + }, + { // extend this to include Cloud so we don't duplicate name: "Azure sets the vmType to standard", source: azure.Config{}, expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}}, @@ -149,10 +155,88 @@ func TestCloudConfigTransformer(t *testing.T) { infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzurePublicCloud), }, { - name: "Non Azure returns an error", + name: "Azure sets the cloud to AzurePublicCloud", + source: azure.Config{}, + expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}}, + infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzurePublicCloud), + }, + { + name: "Azure sets the cloud to AzurePublicCloud", + source: azure.Config{}, + expected: azure.Config{VMType: "standard", 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), + }, + // The matcher should assert err.Error == errMsg - but is not for some + // reason? According to: + // https://onsi.github.io/gomega/#matcherrorexpected-interface the matcher + // asserts that ACTUAL.Error() == EXPECTED + { + name: "Azure throws an error if the infra has an invalid cloud", source: azure.Config{}, - infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzureStackCloud), - errMsg: fmt.Sprintf("invalid platform, expected CloudName to be %s", configv1.AzurePublicCloud), + infra: makeInfrastructureResource(configv1.AzurePlatformType, "AzureAnotherCloud"), + errMsg: "status.platformStatus.azure.cloudName: Unsupported value: \"AzureAnotherCloud\": supported values: \"AzurePublicCloud\", \"AzureUSGovernmentCloud\", \"AzureChinaCloud\", \"AzureGermanCloud\", \"AzureStackCloud\"", + }, + { + 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), }, } @@ -165,12 +249,13 @@ func TestCloudConfigTransformer(t *testing.T) { actual, err := CloudConfigTransformer(string(src), tc.infra, nil) if tc.errMsg != "" { + // TODO: Remove once the above error case is working correctly + fmt.Printf("\n\n err.Error():%s:\n\n", err.Error()) 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)) } }) From 24664edfcd4b4f634c6e50ce05f8e6188e5ce12e Mon Sep 17 00:00:00 2001 From: Theo Barber-Bany Date: Thu, 4 Jan 2024 13:51:08 +0000 Subject: [PATCH 8/9] validAzureCloudNames: use map[T]struct{} `map[T]struct{}` is more efficient than `map[T]bool` --- pkg/cloud/azure/azure.go | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/pkg/cloud/azure/azure.go b/pkg/cloud/azure/azure.go index d4f0f0a37..c28bccd33 100644 --- a/pkg/cloud/azure/azure.go +++ b/pkg/cloud/azure/azure.go @@ -31,12 +31,12 @@ var ( ) var ( - validAzureCloudNames = map[configv1.AzureCloudEnvironment]bool{ - configv1.AzurePublicCloud: true, - configv1.AzureUSGovernmentCloud: true, - configv1.AzureChinaCloud: true, - configv1.AzureGermanCloud: true, - configv1.AzureStackCloud: true, + validAzureCloudNames = map[configv1.AzureCloudEnvironment]struct{}{ + configv1.AzurePublicCloud: struct{}{}, + configv1.AzureUSGovernmentCloud: struct{}{}, + configv1.AzureChinaCloud: struct{}{}, + configv1.AzureGermanCloud: struct{}{}, + configv1.AzureStackCloud: struct{}{}, } validAzureCloudNameValues = func() []string { @@ -111,6 +111,9 @@ 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 && @@ -144,7 +147,7 @@ func CloudConfigTransformer(source string, infra *configv1.Infrastructure, netwo cloud := configv1.AzurePublicCloud if azurePlatform := infra.Status.PlatformStatus.Azure; azurePlatform != nil { if c := azurePlatform.CloudName; c != "" { - if !validAzureCloudNames[c] { + if _, ok := validAzureCloudNames[c]; !ok { return "", field.NotSupported(field.NewPath("status", "platformStatus", "azure", "cloudName"), c, validAzureCloudNameValues) } cloud = c @@ -159,12 +162,8 @@ func CloudConfigTransformer(source string, infra *configv1.Infrastructure, netwo cloud.conf conflicts with infrastructure object`) } } - cfg.Cloud = string(cloud) - // TODO: Remove when you work this out (before merging) - // Why is cfg.Cloud not typed: type AzureCloudEnvironment string - // 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 From cf0220e4c7dcf5ebf4081c98eb19576504a49d99 Mon Sep 17 00:00:00 2001 From: Theo Barber-Bany Date: Thu, 4 Jan 2024 14:09:24 +0000 Subject: [PATCH 9/9] Return sorted validAzureCloudNameValues This ensures that the list validAzureCloudNameValues is in a stable order, making testing a lot more straight forward --- pkg/cloud/azure/azure.go | 2 ++ pkg/cloud/azure/azure_test.go | 24 +++--------------------- 2 files changed, 5 insertions(+), 21 deletions(-) diff --git a/pkg/cloud/azure/azure.go b/pkg/cloud/azure/azure.go index c28bccd33..c9cb043aa 100644 --- a/pkg/cloud/azure/azure.go +++ b/pkg/cloud/azure/azure.go @@ -4,6 +4,7 @@ import ( "embed" "encoding/json" "fmt" + "slices" "strings" "github.com/asaskevich/govalidator" @@ -44,6 +45,7 @@ var ( for n := range validAzureCloudNames { v = append(v, string(n)) } + slices.Sort(v) return v }() ) diff --git a/pkg/cloud/azure/azure_test.go b/pkg/cloud/azure/azure_test.go index a50d028c2..21d2cb218 100644 --- a/pkg/cloud/azure/azure_test.go +++ b/pkg/cloud/azure/azure_test.go @@ -142,8 +142,8 @@ func TestCloudConfigTransformer(t *testing.T) { infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzureStackCloud), errMsg: fmt.Sprintf("invalid platform, expected CloudName to be %s", configv1.AzurePublicCloud), }, - { // extend this to include Cloud so we don't duplicate - name: "Azure sets the vmType to standard", + { + 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), @@ -154,18 +154,6 @@ func TestCloudConfigTransformer(t *testing.T) { 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", - source: azure.Config{}, - expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}}, - infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzurePublicCloud), - }, - { - name: "Azure sets the cloud to AzurePublicCloud", - source: azure.Config{}, - expected: azure.Config{VMType: "standard", 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{ @@ -198,15 +186,11 @@ func TestCloudConfigTransformer(t *testing.T) { expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzureGermanCloud)}}, infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzureGermanCloud), }, - // The matcher should assert err.Error == errMsg - but is not for some - // reason? According to: - // https://onsi.github.io/gomega/#matcherrorexpected-interface the matcher - // asserts that ACTUAL.Error() == EXPECTED { 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: \"AzurePublicCloud\", \"AzureUSGovernmentCloud\", \"AzureChinaCloud\", \"AzureGermanCloud\", \"AzureStackCloud\"", + 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", @@ -249,8 +233,6 @@ func TestCloudConfigTransformer(t *testing.T) { actual, err := CloudConfigTransformer(string(src), tc.infra, nil) if tc.errMsg != "" { - // TODO: Remove once the above error case is working correctly - fmt.Printf("\n\n err.Error():%s:\n\n", err.Error()) g.Expect(err).Should(MatchError(tc.errMsg)) g.Expect(actual).Should(Equal("")) } else {