Skip to content

deploy: make base-infra Postgres optional for bring-your-own database - #92

Open
waldemort-auto[bot] wants to merge 4 commits into
mainfrom
waldemort/byo-postgres
Open

waldemort-auto[bot] wants to merge 4 commits into
mainfrom
waldemort/byo-postgres

Conversation

@waldemort-auto

@waldemort-auto waldemort-auto Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

What changes

The supported Node deploy path can skip its PostgreSQL Flexible Server module and use an existing PostgreSQL-compatible database, including a HorizonDB cluster. This requires deploy-script and runtime-auth changes, not just a conditional Bicep module.

Consumer setting New behavior
DEPLOY_POSTGRES Accepts true/false and legacy 1/0, normalized to a JSON boolean. Defaults to true, including for old env files without the key. Invalid or empty values fail.
BYO connection URLs Supply both DATABASE_URL and PILOTSWARM_CMS_FACTS_DATABASE_URL, or their *_SECRET_NAME references. BYO URLs use version-pinned Key Vault/CSI Secrets and explicit secretKeyRef entries, not ConfigMaps.
Raw URLs and selected versions The manifests stage compares supplied raw URLs against the selected immutable Key Vault versions in memory. A mismatch fails before publication and requires seed-secrets. Reference-only mode resolves identifiers without exporting values.
Database auth PILOTSWARM_USE_MANAGED_IDENTITY=0 selects password auth; 1 selects Entra and requires the externally registered PILOTSWARM_DB_AAD_USER. The AAD-user key is omitted for password BYO.
Blob auth PILOTSWARM_BLOB_USE_MANAGED_IDENTITY is now forwarded independently through both deployed worker entrypoints and the SDK option. It defaults to 1 on this deployment path and must not be disabled merely because the database uses a password.
Ambient shell URLs Ignored with a key-only warning when provisioning Postgres, including when the env file has blank URL keys. Explicit file values and stamp-URL composition remain available. BYO shell overrides are also warned about without exposing values.

Validation is stage-scoped: database-independent --steps invocations do not need BYO credentials. Empty Postgres outputs evict stale cache entries; BYO composition ignores stale provisioned outputs. The conditional module and both guarded output dereferences remain intact.

See the deployment README for setup and split-deployment instructions.

Authorship and review history

The first two commits were produced with Claude Opus 5 and did not provide a working supported BYO deployment. An adversarial GPT-6 Astra review returned BLOCK: boolean rendering, old env compatibility, password auth, shared Blob auth, stage dispatch, and ConfigMap credential exposure were not handled safely.

GPT-6 Astra reworked the change in 329c79bf. An independent Opus 5 reviewer then returned REQUEST-CHANGES, identifying an ambient-URL regression and a stale Key Vault value problem, plus smaller scope and documentation defects. Those findings were addressed in 2263633d. A proposed CSI cold-start regression was withdrawn after source-based rebuttal; versioned Secret references remain. The same reviewer re-read the pushed fixes and returned APPROVE, with all original findings and all open follow-up findings resolved. The independent approval covers the PR changes; the newer-main integration was subsequently tested separately.

Evidence and tested base

Evidence Result and scope
Deployment tests 297 passed, 0 failed, 0 skipped on the merge snapshot (npm run test:deploy-scripts, 24 files). The original-base run was 218; the count is higher because the newer main commits add test files, so the merged suite is a superset. Includes the real deploy CLI with external CLIs simulated, fresh/stale cache paths, old env files, ambient URL rejection, and raw URL/version agreement.
SDK unit tests 974 passed, 0 failed, 0 skipped on the merge snapshot (npm run test:unit --workspace=packages/sdk). This suite contains the Blob/worker-auth and storage-resolver cases relied on by this change.
Build npm run build exits 0 on the merge snapshot, covering packages/sdk, pilotswarm-horizon-store, pilotswarm, the MCP tsc pass, and the portal Vite build.
Offline Kustomize 6 of 6 worker/portal password/Entra cases render on the merge snapshot, including all three portal variants. Every database URL is projected as a versioned Secret reference; no postgres:// or postgresql:// string appears anywhere in the rendered output, and no BYO ConfigMap carries a URL.
Bicep Not recompiled, and not required. deploy/services/base-infra/bicep/main.bicep resolves to the identical blob 4621abc2d13127fd129f741723a93707df83aaab in both the merge tree and the PR head, so the newer main commits do not touch it and the earlier successful az bicep build still applies.
Merge conflicts No conflict markers in any .ts, .mjs or .yaml file. No manual resolution was needed.

