Skip to content

🐛 make create verb a fixed namespaced collection verb for preflight checks - #2587

Merged
openshift-merge-bot[bot] merged 1 commit into
operator-framework:mainfrom
kuiwang02:fix78211
Mar 30, 2026
Merged

🐛 make create verb a fixed namespaced collection verb for preflight checks#2587
openshift-merge-bot[bot] merged 1 commit into
operator-framework:mainfrom
kuiwang02:fix78211

Conversation

@kuiwang02

@kuiwang02kuiwang02 commented Mar 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Makes the create verb a fixed default for namespaced collection verbs in RBAC preflight checks, ensuring consistent permission validation across all appliers (Boxcutter and Helm).

Problem

PR #2539 introduced configurable namespace-scoped collection verbs for preflight permission checks. However:

  • The Boxcutter applier was missing the WithNamespacedCollectionVerbs("create") configuration
  • This caused preflight checks to pass without validating CREATE permissions
  • Installations failed when the applier attempted to create resources like ServiceAccounts

Original issue: Only Boxcutter was affected because Helm had the correct configuration.

Approach Change (Per @perdasilva's Suggestion)

Following feedback from @perdasilva, this PR uses a broader approach instead of just adding the missing configuration to Boxcutter:

Instead of requiring each applier to explicitly call WithNamespacedCollectionVerbs("create"),
We now define namespacedCollectionVerbs as a fixed variable (similar to objectVerbs) and initialize it by default in NewRBACPreAuthorizer.

Related Issues

Assisted-By: Claude Code

CopilotAI review requested due to automatic review settings March 24, 2026 05:30
@openshift-ci
openshift-ciBot requested review from pedjak and trgeigerMarch 24, 2026 05:30
@netlify

netlifyBot commented Mar 24, 2026

Copy link
Copy Markdown

Deploy Preview for olmv1 ready!

NameLink
🔨 Latest commit42bc39a
🔍 Latest deploy loghttps://app.netlify.com/projects/olmv1/deploys/69ca37ec7a49fd0008cee0d7
😎 Deploy Previewhttps://deploy-preview-2587--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.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 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 RBACPreAuthorizer with WithNamespacedCollectionVerbs("create") under the PreflightPermissions feature gate.

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

@kuiwang02kuiwang02 changed the title 🐛add create verb to boxcutter preflight 🐛 add create verb to boxcutter preflightMar 24, 2026
@kuiwang02kuiwang02 changed the title 🐛 add create verb to boxcutter preflightPatch fix: 🐛 (:bug:) add create verb to boxcutter preflightMar 24, 2026
@kuiwang02kuiwang02 changed the title Patch fix: 🐛 (:bug:) add create verb to boxcutter preflight🐛 add create verb to boxcutter preflightMar 24, 2026
@kuiwang02

Copy link
Copy Markdown
ContributorAuthor

/cc @perdasilva

@openshift-ci
openshift-ciBot requested a review from perdasilvaMarch 24, 2026 05:37
@codecov

codecovBot commented Mar 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.86%. Comparing base (a307a6d) to head (42bc39a).
⚠️ Report is 17 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #2587 +/- ##
==========================================
+ Coverage 65.84% 68.86% +3.01% 
==========================================
Files 137 139 +2 Lines 9560 9872 +312 ==========================================
+ Hits 6295 6798 +503 + Misses 2795 2557 -238 - Partials 470 517 +47 
FlagCoverage Δ
e2e37.75% <0.00%> (+26.56%)⬆️
experimental-e2e52.43% <100.00%> (+1.37%)⬆️
unit53.51% <100.00%> (+0.71%)⬆️

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.

@fgiudicifgiudici left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Good catch!

@openshift-ciopenshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2026

@pedjakpedjak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please add e2e tests that assert the change.

@openshift-ciopenshift-ciBot removed the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2026
@kuiwang02

Copy link
Copy Markdown
ContributorAuthor

LGTM. Good catch!

@fgiudici Thanks!

@kuiwang02

Copy link
Copy Markdown
ContributorAuthor

please add e2e tests that assert the change.

@pedjak I add e2e cases for the change and it passes. could you please review it again? Thanks

@tmshort

Copy link
Copy Markdown
Contributor

/approve

@openshift-ciopenshift-ciBot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 25, 2026
@kuiwang02
kuiwang02 requested a review from pedjakMarch 25, 2026 23:54
@kuiwang02

Copy link
Copy Markdown
ContributorAuthor

@pedjak could you please review it again? thanks.

@jianzhangbjz

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ciopenshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2026
@tmshort

Copy link
Copy Markdown
Contributor

ping @pedjak

@@ -0,0 +1,68 @@
---

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this file is not needed, because we execute the new test only with boxcutter runtime.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done, thanks

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 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.

Comment threadtest/e2e/steps/steps.go
@tmshort

Copy link
Copy Markdown
Contributor

@pedjak you'll have to remove your "Changes Requested" for this PR to advance (even if it has an LGTM).
@perdasilva it appears as though updates were made.

CopilotAI review requested due to automatic review settings March 30, 2026 06:27

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 3 comments.


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

Comment threadinternal/operator-controller/authorization/rbac.go
Comment threadcmd/operator-controller/main.go
Comment threadinternal/operator-controller/authorization/rbac.go Outdated
@kuiwang02

Copy link
Copy Markdown
ContributorAuthor

@perdasilva Following your feedback, this PR now uses a different approach:

Instead of requiring each applier to explicitly call WithNamespacedCollectionVerbs("create"), we now define namespaceCollectionVerbs as a fixed variable (similar to objectVerbs) and initialize it by default in NewRBACPreAuthorizer.

please help review it again. thanks

@kuiwang02kuiwang02 changed the title 🐛 add create verb to boxcutter preflight🐛 make create verb a fixed namespaced collection verb for preflight checksMar 30, 2026
authorizer: newRBACAuthorizer(cl),
ruleResolver: newRBACRulesResolver(cl),
restMapper: cl.RESTMapper(),
namespacedCollectionVerbs: slices.Clone(namespacedCollectionVerbs),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's remove this field from the struct and just reference namespacedCollectionVerbs directly like we do with objectVerbs. Wdyt?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@perdasilva based on your comment, I make the change https://github.com/operator-framework/operator-controller/compare/3860ac5bf8ecbcf0207138793f3a9c74df9ce231..42bc39a00af0f6e6c11df5fa6e4a38b0ea2dac6f and the test is failing (before this change, the test is ok)

But I do not find root cause on why it is failing (I do not think the changed code cause it). so, I want to rerun test. but I do not know how to rerun it. could you please help me to rerun the test job? thanks

CopilotAI review requested due to automatic review settings March 30, 2026 08:44

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 no new comments.


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

@kuiwang02

Copy link
Copy Markdown
ContributorAuthor

/retest-required

@perdasilva

Copy link
Copy Markdown
Contributor

/approve

@kuiwang02

Copy link
Copy Markdown
ContributorAuthor

/test experimental-e2e

@openshift-ci

Copy link
Copy Markdown

@kuiwang02: No presubmit jobs available for operator-framework/operator-controller@main

Details

In response to this:

/test experimental-e2e

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@kuiwang02

Copy link
Copy Markdown
ContributorAuthor

/retest

@pedjakpedjak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/approve

@openshift-ciopenshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Mar 30, 2026

@rashmigottipatirashmigottipati left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fgiudici, pedjak, perdasilva, rashmigottipati, tmshort

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

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [pedjak,perdasilva,tmshort]

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

@openshift-merge-bot
openshift-merge-botBot merged commit 7145047 into operator-framework:mainMar 30, 2026
34 of 36 checks passed
@kuiwang02
kuiwang02 deleted the fix78211 branch March 31, 2026 00:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approvedIndicates a PR has been approved by an approver from all required OWNERS files.lgtmIndicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@kuiwang02@tmshort@jianzhangbjz@pedjak@perdasilva@rashmigottipati@fgiudici