Skip to content

✨ Make RBACPreAuthorizer collection verbs configurable#2539

Open
perdasilva wants to merge 1 commit intooperator-framework:mainfrom
perdasilva:generic-preflights
Open

✨ Make RBACPreAuthorizer collection verbs configurable#2539
perdasilva wants to merge 1 commit intooperator-framework:mainfrom
perdasilva:generic-preflights

Conversation

@perdasilva
Copy link
Contributor

@perdasilva perdasilva commented Mar 5, 2026

Summary

  • Make clusterCollectionVerbs and namespacedCollectionVerbs on RBACPreAuthorizer configurable via WithClusterCollectionVerbs and WithNamespacedCollectionVerbs functional options, decoupling them from hardcoded verbs that were tightly coupled to contentmanager
  • Both the helm and boxcutter appliers explicitly configure namespacedCollectionVerbs with create
  • The helm applier's preauthorizer additionally configures clusterCollectionVerbs with list and watch (required by contentmanager)
  • The boxcutter applier's preauthorizer uses no cluster collection verbs
  • Updates unit tests for both configurable options

Closes #1911

Motivation

In Boxcutter, watching is done with the controller's service account which includes list/watch permissions across the cluster. Therefore, they are not required by the clusterextension's nominated service account. Once we fully switch to Boxcutter the option can be dropped entirely.

Making namespacedCollectionVerbs configurable follows the same pattern, allowing appliers to explicitly declare their requirements rather than relying on hidden defaults.

Test plan

  • Existing unit tests updated and passing
  • New unit tests for WithNamespacedCollectionVerbs option
  • New unit tests for WithClusterCollectionVerbs option
  • Update e2e tests with boxcutter rbac template without list/watch verbs on bundle resources

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings March 5, 2026 11:14
@openshift-ci openshift-ci bot requested review from oceanc80 and pedjak March 5, 2026 11:14
@netlify
Copy link

netlify bot commented Mar 5, 2026

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 6b23a34
🔍 Latest deploy log https://app.netlify.com/projects/olmv1/deploys/69aae5537bb1fd000828f4d5
😎 Deploy Preview https://deploy-preview-2539--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR decouples RBACPreAuthorizer from component-specific cluster-scoped permission requirements by making clusterCollectionVerbs configurable via a functional option, allowing different appliers to request different RBAC checks.

Changes:

  • Added WithClusterCollectionVerbs(...) option and plumbed configurable cluster-scoped collection verbs through pre-authorization attribute generation.
  • Updated unit tests to construct the pre-authorizer with explicit list/watch cluster collection verbs.
  • Configured the helm reconciler’s pre-authorizer to include cluster-scoped list/watch checks.

Reviewed changes

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

File Description
internal/operator-controller/authorization/rbac.go Introduces functional option + stores cluster collection verbs on the pre-authorizer; passes verbs into attribute generation.
internal/operator-controller/authorization/rbac_test.go Updates pre-authorizer construction to include configured cluster collection verbs.
cmd/operator-controller/main.go Configures helm reconciler to require cluster-scoped list/watch checks.
Comments suppressed due to low confidence (1)

internal/operator-controller/authorization/rbac.go:342

  • This capacity estimate comment is now stale: it says len(clusterCollectionVerbs) records (2), but clusterCollectionVerbs is configurable and may be 0 or any other length. Update the comment to avoid hardcoding 2 (or clarify it as an example for list/watch).
	// Calculate initial capacity as an upper-bound estimate:
	// - For each key: len(objectVerbs) records (4)
	// - For unique namespaces: len(namespacedCollectionVerbs) records (1 per unique namespace across all keys in a GVR)
	//   We use totalKeys as upper bound (worst case: each key in different namespace)
	// - For each GVR: len(clusterCollectionVerbs) records (2)
	totalKeys := 0

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

Copilot AI review requested due to automatic review settings March 5, 2026 11:34
@perdasilva perdasilva force-pushed the generic-preflights branch from 64b4642 to 4935449 Compare March 5, 2026 11:34
@perdasilva perdasilva force-pushed the generic-preflights branch from 4935449 to e833a37 Compare March 5, 2026 11:35
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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

Comments suppressed due to low confidence (1)

internal/operator-controller/authorization/rbac.go:342

  • The capacity-estimation comment still hardcodes "(2)" cluster-scoped records per GVR, but clusterCollectionVerbs is now configurable. Update the comment to avoid implying a fixed count (e.g., remove "(2)" or describe it generically).
	// Calculate initial capacity as an upper-bound estimate:
	// - For each key: len(objectVerbs) records (4)
	// - For unique namespaces: len(namespacedCollectionVerbs) records (1 per unique namespace across all keys in a GVR)
	//   We use totalKeys as upper bound (worst case: each key in different namespace)
	// - For each GVR: len(clusterCollectionVerbs) records (2)
	totalKeys := 0

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

@codecov
Copy link

codecov bot commented Mar 5, 2026

Codecov Report

❌ Patch coverage is 72.22222% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.61%. Comparing base (ac1fdfd) to head (6b23a34).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
cmd/operator-controller/main.go 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2539      +/-   ##
==========================================
+ Coverage   64.25%   68.61%   +4.36%     
==========================================
  Files         131      131              
  Lines        9288     9301      +13     
==========================================
+ Hits         5968     6382     +414     
+ Misses       2849     2435     -414     
- Partials      471      484      +13     
Flag Coverage Δ
e2e 42.55% <0.00%> (+0.18%) ⬆️
experimental-e2e 51.54% <27.77%> (+39.75%) ⬆️
unit 53.73% <72.22%> (+0.02%) ⬆️

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.