Original tested base: 29f7de792913a01ce4ccb71a8973a323967884de.

Merge-result revalidation, 2026-09-16: run against refs/pull/92/merge = commit 82032bcd8ac137c28c73f908f7ac201dfad46fd9, tree a0df94384b16ecb68f8b3973ea7b2fa7648c2887, whose parents are main 091de4c7b175ec19f75fedb57c2515c99592db53 and the pushed PR head 2263633d6426b393b77ab5a272ea87d60e0540fb. The tested tree SHA was verified against GitHub's own merge preview before the suites ran, so the results above describe the exact tree GitHub produces for this merge rather than a locally approximated one.

This revalidation was performed by the Waldemort parent session, separately from the agent that authored the change. An earlier revision of this description reported the merge-snapshot results as 218 deployment tests with a recompiled Bicep template; those figures were not produced by an executed merge-snapshot run and have been replaced above with the observed results.

The eight newer main commits change 3 deploy/ paths and 46 packages/sdk/ paths, overlapping this change in the environment template, worker overlay, SDK worker implementation, and SDK types; dependencies and CMS migrations also change. packages/sdk/src/worker.ts and types.ts are modified by both sides, so a clean merge status was not treated as evidence. After the merge, this change's behavior survives intact: worker.ts still forwards PILOTSWARM_BLOB_USE_MANAGED_IDENTITY only when the SDK option is set, and types.ts still declares blobUseManagedIdentity.

Rotation behavior was also checked empirically: rendering one overlay twice, changing only the Key Vault secret version, produces a different versioned Secret name, and that name appears directly in secretKeyRef.name and the CSI secretProviderClass volume attribute on the pod template. A version change therefore alters the pod template and forces a rollout, which covers value rotation as well as key-set changes.

The validation used an unreferenced merge snapshot. No branch refs or existing worktrees changed; there was no rebase, force-push, or merge. Local bookkeeping commit 2ffc79d9 and pushed commit 2263633d share the complete tree 55564aa21fa717d8815946423b1fa3183e362853; their commit metadata and ancestry differ, not their files. The merge inputs were the pushed commit and newer main, not local HEAD.

Not verified: no live deployment has been run against a real external database. All evidence is unit tests, offline Kustomize renders, and a type-checked build. Live Azure API behavior, CSI synchronization timing, and HorizonDB migrations, extensions, grants and schema isolation remain untested.

Main advanced again after this revalidation, so these results describe the merge with 091de4c7, not with the newest tip at the time of reading.

Deliberately outside this change

  • External database creation, networking, role/Entra grants, extensions, schema isolation, and live HorizonDB compatibility.
  • The existing provisioned bootstrap URL/ConfigMap path. The new secret-safe projection is for BYO; this is not a complete migration of legacy credentials.
  • General runtime-provider/schema environment projection redesign.
  • Live Azure API behavior, CSI synchronization timing, and worker/portal rollout. Switching provisioning off does not delete a server from an earlier incremental deployment.

Adds a deployPostgres bool param (default true) so base-infra can skip
provisioning a PostgreSQL flexible server when the environment already
supplies DATABASE_URL / PILOTSWARM_CMS_FACTS_DATABASE_URL.

Default behaviour is unchanged: deployPostgres defaults to true, so every
existing stamp compiles and deploys byte-identically.

Follows the existing conditional-module convention in this file
(AppGateway, VpnGateway, Foundry).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@waldemort-auto

Copy link
Copy Markdown
Contributor Author

I reviewed this for the next PilotSwarm release payload. The conditional Bicep module compiles, and deployPostgres=true preserves the existing provisioning path. I recommend holding this PR out of that payload until the bring-your-own database path is usable and covered end to end:

  1. The normal deployer cannot turn it off. deploy/services/base-infra/bicep/base-infra.params.template.json does not pass deployPostgres, and deploy/scripts/lib/deploy-bicep.mjs has no override for it. An operator setting an environment value today still gets the default true; only a direct Bicep invocation or another deployer that explicitly passes the parameter can exercise the new path. Please wire an explicit setting through the supported deploy path and document it.
  2. Cached PostgreSQL outputs can point the worker back at the old server. deploy.mjs loads cached Bicep outputs and runs composeDerivedEnv before the Bicep stage. loadCache retains old POSTGRES_FQDN and POSTGRES_AAD_ADMIN_PRINCIPAL_NAME values, and saveCache does not remove them when the new outputs are empty. I reproduced DATABASE_URL pointing at an external database while PILOTSWARM_CMS_FACTS_DATABASE_URL and PILOTSWARM_DB_AAD_USER were derived from the old stamp. The false path needs to ignore/invalidate those cached values and require the external connection/auth settings it actually needs.
  3. The advertised HorizonDB/password-auth case needs an auth decision. The bicep-deploy template sets PILOTSWARM_USE_MANAGED_IDENTITY=1, which drives Azure PostgreSQL Entra authentication for database consumers. A supplied password URL alone does not switch that off, and the current deploy overlays put DATABASE_URL in a ConfigMap. Please specify and test the supported BYO auth modes, including how non-Entra credentials are provided safely.

