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

nrb · 2025-08-05T21:49:59Z

Prior to 4.14, OCP's installer did not provide any cloud configuration in the Infrastructure resource. This was ok because nothing relied on it.

However in 4.19, we introduced a hard requirement for the cloud configuration on AWS.

This change allows us to gracefully handle missing information in Infrastructure by providing a minimal default cloud.conf on AWS.

openshift-ci · 2025-08-05T21:50:03Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2025-08-05T21:50:05Z

@nrb: This pull request references Jira Issue OCPBUGS-59251, which is invalid:

expected the bug to target the "4.20.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2025-08-05T21:50:36Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign joelspeed for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nrb · 2025-08-05T21:50:53Z

/jira refresh

openshift-ci-robot · 2025-08-05T21:51:04Z

@nrb: This pull request references Jira Issue OCPBUGS-59251, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.20.0) matches configured target version for branch (4.20.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @sunzhaohua2

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

nrb · 2025-08-05T21:51:32Z

This is not currently passing units, hence draft status.

JoelSpeed · 2025-08-07T15:09:13Z

pkg/controllers/cloud_config_sync_controller.go

-	case configv1.AzurePlatformType,
-		configv1.GCPPlatformType,
-		configv1.VSpherePlatformType,
-		configv1.IBMCloudPlatformType,
-		configv1.PowerVSPlatformType,
-		configv1.OpenStackPlatformType,
-		configv1.NutanixPlatformType:


Does removing this mean that we will now sync a metal platform configmap? Or does the sync controller just not run at all on metal?

It does run on metal, and removing this caused their tests to fail. I've added it back and simply moved AWS's entry up. This way we're not changing behavior on platforms that relied on it before.

JoelSpeed · 2025-08-07T15:10:20Z

pkg/controllers/cloud_config_sync_controller.go

+
+	// If there was no relevant data in the configmap, make a default entry for it.
+	cloudConfCm.Data[defaultConfigKey] = ""
+
+	return cloudConfCm, nil


Not sure about this one. If the user has specced a custom key and that custom key isn't there, we probably still want to error

I assume our fallback detects an that neither the managed or unmanaged confs exist, so creates a blank entry which exists this function on L185? So this change shouldn't be needed?

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

nrb · 2025-08-07T18:47:24Z

pkg/cloud/aws/aws_config_transformer.go

 	if len(source) == 0 {
-		return nil, fmt.Errorf("empty INI file")
+		source = defaultConfig


I'm torn on this - should we expand the CloudConfigTransformer interface to provide the defaults? Or is it enough for each platform to set their minimum requirements during parsing?

I suspect each platform can set their minimum requirements? Can they each handle an empty file? Is that something we need to add testing for?

Or is it enough for each platform to set their minimum requirements during parsing?

I think for now, this is likely fine

openshift-ci-robot · 2025-08-07T18:52:24Z

@nrb: This pull request references Jira Issue OCPBUGS-59251, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.20.0) matches configured target version for branch (4.20.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @sunzhaohua2

In response to this:

Prior to 4.14, OCP's installer did not provide any cloud configuration in the Infrastructure resource. This was ok because nothing relied on it.

However in 4.19, we introduced a hard requirement for the cloud configuration on AWS.

This change allows us to gracefully handle missing information in Infrastructure by providing a minimal default cloud.conf on AWS.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2025-08-07T22:12:17Z

@nrb: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-gcp-ovn	`e135317`	link	false	`/test e2e-gcp-ovn`
ci/prow/lint	`e135317`	link	true	`/test lint`
ci/prow/e2e-azure-ovn	`e135317`	link	false	`/test e2e-azure-ovn`
ci/prow/level0-clusterinfra-azure-ipi-proxy-tests	`e135317`	link	false	`/test level0-clusterinfra-azure-ipi-proxy-tests`
ci/prow/e2e-gcp-ovn-upgrade	`e135317`	link	false	`/test e2e-gcp-ovn-upgrade`
ci/prow/e2e-openstack-ovn	`e135317`	link	false	`/test e2e-openstack-ovn`
ci/prow/okd-scos-e2e-aws-ovn	`e135317`	link	false	`/test okd-scos-e2e-aws-ovn`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

theobarberbany · 2025-08-08T11:12:33Z

pkg/controllers/cloud_config_sync_controller.go

@@ -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,


What's with the reordering here? Not sure I'm following this..

It looks like it is not just a reordering, the AWSPlatform now coincides in behaviour with all the other ones

Dam's correct - AWS is now unconditionally trying to sync. Any platform not specifically called out in the list, like bare metal, will not sync.

theobarberbany · 2025-08-08T11:14:13Z

pkg/controllers/cloud_config_sync_controller_test.go

+			Expect(cl.Status().Update(ctx, infraResource.DeepCopy())).To(Succeed())
+
+			_, err := reconciler.Reconcile(context.TODO(), ctrl.Request{})
+			Expect(err).To(Not(BeNil()))


do we wanna expect a specific error - namely "key %s specified in infra resource does not exist in source configmap %s", ?

damdo

Thanks! Left a couple of thoughts about some logic conditions

damdo · 2025-08-08T12:45:21Z

pkg/controllers/cloud_config_sync_controller.go

@@ -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,


It looks like it is not just a reordering, the AWSPlatform now coincides in behaviour with all the other ones

damdo · 2025-08-08T12:58:28Z

pkg/controllers/cloud_config_sync_controller.go

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


unmanaged cloud-config

It looks like above this is referred to as the infrastructure config IIUC
Here we refer to it as unmanaged cloud-config

What is the least surprising name we can use in the logs to refer to this?

Also judging from the code above the default cloud config is an empty one right?
Should we bring that up in the log?

damdo · 2025-08-08T13:00:38Z

pkg/controllers/cloud_config_sync_controller.go

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


It looks like we swallow the error here (I know we were already doing that), does it make sense to propagate it?

damdo · 2025-08-08T13:08:19Z

pkg/controllers/cloud_config_sync_controller.go

 			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


Also with this change it now means that if we enter this else if err != nil branch there was an error in getting the "unmanaged" cloud-config and that error is not the IsNotFound one.

So we should keep retrying to get it (likely it is a transient error e.g. network hiccup or similar) rather than just continuing with the "default" and empty cloud-config, as my understanding is that we should only be using the empty one when one doesn't exist.

damdo · 2025-08-08T13:09:27Z

pkg/controllers/cloud_config_sync_controller.go

 func (r *CloudConfigReconciler) prepareSourceConfigMap(source *corev1.ConfigMap, infra *configv1.Infrastructure) (*corev1.ConfigMap, error) {
+	cloudConfCm := source.DeepCopy()


Can source ever be nil? Should we check it here again for good measure in case the code changes as this is implied?

damdo · 2025-08-08T13:15:32Z

pkg/controllers/cloud_config_sync_controller.go

+	// 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


It looks like it is possible to end up here even if the user specified an infraConfigKey and the len(cloudConfCm.Data) == 0, so either we need to fix up the comment at line 234, or we need to account for this extra case in the logic.

4.20's [1,2] are still Post as the potential fix is discussed. And once the fix lands in 4.20/dev branches, it would still need backporting to the 4.19 branch. [1]: https://issues.redhat.com/browse/OCPBUGS-59251 [2]: openshift/cluster-cloud-controller-manager-operator#404

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 5, 2025

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Aug 5, 2025

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Aug 5, 2025

openshift-ci bot requested a review from sunzhaohua2 August 5, 2025 21:51

JoelSpeed reviewed Aug 7, 2025

View reviewed changes

nrb force-pushed the OCPBUGS-59251 branch from e6eedb0 to 38cbb53 Compare August 7, 2025 18:00

Generate a default cloud.conf on AWS

e135317

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

nrb force-pushed the OCPBUGS-59251 branch from 38cbb53 to e135317 Compare August 7, 2025 18:45

nrb commented Aug 7, 2025

View reviewed changes

nrb marked this pull request as ready for review August 7, 2025 18:47

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 7, 2025

openshift-ci bot requested review from damdo and theobarberbany August 7, 2025 18:48

theobarberbany reviewed Aug 8, 2025

View reviewed changes

damdo reviewed Aug 8, 2025

View reviewed changes

		func (r CloudConfigReconciler) prepareSourceConfigMap(source corev1.ConfigMap, infra configv1.Infrastructure) (corev1.ConfigMap, error) {
		cloudConfCm := source.DeepCopy()

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

nrb commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Aug 5, 2025

Uh oh!

openshift-ci-robot commented Aug 5, 2025

Uh oh!

openshift-ci bot commented Aug 5, 2025

Uh oh!

nrb commented Aug 5, 2025

Uh oh!

openshift-ci-robot commented Aug 5, 2025

Uh oh!

nrb commented Aug 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theobarberbany Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Aug 7, 2025

Uh oh!

openshift-ci bot commented Aug 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damdo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nrb commented Aug 5, 2025 •

edited

Loading

theobarberbany Aug 8, 2025 •

edited

Loading