From 4e00f2cd48f4e0195e59240320697c2af53fd8f4 Mon Sep 17 00:00:00 2001 From: moizpgedge Date: Mon, 30 Mar 2026 21:37:13 +0500 Subject: [PATCH 1/7] feat: add PostgREST as a service type (PLAT-499/500/501/502/503) --- api/apiv1/design/database.go | 6 +- api/apiv1/gen/control_plane/service.go | 3 +- .../gen/http/control_plane/client/types.go | 13 +- .../gen/http/control_plane/server/types.go | 13 +- api/apiv1/gen/http/openapi.json | 4 +- api/apiv1/gen/http/openapi.yaml | 4 +- api/apiv1/gen/http/openapi3.json | 28 +-- api/apiv1/gen/http/openapi3.yaml | 28 +-- server/internal/api/apiv1/validate.go | 2 +- server/internal/api/apiv1/validate_test.go | 10 + .../orchestrator/swarm/postgrest_config.go | 47 +++++ .../swarm/postgrest_config_resource.go | 132 +++++++++++++ .../swarm/postgrest_config_test.go | 186 ++++++++++++++++++ .../swarm/postgrest_preflight_resource.go | 134 +++++++++++++ .../internal/orchestrator/swarm/resources.go | 2 + .../orchestrator/swarm/service_images.go | 8 + .../orchestrator/swarm/service_images_test.go | 65 ++++-- .../swarm/service_instance_spec.go | 28 +-- .../orchestrator/swarm/service_spec.go | 129 +++++++++--- .../orchestrator/swarm/service_spec_test.go | 120 +++++++++++ .../orchestrator/swarm/service_user_role.go | 82 +++++--- .../swarm/service_user_role_test.go | 98 +++++++++ 22 files changed, 1011 insertions(+), 131 deletions(-) create mode 100644 server/internal/orchestrator/swarm/postgrest_config.go create mode 100644 server/internal/orchestrator/swarm/postgrest_config_resource.go create mode 100644 server/internal/orchestrator/swarm/postgrest_config_test.go create mode 100644 server/internal/orchestrator/swarm/postgrest_preflight_resource.go diff --git a/api/apiv1/design/database.go b/api/apiv1/design/database.go index 69493e3e..60799ada 100644 --- a/api/apiv1/design/database.go +++ b/api/apiv1/design/database.go @@ -9,7 +9,7 @@ const ( cpuPattern = `^[0-9]+(\.[0-9]{1,3}|m)?$` postgresVersionPattern = `^\d{2}\.\d{1,2}$` spockVersionPattern = `^\d{1}$` - serviceVersionPattern = `^(\d+\.\d+\.\d+|latest)$` + serviceVersionPattern = `^(\d+\.\d+(\.\d+)?|latest)$` ) var HostIDs = g.ArrayOf(Identifier, func() { @@ -166,10 +166,10 @@ var ServiceSpec = g.Type("ServiceSpec", func() { g.Meta("struct:tag:json", "service_type") }) g.Attribute("version", g.String, func() { - g.Description("The version of the service in semver format (e.g., '1.0.0') or the literal 'latest'.") + g.Description("The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'.") g.Pattern(serviceVersionPattern) g.Example("1.0.0") - g.Example("1.2.3") + g.Example("14.5") g.Example("latest") g.Meta("struct:tag:json", "version") }) diff --git a/api/apiv1/gen/control_plane/service.go b/api/apiv1/gen/control_plane/service.go index 6da4a0a7..bdab95dd 100644 --- a/api/apiv1/gen/control_plane/service.go +++ b/api/apiv1/gen/control_plane/service.go @@ -969,8 +969,7 @@ type ServiceSpec struct { ServiceID Identifier `json:"service_id"` // The type of service to run. ServiceType string `json:"service_type"` - // The version of the service in semver format (e.g., '1.0.0') or the literal - // 'latest'. + // The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'. Version string `json:"version"` // The IDs of the hosts that should run this service. One service instance will // be created per host. diff --git a/api/apiv1/gen/http/control_plane/client/types.go b/api/apiv1/gen/http/control_plane/client/types.go index ffa31778..ccfe9b5b 100644 --- a/api/apiv1/gen/http/control_plane/client/types.go +++ b/api/apiv1/gen/http/control_plane/client/types.go @@ -2082,8 +2082,7 @@ type ServiceSpecRequestBody struct { ServiceID string `json:"service_id"` // The type of service to run. ServiceType string `json:"service_type"` - // The version of the service in semver format (e.g., '1.0.0') or the literal - // 'latest'. + // The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'. Version string `json:"version"` // The IDs of the hosts that should run this service. One service instance will // be created per host. @@ -2483,8 +2482,7 @@ type ServiceSpecResponseBody struct { ServiceID *string `json:"service_id"` // The type of service to run. ServiceType *string `json:"service_type"` - // The version of the service in semver format (e.g., '1.0.0') or the literal - // 'latest'. + // The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'. Version *string `json:"version"` // The IDs of the hosts that should run this service. One service instance will // be created per host. @@ -2812,8 +2810,7 @@ type ServiceSpecRequestBodyRequestBody struct { ServiceID string `json:"service_id"` // The type of service to run. ServiceType string `json:"service_type"` - // The version of the service in semver format (e.g., '1.0.0') or the literal - // 'latest'. + // The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'. Version string `json:"version"` // The IDs of the hosts that should run this service. One service instance will // be created per host. @@ -6256,7 +6253,7 @@ func ValidateServiceSpecRequestBody(body *ServiceSpecRequestBody) (err error) { if !(body.ServiceType == "mcp" || body.ServiceType == "postgrest" || body.ServiceType == "rag") { err = goa.MergeErrors(err, goa.InvalidEnumValueError("body.service_type", body.ServiceType, []any{"mcp", "postgrest", "rag"})) } - err = goa.MergeErrors(err, goa.ValidatePattern("body.version", body.Version, "^(\\d+\\.\\d+\\.\\d+|latest)$")) + err = goa.MergeErrors(err, goa.ValidatePattern("body.version", body.Version, "^(\\d+\\.\\d+(\\.\\d+)?|latest)$")) if len(body.HostIds) < 1 { err = goa.MergeErrors(err, goa.InvalidLengthError("body.host_ids", body.HostIds, len(body.HostIds), 1, true)) } @@ -7020,7 +7017,7 @@ func ValidateServiceSpecRequestBodyRequestBody(body *ServiceSpecRequestBodyReque if !(body.ServiceType == "mcp" || body.ServiceType == "postgrest" || body.ServiceType == "rag") { err = goa.MergeErrors(err, goa.InvalidEnumValueError("body.service_type", body.ServiceType, []any{"mcp", "postgrest", "rag"})) } - err = goa.MergeErrors(err, goa.ValidatePattern("body.version", body.Version, "^(\\d+\\.\\d+\\.\\d+|latest)$")) + err = goa.MergeErrors(err, goa.ValidatePattern("body.version", body.Version, "^(\\d+\\.\\d+(\\.\\d+)?|latest)$")) if len(body.HostIds) < 1 { err = goa.MergeErrors(err, goa.InvalidLengthError("body.host_ids", body.HostIds, len(body.HostIds), 1, true)) } diff --git a/api/apiv1/gen/http/control_plane/server/types.go b/api/apiv1/gen/http/control_plane/server/types.go index 1f3fd0b6..e8f68d2e 100644 --- a/api/apiv1/gen/http/control_plane/server/types.go +++ b/api/apiv1/gen/http/control_plane/server/types.go @@ -2166,8 +2166,7 @@ type ServiceSpecResponseBody struct { ServiceID string `json:"service_id"` // The type of service to run. ServiceType string `json:"service_type"` - // The version of the service in semver format (e.g., '1.0.0') or the literal - // 'latest'. + // The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'. Version string `json:"version"` // The IDs of the hosts that should run this service. One service instance will // be created per host. @@ -2494,8 +2493,7 @@ type ServiceSpecRequestBody struct { ServiceID *string `json:"service_id"` // The type of service to run. ServiceType *string `json:"service_type"` - // The version of the service in semver format (e.g., '1.0.0') or the literal - // 'latest'. + // The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'. Version *string `json:"version"` // The IDs of the hosts that should run this service. One service instance will // be created per host. @@ -2822,8 +2820,7 @@ type ServiceSpecRequestBodyRequestBody struct { ServiceID *string `json:"service_id"` // The type of service to run. ServiceType *string `json:"service_type"` - // The version of the service in semver format (e.g., '1.0.0') or the literal - // 'latest'. + // The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'. Version *string `json:"version"` // The IDs of the hosts that should run this service. One service instance will // be created per host. @@ -5766,7 +5763,7 @@ func ValidateServiceSpecRequestBody(body *ServiceSpecRequestBody) (err error) { } } if body.Version != nil { - err = goa.MergeErrors(err, goa.ValidatePattern("body.version", *body.Version, "^(\\d+\\.\\d+\\.\\d+|latest)$")) + err = goa.MergeErrors(err, goa.ValidatePattern("body.version", *body.Version, "^(\\d+\\.\\d+(\\.\\d+)?|latest)$")) } if len(body.HostIds) < 1 { err = goa.MergeErrors(err, goa.InvalidLengthError("body.host_ids", body.HostIds, len(body.HostIds), 1, true)) @@ -6508,7 +6505,7 @@ func ValidateServiceSpecRequestBodyRequestBody(body *ServiceSpecRequestBodyReque } } if body.Version != nil { - err = goa.MergeErrors(err, goa.ValidatePattern("body.version", *body.Version, "^(\\d+\\.\\d+\\.\\d+|latest)$")) + err = goa.MergeErrors(err, goa.ValidatePattern("body.version", *body.Version, "^(\\d+\\.\\d+(\\.\\d+)?|latest)$")) } if len(body.HostIds) < 1 { err = goa.MergeErrors(err, goa.InvalidLengthError("body.host_ids", body.HostIds, len(body.HostIds), 1, true)) diff --git a/api/apiv1/gen/http/openapi.json b/api/apiv1/gen/http/openapi.json index 03b837ba..9c7508a0 100644 --- a/api/apiv1/gen/http/openapi.json +++ b/api/apiv1/gen/http/openapi.json @@ -8807,9 +8807,9 @@ }, "version": { "type": "string", - "description": "The version of the service in semver format (e.g., '1.0.0') or the literal 'latest'.", + "description": "The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'.", "example": "latest", - "pattern": "^(\\d+\\.\\d+\\.\\d+|latest)$" + "pattern": "^(\\d+\\.\\d+(\\.\\d+)?|latest)$" } }, "example": { diff --git a/api/apiv1/gen/http/openapi.yaml b/api/apiv1/gen/http/openapi.yaml index 2f12e29d..b17cd5d4 100644 --- a/api/apiv1/gen/http/openapi.yaml +++ b/api/apiv1/gen/http/openapi.yaml @@ -6326,9 +6326,9 @@ definitions: - rag version: type: string - description: The version of the service in semver format (e.g., '1.0.0') or the literal 'latest'. + description: The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'. example: latest - pattern: ^(\d+\.\d+\.\d+|latest)$ + pattern: ^(\d+\.\d+(\.\d+)?|latest)$ example: config: llm_model: gpt-4 diff --git a/api/apiv1/gen/http/openapi3.json b/api/apiv1/gen/http/openapi3.json index 490f519c..5446d989 100644 --- a/api/apiv1/gen/http/openapi3.json +++ b/api/apiv1/gen/http/openapi3.json @@ -28072,9 +28072,9 @@ }, "version": { "type": "string", - "description": "The version of the service in semver format (e.g., '1.0.0') or the literal 'latest'.", + "description": "The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'.", "example": "latest", - "pattern": "^(\\d+\\.\\d+\\.\\d+|latest)$" + "pattern": "^(\\d+\\.\\d+(\\.\\d+)?|latest)$" } }, "example": { @@ -28235,9 +28235,9 @@ }, "version": { "type": "string", - "description": "The version of the service in semver format (e.g., '1.0.0') or the literal 'latest'.", + "description": "The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'.", "example": "latest", - "pattern": "^(\\d+\\.\\d+\\.\\d+|latest)$" + "pattern": "^(\\d+\\.\\d+(\\.\\d+)?|latest)$" } }, "example": { @@ -28401,9 +28401,9 @@ }, "version": { "type": "string", - "description": "The version of the service in semver format (e.g., '1.0.0') or the literal 'latest'.", + "description": "The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'.", "example": "latest", - "pattern": "^(\\d+\\.\\d+\\.\\d+|latest)$" + "pattern": "^(\\d+\\.\\d+(\\.\\d+)?|latest)$" } }, "example": { @@ -28565,9 +28565,9 @@ }, "version": { "type": "string", - "description": "The version of the service in semver format (e.g., '1.0.0') or the literal 'latest'.", + "description": "The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'.", "example": "latest", - "pattern": "^(\\d+\\.\\d+\\.\\d+|latest)$" + "pattern": "^(\\d+\\.\\d+(\\.\\d+)?|latest)$" } }, "example": { @@ -28731,9 +28731,9 @@ }, "version": { "type": "string", - "description": "The version of the service in semver format (e.g., '1.0.0') or the literal 'latest'.", + "description": "The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'.", "example": "latest", - "pattern": "^(\\d+\\.\\d+\\.\\d+|latest)$" + "pattern": "^(\\d+\\.\\d+(\\.\\d+)?|latest)$" } }, "example": { @@ -28896,9 +28896,9 @@ }, "version": { "type": "string", - "description": "The version of the service in semver format (e.g., '1.0.0') or the literal 'latest'.", + "description": "The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'.", "example": "latest", - "pattern": "^(\\d+\\.\\d+\\.\\d+|latest)$" + "pattern": "^(\\d+\\.\\d+(\\.\\d+)?|latest)$" } }, "example": { @@ -29061,9 +29061,9 @@ }, "version": { "type": "string", - "description": "The version of the service in semver format (e.g., '1.0.0') or the literal 'latest'.", + "description": "The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'.", "example": "latest", - "pattern": "^(\\d+\\.\\d+\\.\\d+|latest)$" + "pattern": "^(\\d+\\.\\d+(\\.\\d+)?|latest)$" } }, "example": { diff --git a/api/apiv1/gen/http/openapi3.yaml b/api/apiv1/gen/http/openapi3.yaml index dda18e5d..faa924e3 100644 --- a/api/apiv1/gen/http/openapi3.yaml +++ b/api/apiv1/gen/http/openapi3.yaml @@ -19872,9 +19872,9 @@ components: - rag version: type: string - description: The version of the service in semver format (e.g., '1.0.0') or the literal 'latest'. + description: The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'. example: latest - pattern: ^(\d+\.\d+\.\d+|latest)$ + pattern: ^(\d+\.\d+(\.\d+)?|latest)$ example: config: llm_model: gpt-4 @@ -19990,9 +19990,9 @@ components: - rag version: type: string - description: The version of the service in semver format (e.g., '1.0.0') or the literal 'latest'. + description: The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'. example: latest - pattern: ^(\d+\.\d+\.\d+|latest)$ + pattern: ^(\d+\.\d+(\.\d+)?|latest)$ example: config: llm_model: gpt-4 @@ -20111,9 +20111,9 @@ components: - rag version: type: string - description: The version of the service in semver format (e.g., '1.0.0') or the literal 'latest'. + description: The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'. example: latest - pattern: ^(\d+\.\d+\.\d+|latest)$ + pattern: ^(\d+\.\d+(\.\d+)?|latest)$ example: config: llm_model: gpt-4 @@ -20230,9 +20230,9 @@ components: - rag version: type: string - description: The version of the service in semver format (e.g., '1.0.0') or the literal 'latest'. + description: The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'. example: latest - pattern: ^(\d+\.\d+\.\d+|latest)$ + pattern: ^(\d+\.\d+(\.\d+)?|latest)$ example: config: llm_model: gpt-4 @@ -20351,9 +20351,9 @@ components: - rag version: type: string - description: The version of the service in semver format (e.g., '1.0.0') or the literal 'latest'. + description: The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'. example: latest - pattern: ^(\d+\.\d+\.\d+|latest)$ + pattern: ^(\d+\.\d+(\.\d+)?|latest)$ example: config: llm_model: gpt-4 @@ -20471,9 +20471,9 @@ components: - rag version: type: string - description: The version of the service in semver format (e.g., '1.0.0') or the literal 'latest'. + description: The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'. example: latest - pattern: ^(\d+\.\d+\.\d+|latest)$ + pattern: ^(\d+\.\d+(\.\d+)?|latest)$ example: config: llm_model: gpt-4 @@ -20591,9 +20591,9 @@ components: - rag version: type: string - description: The version of the service in semver format (e.g., '1.0.0') or the literal 'latest'. + description: The version of the service (e.g., '1.0.0', '14.5') or the literal 'latest'. example: latest - pattern: ^(\d+\.\d+\.\d+|latest)$ + pattern: ^(\d+\.\d+(\.\d+)?|latest)$ example: config: llm_model: gpt-4 diff --git a/server/internal/api/apiv1/validate.go b/server/internal/api/apiv1/validate.go index fcff19e2..20a3c7d6 100644 --- a/server/internal/api/apiv1/validate.go +++ b/server/internal/api/apiv1/validate.go @@ -641,7 +641,7 @@ func validateS3RepoProperties(props repoProperties, path []string) []error { } var pgBackRestOptionPattern = regexp.MustCompile(`^[a-z0-9-]+$`) -var semverPattern = regexp.MustCompile(`^\d+\.\d+\.\d+$`) +var semverPattern = regexp.MustCompile(`^\d+\.\d+(\.\d+)?$`) // reservedLabelPrefix is the label key prefix reserved for system use. const reservedLabelPrefix = "pgedge." diff --git a/server/internal/api/apiv1/validate_test.go b/server/internal/api/apiv1/validate_test.go index a2603125..e3980c3c 100644 --- a/server/internal/api/apiv1/validate_test.go +++ b/server/internal/api/apiv1/validate_test.go @@ -860,6 +860,16 @@ func TestValidateServiceSpec(t *testing.T) { }, }, }, + { + name: "valid PostgREST service with two-part version", + svc: &api.ServiceSpec{ + ServiceID: "postgrest", + ServiceType: "postgrest", + Version: "14.5", + HostIds: []api.Identifier{"host-1"}, + Config: map[string]any{}, + }, + }, { name: "valid MCP service with 'latest' version", svc: &api.ServiceSpec{ diff --git a/server/internal/orchestrator/swarm/postgrest_config.go b/server/internal/orchestrator/swarm/postgrest_config.go new file mode 100644 index 00000000..d92a917a --- /dev/null +++ b/server/internal/orchestrator/swarm/postgrest_config.go @@ -0,0 +1,47 @@ +package swarm + +import ( + "bytes" + "fmt" + + "github.com/pgEdge/control-plane/server/internal/database" +) + +// PostgRESTConfigParams holds all inputs needed to generate a postgrest.conf file. +type PostgRESTConfigParams struct { + Config *database.PostgRESTServiceConfig +} + +// 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) { + if params == nil { + return nil, fmt.Errorf("GeneratePostgRESTConfig: params must not be nil") + } + if params.Config == nil { + return nil, fmt.Errorf("GeneratePostgRESTConfig: params.Config must not be nil") + } + cfg := params.Config + + var buf bytes.Buffer + + fmt.Fprintf(&buf, "db-schemas = %q\n", cfg.DBSchemas) + fmt.Fprintf(&buf, "db-anon-role = %q\n", cfg.DBAnonRole) + fmt.Fprintf(&buf, "db-pool = %d\n", cfg.DBPool) + fmt.Fprintf(&buf, "db-max-rows = %d\n", cfg.MaxRows) + + if cfg.JWTSecret != nil { + fmt.Fprintf(&buf, "jwt-secret = %q\n", *cfg.JWTSecret) + } + if cfg.JWTAud != nil { + fmt.Fprintf(&buf, "jwt-aud = %q\n", *cfg.JWTAud) + } + if cfg.JWTRoleClaimKey != nil { + fmt.Fprintf(&buf, "jwt-role-claim-key = %q\n", *cfg.JWTRoleClaimKey) + } + if cfg.ServerCORSAllowedOrigins != nil { + fmt.Fprintf(&buf, "server-cors-allowed-origins = %q\n", *cfg.ServerCORSAllowedOrigins) + } + + return buf.Bytes(), nil +} diff --git a/server/internal/orchestrator/swarm/postgrest_config_resource.go b/server/internal/orchestrator/swarm/postgrest_config_resource.go new file mode 100644 index 00000000..cebb8e0b --- /dev/null +++ b/server/internal/orchestrator/swarm/postgrest_config_resource.go @@ -0,0 +1,132 @@ +package swarm + +import ( + "context" + "fmt" + "path/filepath" + + "github.com/samber/do" + "github.com/spf13/afero" + + "github.com/pgEdge/control-plane/server/internal/database" + "github.com/pgEdge/control-plane/server/internal/filesystem" + "github.com/pgEdge/control-plane/server/internal/resource" +) + +var _ resource.Resource = (*PostgRESTConfigResource)(nil) + +const ResourceTypePostgRESTConfig resource.Type = "swarm.postgrest_config" + +func PostgRESTConfigResourceIdentifier(serviceInstanceID string) resource.Identifier { + return resource.Identifier{ + ID: serviceInstanceID, + Type: ResourceTypePostgRESTConfig, + } +} + +// PostgRESTConfigResource manages the postgrest.conf file on the host filesystem. +// The file is bind-mounted read-only into the container; credentials are not included. +type PostgRESTConfigResource struct { + ServiceInstanceID string `json:"service_instance_id"` + ServiceID string `json:"service_id"` + HostID string `json:"host_id"` + DirResourceID string `json:"dir_resource_id"` + Config *database.PostgRESTServiceConfig `json:"config"` +} + +func (r *PostgRESTConfigResource) ResourceVersion() string { + return "1" +} + +func (r *PostgRESTConfigResource) DiffIgnore() []string { + return nil +} + +func (r *PostgRESTConfigResource) Identifier() resource.Identifier { + return PostgRESTConfigResourceIdentifier(r.ServiceInstanceID) +} + +func (r *PostgRESTConfigResource) Executor() resource.Executor { + return resource.HostExecutor(r.HostID) +} + +func (r *PostgRESTConfigResource) Dependencies() []resource.Identifier { + return []resource.Identifier{ + filesystem.DirResourceIdentifier(r.DirResourceID), + } +} + +func (r *PostgRESTConfigResource) TypeDependencies() []resource.Type { + return nil +} + +func (r *PostgRESTConfigResource) Refresh(ctx context.Context, rc *resource.Context) error { + fs, err := do.Invoke[afero.Fs](rc.Injector) + if err != nil { + return err + } + + dirPath, err := filesystem.DirResourceFullPath(rc, r.DirResourceID) + if err != nil { + return fmt.Errorf("failed to get service data dir path: %w", err) + } + + _, err = readResourceFile(fs, filepath.Join(dirPath, "postgrest.conf")) + if err != nil { + return fmt.Errorf("failed to read PostgREST config: %w", err) + } + + return nil +} + +func (r *PostgRESTConfigResource) Create(ctx context.Context, rc *resource.Context) error { + fs, err := do.Invoke[afero.Fs](rc.Injector) + if err != nil { + return err + } + + dirPath, err := filesystem.DirResourceFullPath(rc, r.DirResourceID) + if err != nil { + return fmt.Errorf("failed to get service data dir path: %w", err) + } + + return r.writeConfigFile(fs, dirPath) +} + +func (r *PostgRESTConfigResource) Update(ctx context.Context, rc *resource.Context) error { + fs, err := do.Invoke[afero.Fs](rc.Injector) + if err != nil { + return err + } + + dirPath, err := filesystem.DirResourceFullPath(rc, r.DirResourceID) + if err != nil { + return fmt.Errorf("failed to get service data dir path: %w", err) + } + + return r.writeConfigFile(fs, dirPath) +} + +func (r *PostgRESTConfigResource) Delete(ctx context.Context, rc *resource.Context) error { + // Cleanup is handled by the parent directory resource deletion. + return nil +} + +func (r *PostgRESTConfigResource) writeConfigFile(fs afero.Fs, dirPath string) error { + content, err := GeneratePostgRESTConfig(&PostgRESTConfigParams{ + Config: r.Config, + }) + if err != nil { + return fmt.Errorf("failed to generate PostgREST config: %w", err) + } + + configPath := filepath.Join(dirPath, "postgrest.conf") + if err := afero.WriteFile(fs, configPath, content, 0o600); err != nil { + return fmt.Errorf("failed to write %s: %w", configPath, err) + } + if err := fs.Chown(configPath, postgrestContainerUID, postgrestContainerUID); err != nil { + return fmt.Errorf("failed to change ownership for %s: %w", configPath, err) + } + + return nil +} diff --git a/server/internal/orchestrator/swarm/postgrest_config_test.go b/server/internal/orchestrator/swarm/postgrest_config_test.go new file mode 100644 index 00000000..9582d0ba --- /dev/null +++ b/server/internal/orchestrator/swarm/postgrest_config_test.go @@ -0,0 +1,186 @@ +package swarm + +import ( + "strings" + "testing" + + "github.com/pgEdge/control-plane/server/internal/database" +) + +// parseConf parses the key=value lines from a postgrest.conf into a map. +// String values are returned unquoted; numeric values are returned as-is. +func parseConf(t *testing.T, data []byte) map[string]string { + t.Helper() + m := make(map[string]string) + for _, line := range strings.Split(string(data), "\n") { + line = strings.TrimSpace(line) + if line == "" { + continue + } + parts := strings.SplitN(line, " = ", 2) + if len(parts) != 2 { + t.Fatalf("unexpected line in postgrest.conf: %q", line) + } + key := strings.TrimSpace(parts[0]) + val := strings.TrimSpace(parts[1]) + // Strip surrounding quotes from string values. + if strings.HasPrefix(val, `"`) && strings.HasSuffix(val, `"`) { + val = val[1 : len(val)-1] + } + m[key] = val + } + return m +} + +func TestGeneratePostgRESTConfig_Defaults(t *testing.T) { + params := &PostgRESTConfigParams{ + Config: &database.PostgRESTServiceConfig{ + DBSchemas: "public", + DBAnonRole: "pgedge_application_read_only", + DBPool: 10, + MaxRows: 1000, + }, + } + + data, err := GeneratePostgRESTConfig(params) + if err != nil { + t.Fatalf("GeneratePostgRESTConfig() error = %v", err) + } + + m := parseConf(t, data) + + if m["db-schemas"] != "public" { + t.Errorf("db-schemas = %q, want %q", m["db-schemas"], "public") + } + if m["db-anon-role"] != "pgedge_application_read_only" { + t.Errorf("db-anon-role = %q, want %q", m["db-anon-role"], "pgedge_application_read_only") + } + if m["db-pool"] != "10" { + t.Errorf("db-pool = %q, want %q", m["db-pool"], "10") + } + if m["db-max-rows"] != "1000" { + t.Errorf("db-max-rows = %q, want %q", m["db-max-rows"], "1000") + } +} + +func TestGeneratePostgRESTConfig_CustomCoreFields(t *testing.T) { + params := &PostgRESTConfigParams{ + Config: &database.PostgRESTServiceConfig{ + DBSchemas: "api,private", + DBAnonRole: "web_anon", + DBPool: 5, + MaxRows: 500, + }, + } + + data, err := GeneratePostgRESTConfig(params) + if err != nil { + t.Fatalf("GeneratePostgRESTConfig() error = %v", err) + } + + m := parseConf(t, data) + + if m["db-schemas"] != "api,private" { + t.Errorf("db-schemas = %q, want %q", m["db-schemas"], "api,private") + } + if m["db-anon-role"] != "web_anon" { + t.Errorf("db-anon-role = %q, want %q", m["db-anon-role"], "web_anon") + } + if m["db-pool"] != "5" { + t.Errorf("db-pool = %q, want %q", m["db-pool"], "5") + } + if m["db-max-rows"] != "500" { + t.Errorf("db-max-rows = %q, want %q", m["db-max-rows"], "500") + } +} + +func TestGeneratePostgRESTConfig_JWTFieldsAbsent(t *testing.T) { + // No JWT fields set — none should appear in the config file. + params := &PostgRESTConfigParams{ + Config: &database.PostgRESTServiceConfig{ + DBSchemas: "public", + DBAnonRole: "web_anon", + DBPool: 10, + MaxRows: 1000, + }, + } + + data, err := GeneratePostgRESTConfig(params) + if err != nil { + t.Fatalf("GeneratePostgRESTConfig() error = %v", err) + } + + m := parseConf(t, data) + + for _, key := range []string{"jwt-secret", "jwt-aud", "jwt-role-claim-key", "server-cors-allowed-origins"} { + if _, ok := m[key]; ok { + t.Errorf("%s should be absent when not configured, but it was present", key) + } + } +} + +func TestGeneratePostgRESTConfig_AllJWTFields(t *testing.T) { + secret := "a-very-long-jwt-secret-that-is-at-least-32-chars" + aud := "my-api-audience" + roleClaimKey := ".role" + corsOrigins := "https://example.com" + + params := &PostgRESTConfigParams{ + Config: &database.PostgRESTServiceConfig{ + DBSchemas: "public", + DBAnonRole: "web_anon", + DBPool: 10, + MaxRows: 1000, + JWTSecret: &secret, + JWTAud: &aud, + JWTRoleClaimKey: &roleClaimKey, + ServerCORSAllowedOrigins: &corsOrigins, + }, + } + + data, err := GeneratePostgRESTConfig(params) + if err != nil { + t.Fatalf("GeneratePostgRESTConfig() error = %v", err) + } + + m := parseConf(t, data) + + if m["jwt-secret"] != secret { + t.Errorf("jwt-secret = %q, want %q", m["jwt-secret"], secret) + } + if m["jwt-aud"] != aud { + t.Errorf("jwt-aud = %q, want %q", m["jwt-aud"], aud) + } + if m["jwt-role-claim-key"] != roleClaimKey { + t.Errorf("jwt-role-claim-key = %q, want %q", m["jwt-role-claim-key"], roleClaimKey) + } + if m["server-cors-allowed-origins"] != corsOrigins { + t.Errorf("server-cors-allowed-origins = %q, want %q", m["server-cors-allowed-origins"], corsOrigins) + } +} + +func TestGeneratePostgRESTConfig_CredentialsNotInFile(t *testing.T) { + // Verify that no credential-like keys ever appear in the config file. + secret := "a-very-long-jwt-secret-that-is-at-least-32-chars" + params := &PostgRESTConfigParams{ + Config: &database.PostgRESTServiceConfig{ + DBSchemas: "public", + DBAnonRole: "web_anon", + DBPool: 10, + MaxRows: 1000, + JWTSecret: &secret, + }, + } + + data, err := GeneratePostgRESTConfig(params) + if err != nil { + t.Fatalf("GeneratePostgRESTConfig() error = %v", err) + } + + // None of the libpq / db-uri credential keys should appear. + for _, forbidden := range []string{"db-uri", "PGUSER", "PGPASSWORD", "PGHOST", "PGPORT", "PGDATABASE"} { + if strings.Contains(string(data), forbidden) { + t.Errorf("config file must not contain %q (credentials are env vars)", forbidden) + } + } +} diff --git a/server/internal/orchestrator/swarm/postgrest_preflight_resource.go b/server/internal/orchestrator/swarm/postgrest_preflight_resource.go new file mode 100644 index 00000000..55b5a756 --- /dev/null +++ b/server/internal/orchestrator/swarm/postgrest_preflight_resource.go @@ -0,0 +1,134 @@ +package swarm + +import ( + "context" + "errors" + "fmt" + "strings" + + "github.com/pgEdge/control-plane/server/internal/database" + "github.com/pgEdge/control-plane/server/internal/resource" +) + +var _ resource.Resource = (*PostgRESTPreflightResource)(nil) + +const ResourceTypePostgRESTPreflightResource resource.Type = "swarm.postgrest_preflight" + +func PostgRESTPreflightResourceIdentifier(serviceID string) resource.Identifier { + return resource.Identifier{ + ID: serviceID, + Type: ResourceTypePostgRESTPreflightResource, + } +} + +// PostgRESTPreflightResource validates that the configured schemas and anon role +// exist in the database before PostgREST is provisioned. It uses PrimaryExecutor +// so the check runs on a host with guaranteed database connectivity. +type PostgRESTPreflightResource struct { + ServiceID string `json:"service_id"` + DatabaseID string `json:"database_id"` + DatabaseName string `json:"database_name"` + NodeName string `json:"node_name"` + DBSchemas string `json:"db_schemas"` + DBAnonRole string `json:"db_anon_role"` +} + +func (r *PostgRESTPreflightResource) ResourceVersion() string { return "1" } +func (r *PostgRESTPreflightResource) DiffIgnore() []string { return nil } + +func (r *PostgRESTPreflightResource) Identifier() resource.Identifier { + return PostgRESTPreflightResourceIdentifier(r.ServiceID) +} + +func (r *PostgRESTPreflightResource) Executor() resource.Executor { + return resource.PrimaryExecutor(r.NodeName) +} + +func (r *PostgRESTPreflightResource) Dependencies() []resource.Identifier { + return nil +} + +func (r *PostgRESTPreflightResource) TypeDependencies() []resource.Type { + return nil +} + +// Refresh validates prerequisites and returns ErrNotFound only when validation +// fails, triggering a Create that surfaces the error. When prerequisites are +// 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()) + } + return nil +} + +func (r *PostgRESTPreflightResource) Create(ctx context.Context, rc *resource.Context) error { + return r.validate(ctx, rc) +} + +func (r *PostgRESTPreflightResource) Update(ctx context.Context, rc *resource.Context) error { + return r.validate(ctx, rc) +} + +func (r *PostgRESTPreflightResource) Delete(ctx context.Context, rc *resource.Context) error { + return nil +} + +func (r *PostgRESTPreflightResource) validate(ctx context.Context, rc *resource.Context) error { + primary, err := database.GetPrimaryInstance(ctx, rc, r.NodeName) + if err != nil { + return fmt.Errorf("preflight: failed to get primary instance: %w", err) + } + conn, err := primary.Connection(ctx, rc, r.DatabaseName) + if err != nil { + return fmt.Errorf("preflight: failed to connect to database %s on node %s: %w", r.DatabaseName, r.NodeName, err) + } + defer conn.Close(ctx) + + var errs []error + + for _, schema := range splitSchemas(r.DBSchemas) { + var exists bool + if err := conn.QueryRow(ctx, + "SELECT EXISTS (SELECT 1 FROM information_schema.schemata WHERE schema_name = $1)", + schema, + ).Scan(&exists); err != nil { + errs = append(errs, fmt.Errorf("failed to check schema %q: %w", schema, err)) + continue + } + if !exists { + errs = append(errs, fmt.Errorf( + "schema %q does not exist in database %q; create it before deploying PostgREST", + schema, r.DatabaseName, + )) + } + } + + if r.DBAnonRole != "" { + var exists bool + if err := conn.QueryRow(ctx, + "SELECT EXISTS (SELECT 1 FROM pg_catalog.pg_roles WHERE rolname = $1)", + r.DBAnonRole, + ).Scan(&exists); err != nil { + errs = append(errs, fmt.Errorf("failed to check role %q: %w", r.DBAnonRole, err)) + } else if !exists { + errs = append(errs, fmt.Errorf( + "role %q does not exist on the Postgres cluster; create it before deploying PostgREST", + r.DBAnonRole, + )) + } + } + + return errors.Join(errs...) +} + +func splitSchemas(s string) []string { + parts := strings.Split(s, ",") + schemas := make([]string, 0, len(parts)) + for _, p := range parts { + if p = strings.TrimSpace(p); p != "" { + schemas = append(schemas, p) + } + } + return schemas +} diff --git a/server/internal/orchestrator/swarm/resources.go b/server/internal/orchestrator/swarm/resources.go index 4878137f..e688039e 100644 --- a/server/internal/orchestrator/swarm/resources.go +++ b/server/internal/orchestrator/swarm/resources.go @@ -21,4 +21,6 @@ func RegisterResourceTypes(registry *resource.Registry) { resource.RegisterResourceType[*Switchover](registry, ResourceTypeSwitchover) resource.RegisterResourceType[*ScaleService](registry, ResourceTypeScaleService) resource.RegisterResourceType[*MCPConfigResource](registry, ResourceTypeMCPConfig) + resource.RegisterResourceType[*PostgRESTPreflightResource](registry, ResourceTypePostgRESTPreflightResource) + resource.RegisterResourceType[*PostgRESTConfigResource](registry, ResourceTypePostgRESTConfig) } diff --git a/server/internal/orchestrator/swarm/service_images.go b/server/internal/orchestrator/swarm/service_images.go index d97f4a7f..514eb9fa 100644 --- a/server/internal/orchestrator/swarm/service_images.go +++ b/server/internal/orchestrator/swarm/service_images.go @@ -49,6 +49,14 @@ func NewServiceVersions(cfg config.Config) *ServiceVersions { // No constraints — MCP works with all PG/Spock versions. }) + // PostgREST service versions. + // Images are published to the pgEdge registry under ghcr.io/pgedge/postgrest. + // The bare ref (no registry prefix) lets serviceImageTag prepend the + // configured ImageRepositoryHost (e.g. ghcr.io/pgedge). + versions.addServiceImage("postgrest", "14.5", &ServiceImage{ + Tag: serviceImageTag(cfg, "postgrest:14.5"), + }) + // RAG service versions // TODO: Register semver versions when official releases are published. versions.addServiceImage("rag", "latest", &ServiceImage{ diff --git a/server/internal/orchestrator/swarm/service_images_test.go b/server/internal/orchestrator/swarm/service_images_test.go index e9cd9a9c..1cf40cad 100644 --- a/server/internal/orchestrator/swarm/service_images_test.go +++ b/server/internal/orchestrator/swarm/service_images_test.go @@ -1,6 +1,7 @@ package swarm import ( + "slices" "testing" "github.com/pgEdge/control-plane/server/internal/config" @@ -29,6 +30,13 @@ func TestGetServiceImage(t *testing.T) { wantTag: "ghcr.io/pgedge/postgres-mcp:latest", wantErr: false, }, + { + name: "valid postgrest 14.5", + serviceType: "postgrest", + version: "14.5", + wantTag: "ghcr.io/pgedge/postgrest:14.5", + wantErr: false, + }, { name: "unsupported service type", serviceType: "unknown", @@ -81,21 +89,29 @@ func TestSupportedServiceVersions(t *testing.T) { sv := NewServiceVersions(cfg) tests := []struct { - name string - serviceType string - wantLen int - wantErr bool + name string + serviceType string + wantLatest bool // whether "latest" must be present + minPinnedCount int // minimum number of pinned (non-"latest") versions required + wantErr bool }{ { - name: "mcp service has versions", - serviceType: "mcp", - wantLen: 1, // "latest" - wantErr: false, + name: "mcp service has versions", + serviceType: "mcp", + wantLatest: true, + minPinnedCount: 0, + wantErr: false, + }, + { + name: "postgrest service has versions", + serviceType: "postgrest", + wantLatest: false, + minPinnedCount: 1, // at least one pinned release (e.g. 14.5 or newer) + wantErr: false, }, { name: "unsupported service type", serviceType: "unknown", - wantLen: 0, wantErr: true, }, } @@ -107,8 +123,19 @@ func TestSupportedServiceVersions(t *testing.T) { t.Errorf("SupportedServiceVersions() error = %v, wantErr %v", err, tt.wantErr) return } - if len(got) != tt.wantLen { - t.Errorf("SupportedServiceVersions() returned %d versions, want %d", len(got), tt.wantLen) + if !tt.wantErr { + if tt.wantLatest && !slices.Contains(got, "latest") { + t.Errorf("SupportedServiceVersions() missing required version \"latest\", got %v", got) + } + pinned := 0 + for _, v := range got { + if v != "latest" { + pinned++ + } + } + if pinned < tt.minPinnedCount { + t.Errorf("SupportedServiceVersions() has %d pinned version(s), want at least %d", pinned, tt.minPinnedCount) + } } }) } @@ -183,6 +210,22 @@ func TestGetServiceImage_ConstraintsPopulated(t *testing.T) { } }) + t.Run("postgrest has no constraints", func(t *testing.T) { + img, err := sv.GetServiceImage("postgrest", "14.5") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if img.Tag != "ghcr.io/pgedge/postgrest:14.5" { + t.Errorf("expected ghcr.io/pgedge/postgrest:14.5, got %s", img.Tag) + } + if img.PostgresConstraint != nil { + t.Error("expected nil PostgresConstraint for postgrest") + } + if img.SpockConstraint != nil { + t.Error("expected nil SpockConstraint for postgrest") + } + }) + } func mustVersion(t *testing.T, s string) *ds.Version { diff --git a/server/internal/orchestrator/swarm/service_instance_spec.go b/server/internal/orchestrator/swarm/service_instance_spec.go index f613bac1..8514c8a6 100644 --- a/server/internal/orchestrator/swarm/service_instance_spec.go +++ b/server/internal/orchestrator/swarm/service_instance_spec.go @@ -104,19 +104,21 @@ func (s *ServiceInstanceSpecResource) Refresh(ctx context.Context, rc *resource. } spec, err := ServiceContainerSpec(&ServiceContainerSpecOptions{ - ServiceSpec: s.ServiceSpec, - ServiceInstanceID: s.ServiceInstanceID, - DatabaseID: s.DatabaseID, - DatabaseName: s.DatabaseName, - HostID: s.HostID, - ServiceName: s.ServiceName, - Hostname: s.Hostname, - CohortMemberID: s.CohortMemberID, - ServiceImage: s.ServiceImage, - Credentials: s.Credentials, - DatabaseNetworkID: network.NetworkID, - Port: s.Port, - DataPath: dataPath, + ServiceSpec: s.ServiceSpec, + ServiceInstanceID: s.ServiceInstanceID, + DatabaseID: s.DatabaseID, + DatabaseName: s.DatabaseName, + HostID: s.HostID, + ServiceName: s.ServiceName, + Hostname: s.Hostname, + CohortMemberID: s.CohortMemberID, + ServiceImage: s.ServiceImage, + Credentials: s.Credentials, + DatabaseNetworkID: network.NetworkID, + DatabaseHosts: s.DatabaseHosts, + TargetSessionAttrs: s.TargetSessionAttrs, + Port: s.Port, + DataPath: dataPath, }) if err != nil { return fmt.Errorf("failed to generate service container spec: %w", err) diff --git a/server/internal/orchestrator/swarm/service_spec.go b/server/internal/orchestrator/swarm/service_spec.go index f566fa0f..5aba4211 100644 --- a/server/internal/orchestrator/swarm/service_spec.go +++ b/server/internal/orchestrator/swarm/service_spec.go @@ -2,6 +2,8 @@ package swarm import ( "fmt" + "strconv" + "strings" "time" "github.com/docker/docker/api/types/container" @@ -15,19 +17,53 @@ import ( // mcpContainerUID is the UID of the MCP container user. const mcpContainerUID = 1001 +// postgrestContainerUID is the UID of the PostgREST container user. +// See: https://github.com/PostgREST/postgrest/blob/main/Dockerfile (USER 1000) +const postgrestContainerUID = 1000 + +func buildPostgRESTEnvVars(opts *ServiceContainerSpecOptions) []string { + hosts := make([]string, 0, len(opts.DatabaseHosts)) + ports := make([]string, 0, len(opts.DatabaseHosts)) + for _, h := range opts.DatabaseHosts { + hosts = append(hosts, h.Host) + ports = append(ports, strconv.Itoa(h.Port)) + } + env := []string{ + "PGRST_DB_URI=postgresql://", + "PGRST_SERVER_HOST=0.0.0.0", + "PGRST_SERVER_PORT=8080", + "PGRST_ADMIN_SERVER_PORT=3001", + fmt.Sprintf("PGHOST=%s", strings.Join(hosts, ",")), + fmt.Sprintf("PGPORT=%s", strings.Join(ports, ",")), + fmt.Sprintf("PGDATABASE=%s", opts.DatabaseName), + } + if opts.TargetSessionAttrs != "" { + env = append(env, fmt.Sprintf("PGTARGETSESSIONATTRS=%s", opts.TargetSessionAttrs)) + } + if opts.Credentials != nil { + env = append(env, + fmt.Sprintf("PGUSER=%s", opts.Credentials.Username), + fmt.Sprintf("PGPASSWORD=%s", opts.Credentials.Password), + ) + } + return env +} + // ServiceContainerSpecOptions contains all parameters needed to build a service container spec. type ServiceContainerSpecOptions struct { - ServiceSpec *database.ServiceSpec - ServiceInstanceID string - DatabaseID string - DatabaseName string - HostID string - ServiceName string - Hostname string - CohortMemberID string - ServiceImage *ServiceImage - Credentials *database.ServiceUser - DatabaseNetworkID string + ServiceSpec *database.ServiceSpec + ServiceInstanceID string + DatabaseID string + DatabaseName string + HostID string + ServiceName string + Hostname string + CohortMemberID string + ServiceImage *ServiceImage + Credentials *database.ServiceUser + DatabaseNetworkID string + DatabaseHosts []database.ServiceHostEntry // Ordered Postgres host:port entries + TargetSessionAttrs string // libpq target_session_attrs // Service port configuration Port *int // DataPath is the host-side directory path for the bind mount @@ -88,29 +124,64 @@ func ServiceContainerSpec(opts *ServiceContainerSpecOptions) (swarm.ServiceSpec, } } - // Build bind mount for config/auth files - mounts := []mount.Mount{ - docker.BuildMount(opts.DataPath, "/app/data", false), + // Build the container-spec fields that vary by service type. + var ( + command []string + args []string + env []string + user string + healthcheck *container.HealthConfig + mounts []mount.Mount + ) + + switch opts.ServiceSpec.ServiceType { + case "postgrest": + user = fmt.Sprintf("%d", postgrestContainerUID) + command = []string{"postgrest"} + args = []string{"/app/data/postgrest.conf"} + env = buildPostgRESTEnvVars(opts) + // postgrest --ready exits 0/1; no curl in the static binary image. + healthcheck = &container.HealthConfig{ + Test: []string{"CMD", "postgrest", "--ready"}, + StartPeriod: time.Second * 30, + Interval: time.Second * 10, + Timeout: time.Second * 5, + Retries: 3, + } + mounts = []mount.Mount{ + docker.BuildMount(opts.DataPath, "/app/data", true), + } + case "mcp": + user = fmt.Sprintf("%d", mcpContainerUID) + // Override the default container entrypoint to specify config path on bind mount. + command = []string{"/app/pgedge-postgres-mcp"} + args = []string{"-config", "/app/data/config.yaml"} + healthcheck = &container.HealthConfig{ + Test: []string{"CMD-SHELL", "curl -f http://localhost:8080/health || exit 1"}, + StartPeriod: time.Second * 30, + Interval: time.Second * 10, + Timeout: time.Second * 5, + Retries: 3, + } + mounts = []mount.Mount{ + docker.BuildMount(opts.DataPath, "/app/data", false), + } + default: + return swarm.ServiceSpec{}, fmt.Errorf("unsupported service type: %q", opts.ServiceSpec.ServiceType) } return swarm.ServiceSpec{ TaskTemplate: swarm.TaskSpec{ ContainerSpec: &swarm.ContainerSpec{ - Image: image, - Labels: labels, - Hostname: opts.Hostname, - User: fmt.Sprintf("%d", mcpContainerUID), - // override the default container entrypoint so we can specify path to config on bind mount - Command: []string{"/app/pgedge-postgres-mcp"}, - Args: []string{"-config", "/app/data/config.yaml"}, - Healthcheck: &container.HealthConfig{ - Test: []string{"CMD-SHELL", "curl -f http://localhost:8080/health || exit 1"}, - StartPeriod: time.Second * 30, - Interval: time.Second * 10, - Timeout: time.Second * 5, - Retries: 3, - }, - Mounts: mounts, + Image: image, + Labels: labels, + Hostname: opts.Hostname, + User: user, + Command: command, + Args: args, + Env: env, + Healthcheck: healthcheck, + Mounts: mounts, }, Networks: networks, Placement: &swarm.Placement{ diff --git a/server/internal/orchestrator/swarm/service_spec_test.go b/server/internal/orchestrator/swarm/service_spec_test.go index a9d5eac5..49aea7e7 100644 --- a/server/internal/orchestrator/swarm/service_spec_test.go +++ b/server/internal/orchestrator/swarm/service_spec_test.go @@ -2,6 +2,7 @@ package swarm import ( "fmt" + "strings" "testing" "github.com/docker/docker/api/types/swarm" @@ -307,3 +308,122 @@ func TestBuildServicePortConfig(t *testing.T) { func intPtr(i int) *int { return &i } + +// --- PostgREST container spec tests --- + +func makePostgRESTSpecOpts() *ServiceContainerSpecOptions { + return &ServiceContainerSpecOptions{ + ServiceSpec: &database.ServiceSpec{ + ServiceID: "svc-1", + ServiceType: "postgrest", + }, + ServiceInstanceID: "inst-1", + DatabaseID: "db-1", + DatabaseName: "mydb", + HostID: "host-1", + ServiceName: "svc-mydb-postgrest", + Hostname: "postgrest-host1", + CohortMemberID: "node-abc", + ServiceImage: &ServiceImage{Tag: "postgrest/postgrest:latest"}, + Credentials: &database.ServiceUser{ + Username: "svc_postgrest_host1", + Password: "supersecret", + }, + DatabaseNetworkID: "net-1", + DatabaseHosts: []database.ServiceHostEntry{{Host: "pg-host1", Port: 5432}}, + DataPath: "/var/lib/pgedge/services/inst-1", + } +} + +func TestServiceContainerSpec_PostgREST_Command(t *testing.T) { + spec, err := ServiceContainerSpec(makePostgRESTSpecOpts()) + if err != nil { + t.Fatalf("ServiceContainerSpec() error = %v", err) + } + cs := spec.TaskTemplate.ContainerSpec + if len(cs.Command) != 1 || cs.Command[0] != "postgrest" { + t.Errorf("Command = %v, want [\"postgrest\"]", cs.Command) + } + if len(cs.Args) != 1 || cs.Args[0] != "/app/data/postgrest.conf" { + t.Errorf("Args = %v, want [\"/app/data/postgrest.conf\"]", cs.Args) + } +} + +func TestServiceContainerSpec_PostgREST_HealthCheck(t *testing.T) { + spec, err := ServiceContainerSpec(makePostgRESTSpecOpts()) + if err != nil { + t.Fatalf("ServiceContainerSpec() error = %v", err) + } + hc := spec.TaskTemplate.ContainerSpec.Healthcheck + if hc == nil { + t.Fatal("Healthcheck is nil") + } + want := []string{"CMD", "postgrest", "--ready"} + if len(hc.Test) != len(want) { + t.Fatalf("Healthcheck.Test = %v, want %v", hc.Test, want) + } + for i, v := range want { + if hc.Test[i] != v { + t.Errorf("Healthcheck.Test[%d] = %q, want %q", i, hc.Test[i], v) + } + } +} + +func TestServiceContainerSpec_PostgREST_EnvVars(t *testing.T) { + spec, err := ServiceContainerSpec(makePostgRESTSpecOpts()) + if err != nil { + t.Fatalf("ServiceContainerSpec() error = %v", err) + } + envMap := make(map[string]string) + for _, e := range spec.TaskTemplate.ContainerSpec.Env { + parts := strings.SplitN(e, "=", 2) + if len(parts) == 2 { + envMap[parts[0]] = parts[1] + } + } + checks := map[string]string{ + "PGRST_DB_URI": "postgresql://", + "PGRST_SERVER_PORT": "8080", + "PGRST_ADMIN_SERVER_PORT": "3001", + "PGHOST": "pg-host1", + "PGPORT": "5432", + "PGDATABASE": "mydb", + "PGUSER": "svc_postgrest_host1", + "PGPASSWORD": "supersecret", + } + for key, want := range checks { + if got, ok := envMap[key]; !ok { + t.Errorf("env var %s is missing", key) + } else if got != want { + t.Errorf("env var %s = %q, want %q", key, got, want) + } + } +} + +func TestServiceContainerSpec_PostgREST_MountReadOnly(t *testing.T) { + spec, err := ServiceContainerSpec(makePostgRESTSpecOpts()) + if err != nil { + t.Fatalf("ServiceContainerSpec() error = %v", err) + } + mounts := spec.TaskTemplate.ContainerSpec.Mounts + if len(mounts) != 1 { + t.Fatalf("len(Mounts) = %d, want 1", len(mounts)) + } + if !mounts[0].ReadOnly { + t.Error("data mount should be read-only for PostgREST") + } + if mounts[0].Target != "/app/data" { + t.Errorf("mount target = %q, want \"/app/data\"", mounts[0].Target) + } +} + +func TestServiceContainerSpec_PostgREST_User(t *testing.T) { + spec, err := ServiceContainerSpec(makePostgRESTSpecOpts()) + if err != nil { + t.Fatalf("ServiceContainerSpec() error = %v", err) + } + want := fmt.Sprintf("%d", postgrestContainerUID) + if spec.TaskTemplate.ContainerSpec.User != want { + t.Errorf("User = %q, want %q (PostgREST runs as UID 1000 per official Dockerfile)", spec.TaskTemplate.ContainerSpec.User, want) + } +} diff --git a/server/internal/orchestrator/swarm/service_user_role.go b/server/internal/orchestrator/swarm/service_user_role.go index 8a8b5efd..b9c20fc5 100644 --- a/server/internal/orchestrator/swarm/service_user_role.go +++ b/server/internal/orchestrator/swarm/service_user_role.go @@ -65,8 +65,10 @@ type ServiceUserRole struct { ServiceID string `json:"service_id"` DatabaseID string `json:"database_id"` DatabaseName string `json:"database_name"` - NodeName string `json:"node_name"` // Database node name for PrimaryExecutor routing - Mode string `json:"mode"` // ServiceUserRoleRO or ServiceUserRoleRW + NodeName string `json:"node_name"` // Database node name for PrimaryExecutor routing + Mode string `json:"mode"` // ServiceUserRoleRO or ServiceUserRoleRW + ServiceType string `json:"service_type"` // "mcp" or "postgrest" + DBAnonRole string `json:"db_anon_role"` // PostgREST only: anonymous role granted to the service user Username string `json:"username"` Password string `json:"password"` // Generated on Create, persisted in state CredentialSource *resource.Identifier `json:"credential_source,omitempty"` @@ -191,32 +193,64 @@ func (r *ServiceUserRole) createUserRole(ctx context.Context, rc *resource.Conte } defer conn.Close(ctx) - // 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) + if r.ServiceType == "postgrest" { + attributes, grants := r.roleAttributesAndGrants() + statements, err := postgres.CreateUserRole(postgres.UserRoleOptions{ + Name: r.Username, + Password: r.Password, + 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) + } + 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) + } } - 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) - } + return nil +} - if err := statements.Exec(ctx, conn); err != nil { - return fmt.Errorf("failed to create service user: %w", err) +// roleAttributesAndGrants returns the PostgREST-specific role attributes and +// SQL grant statements. Only called when ServiceType == "postgrest"; +// MCP uses the group-role path in createUserRole() directly. +func (r *ServiceUserRole) roleAttributesAndGrants() ([]string, postgres.Statements) { + // NOINHERIT + GRANT enables PostgREST's SET ROLE mechanism. + attributes := []string{"LOGIN", "NOINHERIT"} + anonRole := r.DBAnonRole + if anonRole == "" { + anonRole = "pgedge_application_read_only" } - - return nil + grants := postgres.Statements{ + postgres.Statement{SQL: fmt.Sprintf("GRANT CONNECT ON DATABASE %s TO %s;", sanitizeIdentifier(r.DatabaseName), sanitizeIdentifier(r.Username))}, + postgres.Statement{SQL: fmt.Sprintf("GRANT %s TO %s;", sanitizeIdentifier(anonRole), sanitizeIdentifier(r.Username))}, + } + return attributes, grants } func (r *ServiceUserRole) Update(ctx context.Context, rc *resource.Context) error { diff --git a/server/internal/orchestrator/swarm/service_user_role_test.go b/server/internal/orchestrator/swarm/service_user_role_test.go index ab23f0c1..735f8fc3 100644 --- a/server/internal/orchestrator/swarm/service_user_role_test.go +++ b/server/internal/orchestrator/swarm/service_user_role_test.go @@ -2,9 +2,11 @@ package swarm import ( "fmt" + "strings" "testing" "github.com/pgEdge/control-plane/server/internal/database" + "github.com/pgEdge/control-plane/server/internal/postgres" "github.com/pgEdge/control-plane/server/internal/resource" ) @@ -209,3 +211,99 @@ func TestServiceUserRolePerNodeIdentifierUniqueness(t *testing.T) { seen[id] = fmt.Sprintf("role[%d] node=%s mode=%s", i, r.NodeName, r.Mode) } } + +// statementsSQL extracts the raw SQL from a postgres.Statements slice. +func statementsSQL(stmts postgres.Statements) []string { + out := make([]string, 0, len(stmts)) + for i, s := range stmts { + if stmt, ok := s.(postgres.Statement); ok { + out = append(out, stmt.SQL) + continue + } + panic(fmt.Sprintf("statementsSQL: unexpected statement type %T at index %d", s, i)) + } + return out +} + +func joinSQL(stmts postgres.Statements) string { + return strings.Join(statementsSQL(stmts), "\n") +} + +func TestRoleAttributesAndGrants_PostgREST_Attributes(t *testing.T) { + r := &ServiceUserRole{ + ServiceType: "postgrest", + DatabaseName: "mydb", + Username: "svc_pgrest", + DBAnonRole: "web_anon", + } + attrs, _ := r.roleAttributesAndGrants() + + attrSet := make(map[string]bool) + for _, a := range attrs { + attrSet[a] = true + } + if !attrSet["LOGIN"] { + t.Error("PostgREST attributes must include LOGIN") + } + if !attrSet["NOINHERIT"] { + t.Error("PostgREST attributes must include NOINHERIT") + } +} + +func TestRoleAttributesAndGrants_PostgREST_GrantsAnonRole(t *testing.T) { + r := &ServiceUserRole{ + ServiceType: "postgrest", + DatabaseName: "mydb", + Username: "svc_pgrest", + DBAnonRole: "web_anon", + } + _, grants := r.roleAttributesAndGrants() + sql := joinSQL(grants) + + if !strings.Contains(sql, "GRANT CONNECT") { + t.Errorf("PostgREST grants missing GRANT CONNECT\nGot:\n%s", sql) + } + if !strings.Contains(sql, `"web_anon"`) { + t.Errorf("PostgREST grants must grant configured DBAnonRole\nGot:\n%s", sql) + } +} + +func TestRoleAttributesAndGrants_PostgREST_DefaultAnonRole(t *testing.T) { + // Empty DBAnonRole → default to pgedge_application_read_only + r := &ServiceUserRole{ + ServiceType: "postgrest", + DatabaseName: "mydb", + Username: "svc_pgrest", + DBAnonRole: "", + } + _, grants := r.roleAttributesAndGrants() + sql := joinSQL(grants) + + if !strings.Contains(sql, `"pgedge_application_read_only"`) { + t.Errorf("PostgREST must default DBAnonRole to pgedge_application_read_only\nGot:\n%s", sql) + } +} + +func TestRoleAttributesAndGrants_PostgREST_NoDirectTableGrants(t *testing.T) { + // PostgREST accesses tables via the anon role — no direct table grants. + r := &ServiceUserRole{ + ServiceType: "postgrest", + DatabaseName: "mydb", + Username: "svc_pgrest", + DBAnonRole: "web_anon", + } + _, grants := r.roleAttributesAndGrants() + sql := joinSQL(grants) + + for _, forbidden := range []string{ + "GRANT SELECT", + "GRANT USAGE ON SCHEMA", + "ALTER DEFAULT PRIVILEGES", + "pg_read_all_settings", + } { + if strings.Contains(sql, forbidden) { + t.Errorf("PostgREST grants must not include %q (accesses tables via anon role)\nGot:\n%s", forbidden, sql) + } + } +} + From 54e6a3d338ee4e2fb1f670e657543b14206aabdc Mon Sep 17 00:00:00 2001 From: moizpgedge Date: Wed, 1 Apr 2026 11:16:50 +0500 Subject: [PATCH 2/7] fix: address PostgREST PR review feedback (PLAT-499/500/501/502/503) 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. --- .../database/postgrest_service_config.go | 65 ++++++ .../database/postgrest_service_config_test.go | 155 +++++++++++++++ .../orchestrator/swarm/postgrest_config.go | 47 ----- .../swarm/postgrest_config_resource.go | 26 ++- .../swarm/postgrest_config_test.go | 186 ------------------ .../orchestrator/swarm/service_spec.go | 28 +-- .../orchestrator/swarm/service_spec_test.go | 19 +- .../orchestrator/swarm/service_user_role.go | 58 +++++- 8 files changed, 309 insertions(+), 275 deletions(-) delete mode 100644 server/internal/orchestrator/swarm/postgrest_config.go delete mode 100644 server/internal/orchestrator/swarm/postgrest_config_test.go diff --git a/server/internal/database/postgrest_service_config.go b/server/internal/database/postgrest_service_config.go index b7ce465a..b090bbbf 100644 --- a/server/internal/database/postgrest_service_config.go +++ b/server/internal/database/postgrest_service_config.go @@ -1,7 +1,9 @@ package database import ( + "bytes" "fmt" + "net/url" "sort" "strings" ) @@ -145,3 +147,66 @@ func validatePostgRESTUnknownKeys(config map[string]any) []error { } return []error{fmt.Errorf("unknown config keys: %s", strings.Join(quoted, ", "))} } + +// PostgRESTConnParams holds the connection and credential details needed to +// generate a complete postgrest.conf. These are kept separate from +// PostgRESTServiceConfig because they are runtime-provisioned values (not +// user-supplied configuration). +type PostgRESTConnParams struct { + Username string + Password string + DatabaseName string + DatabaseHosts []ServiceHostEntry + TargetSessionAttrs string +} + +// GenerateConf renders a postgrest.conf file from the service config and the +// runtime connection parameters. The db-uri (including credentials) is written +// into the file; no credentials are exposed as environment variables. +func (c *PostgRESTServiceConfig) GenerateConf(conn PostgRESTConnParams) ([]byte, error) { + var buf bytes.Buffer + + fmt.Fprintf(&buf, "db-uri = %q\n", buildPostgRESTDBURI(conn)) + fmt.Fprintf(&buf, "db-schemas = %q\n", c.DBSchemas) + fmt.Fprintf(&buf, "db-anon-role = %q\n", c.DBAnonRole) + fmt.Fprintf(&buf, "db-pool = %d\n", c.DBPool) + fmt.Fprintf(&buf, "db-max-rows = %d\n", c.MaxRows) + + if c.JWTSecret != nil { + fmt.Fprintf(&buf, "jwt-secret = %q\n", *c.JWTSecret) + } + if c.JWTAud != nil { + fmt.Fprintf(&buf, "jwt-aud = %q\n", *c.JWTAud) + } + if c.JWTRoleClaimKey != nil { + fmt.Fprintf(&buf, "jwt-role-claim-key = %q\n", *c.JWTRoleClaimKey) + } + if c.ServerCORSAllowedOrigins != nil { + fmt.Fprintf(&buf, "server-cors-allowed-origins = %q\n", *c.ServerCORSAllowedOrigins) + } + + return buf.Bytes(), nil +} + +// buildPostgRESTDBURI constructs a libpq URI with multi-host support. +// Format: postgresql://user:pass@host1:port1,host2:port2/dbname[?target_session_attrs=...] +func buildPostgRESTDBURI(conn PostgRESTConnParams) string { + userInfo := url.UserPassword(conn.Username, conn.Password) + + hostParts := make([]string, len(conn.DatabaseHosts)) + for i, h := range conn.DatabaseHosts { + hostParts[i] = fmt.Sprintf("%s:%d", h.Host, h.Port) + } + + uri := fmt.Sprintf("postgresql://%s@%s/%s", + userInfo.String(), + strings.Join(hostParts, ","), + url.PathEscape(conn.DatabaseName), + ) + + if conn.TargetSessionAttrs != "" { + uri += "?target_session_attrs=" + url.QueryEscape(conn.TargetSessionAttrs) + } + + return uri +} diff --git a/server/internal/database/postgrest_service_config_test.go b/server/internal/database/postgrest_service_config_test.go index c7ab62a1..b2cbe7ed 100644 --- a/server/internal/database/postgrest_service_config_test.go +++ b/server/internal/database/postgrest_service_config_test.go @@ -1,6 +1,7 @@ package database_test import ( + "strings" "testing" "github.com/pgEdge/control-plane/server/internal/database" @@ -8,6 +9,160 @@ import ( "github.com/stretchr/testify/require" ) +// parseConf parses key=value lines from a postgrest.conf into a map. +// Surrounding quotes are stripped from string values. +func parseConf(t *testing.T, data []byte) map[string]string { + t.Helper() + m := make(map[string]string) + for _, line := range strings.Split(string(data), "\n") { + line = strings.TrimSpace(line) + if line == "" { + continue + } + parts := strings.SplitN(line, " = ", 2) + if len(parts) != 2 { + t.Fatalf("unexpected line in postgrest.conf: %q", line) + } + key := strings.TrimSpace(parts[0]) + val := strings.TrimSpace(parts[1]) + if strings.HasPrefix(val, `"`) && strings.HasSuffix(val, `"`) { + val = val[1 : len(val)-1] + } + m[key] = val + } + return m +} + +func makeTestConn() database.PostgRESTConnParams { + return database.PostgRESTConnParams{ + Username: "svc_pgrest", + Password: "testpass", + DatabaseName: "mydb", + DatabaseHosts: []database.ServiceHostEntry{{Host: "pg-host1", Port: 5432}}, + } +} + +func TestGenerateConf_CoreFields(t *testing.T) { + cfg := &database.PostgRESTServiceConfig{ + DBSchemas: "public", + DBAnonRole: "pgedge_application_read_only", + DBPool: 10, + MaxRows: 1000, + } + data, err := cfg.GenerateConf(makeTestConn()) + require.NoError(t, err) + m := parseConf(t, data) + assert.Equal(t, "public", m["db-schemas"]) + assert.Equal(t, "pgedge_application_read_only", m["db-anon-role"]) + assert.Equal(t, "10", m["db-pool"]) + assert.Equal(t, "1000", m["db-max-rows"]) +} + +func TestGenerateConf_CustomCoreFields(t *testing.T) { + cfg := &database.PostgRESTServiceConfig{ + DBSchemas: "api,private", + DBAnonRole: "web_anon", + DBPool: 5, + MaxRows: 500, + } + data, err := cfg.GenerateConf(makeTestConn()) + require.NoError(t, err) + m := parseConf(t, data) + assert.Equal(t, "api,private", m["db-schemas"]) + assert.Equal(t, "web_anon", m["db-anon-role"]) + assert.Equal(t, "5", m["db-pool"]) + assert.Equal(t, "500", m["db-max-rows"]) +} + +func TestGenerateConf_JWTFieldsAbsent(t *testing.T) { + cfg := &database.PostgRESTServiceConfig{ + DBSchemas: "public", + DBAnonRole: "web_anon", + DBPool: 10, + MaxRows: 1000, + } + data, err := cfg.GenerateConf(makeTestConn()) + require.NoError(t, err) + m := parseConf(t, data) + for _, key := range []string{"jwt-secret", "jwt-aud", "jwt-role-claim-key", "server-cors-allowed-origins"} { + assert.NotContains(t, m, key, "%s should be absent when not configured", key) + } +} + +func TestGenerateConf_AllJWTFields(t *testing.T) { + secret := "a-very-long-jwt-secret-that-is-at-least-32-chars" + aud := "my-api-audience" + roleClaimKey := ".role" + corsOrigins := "https://example.com" + cfg := &database.PostgRESTServiceConfig{ + DBSchemas: "public", + DBAnonRole: "web_anon", + DBPool: 10, + MaxRows: 1000, + JWTSecret: &secret, + JWTAud: &aud, + JWTRoleClaimKey: &roleClaimKey, + ServerCORSAllowedOrigins: &corsOrigins, + } + data, err := cfg.GenerateConf(makeTestConn()) + require.NoError(t, err) + m := parseConf(t, data) + assert.Equal(t, secret, m["jwt-secret"]) + assert.Equal(t, aud, m["jwt-aud"]) + assert.Equal(t, roleClaimKey, m["jwt-role-claim-key"]) + assert.Equal(t, corsOrigins, m["server-cors-allowed-origins"]) +} + +func TestGenerateConf_DBURIContainsCredentials(t *testing.T) { + cfg := &database.PostgRESTServiceConfig{ + DBSchemas: "public", + DBAnonRole: "web_anon", + DBPool: 10, + MaxRows: 1000, + } + conn := database.PostgRESTConnParams{ + Username: "svc_pgrest", + Password: "s3cr3t", + DatabaseName: "mydb", + DatabaseHosts: []database.ServiceHostEntry{{Host: "pg-host1", Port: 5432}}, + } + data, err := cfg.GenerateConf(conn) + require.NoError(t, err) + m := parseConf(t, data) + uri, ok := m["db-uri"] + require.True(t, ok, "db-uri must be present in postgrest.conf") + assert.Contains(t, uri, "svc_pgrest") + assert.Contains(t, uri, "s3cr3t") + assert.Contains(t, uri, "pg-host1") + assert.Contains(t, uri, "mydb") +} + +func TestGenerateConf_DBURIMultiHost(t *testing.T) { + cfg := &database.PostgRESTServiceConfig{ + DBSchemas: "public", + DBAnonRole: "web_anon", + DBPool: 10, + MaxRows: 1000, + } + conn := database.PostgRESTConnParams{ + Username: "svc_pgrest", + Password: "pass", + DatabaseName: "mydb", + DatabaseHosts: []database.ServiceHostEntry{ + {Host: "pg-host1", Port: 5432}, + {Host: "pg-host2", Port: 5432}, + }, + TargetSessionAttrs: "read-write", + } + data, err := cfg.GenerateConf(conn) + require.NoError(t, err) + m := parseConf(t, data) + uri := m["db-uri"] + assert.Contains(t, uri, "pg-host1") + assert.Contains(t, uri, "pg-host2") + assert.Contains(t, uri, "target_session_attrs") +} + func TestParsePostgRESTServiceConfig(t *testing.T) { t.Run("defaults applied for empty config", func(t *testing.T) { cfg, errs := database.ParsePostgRESTServiceConfig(map[string]any{}) diff --git a/server/internal/orchestrator/swarm/postgrest_config.go b/server/internal/orchestrator/swarm/postgrest_config.go deleted file mode 100644 index d92a917a..00000000 --- a/server/internal/orchestrator/swarm/postgrest_config.go +++ /dev/null @@ -1,47 +0,0 @@ -package swarm - -import ( - "bytes" - "fmt" - - "github.com/pgEdge/control-plane/server/internal/database" -) - -// PostgRESTConfigParams holds all inputs needed to generate a postgrest.conf file. -type PostgRESTConfigParams struct { - Config *database.PostgRESTServiceConfig -} - -// 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) { - if params == nil { - return nil, fmt.Errorf("GeneratePostgRESTConfig: params must not be nil") - } - if params.Config == nil { - return nil, fmt.Errorf("GeneratePostgRESTConfig: params.Config must not be nil") - } - cfg := params.Config - - var buf bytes.Buffer - - fmt.Fprintf(&buf, "db-schemas = %q\n", cfg.DBSchemas) - fmt.Fprintf(&buf, "db-anon-role = %q\n", cfg.DBAnonRole) - fmt.Fprintf(&buf, "db-pool = %d\n", cfg.DBPool) - fmt.Fprintf(&buf, "db-max-rows = %d\n", cfg.MaxRows) - - if cfg.JWTSecret != nil { - fmt.Fprintf(&buf, "jwt-secret = %q\n", *cfg.JWTSecret) - } - if cfg.JWTAud != nil { - fmt.Fprintf(&buf, "jwt-aud = %q\n", *cfg.JWTAud) - } - if cfg.JWTRoleClaimKey != nil { - fmt.Fprintf(&buf, "jwt-role-claim-key = %q\n", *cfg.JWTRoleClaimKey) - } - if cfg.ServerCORSAllowedOrigins != nil { - fmt.Fprintf(&buf, "server-cors-allowed-origins = %q\n", *cfg.ServerCORSAllowedOrigins) - } - - return buf.Bytes(), nil -} diff --git a/server/internal/orchestrator/swarm/postgrest_config_resource.go b/server/internal/orchestrator/swarm/postgrest_config_resource.go index cebb8e0b..3eb7ec0a 100644 --- a/server/internal/orchestrator/swarm/postgrest_config_resource.go +++ b/server/internal/orchestrator/swarm/postgrest_config_resource.go @@ -25,13 +25,19 @@ func PostgRESTConfigResourceIdentifier(serviceInstanceID string) resource.Identi } // PostgRESTConfigResource manages the postgrest.conf file on the host filesystem. -// The file is bind-mounted read-only into the container; credentials are not included. +// The file is bind-mounted read-only into the container and includes the db-uri +// with embedded credentials. type PostgRESTConfigResource struct { - ServiceInstanceID string `json:"service_instance_id"` - ServiceID string `json:"service_id"` - HostID string `json:"host_id"` - DirResourceID string `json:"dir_resource_id"` - Config *database.PostgRESTServiceConfig `json:"config"` + ServiceInstanceID string `json:"service_instance_id"` + ServiceID string `json:"service_id"` + HostID string `json:"host_id"` + DirResourceID string `json:"dir_resource_id"` + Config *database.PostgRESTServiceConfig `json:"config"` + Username string `json:"username"` + Password string `json:"password"` + DatabaseName string `json:"database_name"` + DatabaseHosts []database.ServiceHostEntry `json:"database_hosts"` + TargetSessionAttrs string `json:"target_session_attrs,omitempty"` } func (r *PostgRESTConfigResource) ResourceVersion() string { @@ -113,8 +119,12 @@ func (r *PostgRESTConfigResource) Delete(ctx context.Context, rc *resource.Conte } func (r *PostgRESTConfigResource) writeConfigFile(fs afero.Fs, dirPath string) error { - content, err := GeneratePostgRESTConfig(&PostgRESTConfigParams{ - Config: r.Config, + content, err := r.Config.GenerateConf(database.PostgRESTConnParams{ + Username: r.Username, + Password: r.Password, + DatabaseName: r.DatabaseName, + DatabaseHosts: r.DatabaseHosts, + TargetSessionAttrs: r.TargetSessionAttrs, }) if err != nil { return fmt.Errorf("failed to generate PostgREST config: %w", err) diff --git a/server/internal/orchestrator/swarm/postgrest_config_test.go b/server/internal/orchestrator/swarm/postgrest_config_test.go deleted file mode 100644 index 9582d0ba..00000000 --- a/server/internal/orchestrator/swarm/postgrest_config_test.go +++ /dev/null @@ -1,186 +0,0 @@ -package swarm - -import ( - "strings" - "testing" - - "github.com/pgEdge/control-plane/server/internal/database" -) - -// parseConf parses the key=value lines from a postgrest.conf into a map. -// String values are returned unquoted; numeric values are returned as-is. -func parseConf(t *testing.T, data []byte) map[string]string { - t.Helper() - m := make(map[string]string) - for _, line := range strings.Split(string(data), "\n") { - line = strings.TrimSpace(line) - if line == "" { - continue - } - parts := strings.SplitN(line, " = ", 2) - if len(parts) != 2 { - t.Fatalf("unexpected line in postgrest.conf: %q", line) - } - key := strings.TrimSpace(parts[0]) - val := strings.TrimSpace(parts[1]) - // Strip surrounding quotes from string values. - if strings.HasPrefix(val, `"`) && strings.HasSuffix(val, `"`) { - val = val[1 : len(val)-1] - } - m[key] = val - } - return m -} - -func TestGeneratePostgRESTConfig_Defaults(t *testing.T) { - params := &PostgRESTConfigParams{ - Config: &database.PostgRESTServiceConfig{ - DBSchemas: "public", - DBAnonRole: "pgedge_application_read_only", - DBPool: 10, - MaxRows: 1000, - }, - } - - data, err := GeneratePostgRESTConfig(params) - if err != nil { - t.Fatalf("GeneratePostgRESTConfig() error = %v", err) - } - - m := parseConf(t, data) - - if m["db-schemas"] != "public" { - t.Errorf("db-schemas = %q, want %q", m["db-schemas"], "public") - } - if m["db-anon-role"] != "pgedge_application_read_only" { - t.Errorf("db-anon-role = %q, want %q", m["db-anon-role"], "pgedge_application_read_only") - } - if m["db-pool"] != "10" { - t.Errorf("db-pool = %q, want %q", m["db-pool"], "10") - } - if m["db-max-rows"] != "1000" { - t.Errorf("db-max-rows = %q, want %q", m["db-max-rows"], "1000") - } -} - -func TestGeneratePostgRESTConfig_CustomCoreFields(t *testing.T) { - params := &PostgRESTConfigParams{ - Config: &database.PostgRESTServiceConfig{ - DBSchemas: "api,private", - DBAnonRole: "web_anon", - DBPool: 5, - MaxRows: 500, - }, - } - - data, err := GeneratePostgRESTConfig(params) - if err != nil { - t.Fatalf("GeneratePostgRESTConfig() error = %v", err) - } - - m := parseConf(t, data) - - if m["db-schemas"] != "api,private" { - t.Errorf("db-schemas = %q, want %q", m["db-schemas"], "api,private") - } - if m["db-anon-role"] != "web_anon" { - t.Errorf("db-anon-role = %q, want %q", m["db-anon-role"], "web_anon") - } - if m["db-pool"] != "5" { - t.Errorf("db-pool = %q, want %q", m["db-pool"], "5") - } - if m["db-max-rows"] != "500" { - t.Errorf("db-max-rows = %q, want %q", m["db-max-rows"], "500") - } -} - -func TestGeneratePostgRESTConfig_JWTFieldsAbsent(t *testing.T) { - // No JWT fields set — none should appear in the config file. - params := &PostgRESTConfigParams{ - Config: &database.PostgRESTServiceConfig{ - DBSchemas: "public", - DBAnonRole: "web_anon", - DBPool: 10, - MaxRows: 1000, - }, - } - - data, err := GeneratePostgRESTConfig(params) - if err != nil { - t.Fatalf("GeneratePostgRESTConfig() error = %v", err) - } - - m := parseConf(t, data) - - for _, key := range []string{"jwt-secret", "jwt-aud", "jwt-role-claim-key", "server-cors-allowed-origins"} { - if _, ok := m[key]; ok { - t.Errorf("%s should be absent when not configured, but it was present", key) - } - } -} - -func TestGeneratePostgRESTConfig_AllJWTFields(t *testing.T) { - secret := "a-very-long-jwt-secret-that-is-at-least-32-chars" - aud := "my-api-audience" - roleClaimKey := ".role" - corsOrigins := "https://example.com" - - params := &PostgRESTConfigParams{ - Config: &database.PostgRESTServiceConfig{ - DBSchemas: "public", - DBAnonRole: "web_anon", - DBPool: 10, - MaxRows: 1000, - JWTSecret: &secret, - JWTAud: &aud, - JWTRoleClaimKey: &roleClaimKey, - ServerCORSAllowedOrigins: &corsOrigins, - }, - } - - data, err := GeneratePostgRESTConfig(params) - if err != nil { - t.Fatalf("GeneratePostgRESTConfig() error = %v", err) - } - - m := parseConf(t, data) - - if m["jwt-secret"] != secret { - t.Errorf("jwt-secret = %q, want %q", m["jwt-secret"], secret) - } - if m["jwt-aud"] != aud { - t.Errorf("jwt-aud = %q, want %q", m["jwt-aud"], aud) - } - if m["jwt-role-claim-key"] != roleClaimKey { - t.Errorf("jwt-role-claim-key = %q, want %q", m["jwt-role-claim-key"], roleClaimKey) - } - if m["server-cors-allowed-origins"] != corsOrigins { - t.Errorf("server-cors-allowed-origins = %q, want %q", m["server-cors-allowed-origins"], corsOrigins) - } -} - -func TestGeneratePostgRESTConfig_CredentialsNotInFile(t *testing.T) { - // Verify that no credential-like keys ever appear in the config file. - secret := "a-very-long-jwt-secret-that-is-at-least-32-chars" - params := &PostgRESTConfigParams{ - Config: &database.PostgRESTServiceConfig{ - DBSchemas: "public", - DBAnonRole: "web_anon", - DBPool: 10, - MaxRows: 1000, - JWTSecret: &secret, - }, - } - - data, err := GeneratePostgRESTConfig(params) - if err != nil { - t.Fatalf("GeneratePostgRESTConfig() error = %v", err) - } - - // None of the libpq / db-uri credential keys should appear. - for _, forbidden := range []string{"db-uri", "PGUSER", "PGPASSWORD", "PGHOST", "PGPORT", "PGDATABASE"} { - if strings.Contains(string(data), forbidden) { - t.Errorf("config file must not contain %q (credentials are env vars)", forbidden) - } - } -} diff --git a/server/internal/orchestrator/swarm/service_spec.go b/server/internal/orchestrator/swarm/service_spec.go index 5aba4211..3032c5e1 100644 --- a/server/internal/orchestrator/swarm/service_spec.go +++ b/server/internal/orchestrator/swarm/service_spec.go @@ -2,8 +2,6 @@ package swarm import ( "fmt" - "strconv" - "strings" "time" "github.com/docker/docker/api/types/container" @@ -21,32 +19,12 @@ const mcpContainerUID = 1001 // See: https://github.com/PostgREST/postgrest/blob/main/Dockerfile (USER 1000) const postgrestContainerUID = 1000 -func buildPostgRESTEnvVars(opts *ServiceContainerSpecOptions) []string { - hosts := make([]string, 0, len(opts.DatabaseHosts)) - ports := make([]string, 0, len(opts.DatabaseHosts)) - for _, h := range opts.DatabaseHosts { - hosts = append(hosts, h.Host) - ports = append(ports, strconv.Itoa(h.Port)) - } - env := []string{ - "PGRST_DB_URI=postgresql://", +func buildPostgRESTEnvVars() []string { + return []string{ "PGRST_SERVER_HOST=0.0.0.0", "PGRST_SERVER_PORT=8080", "PGRST_ADMIN_SERVER_PORT=3001", - fmt.Sprintf("PGHOST=%s", strings.Join(hosts, ",")), - fmt.Sprintf("PGPORT=%s", strings.Join(ports, ",")), - fmt.Sprintf("PGDATABASE=%s", opts.DatabaseName), - } - if opts.TargetSessionAttrs != "" { - env = append(env, fmt.Sprintf("PGTARGETSESSIONATTRS=%s", opts.TargetSessionAttrs)) - } - if opts.Credentials != nil { - env = append(env, - fmt.Sprintf("PGUSER=%s", opts.Credentials.Username), - fmt.Sprintf("PGPASSWORD=%s", opts.Credentials.Password), - ) } - return env } // ServiceContainerSpecOptions contains all parameters needed to build a service container spec. @@ -139,7 +117,7 @@ func ServiceContainerSpec(opts *ServiceContainerSpecOptions) (swarm.ServiceSpec, user = fmt.Sprintf("%d", postgrestContainerUID) command = []string{"postgrest"} args = []string{"/app/data/postgrest.conf"} - env = buildPostgRESTEnvVars(opts) + env = buildPostgRESTEnvVars() // postgrest --ready exits 0/1; no curl in the static binary image. healthcheck = &container.HealthConfig{ Test: []string{"CMD", "postgrest", "--ready"}, diff --git a/server/internal/orchestrator/swarm/service_spec_test.go b/server/internal/orchestrator/swarm/service_spec_test.go index 49aea7e7..83c97573 100644 --- a/server/internal/orchestrator/swarm/service_spec_test.go +++ b/server/internal/orchestrator/swarm/service_spec_test.go @@ -381,23 +381,26 @@ func TestServiceContainerSpec_PostgREST_EnvVars(t *testing.T) { envMap[parts[0]] = parts[1] } } - checks := map[string]string{ - "PGRST_DB_URI": "postgresql://", + // Only server-side vars belong in the container environment; + // credentials are in postgrest.conf via db-uri. + required := map[string]string{ + "PGRST_SERVER_HOST": "0.0.0.0", "PGRST_SERVER_PORT": "8080", "PGRST_ADMIN_SERVER_PORT": "3001", - "PGHOST": "pg-host1", - "PGPORT": "5432", - "PGDATABASE": "mydb", - "PGUSER": "svc_postgrest_host1", - "PGPASSWORD": "supersecret", } - for key, want := range checks { + for key, want := range required { if got, ok := envMap[key]; !ok { t.Errorf("env var %s is missing", key) } else if got != want { t.Errorf("env var %s = %q, want %q", key, got, want) } } + // Credentials and connection details must not appear as env vars. + for _, forbidden := range []string{"PGUSER", "PGPASSWORD", "PGHOST", "PGPORT", "PGDATABASE", "PGRST_DB_URI", "PGTARGETSESSIONATTRS"} { + if _, ok := envMap[forbidden]; ok { + t.Errorf("env var %s must not be set (credentials belong in postgrest.conf)", forbidden) + } + } } func TestServiceContainerSpec_PostgREST_MountReadOnly(t *testing.T) { diff --git a/server/internal/orchestrator/swarm/service_user_role.go b/server/internal/orchestrator/swarm/service_user_role.go index b9c20fc5..dd687895 100644 --- a/server/internal/orchestrator/swarm/service_user_role.go +++ b/server/internal/orchestrator/swarm/service_user_role.go @@ -5,6 +5,7 @@ import ( "fmt" "strings" + "github.com/jackc/pgx/v5" "github.com/rs/zerolog" "github.com/samber/do" @@ -254,7 +255,53 @@ func (r *ServiceUserRole) roleAttributesAndGrants() ([]string, postgres.Statemen } func (r *ServiceUserRole) Update(ctx context.Context, rc *resource.Context) error { - // Service users don't support updates (no credential rotation in Phase 1) + if r.ServiceType != "postgrest" { + // MCP service users don't support updates (no credential rotation in Phase 1) + return nil + } + return r.reconcilePostgRESTGrants(ctx, rc) +} + +// reconcilePostgRESTGrants revokes any stale anon role grants and re-applies +// the desired ones. Safe to call repeatedly — all operations are idempotent. +func (r *ServiceUserRole) reconcilePostgRESTGrants(ctx context.Context, rc *resource.Context) error { + primary, err := database.GetPrimaryInstance(ctx, rc, r.NodeName) + if err != nil { + return fmt.Errorf("failed to get primary instance: %w", err) + } + conn, err := primary.Connection(ctx, rc, "postgres") + if err != nil { + return fmt.Errorf("failed to connect to database postgres on node %s: %w", r.NodeName, err) + } + defer conn.Close(ctx) + + desiredAnon := r.DBAnonRole + if desiredAnon == "" { + desiredAnon = "pgedge_application_read_only" + } + + // Revoke any role memberships that no longer match the desired anon role. + currentRoles, err := postgres.Query[string]{ + SQL: `SELECT r.rolname FROM pg_auth_members m JOIN pg_roles r ON m.roleid = r.oid JOIN pg_roles u ON m.member = u.oid WHERE u.rolname = @username`, + Args: pgx.NamedArgs{"username": r.Username}, + }.Scalars(ctx, conn) + if err != nil { + return fmt.Errorf("failed to query role memberships for %q: %w", r.Username, err) + } + for _, current := range currentRoles { + if current != desiredAnon { + if _, err := conn.Exec(ctx, fmt.Sprintf("REVOKE %s FROM %s", + sanitizeIdentifier(current), sanitizeIdentifier(r.Username))); err != nil { + return fmt.Errorf("failed to revoke stale anon role %q from %q: %w", current, r.Username, err) + } + } + } + + // Re-apply grants idempotently. + _, grants := r.roleAttributesAndGrants() + if err := grants.Exec(ctx, conn); err != nil { + return fmt.Errorf("failed to reconcile PostgREST grants for %q: %w", r.Username, err) + } return nil } @@ -284,6 +331,15 @@ func (r *ServiceUserRole) Delete(ctx context.Context, rc *resource.Context) erro } defer conn.Close(ctx) + // For PostgREST, revoke CONNECT before dropping — PostgreSQL will refuse + // to drop a role that still holds privileges on a database object. + if r.ServiceType == "postgrest" && r.DatabaseName != "" { + if _, rErr := conn.Exec(ctx, fmt.Sprintf("REVOKE CONNECT ON DATABASE %s FROM %s", + sanitizeIdentifier(r.DatabaseName), sanitizeIdentifier(r.Username))); rErr != nil { + logger.Warn().Err(rErr).Msg("failed to revoke connect privilege before drop, continuing anyway") + } + } + // Drop the user role // Using IF EXISTS to handle cases where the user was already dropped manually _, err = conn.Exec(ctx, fmt.Sprintf("DROP ROLE IF EXISTS %s", sanitizeIdentifier(r.Username))) From a5f5f24fa0769eda6d67e7255585dcf5c35d073f Mon Sep 17 00:00:00 2001 From: moizpgedge Date: Wed, 1 Apr 2026 11:34:01 +0500 Subject: [PATCH 3/7] refactor: reduce complexity and harden PostgREST provisioning - 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`. --- .../database/postgrest_service_config.go | 15 +++++-- .../swarm/postgrest_preflight_resource.go | 40 +++++++++++-------- .../orchestrator/swarm/service_user_role.go | 22 ++++++---- 3 files changed, 50 insertions(+), 27 deletions(-) diff --git a/server/internal/database/postgrest_service_config.go b/server/internal/database/postgrest_service_config.go index b090bbbf..9c2c21e2 100644 --- a/server/internal/database/postgrest_service_config.go +++ b/server/internal/database/postgrest_service_config.go @@ -164,9 +164,14 @@ type PostgRESTConnParams struct { // runtime connection parameters. The db-uri (including credentials) is written // into the file; no credentials are exposed as environment variables. func (c *PostgRESTServiceConfig) GenerateConf(conn PostgRESTConnParams) ([]byte, error) { + uri, err := buildPostgRESTDBURI(conn) + if err != nil { + return nil, err + } + var buf bytes.Buffer - fmt.Fprintf(&buf, "db-uri = %q\n", buildPostgRESTDBURI(conn)) + fmt.Fprintf(&buf, "db-uri = %q\n", uri) fmt.Fprintf(&buf, "db-schemas = %q\n", c.DBSchemas) fmt.Fprintf(&buf, "db-anon-role = %q\n", c.DBAnonRole) fmt.Fprintf(&buf, "db-pool = %d\n", c.DBPool) @@ -190,7 +195,11 @@ func (c *PostgRESTServiceConfig) GenerateConf(conn PostgRESTConnParams) ([]byte, // buildPostgRESTDBURI constructs a libpq URI with multi-host support. // Format: postgresql://user:pass@host1:port1,host2:port2/dbname[?target_session_attrs=...] -func buildPostgRESTDBURI(conn PostgRESTConnParams) string { +func buildPostgRESTDBURI(conn PostgRESTConnParams) (string, error) { + if len(conn.DatabaseHosts) == 0 { + return "", fmt.Errorf("PostgRESTConnParams.DatabaseHosts is empty") + } + userInfo := url.UserPassword(conn.Username, conn.Password) hostParts := make([]string, len(conn.DatabaseHosts)) @@ -208,5 +217,5 @@ func buildPostgRESTDBURI(conn PostgRESTConnParams) string { uri += "?target_session_attrs=" + url.QueryEscape(conn.TargetSessionAttrs) } - return uri + return uri, nil } diff --git a/server/internal/orchestrator/swarm/postgrest_preflight_resource.go b/server/internal/orchestrator/swarm/postgrest_preflight_resource.go index 55b5a756..6ce2bcc1 100644 --- a/server/internal/orchestrator/swarm/postgrest_preflight_resource.go +++ b/server/internal/orchestrator/swarm/postgrest_preflight_resource.go @@ -6,6 +6,7 @@ import ( "fmt" "strings" + "github.com/jackc/pgx/v5" "github.com/pgEdge/control-plane/server/internal/database" "github.com/pgEdge/control-plane/server/internal/resource" ) @@ -86,23 +87,7 @@ func (r *PostgRESTPreflightResource) validate(ctx context.Context, rc *resource. defer conn.Close(ctx) var errs []error - - for _, schema := range splitSchemas(r.DBSchemas) { - var exists bool - if err := conn.QueryRow(ctx, - "SELECT EXISTS (SELECT 1 FROM information_schema.schemata WHERE schema_name = $1)", - schema, - ).Scan(&exists); err != nil { - errs = append(errs, fmt.Errorf("failed to check schema %q: %w", schema, err)) - continue - } - if !exists { - errs = append(errs, fmt.Errorf( - "schema %q does not exist in database %q; create it before deploying PostgREST", - schema, r.DatabaseName, - )) - } - } + errs = append(errs, r.checkSchemas(ctx, conn)...) if r.DBAnonRole != "" { var exists bool @@ -122,6 +107,27 @@ func (r *PostgRESTPreflightResource) validate(ctx context.Context, rc *resource. return errors.Join(errs...) } +func (r *PostgRESTPreflightResource) checkSchemas(ctx context.Context, conn *pgx.Conn) []error { + var errs []error + for _, schema := range splitSchemas(r.DBSchemas) { + var exists bool + if err := conn.QueryRow(ctx, + "SELECT EXISTS (SELECT 1 FROM information_schema.schemata WHERE schema_name = $1)", + schema, + ).Scan(&exists); err != nil { + errs = append(errs, fmt.Errorf("failed to check schema %q: %w", schema, err)) + continue + } + if !exists { + errs = append(errs, fmt.Errorf( + "schema %q does not exist in database %q; create it before deploying PostgREST", + schema, r.DatabaseName, + )) + } + } + return errs +} + func splitSchemas(s string) []string { parts := strings.Split(s, ",") schemas := make([]string, 0, len(parts)) diff --git a/server/internal/orchestrator/swarm/service_user_role.go b/server/internal/orchestrator/swarm/service_user_role.go index dd687895..317a6aab 100644 --- a/server/internal/orchestrator/swarm/service_user_role.go +++ b/server/internal/orchestrator/swarm/service_user_role.go @@ -280,7 +280,21 @@ func (r *ServiceUserRole) reconcilePostgRESTGrants(ctx context.Context, rc *reso desiredAnon = "pgedge_application_read_only" } - // Revoke any role memberships that no longer match the desired anon role. + if err := r.revokeStaleAnonRoles(ctx, conn, desiredAnon); err != nil { + return err + } + + // Re-apply grants idempotently. + _, grants := r.roleAttributesAndGrants() + if err := grants.Exec(ctx, conn); err != nil { + return fmt.Errorf("failed to reconcile PostgREST grants for %q: %w", r.Username, err) + } + return nil +} + +// revokeStaleAnonRoles removes any role memberships on r.Username that differ +// from desiredAnon, so that only the intended anon role remains granted. +func (r *ServiceUserRole) revokeStaleAnonRoles(ctx context.Context, conn *pgx.Conn, desiredAnon string) error { currentRoles, err := postgres.Query[string]{ SQL: `SELECT r.rolname FROM pg_auth_members m JOIN pg_roles r ON m.roleid = r.oid JOIN pg_roles u ON m.member = u.oid WHERE u.rolname = @username`, Args: pgx.NamedArgs{"username": r.Username}, @@ -296,12 +310,6 @@ func (r *ServiceUserRole) reconcilePostgRESTGrants(ctx context.Context, rc *reso } } } - - // Re-apply grants idempotently. - _, grants := r.roleAttributesAndGrants() - if err := grants.Exec(ctx, conn); err != nil { - return fmt.Errorf("failed to reconcile PostgREST grants for %q: %w", r.Username, err) - } return nil } From b4359750196571f7b56d1244cae3b395dba5df15 Mon Sep 17 00:00:00 2001 From: moizpgedge Date: Wed, 1 Apr 2026 11:54:40 +0500 Subject: [PATCH 4/7] fix: address PostgREST code review findings - 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. --- .../swarm/postgrest_config_resource.go | 12 +-------- .../orchestrator/swarm/service_spec.go | 24 +++++++++++------ .../orchestrator/swarm/service_user_role.go | 26 ++++++++++++++----- 3 files changed, 37 insertions(+), 25 deletions(-) diff --git a/server/internal/orchestrator/swarm/postgrest_config_resource.go b/server/internal/orchestrator/swarm/postgrest_config_resource.go index 3eb7ec0a..117a51e1 100644 --- a/server/internal/orchestrator/swarm/postgrest_config_resource.go +++ b/server/internal/orchestrator/swarm/postgrest_config_resource.go @@ -100,17 +100,7 @@ func (r *PostgRESTConfigResource) Create(ctx context.Context, rc *resource.Conte } func (r *PostgRESTConfigResource) Update(ctx context.Context, rc *resource.Context) error { - fs, err := do.Invoke[afero.Fs](rc.Injector) - if err != nil { - return err - } - - dirPath, err := filesystem.DirResourceFullPath(rc, r.DirResourceID) - if err != nil { - return fmt.Errorf("failed to get service data dir path: %w", err) - } - - return r.writeConfigFile(fs, dirPath) + return r.Create(ctx, rc) } func (r *PostgRESTConfigResource) Delete(ctx context.Context, rc *resource.Context) error { diff --git a/server/internal/orchestrator/swarm/service_spec.go b/server/internal/orchestrator/swarm/service_spec.go index 3032c5e1..9901620d 100644 --- a/server/internal/orchestrator/swarm/service_spec.go +++ b/server/internal/orchestrator/swarm/service_spec.go @@ -19,6 +19,14 @@ const mcpContainerUID = 1001 // See: https://github.com/PostgREST/postgrest/blob/main/Dockerfile (USER 1000) const postgrestContainerUID = 1000 +// Shared health check timing for all service container types. +const ( + serviceHealthCheckStartPeriod = 30 * time.Second + serviceHealthCheckInterval = 10 * time.Second + serviceHealthCheckTimeout = 5 * time.Second + serviceHealthCheckRetries = 3 +) + func buildPostgRESTEnvVars() []string { return []string{ "PGRST_SERVER_HOST=0.0.0.0", @@ -121,10 +129,10 @@ func ServiceContainerSpec(opts *ServiceContainerSpecOptions) (swarm.ServiceSpec, // postgrest --ready exits 0/1; no curl in the static binary image. healthcheck = &container.HealthConfig{ Test: []string{"CMD", "postgrest", "--ready"}, - StartPeriod: time.Second * 30, - Interval: time.Second * 10, - Timeout: time.Second * 5, - Retries: 3, + StartPeriod: serviceHealthCheckStartPeriod, + Interval: serviceHealthCheckInterval, + Timeout: serviceHealthCheckTimeout, + Retries: serviceHealthCheckRetries, } mounts = []mount.Mount{ docker.BuildMount(opts.DataPath, "/app/data", true), @@ -136,10 +144,10 @@ func ServiceContainerSpec(opts *ServiceContainerSpecOptions) (swarm.ServiceSpec, args = []string{"-config", "/app/data/config.yaml"} healthcheck = &container.HealthConfig{ Test: []string{"CMD-SHELL", "curl -f http://localhost:8080/health || exit 1"}, - StartPeriod: time.Second * 30, - Interval: time.Second * 10, - Timeout: time.Second * 5, - Retries: 3, + StartPeriod: serviceHealthCheckStartPeriod, + Interval: serviceHealthCheckInterval, + Timeout: serviceHealthCheckTimeout, + Retries: serviceHealthCheckRetries, } mounts = []mount.Mount{ docker.BuildMount(opts.DataPath, "/app/data", false), diff --git a/server/internal/orchestrator/swarm/service_user_role.go b/server/internal/orchestrator/swarm/service_user_role.go index 317a6aab..a69289b9 100644 --- a/server/internal/orchestrator/swarm/service_user_role.go +++ b/server/internal/orchestrator/swarm/service_user_role.go @@ -263,7 +263,9 @@ func (r *ServiceUserRole) Update(ctx context.Context, rc *resource.Context) erro } // reconcilePostgRESTGrants revokes any stale anon role grants and re-applies -// the desired ones. Safe to call repeatedly — all operations are idempotent. +// the desired ones. The revoke + regrant pair runs in a single transaction so +// that a failed regrant rolls back the revoke, preventing transient loss of +// anon-role membership. func (r *ServiceUserRole) reconcilePostgRESTGrants(ctx context.Context, rc *resource.Context) error { primary, err := database.GetPrimaryInstance(ctx, rc, r.NodeName) if err != nil { @@ -280,21 +282,33 @@ func (r *ServiceUserRole) reconcilePostgRESTGrants(ctx context.Context, rc *reso desiredAnon = "pgedge_application_read_only" } - if err := r.revokeStaleAnonRoles(ctx, conn, desiredAnon); err != nil { + tx, err := conn.Begin(ctx) + if err != nil { + return fmt.Errorf("failed to begin transaction: %w", err) + } + defer tx.Rollback(ctx) //nolint:errcheck + + if err := r.revokeStaleAnonRoles(ctx, tx, desiredAnon); err != nil { return err } // Re-apply grants idempotently. _, grants := r.roleAttributesAndGrants() - if err := grants.Exec(ctx, conn); err != nil { + if err := grants.Exec(ctx, tx); err != nil { return fmt.Errorf("failed to reconcile PostgREST grants for %q: %w", r.Username, err) } + + if err := tx.Commit(ctx); err != nil { + return fmt.Errorf("failed to commit PostgREST grant reconciliation for %q: %w", r.Username, err) + } return nil } // revokeStaleAnonRoles removes any role memberships on r.Username that differ // from desiredAnon, so that only the intended anon role remains granted. -func (r *ServiceUserRole) revokeStaleAnonRoles(ctx context.Context, conn *pgx.Conn, desiredAnon string) error { +// conn may be a plain connection or a transaction — callers should prefer a +// transaction so revokes and the subsequent regrant are atomic. +func (r *ServiceUserRole) revokeStaleAnonRoles(ctx context.Context, conn postgres.Executor, desiredAnon string) error { currentRoles, err := postgres.Query[string]{ SQL: `SELECT r.rolname FROM pg_auth_members m JOIN pg_roles r ON m.roleid = r.oid JOIN pg_roles u ON m.member = u.oid WHERE u.rolname = @username`, Args: pgx.NamedArgs{"username": r.Username}, @@ -304,7 +318,7 @@ func (r *ServiceUserRole) revokeStaleAnonRoles(ctx context.Context, conn *pgx.Co } for _, current := range currentRoles { if current != desiredAnon { - if _, err := conn.Exec(ctx, fmt.Sprintf("REVOKE %s FROM %s", + if _, err := conn.Exec(ctx, fmt.Sprintf("REVOKE %s FROM %s", // #nosec G201 -- sanitizeIdentifier quotes all identifiers sanitizeIdentifier(current), sanitizeIdentifier(r.Username))); err != nil { return fmt.Errorf("failed to revoke stale anon role %q from %q: %w", current, r.Username, err) } @@ -342,7 +356,7 @@ func (r *ServiceUserRole) Delete(ctx context.Context, rc *resource.Context) erro // For PostgREST, revoke CONNECT before dropping — PostgreSQL will refuse // to drop a role that still holds privileges on a database object. if r.ServiceType == "postgrest" && r.DatabaseName != "" { - if _, rErr := conn.Exec(ctx, fmt.Sprintf("REVOKE CONNECT ON DATABASE %s FROM %s", + if _, rErr := conn.Exec(ctx, fmt.Sprintf("REVOKE CONNECT ON DATABASE %s FROM %s", // #nosec G201 -- sanitizeIdentifier quotes all identifiers sanitizeIdentifier(r.DatabaseName), sanitizeIdentifier(r.Username))); rErr != nil { logger.Warn().Err(rErr).Msg("failed to revoke connect privilege before drop, continuing anyway") } From 65a0351a406f87c8a36e918efafcd4ef1e65330a Mon Sep 17 00:00:00 2001 From: moizpgedge Date: Thu, 2 Apr 2026 04:05:49 +0500 Subject: [PATCH 5/7] refactor: wire PostgREST into orchestrator resource generation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce `PostgRESTAuthenticatorResource` — a dedicated resource that handles all PostgREST-specific Postgres role configuration (NOINHERIT, GRANT CONNECT, GRANT ) 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 . 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 --- .../orchestrator/swarm/orchestrator.go | 175 +++++++----- .../swarm/postgrest_authenticator_resource.go | 249 ++++++++++++++++++ .../swarm/postgrest_config_resource.go | 20 +- .../internal/orchestrator/swarm/resources.go | 1 + .../swarm/service_instance_spec.go | 20 +- .../orchestrator/swarm/service_user_role.go | 164 ++---------- .../swarm/service_user_role_test.go | 95 ------- 7 files changed, 421 insertions(+), 303 deletions(-) create mode 100644 server/internal/orchestrator/swarm/postgrest_authenticator_resource.go diff --git a/server/internal/orchestrator/swarm/orchestrator.go b/server/internal/orchestrator/swarm/orchestrator.go index 29adf4d1..ba7de27a 100644 --- a/server/internal/orchestrator/swarm/orchestrator.go +++ b/server/internal/orchestrator/swarm/orchestrator.go @@ -404,7 +404,7 @@ func (o *Orchestrator) GenerateInstanceRestoreResources(spec *database.InstanceS func (o *Orchestrator) GenerateServiceInstanceResources(spec *database.ServiceInstanceSpec) (*database.ServiceInstanceResources, error) { switch spec.ServiceSpec.ServiceType { - case "mcp": + case "mcp", "postgrest": return o.generateMCPInstanceResources(spec) case "rag": return o.generateRAGInstanceResources(spec) @@ -431,19 +431,6 @@ func (o *Orchestrator) generateMCPInstanceResources(spec *database.ServiceInstan } } - // Only MCP is fully implemented in the orchestrator for now. - // PostgREST provisioning (container spec, config delivery, service user) is - // implemented in follow-up tickets. - if spec.ServiceSpec.ServiceType != "mcp" { - return nil, fmt.Errorf("service type %q is not yet supported for provisioning", spec.ServiceSpec.ServiceType) - } - - // Parse the MCP service config from the untyped config map - mcpConfig, errs := database.ParseMCPServiceConfig(spec.ServiceSpec.Config, false) - if len(errs) > 0 { - return nil, fmt.Errorf("failed to parse MCP service config: %w", errors.Join(errs...)) - } - // Database network (shared with postgres instances) databaseNetwork := &Network{ Scope: "swarm", @@ -473,25 +460,76 @@ func (o *Orchestrator) generateMCPInstanceResources(spec *database.ServiceInstan // Service data directory resource (host-side bind mount directory) dataDirID := spec.ServiceInstanceID + "-data" - dataDir := &filesystem.DirResource{ - ID: dataDirID, - HostID: spec.HostID, - Path: filepath.Join(o.cfg.DataDir, "services", spec.ServiceInstanceID), - OwnerUID: mcpContainerUID, - OwnerGID: mcpContainerUID, - } - // MCP config resource (generates config.yaml, tokens.yaml, users.yaml) - // Credentials are populated from ServiceUserRole resources during refresh. - mcpConfigResource := &MCPConfigResource{ - ServiceInstanceID: spec.ServiceInstanceID, - ServiceID: spec.ServiceSpec.ServiceID, - HostID: spec.HostID, - DirResourceID: dataDirID, - Config: mcpConfig, - DatabaseName: spec.DatabaseName, - DatabaseHosts: spec.DatabaseHosts, - TargetSessionAttrs: spec.TargetSessionAttrs, + // Service-type-specific resources. + var serviceSpecificResources []resource.Resource + + switch spec.ServiceSpec.ServiceType { + case "mcp": + mcpConfig, errs := database.ParseMCPServiceConfig(spec.ServiceSpec.Config, false) + if len(errs) > 0 { + return nil, fmt.Errorf("failed to parse MCP service config: %w", errors.Join(errs...)) + } + + dataDir := &filesystem.DirResource{ + ID: dataDirID, + HostID: spec.HostID, + Path: filepath.Join(o.cfg.DataDir, "services", spec.ServiceInstanceID), + OwnerUID: mcpContainerUID, + OwnerGID: mcpContainerUID, + } + mcpConfigResource := &MCPConfigResource{ + ServiceInstanceID: spec.ServiceInstanceID, + ServiceID: spec.ServiceSpec.ServiceID, + HostID: spec.HostID, + DirResourceID: dataDirID, + Config: mcpConfig, + DatabaseName: spec.DatabaseName, + DatabaseHosts: spec.DatabaseHosts, + TargetSessionAttrs: spec.TargetSessionAttrs, + } + serviceSpecificResources = []resource.Resource{dataDir, mcpConfigResource} + + case "postgrest": + postgrestConfig, errs := database.ParsePostgRESTServiceConfig(spec.ServiceSpec.Config) + if len(errs) > 0 { + return nil, fmt.Errorf("failed to parse PostgREST service config: %w", errors.Join(errs...)) + } + + preflight := &PostgRESTPreflightResource{ + ServiceID: spec.ServiceSpec.ServiceID, + DatabaseID: spec.DatabaseID, + DatabaseName: spec.DatabaseName, + NodeName: spec.NodeName, + DBSchemas: postgrestConfig.DBSchemas, + DBAnonRole: postgrestConfig.DBAnonRole, + } + authenticator := &PostgRESTAuthenticatorResource{ + ServiceID: spec.ServiceSpec.ServiceID, + DatabaseID: spec.DatabaseID, + DatabaseName: spec.DatabaseName, + NodeName: spec.NodeName, + DBAnonRole: postgrestConfig.DBAnonRole, + UserRoleID: canonicalRWID, + } + dataDir := &filesystem.DirResource{ + ID: dataDirID, + HostID: spec.HostID, + Path: filepath.Join(o.cfg.DataDir, "services", spec.ServiceInstanceID), + OwnerUID: postgrestContainerUID, + OwnerGID: postgrestContainerUID, + } + postgrestConfigResource := &PostgRESTConfigResource{ + ServiceInstanceID: spec.ServiceInstanceID, + ServiceID: spec.ServiceSpec.ServiceID, + HostID: spec.HostID, + DirResourceID: dataDirID, + Config: postgrestConfig, + DatabaseName: spec.DatabaseName, + DatabaseHosts: spec.DatabaseHosts, + TargetSessionAttrs: spec.TargetSessionAttrs, + } + serviceSpecificResources = []resource.Resource{preflight, authenticator, dataDir, postgrestConfigResource} } // Service instance spec resource @@ -524,46 +562,55 @@ func (o *Orchestrator) generateMCPInstanceResources(spec *database.ServiceInstan HostID: spec.HostID, } - // Resource chain: Network → ServiceUserRole(RO,RW) → DirResource → MCPConfigResource → ServiceInstanceSpec → ServiceInstance + // Build the full resource list. orchestratorResources := []resource.Resource{ databaseNetwork, serviceUserRoleRO, serviceUserRoleRW, } - - // Per-node RO and RW roles for each additional database node so that - // multi-host DSNs work correctly on all nodes. CREATE ROLE is not - // replicated by Spock, so each node's primary needs its own role. - for _, nodeInst := range spec.DatabaseNodes { - if nodeInst.NodeName == spec.NodeName { - continue + orchestratorResources = append(orchestratorResources, serviceSpecificResources...) + orchestratorResources = append(orchestratorResources, serviceInstanceSpec, serviceInstance) + + // Append per-node ServiceUserRole resources for each additional database node. + // The canonical resources (above) cover the first node; nodes [1:] each get + // their own RO and RW role that sources credentials from the canonical. + if len(spec.DatabaseNodes) > 1 { + for _, nodeInst := range spec.DatabaseNodes[1:] { + perNodeRWID := ServiceUserRolePerNodeIdentifier(spec.ServiceSpec.ServiceID, ServiceUserRoleRW, nodeInst.NodeName) + orchestratorResources = append(orchestratorResources, + &ServiceUserRole{ + ServiceID: spec.ServiceSpec.ServiceID, + DatabaseID: spec.DatabaseID, + DatabaseName: spec.DatabaseName, + NodeName: nodeInst.NodeName, + Mode: ServiceUserRoleRO, + CredentialSource: &canonicalROID, + }, + &ServiceUserRole{ + ServiceID: spec.ServiceSpec.ServiceID, + DatabaseID: spec.DatabaseID, + DatabaseName: spec.DatabaseName, + NodeName: nodeInst.NodeName, + Mode: ServiceUserRoleRW, + CredentialSource: &canonicalRWID, + }, + ) + if spec.ServiceSpec.ServiceType == "postgrest" { + postgrestConfig, _ := database.ParsePostgRESTServiceConfig(spec.ServiceSpec.Config) + orchestratorResources = append(orchestratorResources, + &PostgRESTAuthenticatorResource{ + ServiceID: spec.ServiceSpec.ServiceID, + DatabaseID: spec.DatabaseID, + DatabaseName: spec.DatabaseName, + NodeName: nodeInst.NodeName, + DBAnonRole: postgrestConfig.DBAnonRole, + UserRoleID: perNodeRWID, + }, + ) + } } - orchestratorResources = append(orchestratorResources, - &ServiceUserRole{ - ServiceID: spec.ServiceSpec.ServiceID, - DatabaseID: spec.DatabaseID, - DatabaseName: spec.DatabaseName, - NodeName: nodeInst.NodeName, - Mode: ServiceUserRoleRO, - CredentialSource: &canonicalROID, - }, - &ServiceUserRole{ - ServiceID: spec.ServiceSpec.ServiceID, - DatabaseID: spec.DatabaseID, - DatabaseName: spec.DatabaseName, - NodeName: nodeInst.NodeName, - Mode: ServiceUserRoleRW, - CredentialSource: &canonicalRWID, - }, - ) } - orchestratorResources = append(orchestratorResources, - dataDir, - mcpConfigResource, - serviceInstanceSpec, - serviceInstance, - ) return o.buildServiceInstanceResources(spec, orchestratorResources) } diff --git a/server/internal/orchestrator/swarm/postgrest_authenticator_resource.go b/server/internal/orchestrator/swarm/postgrest_authenticator_resource.go new file mode 100644 index 00000000..30740a13 --- /dev/null +++ b/server/internal/orchestrator/swarm/postgrest_authenticator_resource.go @@ -0,0 +1,249 @@ +package swarm + +import ( + "context" + "fmt" + + "github.com/jackc/pgx/v5" + "github.com/rs/zerolog" + "github.com/samber/do" + + "github.com/pgEdge/control-plane/server/internal/database" + "github.com/pgEdge/control-plane/server/internal/postgres" + "github.com/pgEdge/control-plane/server/internal/resource" +) + +var _ resource.Resource = (*PostgRESTAuthenticatorResource)(nil) + +const ResourceTypePostgRESTAuthenticator resource.Type = "swarm.postgrest_authenticator" + +func PostgRESTAuthenticatorIdentifier(serviceID, nodeName string) resource.Identifier { + return resource.Identifier{ + ID: serviceID + "-auth-" + nodeName, + Type: ResourceTypePostgRESTAuthenticator, + } +} + +// PostgRESTAuthenticatorResource configures a PostgreSQL role as a PostgREST +// authenticator. It depends on the corresponding ServiceUserRole (which creates +// the basic LOGIN+group-role user) and adds PostgREST-specific configuration: +// +// - ALTER ROLE ... WITH NOINHERIT (required for PostgREST's SET ROLE mechanism) +// - GRANT CONNECT ON DATABASE to the authenticator user +// - GRANT to the authenticator user +// +// On Update, the anonymous role grant is reconciled within a single transaction +// to prevent transient loss of anon-role membership when the anon role changes. +// The actual DROP ROLE is handled by ServiceUserRole.Delete; this resource only +// revokes the CONNECT privilege it added. +type PostgRESTAuthenticatorResource struct { + ServiceID string `json:"service_id"` + DatabaseID string `json:"database_id"` + DatabaseName string `json:"database_name"` + NodeName string `json:"node_name"` + DBAnonRole string `json:"db_anon_role"` + UserRoleID resource.Identifier `json:"user_role_id"` // the RW ServiceUserRole this wraps +} + +func (r *PostgRESTAuthenticatorResource) ResourceVersion() string { return "1" } +func (r *PostgRESTAuthenticatorResource) DiffIgnore() []string { return nil } + +func (r *PostgRESTAuthenticatorResource) Identifier() resource.Identifier { + return PostgRESTAuthenticatorIdentifier(r.ServiceID, r.NodeName) +} + +func (r *PostgRESTAuthenticatorResource) Executor() resource.Executor { + return resource.PrimaryExecutor(r.NodeName) +} + +func (r *PostgRESTAuthenticatorResource) Dependencies() []resource.Identifier { + return []resource.Identifier{ + database.NodeResourceIdentifier(r.NodeName), + r.UserRoleID, + } +} + +func (r *PostgRESTAuthenticatorResource) TypeDependencies() []resource.Type { + return nil +} + +func (r *PostgRESTAuthenticatorResource) desiredAnonRole() string { + if r.DBAnonRole != "" { + return r.DBAnonRole + } + return "pgedge_application_read_only" +} + +func (r *PostgRESTAuthenticatorResource) authenticatorUsername() string { + return database.GenerateServiceUsername(r.ServiceID, ServiceUserRoleRW) +} + +// Refresh checks whether the role has NOINHERIT. If not — new deployment or +// manual change — returns ErrNotFound to trigger Create, which is idempotent. +func (r *PostgRESTAuthenticatorResource) Refresh(ctx context.Context, rc *resource.Context) error { + primary, err := database.GetPrimaryInstance(ctx, rc, r.NodeName) + if err != nil { + return fmt.Errorf("authenticator refresh: failed to get primary instance: %w", err) + } + conn, err := primary.Connection(ctx, rc, "postgres") + if err != nil { + return fmt.Errorf("authenticator refresh: failed to connect on node %s: %w", r.NodeName, err) + } + defer conn.Close(ctx) + + var noInherit bool + err = conn.QueryRow(ctx, + "SELECT NOT rolinherit FROM pg_catalog.pg_roles WHERE rolname = $1", + r.authenticatorUsername(), + ).Scan(&noInherit) + if err != nil { + return fmt.Errorf("%w: role %q not found: %w", resource.ErrNotFound, r.authenticatorUsername(), err) + } + if !noInherit { + return fmt.Errorf("%w: role %q does not have NOINHERIT", resource.ErrNotFound, r.authenticatorUsername()) + } + return nil +} + +func (r *PostgRESTAuthenticatorResource) Create(ctx context.Context, rc *resource.Context) error { + logger, err := do.Invoke[zerolog.Logger](rc.Injector) + if err != nil { + return err + } + username := r.authenticatorUsername() + logger = logger.With(). + Str("service_id", r.ServiceID). + Str("username", username). + Logger() + logger.Info().Msg("configuring PostgREST authenticator role") + + primary, err := database.GetPrimaryInstance(ctx, rc, r.NodeName) + if err != nil { + return fmt.Errorf("failed to get primary instance: %w", err) + } + conn, err := primary.Connection(ctx, rc, "postgres") + if err != nil { + return fmt.Errorf("failed to connect to database postgres on node %s: %w", r.NodeName, err) + } + defer conn.Close(ctx) + + tx, err := conn.Begin(ctx) + if err != nil { + return fmt.Errorf("failed to begin transaction: %w", err) + } + defer tx.Rollback(ctx) //nolint:errcheck + + anonRole := r.desiredAnonRole() + statements := postgres.Statements{ + postgres.Statement{SQL: fmt.Sprintf("ALTER ROLE %s WITH NOINHERIT;", sanitizeIdentifier(username))}, // #nosec G201 -- sanitizeIdentifier quotes all identifiers + postgres.Statement{SQL: fmt.Sprintf("GRANT CONNECT ON DATABASE %s TO %s;", sanitizeIdentifier(r.DatabaseName), sanitizeIdentifier(username))}, // #nosec G201 + postgres.Statement{SQL: fmt.Sprintf("GRANT %s TO %s;", sanitizeIdentifier(anonRole), sanitizeIdentifier(username))}, // #nosec G201 + } + if err := statements.Exec(ctx, tx); err != nil { + return fmt.Errorf("failed to configure PostgREST authenticator %q: %w", username, err) + } + if err := tx.Commit(ctx); err != nil { + return fmt.Errorf("failed to commit authenticator configuration for %q: %w", username, err) + } + + logger.Info().Str("anon_role", anonRole).Msg("PostgREST authenticator role configured") + return nil +} + +func (r *PostgRESTAuthenticatorResource) Update(ctx context.Context, rc *resource.Context) error { + return r.reconcileGrants(ctx, rc) +} + +// reconcileGrants revokes stale anon role grants and re-applies the desired +// ones within a single transaction to prevent transient loss of membership. +func (r *PostgRESTAuthenticatorResource) reconcileGrants(ctx context.Context, rc *resource.Context) error { + primary, err := database.GetPrimaryInstance(ctx, rc, r.NodeName) + if err != nil { + return fmt.Errorf("failed to get primary instance: %w", err) + } + conn, err := primary.Connection(ctx, rc, "postgres") + if err != nil { + return fmt.Errorf("failed to connect to database postgres on node %s: %w", r.NodeName, err) + } + defer conn.Close(ctx) + + tx, err := conn.Begin(ctx) + if err != nil { + return fmt.Errorf("failed to begin transaction: %w", err) + } + defer tx.Rollback(ctx) //nolint:errcheck + + username := r.authenticatorUsername() + desiredAnon := r.desiredAnonRole() + + if err := r.revokeStaleAnonRoles(ctx, tx, username, desiredAnon); err != nil { + return err + } + + grants := postgres.Statements{ + postgres.Statement{SQL: fmt.Sprintf("GRANT CONNECT ON DATABASE %s TO %s;", sanitizeIdentifier(r.DatabaseName), sanitizeIdentifier(username))}, // #nosec G201 + postgres.Statement{SQL: fmt.Sprintf("GRANT %s TO %s;", sanitizeIdentifier(desiredAnon), sanitizeIdentifier(username))}, // #nosec G201 + } + if err := grants.Exec(ctx, tx); err != nil { + return fmt.Errorf("failed to reconcile PostgREST grants for %q: %w", username, err) + } + if err := tx.Commit(ctx); err != nil { + return fmt.Errorf("failed to commit grant reconciliation for %q: %w", username, err) + } + return nil +} + +// 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 { + currentRoles, err := postgres.Query[string]{ + SQL: `SELECT r.rolname FROM pg_auth_members m JOIN pg_roles r ON m.roleid = r.oid JOIN pg_roles u ON m.member = u.oid WHERE u.rolname = @username`, + Args: pgx.NamedArgs{"username": username}, + }.Scalars(ctx, conn) + if err != nil { + return fmt.Errorf("failed to query role memberships for %q: %w", username, err) + } + for _, current := range currentRoles { + if current != desiredAnon { + if _, err := conn.Exec(ctx, fmt.Sprintf("REVOKE %s FROM %s", // #nosec G201 -- sanitizeIdentifier quotes all identifiers + sanitizeIdentifier(current), sanitizeIdentifier(username))); err != nil { + return fmt.Errorf("failed to revoke stale anon role %q from %q: %w", current, username, err) + } + } + } + return nil +} + +func (r *PostgRESTAuthenticatorResource) Delete(ctx context.Context, rc *resource.Context) error { + logger, err := do.Invoke[zerolog.Logger](rc.Injector) + if err != nil { + return err + } + username := r.authenticatorUsername() + logger = logger.With(). + Str("service_id", r.ServiceID). + Str("username", username). + Logger() + + if r.DatabaseName == "" { + return nil + } + + primary, err := database.GetPrimaryInstance(ctx, rc, r.NodeName) + if err != nil { + logger.Warn().Err(err).Msg("failed to get primary instance, skipping REVOKE CONNECT") + return nil + } + conn, err := primary.Connection(ctx, rc, "postgres") + if err != nil { + logger.Warn().Err(err).Msg("failed to connect to primary instance, skipping REVOKE CONNECT") + return nil + } + defer conn.Close(ctx) + + if _, rErr := conn.Exec(ctx, fmt.Sprintf("REVOKE CONNECT ON DATABASE %s FROM %s", // #nosec G201 -- sanitizeIdentifier quotes all identifiers + sanitizeIdentifier(r.DatabaseName), sanitizeIdentifier(username))); rErr != nil { + logger.Warn().Err(rErr).Msg("failed to revoke CONNECT privilege, continuing") + } + return nil +} diff --git a/server/internal/orchestrator/swarm/postgrest_config_resource.go b/server/internal/orchestrator/swarm/postgrest_config_resource.go index 117a51e1..9565510a 100644 --- a/server/internal/orchestrator/swarm/postgrest_config_resource.go +++ b/server/internal/orchestrator/swarm/postgrest_config_resource.go @@ -45,7 +45,7 @@ func (r *PostgRESTConfigResource) ResourceVersion() string { } func (r *PostgRESTConfigResource) DiffIgnore() []string { - return nil + return []string{"/username", "/password"} } func (r *PostgRESTConfigResource) Identifier() resource.Identifier { @@ -59,6 +59,7 @@ func (r *PostgRESTConfigResource) Executor() resource.Executor { func (r *PostgRESTConfigResource) Dependencies() []resource.Identifier { return []resource.Identifier{ filesystem.DirResourceIdentifier(r.DirResourceID), + ServiceUserRoleIdentifier(r.ServiceID, ServiceUserRoleRW), } } @@ -86,6 +87,10 @@ func (r *PostgRESTConfigResource) Refresh(ctx context.Context, rc *resource.Cont } func (r *PostgRESTConfigResource) Create(ctx context.Context, rc *resource.Context) error { + if err := r.populateCredentials(rc); err != nil { + return err + } + fs, err := do.Invoke[afero.Fs](rc.Injector) if err != nil { return err @@ -108,6 +113,19 @@ func (r *PostgRESTConfigResource) Delete(ctx context.Context, rc *resource.Conte return nil } +// populateCredentials reads the RW service user's username and password from +// the corresponding ServiceUserRole resource state. Called at Create/Update +// time so that credentials are never stale. +func (r *PostgRESTConfigResource) populateCredentials(rc *resource.Context) error { + rwRole, err := resource.FromContext[*ServiceUserRole](rc, ServiceUserRoleIdentifier(r.ServiceID, ServiceUserRoleRW)) + if err != nil { + return fmt.Errorf("failed to get RW service user role: %w", err) + } + r.Username = rwRole.Username + r.Password = rwRole.Password + return nil +} + func (r *PostgRESTConfigResource) writeConfigFile(fs afero.Fs, dirPath string) error { content, err := r.Config.GenerateConf(database.PostgRESTConnParams{ Username: r.Username, diff --git a/server/internal/orchestrator/swarm/resources.go b/server/internal/orchestrator/swarm/resources.go index e688039e..3ad755e8 100644 --- a/server/internal/orchestrator/swarm/resources.go +++ b/server/internal/orchestrator/swarm/resources.go @@ -23,4 +23,5 @@ func RegisterResourceTypes(registry *resource.Registry) { resource.RegisterResourceType[*MCPConfigResource](registry, ResourceTypeMCPConfig) resource.RegisterResourceType[*PostgRESTPreflightResource](registry, ResourceTypePostgRESTPreflightResource) resource.RegisterResourceType[*PostgRESTConfigResource](registry, ResourceTypePostgRESTConfig) + resource.RegisterResourceType[*PostgRESTAuthenticatorResource](registry, ResourceTypePostgRESTAuthenticator) } diff --git a/server/internal/orchestrator/swarm/service_instance_spec.go b/server/internal/orchestrator/swarm/service_instance_spec.go index 8514c8a6..d46ae158 100644 --- a/server/internal/orchestrator/swarm/service_instance_spec.go +++ b/server/internal/orchestrator/swarm/service_instance_spec.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/docker/docker/api/types/swarm" + "github.com/rs/zerolog/log" "github.com/pgEdge/control-plane/server/internal/database" "github.com/pgEdge/control-plane/server/internal/filesystem" @@ -60,13 +61,20 @@ func (s *ServiceInstanceSpecResource) Executor() resource.Executor { } func (s *ServiceInstanceSpecResource) Dependencies() []resource.Identifier { - // Service instances depend on the database network, service user role, and MCP config - return []resource.Identifier{ + deps := []resource.Identifier{ NetworkResourceIdentifier(s.DatabaseNetworkID), ServiceUserRoleIdentifier(s.ServiceSpec.ServiceID, ServiceUserRoleRO), ServiceUserRoleIdentifier(s.ServiceSpec.ServiceID, ServiceUserRoleRW), - MCPConfigResourceIdentifier(s.ServiceInstanceID), } + 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 } func (s *ServiceInstanceSpecResource) TypeDependencies() []resource.Type { @@ -78,10 +86,14 @@ func (s *ServiceInstanceSpecResource) populateCredentials(rc *resource.Context) if err != nil { return fmt.Errorf("failed to get service user role from state: %w", err) } + role := "pgedge_application_read_only" + if s.ServiceSpec.ServiceType == "postgrest" { + role = "postgrest_authenticator" + } s.Credentials = &database.ServiceUser{ Username: userRole.Username, Password: userRole.Password, - Role: "pgedge_application_read_only", + Role: role, } return nil } diff --git a/server/internal/orchestrator/swarm/service_user_role.go b/server/internal/orchestrator/swarm/service_user_role.go index a69289b9..df5e5634 100644 --- a/server/internal/orchestrator/swarm/service_user_role.go +++ b/server/internal/orchestrator/swarm/service_user_role.go @@ -5,7 +5,6 @@ import ( "fmt" "strings" - "github.com/jackc/pgx/v5" "github.com/rs/zerolog" "github.com/samber/do" @@ -66,10 +65,8 @@ type ServiceUserRole struct { ServiceID string `json:"service_id"` DatabaseID string `json:"database_id"` DatabaseName string `json:"database_name"` - NodeName string `json:"node_name"` // Database node name for PrimaryExecutor routing - Mode string `json:"mode"` // ServiceUserRoleRO or ServiceUserRoleRW - ServiceType string `json:"service_type"` // "mcp" or "postgrest" - DBAnonRole string `json:"db_anon_role"` // PostgREST only: anonymous role granted to the service user + NodeName string `json:"node_name"` // Database node name for PrimaryExecutor routing + Mode string `json:"mode"` // ServiceUserRoleRO or ServiceUserRoleRW Username string `json:"username"` Password string `json:"password"` // Generated on Create, persisted in state CredentialSource *resource.Identifier `json:"credential_source,omitempty"` @@ -83,6 +80,8 @@ func (r *ServiceUserRole) DiffIgnore() []string { return []string{ "/node_name", "/mode", + "/service_type", + "/db_anon_role", "/username", "/password", "/credential_source", @@ -175,7 +174,7 @@ func (r *ServiceUserRole) Create(ctx context.Context, rc *resource.Context) erro r.Password = password } - if err := r.createUserRole(ctx, rc, logger); err != nil { + if err := r.createUserRole(ctx, rc); err != nil { return fmt.Errorf("failed to create service user role: %w", err) } @@ -183,7 +182,7 @@ func (r *ServiceUserRole) Create(ctx context.Context, rc *resource.Context) erro return nil } -func (r *ServiceUserRole) createUserRole(ctx context.Context, rc *resource.Context, logger zerolog.Logger) error { +func (r *ServiceUserRole) createUserRole(ctx context.Context, rc *resource.Context) error { primary, err := database.GetPrimaryInstance(ctx, rc, r.NodeName) if err != nil { return fmt.Errorf("failed to get primary instance: %w", err) @@ -194,136 +193,32 @@ func (r *ServiceUserRole) createUserRole(ctx context.Context, rc *resource.Conte } defer conn.Close(ctx) - if r.ServiceType == "postgrest" { - attributes, grants := r.roleAttributesAndGrants() - statements, err := postgres.CreateUserRole(postgres.UserRoleOptions{ - Name: r.Username, - Password: r.Password, - 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) - } - 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 nil -} - -// roleAttributesAndGrants returns the PostgREST-specific role attributes and -// SQL grant statements. Only called when ServiceType == "postgrest"; -// MCP uses the group-role path in createUserRole() directly. -func (r *ServiceUserRole) roleAttributesAndGrants() ([]string, postgres.Statements) { - // NOINHERIT + GRANT enables PostgREST's SET ROLE mechanism. - attributes := []string{"LOGIN", "NOINHERIT"} - anonRole := r.DBAnonRole - if anonRole == "" { - anonRole = "pgedge_application_read_only" - } - grants := postgres.Statements{ - postgres.Statement{SQL: fmt.Sprintf("GRANT CONNECT ON DATABASE %s TO %s;", sanitizeIdentifier(r.DatabaseName), sanitizeIdentifier(r.Username))}, - postgres.Statement{SQL: fmt.Sprintf("GRANT %s TO %s;", sanitizeIdentifier(anonRole), sanitizeIdentifier(r.Username))}, - } - return attributes, grants -} - -func (r *ServiceUserRole) Update(ctx context.Context, rc *resource.Context) error { - if r.ServiceType != "postgrest" { - // MCP service users don't support updates (no credential rotation in Phase 1) - return nil - } - return r.reconcilePostgRESTGrants(ctx, rc) -} - -// reconcilePostgRESTGrants revokes any stale anon role grants and re-applies -// the desired ones. The revoke + regrant pair runs in a single transaction so -// that a failed regrant rolls back the revoke, preventing transient loss of -// anon-role membership. -func (r *ServiceUserRole) reconcilePostgRESTGrants(ctx context.Context, rc *resource.Context) error { - primary, err := database.GetPrimaryInstance(ctx, rc, r.NodeName) + 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 get primary instance: %w", err) - } - conn, err := primary.Connection(ctx, rc, "postgres") - if err != nil { - return fmt.Errorf("failed to connect to database postgres on node %s: %w", r.NodeName, err) - } - defer conn.Close(ctx) - - desiredAnon := r.DBAnonRole - if desiredAnon == "" { - desiredAnon = "pgedge_application_read_only" - } - - tx, err := conn.Begin(ctx) - if err != nil { - return fmt.Errorf("failed to begin transaction: %w", err) - } - defer tx.Rollback(ctx) //nolint:errcheck - - if err := r.revokeStaleAnonRoles(ctx, tx, desiredAnon); err != nil { - return err + return fmt.Errorf("failed to generate create user role statements: %w", err) } - - // Re-apply grants idempotently. - _, grants := r.roleAttributesAndGrants() - if err := grants.Exec(ctx, tx); err != nil { - return fmt.Errorf("failed to reconcile PostgREST grants for %q: %w", r.Username, err) + if err := statements.Exec(ctx, conn); err != nil { + return fmt.Errorf("failed to create service user: %w", err) } - if err := tx.Commit(ctx); err != nil { - return fmt.Errorf("failed to commit PostgREST grant reconciliation for %q: %w", r.Username, err) - } return nil } -// revokeStaleAnonRoles removes any role memberships on r.Username that differ -// from desiredAnon, so that only the intended anon role remains granted. -// conn may be a plain connection or a transaction — callers should prefer a -// transaction so revokes and the subsequent regrant are atomic. -func (r *ServiceUserRole) revokeStaleAnonRoles(ctx context.Context, conn postgres.Executor, desiredAnon string) error { - currentRoles, err := postgres.Query[string]{ - SQL: `SELECT r.rolname FROM pg_auth_members m JOIN pg_roles r ON m.roleid = r.oid JOIN pg_roles u ON m.member = u.oid WHERE u.rolname = @username`, - Args: pgx.NamedArgs{"username": r.Username}, - }.Scalars(ctx, conn) - if err != nil { - return fmt.Errorf("failed to query role memberships for %q: %w", r.Username, err) - } - for _, current := range currentRoles { - if current != desiredAnon { - if _, err := conn.Exec(ctx, fmt.Sprintf("REVOKE %s FROM %s", // #nosec G201 -- sanitizeIdentifier quotes all identifiers - sanitizeIdentifier(current), sanitizeIdentifier(r.Username))); err != nil { - return fmt.Errorf("failed to revoke stale anon role %q from %q: %w", current, r.Username, err) - } - } - } +func (r *ServiceUserRole) Update(ctx context.Context, rc *resource.Context) error { return nil } @@ -353,15 +248,6 @@ func (r *ServiceUserRole) Delete(ctx context.Context, rc *resource.Context) erro } defer conn.Close(ctx) - // For PostgREST, revoke CONNECT before dropping — PostgreSQL will refuse - // to drop a role that still holds privileges on a database object. - if r.ServiceType == "postgrest" && r.DatabaseName != "" { - if _, rErr := conn.Exec(ctx, fmt.Sprintf("REVOKE CONNECT ON DATABASE %s FROM %s", // #nosec G201 -- sanitizeIdentifier quotes all identifiers - sanitizeIdentifier(r.DatabaseName), sanitizeIdentifier(r.Username))); rErr != nil { - logger.Warn().Err(rErr).Msg("failed to revoke connect privilege before drop, continuing anyway") - } - } - // Drop the user role // Using IF EXISTS to handle cases where the user was already dropped manually _, err = conn.Exec(ctx, fmt.Sprintf("DROP ROLE IF EXISTS %s", sanitizeIdentifier(r.Username))) diff --git a/server/internal/orchestrator/swarm/service_user_role_test.go b/server/internal/orchestrator/swarm/service_user_role_test.go index 735f8fc3..5f0daf7a 100644 --- a/server/internal/orchestrator/swarm/service_user_role_test.go +++ b/server/internal/orchestrator/swarm/service_user_role_test.go @@ -2,11 +2,9 @@ package swarm import ( "fmt" - "strings" "testing" "github.com/pgEdge/control-plane/server/internal/database" - "github.com/pgEdge/control-plane/server/internal/postgres" "github.com/pgEdge/control-plane/server/internal/resource" ) @@ -212,98 +210,5 @@ func TestServiceUserRolePerNodeIdentifierUniqueness(t *testing.T) { } } -// statementsSQL extracts the raw SQL from a postgres.Statements slice. -func statementsSQL(stmts postgres.Statements) []string { - out := make([]string, 0, len(stmts)) - for i, s := range stmts { - if stmt, ok := s.(postgres.Statement); ok { - out = append(out, stmt.SQL) - continue - } - panic(fmt.Sprintf("statementsSQL: unexpected statement type %T at index %d", s, i)) - } - return out -} - -func joinSQL(stmts postgres.Statements) string { - return strings.Join(statementsSQL(stmts), "\n") -} - -func TestRoleAttributesAndGrants_PostgREST_Attributes(t *testing.T) { - r := &ServiceUserRole{ - ServiceType: "postgrest", - DatabaseName: "mydb", - Username: "svc_pgrest", - DBAnonRole: "web_anon", - } - attrs, _ := r.roleAttributesAndGrants() - - attrSet := make(map[string]bool) - for _, a := range attrs { - attrSet[a] = true - } - if !attrSet["LOGIN"] { - t.Error("PostgREST attributes must include LOGIN") - } - if !attrSet["NOINHERIT"] { - t.Error("PostgREST attributes must include NOINHERIT") - } -} - -func TestRoleAttributesAndGrants_PostgREST_GrantsAnonRole(t *testing.T) { - r := &ServiceUserRole{ - ServiceType: "postgrest", - DatabaseName: "mydb", - Username: "svc_pgrest", - DBAnonRole: "web_anon", - } - _, grants := r.roleAttributesAndGrants() - sql := joinSQL(grants) - if !strings.Contains(sql, "GRANT CONNECT") { - t.Errorf("PostgREST grants missing GRANT CONNECT\nGot:\n%s", sql) - } - if !strings.Contains(sql, `"web_anon"`) { - t.Errorf("PostgREST grants must grant configured DBAnonRole\nGot:\n%s", sql) - } -} - -func TestRoleAttributesAndGrants_PostgREST_DefaultAnonRole(t *testing.T) { - // Empty DBAnonRole → default to pgedge_application_read_only - r := &ServiceUserRole{ - ServiceType: "postgrest", - DatabaseName: "mydb", - Username: "svc_pgrest", - DBAnonRole: "", - } - _, grants := r.roleAttributesAndGrants() - sql := joinSQL(grants) - - if !strings.Contains(sql, `"pgedge_application_read_only"`) { - t.Errorf("PostgREST must default DBAnonRole to pgedge_application_read_only\nGot:\n%s", sql) - } -} - -func TestRoleAttributesAndGrants_PostgREST_NoDirectTableGrants(t *testing.T) { - // PostgREST accesses tables via the anon role — no direct table grants. - r := &ServiceUserRole{ - ServiceType: "postgrest", - DatabaseName: "mydb", - Username: "svc_pgrest", - DBAnonRole: "web_anon", - } - _, grants := r.roleAttributesAndGrants() - sql := joinSQL(grants) - - for _, forbidden := range []string{ - "GRANT SELECT", - "GRANT USAGE ON SCHEMA", - "ALTER DEFAULT PRIVILEGES", - "pg_read_all_settings", - } { - if strings.Contains(sql, forbidden) { - t.Errorf("PostgREST grants must not include %q (accesses tables via anon role)\nGot:\n%s", forbidden, sql) - } - } -} From a4cfb96329bfc35bf36cd5db07b7872d6e9d5a76 Mon Sep 17 00:00:00 2001 From: moizpgedge Date: Thu, 2 Apr 2026 09:41:23 +0500 Subject: [PATCH 6/7] fix: scope anon role revoke to exclude base group roles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- .../swarm/postgrest_authenticator_resource.go | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/server/internal/orchestrator/swarm/postgrest_authenticator_resource.go b/server/internal/orchestrator/swarm/postgrest_authenticator_resource.go index 30740a13..0ed93e5b 100644 --- a/server/internal/orchestrator/swarm/postgrest_authenticator_resource.go +++ b/server/internal/orchestrator/swarm/postgrest_authenticator_resource.go @@ -193,15 +193,27 @@ func (r *PostgRESTAuthenticatorResource) reconcileGrants(ctx context.Context, rc return nil } -// revokeStaleAnonRoles removes any role memberships on username that differ -// from desiredAnon. Must be called within a transaction for atomicity. +// revokeStaleAnonRoles revokes any previously-granted anon roles that are no +// longer the desired one. The query is scoped to known anon role candidates so +// that base group roles granted by ServiceUserRole (pgedge_application, +// pgedge_application_read_only) are never touched. Must be called within a +// transaction for atomicity. func (r *PostgRESTAuthenticatorResource) revokeStaleAnonRoles(ctx context.Context, conn postgres.Executor, username, desiredAnon string) error { + // Only query memberships that this resource could have granted — the set of + // known anon role names. This prevents accidentally revoking base group + // roles that ServiceUserRole manages. currentRoles, err := postgres.Query[string]{ - SQL: `SELECT r.rolname FROM pg_auth_members m JOIN pg_roles r ON m.roleid = r.oid JOIN pg_roles u ON m.member = u.oid WHERE u.rolname = @username`, + SQL: `SELECT r.rolname + FROM pg_auth_members m + JOIN pg_roles r ON m.roleid = r.oid + JOIN pg_roles u ON m.member = u.oid + WHERE u.rolname = @username + AND r.rolname != 'pgedge_application' + AND r.rolname != 'pgedge_application_read_only'`, Args: pgx.NamedArgs{"username": username}, }.Scalars(ctx, conn) if err != nil { - return fmt.Errorf("failed to query role memberships for %q: %w", username, err) + return fmt.Errorf("failed to query anon role memberships for %q: %w", username, err) } for _, current := range currentRoles { if current != desiredAnon { From 2932f64157938434d8f8472f2dc538408b99b397 Mon Sep 17 00:00:00 2001 From: moizpgedge Date: Thu, 2 Apr 2026 15:24:58 +0500 Subject: [PATCH 7/7] fix: address PostgREST authenticator review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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`. --- server/internal/orchestrator/swarm/orchestrator.go | 11 ++++++++--- .../swarm/postgrest_authenticator_resource.go | 3 ++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/server/internal/orchestrator/swarm/orchestrator.go b/server/internal/orchestrator/swarm/orchestrator.go index ba7de27a..940b9c97 100644 --- a/server/internal/orchestrator/swarm/orchestrator.go +++ b/server/internal/orchestrator/swarm/orchestrator.go @@ -464,6 +464,10 @@ func (o *Orchestrator) generateMCPInstanceResources(spec *database.ServiceInstan // Service-type-specific resources. var serviceSpecificResources []resource.Resource + // Hoisted PostgREST config — parsed once and reused in both the primary + // resource block and the per-node authenticator loop below. + var parsedPostgRESTConfig *database.PostgRESTServiceConfig + switch spec.ServiceSpec.ServiceType { case "mcp": mcpConfig, errs := database.ParseMCPServiceConfig(spec.ServiceSpec.Config, false) @@ -491,10 +495,12 @@ func (o *Orchestrator) generateMCPInstanceResources(spec *database.ServiceInstan serviceSpecificResources = []resource.Resource{dataDir, mcpConfigResource} case "postgrest": - postgrestConfig, errs := database.ParsePostgRESTServiceConfig(spec.ServiceSpec.Config) + var errs []error + parsedPostgRESTConfig, errs = database.ParsePostgRESTServiceConfig(spec.ServiceSpec.Config) if len(errs) > 0 { return nil, fmt.Errorf("failed to parse PostgREST service config: %w", errors.Join(errs...)) } + postgrestConfig := parsedPostgRESTConfig preflight := &PostgRESTPreflightResource{ ServiceID: spec.ServiceSpec.ServiceID, @@ -596,14 +602,13 @@ func (o *Orchestrator) generateMCPInstanceResources(spec *database.ServiceInstan }, ) if spec.ServiceSpec.ServiceType == "postgrest" { - postgrestConfig, _ := database.ParsePostgRESTServiceConfig(spec.ServiceSpec.Config) orchestratorResources = append(orchestratorResources, &PostgRESTAuthenticatorResource{ ServiceID: spec.ServiceSpec.ServiceID, DatabaseID: spec.DatabaseID, DatabaseName: spec.DatabaseName, NodeName: nodeInst.NodeName, - DBAnonRole: postgrestConfig.DBAnonRole, + DBAnonRole: parsedPostgRESTConfig.DBAnonRole, UserRoleID: perNodeRWID, }, ) diff --git a/server/internal/orchestrator/swarm/postgrest_authenticator_resource.go b/server/internal/orchestrator/swarm/postgrest_authenticator_resource.go index 0ed93e5b..78792204 100644 --- a/server/internal/orchestrator/swarm/postgrest_authenticator_resource.go +++ b/server/internal/orchestrator/swarm/postgrest_authenticator_resource.go @@ -2,6 +2,7 @@ package swarm import ( "context" + "errors" "fmt" "github.com/jackc/pgx/v5" @@ -97,7 +98,7 @@ func (r *PostgRESTAuthenticatorResource) Refresh(ctx context.Context, rc *resour r.authenticatorUsername(), ).Scan(&noInherit) if err != nil { - return fmt.Errorf("%w: role %q not found: %w", resource.ErrNotFound, r.authenticatorUsername(), err) + return fmt.Errorf("%w", errors.Join(resource.ErrNotFound, fmt.Errorf("role %q not found: %w", r.authenticatorUsername(), err))) } if !noInherit { return fmt.Errorf("%w: role %q does not have NOINHERIT", resource.ErrNotFound, r.authenticatorUsername())