From f75121fff7d421f1b486995ed9748747ed7818b3 Mon Sep 17 00:00:00 2001 From: Vimal Solanki Date: Thu, 14 May 2026 20:37:19 +0530 Subject: [PATCH] fix(hcco): re-enable serviceaccount-pull-secrets controller when registry managementState changes from Removed The HCCO previously only disabled the serviceaccount-pull-secrets controller in openshift-controller-manager when the image registry managementState was set to Removed. It had no logic to re-enable the controller when managementState changed back to Managed. Combined with CPO's preservation of existing controller overrides, this caused the controller to remain permanently disabled. This change makes the logic bidirectional: when managementState is no longer Removed and the controller is currently disabled, it clears the controller override to re-enable it. Signed-off-by: Vimal Solanki --- .../controllers/resources/resources.go | 45 ++++- .../controllers/resources/resources_test.go | 182 ++++++++++++++++++ 2 files changed, 222 insertions(+), 5 deletions(-) diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go index 1ee6a3a7180..9b4e37187b0 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go @@ -6,6 +6,7 @@ import ( "fmt" "net" "reflect" + "slices" "sort" "strings" "sync" @@ -132,6 +133,8 @@ exec /bin/azure-cloud-node-manager \ --wait-routes=false ` +var disabledServiceAccountPullSecretsController = fmt.Sprintf("-%s", openshiftcpv1.OpenShiftServiceAccountPullSecretsController) + type reconciler struct { client client.Client uncachedClient client.Client @@ -629,8 +632,7 @@ func (r *reconciler) reconcileRegistryAndIngress(ctx context.Context, hcp *hyper } // 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") + if r.platformType != hyperv1.IBMCloudPlatform && r.platformType != hyperv1.AzurePlatform { ocmConfigMap := cpomanifests.OpenShiftControllerManagerConfig(r.hcpNamespace) if _, err := r.CreateOrUpdate(ctx, r.cpClient, ocmConfigMap, func() error { if ocmConfigMap.Data == nil { @@ -638,12 +640,28 @@ func (r *reconciler) reconcileRegistryAndIngress(ctx context.Context, hcp *hyper } config := &openshiftcpv1.OpenShiftControllerManagerConfig{} if configStr, exists := ocmConfigMap.Data[ocm.ConfigKey]; exists && len(configStr) > 0 { - err := k8sutil.DeserializeResource(configStr, config, api.Scheme) - if err != nil { + if err := k8sutil.DeserializeResource(configStr, config, api.Scheme); err != nil { return fmt.Errorf("unable to decode existing openshift controller manager configuration: %w", err) } } - config.Controllers = []string{"*", fmt.Sprintf("-%s", openshiftcpv1.OpenShiftServiceAccountPullSecretsController)} + if registryConfig.Spec.ManagementState == operatorv1.Removed { + if isServiceAccountPullSecretsControllerDisabled(config.Controllers) { + // Already disabled; returning nil without mutation causes CreateOrUpdate to skip the update. + return nil + } + log.Info("imageregistry operator managementstate is removed, disabling serviceaccount-pull-secrets controller") + if len(config.Controllers) == 0 { + config.Controllers = []string{"*", disabledServiceAccountPullSecretsController} + } else { + config.Controllers = append(config.Controllers, disabledServiceAccountPullSecretsController) + } + } else if isServiceAccountPullSecretsControllerDisabled(config.Controllers) { + log.Info("imageregistry operator managementstate is no longer removed, re-enabling serviceaccount-pull-secrets controller") + config.Controllers = removeDisabledServiceAccountPullSecretsController(config.Controllers) + } else { + // No change needed; returning nil without mutation causes CreateOrUpdate to skip the update. + return nil + } configStr, err := k8sutil.SerializeResource(config, api.Scheme) if err != nil { return fmt.Errorf("failed to serialize openshift controller manager configuration: %w", err) @@ -3651,3 +3669,20 @@ func imageRegistryPlatformWithPVC(platform hyperv1.PlatformType) bool { return false } } + +func isServiceAccountPullSecretsControllerDisabled(controllers []string) bool { + return slices.Contains(controllers, disabledServiceAccountPullSecretsController) +} + +func removeDisabledServiceAccountPullSecretsController(controllers []string) []string { + filtered := make([]string, 0, len(controllers)) + for _, c := range controllers { + if c != disabledServiceAccountPullSecretsController { + filtered = append(filtered, c) + } + } + if len(filtered) == 0 { + return []string{"*"} + } + return filtered +} diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go index b4d08dbb5c7..8e1662bd1ab 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go @@ -14,6 +14,7 @@ import ( hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" cpomanifests "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/manifests" + "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/ocm" "github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/api" "github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/resources/kas" "github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests" @@ -21,6 +22,7 @@ import ( "github.com/openshift/hypershift/hypershift-operator/controllers/nodepool" "github.com/openshift/hypershift/support/azureutil" "github.com/openshift/hypershift/support/globalconfig" + "github.com/openshift/hypershift/support/k8sutil" "github.com/openshift/hypershift/support/netutil" "github.com/openshift/hypershift/support/releaseinfo" fakereleaseprovider "github.com/openshift/hypershift/support/releaseinfo/fake" @@ -29,6 +31,7 @@ import ( configv1 "github.com/openshift/api/config/v1" imageapi "github.com/openshift/api/image/v1" + openshiftcpv1 "github.com/openshift/api/openshiftcontrolplane/v1" operatorv1 "github.com/openshift/api/operator/v1" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" @@ -3911,3 +3914,182 @@ func TestEnsureGuestAdmissionWebhooksAreValid(t *testing.T) { }) } } + +func TestIsServiceAccountPullSecretsControllerDisabled(t *testing.T) { + t.Parallel() + tests := []struct { + name string + controllers []string + expected bool + }{ + { + name: "When controllers is nil, it should return false", + controllers: nil, + expected: false, + }, + { + name: "When controllers is empty, it should return false", + controllers: []string{}, + expected: false, + }, + { + name: "When controller is disabled, it should return true", + controllers: []string{"*", "-openshift.io/serviceaccount-pull-secrets"}, + expected: true, + }, + { + name: "When controllers has other entries but not the disabled one, it should return false", + controllers: []string{"*", "-some-other-controller"}, + expected: false, + }, + { + name: "When only the wildcard is present, it should return false", + controllers: []string{"*"}, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + g.Expect(isServiceAccountPullSecretsControllerDisabled(tt.controllers)).To(Equal(tt.expected)) + }) + } +} + +func TestReconcileRegistryAndIngress_ServiceAccountPullSecretsController(t *testing.T) { + t.Parallel() + + hcpNamespace := "test-hcp-ns" + + serializeOCMConfig := func(t *testing.T, controllers []string) string { + t.Helper() + config := &openshiftcpv1.OpenShiftControllerManagerConfig{ + Controllers: controllers, + } + data, err := k8sutil.SerializeResource(config, api.Scheme) + if err != nil { + t.Fatalf("failed to serialize OCM config: %v", err) + } + return data + } + + tests := []struct { + name string + platformType hyperv1.PlatformType + managementState operatorv1.ManagementState + existingOCMControllers []string + hasExistingOCMConfig bool + expectedControllers []string + }{ + { + name: "When managementState is Removed, it should disable serviceaccount-pull-secrets controller", + platformType: hyperv1.AWSPlatform, + managementState: operatorv1.Removed, + existingOCMControllers: nil, + hasExistingOCMConfig: true, + expectedControllers: []string{"*", disabledServiceAccountPullSecretsController}, + }, + { + name: "When managementState changes from Removed to Managed, it should re-enable serviceaccount-pull-secrets controller", + platformType: hyperv1.AWSPlatform, + managementState: operatorv1.Managed, + existingOCMControllers: []string{"*", disabledServiceAccountPullSecretsController}, + hasExistingOCMConfig: true, + expectedControllers: []string{"*"}, + }, + { + name: "When managementState is Managed and controller is already enabled, it should not change controllers", + platformType: hyperv1.AWSPlatform, + managementState: operatorv1.Managed, + existingOCMControllers: []string{"*"}, + hasExistingOCMConfig: true, + expectedControllers: []string{"*"}, + }, + { + name: "When platform is IBMCloud, it should not modify OCM config regardless of managementState", + platformType: hyperv1.IBMCloudPlatform, + managementState: operatorv1.Removed, + existingOCMControllers: nil, + hasExistingOCMConfig: true, + expectedControllers: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + registryConfig := manifests.Registry() + registryConfig.Spec.ManagementState = tt.managementState + + guestClient := fake.NewClientBuilder(). + WithScheme(api.Scheme). + WithObjects(registryConfig). + Build() + + ocmConfigMap := cpomanifests.OpenShiftControllerManagerConfig(hcpNamespace) + if tt.hasExistingOCMConfig { + ocmConfigMap.Data = map[string]string{} + if tt.existingOCMControllers != nil { + ocmConfigMap.Data[ocm.ConfigKey] = serializeOCMConfig(t, tt.existingOCMControllers) + } else { + ocmConfigMap.Data[ocm.ConfigKey] = serializeOCMConfig(t, nil) + } + } + + cpClient := fake.NewClientBuilder(). + WithScheme(api.Scheme). + WithObjects(ocmConfigMap). + Build() + + hcp := &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-hcp", + Namespace: hcpNamespace, + }, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: tt.platformType, + }, + }, + } + + r := &reconciler{ + client: guestClient, + cpClient: cpClient, + CreateOrUpdateProvider: &simpleCreateOrUpdater{}, + platformType: tt.platformType, + hcpNamespace: hcpNamespace, + } + + log := zapr.NewLogger(zaptest.NewLogger(t)) + errs := r.reconcileRegistryAndIngress(t.Context(), hcp, log) + for _, e := range errs { + g.Expect(e.Error()).ToNot(ContainSubstring("openshift-controller-manager config"), "unexpected OCM config error: %v", e) + } + + resultConfigMap := cpomanifests.OpenShiftControllerManagerConfig(hcpNamespace) + err := cpClient.Get(t.Context(), client.ObjectKeyFromObject(resultConfigMap), resultConfigMap) + g.Expect(err).ToNot(HaveOccurred(), "failed to get OCM ConfigMap") + + if tt.expectedControllers == nil { + config := &openshiftcpv1.OpenShiftControllerManagerConfig{} + if configStr, exists := resultConfigMap.Data[ocm.ConfigKey]; exists && len(configStr) > 0 { + err := k8sutil.DeserializeResource(configStr, config, api.Scheme) + g.Expect(err).ToNot(HaveOccurred(), "failed to deserialize OCM config") + } + g.Expect(config.Controllers).To(BeNil(), "controllers should remain nil for excluded platform") + } else { + config := &openshiftcpv1.OpenShiftControllerManagerConfig{} + configStr, exists := resultConfigMap.Data[ocm.ConfigKey] + g.Expect(exists).To(BeTrue(), "OCM config should exist") + err := k8sutil.DeserializeResource(configStr, config, api.Scheme) + g.Expect(err).ToNot(HaveOccurred(), "failed to deserialize OCM config") + g.Expect(config.Controllers).To(Equal(tt.expectedControllers)) + } + }) + } +}