fix: preserve service secrets on database update

server/internal/api/apiv1/convert.go

@@ -14,35 +14,20 @@ import (
 	api "github.com/pgEdge/control-plane/api/apiv1/gen/control_plane"
 	"github.com/pgEdge/control-plane/server/internal/config"
 	"github.com/pgEdge/control-plane/server/internal/database"
+	"github.com/pgEdge/control-plane/server/internal/ds"
 	"github.com/pgEdge/control-plane/server/internal/host"
 	"github.com/pgEdge/control-plane/server/internal/pgbackrest"
 	"github.com/pgEdge/control-plane/server/internal/task"
 	"github.com/pgEdge/control-plane/server/internal/utils"
 )
 
 // isSensitiveConfigKey returns true if the given config key name likely
-// contains a secret value that should not be returned in API responses.
+// contains a secret value that should not be returned in API responses. The
+// canonical definition lives in the database package, shared with
+// ServiceSpec.DefaultOptionalFieldsFrom, which restores these same keys from
+// stored state when an update omits them.
 func isSensitiveConfigKey(key string) bool {
-	k := strings.ToLower(key)
-	// Use suffix matching for "token" to avoid stripping non-secret keys like
-	// "token_budget". Keys named exactly "token" or ending with "_token" (e.g.
-	// "init_token", "auth_token") are still treated as sensitive.
-	if k == "token" || strings.HasSuffix(k, "_token") {
-		return true
-	}
-	patterns := []string{
-		"password", "secret",
-		"api_key", "apikey", "api-key",
-		"credential", "private_key", "private-key",
-		"access_key", "access-key",
-		"init_users", // mcp 'init_users' contains embedded passwords and must be stripped
-	}
-	for _, p := range patterns {
-		if strings.Contains(k, p) {
-			return true
-		}
-	}
-	return false
+	return database.IsSensitiveConfigKey(key)
 }
 
 // normalizeConfig ensures a nil config map is converted to an empty map so
@@ -748,10 +733,17 @@ func apiToScripts(scripts *api.DatabaseScripts) *database.ScriptStatements {
 	}
 }
 
+// apiToDatabaseSpec validates and converts apiSpec into a database.Spec.
+// existingServiceIDs should hold the service_id of every service already
+// present in the stored spec (nil for a create, where nothing exists yet); it
+// lets validation know which services may omit secrets that
+// Spec.DefaultOptionalFieldsFrom will restore from stored state afterward,
+// versus a newly added service, which must still supply its own secrets.
 func apiToDatabaseSpec(
 	orchestrator config.Orchestrator,
 	id, tID *api.Identifier,
 	apiSpec *api.DatabaseSpec,
+	existingServiceIDs ds.Set[string],
 ) (*database.Spec, error) {
 	var databaseID string
 	var err error
@@ -771,7 +763,7 @@ func apiToDatabaseSpec(
 		}
 		tenantID = &t
 	}
