MCO-1961: Rework MC's OSImageURL merge logic

Template-generated MachineConfigs (e.g., 01-master-kubelet,
01-master-container-runtime) were setting OSImageURL on the final
rendered MachineConfig, even though only user-provided MachineConfigs
should be able to override this field.

The root cause was that template generation explicitly sets OSImageURL
on all generated MCs, and the merge logic (MergeMachineConfigs) was
treating all MCs equally when determining the final OSImageURL value.
This meant template-generated MCs would always propagate the base OS
image URL to the rendered MC, making it impossible for the system to
distinguish between a default value and an intentional override.

This commit fixes the issue by modifying MergeMachineConfigs to skip
any MachineConfig with the
machineconfiguration.openshift.io/generated-by-controller-version
annotation when evaluating OSImageURL and BaseOSExtensionsContainerImage
overrides. This ensures that only user-provided MachineConfigs can
override these fields, while still allowing template-generated MCs to
have the field populated (which is necessary due to resourcemerge not
blanking out previously-set values during upgrades).

The same logic is applied to BaseOSExtensionsContainerImage for
consistency.

Author: pablintino

devex/cmd/onclustertesting/helpers.go

@@ -181,7 +181,7 @@ func deleteAllMachineConfigsForPool(cs *framework.ClientSet, mcp *mcfgv1.Machine
 	for _, mc := range machineConfigs.Items {
 		mc := mc
 		eg.Go(func() error {
-			if _, ok := mc.Annotations[helpers.MCPNameToRole(mcp.Name)]; ok && !strings.HasPrefix(mc.Name, "rendered-") {
+			if _, ok := mc.Annotations[helpers.MCPNameToRole(mcp.Name)]; ok && !strings.HasPrefix(mc.Name, ctrlcommon.RenderedMachineConfigPrefix) {
 				if err := cs.MachineConfigs().Delete(context.TODO(), mc.Name, metav1.DeleteOptions{}); err != nil {
 					return err
 				}

pkg/controller/common/constants.go

@@ -30,6 +30,9 @@ const (
 	// OSImageURLOverriddenKey is used to tag a rendered machineconfig when OSImageURL has been overridden from default using machineconfig
 	OSImageURLOverriddenKey = "machineconfiguration.openshift.io/os-image-url-overridden"
 
+	// RenderedMachineConfigPrefix is the name prefix for rendered MachineConfigs
+	RenderedMachineConfigPrefix = "rendered-"
+
 	// ControllerConfigName is the name of the ControllerConfig object that controllers use
 	ControllerConfigName = "machine-config-controller"
 

pkg/controller/common/helpers.go

@@ -189,6 +189,10 @@ func MergeMachineConfigs(configs []*mcfgv1.MachineConfig, cconfig *mcfgv1.Contro
 	// so the only way we get an override here is if the user adds something different
 	osImageURL := GetDefaultBaseImageContainer(&cconfig.Spec)
 	for _, cfg := range configs {
+		// Ignore generated MCs, only the rendered MC or a user provided MC can set this field
+		if cfg.Annotations[GeneratedByControllerVersionAnnotationKey] != "" {
+			continue
+		}
 		if cfg.Spec.OSImageURL != "" {
 			osImageURL = cfg.Spec.OSImageURL
 		}
@@ -197,6 +201,10 @@ func MergeMachineConfigs(configs []*mcfgv1.MachineConfig, cconfig *mcfgv1.Contro
 	// Allow overriding the extensions container
 	baseOSExtensionsContainerImage := cconfig.Spec.BaseOSExtensionsContainerImage
 	for _, cfg := range configs {
+		// Ignore generated MCs, only the rendered MC or a user provided MC can set this field
+		if cfg.Annotations[GeneratedByControllerVersionAnnotationKey] != "" {
+			continue
+		}
 		if cfg.Spec.BaseOSExtensionsContainerImage != "" {
 			baseOSExtensionsContainerImage = cfg.Spec.BaseOSExtensionsContainerImage
 		}

pkg/controller/render/hash.go

@@ -7,6 +7,7 @@ import (
 
 	"github.com/ghodss/yaml"
 	mcfgv1 "github.com/openshift/api/machineconfiguration/v1"
+	ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common"
 )
 
 var (
@@ -37,7 +38,7 @@ func getMachineConfigHashedName(pool *mcfgv1.MachineConfigPool, config *mcfgv1.M
 	if err != nil {
 		return "", err
 	}
-	return fmt.Sprintf("rendered-%s-%x", pool.GetName(), h), nil
+	return fmt.Sprintf("%s%s-%x", ctrlcommon.RenderedMachineConfigPrefix, pool.GetName(), h), nil
 }
 
 func hashData(data []byte) ([]byte, error) {

pkg/controller/template/render.go

@@ -361,13 +361,11 @@ func generateMachineConfigForName(config *RenderConfig, role, name, templateDir,
 
 	mcfg.Spec.Extensions = append(mcfg.Spec.Extensions, slices.Sorted(maps.Keys(extensions))...)
 
-	// TODO(jkyros): you might think you can remove this since we override later when we merge
-	// config, but resourcemerge doesn't blank this field out once it's populated
-	// so if you end up on a cluster where it was ever populated in this machineconfig, it
-	// will keep that last value forever once you upgrade...which is a problen now that we allow OSImageURL overrides
-	// because it will look like an override when it shouldn't be. So don't take this out until you've solved that.
-	// And inject the osimageurl here
-	mcfg.Spec.OSImageURL = ctrlcommon.GetDefaultBaseImageContainer(config.ControllerConfigSpec)
+	// Note: Previously, the OSImageURL was set here (as well as in the rendered MC) to facilitate
+	// overrides. Now, all the OSImageURL's from generated MCs are ignored and only user provided
+	// MCs with the OSImageURL set are considered during the merge process.
+	// Now the image URL is explicitly cleared to avoid confusion. Its content is never consumed.
+	mcfg.Spec.OSImageURL = ""
 
 	return mcfg, nil
 }

pkg/controller/template/template_controller.go

@@ -6,11 +6,14 @@ import (
 	"crypto/x509"
 	"encoding/json"
 	"encoding/pem"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
 	"reflect"
 	"sort"
+	"strings"
+	"sync"
 	"time"
 
 	mcfgv1 "github.com/openshift/api/machineconfiguration/v1"
@@ -26,7 +29,6 @@ import (
 	configinformersv1 "github.com/openshift/client-go/config/informers/externalversions/config/v1"
 	configlistersv1 "github.com/openshift/client-go/config/listers/config/v1"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -74,6 +76,8 @@ type Controller struct {
 	secretsInformerSynced cache.InformerSynced
 
 	queue workqueue.TypedRateLimitingInterface[string]
+
+	urlClearOnce sync.Once
 }
 
 // New returns a new template controller.
@@ -588,7 +592,7 @@ func (ctrl *Controller) syncControllerConfig(key string) error {
 		return err
 	}
 	controllerconfig, err := ctrl.ccLister.Get(name)
-	if errors.IsNotFound(err) {
+	if apierrors.IsNotFound(err) {
 		klog.V(2).Infof("ControllerConfig %v has been deleted", key)
 		return nil
 	}
@@ -633,6 +637,15 @@ func (ctrl *Controller) syncControllerConfig(key string) error {
 		return ctrl.syncFailingStatus(cfg, err)
 	}
 
+	// TODO: To be removed when 4.22 is EOL
+	// Since 4.22 templated/generated (non-rendered) MCs no longer set the
+	// OS and Extensions URLs and if given, the render controller ignores their
+	// values. To improve UX we remove the URLs from existing MCs once.
+	if clearErr := ctrl.clearImageURLs(); err == nil && apiServer != nil {
+		// Best effort, do not fail
+		klog.Warningf("Clearing MCs URLs has failed: %v", clearErr)
+	}
+
 	mcs, err := getMachineConfigsForControllerConfig(ctrl.templatesDir, cfg, clusterPullSecretRaw, apiServer)
 	if err != nil {
 		return ctrl.syncFailingStatus(cfg, err)
@@ -652,6 +665,39 @@ func (ctrl *Controller) syncControllerConfig(key string) error {
 	return ctrl.syncCompletedStatus(cfg)
 }
 
+// clearImageURLs clears OSImageURL and BaseOSExtensionsContainerImage from template-generated
+// MachineConfigs. This is a one-time migration operation to remove these fields from generated
+// MachineConfigs, as they should only be set on rendered or user-provided MachineConfigs.
+// The function uses sync.Once to ensure it runs only once per controller lifecycle.
+func (ctrl *Controller) clearImageURLs() error {
+	var err error
+	ctrl.urlClearOnce.Do(func() {
+		var mcList *mcfgv1.MachineConfigList
+		mcList, err = ctrl.client.MachineconfigurationV1().MachineConfigs().List(context.TODO(), metav1.ListOptions{})
+		if err != nil {
+			return
+		}
+		for _, mc := range mcList.Items {
+			if mc.Annotations[ctrlcommon.GeneratedByControllerVersionAnnotationKey] == "" || strings.HasPrefix(mc.Name, ctrlcommon.RenderedMachineConfigPrefix) {
+				// It's a rendered MC or a user provided one.
+				// This update code only works with templated/generated MCs
+				continue
+			}
+
+			if mc.Spec.OSImageURL != "" || mc.Spec.BaseOSExtensionsContainerImage != "" {
+				mc.Spec.OSImageURL = ""
+				mc.Spec.BaseOSExtensionsContainerImage = ""
+
+				if _, updateErr := ctrl.client.MachineconfigurationV1().MachineConfigs().Update(context.TODO(), &mc, metav1.UpdateOptions{}); updateErr != nil {
+					err = errors.Join(err, updateErr)
+				}
+				klog.Infof("Removed old imageURLs from MachineConfig %s", mc.Name)
+			}
+		}
+	})
+	return err
+}
+
 func getMachineConfigsForControllerConfig(templatesDir string, config *mcfgv1.ControllerConfig, clusterPullSecretRaw []byte, apiServer *configv1.APIServer) ([]*mcfgv1.MachineConfig, error) {
 	buf := &bytes.Buffer{}
 	if err := json.Compact(buf, clusterPullSecretRaw); err != nil {