Merge pull request #404 from nrb/OCPBUGS-59251

OCPBUGS-59251: Default cloud.conf if no configmap is found

Author: openshift-merge-bot[bot]

pkg/cloud/aws/aws_config_transformer.go

@@ -13,8 +13,13 @@ 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]
+`
+
 // 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 +31,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/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/controllers/cloud_config_sync_controller.go

@@ -108,13 +108,16 @@ 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 {
-			klog.Errorf("unable to get cloud-config for sync")
+		if err := r.Get(ctx, openshiftUnmanagedCMKey, sourceCM); errors.IsNotFound(err) {
+			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: %v", err)
 			if err := r.setDegradedCondition(ctx); err != nil {
 				return ctrl.Result{}, fmt.Errorf("failed to set conditions for cloud config controller: %v", err)
 			}
@@ -188,40 +191,57 @@ 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) {
+	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 {
+		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
+	} 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...
 	infraConfigKey := infra.Spec.CloudConfig.Key
-	if val, ok := cloudConfCm.Data[infraConfigKey]; ok {
-		cloudConfCm.Data[defaultConfigKey] = val
-		delete(cloudConfCm.Data, infraConfigKey)
-		return cloudConfCm, nil
+	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),
+			)
+		}
 	}
-	return nil, fmt.Errorf(
-		"key %s specified in infra resource does not found in source configmap %s",
-		infraConfigKey, client.ObjectKeyFromObject(source),
-	)
+
+	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,29 @@ 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 = ""
+			infraResource.Spec.CloudConfig.Key = ""
 			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 +500,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.Error()).To(ContainSubstring("specified in infra resource does not exist in source configmap"))
+
+		})
 	})
 
 	Context("On Azure platform", func() {