Skip to content

feat: migrate PostgREST to connect_as credential model#352

Merged
rshoemaker merged 2 commits intomainfrom
feat/PLAT-553/PostgREST-switch-to-connect_as-credentials
Apr 20, 2026
Merged

feat: migrate PostgREST to connect_as credential model#352
rshoemaker merged 2 commits intomainfrom
feat/PLAT-553/PostgREST-switch-to-connect_as-credentials

Conversation

@moizpgedge
Copy link
Copy Markdown
Contributor

@moizpgedge moizpgedge commented Apr 17, 2026

Replace auto-created svc_{service_id}_ro/rw service user roles for PostgREST with the connect_as field — a reference to a database_users entry that the customer owns and manages. This aligns PostgREST with the credential model already used by MCP.

  • validateConnectAs was not being called for postgrest service type — fixed: all three service types (mcp, postgrest, rag) now validate connect_as at the API layer.

  • Added noDefaultConnectAs flag in validate tests to cover the missing-connect_as and unknown-user error paths for PostgREST.

  • PostgRESTAuthenticatorResource: removed UserRoleID field, added ConnectAsUsername; Dependencies() now points to PostgresDatabaseResource instead of ServiceUserRole; SQL targets the connect_as user directly instead of a generated username.

  • PostgRESTConfigResource: receives ConnectAsUsername / ConnectAsPassword at construction time from the connect_as database user instead of reading from ServiceUserRole.

  • generateMCPInstanceResources(): ServiceUserRole generation is now skipped for both mcp and postgrest; only rag still creates svc_* roles. Per-node PostgRESTAuthenticatorResource instances carry ConnectAsUsername directly.

  • ServiceInstanceResource.Dependencies(): removed stale PostgREST ServiceUserRole deps that blocked container start on the wrong resource.

  • ServiceInstanceSpecResource.Dependencies(): removed PostgREST ServiceUserRole deps; added PostgRESTPreflightResourceIdentifier — this ensures the container does not start until the database exists, fixing a race condition where PostgREST failed with "database does not exist" when Patroni had not finished bootstrapping.

  • ServiceInstanceSpecResource.populateCredentials(): simplified — all current service types use config-file credentials, so s.Credentials is always nil.

    only 5xx responses use Error. Eliminates spurious ERR flood from 409 conflict responses during cluster initialization.

  • docs/services/index.md: replaced svc_*_ro/rw credential section with connect_as model description.

  • docs/services/managing.md: updated service_id field description, added connect_as row to spec table, added database_users + connect_as to PostgREST example, updated removal warning.

  • docs/services/postgrest.md: updated overview, all three examples now include database_users and connect_as: "app", updated JWT role-claim note to reference connect_as user.

  • e2e/postgrest_test.go: renamed TestPostgRESTServiceUserRolesTestPostgRESTAuthenticatorRole; assertions now verify NOINHERIT and anon-role grant on the connect_as user (admin) instead of svc_* roles; TestPostgRESTRemove verifies connect_as user is NOT dropped; TestPostgRESTMultiHtDBURI checks authenticator setup instead of svc_* role existence; all specs now include ConnectAs: "admin".

  • postgrest_service_config_test.go: updated fixture username from svc_pgrest to myapp to reflect the connect_as model.

  • New: postgrest_preflight_resource_test.go, postgrest_authenticator_resource_test.go, postgrest_config_resource_test.go — unit tests for all three PostgREST-specific resource types (37 tests).

PLAT-553

Summary

Migrate PostgREST from auto-created svc_{service_id}_ro/rw service
user roles to the connect_as credential model — a reference to a
database_users entry that the customer owns and manages. This aligns
PostgREST with the model already used by MCP and fixes a container
start race condition against Patroni bootstrap.

Changes

Validation

  • validateConnectAs was not called for postgrest service type —
    fixed; all three service types (mcp, postgrest, rag) now validate
    connect_as at the API layer
  • Added noDefaultConnectAs flag in validate tests to cover missing
    connect_as and unknown-user error paths for PostgREST

Orchestrator

  • PostgRESTAuthenticatorResource: removed UserRoleID field, added
    ConnectAsUsername; Dependencies() now points to
    PostgresDatabaseResource instead of ServiceUserRole; SQL targets
    the connect_as user directly
  • PostgRESTConfigResource: receives ConnectAsUsername /
    ConnectAsPassword at construction time from the connect_as
    database user instead of reading from ServiceUserRole
  • generateMCPInstanceResources(): ServiceUserRole generation
    skipped for both mcp and postgrest; only rag still creates
    svc_* roles; per-node PostgRESTAuthenticatorResource instances
    now carry ConnectAsUsername directly

