feat: add PostgREST container spec, config file, and service user role#303
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 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 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 |
There was a problem hiding this comment.
Pull request overview
Adds the initial Swarm “runtime layer” for running PostgREST alongside existing services by introducing PostgREST config generation + host-file lifecycle management, extending the service container spec builder, and expanding service-user role SQL to support PostgREST’s SET ROLE flow.
Changes:
- Add PostgREST config generation (
postgrest.conf) plus a host filesystem resource to manage it. - Extend Swarm
ServiceContainerSpecto support a newpostgrestservice type (command/args, healthcheck, env vars, RO mount). - Extend
ServiceUserRolewithServiceTypeandDBAnonRoleto generate PostgREST-appropriate role attributes/grants (and add unit tests).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| server/internal/orchestrator/swarm/postgrest_config.go | Generates postgrest.conf content from typed PostgREST service config. |
| server/internal/orchestrator/swarm/postgrest_config_resource.go | Manages postgrest.conf lifecycle on the host filesystem. |
| server/internal/orchestrator/swarm/postgrest_config_test.go | Unit coverage for config generation and absence/presence of JWT fields. |
| server/internal/orchestrator/swarm/service_spec.go | Adds postgrest container spec branch (env, mounts, healthcheck, command). |
| server/internal/orchestrator/swarm/service_spec_test.go | Adds PostgREST container spec unit tests. |
| server/internal/orchestrator/swarm/service_user_role.go | Adds service-type-driven role/grant generation, bumps resource version. |
| server/internal/orchestrator/swarm/service_user_role_test.go | Adds unit tests for PostgREST + MCP role/grant behavior. |
| server/internal/orchestrator/swarm/resources.go | Registers PostgRESTConfigResource type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| default: // "mcp" and future service types | ||
| // MCP role: direct grants rather than role membership to avoid exposing | ||
| // replication internals via pgedge_application_read_only. | ||
| // https://github.com/pgEdge/pgedge-postgres-mcp/blob/main/docs/guide/security_mgmt.go |
| func statementsSQL(stmts postgres.Statements) []string { | ||
| out := make([]string, 0, len(stmts)) | ||
| for _, s := range stmts { | ||
| if stmt, ok := s.(postgres.Statement); ok { | ||
| out = append(out, stmt.SQL) | ||
| } | ||
| } |
| r := &ServiceUserRole{ | ||
| ServiceType: "mcp", | ||
| DatabaseName: "mydb", | ||
| Username: `"svc_mcp"`, |
| func GeneratePostgRESTConfig(params *PostgRESTConfigParams) ([]byte, error) { | ||
| cfg := params.Config | ||
|
|
||
| var buf bytes.Buffer | ||
|
|
||
| fmt.Fprintf(&buf, "db-schemas = %q\n", cfg.DBSchemas) | ||
| fmt.Fprintf(&buf, "db-anon-role = %q\n", cfg.DBAnonRole) | ||
| fmt.Fprintf(&buf, "db-pool = %d\n", cfg.DBPool) | ||
| fmt.Fprintf(&buf, "db-max-rows = %d\n", cfg.MaxRows) |
| configPath := filepath.Join(dirPath, "postgrest.conf") | ||
| if err := afero.WriteFile(fs, configPath, content, 0o600); err != nil { | ||
| return fmt.Errorf("failed to write %s: %w", configPath, err) | ||
| } |
There was a problem hiding this comment.
@jason-lynch
Can you guide for this issue?
c1b42cd to
f8de470
Compare
- Add PostgRESTConfigResource: writes postgrest.conf to the service data directory from a Go template; re-runs on every apply via ErrNotFound - Add PostgRESTConfigResource registration in RegisterResourceTypes - Extend ServiceUserRole with ServiceType and DBAnonRole fields; add roleAttributesAndGrants() — PostgREST gets LOGIN+NOINHERIT and GRANT <anon_role>, MCP/default uses group role membership - Extend ServiceContainerSpecOptions with DatabaseHosts and TargetSessionAttrs; build PGHOST/PGPORT from the hosts slice in buildPostgRESTEnvVars; pass both fields from ServiceInstanceSpecResource - Add postgrest case to ServiceContainerSpec: postgrest command, read-only config mount, and postgrest --ready health check - Add tests for postgrest container spec, config generation, and service user role attributes/grants PLAT-501, PLAT-502, PLAT-503
99689f0 to
4d38da0
Compare
- Fix dead link in MCP grants comment: .go → .md - Panic on unexpected statement type in statementsSQL test helper instead of silently dropping statements - Use unquoted username in TestRoleAttributesAndGrants_MCP to better reflect production input (quoting is the caller's responsibility) - Guard against nil params/Config in GeneratePostgRESTConfig PLAT-501, PLAT-502, PLAT-503
| // replication internals via pgedge_application_read_only. | ||
| // https://github.com/pgEdge/pgedge-postgres-mcp/blob/main/docs/guide/security_mgmt.md | ||
| attributes := []string{"LOGIN"} | ||
| grants := postgres.Statements{ |
There was a problem hiding this comment.
I think we've removed these in the latest version of this file. Is that right, @rshoemaker?
There was a problem hiding this comment.
Correct, we use the built-in RO/RW roles instead:
control-plane/server/internal/orchestrator/swarm/service_user_role.go
Lines 169 to 178 in 4713732
There was a problem hiding this comment.
@moizpgedge After looking at this more carefully, I can see that the RO/RW roles are still being used correctly in createUserRole(). The "default:" block here is dead code (I think). It should probably be removed and this func should return an error for non-PostgREST cases. Or refactor the code.
| mounts = []mount.Mount{ | ||
| docker.BuildMount(opts.DataPath, "/app/data", true), | ||
| } | ||
| default: // "mcp" |
There was a problem hiding this comment.
Same here - MCP specific code shouldn't be in a "default" branch, it should have an explicit branch check for "mcp".
- Remove dead MCP path from roleAttributesAndGrants; function is now PostgREST-only since MCP uses the group-role path in createUserRole - Remove associated MCP tests (TestRoleAttributesAndGrants_MCP, TestRoleAttributesAndGrants_EmptyServiceTypeIsMCP) - Replace default branch with explicit case "mcp" in ServiceContainerSpec switch; unknown service types now return an error PLAT-501, PLAT-502, PLAT-503
postgrest_config.go: generatespostgrest.confwith db-schemas, db-anon-role,db-pool, db-max-rows, and optional JWT fields; credentials are excluded from the file
and injected as libpq env vars at the container level; nil-guards on params/Config
postgrest_config_resource.go: managespostgrest.conflifecycle on the hostfilesystem; depends on
DirResource; delete is a no-op (parent dir handles cleanup)service_spec.go: PostgREST container usespostgrest /app/data/postgrest.confas command,
postgrest --readyhealth check (no curl in static binary image),read-only bind mount, and PGHOST/PGPORT/PGDATABASE/PGUSER/PGPASSWORD env vars;
PGHOST and PGPORT are built as comma-separated lists from the
DatabaseHostssliceto support multi-host libpq connections; PGTARGETSESSIONATTRS is set when present
service_instance_spec.go: passDatabaseHostsandTargetSessionAttrsthrough to
ServiceContainerSpecOptionsservice_user_role.go: addServiceTypeandDBAnonRolefields; PostgRESTrole is created with
LOGIN NOINHERIT+GRANT <anon_role>to enable theSET ROLEmechanism; bumpResourceVersionto 4PostgRESTConfigResourceinresources.goSummary
Implements the PostgREST container runtime layer: config file generation,
host filesystem resource management, container spec (command, health check,
mounts, env vars), and service user role SQL with NOINHERIT semantics.
Changes
postgrest_config.go: generatespostgrest.confwith db-schemas, db-anon-role,db-pool, db-max-rows, and optional JWT fields; credentials are excluded from the file
and injected as libpq env vars at the container level
postgrest_config_resource.go: managespostgrest.conflifecycle on the hostfilesystem; depends on
DirResource; delete is a no-op (parent dir handles cleanup)service_spec.go: PostgREST container usespostgrest /app/data/postgrest.confas command,
postgrest --readyhealth check (no curl in static binary image),read-only bind mount, and PGHOST/PGPORT/PGDATABASE/PGUSER/PGPASSWORD env vars
service_user_role.go: addServiceTypeandDBAnonRolefields; PostgRESTrole is created with
LOGIN NOINHERIT+GRANT <anon_role>to enable theSET ROLEmechanism; bumpResourceVersionto 4PostgRESTConfigResourceinresources.goTesting
Checklist
Notes for Reviewers
Base branch is feat/PLAT-499/PLAT-500/PostgREST-prerequisites/image-wiring (not main)
End-to-end provisioning is not yet functional — blocked on (PLAT-504, orchestrator wiring + multi-host PGHOST) which depends on PR #301 merging
MCP path is unchanged; both service types share roleAttributesAndGrants() dispatch
Copilot review items addressed: nil guard in GeneratePostgRESTConfig, panic on
unexpected statement type in statementsSQL test helper, unquoted username in MCP test.