Skip to content

OCPBUGS-85151: Re-enable serviceaccount-pull-secrets controller when registry managementState changes from Removed #8522

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

Merged
openshift-merge-bot merged 1 commit into from
May 28, 2026
Merged

OCPBUGS-85151: Re-enable serviceaccount-pull-secrets controller when registry managementState changes from Removed #8522

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
45 changes: 40 additions & 5 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 @@ -6,6 +6,7 @@ import (
"fmt"
"net"
"reflect"
"slices"
"sort"
"strings"
"sync"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -629,21 +632,36 @@ 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 {
return nil
}
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) {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
log.Info("imageregistry operator managementstate is no longer removed, re-enabling serviceaccount-pull-secrets controller")
config.Controllers = removeDisabledServiceAccountPullSecretsController(config.Controllers)
} else {

@bryan-cox bryan-cox May 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] Consider using []string{"*"} instead of nil here.

The Controllers field is tagged json:"controllers" (no omitempty), so nil serializes to "controllers": null. Whether OCM interprets null as "use default (all controllers)" vs "empty list (run nothing)" is ambiguous.

Using []string{"*"} is explicit and unambiguous — it says "run all on-by-default controllers," which is the documented default and the intent here.

This also makes the CPO interaction in adaptConfig deterministic: with nil, CPO's len(existingControllers) > 0 is false so it enters neither branch and relies on the base config default. With ["*"], CPO explicitly preserves it via the else if branch.

@vsolanki12 vsolanki12 May 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bryan-cox ,
Done. Changed to selective removal of the disabled entry, falling back to []string{"*"} when the remaining list is empty.

// No change needed; returning nil without mutation causes CreateOrUpdate to skip the update.
return nil
}

@bryan-cox bryan-cox May 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low — readability] This return nil without mutation is correct — CreateOrUpdate deep-compares before/after and skips the API write when nothing changed. But the intent is non-obvious to readers unfamiliar with the upsert framework. A short comment would help:

} else {
	// No change needed; returning nil without mutation causes CreateOrUpdate to skip the update.
	return nil
}

@vsolanki12 vsolanki12 May 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done added. // No change needed; returning nil without mutation causes CreateOrUpdate to skip the update.

configStr, err := k8sutil.SerializeResource(config, api.Scheme)
if err != nil {
return fmt.Errorf("failed to serialize openshift controller manager configuration: %w", err)
Expand Down Expand Up @@ -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
}
182 changes: 182 additions & 0 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 @@ -14,13 +14,15 @@ 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"
"github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/resources/registry"
"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"
Expand All @@ -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"
Expand Down Expand Up @@ -3911,3 +3914,182 @@ func TestEnsureGuestAdmissionWebhooksAreValid(t *testing.T) {
})
}
}

func TestIsServiceAccountPullSecretsControllerDisabled(t *testing.T) {

@csrwng csrwng May 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: The helper function is well-tested, but it would be good to also have reconciliation-level tests covering the full reconcileRegistryAndIngress flow — e.g., verifying that when managementState transitions from Removed to Managed, the OCM ConfigMap's Controllers field is correctly updated, and conversely that it stays untouched when the state hasn't changed.

@vsolanki12 vsolanki12 May 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Added TestReconcileRegistryAndIngress_ServiceAccountPullSecretsController with 4 test cases: disable when Removed, re-enable when Managed, no-op when already enabled, and platform exclusion for IBMCloud.

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)

@bryan-cox bryan-cox May 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] Nit: subtests are independent and could benefit from t.Parallel() inside each t.Run callback, consistent with Go best practices for table-driven tests.

@vsolanki12 vsolanki12 May 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Added t.Parallel() inside each t.Run callback.

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))
}
})
}
}