diff --git a/steward/core/plugin_manager.py b/steward/core/plugin_manager.py index 5dedde3..b78d8e9 100644 --- a/steward/core/plugin_manager.py +++ b/steward/core/plugin_manager.py @@ -87,7 +87,8 @@ def _import_plugin(name: str, plugin_path: Path): """Load a plugin module by file path, avoiding sys.modules stdlib collisions. Using importlib.import_module(name) fails for plugins whose names shadow - Python stdlib modules (e.g. the 'http' plugin vs stdlib's 'http' package). + Python stdlib modules (e.g. a plugin named 'json' or 'http' vs the stdlib + package of the same name). This helper loads from the filesystem path directly and registers the module under a namespaced key so relative imports within the plugin still work. """ diff --git a/steward/core/settings.py b/steward/core/settings.py index d41c2c3..ad6a80e 100644 --- a/steward/core/settings.py +++ b/steward/core/settings.py @@ -18,6 +18,7 @@ from __future__ import annotations import asyncio import json import logging +from collections.abc import Iterable from datetime import datetime, timezone from typing import Any @@ -106,7 +107,6 @@ DEFAULTS: dict[str, Any] = { # Per-plugin yaml config defaults are merged on top at load time. "plugin.docker": {"enabled": True}, "plugin.host_agent": {"enabled": True}, - "plugin.http": {"enabled": True}, "plugin.snmp": {"enabled": True}, # OIDC single-sign-on "oidc.enabled": False, @@ -229,6 +229,60 @@ async def set_setting(session: AsyncSession, key: str, value: Any) -> None: _undecryptable_secrets.discard(key) +async def delete_setting(session: AsyncSession, key: str) -> bool: + """Remove a stored setting row. Returns True if a row was actually deleted. + + Call in a transaction. Deleting a key that also has a DEFAULT reverts it to + that default rather than unsetting it — so this only meaningfully *removes* + a setting when no default declares it. + """ + result = await session.execute( + select(AppSetting).where(AppSetting.key == key) + ) + row = result.scalar_one_or_none() + if row is None: + return False + await session.delete(row) + return True + + +async def get_stored_plugin_names(session: AsyncSession) -> list[str]: + """Plugin names that have a real stored row, ignoring DEFAULTS. + + Deliberately NOT derived from get_all_settings: that merges DEFAULTS in, and + a plugin.* key present only as a default is a code-level declaration rather + than operator data. Offering to "remove" such a key would be a lie — the + default would simply reassert it on the next load (exactly the bug behind + the phantom `plugin.http` entry). Only stored rows can actually be cleaned up. + + Returns names only, never values: plugin config can hold credentials, and + nothing that lists orphans needs to read them. + """ + prefix = "plugin." + result = await session.execute( + select(AppSetting.key).where(AppSetting.key.like(f"{prefix}%")) + ) + return sorted( + key[len(prefix):] for key in result.scalars() if key != prefix + ) + + +def find_orphaned_plugin_names( + stored_names: Iterable[str], + installed_names: Iterable[str], +) -> list[str]: + """Stored plugin settings whose plugin is not currently installed. + + Pure so it can be tested without a DB or a filesystem. "Not installed" is + intentionally not treated as "safe to delete" — an external plugin under + /data/plugins can be missing merely because the volume isn't mounted or an + install failed, so the caller surfaces these for an explicit operator + decision instead of removing them automatically. + """ + installed = set(installed_names) + return sorted(name for name in stored_names if name and name not in installed) + + async def get_all_settings(session: AsyncSession) -> dict[str, Any]: """Return flat key→value dict with defaults filled in for missing keys.""" result = await session.execute(select(AppSetting)) diff --git a/steward/migrations/versions/0025_drop_http_plugin_setting.py b/steward/migrations/versions/0025_drop_http_plugin_setting.py new file mode 100644 index 0000000..b6f93ad --- /dev/null +++ b/steward/migrations/versions/0025_drop_http_plugin_setting.py @@ -0,0 +1,40 @@ +"""Drop the stored setting for the removed http plugin + +The standalone `http` plugin was dissolved into the unified Monitor entity +(0022_unify_monitors), but its `plugin.http` settings key outlived it: the key +stayed in core DEFAULTS, so the operator saw a plugin that no longer exists +reported as "enabled" with no way to clear it — deleting the row alone did +nothing, because the default reasserted it on the next settings load. + +The DEFAULTS entry is removed in the same change; this migration clears any row +an operator's database still carries so the two agree. Per family rule 22, the +removed subsystem takes its setting row with it. + +Note: `http` remains a valid MONITOR TYPE (icmp/tcp/dns/http). This touches only +the plugin-enablement key, never monitor data. + +Revision ID: 0025_drop_http_plugin_setting +Revises: 0024_plugin_metrics_hourly +Create Date: 2026-08-13 +""" +from typing import Sequence, Union +from alembic import op +import sqlalchemy as sa + +revision: str = "0025_drop_http_plugin_setting" +down_revision: Union[str, None] = "0024_plugin_metrics_hourly" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + op.execute( + sa.text("DELETE FROM app_settings WHERE key = 'plugin.http'") + ) + + +def downgrade() -> None: + # Deliberately empty. Re-inserting `plugin.http` would recreate the exact + # phantom this migration exists to remove, and the plugin it configured no + # longer exists to read it. + pass diff --git a/steward/settings/routes.py b/steward/settings/routes.py index f869aaf..23c3e72 100644 --- a/steward/settings/routes.py +++ b/steward/settings/routes.py @@ -8,7 +8,8 @@ from steward.auth.middleware import require_role from steward.core.audit import log_audit from steward.models.users import UserRole from steward.core.settings import ( - get_all_settings, set_setting, + get_all_settings, set_setting, delete_setting, + get_stored_plugin_names, find_orphaned_plugin_names, to_smtp_cfg, to_webhook_cfg, to_ansible_cfg, to_plugins_cfg, to_oidc_cfg, to_ldap_cfg, to_thresholds_cfg, ) @@ -587,7 +588,7 @@ def _build_plugin_cfg_from_form(plugin: dict, form) -> dict: # of a host), not discrete vendor integrations. Presentation-only split — they # still load through the normal plugin mechanism. A plugin.yaml may set # kind: capability|integration to override. -CAPABILITY_PLUGINS = {"host_agent", "http", "snmp", "docker"} +CAPABILITY_PLUGINS = {"host_agent", "snmp", "docker"} @settings_bp.get("/plugins/") @@ -595,22 +596,56 @@ CAPABILITY_PLUGINS = {"host_agent", "http", "snmp", "docker"} async def plugins(): async with current_app.db_sessionmaker() as db: settings = await get_all_settings(db) + stored_plugin_names = await get_stored_plugin_names(db) discovered = _discover_plugins() _merge_plugin_config(discovered, to_plugins_cfg(settings)) for p in discovered: p["_kind"] = p.get("kind") or ( "capability" if p["_dir"] in CAPABILITY_PLUGINS else "integration") repos = _get_plugin_repos(settings) + # Config left behind by plugins that are no longer installed. Surfaced for an + # explicit operator decision rather than cleaned up automatically: an external + # plugin can be missing because /data/plugins isn't mounted or an install + # failed, and silently dropping its row would destroy stored credentials. + orphans = find_orphaned_plugin_names( + stored_plugin_names, [p["_dir"] for p in discovered]) return await render_template( "settings/plugins.html", capabilities=[p for p in discovered if p["_kind"] == "capability"], integrations=[p for p in discovered if p["_kind"] != "capability"], discovered_plugins=discovered, + orphaned_plugins=orphans, repos=repos, settings=settings, ) +@settings_bp.post("/plugins/orphans//remove/") +@require_role(UserRole.admin) +async def plugin_orphan_remove(name: str): + """Delete the stored settings row for a plugin that is no longer installed. + + Refuses if the plugin IS installed — that would silently wipe a live + plugin's config, and disabling it is what the operator wants there instead. + """ + installed = {p["_dir"] for p in _discover_plugins()} + if name in installed: + return redirect(url_for("settings.plugins")) + + async with current_app.db_sessionmaker() as db: + async with db.begin(): + removed = await delete_setting(db, f"plugin.{name}") + + if removed: + await _reload_app_config() + from steward.core.plugin_index import clear_catalog_cache + clear_catalog_cache() + await log_audit( + current_app, session.get("user_id"), session.get("username", ""), + "plugin.settings_removed", entity_type="plugin", entity_id=name) + return redirect(url_for("settings.plugins")) + + # ── Per-plugin detail (settings) ────────────────────────────────────────────── @settings_bp.get("/plugins//") diff --git a/steward/templates/settings/plugins.html b/steward/templates/settings/plugins.html index 1116d3d..15939ea 100644 --- a/steward/templates/settings/plugins.html +++ b/steward/templates/settings/plugins.html @@ -60,7 +60,7 @@

- Built-in ways to monitor your hosts (agent metrics, HTTP/uptime, SNMP, Docker). These are + Built-in ways to monitor your hosts (agent metrics, SNMP, Docker). These are facets of a host — you'll see their data in the Hosts and Status sections, not as separate areas. On by default.

@@ -88,6 +88,44 @@ {% endif %} +{# ── Orphaned settings ─────────────────────────────────────────────────────── #} +{# Hidden entirely when there is nothing to clean up — an empty "Configured but + not installed" panel would read as a problem rather than a clean state. #} +{% if orphaned_plugins %} +
+
Configured but not installed
+

+ Stored settings for plugins Steward can't find. This usually means the plugin was + removed — but it also happens when an external plugin directory isn't mounted or an + install failed part-way, so nothing is deleted automatically. Removing an entry + discards that plugin's saved configuration, including any credentials. +

+
+ {% for name in orphaned_plugins %} +
+ +
+
+ {{ name }} + Not installed +
+
+ Settings key plugin.{{ name }} has no matching plugin. +
+
+
+ +
+
+ {% endfor %} +
+
+{% endif %} + {# ── Plugin Repositories ───────────────────────────────────────────────────── #}
Plugin Repositories
diff --git a/tests/core/test_plugin_settings_hygiene.py b/tests/core/test_plugin_settings_hygiene.py new file mode 100644 index 0000000..b5b5c64 --- /dev/null +++ b/tests/core/test_plugin_settings_hygiene.py @@ -0,0 +1,88 @@ +"""Guards against plugin settings outliving the plugin they configure. + +The `http` plugin was folded into the unified Monitor entity, but its +`plugin.http` DEFAULTS entry survived — so the operator saw a plugin that no +longer exists reported as enabled, with no way to clear it (deleting the row +did nothing; the default reasserted it). These tests make that class of drift a +CI failure instead of a support question. +""" +from pathlib import Path + +from steward.core.settings import DEFAULTS, find_orphaned_plugin_names +from steward.settings.routes import CAPABILITY_PLUGINS + +BUNDLED_PLUGINS_DIR = Path(__file__).resolve().parents[2] / "plugins" + + +def _bundled_plugin_names() -> set[str]: + """Plugin dirs shipped inside the image, identified by their plugin.yaml.""" + return { + entry.name + for entry in BUNDLED_PLUGINS_DIR.iterdir() + if entry.is_dir() and (entry / "plugin.yaml").exists() + } + + +def _default_plugin_names() -> set[str]: + prefix = "plugin." + return {k[len(prefix):] for k in DEFAULTS if k.startswith(prefix)} + + +def test_bundled_plugins_dir_is_discoverable(): + # Guards the test itself: a wrong path would make everything below vacuous. + assert _bundled_plugin_names(), f"no bundled plugins found under {BUNDLED_PLUGINS_DIR}" + + +def test_every_default_plugin_key_has_a_real_bundled_plugin(): + """A plugin.* default naming a non-existent plugin is always a bug. + + Bundled plugins ship in the image and version atomically with core, so + unlike external plugins they cannot be transiently missing — there is no + benign reason for this to fail. + """ + missing = _default_plugin_names() - _bundled_plugin_names() + assert not missing, ( + f"DEFAULTS declares plugin(s) with no bundled directory: {sorted(missing)}. " + f"If the plugin was removed, delete its plugin. key from DEFAULTS " + f"and add a migration dropping the stored row." + ) + + +def test_http_plugin_default_is_gone(): + # Explicit regression: this exact key is what the operator hit. + assert "plugin.http" not in DEFAULTS + + +def test_capability_plugins_all_exist(): + """CAPABILITY_PLUGINS classifies discovered plugins; stale names are dead weight.""" + missing = CAPABILITY_PLUGINS - _bundled_plugin_names() + assert not missing, f"CAPABILITY_PLUGINS names non-existent plugin(s): {sorted(missing)}" + + +# ── orphan detection ───────────────────────────────────────────────────────── + + +def test_orphans_are_stored_names_with_no_installed_plugin(): + assert find_orphaned_plugin_names( + ["docker", "traefik", "ancient"], ["docker", "traefik"] + ) == ["ancient"] + + +def test_no_orphans_when_everything_is_installed(): + assert find_orphaned_plugin_names(["docker", "snmp"], ["docker", "snmp"]) == [] + + +def test_installed_plugin_without_stored_settings_is_not_an_orphan(): + # Never configured is not the same as left behind. + assert find_orphaned_plugin_names([], ["docker"]) == [] + + +def test_orphans_are_sorted_and_deduped_of_empties(): + assert find_orphaned_plugin_names(["zeta", "", "alpha"], []) == ["alpha", "zeta"] + + +def test_accepts_arbitrary_iterables(): + # Callers pass a generator of discovered dirs, not a list. + assert find_orphaned_plugin_names( + (n for n in ["gone"]), (n for n in ["here"]) + ) == ["gone"] diff --git a/tests/core/test_settings_default_plugins.py b/tests/core/test_settings_default_plugins.py index 32a45c9..122f253 100644 --- a/tests/core/test_settings_default_plugins.py +++ b/tests/core/test_settings_default_plugins.py @@ -9,7 +9,9 @@ and a DEFAULTS-merged-with-stored dict, mirroring get_all_settings' merge. from steward.core import settings as settings_module from steward.core.settings import DEFAULTS, to_plugins_cfg -DEFAULT_ON = {"docker", "host_agent", "http", "snmp"} +# NB: no "http" — that plugin was dissolved into the unified Monitor entity, and +# its lingering default is what test_plugin_settings_hygiene.py now guards against. +DEFAULT_ON = {"docker", "host_agent", "snmp"} VENDOR_OPT_IN = {"traefik", "unifi"} @@ -32,7 +34,7 @@ def test_stored_choice_overrides_default(): cfg = to_plugins_cfg(merged) assert cfg["docker"]["enabled"] is False # untouched defaults remain enabled - assert cfg["http"]["enabled"] is True + assert cfg["snmp"]["enabled"] is True def test_defaults_use_plugin_dot_namespace():