Resource dependencies

  • ServiceInstanceResource.Dependencies(): removed stale PostgREST
    ServiceUserRole deps that were blocking container start on the
    wrong resource
  • ServiceInstanceSpecResource.Dependencies(): removed PostgREST
    ServiceUserRole deps; added PostgRESTPreflightResourceIdentifier
    so the container does not start until the database exists — fixes a
    race condition where PostgREST failed with "database does not exist"
    when Patroni had not finished bootstrapping
  • ServiceInstanceSpecResource.populateCredentials(): simplified —
    all current service types use config-file credentials, so
    s.Credentials is always nil

HTTP middleware

  • 4xx responses now logged at Warn instead of Error; only 5xx
    uses Error — eliminates spurious ERR flood from 409 conflict
    responses during cluster initialization

Documentation

  • docs/services/index.md: replaced svc_*_ro/rw credential section
    with connect_as model description
  • docs/services/managing.md: updated service_id field description,
    added connect_as row to spec table, added database_users +
    connect_as to PostgREST example, updated removal warning to clarify
    the connect_as user is never dropped
  • docs/services/postgrest.md: updated overview prose, all three
    examples now include database_users and connect_as: "app",
    updated JWT role-claim note to reference connect_as user

Tests

  • e2e/postgrest_test.go: renamed TestPostgRESTServiceUserRoles
    TestPostgRESTAuthenticatorRole; assertions now verify NOINHERIT and
    anon-role grant on the connect_as user (admin) instead of
    svc_* roles; TestPostgRESTRemove verifies connect_as user is
    NOT dropped after removal; TestPostgRESTMultiHostDBURI checks
    authenticator setup instead of svc_* role count; all specs now
    include ConnectAs: "admin"
  • postgrest_service_config_test.go: updated fixture username from
    svc_pgrest to myapp to reflect the connect_as model
  • New: postgrest_preflight_resource_test.go,
    postgrest_authenticator_resource_test.go,
    postgrest_config_resource_test.go — unit tests for all three
    PostgREST-specific resource types (37 new tests)
  • ...

Testing

# Unit tests
go test ./server/internal/orchestrator/swarm/... \
        ./server/internal/api/apiv1/... \
        ./server/internal/database/...
# Run all PostgREST E2E tests at once
make test-e2e E2E_FIXTURE=lima E2E_RUN=TestPostgREST
# Or individually
make test-e2e E2E_FIXTURE=lima E2E_RUN=TestProvisionPostgREST
make test-e2e E2E_FIXTURE=lima E2E_RUN=TestProvisionPostgRESTWithJWT
make test-e2e E2E_FIXTURE=lima E2E_RUN=TestPostgRESTAuthenticatorRole
make test-e2e E2E_FIXTURE=lima E2E_RUN=TestPostgRESTRemove
make test-e2e E2E_FIXTURE=lima E2E_RUN=TestPostgRESTConfigUpdate
make test-e2e E2E_FIXTURE=lima E2E_RUN=TestPostgRESTPreflight_MissingSchema
make test-e2e E2E_FIXTURE=lima E2E_RUN=TestPostgRESTPreflight_MissingAnonRole
make test-e2e E2E_FIXTURE=lima E2E_RUN=TestPostgRESTMultiHostDBURI
make test-e2e E2E_FIXTURE=lima E2E_RUN=TestPostgRESTFailover

Checklist

  • Tests added or updated (unit and/or e2e, as needed)
  • Documentation updated (if needed)
  • Issue is linked (branch name or URL in PR description)
  • Changelog entry added for user-facing behavior changes
  • Breaking changes (if any) are clearly called out in the PR description

Replace auto-created `svc_{service_id}_ro/rw` service user roles for
PostgREST with the `connect_as` field — a reference to a
`database_users` entry that the customer owns and manages. This aligns
PostgREST with the credential model already used by MCP.

- `validateConnectAs` was not being called for `postgrest` service type
  — fixed: all three service types (mcp, postgrest, rag) now validate
  `connect_as` at the API layer.
- Added `noDefaultConnectAs` flag in validate tests to cover the
  missing-`connect_as` and unknown-user error paths for PostgREST.

