From 0e19951bd2dc8a591121e409ed171f994764b7b5 Mon Sep 17 00:00:00 2001 From: Seth Jennings Date: Wed, 25 Mar 2026 14:11:11 -0500 Subject: [PATCH] fix(cpo-v2): preserve HCCO modifications to OCM Controllers field CPO v2 was overwriting the entire OCM config on each reconciliation, erasing HCCO's modification to disable the pull secrets controller when the image registry is disabled via managementState: Removed instead of the ImageRegistry capability. This fix: - Fetches the actual ConfigMap from the cluster (not just the static asset) - Preserves the existing Controllers field when ImageRegistry capability is enabled but HCCO has modified it (e.g., via managementState: Removed) - Only overrides Controllers when ImageRegistry capability is explicitly disabled This ensures that registry pull secrets are not created when the registry is disabled via managementState: Removed on non-Azure/non-IBM platforms. Co-Authored-By: Claude Sonnet 4.5 --- .../hostedcontrolplane/v2/ocm/config.go | 26 +++++- .../hostedcontrolplane/v2/ocm/config_test.go | 88 ++++++++++++++++++- 2 files changed, 111 insertions(+), 3 deletions(-) diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/ocm/config.go b/control-plane-operator/controllers/hostedcontrolplane/v2/ocm/config.go index ed7848a1632a..45ca22550432 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/ocm/config.go +++ b/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 { diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/ocm/config_test.go b/control-plane-operator/controllers/hostedcontrolplane/v2/ocm/config_test.go index c79f6941a03f..2439ab07ac9f 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/ocm/config_test.go +++ b/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) + } +}