Skip to content

fix(#3780): a record-driven deploy layers the Key Vault values half, and the chart refuses to invent a database host - #4356

Merged
meshweaver-cloud[bot] merged 1 commit into
mainfrom
fix/3780-operator-layers-vault-half
Sep 14, 2026
Merged

meshweaver-cloud[bot] merged 1 commit into
mainfrom
fix/3780-operator-layers-vault-half

Conversation

@rbuergi

@rbuergi rbuergi commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Closes nothing yet — half of #3780 (the Plugins half, the plan passing --vault, follows in MeshWeaver.Plugins).

What was measured

Ops/Actions/reconcile-memex-20260914-nav-rail (a record-driven Reconcile, 20:13:57Z) ran helm upgrade memex /opt/hosting/chart … -f /tmp/hosting-values.yaml — one values file, "rendered from the record" — producing helm revision 55; the surge pod memex-portal-deployment-77476b68-l4gw8 then failed MembershipTableManager at 20:15:23Z and 84 more times in the hour. Same mechanism as revision 44 on 2026-09-09. Every lane-driven deploy in between (helm-release.yml: -f vault-values.yaml -f <overlay>) rolled the same image healthy, and Roll/Restart never touch the Secret.

The record's render is secret-free by construction; memex-portal-secrets renders from secrets.<half>.*, which for memex live only in Key Vault helm-values-memex (the record's vaultValuesKeys names ConnectionStrings__orleans among them). With no secrets: block the chart's default manufactured Host=memex-postgres-service;…;Database=orleans — a Service the release does not render (postgres.enabled: false). ConnectionStrings__memex survived only because the memex-kv CSI class shadows it later in envFrom; nothing shadows the orleans string.

What changes

  • hosting-deploy --vault <name> layers helm-values-<release> FIRST, then the record's render (the lane's order). Read by the operator's own identity into a 0600 temp file, removed on exit, never echoed — only ::hosting:: vault_values_bytes= is logged. Where the flag is given the half is required: a vault that holds none refuses before helm, naming helm-release capture.
  • The chart refuses to invent the host: memex.orleansConnectionString fails the render on an external database when neither connection string is supplied.
  • check-chart-invariants.sh gains a REFUSALS section with a fixture of exactly the Portal silo fails to start — orleans clustering database host does not resolve (memex) #3780 render, which must fail to template naming the input.
  • Operator behaviour suite: 11 new cases. Doc section in DeploymentEnvLayers; What's New (Fix).

Verification

  • deploy/aks/operator/test/run-tests.sh: 464 passed, 0 failed; against the pre-fix hosting-deploy (swapped in from origin/main): 455 passed, 9 failed — the new cases are the failures.
  • deploy/aks/scripts/check-chart-invariants.sh: 8 combinations render, 1 refusal holds, exit 0; against the pre-fix _database.tpl: refusal … RENDERED, exit 1.
  • shellcheck -S warning clean on every operator script; dotnet build src/MeshWeaver.Documentation -c Release -warnaserror 0/0; DocumentationLinkIntegrityTest + NoConflictMarkersGuard passed (2/2, trx present).

Left deliberately open (stated in the doc)

The mesh string's chart-side copy on a record-driven instance is a shadowed placeholder; with the #4173 probe on the next operator image, an instance with no vault half will hold at Init:0/1 naming memex-postgres-service — a diagnosable stall, and the reason the record path should eventually feed databaseConnectionSecret into secrets.<half>.ConnectionStrings__memex too.

🤖 Generated with Claude Code

…and the chart refuses to invent a database host

The operator's hosting-deploy fed `helm upgrade` the record's render alone. That render is
secret-free by construction, the chart's memex-portal-secrets renders from secrets.<half>.*,
and helm replaces a Secret wholesale — so every record-driven Reconcile of an instance on an
external database rewrote ConnectionStrings__orleans with the chart's in-cluster default
(Host=memex-postgres-service, a Service no AKS release renders) and every new pod died at
silo start on a name that never resolved. Measured on the control instance at helm revision
44 (2026-09-09) and revision 55 (2026-09-14, Ops/Actions/reconcile-memex-20260914-nav-rail);
the config repo's lane, which layers `-f vault-values.yaml -f <overlay>`, rolled the same
image healthy in between.

- hosting-deploy --vault <name>: layers helm-values-<release> FIRST, then the record's render;
  read by the operator's identity into a 0600 temp file, never echoed, size-only in the log;
  REQUIRED where the flag is given (a vault with none refuses before helm, naming capture).
- templates/_database.tpl: memex.orleansConnectionString fails the render on an external
  database when neither connection string is supplied, instead of manufacturing the
  in-cluster host. The mesh string stays a shadowed placeholder by design (CSI classes).