- `PostgRESTAuthenticatorResource`: removed `UserRoleID` field, added
  `ConnectAsUsername`; `Dependencies()` now points to
  `PostgresDatabaseResource` instead of `ServiceUserRole`; SQL targets
  the `connect_as` user directly instead of a generated username.
- `PostgRESTConfigResource`: receives `ConnectAsUsername` /
  `ConnectAsPassword` at construction time from the connect_as
  database user instead of reading from `ServiceUserRole`.
- `generateMCPInstanceResources()`: `ServiceUserRole` generation is
  now skipped for both `mcp` and `postgrest`; only `rag` still creates
  `svc_*` roles. Per-node `PostgRESTAuthenticatorResource` instances
  carry `ConnectAsUsername` directly.

- `ServiceInstanceResource.Dependencies()`: removed stale PostgREST
  `ServiceUserRole` deps that blocked container start on the wrong
  resource.
- `ServiceInstanceSpecResource.Dependencies()`: removed PostgREST
  `ServiceUserRole` deps; added `PostgRESTPreflightResourceIdentifier`
  — this ensures the container does not start until the database exists,
  fixing a race condition where PostgREST failed with "database does
  not exist" when Patroni had not finished bootstrapping.
- `ServiceInstanceSpecResource.populateCredentials()`: simplified —
  all current service types use config-file credentials, so
  `s.Credentials` is always nil.

  only 5xx responses use `Error`. Eliminates spurious ERR flood from
  409 conflict responses during cluster initialization.

- `docs/services/index.md`: replaced `svc_*_ro/rw` credential section
  with `connect_as` model description.
- `docs/services/managing.md`: updated `service_id` field description,
  added `connect_as` row to spec table, added `database_users` +
  `connect_as` to PostgREST example, updated removal warning.
- `docs/services/postgrest.md`: updated overview, all three examples
  now include `database_users` and `connect_as: "app"`, updated JWT
  role-claim note to reference `connect_as` user.

- `e2e/postgrest_test.go`: renamed `TestPostgRESTServiceUserRoles` →
  `TestPostgRESTAuthenticatorRole`; assertions now verify NOINHERIT and
  anon-role grant on the `connect_as` user (`admin`) instead of `svc_*`
  roles; `TestPostgRESTRemove` verifies `connect_as` user is NOT dropped;
  `TestPostgRESTMultiHtDBURI` checks authenticator setup instead of
  `svc_*` role existence; all specs now include `ConnectAs: "admin"`.
- `postgrest_service_config_test.go`: updated fixture username from
  `svc_pgrest` to `myapp` to reflect the connect_as model.
- New: `postgrest_preflight_resource_test.go`,
  `postgrest_authenticator_resource_test.go`,
  `postgrest_config_resource_test.go` — unit tests for all three
  PostgREST-specific resource types (37 tests).

PLAT-553
@moizpgedge moizpgedge marked this pull request as draft April 17, 2026 21:47
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 17, 2026

📝 Walkthrough

Walkthrough

Services (notably PostgREST) now require a customer-managed connect_as username from database_users; the control plane uses that user for connection strings and role grants. Service deletion no longer drops the referenced database user; PostgREST no longer receives auto-provisioned per-service users.

Changes

