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 <[email protected]>

Author: nrb

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)
 	}

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.

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,40 +190,51 @@ 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,
 		configv1.PowerVSPlatformType,
 		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 {

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,21 +459,28 @@ 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())
 
 			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() {