✨ Add preauthorizer checks to Boxcutter applier #2443

perdasilva · 2026-01-13T14:42:13Z

Description

Adds the PreAuthorizer checks to the Boxcutter applier for feature-gate parity between Helm and Boxcutter appliers.

The Boxcutter applier's PreAuthorization check requires clusterextensions/finalizers and clusterextensionrevisions/finalizers update permissions (on top of the permissions to manage the bundle's resources).

Changes:

Refactors PreAuthorizer away from handling ClusterExtension specific permissions
Refactors Helm applier to add ClusterExtension permissions for the PreAuthorization check
Updates the Helm applier PreAuthorization unit tests and adds a component integration test to ensure it is being called with the right parameters
Adds PreAuthorization checks to the Boxcutter Applier and adds the clusterextensionrevisions/finalizers update permission for the check
Refactors Boxcutters createOrUpdate method to call perform the PreAuthorization checks
Adds PreAuthorizator intergration tests to Boxcutter unit test suite

PreAuthorizer Refactoring Notes

Previously, the PreAuthorizer.PreAuthorize method took a ClusterExtension as a parameter and used it to derive the user to check the permissions against and to generate the clusterextensions/finalizers update permission implicitly required by the applier to manage update ownerReferences blockerOwnerDeletion.

This PR makes refactors the PreAuthorize methods to substitute the ClusterExtension parameter by two parameters:

manifestManager -> the user to check the permissions agains
additionsRequiredPermissions -> permissions that are required on top of the permissions strictly required to manage the manifests input through the manifestReader parameter.

This makes the PreAuthorizer more generic by removing ClusterExtension concerns, and allows the applier to define which permissions are needed for its operation beyond those dictated by the bundle manifests. Making the PreAuthorizer more generic, and moving applier specific concerns to the applier. The PreAuthorizer and Applier unit tests are update for this change (removing the clusterextensionrevision perms from the PreAuthorizer tests and adding that check to the applier).

E2E Notes

