feat: wire PostgREST into the orchestrator (PLAT-504, PLAT-505, PLAT-506, PLAT-507)#329
Conversation
…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
|
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 59 minutes and 6 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 (4)
📝 WalkthroughWalkthroughIntroduces PostgREST service support to the Control Plane platform, including comprehensive documentation on deployment and operation, end-to-end test coverage for service provisioning and lifecycle management, updates to service instance naming using sha256 hashing, and orchestrator modifications to handle PostgREST-specific configuration, role management, and primary-write routing. 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 |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
TIP This summary will be updated as you push new changes. Give us feedback
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…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
|
@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
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