-
Notifications
You must be signed in to change notification settings - Fork 69
✨ (feat): When using Boxcutter feature-gate, use ClusterExtension ServiceAccount for revision operations #2429
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?
✨ (feat): When using Boxcutter feature-gate, use ClusterExtension ServiceAccount for revision operations #2429
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
f7ebc0c to
2c82746
Compare
2c82746 to
7bcd536
Compare
7bcd536 to
d7760da
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 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/controllers/revision_engine_factory_test.go
Outdated
Show resolved
Hide resolved
d7760da to
586787b
Compare
586787b to
b6ad85e
Compare
a9a925a to
159f980
Compare
159f980 to
fcf2a0a
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 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fcf2a0a to
2a2d85c
Compare
|
[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 |
2417fd2 to
5e67f84
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 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5e67f84 to
ba37149
Compare
| # for roles it doesn't have and grant permissions beyond its own. This is required | ||
| # because extension bundles contain their own RBAC that must be created. | ||
| # See docs/concepts/permission-model.md for details on these requirements. | ||
| verbs: [ update, create, list, watch, get, delete, patch, bind, escalate ] |
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.
@perdasilva that proves it is working :-)
The permissions were missing, and e2e tests were failing.
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 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/controllers/revision_engine_factory.go
Outdated
Show resolved
Hide resolved
702740f to
3d14643
Compare
…operations Implement serviceAccount-scoped token-based authentication for ClusterExtensionRevision controller using annotation-based configuration. - Add RevisionEngineFactory with CreateRevisionEngine(ctx, rev) interface - Read ServiceAccount from annotations (no ClusterExtension dependency) - Token-based auth using TokenInjectingRoundTripper - ServiceAccount name and namespace in annotations for observability - TrackingCache uses global client for caching/cleanup - Comprehensive error path tests ClusterExtensionRevision can exist independently. Easy mode impersonation deferred until API is finalized. Assisted-by: Cursor
Scoped ServiceAccount clients need `bind` and `escalate` verbs to create ClusterRoleBindings. The admin client previously used bypassed this RBAC requirement. The documentation (docs/concepts/permission-model.md) already specifies that ServiceAccounts need `bind` and `escalate` verbs for RBAC resources to install extensions with their own RBAC. The e2e test template was missing these verbs, causing tests to fail when using scoped ServiceAccount clients (which properly enforce Kubernetes RBAC) instead of the admin client.
Adds documentation comments to all label/annotation constants explaining: - What each constant represents - Where they are applied (labels vs annotations) - ServiceAccount constants document their relationship to ClusterExtension spec Addresses code review feedback for improved maintainability.
The upgrade test ServiceAccount needs permissions to manage ClusterExtensionRevisions when BoxcutterRuntime is enabled. Without these permissions, the upgraded controller cannot create or update ClusterExtensionRevision resources, causing the ClusterExtension to fail reconciliation after upgrade.
3d14643 to
76b836b
Compare
What's New
When BoxcutterRuntime is enabled, OLM v1 now correctly uses the ServiceAccount you specify in your ClusterExtension to install and manage extensions. This ensures your extensions are installed with only the permissions you grant, not admin privileges.
What This Means for You
More Control:
Better Security:
Example
What happens: OLM installs the extension using
my-installerServiceAccount. The extension can only do what you've granted that ServiceAccount permission to do.