:sparkles: fix(boxcutter): cache Secrets only in olmv1-system namespace to ensure better performance by camilamacedo86 · Pull Request #2609 · operator-framework/operator-controller

camilamacedo86 · 2026-03-31T05:22:03Z

Configures the controller manager cache to watch Secrets only in the olmv1-system namespace, preventing a cluster-wide Secret informer while maintaining good performance for bundle Secret reads.

Motivation

Inspired by discussion in:
openshift/operator-framework-operator-controller#687 (comment)

netlify · 2026-03-31T05:22:09Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`d4523d9`
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/69cb76d0510a5c0008acdd35
😎 Deploy Preview	https://deploy-preview-2609--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copilot

Pull request overview

This PR switches ClusterObjectSet Secret lookups from the cached controller-runtime client to the uncached API reader to avoid creating a cluster-wide Secret informer (reducing memory/network/cache-sync overhead on large clusters).

Changes:

Add an APIReader field to ClusterObjectSetReconciler.
Update resolveObjectRef to read Secrets via APIReader.Get instead of Client.Get.
Wire mgr.GetAPIReader() in cmd/operator-controller/main.go and update unit tests accordingly.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
internal/operator-controller/controllers/clusterobjectset_controller.go	Introduces `APIReader` and uses it for Secret retrieval in `resolveObjectRef`.
cmd/operator-controller/main.go	Wires `APIReader` from the manager into the reconciler.
internal/operator-controller/controllers/resolve_ref_test.go	Updates tests to set `APIReader` on the reconciler.
internal/operator-controller/controllers/clusterobjectset_controller_test.go	Updates controller tests to set `APIReader`.
internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go	Updates internal tests to set `APIReader`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

pedjak

Given that we have switched to secret-based approach for all bundles, this is going to add at least one more API call per reconcile round. If this is really needed, I would still suggest to use cache-based client, but either:

disable caching of secrets:

mgr, err:=ctrl.NewManager(cfg, ctrl.Options{ Client: client.Options{ Cache: &client.CacheOptions{ DisableFor: []client.Object{ &corev1.Secret{}, }, }, }, })

or, restrict caching for only olmv1-system namespace

I would prefer the later because it does not add additional API calls for most of bundles.

openshift-ci · 2026-03-31T07:09:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign grokspawn for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

cmd/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Configure cache to watch Secrets exclusively in olmv1-system, avoiding a cluster-wide Secret informer while maintaining performance for bundle Secret lookups via the cached client

codecov · 2026-03-31T07:45:48Z

Codecov Report

❌ Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.97%. Comparing base (43351b2) to head (d4523d9).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/operator-controller/main.go	0.00%	5 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #2609 +/- ##
===========================================
+ Coverage 53.50% 66.97% +13.47% 
===========================================
Files 139 139 Lines 9872 9878 +6 ===========================================
+ Hits 5282 6616 +1334 + Misses 4210 2773 -1437 - Partials 380 489 +109

