diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go index 56c48bc14283..97489816683b 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go @@ -18,8 +18,6 @@ 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" @@ -54,7 +52,6 @@ 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" @@ -425,70 +422,12 @@ func (r *reconciler) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result errs = append(errs, fmt.Errorf("failed to reconcile rbac: %w", err)) } - 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. + // Reconcile the image registry only if the image registry capability is enabled. + // Skip this step if the user explicitly disabled image registry. if capabilities.IsImageRegistryCapabilityEnabled(hcp.Spec.Capabilities) { - 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)) - } - } + log.Info("reconciling image registry") + if regErrs := r.reconcileImageRegistry(ctx, hcp); len(regErrs) > 0 { + errs = append(errs, regErrs...) } } @@ -3158,3 +3097,52 @@ 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 69ac404af2e5..0ab745e5789a 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "math/rand" - "reflect" "strings" "testing" "time" @@ -35,7 +34,6 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" clientset "k8s.io/client-go/kubernetes" @@ -1598,79 +1596,6 @@ region = us-east-1 } } -// TestReconcileOcmConfigChange checks if the OCM(Openshift Controller Manager)'s configuration has changed -// for the platforms Azure and AWS when the ImageRegistry Operator's managementState is set to Removed -func TestReconcileOcmConfigChange(t *testing.T) { - registryConfig := &imageregistryv1.Config{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cluster", - }, - Spec: imageregistryv1.ImageRegistrySpec{ - OperatorSpec: operatorv1.OperatorSpec{ - ManagementState: "Removed", - }, - }, - } - initialOcmConfigMap := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "openshift-controller-manager-config", - Namespace: "bar", - }, - Data: map[string]string{ - "config": "original-data", - }, - } - - testCases := []struct { - name string - platformType hyperv1.PlatformType - expectConfigMapUnchanged bool - }{ - { - name: "OCM configuration remains unchanged for Azure platform", - platformType: hyperv1.AzurePlatform, - expectConfigMapUnchanged: true, - }, - { - // OCM configuration should remain unchanged for AWS platform also after transitioning from - // using managementState: Removed to disable the Image Registry in ROSA HCP - name: "OCM configuration changes for AWS platform", - platformType: hyperv1.AWSPlatform, - expectConfigMapUnchanged: false, - }, - } - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - g := NewWithT(t) - // Create a context with a logger for the test - ctx := logr.NewContext(context.Background(), zapr.NewLogger(zaptest.NewLogger(t))) - - fakeClient := fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(append(initialObjects, registryConfig)...).WithStatusSubresource(&configv1.Infrastructure{}).Build() - cpClient := fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(append(cpObjects, initialOcmConfigMap)...).WithStatusSubresource(&hyperv1.HostedControlPlane{}).Build() - r := &reconciler{ - CreateOrUpdateProvider: &simpleCreateOrUpdater{}, - client: fakeClient, - uncachedClient: fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects().Build(), - cpClient: cpClient, - hcpName: "foo", - hcpNamespace: "bar", - releaseProvider: &fakereleaseprovider.FakeReleaseProvider{}, - ImageMetaDataProvider: &fakeimagemetadataprovider.FakeRegistryClientImageMetadataProviderHCCO{}, - platformType: tc.platformType, - } - _, err := r.Reconcile(ctx, controllerruntime.Request{}) - g.Expect(err).NotTo(HaveOccurred()) - - // Check if the OCM configuration has changed or not - updatedOcmConfigMap := &corev1.ConfigMap{} - err = cpClient.Get(ctx, types.NamespacedName{Name: "openshift-controller-manager-config", Namespace: "bar"}, updatedOcmConfigMap) - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(reflect.DeepEqual(updatedOcmConfigMap.Data, initialOcmConfigMap.Data)).To(Equal(tc.expectConfigMapUnchanged)) - }) - } - -} - func makeKubeletConfigConfigMap(name, namespace, data string) *corev1.ConfigMap { return &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ @@ -2555,3 +2480,127 @@ func Test_reconciler_reconcileControlPlaneDataPlaneConnectivityConditions(t *tes }) } } + +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") + } + }) + } +}