OCPBUGS-59251: Default cloud.conf if no configmap is found #404
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -108,17 +108,19 @@ func (r *CloudConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) | |
| } | ||
| } | ||
|
|
||
| // 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); errors.IsNotFound(err) { | ||
| klog.Warningf("unmanaged cloud-config is not found, falling back to default cloud config.") | ||
|
There was a problem hiding this comment.
It looks like above this is referred to as the infrastructure config IIUC What is the least surprising name we can use in the logs to refer to this? Also judging from the code above the |
||
| } else if err != nil { | ||
| klog.Errorf("unable to get cloud-config for sync") | ||
|
There was a problem hiding this comment. It looks like we swallow the error here (I know we were already doing that), does it make sense to propagate it? |
||
| if err := r.setDegradedCondition(ctx); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to set conditions for cloud config controller: %v", err) | ||
| } | ||
|
There was a problem hiding this comment. Also with this change it now means that if we enter this 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. |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -188,40 +190,51 @@ func (r *CloudConfigReconciler) isCloudConfigSyncNeeded(platformStatus *configv1 | |
| return false, fmt.Errorf("platformStatus is required") | ||
| } | ||
| switch platformStatus.Type { | ||
| case configv1.AWSPlatformType, | ||
|
There was a problem hiding this comment. What's with the reordering here? Not sure I'm following this.. There was a problem hiding this comment. It looks like it is not just a reordering, the AWSPlatform now coincides in behaviour with all the other ones There was a problem hiding this comment. 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. |
||
| configv1.AzurePlatformType, | ||
| configv1.GCPPlatformType, | ||
| configv1.VSpherePlatformType, | ||
| configv1.IBMCloudPlatformType, | ||
| configv1.PowerVSPlatformType, | ||
| configv1.OpenStackPlatformType, | ||
| configv1.NutanixPlatformType: | ||
| return true, 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() | ||
|
There was a problem hiding this comment. Can source ever be nil? Should we check it here again for good measure in case the code changes as this is implied? |
||
| // 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 | ||
| 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), | ||
| ) | ||
| } | ||
|
|
||
| // 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 | ||
|
Comment on lines
+234
to
+237
There was a problem hiding this comment. It looks like it is possible to end up here even if the user specified an |
||
| } | ||
|
|
||
| func (r *CloudConfigReconciler) isCloudConfigEqual(source *corev1.ConfigMap, target *corev1.ConfigMap) bool { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm torn on this - should we expand the
CloudConfigTransformerinterface to provide the defaults? Or is it enough for each platform to set their minimum requirements during parsing?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for now, this is likely fine