Cohort / File(s) Summary
Service Credential Model Documentation
docs/services/index.md, docs/services/managing.md, docs/services/postgrest.md
Docs updated: introduce required connect_as referencing database_users; examples updated to include database_users entries; removal of language that services auto-provision per-service DB users and that service removal drops DB users.
API Validation & Tests
server/internal/api/apiv1/validate.go, server/internal/api/apiv1/validate_test.go
Validation now enforces connect_as for postgrest and checks it matches a database_users entry. Tests updated/expanded to cover missing and invalid connect_as cases and adjusted defaulting logic in tests.
Orchestrator: resource wiring & service spec
server/internal/orchestrator/swarm/orchestrator.go, server/internal/orchestrator/swarm/service_instance.go, server/internal/orchestrator/swarm/service_instance_spec.go, server/internal/orchestrator/swarm/service_spec_test.go
Removed PostgREST from per-service ServiceUserRole generation/dependencies; added preflight dependency for PostgREST; credentials population simplified to nil for container specs; container test fixtures switched to use ConnectAs and nil Credentials.
PostgREST Authenticator Resource & Tests
server/internal/orchestrator/swarm/postgrest_authenticator_resource.go, server/internal/orchestrator/swarm/postgrest_authenticator_resource_test.go
Replaced UserRoleID with ConnectAsUsername; dependencies now include the Postgres DB resource; authenticator username derived from connect_as; revocation now targets previously-applied anon role only; Delete no longer revokes DB CONNECT. Tests added for identifiers, dependencies, desired anon role, and username behavior.
PostgREST Config Resource & Tests
server/internal/orchestrator/swarm/postgrest_config_resource.go, server/internal/orchestrator/swarm/postgrest_config_resource_test.go
Config resource fields renamed to ConnectAsUsername/ConnectAsPassword; credential population removed (set at construction); removed dependency on ServiceUserRole; config generation now embeds connect_as credentials. Extensive tests added for config writing, JWT, and multi-host URIs.
Preflight & Preflight Tests
server/internal/orchestrator/swarm/postgrest_preflight_resource_test.go
Added tests for PostgRESTPreflightResource (identifiers, dependencies, executor) and splitSchemas helper.
End-to-End & DB Config Tests
e2e/postgrest_test.go, server/internal/database/postgrest_service_config_test.go
E2E tests updated to assert behavior for connect_as user (e.g., admin existence, role memberships, absence of svc_* roles); renamed test to TestPostgRESTAuthenticatorRole. PostgREST config tests updated to expect username myapp.
HTTP Middleware Logging
server/internal/api/middleware.go
Logging level mapping changed: 4xx responses now log at Warn, 5xx at Error; /v1/version remains Debug.

Poem

🐇 I nibble lines of YAML and sing,
"connect_as" now gives services their spring.
No auto svc_ hats on my head,
Customers keep users instead.
Hops of joy — credentials reclaim the ring!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.28% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: migrating PostgREST to use a connect_as credential model instead of auto-created service user roles.
Description check ✅ Passed The PR description is comprehensive with sections covering Summary, Changes (organized by category), Testing, Checklist, and linked issue. All required template sections are present and well-populated with details.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/PLAT-553/PostgREST-switch-to-connect_as-credentials

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codacy-production
Copy link
Copy Markdown

codacy-production Bot commented Apr 17, 2026

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics -2 duplication

Metric Results
Duplication -2

View in Codacy

TIP This summary will be updated as you push new changes. Give us feedback

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
server/internal/orchestrator/swarm/service_instance.go (1)

38-38: ⚠️ Potential issue | 🟡 Minor

Stale comment on ConnectAsUsername.

The comment says the field is populated only for RAG, but Refresh() at line 84 sets s.ConnectAsUsername = desired.ServiceSpec.ConnectAs unconditionally, and this PR extends connect_as to mcp and postgrest. Consider updating to reflect that it mirrors ServiceSpec.ConnectAs for all service types that populate it.

📝 Suggested comment update
-	ConnectAsUsername string `json:"connect_as_username"` // Non-empty when RAG uses connect_as credentials
+	ConnectAsUsername string `json:"connect_as_username"` // Mirrors ServiceSpec.ConnectAs (set by Refresh)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server/internal/orchestrator/swarm/service_instance.go` at line 38, The
struct field ConnectAsUsername has a stale comment limiting it to RAG, but
Refresh() sets s.ConnectAsUsername = desired.ServiceSpec.ConnectAs
unconditionally and the PR extends connect_as usage to mcp and postgrest; update
the comment on ConnectAsUsername in service_instance.go to state that it mirrors
ServiceSpec.ConnectAs for any service type that populates connect_as (e.g., RAG,
MCP, PostgREST) and remove the RAG-only wording, and mention Refresh() as the
place that copies the value.
docs/services/managing.md (1)

160-172: ⚠️ Potential issue | 🟡 Minor

Contradictory wording in "Removing a Service".

Line 164 still states the Control Plane "revokes its database credentials" when a service is removed, but the warning on lines 170-172 (and the new model in index.md) explicitly say the connect_as user is customer-managed and is not dropped. These two statements contradict each other. Under the new model, removing a service only deletes service instances/containers — it does not modify the database_users entry.

📝 Proposed fix
 To remove a service, submit an update request that omits the service
 from the `services` array. The Control Plane stops and deletes all