@perdasilva
Copy link
Contributor Author

/hold updating e2es

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 5, 2026
@perdasilva perdasilva force-pushed the generic-preflights branch 2 times, most recently from 6e181ec to fc6d891 Compare March 5, 2026 13:30
Copilot AI review requested due to automatic review settings March 5, 2026 13:30
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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

Comments suppressed due to low confidence (1)

internal/operator-controller/authorization/rbac.go:342

  • The capacity-estimation comment says "len(clusterCollectionVerbs) records (2)", but clusterCollectionVerbs is now configurable and may be 0 or any length. Update the comment to avoid implying a fixed value (or remove the parenthetical).
	// Calculate initial capacity as an upper-bound estimate:
	// - For each key: len(objectVerbs) records (4)
	// - For unique namespaces: len(namespacedCollectionVerbs) records (1 per unique namespace across all keys in a GVR)
	//   We use totalKeys as upper bound (worst case: each key in different namespace)
	// - For each GVR: len(clusterCollectionVerbs) records (2)
	totalKeys := 0

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

@perdasilva perdasilva force-pushed the generic-preflights branch from fc6d891 to 3007b27 Compare March 5, 2026 15:59
Copy link
Contributor

@pedjak pedjak left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2026
Copilot AI review requested due to automatic review settings March 5, 2026 18:29
@perdasilva perdasilva force-pushed the generic-preflights branch from 3007b27 to f8509ea Compare March 5, 2026 18:29
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2026
@openshift-ci
Copy link

openshift-ci bot commented Mar 5, 2026

New changes are detected. LGTM label has been removed.

@openshift-ci
Copy link

openshift-ci bot commented Mar 5, 2026

[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 ask for approval from pedjak. 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:

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

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@perdasilva perdasilva force-pushed the generic-preflights branch from f8509ea to 458224b Compare March 6, 2026 09:02
@perdasilva perdasilva changed the title ✨ Make RBACPreAuthorizer clusterCollectionVerbs configurable ✨ Make RBACPreAuthorizer collection verbs configurable Mar 6, 2026
Copilot AI review requested due to automatic review settings March 6, 2026 09:33
@perdasilva perdasilva force-pushed the generic-preflights branch from 458224b to 267ee0b Compare March 6, 2026 09:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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


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

@perdasilva perdasilva force-pushed the generic-preflights branch from 267ee0b to 48014ec Compare March 6, 2026 09:41
Copilot AI review requested due to automatic review settings March 6, 2026 12:58
@perdasilva perdasilva force-pushed the generic-preflights branch from 48014ec to d754558 Compare March 6, 2026 12:58
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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

Comments suppressed due to low confidence (1)

internal/operator-controller/authorization/rbac.go:349

  • The capacity-estimation comment hardcodes assumptions that no longer hold now that collection verbs are configurable (e.g., it says len(clusterCollectionVerbs) records "(2)"). Update the comment to describe this generically so it stays correct when callers configure different verb counts.
	// Calculate initial capacity as an upper-bound estimate:
	// - For each key: len(objectVerbs) records (4)
	// - For unique namespaces: len(namespacedCollectionVerbs) records (1 per unique namespace across all keys in a GVR)
	//   We use totalKeys as upper bound (worst case: each key in different namespace)
	// - For each GVR: len(clusterCollectionVerbs) records (2)
	totalKeys := 0

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

@perdasilva perdasilva force-pushed the generic-preflights branch from d754558 to 16b6b38 Compare March 6, 2026 13:11
Copilot AI review requested due to automatic review settings March 6, 2026 13:16
@perdasilva perdasilva force-pushed the generic-preflights branch from 16b6b38 to 1d7a64a Compare March 6, 2026 13:16
@perdasilva perdasilva force-pushed the generic-preflights branch from 1d7a64a to 9d66382 Compare March 6, 2026 13:19
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

internal/operator-controller/authorization/rbac.go:349

  • The capacity-estimation comment hardcodes "(2)" cluster collection verb records, but clusterCollectionVerbs is now configurable and can be 0..N. Update the comment to avoid implying a fixed size (or derive it directly from len(clusterCollectionVerbs)).
	// Calculate initial capacity as an upper-bound estimate:
	// - For each key: len(objectVerbs) records (4)
	// - For unique namespaces: len(namespacedCollectionVerbs) records (1 per unique namespace across all keys in a GVR)
	//   We use totalKeys as upper bound (worst case: each key in different namespace)
	// - For each GVR: len(clusterCollectionVerbs) records (2)
	totalKeys := 0

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

@perdasilva
Copy link
Contributor Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 6, 2026
Make both clusterCollectionVerbs and namespacedCollectionVerbs on
RBACPreAuthorizer configurable via functional options, decoupling them
from the hardcoded verbs that were tightly coupled to the
contentmanager's requirements.

Both the helm and boxcutter appliers explicitly configure
namespacedCollectionVerbs with "create". The helm applier additionally
configures clusterCollectionVerbs with "list" and "watch" (needed by
contentmanager), while the boxcutter applier uses no cluster collection
verbs.

Also updates e2e tests to select the appropriate RBAC template based
on the BoxcutterRuntime feature gate, using a narrower template
without list/watch when BoxcutterRuntime is enabled.

Closes: operator-framework#1911

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Signed-off-by: Per G. da Silva <[email protected]>
@perdasilva perdasilva force-pushed the generic-preflights branch from 9d66382 to 6b23a34 Compare March 6, 2026 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Decouple Pre-Authorizer from Component-Specific Permission Requirements

4 participants