mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 13:16:24 +00:00
fix(helm): give each Secret pair one precedence chain (#7291)
* 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 `<release>-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
This commit is contained in:
parent
2f0cd407fc
commit
6c2042a043
@ -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
|
||||
|
||||
94
backend/tests/test_helm_database_secret_precedence.py
Normal file
94
backend/tests/test_helm_database_secret_precedence.py
Normal file
@ -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` →
|
||||
`<release>-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 == []
|
||||
@ -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.
|
||||
|
||||
@ -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 `<release>-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
|
||||
|
||||
@ -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:
|
||||
|
||||
@ -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 := "" -}}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user