Skip to content

[release-4.21] OCPBUGS-76417: fix(hcco): gate image registry workaround to pre-4.20 releases #7684

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
openshift-cherrypick-robot wants to merge 1 commit into from
Closed

[release-4.21] OCPBUGS-76417: fix(hcco): gate image registry workaround to pre-4.20 releases #7684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 54 additions & 66 deletions control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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...)
}
}

Expand Down Expand Up @@ -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
}
199 changes: 124 additions & 75 deletions control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"fmt"
"math/rand"
"reflect"
"strings"
"testing"
"time"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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")
}
})
}
}