fix: keepalived validation gate and deploy acknowledgements (v1.10.13)

Two agent-side fixes found while taking the VIP adoption flow through a real
HA pair, released together.

1. A VALID CONFIG WAS REJECTED BY ITS OWN WARNING (v1.10.12)

Before writing a rendered keepalived.conf the agent validates it with
keepalived -t and, on failure, keeps the running config and does not restart
keepalived. That fail-safe is right, but it treated ANY non-zero exit as
invalid - and keepalived's config-test exit code does not separate fatal from
benign. Measured on 2.2.8:

    clean config ..................... 0
    auth_pass longer than 8 chars .... 5   "Truncating auth_pass to 8 characters"
    missing '}' ...................... 5   "There are 1 missing '}'s"
    unknown keyword .................. 5   "Unknown keyword '...'"
    script without script_security ... 6   "SECURITY VIOLATION ..."

Exit 5 covers both a harmless truncation and a broken file, so a VRRP password
over eight characters was enough to block every apply - including on a node
whose own running config emits the same warning and had been serving the VIP
for weeks. Accepting exit 5 would have accepted broken configs, so the gate now
judges the OUTPUT: known-benign messages are dropped and anything remaining
still fails. It fails CLOSED - an unrecognised message, or a non-zero exit with
no readable output at all, is fatal - and the filter is an allowlist, never a
denylist. The agent also reports what keepalived said, in its log and in the
status the HA/VIP page shows; discarding it left a correct refusal that nobody
could act on.

2. EVERY DEPLOY ACKNOWLEDGEMENT WAS DROPPED

The takeover-retirement clause on POST /agents/{name}/keepalived-status reused
one placeholder for both the assignment `last_deploy_hash=$n` and the
comparison inside its CASE. PostgreSQL types a placeholder per USE, so it came
out as text in one and character varying in the other and asyncpg rejected the
statement with AmbiguousParameterError. The whole UPDATE never ran, so no
member recorded an acknowledgement: VIPs sat at SYNCING with an empty Last ack
while the nodes were verifiably running the config, and teardown acks were lost
the same way. The hash now has its own placeholder, compared only against the
column.

