Address review comments

Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>

Author: nrb

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]
 `
 

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
 `,
 		},
 		{

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.

pkg/controllers/cloud_config_sync_controller.go

@@ -115,12 +115,13 @@ 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)
 			}
+			return ctrl.Result{}, err
 		}
 	}
 
@@ -206,6 +207,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 +220,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
 }
 

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