🐛 add create verb to boxcutter preflight#2587
🐛 add create verb to boxcutter preflight#2587kuiwang02 wants to merge 1 commit intooperator-framework:mainfrom
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.
Pull request overview
This PR fixes Boxcutter applier preflight RBAC validation so it correctly checks for namespace-scoped CREATE permissions, preventing installs from passing preflight but failing later when creating namespaced resources (e.g., ServiceAccounts).
Changes:
- Configure Boxcutter’s
RBACPreAuthorizerwithWithNamespacedCollectionVerbs("create")under thePreflightPermissionsfeature gate.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kuiwang02
changed the title
kuiwang02
changed the title
kuiwang02
changed the title
|
/cc @perdasilva |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2587 +/- ##
==========================================
+ Coverage 65.84% 68.51% +2.66%
==========================================
Files 137 137
Lines 9560 9583 +23
==========================================
+ Hits 6295 6566 +271
+ Misses 2795 2520 -275
- Partials 470 497 +27
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:
|
pedjak
left a comment
pedjak
left a comment
There was a problem hiding this comment.
please add e2e tests that assert the change.
@fgiudici Thanks! |
@pedjak I add e2e cases for the change and it passes. could you please review it again? Thanks |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fgiudici, tmshort The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@pedjak could you please review it again? thanks. |
|
/lgtm |
|
ping @pedjak |
| @@ -0,0 +1,68 @@ | |||
| --- | |||
There was a problem hiding this comment.
this file is not needed, because we execute the new test only with boxcutter runtime.
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@perdasilva would like to have a look at PR? |
test/e2e/steps/steps.go
Outdated
| // Note: This function is specifically for Boxcutter runtime tests (requires @BoxcutterRuntime tag). | ||
| func ServiceAccountWithoutCreatePermissionsIsAvailableInTestNamespace(ctx context.Context, serviceAccount string) error { | ||
| // This test is only valid with Boxcutter runtime enabled | ||
| if enabled, found := featureGates[features.BoxcutterRuntime]; !found || !enabled { |
There was a problem hiding this comment.
would it be worth moving this to a helper function? e.g.
func IsFeatureGateEnabled(feature featuregate.Feature) bool {
enabled, found := featureGates[feature]
return enabled && found
or something like that?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Fixes Boxcutter applier's preflight permission check to properly validate namespace-scoped CREATE permissions.
Problem
After PR #2539, the Boxcutter applier's RBACPreAuthorizer was missing the
WithNamespacedCollectionVerbs("create")configuration. This caused the preflight check to pass without validating CREATE permissions, leading to installation failures when the applier attempted to create resources like ServiceAccounts.The Helm applier was correctly configured with this option (line 746), but the Boxcutter applier configuration (line 620) was missing it.
Changes
WithNamespacedCollectionVerbs("create")to Boxcutter's RBACPreAuthorizer instantiation inboxcutterReconcilerConfigurator.Configure()Note: Boxcutter does not need
WithClusterCollectionVerbs("list", "watch")because it doesn't use the contentmanager component, unlike the Helm applier.Related Issues
Assisted-By: Claude Code