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) + } +} diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go index 9bdc3a04b480..9833a9150796 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go +++ b/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 -} diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go index d74294a2f60a..f0c78a8dd6cb 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go @@ -23,7 +23,6 @@ import ( "github.com/openshift/hypershift/support/util/fakeimagemetadataprovider" configv1 "github.com/openshift/api/config/v1" - imageregistryv1 "github.com/openshift/api/imageregistry/v1" operatorv1 "github.com/openshift/api/operator/v1" appsv1 "k8s.io/api/apps/v1" @@ -2964,127 +2963,3 @@ func Test_reconciler_reconcileKASConnectionCheckerDeployment(t *testing.T) { }) } } - -func TestReconcileImageRegistry(t *testing.T) { - testCases := []struct { - name string - hcp *hyperv1.HostedControlPlane - platformType hyperv1.PlatformType - existingRegistryConfig *imageregistryv1.Config - expectRegistryReconciled bool - expectErrors bool - expectVAPReconciled bool - }{ - { - name: "When OpenStack platform has no existing config it should skip to let CIRO bootstrap", - hcp: func() *hyperv1.HostedControlPlane { - hcp := fakeHCP() - hcp.Spec.Platform.Type = hyperv1.OpenStackPlatform - return hcp - }(), - platformType: hyperv1.OpenStackPlatform, - existingRegistryConfig: nil, - expectRegistryReconciled: false, - expectErrors: false, - }, - { - name: "When OpenStack platform has existing config it should reconcile normally", - hcp: func() *hyperv1.HostedControlPlane { - hcp := fakeHCP() - hcp.Spec.Platform.Type = hyperv1.OpenStackPlatform - return hcp - }(), - platformType: hyperv1.OpenStackPlatform, - existingRegistryConfig: &imageregistryv1.Config{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cluster", - ResourceVersion: "1", - Finalizers: []string{"imageregistry.operator.openshift.io/finalizer"}, - }, - Spec: imageregistryv1.ImageRegistrySpec{ - OperatorSpec: operatorv1.OperatorSpec{ - ManagementState: operatorv1.Managed, - }, - }, - }, - expectRegistryReconciled: true, - expectErrors: false, - }, - { - name: "When Azure platform it should reconcile validating admission policies", - hcp: func() *hyperv1.HostedControlPlane { - hcp := fakeHCP() - hcp.Spec.Platform.Type = hyperv1.AzurePlatform - return hcp - }(), - platformType: hyperv1.AzurePlatform, - expectRegistryReconciled: true, - expectVAPReconciled: true, - expectErrors: false, - }, - { - name: "When AWS platform it should reconcile registry config", - hcp: func() *hyperv1.HostedControlPlane { - hcp := fakeHCP() - hcp.Spec.Platform.Type = hyperv1.AWSPlatform - return hcp - }(), - platformType: hyperv1.AWSPlatform, - expectRegistryReconciled: true, - expectErrors: false, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - g := NewWithT(t) - - var guestObjects []client.Object - if tc.existingRegistryConfig != nil { - guestObjects = append(guestObjects, tc.existingRegistryConfig) - } - - guestClient := fake.NewClientBuilder(). - WithScheme(api.Scheme). - WithObjects(guestObjects...). - Build() - - cpClient := fake.NewClientBuilder(). - WithScheme(api.Scheme). - WithObjects(tc.hcp). - WithStatusSubresource(&hyperv1.HostedControlPlane{}). - Build() - - r := &reconciler{ - client: guestClient, - cpClient: cpClient, - CreateOrUpdateProvider: &simpleCreateOrUpdater{}, - platformType: tc.platformType, - } - - ctx := logr.NewContext(t.Context(), zapr.NewLogger(zaptest.NewLogger(t))) - errs := r.reconcileImageRegistry(ctx, tc.hcp) - - if tc.expectErrors { - g.Expect(len(errs)).To(BeNumerically(">", 0), "expected errors but got none") - } else { - g.Expect(len(errs)).To(Equal(0), "expected no errors but got: %v", errs) - } - - registryConfig := manifests.Registry() - err := guestClient.Get(t.Context(), client.ObjectKeyFromObject(registryConfig), registryConfig) - if tc.expectRegistryReconciled { - g.Expect(err).ToNot(HaveOccurred(), "expected registry config to exist after reconciliation") - g.Expect(registryConfig.Spec.HTTPSecret).ToNot(BeEmpty(), "expected HTTPSecret to be set") - } else if tc.existingRegistryConfig == nil { - g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), "expected registry config to not exist") - } - - if tc.expectVAPReconciled { - vap := manifests.ValidatingAdmissionPolicy("deny-removed-managementstate") - err := guestClient.Get(t.Context(), client.ObjectKeyFromObject(vap), vap) - g.Expect(err).ToNot(HaveOccurred(), "expected ValidatingAdmissionPolicy to exist for Azure platform") - } - }) - } -}