bug(postgres): #2466 follow-up — integer→string upgrade trap and incomplete shell-exec block list #2597

Summary

#2466 (feat(postgres)!: support all PostgreSQL configuration parameters)
introduced two issues that survived merge. Both are verified against current
main and the backport label means they will propagate to a patch release
unless fixed.

1. The schema-level fix that made the int→string change tolerant on upgrade
   was committed and then immediately clobbered by a cozyvalues-gen
   regeneration ~8 minutes later. Existing tenants on a v1.x cluster will be
   stuck on Helm reconciliation failures after upgrade until they manually
   re-apply each Postgres resource.
2. The shell-execution parameter block list is incomplete — two parameters in
   the same family as archive_command/restore_command are missing.

1. Upgrade path: integer max_connections breaks reconciliation after upgrade

What changed

api/apps/v1alpha1/postgresql/types.go went from a typed struct with a single
integer field:

type PostgreSQLParameters struct {
    MaxConnections int `json:"max_connections,omitempty"`
}

to:

Parameters map[string]string `json:"parameters,omitempty"`

The new schema is enforced at:

• Postgres.apps.cozystack.io openAPISchema embedded in
  packages/system/postgres-rd/cozyrds/postgres.yaml:
  parameters.additionalProperties: {type: string}.
• Helm values.schema.json —
  packages/apps/postgres/values.schema.json line ~105:
  additionalProperties: {type: string}.

Existing tenant Postgres resources written against the old API have
spec.postgresql.parameters.max_connections: 100 (integer). Tenants who never
explicitly set the value are also affected — the old type carried
+kubebuilder:default:=100 (integer), so the defaulter persisted the integer
into the stored object at create time.

Where the failure surfaces

The cozystack-api server does not validate user input against the
embedded openAPISchema. It loads the schema (rest.go:99-117) but only
uses it for defaulting (rest_defaulting.go:30-50). The only on-write
checks are name format and reserved-key prefix (rest.go:158-181,
rest.go:497-504). Real enforcement is downstream:

tenant write → cozystack-api (no schema check) → HelmRelease.Spec.Values
                                                       ↓
                                 helm-controller validates Values vs values.schema.json
                                                       ↓
                                                  render & apply

So after upgrade:

1. Existing HelmRelease objects in etcd still carry integer
   max_connections in Spec.Values.
2. helm-controller revalidates Spec.Values against the new
   values.schema.json on every reconcile and rejects the integer.
3. The chart never renders, the CNPG Cluster is no longer reconciled,
   no further configuration changes flow to the database.
4. The toString coercion at packages/apps/postgres/templates/db.yaml:72
   is moot — rendering never reaches that line when the schema check
   fails earlier.
5. The PostgreSQL pod itself stays up because CNPG keeps the last
   successfully-rendered Cluster running. Only the reconciliation
   pipeline is stuck.

Recovery path

A PUT with the corrected max_connections: "100" (string) recovers a
single resource.

1. cozystack-api accepts it (no schema gate on writes).
2. cozystack-api rewrites the underlying HelmRelease.Spec.Values with
   the string-typed JSON (rest.go:507, 539).
3. helm-controller revalidates → passes → renders → applies.

But there are sharp edges that make this a real operational problem rather
than a one-line manual fix:

• PATCH that touches a different field (e.g., replicas) does NOT
  recover. cozystack-api merges the patch onto the existing object and
  forwards the entire merged spec into HelmRelease.Spec.Values. The
  integer max_connections is preserved in the merged result, helm-controller
  still rejects, and the patch effectively no-ops as far as the DB is
  concerned. Tenants must explicitly overwrite parameters with
  string-typed values, even when their intended change is unrelated.
• Defaults do not auto-fix it. Helm/CRD defaulting only fills missing
  keys. The existing object already has parameters.max_connections set,
  so the new string default "100" is never applied.
• No error surfaces on the user-facing Postgres kind. The failure is
  on the underlying HelmRelease. Tenants would only notice when a change
  to their Postgres silently fails to take effect, or by inspecting
  HelmRelease status directly — most tenants wouldn't think to do that.
• Tenants who never explicitly set max_connections are still
  affected, because the old schema's integer default was persisted into
  the stored object at original create time. They will hit this trap
  even though they "never touched" the field.

The fix that was reverted

git log -- packages/apps/postgres/values.schema.json shows:

6ec761f was correct. The regeneration in 53caea3 clobbered it because
cozyvalues-gen derives the schema directly from the typedef and has no
way to express a union type.

Suggested fix

Restore schema tolerance at the chart layer (the template already does
toString | quote, so allowing integer at the schema layer is the missing
piece):

 "additionalProperties": {
-  "type": "string"
+  "anyOf": [
+    {"type": "string"},
+    {"type": "integer"}
+  ]
 }

This needs either:

• A hand-maintained schema override that survives regeneration, or
• cozyvalues-gen support for declaring union additionalProperties from
  values.yaml annotations.

The same fix needs to flow into the embedded openAPISchema in
packages/system/postgres-rd/cozyrds/postgres.yaml so that admission
defaulting also accepts integers without trying to "default in" a string
that conflicts with the existing integer.

Note: this is also relevant for the backport label on #2466 — without
the tolerance fix, in-place upgrades on patch releases will trip the
same trap.

2. Incomplete shell-exec block list

Block list at packages/apps/postgres/templates/db.yaml:66:

archive_command, restore_command, ssl_passphrase_command,
dynamic_library_path, local_preload_libraries,
session_preload_libraries, shared_preload_libraries

Missing entries (same RCE surface)

• archive_cleanup_command — shell command run after each restartpoint
  during recovery. Same system() exec primitive as archive_command.
• recovery_end_command — shell command run once at the end of recovery.
  Same primitive as restore_command.

Both are PostgreSQL-documented recovery-configuration parameters and
execute as the postgres OS user inside the tenant's pod. Blocking
archive_command while leaving archive_cleanup_command open is an
oversight — same audit category, same exec capability, same threat model
the PR was trying to close.

The case-insensitive normalize (lower $key) is correct, so the user
cannot bypass with Archive_Command casing — adding the two names to the
list is sufficient.

Suggested fix

 {{- $dangerousParams := list "archive_command" "restore_command"
-      "ssl_passphrase_command"
+      "ssl_passphrase_command" "archive_cleanup_command" "recovery_end_command"
       "dynamic_library_path" "local_preload_libraries"
       "session_preload_libraries" "shared_preload_libraries" }}

Worth a second look (not RCE, judgment call)

• hba_file, ident_file, include, include_file, include_dir —
  load arbitrary .conf/auth files at server start. CNPG manages
  pg_hba.conf itself, so overriding these could replace the auth config
  with attacker-controlled content. Likely already rejected by the CNPG
  webhook but should be confirmed rather than relied on implicitly.
• fsync, synchronous_commit, full_page_writes, data_sync_retry —
  not RCE, but a tenant flipping fsync=off corrupts their own DB on
  crash. Per the PR's stated stance ("tenant breaks own DB only") this
  is acceptable; documenting the decision explicitly would help future
  reviewers.

Refs

• PR: feat(postgres)!: support all PostgreSQL configuration parameters #2466
• Reverting commit: 53caea3e
• Original tolerance fix (still desired): 6ec761fc
• Files affected:
  • packages/apps/postgres/values.schema.json
  • packages/apps/postgres/templates/db.yaml
  • packages/apps/postgres/values.yaml
  • packages/system/postgres-rd/cozyrds/postgres.yaml
  • api/apps/v1alpha1/postgresql/types.go