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.

Author: moizpgedge

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
+}

server/internal/database/postgrest_service_config_test.go

@@ -1,13 +1,168 @@
 package database_test
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/pgEdge/control-plane/server/internal/database"
 	"github.com/stretchr/testify/assert"
 	"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{})

server/internal/orchestrator/swarm/postgrest_config.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)