Flag	Coverage Δ
e2e	`10.73% <0.00%> (?)`
experimental-e2e	`52.54% <0.00%> (?)`
unit	`53.47% <0.00%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

joelanford · 2026-03-31T13:37:17Z

+	// Ensure bundle Secrets in the configured systemNamespace are cached without enabling a cluster-wide Secret informer.
+	// Bundle Secrets are created in cfg.systemNamespace by SecretPacker.
+	if secretCache, ok := cacheOptions.ByObject[&corev1.Secret{}]; ok {
+ if secretCache.Namespaces == nil {
+ secretCache.Namespaces = make(map[string]crcache.Config)
+ }
+ if _, exists := secretCache.Namespaces[cfg.systemNamespace]; !exists {
+ secretCache.Namespaces[cfg.systemNamespace] = crcache.Config{}
+ }
+ cacheOptions.ByObject[&corev1.Secret{}] = secretCache
+	}


We already cache everything in the system namespace, don't we? See lines 259-261.

joelanford · 2026-03-31T13:38:46Z

 cfg.systemNamespace: {LabelSelector: k8slabels.Everything()},
 },
 DefaultLabelSelector: k8slabels.Nothing(),
 // Memory optimization: strip managed fields and large annotations from cached objects


Aside: I don't think we strip managed fields anymore do we? That would break our SSA clients (I think?)

pedjak

Comment done by Claude:

A few observations on this change:

The map lookup is always `false` — this code is unreachable

cacheOptions.ByObject has type map[client.Object]cache.ByObject, where client.Object is an interface. Go compares interface map keys by (dynamic type, dynamic value). Since the dynamic type here is *corev1.Secret (a pointer), Go compares pointer addresses. The &corev1.Secret{} allocated on line 286 is a different pointer than the one used as key in SetupPullSecretCache (pullsecretcache.go:37), so the lookup never matches and ok is always false.

The entire block is dead code.

Even if it worked, it would be redundant

DefaultNamespaces (lines 258-260) already includes cfg.systemNamespace with LabelSelector: labels.Everything(), so everything in that namespace (including Secrets) is already cached.
SetupPullSecretCache already configures Secret caching scoped to saKey.Namespace, which is cfg.systemNamespace (the operator's own SA namespace).

There is no cluster-wide Secret informer to prevent — the existing configuration already scopes it.

camilamacedo86 · 2026-04-01T02:55:30Z

@pedjak that is 100% true: #2609 (review)

The intisl motivation of this one was look if we should/could address: openshift/operator-framework-operator-controller#687 (comment)

But we discussed and reach the idea to keep the cache: #2609 (review)

therefore, I am closing this one as won't do and if we face an issue actually then we revisit it.

CopilotAI review requested due to automatic review settings March 31, 2026 05:22

openshift-ciBot requested review from joelanford and perdasilva March 31, 2026 05:22

Copilotstarted reviewing on behalf of camilamacedo86 March 31, 2026 05:22 View session

CopilotAI reviewed Mar 31, 2026
View reviewed changes

Comment threadinternal/operator-controller/controllers/clusterobjectset_controller.go
Comment threadinternal/operator-controller/controllers/clusterobjectset_controller.go Outdated

pedjak reviewed Mar 31, 2026
View reviewed changes

camilamacedo86 force-pushed the fix-ai-secret branch from 927f5d2 to 8427453Compare March 31, 2026 07:09

camilamacedo86 requested review from Copilot and pedjak March 31, 2026 07:10

camilamacedo86 changed the title ~~🐛 fix: (boxcutter) use uncached reader for Secret lookups in ClusterObjectSet~~🐛 fix(boxcutter): cache Secrets only in olmv1-system namespaceMar 31, 2026

camilamacedo86 changed the title ~~🐛 fix(boxcutter): cache Secrets only in olmv1-system namespace~~✨ fix(boxcutter): cache Secrets only in olmv1-system namespace to ensure better performanceMar 31, 2026

Copilotstarted reviewing on behalf of camilamacedo86 March 31, 2026 07:10 View session

CopilotAI reviewed Mar 31, 2026
View reviewed changes

Comment threadcmd/operator-controller/main.go Outdated
Comment threadcmd/operator-controller/main.go Outdated
Comment threadcmd/operator-controller/main.go Outdated

fix(boxcutter): cache Secrets only in olmv1-system namespace
d4523d9
Configure cache to watch Secrets exclusively in olmv1-system, avoiding a cluster-wide Secret informer while maintaining performance for bundle Secret lookups via the cached client

camilamacedo86 force-pushed the fix-ai-secret branch from 8427453 to d4523d9Compare March 31, 2026 07:25

joelanford reviewed Mar 31, 2026
View reviewed changes

pedjak reviewed Mar 31, 2026
View reviewed changes

camilamacedo86 closed this Apr 1, 2026

Navigation Menu

Uh oh!

✨ fix(boxcutter): cache Secrets only in olmv1-system namespace to ensure better performance - #2609

✨ fix(boxcutter): cache Secrets only in olmv1-system namespace to ensure better performance#2609
camilamacedo86 wants to merge 1 commit into
operator-framework:mainfrom
camilamacedo86:fix-ai-secret

camilamacedo86 commented Mar 31, 2026•
edited
Loading

Uh oh!

netlifyBot commented Mar 31, 2026•
edited
Loading

Uh oh!

CopilotAI left a comment

Uh oh!

Uh oh!

Uh oh!

pedjak left a comment

Uh oh!

openshift-ciBot commented Mar 31, 2026

Uh oh!

CopilotAI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecovBot commented Mar 31, 2026•
edited
Loading

Uh oh!

joelanfordMar 31, 2026

Uh oh!

joelanfordMar 31, 2026

Uh oh!

pedjak left a comment •
edited
Loading

Uh oh!

camilamacedo86 commented Apr 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

camilamacedo86 commented Mar 31, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Uh oh!

netlifyBot commented Mar 31, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

pedjak left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ciBot commented Mar 31, 2026

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecovBot commented Mar 31, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

joelanfordMar 31, 2026

Choose a reason for hiding this comment

Uh oh!

joelanfordMar 31, 2026

Choose a reason for hiding this comment

Uh oh!

pedjak left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

The map lookup is always false — this code is unreachable

Even if it worked, it would be redundant

Uh oh!

camilamacedo86 commented Apr 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

camilamacedo86 commented Mar 31, 2026•
edited
Loading

netlifyBot commented Mar 31, 2026•
edited
Loading

codecovBot commented Mar 31, 2026•
edited
Loading

pedjak left a comment •
edited
Loading

The map lookup is always `false` — this code is unreachable