-service instances for that service and revokes its database credentials.
+service instances for that service. The `connect_as` database user is
+not modified — it remains in `database_users` and must be removed
+separately if no longer needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/services/managing.md` around lines 160 - 172, The "Removing a Service"
section contradicts the new model: it currently says the Control Plane "revokes
its database credentials" but elsewhere (and in index.md) you state the
`connect_as` user is customer-managed and not dropped; update the prose in the
"Removing a Service" paragraph to remove or replace "revokes its database
credentials" with a clear statement that removing a service only stops and
deletes service instances and their data directories and does not modify the
`database_users` entry or drop the `connect_as` user (mention `connect_as` and
`database_users` by name to be explicit).
server/internal/orchestrator/swarm/orchestrator.go (1)

412-416: ⚠️ Potential issue | 🟠 Major

Restore RAG ServiceUserRole generation on the live RAG path.

rag is dispatched to generateRAGInstanceResources, so the ServiceType == "rag" block in generateMCPInstanceResources never runs. As written, RAG no longer emits the svc role resources the PR says should still exist.

Suggested direction

Move the RAG role resources into generateRAGInstanceResources before serializing the resource list, for example:

 func (o *Orchestrator) generateRAGInstanceResources(spec *database.ServiceInstanceSpec) (*database.ServiceInstanceResources, error) {
     ...
-    orchestratorResources := []resource.Resource{databaseNetwork}
+    canonicalROID := ServiceUserRoleIdentifier(spec.ServiceSpec.ServiceID, ServiceUserRoleRO)
+    orchestratorResources := []resource.Resource{
+        databaseNetwork,
+        &ServiceUserRole{
+            ServiceID:    spec.ServiceSpec.ServiceID,
+            DatabaseID:   spec.DatabaseID,
+            DatabaseName: spec.DatabaseName,
+            NodeName:     spec.NodeName,
+            Mode:         ServiceUserRoleRO,
+        },
+    }
+
+    for _, nodeInst := range spec.DatabaseNodes {
+        if nodeInst.NodeName == spec.NodeName {
+            continue
+        }
+        orchestratorResources = append(orchestratorResources, &ServiceUserRole{
+            ServiceID:        spec.ServiceSpec.ServiceID,
+            DatabaseID:       spec.DatabaseID,
+            DatabaseName:     spec.DatabaseName,
+            NodeName:         nodeInst.NodeName,
+            Mode:             ServiceUserRoleRO,
+            CredentialSource: &canonicalROID,
+        })
+    }

If RAG still requires both RO and RW roles, mirror the previous two-role pattern instead.

Also applies to: 448-453, 663-782

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server/internal/orchestrator/swarm/orchestrator.go` around lines 412 - 416,
The RAG ServiceUserRole creation was removed from the live RAG path because
"rag" now routes to generateRAGInstanceResources; restore the svc role
generation by moving the RAG-specific ServiceUserRole logic out of
generateMCPInstanceResources and into generateRAGInstanceResources (add the
role(s) before the final resource list serialization in
generateRAGInstanceResources). Locate the RAG-related role creation code
patterns in generateMCPInstanceResources (the previous "rag" branch that created
ServiceUserRole(s)) and replicate or mirror the two-role (RO/RW) pattern inside
generateRAGInstanceResources so RAG still emits the expected svc role resources;
ensure you update any helper names used for role creation and keep the roles
appended to the same resources slice just prior to returning from
generateRAGInstanceResources.
docs/services/postgrest.md (1)

217-232: ⚠️ Potential issue | 🟡 Minor

Align the JWT example with the grant requirement.

The token claims role: "app", but the surrounding text says JWT roles must be granted to the connect_as user; this example never creates or grants a separate app role. Use a role the example guarantees is granted, or add explicit role/grant setup.

