feat(controller): create prerequisites from annotation - #227
Conversation
k8s-ci-robot
added
the
do-not-merge/invalid-commit-message
✅ Deploy Preview for mcp-lifecycle-operator ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rdwj 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 |
|
|
|
Welcome @rdwj! |
k8s-ci-robot
added
the
cncf-cla: no
|
Hi @rdwj. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
k8s-ci-robot
added
needs-ok-to-test
When the annotation mcp.x-k8s.io/auto-create-prerequisites is set to "true", the operator creates missing ServiceAccounts and ConfigMaps referenced by the MCPServer CR before validation. Resources are owned by the MCPServer so they are cleaned up on deletion. Related: kubernetes-sigs#226
Controller-runtime's client cache requires list and watch permissions to set up informers. Without them, the ensurePrerequisites function cannot check ServiceAccount existence.
d7cb3fc to
9c3cbfe
Compare
k8s-ci-robot
removed
the
do-not-merge/invalid-commit-message
|
ServiceAccount lookups should use The prerequisites code uses This is the same problem PR #204 fixed for pods. That PR switched Note that ConfigMaps are fine with Fix: Use // prerequisites code:
err := r.APIReader.Get(ctx, client.ObjectKey{Name: saName, Namespace: server.Namespace}, sa)
// RBAC marker:
// +kubebuilder:rbac:groups="",resources=serviceaccounts,verbs=get;create
https://github.com/kubernetes-sigs/mcp-lifecycle-operator/blob/9c3cbfed38dcf7e30a5a03e9b115e57eaabb2deb/internal/controller/mcpserver_controller_prerequisites.go#L72-L80 |
|
Error swallowing in The error from if err := r.ensurePrerequisites(ctx, mcpServer); err != nil {
logger.Error(err, "Failed to create prerequisites")
// Don't return error — fall through to validation which will report the specific missing resource
}The comment says "fall through to validation which will report the specific missing resource". This assumption holds for ConfigMaps (validated by validateStorageMount), Failure scenario:
The transient error never triggers a retry because it's never returned. The controller's established pattern (see classifyAPIError) returns transient errors so the Fix options:
|
|
Thinking about it - why should the operator create resources for 3rd party functionality This is however to me a big diff to what we create for "running" (or operating) the mcp server (e.g. Perhaps something on that "catalog" should do more - as an integrating factor? |
Use r.APIReader.Get() instead of r.Get() for ServiceAccount existence checks to avoid spinning up an unnecessary informer on the opt-in annotation path. Follows the pattern established in PR kubernetes-sigs#204. Return errors from ensurePrerequisites() so transient failures trigger reconcile retries with backoff, rather than swallowing them and proceeding to create a Deployment referencing a potentially missing SA. Drop list/watch from ServiceAccount RBAC since the controller doesn't need an informer for this resource type. Assisted-by: Claude Code (Opus 4.6)
📝 WalkthroughWalkthroughThe PR adds auto-create-prerequisites functionality to the MCPServer controller. When the ChangesAuto-Create Prerequisites Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Thanks for the thorough review @matzew — both catches are spot on. APIReader for ServiceAccount lookups: Great catch — I should have followed the pattern from #204. Switched to Error swallowing: You're right that the "fall through to validation" assumption doesn't hold for ServiceAccounts since Both fixes are in 6aef031. All tests and lint pass. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/controller/mcpserver_controller_prerequisites_test.go (1)
132-371: ⚡ Quick winAdd one regression test for prerequisite API-error propagation.
Current coverage validates happy paths/gating well, but it does not assert that a failure inside prerequisite creation is returned from
Reconcile(so controller-runtime retries). Add a case that injects a failing read/create for prerequisite resources and expects a non-nil reconcile error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/mcpserver_controller_prerequisites_test.go` around lines 132 - 371, Add a regression test that injects a failing client behavior and asserts Reconcile returns an error: create a new It case (e.g. "should propagate API errors when creating prerequisites") that builds an MCPServer with AnnotationAutoCreatePrerequisites="true" and prerequisite names (ServiceAccount/ConfigMap), then construct the reconciler using the test helper that accepts a client wrapper (use or add a failing client/fake that returns an error from Get/Create for resources used in prerequisite creation), call controllerReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: typeNamespacedName}) and Expect(err).To(HaveOccurred()) to verify the error is propagated; reference helpers/newReconcilerForTest (or newReconcilerForTestWithFakeEvents) and the Reconcile method to locate where to inject the failing client and the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/controller/mcpserver_controller_prerequisites_test.go`:
- Around line 132-371: Add a regression test that injects a failing client
behavior and asserts Reconcile returns an error: create a new It case (e.g.
"should propagate API errors when creating prerequisites") that builds an
MCPServer with AnnotationAutoCreatePrerequisites="true" and prerequisite names
(ServiceAccount/ConfigMap), then construct the reconciler using the test helper
that accepts a client wrapper (use or add a failing client/fake that returns an
error from Get/Create for resources used in prerequisite creation), call
controllerReconciler.Reconcile(ctx, reconcile.Request{NamespacedName:
typeNamespacedName}) and Expect(err).To(HaveOccurred()) to verify the error is
propagated; reference helpers/newReconcilerForTest (or
newReconcilerForTestWithFakeEvents) and the Reconcile method to locate where to
inject the failing client and the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ec28f6c-bc20-4942-9bde-48f7065e2e7e
📒 Files selected for processing (4)
config/rbac/role.yamlinternal/controller/mcpserver_controller.gointernal/controller/mcpserver_controller_prerequisites.gointernal/controller/mcpserver_controller_prerequisites_test.go
|
I am not sure we really must have this knob? See: #227 (comment) |
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
Useful feature and a clean implementation — the annotation matches the reserved mcp.x-k8s.io/ prefix, events follow the house style, and the envtest cases assert real behavior (including the existing-resources-untouched case).
Requesting changes for three things:
- SA auto-creation needs a security story: it can materialize a principal that pre-existing RoleBindings already grant permissions to (details inline).
- The annotation is user-facing but undocumented — site-src deserves a section; the introduction.md labels/annotations area is a natural home.
- The empty-ConfigMap-plus-owner-reference semantics should be documented or reconsidered (inline).
Nit: the PR body says serviceaccounts gained list/watch/create, but the diff adds get/create — worth syncing the description.
| // ensureServiceAccount creates the ServiceAccount referenced by | ||
| // spec.runtime.security.serviceAccountName if it does not already exist. | ||
| func (r *MCPServerReconciler) ensureServiceAccount(ctx context.Context, server *mcpv1alpha1.MCPServer) error { | ||
| saName := server.Spec.Runtime.Security.ServiceAccountName |
There was a problem hiding this comment.
Auto-creating SAs lends the operator's create serviceaccounts privilege to anyone who can create an MCPServer. The sharp edge: if a RoleBinding or ClusterRoleBinding already exists granting permissions to a not-yet-existing SA name in the namespace, creating an MCPServer with that serviceAccountName materializes the principal and runs the workload under those permissions — a capability the MCPServer author may not otherwise have. Worth covering in the docs/threat model, and consider limiting auto-creation to a convention-derived name (e.g. <mcpserver-name>-sa) rather than any spec-supplied value.
| } | ||
|
|
||
| sa := &corev1.ServiceAccount{} | ||
| err := r.APIReader.Get(ctx, client.ObjectKey{Name: saName, Namespace: server.Namespace}, sa) |
There was a problem hiding this comment.
Good call using APIReader here — with only get;create RBAC on serviceaccounts, a cached r.Get would try to start an SA informer and fail at runtime. Worth a short code comment saying so, so a future refactor doesn't simplify this back to r.Get.
| Name: cmName, | ||
| Namespace: server.Namespace, | ||
| }, | ||
| Data: map[string]string{}, |
There was a problem hiding this comment.
Two questions on the empty-ConfigMap semantics: (1) the server now passes validation and starts with an empty config mount — is that useful on its own for the catalog flow, or does something populate the CM afterwards? (2) If something does populate it later, the owner reference means deleting the MCPServer garbage-collects the populated CM, and a recreate starts from empty again. If the CM is meant to outlive the CR once it carries real data, owning it may be the wrong default; at minimum the behavior should be documented with the annotation.
| // +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch | ||
| // +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch;create | ||
| // +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch | ||
| // +kubebuilder:rbac:groups="",resources=serviceaccounts,verbs=get;create |
There was a problem hiding this comment.
Deletion drift: ConfigMaps are watched (findMCPServersForConfigMap), so removing an auto-created CM re-triggers reconcile and it comes back. ServiceAccounts have no watch, so a deleted auto-created SA stays missing until an unrelated event hits the MCPServer. If you want symmetric self-healing, Owns(&corev1.ServiceAccount{}) plus list;watch verbs would do it — otherwise fine to leave, but the asymmetry is worth a comment.
|
@matzew — fair point, and after running this in practice I've come around to agreeing with you. When we first hit this with the RHOAI catalog, the manual SA/CM setup felt like a sharp edge worth smoothing. But we've since run the workshop roughly 10 times with proper documentation, and the prerequisite creation hasn't been a real blocker. The operator probably shouldn't be in the business of materializing resources for third-party functionality — that's a catalog/packaging concern. That said, @ArangoGutierrez left a thorough review with valid points (security implications of SA auto-creation, ownership semantics, watch asymmetry). I plan to address those so the diff reflects sound design in case the feature is ever reconsidered. But I'm equally comfortable closing this PR if you'd prefer. Let me know how you'd like to proceed — happy to withdraw or iterate. |
|
@ArangoGutierrez — thank you for the detailed review. Addressing each item: SA security / naming: You're right that auto-creating a named SA can silently activate pre-existing RoleBindings. A convention-derived name like APIReader comment: Will add a comment explaining the uncached read rationale so it survives future refactors. ConfigMap ownership semantics: These are intended as structural placeholders — in the catalog flow, actual configuration is injected separately. The owner reference for GC is intentional, but I agree this needs clear documentation so users don't lose data unexpectedly. SA watch asymmetry: Good catch. At minimum I'll add a comment noting the gap. A full SA watch can be a follow-up if the feature moves forward. I'll also fix the PR body (list/watch/create should be get/create). One note: @matzew raised a broader question about whether this feature belongs in the operator at all. After reflection, I tend to agree — documentation has been sufficient in practice. If the maintainers prefer to close this, I'm comfortable withdrawing. |
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions 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. |
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
Clean, well-scoped implementation: the annotation uses the reserved prefix, resources are owned and created idempotently, prior review feedback on APIReader and error propagation is addressed, and most envtest cases assert real behavior. Holding at request-changes for one make-or-break item plus two supporting ones. The security concern raised earlier is unaddressed: auto-creating a ServiceAccount from a spec-supplied name lets an MCPServer author activate a dormant RoleBinding and run under privileges they don't otherwise hold (no webhook constrains the name); please constrain to a CR-derived name and document the threat model. The head commit's error-propagation fix has no test pinning it — a swallowed error would go unnoticed, so please add the regression case CodeRabbit suggested. Finally the branch needs a rebase: there's a real content conflict in mcpserver_controller.go against main's NetworkPolicy work (role.yaml auto-merges). build/vet/test are all green on the head as-is.
| // ensureServiceAccount creates the ServiceAccount referenced by | ||
| // spec.runtime.security.serviceAccountName if it does not already exist. | ||
| func (r *MCPServerReconciler) ensureServiceAccount(ctx context.Context, server *mcpv1alpha1.MCPServer) error { | ||
| saName := server.Spec.Runtime.Security.ServiceAccountName |
There was a problem hiding this comment.
Security: this auto-creates a SA whose name comes straight from spec.runtime.security.serviceAccountName. Because the operator holds cluster-wide create-serviceaccounts, an MCPServer author who can't create SAs directly can name a SA that a pre-existing RoleBinding already privileges, and the workload then runs with those rights (confined to this namespace). Consider deriving the name from the CR (-sa) or restricting to that convention, and document the threat model.
| } | ||
|
|
||
| sa := &corev1.ServiceAccount{} | ||
| err := r.APIReader.Get(ctx, client.ObjectKey{Name: saName, Namespace: server.Namespace}, sa) |
There was a problem hiding this comment.
Worth a short comment that APIReader is intentional here: SA RBAC is get;create with no watch, so a cached r.Get would try to start an SA informer and fail at runtime. Keeps a future refactor from simplifying it back to r.Get.
| Name: cmName, | ||
| Namespace: server.Namespace, | ||
| }, | ||
| Data: map[string]string{}, |
There was a problem hiding this comment.
The auto-created ConfigMap is empty and owned by the CR. If something populates it later, deleting the MCPServer will GC the populated data and a recreate starts empty. If the CM is meant to outlive the CR once it holds real data, owning it may be the wrong default — at least document this with the annotation.
| NamespacedName: typeNamespacedName, | ||
| }) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) |
There was a problem hiding this comment.
This case only asserts err == nil, so it doesn't actually verify the skip. Add asserts that no ConfigMap was created and no CreatedPrerequisite event fired for the emptyDir entry, otherwise a bug that created a CM here would pass.
| } | ||
| }) | ||
|
|
||
| It("should create a missing ServiceAccount", func() { |
There was a problem hiding this comment.
Please add a case that injects a failing Get/Create and asserts Reconcile returns a non-nil error. The head commit's whole point is returning prerequisite errors so controller-runtime retries, but nothing pins that today — reverting the return to a swallow would keep the suite green.
| // +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch | ||
| // +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch;create | ||
| // +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch | ||
| // +kubebuilder:rbac:groups="",resources=serviceaccounts,verbs=get;create |
There was a problem hiding this comment.
Deletion asymmetry: watched ConfigMaps self-heal if deleted, but an auto-created SA won't (no watch/Owns) until an unrelated event hits the MCPServer. Fine to leave given the APIReader-avoids-informer choice, but worth a one-line comment noting the asymmetry is intentional.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rdwj 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 |
4 participants
Summary
When the annotation
mcp.x-k8s.io/auto-create-prerequisitesis set to"true"on an MCPServer CR, the operator creates missing ServiceAccounts and ConfigMaps before validation. Resources are owned by the MCPServer and cleaned up on deletion.Motivation
Deploying MCP servers from the RHOAI catalog requires manually creating ServiceAccounts and ConfigMaps per namespace before the MCPServer CR will reconcile. This annotation-gated feature automates that step for catalog-based deployments.
Addresses #226
Changes
internal/controller/mcpserver_controller_prerequisites.go—ensurePrerequisites()functioninternal/controller/mcpserver_controller.go— callensurePrerequisites()before validation, updated RBAC markersconfig/rbac/role.yaml— addedlist,watch, andcreateverbs for serviceaccountsinternal/controller/mcpserver_controller_prerequisites_test.go— 7 test cases covering annotation gating, resource creation, idempotency, and owner referencesTesting
Validated on a live RHOAI 3.4 / OpenShift 4.20 cluster:
make test)make lint)Summary by CodeRabbit
New Features
mcp.x-k8s.io/auto-create-prerequisitesannotation.Chores