Added the ClusterExtension reports <condition> as <status> with Reason <reason> and Message including <message fragment> to avoid checking the entire error message but only the salient points as the set could change in the future
Refactored the templating functions to split concerns
Split the namespace and service account from the RBAC step into its own step (RBAC steps call the service account step - so their behavior hasn't changes completely) - this enables testing the ClusterExtension before the service account gets its permissions

Note

The Boxcutter applier still uses the manager client to create and update the revisions. The PreAuthorizer just ensures the service account has all the necessary permissions to do its job.

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2026-01-13T14:42:20Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`12adada`
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/696a5d2d0a15210008840673
😎 Deploy Preview	https://deploy-preview-2443--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 adds PreAuthorizer checks to the Boxcutter applier to achieve feature-gate parity with the Helm applier. The implementation validates that service accounts have the necessary RBAC permissions before applying cluster extensions, including the ability to update clusterextensionrevisions/finalizers which is specific to the Boxcutter workflow.

Changes:

Added an Option pattern to configure PreAuthorizer with ClusterExtensionRevision finalizer permission checks
Integrated PreAuthorizer into the Boxcutter applier with manifest generation and permission validation
Updated main.go to initialize PreAuthorizer with the new option when the PreflightPermissions feature gate is enabled

Reviewed changes

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

Show a summary per file

File	Description
internal/operator-controller/authorization/rbac.go	Added Option pattern and WithClusterExtensionRevisionPerms to optionally check for update permissions on clusterextensionrevisions/finalizers
internal/operator-controller/authorization/rbac_test.go	Added test case for PreAuthorizer with ClusterExtensionRevision permissions
internal/operator-controller/applier/boxcutter.go	Added PreAuthorizer field and runPreAuthorizationChecks method to validate permissions before applying revisions
internal/operator-controller/applier/boxcutter_test.go	Added integration test for PreAuthorizer with fake implementation
cmd/operator-controller/main.go	Initialize PreAuthorizer with WithClusterExtensionRevisionPerms option when PreflightPermissions feature gate is enabled

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

internal/operator-controller/authorization/rbac.go

internal/operator-controller/authorization/rbac_test.go

internal/operator-controller/authorization/rbac.go

internal/operator-controller/applier/boxcutter.go

codecov · 2026-01-13T15:40:54Z

Codecov Report

❌ Patch coverage is 95.18072% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.67%. Comparing base (dc20dfb) to head (12adada).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/operator-controller/applier/boxcutter.go	90.90%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2443      +/-   ##
==========================================
+ Coverage   69.48%   73.67%   +4.18%     
==========================================
  Files         101      101              
  Lines        7701     7741      +40     
==========================================
+ Hits         5351     5703     +352     
+ Misses       1914     1592     -322     
- Partials      436      446      +10

Flag	Coverage Δ
e2e	`46.19% <0.00%> (-0.28%)`	⬇️
experimental-e2e	`53.65% <74.69%> (+40.09%)`	⬆️
unit	`57.22% <90.36%> (+0.14%)`	⬆️

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.

Copilot

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

Copilot

Pull request overview

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

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

internal/operator-controller/applier/boxcutter.go

internal/operator-controller/applier/boxcutter_test.go

internal/operator-controller/applier/boxcutter.go

internal/operator-controller/authorization/rbac_test.go

Copilot

Pull request overview

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

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

internal/operator-controller/applier/helm_test.go

internal/operator-controller/authorization/rbac.go

perdasilva · 2026-01-15T12:44:17Z

/hold I think I've found an issue

openshift-ci · 2026-01-15T14:08:18Z

New changes are detected. LGTM label has been removed.

openshift-ci · 2026-01-15T14:08:21Z

[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 camilamacedo86. 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:

OWNERS

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

perdasilva · 2026-01-15T14:08:24Z

/uhold

Copilot

Pull request overview

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

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

perdasilva · 2026-01-15T14:23:56Z

/unhold

pedjak

one or more e2e tests asserting the behavior would be very helpful to add.

pedjak · 2026-01-15T17:39:35Z

internal/operator-controller/applier/boxcutter.go

+	return strings.NewReader(manifestBuilder.String()), nil
+}
+
+func clusterExtensionRevisionManagementPermissions(manifestManager user.Info, rev *ocv1.ClusterExtensionRevision) []authorizer.AttributesRecord {


nit: can we come up with some shorter function name perhaps?

Updated to revisionManagementPerms, I also updated the other one to extManagementPerms

pedjak · 2026-01-15T17:48:58Z

internal/operator-controller/applier/boxcutter.go

-func (bc *Boxcutter) createOrUpdate(ctx context.Context, obj client.Object) error {
-	if obj.GetObjectKind().GroupVersionKind().Empty() {
-		gvk, err := apiutil.GVKForObject(obj, bc.Scheme)
+func (bc *Boxcutter) createOrUpdateRevisionWithPreAuthorization(ctx context.Context, ext *ocv1.ClusterExtension, rev *ocv1.ClusterExtensionRevision) error {


nit: was function name changing needed? createOrUpdate was nice and short. Perhaps we do not need to pass ClusterExtension to just extract user infos, we could actually pass userinfos here instead?

I'd say if there's you're adding a side-effect it's a good idea to change the function. Also, it's a private function, so it's it shouldn't an issue. But, I'm not married to it. I've reverted the name and added a call out to the side-effect to godoc and refactored the function signature.

Copilot

Pull request overview

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

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

test/e2e/steps/steps.go

internal/operator-controller/applier/boxcutter.go

test/e2e/features/recover.feature

trgeiger · 2026-01-16T15:10:39Z

internal/operator-controller/applier/boxcutter.go


+// runPreAuthorizationChecks runs PreAuthorization checks if the PreAuthorizer is set. An error will be returned if
+// the ClusterExtension service account does not have the necessary permissions to manage the revision's resources
+func (bc *Boxcutter) runPreAuthorizationChecks(ctx context.Context, manifestManager user.Info, rev *ocv1.ClusterExtensionRevision) error {


I know that you're taking this variable name from the example already set in rbac.go, but to me the term "manifestManager" implies a more complex object than just user.Info, especially given we have stuff like the manager.Manager in our codebase.

This is purely a nitpick, but I wonder if a different variable name would help future legibility of the code. Maybe manifestUser? or manifestOwner? The latter might not be entirely accurate though.

I took the previous variable name, I think. Though, I'm happy to updated it to anything. The reason I thought manifestManager was reasonable is that it fits with the mental model of "we're checking permissions against the user that will manage the lifecycle of these manifests". Even just, user would be ok, imo, tho.

I've updated to just user

Yeah, user works. I see what you're saying about manifestManager, it definitely makes sense but I think it carries a bit of implied complexity that doesn't seem to fit. User is perfect.

Copilot

Pull request overview

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

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

internal/operator-controller/authorization/rbac.go

internal/operator-controller/applier/helm.go

test/e2e/steps/steps.go

Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>

Copilot

Pull request overview

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

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

Copilot · 2026-01-16T15:54:12Z

internal/operator-controller/applier/boxcutter_test.go

+						require.Len(t, perms, 1)
+						require.Equal(t, authorizer.AttributesRecord{
+							User:            user,
+							Name:            "test-ext-1",


The revision name is hardcoded as 'test-ext-1' but doesn't match the ClusterExtension name 'test-ext' defined on line 964. This appears to be intentional based on revision naming conventions, but consider adding a comment explaining why the name differs or deriving it from the extension name for clarity.

Copilot AI review requested due to automatic review settings January 13, 2026 14:42

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 13, 2026

Copilot started reviewing on behalf of perdasilva January 13, 2026 14:42 View session

Copilot AI reviewed Jan 13, 2026

View reviewed changes

perdasilva force-pushed the boxcutter-preflight-auth-checks branch from 6f94c27 to 9d08956 Compare January 13, 2026 14:56

Copilot AI review requested due to automatic review settings January 13, 2026 15:00

perdasilva force-pushed the boxcutter-preflight-auth-checks branch from 9d08956 to 7cdc319 Compare January 13, 2026 15:00

Copilot started reviewing on behalf of perdasilva January 13, 2026 15:00 View session

perdasilva force-pushed the boxcutter-preflight-auth-checks branch 2 times, most recently from 876225e to 7f4a867 Compare January 13, 2026 15:25

Copilot AI reviewed Jan 13, 2026

View reviewed changes

perdasilva force-pushed the boxcutter-preflight-auth-checks branch from 7f4a867 to d542d16 Compare January 13, 2026 17:09

perdasilva marked this pull request as ready for review January 13, 2026 17:09

Copilot AI review requested due to automatic review settings January 13, 2026 17:09

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 13, 2026

openshift-ci bot requested review from joelanford and tmshort January 13, 2026 17:09

Copilot started reviewing on behalf of perdasilva January 13, 2026 17:11 View session

Copilot AI reviewed Jan 13, 2026

View reviewed changes

perdasilva force-pushed the boxcutter-preflight-auth-checks branch from d542d16 to 53a9309 Compare January 14, 2026 16:06

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 14, 2026

Copilot AI review requested due to automatic review settings January 14, 2026 16:08

perdasilva force-pushed the boxcutter-preflight-auth-checks branch from 53a9309 to 5cd737f Compare January 14, 2026 16:08

Copilot started reviewing on behalf of perdasilva January 14, 2026 16:09 View session

Copilot AI reviewed Jan 14, 2026

View reviewed changes

internal/operator-controller/applier/helm_test.go Outdated Show resolved Hide resolved

internal/operator-controller/applier/helm_test.go Outdated Show resolved Hide resolved

internal/operator-controller/authorization/rbac.go Outdated Show resolved Hide resolved

perdasilva force-pushed the boxcutter-preflight-auth-checks branch from 5cd737f to 2b041e5 Compare January 14, 2026 16:26

Copilot AI review requested due to automatic review settings January 14, 2026 16:28

perdasilva force-pushed the boxcutter-preflight-auth-checks branch from 2b041e5 to 13cdb4b Compare January 14, 2026 16:28

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 15, 2026

Copilot AI review requested due to automatic review settings January 15, 2026 14:08

perdasilva force-pushed the boxcutter-preflight-auth-checks branch from 12ab842 to b295770 Compare January 15, 2026 14:08

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2026

Copilot started reviewing on behalf of perdasilva January 15, 2026 14:08 View session

Copilot AI reviewed Jan 15, 2026

View reviewed changes

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 15, 2026

pedjak reviewed Jan 15, 2026

View reviewed changes

perdasilva force-pushed the boxcutter-preflight-auth-checks branch from b295770 to d489d7b Compare January 16, 2026 09:27

Copilot AI review requested due to automatic review settings January 16, 2026 13:54

perdasilva force-pushed the boxcutter-preflight-auth-checks branch from d489d7b to 85dfbf8 Compare January 16, 2026 13:54

Copilot started reviewing on behalf of perdasilva January 16, 2026 13:54 View session

Copilot AI reviewed Jan 16, 2026

View reviewed changes

test/e2e/steps/steps.go Show resolved Hide resolved

internal/operator-controller/applier/boxcutter.go Outdated Show resolved Hide resolved

test/e2e/features/recover.feature Show resolved Hide resolved

perdasilva force-pushed the boxcutter-preflight-auth-checks branch from 85dfbf8 to c96c4bc Compare January 16, 2026 14:04

trgeiger reviewed Jan 16, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings January 16, 2026 15:19

perdasilva force-pushed the boxcutter-preflight-auth-checks branch from c96c4bc to 40a2645 Compare January 16, 2026 15:19

Copilot started reviewing on behalf of perdasilva January 16, 2026 15:19 View session

Copilot AI reviewed Jan 16, 2026

View reviewed changes

perdasilva force-pushed the boxcutter-preflight-auth-checks branch from 40a2645 to 325f80d Compare January 16, 2026 15:35

Add preflight checks to Boxcutter applier

12adada

Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>

Copilot AI review requested due to automatic review settings January 16, 2026 15:45

perdasilva force-pushed the boxcutter-preflight-auth-checks branch from 325f80d to 12adada Compare January 16, 2026 15:45

Copilot started reviewing on behalf of perdasilva January 16, 2026 15:46 View session

Copilot AI reviewed Jan 16, 2026

View reviewed changes

✨ Add preauthorizer checks to Boxcutter applier #2443

Are you sure you want to change the base?

✨ Add preauthorizer checks to Boxcutter applier #2443

Uh oh!

Conversation

perdasilva commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

PreAuthorizer Refactoring Notes

E2E Notes

Note

Reviewer Checklist

Uh oh!

netlify bot commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

perdasilva commented Jan 15, 2026

Uh oh!

openshift-ci bot commented Jan 15, 2026

Uh oh!

openshift-ci bot commented Jan 15, 2026

Uh oh!

perdasilva commented Jan 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

perdasilva commented Jan 15, 2026

Uh oh!

pedjak left a comment

Choose a reason for hiding this comment

Uh oh!

pedjak Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

perdasilva Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedjak Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

perdasilva Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

perdasilva commented Jan 13, 2026 •

edited

Loading

netlify bot commented Jan 13, 2026 •

edited

Loading

codecov bot commented Jan 13, 2026 •

edited

Loading

perdasilva Jan 16, 2026 •

edited

Loading