Suggested documentation adjustment
-    payload = b64url(json.dumps({"role":"app","exp":int(time.time())+3600}))
+    payload = b64url(json.dumps({"role":"pgedge_application_read_only","exp":int(time.time())+3600}))
...
-    curl -X POST http://host-1:3100/products \
-        -H "Content-Type: application/json" \
-        -H "Authorization: Bearer $TOKEN" \
-        --data '{"name": "Widget", "price": 9.99}'
+    curl http://host-1:3100/products \
+        -H "Authorization: Bearer $TOKEN"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/services/postgrest.md` around lines 217 - 232, The JWT example sets
payload with role:"app" but the docs require that the role be granted to the
connect_as user; either change the payload in the sample (the payload variable
used to build TOKEN) to a role that the example actually grants, or add the
explicit SQL grant statements for creating/granting the "app" role to the
connect_as user before the curl example; update references to payload/header/sig
and the TOKEN usage so the example role and the connect_as grant are consistent.
🧹 Nitpick comments (1)
e2e/postgrest_test.go (1)

31-31: Use a non-superuser connect_as fixture.

ConnectAs: "admin" points PostgREST at a SUPERUSER, which can mask missing grants and least-privilege failures in the new authenticator path. Keep admin for test inspection if needed, but add a separate LOGIN user for PostgREST and update role assertions to target that user.

Also applies to: 51-57

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@e2e/postgrest_test.go` at line 31, Replace the PostgREST fixture's superuser
connect role by creating and using a dedicated non-superuser login role for
PostgREST: change the ConnectAs value from "admin" to the new login user (e.g.,
"postgrest_user"), add SQL to the test setup to CREATE ROLE postgrest_user LOGIN
with only the minimal grants required, keep the existing "admin" role available
for inspection only, and update any role assertions (currently asserting
behavior for the admin connection) to assert against the new postgrest_user
connection and its privileges (symbols: ConnectAs, the test setup/fixture that
creates roles, and the assertions that check grants/roles).
🤖 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/postgrest_authenticator_resource.go`:
- Around line 76-77: The code currently uses ConnectAsUsername in
authenticatorUsername(), which causes revokeStaleAnonRoles and Delete to revoke
grants from a customer-managed connect_as role; change the logic so these
cleanup routines only target grants that this resource created: stop using
ConnectAsUsername as the identifier for revocation and instead either (A) return
the resource-managed authenticator identifier (e.g. a dedicated field like
ManagedAuthenticatorUsername or the original anonymous role name used when
creating grants) from authenticatorUsername(), or (B) add and consult a tracked
state field (e.g. ManagedGrantID/CreatedByThisResource boolean) so
revokeStaleAnonRoles and Delete only revoke the exact grant recorded in state;
update authenticatorUsername(), revokeStaleAnonRoles, and Delete to reference
that managed identifier/state check and never revoke roles based solely on
ConnectAsUsername.

---

Outside diff comments:
In `@docs/services/managing.md`:
- Around line 160-172: The "Removing a Service" section contradicts the new
model: it currently says the Control Plane "revokes its database credentials"
but elsewhere (and in index.md) you state the `connect_as` user is
customer-managed and not dropped; update the prose in the "Removing a Service"
paragraph to remove or replace "revokes its database credentials" with a clear
statement that removing a service only stops and deletes service instances and
their data directories and does not modify the `database_users` entry or drop
the `connect_as` user (mention `connect_as` and `database_users` by name to be
explicit).

In `@docs/services/postgrest.md`:
- Around line 217-232: The JWT example sets payload with role:"app" but the docs
require that the role be granted to the connect_as user; either change the
payload in the sample (the payload variable used to build TOKEN) to a role that
the example actually grants, or add the explicit SQL grant statements for
creating/granting the "app" role to the connect_as user before the curl example;
update references to payload/header/sig and the TOKEN usage so the example role
and the connect_as grant are consistent.

In `@server/internal/orchestrator/swarm/orchestrator.go`:
- Around line 412-416: The RAG ServiceUserRole creation was removed from the
live RAG path because "rag" now routes to generateRAGInstanceResources; restore
the svc role generation by moving the RAG-specific ServiceUserRole logic out of
generateMCPInstanceResources and into generateRAGInstanceResources (add the
role(s) before the final resource list serialization in
generateRAGInstanceResources). Locate the RAG-related role creation code
patterns in generateMCPInstanceResources (the previous "rag" branch that created
ServiceUserRole(s)) and replicate or mirror the two-role (RO/RW) pattern inside
generateRAGInstanceResources so RAG still emits the expected svc role resources;
ensure you update any helper names used for role creation and keep the roles
appended to the same resources slice just prior to returning from
generateRAGInstanceResources.

In `@server/internal/orchestrator/swarm/service_instance.go`:
- Line 38: The struct field ConnectAsUsername has a stale comment limiting it to
RAG, but Refresh() sets s.ConnectAsUsername = desired.ServiceSpec.ConnectAs
unconditionally and the PR extends connect_as usage to mcp and postgrest; update
the comment on ConnectAsUsername in service_instance.go to state that it mirrors
ServiceSpec.ConnectAs for any service type that populates connect_as (e.g., RAG,
MCP, PostgREST) and remove the RAG-only wording, and mention Refresh() as the
place that copies the value.

---

Nitpick comments:
In `@e2e/postgrest_test.go`:
- Line 31: Replace the PostgREST fixture's superuser connect role by creating
and using a dedicated non-superuser login role for PostgREST: change the
ConnectAs value from "admin" to the new login user (e.g., "postgrest_user"), add
SQL to the test setup to CREATE ROLE postgrest_user LOGIN with only the minimal
grants required, keep the existing "admin" role available for inspection only,
and update any role assertions (currently asserting behavior for the admin
connection) to assert against the new postgrest_user connection and its
privileges (symbols: ConnectAs, the test setup/fixture that creates roles, and
the assertions that check grants/roles).
🪄 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: 90865f4d-5dde-4cd7-87c2-0e8083d85d54

📥 Commits

Reviewing files that changed from the base of the PR and between 107121a and 987a891.

📒 Files selected for processing (17)
  • docs/services/index.md
  • docs/services/managing.md
  • docs/services/postgrest.md
  • e2e/postgrest_test.go
  • server/internal/api/apiv1/validate.go
  • server/internal/api/apiv1/validate_test.go
  • server/internal/api/middleware.go
  • server/internal/database/postgrest_service_config_test.go
  • server/internal/orchestrator/swarm/orchestrator.go
  • server/internal/orchestrator/swarm/postgrest_authenticator_resource.go
  • server/internal/orchestrator/swarm/postgrest_authenticator_resource_test.go
  • server/internal/orchestrator/swarm/postgrest_config_resource.go
  • server/internal/orchestrator/swarm/postgrest_config_resource_test.go
  • server/internal/orchestrator/swarm/postgrest_preflight_resource_test.go
  • server/internal/orchestrator/swarm/service_instance.go
  • server/internal/orchestrator/swarm/service_instance_spec.go
  • server/internal/orchestrator/swarm/service_spec_test.go

@moizpgedge moizpgedge marked this pull request as ready for review April 18, 2026 13:55
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
server/internal/orchestrator/swarm/postgrest_authenticator_resource.go (1)

106-149: ⚠️ Potential issue | 🟠 Major

PostgREST requires NOINHERIT on the connect_as user for security, but this conflicts with documented support for shared users.

Create unconditionally sets rolinherit = false on the ConnectAsUsername role (line 136). PostgREST's official documentation requires this for security: NOINHERIT prevents the authenticator from automatically inheriting privileges from member roles during connection, which is critical to avoid privilege leakage across requests.

However, documentation explicitly states the connect_as user "is customer-managed and may be shared with other services or applications." If the same user is:

  • Configured as connect_as for a PostgREST service (gets NOINHERIT)
  • Also used as a regular application user or by other services that expect to inherit from role memberships

then the NOINHERIT attribute breaks the intended semantics for non-PostgREST usage. Delete does not revert the change (line 226-228), leaving the user permanently mutated.

Either:

  1. Validate at the API layer that PostgREST's connect_as user is not reused across services, or
  2. Track the prior rolinherit value in state and restore it on Delete.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server/internal/orchestrator/swarm/postgrest_authenticator_resource.go`
