MCO-1961: Rework MC's OSImageURL merge logic

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,12 @@ 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 = ""
+	mcfg.Spec.BaseOSExtensionsContainerImage = ""
 
 	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 {