-
Notifications
You must be signed in to change notification settings - Fork 70
✨ Add preauthorizer checks to Boxcutter applier #2443
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.
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
base: main
Are you sure you want to change the base?
✨ Add preauthorizer checks to Boxcutter applier #2443
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this 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 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.
6f94c27 to
9d08956
Compare
9d08956 to
7cdc319
Compare
876225e to
7f4a867
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2443 +/- ##
==========================================
+ Coverage 69.48% 69.52% +0.04%
==========================================
Files 101 101
Lines 7701 7738 +37
==========================================
+ Hits 5351 5380 +29
- Misses 1914 1920 +6
- Partials 436 438 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this 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.
7f4a867 to
d542d16
Compare
There was a problem hiding this 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 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.
d542d16 to
53a9309
Compare
53a9309 to
5cd737f
Compare
There was a problem hiding this 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 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5cd737f to
2b041e5
Compare
2b041e5 to
13cdb4b
Compare
13cdb4b to
9e07dab
Compare
There was a problem hiding this 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 no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9e07dab to
387691d
Compare
There was a problem hiding this 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 no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
387691d to
12ab842
Compare
|
I think I am ok with the change It would be nice get a second reviewer maybe @tylerslaton who works in this area and has more context as we could get help from @pedjak as well. |
|
/hold I think I've found an issue |
Signed-off-by: Per Goncalves da Silva <[email protected]>
12ab842 to
b295770
Compare
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/uhold |
There was a problem hiding this 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 no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/unhold |
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:
createOrUpdatemethod to call perform the PreAuthorization checksPreAuthorizer Refactoring Notes
Previously, the
PreAuthorizer.PreAuthorizemethod took a ClusterExtension as a parameter and used it to derive the user to check the permissions against and to generate theclusterextensions/finalizersupdate 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:
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).
Note
Reviewer Checklist