around lines 106 - 149, The Create method
(PostgRESTAuthenticatorResource.Create) unconditionally sets NOINHERIT on the
connect-as role (username from r.authenticatorUsername()), which can break
shared users; before executing the ALTER ROLE statement, query pg_roles for the
current rolinherit value for that username (e.g. SELECT rolinherit FROM pg_roles
WHERE rolname=...), persist that original boolean into the resource state via
the provided resource.Context (rc) or the resource's metadata, then continue to
apply NOINHERIT; update PostgRESTAuthenticatorResource.Delete to read that
stored original rolinherit value and restore it (using ALTER ROLE ...
INHERIT/NOINHERIT as appropriate) in a transactional manner (use the same
connection/tx pattern used in Create), and handle absent/unknown stored state by
skipping restore and logging a warning.
🤖 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/postgrest_authenticator_resource.go`:
- Around line 106-149: The Create method (PostgRESTAuthenticatorResource.Create)
unconditionally sets NOINHERIT on the connect-as role (username from
r.authenticatorUsername()), which can break shared users; before executing the
ALTER ROLE statement, query pg_roles for the current rolinherit value for that
username (e.g. SELECT rolinherit FROM pg_roles WHERE rolname=...), persist that
original boolean into the resource state via the provided resource.Context (rc)
or the resource's metadata, then continue to apply NOINHERIT; update
PostgRESTAuthenticatorResource.Delete to read that stored original rolinherit
value and restore it (using ALTER ROLE ... INHERIT/NOINHERIT as appropriate) in
a transactional manner (use the same connection/tx pattern used in Create), and
handle absent/unknown stored state by skipping restore and logging a warning.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bc53282b-49ee-44f9-98a3-16ffc08e9ddc

📥 Commits

Reviewing files that changed from the base of the PR and between 987a891 and f7e9248.

📒 Files selected for processing (1)
  • server/internal/orchestrator/swarm/postgrest_authenticator_resource.go

@rshoemaker
Copy link
Copy Markdown
Contributor

@CodeRabbit

  1. PostgREST docs say NOINHERIT on the role is a "should" not "must" — NOINHERIT is a security best practice, not enforced by PostgREST. SET LOCAL ROLE works regardless.
  2. Users who want NOINHERIT can create a dedicated authenticator role via scripts.post_init and point connect_as at it. The control plane doesn't need to prescribe it.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 20, 2026

Tip

For best results, initiate chat on the files or code changes.

@rshoemaker Thanks for the clarification — that makes sense. Since SET LOCAL ROLE works regardless of the NOINHERIT attribute, and the PostgREST docs treat it as a recommendation rather than a requirement, the control plane doesn't need to enforce it. The escape hatch via scripts.post_init + a dedicated authenticator role is a clean way to support users who want the stricter security posture without baking it into the platform.


✏️ Learnings added
Learnt from: rshoemaker
URL: https://github.com/pgEdge/control-plane/pull/352

Timestamp: 2026-04-20T19:40:04.019Z
Learning: In pgEdge/control-plane, PostgREST authenticator roles do NOT need to have NOINHERIT enforced by the control plane. PostgREST's SET LOCAL ROLE works regardless of the NOINHERIT attribute — NOINHERIT is a security best practice ("should") per PostgREST docs, not a requirement ("must"). Users who want NOINHERIT can create a dedicated authenticator role via scripts.post_init and point connect_as at it. Do not flag the absence of NOINHERIT enforcement on the connect_as user as a bug or security issue.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: jason-lynch
Repo: pgEdge/control-plane PR: 348
File: server/internal/database/dump_roles.go:79-94
Timestamp: 2026-04-17T14:48:22.962Z
Learning: In `server/internal/database/dump_roles.go` (Go), the `DumpRolesResource` is only used during add-node/Spock-populate operations, not for delta or in-place restores. Replaying ALL `ALTER ROLE` statements from the dump — including those targeting built-in roles such as `pgedge`, `patroni_replicator`, `pgedge_application`, `pgedge_application_read_only`, and `pgedge_superuser` — is intentional in this context. Do not flag the absence of built-in-role filtering in `SanitizeRolesDump` or the statement-replay loop in `instance_resource.go` as a bug or security issue for this code path.

Learnt from: rshoemaker
Repo: pgEdge/control-plane PR: 338
File: server/internal/orchestrator/swarm/service_instance_spec.go:93-96
Timestamp: 2026-04-10T22:03:19.727Z
Learning: In `server/internal/orchestrator/swarm/service_instance_spec.go`, `ServiceInstanceSpecResource.Credentials` is never set for MCP service instances: `plan_update.go` never populates it, `populateCredentials` has always short-circuited for MCP before this PR, and converting a non-MCP service to MCP is not a supported operation. Do not flag the early return in `populateCredentials` for MCP as a risk of carrying stale credentials — the field is always nil for MCP resources.

Learnt from: CR
Repo: pgEdge/control-plane PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-14T14:51:46.669Z
Learning: Use optional TLS certificates for API communication and TLS certificates for system Postgres users; implement etcd RBAC for access control and use join tokens for secure cluster joining

Learnt from: rshoemaker
Repo: pgEdge/control-plane PR: 338
File: server/internal/workflows/plan_update.go:126-138
Timestamp: 2026-04-10T22:04:22.172Z
Learning: In `server/internal/workflows/plan_update.go` (and the pgEdge/control-plane codebase generally), do NOT add a defensive empty-password guard when resolving `connect_as` credentials from `spec.DatabaseUsers` in the workflow planner. The `database_users` password field is already validated as required and non-empty at the API layer (`server/internal/api/apiv1/validate.go`) before the spec reaches the planner, making an empty-password check in the planner redundant defensive coding against an impossible state.

@rshoemaker rshoemaker merged commit a7e1e07 into main Apr 20, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants