fix(plugins): remove the phantom http plugin; surface orphaned plugin settings
Operator saw the `http` plugin -- deleted when ping/dns/http were unified into the Monitor entity -- still reported as enabled, with no way to clear it. It was never an orphaned row. `plugin.http` was hardcoded in core DEFAULTS, so deleting the app_settings row did nothing: the default reasserted it on the next settings load. That is precisely why no cleanup UI could have fixed it. Rule 22 says the removed subsystem should have taken its setting with it. Purged the surviving references -- the DEFAULTS key, "http" in CAPABILITY_PLUGINS, the stale plugin_manager docstring example, and the capabilities blurb still advertising HTTP/uptime as a bundled capability -- plus a migration dropping any stored plugin.http row. Untouched: `http` as a MONITOR TYPE (icmp/tcp/dns/http) everywhere it appears, and http_001_initial, which is kept deliberately so existing DBs resolve the revision graph. For the general case, cleanup splits by provenance rather than being uniformly automatic or uniformly manual: Bundled plugins ship in the image and version atomically with core, so they cannot be transiently missing -- a plugin.* default with no bundled directory is unambiguously a bug. Guarded by a unit test that fails CI, which is the only part safe to automate. External plugins live in operator-mounted /data/plugins, where absence is ambiguous: unmounted volume, failed install, mid-upgrade. Auto-deleting their config would silently destroy unrecoverable credentials on a transient condition, so Settings now lists them under "Configured but not installed" with an explicit, confirmed, audit-logged Remove. The section hides entirely when empty, and the remove route refuses if the plugin is actually installed. Orphan detection reads STORED rows, never the DEFAULTS-merged view -- offering to remove a default-only key would be a lie -- and returns names only, since plugin config can hold credentials and listing orphans never needs their values. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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))
|
||||
|
||||
Reference in New Issue
Block a user