OCPBUGS-79539: fix(cpo-v2): preserve HCCO modifications to OCM Controllers field

control-plane-operator/controllers/hostedcontrolplane/v2/ocm/config.go

@@ -17,6 +17,8 @@ import (
 	openshiftcpv1 "github.com/openshift/api/openshiftcontrolplane/v1"
 
 	corev1 "k8s.io/api/core/v1"
+
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 const (
@@ -34,6 +36,20 @@ func adaptConfigMap(cpContext component.WorkloadContext, cm *corev1.ConfigMap) e
 		return fmt.Errorf("unable to decode existing openshift controller manager configuration: %w", err)
 	}
 
+	// Fetch the existing ConfigMap from the cluster to preserve HCCO modifications
+	// (e.g., Controllers field when registry is disabled via managementState: Removed)
+	var existingControllers []string
+	existingCM := &corev1.ConfigMap{}
+	err = cpContext.Client.Get(cpContext.Context, client.ObjectKeyFromObject(cm), existingCM)
+	if err == nil && existingCM.Data != nil {
+		if existingConfigStr, exists := existingCM.Data[configKey]; exists && len(existingConfigStr) > 0 {
+			existingConfig := &openshiftcpv1.OpenShiftControllerManagerConfig{}
+			if err := util.DeserializeResource(existingConfigStr, existingConfig, api.Scheme); err == nil {
+				existingControllers = existingConfig.Controllers
+			}
+		}
+	}
+
 	observedConfig := &globalconfig.ObservedConfig{}
 	if err := globalconfig.ReadObservedConfig(cpContext, cpContext.Client, observedConfig, cpContext.HCP.Namespace); err != nil {
 		return fmt.Errorf("failed to read observed global config: %w", err)
@@ -43,7 +59,7 @@ func adaptConfigMap(cpContext component.WorkloadContext, cm *corev1.ConfigMap) e
 	if err != nil {
 		return err
 	}
-	adaptConfig(ocmConfig, cpContext.HCP.Spec.Configuration, cpContext.ReleaseImageProvider, observedConfig.Build, cpContext.HCP.Spec.Capabilities, featureGates)
+	adaptConfig(ocmConfig, cpContext.HCP.Spec.Configuration, cpContext.ReleaseImageProvider, observedConfig.Build, cpContext.HCP.Spec.Capabilities, featureGates, existingControllers)
 	configStr, err := util.SerializeResource(ocmConfig, api.Scheme)
 	if err != nil {
 		return fmt.Errorf("failed to serialize openshift controller manager configuration: %w", err)
@@ -52,13 +68,19 @@ func adaptConfigMap(cpContext component.WorkloadContext, cm *corev1.ConfigMap) e
 	return nil
 }
 
-func adaptConfig(cfg *openshiftcpv1.OpenShiftControllerManagerConfig, configuration *hyperv1.ClusterConfiguration, releaseImageProvider imageprovider.ReleaseImageProvider, buildConfig *configv1.Build, caps *hyperv1.Capabilities, featureGates []string) {
+func adaptConfig(cfg *openshiftcpv1.OpenShiftControllerManagerConfig, configuration *hyperv1.ClusterConfiguration, releaseImageProvider imageprovider.ReleaseImageProvider, buildConfig *configv1.Build, caps *hyperv1.Capabilities, featureGates []string, existingControllers []string) {
 	cfg.Build.ImageTemplateFormat.Format = releaseImageProvider.GetImage("docker-builder")
 	cfg.Deployer.ImageTemplateFormat.Format = releaseImageProvider.GetImage("deployer")
 
+	// Preserve any existing Controllers configuration (e.g., modifications by HCCO)
+	// Only override if the ImageRegistry capability is explicitly disabled
 	if !capabilities.IsImageRegistryCapabilityEnabled(caps) {
 		cfg.Controllers = []string{"*", fmt.Sprintf("-%s", openshiftcpv1.OpenShiftServiceAccountPullSecretsController)}
 		cfg.DockerPullSecret.InternalRegistryHostname = ""
+	} else if len(existingControllers) > 0 {
+		// Preserve existing Controllers field from the cluster to maintain any HCCO modifications
+		// (e.g., when registry is disabled via managementState: Removed instead of capability)
+		cfg.Controllers = existingControllers
 	}
 
 	if configuration != nil && configuration.Image != nil {

control-plane-operator/controllers/hostedcontrolplane/v2/ocm/config_test.go

@@ -74,7 +74,7 @@ func TestReconcileOpenShiftControllerManagerConfig(t *testing.T) {
 				t.Fatalf("unable to decode existing openshift controller manager configuration: %v", err)
 			}
 
-			adaptConfig(config, hcp.Spec.Configuration, imageProvider, buildConfig, hcp.Spec.Capabilities, []string{"foo=true", "bar=false"})
+			adaptConfig(config, hcp.Spec.Configuration, imageProvider, buildConfig, hcp.Spec.Capabilities, []string{"foo=true", "bar=false"}, nil)
 			configStr, err := util.SerializeResource(config, api.Scheme)
 			if err != nil {
 				t.Fatalf("failed to serialize openshift controller manager configuration: %v", err)
@@ -97,3 +97,89 @@ func TestReconcileOpenShiftControllerManagerConfig(t *testing.T) {
 	t.Run("WithAllCapabilitiesEnabled", testFunc(nil))
 	t.Run("WithCapabilitiesEnabledAndDisabled", testFunc(caps))
 }
+
+func TestAdaptConfig_PreservesExistingControllers(t *testing.T) {
+	hcp := &hyperv1.HostedControlPlane{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test",
+			Namespace: "test-namespace",
+		},
+		Spec: hyperv1.HostedControlPlaneSpec{
+			ReleaseImage: "quay.io/ocp-dev/test-release-image:latest",
+			Platform: hyperv1.PlatformSpec{
+				Type: hyperv1.AWSPlatform,
+			},
+			IssuerURL: "https://www.example.com",
+			Configuration: &hyperv1.ClusterConfiguration{
+				Image: &v1.ImageSpec{},
+			},
+			// ImageRegistry capability is enabled (default)
+		},
+	}
+	images := map[string]string{
+		"docker-builder": "quay.io/test/docker-builder",
+		"deployer":       "quay.io/test/deployer",
+	}
+	imageProvider := imageprovider.NewFromImages(images)
+
+	// Simulate HCCO setting the Controllers field to disable pull secrets controller
+	// (e.g., when registry is disabled via managementState: Removed)
+	existingControllersFromCluster := []string{"*", "-openshift.io/serviceaccount-pull-secrets"}
+
+	config := &openshiftcpv1.OpenShiftControllerManagerConfig{}
+	config.ServingInfo = &v1.HTTPServingInfo{}
+
+	// Adapt config with ImageRegistry capability enabled (not explicitly disabled)
+	adaptConfig(config, hcp.Spec.Configuration, imageProvider, &v1.Build{}, nil, []string{}, existingControllersFromCluster)
+
+	// Verify that the Controllers field is preserved
+	if len(config.Controllers) != 2 {
+		t.Errorf("expected Controllers to be preserved with 2 entries, got %d: %v", len(config.Controllers), config.Controllers)
+	}
+	if config.Controllers[0] != "*" || config.Controllers[1] != "-openshift.io/serviceaccount-pull-secrets" {
+		t.Errorf("expected Controllers to be preserved as ['*', '-openshift.io/serviceaccount-pull-secrets'], got %v", config.Controllers)
+	}
+}
+
+func TestAdaptConfig_DisabledImageRegistryCapability(t *testing.T) {
+	hcp := &hyperv1.HostedControlPlane{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test",
+			Namespace: "test-namespace",
+		},
+		Spec: hyperv1.HostedControlPlaneSpec{
+			ReleaseImage: "quay.io/ocp-dev/test-release-image:latest",
+			Platform: hyperv1.PlatformSpec{
+				Type: hyperv1.AWSPlatform,
+			},
+			IssuerURL: "https://www.example.com",
+			Configuration: &hyperv1.ClusterConfiguration{
+				Image: &v1.ImageSpec{},
+			},
+			Capabilities: &hyperv1.Capabilities{
+				Disabled: []hyperv1.OptionalCapability{
+					hyperv1.ImageRegistryCapability,
+				},
+			},
+		},
+	}
+	images := map[string]string{
+		"docker-builder": "quay.io/test/docker-builder",
+		"deployer":       "quay.io/test/deployer",
+	}
+	imageProvider := imageprovider.NewFromImages(images)
+
+	config := &openshiftcpv1.OpenShiftControllerManagerConfig{}
+	config.ServingInfo = &v1.HTTPServingInfo{}
+
+	// Adapt config with ImageRegistry capability explicitly disabled
+	adaptConfig(config, hcp.Spec.Configuration, imageProvider, &v1.Build{}, hcp.Spec.Capabilities, []string{}, nil)
+
+	// Verify that the Controllers field is set to disable pull secrets controller
+	if len(config.Controllers) != 2 {
+		t.Errorf("expected Controllers to be set with 2 entries, got %d: %v", len(config.Controllers), config.Controllers)
+	}
+	if config.Controllers[0] != "*" || config.Controllers[1] != "-openshift.io/serviceaccount-pull-secrets" {
+		t.Errorf("expected Controllers to be set as ['*', '-openshift.io/serviceaccount-pull-secrets'], got %v", config.Controllers)
+	}
+}

control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go

@@ -18,6 +18,8 @@ import (
 	"github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/cvo"
 	cpomanifests "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/manifests"
 	cpoauth "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/oauth"
+	"github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/ocm"
+	"github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/api"
 	alerts "github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/resources/alerts"
 	azureresources "github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/resources/azure"
 	"github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/resources/cco"
@@ -52,6 +54,7 @@ import (
 	"github.com/openshift/api/annotations"
 	configv1 "github.com/openshift/api/config/v1"
 	imageregistryv1 "github.com/openshift/api/imageregistry/v1"
+	openshiftcpv1 "github.com/openshift/api/openshiftcontrolplane/v1"
 	operatorv1 "github.com/openshift/api/operator/v1"
 
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
@@ -422,12 +425,70 @@ func (r *reconciler) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result
 		errs = append(errs, fmt.Errorf("failed to reconcile rbac: %w", err))
 	}
 
-	// Reconcile the image registry only if the image registry capability is enabled.
-	// Skip this step if the user explicitly disabled image registry.
+	registryConfig := manifests.Registry()
+	var registryConfigExists bool
+	// Check if the registry config exists
+	if err := r.client.Get(ctx, client.ObjectKeyFromObject(registryConfig), registryConfig); err != nil {
+		if !apierrors.IsNotFound(err) {
+			return ctrl.Result{}, fmt.Errorf("failed to get registry config: %w", err)
+		}
+	} else {
+		registryConfigExists = true
+	}
+
+	// For platforms where cluster-image-registry-operator (CIRO) needs a PVC to be created, bootstrap needs to happen
+	// in CIRO before the registry config is created. For now, this is the case for the OpenStack platform.
+	// If the object exist, we reconcile the registry config for other fields as it should be fine since the PVC would
+	// exist at this point.
 	if capabilities.IsImageRegistryCapabilityEnabled(hcp.Spec.Capabilities) {
-		log.Info("reconciling image registry")
-		if regErrs := r.reconcileImageRegistry(ctx, hcp); len(regErrs) > 0 {
-			errs = append(errs, regErrs...)
+		if imageRegistryPlatformWithPVC(hcp.Spec.Platform.Type) && (!registryConfigExists || registryConfig == nil) {
+			log.Info("skipping registry config to let CIRO bootstrap")
+		} else {
+			log.Info("reconciling image registry validating admission policy")
+			if r.platformType == hyperv1.AzurePlatform {
+				if err := registry.ReconcileRegistryConfigValidatingAdmissionPolicies(ctx, hcp, r.client, r.CreateOrUpdate); err != nil {
+					errs = append(errs, fmt.Errorf("failed to reconcile image registry validating admission policy: %w", err))
+				}
+			}
+			log.Info("reconciling registry config")
+			if _, err := r.CreateOrUpdate(ctx, r.client, registryConfig, func() error {
+				err = registry.ReconcileRegistryConfig(registryConfig, r.platformType, hcp.Spec.InfrastructureAvailabilityPolicy)
+				if err != nil {
+					return err
+				}
+				return nil
+			}); err != nil {
+				errs = append(errs, fmt.Errorf("failed to reconcile imageregistry config: %w", err))
+			}
+
+			// TODO: remove this when ROSA HCP stops setting the managementState to Removed to disable the Image Registry
+			if registryConfig.Spec.ManagementState == operatorv1.Removed && r.platformType != hyperv1.IBMCloudPlatform && r.platformType != hyperv1.AzurePlatform {
+				log.Info("imageregistry operator managementstate is removed, disabling openshift-controller-manager controllers and cleaning up resources")
+				ocmConfigMap := cpomanifests.OpenShiftControllerManagerConfig(r.hcpNamespace)
+				if _, err := r.CreateOrUpdate(ctx, r.cpClient, ocmConfigMap, func() error {
+					if ocmConfigMap.Data == nil {
+						// CPO has not created the configmap yet, wait for create
+						// This should not happen as we are started by the CPO after the configmap should be created
+						return nil
+					}
+					config := &openshiftcpv1.OpenShiftControllerManagerConfig{}
+					if configStr, exists := ocmConfigMap.Data[ocm.ConfigKey]; exists && len(configStr) > 0 {
+						err := util.DeserializeResource(configStr, config, api.Scheme)
+						if err != nil {
+							return fmt.Errorf("unable to decode existing openshift controller manager configuration: %w", err)
+						}
+					}
+					config.Controllers = []string{"*", fmt.Sprintf("-%s", openshiftcpv1.OpenShiftServiceAccountPullSecretsController)}
+					configStr, err := util.SerializeResource(config, api.Scheme)
+					if err != nil {
+						return fmt.Errorf("failed to serialize openshift controller manager configuration: %w", err)
+					}
+					ocmConfigMap.Data[ocm.ConfigKey] = configStr
+					return nil
+				}); err != nil {
+					errs = append(errs, fmt.Errorf("failed to reconcile openshift-controller-manager config: %w", err))
+				}
+			}
 		}
 	}
 
@@ -3417,52 +3478,3 @@ func imageRegistryPlatformWithPVC(platform hyperv1.PlatformType) bool {
 		return false
 	}
 }
-
-// reconcileImageRegistry reconciles the image registry configuration.
-// It handles:
-// - Platform-specific PVC logic (e.g., OpenStack needs CIRO bootstrap first)
-// - Validating admission policies (Azure only)
-// - Registry configuration reconciliation
-func (r *reconciler) reconcileImageRegistry(
-	ctx context.Context,
-	hcp *hyperv1.HostedControlPlane,
-) []error {
-	log := ctrl.LoggerFrom(ctx)
-	var errs []error
-
-	registryConfig := manifests.Registry()
-	var registryConfigExists bool
-	// Check if the registry config exists
-	if err := r.client.Get(ctx, client.ObjectKeyFromObject(registryConfig), registryConfig); err != nil {
-		if !apierrors.IsNotFound(err) {
-			return []error{fmt.Errorf("failed to get registry config: %w", err)}
-		}
-	} else {
-		registryConfigExists = true
-	}
-
-	// For platforms where cluster-image-registry-operator (CIRO) needs a PVC to be created, bootstrap needs to happen
-	// in CIRO before the registry config is created. For now, this is the case for the OpenStack platform.
-	// If the object exist, we reconcile the registry config for other fields as it should be fine since the PVC would
-	// exist at this point.
-	if imageRegistryPlatformWithPVC(hcp.Spec.Platform.Type) && (!registryConfigExists || registryConfig == nil) {
-		log.Info("skipping registry config to let CIRO bootstrap")
-		return nil
-	}
-
-	log.Info("reconciling image registry validating admission policy")
-	if r.platformType == hyperv1.AzurePlatform {
-		if err := registry.ReconcileRegistryConfigValidatingAdmissionPolicies(ctx, hcp, r.client, r.CreateOrUpdate); err != nil {
-			errs = append(errs, fmt.Errorf("failed to reconcile image registry validating admission policy: %w", err))
-		}
-	}
-
-	log.Info("reconciling registry config")
-	if _, err := r.CreateOrUpdate(ctx, r.client, registryConfig, func() error {
-		return registry.ReconcileRegistryConfig(registryConfig, r.platformType, hcp.Spec.InfrastructureAvailabilityPolicy)
-	}); err != nil {
-		errs = append(errs, fmt.Errorf("failed to reconcile imageregistry config: %w", err))
-	}
-
-	return errs
-}