feat: add connect_as field for service database credentials#338
Open
rshoemaker wants to merge 3 commits intomainfrom
Open
feat: add connect_as field for service database credentials#338rshoemaker wants to merge 3 commits intomainfrom
rshoemaker wants to merge 3 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (9)
📒 Files selected for processing (16)
💤 Files with no reviewable changes (4)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds a new 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)
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 |
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
🟢 Metrics 5 complexity · -2 duplication
Metric Results Complexity 5 Duplication -2
TIP This summary will be updated as you push new changes. Give us feedback
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/internal/orchestrator/swarm/mcp_config_resource.go`:
- Around line 55-60: Remove the connect_as credential paths from
MCPConfigResource.DiffIgnore so changes to "/connect_as_username" and
"/connect_as_password" will be detected and trigger Update; additionally, adjust
Refresh logic (related to MCPConfigResource.Refresh) to only check for the
existence of "config.yaml" and avoid rewriting runtime-owned files
("tokens.yaml" and "users.yaml") so those files remain runtime-managed while
config.yaml presence is used as the sole refresh existence check.
In `@server/internal/orchestrator/swarm/service_instance_spec.go`:
- Around line 93-96: The MCP branch in populateCredentials returns early without
clearing existing credentials, which can leave stale secrets in s.Credentials;
modify populateCredentials so that when s.ServiceSpec.ServiceType == "mcp" you
explicitly reset/clear s.Credentials (e.g., set to nil or an empty credentials
struct) before returning, ensuring old credentials are not carried forward.
In `@server/internal/resource/migrations/2_0_0.go`:
- Around line 19-35: The migration currently deletes all swarm.service_user_role
entries; instead restrict deletions to MCP-owned role state only: in
Version_2_0_0.Run, when iterating state.Resources for serviceUserRoleType and
when filtering data.Dependencies, check each resource's ownership/metadata
(e.g., an owner/managedBy field on the resource or data.Metadata) and only
remove resources and dependency edges where that ownership indicates MCP (e.g.,
managedBy == "mcp"); leave non-MCP service_user_role resources and their
dependency edges untouched so ServiceUserRole resolution in ServiceInstanceSpec
and rag_config_resource continues to work.
In `@server/internal/workflows/plan_update.go`:
- Around line 126-138: The planner currently copies the referenced user's
ConnectAsPassword from spec.DatabaseUsers into the service without validating
it; if the referenced database.User has an empty password the plan will succeed
but service DB auth will fail. Update the lookup block that handles
serviceSpec.ConnectAs in plan_update.go (where connectAsUser is resolved from
spec.DatabaseUsers) to reject empty secrets: after finding database.User (type
database.User) check that its Password (or ConnectAsPassword field used when
assigning to the service) is non-empty and return an error like "connect_as user
%q has empty password" if empty; apply the same validation for the additional
occurrence noted around lines 154-156 so the planner never copies an empty
ConnectAsPassword into the service spec.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab39b018-ec33-43a9-9704-06e8bac3b037
⛔ Files ignored due to path filters (9)
api/apiv1/gen/control_plane/service.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/encode_decode.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/types.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/server/encode_decode.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/server/types.gois excluded by!**/gen/**api/apiv1/gen/http/openapi.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi.yamlis excluded by!**/gen/**api/apiv1/gen/http/openapi3.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi3.yamlis excluded by!**/gen/**
📒 Files selected for processing (26)
api/apiv1/design/database.goe2e/service_provisioning_test.goserver/internal/api/apiv1/convert.goserver/internal/api/apiv1/validate.goserver/internal/api/apiv1/validate_test.goserver/internal/database/operations/golden_test/TestUpdateDatabase/add_service_to_existing_database.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/remove_service_from_existing_database.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/single_node_with_service_from_empty.jsonserver/internal/database/operations/helpers_test.goserver/internal/database/service_instance.goserver/internal/database/spec.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/resource/migrations/2_0_0.goserver/internal/resource/migrations/2_0_0_test.goserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/empty.jsonserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/no_nodes.jsonserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/populate_n3_with_n1_source.jsonserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/single_node_with_replicas.jsonserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/three_nodes.jsonserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/with_restore_config.jsonserver/internal/resource/migrations/provide.goserver/internal/resource/state.goserver/internal/workflows/plan_update.go
💤 Files with no reviewable changes (4)
- server/internal/database/operations/golden_test/TestUpdateDatabase/remove_service_from_existing_database.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/add_service_to_existing_database.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/single_node_with_service_from_empty.json
- server/internal/database/operations/helpers_test.go
Services now specify which database_users entry to connect as via the new required connect_as field on ServiceSpec, replacing the auto-created svc_*_ro and svc_*_rw service accounts for MCP. - Add connect_as to Goa DSL, domain model, and API convert layer - Validate connect_as references an existing database_users entry - Validate allow_writes requires db_owner on the connect_as user - Wire connect_as credentials through ServiceInstanceSpec into MCPConfigResource, bypassing ServiceUserRole for MCP - Disconnect ServiceUserRole creation for MCP services (PostgREST and RAG still use it until they adopt connect_as) - Add state migration (v2.0.0) to remove swarm.service_user_role resources and dependency references from existing deployments - Update E2E test fixtures with connect_as
…nore so credential changes trigger an Update
rshoemaker
force-pushed
the
feat/PLAT-539/service_connect_as
branch
from
1f90726 to
81e9b63
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
database_usersentry to connect as via the new requiredconnect_asfield onServiceSpec, replacing the auto-createdsvc_*_ro/svc_*_rwservice accounts for MCPconnect_asreferences an existingdatabase_usersentry, and thatallow_writes: truerequiresdb_owner: trueServiceUserRolestateServiceUserRolecreation is disconnected for MCP (PostgREST/RAG still use it until they adoptconnect_as)swarm.service_user_roleresources and dependency references from existing deploymentsconnect_asTest plan