From 6c2042a043346d05da85549fdfbf8aed4d668c62 Mon Sep 17 00:00:00 2001 From: NanHan Qing <157583847+nanhanq1@users.noreply.github.com> Date: Fri, 23 Oct 2026 00:26:48 +0800 Subject: [PATCH] fix(helm): give each Secret pair one precedence chain (#7291) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(helm): derive the postgres Secret name from one helper `databaseUrlSecret` re-encoded the `postgresql.external.existingSecret` then `postgresql.existingSecret` chain in its own `if`/`else if`. Three consumers read it — the gateway's `DATABASE_URL`, the postgres StatefulSet's `POSTGRES_PASSWORD`, and `postgres-secret.yaml`'s own `metadata.name` — so a source added to one helper and not the other would make the gateway and the StatefulSet resolve different Secrets. That is the class of bug #7149 fixed for the redis pair. Extract `databaseUserSecret` as the single encoding of the chain and let `databaseUrlSecret` fall back to the chart-managed name when it is empty, mirroring `redisUserSecret` / `redisUrlSecret`. Pure refactor: the rendered manifest is byte-identical before and after across the four configurations (chart-managed, `existingSecret`, `external.existingSecret`, and both set), compared with `helm template`. Adds `test_helm_database_secret_precedence.py`, which pins that both consumers name the same Secret and that only the chart-managed branch renders a Secret. Dropping either precedence source fails the two cases that depend on it. Suggested in review of #7161. * fix(helm): keep the postgres Secret helper's truthiness Review follow-up on the previous commit. Two things were still wrong: `postgres-secret.yaml` kept its own copy of the precedence chain for the render gate, so "the only place the chain is encoded" was not yet true — a source added to `databaseUserSecret` alone would have left that gate empty and rendered an unreferenced `-postgres` Secret. It now reads the helper, like `redis-secret.yaml` already did. `or` returns its last operand even when it is falsy, so `postgresql.existingSecret: false` (how people spell "not set") named the Secret `false` instead of falling back to the chart-managed one. Chaining `default` keeps the old if/else-if truthiness and yields an empty string for any falsy value; the gateway's `$pgConfigured` now reads the same helper rather than re-listing the two keys. Two falsy cases join the parametrize list: `false` in the lower-precedence slot and in both slots. Both fail against the previous `or` version with `assert [] == [False]` — the Secret literally named `false` — so they are regressions this test can actually catch. The other four configurations render byte-identically before and after. * fix(helm): keep the redis user-Secret helper's truthiness Same defect the postgres helper had, one release later: `redisUserSecret` used `or`, which returns its last operand even when falsy, so `redis.existingSecret: false` named both the gateway's `redis-url` ref and the StatefulSet's `redis-password` ref after a Secret called `false`. The render gate in `redis-secret.yaml` saw the string "false" as truthy, so it skipped creating the chart-managed Secret — leaving two dangling refs and a StatefulSet that cannot start. `default` chaining restores the pre-`redisUserSecret` if/else-if behaviour: any falsy value yields an empty string, so the chart-managed branch is taken. `redisConfigured` now reads the helper instead of re-listing the two keys, matching the gateway's `$pgConfigured`. Verified against the four string-valued configurations (chart-managed, `existingSecret`, `external.existingSecret`, both): renders are byte-identical before and after. The two new falsy cases fail on the previous version, and `--basetemp` makes all 14 cases in this file pass. * docs(helm): drop a stray article in the helper comment --- backend/tests/AGENTS.md | 12 +++ .../test_helm_database_secret_precedence.py | 94 +++++++++++++++++++ .../tests/test_helm_redis_existing_secret.py | 21 +++++ deploy/helm/deer-flow/templates/_helpers.tpl | 28 ++++-- .../templates/gateway-deployment.yaml | 2 +- .../deer-flow/templates/postgres-secret.yaml | 2 +- 6 files changed, 151 insertions(+), 8 deletions(-) create mode 100644 backend/tests/test_helm_database_secret_precedence.py diff --git a/backend/tests/AGENTS.md b/backend/tests/AGENTS.md index e98283a7a4..c0725dcf30 100644 --- a/backend/tests/AGENTS.md +++ b/backend/tests/AGENTS.md @@ -371,6 +371,18 @@ and both probe commands with argument-recording Redis stubs through `redis.auth.password` values, Secret precedence, external mode and shell metacharacters covered. Render tests skip when Helm is unavailable; they need no Kubernetes cluster. +A falsy `existingSecret` (`false` is how people spell "not set") must fall +through to the chart-managed Secret, not name the Secret after the value. + +## Helm postgres Secret precedence + +`test_helm_database_secret_precedence.py` pins that the gateway's `DATABASE_URL` +and the postgres StatefulSet's `POSTGRES_PASSWORD` resolve the same Secret in +every branch of `deer-flow.databaseUserSecret`, and that only the chart-managed +branch renders a Secret. Teaching one consumer a new Secret source without the +other has to fail here. The falsy cases exist because `or` returns its last +operand even when falsy, which would name the Secret `false`. + ## Multi-instance harness Multi-instance harness tests use `support.shell.require_script_bash()` for shell diff --git a/backend/tests/test_helm_database_secret_precedence.py b/backend/tests/test_helm_database_secret_precedence.py new file mode 100644 index 0000000000..d4d36346ca --- /dev/null +++ b/backend/tests/test_helm_database_secret_precedence.py @@ -0,0 +1,94 @@ +"""The gateway and the bundled postgres must resolve the same Secret. + +`deer-flow.databaseUrlSecret` is read by three consumers — the gateway's +`DATABASE_URL`, the postgres StatefulSet's `POSTGRES_PASSWORD`, and +`postgres-secret.yaml`'s own `metadata.name` — so the precedence chain +(`postgresql.external.existingSecret` → `postgresql.existingSecret` → +`-postgres`) has to exist once. This suite pins that chain and that +every consumer follows it. +""" + +from __future__ import annotations + +import shutil +import subprocess +from pathlib import Path + +import pytest +import yaml + +REPO_ROOT = Path(__file__).resolve().parents[2] +CHART = REPO_ROOT / "deploy" / "helm" / "deer-flow" +RELEASE = "deer-flow" + + +def _render_chart(*settings: str) -> list[dict]: + helm = shutil.which("helm") + if helm is None: + pytest.skip("helm is unavailable") + # Pin the generated app secrets so a render is reproducible. + command = [helm, "template", RELEASE, str(CHART), "--set", "existingAppSecret=fixed-app-secret"] + for setting in settings: + command.extend(["--set", setting]) + rendered = subprocess.run(command, check=True, capture_output=True, text=True, encoding="utf-8").stdout + return [document for document in yaml.safe_load_all(rendered) if isinstance(document, dict)] + + +def _container(documents: list[dict], name: str) -> dict: + workload = next(document for document in documents if document.get("kind") in {"Deployment", "StatefulSet"} and document["metadata"]["name"].endswith(f"-{name}")) + return next(container for container in workload["spec"]["template"]["spec"]["containers"] if container["name"] == name) + + +def _secret_ref(container: dict, env_name: str) -> dict: + return next(item for item in container["env"] if item["name"] == env_name)["valueFrom"]["secretKeyRef"] + + +def _postgres_secrets(documents: list[dict]) -> list[str]: + return sorted(document["metadata"]["name"] for document in documents if document.get("kind") == "Secret" and document["metadata"]["name"].endswith("-postgres")) + + +@pytest.mark.parametrize( + ("settings", "expected"), + [ + ((), None), + (("postgresql.existingSecret=custom-pg",), "custom-pg"), + (("postgresql.external.existingSecret=ext-pg",), "ext-pg"), + (("postgresql.existingSecret=custom-pg", "postgresql.external.existingSecret=ext-pg"), "ext-pg"), + # Falsy non-string values are how people spell "not set" in values.yaml. + # The old if/else-if treated them as unset, and the helper has to keep + # doing that: `or` returns its last operand even when it is falsy, which + # would name the Secret `false`. `default ""` yields an empty string + # instead, so the chart-managed branch is taken. + (("postgresql.existingSecret=false",), None), + (("postgresql.external.existingSecret=false", "postgresql.existingSecret=false"), None), + ], + ids=[ + "chart-managed", + "existing-secret", + "external-secret", + "external-wins", + "existing-secret-false", + "both-secrets-false", + ], +) +def test_gateway_and_postgres_resolve_the_same_secret(settings: tuple[str, ...], expected: str | None) -> None: + documents = _render_chart(*settings) + + gateway_ref = _secret_ref(_container(documents, "gateway"), "DATABASE_URL") + postgres_ref = _secret_ref(_container(documents, "postgres"), "POSTGRES_PASSWORD") + assert gateway_ref["key"] == "database-url" + assert postgres_ref["key"] == "postgres-password" + + # The point of the shared helper: both consumers name the same Secret, so a + # source added to one of them cannot silently split the credentials. + assert gateway_ref["name"] == postgres_ref["name"] + + secrets = _postgres_secrets(documents) + if expected is None: + # Chart-managed branch: the chart creates the Secret the consumers name. + assert secrets == [gateway_ref["name"]] + else: + # User-managed branches: the Secret must already exist, so rendering one + # would shadow the operator's. + assert gateway_ref["name"] == expected + assert secrets == [] diff --git a/backend/tests/test_helm_redis_existing_secret.py b/backend/tests/test_helm_redis_existing_secret.py index c1d00528b8..56604feffd 100644 --- a/backend/tests/test_helm_redis_existing_secret.py +++ b/backend/tests/test_helm_redis_existing_secret.py @@ -94,6 +94,27 @@ def test_bundled_redis_uses_optional_existing_secret_password(password: str | No assert argv == ["redis-cli", *(["--no-auth-warning", "-a", password] if password else []), "ping"] +@pytest.mark.parametrize( + "settings", + [ + ("redis.existingSecret=false",), + ("redis.external.existingSecret=false", "redis.existingSecret=false"), + ], + ids=["existing-secret-false", "both-secrets-false"], +) +def test_falsy_existing_secret_still_uses_the_chart_managed_secret(settings: tuple[str, ...]) -> None: + # `false` is how people spell "not set" in values.yaml. `or` returns its + # last operand even when falsy, so it would name the Secret `false` — and + # nothing creates a Secret by that name, leaving both refs dangling. + documents = _render_chart(*settings, f"redis.auth.password={PASSWORD}") + gateway_ref = _secret_ref(_container(documents, "gateway"), "DEER_FLOW_STREAM_BRIDGE_REDIS_URL") + redis_ref = _secret_ref(_container(documents, "redis"), "REDIS_PASSWORD") + assert isinstance(gateway_ref["name"], str) + assert gateway_ref == {"name": redis_ref["name"], "key": "redis-url"} + assert redis_ref["key"] == "redis-password" + assert any(document.get("kind") == "Secret" and document["metadata"]["name"] == gateway_ref["name"] for document in documents) + + @pytest.mark.parametrize("enabled", ["true", "false"]) def test_external_secret_precedence_is_unchanged(enabled: str) -> None: # The existing helper gives external.existingSecret precedence in both modes. diff --git a/deploy/helm/deer-flow/templates/_helpers.tpl b/deploy/helm/deer-flow/templates/_helpers.tpl index d3bbb6959c..8812a73c88 100644 --- a/deploy/helm/deer-flow/templates/_helpers.tpl +++ b/deploy/helm/deer-flow/templates/_helpers.tpl @@ -133,6 +133,20 @@ imagePullSecrets: {{- printf "%s-postgres" (include "deer-flow.fullname" .) -}} {{- end -}} +{{/* Name of the operator-managed postgres Secret, or empty when the chart owns + the credentials. This is the only place the precedence chain is encoded: + databaseUrlSecret, the gateway's DATABASE_URL, the StatefulSet's + POSTGRES_PASSWORD and postgres-secret.yaml's render gate all read it, so a + source added here cannot make them disagree. + + `default` rather than `or`: `or` returns its last operand even when falsy, + so `existingSecret: false` (or `0`) would render the literal name + "false". Chaining `default` keeps the old if/else-if truthiness and yields + an empty string for any falsy value. */}} +{{- define "deer-flow.databaseUserSecret" -}} +{{- .Values.postgresql.external.existingSecret | default .Values.postgresql.existingSecret | default "" -}} +{{- end -}} + {{/* Name of the Secret holding DATABASE_URL (and, in bundled mode, the postgres superuser password). Resolution order: 1. postgresql.external.existingSecret (user-managed, key=database-url) @@ -140,9 +154,7 @@ imagePullSecrets: 3. chart-managed secret `-postgres` Only #3 is created by this chart; #1/#2 must exist already. */}} {{- define "deer-flow.databaseUrlSecret" -}} -{{- if .Values.postgresql.external.existingSecret -}}{{- .Values.postgresql.external.existingSecret -}} -{{- else if .Values.postgresql.existingSecret -}}{{- .Values.postgresql.existingSecret -}} -{{- else -}}{{- include "deer-flow.postgresFullname" . -}}{{- end -}} +{{- include "deer-flow.databaseUserSecret" . | default (include "deer-flow.postgresFullname" .) -}} {{- end -}} {{/* Name of the redis StatefulSet/Service. */}} @@ -163,9 +175,13 @@ imagePullSecrets: {{/* Name of the operator-managed redis Secret, or empty when the chart owns the credentials. Only the chart-managed Secret is created by this chart, and only there is the AUTH password known at render time — a user-managed - Secret may or may not carry `redis-password`. */}} + Secret may or may not carry `redis-password`. + + Same truthiness rule as databaseUserSecret: `default` (not `or`) so a + falsy `existingSecret` yields an empty string instead of the literal name + "false". */}} {{- define "deer-flow.redisUserSecret" -}} -{{- or .Values.redis.external.existingSecret .Values.redis.existingSecret -}} +{{- .Values.redis.external.existingSecret | default .Values.redis.existingSecret | default "" -}} {{- end -}} {{/* Args for the bundled redis container. With a user-managed Secret the @@ -201,7 +217,7 @@ redis-cli ping external URL, or a user-managed Secret). Drives the env injection in the gateway deployment. */}} {{- define "deer-flow.redisConfigured" -}} -{{- or .Values.redis.enabled .Values.redis.external.redisUrl .Values.redis.external.existingSecret .Values.redis.existingSecret -}} +{{- or .Values.redis.enabled .Values.redis.external.redisUrl (include "deer-flow.redisUserSecret" .) -}} {{- end -}} {{/* SHA256 checksums of the ConfigMaps. Mount these as pod-template diff --git a/deploy/helm/deer-flow/templates/gateway-deployment.yaml b/deploy/helm/deer-flow/templates/gateway-deployment.yaml index d0cb228ac1..df1d73aa05 100644 --- a/deploy/helm/deer-flow/templates/gateway-deployment.yaml +++ b/deploy/helm/deer-flow/templates/gateway-deployment.yaml @@ -195,7 +195,7 @@ spec: (bundled StatefulSet, external databaseUrl, or an existing Secret). In sqlite/memory mode the env is omitted and config.yaml's database.postgres_url is never read. */ -}} - {{- $pgConfigured := or .Values.postgresql.enabled .Values.postgresql.external.databaseUrl .Values.postgresql.external.existingSecret .Values.postgresql.existingSecret -}} + {{- $pgConfigured := or .Values.postgresql.enabled .Values.postgresql.external.databaseUrl (include "deer-flow.databaseUserSecret" .) -}} {{- if $pgConfigured }} - name: DATABASE_URL valueFrom: diff --git a/deploy/helm/deer-flow/templates/postgres-secret.yaml b/deploy/helm/deer-flow/templates/postgres-secret.yaml index e81c4e5913..83eefeb704 100644 --- a/deploy/helm/deer-flow/templates/postgres-secret.yaml +++ b/deploy/helm/deer-flow/templates/postgres-secret.yaml @@ -8,7 +8,7 @@ NOT created when the user manages the Secret themselves: • postgresql.external.existingSecret OR postgresql.existingSecret → referenced only */ -}} -{{- $userSecret := or .Values.postgresql.external.existingSecret .Values.postgresql.existingSecret -}} +{{- $userSecret := include "deer-flow.databaseUserSecret" . -}} {{- $wrapExternal := and (not .Values.postgresql.enabled) .Values.postgresql.external.databaseUrl -}} {{- if and (not $userSecret) (or .Values.postgresql.enabled $wrapExternal) -}} {{- $password := "" -}}