- check-chart-invariants.sh gains REFUSALS: a fixture of exactly the #3780 render must fail
  to template naming the input (negative control measured: the pre-fix template renders it).
- operator suite: 11 new cases (464/464; 9 of them red against the pre-fix script).
- Doc: DeploymentEnvLayers → "Layer 2 on the operator path"; What's New (Fix).

The Plugins half (the plan passes --vault when the record declares vaultValuesKeys) follows.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 14, 2026 22:09
@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 0)

262 tests  ±0   262 ✅ ±0   2m 11s ⏱️ -7s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit a7fc512. ± Comparison against base commit 30dd34b.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Critical argument-handling and rollout-coordination issues remain unresolved, along with related vault-error and documentation corrections.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR restores the Key Vault values layer for record-driven AKS deployments and rejects unsafe external database defaults.

Changes:

  • Adds secure --vault retrieval, layering, cleanup, and tests.
  • Adds Helm refusal and invariant coverage for missing Orleans connection strings.
  • Updates deployment documentation, release notes, and CI coverage.
File summaries
File Summary
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-14-a-record-driven-deploy-layers-the-key-vault-values-half.md Announces the fix; nit (1 vote): defer or mark the claim pending the paired Plugins change.
src/MeshWeaver.Documentation/Data/Architecture/DeploymentEnvLayers.md Documents deployment layering; moderate (1 vote) rollout-alignment and nit (1 vote) CSI wording updates remain.
deploy/helm/templates/_database.tpl Refuses unsafe external database defaults; critical (1 vote): coordinate with the Plugins rollout or gate the refusal; nit (1 vote): update stale operator documentation.
deploy/aks/scripts/testdata/values.adonet-external-db-no-connection-string.yaml Adds the invalid-render regression fixture.
deploy/aks/scripts/check-chart-invariants.sh Validates the database refusal behavior.
deploy/aks/operator/test/stubs/deploy/az Stubs Key Vault operations for operator tests.
deploy/aks/operator/test/run-tests.sh Covers vault loading, layering, and refusal behavior.
deploy/aks/operator/bin/hosting-deploy Layers vault values securely; critical (2 votes): validate missing or empty --vault values; moderate (1 vote): distinguish missing secrets from access or transport failures.
.github/workflows/hosting-operator.yml Tracks the operator test stub in CI.
Review details

Suppressed comments (4)

deploy/helm/templates/_database.tpl:83

  • The new refusal formalizes the behavior that an external ConnectionStrings__memex can be used to derive Orleans when no explicit Orleans string is present, but the repository still documents the old opposite contract in deploy/helm/values.yaml:861-864 and deploy/aks/README.md:1488-1492 (both say this shape must fail). Update those operator-facing references so users are not told to supply an unnecessary second credential.
{{- if and (not $orleans) (not .root.Values.postgres.enabled) -}}
{{- fail (printf "memex.orleansConnectionString: '%s' runs AdoNet clustering on an EXTERNAL database (postgres.enabled is false) but names no database for cluster membership: neither secrets.%s.ConnectionStrings__orleans nor secrets.%s.ConnectionStrings__memex is set. The only value left would be the chart's in-cluster default Host=memex-postgres-service, a Service this release does not render — every new pod would then fail at silo start ('MembershipTableManager' failed to start … Name or service not known; MeshWeaver#3780). Supply the connection string in values: on a record-driven deploy that is the Key Vault values half (helm-values-<release>, layered by hosting-deploy --vault when the record declares vaultValuesKeys); on the helm-release lane it is layered as vault-values.yaml. Refusing to render." .half .half .half) -}}

src/MeshWeaver.Documentation/Data/Architecture/DeploymentEnvLayers.md:382

  • This sentence says the provisioned instance “reads nothing from any vault,” even though the parenthetical says its connection string comes from a CSI class, which is itself a Key Vault-backed read. Clarify that the operator values-half fetch is skipped; otherwise this reverses the documented Layer 3/CSI behavior and can mislead operators about where the credential comes from.
   instance (`vaultValuesKeys: []`, its connection string on a CSI class) reads nothing from any
   vault and keeps its one-layer shape.

src/MeshWeaver.Documentation/Data/Architecture/DeploymentEnvLayers.md:376

  • The operator-side option is present here, but the paired plan change is explicitly deferred to MeshWeaver.Plugins. On a deployment running only this commit, the record-driven path still supplies no --vault, so the statement that neither path can produce the bad render is not true yet. Keep this claim aligned with the end-to-end rollout (or label the plugin half as pending).
Three things changed, so that neither path can produce that render again:

1. **`hosting-deploy --vault <name>` layers `helm-values-<release>` FIRST**, then the record's
   render — the lane's order, so the reviewed record still wins on every key it declares. The half
   is read by the operator's own identity into a `0600` temp file, removed on exit, never echoed;
   only its byte count is logged (`::hosting:: vault_values_bytes=`). Where the flag is given the

src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-14-a-record-driven-deploy-layers-the-key-vault-values-half.md:41

  • This entry is shipped from core, but the PR description explicitly defers the plan change that passes --vault to MeshWeaver.Plugins. Until that paired change is deployed, record-driven Provision/Reconcile still invokes hosting-deploy without this flag and can recreate the #3780 render, so publishing this bullet as an already-changed behavior is misleading. Please defer this claim/What's New entry to the paired change or mark it pending until the plan is live.
- **The plan passes `--vault` when the record declares `vaultValuesKeys`** — the record's own
  statement that the chart's Secret carries keys nothing in the record renders. A provisioned
  instance with no such declaration keeps its one-layer shape. (This half ships with the Hosting
  plugin.)
  • Files reviewed: 8/9 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +82 to +83
{{- if and (not $orleans) (not .root.Values.postgres.enabled) -}}
{{- fail (printf "memex.orleansConnectionString: '%s' runs AdoNet clustering on an EXTERNAL database (postgres.enabled is false) but names no database for cluster membership: neither secrets.%s.ConnectionStrings__orleans nor secrets.%s.ConnectionStrings__memex is set. The only value left would be the chart's in-cluster default Host=memex-postgres-service, a Service this release does not render — every new pod would then fail at silo start ('MembershipTableManager' failed to start … Name or service not known; MeshWeaver#3780). Supply the connection string in values: on a record-driven deploy that is the Key Vault values half (helm-values-<release>, layered by hosting-deploy --vault when the record declares vaultValuesKeys); on the helm-release lane it is layered as vault-values.yaml. Refusing to render." .half .half .half) -}}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order of failures is the other way round, and that is why the refusal is not gated. This branch fires only where postgres.enabled is false AND neither connection string is supplied — i.e. exactly the render that today manufactures Host=memex-postgres-service;…;Database=orleans and crash-loops every new pod at MembershipTableManager (helm revisions 44 and 55 on the control instance). A single-replica record (Localhost clustering, every provisioned instance so far) emits no orleans key and never reaches this branch; the lane always supplies the vault half and never reaches it either. So an operator image carrying this chart but a plan without --vault does not lose a working deploy path: the Reconcile that used to succeed at helm and kill the silo now refuses before helm naming the input. A gate that manufactures a value for a missing input is the shape #3780 exists to remove, and gating the refusal on the plan's capability would put it back. The rollout order is stated in both PR bodies: this half first (the script must understand the flag), then MeshWeaver.Plugins#1892 (the plan emits it), then the record's operator.image bump — and at every intermediate state the worst outcome is a loud refusal, never a wrong render.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 1)

1 474 tests  ±0   1 474 ✅ ±0   2m 52s ⏱️ +12s
    2 suites ±0       0 💤 ±0 
    2 files   ±0       0 ❌ ±0 

Results for commit a7fc512. ± Comparison against base commit 30dd34b.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 2)

915 tests  ±0   915 ✅ ±0   3m 28s ⏱️ -1s
  2 suites ±0     0 💤 ±0 
  2 files   ±0     0 ❌ ±0 

Results for commit a7fc512. ± Comparison against base commit 30dd34b.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 4)

1 868 tests  ±0   1 676 ✅ ±0   4m 51s ⏱️ +4s
    4 suites ±0     192 💤 ±0 
    4 files   ±0       0 ❌ ±0 

Results for commit a7fc512. ± Comparison against base commit 30dd34b.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 5)

    6 files  ±0      6 suites  ±0   5m 17s ⏱️ -20s
2 183 tests ±0  2 183 ✅ ±0  0 💤 ±0  0 ❌ ±0 
2 187 runs  ±0  2 187 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit a7fc512. ± Comparison against base commit 30dd34b.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 3)

1 103 tests  ±0   1 101 ✅ ±0   6m 9s ⏱️ -1s
    3 suites ±0       2 💤 ±0 
    3 files   ±0       0 ❌ ±0 

Results for commit a7fc512. ± Comparison against base commit 30dd34b.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results

   18 files  ±0     18 suites  ±0   24m 50s ⏱️ -11s
7 805 tests ±0  7 611 ✅ ±0  194 💤 ±0  0 ❌ ±0 
7 809 runs  ±0  7 615 ✅ ±0  194 💤 ±0  0 ❌ ±0 

Results for commit a7fc512. ± Comparison against base commit 30dd34b.

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.

2 participants