feat: provision dual RO/RW service accounts for MCP services#304
feat: provision dual RO/RW service accounts for MCP services#304rshoemaker merged 2 commits intomainfrom
Conversation
Service users now use built-in group roles (pgedge_application, pgedge_application_read_only) instead of custom grants. MCP config.yaml credential selection is driven by allow_writes.
📝 WalkthroughWalkthroughMode-aware database service users introduced: usernames now include a mode suffix and generation accepts a mode parameter. Orchestration and MCP config now track separate RO/RW user-role resources and credentials; user-role creation uses group-role membership instead of explicit GRANT statements. Changes
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/internal/orchestrator/swarm/mcp_config_resource.go (1)
95-98:⚠️ Potential issue | 🟠 MajorKeep
Refresh()limited to observingconfig.yaml.Hydrating RO/RW credentials here makes the refreshed state reflect
ServiceUserRole, not the file on disk. If a role gets recreated with a new password, this can make the resource look current without provingconfig.yamlwas rewritten.Suggested change
- // Populate credentials from ServiceUserRole - if err := r.populateCredentials(rc); err != nil { - return err - } - // Check if config.yaml exists _, err = readResourceFile(fs, filepath.Join(dirPath, "config.yaml"))Based on learnings: Refresh should only verify the existence of
config.yaml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/swarm/mcp_config_resource.go` around lines 95 - 98, The Refresh() implementation is hydrating credentials by calling r.populateCredentials(rc), causing the refreshed state to reflect ServiceUserRole instead of the on-disk config.yaml; remove that call so Refresh() only verifies the presence and timestamp/contents of config.yaml (e.g., check file existence/read and update observed state accordingly) rather than populating RO/RW credentials from ServiceUserRole. Locate the Refresh method and remove or disable the r.populateCredentials(rc) invocation, ensuring any credential population remains only in reconcile/create/update paths, and keep Refresh limited to observing config.yaml only.server/internal/orchestrator/swarm/service_user_role.go (1)
136-143:⚠️ Potential issue | 🟠 MajorProvision the role on every node, not only on the current primary.
This still routes both RO and RW account creation through
connectToPrimary(). In a multi-node database, failover or host selection can land MCP on a node that never received theCREATE ROLE, which turns authentication failures into a topology event instead of a config bug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/swarm/service_user_role.go` around lines 136 - 143, createUserRole currently opens a single connection via connectToPrimary which causes the CREATE ROLE to only run on the primary; instead enumerate all cluster nodes and run the role creation on each node so the role exists everywhere. Replace the single connectToPrimary(...) call in ServiceUserRole.createUserRole with a loop over the cluster nodes (e.g., from rc or a node list on ServiceUserRole), use the per-node connector (e.g., r.connectToNode or an equivalent helper) to connect to each node with r.DatabaseName, execute the same role creation logic for each connection, Close each connection, and aggregate/return errors (return the first error or a combined error) so failures creating the role on any node are surfaced. Ensure you keep the original behavior when there is only one node.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@server/internal/orchestrator/swarm/mcp_config_resource.go`:
- Around line 95-98: The Refresh() implementation is hydrating credentials by
calling r.populateCredentials(rc), causing the refreshed state to reflect
ServiceUserRole instead of the on-disk config.yaml; remove that call so
Refresh() only verifies the presence and timestamp/contents of config.yaml
(e.g., check file existence/read and update observed state accordingly) rather
than populating RO/RW credentials from ServiceUserRole. Locate the Refresh
method and remove or disable the r.populateCredentials(rc) invocation, ensuring
any credential population remains only in reconcile/create/update paths, and
keep Refresh limited to observing config.yaml only.
In `@server/internal/orchestrator/swarm/service_user_role.go`:
- Around line 136-143: createUserRole currently opens a single connection via
connectToPrimary which causes the CREATE ROLE to only run on the primary;
instead enumerate all cluster nodes and run the role creation on each node so
the role exists everywhere. Replace the single connectToPrimary(...) call in
ServiceUserRole.createUserRole with a loop over the cluster nodes (e.g., from rc
or a node list on ServiceUserRole), use the per-node connector (e.g.,
r.connectToNode or an equivalent helper) to connect to each node with
r.DatabaseName, execute the same role creation logic for each connection, Close
each connection, and aggregate/return errors (return the first error or a
combined error) so failures creating the role on any node are surfaced. Ensure
you keep the original behavior when there is only one node.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c30c0de2-1912-4775-b043-bec2faa2ffae
📒 Files selected for processing (7)
server/internal/database/service_instance.goserver/internal/database/service_instance_test.goserver/internal/orchestrator/swarm/mcp_config_resource.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/service_instance.goserver/internal/orchestrator/swarm/service_instance_spec.goserver/internal/orchestrator/swarm/service_user_role.go
|
@coderabbitai I fixed the first issue. The second issue will be fixed in a different PR. |
|
Tip For best results, initiate chat on the files or code changes.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/internal/orchestrator/swarm/mcp_config_resource.go (1)
54-60: Update theDiffIgnorecomment to match the new lifecycle.The code now populates RO/RW credentials in
Create/Update, notRefresh, so this comment is misleading.✏️ Suggested diff
- // Credentials are populated from ServiceUserRole resources during refresh. + // Credentials are populated from ServiceUserRole resources during Create/Update.Based on learnings: "Refresh should only verify the existence of config.yaml. Tokens.yaml and users.yaml are owned and managed by the MCP server at runtime and should not be rewritten on Update."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/swarm/mcp_config_resource.go` around lines 54 - 60, Update the misleading comment in the MCPConfigResource.DiffIgnore function: change the line that currently reads "Credentials are populated from ServiceUserRole resources during refresh." to accurately state that RO/RW credentials are populated during Create/Update (not Refresh), and note that tokens.yaml and users.yaml are owned/managed by the MCP server at runtime and should not be rewritten on Update; modify the comment inside the DiffIgnore method (MCPConfigResource.DiffIgnore) accordingly so it reflects the new lifecycle and ownership.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/internal/orchestrator/swarm/mcp_config_resource.go`:
- Around line 54-60: Update the misleading comment in the
MCPConfigResource.DiffIgnore function: change the line that currently reads
"Credentials are populated from ServiceUserRole resources during refresh." to
accurately state that RO/RW credentials are populated during Create/Update (not
Refresh), and note that tokens.yaml and users.yaml are owned/managed by the MCP
server at runtime and should not be rewritten on Update; modify the comment
inside the DiffIgnore method (MCPConfigResource.DiffIgnore) accordingly so it
reflects the new lifecycle and ownership.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d16c217-2e68-483f-81fa-eed10b0edc95
📒 Files selected for processing (1)
server/internal/orchestrator/swarm/mcp_config_resource.go
mmols
left a comment
mmols
left a comment
There was a problem hiding this comment.
Worked well for me with a single node database. I was able to switch back and forth successfully by restarting the MCP container.
2 participants
Summary
Changes
Known issue
Service accounts are currently only created on the primary node and not propagated to other nodes. This will be addressed in a follow-up ticket by creating accounts on each node independently rather than relying on Spock DDL
replication.
Test plan
PLAT-461