-	if err := validateDatabaseSpec(orchestrator, databaseID, apiSpec); err != nil {
+	if err := validateDatabaseSpec(orchestrator, databaseID, apiSpec, existingServiceIDs); err != nil {
 		return nil, err
 	}
 

server/internal/api/apiv1/post_init_handlers.go

@@ -310,7 +310,9 @@ func (s *PostInitHandlers) ListDatabases(ctx context.Context, req *api.ListDatab
 }
 
 func (s *PostInitHandlers) CreateDatabase(ctx context.Context, req *api.CreateDatabaseRequest) (*api.CreateDatabaseResponse, error) {
-	spec, err := apiToDatabaseSpec(s.cfg.Orchestrator, req.ID, req.TenantID, req.Spec)
+	// No existing services on a create, so every service must supply its own
+	// secrets.
+	spec, err := apiToDatabaseSpec(s.cfg.Orchestrator, req.ID, req.TenantID, req.Spec, nil)
 	if err != nil {
 		return nil, makeInvalidInputErr(err)
 	}
@@ -378,23 +380,39 @@ func (s *PostInitHandlers) GetDatabase(ctx context.Context, req *api.GetDatabase
 }
 
 func (s *PostInitHandlers) UpdateDatabase(ctx context.Context, req *api.UpdateDatabasePayload) (*api.UpdateDatabaseResponse, error) {
-	spec, err := apiToDatabaseSpec(s.cfg.Orchestrator, &req.DatabaseID, req.Request.TenantID, req.Request.Spec)
+	databaseID, err := dbIdentToString(req.DatabaseID)
 	if err != nil {
-		return nil, makeInvalidInputErr(err)
+		return nil, err
 	}
 
-	existing, err := s.dbSvc.GetDatabase(ctx, spec.DatabaseID)
+	existing, err := s.dbSvc.GetDatabase(ctx, databaseID)
 	if err != nil {
 		return nil, apiErr(err)
 	}
+
+	// A service already present in the stored spec may omit secrets that GET
+	// stripped from a prior read (see ServiceSpec.DefaultOptionalFieldsFrom) —
+	// tell validation which services those are so it doesn't reject the
+	// omission before the merge below has a chance to restore the stored
+	// value. A newly added service must still supply its own secrets.
+	existingServiceIDs := make(ds.Set[string], len(existing.Spec.Services))
+	for _, svc := range existing.Spec.Services {
+		existingServiceIDs.Add(svc.ServiceID)
+	}
+
+	spec, err := apiToDatabaseSpec(s.cfg.Orchestrator, &req.DatabaseID, req.Request.TenantID, req.Request.Spec, existingServiceIDs)
+	if err != nil {
+		return nil, makeInvalidInputErr(err)
+	}
 	// API-level update validation:
 	// ensure that for any newly added nodes, source_node (if set) refers
 	// only to nodes that exist in the old spec.
 	if err := validateDatabaseUpdate(existing.Spec, req.Request.Spec); err != nil {
 		return nil, makeInvalidInputErr(err)
 	}
 	// Copy optional fields from the previous spec to the current spec if they
-	// are unset.
+	// are unset. This also restores service secrets omitted from an existing
+	// service's config (see ServiceSpec.DefaultOptionalFieldsFrom).
 	spec.DefaultOptionalFieldsFrom(existing.Spec)
 
 	err = s.dbSvc.PopulateSpecDefaults(ctx, spec)

server/internal/api/apiv1/validate.go

@@ -95,7 +95,11 @@ func validatePgIdentConf(lines []string, path validation.Path) []error {
 	return errs
 }
 
-func validateDatabaseSpec(orchestrator config.Orchestrator, databaseID string, spec *api.DatabaseSpec) error {
+// validateDatabaseSpec validates spec. existingServiceIDs holds the
+// service_id of every service already present in the stored spec (nil for a
+// create); a service whose ID is in this set may omit secrets that
+// Spec.DefaultOptionalFieldsFrom will restore from stored state afterward.
+func validateDatabaseSpec(orchestrator config.Orchestrator, databaseID string, spec *api.DatabaseSpec, existingServiceIDs ds.Set[string]) error {
 	var errs []error
 
 	errs = append(errs, validateCPUs(spec.Cpus, validation.NewPath("cpus"))...)
@@ -184,14 +188,20 @@ func validateDatabaseSpec(orchestrator config.Orchestrator, databaseID string, s
 		for i, svc := range spec.Services {
 			svcPath := servicesPath.AppendArrayIndex(i)
 
+			if svc == nil {
+				errs = append(errs, validation.NewError(errors.New("service must not be null"), svcPath))
+				continue
+			}
+
 			// Check for duplicate service IDs
 			if seenServiceIDs.Has(string(svc.ServiceID)) {
 				err := errors.New("service IDs must be unique within a database")
 				errs = append(errs, validation.NewError(err, svcPath))
 			}
 			seenServiceIDs.Add(string(svc.ServiceID))
 
-			errs = append(errs, validateServiceSpec(svc, svcPath, false, databaseID, spec.DatabaseUsers, seenNodeNames)...)
+			isExistingService := existingServiceIDs.Has(string(svc.ServiceID))
+			errs = append(errs, validateServiceSpec(svc, svcPath, isExistingService, databaseID, spec.DatabaseUsers, seenNodeNames)...)
 		}
 	}
 
@@ -252,6 +262,12 @@ func validateDatabaseUpdate(old *database.Spec, new *api.DatabaseSpec) error {
 	// have no bootstrap fields (e.g. postgrest) the flag has no effect.
 	for i, svc := range new.Services {
 		svcPath := validation.NewPath("services", validation.ArrayIndexElement(i))
+
+		if svc == nil {
+			errs = append(errs, validation.NewError(errors.New("service must not be null"), svcPath))
+			continue
+		}
+
 		isExistingService := existingServiceIDs.Has(string(svc.ServiceID))
 
 		errs = append(errs, validateServiceSpec(svc, svcPath, isExistingService, old.DatabaseID, new.DatabaseUsers, newNodeNames)...)
@@ -577,6 +593,9 @@ func validateUniquePorts(spec *api.DatabaseSpec) []error {
 
 	servicesPath := validation.NewPath("services")
 	for i, service := range spec.Services {
+		if service == nil {
+			continue
+		}
 		servicePath := servicesPath.AppendArrayIndex(i)
 
 		for _, h := range service.HostIds {

server/internal/api/apiv1/validate_test.go

@@ -690,9 +690,10 @@ func TestValidateNode(t *testing.T) {
 
 func TestValidateDatabaseSpec(t *testing.T) {
 	for _, tc := range []struct {
-		name     string
-		spec     *api.DatabaseSpec
-		expected []string
+		name               string
+		spec               *api.DatabaseSpec
+		existingServiceIDs ds.Set[string]
+		expected           []string
 	}{
 		{
 			name: "valid minimal",
@@ -1424,16 +1425,33 @@ func TestValidateDatabaseSpec(t *testing.T) {
 				`"spock" must be included in shared_preload_libraries`,
 			},
 		},
+		{
+			name: "null service entry is rejected, not dereferenced",
+			spec: &api.DatabaseSpec{
+				Nodes: []*api.DatabaseNodeSpec{
+					{
+						Name:    "n1",
+						HostIds: []api.Identifier{api.Identifier("host-1")},
+					},
+				},
+				Services: []*api.ServiceSpec{nil},
+			},
+			expected: []string{
+				`services[0]: service must not be null`,
+			},
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
-			err := validateDatabaseSpec(config.OrchestratorSwarm, "test-db", tc.spec)
-			if len(tc.expected) < 1 {
-				assert.NoError(t, err)
-			} else {
-				for _, expected := range tc.expected {
-					assert.ErrorContains(t, err, expected)
+			assert.NotPanics(t, func() {
+				err := validateDatabaseSpec(config.OrchestratorSwarm, "test-db", tc.spec, tc.existingServiceIDs)
+				if len(tc.expected) < 1 {
+					assert.NoError(t, err)
+				} else {
+					for _, expected := range tc.expected {
+						assert.ErrorContains(t, err, expected)
+					}
 				}
-			}
+			})
 		})
 	}
 }
@@ -2035,6 +2053,49 @@ func TestValidateServiceSpec(t *testing.T) {
 	}
 }
 
+func TestValidateServiceSpec_RAGUpdateOmitsAPIKey(t *testing.T) {
+	// PLAT-715: GET strips api_key from a RAG service's config, so a
+	// read-edit-write cycle resubmits the pipeline without it. On create
+	// (isUpdate=false) that must still fail — there's no stored value to fall
+	// back to. On an existing service (isUpdate=true), the omission must be
+	// allowed here: Spec.DefaultOptionalFieldsFrom restores the real value
+	// afterward, and the final, merged config is re-validated strictly
+	// (isUpdate=false) at deploy time.
+	testDBUsers := []*api.DatabaseUserSpec{
+		{Username: "app", DbOwner: utils.PointerTo(true)},
+	}
+	svc := &api.ServiceSpec{
+		ServiceID:   "rag",
+		ServiceType: "rag",
+		Version:     "latest",
+		HostIds:     []api.Identifier{"host-1"},
+		ConnectAs:   "app",
+		Config: map[string]any{
+			"pipelines": []any{
+				map[string]any{
+					"name": "docs",
+					"tables": []any{
+						map[string]any{"table": "docs", "text_column": "content", "vector_column": "embedding"},
+					},
+					"embedding_llm": map[string]any{"provider": "voyage", "model": "voyage-3"},
+					"rag_llm":       map[string]any{"provider": "anthropic", "model": "claude-3"},
+				},
+			},
+		},
+	}
+
+	t.Run("create requires api_key", func(t *testing.T) {
+		err := errors.Join(validateServiceSpec(svc, nil, false, "test-db", testDBUsers)...)
+		assert.ErrorContains(t, err, "embedding_llm.api_key is required")
+		assert.ErrorContains(t, err, "rag_llm.api_key is required")
+	})
+
+	t.Run("update on an existing service allows the omission", func(t *testing.T) {
+		err := errors.Join(validateServiceSpec(svc, nil, true, "test-db", testDBUsers)...)
+		assert.NoError(t, err)
+	})
+}
+
 func TestValidateServiceSpec_NameBudget(t *testing.T) {
 	testDBUsers := []*api.DatabaseUserSpec{
 		{Username: "app", DbOwner: utils.PointerTo(true)},
@@ -2452,3 +2513,14 @@ func TestValidateDatabaseUpdate_ServiceBootstrapFields(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateDatabaseUpdate_NullServiceEntry(t *testing.T) {
+	old := &database.Spec{Services: []*database.ServiceSpec{{ServiceID: "rag"}}}
+	newSpec := &api.DatabaseSpec{Services: []*api.ServiceSpec{nil}}
+
+	var err error
+	assert.NotPanics(t, func() {
+		err = validateDatabaseUpdate(old, newSpec)
+	})
+	assert.ErrorContains(t, err, "services[0]: service must not be null")
+}