fix(ha-diag): node_name from env + fail-fast on unknown — evt-unknown-* files poisoned observer checkpoint (see d5139c9)
An "unknown"/empty node_name silently produced evt-unknown-* event files. One landed in events/piha/ and, being lexically greater than every evt-piha-* name, pinned the observer's (then lexical) checkpoint and blocked PIHA for ~34 days. Defense in depth, two layers, both refuse rather than emit poison: - config.py: node_name field_validator rejects ""/"unknown" (case-insensitive, trimmed); validate_default=True so the "unknown" default itself is rejected when NODE_NAME never reaches the process. main() catches ValidationError and exits 1 with a clear FATAL message instead of a raw traceback. - event_emitter.py: EventEmitter.__init__ guards node_name at the exact poison site (node_name is embedded in the filename), so no future call path can reintroduce evt-unknown-*. Precedence unchanged and correct: Settings.load() uses os.environ.setdefault, so env NODE_NAME wins over YAML; YAML only supplies node_name when env is absent. api.py never writes event files (only /health, /trigger), so its _node_name global cannot produce poison — verified, left as-is. Tests (services/ha-diag-agent/tests/): new test_config.py (env load, env>YAML precedence, fail-fast on unknown/empty/whitespace/default/YAML-unknown) and EventEmitter guard tests. Full unit suite: 139 passed, 0 regressions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
f57a01a20d
commit
f2ba81bcc1
|
|
@ -4,11 +4,18 @@ import os
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
|
||||||
import yaml
|
import yaml
|
||||||
from pydantic import field_validator
|
from pydantic import Field, field_validator
|
||||||
from pydantic_settings import BaseSettings
|
from pydantic_settings import BaseSettings
|
||||||
|
|
||||||
_CONFIG_YAML = Path("/config/ha-diag-agent.yaml")
|
_CONFIG_YAML = Path("/config/ha-diag-agent.yaml")
|
||||||
|
|
||||||
|
# node_name values that are unsafe to run with. A node_name of "unknown" (or
|
||||||
|
# empty) silently produces evt-unknown-* event files. Such a file once landed in
|
||||||
|
# events/piha/ and, being lexically greater than every evt-piha-* name, pinned
|
||||||
|
# the observer's (then lexical) checkpoint and blocked PIHA for ~34 days — see
|
||||||
|
# fix d5139c9. We refuse to start rather than ever emit that poison again.
|
||||||
|
_FORBIDDEN_NODE_NAMES = {"", "unknown"}
|
||||||
|
|
||||||
|
|
||||||
class Settings(BaseSettings):
|
class Settings(BaseSettings):
|
||||||
# HA connection
|
# HA connection
|
||||||
|
|
@ -16,8 +23,11 @@ class Settings(BaseSettings):
|
||||||
ha_token: str = ""
|
ha_token: str = ""
|
||||||
ha_timeout: float = 10.0
|
ha_timeout: float = 10.0
|
||||||
|
|
||||||
# Node identity
|
# Node identity.
|
||||||
node_name: str = "unknown"
|
# validate_default=True so the "unknown" default itself is rejected: if
|
||||||
|
# NODE_NAME never reaches the process the agent must fail fast, not run as
|
||||||
|
# "unknown" and poison another node's event directory (see d5139c9).
|
||||||
|
node_name: str = Field(default="unknown", validate_default=True)
|
||||||
location_tag: str = "default"
|
location_tag: str = "default"
|
||||||
|
|
||||||
# Intervals (seconds)
|
# Intervals (seconds)
|
||||||
|
|
@ -69,6 +79,26 @@ class Settings(BaseSettings):
|
||||||
def strip_trailing_slash(cls, v: str) -> str:
|
def strip_trailing_slash(cls, v: str) -> str:
|
||||||
return v.rstrip("/")
|
return v.rstrip("/")
|
||||||
|
|
||||||
|
@field_validator("node_name")
|
||||||
|
@classmethod
|
||||||
|
def reject_unknown_node(cls, v: str) -> str:
|
||||||
|
"""Fail-fast: never let the agent run as "unknown"/empty.
|
||||||
|
|
||||||
|
A node_name of "unknown" makes EventEmitter write evt-unknown-* files.
|
||||||
|
Landing in another node's events dir, such a file poisoned the observer
|
||||||
|
checkpoint and blocked PIHA for ~34 days (d5139c9). Refusing to start is
|
||||||
|
strictly safer than silently emitting the poison.
|
||||||
|
"""
|
||||||
|
normalized = (v or "").strip()
|
||||||
|
if normalized.lower() in _FORBIDDEN_NODE_NAMES:
|
||||||
|
raise ValueError(
|
||||||
|
"NODE_NAME not set — refusing to start, would poison event "
|
||||||
|
"pipeline with evt-unknown-* files (see d5139c9). "
|
||||||
|
"Set NODE_NAME to the node's canonical inventory name "
|
||||||
|
"(e.g. piha, chelsty-infra)."
|
||||||
|
)
|
||||||
|
return normalized
|
||||||
|
|
||||||
@classmethod
|
@classmethod
|
||||||
def load(cls) -> "Settings":
|
def load(cls) -> "Settings":
|
||||||
"""Load settings: YAML file provides defaults; env vars override."""
|
"""Load settings: YAML file provides defaults; env vars override."""
|
||||||
|
|
|
||||||
|
|
@ -16,8 +16,19 @@ class EventEmitter:
|
||||||
def __init__(
|
def __init__(
|
||||||
self, events_dir: Path, node_name: str, location_tag: str = ""
|
self, events_dir: Path, node_name: str, location_tag: str = ""
|
||||||
) -> None:
|
) -> None:
|
||||||
|
# Defense in depth (config.py already fails fast on this): the event id
|
||||||
|
# embeds node_name into the filename, so "unknown"/empty here is exactly
|
||||||
|
# what produces evt-unknown-* poison files. Refuse at the emit boundary
|
||||||
|
# too, so no future call path can quietly reintroduce it (see d5139c9).
|
||||||
|
normalized = (node_name or "").strip()
|
||||||
|
if normalized.lower() in ("", "unknown"):
|
||||||
|
raise ValueError(
|
||||||
|
f"EventEmitter refuses node_name={node_name!r} — would write "
|
||||||
|
"evt-unknown-* files that poison the observer checkpoint "
|
||||||
|
"(see d5139c9). node_name must be a real node identity."
|
||||||
|
)
|
||||||
self._events_dir = events_dir
|
self._events_dir = events_dir
|
||||||
self._node_name = node_name
|
self._node_name = normalized
|
||||||
self._location_tag = location_tag
|
self._location_tag = location_tag
|
||||||
self._seq = 0
|
self._seq = 0
|
||||||
events_dir.mkdir(parents=True, exist_ok=True)
|
events_dir.mkdir(parents=True, exist_ok=True)
|
||||||
|
|
|
||||||
|
|
@ -3,12 +3,14 @@ from __future__ import annotations
|
||||||
import asyncio
|
import asyncio
|
||||||
import json
|
import json
|
||||||
import logging
|
import logging
|
||||||
|
import sys
|
||||||
import time
|
import time
|
||||||
from datetime import datetime
|
from datetime import datetime
|
||||||
|
|
||||||
import structlog
|
import structlog
|
||||||
import uvicorn
|
import uvicorn
|
||||||
from apscheduler.schedulers.asyncio import AsyncIOScheduler
|
from apscheduler.schedulers.asyncio import AsyncIOScheduler
|
||||||
|
from pydantic import ValidationError
|
||||||
|
|
||||||
from .api import app, register_checks, register_ws_monitor
|
from .api import app, register_checks, register_ws_monitor
|
||||||
from .checks.automation_failures import AutomationFailuresCheck
|
from .checks.automation_failures import AutomationFailuresCheck
|
||||||
|
|
@ -196,7 +198,18 @@ async def run(settings: Settings) -> None:
|
||||||
|
|
||||||
|
|
||||||
def main() -> None:
|
def main() -> None:
|
||||||
|
try:
|
||||||
settings = Settings.load()
|
settings = Settings.load()
|
||||||
|
except ValidationError as exc:
|
||||||
|
# Fail fast and loud rather than run mis-identified. The most important
|
||||||
|
# case is node_name="unknown"/empty, which would emit evt-unknown-*
|
||||||
|
# poison files into another node's events dir (see d5139c9).
|
||||||
|
print(
|
||||||
|
"FATAL: refusing to start ha-diag-agent — invalid configuration:\n"
|
||||||
|
f"{exc}",
|
||||||
|
file=sys.stderr,
|
||||||
|
)
|
||||||
|
raise SystemExit(1)
|
||||||
asyncio.run(run(settings))
|
asyncio.run(run(settings))
|
||||||
|
|
||||||
|
|
||||||
|
|
|
||||||
141
services/ha-diag-agent/tests/test_config.py
Normal file
141
services/ha-diag-agent/tests/test_config.py
Normal file
|
|
@ -0,0 +1,141 @@
|
||||||
|
"""Tests for Settings / node_name fail-fast.
|
||||||
|
|
||||||
|
Regression coverage for the evt-unknown-* poison bug: a node_name of "unknown"
|
||||||
|
(or empty) produced event files that pinned the observer checkpoint and blocked
|
||||||
|
PIHA for ~34 days (see fix d5139c9). The agent must now refuse to start rather
|
||||||
|
than ever emit such a file.
|
||||||
|
"""
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import os
|
||||||
|
from pathlib import Path
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
from pydantic import ValidationError
|
||||||
|
|
||||||
|
from ha_diag import config
|
||||||
|
from ha_diag.config import Settings
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture(autouse=True)
|
||||||
|
def _isolate_environ():
|
||||||
|
"""Snapshot os.environ — Settings.load() mutates it via setdefault()."""
|
||||||
|
saved = dict(os.environ)
|
||||||
|
yield
|
||||||
|
os.environ.clear()
|
||||||
|
os.environ.update(saved)
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# node_name loaded from env NODE_NAME
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
def test_node_name_from_env(monkeypatch):
|
||||||
|
monkeypatch.setenv("NODE_NAME", "piha")
|
||||||
|
settings = Settings()
|
||||||
|
assert settings.node_name == "piha"
|
||||||
|
|
||||||
|
|
||||||
|
def test_node_name_from_env_case_insensitive_var(monkeypatch):
|
||||||
|
# BaseSettings is case-insensitive: lowercase env var maps to the field too.
|
||||||
|
monkeypatch.delenv("NODE_NAME", raising=False)
|
||||||
|
monkeypatch.setenv("node_name", "chelsty-infra")
|
||||||
|
settings = Settings()
|
||||||
|
assert settings.node_name == "chelsty-infra"
|
||||||
|
|
||||||
|
|
||||||
|
def test_node_name_whitespace_is_stripped(monkeypatch):
|
||||||
|
monkeypatch.setenv("NODE_NAME", " piha ")
|
||||||
|
settings = Settings()
|
||||||
|
assert settings.node_name == "piha"
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# fail-fast: unknown / empty node_name refuses to start
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
def test_explicit_unknown_node_name_refuses(monkeypatch):
|
||||||
|
monkeypatch.delenv("NODE_NAME", raising=False)
|
||||||
|
with pytest.raises(ValidationError) as exc:
|
||||||
|
Settings(node_name="unknown")
|
||||||
|
assert "refusing to start" in str(exc.value)
|
||||||
|
|
||||||
|
|
||||||
|
def test_unknown_case_insensitive_refuses(monkeypatch):
|
||||||
|
monkeypatch.delenv("NODE_NAME", raising=False)
|
||||||
|
with pytest.raises(ValidationError):
|
||||||
|
Settings(node_name="UNKNOWN")
|
||||||
|
|
||||||
|
|
||||||
|
def test_empty_node_name_refuses(monkeypatch):
|
||||||
|
monkeypatch.delenv("NODE_NAME", raising=False)
|
||||||
|
with pytest.raises(ValidationError):
|
||||||
|
Settings(node_name="")
|
||||||
|
|
||||||
|
|
||||||
|
def test_whitespace_only_node_name_refuses(monkeypatch):
|
||||||
|
monkeypatch.delenv("NODE_NAME", raising=False)
|
||||||
|
with pytest.raises(ValidationError):
|
||||||
|
Settings(node_name=" ")
|
||||||
|
|
||||||
|
|
||||||
|
def test_env_unknown_refuses(monkeypatch):
|
||||||
|
monkeypatch.setenv("NODE_NAME", "unknown")
|
||||||
|
with pytest.raises(ValidationError):
|
||||||
|
Settings()
|
||||||
|
|
||||||
|
|
||||||
|
def test_default_without_env_refuses(monkeypatch):
|
||||||
|
# No NODE_NAME anywhere → the "unknown" default must be rejected, not used.
|
||||||
|
monkeypatch.delenv("NODE_NAME", raising=False)
|
||||||
|
monkeypatch.delenv("node_name", raising=False)
|
||||||
|
with pytest.raises(ValidationError):
|
||||||
|
Settings()
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# precedence: env wins over YAML for node_name
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
def test_env_wins_over_yaml(monkeypatch, tmp_path: Path):
|
||||||
|
yaml_file = tmp_path / "ha-diag-agent.yaml"
|
||||||
|
yaml_file.write_text("node_name: solaria\n")
|
||||||
|
monkeypatch.setattr(config, "_CONFIG_YAML", yaml_file)
|
||||||
|
monkeypatch.setenv("NODE_NAME", "piha")
|
||||||
|
|
||||||
|
settings = Settings.load()
|
||||||
|
# env value must win; YAML only supplies a default when env is absent.
|
||||||
|
assert settings.node_name == "piha"
|
||||||
|
|
||||||
|
|
||||||
|
def test_yaml_supplies_node_name_when_env_absent(monkeypatch, tmp_path: Path):
|
||||||
|
yaml_file = tmp_path / "ha-diag-agent.yaml"
|
||||||
|
yaml_file.write_text("node_name: chelsty-infra\n")
|
||||||
|
monkeypatch.setattr(config, "_CONFIG_YAML", yaml_file)
|
||||||
|
monkeypatch.delenv("NODE_NAME", raising=False)
|
||||||
|
monkeypatch.delenv("node_name", raising=False)
|
||||||
|
|
||||||
|
settings = Settings.load()
|
||||||
|
assert settings.node_name == "chelsty-infra"
|
||||||
|
|
||||||
|
|
||||||
|
def test_yaml_unknown_still_refuses(monkeypatch, tmp_path: Path):
|
||||||
|
# Even if YAML carries the poison value, fail-fast must still fire.
|
||||||
|
yaml_file = tmp_path / "ha-diag-agent.yaml"
|
||||||
|
yaml_file.write_text("node_name: unknown\n")
|
||||||
|
monkeypatch.setattr(config, "_CONFIG_YAML", yaml_file)
|
||||||
|
monkeypatch.delenv("NODE_NAME", raising=False)
|
||||||
|
monkeypatch.delenv("node_name", raising=False)
|
||||||
|
|
||||||
|
with pytest.raises(ValidationError):
|
||||||
|
Settings.load()
|
||||||
|
|
||||||
|
|
||||||
|
def test_load_valid_node_name(monkeypatch, tmp_path: Path):
|
||||||
|
monkeypatch.setattr(config, "_CONFIG_YAML", tmp_path / "does-not-exist.yaml")
|
||||||
|
monkeypatch.setenv("NODE_NAME", "piha")
|
||||||
|
settings = Settings.load()
|
||||||
|
assert settings.node_name == "piha"
|
||||||
|
|
@ -76,6 +76,21 @@ def test_location_tag_empty_not_in_payload(tmp_events_dir: Path):
|
||||||
assert "location_tag" not in data["payload"]
|
assert "location_tag" not in data["payload"]
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("bad_node", ["unknown", "UNKNOWN", "", " "])
|
||||||
|
def test_emitter_refuses_unknown_or_empty_node(tmp_events_dir: Path, bad_node: str):
|
||||||
|
# No emit path may ever produce evt-unknown-* poison files (see d5139c9).
|
||||||
|
with pytest.raises(ValueError):
|
||||||
|
EventEmitter(tmp_events_dir, node_name=bad_node)
|
||||||
|
|
||||||
|
|
||||||
|
def test_emitter_never_writes_unknown_node_in_filename(tmp_events_dir: Path):
|
||||||
|
emitter = EventEmitter(tmp_events_dir, node_name="piha")
|
||||||
|
event_id = emitter.emit("ha_websocket_dead", "error", "homeassistant", "msg")
|
||||||
|
assert "unknown" not in event_id
|
||||||
|
data = json.loads((tmp_events_dir / f"{event_id}.json").read_text())
|
||||||
|
assert data["node"] == "piha"
|
||||||
|
|
||||||
|
|
||||||
def test_location_tag_does_not_override_explicit_payload_key(tmp_events_dir: Path):
|
def test_location_tag_does_not_override_explicit_payload_key(tmp_events_dir: Path):
|
||||||
emitter = EventEmitter(tmp_events_dir, node_name="piha", location_tag="ken")
|
emitter = EventEmitter(tmp_events_dir, node_name="piha", location_tag="ken")
|
||||||
event_id = emitter.emit(
|
event_id = emitter.emit(
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue