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?
There was a problem hiding this comment.
This is an accurate comment. What it's telling you is that in order for the postgrest container to read this file, it needs to be owned by the user that postgrest runs as. It also gives you a suggestion to explicitly set the UID for the container, which is a good suggestion.
If you look at the official postgrest Dockerfile: https://github.com/PostgREST/postgrest/blob/main/Dockerfile
It says:
USER 1000Which means that postgrest runs with UID = 1000 by default. What I would do in this case is to do the chown, like Copilot suggests, and I would do it with UID and GID = 1000. You can find a few examples of that operation by searching the code for fs.Chown, such as this one in the DirResource that copilot mentions:
control-plane/server/internal/filesystem/dir_resource.go
Lines 115 to 116 in 6ee7a84
I'd also take its other suggestion, and explicitly set the container UID to 1000. Since that's also the default, it doesn't have any effect now, but it protects us in case that container image changes its UID.
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
- Add postgrestContainerUID (1000) from official PostgREST Dockerfile and set explicit container User in the PostgREST service spec - Chown postgrest.conf to UID 1000 so the non-root container user can read it via the bind mount - Add PGRST_SERVER_HOST=0.0.0.0 to env vars; without this PostgREST defaults to server-host="!4" which causes postgrest --ready to fail with "cannot be used when server-host is configured as !4" PLAT-501, PLAT-502, PLAT-503
PLAT-501, PLAT-502, PLAT-503
1de877b
into
feat/PLAT-499/PLAT-500/PostgREST-prerequisites/image-wiring
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
Testing
Unit tests
go test ./server/internal/orchestrator/swarm/... -run "TestGeneratePostgRESTConfig|TestServiceContainerSpec|TestRoleAttributesAndGrants" -v
go test ./server/internal/database/... -run TestParsePostgRESTServiceConfig -v
Checklist
Notes for Reviewers
Base branch is feat/PLAT-499/PLAT-500/PostgREST-prerequisites/image-wiring (not main)
End-to-end provisioning is complete — orchestrator wiring is in PLAT-504 (separate PR on top of this one)
MCP path is unchanged; roleAttributesAndGrants() is now PostgREST-only — MCP uses built-in group roles (pgedge_application_read_only / pgedge_application) via Roles field in CreateUserRole, not this function
Copilot review items addressed: nil guards in GeneratePostgRESTConfig, panic on unexpected statement type in statementsSQL, container UID and chown for postgrest.conf