Verified against real keepalived and a real PostgreSQL rather than by
inspection, including on busybox and bash 3.2, and a test asserts every $n in
those statements is bound exactly once.
This commit is contained in:
taylanbakircioglu
2026-08-14 18:38:36 +03:00
parent 9e64002f1c
commit 0eb587dfa8
7 changed files with 237 additions and 18 deletions
+2
View File
@@ -2474,6 +2474,8 @@ Developed with ❤️ for the HAProxy community
## Release Notes
- **v1.10.13** (2026-08-14) — **Agent deploy acknowledgements were silently dropped** (regression in v1.10.12, fix it before or with that release): the takeover-retirement clause added to `POST /agents/{name}/keepalived-status` in v1.10.12 reused one query placeholder for both the assignment `last_deploy_hash=$n` and the comparison inside its `CASE`. PostgreSQL deduces a type per **use**, so the same placeholder came out as `text` in one and `character varying` in the other, and asyncpg rejected the statement with `AmbiguousParameterError`. The failure was not partial: the whole UPDATE never ran, so **no member ever recorded an acknowledgement**. Every VIP sat at `SYNCING (0/n)` with an empty *Last ack*, even after the nodes had deployed the config successfully, and teardown acknowledgements were lost the same way. The hash is now bound to its own placeholder, which is only ever compared against the column and therefore unambiguous. Verified against a real PostgreSQL: both statements execute, a matching hash retires the takeover authorisation, a non-matching hash and a NULL `applied_config_hash` both leave it in place, and every case records the acknowledgement. A test now asserts every `$n` in these statements is bound exactly once and that the count matches the arguments passed. Backend only: no schema, agent or API-shape change.
- **v1.10.12** (2026-08-14) — **A valid keepalived config is no longer rejected by its own warning**: before writing a rendered `keepalived.conf` the agent validates it with `keepalived -t` and, on failure, keeps the running config and does not restart keepalived. That fail-safe is right, but it treated **any** non-zero exit as invalid, and keepalived's config-test exit code does not separate fatal from benign. Measured on 2.2.8: a clean config exits 0, but `Truncating auth_pass to 8 characters` exits **5** and so does a missing `}` or an `Unknown keyword`. A VRRP password longer than eight characters was therefore enough to make every apply fail, including on nodes whose own running config produces the same warning and has been serving the VIP for weeks. The gate now judges the **output**: messages known to be benign are dropped and anything that remains still fails, so it fails **closed** and an unrecognised message is treated as fatal. Verified against real keepalived: a truncation warning passes while a missing brace, an unknown keyword and a `SECURITY VIOLATION` are all still refused. The agent also **reports what keepalived said** now, in the log and in the status the HA/VIP page shows; discarding it left a correct refusal with no way to act on it. Agent-script change: sync the script from Agent Management and let the agents upgrade for it to take effect. No schema or API change.
- **v1.10.11** (2026-08-14) — **The *Adoptable* tag names the problem that actually blocks adoption**: the tag and the disabled *Adopt* button were computed separately and could disagree. A pair blocked because its peer's `keepalived.conf` could not be parsed was labelled **MASTER missing** — technically true, since the unreadable node's `state MASTER` had not been counted, but it pointed the operator at the wrong node while the real reason sat in the button's own tooltip. Both now come from one ordered decision, so the label, its colour and the tooltip always describe the condition that stops the adoption; a group held up by an unreadable or unreachable peer reads **blocked by peer**, and two MASTERs is now distinct from none. Display only: what the endpoint accepts or refuses is unchanged. On the public repo this is the first artifact carrying v1.10.4 through v1.10.10: none was released separately, because VIP adoption did not work end to end until these fixes landed.
- **v1.10.10** (2026-08-14) — **Adoption blockers are listed once per instance**: with the instance-based panel a two-node pair reported the *same* problems about the *same* shared config twice, once per member, and the line numbers differ between the two files so plain de-duplication did not collapse them. Four issues on a pair read as eight, in both the *Adoptable* tooltip and the adopt dialog. They are now merged on the message text with the leading `line N:` ignored, so each distinct problem appears once. Display only: the endpoint already evaluated the combined set and its refusals are unchanged.
- **v1.10.9** (2026-08-14) — **Adoption refuses to strand a node or silently normalise a peer's settings**: v1.10.8 adopted the whole VRRP instance, but it could only *match* a node it was able to read, that was enabled, and that sat in the same pool. Each of those was a door a real member of the group left through silently — the nodes that remained were rewritten while the one that left kept serving the same address from an unmanaged config. Seen on a live pool: one node of a pair had an unclosed `vrrp_instance` block, so it parsed to nothing while its partner parsed cleanly. Instead of guarding each door, adoption now asks the question directly — is there **any** reported `keepalived.conf` that mentions this virtual address and is not among the nodes being taken over — and refuses naming the node and the reason (unparseable, agent disabled, different pool). Separately, four VIP-level fields (`prefix_length`, unicast/multicast mode, HAProxy tracking and the VRRP password) are stored once and re-rendered onto **every** member, so taking them from whichever node was clicked imposed its settings on the others; the prefix length is the sharpest, because the design refuses to *guess* a netmask for a live VIP and copying one node's netmask onto another is that same change by another name. Adoption now requires the nodes to agree on all four, and compares the VRRP secret by decrypting each node's token (Fernet is non-deterministic, so the ciphertexts cannot be compared). Finally, the takeover authorisation is genuinely one-shot: `takeover_expected_hash` was written at adoption and never cleared, so it stayed valid for that file content indefinitely — it is now retired the moment a member acknowledges our rendered config, gated on the acked hash matching so a failed deploy never drops it. No schema change, no agent change.
+50
View File
@@ -1,3 +1,53 @@
# Upgrade Notes — v1.10.13 (deploy acknowledgements were dropped)
**Backend only, no schema change.** No `SCHEMA_VERSION` bump, no agent change. If you deployed
v1.10.12, deploy this one too.
- **Regression in v1.10.12.** The takeover-retirement clause added to the keepalived status
endpoint reused a query placeholder for both an assignment and a comparison. PostgreSQL types a
placeholder per use, so it was deduced as `text` in one place and `character varying` in the
other, and asyncpg refused the statement outright.
- **Symptom:** a VIP stayed at `SYNCING (0/n)` with an empty *Last ack* even though the agent log
showed `applied config for VIP <id>` on every member. Teardown acknowledgements were lost the
same way, so a deletion never showed as complete.
- **Nothing was damaged.** The failure was on the write of the acknowledgement, not on the node.
Configs were deployed correctly throughout; only the reporting was lost. Existing VIPs converge
on the next poll once this is deployed, with no action on the nodes.
- **Verified against a real PostgreSQL**, not by inspection: both statements execute, a matching
hash retires the takeover authorisation, a non-matching hash and a NULL `applied_config_hash`
leave it in place, and the acknowledgement is recorded in every case.
**Rollback:** do not roll back to v1.10.12; roll back to v1.10.11 instead, which predates the
clause entirely.
---
# Upgrade Notes — v1.10.12 (valid config rejected by its own warning)
**Agent-script change, no schema change.** No `SCHEMA_VERSION` bump. After deploying, sync the
Linux agent script from **Agent Management** and let the agents upgrade, or the fix does not
reach the nodes.
- **Symptom:** applying a VIP left it stuck at `SYNCING`, the node kept its previous config and
the agent logged only `config validation failed (keepalived -t)`.
- **Cause:** the agent treated any non-zero exit from `keepalived -t` as invalid. keepalived's
config-test exit code does not separate fatal from benign: on 2.2.8 a clean config exits 0,
while `Truncating auth_pass to 8 characters` exits 5 and so do a missing `}` and an unknown
keyword. A VRRP password longer than eight characters was enough to block every apply, even on
a node whose own running config emits the same warning.
- **Fix:** the gate judges the output instead. Known-benign messages are dropped and anything
left still fails, so it fails closed. Verified against real keepalived: the truncation warning
passes; a missing brace, an unknown keyword and a `SECURITY VIOLATION` are refused.
- **Also:** the agent now reports what keepalived actually said, in its log and in the status the
HA/VIP page shows. The refusal was correct but unactionable without reproducing it by hand.
- **The fail-safe itself is unchanged:** a config that genuinely fails validation is never
written and keepalived is never restarted.
**Rollback:** safe. Reverting restores the stricter gate, which rejects valid configs whose
password exceeds eight characters.
---
# Upgrade Notes — v1.10.11 (Adoptable tag names the real blocker)
**Frontend only, no schema change.** No `SCHEMA_VERSION` bump, no API change, no agent change.
+12 -5
View File
@@ -2433,23 +2433,30 @@ async def agent_keepalived_status(agent_name: str, status_data: dict, x_api_key:
#
# Gated on the acked hash MATCHING applied_config_hash, so a partial or failed deploy
# never drops the authorisation and leaves the VIP unable to converge.
# The hash is passed TWICE on purpose. Reusing one placeholder for both the assignment
# (`last_deploy_hash=$n`, a VARCHAR column) and the comparison inside the CASE made
# PostgreSQL deduce two different types for it and asyncpg refused the whole statement
# with AmbiguousParameterError ("text versus character varying"). Because the failure is
# in the UPDATE itself, not in one column, EVERY status ack was lost and every VIP sat at
# SYNCING forever — including teardown acks. A separate placeholder is only ever compared
# against the column, so its type is unambiguous.
_retire_takeover = ("takeover_expected_hash = CASE WHEN applied_config_hash IS NOT NULL "
"AND {p} = applied_config_hash THEN NULL ELSE takeover_expected_hash END")
"AND applied_config_hash = {p} THEN NULL ELSE takeover_expected_hash END")
if vip_id is None:
# No specific VIP (e.g. a teardown ack) — update all this agent's memberships.
await conn.execute(f"""
UPDATE vip_members SET last_deploy_state=$2, last_deploy_message=$3,
last_deploy_hash=$4, last_deploy_at=CURRENT_TIMESTAMP, updated_at=CURRENT_TIMESTAMP,
{_retire_takeover.format(p="$4")}
{_retire_takeover.format(p="$5")}
WHERE agent_id=$1
""", agent['id'], state, message, config_hash)
""", agent['id'], state, message, config_hash, config_hash)
else:
await conn.execute(f"""
UPDATE vip_members SET last_deploy_state=$3, last_deploy_message=$4,
last_deploy_hash=$5, last_deploy_at=CURRENT_TIMESTAMP, updated_at=CURRENT_TIMESTAMP,
{_retire_takeover.format(p="$5")}
{_retire_takeover.format(p="$6")}
WHERE agent_id=$1 AND vip_id=$2
""", agent['id'], int(vip_id), state, message, config_hash)
""", agent['id'], int(vip_id), state, message, config_hash, config_hash)
return {"status": "ok"}
except HTTPException:
raise
+113
View File
@@ -261,6 +261,119 @@ def test_takeover_authorisation_is_retired_once_the_node_acks_our_config():
)
def test_validation_judges_the_output_not_the_exit_code():
"""keepalived's config-test exit code cannot separate fatal from benign.
Measured on keepalived 2.2.8 rather than assumed:
clean config ..................... 0
auth_pass longer than 8 chars .... 5 "Truncating auth_pass to 8 characters"
missing '}' ...................... 5 "There are 1 missing '}'s"
unknown keyword .................. 5 "Unknown keyword '...'"
script without script_security ... 6 "SECURITY VIOLATION ..."
So 5 covers both a harmless truncation and a broken file. Treating any non-zero exit as
invalid rejected VALID configs: a VRRP password over 8 characters is enough, and keepalived
truncates it to 8 anyway, exactly as it does for the file the operator already runs. Found
on the first live adoption, where the node's OWN running config also exited non-zero.
The gate must therefore judge the output, and it must fail CLOSED: anything not on the
benign list still counts as fatal.
"""
script = (BACKEND / "utils" / "agent_scripts" / "linux_install.sh").read_text()
assert script.count("grep -v 'Truncating auth_pass to 8 characters'") == 2, (
"both daemon copies must drop the known-benign truncation warning before judging"
)
assert script.count('if [[ -n "$kp_fatal" ]]; then') == 2, (
"the decision must be made on what REMAINS after the benign lines are dropped"
)
# Fail-closed: the benign list is an allowlist, never a denylist of fatal messages. The
# measured table above is quoted in a comment inside the script, so assert on what is used
# as a grep PATTERN rather than on the text appearing anywhere.
patterns = re.findall(r"grep -v '([^']*)'", script)
assert set(patterns) == {"Truncating auth_pass to 8 characters", "^[[:space:]]*$"}, (
f"the validation filter greps for {sorted(set(patterns))}. It must drop only messages "
f"known to be benign; matching on fatal messages instead would let an unrecognised "
f"error through."
)
def test_validation_fails_closed_when_keepalived_says_nothing():
"""A non-zero exit with no output must stay fatal.
Filtering the output introduces a way to reach the accept path with an EMPTY filter result,
and some keepalived builds log to syslog rather than stderr — on such a host every config
would then be accepted regardless of what is wrong with it. Verified against a stub that
exits non-zero silently, and against one that emits only whitespace.
"""
script = (BACKEND / "utils" / "agent_scripts" / "linux_install.sh").read_text()
assert script.count('if [[ -z "${kp_out//[[:space:]]/}" ]]; then') == 2, (
"both daemon copies must treat a non-zero exit with no readable output as fatal"
)
assert script.count('kp_fatal="keepalived -t exited non-zero without output"') == 2
def test_validation_failure_reports_what_keepalived_said():
"""The fail-safe protected the node correctly on a live adoption but logged only
"config validation failed", with keepalived's own output sent to /dev/null. The operator
had no way to act on it without reproducing the check by hand on the node."""
script = (BACKEND / "utils" / "agent_scripts" / "linux_install.sh").read_text()
assert 'keepalived -t -f "$tmp_conf" >/dev/null 2>&1' not in script, (
"keepalived's output must not be discarded — a fail-safe that cannot say why it fired "
"is only half a safety feature"
)
assert script.count('kp_out=$(keepalived -t -f "$tmp_conf" 2>&1)') == 2, (
"both daemon copies must capture the validation output"
)
# The text is interpolated into JSON by both `log` and _kp_report, so it must be sanitised.
assert script.count(r"""tr -d '"\\'""") == 2, (
"captured output must have quotes and backslashes stripped before it reaches the JSON "
"log line and the status report"
)
assert script.count('keepalived -t failed: ${kp_err}') == 2, (
"the reason must also travel to the server so the UI can show it"
)
def test_status_ack_statements_bind_each_placeholder_once():
"""Every `$n` in the keepalived-status UPDATEs must be used exactly once, and the count must
match the arguments passed.
Reusing one placeholder for both the assignment (`last_deploy_hash=$n`, a VARCHAR column)
and the comparison inside the takeover-retirement CASE made PostgreSQL deduce two types for
it, and asyncpg rejected the whole statement with AmbiguousParameterError. The failure was
not partial: no ack was written at all, so every VIP sat at SYNCING forever and teardown acks
were lost too. Shipped in v1.10.12 and caught in the field.
The suite has no database, so this pins the shape that made it possible rather than the SQL
behaviour: one placeholder, one binding site.
"""
src = (BACKEND / "routers" / "agent.py").read_text()
start = src.index("async def agent_keepalived_status")
seg = src[start:src.index('return {"status": "ok"}', start)]
retire = "".join(re.findall(r'"([^"]*)"',
re.search(r"_retire_takeover = \((.*?)\)\n", seg, re.S).group(1)))
calls = re.findall(r'await conn\.execute\(f"""(.*?)""",\s*(.*?)\)\n', seg, re.S)
assert len(calls) == 2, f"expected the two ack UPDATEs, found {len(calls)}"
for sql, args in calls:
placeholder = re.search(r'_retire_takeover\.format\(p="(\$\d+)"\)', sql).group(1)
rendered = re.sub(r"\{_retire_takeover\.format\(p=\"\$\d+\"\)\}",
retire.replace("{p}", placeholder), sql)
used = re.findall(r"\$(\d+)", rendered)
dupes = {n for n in used if used.count(n) > 1}
assert not dupes, (
f"placeholder(s) {sorted('$'+d for d in dupes)} are bound more than once. PostgreSQL "
f"deduces a type per USE, so a placeholder that is both assigned to a column and "
f"compared against one is ambiguous and the whole UPDATE is rejected."
)
n_args = len([a for a in args.split(",") if a.strip()])
assert max(int(n) for n in used) == n_args, (
f"the statement uses ${max(int(n) for n in used)} but {n_args} arguments are passed"
)
def test_takeover_still_requires_the_pinned_hash_to_match_on_disk():
"""The guard that stops an edit between adoption and Apply from being overwritten."""
script = (BACKEND / "utils" / "agent_scripts" / "linux_install.sh").read_text()
+57 -10
View File
@@ -1880,11 +1880,43 @@ fetch_and_deploy_keepalived_config() {
local tmp_conf="${conf}.hom.tmp"
printf '%s' "$new_conf" > "$tmp_conf"
chmod 0644 "$tmp_conf"
if ! keepalived -t -f "$tmp_conf" >/dev/null 2>&1; then
rm -f "$tmp_conf"
log "ERROR" "KEEPALIVED: config validation failed (keepalived -t) — keeping current config, not (re)starting"
_kp_report "error" "$vip_id" "$new_hash" "keepalived -t failed"
return 0
# Capture what keepalived actually said. Discarding it made the fail-safe useless in
# practice: the node was correctly protected, but neither the log nor the UI could say WHY,
# so the only way forward was to reproduce the check by hand on the node. Sanitised hard
# (quotes, backslashes and newlines removed, tail kept) because both `log` and _kp_report
# embed the text in JSON built by string interpolation.
local kp_out kp_err kp_fatal
if ! kp_out=$(keepalived -t -f "$tmp_conf" 2>&1); then
# keepalived's config-test EXIT CODE does not separate a fatal config error from a
# harmless warning. Measured on 2.2.8, not assumed:
# clean config .................. 0
# auth_pass longer than 8 chars . 5 "Truncating auth_pass to 8 characters"
# missing '}' ................... 5 "There are 1 missing '}'s"
# unknown keyword ............... 5 "Unknown keyword '...'"
# script without script_security 6 "SECURITY VIOLATION ..."
# So 5 covers both a benign truncation and a broken file, and treating any non-zero
# exit as invalid rejected VALID configs: a VRRP password over 8 characters is enough,
# and keepalived truncates it to 8 regardless, exactly as it does for the file the
# operator already runs. Judge on the OUTPUT instead, dropping only messages known to
# be benign; anything unrecognised is still fatal, so this fails CLOSED.
if [[ -z "${kp_out//[[:space:]]/}" ]]; then
# Non-zero with NOTHING to read. We cannot confirm the reason is benign, and some
# builds log to syslog rather than stderr, so proceeding here would silently accept
# every config on such a host. Treat as fatal — the gate must fail closed.
kp_fatal="keepalived -t exited non-zero without output"
else
kp_fatal=$(printf '%s\n' "$kp_out" \
| grep -v 'Truncating auth_pass to 8 characters' \
| grep -v '^[[:space:]]*$' || true)
fi
if [[ -n "$kp_fatal" ]]; then
rm -f "$tmp_conf"
kp_err=$(printf '%s' "$kp_fatal" | tr '\n\r\t' ' ' | tr -d '"\\' | tail -c 300)
log "ERROR" "KEEPALIVED: config validation failed (keepalived -t): ${kp_err} — keeping current config, not (re)starting"
_kp_report "error" "$vip_id" "$new_hash" "keepalived -t failed: ${kp_err}"
return 0
fi
log "WARN" "KEEPALIVED: keepalived -t exited non-zero with only known-benign warnings; proceeding"
fi
mv -f "$tmp_conf" "$conf"
chown root:root "$conf" 2>/dev/null
@@ -3478,11 +3510,26 @@ CONFIG_RESPONSE_EOF
local tmp_conf="${conf}.hom.tmp"
printf '%s' "$new_conf" > "$tmp_conf"
chmod 0644 "$tmp_conf"
if ! keepalived -t -f "$tmp_conf" >/dev/null 2>&1; then
rm -f "$tmp_conf"
log "ERROR" "KEEPALIVED: config validation failed (keepalived -t) — keeping current config, not (re)starting"
_kp_report "error" "$vip_id" "$new_hash" "keepalived -t failed"
return 0
# See the heredoc copy for why the output is captured rather than discarded.
# See the heredoc copy for the measured exit-code table and why the OUTPUT, not the
# exit code, decides. Fails closed on anything not known to be benign.
local kp_out kp_err kp_fatal
if ! kp_out=$(keepalived -t -f "$tmp_conf" 2>&1); then
if [[ -z "${kp_out//[[:space:]]/}" ]]; then
kp_fatal="keepalived -t exited non-zero without output"
else
kp_fatal=$(printf '%s\n' "$kp_out" \
| grep -v 'Truncating auth_pass to 8 characters' \
| grep -v '^[[:space:]]*$' || true)
fi
if [[ -n "$kp_fatal" ]]; then
rm -f "$tmp_conf"
kp_err=$(printf '%s' "$kp_fatal" | tr '\n\r\t' ' ' | tr -d '"\\' | tail -c 300)
log "ERROR" "KEEPALIVED: config validation failed (keepalived -t): ${kp_err} — keeping current config, not (re)starting"
_kp_report "error" "$vip_id" "$new_hash" "keepalived -t failed: ${kp_err}"
return 0
fi
log "WARN" "KEEPALIVED: keepalived -t exited non-zero with only known-benign warnings; proceeding"
fi
mv -f "$tmp_conf" "$conf"
chown root:root "$conf" 2>/dev/null
+2 -2
View File
@@ -1,5 +1,5 @@
{
"version": "1.10.11",
"releaseName": "The Adoptable tag names the problem that actually blocks adoption",
"version": "1.10.13",
"releaseName": "Agent deploy acknowledgements were silently dropped",
"releaseDate": "2026-08-14"
}
+1 -1
View File
@@ -1,6 +1,6 @@
{
"name": "haproxy-openmanager-frontend",
"version": "1.10.11",
"version": "1.10.13",
"description": "HAProxy Load Balancer Management UI",
"license": "AGPL-3.0-or-later",
"dependencies": {