Validation: I built this PR's main.bicep and confirmed the generated ARM condition and guarded outputs. The existing 12 compose-env tests pass, but there is no test for deployPostgres=false or the cached-output transition.

Addresses review feedback on the conditional Postgres module.

1. Wire it through the supported deploy path. base-infra.params.template.json
   now passes deployPostgres from a new DEPLOY_POSTGRES env key, documented in
   deploy/envs/template.env. Previously only a direct Bicep invocation could
   reach the parameter, so an operator always got the default.

2. Do not let cached outputs point the worker at the old server.
   composeDerivedEnv now drops stale POSTGRES_* under DEPLOY_POSTGRES=0 and
   requires both connection strings; saveCache evicts a key whose new
   deployment output is empty.

3. Make the auth mode explicit. Under DEPLOY_POSTGRES=0,
   PILOTSWARM_USE_MANAGED_IDENTITY=1 now requires PILOTSWARM_DB_AAD_USER.

Tests: compose-env 16/16, bicep-outputs-cache 7/7, az bicep build OK.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@waldemort-auto

Copy link
Copy Markdown
Contributor Author

Pushed d3ef9f79 addressing all three points.

1. Deployer wiring. base-infra.params.template.json now passes deployPostgres from a new DEPLOY_POSTGRES env key, documented in deploy/envs/template.env. The supported deploy path can reach it; previously only a direct Bicep invocation could.

2. Stale cached outputs. Confirmed your reading: composeDerivedEnv runs before the bicep stage, loadCache retains prior keys, and saveCache skipped empty values — so a stale POSTGRES_FQDN could compose PILOTSWARM_CMS_FACTS_DATABASE_URL for the old stamp while DATABASE_URL pointed at the supplied database. Two changes: composeDerivedEnv drops stale POSTGRES_* under DEPLOY_POSTGRES=0 and fails fast unless both connection strings are supplied; saveCache now evicts a key whose new deployment output is empty, so the transition cannot survive into a later run.

3. Auth decision. DEPLOY_POSTGRES=0 with PILOTSWARM_USE_MANAGED_IDENTITY=1 now requires an explicit PILOTSWARM_DB_AAD_USER (the Entra principal registered on your database), or PILOTSWARM_USE_MANAGED_IDENTITY=0 for credentials carried in the supplied URLs. The template default is no longer inherited silently.

Tests. Added 4 compose-env cases — the stale-cache transition, required connection strings, the auth decision, and that DEPLOY_POSTGRES=1/unset are byte-unchanged — plus 2 cache cases for empty-output eviction. compose-env 16/16, bicep-outputs-cache 7/7, az bicep build still passes with the full dependency tree.

Still not covered, and worth saying plainly: no live deployment has been run with DEPLOY_POSTGRES=0. The guards are unit-tested, not end-to-end verified against a real external database.

waldemort-auto Bot and others added 2 commits September 16, 2026 19:24
…safe

Normalize DEPLOY_POSTGRES once with old-env defaults and legacy 1/0 support.
Validate BYO settings only for secret and runtime-manifest consumers.
Seed or resolve versioned Key Vault references and project connection URLs
through CSI-backed Secrets, omitting URLs and unused AAD users from ConfigMaps.
Wire independent Blob managed identity through SDK and worker entrypoints.

Cover the actual deploy CLI, all overlay modes, and runtime option forwarding.
Validation: 207 deploy tests, 16 auth tests, 10 storage tests, SDK build,
Bicep compile, and six offline Kustomize renders. No live database rollout.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Ignore ambient database URLs on the provisioned path, including scaffolded
blank keys, and warn without exposing values. Normalize intent once.
Verify supplied BYO URLs against the selected immutable Key Vault version
in memory; fail before publishing if they differ, without logging responses.
Keep provisioned AAD ConfigMap keys and clarify base/database SPC contracts.

Addresses adversarial review findings with 11 added regression cases.
Validation: 218 deployment tests and six offline Kustomize renders pass.
SDK/Bicep code is unchanged from the earlier validated rework.
No live database or application rollout was performed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants