From e1353171c5cfb26be15d155e5e308ffcfda56d9a Mon Sep 17 00:00:00 2001 From: Nolan Brubaker Date: Tue, 5 Aug 2025 17:48:49 -0400 Subject: [PATCH 1/2] Generate a default cloud.conf on AWS OpenShift clusters prior to v4.14 did not explicitly need a cloud.conf file for AWS. In v.4.19, they do. This change makes sure that there is a minimally usable default cloud configuration present. Signed-off-by: Nolan Brubaker --- pkg/cloud/aws/aws_config_transformer.go | 15 +++++-- pkg/cloud/cloud.go | 4 ++ .../cloud_config_sync_controller.go | 37 +++++++++++------ .../cloud_config_sync_controller_test.go | 40 ++++++++++++------- 4 files changed, 67 insertions(+), 29 deletions(-) diff --git a/pkg/cloud/aws/aws_config_transformer.go b/pkg/cloud/aws/aws_config_transformer.go index 2caf36ff9..caa862d5f 100644 --- a/pkg/cloud/aws/aws_config_transformer.go +++ b/pkg/cloud/aws/aws_config_transformer.go @@ -13,8 +13,11 @@ import ( awsconfig "k8s.io/cloud-provider-aws/pkg/providers/v1/config" ) +const defaultConfig = `[Global] +` + // CloudConfigTransformer is used to inject OpenShift configuration defaults into the Cloud Provider config -// for the AWS Cloud Provider. +// for the AWS Cloud Provider. If an empty source string is provided, a minimal default configuration will be created. func CloudConfigTransformer(source string, infra *configv1.Infrastructure, network *configv1.Network) (string, error) { cfg, err := readAWSConfig(source) if err != nil { @@ -26,13 +29,19 @@ func CloudConfigTransformer(source string, infra *configv1.Infrastructure, netwo return marshalAWSConfig(cfg) } +// readAWSConfig will parse a source string into a proper *awsconfig.CloudConfig. +// If an empty source string is provided, a default configuration will be used. func readAWSConfig(source string) (*awsconfig.CloudConfig, error) { + cfg := &awsconfig.CloudConfig{} + + // There are cases in which a cloud config was not installed with a cluster, and this is valid. + // We should, however, populate the configuration so that it doesn't fail on later versions that require + // a cloud.conf. if len(source) == 0 { - return nil, fmt.Errorf("empty INI file") + source = defaultConfig } // Use the same method the AWS CCM uses to load configuration. - cfg := &awsconfig.CloudConfig{} if err := gcfg.FatalOnly(gcfg.ReadStringInto(cfg, source)); err != nil { return nil, fmt.Errorf("failed to parse INI file: %w", err) } diff --git a/pkg/cloud/cloud.go b/pkg/cloud/cloud.go index 2ae88af49..c923f1d49 100644 --- a/pkg/cloud/cloud.go +++ b/pkg/cloud/cloud.go @@ -25,6 +25,10 @@ import ( // consumer of the transformer. type cloudConfigTransformer func(source string, infra *configv1.Infrastructure, network *configv1.Network) (string, error) +// defaultCloudConfig returns a default cloud configuration in the event that a cluster does not have a source config map. +// Clusters created prior to 4.14 do not always have a source config map. +type defaultCloudConfig func(platform configv1.PlatformType) (string, error) + // GetCloudConfigTransformer returns the function that should be used to transform // the cloud configuration config map, and a boolean to indicate if the config should // be synced from the CCO namespace before applying the transformation. diff --git a/pkg/controllers/cloud_config_sync_controller.go b/pkg/controllers/cloud_config_sync_controller.go index c77f19948..2d9ce01ae 100644 --- a/pkg/controllers/cloud_config_sync_controller.go +++ b/pkg/controllers/cloud_config_sync_controller.go @@ -108,17 +108,19 @@ func (r *CloudConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) } } - if !managedConfigFound { + // Only look for an unmanaged config if the managed one isn't found and a name was specified. + if !managedConfigFound && infra.Spec.CloudConfig.Name != "" { openshiftUnmanagedCMKey := client.ObjectKey{ Name: infra.Spec.CloudConfig.Name, Namespace: OpenshiftConfigNamespace, } - if err := r.Get(ctx, openshiftUnmanagedCMKey, sourceCM); err != nil { + if err := r.Get(ctx, openshiftUnmanagedCMKey, sourceCM); errors.IsNotFound(err) { + klog.Warningf("unmanaged cloud-config is not found, falling back to default cloud config.") + } else if err != nil { klog.Errorf("unable to get cloud-config for sync") if err := r.setDegradedCondition(ctx); err != nil { return ctrl.Result{}, fmt.Errorf("failed to set conditions for cloud config controller: %v", err) } - return ctrl.Result{}, err } } @@ -188,7 +190,8 @@ func (r *CloudConfigReconciler) isCloudConfigSyncNeeded(platformStatus *configv1 return false, fmt.Errorf("platformStatus is required") } switch platformStatus.Type { - case configv1.AzurePlatformType, + case configv1.AWSPlatformType, + configv1.AzurePlatformType, configv1.GCPPlatformType, configv1.VSpherePlatformType, configv1.IBMCloudPlatformType, @@ -196,32 +199,42 @@ func (r *CloudConfigReconciler) isCloudConfigSyncNeeded(platformStatus *configv1 configv1.OpenStackPlatformType, configv1.NutanixPlatformType: return true, nil - case configv1.AWSPlatformType: - // Some of AWS regions might require to sync a cloud-config, in such case reference in infra resource will be presented - return infraCloudConfigRef.Name != "", nil default: return false, nil } } +// prepareSourceConfigMap creates a usable ConfigMap for further processing into a cloud.conf file. func (r *CloudConfigReconciler) prepareSourceConfigMap(source *corev1.ConfigMap, infra *configv1.Infrastructure) (*corev1.ConfigMap, error) { + cloudConfCm := source.DeepCopy() + // We might have an empty ConfigMap in clusters created before 4.14. + if cloudConfCm.Data == nil { + cloudConfCm.Data = make(map[string]string) + } + // Keys might be different between openshift-config/cloud-config and openshift-config-managed/kube-cloud-config // Always use "cloud.conf" which is default one across openshift - cloudConfCm := source.DeepCopy() if _, ok := cloudConfCm.Data[defaultConfigKey]; ok { return cloudConfCm, nil } + // If a user provides their own cloud config, copy that over into the default key. infraConfigKey := infra.Spec.CloudConfig.Key if val, ok := cloudConfCm.Data[infraConfigKey]; ok { cloudConfCm.Data[defaultConfigKey] = val delete(cloudConfCm.Data, infraConfigKey) return cloudConfCm, nil + } else if !ok && len(cloudConfCm.Data) > 0 { + // Return an error if they provided a non-existent one and there was a cloud.conf specified. + return nil, fmt.Errorf("key %s specified in infra resource does not exist in source configmap %s", + infraConfigKey, client.ObjectKeyFromObject(source), + ) } - return nil, fmt.Errorf( - "key %s specified in infra resource does not found in source configmap %s", - infraConfigKey, client.ObjectKeyFromObject(source), - ) + + // If there was no data in the configmap and the user didn't specify their own make a default entry for it. + cloudConfCm.Data[defaultConfigKey] = "" + + return cloudConfCm, nil } func (r *CloudConfigReconciler) isCloudConfigEqual(source *corev1.ConfigMap, target *corev1.ConfigMap) bool { diff --git a/pkg/controllers/cloud_config_sync_controller_test.go b/pkg/controllers/cloud_config_sync_controller_test.go index e818ef188..5d54a834c 100644 --- a/pkg/controllers/cloud_config_sync_controller_test.go +++ b/pkg/controllers/cloud_config_sync_controller_test.go @@ -28,6 +28,8 @@ const ( infraCloudConfKey = "foo" defaultAzureConfig = `{"cloud":"AzurePublicCloud","tenantId":"0000000-0000-0000-0000-000000000000","Entries":null,"subscriptionId":"0000000-0000-0000-0000-000000000000","vmType":"standard","putVMSSVMBatchSize":0,"enableMigrateToIPBasedBackendPoolAPI":false,"clusterServiceLoadBalancerHealthProbeMode":"shared"}` + defaultAWSConfig = `[Global] +` ) func makeInfrastructureResource(platform configv1.PlatformType) *configv1.Infrastructure { @@ -84,8 +86,7 @@ func makeInfraStatus(platform configv1.PlatformType) configv1.InfrastructureStat } func makeInfraCloudConfig(platform configv1.PlatformType) *corev1.ConfigMap { - defaultConfig := `[Global] -` + defaultConfig := defaultAWSConfig if platform == configv1.AzurePlatformType { defaultConfig = defaultAzureConfig @@ -98,8 +99,7 @@ func makeInfraCloudConfig(platform configv1.PlatformType) *corev1.ConfigMap { } func makeManagedCloudConfig(platform configv1.PlatformType) *corev1.ConfigMap { - defaultConfig := `[Global] -` + defaultConfig := defaultAWSConfig if platform == configv1.AzurePlatformType { defaultConfig = defaultAzureConfig @@ -151,14 +151,6 @@ var _ = Describe("prepareSourceConfigMap reconciler method", func() { Expect(reconciler.isCloudConfigEqual(preparedConfig, managedCloudConfig)).Should(BeTrue()) }) - It("config preparation should fail if key from infra resource does not found", func() { - brokenInfraConfig := infraCloudConfig.DeepCopy() - brokenInfraConfig.Data = map[string]string{"hehehehehe": "bar"} - _, err := reconciler.prepareSourceConfigMap(brokenInfraConfig, infra) - Expect(err).Should(Not(Succeed())) - Expect(err.Error()).Should(BeEquivalentTo("key foo specified in infra resource does not found in source configmap openshift-config/test-config")) - }) - It("config preparation should not touch extra fields in infra ConfigMap", func() { extendedInfraConfig := infraCloudConfig.DeepCopy() extendedInfraConfig.Data = map[string]string{infraCloudConfKey: "{}", "{}": "{}"} @@ -467,7 +459,7 @@ var _ = Describe("Cloud config sync reconciler", func() { Expect(cl.Create(ctx, makeInfraCloudConfig(configv1.AWSPlatformType))).To(Succeed()) }) - It("should skip config sync for AWS platform if there is no reference in infra resource", func() { + It("should sync a default config AWS platform if there is no reference in infra resource", func() { infraResource := makeInfrastructureResource(configv1.AWSPlatformType) infraResource.Spec.CloudConfig.Name = "" Expect(cl.Create(ctx, infraResource)).To(Succeed()) @@ -475,13 +467,20 @@ var _ = Describe("Cloud config sync reconciler", func() { infraResource.Status = makeInfraStatus(infraResource.Spec.PlatformSpec.Type) Expect(cl.Status().Update(ctx, infraResource.DeepCopy())).To(Succeed()) + fetchedResource := &configv1.Infrastructure{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(infraResource), fetchedResource)).To(Succeed()) + Expect(fetchedResource.Spec.CloudConfig.Name).To(Equal("")) + _, err := reconciler.Reconcile(context.TODO(), ctrl.Request{}) Expect(err).To(BeNil()) allCMs := &corev1.ConfigMapList{} Expect(cl.List(ctx, allCMs, &client.ListOptions{Namespace: targetNamespaceName})).To(Succeed()) - Expect(len(allCMs.Items)).To(BeZero()) + Expect(len(allCMs.Items)).To(BeEquivalentTo(1)) + // Our code ensures that there is at a minimum a global section. + // The CCM itself may end up defaulting values for us, so don't use exact string matching. + Expect(allCMs.Items[0].Data[defaultConfigKey]).To(HavePrefix(defaultAWSConfig)) }) It("should perform config sync for AWS platform if there is a reference in infra resource", func() { @@ -500,6 +499,19 @@ var _ = Describe("Cloud config sync reconciler", func() { Expect(len(allCMs.Items)).NotTo(BeZero()) Expect(len(allCMs.Items)).To(BeEquivalentTo(1)) }) + + It("should error if a user-specified configmap key isn't present", func() { + infraResource := makeInfrastructureResource(configv1.AWSPlatformType) + infraResource.Spec.CloudConfig.Key = "notfound" + Expect(cl.Create(ctx, infraResource)).To(Succeed()) + + infraResource.Status = makeInfraStatus(infraResource.Spec.PlatformSpec.Type) + Expect(cl.Status().Update(ctx, infraResource.DeepCopy())).To(Succeed()) + + _, err := reconciler.Reconcile(context.TODO(), ctrl.Request{}) + Expect(err).To(Not(BeNil())) + + }) }) Context("On Azure platform", func() { From 9115b4d00203528672d993f511c98f1aebf95877 Mon Sep 17 00:00:00 2001 From: Nolan Brubaker Date: Mon, 25 Aug 2025 18:20:31 -0400 Subject: [PATCH 2/2] Address review comments Signed-off-by: Nolan Brubaker --- pkg/cloud/aws/aws_config_transformer.go | 2 ++ pkg/cloud/aws/aws_config_transformer_test.go | 11 +++++- pkg/cloud/cloud.go | 4 --- .../cloud_config_sync_controller.go | 36 +++++++++++-------- .../cloud_config_sync_controller_test.go | 3 +- 5 files changed, 35 insertions(+), 21 deletions(-) diff --git a/pkg/cloud/aws/aws_config_transformer.go b/pkg/cloud/aws/aws_config_transformer.go index caa862d5f..594b5093a 100644 --- a/pkg/cloud/aws/aws_config_transformer.go +++ b/pkg/cloud/aws/aws_config_transformer.go @@ -13,6 +13,8 @@ import ( awsconfig "k8s.io/cloud-provider-aws/pkg/providers/v1/config" ) +// defaultConfig is a string holding the absolute bare minimum INI string that the AWS CCM needs to start. +// The value will be further customized for OCP in the CloudConfigTransformer. const defaultConfig = `[Global] ` diff --git a/pkg/cloud/aws/aws_config_transformer_test.go b/pkg/cloud/aws/aws_config_transformer_test.go index 282d8485f..adb857395 100644 --- a/pkg/cloud/aws/aws_config_transformer_test.go +++ b/pkg/cloud/aws/aws_config_transformer_test.go @@ -13,13 +13,22 @@ func TestCloudConfigTransformer(t *testing.T) { expected string }{ { - name: "empty source", + name: "default source", source: `[Global] `, // This is the default that gets created for any OpenShift Cluster. expected: `[Global] DisableSecurityGroupIngress = false ClusterServiceLoadBalancerHealthProbeMode = Shared ClusterServiceSharedLoadBalancerHealthProbePort = 0 +`, + }, + { + name: "completely empty source", + source: "", // This could happen in cases where the cluster was born prior to a cloud.conf being required. + expected: `[Global] +DisableSecurityGroupIngress = false +ClusterServiceLoadBalancerHealthProbeMode = Shared +ClusterServiceSharedLoadBalancerHealthProbePort = 0 `, }, { diff --git a/pkg/cloud/cloud.go b/pkg/cloud/cloud.go index c923f1d49..2ae88af49 100644 --- a/pkg/cloud/cloud.go +++ b/pkg/cloud/cloud.go @@ -25,10 +25,6 @@ import ( // consumer of the transformer. type cloudConfigTransformer func(source string, infra *configv1.Infrastructure, network *configv1.Network) (string, error) -// defaultCloudConfig returns a default cloud configuration in the event that a cluster does not have a source config map. -// Clusters created prior to 4.14 do not always have a source config map. -type defaultCloudConfig func(platform configv1.PlatformType) (string, error) - // GetCloudConfigTransformer returns the function that should be used to transform // the cloud configuration config map, and a boolean to indicate if the config should // be synced from the CCO namespace before applying the transformation. diff --git a/pkg/controllers/cloud_config_sync_controller.go b/pkg/controllers/cloud_config_sync_controller.go index 2d9ce01ae..847a3afa0 100644 --- a/pkg/controllers/cloud_config_sync_controller.go +++ b/pkg/controllers/cloud_config_sync_controller.go @@ -115,9 +115,9 @@ func (r *CloudConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) Namespace: OpenshiftConfigNamespace, } if err := r.Get(ctx, openshiftUnmanagedCMKey, sourceCM); errors.IsNotFound(err) { - klog.Warningf("unmanaged cloud-config is not found, falling back to default cloud config.") + klog.Warningf("managed cloud-config is not found, falling back to default cloud config.") } else if err != nil { - klog.Errorf("unable to get cloud-config for sync") + klog.Errorf("unable to get cloud-config for sync: %v", err) if err := r.setDegradedCondition(ctx); err != nil { return ctrl.Result{}, fmt.Errorf("failed to set conditions for cloud config controller: %v", err) } @@ -206,6 +206,9 @@ func (r *CloudConfigReconciler) isCloudConfigSyncNeeded(platformStatus *configv1 // prepareSourceConfigMap creates a usable ConfigMap for further processing into a cloud.conf file. func (r *CloudConfigReconciler) prepareSourceConfigMap(source *corev1.ConfigMap, infra *configv1.Infrastructure) (*corev1.ConfigMap, error) { + if source == nil { + return nil, fmt.Errorf("received empty configmap for cloud config") + } cloudConfCm := source.DeepCopy() // We might have an empty ConfigMap in clusters created before 4.14. if cloudConfCm.Data == nil { @@ -216,24 +219,27 @@ func (r *CloudConfigReconciler) prepareSourceConfigMap(source *corev1.ConfigMap, // Always use "cloud.conf" which is default one across openshift if _, ok := cloudConfCm.Data[defaultConfigKey]; ok { return cloudConfCm, nil + } else { + // Make an entry for the default key even if it didn't exist. + cloudConfCm.Data[defaultConfigKey] = "" } - // If a user provides their own cloud config, copy that over into the default key. + // If a user provides their own cloud config... infraConfigKey := infra.Spec.CloudConfig.Key - if val, ok := cloudConfCm.Data[infraConfigKey]; ok { - cloudConfCm.Data[defaultConfigKey] = val - delete(cloudConfCm.Data, infraConfigKey) - return cloudConfCm, nil - } else if !ok && len(cloudConfCm.Data) > 0 { - // Return an error if they provided a non-existent one and there was a cloud.conf specified. - return nil, fmt.Errorf("key %s specified in infra resource does not exist in source configmap %s", - infraConfigKey, client.ObjectKeyFromObject(source), - ) + if infraConfigKey != "" { + if val, ok := cloudConfCm.Data[infraConfigKey]; ok { + // ..., copy that over into the default key. + cloudConfCm.Data[defaultConfigKey] = val + delete(cloudConfCm.Data, infraConfigKey) + return cloudConfCm, nil + } else if !ok { + // Return an error if they provided a non-existent one and there was a cloud.conf specified. + return nil, fmt.Errorf("key %s specified in infra resource does not exist in source configmap %s", + infraConfigKey, client.ObjectKeyFromObject(source), + ) + } } - // If there was no data in the configmap and the user didn't specify their own make a default entry for it. - cloudConfCm.Data[defaultConfigKey] = "" - return cloudConfCm, nil } diff --git a/pkg/controllers/cloud_config_sync_controller_test.go b/pkg/controllers/cloud_config_sync_controller_test.go index 5d54a834c..6588ace28 100644 --- a/pkg/controllers/cloud_config_sync_controller_test.go +++ b/pkg/controllers/cloud_config_sync_controller_test.go @@ -462,6 +462,7 @@ var _ = Describe("Cloud config sync reconciler", func() { It("should sync a default config AWS platform if there is no reference in infra resource", func() { infraResource := makeInfrastructureResource(configv1.AWSPlatformType) infraResource.Spec.CloudConfig.Name = "" + infraResource.Spec.CloudConfig.Key = "" Expect(cl.Create(ctx, infraResource)).To(Succeed()) infraResource.Status = makeInfraStatus(infraResource.Spec.PlatformSpec.Type) @@ -509,7 +510,7 @@ var _ = Describe("Cloud config sync reconciler", func() { Expect(cl.Status().Update(ctx, infraResource.DeepCopy())).To(Succeed()) _, err := reconciler.Reconcile(context.TODO(), ctrl.Request{}) - Expect(err).To(Not(BeNil())) + Expect(err.Error()).To(ContainSubstring("specified in infra resource does not exist in source configmap")) }) })