feat: wire PostgREST into the orchestrator (PLAT-504, PLAT-505, PLAT-506, PLAT-507)#329
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 50 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request adds comprehensive PostgREST service provisioning and management support, including end-to-end tests, database dependency tracking, service naming updates using SHA256 hashing, role-based credential selection, primary-database connection targeting, and documentation covering deployment, configuration, and usage patterns. Changes
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
TIP This summary will be updated as you push new changes. Give us feedback
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
e2e/postgrest_test.go (2)
36-45: Minor: node naming logic may produce unexpected results for 2-node clusters.When
len(nodeHosts) == 2, the slice indexing at line 43 will work (["n1", "n2", "n3"][1]= "n2"), but the first node at index 0 will haveName: "n1"from line 39 (set before the conditional). This is correct but the logic flow is slightly confusing. Consider restructuring for clarity if this helper is used elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/postgrest_test.go` around lines 36 - 45, The node naming logic in the nodes creation loop mixes an initial default Name assignment with a conditional override, which is confusing for 2-node clusters; update the loop that fills nodes (the nodes slice of *controlplane.DatabaseNodeSpec) to determine the name list first (e.g., names := []string{"n1","n2","n3"} truncated to len(nodeHosts)) and then assign Name from that list for each index i, instead of setting Name to "n1" first and conditionally overriding it, so DatabaseNodeSpec.Name is consistently assigned in one place.
489-512: Loop only checks single node despite comment implying all nodes.The comment at line 488 says "confirm service roles exist on all nodes" but the loop iterates only over
[]string{"n1"}. If checking multiple nodes was intended, update the loop. If single-node check is intentional, consider updating the comment to match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/postgrest_test.go` around lines 489 - 512, The test's loop over nodes uses a hardcoded slice []string{"n1"} but the preceding comment says "confirm service roles exist on all nodes"; update the slice used in the for loop (the []string literal referenced by nodeName) to include all intended node names (e.g., "n1","n2", etc.) so the check runs per-node, or alternatively change the comment to accurately state that the check is only for node "n1" if a single-node check was intended; the change should be made where the for _, nodeName := range []string{"n1"} { loop is declared.server/internal/orchestrator/swarm/orchestrator_test.go (1)
7-62: Well-structured property-based tests for the new naming scheme.The test correctly validates the core contract properties:
- Docker Swarm's 63-character limit
- Determinism (same inputs → same output)
- Coverage for PostgREST service type and long service type truncation
One optional enhancement: consider validating that the generated name conforms to Docker Swarm's character constraints (lowercase alphanumeric and hyphens, no leading/trailing hyphens). This would catch any future implementation changes that accidentally produce invalid service names.
🧪 Optional: Add character validation helper
+import ( + "regexp" + "testing" +) + +// dockerSwarmNamePattern matches valid Docker Swarm service names. +var dockerSwarmNamePattern = regexp.MustCompile(`^[a-z0-9][a-z0-9-]*[a-z0-9]$|^[a-z0-9]$`) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got := ServiceInstanceName(tt.serviceType, tt.databaseID, tt.serviceID, tt.hostID) // Must be within Docker Swarm's 63-char limit. if len(got) > 63 { t.Errorf("ServiceInstanceName() = %q (len %d), must be <= 63 chars", got, len(got)) } + // Must be a valid Docker Swarm service name. + if !dockerSwarmNamePattern.MatchString(got) { + t.Errorf("ServiceInstanceName() = %q is not a valid Docker Swarm service name", got) + } + // Must be deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/swarm/orchestrator_test.go` around lines 7 - 62, Add a character-validation assertion to the TestServiceInstanceName tests: after computing got (from ServiceInstanceName(serviceType, databaseID, serviceID, hostID)), verify the name is all lowercase and matches Docker Swarm's allowed pattern (only lowercase letters, digits and hyphens, no leading or trailing hyphen) — e.g. validate against the regex for valid service names and assert the match; reference the ServiceInstanceName function when locating where to add this check.
🤖 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 159-174: The ServiceInstanceName change will orphan existing MCP
services; update the lookup/migration so existing deployments are found or
cleaned up: add a legacy-name fallback and/or one-time migration when resolving
services by name in ServiceInstance.Sync (and where generateMCPInstanceResources
calls ServiceInstanceName for "mcp"/"postgrest"): implement a fallback that
computes the old name format (the previous
`{serviceType}-{databaseID}-{serviceID}-{sha1(hostID)[:8]}` scheme) and try
ServiceInspectByLabels/ServiceInspectByName using that legacy name before
declaring a missing service, or add a migration/cleanup routine that enumerates
services (using labels like pgedge.service) and renames/removes or re-labels
them to the new ServiceInstanceName format.
In `@server/internal/orchestrator/swarm/service_instance_spec.go`:
- Around line 85-96: The PostgREST branch sets the local variable role to an
invalid value ("postgrest_authenticator"); update the PostgREST case in the
block that computes mode and role (where ServiceUserRoleRO/ServiceUserRoleRW and
s.ServiceSpec.ServiceType are used) so that role is "pgedge_application" for the
RW path instead of "postgrest_authenticator", ensuring ServiceUser.Role matches
the allowed values used by ServiceUserRole.createUserRole() and
ServiceUserRoleIdentifier.
---
Nitpick comments:
In `@e2e/postgrest_test.go`:
- Around line 36-45: The node naming logic in the nodes creation loop mixes an
initial default Name assignment with a conditional override, which is confusing
for 2-node clusters; update the loop that fills nodes (the nodes slice of
*controlplane.DatabaseNodeSpec) to determine the name list first (e.g., names :=
[]string{"n1","n2","n3"} truncated to len(nodeHosts)) and then assign Name from
that list for each index i, instead of setting Name to "n1" first and
conditionally overriding it, so DatabaseNodeSpec.Name is consistently assigned
in one place.
- Around line 489-512: The test's loop over nodes uses a hardcoded slice
[]string{"n1"} but the preceding comment says "confirm service roles exist on
all nodes"; update the slice used in the for loop (the []string literal
referenced by nodeName) to include all intended node names (e.g., "n1","n2",
etc.) so the check runs per-node, or alternatively change the comment to
accurately state that the check is only for node "n1" if a single-node check was
intended; the change should be made where the for _, nodeName := range
[]string{"n1"} { loop is declared.
In `@server/internal/orchestrator/swarm/orchestrator_test.go`:
- Around line 7-62: Add a character-validation assertion to the
TestServiceInstanceName tests: after computing got (from
ServiceInstanceName(serviceType, databaseID, serviceID, hostID)), verify the
name is all lowercase and matches Docker Swarm's allowed pattern (only lowercase
letters, digits and hyphens, no leading or trailing hyphen) — e.g. validate
against the regex for valid service names and assert the match; reference the
ServiceInstanceName function when locating where to add this check.
🪄 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: 21e8d5c6-3612-49e7-8d13-b8d90a236122
📒 Files selected for processing (9)
docs/Supported-services/Postgrest-docs.mde2e/postgrest_test.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/orchestrator_test.goserver/internal/orchestrator/swarm/postgrest_preflight_resource.goserver/internal/orchestrator/swarm/service_images.goserver/internal/orchestrator/swarm/service_instance_spec.goserver/internal/orchestrator/swarm/service_spec.goserver/internal/workflows/plan_update.go
| // databaseID and serviceID are often UUIDs or long strings, all four components | ||
| // are hashed together into a single 16-char base-36 suffix. The serviceType | ||
| // prefix is kept for readability and is truncated to 10 chars to fit comfortably. | ||
| func ServiceInstanceName(serviceType, databaseID, serviceID, hostID string) string { |
There was a problem hiding this comment.
What's the reason for this change? This might break existing services, and it deviates from the database instance service name implementation.
There was a problem hiding this comment.
This handles the case where databaseID and serviceID are UUIDs Since services are discovered by label not name, the rename just triggers a brief restart on next reconciliation rather than orphaning anything. Happy to revert if you prefer keeping it closer to the existing pattern.
There was a problem hiding this comment.
Yes, please revert this.
Host IDs and Database IDs need to be globally unique, whereas service IDs only need to be unique within a database, so it's much less likely that someone will use a UUID for a service ID. The previous implementation was human-readable and matched the same scheme as database instances, which is very handy when you're trying to troubleshoot.
There was a problem hiding this comment.
If you're looking to shorten these IDs: do we need to include serviceType?
The reason that the Postgres service includes the word postgres is that it allows for other database services that do not have a unique identifier. We don't have the same problem with services because they already have a unique identifier.
moizpgedge
force-pushed
the
feat/PLAT-504-505-506-507/Wire-PostgREST-into-the-orchestrator
branch
from
37da3dd to
a661851
Compare
…506, PLAT-507) - Route target_session_attrs=read-write for postgrest in plan_update - PostgREST container env contains only PGRST_* vars; connection details live in postgrest.conf db-uri - Register "latest" image version for postgrest - populateCredentials reads from _rw role for postgrest (authenticator role) - Guard DataDirID usage so non-data-dir services don't break - Fix ServiceInstanceName to hash variable inputs into a 16-char base36 suffix, staying well under Docker Swarm's 63-char limit - Add PostgRESTPreflightResource dependency on PostgresDatabaseResourceIdentifier - Add docs/Supported-services/Postgrest-docs.md - Add e2e/postgrest_test.go covering provisioning, lifecycle, JWT, failover, and role verification
…2E tests - Use crypto/sha256 instead of crypto/sha1 to satisfy static analysis - Preflight E2E tests now create the DB first, then update with the bad config — asserting the update task fails rather than expecting NewDatabaseFixture itself to fail
Drop serviceType from ServiceInstanceName — serviceID is already
unique within a database, so the type prefix adds no value. The
output now follows the same {databaseID}-{serviceID}-{8charHash}
scheme as InstanceIDFor, hashing only hostID for readability.
Also resolves rebase conflict in plan_update.go: keeps both the
rag and postgrest cases in resolveTargetSessionAttrs.
PLAT-504, PLAT-505, PLAT-506, PLAT-507
moizpgedge
force-pushed
the
feat/PLAT-504-505-506-507/Wire-PostgREST-into-the-orchestrator
branch
from
a661851 to
d7ede13
Compare
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/orchestrator.go`:
- Around line 170-173: ServiceInstanceName can produce names longer than Docker
Swarm's 63-char limit; change it to enforce a max total length of 63 by
reserving 10 chars for two separators and the 8-char hash (allowedSum = 53) and
truncating databaseID and serviceID so len(databaseID)+len(serviceID) <=
allowedSum (e.g., if sum > allowedSum, truncate databaseID to at most
allowedSum/2 and then truncate serviceID to allowedSum - len(databaseID) so the
final fmt.Sprintf("%s-%s-%s", databaseID, serviceID, base36[:8]) is <=63), and
add a test case in orchestrator_test.go with maximum-length identifiers (63-char
databaseID and 63-char serviceID) to assert the produced name length is <=63.
🪄 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: 6444cf09-7dfd-4d46-9c05-a4803c93072e
📒 Files selected for processing (9)
docs/Supported-services/Postgrest-docs.mde2e/postgrest_test.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/orchestrator_test.goserver/internal/orchestrator/swarm/postgrest_preflight_resource.goserver/internal/orchestrator/swarm/service_images.goserver/internal/orchestrator/swarm/service_instance_spec.goserver/internal/orchestrator/swarm/service_spec.goserver/internal/workflows/plan_update.go
✅ Files skipped from review due to trivial changes (5)
- server/internal/orchestrator/swarm/service_images.go
- server/internal/orchestrator/swarm/postgrest_preflight_resource.go
- server/internal/orchestrator/swarm/service_spec.go
- docs/Supported-services/Postgrest-docs.md
- e2e/postgrest_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/internal/orchestrator/swarm/service_instance_spec.go
- server/internal/orchestrator/swarm/orchestrator_test.go
Fix stale etcd license URL caught by licenses-ci. PLAT-504
databaseID and serviceID can each be up to 63 chars. The previous implementation could produce names up to 136 chars, exceeding Docker Swarm's limit. Now caps the combined ID budget to 53 chars (giving databaseID up to 26, serviceID the rest), keeping the total ≤ 63. Adds three test cases covering max-length inputs.
Add postgrest.md covering configuration reference, JWT auth, preflight checks, multi-host failover, and schema cache reload. Remove the old draft Postgrest-docs.md. Update index.md to link PostgREST as a live service type and managing.md to include a PostgREST provisioning example. PLAT-504, PLAT-505, PLAT-506, PLAT-507
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/services/postgrest.md (1)
157-159: Normalize code block style to satisfy MD046.markdownlint flags these blocks as fenced where indented style is expected (Lines 157, 166, 204, 232).
Also applies to: 166-168, 204-206, 232-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/services/postgrest.md` around lines 157 - 159, Replace the fenced code blocks shown (for example the block containing the text "http://{host}:{port}/{table_or_view}") with indented code block style to satisfy MD046; locate each occurrence of those fenced triple-backtick blocks (the ones at the examples matching that URL snippet and the other fenced blocks flagged) and convert them to standard indented code blocks (prefix each line with four spaces) so markdownlint no longer reports fenced blocks where indented style is expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/services/postgrest.md`:
- Around line 201-206: The GRANT example uses the wrong target role; update the
SQL to grant the role named in the `role` JWT claim to the PostgREST
authenticator role (postgrest_authenticator) instead of
pgedge_application_read_only so JWT role switching works; locate the example
around the `GRANT app TO pgedge_application_read_only;` snippet and change it to
grant the claimed role to postgrest_authenticator (mentioning the `role` claim
and `postgrest_authenticator`) so the doc shows the correct grant for PostgREST
auth switching.
In `@server/internal/orchestrator/swarm/orchestrator.go`:
- Around line 171-191: The current name-generation uses
sha256.Sum256([]byte(hostID)) (variable base36) but truncates
databaseID/serviceID later, which can cause deterministic collisions; change the
hash input to include hostID, databaseID and serviceID (e.g., concatenate with a
separator) before computing sha256 and converting to base36 so the final
base36[:8] reflects all three identifiers; keep the existing truncation logic
for databaseID/serviceID and the final fmt.Sprintf("%s-%s-%s", databaseID,
serviceID, base36[:8]) format.
---
Nitpick comments:
In `@docs/services/postgrest.md`:
- Around line 157-159: Replace the fenced code blocks shown (for example the
block containing the text "http://{host}:{port}/{table_or_view}") with indented
code block style to satisfy MD046; locate each occurrence of those fenced
triple-backtick blocks (the ones at the examples matching that URL snippet and
the other fenced blocks flagged) and convert them to standard indented code
blocks (prefix each line with four spaces) so markdownlint no longer reports
fenced blocks where indented style is expected.
🪄 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: 7efea2f0-27a1-450a-8d0e-161012e2aca4
📒 Files selected for processing (5)
docs/services/index.mddocs/services/managing.mddocs/services/postgrest.mdserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/orchestrator_test.go
✅ Files skipped from review due to trivial changes (2)
- docs/services/managing.md
- docs/services/index.md
🚧 Files skipped from review as they are similar to previous changes (1)
- server/internal/orchestrator/swarm/orchestrator_test.go
docs/services/postgrest.md
Outdated
| The `role` claim must name a PostgreSQL role granted to the PostgREST | ||
| authenticator. Grant the role before sending authenticated requests: | ||
|
|
||
| ```sql | ||
| GRANT app TO pgedge_application_read_only; | ||
| ``` |
There was a problem hiding this comment.
Role grant example targets the wrong PostgREST role.
Line 205 grants to pgedge_application_read_only, but PostgREST auth switching is wired through the authenticator role (postgrest_authenticator). Following this doc as-is can make JWT role switching fail.
Suggested doc fix
-GRANT app TO pgedge_application_read_only;
+GRANT app TO postgrest_authenticator;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The `role` claim must name a PostgreSQL role granted to the PostgREST | |
| authenticator. Grant the role before sending authenticated requests: | |
| ```sql | |
| GRANT app TO pgedge_application_read_only; | |
| ``` | |
| The `role` claim must name a PostgreSQL role granted to the PostgREST | |
| authenticator. Grant the role before sending authenticated requests: | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 204-204: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/services/postgrest.md` around lines 201 - 206, The GRANT example uses
the wrong target role; update the SQL to grant the role named in the `role` JWT
claim to the PostgREST authenticator role (postgrest_authenticator) instead of
pgedge_application_read_only so JWT role switching works; locate the example
around the `GRANT app TO pgedge_application_read_only;` snippet and change it to
grant the claimed role to postgrest_authenticator (mentioning the `role` claim
and `postgrest_authenticator`) so the doc shows the correct grant for PostgREST
auth switching.
Include databaseID, serviceID, and hostID in the hash so truncated prefixes never collide for different services on the same host. Fix the JWT role grant example in postgrest.md — the authenticator username is auto-managed, so remove the wrong SQL and direct users to the database status response for the correct username. PLAT-504, PLAT-507
| // different databases or services on the same host. serviceType is omitted because | ||
| // serviceID is already unique within a database. | ||
| func ServiceInstanceName(databaseID, serviceID, hostID string) string { | ||
| hash := sha256.Sum256([]byte(databaseID + ":" + serviceID + ":" + hostID)) |
There was a problem hiding this comment.
Could you please change this back to:
hash := sha1.Sum([]byte(hostID))That will make this hash match the scheme we use for instances. Sometimes security tools alert about using sha1 because they want to make sure you're not using it for cryptographic purposes. This is not a cryptographic use case, so it's fine to ignore the alert.
There was a problem hiding this comment.
My main issue with changing this scheme is that it makes services and database instances different. I understand you're addressing the service name length issue. Could you please open a dedicated ticket for it and not address it in this PR? That way, we can ensure service names remain consistent across both services and database instances.
2 participants
Summary
Wires PostgREST into the orchestrator as a fully functional service type: connection routing, container spec, credential role selection, service name collision fix, preflight dependency, docs, and E2E tests.
Changes
plan_update.go: routetarget_session_attrs=read-writefor postgrest so libpq always connects to the primaryservice_spec.go: PostgREST container env is exactly 3PGRST_*vars — connection details live inpostgrest.confdb-uri onlyservice_images.go: register"latest"image version for postgrestservice_instance_spec.go:populateCredentialsreads from_rwrole for postgrest (authenticator); guardDataDirIDso services without a data dir don't breakorchestrator.go: fixServiceInstanceName— hash all variable inputs into a 16-char base36 suffix to stay under Docker Swarm's 63-char limitpostgrest_preflight_resource.go: addPostgresDatabaseResourceIdentifieras a dependency so preflight runs after the DB is readydocs/Supported-services/Postgrest-docs.md: operator runbook — DBA grants, config reference, deploy/remove commandse2e/postgrest_test.go: 11 E2E tests covering provisioning, JWT auth, preflight failures, role verification, config updates, multi-host, and failoverTesting
Checklist
Notes for Reviewers