diff --git a/Dockerfile b/Dockerfile index fd7eb17..8b293bf 100644 --- a/Dockerfile +++ b/Dockerfile @@ -10,6 +10,8 @@ RUN DEBIAN_FRONTEND=noninteractive apt-get update \ # sshpass — Ansible shells out to it for first-contact password SSH # (provisioning a fresh host before the managed key is installed) sshpass \ + # git — clones and pulls Ansible git sources (Settings > Ansible) + git \ && rm -rf /var/lib/apt/lists/* # Upgrade pip to suppress the version notice @@ -37,7 +39,7 @@ RUN chmod +x /entrypoint.sh # so the entrypoint can fix up /data permissions if needed, then drops to # 'app' (uid 1000) via gosu before launching steward. RUN useradd -m -u 1000 app \ - && mkdir -p /data/playbook_cache /data/plugins \ + && mkdir -p /data/ansible/sources /data/ansible/runs /data/ansible/playbooks /data/plugins \ && chown -R app:app /data /app EXPOSE 5000 diff --git a/docs/core/ansible.md b/docs/core/ansible.md index b8366d5..f1a4fd8 100644 --- a/docs/core/ansible.md +++ b/docs/core/ansible.md @@ -31,10 +31,9 @@ ansible.sources: url: "https://github.com/example/infra.git" branch: "main" pull_interval_seconds: 300 - cache_path: "/data/playbook_cache/infra-repo" ``` -Git sources register a `ScheduledTask` named `ansible_git_pull_` that runs `git pull` on the configured interval. +Each git source is checked out under `/data/ansible/sources/` (override with `STEWARD_ANSIBLE_SOURCE_DIR`), on the persistent volume, so a redeploy keeps it. One scheduled task, `ansible_git_pulls`, ticks every 30 s and pulls each source once its `pull_interval_seconds` has passed; a source added in Settings is pulled within a tick, with no restart. A clone or pull that runs past 120 s is cut (its process group terminated), a clone that fails is removed so the next attempt starts clean, and git never prompts for credentials. --- diff --git a/steward/ansible/sources.py b/steward/ansible/sources.py index b1919ca..cd6e910 100644 --- a/steward/ansible/sources.py +++ b/steward/ansible/sources.py @@ -3,8 +3,10 @@ import asyncio import logging import os import re +import shutil import stat import tempfile +import time from pathlib import Path logger = logging.getLogger(__name__) @@ -76,6 +78,9 @@ BUILTIN_SOURCE_NAME = "steward-builtin" # persistent /data volume; created lazily on first save. LOCAL_SOURCE_NAME = "steward-local" USER_PLAYBOOK_DIR = os.environ.get("STEWARD_PLAYBOOK_DIR", "/data/ansible/playbooks") +# Git sources are checked out here, on the persistent /data volume, so a +# redeploy keeps them instead of re-cloning into the container's layer (#5157). +SOURCE_CACHE_DIR = os.environ.get("STEWARD_ANSIBLE_SOURCE_DIR", "/data/ansible/sources") def _builtin_source() -> dict: @@ -124,7 +129,7 @@ def get_sources(ansible_cfg: dict) -> list[dict]: Config structure in config.yaml:: ansible: - cache_dir: /var/cache/steward/ansible + cache_dir: /data/ansible/sources sources: - name: my-playbooks type: local @@ -136,7 +141,7 @@ def get_sources(ansible_cfg: dict) -> list[dict]: pull_interval_seconds: 3600 """ sources = ansible_cfg.get("sources", []) - cache_dir = ansible_cfg.get("cache_dir", "/var/cache/steward/ansible") + cache_dir = ansible_cfg.get("cache_dir") or SOURCE_CACHE_DIR # Always expose the bundled (read-only) + local (writable) first-party sources. result = [_builtin_source(), _local_source()] for src in sources: @@ -343,8 +348,43 @@ ASKPASS_SCRIPT = ( ) -async def git_pull(source: dict) -> None: - """Clone the git repo if absent; pull if already present. +# A clone or pull past this is cut, so a hung remote can't hold a source's +# pull (or the Sync button) open forever (#5157). +GIT_TIMEOUT_SECONDS = 120.0 + + +class GitPullError(RuntimeError): + """A clone or pull that failed or ran past its timeout; says which and why.""" + + +async def _git(argv: list[str], env: dict, timeout: float, what: str) -> None: + from steward.ansible.executor import _terminate_then_kill + + proc = await asyncio.create_subprocess_exec( + *argv, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + env=env, + start_new_session=True, # its own group, so a timeout ends ssh/helpers too + ) + try: + _, stderr = await asyncio.wait_for(proc.communicate(), timeout) + except asyncio.TimeoutError: + raise GitPullError(f"{what} did not finish within {timeout:g} s") from None + finally: + if proc.returncode is None: + await asyncio.shield(_terminate_then_kill(proc, grace=2.0)) + if proc.returncode != 0: + raise GitPullError(f"{what} failed: {stderr.decode(errors='replace').strip()[-500:]}") + + +async def git_pull(source: dict, timeout: float = GIT_TIMEOUT_SECONDS) -> None: + """Clone the git repo if absent; fast-forward it if already present. + + Raises GitPullError when git fails or runs past `timeout`; a clone that + did not finish is removed, so the next attempt clones afresh rather than + pulling into a broken checkout. git never prompts (GIT_TERMINAL_PROMPT=0): + a missing credential fails at once instead of waiting on input. When source['http_token'] is set, git asks a temporary GIT_ASKPASS script, which prints the token from the child's environment, so it never touches @@ -353,7 +393,7 @@ async def git_pull(source: dict) -> None: path = Path(source["path"]) token = source.get("http_token", "") - env = None + env = {**os.environ, "GIT_TERMINAL_PROMPT": "0"} askpass_path = None if token: fd, askpass_path = tempfile.mkstemp(prefix="steward-askpass-", suffix=".sh") @@ -362,41 +402,50 @@ async def git_pull(source: dict) -> None: finally: os.close(fd) os.chmod(askpass_path, stat.S_IRWXU) - env = {**os.environ, "GIT_ASKPASS": askpass_path, ASKPASS_TOKEN_ENV: token} + env.update({"GIT_ASKPASS": askpass_path, ASKPASS_TOKEN_ENV: token}) try: if not (path / ".git").exists(): path.mkdir(parents=True, exist_ok=True) - proc = await asyncio.create_subprocess_exec( - "git", "clone", - "--branch", source["branch"], - "--single-branch", - source["url"], str(path), - stdout=asyncio.subprocess.PIPE, - stderr=asyncio.subprocess.PIPE, - env=env, - ) - _, stderr = await proc.communicate() - if proc.returncode != 0: - logger.error( - "git clone failed for %r: %s", - source["name"], - stderr.decode(errors="replace"), - ) + try: + await _git(["git", "clone", "--branch", source["branch"], "--single-branch", + source["url"], str(path)], + env, timeout, f"git clone of {source['name']!r}") + except BaseException: + shutil.rmtree(path, ignore_errors=True) + raise else: - proc = await asyncio.create_subprocess_exec( - "git", "-C", str(path), "pull", - stdout=asyncio.subprocess.PIPE, - stderr=asyncio.subprocess.PIPE, - env=env, - ) - _, stderr = await proc.communicate() - if proc.returncode != 0: - logger.error( - "git pull failed for %r: %s", - source["name"], - stderr.decode(errors="replace"), - ) + await _git(["git", "-C", str(path), "pull", "--ff-only"], + env, timeout, f"git pull of {source['name']!r}") finally: if askpass_path and os.path.exists(askpass_path): os.unlink(askpass_path) + + +# When each git source was last pulled (monotonic seconds), by name. +_last_pull: dict[str, float] = {} + + +async def pull_due_sources(ansible_cfg: dict, now: float | None = None) -> list[str]: + """Pull every git source whose pull_interval_seconds has passed. + + Run by one scheduled task on a short tick, reading the sources as they are + now, so a source added in Settings is pulled within a tick and an edited + interval applies without a restart (#5157). A failed pull is logged and + waits out its interval like a good one. Returns the names pulled. + """ + now = time.monotonic() if now is None else now + sources = get_sources(ansible_cfg) + due = [src for src in sources + if src["type"] == "git" + and now - _last_pull.get(src["name"], float("-inf")) >= src["pull_interval_seconds"]] + names = {src["name"] for src in sources} + for gone in [n for n in _last_pull if n not in names]: + del _last_pull[gone] + for src in due: + _last_pull[src["name"]] = now + try: + await git_pull(src) + except Exception: + logger.exception("Ansible source %r: pull failed", src["name"]) + return [src["name"] for src in due] diff --git a/steward/app.py b/steward/app.py index 1a26c6a..15d9d5a 100644 --- a/steward/app.py +++ b/steward/app.py @@ -434,26 +434,21 @@ def _register_core_tasks(app: Quart) -> None: ), ]) - # Register git-pull tasks for each ansible git source - ansible_cfg = app.config.get("ANSIBLE", {}) - from .ansible.sources import get_sources - for source in get_sources(ansible_cfg): - if source["type"] != "git": - continue - _source = source + # Pull Ansible git sources as each falls due. One task reads the sources + # as they are on every tick, so one added in Settings is pulled within + # half a minute, no restart (#5157). + async def run_git_pulls(): + from .ansible.sources import pull_due_sources + await pull_due_sources(app.config.get("ANSIBLE", {})) - async def run_git_pull(src=_source): - from .ansible.sources import git_pull - await git_pull(src) - - app._task_registry.append( - ScheduledTask( - name=f"ansible_git_pull_{_source['name']}", - coro_factory=run_git_pull, - interval_seconds=_source["pull_interval_seconds"], - run_on_startup=True, - ) + app._task_registry.append( + ScheduledTask( + name="ansible_git_pulls", + coro_factory=run_git_pulls, + interval_seconds=30, + run_on_startup=True, ) + ) # Fire due Ansible schedules (recurring playbook runs). Checks every minute; # each schedule's own interval gates whether it actually runs. diff --git a/steward/core/alerts.py b/steward/core/alerts.py index 852be9f..68d422b 100644 --- a/steward/core/alerts.py +++ b/steward/core/alerts.py @@ -34,21 +34,35 @@ def init_alerts(app: "Quart") -> None: _app = app -def alert_extra_vars(rule: AlertRule, value: float) -> list[str]: - """Ansible extra-vars describing the alert that fired, as `key=value` strings. +def alert_extra_vars(rule: AlertRule, value: float) -> dict: + """Ansible extra-vars describing the alert that fired. Injected into every alert-triggered playbook run so the playbook can target - the exact resource/metric that fired without templating. Pure function. + the exact resource/metric that fired without templating. They reach + ansible-playbook in its JSON extra-vars file, not as `-e key=value`, which + Ansible splits on whitespace: a rule named "High CPU on web" stays one + value (#5157). Pure function. """ - return [ - f"steward_alert_rule={rule.name}", - f"steward_alert_source={rule.source_module}", - f"steward_alert_resource={rule.resource_name}", - f"steward_alert_metric={rule.metric_name}", - f"steward_alert_value={value}", - f"steward_alert_threshold={rule.threshold}", - f"steward_alert_operator={rule.operator.value}", - ] + return { + "steward_alert_rule": rule.name, + "steward_alert_source": rule.source_module, + "steward_alert_resource": rule.resource_name, + "steward_alert_metric": rule.metric_name, + "steward_alert_value": value, + "steward_alert_threshold": rule.threshold, + "steward_alert_operator": rule.operator.value, + } + + +def action_vars(lines) -> dict: + """An alert action's stored `key=value` lines as a map, split at the first + `=` so a value keeps its spaces, quotes and further `=`.""" + out: dict = {} + for line in lines or (): + key, sep, val = str(line).partition("=") + if sep and key.strip(): + out[key.strip()] = val + return out async def record_metric( @@ -489,8 +503,10 @@ async def _run_ansible_action( playbook = action.get("playbook") or "" inventory = action.get("inventory") or "" source_name = action.get("source") or "" + # One JSON extra-vars file; the firing's own vars win over the action's. params: dict = { - "extra_vars": list(action.get("extra_vars") or []) + alert_extra_vars(rule, value), + "extra_vars_map": {**action_vars(action.get("extra_vars")), + **alert_extra_vars(rule, value)}, } if action.get("limit"): params["limit"] = action["limit"] diff --git a/steward/settings/routes.py b/steward/settings/routes.py index 893ef1c..13a7533 100644 --- a/steward/settings/routes.py +++ b/steward/settings/routes.py @@ -1,5 +1,6 @@ # steward/settings/routes.py from __future__ import annotations +import html import logging from pathlib import Path from quart import Blueprint, current_app, render_template, request, redirect, url_for, session @@ -505,7 +506,8 @@ async def ansible_sync_source(idx: int): return 'Synced' except Exception as exc: logger.exception("git pull failed for source %r", source["name"]) - return f'Sync failed: {exc}' + return ('Sync failed: ' + f'{html.escape(str(exc))}') # ── Auth (OIDC / LDAP) ──────────────────────────────────────────────────────── diff --git a/tests/core/test_git_sources.py b/tests/core/test_git_sources.py new file mode 100644 index 0000000..af65105 --- /dev/null +++ b/tests/core/test_git_sources.py @@ -0,0 +1,102 @@ +"""Git sources: the checkout location, a pull cut at its timeout, a failure that +says why, and one pull task that picks up sources as they change (#5157).""" +import os +import time +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from steward.ansible import sources + + +def _fake_git(tmp_path, monkeypatch, body: str) -> None: + """Put a `git` on PATH that runs `body` instead of the real one.""" + bindir = tmp_path / "bin" + bindir.mkdir() + git = bindir / "git" + git.write_text("#!/bin/sh\n" + body + "\n") + git.chmod(0o755) + monkeypatch.setenv("PATH", f"{bindir}{os.pathsep}{os.environ.get('PATH', '')}") + + +def _source(path, **kw): + return {"name": "infra", "path": str(path), "url": "https://git.example/infra.git", + "branch": "main", "http_token": "", **kw} + + +def test_a_git_source_is_checked_out_under_data(): + src = next(s for s in sources.get_sources( + {"sources": [{"name": "infra", "type": "git", "url": "https://x/infra.git"}]}) + if s["name"] == "infra") + assert src["path"] == os.path.join(sources.SOURCE_CACHE_DIR, "infra") + # On the persistent volume, not the container's layer. + assert sources.SOURCE_CACHE_DIR == os.environ.get( + "STEWARD_ANSIBLE_SOURCE_DIR", "/data/ansible/sources") + + +@pytest.mark.asyncio +async def test_a_hung_clone_is_cut_at_its_timeout_and_removed(tmp_path, monkeypatch): + _fake_git(tmp_path, monkeypatch, "sleep 30") + checkout = tmp_path / "cache" / "infra" + started = time.monotonic() + with pytest.raises(sources.GitPullError, match="did not finish within 1 s"): + await sources.git_pull(_source(checkout), timeout=1) + assert time.monotonic() - started < 10 + assert not checkout.exists() # the next attempt clones afresh + + +@pytest.mark.asyncio +async def test_a_failed_pull_says_why_and_keeps_the_checkout(tmp_path, monkeypatch): + _fake_git(tmp_path, monkeypatch, "echo 'fatal: Not possible to fast-forward' >&2; exit 128") + checkout = tmp_path / "infra" + (checkout / ".git").mkdir(parents=True) + with pytest.raises(sources.GitPullError, match="Not possible to fast-forward"): + await sources.git_pull(_source(checkout)) + assert (checkout / ".git").exists() + + +@pytest.mark.asyncio +async def test_git_never_prompts_and_only_fast_forwards(tmp_path): + checkout = tmp_path / "infra" + (checkout / ".git").mkdir(parents=True) + seen = {} + + async def fake_exec(*args, **kwargs): + seen["argv"], seen["env"] = args, kwargs.get("env") or {} + proc = MagicMock() + proc.returncode = 0 + proc.communicate = AsyncMock(return_value=(b"", b"")) + return proc + + with patch("asyncio.create_subprocess_exec", side_effect=fake_exec): + await sources.git_pull(_source(checkout)) + assert seen["env"]["GIT_TERMINAL_PROMPT"] == "0" + assert seen["argv"][-2:] == ("pull", "--ff-only") + + +@pytest.mark.asyncio +async def test_one_task_pulls_sources_as_they_fall_due(monkeypatch): + pulled: list[str] = [] + + async def fake_pull(src): + pulled.append(src["name"]) + if src["name"] == "broken": + raise sources.GitPullError("git pull of 'broken' failed: nope") + + monkeypatch.setattr(sources, "git_pull", fake_pull) + monkeypatch.setattr(sources, "_last_pull", {}) + + def cfg(*names): + intervals = {"fast": 60, "slow": 600, "broken": 60, "new": 60} + return {"cache_dir": "/tmp/x", "sources": [ + {"name": n, "type": "git", "url": f"https://x/{n}.git", + "pull_interval_seconds": intervals[n]} for n in names]} + + assert await sources.pull_due_sources(cfg("fast", "slow", "broken"), now=1000) == [ + "fast", "slow", "broken"] # all due at start; a failure is logged + assert await sources.pull_due_sources(cfg("fast", "slow", "broken"), now=1030) == [] + assert await sources.pull_due_sources(cfg("fast", "slow", "broken"), now=1061) == [ + "fast", "broken"] # a failed one waits its interval too + # A source added in Settings is pulled on the next tick, no restart. + assert await sources.pull_due_sources(cfg("fast", "slow", "new"), now=1062) == ["new"] + assert "broken" not in sources._last_pull # a removed source is forgotten diff --git a/tests/test_alert_ansible.py b/tests/test_alert_ansible.py index d7beb9d..342f757 100644 --- a/tests/test_alert_ansible.py +++ b/tests/test_alert_ansible.py @@ -1,5 +1,5 @@ -"""Unit tests for alert -> Ansible action context vars (task 250).""" -from steward.core.alerts import alert_extra_vars +"""Unit tests for alert -> Ansible action context vars (task 250, #5157).""" +from steward.core.alerts import action_vars, alert_extra_vars from steward.models.alerts import AlertOperator, AlertRule @@ -9,22 +9,33 @@ def test_alert_extra_vars_describes_the_firing(): metric_name="battery_status", operator=AlertOperator.lt, threshold=3.0, created_by="u", ) - ev = alert_extra_vars(rule, 2.0) - assert "steward_alert_rule=UPS low battery" in ev - assert "steward_alert_source=snmp" in ev - assert "steward_alert_resource=ups" in ev - assert "steward_alert_metric=battery_status" in ev - assert "steward_alert_value=2.0" in ev - assert "steward_alert_threshold=3.0" in ev - assert "steward_alert_operator=<" in ev + assert alert_extra_vars(rule, 2.0) == { + "steward_alert_rule": "UPS low battery", # one value, spaces and all + "steward_alert_source": "snmp", + "steward_alert_resource": "ups", + "steward_alert_metric": "battery_status", + "steward_alert_value": 2.0, + "steward_alert_threshold": 3.0, + "steward_alert_operator": "<", + } -def test_alert_extra_vars_are_key_value_strings(): - rule = AlertRule( - name="r", source_module="host_agent", resource_name="srv1", - metric_name="cpu_pct", operator=AlertOperator.gt, threshold=90.0, - created_by="u", - ) - ev = alert_extra_vars(rule, 95.5) - assert all("=" in item for item in ev) - assert all(item.startswith("steward_alert_") for item in ev) +def test_action_vars_split_at_the_first_equals(): + assert action_vars(["greeting=hello world", "q=a=b", " k = v", "bare", "=x"]) == { + "greeting": "hello world", "q": "a=b", "k": " v"} + assert action_vars(None) == {} + + +def test_an_alert_run_passes_its_vars_as_a_json_file_not_on_the_command_line(): + import json + from steward.ansible.executor import build_ansible_command, build_extra_vars_file + rule = AlertRule(name="High CPU on web", source_module="host_agent", resource_name="web", + metric_name="cpu_pct", operator=AlertOperator.gt, threshold=90.0, + created_by="u") + merged = {**action_vars(["note=from the action"]), **alert_extra_vars(rule, 95.5)} + cmd = build_ansible_command("p.yml", "inv", {"extra_vars_map": merged}) + assert "-e" not in cmd # nothing interpolated into argv + argv, files = build_extra_vars_file(merged, "/tmp/x") + assert argv == ["-e", "@/tmp/x/extravars.json"] + assert json.loads(files[0][1])["steward_alert_rule"] == "High CPU on web" + assert json.loads(files[0][1])["note"] == "from the action"