Refactor RBAC management to eliminate code duplication#3368
Conversation
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3368 +/- ##
==========================================
+ Coverage 64.88% 64.89% +0.01%
==========================================
Files 383 383
Lines 37256 37224 -32
==========================================
- Hits 24173 24156 -17
+ Misses 11199 11191 -8
+ Partials 1884 1877 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors RBAC management across the operator to eliminate code duplication by introducing a new pkg/kubernetes/rbac package with a centralized EnsureRBACResources method. The refactoring consolidates the pattern of creating ServiceAccount, Role, and RoleBinding resources that was previously duplicated across multiple controllers.
Changes:
- Introduced new
pkg/kubernetes/rbacpackage with comprehensive RBAC resource management utilities - Refactored MCPServer, VirtualMCPServer, MCPRemoteProxy, and RegistryAPI controllers to use the new RBAC client
- Removed deprecated
pkg/controllerutil/rbac.gothat only supported creation without updates
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/thv-operator/pkg/kubernetes/rbac/rbac.go | New RBAC client implementation with upsert methods for ServiceAccount, Role, RoleBinding, and consolidated EnsureRBACResources method |
| cmd/thv-operator/pkg/kubernetes/rbac/rbac_test.go | Comprehensive test suite (2131 lines) covering all RBAC operations including edge cases and error scenarios |
| cmd/thv-operator/pkg/kubernetes/rbac/doc.go | Excellent documentation explaining error handling patterns and usage examples |
| cmd/thv-operator/pkg/registryapi/rbac.go | Refactored to use new RBAC client, eliminating 135 lines of duplicated code |
| cmd/thv-operator/controllers/virtualmcpserver_controller.go | Migrated to use rbac.Client.EnsureRBACResources, removed 53 lines of duplication |
| cmd/thv-operator/controllers/mcpserver_controller.go | Migrated to use rbac.Client for both proxy runner and MCP server service accounts, removed 77 lines of duplication |
| cmd/thv-operator/controllers/mcpremoteproxy_controller.go | Migrated to use rbac.Client.EnsureRBACResources, removed 59 lines of duplication |
| cmd/thv-operator/controllers/mcpremoteproxy_reconciler_test.go | Updated test to use new rbac.Client API |
| cmd/thv-operator/pkg/controllerutil/rbac.go | Removed deprecated file with limited functionality (create-only, no updates) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Large PR justification has been provided. Thank you!
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Let's just make sure that this keeps working https://discord.com/channels/1184987096302239844/1463493541513793690 |
Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
Introduce a common EnsureRBACResources method in pkg/kubernetes/rbac that consolidates the pattern of creating ServiceAccount, Role, and RoleBinding resources. This eliminates ~170 lines of duplicated code across controllers. Co-authored-by: Yolanda Robla <yolanda@stacklok.com>
Introduce a common EnsureRBACResources method in pkg/kubernetes/rbac
that consolidates the pattern of creating ServiceAccount, Role, and
RoleBinding resources.
Large PR Justification
This is an atomic PR that covers a single functionality. It affects several parts of the code because there are methods used everywhere. It is properly tested and can't be split.