Feat: add PostgREST as a service type (PLAT-499, PLAT-500,PLAT-501,PLAT-502,PLAT-503)#300
Merged
moizpgedge merged 7 commits intomainfrom Apr 3, 2026
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds PostgREST support to the swarm orchestrator: registers 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/service_images_test.go`:
- Around line 109-114: The test case named "postgrest service has versions"
currently only checks the length; change it to assert the actual returned
versions from SupportedServiceVersions("postgrest") rather than just count:
collect the map keys returned by SupportedServiceVersions, sort them (or compare
as an unordered set) and assert they equal the expected set {"latest","v14.5"}
so the test fails if any other two-element combination is returned; update the
assertion in the test function in service_images_test.go accordingly (use the
test helper or testify/reflect-based equality as appropriate).
In `@server/internal/workflows/activities/validate_postgrest_prereqs.go`:
- Around line 134-150: The code currently falls back to db.Instances[0] when
Patroni discovery fails; instead, iterate db.Instances and pick the first
instance whose connection info successfully resolves via
a.DatabaseService.GetInstanceConnectionInfo (and optionally verify
reachability), then assign its InstanceID to primaryInstanceID. Update the
fallback block so that if primaryInstanceID == "" you loop through db.Instances,
call a.DatabaseService.GetInstanceConnectionInfo(ctx, databaseID,
inst.InstanceID), use the first call that returns no error (and/or a lightweight
reachability check) to set primaryInstanceID, rather than unconditionally using
db.Instances[0]; keep references to patroni.NewClient and
database.GetPrimaryInstanceID unchanged.
In `@server/internal/workflows/update_database.go`:
- Around line 106-117: The code currently ignores parseErrs from
ParsePostgRESTServiceConfig and only runs preflight when parseErrs is empty;
change this to fail fast when parseErrs is non-empty: if
ParsePostgRESTServiceConfig returns any parseErrs, return an error (wrap via
handleError) describing the invalid PostgREST config and include parseErrs
details so the workflow stops before planning; keep the existing preflight call
(ExecuteValidatePostgRESTPrereqs with ValidatePostgRESTPrereqsInput using
cfg.DBSchemas and cfg.DBAnonRole) for the valid-path case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc966f26-8fa1-4b2b-acdb-4b3e618ba438
📒 Files selected for processing (5)
server/internal/orchestrator/swarm/service_images.goserver/internal/orchestrator/swarm/service_images_test.goserver/internal/workflows/activities/activities.goserver/internal/workflows/activities/validate_postgrest_prereqs.goserver/internal/workflows/update_database.go
server/internal/workflows/activities/validate_postgrest_prereqs.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Registers PostgREST service images in the Swarm service image registry and adds a preflight workflow activity that validates required Postgres schemas and the configured anonymous role exist before provisioning PostgREST.
Changes:
- Add
ValidatePostgRESTPrereqsworkflow activity to connect to the database and verify configured schemas + anon role exist. - Run the PostgREST preflight checks in
UpdateDatabasebefore planning/provisioning. - Register PostgREST service image versions and extend Swarm service image tests accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/internal/workflows/update_database.go | Runs PostgREST prerequisite validation before the plan/apply phases. |
| server/internal/workflows/activities/validate_postgrest_prereqs.go | New activity that connects to a DB instance and checks schemas + anon role existence. |
| server/internal/workflows/activities/activities.go | Registers the new ValidatePostgRESTPrereqs activity with the worker. |
| server/internal/orchestrator/swarm/service_images.go | Adds PostgREST image entries (latest, v14.5) to the service image registry. |
| server/internal/orchestrator/swarm/service_images_test.go | Adds/updates tests to cover PostgREST image resolution and version listing expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
server/internal/workflows/activities/validate_postgrest_prereqs.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/workflows/activities/validate_postgrest_prereqs.go`:
- Around line 160-163: The current call to
DatabaseService.GetInstanceConnectionInfo(databaseID, primaryInstanceID) returns
an error and bails out immediately; instead, when that call fails, fall back to
the previously-captured reachable connection info (e.g., reachableConnInfo /
reachableInstanceID) for the read-only checks: attempt to assign connInfo =
reachableConnInfo and proceed (optionally log or warn that primary lookup failed
and reachable instance is being used); only return the wrapped error if no
reachableConnInfo is available. Update the logic around connInfo,
primaryInstanceID, and reachableConnInfo in the ValidatePostgrestPrereqs flow to
prefer the primary result but accept the reachable fallback on error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b76c1ad0-1ff4-4562-aa87-401f010c42b8
📒 Files selected for processing (4)
server/internal/orchestrator/swarm/service_images.goserver/internal/orchestrator/swarm/service_images_test.goserver/internal/workflows/activities/validate_postgrest_prereqs.goserver/internal/workflows/update_database.go
server/internal/workflows/activities/validate_postgrest_prereqs.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/internal/api/apiv1/validate.go (1)
260-264: Consider updating the error message to reflect two-part version support.The error message references "semver format (e.g., '1.0.0')" but the validation now also accepts two-part versions like "14.5". Consider updating the message to be more accurate:
- err := errors.New("version must be in semver format (e.g., '1.0.0') or 'latest'") + err := errors.New("version must be in format 'X.Y' or 'X.Y.Z' (e.g., '14.5', '1.0.0') or 'latest'")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/api/apiv1/validate.go` around lines 260 - 264, The validation error message for svc.Version is out of date: update the string created where svc.Version is checked (the block using semverPattern, newValidationError, appendPath, and errs) to accurately state that versions may be semver (e.g., "1.0.0"), two-part versions (e.g., "14.5"), or "latest" so the message matches the accepted formats.
🤖 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/api/apiv1/validate.go`:
- Around line 260-264: The validation error message for svc.Version is out of
date: update the string created where svc.Version is checked (the block using
semverPattern, newValidationError, appendPath, and errs) to accurately state
that versions may be semver (e.g., "1.0.0"), two-part versions (e.g., "14.5"),
or "latest" so the message matches the accepted formats.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90ed306b-89ae-47e2-b987-2070738d8d56
⛔ Files ignored due to path filters (7)
api/apiv1/gen/control_plane/service.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/types.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 (6)
api/apiv1/design/database.goserver/internal/api/apiv1/validate.goserver/internal/api/apiv1/validate_test.goserver/internal/orchestrator/swarm/service_images.goserver/internal/orchestrator/swarm/service_images_test.goserver/internal/workflows/activities/validate_postgrest_prereqs.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/internal/orchestrator/swarm/service_images_test.go
- server/internal/workflows/activities/validate_postgrest_prereqs.go
moizpgedge
force-pushed
the
feat/PLAT-499/PLAT-500/PostgREST-prerequisites/image-wiring
branch
from
99689f0 to
e80ba4c
Compare
Contributor
Author
|
Post Pr for PLAT-501/502/503 PR:-#303 |
jason-lynch
reviewed
Mar 19, 2026
server/internal/workflows/activities/validate_postgrest_prereqs.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/internal/orchestrator/swarm/postgrest_preflight_resource.go (1)
74-81: Logger obtained but never used; error silently discarded.The logger is retrieved but not used anywhere in the
validatemethod. Additionally, the error fromdo.Invokeis discarded with_, which could mask DI configuration issues.Consider either:
- Using the logger to log validation progress/results (e.g., schemas being checked, successful validation)
- Removing the logger invocation if it's not needed
♻️ Option A: Remove unused logger
func (r *PostgRESTPreflightResource) validate(ctx context.Context, rc *resource.Context) error { - logger, _ := do.Invoke[zerolog.Logger](rc.Injector) - - conn, err := connectToPrimaryDB(ctx, rc, r.DatabaseID, r.DatabaseName, logger) + conn, err := connectToPrimaryDB(ctx, rc, r.DatabaseID, r.DatabaseName, zerolog.Nop()) if err != nil { return fmt.Errorf("preflight: failed to connect to database: %w", err) }♻️ Option B: Use logger and handle error
func (r *PostgRESTPreflightResource) validate(ctx context.Context, rc *resource.Context) error { - logger, _ := do.Invoke[zerolog.Logger](rc.Injector) + logger, err := do.Invoke[zerolog.Logger](rc.Injector) + if err != nil { + return fmt.Errorf("failed to get logger: %w", err) + } + logger = logger.With(). + Str("service_id", r.ServiceID). + Str("database_id", r.DatabaseID). + Logger() + logger.Debug().Msg("running PostgREST preflight validation") conn, err := connectToPrimaryDB(ctx, rc, r.DatabaseID, r.DatabaseName, logger)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/swarm/postgrest_preflight_resource.go` around lines 74 - 81, The call to do.Invoke[zerolog.Logger](rc.Injector) in PostgRESTPreflightResource.validate either needs its error handled and the logger used, or the unused invocation removed; update the validate function to call do.Invoke and check the returned error (not discard it) and then use the logger to emit at least a start/ok/failure message around connectToPrimaryDB and validation steps (include the database id/name from r.DatabaseID/r.DatabaseName in logs), or simply delete the do.Invoke line if no logger is required—ensure any retained logger variable is referenced so the compiler won’t flag it and that DI errors from do.Invoke are returned instead of ignored.
🤖 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/postgrest_preflight_resource.go`:
- Around line 74-81: The call to do.Invoke[zerolog.Logger](rc.Injector) in
PostgRESTPreflightResource.validate either needs its error handled and the
logger used, or the unused invocation removed; update the validate function to
call do.Invoke and check the returned error (not discard it) and then use the
logger to emit at least a start/ok/failure message around connectToPrimaryDB and
validation steps (include the database id/name from r.DatabaseID/r.DatabaseName
in logs), or simply delete the do.Invoke line if no logger is required—ensure
any retained logger variable is referenced so the compiler won’t flag it and
that DI errors from do.Invoke are returned instead of ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11b93e64-2be7-4519-ad38-e99af521a779
📒 Files selected for processing (3)
server/internal/orchestrator/swarm/postgrest_preflight_resource.goserver/internal/orchestrator/swarm/resources.goserver/internal/orchestrator/swarm/service_user_role.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/internal/orchestrator/swarm/resources.go
moizpgedge
force-pushed
the
feat/PLAT-499/PLAT-500/PostgREST-prerequisites/image-wiring
branch
from
c1b42cd to
f8de470
Compare
jason-lynch
reviewed
Mar 25, 2026
Member
jason-lynch
left a comment
jason-lynch
left a comment
There was a problem hiding this comment.
Nice! This is looking pretty close.
server/internal/orchestrator/swarm/postgrest_preflight_resource.go
Outdated
Show resolved
Hide resolved
server/internal/orchestrator/swarm/postgrest_preflight_resource.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // satisfied the resource is considered up-to-date (no permadrift). | ||
| func (r *PostgRESTPreflightResource) Refresh(ctx context.Context, rc *resource.Context) error { | ||
| if err := r.validate(ctx, rc); err != nil { | ||
| return fmt.Errorf("%w: %s", resource.ErrNotFound, err.Error()) |
There was a problem hiding this comment.
Refresh wraps validation failures as fmt.Errorf("%w: %s", resource.ErrNotFound, err.Error()), which stringifies and drops the underlying error chain. Prefer wrapping the original error value (e.g., using %v or joining errors) so callers can inspect/unwrap and so multi-error context from errors.Join is preserved more faithfully.
Suggested change
| return fmt.Errorf("%w: %s", resource.ErrNotFound, err.Error()) | |
| return errors.Join(resource.ErrNotFound, err) |
Comment on lines
+171
to
+197
| if r.ServiceType == "postgrest" { | ||
| attributes, grants := r.roleAttributesAndGrants() | ||
| statements, err := postgres.CreateUserRole(postgres.UserRoleOptions{ | ||
| Name: r.Username, | ||
| Password: r.Password, | ||
| DBOwner: false, | ||
| Attributes: attributes, | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to generate create user role statements: %w", err) | ||
| } | ||
| if err := statements.Exec(ctx, conn); err != nil { | ||
| return fmt.Errorf("failed to create service user: %w", err) | ||
| } | ||
| if err := grants.Exec(ctx, conn); err != nil { | ||
| return fmt.Errorf("failed to grant service user permissions: %w", err) | ||
| } | ||
| } else { | ||
| var groupRole string | ||
| switch r.Mode { | ||
| case ServiceUserRoleRO: | ||
| groupRole = "pgedge_application_read_only" | ||
| case ServiceUserRoleRW: | ||
| groupRole = "pgedge_application" | ||
| default: | ||
| return fmt.Errorf("unknown service user role mode: %q", r.Mode) | ||
| } |
There was a problem hiding this comment.
createUserRole treats any ServiceType other than exactly "postgrest" as the MCP/group-role path. This means typos or unexpected values in ServiceType will silently grant the wrong permissions. Consider switching on ServiceType and returning an explicit error for unknown non-empty values (and/or defaulting empty to "mcp" explicitly) to avoid misprovisioning.
| } | ||
|
|
||
| func validateS3RepoProperties(props repoProperties, path []string) []error { | ||
| var errs []error |
There was a problem hiding this comment.
semverPattern now allows two-part versions (e.g. 14.5), but validateServiceSpec still produces the error message "version must be in semver format (e.g., '1.0.0') or 'latest'". Update the message to reflect the accepted formats (e.g. '1.0.0', '14.5', or 'latest') so API validation feedback matches the actual pattern.
moizpgedge
force-pushed
the
feat/PLAT-499/PLAT-500/PostgREST-prerequisites/image-wiring
branch
from
2f2e1bf to
069f53b
Compare
moizpgedge
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (4)
server/internal/orchestrator/swarm/service_spec_test.go (1)
384-400: Consider strengthening PGRST_DB_URI assertion.The current check only verifies the value starts with
"postgresql://". Consider usingstrings.HasPrefixor a more complete assertion to ensure the URI format is correct:checks := map[string]string{ // ... } // Special case for PGRST_DB_URI prefix check if uri, ok := envMap["PGRST_DB_URI"]; !ok { t.Error("env var PGRST_DB_URI is missing") } else if !strings.HasPrefix(uri, "postgresql://") { t.Errorf("PGRST_DB_URI should start with postgresql://, got %q", uri) }However, the current approach works since
"postgresql://" != "postgresql://..."would fail the equality check. The test effectively validates the URI scheme is present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/swarm/service_spec_test.go` around lines 384 - 400, The PGRST_DB_URI equality check is weak; modify the test in service_spec_test.go (within the envMap checks loop or right before it) to special-case "PGRST_DB_URI": first assert the key exists in envMap, then assert the value has the correct prefix using strings.HasPrefix(uri, "postgresql://") (or a more complete URI format check) instead of strict equality; keep the existing exact equality checks for the other keys ("PGRST_SERVER_PORT", "PGHOST", etc.) and import "strings" if not already present.server/internal/orchestrator/swarm/postgrest_preflight_resource.go (1)
47-53: Add database node dependency to ensure preflight validation runs after the Postgres instance is ready.
Dependencies()returnsnil, but this resource accesses the database viadatabase.GetPrimaryInstance(ctx, rc, r.NodeName)to validate schemas and roles. Other resources that access the database through aNodeNamefield declare this dependency (seepgbackrest_stanza.goandservice_user_role.go). Add:Suggested change
func (r *PostgRESTPreflightResource) Dependencies() []resource.Identifier { - return nil + return []resource.Identifier{ + database.NodeResourceIdentifier(r.NodeName), + } }This aligns with the established pattern in the codebase and ensures the preflight check doesn't run before the Postgres instance is available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/swarm/postgrest_preflight_resource.go` around lines 47 - 53, Dependencies() currently returns nil but PostgRESTPreflightResource calls database.GetPrimaryInstance(ctx, rc, r.NodeName), so change Dependencies() to declare a dependency on the database node identified by r.NodeName using the same resource identifier pattern used by pgbackrest_stanza and service_user_role (i.e., return a []resource.Identifier containing the DB instance identifier for r.NodeName) so the preflight runs only after the Postgres instance is ready.server/internal/orchestrator/swarm/postgrest_config_test.go (1)
35-43: This test doesn't exercise the actual empty-config defaulting path.
TestGeneratePostgRESTConfig_Defaultspre-populates every field with the expected defaults, so it won't catch a regression whereconfig: {}stops defaulting correctly. I'd either make this fixture empty or rename the test and cover default injection at the layer that owns it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/swarm/postgrest_config_test.go` around lines 35 - 43, The test TestGeneratePostgRESTConfig_Defaults currently pre-populates PostgRESTConfigParams.Config with the expected defaults (database.PostgRESTServiceConfig fields), so it doesn't verify the defaulting logic; update the test to pass an empty/nil Config (e.g., &PostgRESTConfigParams{Config: &database.PostgRESTServiceConfig{}} or nil) and assert that GeneratePostgRESTConfig (or the function under test that applies defaults) fills in DBSchemas, DBAnonRole, DBPool, MaxRows with the expected default values, or alternatively rename the test to reflect it is a "pre-populated values" case if you intend to keep it as-is.server/internal/orchestrator/swarm/service_spec.go (1)
137-142: Fail fast when no Postgres hosts are available.If
DatabaseHostsis empty,buildPostgRESTEnvVars()emitsPGHOST=/PGPORT=and the service only fails later inside the container. SinceServiceContainerSpec()already returns an error, it would be better to reject that state here and keep the failure actionable during provisioning.💡 Proposed change
switch opts.ServiceSpec.ServiceType { case "postgrest": + if len(opts.DatabaseHosts) == 0 { + return swarm.ServiceSpec{}, fmt.Errorf("postgrest service requires at least one database host") + } user = fmt.Sprintf("%d", postgrestContainerUID) command = []string{"postgrest"} args = []string{"/app/data/postgrest.conf"} env = buildPostgRESTEnvVars(opts)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/swarm/service_spec.go` around lines 137 - 142, When handling the "postgrest" branch in ServiceContainerSpec, fail fast if no Postgres hosts are configured: before calling buildPostgRESTEnvVars(opts) check opts.DatabaseHosts (or the field on opts that lists DB hosts) and return an error indicating no Postgres hosts available; this prevents emitting empty PGHOST/PGPORT env vars and surfaces the provisioning failure earlier. Reference the "postgrest" case in ServiceContainerSpec, the opts parameter, and the buildPostgRESTEnvVars function when adding the check and returning the error.
🤖 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/postgrest_config_test.go`:
- Around line 35-43: The test TestGeneratePostgRESTConfig_Defaults currently
pre-populates PostgRESTConfigParams.Config with the expected defaults
(database.PostgRESTServiceConfig fields), so it doesn't verify the defaulting
logic; update the test to pass an empty/nil Config (e.g.,
&PostgRESTConfigParams{Config: &database.PostgRESTServiceConfig{}} or nil) and
assert that GeneratePostgRESTConfig (or the function under test that applies
defaults) fills in DBSchemas, DBAnonRole, DBPool, MaxRows with the expected
default values, or alternatively rename the test to reflect it is a
"pre-populated values" case if you intend to keep it as-is.
In `@server/internal/orchestrator/swarm/postgrest_preflight_resource.go`:
- Around line 47-53: Dependencies() currently returns nil but
PostgRESTPreflightResource calls database.GetPrimaryInstance(ctx, rc,
r.NodeName), so change Dependencies() to declare a dependency on the database
node identified by r.NodeName using the same resource identifier pattern used by
pgbackrest_stanza and service_user_role (i.e., return a []resource.Identifier
containing the DB instance identifier for r.NodeName) so the preflight runs only
after the Postgres instance is ready.
In `@server/internal/orchestrator/swarm/service_spec_test.go`:
- Around line 384-400: The PGRST_DB_URI equality check is weak; modify the test
in service_spec_test.go (within the envMap checks loop or right before it) to
special-case "PGRST_DB_URI": first assert the key exists in envMap, then assert
the value has the correct prefix using strings.HasPrefix(uri, "postgresql://")
(or a more complete URI format check) instead of strict equality; keep the
existing exact equality checks for the other keys ("PGRST_SERVER_PORT",
"PGHOST", etc.) and import "strings" if not already present.
In `@server/internal/orchestrator/swarm/service_spec.go`:
- Around line 137-142: When handling the "postgrest" branch in
ServiceContainerSpec, fail fast if no Postgres hosts are configured: before
calling buildPostgRESTEnvVars(opts) check opts.DatabaseHosts (or the field on
opts that lists DB hosts) and return an error indicating no Postgres hosts
available; this prevents emitting empty PGHOST/PGPORT env vars and surfaces the
provisioning failure earlier. Reference the "postgrest" case in
ServiceContainerSpec, the opts parameter, and the buildPostgRESTEnvVars function
when adding the check and returning the error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 337e97d9-e28a-4bc8-9d01-1a1daad7b821
⛔ Files ignored due to path filters (7)
api/apiv1/gen/control_plane/service.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/types.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 (15)
api/apiv1/design/database.goserver/internal/api/apiv1/validate.goserver/internal/api/apiv1/validate_test.goserver/internal/orchestrator/swarm/postgrest_config.goserver/internal/orchestrator/swarm/postgrest_config_resource.goserver/internal/orchestrator/swarm/postgrest_config_test.goserver/internal/orchestrator/swarm/postgrest_preflight_resource.goserver/internal/orchestrator/swarm/resources.goserver/internal/orchestrator/swarm/service_images.goserver/internal/orchestrator/swarm/service_images_test.goserver/internal/orchestrator/swarm/service_instance_spec.goserver/internal/orchestrator/swarm/service_spec.goserver/internal/orchestrator/swarm/service_spec_test.goserver/internal/orchestrator/swarm/service_user_role.goserver/internal/orchestrator/swarm/service_user_role_test.go
✅ Files skipped from review due to trivial changes (3)
- server/internal/orchestrator/swarm/resources.go
- server/internal/api/apiv1/validate_test.go
- server/internal/orchestrator/swarm/service_images_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/internal/api/apiv1/validate.go
rshoemaker
reviewed
Mar 30, 2026
Contributor
rshoemaker
left a comment
rshoemaker
left a comment
There was a problem hiding this comment.
I think you're missing some diffs in other files.
This should allow "postgrest" as a service type:
control-plane/server/internal/orchestrator/swarm/orchestrator.go
Lines 405 to 408 in 6ee7a84
These should set you new "ServiceType" field:
control-plane/server/internal/orchestrator/swarm/orchestrator.go
Lines 454 to 467 in 6ee7a84
ServiceType: spec.ServiceSpec.ServiceType
I'm not sure about forcing DBAnon into the ServiceUserRole - it feels different enough for its own resource. Consider a PostgRESTAuthenticatorResource that depends on the generic ServiceUserRole and adds the NOINHERIT + grant on top. The generic ServiceUserRole would just create a normal LOGIN user for all service types, and PostgREST would alter it in its own resource. Keeps the shared path clean for RAG and whatever comes next.
Contributor
There was a problem hiding this comment.
This func needs to be updated to do something like:
func (s *ServiceInstanceSpecResource) Dependencies() []resource.Identifier {
deps := []resource.Identifier{
NetworkResourceIdentifier(s.DatabaseNetworkID),
ServiceUserRoleIdentifier(s.ServiceSpec.ServiceID, ServiceUserRoleRO),
ServiceUserRoleIdentifier(s.ServiceSpec.ServiceID, ServiceUserRoleRW),
}
switch s.ServiceSpec.ServiceType {
case "mcp":
deps = append(deps, MCPConfigResourceIdentifier(s.ServiceInstanceID))
case "postgrest":
deps = append(deps, PostgRESTConfigResourceIdentifier(s.ServiceInstanceID))
default:
log.Warn().Str("service_type", s.ServiceSpec.ServiceType).Msg("unknown service type in dependencies")
}
return deps
}
Contributor
Author
There was a problem hiding this comment.
In next Pr for PLAT-504I will solve this issue.
| if err := grants.Exec(ctx, conn); err != nil { | ||
| return fmt.Errorf("failed to grant service user permissions: %w", err) | ||
| } | ||
| } else { |
Contributor
There was a problem hiding this comment.
This "else" has to be an explicit check:
else if r.ServiceType == "mcp"
The "else" (default) case should throw an error for unknown service.
jason-lynch
reviewed
Mar 31, 2026
| } | ||
|
|
||
| // GeneratePostgRESTConfig generates the postgrest.conf file content. | ||
| // Credentials are not written here; they are injected as libpq env vars at the container level. |
Member
There was a problem hiding this comment.
Is this comment accurate? We don't want to use environment variables for credentials. They should be passed in via this configuration file.
|
|
||
| // GeneratePostgRESTConfig generates the postgrest.conf file content. | ||
| // Credentials are not written here; they are injected as libpq env vars at the container level. | ||
| func GeneratePostgRESTConfig(params *PostgRESTConfigParams) ([]byte, error) { |
Member
There was a problem hiding this comment.
This seems like it should be a method on database.PostgRESTServiceConfig rather than a standalone function in this package.
| // See: https://github.com/PostgREST/postgrest/blob/main/Dockerfile (USER 1000) | ||
| const postgrestContainerUID = 1000 | ||
|
|
||
| func buildPostgRESTEnvVars(opts *ServiceContainerSpecOptions) []string { |
Member
There was a problem hiding this comment.
Can you please change it so that we pass all of these connection parameters via the configuration file? Environment variables are visible to other users on the machine, whereas the configuration file is much more restricted.
jason-lynch
dismissed
their stale review
re-reviewing after merging stacked PRs
moizpgedge
changed the title
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 89 |
| Duplication | 19 |
TIP This summary will be updated as you push new changes. Give us feedback
moizpgedge
force-pushed
the
feat/PLAT-499/PLAT-500/PostgREST-prerequisites/image-wiring
branch
from
ddbbc1c to
069f53b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
server/internal/orchestrator/swarm/service_user_role.go (1)
171-188:⚠️ Potential issue | 🔴 CriticalFinish the service-type split; the old MCP block is still live.
Line 188 should be an explicit
else if r.ServiceType == "mcp", but the previous MCP block is still present below it. As written, Line 212 redeclaresgroupRole, Line 222 reuses:=with no new variable in scope, and unknown service types still fall through the MCP path instead of failing fast.🛠️ Suggested cleanup
- } else { + } else if r.ServiceType == "mcp" { var groupRole string switch r.Mode { case ServiceUserRoleRO: groupRole = "pgedge_application_read_only" case ServiceUserRoleRW: groupRole = "pgedge_application" default: return fmt.Errorf("unknown service user role mode: %q", r.Mode) } statements, err := postgres.CreateUserRole(postgres.UserRoleOptions{ Name: r.Username, Password: r.Password, DBOwner: false, Attributes: []string{"LOGIN"}, Roles: []string{groupRole}, }) if err != nil { return fmt.Errorf("failed to generate create user role statements: %w", err) } if err := statements.Exec(ctx, conn); err != nil { return fmt.Errorf("failed to create service user: %w", err) } + } else { + return fmt.Errorf("unknown service type: %q", r.ServiceType) } - - // Determine group role based on mode - var groupRole string - switch r.Mode { - case ServiceUserRoleRO: - groupRole = "pgedge_application_read_only" - case ServiceUserRoleRW: - groupRole = "pgedge_application" - default: - return fmt.Errorf("unknown service user role mode: %q", r.Mode) - } - - statements, err := postgres.CreateUserRole(postgres.UserRoleOptions{ - Name: r.Username, - Password: r.Password, - Attributes: []string{"LOGIN"}, - Roles: []string{groupRole}, - }) - if err != nil { - return fmt.Errorf("failed to generate create user role statements: %w", err) - } - - if err := statements.Exec(ctx, conn); err != nil { - return fmt.Errorf("failed to create service user: %w", err) - } return nilThis should show the duplicated declarations still sitting inside
createUserRole():#!/bin/bash sed -n '160,237p' server/internal/orchestrator/swarm/service_user_role.go printf '\n---- duplicate declarations inside createUserRole() ----\n' rg -n 'var groupRole string|statements, err :=' server/internal/orchestrator/swarm/service_user_role.goAlso applies to: 211-234
🤖 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 171 - 188, In createUserRole(), stop treating the MCP branch as the default: change the existing else into an explicit else if r.ServiceType == "mcp" and add a final else that returns an error for unknown r.ServiceType; also remove the duplicated declarations by reusing variables instead of redeclaring (e.g., don’t redeclare groupRole or use := for statements/err when those names are already in scope), and ensure the MCP block and the postgrest block each only declare their own unique variables so there are no shadowing/redefinition issues.
🧹 Nitpick comments (1)
server/internal/api/apiv1/validate.go (1)
283-285: Consider updating the error message to reflect the new format.The validation now accepts two-part versions (e.g.,
14.5), but the error message still references only the three-part semver format. A clearer message would help users understand valid inputs.💡 Suggested improvement
// Validate version (semver pattern or "latest") if svc.Version != "latest" && !semverPattern.MatchString(svc.Version) { - err := errors.New("version must be in semver format (e.g., '1.0.0') or 'latest'") + err := errors.New("version must be in 'major.minor' or 'major.minor.patch' format (e.g., '14.5', '1.0.0') or 'latest'") errs = append(errs, newValidationError(err, appendPath(path, "version"))) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/api/apiv1/validate.go` around lines 283 - 285, The error message for version validation is outdated: in validate.go the check using svc.Version and semverPattern appends an error saying "version must be in semver format (e.g., '1.0.0') or 'latest'"; update that message to reflect the accepted two-part or three-part formats (e.g., "'14.5' or '1.0.0'") so users know both are valid. Modify the string passed into errors.New where the validation failure is created (the code that builds err and calls newValidationError with appendPath(path, "version") and appends to errs) to include examples of both two-segment and three-segment version formats and mention 'latest'.
🤖 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/service_user_role.go`:
- Around line 68-71: The Update() implementation for ServiceUserRole must handle
changes to the ServiceType and DBAnonRole fields instead of being a no-op: in
the ServiceUserRole.Update(...) method compare the incoming desired ServiceType
(service_type) and DBAnonRole (db_anon_role) against the current stored values,
and if they differ either (preferred) reconcile by invoking the same idempotent
provisioning logic used by ProvisionServices/ProvisionServiceInstance to update
grants/roles in the authenticator for the existing service user, or (if
reconciliation is not feasible) return a signal that forces recreation of the
service (i.e., mark the resource as requiring replacement and ensure the service
identifier changes when service_type or db_anon_role change). Ensure the chosen
path updates the identifier used for routing/authentication or triggers
teardown-and-recreate so PostgREST won't attempt SET ROLE to an ungranted role.
- Around line 249-252: The Delete() path drops the service role without revoking
the CONNECT privilege that role was granted in roleAttributesAndGrants() (which
grants CONNECT on r.DatabaseName to r.Username and grants anonRole to
r.Username), causing DROP ROLE to fail; before calling DROP ROLE IF EXISTS in
Delete(), execute a REVOKE CONNECT ON DATABASE <r.DatabaseName> FROM
<r.Username> using sanitizeIdentifier for both identifiers via conn.Exec to
remove the privilege, then proceed to DROP ROLE IF EXISTS <r.Username> (keep
using conn.Exec and handle errors as existing code does).
---
Duplicate comments:
In `@server/internal/orchestrator/swarm/service_user_role.go`:
- Around line 171-188: In createUserRole(), stop treating the MCP branch as the
default: change the existing else into an explicit else if r.ServiceType ==
"mcp" and add a final else that returns an error for unknown r.ServiceType; also
remove the duplicated declarations by reusing variables instead of redeclaring
(e.g., don’t redeclare groupRole or use := for statements/err when those names
are already in scope), and ensure the MCP block and the postgrest block each
only declare their own unique variables so there are no shadowing/redefinition
issues.
---
Nitpick comments:
In `@server/internal/api/apiv1/validate.go`:
- Around line 283-285: The error message for version validation is outdated: in
validate.go the check using svc.Version and semverPattern appends an error
saying "version must be in semver format (e.g., '1.0.0') or 'latest'"; update
that message to reflect the accepted two-part or three-part formats (e.g.,
"'14.5' or '1.0.0'") so users know both are valid. Modify the string passed into
errors.New where the validation failure is created (the code that builds err and
calls newValidationError with appendPath(path, "version") and appends to errs)
to include examples of both two-segment and three-segment version formats and
mention 'latest'.
🪄 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: 0ca46920-8b97-48f4-b977-a9da1ef590ee
⛔ Files ignored due to path filters (4)
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 (6)
api/apiv1/design/database.goserver/internal/api/apiv1/validate.goserver/internal/api/apiv1/validate_test.goserver/internal/orchestrator/swarm/service_images.goserver/internal/orchestrator/swarm/service_images_test.goserver/internal/orchestrator/swarm/service_user_role.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/internal/orchestrator/swarm/service_images.go
- server/internal/orchestrator/swarm/service_images_test.go
moizpgedge
force-pushed
the
feat/PLAT-499/PLAT-500/PostgREST-prerequisites/image-wiring
branch
from
4632965 to
4005f53
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
server/internal/database/postgrest_service_config.go (2)
166-189: GenerateConf always returns nil error.The method signature includes an error return but always returns
nil. Consider either:
- Removing the error return if validation is handled upstream
- Adding validation (e.g., empty
DatabaseHosts)Given that callers like
writeConfigFile(context snippet 1) already wrap the error, keeping the signature for future extensibility is reasonable, but documenting this would help.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/postgrest_service_config.go` around lines 166 - 189, GenerateConf currently always returns a nil error; add input validation and return meaningful errors instead of always nil: inside PostgRESTServiceConfig.GenerateConf (which calls buildPostgRESTDBURI and reads fields like DBSchemas, DBAnonRole, DBPool, MaxRows) validate required fields on the receiver and the PostgRESTConnParams (e.g., ensure conn.DatabaseHosts is not empty, DBAnonRole and DBSchemas are non-empty, DBPool > 0, MaxRows >= 0) and return errors describing the missing/invalid field(s) before writing to the buffer; keep the existing signature so callers don’t change, and use the buildPostgRESTDBURI, PostgRESTServiceConfig, and PostgRESTConnParams symbols to locate where to add these checks.
193-212: Handle emptyDatabaseHostsdefensively.If
conn.DatabaseHostsis empty,buildPostgRESTDBURIwill produce an invalid URI likepostgresql://user:pass@/dbname. Consider returning an error or at least documenting the precondition.🛡️ Proposed defensive check
func buildPostgRESTDBURI(conn PostgRESTConnParams) string { + if len(conn.DatabaseHosts) == 0 { + // Caller must ensure at least one host; this is a programming error. + panic("buildPostgRESTDBURI: DatabaseHosts cannot be empty") + } userInfo := url.UserPassword(conn.Username, conn.Password)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/postgrest_service_config.go` around lines 193 - 212, The function buildPostgRESTDBURI should defensively handle empty conn.DatabaseHosts: change its signature to return (string, error), then add a guard at the start that if len(conn.DatabaseHosts) == 0 returns "", fmt.Errorf("PostgRESTConnParams.DatabaseHosts is empty") (or a similarly descriptive error), otherwise proceed as before; update all callers of buildPostgRESTDBURI to handle the returned error. This uses the PostgRESTConnParams and buildPostgRESTDBURI symbols to locate where to add the check and where to propagate the error.server/internal/orchestrator/swarm/service_user_role.go (1)
266-273: Revoking all role memberships may be overly aggressive.The loop revokes any role membership that doesn't match
desiredAnon, but this could revoke legitimate memberships (e.g., if the user was intentionally granted additional roles outside of this system). Consider whether the revocation should be limited to only roles that were previously granted by this resource.🤖 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 266 - 273, The current loop over currentRoles revokes any role not equal to desiredAnon, which may remove legitimate external grants; change the logic in the block that iterates currentRoles to only revoke roles that this resource actually manages (e.g., compute toRevoke := intersection(currentRoles, rolesManagedByResource) or filter by a managed-role naming convention/tag) and then call conn.Exec for each role in toRevoke using the same sanitizeIdentifier(r.Username) and sanitizeIdentifier(role) pattern; locate the loop using identifiers currentRoles, desiredAnon, r.Username, conn.Exec and sanitizeIdentifier and replace the unconditional revoke with the filtered revoke set (or add a check that the role exists in a persisted "granted by this system" list before revoking).
🤖 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/database/postgrest_service_config.go`:
- Around line 166-189: GenerateConf currently always returns a nil error; add
input validation and return meaningful errors instead of always nil: inside
PostgRESTServiceConfig.GenerateConf (which calls buildPostgRESTDBURI and reads
fields like DBSchemas, DBAnonRole, DBPool, MaxRows) validate required fields on
the receiver and the PostgRESTConnParams (e.g., ensure conn.DatabaseHosts is not
empty, DBAnonRole and DBSchemas are non-empty, DBPool > 0, MaxRows >= 0) and
return errors describing the missing/invalid field(s) before writing to the
buffer; keep the existing signature so callers don’t change, and use the
buildPostgRESTDBURI, PostgRESTServiceConfig, and PostgRESTConnParams symbols to
locate where to add these checks.
- Around line 193-212: The function buildPostgRESTDBURI should defensively
handle empty conn.DatabaseHosts: change its signature to return (string, error),
then add a guard at the start that if len(conn.DatabaseHosts) == 0 returns "",
fmt.Errorf("PostgRESTConnParams.DatabaseHosts is empty") (or a similarly
descriptive error), otherwise proceed as before; update all callers of
buildPostgRESTDBURI to handle the returned error. This uses the
PostgRESTConnParams and buildPostgRESTDBURI symbols to locate where to add the
check and where to propagate the error.
In `@server/internal/orchestrator/swarm/service_user_role.go`:
- Around line 266-273: The current loop over currentRoles revokes any role not
equal to desiredAnon, which may remove legitimate external grants; change the
logic in the block that iterates currentRoles to only revoke roles that this
resource actually manages (e.g., compute toRevoke := intersection(currentRoles,
rolesManagedByResource) or filter by a managed-role naming convention/tag) and
then call conn.Exec for each role in toRevoke using the same
sanitizeIdentifier(r.Username) and sanitizeIdentifier(role) pattern; locate the
loop using identifiers currentRoles, desiredAnon, r.Username, conn.Exec and
sanitizeIdentifier and replace the unconditional revoke with the filtered revoke
set (or add a check that the role exists in a persisted "granted by this system"
list before revoking).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5861b72c-4517-47d1-8ffc-58dd6b89cbef
⛔ Files ignored due to path filters (7)
api/apiv1/gen/control_plane/service.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/types.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 (15)
api/apiv1/design/database.goserver/internal/api/apiv1/validate.goserver/internal/api/apiv1/validate_test.goserver/internal/database/postgrest_service_config.goserver/internal/database/postgrest_service_config_test.goserver/internal/orchestrator/swarm/postgrest_config_resource.goserver/internal/orchestrator/swarm/postgrest_preflight_resource.goserver/internal/orchestrator/swarm/resources.goserver/internal/orchestrator/swarm/service_images.goserver/internal/orchestrator/swarm/service_images_test.goserver/internal/orchestrator/swarm/service_instance_spec.goserver/internal/orchestrator/swarm/service_spec.goserver/internal/orchestrator/swarm/service_spec_test.goserver/internal/orchestrator/swarm/service_user_role.goserver/internal/orchestrator/swarm/service_user_role_test.go
✅ Files skipped from review due to trivial changes (3)
- server/internal/api/apiv1/validate.go
- server/internal/api/apiv1/validate_test.go
- server/internal/orchestrator/swarm/postgrest_config_resource.go
🚧 Files skipped from review as they are similar to previous changes (4)
- server/internal/orchestrator/swarm/resources.go
- server/internal/orchestrator/swarm/service_instance_spec.go
- api/apiv1/design/database.go
- server/internal/orchestrator/swarm/postgrest_preflight_resource.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/internal/orchestrator/swarm/service_user_role.go (1)
172-210:⚠️ Potential issue | 🔴 CriticalDo not silently treat unknown
ServiceTypeas MCP.
server/internal/orchestrator/swarm/orchestrator.go, Lines 455-468 and 540-555 still constructServiceUserRolewithoutServiceTypeorDBAnonRole. WithServiceType == "", Line 172 misses the PostgREST branch and this fallback provisions an MCP-style role instead, so PostgREST services never getNOINHERITor the anon-role grant. Please make this an explicitmcpbranch and return an error for unknown/empty types so the wiring bug fails fast.🤖 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 172 - 210, The code currently treats any non-"postgrest" ServiceType as MCP implicitly; change the logic in ServiceUserRole.Create (the block using r.ServiceType and r.roleAttributesAndGrants) to use an explicit switch: case "postgrest" keep current PostgREST flow, case "mcp" keep the group-role/Roles path, and default return an error for unknown/empty ServiceType (include r.ServiceType in the message). Also ensure callers that construct ServiceUserRole (notably in orchestrator.go where instances are created without ServiceType or DBAnonRole) are updated to populate ServiceType and DBAnonRole so PostgREST users get NOINHERIT and anon-role grants instead of silently falling into the MCP branch.
🧹 Nitpick comments (1)
server/internal/database/postgrest_service_config.go (1)
56-57: Extract the default anon role into a shared constant.
"pgedge_application_read_only"is defaulted here and again inserver/internal/orchestrator/swarm/service_user_role.go, Lines 221-223 and 253-255. Pulling it into one shared constant will keep config parsing and role reconciliation aligned if the default ever changes.♻️ Sketch
+const DefaultPostgRESTAnonRole = "pgedge_application_read_only" + // db_anon_role — optional string, default "pgedge_application_read_only" - dbAnonRole := "pgedge_application_read_only" + dbAnonRole := DefaultPostgRESTAnonRole🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/postgrest_service_config.go` around lines 56 - 57, Extract the hard-coded string "pgedge_application_read_only" into a single exported constant (e.g., DefaultDBAnonRole) in a shared package (such as server/internal/database or server/internal/config) and replace the local literal usage in postgrest_service_config.go (dbAnonRole) and in server/internal/orchestrator/swarm/service_user_role.go (the defaults at the two locations around lines 221–223 and 253–255) to reference that constant; ensure the constant name is exported if files are in different packages and update any comments to reference the constant instead of duplicating the literal.
🤖 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/service_user_role.go`:
- Around line 258-266: The revoke+regrant path must be executed inside a single
DB transaction to avoid transient loss of anon-role membership; update the
sequence in service_user_role.go so that the call to r.revokeStaleAnonRoles(ctx,
conn, desiredAnon) and the subsequent re-application via grants.Exec(ctx, conn)
(from roleAttributesAndGrants()) are performed within one transaction (begin,
run both operations, commit/rollback on error), ensuring atomicity and that
failures during grants.Exec roll back the revoke; apply the same transactional
change to the analogous block around lines 272-288.
---
Duplicate comments:
In `@server/internal/orchestrator/swarm/service_user_role.go`:
- Around line 172-210: The code currently treats any non-"postgrest" ServiceType
as MCP implicitly; change the logic in ServiceUserRole.Create (the block using
r.ServiceType and r.roleAttributesAndGrants) to use an explicit switch: case
"postgrest" keep current PostgREST flow, case "mcp" keep the group-role/Roles
path, and default return an error for unknown/empty ServiceType (include
r.ServiceType in the message). Also ensure callers that construct
ServiceUserRole (notably in orchestrator.go where instances are created without
ServiceType or DBAnonRole) are updated to populate ServiceType and DBAnonRole so
PostgREST users get NOINHERIT and anon-role grants instead of silently falling
into the MCP branch.
---
Nitpick comments:
In `@server/internal/database/postgrest_service_config.go`:
- Around line 56-57: Extract the hard-coded string
"pgedge_application_read_only" into a single exported constant (e.g.,
DefaultDBAnonRole) in a shared package (such as server/internal/database or
server/internal/config) and replace the local literal usage in
postgrest_service_config.go (dbAnonRole) and in
server/internal/orchestrator/swarm/service_user_role.go (the defaults at the two
locations around lines 221–223 and 253–255) to reference that constant; ensure
the constant name is exported if files are in different packages and update any
comments to reference the constant instead of duplicating the literal.
🪄 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: f6141142-b0c0-4dd8-9ba2-12fc0a137747
📒 Files selected for processing (3)
server/internal/database/postgrest_service_config.goserver/internal/orchestrator/swarm/postgrest_preflight_resource.goserver/internal/orchestrator/swarm/service_user_role.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/service_spec.go`:
- Around line 155-156: The switch that returns swarm.ServiceSpec currently
rejects any ServiceType other than "mcp" and "postgrest" (default returns error
using opts.ServiceSpec.ServiceType) but upstream validation still allows "rag",
causing successful validation but provisioning failure; fix by either (A) adding
a dedicated "rag" case in this switch that constructs and returns the correct
swarm.ServiceSpec for "rag" (follow the same pattern as the "mcp"/"postgrest"
cases and reference opts.ServiceSpec.ServiceType to branch), or (B) remove "rag"
from the accepted types in the upstream validator
(server/internal/api/apiv1/validate.go) so it never reaches this function—choose
one consistent approach and update the switch or the validator accordingly.
🪄 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: 163b1feb-a2fa-4a33-818a-2367471b422a
📒 Files selected for processing (3)
server/internal/orchestrator/swarm/postgrest_config_resource.goserver/internal/orchestrator/swarm/service_spec.goserver/internal/orchestrator/swarm/service_user_role.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/internal/orchestrator/swarm/service_user_role.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
server/internal/orchestrator/swarm/postgrest_authenticator_resource.go (2)
196-215: Confirm that revoking all non-desired memberships is intentional.
revokeStaleAnonRolesrevokes all role memberships that differ fromdesiredAnon, not just previously-managed anon roles. If this authenticator user ever receives additional grants from other sources (manual intervention, other resources), they will be silently revoked on the next Update cycle.If this aggressive reconciliation is by design for PostgREST's security model (authenticator should only have the single anon role), consider adding a brief inline comment to clarify this intent for future maintainers.
🤖 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 196 - 215, revokeStaleAnonRoles currently revokes every role on the user that does not equal desiredAnon (in function revokeStaleAnonRoles), which will remove grants applied by other actors; either narrow the revocation to only roles you manage (e.g., filter currentRoles by a managed-role naming pattern or by consulting a managed-role list/source before calling REVOKE) or, if the aggressive behavior is intentional, add a concise inline comment above revokeStaleAnonRoles stating that the authenticator must only ever have the single anon role and that revoking all other roles is deliberate for PostgREST security, referencing the function name revokeStaleAnonRoles to make intent explicit for future maintainers.
70-75: Silent default fallback may mask deserialization issues.When the resource is deserialized from etcd/JSON and
DBAnonRoleis missing, this helper silently falls back to the default. Combined with the context snippet showingRegisterResourceTypeuses standard JSON unmarshaling, a serialization mismatch could cause per-node authenticators to silently use a different anon role than intended.This matches the default in
server/internal/database/postgrest_service_config.go:56-67, so the fallback is correct. However, consider adding a debug/trace log when the fallback is triggered to aid troubleshooting configuration drift.🤖 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 70 - 75, The helper desiredAnonRole should emit a debug/trace log when it uses the hardcoded fallback so configuration drift is observable; modify PostgRESTAuthenticatorResource.desiredAnonRole to check r.DBAnonRole == "" and, before returning "pgedge_application_read_only", log a concise debug/trace message including the resource identity and that DBAnonRole was missing, using the resource's existing logger field (e.g., r.logger or equivalent); if the resource lacks a logger, add a lightweight logger field and use it for this single debug message.
🤖 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/orchestrator.go`:
- Around line 596-608: The code re-parses the PostgREST config and discards
errors in the per-node loop (ParsePostgRESTServiceConfig called with `_`), which
can yield an empty DBAnonRole and cause mismatched anon roles between primary
and per-node authenticators; instead hoist and reuse the successfully parsed
postgrestConfig from the earlier parse (the result used to set the primary
authenticator) so PostgRESTAuthenticatorResource initialization uses that same
postgrestConfig.DBAnonRole, and remove the second ParsePostgRESTServiceConfig
call (or check and propagate its error) so desiredAnonRole() can't silently fall
back to the default; update the code paths constructing
PostgRESTAuthenticatorResource and any uses of spec.ServiceSpec.Config to
reference the hoisted postgrestConfig.
In `@server/internal/orchestrator/swarm/postgrest_authenticator_resource.go`:
- Around line 99-101: The current fmt.Errorf call uses two %w verbs which
prevents unwrapping the inner error; change it to join the sentinel and the
original error (errors.Join(resource.ErrNotFound, err)) and wrap that single
joined error with one %w so both resource.ErrNotFound and the original err are
discoverable; i.e., replace the double-%w return with a single-wrapped
errors.Join result in the code around r.authenticatorUsername() where the error
is constructed.
---
Nitpick comments:
In `@server/internal/orchestrator/swarm/postgrest_authenticator_resource.go`:
- Around line 196-215: revokeStaleAnonRoles currently revokes every role on the
user that does not equal desiredAnon (in function revokeStaleAnonRoles), which
will remove grants applied by other actors; either narrow the revocation to only
roles you manage (e.g., filter currentRoles by a managed-role naming pattern or
by consulting a managed-role list/source before calling REVOKE) or, if the
aggressive behavior is intentional, add a concise inline comment above
revokeStaleAnonRoles stating that the authenticator must only ever have the
single anon role and that revoking all other roles is deliberate for PostgREST
security, referencing the function name revokeStaleAnonRoles to make intent
explicit for future maintainers.
- Around line 70-75: The helper desiredAnonRole should emit a debug/trace log
when it uses the hardcoded fallback so configuration drift is observable; modify
PostgRESTAuthenticatorResource.desiredAnonRole to check r.DBAnonRole == "" and,
before returning "pgedge_application_read_only", log a concise debug/trace
message including the resource identity and that DBAnonRole was missing, using
the resource's existing logger field (e.g., r.logger or equivalent); if the
resource lacks a logger, add a lightweight logger field and use it for this
single debug message.
🪄 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: 73e61511-5e90-4161-acce-a7b0fbb793b1
📒 Files selected for processing (7)
server/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/postgrest_authenticator_resource.goserver/internal/orchestrator/swarm/postgrest_config_resource.goserver/internal/orchestrator/swarm/resources.goserver/internal/orchestrator/swarm/service_instance_spec.goserver/internal/orchestrator/swarm/service_user_role.goserver/internal/orchestrator/swarm/service_user_role_test.go
✅ Files skipped from review due to trivial changes (1)
- server/internal/orchestrator/swarm/service_user_role_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- server/internal/orchestrator/swarm/resources.go
- server/internal/orchestrator/swarm/service_instance_spec.go
- server/internal/orchestrator/swarm/service_user_role.go
- server/internal/orchestrator/swarm/postgrest_config_resource.go
rshoemaker
reviewed
Apr 2, 2026
|
|
||
| // revokeStaleAnonRoles removes any role memberships on username that differ | ||
| // from desiredAnon. Must be called within a transaction for atomicity. | ||
| func (r *PostgRESTAuthenticatorResource) revokeStaleAnonRoles(ctx context.Context, conn postgres.Executor, username, desiredAnon string) error { |
Contributor
There was a problem hiding this comment.
Claude flagged a logic bug here:
revokeStaleAnonRoles also revokes the group role from ServiceUserRole
The query at line 200 finds ALL role memberships for the username. ServiceUserRole grants
pgedge_application (RW group role) to this user. Then revokeStaleAnonRoles will revoke any
membership that isn't desiredAnon. So on Update, it will revoke pgedge_application because
it's not the anon role — breaking the base group role membership that ServiceUserRole set up.
This is a real bug. The revoke query needs to exclude the base group roles, something like:
if current != desiredAnon && current != "pgedge_application" && current != "pgedge_application_read_only" {
Or better, track which roles this resource granted and only manage those.
rshoemaker
reviewed
Apr 3, 2026
| return resource.PrimaryExecutor(r.NodeName) | ||
| } | ||
|
|
||
| func (r *PostgRESTPreflightResource) Dependencies() []resource.Identifier { |
Contributor
There was a problem hiding this comment.
@jason-lynch do you think this resource should depend on r.NodeName? With nil here, it seems like this resource could be created before the database nodes are ready.
Member
There was a problem hiding this comment.
Good call-out! I think this should depend on database.PostgresDatabaseResourceIdentifier(r.NodeName, r.DatabaseName) because it needs to connect to that specific database.
rshoemaker
approved these changes
Apr 3, 2026
Contributor
rshoemaker
left a comment
rshoemaker
left a comment
There was a problem hiding this comment.
Looks good - thanks Moiz!
I dropped a question for Jason about the preflight resource dependency, so keep an eye out for his response. You can change it in the next PR if he suggests a diff there.
Move credential handling out of container env vars into postgrest.conf via db-uri. Environment variables are visible to all users on the host via docker inspect and /proc; the config file is restricted to the service user (mode 0600). Move config file generation to PostgRESTServiceConfig.GenerateConf in the database package, where it belongs alongside the type it serializes. PostgRESTConnParams carries the runtime connection details (host, port, credentials) separately from the user-supplied PostgRESTServiceConfig. Fix merge conflict resolution in service_user_role.go: remove the duplicate MCP code block that was left in and drop DBOwner: false to align with the upstream change in main. Implement Update() for PostgREST ServiceUserRole to reconcile DBAnonRole changes at runtime. Queries pg_auth_members for stale role memberships, revokes them, and re-applies the desired grants idempotently. Without this, a DBAnonRole change would leave the authenticator role unable to SET ROLE to the new anon role. Add REVOKE CONNECT ON DATABASE before DROP ROLE in Delete() for PostgREST service users. PostgreSQL refuses to drop a role that holds database privileges, causing the DROP to fail silently. Revoking first ensures clean deletion.
- Extract `checkSchemas` helper from `PostgRESTPreflightResource.validate` to bring cyclomatic complexity within the limit of 8. - Extract `revokeStaleAnonRoles` helper from `reconcilePostgRESTGrants` for the same reason. - Change `buildPostgRESTDBURI` to return `(string, error)` and guard against empty `DatabaseHosts`, preventing a silently malformed db-uri in `postgrest.conf`.
- Wrap anon-role revoke + regrant in a transaction in `reconcilePostgRESTGrants` so a failed regrant rolls back the revoke, preventing transient loss of anon-role membership. - Change `revokeStaleAnonRoles` to accept `postgres.Executor` instead of `*pgx.Conn` so it can be called with a transaction. - Extract shared health check timing into named constants to remove duplication between PostgREST and MCP container specs. - Collapse identical `PostgRESTConfigResource.Update` into `Create`. - Add `#nosec G201` suppressions for SQL injection false positives where `sanitizeIdentifier` already quotes all dynamic identifiers.
Introduce `PostgRESTAuthenticatorResource` — a dedicated resource that handles all PostgREST-specific Postgres role configuration (NOINHERIT, GRANT CONNECT, GRANT <anon_role>) on top of the generic `ServiceUserRole`. This keeps `ServiceUserRole` as a clean shared path for all service types. - `ServiceUserRole`: remove `ServiceType`/`DBAnonRole` fields and all PostgREST-specific logic (`roleAttributesAndGrants`, `reconcilePostgRESTGrants`, `revokeStaleAnonRoles`). `Update` is now a no-op; both removed fields are added to `DiffIgnore` for state migration. - `PostgRESTAuthenticatorResource` (new): owns ALTER ROLE NOINHERIT, GRANT CONNECT, and GRANT <anon_role>. Update reconciles the anon role in a single transaction to prevent transient loss of membership. - `PostgRESTConfigResource`: add `ServiceUserRoleRW` dependency so `populateCredentials` can read the RW role from state at Create time; add `/username` and `/password` toDiffIgnore`. - `orchestrator.go`: replace the `!= "mcp"` guard with a switch that generates the correct resource chain for both `"mcp"` and `"postgrest"`. PostgREST chain: preflight → authenticator → dataDir → configResource. - `service_instance_spec.go`: `Dependencies()` is now service-type-aware (PostgREST → `PostgRESTConfigResource`, MCP → `MCPConfigResource`); `populateCredentials` sets role label to `postgrest_authenticator` for PostgREST and `pgedge_application_read_only` for MCP; unknown types log a warning in `Dependencies()`. - `resources.go`: register `PostgRESTAuthenticatorResource`. PLAT-499, PLAT-500, PLAT-501, PLAT-502, PLAT-503
`revokeStaleAnonRoles` queried all role memberships for the authenticator user, which included `pgedge_application` and `pgedge_application_read_only` granted by `ServiceUserRole`. On Update, any role not matching the desired anon role was revoked — silently breaking the base group role membership. Fix by excluding the two known base group roles in the SQL query so only anon role candidates (user-supplied custom roles) are considered for revocation.
- Hoist parsed PostgREST config outside the service type switch so the per-node authenticator loop reuses the already-validated result instead of re-parsing and silently discarding errors, which could cause per-node authenticators to use a different anon role than the primary. - Fix double `%w` in `PostgRESTAuthenticatorResource.Refresh` — replace with `errors.Join` so both `resource.ErrNotFound` and the original error are independently unwrappable via `errors.Is`/`errors.As`.
moizpgedge
force-pushed
the
feat/PLAT-499/PLAT-500/PostgREST-prerequisites/image-wiring
branch
from
bd06bab to
2932f64
Compare
jason-lynch
deleted the
feat/PLAT-499/PLAT-500/PostgREST-prerequisites/image-wiring
branch
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
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.
Implements PostgREST as a deployable service type in the Control Plane, covering image registry, schema/role preflight validation, container spec, config file generation, and service user provisioning.
PLAT-499
PLAT-500
PLAT-501
PLAT-502
PLAT-503
Summary
A minimal deployment requires only:
"config": {}Optional fields:
Changes
Deploy PostgREST service (once storefront database exists)
Checklist
Notes for Reviewers